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

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

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