.../X64/PeiDxeVirtualMemory.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)
Edk2 was failing, rather than creating more PML4 entries, when they
weren't present in the initial memory acceptance flow. Because of that
VMs with more than 512G memory were crashing. This code fixes that.
This change affects only SEV-SNP VMs.
The code was tested by successfully booting a 512G SEV-SNP VM.
Signed-off-by: Mikolaj Lisik <lisik@google.com>
---
.../X64/PeiDxeVirtualMemory.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index b9c0a5b25a..3dbff51ac2 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -548,6 +548,7 @@ InternalMemEncryptSevCreateIdentityMap1G (
PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
PAGE_TABLE_1G_ENTRY *PageDirectory1GEntry;
UINT64 PgTableMask;
+ UINT64 *NewPageTable;
UINT64 AddressEncMask;
BOOLEAN IsWpEnabled;
RETURN_STATUS Status;
@@ -602,15 +603,13 @@ InternalMemEncryptSevCreateIdentityMap1G (
PageMapLevel4Entry = (VOID *)(Cr3BaseAddress & ~PgTableMask);
PageMapLevel4Entry += PML4_OFFSET (PhysicalAddress);
if (!PageMapLevel4Entry->Bits.Present) {
- DEBUG ((
- DEBUG_ERROR,
- "%a:%a: bad PML4 for Physical=0x%Lx\n",
- gEfiCallerBaseName,
- __FUNCTION__,
- PhysicalAddress
- ));
- Status = RETURN_NO_MAPPING;
- goto Done;
+ NewPageTable = AllocatePageTableMemory(1);
+ ASSERT(NewPageTable != NULL);
+ SetMem (NewPageTable, EFI_PAGE_SIZE, 0);
+ PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)NewPageTable | AddressEncMask;
+ PageMapLevel4Entry->Bits.Present = 1;
+ PageMapLevel4Entry->Bits.MustBeZero = 0;
+ PageMapLevel4Entry->Bits.ReadWrite = 1;
}
PageDirectory1GEntry = (VOID *)(
@@ -640,7 +639,6 @@ InternalMemEncryptSevCreateIdentityMap1G (
//
CpuFlushTlb ();
-Done:
//
// Restore page table write protection, if any.
//
--
2.39.0.314.g84b9a713c41-goog
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98493): https://edk2.groups.io/g/devel/message/98493
Mute This Topic: https://groups.io/mt/96250316/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-On Fri, Jan 13, 2023 at 5:02 PM Mikolaj Lisik via groups.io
<lisik=google.com@groups.io> wrote:
>
> Edk2 was failing, rather than creating more PML4 entries, when they
> weren't present in the initial memory acceptance flow. Because of that
> VMs with more than 512G memory were crashing. This code fixes that.
>
> This change affects only SEV-SNP VMs.
>
> The code was tested by successfully booting a 512G SEV-SNP VM.
>
> Signed-off-by: Mikolaj Lisik <lisik@google.com>
> ---
> .../X64/PeiDxeVirtualMemory.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> index b9c0a5b25a..3dbff51ac2 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -548,6 +548,7 @@ InternalMemEncryptSevCreateIdentityMap1G (
> PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
> PAGE_TABLE_1G_ENTRY *PageDirectory1GEntry;
> UINT64 PgTableMask;
> + UINT64 *NewPageTable;
> UINT64 AddressEncMask;
> BOOLEAN IsWpEnabled;
> RETURN_STATUS Status;
> @@ -602,15 +603,13 @@ InternalMemEncryptSevCreateIdentityMap1G (
> PageMapLevel4Entry = (VOID *)(Cr3BaseAddress & ~PgTableMask);
> PageMapLevel4Entry += PML4_OFFSET (PhysicalAddress);
> if (!PageMapLevel4Entry->Bits.Present) {
> - DEBUG ((
> - DEBUG_ERROR,
> - "%a:%a: bad PML4 for Physical=0x%Lx\n",
> - gEfiCallerBaseName,
> - __FUNCTION__,
> - PhysicalAddress
> - ));
> - Status = RETURN_NO_MAPPING;
> - goto Done;
> + NewPageTable = AllocatePageTableMemory(1);
> + ASSERT(NewPageTable != NULL);
Hi,
(+cc OVMF SEV maintainers)
This should not use an ASSERT as those can get (purposefully) deleted
on release builds. Please do proper error handling like in the code
block you deleted.
> + SetMem (NewPageTable, EFI_PAGE_SIZE, 0);
> + PageMapLevel4Entry->Uint64 = (UINT64)(UINTN)NewPageTable | AddressEncMask;
> + PageMapLevel4Entry->Bits.Present = 1;
> + PageMapLevel4Entry->Bits.MustBeZero = 0;
> + PageMapLevel4Entry->Bits.ReadWrite = 1;
Please swap the Present and ReadWrite lines. You should only set
Present after everything else as to avoid explicit caching (TLB and
paging structure) issues.
Although this whole file is full of spotty behavior. ASSERTS can get
deleted and the file is full of them.
There are plenty of Present = 1 sets before setting other important
bits like RW, which *will* cause you to get bad TLB entries if the CPU
speculates a load/store to that address.
Unions don't define how they write back data; meaning that they can
write everything on one go, or write the u64 and then set bits
individually, or write the u64 and then set the bits all at once.
Bit fields' layouts are not specified in the standard and because of
that struct S{int bit : 1; int bit2 : 1;}; doesn't guarantee bit and
bit2 are contiguous.
Union type punning is also undefined behavior in standard C (it is not
in GNU C, no idea about MSVC).
If everything was done correctly, there would be no need for the hacky
CpuFlushTlb (); down there, as this file adds PTEs, and doesn't modify
them.
(now that I check, all of this insane behavior seems to have been
inherited from MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c. Sorry
for the rant.)
*sigh*
In any case, because of that CpuFlushTlb () your patch isn't wrong,
but it should be changed into something more MMU-code natural.
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98509): https://edk2.groups.io/g/devel/message/98509
Mute This Topic: https://groups.io/mt/96250316/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Thanks for the review! > Please swap the Present and ReadWrite lines. You should only set > Present after everything else as to avoid explicit caching (TLB and > paging structure) issues. > > Although this whole file is full of spotty behavior. ASSERTS can get > deleted and the file is full of them. Yes, keeping consistency with the rest of the file was my main motivation for doing things this way. Will add popper error handling bere in V2. > There are plenty of Present = 1 sets before setting other important > bits like RW, which *will* cause you to get bad TLB entries if the CPU > speculates a load/store to that address. ... > *sigh* > In any case, because of that CpuFlushTlb () your patch isn't wrong, > but it should be changed into something more MMU-code natural. Makes sense! Thanks for all this information. Yes, I simply wanted to order setting of the bits exactly as they have been done before in the file. Will adjust in V2 and send a second patch adjusting them for other instances. Mikolaj -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98989): https://edk2.groups.io/g/devel/message/98989 Mute This Topic: https://groups.io/mt/96250316/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi, > (now that I check, all of this insane behavior seems to have been > inherited from MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c. Sorry > for the rant.) We have UefiCpuPkg/Library/CpuPageTableLib meanwhile. So switch the code over to use that instead, and reduce the number of VirtualMemory.c copies floating around in the tree? take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98566): https://edk2.groups.io/g/devel/message/98566 Mute This Topic: https://groups.io/mt/96250316/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi, > > (now that I check, all of this insane behavior seems to have been > > inherited from MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c. Sorry > > for the rant.) > > We have UefiCpuPkg/Library/CpuPageTableLib meanwhile. > > So switch the code over to use that instead, and reduce the number > of VirtualMemory.c copies floating around in the tree? As much sense as this makes to me I would stick to using the structure for this patch. It's simply a bug fix to an existing code path. I'm of course open to the discussion of making larger changes in the codebase in a separate series. Thanks! Mikolaj -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98990): https://edk2.groups.io/g/devel/message/98990 Mute This Topic: https://groups.io/mt/96250316/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.