Re: [PATCH v13 22/32] UefiCpuPkg/MpInitLib: use PcdConfidentialComputingAttr to check SEV status

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

Acked-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Brijesh Singh <brijesh.singh@amd.com>
> Sent: Saturday, November 13, 2021 1:40 AM
> To: devel@edk2.groups.io
> Cc: James Bottomley <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Erdem Aktas <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Michael Roth <michael.roth@amd.com>
> Subject: [PATCH v13 22/32] UefiCpuPkg/MpInitLib: use PcdConfidentialComputingAttr to check SEV status
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> Previous commit introduced a generic confidential computing PCD that can
> determine whether AMD SEV-ES is enabled. Update the MpInitLib to drop the
> PcdSevEsIsEnabled in favor of PcdConfidentialComputingAttr.
> 
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Suggested-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  2 +-
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  2 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 13 ++++
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  6 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 73 ++++++++++++++++++-
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  4 +-
>  6 files changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 6e510aa89120..de705bc54bb4 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -73,7 +73,7 @@ [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                           ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                       ## SOMETIMES_CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds  ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                          ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                       ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                           ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr           ## CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 2cbd9b8b8acc..b7e15ee023f0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -63,9 +63,9 @@ [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled                      ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase                   ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                       ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdConfidentialComputingGuestAttr       ## CONSUMES
> 
>  [Ppis]
>    gEdkiiPeiShadowMicrocodePpiGuid        ## SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 3d4446df8ce6..2107f3f705a2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -33,6 +33,7 @@
>  #include <Library/HobLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/MicrocodeLib.h>
> +#include <ConfidentialComputingGuestAttr.h>
> 
>  #include <Register/Amd/Fam17Msr.h>
>  #include <Register/Amd/Ghcb.h>
> @@ -774,5 +775,17 @@ SevEsPlaceApHlt (
>    CPU_MP_DATA                *CpuMpData
>    );
> 
> +/**
> + Check if the specified confidential computing attribute is active.
> +
> + @retval TRUE   The specified Attr is active.
> + @retval FALSE  The specified Attr is not active.
> +**/
> +BOOLEAN
> +EFIAPI
> +ConfidentialComputingGuestHas (
> +  CONFIDENTIAL_COMPUTING_GUEST_ATTR     Attr
> +  );
> +
>  #endif
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 93fc63bf93e3..657a73dca05e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -93,7 +93,7 @@ GetWakeupBuffer (
>    EFI_PHYSICAL_ADDRESS    StartAddress;
>    EFI_MEMORY_TYPE         MemoryType;
> 
> -  if (PcdGetBool (PcdSevEsIsEnabled)) {
> +  if (ConfidentialComputingGuestHas (CCAttrAmdSevEs)) {
>      MemoryType = EfiReservedMemoryType;
>    } else {
>      MemoryType = EfiBootServicesData;
> @@ -107,7 +107,7 @@ GetWakeupBuffer (
>    // LagacyBios driver depends on CPU Arch protocol which guarantees below
>    // allocation runs earlier than LegacyBios driver.
>    //
> -  if (PcdGetBool (PcdSevEsIsEnabled)) {
> +  if (ConfidentialComputingGuestHas (CCAttrAmdSevEs)) {
>      //
>      // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
>      //
> @@ -124,7 +124,7 @@ GetWakeupBuffer (
>    ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
> -  } else if (PcdGetBool (PcdSevEsIsEnabled)) {
> +  } else if (ConfidentialComputingGuestHas (CCAttrAmdSevEs)) {
>      //
>      // Next SEV-ES wakeup buffer allocation must be below this allocation
>      //
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 890945bc5994..b6c8a1a04d9f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -295,7 +295,7 @@ GetApLoopMode (
>        ApLoopMode = ApInHltLoop;
>      }
> 
> -    if (PcdGetBool (PcdSevEsIsEnabled)) {
> +    if (ConfidentialComputingGuestHas (CCAttrAmdSevEs)) {
>        //
>        // For SEV-ES, force AP in Hlt-loop mode in order to use the GHCB
>        // protocol for starting APs
> @@ -1046,7 +1046,7 @@ AllocateResetVector (
>      // The AP reset stack is only used by SEV-ES guests. Do not allocate it
>      // if SEV-ES is not enabled.
>      //
> -    if (PcdGetBool (PcdSevEsIsEnabled)) {
> +    if (ConfidentialComputingGuestHas (CCAttrAmdSevEs)) {
>        //
>        // Stack location is based on ProcessorNumber, so use the total number
>        // of processors for calculating the total stack area.
> @@ -1816,7 +1816,7 @@ MpInitLibInitialize (
>    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
>    CpuMpData->CpuInfoInHob     = (UINT64) (UINTN) (CpuMpData->CpuData + MaxLogicalProcessorNumber);
>    InitializeSpinLock(&CpuMpData->MpLock);
> -  CpuMpData->SevEsIsEnabled = PcdGetBool (PcdSevEsIsEnabled);
> +  CpuMpData->SevEsIsEnabled = ConfidentialComputingGuestHas (CCAttrAmdSevEs);
>    CpuMpData->SevEsAPBuffer  = (UINTN) -1;
>    CpuMpData->GhcbBase       = PcdGet64 (PcdGhcbBase);
> 
> @@ -2706,3 +2706,70 @@ MpInitLibStartupAllCPUs (
>             NULL
>             );
>  }
> +
> +/**
> +  The function check if the specified Attr is set.
> +
> +  @param[in]  CurrentAttr   The current attribute.
> +  @param[in]  Attr          The attribute to check.
> +
> +  @retval  TRUE      The specified Attr is set.
> +  @retval  FALSE     The specified Attr is not set.
> +
> +**/
> +STATIC
> +BOOLEAN
> +AmdMemEncryptionAttrCheck (
> +  IN  UINT64                                CurrentAttr,
> +  IN  CONFIDENTIAL_COMPUTING_GUEST_ATTR     Attr
> +  )
> +{
> +  switch (Attr) {
> +    case CCAttrAmdSev:
> +      //
> +      // SEV is automatically enabled if SEV-ES or SEV-SNP is active.
> +      //
> +      return CurrentAttr >= CCAttrAmdSev;
> +    case CCAttrAmdSevEs:
> +      //
> +      // SEV-ES is automatically enabled if SEV-SNP is active.
> +      //
> +      return CurrentAttr >= CCAttrAmdSevEs;
> +    case CCAttrAmdSevSnp:
> +      return CurrentAttr == CCAttrAmdSevSnp;
> +    default:
> +      return FALSE;
> +  }
> +}
> +
> +/**
> +  Check if the specified confidential computing attribute is active.
> +
> +  @param[in]  Attr          The attribute to check.
> +
> +  @retval TRUE   The specified Attr is active.
> +  @retval FALSE  The specified Attr is not active.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +ConfidentialComputingGuestHas (
> +  IN  CONFIDENTIAL_COMPUTING_GUEST_ATTR     Attr
> +  )
> +{
> +  UINT64    CurrentAttr;
> +
> +  //
> +  // Get the current CC attribute.
> +  //
> +  CurrentAttr = PcdGet64 (PcdConfidentialComputingGuestAttr);
> +
> +  //
> +  // If attr is for the AMD group then call AMD specific checks.
> +  //
> +  if (((RShiftU64 (CurrentAttr, 8)) & 0xff) == 1) {
> +    return AmdMemEncryptionAttrCheck (CurrentAttr, Attr);
> +  }
> +
> +  return (CurrentAttr == Attr);
> +}
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 90015c650c68..2f333a00460a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -222,7 +222,7 @@ GetWakeupBuffer (
>          // Need memory under 1MB to be collected here
>          //
>          WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
> -        if (PcdGetBool (PcdSevEsIsEnabled) &&
> +        if (ConfidentialComputingGuestHas (CCAttrAmdSevEs) &&
>              WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
>            //
>            // SEV-ES Wakeup buffer should be under 1MB and under any previous one
> @@ -253,7 +253,7 @@ GetWakeupBuffer (
>            DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>                                 WakeupBufferStart, WakeupBufferSize));
> 
> -          if (PcdGetBool (PcdSevEsIsEnabled)) {
> +          if (ConfidentialComputingGuestHas (CCAttrAmdSevEs)) {
>              //
>              // Next SEV-ES wakeup buffer allocation must be below this
>              // allocation
> --
> 2.25.1