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

["Ni, Ray" <ray.ni@intel.com>]

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