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

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

On January 10, 2022 3:56 PM, Gerd Hoffmann wrote:
> On Fri, Jan 07, 2022 at 06:13:37AM +0000, Xu, Min M wrote:
> > 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.
> 
> Yes.  Your patches add a PlatformInitHob because of that.  I think right now it
> only has some tdx-specific variables, but we can move more variables into the
> hob to allow platform init code run in both SEC and PEI phase.  I think it makes
> sense to have the hob in both PEI and PEI-less mode to minimize the code
> differences.
Yes, we can use EFI_HOB_PLATFORM_INFO.
> 
> > 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.
> 
> Use PlatformInitHob?
Yes, we can use this data structure.
> 
> > 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.
> 
> Well.  The whole point of adding PlatformInitLib is to move over (and refactor if
> needed) existing code in PlatformPei so we can avoid code duplication.  Now
> you want add PlatformInitLib without touching PlatformPei, probably by
> copying code.  That doesn't make sense at all.
> 
> > 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.
> 
> Well, if you want avoid the refactoring because of the risk there is still the
> option to have tdx config-b use the normal PEI boot flow.
> Then revisit refactoring and adding support for PEI-less boot later.
> 
I think it still makes sense (Adding a basic PlatformInitLib which brings up tdx guest and legacy guest in Pei-less boot, but not touch PlatformPei).
1. The goal of TDVF-Config-B is to bring up tdx guest and legacy guest without PEI. So that attack surface can be reduced.
2. There are common functions when bring up tdx guest and legacy guest in Config-B. So PlatformInitLib is necessary.
3. As I explained there are many if-else checks in PlatformPei and the logics are rather complicated (because PlatformPei serves S3/SMM/SEV/TDX/Legacy/Microvm/CloudHypervisor, etc). To be honest I have not so much confidence to abstract PlatformPei's common function to PlatformInitLib.
4. But a basic version of PlatformInitLib is a good start. During the development and community review, we can understand better what functions should be wrapped into PlatformInitLib. After that PlatformInitLib can be evolved for OvmfPkg/PlatformPei, Bhyve/PlatformPei, XenPlatformPei.

Thanks
Min