[PATCH v2 0/2] SEV-SNP guest support fixes

[PATCH v2 0/2] SEV-SNP guest support fixes

[Lendacky, Thomas]

This patch series provides some fixes around AP creation:

- An erratum on AMD hardware requires that a VMSA not be aligned on a
  2MB boundary. To work around this issue, allocate 2 pages of memory
  and using the page that is not 2MB aligned and freeing the other.

- When parking APs after exiting boot services, the current SNP support
  will perform an allocation that will not be reflected in memory map
  being supplied to the OS. Instead of allocating new VMSAs each time,
  re-use the current VMSA.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4353

---

Changes since v1:
  - Change the order of the patches to make the patch diffs easier
    to review
  - Add erratum number to the patch description and code comment
  - Use existing ALIGN_POINTER macro

These patches are based on commit:
5eb3d1bcc16f ("ArmVirtPkg: can't find gUefiOvmfPkgTokenSpaceGuid")

Cc:	Eric Dong <eric.dong@intel.com>
Cc:	Ray Ni <ray.ni@intel.com>
Cc:	Rahul Kumar <rahul1.kumar@intel.com>
Cc:	Gerd Hoffmann <kraxel@redhat.com>
Cc:	Michael Roth <michael.roth@amd.com>
Cc:	Ashish Kalra <Ashish.Kalra@amd.com>

Tom Lendacky (2):
  UefiCpuPkg/MpInitLib: Reuse VMSA allocation to avoid unreserved
    allocation
  UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB
    aligned

 UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c | 224 ++++++++++++++--------
 1 file changed, 144 insertions(+), 80 deletions(-)

-- 
2.40.0

[PATCH v2 1/2] UefiCpuPkg/MpInitLib: Reuse VMSA allocation to avoid unreserved allocation

[Lendacky, Thomas]

https://bugzilla.tianocore.org/show_bug.cgi?id=4353

When parking the APs on exiting from UEFI, a new page allocation is made.
This allocation, however, does not end up being marked reserved in the
memory map supplied to the OS. To avoid this, re-use the VMSA by clearing
the VMSA RMP flag, updating the page contents and re-setting the VMSA RMP
flag.

Fixes: 06544455d0d4 ("UefiCpuPkg/MpInitLib: Use SEV-SNP AP Creation ...")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c | 204 +++++++++++++---------
 1 file changed, 124 insertions(+), 80 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
index bfda1e19030d..509be9b41757 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
+++ b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
@@ -14,40 +14,140 @@
 #include <Register/Amd/Ghcb.h>
 
 /**
-  Create an SEV-SNP AP save area (VMSA) for use in running the vCPU.
+  Perform the requested AP Creation action.
 
-  @param[in]  CpuMpData        Pointer to CPU MP Data
-  @param[in]  CpuData          Pointer to CPU AP Data
+  @param[in]  SaveArea         Pointer to VM save area (VMSA)
   @param[in]  ApicId           APIC ID of the vCPU
+  @param[in]  Action           AP action to perform
+
+  @retval  TRUE   Action completed successfully
+  @retval  FALSE  Action did not complete successfully
 **/
-VOID
-SevSnpCreateSaveArea (
-  IN CPU_MP_DATA  *CpuMpData,
-  IN CPU_AP_DATA  *CpuData,
-  UINT32          ApicId
+STATIC
+BOOLEAN
+SevSnpPerformApAction (
+  IN SEV_ES_SAVE_AREA  *SaveArea,
+  IN UINT32            ApicId,
+  IN UINTN             Action
   )
 {
-  SEV_ES_SAVE_AREA          *SaveArea;
-  IA32_CR0                  ApCr0;
-  IA32_CR0                  ResetCr0;
-  IA32_CR4                  ApCr4;
-  IA32_CR4                  ResetCr4;
-  UINTN                     StartIp;
-  UINT8                     SipiVector;
-  UINT32                    RmpAdjustStatus;
-  UINT64                    VmgExitStatus;
   MSR_SEV_ES_GHCB_REGISTER  Msr;
   GHCB                      *Ghcb;
   BOOLEAN                   InterruptState;
   UINT64                    ExitInfo1;
   UINT64                    ExitInfo2;
+  UINT32                    RmpAdjustStatus;
+  UINT64                    VmgExitStatus;
 
-  //
-  // Allocate a single page for the SEV-ES Save Area and initialize it.
-  //
-  SaveArea = AllocateReservedPages (1);
-  if (!SaveArea) {
-    return;
+  if (Action == SVM_VMGEXIT_SNP_AP_CREATE) {
+    //
+    // To turn the page into a recognized VMSA page, issue RMPADJUST:
+    //   Target VMPL but numerically higher than current VMPL
+    //   Target PermissionMask is not used
+    //
+    RmpAdjustStatus = SevSnpRmpAdjust (
+                        (EFI_PHYSICAL_ADDRESS)(UINTN)SaveArea,
+                        TRUE
+                        );
+    if (RmpAdjustStatus != 0) {
+      DEBUG ((DEBUG_INFO, "SEV-SNP: RMPADJUST failed for VMSA creation\n"));
+      ASSERT (FALSE);
+
+      return FALSE;
+    }
+  }
+
+  ExitInfo1  = (UINT64)ApicId << 32;
+  ExitInfo1 |= Action;
+  ExitInfo2  = (UINT64)(UINTN)SaveArea;
+
+  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
+  Ghcb                    = Msr.Ghcb;
+
+  CcExitVmgInit (Ghcb, &InterruptState);
+
+  if (Action == SVM_VMGEXIT_SNP_AP_CREATE) {
+    Ghcb->SaveArea.Rax = SaveArea->SevFeatures;
+    CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
+  }
+
+  VmgExitStatus = CcExitVmgExit (
+                    Ghcb,
+                    SVM_EXIT_SNP_AP_CREATION,
+                    ExitInfo1,
+                    ExitInfo2
+                    );
+
+  CcExitVmgDone (Ghcb, InterruptState);
+
+  if (VmgExitStatus != 0) {
+    DEBUG ((DEBUG_INFO, "SEV-SNP: AP Destroy failed\n"));
+    ASSERT (FALSE);
+
+    return FALSE;
+  }
+
+  if (Action == SVM_VMGEXIT_SNP_AP_DESTROY) {
+    //
+    // Make the current VMSA not runnable and accessible to be
+    // reprogrammed.
+    //
+    RmpAdjustStatus = SevSnpRmpAdjust (
+                        (EFI_PHYSICAL_ADDRESS)(UINTN)SaveArea,
+                        FALSE
+                        );
+    if (RmpAdjustStatus != 0) {
+      DEBUG ((DEBUG_INFO, "SEV-SNP: RMPADJUST failed for VMSA reset\n"));
+      ASSERT (FALSE);
+
+      return FALSE;
+    }
+  }
+
+  return TRUE;
+}
+
+/**
+  Create an SEV-SNP AP save area (VMSA) for use in running the vCPU.
+
+  @param[in]  CpuMpData        Pointer to CPU MP Data
+  @param[in]  CpuData          Pointer to CPU AP Data
+  @param[in]  ApicId           APIC ID of the vCPU
+**/
+VOID
+SevSnpCreateSaveArea (
+  IN CPU_MP_DATA  *CpuMpData,
+  IN CPU_AP_DATA  *CpuData,
+  UINT32          ApicId
+  )
+{
+  SEV_ES_SAVE_AREA  *SaveArea;
+  IA32_CR0          ApCr0;
+  IA32_CR0          ResetCr0;
+  IA32_CR4          ApCr4;
+  IA32_CR4          ResetCr4;
+  UINTN             StartIp;
+  UINT8             SipiVector;
+
+  if (CpuData->SevEsSaveArea == NULL) {
+    //
+    // Allocate a single page for the SEV-ES Save Area and initialize it.
+    //
+    SaveArea = AllocateReservedPages (1);
+    if (!SaveArea) {
+      return;
+    }
+
+    CpuData->SevEsSaveArea = SaveArea;
+  } else {
+    SaveArea = CpuData->SevEsSaveArea;
+
+    //
+    // Tell the hypervisor to not use the current VMSA
+    //
+    if (!SevSnpPerformApAction (SaveArea, ApicId, SVM_VMGEXIT_SNP_AP_DESTROY)) {
+      return;
+    }
   }
 
   ZeroMem (SaveArea, EFI_PAGE_SIZE);
@@ -132,63 +232,7 @@ SevSnpCreateSaveArea (
   SaveArea->Vmpl        = 0;
   SaveArea->SevFeatures = AsmReadMsr64 (MSR_SEV_STATUS) >> 2;
 
-  //
-  // To turn the page into a recognized VMSA page, issue RMPADJUST:
-  //   Target VMPL but numerically higher than current VMPL
-  //   Target PermissionMask is not used
-  //
-  RmpAdjustStatus = SevSnpRmpAdjust (
-                      (EFI_PHYSICAL_ADDRESS)(UINTN)SaveArea,
-                      TRUE
-                      );
-  ASSERT (RmpAdjustStatus == 0);
-
-  ExitInfo1  = (UINT64)ApicId << 32;
-  ExitInfo1 |= SVM_VMGEXIT_SNP_AP_CREATE;
-  ExitInfo2  = (UINT64)(UINTN)SaveArea;
-
-  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
-  Ghcb                    = Msr.Ghcb;
-
-  CcExitVmgInit (Ghcb, &InterruptState);
-  Ghcb->SaveArea.Rax = SaveArea->SevFeatures;
-  CcExitVmgSetOffsetValid (Ghcb, GhcbRax);
-  VmgExitStatus = CcExitVmgExit (
-                    Ghcb,
-                    SVM_EXIT_SNP_AP_CREATION,
-                    ExitInfo1,
-                    ExitInfo2
-                    );
-  CcExitVmgDone (Ghcb, InterruptState);
-
-  ASSERT (VmgExitStatus == 0);
-  if (VmgExitStatus != 0) {
-    RmpAdjustStatus = SevSnpRmpAdjust (
-                        (EFI_PHYSICAL_ADDRESS)(UINTN)SaveArea,
-                        FALSE
-                        );
-    if (RmpAdjustStatus == 0) {
-      FreePages (SaveArea, 1);
-    } else {
-      DEBUG ((DEBUG_INFO, "SEV-SNP: RMPADJUST failed, leaking VMSA page\n"));
-    }
-
-    SaveArea = NULL;
-  }
-
-  if (CpuData->SevEsSaveArea) {
-    RmpAdjustStatus = SevSnpRmpAdjust (
-                        (EFI_PHYSICAL_ADDRESS)(UINTN)CpuData->SevEsSaveArea,
-                        FALSE
-                        );
-    if (RmpAdjustStatus == 0) {
-      FreePages (CpuData->SevEsSaveArea, 1);
-    } else {
-      DEBUG ((DEBUG_INFO, "SEV-SNP: RMPADJUST failed, leaking VMSA page\n"));
-    }
-  }
-
-  CpuData->SevEsSaveArea = SaveArea;
+  SevSnpPerformApAction (SaveArea, ApicId, SVM_VMGEXIT_SNP_AP_CREATE);
 }
 
 /**
-- 
2.40.0

[PATCH v2 2/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned

[Lendacky, Thomas]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4353

Due to AMD erratum #1467, an SEV-SNP VMSA should not be 2MB aligned. To
work around this issue, allocate two pages instead of one. Because of the
way that page allocation is implemented, always try to use the second
page. If the second page is not 2MB aligned, free the first page and use
the second page. If the second page is 2MB aligned, free the second page
and use the first page. Freeing in this way reduces holes in the memory
map.

Fixes: 06544455d0d4 ("UefiCpuPkg/MpInitLib: Use SEV-SNP AP Creation ...")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c | 26 ++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
index 509be9b41757..c9f0984f41a2 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
+++ b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
@@ -13,6 +13,8 @@
 #include <Register/Amd/Fam17Msr.h>
 #include <Register/Amd/Ghcb.h>
 
+#define _IS_ALIGNED(x, y)  (ALIGN_POINTER((x), (y)) == (x))
+
 /**
   Perform the requested AP Creation action.
 
@@ -121,6 +123,7 @@ SevSnpCreateSaveArea (
   UINT32          ApicId
   )
 {
+  UINT8             *Pages;
   SEV_ES_SAVE_AREA  *SaveArea;
   IA32_CR0          ApCr0;
   IA32_CR0          ResetCr0;
@@ -131,13 +134,30 @@ SevSnpCreateSaveArea (
 
   if (CpuData->SevEsSaveArea == NULL) {
     //
-    // Allocate a single page for the SEV-ES Save Area and initialize it.
+    // Allocate a page for the SEV-ES Save Area and initialize it. Due to AMD
+    // erratum #1467 (VMSA cannot be on a 2MB boundary), allocate an extra page
+    // to choose from to work around the issue.
     //
-    SaveArea = AllocateReservedPages (1);
-    if (!SaveArea) {
+    Pages = AllocateReservedPages (2);
+    if (!Pages) {
       return;
     }
 
+    //
+    // Since page allocation works by allocating downward in the address space,
+    // try to always free the first (lower address) page to limit possible holes
+    // in the memory map. So, if the address of the second page is 2MB aligned,
+    // then use the first page and free the second page. Otherwise, free the
+    // first page and use the second page.
+    //
+    if (_IS_ALIGNED (Pages + EFI_PAGE_SIZE, SIZE_2MB)) {
+      SaveArea = (SEV_ES_SAVE_AREA *)Pages;
+      FreePages (Pages + EFI_PAGE_SIZE, 1);
+    } else {
+      SaveArea = (SEV_ES_SAVE_AREA *)(Pages + EFI_PAGE_SIZE);
+      FreePages (Pages, 1);
+    }
+
     CpuData->SevEsSaveArea = SaveArea;
   } else {
     SaveArea = CpuData->SevEsSaveArea;
-- 
2.40.0

Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned

[Lendacky, Thomas]

On 3/28/23 13:09, Lendacky, Thomas via groups.io wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4353
> 
> Due to AMD erratum #1467, an SEV-SNP VMSA should not be 2MB aligned. To
> work around this issue, allocate two pages instead of one. Because of the
> way that page allocation is implemented, always try to use the second
> page. If the second page is not 2MB aligned, free the first page and use
> the second page. If the second page is 2MB aligned, free the second page
> and use the first page. Freeing in this way reduces holes in the memory
> map.
> 
> Fixes: 06544455d0d4 ("UefiCpuPkg/MpInitLib: Use SEV-SNP AP Creation ...")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>   UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c | 26 ++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
> index 509be9b41757..c9f0984f41a2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
> @@ -13,6 +13,8 @@
>   #include <Register/Amd/Fam17Msr.h>
>   #include <Register/Amd/Ghcb.h>
>   
> +#define _IS_ALIGNED(x, y)  (ALIGN_POINTER((x), (y)) == (x))

I haven't seen any movement on Gerd's series to create common macros for 
alignment checks, but can't wait forever. So here I use an underscore in 
front of IS_ALIGNED() so that there won't be a conflict either before or 
after Gerd's series is applied. Once Gerd's series is applied, I can go 
back and submit another patch to use the new macros.

Thanks,
Tom

> +
>   /**
>     Perform the requested AP Creation action.
>   
> @@ -121,6 +123,7 @@ SevSnpCreateSaveArea (
>     UINT32          ApicId
>     )
>   {
> +  UINT8             *Pages;
>     SEV_ES_SAVE_AREA  *SaveArea;
>     IA32_CR0          ApCr0;
>     IA32_CR0          ResetCr0;
> @@ -131,13 +134,30 @@ SevSnpCreateSaveArea (
>   
>     if (CpuData->SevEsSaveArea == NULL) {
>       //
> -    // Allocate a single page for the SEV-ES Save Area and initialize it.
> +    // Allocate a page for the SEV-ES Save Area and initialize it. Due to AMD
> +    // erratum #1467 (VMSA cannot be on a 2MB boundary), allocate an extra page
> +    // to choose from to work around the issue.
>       //
> -    SaveArea = AllocateReservedPages (1);
> -    if (!SaveArea) {
> +    Pages = AllocateReservedPages (2);
> +    if (!Pages) {
>         return;
>       }
>   
> +    //
> +    // Since page allocation works by allocating downward in the address space,
> +    // try to always free the first (lower address) page to limit possible holes
> +    // in the memory map. So, if the address of the second page is 2MB aligned,
> +    // then use the first page and free the second page. Otherwise, free the
> +    // first page and use the second page.
> +    //
> +    if (_IS_ALIGNED (Pages + EFI_PAGE_SIZE, SIZE_2MB)) {
> +      SaveArea = (SEV_ES_SAVE_AREA *)Pages;
> +      FreePages (Pages + EFI_PAGE_SIZE, 1);
> +    } else {
> +      SaveArea = (SEV_ES_SAVE_AREA *)(Pages + EFI_PAGE_SIZE);
> +      FreePages (Pages, 1);
> +    }
> +
>       CpuData->SevEsSaveArea = SaveArea;
>     } else {
>       SaveArea = CpuData->SevEsSaveArea;

Re: [edk2-devel] [PATCH v2 0/2] SEV-SNP guest support fixes

[Gerd Hoffmann]

On Tue, Mar 28, 2023 at 01:09:22PM -0500, Lendacky, Thomas via groups.io wrote:
> This patch series provides some fixes around AP creation:
> 
> - An erratum on AMD hardware requires that a VMSA not be aligned on a
>   2MB boundary. To work around this issue, allocate 2 pages of memory
>   and using the page that is not 2MB aligned and freeing the other.
> 
> - When parking APs after exiting boot services, the current SNP support
>   will perform an allocation that will not be reflected in memory map
>   being supplied to the OS. Instead of allocating new VMSAs each time,
>   re-use the current VMSA.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4353
> 
> ---
> 
> Changes since v1:
>   - Change the order of the patches to make the patch diffs easier
>     to review
>   - Add erratum number to the patch description and code comment
>   - Use existing ALIGN_POINTER macro

Series:
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [edk2-devel] [PATCH v2 0/2] SEV-SNP guest support fixes

[Ni, Ray]

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

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, March 30, 2023 3:36 PM
> To: devel@edk2.groups.io; thomas.lendacky@amd.com
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Michael
> Roth <michael.roth@amd.com>; Ashish Kalra <Ashish.Kalra@amd.com>
> Subject: Re: [edk2-devel] [PATCH v2 0/2] SEV-SNP guest support fixes
> 
> On Tue, Mar 28, 2023 at 01:09:22PM -0500, Lendacky, Thomas via groups.io wrote:
> > This patch series provides some fixes around AP creation:
> >
> > - An erratum on AMD hardware requires that a VMSA not be aligned on a
> >   2MB boundary. To work around this issue, allocate 2 pages of memory
> >   and using the page that is not 2MB aligned and freeing the other.
> >
> > - When parking APs after exiting boot services, the current SNP support
> >   will perform an allocation that will not be reflected in memory map
> >   being supplied to the OS. Instead of allocating new VMSAs each time,
> >   re-use the current VMSA.
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4353
> >
> > ---
> >
> > Changes since v1:
> >   - Change the order of the patches to make the patch diffs easier
> >     to review
> >   - Add erratum number to the patch description and code comment
> >   - Use existing ALIGN_POINTER macro
> 
> Series:
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> take care,
>   Gerd