[coreboot-gerrit] Change in coreboot[master]: [RFC|WIP] soc/intel/skylake: replace native SGX initialization by FSP

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44244 )

Change subject: [RFC|WIP] soc/intel/skylake: replace native SGX initialization by FSP
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2:
> > > > > Patch Set 2:
> > > > > > Patch Set 2:
> > > > > >
> > > > > > > Patch Set 2:
> > > > > > > 
> > > > > > > if i remember correctly, this programming sequence requirement was from cannon-lake onwards. skylake/kabylake can still use native (coreboot) sgx init flow.
> > > > > >
> > > > > > On SKL LT_MEMORY_LOCK has to be set, too, and FSP does this with SkipMpInit=0. AFAICT native SGX init can't be used on SKL for the same reasons we can't use it on CNL.
> > > > > >
> > > > > > > 
> > > > > > > From cannon-lake onwards, sgx init needs fsp for the reasons mentioned in https://review.coreboot.org/c/coreboot/+/36356/2/src/soc/intel/cannonlake/fi.... i guess mp-ppi may also be used for cannon-lake onwards. Please follow Nate's recommendation for that.
> > > > > > > 
> > > > > > > PS: I see my gmail was added earlier and that notification was in spam. Thanks for adding my @intel email. I am off next week, I’ll respond the following week if there are follow-up questions.
> > > > > >
> > > > > > See CB:36356. Native SGX init is not used in CNL, so there is nothing more to do.
> > > > >
> > > > > "FSP-S must be able to write and lock some registers that would be immutable after setting LT_LOCK_MEMORY."* I dont think i got into this issue when i wrote native sgx init flow. At that time i was able to activate the SGX and lock it correctly by using native code only. i created enclaves also from the application and checked it was encrypted and working as expected.
> > > > >
> > > > > @Nate: is this* valid for SKL/KBL as well?
> > >
> > > Yes, this is valid and exactly the point, why native SGX init can't be used. LT_MEMORY_LOCK must be set, *before* native SGX init (well, more specifically before microcode reload). That conflicts with the requirement of FSP-S to be able to write registers.
> > > > At that time i was able to activate the SGX and lock it correctly by using native code only. i created enclaves also from the application and checked it was encrypted and working as expected
> > >
> > > Yes, SGX works. No, not in a secure way with native SGX init without LT_MEMORY_LOCK.
> >
> > in my original code i.e. native sgx init, i was doing exactly that.
> > lock LT mem, reload ucode and so on. (my original code below - commit 418535e222ccd9f688facbb7d9663ca4cacc2739) which was working.

No, it was not working, because FSP-S can't lock TSEGMB and PAVPC. See the relation chain of this CB, please.
> > void sgx_configure(void)
> > {
> >         const void *microcode_patch = intel_mp_current_microcode();
> >     if (!soc_sgx_enabled() || !is_sgx_supported() || !is_prmrr_set()) {
> >             printk(BIOS_ERR, "SGX: pre-conditions not met\n");
> >             return;
> >     }
> >
> >     /* Enable the SGX feature */
> >     enable_sgx();
> >
> >     /* Update the owner epoch value */
> >     if (owner_epoch_update() < 0)
> >             return;
> >
> >     /* Ensure to lock memory before reload microcode patch */
> >     cpu_lock_sgx_memory();
> >
> >     /* Reload the microcode patch */
> >     intel_microcode_load_unlocked(microcode_patch);
> >
> >     /* Lock the SGX feature */
> >     lock_sgx();
> >
> >     /* Activate the SGX feature, if PRMRR config was aprroved by MCHECK */
> >     if (is_prmrr_approved())
> >             activate_sgx();
> >
> > }
>
> I'll let Nate comment on "FSP-S must be able to write and lock some registers that would be immutable after setting LT_LOCK_MEMORY."*

That was already discussed and ack'ed by him.