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

["Laszlo Ersek" <lersek@redhat.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-