Re: [PATCH] ArmVirtPkg: clear PcdPerformanceLibraryPropertyMask PCD

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 24 February 2017 at 23:17, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/24/17 19:16, Ard Biesheuvel wrote:
>> The only observeable effect of having PcdPerformanceLibraryPropertyMask
>> set to 1 is that a EfiReservedMemory region of 4 pages is allocated right
>> below the 4 GB mark. This region is out of bounds for the OS, which means
>> it is not even allowed to map it, to avoid speculative loads from it.
>>
>> On Linux, this may prevent the kernel from using a 1 GB block mappings for
>> this region, and instead it has to carve up the block as follows:
>>
>>   0xffffffff80000000-0xffffffffbe000000         992M PMD CON BLK
>>   0xffffffffbe000000-0xffffffffbfe00000          30M PMD     BLK
>>   0xffffffffbfe00000-0xffffffffbfff0000        1984K PTE CON
>>   0xffffffffbfff0000-0xffffffffbfffc000          48K PTE
>>
>> where it would otherwise use a single 1 GB mapping (*), i.e.,
>>
>>   0xffffffff80000000-0xffffffffc0000000           1G PGD
>>
>> To clarify, the latter is a single 8 byte entry in the top level page
>> table, whereas in the former case, we have two additional levels of
>> paging, requiring two extra 4 KB pages (on a 4 KB pagesize kernel).
>>
>> The real cost, however, is the TLB footprint, which goes up from a
>> single entry to a value between 90 and 1020, depending on whether
>> contiguous hints are honoured by the hardware.
>>
>> So let's set PcdPerformanceLibraryPropertyMask to zero until we find
>> a reason why we shouldn't.
>>
>> (*) provided that no other allocations were deliberately located right
>>     below the 4 GB mark, and that we are running with more than 3 GB of
>>     memory, in which case most allocations will be over 4 GB, given EDK2's
>>     default top-down allocation policy.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/ArmVirt.dsc.inc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index 61d4a6642eb7..ca0a4d31a03d 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -295,7 +295,7 @@ [PcdsFixedAtBuild.common]
>>    gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|1000000
>>    gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout|10000000
>>    gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue|0xAF
>> -  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|1
>> +  gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0
>>    gEfiMdePkgTokenSpaceGuid.PcdPostCodePropertyMask|0
>>    gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|320
>>
>>
>
> The value 1 dates back to ancient commit 6f5872b1f401 ("ArmPlatformPkg/ArmVirtualizationPkg: Add ArmVirtualizationQemu platform", 2014-09-18).
>
> The default is zero in "MdePkg/MdePkg.dec":
>
>   ## The mask is used to control PerformanceLib behavior.<BR><BR>
>   #  BIT0 - Enable Performance Measurement.<BR>
>   # @Prompt Performance Measurement Property.
>   # @Expression  0x80000002 | (gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask & 0xFE) == 0
>   gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask|0|UINT8|0x00000009
>
> Shouldn't we just remove the line?
>

Sure, even better!