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          | 67 ++++++++++++++++++-
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       |  4 +-
 6 files changed, 84 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..9109607c87a9 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,64 @@ 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:
+      return CurrentAttr >= CCAttrAmdSev;
+    case CCAttrAmdSevEs:
+      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



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

2 minor comments. 

> +  switch (Attr) {
> +    case CCAttrAmdSev:
> +      return CurrentAttr >= CCAttrAmdSev;
> +    case CCAttrAmdSevEs:
> +      return CurrentAttr >= CCAttrAmdSevEs;
> +    case CCAttrAmdSevSnp:
> +      return CurrentAttr == CCAttrAmdSevSnp;

1.  Can you put comments to explain that the relationship between the three features?
That can explain why ">=" is used here.
You may use ">=" for SEV-SNP as well, in case AMD invents a more advanced SEV.:)


> +
> +  return (CurrentAttr == Attr);

2. I guess a "BOOLEAN" type cast is needed.



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

On 11/11/21 7:27 PM, Ni, Ray wrote:
> 2 minor comments. 
>
>> +  switch (Attr) {
>> +    case CCAttrAmdSev:
>> +      return CurrentAttr >= CCAttrAmdSev;
>> +    case CCAttrAmdSevEs:
>> +      return CurrentAttr >= CCAttrAmdSevEs;
>> +    case CCAttrAmdSevSnp:
>> +      return CurrentAttr == CCAttrAmdSevSnp;
> 1.  Can you put comments to explain that the relationship between the three features?
> That can explain why ">=" is used here.
> You may use ">=" for SEV-SNP as well, in case AMD invents a more advanced SEV.:)

Sure, I can add a comment.


>
>> +
>> +  return (CurrentAttr == Attr);
> 2. I guess a "BOOLEAN" type cast is needed.
>
I can cast it but CI didn't complain about it.




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

On Fri, 2021-11-12 at 01:27 +0000, Ni, Ray wrote:
[...]
> > +
> > +  return (CurrentAttr == Attr);
> 
> 2. I guess a "BOOLEAN" type cast is needed.

It shouldn't.  Unless there's a major screw up in the way BOOLEAN works
in the UEFI API, all logic operations should already be of type BOOLEAN
and if they're not that would be what needs fixing.

James




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