RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

Tdvf Config-B skip PEI phase to reduce attack surface. So instead of
jumping to SecStartupPhase2 (), TdxStartup () is called. This function
brings up Tdx guest from SEC phase to DXE phase.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/Sec/IntelTdx.c  |  7 ++++++-
 OvmfPkg/Sec/SecMain.c   | 17 +++++++++++++++++
 OvmfPkg/Sec/SecMain.inf |  2 ++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Sec/IntelTdx.c b/OvmfPkg/Sec/IntelTdx.c
index d1d952e8d433..f9d44617b211 100644
--- a/OvmfPkg/Sec/IntelTdx.c
+++ b/OvmfPkg/Sec/IntelTdx.c
@@ -12,7 +12,7 @@
 #include <Uefi/UefiBaseType.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
-#include <Library/HobLib.h>
+#include <Library/PrePiLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <IndustryStandard/UefiTcgPlatform.h>
 #include <Library/MemoryAllocationLib.h>
@@ -25,6 +25,11 @@
 
 #define ALIGNED_2MB_MASK  0x1fffff
 
+#define GET_HOB_TYPE(Hob)     ((Hob).Header->HobType)
+#define GET_HOB_LENGTH(Hob)   ((Hob).Header->HobLength)
+#define GET_NEXT_HOB(Hob)     ((Hob).Raw + GET_HOB_LENGTH (Hob))
+#define END_OF_HOB_LIST(Hob)  (GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_END_OF_HOB_LIST)
+
 /**
   Check TDX is enabled.
 
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index e2f3ede93901..c5dd066941fe 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -33,6 +33,10 @@
 #include "IntelTdx.h"
 #include "AmdSev.h"
 
+#ifdef INTEL_TDX_FULL_FEATURE
+  #include <Library/TdxStartupLib.h>
+#endif
+
 #define SEC_IDT_ENTRY_COUNT  34
 
 typedef struct _SEC_IDT_TABLE {
@@ -913,6 +917,19 @@ SecCoreStartupWithStack (
   InitializeApicTimer (0, MAX_UINT32, TRUE, 5);
   DisableApicTimerInterrupt ();
 
+ #ifdef INTEL_TDX_FULL_FEATURE
+  if (SecTdxIsEnabled ()) {
+    TdxStartup (&SecCoreData);
+
+    //
+    // Never arrived here
+    //
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+
+ #endif
+
   //
   // Initialize Debug Agent to support source level debug in SEC/PEI phases before memory ready.
   //
diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
index 230ee5e465b9..05e49ab5ae81 100644
--- a/OvmfPkg/Sec/SecMain.inf
+++ b/OvmfPkg/Sec/SecMain.inf
@@ -38,6 +38,7 @@
   MdeModulePkg/MdeModulePkg.dec
   UefiCpuPkg/UefiCpuPkg.dec
   OvmfPkg/OvmfPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
 
 [LibraryClasses]
   BaseLib
@@ -58,6 +59,7 @@
 
 [LibraryClasses.X64]
   TdxLib
+  TdxStartupLib
 
 [Ppis]
   gEfiTemporaryRamSupportPpiGuid                # PPI ALWAYS_PRODUCED
-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84812): https://edk2.groups.io/g/devel/message/84812
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Dec 14, 2021 at 09:41:24PM +0800, Min Xu wrote:
> RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
> 
> Tdvf Config-B skip PEI phase to reduce attack surface. So instead of
> jumping to SecStartupPhase2 (), TdxStartup () is called. This function
> brings up Tdx guest from SEC phase to DXE phase.

> + #ifdef INTEL_TDX_FULL_FEATURE
> +  if (SecTdxIsEnabled ()) {
> +    TdxStartup (&SecCoreData);
> +
> +    //
> +    // Never arrived here
> +    //
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +
> + #endif

Oh, wow.  So you compile in PEI, then decide at runtime whenever you
use it or not?

No.  Please don't.  That's just silly.  If you don't want use PEI, ok,
fine, but please go the way then, remove PEI from the build and take
the PEI-less code path in all cases.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84892): https://edk2.groups.io/g/devel/message/84892
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On December 15, 2021 6:28 PM, Gerd Hoffmann wrote:
> On Tue, Dec 14, 2021 at 09:41:24PM +0800, Min Xu wrote:
> > RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
> >
> > Tdvf Config-B skip PEI phase to reduce attack surface. So instead of
> > jumping to SecStartupPhase2 (), TdxStartup () is called. This function
> > brings up Tdx guest from SEC phase to DXE phase.
> 
> > + #ifdef INTEL_TDX_FULL_FEATURE
> > +  if (SecTdxIsEnabled ()) {
> > +    TdxStartup (&SecCoreData);
> > +
> > +    //
> > +    // Never arrived here
> > +    //
> > +    ASSERT (FALSE);
> > +    CpuDeadLoop ();
> > +  }
> > +
> > + #endif
> 
> Oh, wow.  So you compile in PEI, then decide at runtime whenever you use it
> or not?
Yes.
In OvmfPkgX64.dsc above code will not be built into the image. So it follows the SEC->PEI->DXE flow.
In IntelTdxX64.dsc, it if is Tdx guest, it jumps from SEC to DXE (see TdxStartup ()). Otherwise, it follows the SEC->PEI->DXE flow (Legacy guest, SEV guest, etc).
> 
> No.  Please don't.  That's just silly.  If you don't want use PEI, ok, fine, but
> please go the way then, remove PEI from the build and take the PEI-less code
> path in all cases.
In the first version TDVF, we do remove the PEI from the image. The image only contains the SEC and DXE, and only the components TDVF needs. It's a slim image.
Then the *ONE BINARY* requirement is proposed. It requires to bring up Legacy guest and Tdx guest with the same image. So PEI must be included in the build, and it probes Tdx guest in run-time so that it decides to go to the legacy flow (SEC->PEI->DXE) or Tdx flow (SEC->DXE).
Below are some of the links about the discussion.
https://edk2.groups.io/g/devel/message/76023  Laszlo
https://edk2.groups.io/g/devel/message/76024  Jiewen
https://edk2.groups.io/g/devel/message/76065  Laszlo
https://edk2.groups.io/g/devel/message/76339  Erdem Aktas
https://edk2.groups.io/g/devel/message/76367  Config-A & Config-B

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84987): https://edk2.groups.io/g/devel/message/84987
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> > Oh, wow.  So you compile in PEI, then decide at runtime whenever you use it
> > or not?
> Yes.
> In OvmfPkgX64.dsc above code will not be built into the image. So it follows the SEC->PEI->DXE flow.
> In IntelTdxX64.dsc, it if is Tdx guest, it jumps from SEC to DXE (see TdxStartup ()). Otherwise, it follows the SEC->PEI->DXE flow (Legacy guest, SEV guest, etc).

> > No.  Please don't.  That's just silly.  If you don't want use PEI, ok, fine, but
> > please go the way then, remove PEI from the build and take the PEI-less code
> > path in all cases.

> In the first version TDVF, we do remove the PEI from the image. The
> image only contains the SEC and DXE, and only the components TDVF
> needs. It's a slim image.  Then the *ONE BINARY* requirement is
> proposed. It requires to bring up Legacy guest and Tdx guest with the
> same image. So PEI must be included in the build,

Why?  Booting non-tdx guests without PEI shouldn't be fundamentally
different from a TDX guest.  Memory detection needs fw_cfg instead of
the td_hob, and you have to skip some tdx setup steps, but that should
be it.  Code for all that exists in PlatformPei, it only needs to be
moved to a place where SEC can use it too.

Yes, you can't include a number of features which depend on PEI into the
build then.  But config-b wants be a stripped down build anyway, right?

One major advantage of having a single binary is that most aspects of
the SEC->DXE boot workflow can also be tested without TDX.  Easier for
developers.  Easier for CI coverage.  Especially now where we talk about
pre-production hardware support.

When builing a frankenstein image which uses SEC->DXE with TDX and
SEC->PEI->DXE without TDX you loose that advantage, because that is
effectively a two-in-one binary.

> and it probes Tdx
> guest in run-time so that it decides to go to the legacy flow
> (SEC->PEI->DXE) or Tdx flow (SEC->DXE).

Ok, so the state with wave-2 merged will be:

  * We have the ovmf build, which supports native/sev/tdx guests,
    with basic tdx support (aka config-a).

  * We have the amdsev variant (supports native/sev/not-sure-about-tdx),
    which is largely identical to the normal build, only unwanted
    drivers removed (no network etc), grub boot loader added and its own
    PlatformBootManagerLib to have a more strict boot policy (all dxe
    phase changes).

So, where to go from here?


I still think the best way forward would be to model the inteltdx build
(aka config-b) similar to the amdsev variant.  Just disable the stuff
you don't need, add support for the advanced tdx features (measurement
etc), but otherwise continue to use the same SEC->PEI->DXE boot
workflow.

Advantages:
  * It should be relatively easy to unify amdsev + inteltdx into one
    binary.
  * No quirks needed due to SEC/PEI differences.  SEC can't set PCDs,
    leading to patches like #9 of this series (and there was another
    similar one ...).


The other route (as preferred by Jiewen) would be to not use PEI in
inteltdx.  Requires some reorganization of the qemu platform
initialization code (probably move to lib) so we can run the same code
(without using cut+paste programming) in both sec and pei phase.
Clearly not my preference, but should work too.

A better solution for the PCD issue (and possibly other simliar issues
poping up later) would be good.  Can't we handle that early in
PlatformDxe?  So we have one single place for those quirks, and the dxe
drivers don't need to know about the SEC->DXE and SEC->PEI->DXE
differences?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84994): https://edk2.groups.io/g/devel/message/84994
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On December 16, 2021 10:25 PM, Gerd Hoffmann wrote:
> > > Oh, wow.  So you compile in PEI, then decide at runtime whenever you
> > > use it or not?
> > Yes.
> > In OvmfPkgX64.dsc above code will not be built into the image. So it follows
> the SEC->PEI->DXE flow.
> > In IntelTdxX64.dsc, it if is Tdx guest, it jumps from SEC to DXE (see TdxStartup
> ()). Otherwise, it follows the SEC->PEI->DXE flow (Legacy guest, SEV guest, etc).
> 
> > > No.  Please don't.  That's just silly.  If you don't want use PEI,
> > > ok, fine, but please go the way then, remove PEI from the build and
> > > take the PEI-less code path in all cases.
> 
> > In the first version TDVF, we do remove the PEI from the image. The
> > image only contains the SEC and DXE, and only the components TDVF
> > needs. It's a slim image.  Then the *ONE BINARY* requirement is
> > proposed. It requires to bring up Legacy guest and Tdx guest with the
> > same image. So PEI must be included in the build,
> 
> Why?  Booting non-tdx guests without PEI shouldn't be fundamentally
> different from a TDX guest.  Memory detection needs fw_cfg instead of the
> td_hob, and you have to skip some tdx setup steps, but that should be it.
> Code for all that exists in PlatformPei, it only needs to be moved to a place
> where SEC can use it too.
> 
> Yes, you can't include a number of features which depend on PEI into the build
> then.  But config-b wants be a stripped down build anyway, right?
> 
> One major advantage of having a single binary is that most aspects of the SEC-
> >DXE boot workflow can also be tested without TDX.  Easier for developers.
> Easier for CI coverage.  Especially now where we talk about pre-production
> hardware support.
> 
> When builing a frankenstein image which uses SEC->DXE with TDX and
> SEC->PEI->DXE without TDX you loose that advantage, because that is
> effectively a two-in-one binary.
> 
> > and it probes Tdx
> > guest in run-time so that it decides to go to the legacy flow
> > (SEC->PEI->DXE) or Tdx flow (SEC->DXE).
> 
> Ok, so the state with wave-2 merged will be:
> 
>   * We have the ovmf build, which supports native/sev/tdx guests,
>     with basic tdx support (aka config-a).
> 
>   * We have the amdsev variant (supports native/sev/not-sure-about-tdx),
>     which is largely identical to the normal build, only unwanted
>     drivers removed (no network etc), grub boot loader added and its own
>     PlatformBootManagerLib to have a more strict boot policy (all dxe
>     phase changes).
> 
> So, where to go from here?
> 
> 
> I still think the best way forward would be to model the inteltdx build (aka
> config-b) similar to the amdsev variant.  Just disable the stuff you don't need,
> add support for the advanced tdx features (measurement etc), but otherwise
> continue to use the same SEC->PEI->DXE boot workflow.
> 
> Advantages:
>   * It should be relatively easy to unify amdsev + inteltdx into one
>     binary.
>   * No quirks needed due to SEC/PEI differences.  SEC can't set PCDs,
>     leading to patches like #9 of this series (and there was another
>     similar one ...).
> 
> 
> The other route (as preferred by Jiewen) would be to not use PEI in inteltdx.
> Requires some reorganization of the qemu platform initialization code
> (probably move to lib) so we can run the same code (without using cut+paste
> programming) in both sec and pei phase.
> Clearly not my preference, but should work too.
> 
> A better solution for the PCD issue (and possibly other simliar issues poping up
> later) would be good.  Can't we handle that early in PlatformDxe?  So we have
> one single place for those quirks, and the dxe drivers don't need to know
> about the SEC->DXE and SEC->PEI->DXE differences?
> 
Thank Gerd for the review comments.
Yes, TDVF Config-B is a strip down and it is to be a more secure solution (because RTMR based measurement/measure boot is enabled, un-used drivers are excluded to reduce attack surface, sanity check/measure all external inputs, etc).

We would like to split TDVF Config-B into below stages.
1. Basic Config-B (wave-3)
1.1 A standalone IntelTdxX64.dsc/.fdf. Un-used drivers/libs are removed from the fdf, such as network components, SMM drivers, TPM drivers, etc.
1.2 PEI FV is excluded from the build. Only DxeFV is included.
1.3 Since PEI FV is excluded from the build, so Basic Config-B can only bring up Tdx guest. It *CAN NOT* bring up legacy guest.

2. Advanced Config-B (wave-4)
2.1 RTMR based measurement and measure boot are enabled
2.2 External input is checked and measured

3. Full feature Config-B (wave-5)
3.1 Add *basic* Ovmf feature without PEI, to achieve *ONE Binary* goal. (here basic means S3 is not supported without PEI)

@Gerd, What's your thought?

Thanks
Min



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85094): https://edk2.groups.io/g/devel/message/85094
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> > Why?  Booting non-tdx guests without PEI shouldn't be fundamentally
> > different from a TDX guest.  Memory detection needs fw_cfg instead of the
> > td_hob, and you have to skip some tdx setup steps, but that should be it.
> > Code for all that exists in PlatformPei, it only needs to be moved to a place
> > where SEC can use it too.

> We would like to split TDVF Config-B into below stages.
> 1. Basic Config-B (wave-3)
> 1.1 A standalone IntelTdxX64.dsc/.fdf. Un-used drivers/libs are removed from the fdf, such as network components, SMM drivers, TPM drivers, etc.
> 1.2 PEI FV is excluded from the build. Only DxeFV is included.
> 1.3 Since PEI FV is excluded from the build, so Basic Config-B can only bring up Tdx guest. It *CAN NOT* bring up legacy guest.

What blocks legacy guest bringup?

See above, I think it should not be hard to do, 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.

> 3.1 Add *basic* Ovmf feature without PEI, to achieve *ONE Binary* goal. (here basic means S3 is not supported without PEI)

Sure, pei-less ovmf has to drop some features, that is perfectly fine.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85106): https://edk2.groups.io/g/devel/message/85106
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi
> 
> > > Why?  Booting non-tdx guests without PEI shouldn't be fundamentally
> > > different from a TDX guest.  Memory detection needs fw_cfg instead
> > > of the td_hob, and you have to skip some tdx setup steps, but that
> should be it.
> > > Code for all that exists in PlatformPei, it only needs to be moved
> > > to a place where SEC can use it too.
> 
> > We would like to split TDVF Config-B into below stages.
> > 1. Basic Config-B (wave-3)
> > 1.1 A standalone IntelTdxX64.dsc/.fdf. Un-used drivers/libs are removed
> from the fdf, such as network components, SMM drivers, TPM drivers, etc.
> > 1.2 PEI FV is excluded from the build. Only DxeFV is included.
> > 1.3 Since PEI FV is excluded from the build, so Basic Config-B can only bring
> up Tdx guest. It *CAN NOT* bring up legacy guest.
> 
> What blocks legacy guest bringup?
> 
> See above, I think it should not be hard to do, 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.
Current PlatformPei does below tasks (no SMM, no S3):
1. Fetch the memory information from either e820 or CMOS, then create the ResourceDescriptorHob.
2. Create MemoryAllocationHob for PeiFV/DxeFV, create FvHob for DxeFV.
3. Read the CPU count from QEMU and set the PCDs.
4. Create the ResourceDescriptorHob for MMIO and set the PCDs
5. Other Hobs, such as MemTypeInfoHob, CpuHob
6. Set PCDs, such as PcdSetNxForStack, PcdOvmfHostBridgePciDevId, PcdPciIoBase, etc
7. Calculate the memory for PEI and PublishPeiMemory
8. InstallClearCacheCallback/InstallFeatureControlCallback

Task 7 is not needed in PEI-less boot up.
Task 8 is not needed either because it is for MP Services.

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. 

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.
2. PlatformPeiLib:
    - Wrap the functions to do memory initialization, etc. (see tasks 1-5)
3. OvmfLegacyDxe
    - Set the PCDs (see task 6)

Base upon above consideration, It's a big change. That's why we suggest implement Config-B in 3 stages.

I am also thinking about another option which includes PEI in build. (That's Config-B v1)
In this option, Ovmf image layout is kept unchanged. In run-time Tdx guest is probed. If it is Tdx guest, it goes to TdxStartup and brings up Tdx guest. Otherwise it follows normal Ovmf boot flow.
The advantages are:
1. The change is small.
2. It doesn't impact the current legacy guest, nor the SEV guest.

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)

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

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85210): https://edk2.groups.io/g/devel/message/85210
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

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

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

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

> 3. OvmfLegacyDxe
>     - Set the PCDs (see task 6)

Well, in Tdx mode you have to set some PCDs too ...

Also not sure we actually need a new Dxe.  Can't we just handle
that in PlatformDxe in case of a PEI-less boot?

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

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

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85262): https://edk2.groups.io/g/devel/message/85262
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85319): https://edk2.groups.io/g/devel/message/85319
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85445): https://edk2.groups.io/g/devel/message/85445
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85510): https://edk2.groups.io/g/devel/message/85510
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

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

Hmm?  Isn't the main goal of config-b to support the advanced tdx
features (attestation etc)?

I don't see that PEI-less boot is required for that.  Sure, when
stripping down the build and removing all the features which require
PEIMs there isn't much left to do for the PEI phase.  So it makes sense
to look into dropping PEI altogether.  But it's more a "nice to have"
than a hard requirement, no?

> 2. There are common functions when bring up tdx guest and legacy guest
> in Config-B. So PlatformInitLib is necessary.

Sure.

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

What is the problem with moving code?  After some preparing steps (add
platform info hob, move global variables to the hob) it should be
possible to move the code needed by config-b (memory detection via
fw_cfg or tdx hob, pci init, ...) from PlatformPei to PlatformInitLib
and (also) use it in the SEC phase.  Likewise for code which runs in DXE
in PEI-less mode (setting PCDs).

The code not needed by config-b (smm, s3, ...) can stay in PlatformPei.

> 4. But a basic version of PlatformInitLib is a good start.

Yes.  Having initially only the functions needed by config-b in
PlatformInitLib is perfectly fine, but this should be a code *move*
not a copy.

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

Yes, most likely there are a number of opportunities to reduce code
duplication in the three PlatformPei variants we have by moving code
to the (shared) PlatformInitLib.

That can be looked at later.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85522): https://edk2.groups.io/g/devel/message/85522
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On January 11, 2022 5:23 PM, Gerd Hoffmann wrote:
> 
> > > 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.
> 
> Hmm?  Isn't the main goal of config-b to support the advanced tdx features
> (attestation etc)?
PEI-less boot is one of the main goal of Config-B. Actually PEI-less boot is in the original design of TDVF. RTMR-based measurement and measure boot are another important goals.
> 
> I don't see that PEI-less boot is required for that.  Sure, when stripping down
> the build and removing all the features which require PEIMs there isn't much
> left to do for the PEI phase.  So it makes sense to look into dropping PEI
> altogether.  But it's more a "nice to have"
> than a hard requirement, no?
No. I have to say PEI-less boot in Config-B is a hard requirement.
> 
> > 2. There are common functions when bring up tdx guest and legacy guest
> > in Config-B. So PlatformInitLib is necessary.
> 
> Sure.
> 
> > 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.
> 
> What is the problem with moving code?  After some preparing steps (add
> platform info hob, move global variables to the hob) it should be possible to
> move the code needed by config-b (memory detection via fw_cfg or tdx hob,
> pci init, ...) from PlatformPei to PlatformInitLib and (also) use it in the SEC
> phase.  Likewise for code which runs in DXE in PEI-less mode (setting PCDs).
> 
> The code not needed by config-b (smm, s3, ...) can stay in PlatformPei.
Yes, PlatformPei can be refactored in this way.
> 
> > 4. But a basic version of PlatformInitLib is a good start.
> 
> Yes.  Having initially only the functions needed by config-b in PlatformInitLib
> is perfectly fine, but this should be a code *move* not a copy.
> 
> > 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.
> 
> Yes, most likely there are a number of opportunities to reduce code
> duplication in the three PlatformPei variants we have by moving code to the
> (shared) PlatformInitLib.
> 
> That can be looked at later.

So let me summarize the discussion about PlatformInitLib.
1. PlatformInitLib wraps the common functions in OvmfPkg/PlatformPei. These common functions covers the memory detection via fw_cfg, pci init, cmos, (MemDetect.c/Platform.c/Cmos.c). And PlatformInitLib will not handle the S3/SMM variants.
2. OvmfPkg/PlatformPei will be refactored with PlatformInitLib. The functions not needed by config-b stay in PlatformPei.
3. Config-B support PEI-less boot for both legacy guest and td guest.

If you agree, then I will update the patch-sets based on above discussions.

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85691): https://edk2.groups.io/g/devel/message/85691
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> > I don't see that PEI-less boot is required for that.  Sure, when stripping down
> > the build and removing all the features which require PEIMs there isn't much
> > left to do for the PEI phase.  So it makes sense to look into dropping PEI
> > altogether.  But it's more a "nice to have"
> > than a hard requirement, no?

> No. I have to say PEI-less boot in Config-B is a hard requirement.

I'm still wondering why though.  I have not yet seen a reason why
config-b can't use the PEI-based boot flow.

> > > 4. But a basic version of PlatformInitLib is a good start.
> > 
> > Yes.  Having initially only the functions needed by config-b in PlatformInitLib
> > is perfectly fine, but this should be a code *move* not a copy.
> > 
> > > 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.
> > 
> > Yes, most likely there are a number of opportunities to reduce code
> > duplication in the three PlatformPei variants we have by moving code to the
> > (shared) PlatformInitLib.
> > 
> > That can be looked at later.
> 
> So let me summarize the discussion about PlatformInitLib.

> 1. PlatformInitLib wraps the common functions in OvmfPkg/PlatformPei.
> These common functions covers the memory detection via fw_cfg, pci
> init, cmos, (MemDetect.c/Platform.c/Cmos.c).

Yes.  Everything needed for PEI-less / config-b boot moves to
PlatformInitLib.

PlatformInitLib is added as dependency to OvmfPkg/PlatformPei, so
PlatformPei can call those functions when booting with PEI.

PEI-less boot will add PlatformInitLib to SEC (and DXE) instead
so the same code can be used then.

Not sure how to handle cmos best.  Not needed for memory detection on
qemu, but cloudhw depends on it so it is back for now.  Will cloudhw
support tdx too btw?

> And PlatformInitLib will
> not handle the S3/SMM variants.

At least not initially.  Maybe later when we move more code to the lib
to reduce code duplication in xen/bhyve/qemu PlatformPei variants.

> 2. OvmfPkg/PlatformPei will be refactored with PlatformInitLib. The
> functions not needed by config-b stay in PlatformPei.

> 3. Config-B support PEI-less boot for both legacy guest and td guest.

Yes.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85710): https://edk2.groups.io/g/devel/message/85710
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On January 14, 2022 4:32 PM, Gerd Hoffmann wrote:
> > > I don't see that PEI-less boot is required for that.  Sure, when
> > > stripping down the build and removing all the features which require
> > > PEIMs there isn't much left to do for the PEI phase.  So it makes
> > > sense to look into dropping PEI altogether.  But it's more a "nice to have"
> > > than a hard requirement, no?
> 
> > No. I have to say PEI-less boot in Config-B is a hard requirement.
> 
> I'm still wondering why though.  I have not yet seen a reason why config-b
> can't use the PEI-based boot flow.
Hi, Gerd, I think Jiewen has discussed this (PEI-less boot in Config-B) in another mail thread. We can continue the discussion there. Let's first focus on the PlatformInitLib here. Thanks for your understanding.
> 
> > > > 4. But a basic version of PlatformInitLib is a good start.
> > >
> > > Yes.  Having initially only the functions needed by config-b in
> > > PlatformInitLib is perfectly fine, but this should be a code *move* not a
> copy.
> > >
> > > > 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.
> > >
> > > Yes, most likely there are a number of opportunities to reduce code
> > > duplication in the three PlatformPei variants we have by moving code
> > > to the
> > > (shared) PlatformInitLib.
> > >
> > > That can be looked at later.
> >
> > So let me summarize the discussion about PlatformInitLib.
> 
> > 1. PlatformInitLib wraps the common functions in OvmfPkg/PlatformPei.
> > These common functions covers the memory detection via fw_cfg, pci
> > init, cmos, (MemDetect.c/Platform.c/Cmos.c).
> 
> Yes.  Everything needed for PEI-less / config-b boot moves to PlatformInitLib.
> 
> PlatformInitLib is added as dependency to OvmfPkg/PlatformPei, so
> PlatformPei can call those functions when booting with PEI.
> 
> PEI-less boot will add PlatformInitLib to SEC (and DXE) instead so the same
> code can be used then.
> 
> Not sure how to handle cmos best.  Not needed for memory detection on
> qemu, but cloudhw depends on it so it is back for now.  Will cloudhw support
> tdx too btw?
Yes, Cloudhw support TDX too. Actually we have some PoC and plan to upstream it later.
BTW, cmos is needed in GetSystemMemorySizeBelow4gb which call CmosRead for 0x34/0x35.
> 
> > And PlatformInitLib will
> > not handle the S3/SMM variants.
> 
> At least not initially.  Maybe later when we move more code to the lib to
> reduce code duplication in xen/bhyve/qemu PlatformPei variants.
> 
> > 2. OvmfPkg/PlatformPei will be refactored with PlatformInitLib. The
> > functions not needed by config-b stay in PlatformPei.
> 
> > 3. Config-B support PEI-less boot for both legacy guest and td guest.
> 
> Yes.

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#85718): https://edk2.groups.io/g/devel/message/85718
Mute This Topic: https://groups.io/mt/87720802/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-