Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Laszlo Ersek <lersek@redhat.com>]

On 11/26/18 10:42, Laszlo Ersek wrote:
> On 11/23/18 13:14, Ard Biesheuvel wrote:
>> In preparation of permitting the virt code to define a larger PA space
>> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
>> CPU actually supports, take the CPU's capabilities into account when
>> setting up the page tables. This is necessary because KVM will shortly
>> support variable PA space sizes, and to support running the same UEFI
>> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
>> treated as an upper bound rather than a fixed size.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> index 4b62ecb6a476..a4fde9b59383 100644
>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> @@ -593,6 +593,7 @@ ArmConfigureMmu (
>>  {
>>    VOID*                         TranslationTable;
>>    UINT32                        TranslationTableAttribute;
>> +  UINTN                         MaxAddressBits;
>>    UINT64                        MaxAddress;
>>    UINTN                         T0SZ;
>>    UINTN                         RootTableEntryCount;
>> @@ -605,7 +606,9 @@ ArmConfigureMmu (
>>    }
>>  
>>    // Cover the entire GCD memory space
>> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
>> +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),
>> +                        PcdGet8 (PcdPrePiCpuMemorySize));
> 
> (1) Superficial comment: sticking to the letter of MIN() in
> "MdePkg/Include/Base.h", the arguments should be of the exact same type.
> Currently they aren't. (It's a different question whether that
> requirement makes any sense for MIN().)
> 
> (2) Why does it make sense to use MIN() here? Why not just disregard
> PcdPrePiCpuMemorySize and map what the CPU is capable of? (This ties in
> with my lack of understanding of PcdPrePiCpuMemorySize.)
> 
> I mean, not going above ArmGetPhysicalAddressBits() makes total sense.
> What's the point of staying below it though? And if so, how much exactly
> do we want to stay below it? (I.e., how is a platform supposed to set
> PcdPrePiCpuMemorySize, in order to clamp down ArmGetPhysicalAddressBits()?)

Ugh, okay. (Maybe you've responded already, I'm still in write-only
mode, sorry.) I think I'm getting the idea here. This is just being done
to keep the series bisectable / regression-free in the middle too. Right?

Thanks
Laszlo

> 
>> +  MaxAddress = (1UL << MaxAddressBits) - 1;
> 
> (3) I understand this just reworks the original code, but the original
> code isn't stellar. If we are left-shifting a UINT32 or UINTN value,
> then the result is the same, and the << operator is OK. However:
> 
> - Here we intend to accommodate a UINT64 result, judged from the type of
> MaxAddress (UINT64).
> 
> - For that, we should likely left-shift 1ULL, not 1 U;, which in turn
> requires LShiftU64() from BaseLib.
> 
> (If we indeed want to use UINTN, then I think we should change the type
> of MaxAddress, plus write "(UINTN)1", not 1UL.)
> 
>>  
>>    // Lookup the Table Level to get the information
>>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
>>
> 
> Thanks,
> Laszlo
>