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

Lendacky, Thomas via groups.io posted 2 patches 11 months, 3 weeks ago
There is a newer version of this series
[edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned
Posted by Lendacky, Thomas via groups.io 11 months, 3 weeks ago
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4353

Due to an erratum, an SEV-SNP VMSA cannot 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 | 24 +++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c
index bfda1e19030d..7abdda3e1c7e 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)  ((((UINTN)(x) & (y - 1)) == 0))
+
 /**
   Create an SEV-SNP AP save area (VMSA) for use in running the vCPU.
 
@@ -27,6 +29,7 @@ SevSnpCreateSaveArea (
   UINT32          ApicId
   )
 {
+  UINT8                     *Pages;
   SEV_ES_SAVE_AREA          *SaveArea;
   IA32_CR0                  ApCr0;
   IA32_CR0                  ResetCr0;
@@ -44,12 +47,29 @@ SevSnpCreateSaveArea (
 
   //
   // Allocate a single page for the SEV-ES Save Area and initialize it.
+  // Due to an erratum that prevents a VMSA being on a 2MB boundary,
+  // allocate an extra page 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);
+  }
+
   ZeroMem (SaveArea, EFI_PAGE_SIZE);
 
   //
-- 
2.39.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101008): https://edk2.groups.io/g/devel/message/101008
Mute This Topic: https://groups.io/mt/97524218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned
Posted by Ni, Ray 11 months, 2 weeks ago
> 
> +#define IS_ALIGNED(x, y)  ((((UINTN)(x) & (y - 1)) == 0))

1. Can you use the existing macro ALIGN_POINTER() defined in Base.h?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101059): https://edk2.groups.io/g/devel/message/101059
Mute This Topic: https://groups.io/mt/97524218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned
Posted by Lendacky, Thomas via groups.io 11 months, 2 weeks ago
On 3/13/23 03:28, Ni, Ray wrote:
>>
>> +#define IS_ALIGNED(x, y)  ((((UINTN)(x) & (y - 1)) == 0))
> 
> 1. Can you use the existing macro ALIGN_POINTER() defined in Base.h?

See my reply to the cover letter where I say I want to replace the usage 
with Gerd's definitions/updates series (but wanted general feedback now on 
the series).

Thanks,
Tom


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101093): https://edk2.groups.io/g/devel/message/101093
Mute This Topic: https://groups.io/mt/97524218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned
Posted by Gerd Hoffmann 11 months, 2 weeks ago
On Mon, Mar 13, 2023 at 08:28:57AM +0000, Ni, Ray wrote:
> > 
> > +#define IS_ALIGNED(x, y)  ((((UINTN)(x) & (y - 1)) == 0))
> 
> 1. Can you use the existing macro ALIGN_POINTER() defined in Base.h?

Having copies of this all over the tree is indeed a bad idea.

See https://edk2.groups.io/g/devel/message/100695 which adds this
and a few more commonly used macros to Base.h.  Can we get that
reviewed and merged please?

thanks & take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101086): https://edk2.groups.io/g/devel/message/101086
Mute This Topic: https://groups.io/mt/97524218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned
Posted by Ni, Ray 11 months, 2 weeks ago
That depends on review from other package maintainers.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Monday, March 13, 2023 4:46 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>; devel@edk2.groups.io;
> Dong, Eric <eric.dong@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 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-
> SNP VMSA allocations are not 2MB aligned
> 
> On Mon, Mar 13, 2023 at 08:28:57AM +0000, Ni, Ray wrote:
> > >
> > > +#define IS_ALIGNED(x, y)  ((((UINTN)(x) & (y - 1)) == 0))
> >
> > 1. Can you use the existing macro ALIGN_POINTER() defined in Base.h?
> 
> Having copies of this all over the tree is indeed a bad idea.
> 
> See https://edk2.groups.io/g/devel/message/100695 which adds this
> and a few more commonly used macros to Base.h.  Can we get that
> reviewed and merged please?
> 
> thanks & take care,
>   Gerd
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101087): https://edk2.groups.io/g/devel/message/101087
Mute This Topic: https://groups.io/mt/97524218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned
Posted by Gerd Hoffmann 11 months, 2 weeks ago
  Hi,

>    // Allocate a single page for the SEV-ES Save Area and initialize it.
> +  // Due to an erratum that prevents a VMSA being on a 2MB boundary,
> +  // allocate an extra page to work around the issue.

A reference to the erratum (web link or erratum id) would be nice here.
Also swapping the order of the two patches might simplify the other
patch which happens to shuffle around the code this patch has just
added.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101058): https://edk2.groups.io/g/devel/message/101058
Mute This Topic: https://groups.io/mt/97524218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned
Posted by Lendacky, Thomas via groups.io 11 months, 2 weeks ago
On 3/13/23 03:00, Gerd Hoffmann wrote:
>    Hi,
> 
>>     // Allocate a single page for the SEV-ES Save Area and initialize it.
>> +  // Due to an erratum that prevents a VMSA being on a 2MB boundary,
>> +  // allocate an extra page to work around the issue.
> 
> A reference to the erratum (web link or erratum id) would be nice here.
> Also swapping the order of the two patches might simplify the other
> patch which happens to shuffle around the code this patch has just
> added.

Will do.

Thanks,
Tom

> 
> take care,
>    Gerd
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101096): https://edk2.groups.io/g/devel/message/101096
Mute This Topic: https://groups.io/mt/97524218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned
Posted by Ni, Ray 11 months, 2 weeks ago
Agree with both comments😊

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Monday, March 13, 2023 4:00 PM
> To: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: devel@edk2.groups.io; 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: [PATCH 1/2] UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA
> allocations are not 2MB aligned
> 
>   Hi,
> 
> >    // Allocate a single page for the SEV-ES Save Area and initialize it.
> > +  // Due to an erratum that prevents a VMSA being on a 2MB boundary,
> > +  // allocate an extra page to work around the issue.
> 
> A reference to the erratum (web link or erratum id) would be nice here.
> Also swapping the order of the two patches might simplify the other
> patch which happens to shuffle around the code this patch has just
> added.
> 
> take care,
>   Gerd



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