[PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr

[PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr

[Min Xu]

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

There is an issue reported when booting multiple vCPU guests (regular
and SEV). This issues consist of EfiAcquireLock() / EfiReleaseLock()
ASSERTS and TPL level ASSERTS that occur during ExitBootServices when
the APs are being parked by RelocateApLoop(). The root cause is that
PCD accesses use locking which is not SMP safe.

To fix this issue PCD usage can be reduced to getting the
PcdConfidentialComputingGuestAttr value at init (MpInitLibInitialize)
and caching it in a STATIC variable (mCcGuestAttr).

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: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 91c7afaeb2ad..fb9247f58427 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -16,6 +16,7 @@
 #include <ConfidentialComputingGuestAttr.h>
 
 EFI_GUID  mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
+UINT64    mCcGuestAttr         = 0;
 
 /**
   The function will check if BSP Execute Disable is enabled.
@@ -1805,7 +1806,9 @@ MpInitLibInitialize (
   UINTN                    BackupBufferAddr;
   UINTN                    ApIdtBase;
 
-  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
+  mCcGuestAttr = PcdGet64 (PcdConfidentialComputingGuestAttr);
+
+  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
     return EFI_SUCCESS;
   }
 
@@ -2079,7 +2082,7 @@ MpInitLibGetProcessorInfo (
   CPU_INFO_IN_HOB  *CpuInfoInHob;
   UINTN            OriginalProcessorNumber;
 
-  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
+  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
     return TdxMpInitLibGetProcessorInfo (ProcessorNumber, ProcessorInfoBuffer, HealthData);
   }
 
@@ -2177,7 +2180,7 @@ SwitchBSPWorker (
   BOOLEAN                      OldInterruptState;
   BOOLEAN                      OldTimerInterruptState;
 
-  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
+  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
     return EFI_UNSUPPORTED;
   }
 
@@ -2321,7 +2324,7 @@ EnableDisableApWorker (
   CPU_MP_DATA  *CpuMpData;
   UINTN        CallerNumber;
 
-  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
+  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
     return EFI_UNSUPPORTED;
   }
 
@@ -2385,7 +2388,7 @@ MpInitLibWhoAmI (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
+  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
     *ProcessorNumber = 0;
     return EFI_SUCCESS;
   }
@@ -2432,7 +2435,7 @@ MpInitLibGetNumberOfProcessors (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
+  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
     return TdxMpInitLibGetNumberOfProcessors (NumberOfProcessors, NumberOfEnabledProcessors);
   }
 
@@ -2534,7 +2537,7 @@ StartupAllCPUsWorker (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
+  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
     //
     // For Td guest ExcludeBsp must be FALSE. Otherwise it will return in above checks.
     //
@@ -2692,7 +2695,7 @@ StartupThisAPWorker (
   //
   // In Td guest, startup of AP is not supported in current stage.
   //
-  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
+  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
     return EFI_UNSUPPORTED;
   }
 
-- 
2.29.2.windows.2

Re: [PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr

[Ni, Ray]

Min,
You cannot use C global variable in PEIM.
Can you add a new field in _CPU_MP_DATA?

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Friday, April 29, 2022 11:03 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>;
> Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3918
> 
> There is an issue reported when booting multiple vCPU guests (regular
> and SEV). This issues consist of EfiAcquireLock() / EfiReleaseLock()
> ASSERTS and TPL level ASSERTS that occur during ExitBootServices when
> the APs are being parked by RelocateApLoop(). The root cause is that
> PCD accesses use locking which is not SMP safe.
> 
> To fix this issue PCD usage can be reduced to getting the
> PcdConfidentialComputingGuestAttr value at init (MpInitLibInitialize)
> and caching it in a STATIC variable (mCcGuestAttr).
> 
> 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: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 91c7afaeb2ad..fb9247f58427 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -16,6 +16,7 @@
>  #include <ConfidentialComputingGuestAttr.h>
> 
>  EFI_GUID  mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
> +UINT64    mCcGuestAttr         = 0;
> 
>  /**
>    The function will check if BSP Execute Disable is enabled.
> @@ -1805,7 +1806,9 @@ MpInitLibInitialize (
>    UINTN                    BackupBufferAddr;
>    UINTN                    ApIdtBase;
> 
> -  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +  mCcGuestAttr = PcdGet64 (PcdConfidentialComputingGuestAttr);
> +
> +  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
>      return EFI_SUCCESS;
>    }
> 
> @@ -2079,7 +2082,7 @@ MpInitLibGetProcessorInfo (
>    CPU_INFO_IN_HOB  *CpuInfoInHob;
>    UINTN            OriginalProcessorNumber;
> 
> -  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
>      return TdxMpInitLibGetProcessorInfo (ProcessorNumber, ProcessorInfoBuffer, HealthData);
>    }
> 
> @@ -2177,7 +2180,7 @@ SwitchBSPWorker (
>    BOOLEAN                      OldInterruptState;
>    BOOLEAN                      OldTimerInterruptState;
> 
> -  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
>      return EFI_UNSUPPORTED;
>    }
> 
> @@ -2321,7 +2324,7 @@ EnableDisableApWorker (
>    CPU_MP_DATA  *CpuMpData;
>    UINTN        CallerNumber;
> 
> -  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
>      return EFI_UNSUPPORTED;
>    }
> 
> @@ -2385,7 +2388,7 @@ MpInitLibWhoAmI (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> -  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
>      *ProcessorNumber = 0;
>      return EFI_SUCCESS;
>    }
> @@ -2432,7 +2435,7 @@ MpInitLibGetNumberOfProcessors (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> -  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
>      return TdxMpInitLibGetNumberOfProcessors (NumberOfProcessors, NumberOfEnabledProcessors);
>    }
> 
> @@ -2534,7 +2537,7 @@ StartupAllCPUsWorker (
>      return EFI_INVALID_PARAMETER;
>    }
> 
> -  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
>      //
>      // For Td guest ExcludeBsp must be FALSE. Otherwise it will return in above checks.
>      //
> @@ -2692,7 +2695,7 @@ StartupThisAPWorker (
>    //
>    // In Td guest, startup of AP is not supported in current stage.
>    //
> -  if (CC_GUEST_IS_TDX (PcdGet64 (PcdConfidentialComputingGuestAttr))) {
> +  if (CC_GUEST_IS_TDX (mCcGuestAttr)) {
>      return EFI_UNSUPPORTED;
>    }
> 
> --
> 2.29.2.windows.2

Re: [PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr

[Min Xu]

On April 29, 2022 11:07 AM, Ni Ray wrote:
> 
> Min,
> You cannot use C global variable in PEIM.
> Can you add a new field in _CPU_MP_DATA?
> 
The reason why C global variable cannot be used in PEIM is that in some scenario PEIM is executed in FLASH so that the value of C global variable cannot be kept in different calls. But I don't think it is a problem in this situation.
1. This global variable is to keep the PcdConfidentialComputingGuestAttribute in mCcGuestAttr. So if this is Tdx guest, then the global variable can be kept (CC_GUEST_IS_TDX (mCcGuestAttr) == TRUE). 
2. If this is Non-Td guest, then even the global variable cannot be kept, CC_GUEST_IS_TDX (mCcGuestAttr) is FALSE. So mCcGuestAttr can still work.

There is another solution that we can use CcProbe (which is in MdePkg/CcProbeLib). CcProbe checks the work area to fetch the guest type. It calls FixedPcdGet32 (PcdOvmfWorkAreaBase) so there is no SMP safe issue in PcdLib.

As to a new field in CPU_MP_DATA, Tdx guest doesn't create CPU_MP_DATA object in MpInitLibInitialize, instead it return right after it detects the working guest is Tdx guest. So this fix will be more complicated than above 2 solutions.

Ray, What's your thought?

Thanks
Min

Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 2016 bytes --]

On Mon, May  2, 2022 at 03:03 PM, Min Xu wrote:

>
> The reason why C global variable cannot be used in PEIM is that in some
> scenario PEIM is executed in FLASH so that the value of C global variable
> cannot be kept in different calls. But I don't think it is a problem in this
> situation.
> 1. This global variable is to keep the PcdConfidentialComputingGuestAttribute
> in mCcGuestAttr. So if this is Tdx guest, then the global variable can be kept
> (CC_GUEST_IS_TDX (mCcGuestAttr) == TRUE).
> 2. If this is Non-Td guest, then even the global variable cannot be kept,
> CC_GUEST_IS_TDX (mCcGuestAttr) is FALSE. So mCcGuestAttr can still work.

Directly writing to flash area might cause some side effects.
Usually we need to avoid that.

> 
> There is another solution that we can use CcProbe (which is in
> MdePkg/CcProbeLib). CcProbe checks the work area to fetch the guest type. It
> calls FixedPcdGet32 (PcdOvmfWorkAreaBase) so there is no SMP safe issue in
> PcdLib.

It seems ok. But it's unclear to me how the work area is built.



> 
> As to a new field in CPU_MP_DATA, Tdx guest doesn't create CPU_MP_DATA object
> in MpInitLibInitialize, instead it return right after it detects the working
> guest is Tdx guest. So this fix will be more complicated than above 2
> solutions.

I agree it'll be more complicated to let the TDX version of MpLib use the CPU_MP_DATA.
But, I am very curious why not leave the MpInitLib untouched. The solution is:
1. Platform FDF includes two CpuMpPeim drivers. One links to PeiMpInitLib, the other links to MpInitLibUp.
    Change platform DSC to let first PEIM depend on "FullMpPpi"
    Change platform DSC to let second PEIM depend on "UpMpPpi"
2. SEC code passes "FullMpPpi" to PEI Core when it's not a TDX boot. It passes "UpMpPpi" when it's a TDX boot.

I think this solution doesn't even change the MP code. Or saying in another way, MP and TDX are decoupled.
I prefer this solution.

> 
> Ray, What's your thought?
>

[-- Attachment #2: Type: text/html, Size: 2177 bytes --]

Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr

[Min Xu]

[-- Attachment #1: Type: text/plain, Size: 2797 bytes --]

Thanks for the suggestion. I tried the solution and it does work.
In a summary, the solution imports 2 CpuMpPei in OvmfPkgX64. Each CpuMpPei driver owns unique FILE_GUID so that both 2 drivers can be built into one image. A set of MpInitLibDepLib are imported. These libs are very simple and they just depends on the PPIs. Different PPI is installed based on the working guest type. With the help of these MpInitLibDepLib, we can leave the CpuMpPei untouched but in fact it depends on the PPIs.
The PoC is in: https://github.com/tianocore/edk2/pull/2852

I will send out the patch-set after some comments/words refinement.

From: Ni, Ray <ray.ni@intel.com>
Sent: Thursday, May 5, 2022 9:41 AM
To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Save PcdConfidentialComputingGuestAttr in mCcGuestAttr


On Mon, May 2, 2022 at 03:03 PM, Min Xu wrote:

The reason why C global variable cannot be used in PEIM is that in some scenario PEIM is executed in FLASH so that the value of C global variable cannot be kept in different calls. But I don't think it is a problem in this situation. 1. This global variable is to keep the PcdConfidentialComputingGuestAttribute in mCcGuestAttr. So if this is Tdx guest, then the global variable can be kept (CC_GUEST_IS_TDX (mCcGuestAttr) == TRUE). 2. If this is Non-Td guest, then even the global variable cannot be kept, CC_GUEST_IS_TDX (mCcGuestAttr) is FALSE. So mCcGuestAttr can still work.

Directly writing to flash area might cause some side effects. Usually we need to avoid that.

There is another solution that we can use CcProbe (which is in MdePkg/CcProbeLib). CcProbe checks the work area to fetch the guest type. It calls FixedPcdGet32 (PcdOvmfWorkAreaBase) so there is no SMP safe issue in PcdLib.

It seems ok. But it's unclear to me how the work area is built.

As to a new field in CPU_MP_DATA, Tdx guest doesn't create CPU_MP_DATA object in MpInitLibInitialize, instead it return right after it detects the working guest is Tdx guest. So this fix will be more complicated than above 2 solutions.

I agree it'll be more complicated to let the TDX version of MpLib use the CPU_MP_DATA. But, I am very curious why not leave the MpInitLib untouched. The solution is: 1. Platform FDF includes two CpuMpPeim drivers. One links to PeiMpInitLib, the other links to MpInitLibUp. Change platform DSC to let first PEIM depend on "FullMpPpi" Change platform DSC to let second PEIM depend on "UpMpPpi" 2. SEC code passes "FullMpPpi" to PEI Core when it's not a TDX boot. It passes "UpMpPpi" when it's a TDX boot.

I think this solution doesn't even change the MP code. Or saying in another way, MP and TDX are decoupled. I prefer this solution.

Ray, What's your thought?

[-- Attachment #2: Type: text/html, Size: 5386 bytes --]