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