Re: [PATCH 5/5] ArmVirtPkg: revert PcdPrePiCpuMemorySize to is default value of 48

[Laszlo Ersek <lersek@redhat.com>]

(1) s/is/its/ in $SUBJECT, please.

On 11/23/18 13:14, Ard Biesheuvel wrote:
> Drop the PcdPrePiCpuMemorySize definitions that limit it to 40
> bits on AArch64 targets.

Indeed, after this series is applied, we still have

[PcdsFixedAtBuild.ARM]
  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40

left, in "ArmVirt.dsc.inc". Therefore, highlighting AArch64 in the above
sentence seems justified.

(2) Now, I'm noticing that the DEC default for ARM (which we continue
overriding as 40) is not 48 however, but 32. Can you please remind me
why we do that?

This is BTW not just for my own education -- the subject seems to imply
that we "generally" revert the PCD to its DEC default, *and* that the
DEC default is 48. These are two statements, and for ARM, they are both
false.

If you agree, can you please stick AArch64 somewhere in the subject?

> This is no longer appropriate now that
> KVM has been enhanced to permit any IPA space size permitted by
> the architecture. This means the value will revert back to its
> default of 48.

OK, so just to repeat my general question and documentation request,
regarding EmbeddedPkg.dec / PcdPrePiCpuMemorySize, in this specific context:

what *remains* affected by PcdPrePiCpuMemorySize, in ArmVirtPkg platforms?

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc       | 4 ----
>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ----
>  2 files changed, 8 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index cb59c790afcc..42f2adce80e6 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -143,10 +143,6 @@
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>  
>  [PcdsFixedAtBuild.AARCH64]
> -  # KVM limits it IPA space to 40 bits (1 TB), so there is no need to
> -  # support anything bigger, even if the host hardware does
> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
> -
>    # Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point,
>    # if the entry point version is >= 3.0. AARCH64 OSes cannot assume the
>    # presence of the 32-bit entry point anyway (because many AARCH64 systems
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 434d6861a56f..d8fbf14e8f4e 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -157,10 +157,6 @@
>    #
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>  
> -  # KVM limits it IPA space to 40 bits (1 TB), so there is no need to
> -  # support anything bigger, even if the host hardware does
> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
> -
>  [PcdsDynamicDefault.common]
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>  
> 

I'm sorry if my review comments / questions are a mess: I haven't looked
at this in ages.

Thanks!
Laszlo