[edk2-devel] [PATCH 0/3] OvmfPkg/PlatformPei: scaleability fixes for GetPeiMemoryCap()

[edk2-devel] [PATCH 0/3] OvmfPkg/PlatformPei: scaleability fixes for GetPeiMemoryCap()

[Gerd Hoffmann]

Gerd Hoffmann (3):
  OvmfPkg/PlatformPei: consider AP stacks for pei memory cap
  OvmfPkg/PlatformPei: rewrite page table calculation
  OvmfPkg/PlatformPei: log pei memory cap details

 OvmfPkg/PlatformPei/MemDetect.c | 76 ++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 20 deletions(-)

-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114882): https://edk2.groups.io/g/devel/message/114882
Mute This Topic: https://groups.io/mt/104073298/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/3] OvmfPkg/PlatformPei: consider AP stacks for pei memory cap

[Gerd Hoffmann]

Needed to avoid running out of memory when booting
with a large (~2048) number of vcpus.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/PlatformPei/MemDetect.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 493cb1fbeb01..338798b54171 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -187,6 +187,8 @@ GetPeiMemoryCap (
   UINT32   Pml4Entries;
   UINT32   PdpEntries;
   UINTN    TotalPages;
+  UINTN    ApStacks;
+  UINTN    MemoryCap;
 
   //
   // If DXE is 32-bit, then just return the traditional 64 MB cap.
@@ -234,12 +236,18 @@ GetPeiMemoryCap (
                (PdpEntries + 1) * Pml4Entries + 1;
   ASSERT (TotalPages <= 0x40201);
 
+  ApStacks  = PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber * PcdGet32 (PcdCpuApStackSize);
+  MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;
+  if (MemoryCap > MAX_UINT32) {
+    MemoryCap = MAX_UINT32;
+  }
+
   //
   // Add 64 MB for miscellaneous allocations. Note that for
   // PhysMemAddressWidth values close to 36, the cap will actually be
   // dominated by this increment.
   //
-  return (UINT32)(EFI_PAGES_TO_SIZE (TotalPages) + SIZE_64MB);
+  return (UINT32)(MemoryCap);
 }
 
 /**
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114881): https://edk2.groups.io/g/devel/message/114881
Mute This Topic: https://groups.io/mt/104073297/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/3] OvmfPkg/PlatformPei: rewrite page table calculation

[Gerd Hoffmann]

Consider 5-level paging.  Simplify calculation to make it easier to
understand.  The new calculation is not 100% exact, but we only need
a rough estimate to reserve enough memory.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/PlatformPei/MemDetect.c | 42 ++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 338798b54171..e22743b4bfaa 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -184,8 +184,10 @@ GetPeiMemoryCap (
   BOOLEAN  Page1GSupport;
   UINT32   RegEax;
   UINT32   RegEdx;
-  UINT32   Pml4Entries;
-  UINT32   PdpEntries;
+  UINT32   Level5Pages;
+  UINT32   Level4Pages;
+  UINT32   Level3Pages;
+  UINT32   Level2Pages;
   UINTN    TotalPages;
   UINTN    ApStacks;
   UINTN    MemoryCap;
@@ -203,8 +205,7 @@ GetPeiMemoryCap (
   //
   // Dependent on physical address width, PEI memory allocations can be
   // dominated by the page tables built for 64-bit DXE. So we key the cap off
-  // of those. The code below is based on CreateIdentityMappingPageTables() in
-  // "MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c".
+  // of those.
   //
   Page1GSupport = FALSE;
   if (PcdGetBool (PcdUse1GPageTable)) {
@@ -217,23 +218,26 @@ GetPeiMemoryCap (
     }
   }
 
-  if (PlatformInfoHob->PhysMemAddressWidth <= 39) {
-    Pml4Entries = 1;
-    PdpEntries  = 1 << (PlatformInfoHob->PhysMemAddressWidth - 30);
-    ASSERT (PdpEntries <= 0x200);
-  } else {
-    if (PlatformInfoHob->PhysMemAddressWidth > 48) {
-      Pml4Entries = 0x200;
-    } else {
-      Pml4Entries = 1 << (PlatformInfoHob->PhysMemAddressWidth - 39);
-    }
-
-    ASSERT (Pml4Entries <= 0x200);
-    PdpEntries = 512;
+  //
+  // This calculation doesn't return the *exact* number of page table
+  // pages needed.  But we only need a rough estimate to reserve
+  // enough memory, being off by a few pages isn't much of problem.
+  // So keep the calculation simple and easy to understand.
+  //
+  // Page size is 4k, which needs 12 address bits.
+  // Page tables on all levels have 512 entries, mapping to 9 address bits.
+  // We use large pages (2M or 1G), so level 1 never exists.
+  // Start with level 2, where a page covers 12 + 9 + 9 = 30 address bits.
+  //
+  Level2Pages = (1 << (PlatformInfoHob->PhysMemAddressWidth - 30)) + 1;
+  Level3Pages = (Level2Pages >> 9) + 1;
+  Level4Pages = (Level3Pages >> 9) + 1;
+  Level5Pages = (Level4Pages >> 9) + 1;
+  if (Page1GSupport) {
+    Level2Pages = 0;
   }
 
-  TotalPages = Page1GSupport ? Pml4Entries + 1 :
-               (PdpEntries + 1) * Pml4Entries + 1;
+  TotalPages = Level5Pages + Level4Pages + Level3Pages + Level2Pages;
   ASSERT (TotalPages <= 0x40201);
 
   ApStacks  = PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber * PcdGet32 (PcdCpuApStackSize);
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114884): https://edk2.groups.io/g/devel/message/114884
Mute This Topic: https://groups.io/mt/104073300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: log pei memory cap details

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/PlatformPei/MemDetect.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index e22743b4bfaa..988fcc512362 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -246,6 +246,30 @@ GetPeiMemoryCap (
     MemoryCap = MAX_UINT32;
   }
 
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: page tables: %6lld kb (%d/%d/%d/%d)\n",
+    __func__,
+    (UINT64)(EFI_PAGES_TO_SIZE (TotalPages) / 1024),
+    Level5Pages,
+    Level4Pages,
+    Level3Pages,
+    Level2Pages
+    ));
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: ap stacks:   %6lld kb (%d cpus)\n",
+    __func__,
+    (UINT64)(ApStacks / 1024),
+    PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber
+    ));
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: memory cap:  %6lld kb\n",
+    __func__,
+    (UINT64)(MemoryCap / 1024)
+    ));
+
   //
   // Add 64 MB for miscellaneous allocations. Note that for
   // PhysMemAddressWidth values close to 36, the cap will actually be
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114883): https://edk2.groups.io/g/devel/message/114883
Mute This Topic: https://groups.io/mt/104073299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformPei: consider AP stacks for pei memory cap

[Laszlo Ersek]

On 1/31/24 12:59, Gerd Hoffmann wrote:
> Needed to avoid running out of memory when booting
> with a large (~2048) number of vcpus.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

This idea seems justified; it resembles commit 45d870815156 ("OvmfPkg/PlatformPei: rebase and resize the permanent PEI memory for S3", 2016-07-15), which also operates with the (PcdCpuMaxLogicalProcessorNumber * PcdCpuApStackSize) product. OK.

> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 493cb1fbeb01..338798b54171 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -187,6 +187,8 @@ GetPeiMemoryCap (
>    UINT32   Pml4Entries;
>    UINT32   PdpEntries;
>    UINTN    TotalPages;
> +  UINTN    ApStacks;
> +  UINTN    MemoryCap;
>  
>    //
>    // If DXE is 32-bit, then just return the traditional 64 MB cap.
> @@ -234,12 +236,18 @@ GetPeiMemoryCap (
>                 (PdpEntries + 1) * Pml4Entries + 1;
>    ASSERT (TotalPages <= 0x40201);
>  
> +  ApStacks  = PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber * PcdGet32 (PcdCpuApStackSize);

Doing this here is safe; we have the following call tree:

  InitializePlatform()                    [OvmfPkg/PlatformPei/Platform.c]
    MaxCpuCountInitialization()           [OvmfPkg/PlatformPei/Platform.c]
      PlatformMaxCpuCountInitialization() [OvmfPkg/Library/PlatformInitLib/Platform.c]
    PublishPeiMemory()                    [OvmfPkg/PlatformPei/MemDetect.c]
      GetPeiMemoryCap()                   [OvmfPkg/PlatformPei/MemDetect.c]

Meaning that "PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber" will be set here. OK.

> +  MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;

(1) The comment at the bottom is now both misplaced, and out of date.  First, we perform the addition here, not down there. Second, we now have an extra addend, which may even dominate the cap.

> +  if (MemoryCap > MAX_UINT32) {
> +    MemoryCap = MAX_UINT32;
> +  }
> +

(2) This doesn't look good, for two reasons.

First, if UINTN is UINT32, then we're too late to check. (If it's intentional that the code be dead on IA32, then that should be documented.)

Second, while returning MAX_UINT32 from this function is safe, it's also obscure. After the call site, we've got

    MemoryBase = PlatformInfoHob->S3Supported && PlatformInfoHob->SmmSmramRequire ?
                 PcdGet32 (PcdOvmfDecompressionScratchEnd) :
                 PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32 (PcdOvmfDxeMemFvSize);
    MemorySize = LowerMemorySize - MemoryBase;
    if (MemorySize > PeiMemoryCap) {
      MemoryBase = LowerMemorySize - PeiMemoryCap;
      MemorySize = PeiMemoryCap;
    }

If PeiMemoryCap is huge (e.g., it approximates 4GB), then (MemorySize > PeiMemoryCap) will evaluate to FALSE, and we'll just give most of the low RAM to PEI. This effectively means that permanent PEI RAM is "uncapped" (while remaining 32-bit only, of course). Whether that will cause a boot failure later on, or not, remains to be seen, but as far as this code is affected, it's not a problem if GetPeiMemoryCap() returns MAX_UINT32. *However*, this argument should be captured in a comment.

>    //
>    // Add 64 MB for miscellaneous allocations. Note that for
>    // PhysMemAddressWidth values close to 36, the cap will actually be
>    // dominated by this increment.
>    //
> -  return (UINT32)(EFI_PAGES_TO_SIZE (TotalPages) + SIZE_64MB);
> +  return (UINT32)(MemoryCap);
>  }
>  
>  /**

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114902): https://edk2.groups.io/g/devel/message/114902
Mute This Topic: https://groups.io/mt/104073297/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformPei: consider AP stacks for pei memory cap

[Gerd Hoffmann]

  Hi,

> > +  if (MemoryCap > MAX_UINT32) {
> > +    MemoryCap = MAX_UINT32;
> > +  }
> > +
> 
> (2) This doesn't look good, for two reasons.
> 
> First, if UINTN is UINT32, then we're too late to check. (If it's intentional that the code be dead on IA32, then that should be documented.)

IA32 has an early exit in this function.

Looking again I see this applies to 32-bit DXE only, so with mixed
32-bit PEI / 64-bit DXE we can still land here.  Hmm, so yes, you
have a point here.

In practice we don't come even close to MAX_UINT32.  With 4096 vcpus the
amount of memory needed for the AP stacks sums up to 128 MB.  The page
table memory wouldn't grow that big too because OVMF will use at most
1 TB of address space if gigabyte pages are not available (to limit page
table memory and boot time needed to set them all up).

> Second, while returning MAX_UINT32 from this function is safe, it's also obscure. After the call site, we've got
> 
>     MemoryBase = PlatformInfoHob->S3Supported && PlatformInfoHob->SmmSmramRequire ?
>                  PcdGet32 (PcdOvmfDecompressionScratchEnd) :
>                  PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32 (PcdOvmfDxeMemFvSize);
>     MemorySize = LowerMemorySize - MemoryBase;
>     if (MemorySize > PeiMemoryCap) {
>       MemoryBase = LowerMemorySize - PeiMemoryCap;
>       MemorySize = PeiMemoryCap;
>     }

MemorySize is UINT64.  So I guess the easiest would be to just switch
MemoryCap variable and GetPeiMemoryCap() return value to UINT64 too.
Avoids all the thinking and arguing whenever UINT32 is safe or not.

And maybe add an else branch to log a warning in case
MemorySize < PeiMemoryCap.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114903): https://edk2.groups.io/g/devel/message/114903
Mute This Topic: https://groups.io/mt/104073297/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformPei: rewrite page table calculation

[Laszlo Ersek]

On 1/31/24 12:59, Gerd Hoffmann wrote:
> Consider 5-level paging.  Simplify calculation to make it easier to
> understand.  The new calculation is not 100% exact, but we only need
> a rough estimate to reserve enough memory.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 42 ++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 19 deletions(-)

The cover letter should have explained that this series depends on the
5-level paging series -- or else, this one should be appended to that
series.

With no on-list connection between them, it's a mess for me to keep
track of which one to merge first.

> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 338798b54171..e22743b4bfaa 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -184,8 +184,10 @@ GetPeiMemoryCap (
>    BOOLEAN  Page1GSupport;
>    UINT32   RegEax;
>    UINT32   RegEdx;
> -  UINT32   Pml4Entries;
> -  UINT32   PdpEntries;
> +  UINT32   Level5Pages;
> +  UINT32   Level4Pages;
> +  UINT32   Level3Pages;
> +  UINT32   Level2Pages;
>    UINTN    TotalPages;
>    UINTN    ApStacks;
>    UINTN    MemoryCap;
> @@ -203,8 +205,7 @@ GetPeiMemoryCap (
>    //
>    // Dependent on physical address width, PEI memory allocations can be
>    // dominated by the page tables built for 64-bit DXE. So we key the cap off
> -  // of those. The code below is based on CreateIdentityMappingPageTables() in
> -  // "MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c".
> +  // of those.
>    //
>    Page1GSupport = FALSE;
>    if (PcdGetBool (PcdUse1GPageTable)) {
> @@ -217,23 +218,26 @@ GetPeiMemoryCap (
>      }
>    }
>  
> -  if (PlatformInfoHob->PhysMemAddressWidth <= 39) {
> -    Pml4Entries = 1;
> -    PdpEntries  = 1 << (PlatformInfoHob->PhysMemAddressWidth - 30);
> -    ASSERT (PdpEntries <= 0x200);
> -  } else {
> -    if (PlatformInfoHob->PhysMemAddressWidth > 48) {
> -      Pml4Entries = 0x200;
> -    } else {
> -      Pml4Entries = 1 << (PlatformInfoHob->PhysMemAddressWidth - 39);
> -    }
> -
> -    ASSERT (Pml4Entries <= 0x200);
> -    PdpEntries = 512;
> +  //
> +  // This calculation doesn't return the *exact* number of page table
> +  // pages needed.  But we only need a rough estimate to reserve
> +  // enough memory, being off by a few pages isn't much of problem.
> +  // So keep the calculation simple and easy to understand.
> +  //
> +  // Page size is 4k, which needs 12 address bits.
> +  // Page tables on all levels have 512 entries, mapping to 9 address bits.
> +  // We use large pages (2M or 1G), so level 1 never exists.
> +  // Start with level 2, where a page covers 12 + 9 + 9 = 30 address bits.
> +  //

The calculation seems correct. On level 2, using 2MB page size for the
physical address space, one entry covers 2MB of phys address space, and
we have 512 entries in one 4KB page table page. This means one page
table page covers 512 * 2MB = 1024MB = 1GB = 2^30 B.

(1) The wording is difficult to follow though, for me anyway. How about
this:

-------
- A 4KB page accommodates the least significant 12 bits of the virtual
address.
- A page table entry at any level consumes 8 bytes, so a 4KB page table
page (at any level) contains 512 entries, and accommodates 9 bits of the
virtual address.
- we minimally cover the phys address space with 2MB pages, so level 1
never exists.
- If 1G paging is available, then level 2 doesn't exist either.
- Start with level 2, where a page table page accommodates 9 + 9 + 12 =
30 bits of the virtual address (and covers 1GB of physical address space).
-------

If you think this isn't any easier to follow, then feel free to stick
with your description.

> +  Level2Pages = (1 << (PlatformInfoHob->PhysMemAddressWidth - 30)) + 1;

General calculation:

  2^PhysMemAddressWidth / 2^21 / 2^9 ==
  2^(PhysMemAddressWidth - 21 - 9) ==
  1 << (PhysMemAddressWidth - 30)

OK. The left-shift of the signed int (INT32) constant "1" is also safe,
given that (I think?) PhysMemAddressWidth cannot be larger than 57. So
the shift count is at most 27, and 1 << 27 is safe.

(2) Why do we need to add 1?

I don't think there's a need for rounding up. The original dividend,
2^PhysMemAddressWidth, is an integral power of two, and
PhysMemAddressWidth is minimally 36 (IIRC). (If PhysMemAddressWidth were
less than 30, then the left shift would be undefined, to begin with.)

> +  Level3Pages = (Level2Pages >> 9) + 1;
> +  Level4Pages = (Level3Pages >> 9) + 1;
> +  Level5Pages = (Level4Pages >> 9) + 1;
> +  if (Page1GSupport) {
> +    Level2Pages = 0;
>    }

(3) I'm sorry, these +1 additions *really* annoy me, not to mention the
fact that we *include* those increments in the further shifting. Can we do:

  UINT64  End;
  UINT64  Level2Pages, Level3Pages, Level4Pages, Level5Pages;

  End         = 1LLU << PlatformInfoHob->PhysMemAddressWidth;
  Level2Pages = Page1GSupport ? 0LLU : End >> 30;
  Level3Pages = MAX (End >> 39, 1LLU);
  Level4Pages = MAX (End >> 48, 1LLU);
  Level5Pages = 1;

This doesn't seem any more complicated, and it's exact, I believe.

>  
> -  TotalPages = Page1GSupport ? Pml4Entries + 1 :
> -               (PdpEntries + 1) * Pml4Entries + 1;
> +  TotalPages = Level5Pages + Level4Pages + Level3Pages + Level2Pages;
>    ASSERT (TotalPages <= 0x40201);

(4) The ASSERT() is no longer correct, for two reasons: (a) it does not
consider 5-level paging, (b) the calculation from the patch is not exact
anyway.

But, I think the method I'm proposing should be exact (discounting that
with 5-level paging unavailable, Level5Pages should be set to zero).

Assuming PhysMemAddressWidth is 57, and 1GB pages are not supported, we get:

  Level2Pages = BIT27;
  Level3Pages = BIT18;
  Level4Pages = BIT9;
  Level5Pages = BIT0;

therefore

  ASSERT (TotalPages <= 0x8040201);

in other words, we only need to add BIT27 to the existing constant
0x40201, in the ASSERT().

>  
>    ApStacks  = PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber * PcdGet32 (PcdCpuApStackSize);

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114905): https://edk2.groups.io/g/devel/message/114905
Mute This Topic: https://groups.io/mt/104073300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformPei: rewrite page table calculation

[Laszlo Ersek]

On 1/31/24 16:13, Laszlo Ersek wrote:

> (3) I'm sorry, these +1 additions *really* annoy me, not to mention the
> fact that we *include* those increments in the further shifting. Can we do:
> 
>   UINT64  End;
>   UINT64  Level2Pages, Level3Pages, Level4Pages, Level5Pages;
> 
>   End         = 1LLU << PlatformInfoHob->PhysMemAddressWidth;
>   Level2Pages = Page1GSupport ? 0LLU : End >> 30;
>   Level3Pages = MAX (End >> 39, 1LLU);
>   Level4Pages = MAX (End >> 48, 1LLU);
>   Level5Pages = 1;
> 
> This doesn't seem any more complicated, and it's exact, I believe.

Sorry, I forgot about an edk2 portability rule here. We shouldn't do
64-bit wide "native" shifts in code that may be compiled for 32-bit.
Instead, we're supposed to use LShiftU64() and RShiftU64(), from
BaseLib. Thus:

  UINT64  End;
  UINT64  Level2Pages, Level3Pages, Level4Pages, Level5Pages;

  End         = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth);
  Level2Pages = Page1GSupport ? 0LLU : RShiftU64 (End, 30);
  Level3Pages = MAX (RShiftU64 (End, 39), 1LLU);
  Level4Pages = MAX (RShiftU64 (End, 48), 1LLU);
  Level5Pages = 1;

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114906): https://edk2.groups.io/g/devel/message/114906
Mute This Topic: https://groups.io/mt/104073300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: log pei memory cap details

[Laszlo Ersek]

On 1/31/24 13:00, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index e22743b4bfaa..988fcc512362 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -246,6 +246,30 @@ GetPeiMemoryCap (
>      MemoryCap = MAX_UINT32;
>    }
>  
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: page tables: %6lld kb (%d/%d/%d/%d)\n",
> +    __func__,
> +    (UINT64)(EFI_PAGES_TO_SIZE (TotalPages) / 1024),

(1) There is no "ll" size modifier in edk2; <PrintLib.h> is not standard
C :)

Use a single "l" or "L" (doesn't matter which).

(2) "d" is not right for UINT*; we should use "u".

In total, please use %lu or %Lu.

(3) Using the exact inclusive page count limit, from my comments on the
previous patch in the series (0x8040201), the memory usage could be as
high as 537,921,540 KB (decimal). Setting the field width to 6 is not
consistent with that; we should use 9 (if we want padding).

(4) we shouldn't divide in 64-bit "natively", in such code that may be
compiled for 32-bit; either use RShiftU64() or DivU64x32(), from
BaseLib. (This is really hard to remember, unfortunately.)

Ooops, wait, ignore this: you are not dividing in UINT64, but in UINTN.
That's fine! The cast only refers to the quotient.


> +    Level5Pages,
> +    Level4Pages,
> +    Level3Pages,
> +    Level2Pages

(5) In my proposal for the previous patch, I changed the type of these
variables to UINT64 (for simplicity), so they should all be formatted
with %Lu or %lu. (%d is not right in any case; it should be %u even for
UINT32.)

> +    ));
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: ap stacks:   %6lld kb (%d cpus)\n",
> +    __func__,
> +    (UINT64)(ApStacks / 1024),

The division should be OK... ApStacks is UINTN (in fact it is a UINT32
product).

Not sure what to specify as field width... 4GB is 4,194,304 KB, so in
theory, we should use 7 for the field width.

(6) I propose "%7lu".

> +    PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber

(7) UINT32, so should be formatted with %u.

(BTW, our ApStacks calculation includes the BSP as well (!), so it is a
bit of an over-estimate. The name "ApStacks" is slightly misleading.)

> +    ));
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: memory cap:  %6lld kb\n",
> +    __func__,
> +    (UINT64)(MemoryCap / 1024)
> +    ));
> +

(8) At this point, MemoryCap is limited to MAX_UINT32, therefore I again
propose "%7lu".

>    //
>    // Add 64 MB for miscellaneous allocations. Note that for
>    // PhysMemAddressWidth values close to 36, the cap will actually be

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114907): https://edk2.groups.io/g/devel/message/114907
Mute This Topic: https://groups.io/mt/104073299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformPei: rewrite page table calculation

[Gerd Hoffmann]

On Wed, Jan 31, 2024 at 04:13:24PM +0100, Laszlo Ersek wrote:
> On 1/31/24 12:59, Gerd Hoffmann wrote:
> > Consider 5-level paging.  Simplify calculation to make it easier to
> > understand.  The new calculation is not 100% exact, but we only need
> > a rough estimate to reserve enough memory.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  OvmfPkg/PlatformPei/MemDetect.c | 42 ++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> The cover letter should have explained that this series depends on the
> 5-level paging series -- or else, this one should be appended to that
> series.
> 
> With no on-list connection between them, it's a mess for me to keep
> track of which one to merge first.

There is no hard dependency between the two, it doesn't matter much
which is merged first.  The connection between the two series is that
for guests with alot of memory you'll need both.  Alot means hundreds
of TB, exceeding the address space which 4-level paging can handle,
so the TotalPages calculation done by the old code is *significantly*
off (more than just the one extra page for the 5th level).

> (1) The wording is difficult to follow though, for me anyway. How about
> this:
> 
> -------
> - A 4KB page accommodates the least significant 12 bits of the virtual
> address.
> - A page table entry at any level consumes 8 bytes, so a 4KB page table
> page (at any level) contains 512 entries, and accommodates 9 bits of the
> virtual address.
> - we minimally cover the phys address space with 2MB pages, so level 1
> never exists.
> - If 1G paging is available, then level 2 doesn't exist either.
> - Start with level 2, where a page table page accommodates 9 + 9 + 12 =
> 30 bits of the virtual address (and covers 1GB of physical address space).
> -------
> 
> If you think this isn't any easier to follow, then feel free to stick
> with your description.

I happily go with your more verbose version.

> (3) I'm sorry, these +1 additions *really* annoy me, not to mention the
> fact that we *include* those increments in the further shifting. Can we do:
> 
>   UINT64  End;
>   UINT64  Level2Pages, Level3Pages, Level4Pages, Level5Pages;
> 
>   End         = 1LLU << PlatformInfoHob->PhysMemAddressWidth;
>   Level2Pages = Page1GSupport ? 0LLU : End >> 30;
>   Level3Pages = MAX (End >> 39, 1LLU);
>   Level4Pages = MAX (End >> 48, 1LLU);
>   Level5Pages = 1;
> 
> This doesn't seem any more complicated, and it's exact, I believe.

Looks good, I'll take it, thanks alot.

> >    ASSERT (TotalPages <= 0x40201);
> 
> (4) The ASSERT() is no longer correct, for two reasons: (a) it does not
> consider 5-level paging, (b) the calculation from the patch is not exact
> anyway.
> 
> But, I think the method I'm proposing should be exact (discounting that
> with 5-level paging unavailable, Level5Pages should be set to zero).
> 
> Assuming PhysMemAddressWidth is 57, and 1GB pages are not supported, we get:
> 
>   Level2Pages = BIT27;
>   Level3Pages = BIT18;
>   Level4Pages = BIT9;
>   Level5Pages = BIT0;
> 
> therefore
> 
>   ASSERT (TotalPages <= 0x8040201);

Ah, *this* is how this constant was calculated.

> in other words, we only need to add BIT27 to the existing constant
> 0x40201, in the ASSERT().

With 1GB pages Level2Pages will be zero, so 0x40201 is correct in that
case.

Without 1GB pages OVMF will use at most PhysMemAddressWidth = 40 (1TB),
so:

   Level2Pages = BIT10;
   Level3Pages = BIT1;
   Level4Pages = 1;
   Level5Pages = 1;
   -> SUM      = 0x404;

Which is smaller than 0x40201.  So the ASSERT happens to be correct.
Which makes sense.  The max page table tree is identical for 4-level
paging with 2M pages and 5-level paging with 1G pages.

I'll add a comment explaining this.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114909): https://edk2.groups.io/g/devel/message/114909
Mute This Topic: https://groups.io/mt/104073300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] OvmfPkg/PlatformPei: consider AP stacks for pei memory cap

[Laszlo Ersek]

On 1/31/24 15:55, Gerd Hoffmann wrote:
>   Hi,
>
>>> +  if (MemoryCap > MAX_UINT32) {
>>> +    MemoryCap = MAX_UINT32;
>>> +  }
>>> +
>>
>> (2) This doesn't look good, for two reasons.
>>
>> First, if UINTN is UINT32, then we're too late to check. (If it's
>> intentional that the code be dead on IA32, then that should be
>> documented.)
>
> IA32 has an early exit in this function.
>
> Looking again I see this applies to 32-bit DXE only, so with mixed
> 32-bit PEI / 64-bit DXE we can still land here.  Hmm, so yes, you
> have a point here.
>
> In practice we don't come even close to MAX_UINT32.  With 4096 vcpus
> the amount of memory needed for the AP stacks sums up to 128 MB.  The
> page table memory wouldn't grow that big too because OVMF will use at
> most 1 TB of address space if gigabyte pages are not available (to
> limit page table memory and boot time needed to set them all up).
>
>> Second, while returning MAX_UINT32 from this function is safe, it's
>> also obscure. After the call site, we've got
>>
>>     MemoryBase = PlatformInfoHob->S3Supported && PlatformInfoHob->SmmSmramRequire ?
>>                  PcdGet32 (PcdOvmfDecompressionScratchEnd) :
>>                  PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32 (PcdOvmfDxeMemFvSize);
>>     MemorySize = LowerMemorySize - MemoryBase;
>>     if (MemorySize > PeiMemoryCap) {
>>       MemoryBase = LowerMemorySize - PeiMemoryCap;
>>       MemorySize = PeiMemoryCap;
>>     }
>
> MemorySize is UINT64.  So I guess the easiest would be to just switch
> MemoryCap variable and GetPeiMemoryCap() return value to UINT64 too.
> Avoids all the thinking and arguing whenever UINT32 is safe or not.

I consider that a cop-out; I'd want to see evidence (or deduce the
evidence myself) that UINT64 was safe, too. UINT64 is not "unlimited"
either.

In general, I don't want to do integer arithmetic without knowing the
potential value ranges at all times.

(1) It's great that we're discussing this, BTW -- an integer overflow is
already possible (at least in theory) after the second patch in the
series.

Before the series, TotalPages is at most 0x40201, so
EFI_PAGES_TO_SIZE(TotalPages) is at most ~1GB. We add SIZE_64MB to it;
that still fits in the UINT32 return value.

After the first patch in the series, a new increment is ApStacks; you
mention it never exceeds 128 MB in practice. So that's safe (in
practice).

However, after the second patch in the series, that no longer suffices
(in theory). As I calculated under patch#2, TotalPages can grow up to
and including 0x8040201, which means EFI_PAGES_TO_SIZE(TotalPages) is
~513 GB.

Meaning that,

(1.1) on IA32X64, the following

  MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;

already truncates (because EFI_PAGES_TO_SIZE takes a UINTN and produces
a UINTN, but UINTN is just UINT32); and that

(1.2) on X64, while "MemoryCap" itself is fine, the final

   return (UINT32)(MemoryCap);

will truncate.

(2) Now, if we consider the 1TB address space limitation from
PlatformAddressWidthFromCpuid()
[OvmfPkg/Library/PlatformInitLib/MemDetect.c] that you highlight:

    if (!Page1GSupport && (PhysBits > 40)) {
      DEBUG ((DEBUG_INFO, "%a: limit PhysBits to 40 (no 1G pages available)\n", __func__));
      PhysBits = 40;
    }

then we get (from the exact calculation):

  Level2Pages = BIT10;
  Level3Pages = BIT1;
  Level4Pages = 1;
  Level5Pages = 1;
  TotalPages  = 1024 + 2 + 1 + 1;

and then EFI_PAGES_TO_SIZE(1028) is just ~ 4 MB. So the UINT32 retval is
safe, with the 1TB address space limitation in place -- but then we
should ASSERT the predicate here that PlatformAddressWidthFromCpuid()
ensures: ((PhysBits <= 40) || Page1GSupport)!

(3) Finally, if Page1GSupport is TRUE, and PhysMemAddressWidth is 57, we
get:

  Level2Pages = 0;
  Level3Pages = BIT18;
  Level4Pages = BIT9;
  Level5Pages = BIT0;

which produces the pre-series value 0x40201 for TotalPages -- so that
again will be safe for the UINT32 retval (the memory allocated for page
tables will be again ~1GB).


OK, summary:

- before calculating "End" and "Level2Pages" in patch#2, we should
  ASSERT ((PhysBits <= 40) || Page1GSupport).

- I guess it is fine to assume that
  (PcdCpuMaxLogicalProcessorNumber*PcdCpuApStackSize) will never
  exceed a few hundred MB. Maybe add a comment?

- I suggest keeping the retval UINT32; there is no reason for widening
  it to UINT64 (thanks to the 1TB address space limitation, without 1G
  pages!)

- The pre-series assertion that (TotalPages <= 0x40201) remains correct,
  after all, using the exact calculation (again due to the 1TB address
  space limitation, without 1G pages).

>
> And maybe add an else branch to log a warning in case
> MemorySize < PeiMemoryCap.

That is a good idea even with the unchanged UINT32 retval type!

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114917): https://edk2.groups.io/g/devel/message/114917
Mute This Topic: https://groups.io/mt/104073297/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformPei: rewrite page table calculation

[Laszlo Ersek]

On 1/31/24 17:28, Gerd Hoffmann wrote:
> On Wed, Jan 31, 2024 at 04:13:24PM +0100, Laszlo Ersek wrote:
>> On 1/31/24 12:59, Gerd Hoffmann wrote:
>>> Consider 5-level paging.  Simplify calculation to make it easier to
>>> understand.  The new calculation is not 100% exact, but we only need
>>> a rough estimate to reserve enough memory.
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>>>  OvmfPkg/PlatformPei/MemDetect.c | 42 ++++++++++++++++++---------------
>>>  1 file changed, 23 insertions(+), 19 deletions(-)
>>
>> The cover letter should have explained that this series depends on the
>> 5-level paging series -- or else, this one should be appended to that
>> series.
>>
>> With no on-list connection between them, it's a mess for me to keep
>> track of which one to merge first.
> 
> There is no hard dependency between the two, it doesn't matter much
> which is merged first.  The connection between the two series is that
> for guests with alot of memory you'll need both.  Alot means hundreds
> of TB, exceeding the address space which 4-level paging can handle,
> so the TotalPages calculation done by the old code is *significantly*
> off (more than just the one extra page for the 5th level).
> 
>> (1) The wording is difficult to follow though, for me anyway. How about
>> this:
>>
>> -------
>> - A 4KB page accommodates the least significant 12 bits of the virtual
>> address.
>> - A page table entry at any level consumes 8 bytes, so a 4KB page table
>> page (at any level) contains 512 entries, and accommodates 9 bits of the
>> virtual address.
>> - we minimally cover the phys address space with 2MB pages, so level 1
>> never exists.
>> - If 1G paging is available, then level 2 doesn't exist either.
>> - Start with level 2, where a page table page accommodates 9 + 9 + 12 =
>> 30 bits of the virtual address (and covers 1GB of physical address space).
>> -------
>>
>> If you think this isn't any easier to follow, then feel free to stick
>> with your description.
> 
> I happily go with your more verbose version.
> 
>> (3) I'm sorry, these +1 additions *really* annoy me, not to mention the
>> fact that we *include* those increments in the further shifting. Can we do:
>>
>>   UINT64  End;
>>   UINT64  Level2Pages, Level3Pages, Level4Pages, Level5Pages;
>>
>>   End         = 1LLU << PlatformInfoHob->PhysMemAddressWidth;
>>   Level2Pages = Page1GSupport ? 0LLU : End >> 30;
>>   Level3Pages = MAX (End >> 39, 1LLU);
>>   Level4Pages = MAX (End >> 48, 1LLU);
>>   Level5Pages = 1;
>>
>> This doesn't seem any more complicated, and it's exact, I believe.
> 
> Looks good, I'll take it, thanks alot.
> 
>>>    ASSERT (TotalPages <= 0x40201);
>>
>> (4) The ASSERT() is no longer correct, for two reasons: (a) it does not
>> consider 5-level paging, (b) the calculation from the patch is not exact
>> anyway.
>>
>> But, I think the method I'm proposing should be exact (discounting that
>> with 5-level paging unavailable, Level5Pages should be set to zero).
>>
>> Assuming PhysMemAddressWidth is 57, and 1GB pages are not supported, we get:
>>
>>   Level2Pages = BIT27;
>>   Level3Pages = BIT18;
>>   Level4Pages = BIT9;
>>   Level5Pages = BIT0;
>>
>> therefore
>>
>>   ASSERT (TotalPages <= 0x8040201);
> 
> Ah, *this* is how this constant was calculated.
> 
>> in other words, we only need to add BIT27 to the existing constant
>> 0x40201, in the ASSERT().
> 
> With 1GB pages Level2Pages will be zero, so 0x40201 is correct in that
> case.

Right! :)

> 
> Without 1GB pages OVMF will use at most PhysMemAddressWidth = 40 (1TB),
> so:
> 
>    Level2Pages = BIT10;
>    Level3Pages = BIT1;
>    Level4Pages = 1;
>    Level5Pages = 1;
>    -> SUM      = 0x404;
> 
> Which is smaller than 0x40201.  So the ASSERT happens to be correct.
> Which makes sense.  The max page table tree is identical for 4-level
> paging with 2M pages and 5-level paging with 1G pages.

I was amazed to find that, myself; but exactly as you explain, in
retrospect, it's "obvious". :) It just feels nice that we keep the
original "large page" tree, just shift the leaf granularity by 9 bits!

> 
> I'll add a comment explaining this.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114963): https://edk2.groups.io/g/devel/message/114963
Mute This Topic: https://groups.io/mt/104073300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-