Re: [edk2-devel] [PATCH v12 22/32] UefiCpuPkg/MpInitLib: use PcdConfidentialComputingAttr to check SEV status

2021-11-12 Thread James Bottomley 
On Fri, 2021-11-12 at 01:27 +, 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v12 22/32] UefiCpuPkg/MpInitLib: use PcdConfidentialComputingAttr to check SEV status

2021-11-12 Thread Brijesh Singh via groups.io 


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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v12 22/32] UefiCpuPkg/MpInitLib: use PcdConfidentialComputingAttr to check SEV status

2021-11-11 Thread Ni, Ray 
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v12 22/32] UefiCpuPkg/MpInitLib: use PcdConfidentialComputingAttr to check SEV status

2021-11-10 Thread Brijesh Singh via groups.io 
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 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Eric Dong 
Cc: James Bottomley 
Cc: Min Xu 
Cc: Jiewen Yao 
Cc: Tom Lendacky 
Cc: Jordan Justen 
Cc: Ard Biesheuvel 
Cc: Erdem Aktas 
Cc: Gerd Hoffmann 
Acked-by: Gerd Hoffmann 
Suggested-by: Jiewen Yao 
Signed-off-by: Brijesh Singh 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_ADDRESSStartAddress;
   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/MpI