[PATCH v2 0/4] Fixes for SEV-SNP CC blob and CPUID table handling

[PATCH v2 0/4] Fixes for SEV-SNP CC blob and CPUID table handling

[Roth, Michael]

(Mainly a resend of v1, but rolled in Gerd's Acked-by's, addressed
new coding style check in the CI, and updated Cc list)

Here are a number of fixes related to OVMF handling of the SEV-SNP
Confidential Computing blob and CPUID table.

Patch #1 is a fix for recently-reported issue that can cause
significant problems with some SEV-SNP guest operating systems.
Please consider applying this patch directly if the other
patches in this series are held up for any reason.

Patches 2-4 are minor changes for things that aren't currently
triggered in practice, but make OVMF's SEV-SNP implementation more
robust for different build/hypervisor environments in the future.
Patch #2 was submitted previously, but refreshed here to apply
cleanly on top of Patch #1, with no other functional changes since
the initial review.

v2:
 - rebased/retested on latest master
 - replaced usage of __FUNCTION__ with __func__ to comply with new CI
   test cases

----------------------------------------------------------------
Michael Roth (4):
      OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory
      OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition
      OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation
      OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP

 OvmfPkg/AmdSevDxe/AmdSevDxe.c                          | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++--------------
 OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h |  6 ++++--
 OvmfPkg/Library/CcExitLib/CcExitVcHandler.c            | 13 +++++--------
 3 files changed, 59 insertions(+), 24 deletions(-)

[PATCH v2 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory

[Roth, Michael]

The SEV-SNP Confidential Computing blob contains metadata that should
remain accessible for the life of the guest. Allocate it as
EfiACPIReclaimMemory to ensure the memory isn't overwritten by the guest
operating system later.

Reported-by: Dov Murik <dovmurik@linux.ibm.com>
Suggested-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 OvmfPkg/AmdSevDxe/AmdSevDxe.c | 62 +++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 05b728d32a..df807066fa 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -28,15 +28,36 @@
 // Present, initialized, tested bits defined in MdeModulePkg/Core/Dxe/DxeMain.h
 #define EFI_MEMORY_INTERNAL_MASK  0x0700000000000000ULL
 
-STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
-  SIGNATURE_32 ('A',                                    'M', 'D', 'E'),
-  1,
-  0,
-  (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase),
-  FixedPcdGet32 (PcdOvmfSnpSecretsSize),
-  (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase),
-  FixedPcdGet32 (PcdOvmfCpuidSize),
-};
+STATIC
+EFI_STATUS
+AllocateConfidentialComputingBlob (
+  OUT CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  **CcBlobPtr
+  )
+{
+  EFI_STATUS                                Status;
+  CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  *CcBlob;
+
+  Status = gBS->AllocatePool (
+                  EfiACPIReclaimMemory,
+                  sizeof (CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION),
+                  (VOID **)&CcBlob
+                  );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  CcBlob->Header                 = SIGNATURE_32 ('A', 'M', 'D', 'E');
+  CcBlob->Version                = 1;
+  CcBlob->Reserved1              = 0;
+  CcBlob->SecretsPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase);
+  CcBlob->SecretsSize            = FixedPcdGet32 (PcdOvmfSnpSecretsSize);
+  CcBlob->CpuidPhysicalAddress   = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase);
+  CcBlob->CpuidLSize             = FixedPcdGet32 (PcdOvmfCpuidSize);
+
+  *CcBlobPtr = CcBlob;
+
+  return EFI_SUCCESS;
+}
 
 STATIC EFI_HANDLE  mAmdSevDxeHandle = NULL;
 
@@ -175,10 +196,11 @@ AmdSevDxeEntryPoint (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
-  EFI_STATUS                       Status;
-  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *AllDescMap;
-  UINTN                            NumEntries;
-  UINTN                            Index;
+  EFI_STATUS                                Status;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR           *AllDescMap;
+  UINTN                                     NumEntries;
+  UINTN                                     Index;
+  CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  *SnpBootDxeTable;
 
   //
   // Do nothing when SEV is not enabled
@@ -284,6 +306,18 @@ AmdSevDxeEntryPoint (
     }
   }
 
+  Status = AllocateConfidentialComputingBlob (&SnpBootDxeTable);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: AllocateConfidentialComputingBlob(): %r\n",
+      __func__,
+      Status
+      ));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+
   if (MemEncryptSevSnpIsEnabled ()) {
     //
     // Memory acceptance began being required in SEV-SNP, so install the
@@ -321,7 +355,7 @@ AmdSevDxeEntryPoint (
     //
     return gBS->InstallConfigurationTable (
                   &gConfidentialComputingSevSnpBlobGuid,
-                  &mSnpBootDxeTable
+                  SnpBootDxeTable
                   );
   }
 
-- 
2.25.1

[PATCH v2 2/4] OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition

[Roth, Michael]

The Confidential Computing blob defined here is intended to match the
definition defined by linux guest kernel. Previously, both definitions
relied on natural alignment, but that relies on both OVMF and kernel
being compiled as 64-bit. While there aren't currently any plans to
enable SNP support for 32-bit compilations, the kernel definition has
since been updated to use explicit padding/reserved fields to avoid
this dependency. Update OVMF to match that definition.

While at it, also fix up the Reserved fields to match the numbering
used in the kernel.

No functional changes (for currently-supported environments, at least).

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Acked-by: Jiewen Yao <jiewen.yao@intel.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 OvmfPkg/AmdSevDxe/AmdSevDxe.c                          | 4 +++-
 OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index df807066fa..db3675ae86 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -48,11 +48,13 @@ AllocateConfidentialComputingBlob (
 
   CcBlob->Header                 = SIGNATURE_32 ('A', 'M', 'D', 'E');
   CcBlob->Version                = 1;
-  CcBlob->Reserved1              = 0;
+  CcBlob->Reserved               = 0;
   CcBlob->SecretsPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase);
   CcBlob->SecretsSize            = FixedPcdGet32 (PcdOvmfSnpSecretsSize);
+  CcBlob->Reserved1              = 0;
   CcBlob->CpuidPhysicalAddress   = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase);
   CcBlob->CpuidLSize             = FixedPcdGet32 (PcdOvmfCpuidSize);
+  CcBlob->Reserved2              = 0;
 
   *CcBlobPtr = CcBlob;
 
diff --git a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
index b328310fd0..83620e31b8 100644
--- a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
+++ b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
@@ -18,14 +18,16 @@
     { 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42 }, \
   }
 
-typedef struct {
+typedef PACKED struct {
   UINT32    Header;
   UINT16    Version;
-  UINT16    Reserved1;
+  UINT16    Reserved;
   UINT64    SecretsPhysicalAddress;
   UINT32    SecretsSize;
+  UINT32    Reserved1;
   UINT64    CpuidPhysicalAddress;
   UINT32    CpuidLSize;
+  UINT32    Reserved2;
 } CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION;
 
 extern EFI_GUID  gConfidentialComputingSevSnpBlobGuid;
-- 
2.25.1

[PATCH v2 3/4] OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation

[Roth, Michael]

CPUID leaf 0xD sub-leafs 0x0 and 0x1 contain cumulative sizes for the
enabled XSave areas. Those sizes are calculated by tallying up all the
other sub-leafs that contain per-area size information for XSave areas
that are currently enabled in XCr0/XSS. The current check has the logic
inverted. Fix that.

This doesn't seem to cause problems currently, but could in the future
if OVMF made more extensive use of XSave areas. It was noticed while
implementing SNP-related tests for KVM Unit Tests, which re-uses the
OVMF #VC handler in some cases.

Reported-by: Pavan Kumar Paluri <papaluri@amd.com>
Cc: Pavan Kumar Paluri <papaluri@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Acked-by: Jiewen Yao <jiewen.yao@intel.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
index 7fe11c5324..94f0c4872c 100644
--- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
+++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
@@ -1145,9 +1145,7 @@ GetCpuidXSaveSize (
   for (Idx = 0; Idx < CpuidInfo->Count; Idx++) {
     SEV_SNP_CPUID_FUNCTION  *CpuidFn = &CpuidInfo->function[Idx];
 
-    if (!((CpuidFn->EaxIn == 0xD) &&
-          ((CpuidFn->EcxIn == 0) || (CpuidFn->EcxIn == 1))))
-    {
+    if (!((CpuidFn->EaxIn == 0xD) && (CpuidFn->EcxIn > 1))) {
       continue;
     }
 
-- 
2.25.1

[PATCH v2 4/4] OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP

[Roth, Michael]

Currently OVMF tries to rely on the base size advertised via the CPUID
table entries corresponding to leaf 0xD, sub-leafs 0x0/0x1. This will
generally work for KVM guests, but might not for other SEV-SNP
hypervisor implementations. Make the handling more robust by simply
using the base area size documented by the APM.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Acked-by: Jiewen Yao <jiewen.yao@intel.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
index 94f0c4872c..0fc30f7bc4 100644
--- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
+++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c
@@ -1114,8 +1114,6 @@ SnpEnabled (
 
   @param[in]      XFeaturesEnabled  Bit-mask of enabled XSAVE features/areas as
                                     indicated by XCR0/MSR_IA32_XSS bits
-  @param[in]      XSaveBaseSize     Base/legacy XSAVE area size (e.g. when
-                                    XCR0 is 1)
   @param[in, out] XSaveSize         Pointer to storage for calculated XSAVE area
                                     size
   @param[in]      Compacted         Whether or not the calculation is for the
@@ -1130,7 +1128,6 @@ STATIC
 BOOLEAN
 GetCpuidXSaveSize (
   IN     UINT64   XFeaturesEnabled,
-  IN     UINT32   XSaveBaseSize,
   IN OUT UINT32   *XSaveSize,
   IN     BOOLEAN  Compacted
   )
@@ -1139,7 +1136,10 @@ GetCpuidXSaveSize (
   UINT64              XFeaturesFound = 0;
   UINT32              Idx;
 
-  *XSaveSize = XSaveBaseSize;
+  //
+  // The base/legacy XSave size is documented to be 0x240 in the APM.
+  //
+  *XSaveSize = 0x240;
   CpuidInfo  = (SEV_SNP_CPUID_INFO *)(UINT64)PcdGet32 (PcdOvmfCpuidBase);
 
   for (Idx = 0; Idx < CpuidInfo->Count; Idx++) {
@@ -1355,7 +1355,6 @@ GetCpuidFw (
 
     if (!GetCpuidXSaveSize (
            XCr0 | XssMsr.Uint64,
-           *Ebx,
            &XSaveSize,
            Compacted
            ))
-- 
2.25.1

Re: [PATCH v2 0/4] Fixes for SEV-SNP CC blob and CPUID table handling

[Yao, Jiewen]

Merged https://github.com/tianocore/edk2/pull/4313

> -----Original Message-----
> From: Michael Roth <michael.roth@amd.com>
> Sent: Wednesday, April 26, 2023 4:33 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Tom Lendacky
> <thomas.lendacky@amd.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Aktas,
> Erdem <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
> Xu, Min M <min.m.xu@intel.com>
> Subject: [PATCH v2 0/4] Fixes for SEV-SNP CC blob and CPUID table handling
> 
> (Mainly a resend of v1, but rolled in Gerd's Acked-by's, addressed
> new coding style check in the CI, and updated Cc list)
> 
> Here are a number of fixes related to OVMF handling of the SEV-SNP
> Confidential Computing blob and CPUID table.
> 
> Patch #1 is a fix for recently-reported issue that can cause
> significant problems with some SEV-SNP guest operating systems.
> Please consider applying this patch directly if the other
> patches in this series are held up for any reason.
> 
> Patches 2-4 are minor changes for things that aren't currently
> triggered in practice, but make OVMF's SEV-SNP implementation more
> robust for different build/hypervisor environments in the future.
> Patch #2 was submitted previously, but refreshed here to apply
> cleanly on top of Patch #1, with no other functional changes since
> the initial review.
> 
> v2:
>  - rebased/retested on latest master
>  - replaced usage of __FUNCTION__ with __func__ to comply with new CI
>    test cases
> 
> ----------------------------------------------------------------
> Michael Roth (4):
>       OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as
> EfiACPIReclaimMemory
>       OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct
> definition
>       OvmfPkg/CcExitLib: Fix SEV-SNP XSave area size calculation
>       OvmfPkg/CcExitLib: Use documented XSave area base size for SEV-SNP
> 
>  OvmfPkg/AmdSevDxe/AmdSevDxe.c                          | 64
> ++++++++++++++++++++++++++++++++++++++++++++++++++--------------
>  OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h |  6 ++++--
>  OvmfPkg/Library/CcExitLib/CcExitVcHandler.c            | 13 +++++--------
>  3 files changed, 59 insertions(+), 24 deletions(-)
>