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]
-=-=-=-=-=-=-=-=-=-=-=-> > +#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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
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] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.