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

["Gerd Hoffmann" <kraxel@redhat.com>]

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