[PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Min Xu]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3904

TdxDxe driver is introduced for Intel TDX feature. Unfortunately, this
driver also breaks boot process in SEV-ES guest. The root cause is in
the PciLib which is imported by TdxDxe driver.

In a SEV-ES guest the AmdSevDxe driver performs a
MemEncryptSevClearMmioPageEncMask() call against the
PcdPciExpressBaseAddress range to mark it shared/unencrypted. However,
the TdxDxe driver is loaded before the AmdSevDxe driver, and the PciLib
in TdxDxe is DxePciLibI440FxQ35 which will access the
PcdPciExpressBaseAddress range. Since the range has not been marked
shared/unencrypted, the #VC handler terminates the guest for trying to
do MMIO to an encrypted region.

To fix the issue TdxDxe driver set the PciLib to BasePciLibCf8.inf as
AmdSevDxe driver does.

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>
SEV-Tested-by: Tom Lendacky <thomas.lendacky@amd.com>
TDX-Tested-by: Min Xu <min.m.xu@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/IntelTdx/IntelTdxX64.dsc | 5 ++++-
 OvmfPkg/OvmfPkgX64.dsc           | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 245155d41b..f58f14a1d8 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -704,7 +704,10 @@
   OvmfPkg/PlatformDxe/Platform.inf
   OvmfPkg/IoMmuDxe/IoMmuDxe.inf
 
-  OvmfPkg/TdxDxe/TdxDxe.inf
+  OvmfPkg/TdxDxe/TdxDxe.inf {
+    <LibraryClasses>
+    PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
+  }
 
   #
   # Variable driver stack (non-SMM)
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index fb2899f8a1..68e7d051d0 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -967,7 +967,10 @@
   }
   OvmfPkg/IoMmuDxe/IoMmuDxe.inf
 
-  OvmfPkg/TdxDxe/TdxDxe.inf
+  OvmfPkg/TdxDxe/TdxDxe.inf {
+    <LibraryClasses>
+    PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
+  }
 
 !if $(SMM_REQUIRE) == TRUE
   OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
-- 
2.29.2.windows.2

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Yao, Jiewen]

Hi
If TdxDxe breaks SEV, should we skip the TdxDxe in SEV path?

I don't understand why we need use Cfg8.

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Min Xu
> Sent: Tuesday, April 19, 2022 9:58 AM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3904
> 
> TdxDxe driver is introduced for Intel TDX feature. Unfortunately, this
> driver also breaks boot process in SEV-ES guest. The root cause is in
> the PciLib which is imported by TdxDxe driver.
> 
> In a SEV-ES guest the AmdSevDxe driver performs a
> MemEncryptSevClearMmioPageEncMask() call against the
> PcdPciExpressBaseAddress range to mark it shared/unencrypted. However,
> the TdxDxe driver is loaded before the AmdSevDxe driver, and the PciLib
> in TdxDxe is DxePciLibI440FxQ35 which will access the
> PcdPciExpressBaseAddress range. Since the range has not been marked
> shared/unencrypted, the #VC handler terminates the guest for trying to
> do MMIO to an encrypted region.
> 
> To fix the issue TdxDxe driver set the PciLib to BasePciLibCf8.inf as
> AmdSevDxe driver does.
> 
> 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>
> SEV-Tested-by: Tom Lendacky <thomas.lendacky@amd.com>
> TDX-Tested-by: Min Xu <min.m.xu@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc | 5 ++++-
>  OvmfPkg/OvmfPkgX64.dsc           | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> index 245155d41b..f58f14a1d8 100644
> --- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> +++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> @@ -704,7 +704,10 @@
>    OvmfPkg/PlatformDxe/Platform.inf
>    OvmfPkg/IoMmuDxe/IoMmuDxe.inf
> 
> -  OvmfPkg/TdxDxe/TdxDxe.inf
> +  OvmfPkg/TdxDxe/TdxDxe.inf {
> +    <LibraryClasses>
> +    PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
> +  }
> 
>    #
>    # Variable driver stack (non-SMM)
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index fb2899f8a1..68e7d051d0 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -967,7 +967,10 @@
>    }
>    OvmfPkg/IoMmuDxe/IoMmuDxe.inf
> 
> -  OvmfPkg/TdxDxe/TdxDxe.inf
> +  OvmfPkg/TdxDxe/TdxDxe.inf {
> +    <LibraryClasses>
> +    PciLib|MdePkg/Library/BasePciLibCf8/BasePciLibCf8.inf
> +  }
> 
>  !if $(SMM_REQUIRE) == TRUE
>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
> --
> 2.29.2.windows.2
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Min Xu]

On April 19, 2022 10:08 AM, Yao Jiewen wrote:
> 
> If TdxDxe breaks SEV, should we skip the TdxDxe in SEV path?
> 
> I don't understand why we need use Cfg8.
> 
In TdxDxe driver we need to relocate APs and it requires the TdxMailboxLib. 
The lib chain is that TdxMailbox ->  SynchronizationLib -> TimerLib (which is DxeAcpiTimerLib) -> PciLib (Which was DxePciLibI440FxQ35).
TdxDxe driver was loaded before AmdSevDxe so its lib constructors are called even it is in SEV-ES guest. In other words those lib constructor cannot be skipped in SEV-ES guest.

The analysis of the root cause (why DxePciLibI440FxQ35 trigger the broken) is in below link:
https://edk2.groups.io/g/devel/message/88968

Thanks
Min

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Yao, Jiewen]

Why does TdxDxe call TdxMailbox in an SEV platform?
Or why does TdxMailbox call SynchronizationLib in an SEV platform?

There are many places we can do CcProbe to stop action. Why we need do it in DSC?

In general, DSC lib override should be the last option, because it requires the platform integrator aware such tricky thing. I strongly discourage to use this, although it is architecturally support. 

I do believe we have many other ways to achieve the same goal.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Tuesday, April 19, 2022 10:42 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> 
> On April 19, 2022 10:08 AM, Yao Jiewen wrote:
> >
> > If TdxDxe breaks SEV, should we skip the TdxDxe in SEV path?
> >
> > I don't understand why we need use Cfg8.
> >
> In TdxDxe driver we need to relocate APs and it requires the TdxMailboxLib.
> The lib chain is that TdxMailbox ->  SynchronizationLib -> TimerLib (which is
> DxeAcpiTimerLib) -> PciLib (Which was DxePciLibI440FxQ35).
> TdxDxe driver was loaded before AmdSevDxe so its lib constructors are called
> even it is in SEV-ES guest. In other words those lib constructor cannot be skipped
> in SEV-ES guest.
> 
> The analysis of the root cause (why DxePciLibI440FxQ35 trigger the broken) is in
> below link:
> https://edk2.groups.io/g/devel/message/88968
> 
> Thanks
> Min

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Min Xu]

On April 19, 2022 10:54 AM, Yao Jiewen wrote:
> 
> Why does TdxDxe call TdxMailbox in an SEV platform?
> Or why does TdxMailbox call SynchronizationLib in an SEV platform?
>
TdxDxe will not call TdxMailbox/SynchronizationLib in SEV platform. The problem is in the lib constructor. When TdxDxe driver is loaded, before its entry point is called, the lib constructors will be called even in a SEV platform.
> 
> There are many places we can do CcProbe to stop action. Why we need do it
> in DSC?
So we cannot stop the lib constructor with CcProbe in this case.

Thanks
Min 

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Yao, Jiewen]

Do you mean, with SEV introduced, OVMF cannot use PCI express any more?

Thank you
Yao Jiewen


> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Tuesday, April 19, 2022 11:05 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> 
> On April 19, 2022 10:54 AM, Yao Jiewen wrote:
> >
> > Why does TdxDxe call TdxMailbox in an SEV platform?
> > Or why does TdxMailbox call SynchronizationLib in an SEV platform?
> >
> TdxDxe will not call TdxMailbox/SynchronizationLib in SEV platform. The
> problem is in the lib constructor. When TdxDxe driver is loaded, before its entry
> point is called, the lib constructors will be called even in a SEV platform.
> >
> > There are many places we can do CcProbe to stop action. Why we need do it
> > in DSC?
> So we cannot stop the lib constructor with CcProbe in this case.
> 
> Thanks
> Min

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Min Xu]

In AmdSevDxe's entry point it clears the C-bit from PcdPciExpressBaseAddress and other memory spaces if needed. Please see https://github.com/tianocore/edk2/blob/master/OvmfPkg/AmdSevDxe/AmdSevDxe.c#L81-L95. After that OVMF can use PCI express.

This broken is caused by the call sequence of TdxDxe driver and AmdSevDxe driver. Currently TdxDxe driver is loaded before AmdSevDxe, so in SEV-ES guest the C-bit of PcdPciExpressBaseAddress hasn't been cleared. In this situation the access to PciExpressBaseAddress trigger exceptions (lib constructor in TdxDxe).

There are 2 options to fix this issue.
1. Adjust the load sequence of AmdSevDxe and TdxDxe (Load AmdSevDxe before TdxDxe)
2. Make TdxDxe to import BasePciLibCf8.inf instead of DxePciLibI440FxQ35.inf (just like AmdSevDxe)

Tom and I tested above 2 options in SEV and TDX and all work.

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Tuesday, April 19, 2022 12:16 PM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> 
> Do you mean, with SEV introduced, OVMF cannot use PCI express any more?
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Tuesday, April 19, 2022 11:05 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> > Lendacky <thomas.lendacky@amd.com>
> > Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe
> > driver
> >
> > On April 19, 2022 10:54 AM, Yao Jiewen wrote:
> > >
> > > Why does TdxDxe call TdxMailbox in an SEV platform?
> > > Or why does TdxMailbox call SynchronizationLib in an SEV platform?
> > >
> > TdxDxe will not call TdxMailbox/SynchronizationLib in SEV platform.
> > The problem is in the lib constructor. When TdxDxe driver is loaded,
> > before its entry point is called, the lib constructors will be called even in a
> SEV platform.
> > >
> > > There are many places we can do CcProbe to stop action. Why we need
> > > do it in DSC?
> > So we cannot stop the lib constructor with CcProbe in this case.
> >
> > Thanks
> > Min

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Yao, Jiewen]

Can SEV clear the C-bit in SEC phase?

I think that is right way to ensure PCI Express can always be accessed by anyone.


> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Tuesday, April 19, 2022 12:39 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> 
> In AmdSevDxe's entry point it clears the C-bit from PcdPciExpressBaseAddress
> and other memory spaces if needed. Please see
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AmdSevDxe/AmdSev
> Dxe.c#L81-L95. After that OVMF can use PCI express.
> 
> This broken is caused by the call sequence of TdxDxe driver and AmdSevDxe
> driver. Currently TdxDxe driver is loaded before AmdSevDxe, so in SEV-ES guest
> the C-bit of PcdPciExpressBaseAddress hasn't been cleared. In this situation the
> access to PciExpressBaseAddress trigger exceptions (lib constructor in TdxDxe).
> 
> There are 2 options to fix this issue.
> 1. Adjust the load sequence of AmdSevDxe and TdxDxe (Load AmdSevDxe before
> TdxDxe)
> 2. Make TdxDxe to import BasePciLibCf8.inf instead of DxePciLibI440FxQ35.inf
> (just like AmdSevDxe)
> 
> Tom and I tested above 2 options in SEV and TDX and all work.
> 
> > -----Original Message-----
> > From: Yao, Jiewen <jiewen.yao@intel.com>
> > Sent: Tuesday, April 19, 2022 12:16 PM
> > To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> > Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> > Lendacky <thomas.lendacky@amd.com>
> > Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> >
> > Do you mean, with SEV introduced, OVMF cannot use PCI express any more?
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Xu, Min M <min.m.xu@intel.com>
> > > Sent: Tuesday, April 19, 2022 11:05 AM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > > Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> > > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> > > Lendacky <thomas.lendacky@amd.com>
> > > Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe
> > > driver
> > >
> > > On April 19, 2022 10:54 AM, Yao Jiewen wrote:
> > > >
> > > > Why does TdxDxe call TdxMailbox in an SEV platform?
> > > > Or why does TdxMailbox call SynchronizationLib in an SEV platform?
> > > >
> > > TdxDxe will not call TdxMailbox/SynchronizationLib in SEV platform.
> > > The problem is in the lib constructor. When TdxDxe driver is loaded,
> > > before its entry point is called, the lib constructors will be called even in a
> > SEV platform.
> > > >
> > > > There are many places we can do CcProbe to stop action. Why we need
> > > > do it in DSC?
> > > So we cannot stop the lib constructor with CcProbe in this case.
> > >
> > > Thanks
> > > Min

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Yao, Jiewen]

OK. Let me describe what I think.

PCI Express BAR need to be initialized by someone in the platform.
This initialization may require CFG8. That is understandable.

A good design is that: After the PCIE BAR is initialized, it can be accessed.
Requires additional step (such as clear C-bit) means the PCIE BAR is not fully initialized originally. I don't think it is a good idea.

So far, the problem is TdxDxe, but what if a PEI driver also wants to use access PCIE space? It may run into same problem.

I think the best way is to clear C-bit in PciExBarInitialization(), as SEV specific step to finish initialization. https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformInitLib/Platform.c#L261

As such, no matter how many drivers want to use PCIE, they can.


Splitting PCIE bar programming and C bit clearing is a big problem. In this window, no one can actually touch the PCIE bar, although it seems being initialized...


Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
> Sent: Tuesday, April 19, 2022 12:47 PM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> 
> Can SEV clear the C-bit in SEC phase?
> 
> I think that is right way to ensure PCI Express can always be accessed by anyone.
> 
> 
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Tuesday, April 19, 2022 12:39 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> > Lendacky <thomas.lendacky@amd.com>
> > Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> >
> > In AmdSevDxe's entry point it clears the C-bit from PcdPciExpressBaseAddress
> > and other memory spaces if needed. Please see
> >
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AmdSevDxe/AmdSev
> > Dxe.c#L81-L95. After that OVMF can use PCI express.
> >
> > This broken is caused by the call sequence of TdxDxe driver and AmdSevDxe
> > driver. Currently TdxDxe driver is loaded before AmdSevDxe, so in SEV-ES guest
> > the C-bit of PcdPciExpressBaseAddress hasn't been cleared. In this situation
> the
> > access to PciExpressBaseAddress trigger exceptions (lib constructor in TdxDxe).
> >
> > There are 2 options to fix this issue.
> > 1. Adjust the load sequence of AmdSevDxe and TdxDxe (Load AmdSevDxe
> before
> > TdxDxe)
> > 2. Make TdxDxe to import BasePciLibCf8.inf instead of DxePciLibI440FxQ35.inf
> > (just like AmdSevDxe)
> >
> > Tom and I tested above 2 options in SEV and TDX and all work.
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen <jiewen.yao@intel.com>
> > > Sent: Tuesday, April 19, 2022 12:16 PM
> > > To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> > > Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> > > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> > > Lendacky <thomas.lendacky@amd.com>
> > > Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
> > >
> > > Do you mean, with SEV introduced, OVMF cannot use PCI express any more?
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > > -----Original Message-----
> > > > From: Xu, Min M <min.m.xu@intel.com>
> > > > Sent: Tuesday, April 19, 2022 11:05 AM
> > > > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
> > > > Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
> > > > <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
> Tom
> > > > Lendacky <thomas.lendacky@amd.com>
> > > > Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe
> > > > driver
> > > >
> > > > On April 19, 2022 10:54 AM, Yao Jiewen wrote:
> > > > >
> > > > > Why does TdxDxe call TdxMailbox in an SEV platform?
> > > > > Or why does TdxMailbox call SynchronizationLib in an SEV platform?
> > > > >
> > > > TdxDxe will not call TdxMailbox/SynchronizationLib in SEV platform.
> > > > The problem is in the lib constructor. When TdxDxe driver is loaded,
> > > > before its entry point is called, the lib constructors will be called even in a
> > > SEV platform.
> > > > >
> > > > > There are many places we can do CcProbe to stop action. Why we need
> > > > > do it in DSC?
> > > > So we cannot stop the lib constructor with CcProbe in this case.
> > > >
> > > > Thanks
> > > > Min
> 
> 
> 
> 

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Lendacky, Thomas]

On 4/18/22 23:47, Yao, Jiewen wrote:
> Can SEV clear the C-bit in SEC phase?

Not really. IIRC, even if cleared in the SEC phase, the DXE phase replaces 
the page tables and it has to be cleared again.

Thanks,
Tom

> 
> I think that is right way to ensure PCI Express can always be accessed by anyone.
> 
> 
>> -----Original Message-----
>> From: Xu, Min M <min.m.xu@intel.com>
>> Sent: Tuesday, April 19, 2022 12:39 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
>> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
>> Lendacky <thomas.lendacky@amd.com>
>> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
>>
>> In AmdSevDxe's entry point it clears the C-bit from PcdPciExpressBaseAddress
>> and other memory spaces if needed. Please see
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FOvmfPkg%2FAmdSevDxe%2FAmdSev&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7Cc39c49fd4e944900bdb708da21bfac91%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637859404370071519%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=9sxJnGXyaiHTdIzS%2BTzziBnwTAsKvSLFRMmHT4HGe60%3D&amp;reserved=0
>> Dxe.c#L81-L95. After that OVMF can use PCI express.
>>
>> This broken is caused by the call sequence of TdxDxe driver and AmdSevDxe
>> driver. Currently TdxDxe driver is loaded before AmdSevDxe, so in SEV-ES guest
>> the C-bit of PcdPciExpressBaseAddress hasn't been cleared. In this situation the
>> access to PciExpressBaseAddress trigger exceptions (lib constructor in TdxDxe).
>>
>> There are 2 options to fix this issue.
>> 1. Adjust the load sequence of AmdSevDxe and TdxDxe (Load AmdSevDxe before
>> TdxDxe)
>> 2. Make TdxDxe to import BasePciLibCf8.inf instead of DxePciLibI440FxQ35.inf
>> (just like AmdSevDxe)
>>
>> Tom and I tested above 2 options in SEV and TDX and all work.
>>
>>> -----Original Message-----
>>> From: Yao, Jiewen <jiewen.yao@intel.com>
>>> Sent: Tuesday, April 19, 2022 12:16 PM
>>> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
>>> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
>>> Lendacky <thomas.lendacky@amd.com>
>>> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
>>>
>>> Do you mean, with SEV introduced, OVMF cannot use PCI express any more?
>>>
>>> Thank you
>>> Yao Jiewen
>>>
>>>
>>>> -----Original Message-----
>>>> From: Xu, Min M <min.m.xu@intel.com>
>>>> Sent: Tuesday, April 19, 2022 11:05 AM
>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
>>>> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
>>>> Lendacky <thomas.lendacky@amd.com>
>>>> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe
>>>> driver
>>>>
>>>> On April 19, 2022 10:54 AM, Yao Jiewen wrote:
>>>>>
>>>>> Why does TdxDxe call TdxMailbox in an SEV platform?
>>>>> Or why does TdxMailbox call SynchronizationLib in an SEV platform?
>>>>>
>>>> TdxDxe will not call TdxMailbox/SynchronizationLib in SEV platform.
>>>> The problem is in the lib constructor. When TdxDxe driver is loaded,
>>>> before its entry point is called, the lib constructors will be called even in a
>>> SEV platform.
>>>>>
>>>>> There are many places we can do CcProbe to stop action. Why we need
>>>>> do it in DSC?
>>>> So we cannot stop the lib constructor with CcProbe in this case.
>>>>
>>>> Thanks
>>>> Min

Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver

[Lendacky, Thomas]

On 4/19/22 00:06, Yao, Jiewen wrote:
> OK. Let me describe what I think.
> 
> PCI Express BAR need to be initialized by someone in the platform.
> This initialization may require CFG8. That is understandable.
> 
> A good design is that: After the PCIE BAR is initialized, it can be accessed.
> Requires additional step (such as clear C-bit) means the PCIE BAR is not fully initialized originally. I don't think it is a good idea.
> 
> So far, the problem is TdxDxe, but what if a PEI driver also wants to use access PCIE space? It may run into same problem.
> 
> I think the best way is to clear C-bit in PciExBarInitialization(), as SEV specific step to finish initialization. https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PlatformInitLib/Platform.c#L261
> 
> As such, no matter how many drivers want to use PCIE, they can.
> 
> 
> Splitting PCIE bar programming and C bit clearing is a big problem. In this window, no one can actually touch the PCIE bar, although it seems being initialized...

I tried this approach and it does not work. It is because new page tables 
are used in the DXE phase and so the c-bit has to be cleared in the new 
page tables vs the page tables used in PEI.

Thanks,
Tom

> 
> 
> Thank you
> Yao Jiewen
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
>> Sent: Tuesday, April 19, 2022 12:47 PM
>> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
>> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
>> Lendacky <thomas.lendacky@amd.com>
>> Subject: Re: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
>>
>> Can SEV clear the C-bit in SEC phase?
>>
>> I think that is right way to ensure PCI Express can always be accessed by anyone.
>>
>>
>>> -----Original Message-----
>>> From: Xu, Min M <min.m.xu@intel.com>
>>> Sent: Tuesday, April 19, 2022 12:39 PM
>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
>>> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
>>> Lendacky <thomas.lendacky@amd.com>
>>> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
>>>
>>> In AmdSevDxe's entry point it clears the C-bit from PcdPciExpressBaseAddress
>>> and other memory spaces if needed. Please see
>>>
>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/AmdSevDxe/AmdSev
>>> Dxe.c#L81-L95. After that OVMF can use PCI express.
>>>
>>> This broken is caused by the call sequence of TdxDxe driver and AmdSevDxe
>>> driver. Currently TdxDxe driver is loaded before AmdSevDxe, so in SEV-ES guest
>>> the C-bit of PcdPciExpressBaseAddress hasn't been cleared. In this situation
>> the
>>> access to PciExpressBaseAddress trigger exceptions (lib constructor in TdxDxe).
>>>
>>> There are 2 options to fix this issue.
>>> 1. Adjust the load sequence of AmdSevDxe and TdxDxe (Load AmdSevDxe
>> before
>>> TdxDxe)
>>> 2. Make TdxDxe to import BasePciLibCf8.inf instead of DxePciLibI440FxQ35.inf
>>> (just like AmdSevDxe)
>>>
>>> Tom and I tested above 2 options in SEV and TDX and all work.
>>>
>>>> -----Original Message-----
>>>> From: Yao, Jiewen <jiewen.yao@intel.com>
>>>> Sent: Tuesday, April 19, 2022 12:16 PM
>>>> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
>>>> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
>>>> Lendacky <thomas.lendacky@amd.com>
>>>> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe driver
>>>>
>>>> Do you mean, with SEV introduced, OVMF cannot use PCI express any more?
>>>>
>>>> Thank you
>>>> Yao Jiewen
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Xu, Min M <min.m.xu@intel.com>
>>>>> Sent: Tuesday, April 19, 2022 11:05 AM
>>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Aktas, Erdem
>>>>> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
>> Tom
>>>>> Lendacky <thomas.lendacky@amd.com>
>>>>> Subject: RE: [edk2-devel] [PATCH] OvmfPkg: Set PciLib for TdxDxe
>>>>> driver
>>>>>
>>>>> On April 19, 2022 10:54 AM, Yao Jiewen wrote:
>>>>>>
>>>>>> Why does TdxDxe call TdxMailbox in an SEV platform?
>>>>>> Or why does TdxMailbox call SynchronizationLib in an SEV platform?
>>>>>>
>>>>> TdxDxe will not call TdxMailbox/SynchronizationLib in SEV platform.
>>>>> The problem is in the lib constructor. When TdxDxe driver is loaded,
>>>>> before its entry point is called, the lib constructors will be called even in a
>>>> SEV platform.
>>>>>>
>>>>>> There are many places we can do CcProbe to stop action. Why we need
>>>>>> do it in DSC?
>>>>> So we cannot stop the lib constructor with CcProbe in this case.
>>>>>
>>>>> Thanks
>>>>> Min
>>
>>
>> 
>>
>