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

["Gerd Hoffmann" <kraxel@redhat.com>]

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.

> 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?

> 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.

take care,
  Gerd