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

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

[Gerd Hoffmann]

v2:
 - rewrite page table calculation, rewrite comment (Laszlo)
 - add warning message if PEI memory is tight.
 - format string fixes.
 - misc small tweaks.

Gerd Hoffmann (4):
  OvmfPkg/PlatformPei: log a warning when memory is tight
  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 | 103 +++++++++++++++++++++++++-------
 1 file changed, 80 insertions(+), 23 deletions(-)

-- 
2.43.0



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

[edk2-devel] [PATCH v2 1/4] OvmfPkg/PlatformPei: log a warning when memory is tight

[Gerd Hoffmann]

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

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 493cb1fbeb01..1684a3783546 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -319,6 +319,14 @@ PublishPeiMemory (
     if (MemorySize > PeiMemoryCap) {
       MemoryBase = LowerMemorySize - PeiMemoryCap;
       MemorySize = PeiMemoryCap;
+    } else {
+      DEBUG ((
+        DEBUG_WARN,
+        "%a: Not enough memory for PEI (have %llu KB, estimated need %llu KB)\n",
+        __func__,
+        RShiftU64 (MemorySize, 10),
+        RShiftU64 (PeiMemoryCap, 10)
+        ));
     }
   }
 
-- 
2.43.0



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

Re: [edk2-devel] [PATCH v2 1/4] OvmfPkg/PlatformPei: log a warning when memory is tight

[Laszlo Ersek]

On 2/2/24 11:47, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 493cb1fbeb01..1684a3783546 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -319,6 +319,14 @@ PublishPeiMemory (
>      if (MemorySize > PeiMemoryCap) {
>        MemoryBase = LowerMemorySize - PeiMemoryCap;
>        MemorySize = PeiMemoryCap;
> +    } else {
> +      DEBUG ((
> +        DEBUG_WARN,
> +        "%a: Not enough memory for PEI (have %llu KB, estimated need %llu KB)\n",
> +        __func__,
> +        RShiftU64 (MemorySize, 10),
> +        RShiftU64 (PeiMemoryCap, 10)
> +        ));
>      }
>    }
>  

both %llu should be just %lu

With that updated:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

[edk2-devel] [PATCH v2 2/4] 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 | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 1684a3783546..83f1c1d02a26 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -187,6 +187,8 @@ GetPeiMemoryCap (
   UINT32   Pml4Entries;
   UINT32   PdpEntries;
   UINTN    TotalPages;
+  UINT64   ApStacks;
+  UINT64   MemoryCap;
 
   //
   // If DXE is 32-bit, then just return the traditional 64 MB cap.
@@ -234,12 +236,21 @@ GetPeiMemoryCap (
                (PdpEntries + 1) * Pml4Entries + 1;
   ASSERT (TotalPages <= 0x40201);
 
+  //
+  // With 32k stacks and 4096 vcpus this lands at 128 MB (far away
+  // from MAX_UINT32).
+  //
+  ApStacks = PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber * PcdGet32 (PcdCpuApStackSize);
+
   //
   // Add 64 MB for miscellaneous allocations. Note that for
-  // PhysMemAddressWidth values close to 36, the cap will actually be
-  // dominated by this increment.
+  // PhysMemAddressWidth values close to 36 and a small number of
+  // CPUs, the cap will actually be dominated by this increment.
   //
-  return (UINT32)(EFI_PAGES_TO_SIZE (TotalPages) + SIZE_64MB);
+  MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;
+
+  ASSERT (MemoryCap <= MAX_UINT32);
+  return (UINT32)MemoryCap;
 }
 
 /**
-- 
2.43.0



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

Re: [edk2-devel] [PATCH v2 2/4] OvmfPkg/PlatformPei: consider AP stacks for pei memory cap

[Laszlo Ersek]

On 2/2/24 11:47, 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 | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 1684a3783546..83f1c1d02a26 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -187,6 +187,8 @@ GetPeiMemoryCap (
>    UINT32   Pml4Entries;
>    UINT32   PdpEntries;
>    UINTN    TotalPages;
> +  UINT64   ApStacks;
> +  UINT64   MemoryCap;
>  
>    //
>    // If DXE is 32-bit, then just return the traditional 64 MB cap.
> @@ -234,12 +236,21 @@ GetPeiMemoryCap (
>                 (PdpEntries + 1) * Pml4Entries + 1;
>    ASSERT (TotalPages <= 0x40201);
>  
> +  //
> +  // With 32k stacks and 4096 vcpus this lands at 128 MB (far away
> +  // from MAX_UINT32).
> +  //
> +  ApStacks = PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber * PcdGet32 (PcdCpuApStackSize);
> +
>    //
>    // Add 64 MB for miscellaneous allocations. Note that for
> -  // PhysMemAddressWidth values close to 36, the cap will actually be
> -  // dominated by this increment.
> +  // PhysMemAddressWidth values close to 36 and a small number of
> +  // CPUs, the cap will actually be dominated by this increment.
>    //
> -  return (UINT32)(EFI_PAGES_TO_SIZE (TotalPages) + SIZE_64MB);
> +  MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;
> +
> +  ASSERT (MemoryCap <= MAX_UINT32);
> +  return (UINT32)MemoryCap;
>  }
>  
>  /**

Yes, this looks good. At this point, EFI_PAGES_TO_SIZE (TotalPages) is
~1GB at most, and ApStacks is empirically <= 128 MB, plus ApStacks is
UINT64. Therefore on IA32X64 we have

  UINT64 = (UINT32 + UINT64) + INT32
->
  UINT64 = (UINT64 + UINT64) + INT32
->
  UINT64 = UINT64 + INT32
->
  UINT64 = UINT64 + UINT64
->
  UINT64 = UINT64

OK!

Reviewed-by: Laszlo Ersek <lersek@redhat.com>




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

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

[Gerd Hoffmann]

Consider 5-level paging.  Simplify calculation to make it easier
to understand.  Add some comments, improve ASSERTs.

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

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 83f1c1d02a26..48ad0b83a55e 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -184,9 +184,12 @@ GetPeiMemoryCap (
   BOOLEAN  Page1GSupport;
   UINT32   RegEax;
   UINT32   RegEdx;
-  UINT32   Pml4Entries;
-  UINT32   PdpEntries;
-  UINTN    TotalPages;
+  UINT64   MaxAddr;
+  UINT32   Level5Pages;
+  UINT32   Level4Pages;
+  UINT32   Level3Pages;
+  UINT32   Level2Pages;
+  UINT32   TotalPages;
   UINT64   ApStacks;
   UINT64   MemoryCap;
 
@@ -203,8 +206,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,25 +219,37 @@ GetPeiMemoryCap (
     }
   }
 
-  if (PlatformInfoHob->PhysMemAddressWidth <= 39) {
-    Pml4Entries = 1;
-    PdpEntries  = 1 << (PlatformInfoHob->PhysMemAddressWidth - 30);
-    ASSERT (PdpEntries <= 0x200);
+  //
+  // - 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).
+  //
+
+  MaxAddr     = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth);
+  Level2Pages = (UINT32)RShiftU64 (MaxAddr, 30);
+  Level3Pages = MAX (Level2Pages >> 9, 1);
+  Level4Pages = MAX (Level3Pages >> 9, 1);
+  Level5Pages = 1;
+
+  if (Page1GSupport) {
+    Level2Pages = 0;
+    TotalPages  = Level5Pages + Level4Pages + Level3Pages;
+    ASSERT (TotalPages <= 0x40201);
   } else {
-    if (PlatformInfoHob->PhysMemAddressWidth > 48) {
-      Pml4Entries = 0x200;
-    } else {
-      Pml4Entries = 1 << (PlatformInfoHob->PhysMemAddressWidth - 39);
-    }
-
-    ASSERT (Pml4Entries <= 0x200);
-    PdpEntries = 512;
+    TotalPages = Level5Pages + Level4Pages + Level3Pages + Level2Pages;
+    // PlatformAddressWidthFromCpuid() caps at 40 phys bits without 1G pages.
+    ASSERT (PlatformInfoHob->PhysMemAddressWidth <= 40);
+    ASSERT (TotalPages <= 0x404);
   }
 
-  TotalPages = Page1GSupport ? Pml4Entries + 1 :
-               (PdpEntries + 1) * Pml4Entries + 1;
-  ASSERT (TotalPages <= 0x40201);
-
   //
   // With 32k stacks and 4096 vcpus this lands at 128 MB (far away
   // from MAX_UINT32).
-- 
2.43.0



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

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

[Laszlo Ersek]

On 2/2/24 11:47, Gerd Hoffmann wrote:
> Consider 5-level paging.  Simplify calculation to make it easier
> to understand.  Add some comments, improve ASSERTs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 56 ++++++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 83f1c1d02a26..48ad0b83a55e 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -184,9 +184,12 @@ GetPeiMemoryCap (
>    BOOLEAN  Page1GSupport;
>    UINT32   RegEax;
>    UINT32   RegEdx;
> -  UINT32   Pml4Entries;
> -  UINT32   PdpEntries;
> -  UINTN    TotalPages;
> +  UINT64   MaxAddr;
> +  UINT32   Level5Pages;
> +  UINT32   Level4Pages;
> +  UINT32   Level3Pages;
> +  UINT32   Level2Pages;
> +  UINT32   TotalPages;
>    UINT64   ApStacks;
>    UINT64   MemoryCap;
>  
> @@ -203,8 +206,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,25 +219,37 @@ GetPeiMemoryCap (
>      }
>    }
>  
> -  if (PlatformInfoHob->PhysMemAddressWidth <= 39) {
> -    Pml4Entries = 1;
> -    PdpEntries  = 1 << (PlatformInfoHob->PhysMemAddressWidth - 30);
> -    ASSERT (PdpEntries <= 0x200);
> +  //
> +  // - 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).
> +  //
> +
> +  MaxAddr     = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth);
> +  Level2Pages = (UINT32)RShiftU64 (MaxAddr, 30);
> +  Level3Pages = MAX (Level2Pages >> 9, 1);

(1) The MAX() macro in "MdePkg/Include/Base.h" is documented like this:

/**
  Return the maximum of two operands.

  This macro returns the maximum of two operand specified by a and b.
  Both a and b must be the same numerical types, signed or unsigned.

  @param   a        The first operand with any numerical type.
  @param   b        The second operand. Can be any numerical type as long as is
                    the same type as a.

  @return  Maximum of two operands.

**/

This does not hold to the above expression. (Level2Pages >> 9) is a UINT32, but "1" is an INT32. Please use "1u".

(Note that the replacement text of the macro, namely (((a) > (b)) ? (a) : (b)), would work fine as-is, but we still should not break the documented interface.)

> +  Level4Pages = MAX (Level3Pages >> 9, 1);

(2) Same comment as above, plus another one:

(3) I'm slightly disturbed by the fact that here we don't shift the original MaxAddr by 48 bits, but Level3Pages by 9 bits. Namely, if Level3Pages was set to 1 by the MAX (i.e., because the >> 39 resulted in zero), then the input of *this* bit shift is nonsensical. It's a happenstance that 1 >> 9 is zero too, for Level4Pages, and we're just exploiting that practical result here.

I think this is semantically unclean, especially without a comment, but I don't want to insist anymore.

> +  Level5Pages = 1;
> +
> +  if (Page1GSupport) {
> +    Level2Pages = 0;
> +    TotalPages  = Level5Pages + Level4Pages + Level3Pages;
> +    ASSERT (TotalPages <= 0x40201);
>    } else {
> -    if (PlatformInfoHob->PhysMemAddressWidth > 48) {
> -      Pml4Entries = 0x200;
> -    } else {
> -      Pml4Entries = 1 << (PlatformInfoHob->PhysMemAddressWidth - 39);
> -    }
> -
> -    ASSERT (Pml4Entries <= 0x200);
> -    PdpEntries = 512;
> +    TotalPages = Level5Pages + Level4Pages + Level3Pages + Level2Pages;
> +    // PlatformAddressWidthFromCpuid() caps at 40 phys bits without 1G pages.
> +    ASSERT (PlatformInfoHob->PhysMemAddressWidth <= 40);
> +    ASSERT (TotalPages <= 0x404);
>    }
>  
> -  TotalPages = Page1GSupport ? Pml4Entries + 1 :
> -               (PdpEntries + 1) * Pml4Entries + 1;
> -  ASSERT (TotalPages <= 0x40201);
> -
>    //
>    // With 32k stacks and 4096 vcpus this lands at 128 MB (far away
>    // from MAX_UINT32).

With (1) and (2) updated (feel free to ignore (3)):

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

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

[Laszlo Ersek]

On 2/5/24 09:14, Laszlo Ersek wrote:
> On 2/2/24 11:47, Gerd Hoffmann wrote:
>> Consider 5-level paging.  Simplify calculation to make it easier
>> to understand.  Add some comments, improve ASSERTs.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  OvmfPkg/PlatformPei/MemDetect.c | 56 ++++++++++++++++++++-------------
>>  1 file changed, 35 insertions(+), 21 deletions(-)
>>
>> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
>> index 83f1c1d02a26..48ad0b83a55e 100644
>> --- a/OvmfPkg/PlatformPei/MemDetect.c
>> +++ b/OvmfPkg/PlatformPei/MemDetect.c
>> @@ -184,9 +184,12 @@ GetPeiMemoryCap (
>>    BOOLEAN  Page1GSupport;
>>    UINT32   RegEax;
>>    UINT32   RegEdx;
>> -  UINT32   Pml4Entries;
>> -  UINT32   PdpEntries;
>> -  UINTN    TotalPages;
>> +  UINT64   MaxAddr;
>> +  UINT32   Level5Pages;
>> +  UINT32   Level4Pages;
>> +  UINT32   Level3Pages;
>> +  UINT32   Level2Pages;
>> +  UINT32   TotalPages;
>>    UINT64   ApStacks;
>>    UINT64   MemoryCap;
>>  
>> @@ -203,8 +206,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,25 +219,37 @@ GetPeiMemoryCap (
>>      }
>>    }
>>  
>> -  if (PlatformInfoHob->PhysMemAddressWidth <= 39) {
>> -    Pml4Entries = 1;
>> -    PdpEntries  = 1 << (PlatformInfoHob->PhysMemAddressWidth - 30);
>> -    ASSERT (PdpEntries <= 0x200);
>> +  //
>> +  // - 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).
>> +  //
>> +
>> +  MaxAddr     = LShiftU64 (1, PlatformInfoHob->PhysMemAddressWidth);
>> +  Level2Pages = (UINT32)RShiftU64 (MaxAddr, 30);
>> +  Level3Pages = MAX (Level2Pages >> 9, 1);
> 
> (1) The MAX() macro in "MdePkg/Include/Base.h" is documented like this:
> 
> /**
>   Return the maximum of two operands.
> 
>   This macro returns the maximum of two operand specified by a and b.
>   Both a and b must be the same numerical types, signed or unsigned.
> 
>   @param   a        The first operand with any numerical type.
>   @param   b        The second operand. Can be any numerical type as long as is
>                     the same type as a.
> 
>   @return  Maximum of two operands.
> 
> **/
> 
> This does not hold to the above expression. (Level2Pages >> 9) is a UINT32, but "1" is an INT32. Please use "1u".
> 
> (Note that the replacement text of the macro, namely (((a) > (b)) ? (a) : (b)), would work fine as-is, but we still should not break the documented interface.)
> 
>> +  Level4Pages = MAX (Level3Pages >> 9, 1);
> 
> (2) Same comment as above, plus another one:
> 
> (3) I'm slightly disturbed by the fact that here we don't shift the original MaxAddr by 48 bits, but Level3Pages by 9 bits. Namely, if Level3Pages was set to 1 by the MAX (i.e., because the >> 39 resulted in zero), then the input of *this* bit shift is nonsensical. It's a happenstance that 1 >> 9 is zero too, for Level4Pages, and we're just exploiting that practical result here.
> 
> I think this is semantically unclean, especially without a comment, but I don't want to insist anymore.
> 
>> +  Level5Pages = 1;
>> +
>> +  if (Page1GSupport) {
>> +    Level2Pages = 0;
>> +    TotalPages  = Level5Pages + Level4Pages + Level3Pages;
>> +    ASSERT (TotalPages <= 0x40201);
>>    } else {
>> -    if (PlatformInfoHob->PhysMemAddressWidth > 48) {
>> -      Pml4Entries = 0x200;
>> -    } else {
>> -      Pml4Entries = 1 << (PlatformInfoHob->PhysMemAddressWidth - 39);
>> -    }
>> -
>> -    ASSERT (Pml4Entries <= 0x200);
>> -    PdpEntries = 512;
>> +    TotalPages = Level5Pages + Level4Pages + Level3Pages + Level2Pages;
>> +    // PlatformAddressWidthFromCpuid() caps at 40 phys bits without 1G pages.
>> +    ASSERT (PlatformInfoHob->PhysMemAddressWidth <= 40);
>> +    ASSERT (TotalPages <= 0x404);
>>    }
>>  
>> -  TotalPages = Page1GSupport ? Pml4Entries + 1 :
>> -               (PdpEntries + 1) * Pml4Entries + 1;
>> -  ASSERT (TotalPages <= 0x40201);
>> -
>>    //
>>    // With 32k stacks and 4096 vcpus this lands at 128 MB (far away
>>    // from MAX_UINT32).
> 
> With (1) and (2) updated (feel free to ignore (3)):
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

Sorry, just noticed:

(4) after this patch, TotalPages is UINT32, but in the subsequent context, we have:

   MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;

Per "MdePkg/Include/Uefi/UefiBaseType.h":

/**
  Macro that converts a number of EFI_PAGEs to a size in bytes.

  @param  Pages     The number of EFI_PAGES.  This parameter is assumed to be
                    type UINTN.  Passing in a parameter that is larger than
                    UINTN may produce unexpected results.

  @return  The number of bytes associated with the number of EFI_PAGEs specified
           by Pages.

**/

So in this patch, please also cast TotalPages to UINTN inside the argument of EFI_PAGES_TO_SIZE().

Thanks
Laszlo



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

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

[Gerd Hoffmann]

  Hi,

> (3) I'm slightly disturbed by the fact that here we don't shift the
> original MaxAddr by 48 bits, but Level3Pages by 9 bits. Namely, if
> Level3Pages was set to 1 by the MAX (i.e., because the >> 39 resulted
> in zero), then the input of *this* bit shift is nonsensical. It's a
> happenstance that 1 >> 9 is zero too, for Level4Pages, and we're just
> exploiting that practical result here.

I had it that way initially.  Got failures for 32-bit builds in CI,
because the compiler used 64-bit math intrinsics somewhere.  So I went
back to the version working with UINT32 ...

take care,
  Gerd



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

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

[Laszlo Ersek]

On 2/14/24 10:32, Gerd Hoffmann wrote:
>   Hi,
> 
>> (3) I'm slightly disturbed by the fact that here we don't shift the
>> original MaxAddr by 48 bits, but Level3Pages by 9 bits. Namely, if
>> Level3Pages was set to 1 by the MAX (i.e., because the >> 39 resulted
>> in zero), then the input of *this* bit shift is nonsensical. It's a
>> happenstance that 1 >> 9 is zero too, for Level4Pages, and we're just
>> exploiting that practical result here.
> 
> I had it that way initially.  Got failures for 32-bit builds in CI,
> because the compiler used 64-bit math intrinsics somewhere.

Right, I had certainly expected that in advance. You must have missed my
earlier update on that, still in the v1 thread. My original proposal
there was indeed problematic in that sense, but a few minutes later I
posted an update, replacing the bit-shifts inside the MAX() macro
invocations with RShiftU64() calls:

  Re: [edk2-devel] [PATCH 2/3] OvmfPkg/PlatformPei: rewrite page table calculation
  msgid: 74ccb355-7d71-150b-7258-305149699c0d@redhat.com
  https://edk2.groups.io/g/devel/message/114906

> So I went back to the version working with UINT32 ...

OK.

Laszlo



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

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

[Gerd Hoffmann]

On Wed, Feb 14, 2024 at 11:48:57AM +0100, Laszlo Ersek wrote:
> On 2/14/24 10:32, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >> (3) I'm slightly disturbed by the fact that here we don't shift the
> >> original MaxAddr by 48 bits, but Level3Pages by 9 bits. Namely, if
> >> Level3Pages was set to 1 by the MAX (i.e., because the >> 39 resulted
> >> in zero), then the input of *this* bit shift is nonsensical. It's a
> >> happenstance that 1 >> 9 is zero too, for Level4Pages, and we're just
> >> exploiting that practical result here.
> > 
> > I had it that way initially.  Got failures for 32-bit builds in CI,
> > because the compiler used 64-bit math intrinsics somewhere.
> 
> Right, I had certainly expected that in advance. You must have missed my
> earlier update on that, still in the v1 thread. My original proposal
> there was indeed problematic in that sense, but a few minutes later I
> posted an update, replacing the bit-shifts inside the MAX() macro
> invocations with RShiftU64() calls:

Happened even with the RShiftU64() calls, must have been something else.

take care,
  Gerd



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

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

[Laszlo Ersek]

On 2/14/24 12:07, Gerd Hoffmann wrote:
> On Wed, Feb 14, 2024 at 11:48:57AM +0100, Laszlo Ersek wrote:
>> On 2/14/24 10:32, Gerd Hoffmann wrote:
>>>   Hi,
>>>
>>>> (3) I'm slightly disturbed by the fact that here we don't shift the
>>>> original MaxAddr by 48 bits, but Level3Pages by 9 bits. Namely, if
>>>> Level3Pages was set to 1 by the MAX (i.e., because the >> 39 resulted
>>>> in zero), then the input of *this* bit shift is nonsensical. It's a
>>>> happenstance that 1 >> 9 is zero too, for Level4Pages, and we're just
>>>> exploiting that practical result here.
>>>
>>> I had it that way initially.  Got failures for 32-bit builds in CI,
>>> because the compiler used 64-bit math intrinsics somewhere.
>>
>> Right, I had certainly expected that in advance. You must have missed my
>> earlier update on that, still in the v1 thread. My original proposal
>> there was indeed problematic in that sense, but a few minutes later I
>> posted an update, replacing the bit-shifts inside the MAX() macro
>> invocations with RShiftU64() calls:
> 
> Happened even with the RShiftU64() calls, must have been something else.

Huh! That's really strange! Thanks for trying it out!

Laszlo



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

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

[Gerd Hoffmann]

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

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 48ad0b83a55e..737166f1b0ad 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -263,6 +263,30 @@ GetPeiMemoryCap (
   //
   MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;
 
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: page tables: %6lu KB (%lu/%lu/%lu/%lu pages for levels 5/4/3/2)\n",
+    __func__,
+    RShiftU64 (EFI_PAGES_TO_SIZE (TotalPages), 10),
+    Level5Pages,
+    Level4Pages,
+    Level3Pages,
+    Level2Pages
+    ));
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: ap stacks:   %6lu KB (%d cpus)\n",
+    __func__,
+    RShiftU64 (ApStacks, 10),
+    PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber
+    ));
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: memory cap:  %6lu KB\n",
+    __func__,
+    RShiftU64 (MemoryCap, 10)
+    ));
+
   ASSERT (MemoryCap <= MAX_UINT32);
   return (UINT32)MemoryCap;
 }
@@ -347,10 +371,10 @@ PublishPeiMemory (
     } else {
       DEBUG ((
         DEBUG_WARN,
-        "%a: Not enough memory for PEI (have %llu KB, estimated need %llu KB)\n",
+        "%a: Not enough memory for PEI (have %lu KB, estimated need %u KB)\n",
         __func__,
         RShiftU64 (MemorySize, 10),
-        RShiftU64 (PeiMemoryCap, 10)
+        PeiMemoryCap >> 10
         ));
     }
   }
-- 
2.43.0



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

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

[Laszlo Ersek]

On 2/2/24 11:47, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/PlatformPei/MemDetect.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 48ad0b83a55e..737166f1b0ad 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -263,6 +263,30 @@ GetPeiMemoryCap (
>    //
>    MemoryCap = EFI_PAGES_TO_SIZE (TotalPages) + ApStacks + SIZE_64MB;
>  
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: page tables: %6lu KB (%lu/%lu/%lu/%lu pages for levels 5/4/3/2)\n",
> +    __func__,
> +    RShiftU64 (EFI_PAGES_TO_SIZE (TotalPages), 10),

should be

  RShiftU64 (EFI_PAGES_TO_SIZE ((UINTN)TotalPages), 10)

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

These values are now UINT32s, so the format specifier for each should be %u

> +    ));
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: ap stacks:   %6lu KB (%d cpus)\n",
> +    __func__,
> +    RShiftU64 (ApStacks, 10),
> +    PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber

"PlatformInfoHob->PcdCpuMaxLogicalProcessorNumber" is a UINT32, should
be formatted with %u, not %d

> +    ));
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: memory cap:  %6lu KB\n",
> +    __func__,
> +    RShiftU64 (MemoryCap, 10)
> +    ));
> +
>    ASSERT (MemoryCap <= MAX_UINT32);
>    return (UINT32)MemoryCap;
>  }

OK

> @@ -347,10 +371,10 @@ PublishPeiMemory (
>      } else {
>        DEBUG ((
>          DEBUG_WARN,
> -        "%a: Not enough memory for PEI (have %llu KB, estimated need %llu KB)\n",
> +        "%a: Not enough memory for PEI (have %lu KB, estimated need %u KB)\n",
>          __func__,
>          RShiftU64 (MemorySize, 10),
> -        RShiftU64 (PeiMemoryCap, 10)
> +        PeiMemoryCap >> 10
>          ));
>      }
>    }

Did you mean to squash this hunk into patch#1 instead? (It seems
correct, just doesn't belong here IMO.)

Laszlo



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