On 23 November 2015 at 12:41, Laszlo Ersek <ler...@redhat.com> wrote:
> On 11/21/15 10:44, Ard Biesheuvel wrote:
>> The ID mapping routines on virtual platforms simply map the entire
>> address space as device memory, and then punch some holes for regions
>> that need to be mapped cacheable. On virtual platforms hosted on CPUs
>> that support a large physical address range, this may result in a lot
>> of overhead, i.e., 4 KB of page tables for each 512 GB of address
>> space, which quickly adds up (i.e. 2 MB for the architectural maximum
>> of 48 bits).
>>
>> Since there may be a platform specific limit to the size of the (I)PA
>> space that is not reflected by CPU id registers, restrict the range of
>> the ID mapping to gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize bits,
>> since we cannot manipulate mappings above that limit anwyay (because
>> they are not covered by GCD). This allows the PCD to be set by platforms
>> whose (I)PA space has a fixed limit.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> ---
>> ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf | 1 +
>> ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c | 4 +++-
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
>> b/ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
>> index 22ee3625c37a..3cb3fb1f3aea 100644
>> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
>> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
>> @@ -61,3 +61,4 @@ [FixedPcd]
>> gArmTokenSpaceGuid.PcdArmPrimaryCore
>> gArmTokenSpaceGuid.PcdFdBaseAddress
>> gArmTokenSpaceGuid.PcdFdSize
>> + gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
>> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> index 530f7d608e0b..f6f95b02827c 100644
>> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c
>> @@ -97,7 +97,9 @@ ArmPlatformGetVirtualMemoryMap (
>> // Peripheral space after DRAM
>> VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length +
>> VirtualMemoryTable[1].Length;
>> VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase;
>> - VirtualMemoryTable[2].Length = ArmGetPhysAddrTop () -
>> VirtualMemoryTable[2].PhysicalBase;
>> + VirtualMemoryTable[2].Length = MIN(1UL << FixedPcdGet8
>> (PcdPrePiCpuMemorySize),
>> + ArmGetPhysAddrTop ()) -
>> + VirtualMemoryTable[2].PhysicalBase;
>
> (2) Can you please confirm that ArmGetPhysAddrTop() also retuns an
> exclusive limit? (I think so, but let me be sure...)
>
Yes, it is (1 << PArange), where PArange is one of { 32, 36, 40, 42, 44, 48 }
> (3) (Just a remark, there's no need to act upon this.) I think that the
> "PrePi" part in "PcdPrePiCpuMemorySize" is a misnomer here, but I agree
> that it doesn't matter much and/or there's no good way around it. For
> example, I did a very similar thing in SVN r16902. The
> "ArmPkg/Drivers/CpuPei" driver simply wants these PCDs, end of story.
>
Oh, absolutely. I just haven't gotten around to cosmetics yet, since
there are still so many actual issues to address :-)
> (4) Can please you fix up the whitespace between "MIN" and "(", plus
> also indent ArmGetPhysAddrTop() so that it matches the edk2 coding style?
>
Re space before ), ok
Re indent: shouldn't the line wrapper arguments be indented two spaces
wrt to the function they belong to?
> (5) We already set PcdPrePiCpuMemorySize in
> "ArmVirtPkg/ArmVirt.dsc.inc", namely to 40, from SVN r18428. (It is only
> done for 32-bit.) That has two consequences:
>
> (5a) In patch #2, instead of the change being currently proposed, can we
> simply make the setting independent of ARM, and let it take effect also
> for AARCH64?
>
>From an editorial pov, that makes sense, of course, but the motivation
for their presence is different:
- for AARCH64, it is an optimization, since it reduces wasted memory
on page tables when we know we are running on KVM
- for ARM, the limit is increased from the default value of 32 to 40,
since the virtualization extensions on ARM imply support for LPAE
> (That could even take care of the Xen case -- see BuildCpuHob() in
> "ArmVirtPkg/PrePi/PrePi.c".)
>
I think Xen may decide to use a higher value than 40. I asked Ian
informally a while ago, and he said the PArange value should be
considered authoritative
> (5b) The other consequence is that the above change would lead to
> undefined behavior in the 32-bit (=ARM) QEMU build. Namely, 1UL
> ("unsigned long") has 32 bits there, and shifting it left by 40 bits is
> undefined behavior (C89 6.3.7 Bitwise shift operators, first paragraph
> under Semantics). Therefore please write 1ULL instead.
>
Very good point. I will change that.
> (6) The default value of PcdPrePiCpuMemorySize (from
> "EmbeddedPkg/EmbeddedPkg.dec") for AARCH64 is 48. Therefore I feel that
> the patches should be reordered: first we should set the PCD to 40 (see
> 5a for "how"), and then let's apply this change as second step.
>
> I don't feel strongly about this one, so feel free to disagree. :)
>
I will reorder them since I couldn't care less about in which order
they are committed :-)
>> VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>>
>> // Remap the FD region as normal executable memory
>>
>
> This change is a great idea BTW; thank you.
>
Cheers,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
