[edk2,5/5] ArmVirtPkg: revert PcdPrePiCpuMemorySize to is default value of 48

Message ID 20181123121431.22353-6-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit
Related show

Commit Message

Ard Biesheuvel Nov. 23, 2018, 12:14 p.m.
Drop the PcdPrePiCpuMemorySize definitions that limit it to 40
bits on AArch64 targets. 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.

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(-)

-- 
2.17.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel Nov. 23, 2018, 1:45 p.m. | #1
On Fri, 23 Nov 2018 at 14:36, Andrew Jones <drjones@redhat.com> wrote:
>

> On Fri, Nov 23, 2018 at 01:14:31PM +0100, Ard Biesheuvel wrote:

> > Drop the PcdPrePiCpuMemorySize definitions that limit it to 40

> > bits on AArch64 targets. 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.

>

> If we're running on older KVM, then, since KVM just passes through

> the host value of id_aa64mmfr0_el1, firmware will see whatever

> the host supports and use that (I'm not sure if the 48-bit default

> ever can come into play too). In either case, it probably doesn't

> matter, because just like the guest kernel works today on older

> KVM, as long as nothing is placed above 40 bits there isn't any

> problem. Is that the case for edk2 too?

>


The value of 48 serves as a limit now, which makes sense given that
52-bit requires 64k pages, which we don't support.

But as I said, it might make sense to permit the GCD space to describe
that much, which is actually a nice side effect of the previous patch,
which takes the value directly from the CPU system register on virt
targets.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 26, 2018, 10:58 a.m. | #2
(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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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