Re: [PATCH] ArmVirtPkg: clear PcdPerformanceLibraryPropertyMask PCD

[Laszlo Ersek <lersek@redhat.com>]

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?

Thanks
Laszlo