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

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

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