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 <[email protected]> >> --- >> 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 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
