Re: [edk2-devel] [PATCH 08/10] OvmfPkg: Update Sec to support Tdvf Config-B

["Min Xu" <min.m.xu@intel.com>]

On January 3, 2022 4:02 PM, Gerd Hoffmann wrote:
> 
> > PCDs cannot be set in SEC phase, so the values should be saved in a
> > Hob (for example, PLATFORM_INFO_HOB). In early DXE phase these values
> > are set to the PCDs. This is how TdxDxe does today.
> >
> > Other tasks can be done in SEC phase. I think there should be a lib
> > (for example, PlatformPeiLib) to wrap these functions so that they can
> > be re-used by OvmfPkg/PlatformPei.
> 
> Yes, I think we need a PlatformLib for the platform initialization code.  With
> PEI we would simply link the lib into PlatformPei, without PEI we would link
> parts of the lib into SEC and parts of the lib into DXE.
After carefully study the PlatformPei code and a quick PoC (PlatformInitLib which wraps the basic functions in PlatformPei), I found it's not a easy task for such a lib which can be used in both PlatformPei and Pei-less boot.
1. PlatformInitLib should work both in SEC and PEI. So it cannot use global variables between different functions. mHostBridgeDevId and mPhysMemAddressWidth are the examples. So these variables must be provided by the caller thru the input function parameters.
2. PlatformInitLib cannot set PCDs in the code. So a Guid hob should be created to store the PCDs and pass them to DXE phase. Then these PCDs will be set at the very beginning of DXE phase.
3. The pointer to the HobList should be saved somewhere so that HobLib functions can be called in SEC phase. In my PoC it is saved in OVMF_WORK_AREA.
4. In PlatformPei there are many if-else to check if it is SMM/S3/Microvm/Cloud-Hypervisor/SEV/TDX. There are also Bhyve and Xen PlatformPei variants. In the current PlatformPei those if-else check depends on the PCDs and global variables. Because of (1) it needs input parameters for all these if-else check. Maybe a big environment variable data structure is needed.
But anyway a complete functional PlatformInitLib is a big task. My suggestion is that in TDVF-Config-B we first propose a basic functional PlatformInitLib. This lib can boot up Tdx guest and legacy OVMF guest in TDVF-Config-B. OvmfPkg/PlatformPei is not refactored by this basic PlatformInitLib this time. This is because PlatformPei serves SMM/S3/Microvm/Cloud-Hypervisor/SEV/TDX. It is a big risk for such refactor. We can revisit PlatformPei in the future.
> 
> > PEI-less booting up legacy guest doesn't support TPM.
> >
> > So to boot up legacy guest without PEI phase, there will be below changes.
> > 1. OvmfStartupLib:  (like TdxStartupLib)
> >     - Decompress DxeFv, locate DxeCore, create IdentityMappingPageTables,
> then jump to DxeCore.
> 
> Yes.  Basically rename TdxStartupLib to OvmfStartupLib and add some
> IfTdx() checks.
Yes, agree.
> 
> > 2. PlatformPeiLib:
> >     - Wrap the functions to do memory initialization, etc. (see tasks
> > 1-5)
> 
> Yes.  Move code from PlatformPei to PlatformLib.  Might also need some
> reorganization due to SEC restrictions.
As I explained above, a basic PlatformInitLib is the first stage and some reorganization is needed.
> 
> > 3. OvmfLegacyDxe
> >     - Set the PCDs (see task 6)
> 
> Well, in Tdx mode you have to set some PCDs too ...
TdxDxe.inf can set the PCDs.
> 
> Also not sure we actually need a new Dxe.  Can't we just handle that in
> PlatformDxe in case of a PEI-less boot?
Do you mean "OvmfPkg/PlatformDxe/Platform.inf"? I am afraid PlatformDxe cannot do this task. 
It is not in APRIORI DXE list so it cannot be guaranteed to be loaded at the very beginning of DXE phase. While some PCDs are required in the very early stage of DXE phase.
> 
> > I know there are many discussions in above options. Can we follow below
> road map so that we can discuss 3 (How to achieve ONE Binary) in more
> details?
> > 1. Basic Config-B (PEI-less and only Tdx guest) 2. Advanced Config-B
> > (RTMR based measurement) 3. One Binary Config-B (support legacy guest)
> 
> IMHO step #1 must be reorganizing the platform initialization code for PEI-
> less boot (create PlatformLib as discussed above).
> 
> This patch series side-steps that by simply duplicating the code.  PCI
> initialization for example.  Also setting the tdx PCDs.  Having two (or even
> more) copies of the same code in the tree is a bad idea though.
> It makes long-term maintenance harder for various reasons.
As I explained above, a basic PlatformInitLib is the first stage. There will be an advanced PlatformInitLib in the future which implements more complicated functions.
> 
> > > ... and given that TDX-capable
> > > hardware is not yet production ready I find it rather important that
> > > testing the PEI-less boot workflow does not require TDX.
> > >
> > > It'll also make it much easier to add CI coverage.
> > I am thinking if SEV features are covered in CI?
> > Because I want to make sure our changes don't impact SEV.
> 
> AmdSevX64.dsc has build-test coverage.  There is no qemu boot test
> because FlashRomImage() (in OvmfPkg/PlatformCI/PlatformBuildLib.py)
> is not flexible enough for that.  Fixing that and adding a boot test (in non-sev
> mode) shouldn't be that difficult though.
> 
> Same for IntelTdx.dsc: adding a CI boot test (in non-tdx mode) should be
> easy, and it should help preventing regressions in PEI-less boot flow.
Agree. We will add a CI boot test (in non-tdx mode).

Thanks
Min