[PATCH 0/5] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit

[PATCH 0/5] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit

[Ard Biesheuvel]

The ArmVirtQemu targets currently limit the size of the IPA space to
40 bits because that is all what KVM supports. However, this is about
to change, and so we need to update the code if we want to ensure that
our UEFI firmware builds can keep running on systems that set values
other than 40 (which could be > 40 or < 40)

So add a helper to ArmLib to read the number of supported address bits (#1)
and take this into account in the page table code (#2), which allows
PcdPrePiCpuMemorySize to assume a value that exceeds the capabilities of
the CPU.

Patch #3 is mostly a cleanup patch, to switch to the new helper added in
patch #1. No functional changes intended.

Patch #4 builds the CPU hob (and thus declares the size of the GCD memory
space) based on the CPU capabilities rather than the value of 
PcdPrePiCpuMemorySize, to prevent any potential regressions in memory
utilization when we bump PcdPrePiCpuMemorySize back to 48.

Patch #5 drops the definitions of PcdPrePiCpuMemorySize, reverting its
value back to the default 48.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Philippe Mathieu-Daude <philmd@redhat.com>
Cc: Julien Grall <julien.grall@linaro.org>

Ard Biesheuvel (5):
  ArmPkg/ArmLib: add support for reading the max physical address space
    size
  ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account
  ArmVirtPkg: refactor reading of the physical address space size
  ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD
    space
  ArmVirtPkg: revert PcdPrePiCpuMemorySize to is default value of 48

 ArmPkg/Include/Library/ArmLib.h               |  6 +++
 ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++
 ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++
 .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  5 ++-
 ArmVirtPkg/ArmVirtQemu.dsc                    |  5 ---
 ArmVirtPkg/ArmVirtQemu.fdf                    |  1 -
 ArmVirtPkg/ArmVirtQemuKernel.dsc              |  4 --
 .../Include/Library/ArmVirtMemInfoLib.h       |  1 +
 .../ArmVirtMemoryInitPeiLib.c                 |  7 +++-
 .../ArmVirtMemoryInitPeiLib.inf               |  1 +
 .../QemuVirtMemInfoLib/AArch64/PhysAddrTop.S  | 39 -------------------
 .../QemuVirtMemInfoLib/Arm/PhysAddrTop.S      | 24 ------------
 .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   |  6 +--
 .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  7 ----
 .../QemuVirtMemInfoPeiLib.inf                 |  7 ----
 .../XenVirtMemInfoLib/AArch64/PhysAddrTop.S   | 39 -------------------
 .../XenVirtMemInfoLib/Arm/PhysAddrTop.S       | 24 ------------
 .../XenVirtMemInfoLib/XenVirtMemInfoLib.c     |  8 +---
 .../XenVirtMemInfoLib/XenVirtMemInfoLib.inf   |  6 ---
 .../PrePi/ArmVirtPrePiUniCoreRelocatable.inf  |  3 --
 ArmVirtPkg/PrePi/PrePi.c                      |  3 --
 21 files changed, 46 insertions(+), 174 deletions(-)
 delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
 delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
 delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
 delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S

-- 
2.17.1

[PATCH 1/5] ArmPkg/ArmLib: add support for reading the max physical address space size

[Ard Biesheuvel]

Add a helper function that returns the maximum physical address space
size as supported by the current CPU.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Include/Library/ArmLib.h               |  6 ++++++
 ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++++++++++
 ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++++++
 3 files changed, 30 insertions(+)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index ffda50e9d767..9a804c15fdb6 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -733,4 +733,10 @@ ArmWriteCntvOff (
   UINT64   Val
   );
 
+UINTN
+EFIAPI
+ArmGetPhysicalAddressBits (
+  VOID
+  );
+
 #endif // __ARM_LIB__
diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
index 1ef2f61f5979..75ab8dade485 100644
--- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
+++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
@@ -196,4 +196,20 @@ ASM_FUNC(ArmWriteSctlr)
 3:msr   sctlr_el3, x0
 4:ret
 
+ASM_FUNC(ArmGetPhysicalAddressBits)
+  mrs   x0, id_aa64mmfr0_el1
+  adr   x1, .LPARanges
+  and   x0, x0, #7
+  ldrb  w0, [x1, x0]
+  ret
+
+//
+// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
+// physical address space support on this CPU:
+// 0 == 32 bits, 1 == 36 bits, etc etc
+// 6 and 7 are reserved
+//
+.LPARanges:
+  .byte 32, 36, 40, 42, 44, 48, -1, -1
+
 ASM_FUNCTION_REMOVE_IF_UNREFERENCED
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
index f2a517671f0a..f2f3c9a25991 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
+++ b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
@@ -165,4 +165,12 @@ ASM_FUNC(ArmWriteCpuActlr)
   isb
   bx      lr
 
+ASM_FUNC (ArmGetPhysicalAddressBits)
+  mrc     p15, 0, r0, c0, c1, 4   // MMFR0
+  and     r0, r0, #0xf            // VMSA [3:0]
+  cmp     r0, #5                  // >5 implies LPAE support
+  movlt   r0, #32                 // 32 bits if no LPAE
+  movge   r0, #40                 // 40 bits if LPAE
+  bx      lr
+
 ASM_FUNCTION_REMOVE_IF_UNREFERENCED
-- 
2.17.1

Re: [PATCH 1/5] ArmPkg/ArmLib: add support for reading the max physical address space size

[Ard Biesheuvel]

On Fri, 23 Nov 2018 at 14:16, Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Nov 23, 2018 at 01:14:27PM +0100, Ard Biesheuvel wrote:
> > Add a helper function that returns the maximum physical address space
> > size as supported by the current CPU.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmPkg/Include/Library/ArmLib.h               |  6 ++++++
> >  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++++++++++
> >  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++++++
> >  3 files changed, 30 insertions(+)
> >
> > diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> > index ffda50e9d767..9a804c15fdb6 100644
> > --- a/ArmPkg/Include/Library/ArmLib.h
> > +++ b/ArmPkg/Include/Library/ArmLib.h
> > @@ -733,4 +733,10 @@ ArmWriteCntvOff (
> >    UINT64   Val
> >    );
> >
> > +UINTN
> > +EFIAPI
> > +ArmGetPhysicalAddressBits (
> > +  VOID
> > +  );
> > +
> >  #endif // __ARM_LIB__
> > diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> > index 1ef2f61f5979..75ab8dade485 100644
> > --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> > +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> > @@ -196,4 +196,20 @@ ASM_FUNC(ArmWriteSctlr)
> >  3:msr   sctlr_el3, x0
> >  4:ret
> >
> > +ASM_FUNC(ArmGetPhysicalAddressBits)
> > +  mrs   x0, id_aa64mmfr0_el1
> > +  adr   x1, .LPARanges
> > +  and   x0, x0, #7
> > +  ldrb  w0, [x1, x0]
> > +  ret
> > +
> > +//
> > +// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> > +// physical address space support on this CPU:
> > +// 0 == 32 bits, 1 == 36 bits, etc etc
> > +// 6 and 7 are reserved
> > +//
> > +.LPARanges:
> > +  .byte 32, 36, 40, 42, 44, 48, -1, -1
>
> Hi Ard,
>
> One of the things I was wondering is how much it matters what the
> firmware's opinion of highest physical address is vs. the guest
> kernel. Do they need to match? This patch series implies they do,
> or at least that 40-bits won't always be sufficient for firmware.

Yes. The size of the GCD space limits how much memory we can report as
present to the OS. So it only matters if there is DRAM there.

> However, guests using 64k pages running on supporting hardware can
> use 52-bits. Considering ArmVirtPkg only uses 4k pages, that's not
> an option for it, and that justifies not defining index 6 == 52 in
> the above array, but will that also restrict the guest?
>

At the moment, yes. UEFI support for 52-bit/64k pages is still under
discussion, and is presently not supported.


> > diff --git a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
> > index f2a517671f0a..f2f3c9a25991 100644
> > --- a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
> > +++ b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
> > @@ -165,4 +165,12 @@ ASM_FUNC(ArmWriteCpuActlr)
> >    isb
> >    bx      lr
> >
> > +ASM_FUNC (ArmGetPhysicalAddressBits)
> > +  mrc     p15, 0, r0, c0, c1, 4   // MMFR0
> > +  and     r0, r0, #0xf            // VMSA [3:0]
> > +  cmp     r0, #5                  // >5 implies LPAE support
> > +  movlt   r0, #32                 // 32 bits if no LPAE
> > +  movge   r0, #40                 // 40 bits if LPAE
> > +  bx      lr
> > +
> >  ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> > --
> > 2.17.1
> >

Re: [PATCH 1/5] ArmPkg/ArmLib: add support for reading the max physical address space size

[Ard Biesheuvel]

On Fri, 23 Nov 2018 at 14:20, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 23 Nov 2018 at 14:16, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Fri, Nov 23, 2018 at 01:14:27PM +0100, Ard Biesheuvel wrote:
> > > Add a helper function that returns the maximum physical address space
> > > size as supported by the current CPU.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  ArmPkg/Include/Library/ArmLib.h               |  6 ++++++
> > >  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++++++++++
> > >  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++++++
> > >  3 files changed, 30 insertions(+)
> > >
> > > diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> > > index ffda50e9d767..9a804c15fdb6 100644
> > > --- a/ArmPkg/Include/Library/ArmLib.h
> > > +++ b/ArmPkg/Include/Library/ArmLib.h
> > > @@ -733,4 +733,10 @@ ArmWriteCntvOff (
> > >    UINT64   Val
> > >    );
> > >
> > > +UINTN
> > > +EFIAPI
> > > +ArmGetPhysicalAddressBits (
> > > +  VOID
> > > +  );
> > > +
> > >  #endif // __ARM_LIB__
> > > diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> > > index 1ef2f61f5979..75ab8dade485 100644
> > > --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> > > +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> > > @@ -196,4 +196,20 @@ ASM_FUNC(ArmWriteSctlr)
> > >  3:msr   sctlr_el3, x0
> > >  4:ret
> > >
> > > +ASM_FUNC(ArmGetPhysicalAddressBits)
> > > +  mrs   x0, id_aa64mmfr0_el1
> > > +  adr   x1, .LPARanges
> > > +  and   x0, x0, #7
> > > +  ldrb  w0, [x1, x0]
> > > +  ret
> > > +
> > > +//
> > > +// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> > > +// physical address space support on this CPU:
> > > +// 0 == 32 bits, 1 == 36 bits, etc etc
> > > +// 6 and 7 are reserved
> > > +//
> > > +.LPARanges:
> > > +  .byte 32, 36, 40, 42, 44, 48, -1, -1
> >
> > Hi Ard,
> >
> > One of the things I was wondering is how much it matters what the
> > firmware's opinion of highest physical address is vs. the guest
> > kernel. Do they need to match? This patch series implies they do,
> > or at least that 40-bits won't always be sufficient for firmware.
>
> Yes. The size of the GCD space limits how much memory we can report as
> present to the OS. So it only matters if there is DRAM there.
>
> > However, guests using 64k pages running on supporting hardware can
> > use 52-bits. Considering ArmVirtPkg only uses 4k pages, that's not
> > an option for it, and that justifies not defining index 6 == 52 in
> > the above array, but will that also restrict the guest?
> >
>
> At the moment, yes. UEFI support for 52-bit/64k pages is still under
> discussion, and is presently not supported.
>

... which btw doesn't mean we can't report that much memory in the GCD
memory map, we just can't map it in UEFI.

There were some discussions about how to proceed here, since there
appear to be some SoC vendors that want to use bit 51 to distinguish
between DRAM and MMIO regions, which implies that we have to support
it to be able to boot such systems (and the architecture does not
forbid or discourage the practice)

Re: [PATCH 1/5] ArmPkg/ArmLib: add support for reading the max physical address space size

[Laszlo Ersek]

meta:

On 11/23/18 14:20, Ard Biesheuvel wrote:
> On Fri, 23 Nov 2018 at 14:16, Andrew Jones <drjones@redhat.com> wrote:
>>
>> [...]

Drew, Eric, please subscribe to edk2-devel at

  https://lists.01.org/mailman/listinfo/edk2-devel

otherwise, the list will drop your messages.

This is a completely unacceptable requirement for posting; I apologize
for it. For a while it looked like we had replaced it with manual
white-listing / moderation
<https://bugzilla.tianocore.org/show_bug.cgi?id=451>, but then the
policy was reverted to the original (broken) one, as a (misguided)
response to a spam attack. (And to how Red Hat's mail infrastructure
reacted, when those spam moderation requests were delivered to my mailbox.)

So, for now, please subscribe. If you don't intend to receive edk2-devel
messages, you can still stay subscribed: after your subscription
completes, go to the same URL as above, click "Unsubscribe or edit
options", and then disable mail delivery.

Thanks,
Laszlo

Re: [PATCH 1/5] ArmPkg/ArmLib: add support for reading the max physical address space size

[Ard Biesheuvel]

On Fri, 23 Nov 2018 at 13:14, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> Add a helper function that returns the maximum physical address space
> size as supported by the current CPU.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Include/Library/ArmLib.h               |  6 ++++++
>  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++++++++++
>  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++++++
>  3 files changed, 30 insertions(+)
>
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index ffda50e9d767..9a804c15fdb6 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -733,4 +733,10 @@ ArmWriteCntvOff (
>    UINT64   Val
>    );
>
> +UINTN
> +EFIAPI
> +ArmGetPhysicalAddressBits (
> +  VOID
> +  );
> +
>  #endif // __ARM_LIB__
> diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> index 1ef2f61f5979..75ab8dade485 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
> @@ -196,4 +196,20 @@ ASM_FUNC(ArmWriteSctlr)
>  3:msr   sctlr_el3, x0
>  4:ret
>
> +ASM_FUNC(ArmGetPhysicalAddressBits)
> +  mrs   x0, id_aa64mmfr0_el1
> +  adr   x1, .LPARanges
> +  and   x0, x0, #7
> +  ldrb  w0, [x1, x0]
> +  ret
> +
> +//
> +// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> +// physical address space support on this CPU:
> +// 0 == 32 bits, 1 == 36 bits, etc etc
> +// 6 and 7 are reserved
> +//
> +.LPARanges:
> +  .byte 32, 36, 40, 42, 44, 48, -1, -1
> +

Note: as Drew pointed out, we want 52 bits included as well in this
enumeration. I will fix that up when applying (unless anyone objects)

>  ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
> index f2a517671f0a..f2f3c9a25991 100644
> --- a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
> +++ b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
> @@ -165,4 +165,12 @@ ASM_FUNC(ArmWriteCpuActlr)
>    isb
>    bx      lr
>
> +ASM_FUNC (ArmGetPhysicalAddressBits)
> +  mrc     p15, 0, r0, c0, c1, 4   // MMFR0
> +  and     r0, r0, #0xf            // VMSA [3:0]
> +  cmp     r0, #5                  // >5 implies LPAE support
> +  movlt   r0, #32                 // 32 bits if no LPAE
> +  movge   r0, #40                 // 40 bits if LPAE
> +  bx      lr
> +
>  ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> --
> 2.17.1
>

Re: [PATCH 1/5] ArmPkg/ArmLib: add support for reading the max physical address space size

[Philippe Mathieu-Daudé]

On 26/11/18 12:46, Ard Biesheuvel wrote:
> On Fri, 23 Nov 2018 at 13:14, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> Add a helper function that returns the maximum physical address space
>> size as supported by the current CPU.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Include/Library/ArmLib.h               |  6 ++++++
>>  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++++++++++
>>  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++++++
>>  3 files changed, 30 insertions(+)
>>
>> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
>> index ffda50e9d767..9a804c15fdb6 100644
>> --- a/ArmPkg/Include/Library/ArmLib.h
>> +++ b/ArmPkg/Include/Library/ArmLib.h
>> @@ -733,4 +733,10 @@ ArmWriteCntvOff (
>>    UINT64   Val
>>    );
>>
>> +UINTN
>> +EFIAPI
>> +ArmGetPhysicalAddressBits (
>> +  VOID
>> +  );
>> +
>>  #endif // __ARM_LIB__
>> diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
>> index 1ef2f61f5979..75ab8dade485 100644
>> --- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
>> +++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
>> @@ -196,4 +196,20 @@ ASM_FUNC(ArmWriteSctlr)
>>  3:msr   sctlr_el3, x0
>>  4:ret
>>
>> +ASM_FUNC(ArmGetPhysicalAddressBits)
>> +  mrs   x0, id_aa64mmfr0_el1
>> +  adr   x1, .LPARanges
>> +  and   x0, x0, #7
>> +  ldrb  w0, [x1, x0]
>> +  ret
>> +
>> +//
>> +// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
>> +// physical address space support on this CPU:
>> +// 0 == 32 bits, 1 == 36 bits, etc etc
>> +// 6 and 7 are reserved
>> +//
>> +.LPARanges:
>> +  .byte 32, 36, 40, 42, 44, 48, -1, -1
>> +
> 
> Note: as Drew pointed out, we want 52 bits included as well in this
> enumeration. I will fix that up when applying (unless anyone objects)

Using index 6 == 52 and updating the comment:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>  ASM_FUNCTION_REMOVE_IF_UNREFERENCED
>> diff --git a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
>> index f2a517671f0a..f2f3c9a25991 100644
>> --- a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
>> +++ b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
>> @@ -165,4 +165,12 @@ ASM_FUNC(ArmWriteCpuActlr)
>>    isb
>>    bx      lr
>>
>> +ASM_FUNC (ArmGetPhysicalAddressBits)
>> +  mrc     p15, 0, r0, c0, c1, 4   // MMFR0
>> +  and     r0, r0, #0xf            // VMSA [3:0]
>> +  cmp     r0, #5                  // >5 implies LPAE support
>> +  movlt   r0, #32                 // 32 bits if no LPAE
>> +  movge   r0, #40                 // 40 bits if LPAE
>> +  bx      lr
>> +
>>  ASM_FUNCTION_REMOVE_IF_UNREFERENCED
>> --
>> 2.17.1
>>

[PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Ard Biesheuvel]

In preparation of permitting the virt code to define a larger PA space
size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
CPU actually supports, take the CPU's capabilities into account when
setting up the page tables. This is necessary because KVM will shortly
support variable PA space sizes, and to support running the same UEFI
binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
treated as an upper bound rather than a fixed size.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 4b62ecb6a476..a4fde9b59383 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -593,6 +593,7 @@ ArmConfigureMmu (
 {
   VOID*                         TranslationTable;
   UINT32                        TranslationTableAttribute;
+  UINTN                         MaxAddressBits;
   UINT64                        MaxAddress;
   UINTN                         T0SZ;
   UINTN                         RootTableEntryCount;
@@ -605,7 +606,9 @@ ArmConfigureMmu (
   }
 
   // Cover the entire GCD memory space
-  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
+  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),
+                        PcdGet8 (PcdPrePiCpuMemorySize));
+  MaxAddress = (1UL << MaxAddressBits) - 1;
 
   // Lookup the Table Level to get the information
   LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
-- 
2.17.1

Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Laszlo Ersek]

On 11/23/18 13:14, Ard Biesheuvel wrote:
> In preparation of permitting the virt code to define a larger PA space
> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
> CPU actually supports, take the CPU's capabilities into account when
> setting up the page tables. This is necessary because KVM will shortly
> support variable PA space sizes, and to support running the same UEFI
> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
> treated as an upper bound rather than a fixed size.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 4b62ecb6a476..a4fde9b59383 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -593,6 +593,7 @@ ArmConfigureMmu (
>  {
>    VOID*                         TranslationTable;
>    UINT32                        TranslationTableAttribute;
> +  UINTN                         MaxAddressBits;
>    UINT64                        MaxAddress;
>    UINTN                         T0SZ;
>    UINTN                         RootTableEntryCount;
> @@ -605,7 +606,9 @@ ArmConfigureMmu (
>    }
>  
>    // Cover the entire GCD memory space
> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
> +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),
> +                        PcdGet8 (PcdPrePiCpuMemorySize));

(1) Superficial comment: sticking to the letter of MIN() in
"MdePkg/Include/Base.h", the arguments should be of the exact same type.
Currently they aren't. (It's a different question whether that
requirement makes any sense for MIN().)

(2) Why does it make sense to use MIN() here? Why not just disregard
PcdPrePiCpuMemorySize and map what the CPU is capable of? (This ties in
with my lack of understanding of PcdPrePiCpuMemorySize.)

I mean, not going above ArmGetPhysicalAddressBits() makes total sense.
What's the point of staying below it though? And if so, how much exactly
do we want to stay below it? (I.e., how is a platform supposed to set
PcdPrePiCpuMemorySize, in order to clamp down ArmGetPhysicalAddressBits()?)

> +  MaxAddress = (1UL << MaxAddressBits) - 1;

(3) I understand this just reworks the original code, but the original
code isn't stellar. If we are left-shifting a UINT32 or UINTN value,
then the result is the same, and the << operator is OK. However:

- Here we intend to accommodate a UINT64 result, judged from the type of
MaxAddress (UINT64).

- For that, we should likely left-shift 1ULL, not 1 U;, which in turn
requires LShiftU64() from BaseLib.

(If we indeed want to use UINTN, then I think we should change the type
of MaxAddress, plus write "(UINTN)1", not 1UL.)

>  
>    // Lookup the Table Level to get the information
>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
> 

Thanks,
Laszlo

Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Laszlo Ersek]

On 11/26/18 10:42, Laszlo Ersek wrote:
> On 11/23/18 13:14, Ard Biesheuvel wrote:

>> +  MaxAddress = (1UL << MaxAddressBits) - 1;

> not 1 U;, which

Sorry, typo: s/;/L/, clearly. (They are adjacent in my keyboard layout.)

Laszlo

Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Laszlo Ersek]

On 11/26/18 10:42, Laszlo Ersek wrote:
> On 11/23/18 13:14, Ard Biesheuvel wrote:
>> In preparation of permitting the virt code to define a larger PA space
>> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
>> CPU actually supports, take the CPU's capabilities into account when
>> setting up the page tables. This is necessary because KVM will shortly
>> support variable PA space sizes, and to support running the same UEFI
>> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
>> treated as an upper bound rather than a fixed size.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> index 4b62ecb6a476..a4fde9b59383 100644
>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> @@ -593,6 +593,7 @@ ArmConfigureMmu (
>>  {
>>    VOID*                         TranslationTable;
>>    UINT32                        TranslationTableAttribute;
>> +  UINTN                         MaxAddressBits;
>>    UINT64                        MaxAddress;
>>    UINTN                         T0SZ;
>>    UINTN                         RootTableEntryCount;
>> @@ -605,7 +606,9 @@ ArmConfigureMmu (
>>    }
>>  
>>    // Cover the entire GCD memory space
>> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
>> +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),
>> +                        PcdGet8 (PcdPrePiCpuMemorySize));
> 
> (1) Superficial comment: sticking to the letter of MIN() in
> "MdePkg/Include/Base.h", the arguments should be of the exact same type.
> Currently they aren't. (It's a different question whether that
> requirement makes any sense for MIN().)
> 
> (2) Why does it make sense to use MIN() here? Why not just disregard
> PcdPrePiCpuMemorySize and map what the CPU is capable of? (This ties in
> with my lack of understanding of PcdPrePiCpuMemorySize.)
> 
> I mean, not going above ArmGetPhysicalAddressBits() makes total sense.
> What's the point of staying below it though? And if so, how much exactly
> do we want to stay below it? (I.e., how is a platform supposed to set
> PcdPrePiCpuMemorySize, in order to clamp down ArmGetPhysicalAddressBits()?)

Ugh, okay. (Maybe you've responded already, I'm still in write-only
mode, sorry.) I think I'm getting the idea here. This is just being done
to keep the series bisectable / regression-free in the middle too. Right?

Thanks
Laszlo

> 
>> +  MaxAddress = (1UL << MaxAddressBits) - 1;
> 
> (3) I understand this just reworks the original code, but the original
> code isn't stellar. If we are left-shifting a UINT32 or UINTN value,
> then the result is the same, and the << operator is OK. However:
> 
> - Here we intend to accommodate a UINT64 result, judged from the type of
> MaxAddress (UINT64).
> 
> - For that, we should likely left-shift 1ULL, not 1 U;, which in turn
> requires LShiftU64() from BaseLib.
> 
> (If we indeed want to use UINTN, then I think we should change the type
> of MaxAddress, plus write "(UINTN)1", not 1UL.)
> 
>>  
>>    // Lookup the Table Level to get the information
>>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
>>
> 
> Thanks,
> Laszlo
> 

Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Ard Biesheuvel]

On Mon, 26 Nov 2018 at 10:42, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/23/18 13:14, Ard Biesheuvel wrote:
> > In preparation of permitting the virt code to define a larger PA space
> > size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
> > CPU actually supports, take the CPU's capabilities into account when
> > setting up the page tables. This is necessary because KVM will shortly
> > support variable PA space sizes, and to support running the same UEFI
> > binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
> > treated as an upper bound rather than a fixed size.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > index 4b62ecb6a476..a4fde9b59383 100644
> > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > @@ -593,6 +593,7 @@ ArmConfigureMmu (
> >  {
> >    VOID*                         TranslationTable;
> >    UINT32                        TranslationTableAttribute;
> > +  UINTN                         MaxAddressBits;
> >    UINT64                        MaxAddress;
> >    UINTN                         T0SZ;
> >    UINTN                         RootTableEntryCount;
> > @@ -605,7 +606,9 @@ ArmConfigureMmu (
> >    }
> >
> >    // Cover the entire GCD memory space
> > -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
> > +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),
> > +                        PcdGet8 (PcdPrePiCpuMemorySize));
>
> (1) Superficial comment: sticking to the letter of MIN() in
> "MdePkg/Include/Base.h", the arguments should be of the exact same type.
> Currently they aren't. (It's a different question whether that
> requirement makes any sense for MIN().)
>

OK

> (2) Why does it make sense to use MIN() here? Why not just disregard
> PcdPrePiCpuMemorySize and map what the CPU is capable of? (This ties in
> with my lack of understanding of PcdPrePiCpuMemorySize.)
>

PcdPrePiCpuMemorySize defines the size of the GCD memory space.
PcdPrePiCpuIoSize defines the size of the GCD I/O space.

However, we infer other quantities from it as well:
- Up until now, it never made sense to make the GCD space larger than
what the CPU is able to address, so we used it as an upper bound for
the page table code;
- On a given platform, the physical address space may not be entirely
populated, and fewer PA bits means fewer levels of page tables, and so
less memory used *. This means it might make sense to reduce
PcdPrePiCpuMemorySize to a lower value than what the h/w supports.

* this was briefly a concern when some (all?) levels of page tables
were allocated from temporary RAM but that broke some platforms IIRC
so that was reverted.

> I mean, not going above ArmGetPhysicalAddressBits() makes total sense.
> What's the point of staying below it though? And if so, how much exactly
> do we want to stay below it? (I.e., how is a platform supposed to set
> PcdPrePiCpuMemorySize, in order to clamp down ArmGetPhysicalAddressBits()?)
>

As explained above, staying below it gives you less overhead in the
GCD memory map and the page tables.

> > +  MaxAddress = (1UL << MaxAddressBits) - 1;
>
> (3) I understand this just reworks the original code, but the original
> code isn't stellar. If we are left-shifting a UINT32 or UINTN value,
> then the result is the same, and the << operator is OK. However:
>
> - Here we intend to accommodate a UINT64 result, judged from the type of
> MaxAddress (UINT64).
>
> - For that, we should likely left-shift 1ULL, not 1 U;, which in turn
> requires LShiftU64() from BaseLib.
>

This code currently only executes on AArch64, but for correctness (and
in case we ever use this code for SMMUs on 32-bit ARM), it should
indeed be fixed.

> (If we indeed want to use UINTN, then I think we should change the type
> of MaxAddress, plus write "(UINTN)1", not 1UL.)
>
> >
> >    // Lookup the Table Level to get the information
> >    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
> >
>
> Thanks,
> Laszlo

Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Leif Lindholm]

On Fri, Nov 23, 2018 at 01:14:28PM +0100, Ard Biesheuvel wrote:
> In preparation of permitting the virt code to define a larger PA space
> size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
> CPU actually supports, take the CPU's capabilities into account when
> setting up the page tables. This is necessary because KVM will shortly
> support variable PA space sizes, and to support running the same UEFI
> binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
> treated as an upper bound rather than a fixed size.

Why do we keep PcdPrePiCpuMemorySize at all? (I.e., rather than just
using the probed value?
Mainly for the purpose of being able to restrict ourselves to 32/48
bits?

If we keep it, should we rename
PcdPrePiCpuMemorySize -> PcdPrePiCpuMemoryBits
and
PcdPrePiCpuIoSize -> PcdPrePiCpuIoBits
?

Argument against this would be that later consumers still refer to
the value extracted from the HOB as SizeOf*Space, and happily shifts
1s around by it :|

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 4b62ecb6a476..a4fde9b59383 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -593,6 +593,7 @@ ArmConfigureMmu (
>  {
>    VOID*                         TranslationTable;
>    UINT32                        TranslationTableAttribute;
> +  UINTN                         MaxAddressBits;
>    UINT64                        MaxAddress;
>    UINTN                         T0SZ;
>    UINTN                         RootTableEntryCount;
> @@ -605,7 +606,9 @@ ArmConfigureMmu (
>    }
>  
>    // Cover the entire GCD memory space
> -  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
> +  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (),
> +                        PcdGet8 (PcdPrePiCpuMemorySize));
> +  MaxAddress = (1UL << MaxAddressBits) - 1;
>  
>    // Lookup the Table Level to get the information
>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
> -- 
> 2.17.1
> 

Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Ard Biesheuvel]

On Mon, 26 Nov 2018 at 18:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Fri, Nov 23, 2018 at 01:14:28PM +0100, Ard Biesheuvel wrote:
> > In preparation of permitting the virt code to define a larger PA space
> > size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
> > CPU actually supports, take the CPU's capabilities into account when
> > setting up the page tables. This is necessary because KVM will shortly
> > support variable PA space sizes, and to support running the same UEFI
> > binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
> > treated as an upper bound rather than a fixed size.
>
> Why do we keep PcdPrePiCpuMemorySize at all? (I.e., rather than just
> using the probed value?
> Mainly for the purpose of being able to restrict ourselves to 32/48
> bits?
>
> If we keep it, should we rename
> PcdPrePiCpuMemorySize -> PcdPrePiCpuMemoryBits
> and
> PcdPrePiCpuIoSize -> PcdPrePiCpuIoBits
> ?
>
> Argument against this would be that later consumers still refer to
> the value extracted from the HOB as SizeOf*Space, and happily shifts
> 1s around by it :|
>

I am reworking this so PcdPrePiCpuMemorySize retains its meaning.
Instead, I will add something like this to ArmLib.h

//
// ARM_MMU_IDMAP_LIMIT defines the maximum size of the identity mapping
// that covers the entire address space when running in UEFI. This is limited
// to what can architecturally be mapped using a 4 KB granule, even if the
// hardware is capable of mapping more using larger pages.
//
#ifdef MDE_CPU_ARM
#define ARM_MMU_IDMAP_LIMIT     (MAX_UINT32)
#else
#define ARM_MMU_IDMAP_LIMIT     (1ULL << 48)
#endif

and use that in the MMU code as an upper bound in case the CPU supports 52 bits.

Re: [PATCH 2/5] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

[Leif Lindholm]

On Mon, Nov 26, 2018 at 06:50:09PM +0100, Ard Biesheuvel wrote:
> On Mon, 26 Nov 2018 at 18:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Fri, Nov 23, 2018 at 01:14:28PM +0100, Ard Biesheuvel wrote:
> > > In preparation of permitting the virt code to define a larger PA space
> > > size via gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize than what the
> > > CPU actually supports, take the CPU's capabilities into account when
> > > setting up the page tables. This is necessary because KVM will shortly
> > > support variable PA space sizes, and to support running the same UEFI
> > > binaries regardless of that limit, PcdPrePiCpuMemorySize needs to be
> > > treated as an upper bound rather than a fixed size.
> >
> > Why do we keep PcdPrePiCpuMemorySize at all? (I.e., rather than just
> > using the probed value?
> > Mainly for the purpose of being able to restrict ourselves to 32/48
> > bits?
> >
> > If we keep it, should we rename
> > PcdPrePiCpuMemorySize -> PcdPrePiCpuMemoryBits
> > and
> > PcdPrePiCpuIoSize -> PcdPrePiCpuIoBits
> > ?
> >
> > Argument against this would be that later consumers still refer to
> > the value extracted from the HOB as SizeOf*Space, and happily shifts
> > 1s around by it :|
> >
> 
> I am reworking this so PcdPrePiCpuMemorySize retains its meaning.
> Instead, I will add something like this to ArmLib.h
> 
> //
> // ARM_MMU_IDMAP_LIMIT defines the maximum size of the identity mapping
> // that covers the entire address space when running in UEFI. This is limited
> // to what can architecturally be mapped using a 4 KB granule, even if the
> // hardware is capable of mapping more using larger pages.
> //
> #ifdef MDE_CPU_ARM
> #define ARM_MMU_IDMAP_LIMIT     (MAX_UINT32)
> #else
> #define ARM_MMU_IDMAP_LIMIT     (1ULL << 48)
> #endif
> 
> and use that in the MMU code as an upper bound in case the CPU supports 52 bits.

OK, that works for me.

And gets you off the hook for Pcd name bikesheding :)

/
    Leif

[PATCH 3/5] ArmVirtPkg: refactor reading of the physical address space size

[Ard Biesheuvel]

In preparation of dropping the hardcoded 40-bit limit on the physical
address space when running under virtualization, let's refactor the
code a bit so we read the hardware limit in a single place.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h                       |  1 +
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c |  4 +-
 ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S          | 39 --------------------
 ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S              | 24 ------------
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c           |  3 +-
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf         |  6 ---
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf      |  6 ---
 ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S           | 39 --------------------
 ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S               | 24 ------------
 ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c             |  8 +---
 ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf           |  6 ---
 11 files changed, 8 insertions(+), 152 deletions(-)

diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
index bdf1c513bc6d..15562b35c730 100644
--- a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
+++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
@@ -35,6 +35,7 @@
 VOID
 EFIAPI
 ArmVirtGetMemoryMap (
+  IN  EFI_PHYSICAL_ADDRESS            TopOfAddressSpace,
   OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
   );
 
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
index 05afd1282422..3f0e9b3a0579 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
@@ -15,6 +15,7 @@
 
 #include <PiPei.h>
 
+#include <Library/ArmLib.h>
 #include <Library/ArmMmuLib.h>
 #include <Library/ArmVirtMemInfoLib.h>
 #include <Library/DebugLib.h>
@@ -39,7 +40,8 @@ InitMmu (
   RETURN_STATUS                 Status;
 
   // Get Virtual Memory Map from the Platform Library
-  ArmVirtGetMemoryMap (&MemoryTable);
+  ArmVirtGetMemoryMap (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()),
+    &MemoryTable);
 
   //Note: Because we called PeiServicesInstallPeiMemory() before to call InitMmu() the MMU Page Table resides in
   //      DRAM (even at the top of DRAM as it is the first permanent memory allocation)
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
deleted file mode 100644
index a1f6a194d59b..000000000000
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
+++ /dev/null
@@ -1,39 +0,0 @@
-#
-#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
-#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
-#
-#  This program and the accompanying materials
-#  are licensed and made available under the terms and conditions of the BSD License
-#  which accompanies this distribution.  The full text of the license may be found at
-#  http://opensource.org/licenses/bsd-license.php
-#
-#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-#
-#
-
-#include <AsmMacroIoLibV8.h>
-
-//EFI_PHYSICAL_ADDRESS
-//GetPhysAddrTop (
-//  VOID
-//  );
-ASM_FUNC(ArmGetPhysAddrTop)
-  mrs   x0, id_aa64mmfr0_el1
-  adr   x1, .LPARanges
-  and   x0, x0, #7
-  ldrb  w1, [x1, x0]
-  mov   x0, #1
-  lsl   x0, x0, x1
-  ret
-
-//
-// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
-// physical address space support on this CPU:
-// 0 == 32 bits, 1 == 36 bits, etc etc
-// 6 and 7 are reserved
-//
-.LPARanges:
-  .byte 32, 36, 40, 42, 44, 48, -1, -1
-
-ASM_FUNCTION_REMOVE_IF_UNREFERENCED
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
deleted file mode 100644
index 9cd81529fb3d..000000000000
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
+++ /dev/null
@@ -1,24 +0,0 @@
-#
-#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
-#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
-#
-#  This program and the accompanying materials
-#  are licensed and made available under the terms and conditions of the BSD License
-#  which accompanies this distribution.  The full text of the license may be found at
-#  http://opensource.org/licenses/bsd-license.php
-#
-#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-#
-#
-
-#include <AsmMacroIoLib.h>
-
-//EFI_PHYSICAL_ADDRESS
-//GetPhysAddrTop (
-//  VOID
-//  );
-ASM_FUNC(ArmGetPhysAddrTop)
-  mov   r0, #0x00000000
-  mov   r1, #0x10000
-  bx    lr
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 760bcc169cf4..4eca9b895354 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -41,6 +41,7 @@ ArmGetPhysAddrTop (
 **/
 VOID
 ArmVirtGetMemoryMap (
+  IN  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace,
   OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
   )
 {
@@ -80,7 +81,7 @@ ArmVirtGetMemoryMap (
 
   // Peripheral space after DRAM
   TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
-                     ArmGetPhysAddrTop ());
+                     TopOfAddressSpace);
   VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
   VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
   VirtualMemoryTable[2].Length       = TopOfMemory -
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
index c72a97f9e78a..f2c461e3b55a 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
@@ -24,12 +24,6 @@
 [Sources]
   QemuVirtMemInfoLib.c
 
-[Sources.ARM]
-  Arm/PhysAddrTop.S
-
-[Sources.AARCH64]
-  AArch64/PhysAddrTop.S
-
 [Packages]
   ArmPkg/ArmPkg.dec
   ArmVirtPkg/ArmVirtPkg.dec
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
index e4032d3efb53..f54fb51ee1d4 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
@@ -26,12 +26,6 @@
   QemuVirtMemInfoLib.c
   QemuVirtMemInfoPeiLibConstructor.c
 
-[Sources.ARM]
-  Arm/PhysAddrTop.S
-
-[Sources.AARCH64]
-  AArch64/PhysAddrTop.S
-
 [Packages]
   ArmPkg/ArmPkg.dec
   ArmVirtPkg/ArmVirtPkg.dec
diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
deleted file mode 100644
index a1f6a194d59b..000000000000
--- a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
+++ /dev/null
@@ -1,39 +0,0 @@
-#
-#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
-#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
-#
-#  This program and the accompanying materials
-#  are licensed and made available under the terms and conditions of the BSD License
-#  which accompanies this distribution.  The full text of the license may be found at
-#  http://opensource.org/licenses/bsd-license.php
-#
-#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-#
-#
-
-#include <AsmMacroIoLibV8.h>
-
-//EFI_PHYSICAL_ADDRESS
-//GetPhysAddrTop (
-//  VOID
-//  );
-ASM_FUNC(ArmGetPhysAddrTop)
-  mrs   x0, id_aa64mmfr0_el1
-  adr   x1, .LPARanges
-  and   x0, x0, #7
-  ldrb  w1, [x1, x0]
-  mov   x0, #1
-  lsl   x0, x0, x1
-  ret
-
-//
-// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
-// physical address space support on this CPU:
-// 0 == 32 bits, 1 == 36 bits, etc etc
-// 6 and 7 are reserved
-//
-.LPARanges:
-  .byte 32, 36, 40, 42, 44, 48, -1, -1
-
-ASM_FUNCTION_REMOVE_IF_UNREFERENCED
diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
deleted file mode 100644
index 9cd81529fb3d..000000000000
--- a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
+++ /dev/null
@@ -1,24 +0,0 @@
-#
-#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
-#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
-#
-#  This program and the accompanying materials
-#  are licensed and made available under the terms and conditions of the BSD License
-#  which accompanies this distribution.  The full text of the license may be found at
-#  http://opensource.org/licenses/bsd-license.php
-#
-#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-#
-#
-
-#include <AsmMacroIoLib.h>
-
-//EFI_PHYSICAL_ADDRESS
-//GetPhysAddrTop (
-//  VOID
-//  );
-ASM_FUNC(ArmGetPhysAddrTop)
-  mov   r0, #0x00000000
-  mov   r1, #0x10000
-  bx    lr
diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
index 88ff3167cbfd..3d4e3e38c3f1 100644
--- a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
@@ -18,11 +18,6 @@
 
 STATIC ARM_MEMORY_REGION_DESCRIPTOR  mVirtualMemoryTable[2];
 
-EFI_PHYSICAL_ADDRESS
-ArmGetPhysAddrTop (
-  VOID
-  );
-
 /**
   Return the Virtual Memory Map of your platform
 
@@ -39,6 +34,7 @@ ArmGetPhysAddrTop (
 VOID
 EFIAPI
 ArmVirtGetMemoryMap (
+  IN  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace,
   OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
   )
 {
@@ -51,7 +47,7 @@ ArmVirtGetMemoryMap (
   //
   mVirtualMemoryTable[0].PhysicalBase = 0x0;
   mVirtualMemoryTable[0].VirtualBase  = 0x0;
-  mVirtualMemoryTable[0].Length       = ArmGetPhysAddrTop ();
+  mVirtualMemoryTable[0].Length       = TopOfAddressSpace;
   mVirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
 
   mVirtualMemoryTable[1].PhysicalBase = 0x0;
diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
index cd4c805a4db9..ae107810e927 100644
--- a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
+++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
@@ -24,12 +24,6 @@
 [Sources]
   XenVirtMemInfoLib.c
 
-[Sources.ARM]
-  Arm/PhysAddrTop.S
-
-[Sources.AARCH64]
-  AArch64/PhysAddrTop.S
-
 [Packages]
   ArmPkg/ArmPkg.dec
   ArmVirtPkg/ArmVirtPkg.dec
-- 
2.17.1

Re: [PATCH 3/5] ArmVirtPkg: refactor reading of the physical address space size

[Laszlo Ersek]

On 11/23/18 13:14, Ard Biesheuvel wrote:
> In preparation of dropping the hardcoded 40-bit limit on the physical
> address space when running under virtualization, let's refactor the
> code a bit so we read the hardware limit in a single place.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h                       |  1 +
>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c |  4 +-
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S          | 39 --------------------
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S              | 24 ------------
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c           |  3 +-
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf         |  6 ---
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf      |  6 ---
>  ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S           | 39 --------------------
>  ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S               | 24 ------------
>  ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c             |  8 +---
>  ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf           |  6 ---
>  11 files changed, 8 insertions(+), 152 deletions(-)

OK so we got:

- one declaration for ArmVirtGetMemoryMap(), namely in
"ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h";

- two implementations (in
"ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c" and
"ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c");

- and one call site (in
"ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c").

> 
> diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> index bdf1c513bc6d..15562b35c730 100644
> --- a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> @@ -35,6 +35,7 @@
>  VOID
>  EFIAPI
>  ArmVirtGetMemoryMap (
> +  IN  EFI_PHYSICAL_ADDRESS            TopOfAddressSpace,

Good parameter name; in OvmfPkg/PlatformPei, I call the same thing
"FirstNonAddress". :)

This handles the declaration of the API.

>    OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
>    );
>  
> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> index 05afd1282422..3f0e9b3a0579 100644
> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> @@ -15,6 +15,7 @@
>  
>  #include <PiPei.h>
>  
> +#include <Library/ArmLib.h>

Right, the INF file already spells out the ArmLib class, but the header
was never included. (Has the lib class been superfluous all this time?
Who knows. We do need it now.)

>  #include <Library/ArmMmuLib.h>
>  #include <Library/ArmVirtMemInfoLib.h>
>  #include <Library/DebugLib.h>
> @@ -39,7 +40,8 @@ InitMmu (
>    RETURN_STATUS                 Status;
>  
>    // Get Virtual Memory Map from the Platform Library
> -  ArmVirtGetMemoryMap (&MemoryTable);
> +  ArmVirtGetMemoryMap (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()),
> +    &MemoryTable);
>  
>    //Note: Because we called PeiServicesInstallPeiMemory() before to call InitMmu() the MMU Page Table resides in
>    //      DRAM (even at the top of DRAM as it is the first permanent memory allocation)

Right. I think I understand the use of ArmGetPhysicalAddressBits() here,
and I agree about the LShiftU64() too.

So this covers the call site. OK.

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> deleted file mode 100644
> index a1f6a194d59b..000000000000
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -#
> -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> -#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> -#
> -#  This program and the accompanying materials
> -#  are licensed and made available under the terms and conditions of the BSD License
> -#  which accompanies this distribution.  The full text of the license may be found at
> -#  http://opensource.org/licenses/bsd-license.php
> -#
> -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#
> -
> -#include <AsmMacroIoLibV8.h>
> -
> -//EFI_PHYSICAL_ADDRESS
> -//GetPhysAddrTop (
> -//  VOID
> -//  );
> -ASM_FUNC(ArmGetPhysAddrTop)
> -  mrs   x0, id_aa64mmfr0_el1
> -  adr   x1, .LPARanges
> -  and   x0, x0, #7
> -  ldrb  w1, [x1, x0]
> -  mov   x0, #1
> -  lsl   x0, x0, x1
> -  ret
> -
> -//
> -// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> -// physical address space support on this CPU:
> -// 0 == 32 bits, 1 == 36 bits, etc etc
> -// 6 and 7 are reserved
> -//
> -.LPARanges:
> -  .byte 32, 36, 40, 42, 44, 48, -1, -1
> -
> -ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> deleted file mode 100644
> index 9cd81529fb3d..000000000000
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -#
> -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> -#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> -#
> -#  This program and the accompanying materials
> -#  are licensed and made available under the terms and conditions of the BSD License
> -#  which accompanies this distribution.  The full text of the license may be found at
> -#  http://opensource.org/licenses/bsd-license.php
> -#
> -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#
> -
> -#include <AsmMacroIoLib.h>
> -
> -//EFI_PHYSICAL_ADDRESS
> -//GetPhysAddrTop (
> -//  VOID
> -//  );
> -ASM_FUNC(ArmGetPhysAddrTop)
> -  mov   r0, #0x00000000
> -  mov   r1, #0x10000
> -  bx    lr
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> index 760bcc169cf4..4eca9b895354 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> @@ -41,6 +41,7 @@ ArmGetPhysAddrTop (
>  **/
>  VOID
>  ArmVirtGetMemoryMap (
> +  IN  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace,
>    OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
>    )
>  {
> @@ -80,7 +81,7 @@ ArmVirtGetMemoryMap (
>  
>    // Peripheral space after DRAM
>    TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
> -                     ArmGetPhysAddrTop ());
> +                     TopOfAddressSpace);
>    VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
>    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
>    VirtualMemoryTable[2].Length       = TopOfMemory -

This updates one of the implementations, replacing the internal (asm)
helper function with the external param. OK.

(1) Can you remove the declaration too, of the helper function
ArmGetPhysAddrTop(), in "QemuVirtMemInfoLib.c"? The 32-bit & 64-bit
definitions are removed with the assembly files, but the declaration is
currently left over.

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> index c72a97f9e78a..f2c461e3b55a 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> @@ -24,12 +24,6 @@
>  [Sources]
>    QemuVirtMemInfoLib.c
>  
> -[Sources.ARM]
> -  Arm/PhysAddrTop.S
> -
> -[Sources.AARCH64]
> -  AArch64/PhysAddrTop.S
> -
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> index e4032d3efb53..f54fb51ee1d4 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> @@ -26,12 +26,6 @@
>    QemuVirtMemInfoLib.c
>    QemuVirtMemInfoPeiLibConstructor.c
>  
> -[Sources.ARM]
> -  Arm/PhysAddrTop.S
> -
> -[Sources.AARCH64]
> -  AArch64/PhysAddrTop.S
> -
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec

These look good.

> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
> deleted file mode 100644
> index a1f6a194d59b..000000000000
> --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -#
> -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> -#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> -#
> -#  This program and the accompanying materials
> -#  are licensed and made available under the terms and conditions of the BSD License
> -#  which accompanies this distribution.  The full text of the license may be found at
> -#  http://opensource.org/licenses/bsd-license.php
> -#
> -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#
> -
> -#include <AsmMacroIoLibV8.h>
> -
> -//EFI_PHYSICAL_ADDRESS
> -//GetPhysAddrTop (
> -//  VOID
> -//  );
> -ASM_FUNC(ArmGetPhysAddrTop)
> -  mrs   x0, id_aa64mmfr0_el1
> -  adr   x1, .LPARanges
> -  and   x0, x0, #7
> -  ldrb  w1, [x1, x0]
> -  mov   x0, #1
> -  lsl   x0, x0, x1
> -  ret
> -
> -//
> -// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> -// physical address space support on this CPU:
> -// 0 == 32 bits, 1 == 36 bits, etc etc
> -// 6 and 7 are reserved
> -//
> -.LPARanges:
> -  .byte 32, 36, 40, 42, 44, 48, -1, -1
> -
> -ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> deleted file mode 100644
> index 9cd81529fb3d..000000000000
> --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -#
> -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> -#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> -#
> -#  This program and the accompanying materials
> -#  are licensed and made available under the terms and conditions of the BSD License
> -#  which accompanies this distribution.  The full text of the license may be found at
> -#  http://opensource.org/licenses/bsd-license.php
> -#
> -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#
> -
> -#include <AsmMacroIoLib.h>
> -
> -//EFI_PHYSICAL_ADDRESS
> -//GetPhysAddrTop (
> -//  VOID
> -//  );
> -ASM_FUNC(ArmGetPhysAddrTop)
> -  mov   r0, #0x00000000
> -  mov   r1, #0x10000
> -  bx    lr
> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> index 88ff3167cbfd..3d4e3e38c3f1 100644
> --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> @@ -18,11 +18,6 @@
>  
>  STATIC ARM_MEMORY_REGION_DESCRIPTOR  mVirtualMemoryTable[2];
>  
> -EFI_PHYSICAL_ADDRESS
> -ArmGetPhysAddrTop (
> -  VOID
> -  );
> -
>  /**
>    Return the Virtual Memory Map of your platform
>  

Aha, here you didn't miss the helper's declaration. :)

> @@ -39,6 +34,7 @@ ArmGetPhysAddrTop (
>  VOID
>  EFIAPI
>  ArmVirtGetMemoryMap (
> +  IN  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace,
>    OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
>    )
>  {
> @@ -51,7 +47,7 @@ ArmVirtGetMemoryMap (
>    //
>    mVirtualMemoryTable[0].PhysicalBase = 0x0;
>    mVirtualMemoryTable[0].VirtualBase  = 0x0;
> -  mVirtualMemoryTable[0].Length       = ArmGetPhysAddrTop ();
> +  mVirtualMemoryTable[0].Length       = TopOfAddressSpace;
>    mVirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>  
>    mVirtualMemoryTable[1].PhysicalBase = 0x0;

And this covers implementation #2.

> diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> index cd4c805a4db9..ae107810e927 100644
> --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> @@ -24,12 +24,6 @@
>  [Sources]
>    XenVirtMemInfoLib.c
>  
> -[Sources.ARM]
> -  Arm/PhysAddrTop.S
> -
> -[Sources.AARCH64]
> -  AArch64/PhysAddrTop.S
> -
>  [Packages]
>    ArmPkg/ArmPkg.dec
>    ArmVirtPkg/ArmVirtPkg.dec
> 

With (1) fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

This is a good clean-up (with the help of patch #1) even without the
specific use case.

Thanks!
Laszlo

Re: [PATCH 3/5] ArmVirtPkg: refactor reading of the physical address space size

[Ard Biesheuvel]

On Mon, 26 Nov 2018 at 11:00, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/23/18 13:14, Ard Biesheuvel wrote:
> > In preparation of dropping the hardcoded 40-bit limit on the physical
> > address space when running under virtualization, let's refactor the
> > code a bit so we read the hardware limit in a single place.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h                       |  1 +
> >  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c |  4 +-
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S          | 39 --------------------
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S              | 24 ------------
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c           |  3 +-
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf         |  6 ---
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf      |  6 ---
> >  ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S           | 39 --------------------
> >  ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S               | 24 ------------
> >  ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c             |  8 +---
> >  ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf           |  6 ---
> >  11 files changed, 8 insertions(+), 152 deletions(-)
>
> OK so we got:
>
> - one declaration for ArmVirtGetMemoryMap(), namely in
> "ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h";
>
> - two implementations (in
> "ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c" and
> "ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c");
>
> - and one call site (in
> "ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c").
>
> >
> > diff --git a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> > index bdf1c513bc6d..15562b35c730 100644
> > --- a/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> > +++ b/ArmVirtPkg/Include/Library/ArmVirtMemInfoLib.h
> > @@ -35,6 +35,7 @@
> >  VOID
> >  EFIAPI
> >  ArmVirtGetMemoryMap (
> > +  IN  EFI_PHYSICAL_ADDRESS            TopOfAddressSpace,
>
> Good parameter name; in OvmfPkg/PlatformPei, I call the same thing
> "FirstNonAddress". :)
>
> This handles the declaration of the API.
>
> >    OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
> >    );
> >
> > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > index 05afd1282422..3f0e9b3a0579 100644
> > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > @@ -15,6 +15,7 @@
> >
> >  #include <PiPei.h>
> >
> > +#include <Library/ArmLib.h>
>
> Right, the INF file already spells out the ArmLib class, but the header
> was never included. (Has the lib class been superfluous all this time?
> Who knows. We do need it now.)
>
> >  #include <Library/ArmMmuLib.h>
> >  #include <Library/ArmVirtMemInfoLib.h>
> >  #include <Library/DebugLib.h>
> > @@ -39,7 +40,8 @@ InitMmu (
> >    RETURN_STATUS                 Status;
> >
> >    // Get Virtual Memory Map from the Platform Library
> > -  ArmVirtGetMemoryMap (&MemoryTable);
> > +  ArmVirtGetMemoryMap (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()),
> > +    &MemoryTable);
> >
> >    //Note: Because we called PeiServicesInstallPeiMemory() before to call InitMmu() the MMU Page Table resides in
> >    //      DRAM (even at the top of DRAM as it is the first permanent memory allocation)
>
> Right. I think I understand the use of ArmGetPhysicalAddressBits() here,
> and I agree about the LShiftU64() too.
>
> So this covers the call site. OK.
>
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> > deleted file mode 100644
> > index a1f6a194d59b..000000000000
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
> > +++ /dev/null
> > @@ -1,39 +0,0 @@
> > -#
> > -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> > -#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> > -#
> > -#  This program and the accompanying materials
> > -#  are licensed and made available under the terms and conditions of the BSD License
> > -#  which accompanies this distribution.  The full text of the license may be found at
> > -#  http://opensource.org/licenses/bsd-license.php
> > -#
> > -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > -#
> > -#
> > -
> > -#include <AsmMacroIoLibV8.h>
> > -
> > -//EFI_PHYSICAL_ADDRESS
> > -//GetPhysAddrTop (
> > -//  VOID
> > -//  );
> > -ASM_FUNC(ArmGetPhysAddrTop)
> > -  mrs   x0, id_aa64mmfr0_el1
> > -  adr   x1, .LPARanges
> > -  and   x0, x0, #7
> > -  ldrb  w1, [x1, x0]
> > -  mov   x0, #1
> > -  lsl   x0, x0, x1
> > -  ret
> > -
> > -//
> > -// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> > -// physical address space support on this CPU:
> > -// 0 == 32 bits, 1 == 36 bits, etc etc
> > -// 6 and 7 are reserved
> > -//
> > -.LPARanges:
> > -  .byte 32, 36, 40, 42, 44, 48, -1, -1
> > -
> > -ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> > deleted file mode 100644
> > index 9cd81529fb3d..000000000000
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
> > +++ /dev/null
> > @@ -1,24 +0,0 @@
> > -#
> > -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> > -#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> > -#
> > -#  This program and the accompanying materials
> > -#  are licensed and made available under the terms and conditions of the BSD License
> > -#  which accompanies this distribution.  The full text of the license may be found at
> > -#  http://opensource.org/licenses/bsd-license.php
> > -#
> > -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > -#
> > -#
> > -
> > -#include <AsmMacroIoLib.h>
> > -
> > -//EFI_PHYSICAL_ADDRESS
> > -//GetPhysAddrTop (
> > -//  VOID
> > -//  );
> > -ASM_FUNC(ArmGetPhysAddrTop)
> > -  mov   r0, #0x00000000
> > -  mov   r1, #0x10000
> > -  bx    lr
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > index 760bcc169cf4..4eca9b895354 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > @@ -41,6 +41,7 @@ ArmGetPhysAddrTop (
> >  **/
> >  VOID
> >  ArmVirtGetMemoryMap (
> > +  IN  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace,
> >    OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
> >    )
> >  {
> > @@ -80,7 +81,7 @@ ArmVirtGetMemoryMap (
> >
> >    // Peripheral space after DRAM
> >    TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
> > -                     ArmGetPhysAddrTop ());
> > +                     TopOfAddressSpace);
> >    VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
> >    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> >    VirtualMemoryTable[2].Length       = TopOfMemory -
>
> This updates one of the implementations, replacing the internal (asm)
> helper function with the external param. OK.
>
> (1) Can you remove the declaration too, of the helper function
> ArmGetPhysAddrTop(), in "QemuVirtMemInfoLib.c"? The 32-bit & 64-bit
> definitions are removed with the assembly files, but the declaration is
> currently left over.
>

OK

> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > index c72a97f9e78a..f2c461e3b55a 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > @@ -24,12 +24,6 @@
> >  [Sources]
> >    QemuVirtMemInfoLib.c
> >
> > -[Sources.ARM]
> > -  Arm/PhysAddrTop.S
> > -
> > -[Sources.AARCH64]
> > -  AArch64/PhysAddrTop.S
> > -
> >  [Packages]
> >    ArmPkg/ArmPkg.dec
> >    ArmVirtPkg/ArmVirtPkg.dec
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > index e4032d3efb53..f54fb51ee1d4 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > @@ -26,12 +26,6 @@
> >    QemuVirtMemInfoLib.c
> >    QemuVirtMemInfoPeiLibConstructor.c
> >
> > -[Sources.ARM]
> > -  Arm/PhysAddrTop.S
> > -
> > -[Sources.AARCH64]
> > -  AArch64/PhysAddrTop.S
> > -
> >  [Packages]
> >    ArmPkg/ArmPkg.dec
> >    ArmVirtPkg/ArmVirtPkg.dec
>
> These look good.
>
> > diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S b/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
> > deleted file mode 100644
> > index a1f6a194d59b..000000000000
> > --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
> > +++ /dev/null
> > @@ -1,39 +0,0 @@
> > -#
> > -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> > -#  Copyright (c) 2016-2017, Linaro Limited. All rights reserved.
> > -#
> > -#  This program and the accompanying materials
> > -#  are licensed and made available under the terms and conditions of the BSD License
> > -#  which accompanies this distribution.  The full text of the license may be found at
> > -#  http://opensource.org/licenses/bsd-license.php
> > -#
> > -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > -#
> > -#
> > -
> > -#include <AsmMacroIoLibV8.h>
> > -
> > -//EFI_PHYSICAL_ADDRESS
> > -//GetPhysAddrTop (
> > -//  VOID
> > -//  );
> > -ASM_FUNC(ArmGetPhysAddrTop)
> > -  mrs   x0, id_aa64mmfr0_el1
> > -  adr   x1, .LPARanges
> > -  and   x0, x0, #7
> > -  ldrb  w1, [x1, x0]
> > -  mov   x0, #1
> > -  lsl   x0, x0, x1
> > -  ret
> > -
> > -//
> > -// Bits 0..2 of the AA64MFR0_EL1 system register encode the size of the
> > -// physical address space support on this CPU:
> > -// 0 == 32 bits, 1 == 36 bits, etc etc
> > -// 6 and 7 are reserved
> > -//
> > -.LPARanges:
> > -  .byte 32, 36, 40, 42, 44, 48, -1, -1
> > -
> > -ASM_FUNCTION_REMOVE_IF_UNREFERENCED
> > diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S b/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> > deleted file mode 100644
> > index 9cd81529fb3d..000000000000
> > --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> > +++ /dev/null
> > @@ -1,24 +0,0 @@
> > -#
> > -#  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> > -#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
> > -#
> > -#  This program and the accompanying materials
> > -#  are licensed and made available under the terms and conditions of the BSD License
> > -#  which accompanies this distribution.  The full text of the license may be found at
> > -#  http://opensource.org/licenses/bsd-license.php
> > -#
> > -#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > -#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > -#
> > -#
> > -
> > -#include <AsmMacroIoLib.h>
> > -
> > -//EFI_PHYSICAL_ADDRESS
> > -//GetPhysAddrTop (
> > -//  VOID
> > -//  );
> > -ASM_FUNC(ArmGetPhysAddrTop)
> > -  mov   r0, #0x00000000
> > -  mov   r1, #0x10000
> > -  bx    lr
> > diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> > index 88ff3167cbfd..3d4e3e38c3f1 100644
> > --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> > +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.c
> > @@ -18,11 +18,6 @@
> >
> >  STATIC ARM_MEMORY_REGION_DESCRIPTOR  mVirtualMemoryTable[2];
> >
> > -EFI_PHYSICAL_ADDRESS
> > -ArmGetPhysAddrTop (
> > -  VOID
> > -  );
> > -
> >  /**
> >    Return the Virtual Memory Map of your platform
> >
>
> Aha, here you didn't miss the helper's declaration. :)
>
> > @@ -39,6 +34,7 @@ ArmGetPhysAddrTop (
> >  VOID
> >  EFIAPI
> >  ArmVirtGetMemoryMap (
> > +  IN  EFI_PHYSICAL_ADDRESS           TopOfAddressSpace,
> >    OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
> >    )
> >  {
> > @@ -51,7 +47,7 @@ ArmVirtGetMemoryMap (
> >    //
> >    mVirtualMemoryTable[0].PhysicalBase = 0x0;
> >    mVirtualMemoryTable[0].VirtualBase  = 0x0;
> > -  mVirtualMemoryTable[0].Length       = ArmGetPhysAddrTop ();
> > +  mVirtualMemoryTable[0].Length       = TopOfAddressSpace;
> >    mVirtualMemoryTable[0].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> >
> >    mVirtualMemoryTable[1].PhysicalBase = 0x0;
>
> And this covers implementation #2.
>
> > diff --git a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> > index cd4c805a4db9..ae107810e927 100644
> > --- a/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> > +++ b/ArmVirtPkg/Library/XenVirtMemInfoLib/XenVirtMemInfoLib.inf
> > @@ -24,12 +24,6 @@
> >  [Sources]
> >    XenVirtMemInfoLib.c
> >
> > -[Sources.ARM]
> > -  Arm/PhysAddrTop.S
> > -
> > -[Sources.AARCH64]
> > -  AArch64/PhysAddrTop.S
> > -
> >  [Packages]
> >    ArmPkg/ArmPkg.dec
> >    ArmVirtPkg/ArmVirtPkg.dec
> >
>
> With (1) fixed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> This is a good clean-up (with the help of patch #1) even without the
> specific use case.
>

Indeed. I will reorder this with #2

Thanks

[PATCH 4/5] ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD space

[Ard Biesheuvel]

Move the call to BuildCpuHob () into ArmVirtMemoryInitPeiLib (which is
shared between all the ArmVirtPkg targets), and drop the inclusion of
CpuPei.inf [which calls it on ArmVirtQemu] and the BuildCpuHob () call
from ArmVirtPrePiUniCoreRelocatable [for the other targets].

This makes the size of the GCD address space and page table mappings
depend only on the size of the physical address space as exposed by
the CPU system registers.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirtQemu.dsc                                             | 1 -
 ArmVirtPkg/ArmVirtQemu.fdf                                             | 1 -
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 3 +++
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 1 +
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c             | 5 +----
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf           | 1 -
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf        | 1 -
 ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf                    | 3 ---
 ArmVirtPkg/PrePi/PrePi.c                                               | 3 ---
 9 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 885c6b14b844..cb59c790afcc 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -226,7 +226,6 @@
   }
   ArmPlatformPkg/PlatformPei/PlatformPeim.inf
   ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
-  ArmPkg/Drivers/CpuPei/CpuPei.inf
 
   MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
 
diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
index c6a22dc018f3..12bc570c4cb3 100644
--- a/ArmVirtPkg/ArmVirtQemu.fdf
+++ b/ArmVirtPkg/ArmVirtQemu.fdf
@@ -106,7 +106,6 @@ READ_LOCK_STATUS   = TRUE
   INF MdeModulePkg/Core/Pei/PeiMain.inf
   INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
   INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
-  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
   INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
   INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
   INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
index 3f0e9b3a0579..3d86d31ab50e 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
@@ -116,5 +116,8 @@ MemoryPeim (
     BuildMemoryTypeInformationHob ();
   }
 
+  // Publish the CPU memory and io spaces sizes
+  BuildCpuHob (ArmGetPhysicalAddressBits (), FixedPcdGet8 (PcdPrePiCpuIoSize));
+
   return EFI_SUCCESS;
 }
diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
index 54879d590a8a..d0e39df84b20 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
@@ -59,6 +59,7 @@
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
+  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
 
 [Pcd]
   gArmTokenSpaceGuid.PcdSystemMemoryBase
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 4eca9b895354..ded87d604f4f 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -46,7 +46,6 @@ ArmVirtGetMemoryMap (
   )
 {
   ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
-  UINT64                        TopOfMemory;
 
   ASSERT (VirtualMemoryMap != NULL);
 
@@ -80,11 +79,9 @@ ArmVirtGetMemoryMap (
   VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
   // Peripheral space after DRAM
-  TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
-                     TopOfAddressSpace);
   VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
   VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
-  VirtualMemoryTable[2].Length       = TopOfMemory -
+  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
                                        VirtualMemoryTable[2].PhysicalBase;
   VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
index f2c461e3b55a..5c5b841051ad 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
@@ -45,4 +45,3 @@
 
 [FixedPcd]
   gArmTokenSpaceGuid.PcdFdSize
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
index f54fb51ee1d4..d12089760b22 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
@@ -49,4 +49,3 @@
 [FixedPcd]
   gArmTokenSpaceGuid.PcdFdSize
   gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
index 1587bd92f206..e04bd1b7c497 100755
--- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
+++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
@@ -85,9 +85,6 @@
 
   gArmPlatformTokenSpaceGuid.PcdCoreCount
 
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
-  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
-
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
   gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
index f6abe2f2016b..ecaaac1545c4 100755
--- a/ArmVirtPkg/PrePi/PrePi.c
+++ b/ArmVirtPkg/PrePi/PrePi.c
@@ -79,9 +79,6 @@ PrePiMain (
   StacksSize = PcdGet32 (PcdCPUCorePrimaryStackSize);
   BuildStackHob (StacksBase, StacksSize);
 
-  //TODO: Call CpuPei as a library
-  BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize));
-
   // Set the Boot Mode
   SetBootMode (BOOT_WITH_FULL_CONFIGURATION);
 
-- 
2.17.1

Re: [PATCH 4/5] ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD space

[Laszlo Ersek]

On 11/23/18 13:14, Ard Biesheuvel wrote:
> Move the call to BuildCpuHob () into ArmVirtMemoryInitPeiLib (which is
> shared between all the ArmVirtPkg targets), and drop the inclusion of
> CpuPei.inf [which calls it on ArmVirtQemu] and the BuildCpuHob () call
> from ArmVirtPrePiUniCoreRelocatable [for the other targets].

(1) Can you please confirm, in the commit message, that we don't need --
or that we don't exercise anyway -- the other things that
"ArmPkg/Drivers/CpuPei/CpuPei.inf" does, namely:

- ArmEnableBranchPrediction()
- building gArmMpCoreInfoGuid

?

>
> This makes the size of the GCD address space and page table mappings
> depend only on the size of the physical address space as exposed by
> the CPU system registers.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc                                             | 1 -
>  ArmVirtPkg/ArmVirtQemu.fdf                                             | 1 -
>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 3 +++
>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 1 +
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c             | 5 +----
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf           | 1 -
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf        | 1 -
>  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf                    | 3 ---
>  ArmVirtPkg/PrePi/PrePi.c                                               | 3 ---
>  9 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 885c6b14b844..cb59c790afcc 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -226,7 +226,6 @@
>    }
>    ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>    ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> -  ArmPkg/Drivers/CpuPei/CpuPei.inf
>
>    MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> index c6a22dc018f3..12bc570c4cb3 100644
> --- a/ArmVirtPkg/ArmVirtQemu.fdf
> +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> @@ -106,7 +106,6 @@ READ_LOCK_STATUS   = TRUE
>    INF MdeModulePkg/Core/Pei/PeiMain.inf
>    INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
>    INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> -  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
>    INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
>    INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf

OK, so this removes the previous CPU HOB building in ArmVirtQemu.

Quoting out of order:

> diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> index 1587bd92f206..e04bd1b7c497 100755
> --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> @@ -85,9 +85,6 @@
>
>    gArmPlatformTokenSpaceGuid.PcdCoreCount
>
> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
> -
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
> diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
> index f6abe2f2016b..ecaaac1545c4 100755
> --- a/ArmVirtPkg/PrePi/PrePi.c
> +++ b/ArmVirtPkg/PrePi/PrePi.c
> @@ -79,9 +79,6 @@ PrePiMain (
>    StacksSize = PcdGet32 (PcdCPUCorePrimaryStackSize);
>    BuildStackHob (StacksBase, StacksSize);
>
> -  //TODO: Call CpuPei as a library
> -  BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize));
> -

Best way to solve a TODO is to delete it. :)

>    // Set the Boot Mode
>    SetBootMode (BOOT_WITH_FULL_CONFIGURATION);
>
>

OK, this removes the previous CPU HOB building in ArmVirtQemuKernel and
ArmVirtXen.


> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> index 3f0e9b3a0579..3d86d31ab50e 100644
> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> @@ -116,5 +116,8 @@ MemoryPeim (
>      BuildMemoryTypeInformationHob ();
>    }
>
> +  // Publish the CPU memory and io spaces sizes
> +  BuildCpuHob (ArmGetPhysicalAddressBits (), FixedPcdGet8 (PcdPrePiCpuIoSize));
> +
>    return EFI_SUCCESS;
>  }

Argh, it took me a while to see that this will indeed supplement the CPU
HOB on all ArmVirtPlatforms. The build report file(s) helped. I didn't
think of ArmPlatformPkg originally. :)

So, yes, this is fine; it's OK to produce the CPU HOB anywhere in the
PEI phase.

> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> index 54879d590a8a..d0e39df84b20 100644
> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> @@ -59,6 +59,7 @@
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
>
>  [Pcd]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase

OK, expected.

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> index 4eca9b895354..ded87d604f4f 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> @@ -46,7 +46,6 @@ ArmVirtGetMemoryMap (
>    )
>  {
>    ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> -  UINT64                        TopOfMemory;
>
>    ASSERT (VirtualMemoryMap != NULL);
>
> @@ -80,11 +79,9 @@ ArmVirtGetMemoryMap (
>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>
>    // Peripheral space after DRAM
> -  TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
> -                     TopOfAddressSpace);
>    VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
>    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> -  VirtualMemoryTable[2].Length       = TopOfMemory -
> +  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
>                                         VirtualMemoryTable[2].PhysicalBase;
>    VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> index f2c461e3b55a..5c5b841051ad 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> @@ -45,4 +45,3 @@
>
>  [FixedPcd]
>    gArmTokenSpaceGuid.PcdFdSize
> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> index f54fb51ee1d4..d12089760b22 100644
> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> @@ -49,4 +49,3 @@
>  [FixedPcd]
>    gArmTokenSpaceGuid.PcdFdSize
>    gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize

This last part, for QemuVirtMemInfoLib, is functionally valid, in my
opinion. We are eliminating the clamping to PcdPrePiCpuMemorySize, which
was one of my earlier points, so what remains is "TopOfMemory =
TopOfAddressSpace"; good.

However: this change does not relate to the GCD memory space. The
ArmVirtGetMemoryMap() function partakes in the MMU configuration / page
table setup. Its leading comment says,

  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
  on your platform.

(2) Therefore, it should be split to a separate patch. Earlier, I
suggested a slight restructuring for the patch series, so let me refine
that:

- new patch #1: current patch #1 (introduce helper)

- new patch #2: current patch #3 (refactor ArmVirtPkg)

- new patch #3: current patch #2 (consider CPU capabilities in generic
                MMU setup code)

- new patch #4: this specific hunk, from current patch #4 (consider CPU
                capabilities in specific MMU setup code)

- new patch #5: the rest of current patch #4 (consider CPU caps in GCD /
                CPU HOB)

- new patch #6: kill PcdPrePiCpuMemorySize in ArmVirtPkg DSC files

Does this sound acceptable?

Thanks!
Laszlo

Re: [PATCH 4/5] ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD space

[Ard Biesheuvel]

On Mon, 26 Nov 2018 at 11:47, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/23/18 13:14, Ard Biesheuvel wrote:
> > Move the call to BuildCpuHob () into ArmVirtMemoryInitPeiLib (which is
> > shared between all the ArmVirtPkg targets), and drop the inclusion of
> > CpuPei.inf [which calls it on ArmVirtQemu] and the BuildCpuHob () call
> > from ArmVirtPrePiUniCoreRelocatable [for the other targets].
>
> (1) Can you please confirm, in the commit message, that we don't need --
> or that we don't exercise anyway -- the other things that
> "ArmPkg/Drivers/CpuPei/CpuPei.inf" does, namely:
>
> - ArmEnableBranchPrediction()

This does nothing on AArch64, and even on ARM, this is something that
is enabled by the bare metal boot code, not per-guest.

> - building gArmMpCoreInfoGuid
>

That is for the MPCore versions of PrePi and PrePeiCore, and we only
use UniCore versions in ArmVirtPkg

I will add some clarification to the commit log.

> >
> > This makes the size of the GCD address space and page table mappings
> > depend only on the size of the physical address space as exposed by
> > the CPU system registers.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/ArmVirtQemu.dsc                                             | 1 -
> >  ArmVirtPkg/ArmVirtQemu.fdf                                             | 1 -
> >  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c   | 3 +++
> >  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 1 +
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c             | 5 +----
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf           | 1 -
> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf        | 1 -
> >  ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf                    | 3 ---
> >  ArmVirtPkg/PrePi/PrePi.c                                               | 3 ---
> >  9 files changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > index 885c6b14b844..cb59c790afcc 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > @@ -226,7 +226,6 @@
> >    }
> >    ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> >    ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> > -  ArmPkg/Drivers/CpuPei/CpuPei.inf
> >
> >    MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf
> > index c6a22dc018f3..12bc570c4cb3 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.fdf
> > +++ b/ArmVirtPkg/ArmVirtQemu.fdf
> > @@ -106,7 +106,6 @@ READ_LOCK_STATUS   = TRUE
> >    INF MdeModulePkg/Core/Pei/PeiMain.inf
> >    INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf
> >    INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf
> > -  INF ArmPkg/Drivers/CpuPei/CpuPei.inf
> >    INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
> >    INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> >    INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>
> OK, so this removes the previous CPU HOB building in ArmVirtQemu.
>
> Quoting out of order:
>
> > diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > index 1587bd92f206..e04bd1b7c497 100755
> > --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> > @@ -85,9 +85,6 @@
> >
> >    gArmPlatformTokenSpaceGuid.PcdCoreCount
> >
> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
> > -
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
> > diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c
> > index f6abe2f2016b..ecaaac1545c4 100755
> > --- a/ArmVirtPkg/PrePi/PrePi.c
> > +++ b/ArmVirtPkg/PrePi/PrePi.c
> > @@ -79,9 +79,6 @@ PrePiMain (
> >    StacksSize = PcdGet32 (PcdCPUCorePrimaryStackSize);
> >    BuildStackHob (StacksBase, StacksSize);
> >
> > -  //TODO: Call CpuPei as a library
> > -  BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize));
> > -
>
> Best way to solve a TODO is to delete it. :)
>
> >    // Set the Boot Mode
> >    SetBootMode (BOOT_WITH_FULL_CONFIGURATION);
> >
> >
>
> OK, this removes the previous CPU HOB building in ArmVirtQemuKernel and
> ArmVirtXen.
>
>
> > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > index 3f0e9b3a0579..3d86d31ab50e 100644
> > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> > @@ -116,5 +116,8 @@ MemoryPeim (
> >      BuildMemoryTypeInformationHob ();
> >    }
> >
> > +  // Publish the CPU memory and io spaces sizes
> > +  BuildCpuHob (ArmGetPhysicalAddressBits (), FixedPcdGet8 (PcdPrePiCpuIoSize));
> > +
> >    return EFI_SUCCESS;
> >  }
>
> Argh, it took me a while to see that this will indeed supplement the CPU
> HOB on all ArmVirtPlatforms. The build report file(s) helped. I didn't
> think of ArmPlatformPkg originally. :)
>
> So, yes, this is fine; it's OK to produce the CPU HOB anywhere in the
> PEI phase.
>
> > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> > index 54879d590a8a..d0e39df84b20 100644
> > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf
> > @@ -59,6 +59,7 @@
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode
> >    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData
> > +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
> >
> >  [Pcd]
> >    gArmTokenSpaceGuid.PcdSystemMemoryBase
>
> OK, expected.
>
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > index 4eca9b895354..ded87d604f4f 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
> > @@ -46,7 +46,6 @@ ArmVirtGetMemoryMap (
> >    )
> >  {
> >    ARM_MEMORY_REGION_DESCRIPTOR  *VirtualMemoryTable;
> > -  UINT64                        TopOfMemory;
> >
> >    ASSERT (VirtualMemoryMap != NULL);
> >
> > @@ -80,11 +79,9 @@ ArmVirtGetMemoryMap (
> >    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> >    // Peripheral space after DRAM
> > -  TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize),
> > -                     TopOfAddressSpace);
> >    VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
> >    VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
> > -  VirtualMemoryTable[2].Length       = TopOfMemory -
> > +  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
> >                                         VirtualMemoryTable[2].PhysicalBase;
> >    VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > index f2c461e3b55a..5c5b841051ad 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf
> > @@ -45,4 +45,3 @@
> >
> >  [FixedPcd]
> >    gArmTokenSpaceGuid.PcdFdSize
> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > index f54fb51ee1d4..d12089760b22 100644
> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> > @@ -49,4 +49,3 @@
> >  [FixedPcd]
> >    gArmTokenSpaceGuid.PcdFdSize
> >    gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> > -  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
>
> This last part, for QemuVirtMemInfoLib, is functionally valid, in my
> opinion. We are eliminating the clamping to PcdPrePiCpuMemorySize, which
> was one of my earlier points, so what remains is "TopOfMemory =
> TopOfAddressSpace"; good.
>

Now, I'm not so sure anymore whether this hunk is correct.

This describes the extent of DRAM to the generic MMU code, and we only
want to map what
a) the CPU can support and
b) what we actually describe as being populated by anything

So if your guest supports 52 bits of PA space in hardware, we want the
GCD space to accommodate that (so we can report memory as residing
there). However, we cannot map all of that memory in UEFI anyway, and
so capping the mapping (this hunk) to the PcdPrePiCpuMemorySize size
actually makes sense.


> However: this change does not relate to the GCD memory space. The
> ArmVirtGetMemoryMap() function partakes in the MMU configuration / page
> table setup. Its leading comment says,
>
>   This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
>   on your platform.
>
> (2) Therefore, it should be split to a separate patch. Earlier, I
> suggested a slight restructuring for the patch series, so let me refine
> that:
>
> - new patch #1: current patch #1 (introduce helper)
>
> - new patch #2: current patch #3 (refactor ArmVirtPkg)
>
> - new patch #3: current patch #2 (consider CPU capabilities in generic
>                 MMU setup code)
>
> - new patch #4: this specific hunk, from current patch #4 (consider CPU
>                 capabilities in specific MMU setup code)
>
> - new patch #5: the rest of current patch #4 (consider CPU caps in GCD /
>                 CPU HOB)
>
> - new patch #6: kill PcdPrePiCpuMemorySize in ArmVirtPkg DSC files
>
> Does this sound acceptable?
>

Works for me.

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

[Ard Biesheuvel]

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

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
 
-- 
2.17.1

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

[Ard Biesheuvel]

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.

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

[Laszlo Ersek]

(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

Re: [PATCH 0/5] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit

[Laszlo Ersek]

On 11/23/18 13:14, Ard Biesheuvel wrote:
> The ArmVirtQemu targets currently limit the size of the IPA space to
> 40 bits because that is all what KVM supports. However, this is about
> to change, and so we need to update the code if we want to ensure that
> our UEFI firmware builds can keep running on systems that set values
> other than 40 (which could be > 40 or < 40)
> 
> So add a helper to ArmLib to read the number of supported address bits (#1)
> and take this into account in the page table code (#2), which allows
> PcdPrePiCpuMemorySize to assume a value that exceeds the capabilities of
> the CPU.
> 
> Patch #3 is mostly a cleanup patch, to switch to the new helper added in
> patch #1. No functional changes intended.
> 
> Patch #4 builds the CPU hob (and thus declares the size of the GCD memory
> space) based on the CPU capabilities rather than the value of 
> PcdPrePiCpuMemorySize, to prevent any potential regressions in memory
> utilization when we bump PcdPrePiCpuMemorySize back to 48.
> 
> Patch #5 drops the definitions of PcdPrePiCpuMemorySize, reverting its
> value back to the default 48.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Philippe Mathieu-Daude <philmd@redhat.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> 
> Ard Biesheuvel (5):
>   ArmPkg/ArmLib: add support for reading the max physical address space
>     size
>   ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account
>   ArmVirtPkg: refactor reading of the physical address space size
>   ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD
>     space
>   ArmVirtPkg: revert PcdPrePiCpuMemorySize to is default value of 48
> 
>  ArmPkg/Include/Library/ArmLib.h               |  6 +++
>  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++
>  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++
>  .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  5 ++-
>  ArmVirtPkg/ArmVirtQemu.dsc                    |  5 ---
>  ArmVirtPkg/ArmVirtQemu.fdf                    |  1 -
>  ArmVirtPkg/ArmVirtQemuKernel.dsc              |  4 --
>  .../Include/Library/ArmVirtMemInfoLib.h       |  1 +
>  .../ArmVirtMemoryInitPeiLib.c                 |  7 +++-
>  .../ArmVirtMemoryInitPeiLib.inf               |  1 +
>  .../QemuVirtMemInfoLib/AArch64/PhysAddrTop.S  | 39 -------------------
>  .../QemuVirtMemInfoLib/Arm/PhysAddrTop.S      | 24 ------------
>  .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   |  6 +--
>  .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  7 ----
>  .../QemuVirtMemInfoPeiLib.inf                 |  7 ----
>  .../XenVirtMemInfoLib/AArch64/PhysAddrTop.S   | 39 -------------------
>  .../XenVirtMemInfoLib/Arm/PhysAddrTop.S       | 24 ------------
>  .../XenVirtMemInfoLib/XenVirtMemInfoLib.c     |  8 +---
>  .../XenVirtMemInfoLib/XenVirtMemInfoLib.inf   |  6 ---
>  .../PrePi/ArmVirtPrePiUniCoreRelocatable.inf  |  3 --
>  ArmVirtPkg/PrePi/PrePi.c                      |  3 --
>  21 files changed, 46 insertions(+), 174 deletions(-)
>  delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> 

I've worked way too much over this weekend & now I'm tired; I'll try to
get to this series some time next week. Thanks for your patience!

Laszlo

Re: [PATCH 0/5] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit

[Laszlo Ersek]

Hi Ard,

On 11/23/18 13:14, Ard Biesheuvel wrote:
> The ArmVirtQemu targets currently limit the size of the IPA space to
> 40 bits because that is all what KVM supports. However, this is about
> to change, and so we need to update the code if we want to ensure that
> our UEFI firmware builds can keep running on systems that set values
> other than 40 (which could be > 40 or < 40)
> 
> So add a helper to ArmLib to read the number of supported address bits (#1)
> and take this into account in the page table code (#2), which allows
> PcdPrePiCpuMemorySize to assume a value that exceeds the capabilities of
> the CPU.
> 
> Patch #3 is mostly a cleanup patch, to switch to the new helper added in
> patch #1. No functional changes intended.
> 
> Patch #4 builds the CPU hob (and thus declares the size of the GCD memory
> space) based on the CPU capabilities rather than the value of 
> PcdPrePiCpuMemorySize, to prevent any potential regressions in memory
> utilization when we bump PcdPrePiCpuMemorySize back to 48.
> 
> Patch #5 drops the definitions of PcdPrePiCpuMemorySize, reverting its
> value back to the default 48.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Philippe Mathieu-Daude <philmd@redhat.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> 
> Ard Biesheuvel (5):
>   ArmPkg/ArmLib: add support for reading the max physical address space
>     size
>   ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account
>   ArmVirtPkg: refactor reading of the physical address space size
>   ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD
>     space
>   ArmVirtPkg: revert PcdPrePiCpuMemorySize to is default value of 48
> 
>  ArmPkg/Include/Library/ArmLib.h               |  6 +++
>  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++
>  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++
>  .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  5 ++-
>  ArmVirtPkg/ArmVirtQemu.dsc                    |  5 ---
>  ArmVirtPkg/ArmVirtQemu.fdf                    |  1 -
>  ArmVirtPkg/ArmVirtQemuKernel.dsc              |  4 --
>  .../Include/Library/ArmVirtMemInfoLib.h       |  1 +
>  .../ArmVirtMemoryInitPeiLib.c                 |  7 +++-
>  .../ArmVirtMemoryInitPeiLib.inf               |  1 +
>  .../QemuVirtMemInfoLib/AArch64/PhysAddrTop.S  | 39 -------------------
>  .../QemuVirtMemInfoLib/Arm/PhysAddrTop.S      | 24 ------------
>  .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   |  6 +--
>  .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  7 ----
>  .../QemuVirtMemInfoPeiLib.inf                 |  7 ----
>  .../XenVirtMemInfoLib/AArch64/PhysAddrTop.S   | 39 -------------------
>  .../XenVirtMemInfoLib/Arm/PhysAddrTop.S       | 24 ------------
>  .../XenVirtMemInfoLib/XenVirtMemInfoLib.c     |  8 +---
>  .../XenVirtMemInfoLib/XenVirtMemInfoLib.inf   |  6 ---
>  .../PrePi/ArmVirtPrePiUniCoreRelocatable.inf  |  3 --
>  ArmVirtPkg/PrePi/PrePi.c                      |  3 --
>  21 files changed, 46 insertions(+), 174 deletions(-)
>  delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> 

sorry, I'm confused. In the first place, I don't understand what
PcdPrePiCpuMemorySize stands for. I checked the declaration in
"EmbeddedPkg/EmbeddedPkg.dec", and there is no comment on it.

And, this patch set doesn't clear it up for me. According to patch #2,
it will not dictate the size of the address space that we map 1:1.
According to patch #3 it will also not dictate the size of the GCD
memory address space.

So: what *is* it good for? Can you add a patch for
EmbeddedPkg/EmbeddedPkg.dec that supplies the precise specification?

Thanks,
Laszlo

Re: [PATCH 0/5] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit

[Laszlo Ersek]

On 11/23/18 13:14, Ard Biesheuvel wrote:
> The ArmVirtQemu targets currently limit the size of the IPA space to
> 40 bits because that is all what KVM supports. However, this is about
> to change, and so we need to update the code if we want to ensure that
> our UEFI firmware builds can keep running on systems that set values
> other than 40 (which could be > 40 or < 40)
> 
> So add a helper to ArmLib to read the number of supported address bits (#1)
> and take this into account in the page table code (#2), which allows
> PcdPrePiCpuMemorySize to assume a value that exceeds the capabilities of
> the CPU.
> 
> Patch #3 is mostly a cleanup patch, to switch to the new helper added in
> patch #1. No functional changes intended.
> 
> Patch #4 builds the CPU hob (and thus declares the size of the GCD memory
> space) based on the CPU capabilities rather than the value of 
> PcdPrePiCpuMemorySize, to prevent any potential regressions in memory
> utilization when we bump PcdPrePiCpuMemorySize back to 48.
> 
> Patch #5 drops the definitions of PcdPrePiCpuMemorySize, reverting its
> value back to the default 48.

Staring a bit more at this, I wonder if it were helpful to reorder the
patches like this (just thinking loudly, I'm not even suggesting it, I'm
just curious about your opinion):

- patch #1: keep in place (introduce new helper)
- patch #2: current patch #3 (ArmVirtPkg refactoring), to benefit from
  the new helper at once, without any relation to the feature that's the
  end goal here.
- patch #3: current patch #2, build page tables with CPU PA limits
  accounted for
- patch #4: current patch #4, build GCD memory space map with CPU PA
  limits accounted for
- patch #5: remove the traces of the now useless PCD from ArmVirt

So basically, swap around #2 and #3. It's not really important; the
reason I'm thinking of it is the following though: while removing the
40-bit limitation, my first thought is, "what are the current consumers,
what needs to be updated".

The current structuring of the series, with patch #3 in the middle,
suggests that ArmVirtMemInfoLib instances are consumers. That's not the
case however, they already fetch the CPU PA limits dynamically. So they
need no functional updates, just a cleanup / centralization. That's why
I'd find it helpful to handle ArmVirtMemInfoLib right after the
introduction of the helper.

The actual consumers (in need of functional updates) are the page tables
and the GCD memory space map (two concepts, not three).

If you think this would be an improvement, please consider the
reordering. No need to repost just for this.

Thanks!
Laszlo


> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Philippe Mathieu-Daude <philmd@redhat.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> 
> Ard Biesheuvel (5):
>   ArmPkg/ArmLib: add support for reading the max physical address space
>     size
>   ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account
>   ArmVirtPkg: refactor reading of the physical address space size
>   ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD
>     space
>   ArmVirtPkg: revert PcdPrePiCpuMemorySize to is default value of 48
> 
>  ArmPkg/Include/Library/ArmLib.h               |  6 +++
>  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S | 16 ++++++++
>  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S     |  8 ++++
>  .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  5 ++-
>  ArmVirtPkg/ArmVirtQemu.dsc                    |  5 ---
>  ArmVirtPkg/ArmVirtQemu.fdf                    |  1 -
>  ArmVirtPkg/ArmVirtQemuKernel.dsc              |  4 --
>  .../Include/Library/ArmVirtMemInfoLib.h       |  1 +
>  .../ArmVirtMemoryInitPeiLib.c                 |  7 +++-
>  .../ArmVirtMemoryInitPeiLib.inf               |  1 +
>  .../QemuVirtMemInfoLib/AArch64/PhysAddrTop.S  | 39 -------------------
>  .../QemuVirtMemInfoLib/Arm/PhysAddrTop.S      | 24 ------------
>  .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.c   |  6 +--
>  .../QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf |  7 ----
>  .../QemuVirtMemInfoPeiLib.inf                 |  7 ----
>  .../XenVirtMemInfoLib/AArch64/PhysAddrTop.S   | 39 -------------------
>  .../XenVirtMemInfoLib/Arm/PhysAddrTop.S       | 24 ------------
>  .../XenVirtMemInfoLib/XenVirtMemInfoLib.c     |  8 +---
>  .../XenVirtMemInfoLib/XenVirtMemInfoLib.inf   |  6 ---
>  .../PrePi/ArmVirtPrePiUniCoreRelocatable.inf  |  3 --
>  ArmVirtPkg/PrePi/PrePi.c                      |  3 --
>  21 files changed, 46 insertions(+), 174 deletions(-)
>  delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/AArch64/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/QemuVirtMemInfoLib/Arm/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/AArch64/PhysAddrTop.S
>  delete mode 100644 ArmVirtPkg/Library/XenVirtMemInfoLib/Arm/PhysAddrTop.S
> 

Re: [PATCH 0/5] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit

[Ard Biesheuvel]

On Mon, 26 Nov 2018 at 11:22, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/23/18 13:14, Ard Biesheuvel wrote:
> > The ArmVirtQemu targets currently limit the size of the IPA space to
> > 40 bits because that is all what KVM supports. However, this is about
> > to change, and so we need to update the code if we want to ensure that
> > our UEFI firmware builds can keep running on systems that set values
> > other than 40 (which could be > 40 or < 40)
> >
> > So add a helper to ArmLib to read the number of supported address bits (#1)
> > and take this into account in the page table code (#2), which allows
> > PcdPrePiCpuMemorySize to assume a value that exceeds the capabilities of
> > the CPU.
> >
> > Patch #3 is mostly a cleanup patch, to switch to the new helper added in
> > patch #1. No functional changes intended.
> >
> > Patch #4 builds the CPU hob (and thus declares the size of the GCD memory
> > space) based on the CPU capabilities rather than the value of
> > PcdPrePiCpuMemorySize, to prevent any potential regressions in memory
> > utilization when we bump PcdPrePiCpuMemorySize back to 48.
> >
> > Patch #5 drops the definitions of PcdPrePiCpuMemorySize, reverting its
> > value back to the default 48.
>
> Staring a bit more at this, I wonder if it were helpful to reorder the
> patches like this (just thinking loudly, I'm not even suggesting it, I'm
> just curious about your opinion):
>
> - patch #1: keep in place (introduce new helper)
> - patch #2: current patch #3 (ArmVirtPkg refactoring), to benefit from
>   the new helper at once, without any relation to the feature that's the
>   end goal here.
> - patch #3: current patch #2, build page tables with CPU PA limits
>   accounted for
> - patch #4: current patch #4, build GCD memory space map with CPU PA
>   limits accounted for
> - patch #5: remove the traces of the now useless PCD from ArmVirt
>
> So basically, swap around #2 and #3. It's not really important; the
> reason I'm thinking of it is the following though: while removing the
> 40-bit limitation, my first thought is, "what are the current consumers,
> what needs to be updated".
>
> The current structuring of the series, with patch #3 in the middle,
> suggests that ArmVirtMemInfoLib instances are consumers. That's not the
> case however, they already fetch the CPU PA limits dynamically. So they
> need no functional updates, just a cleanup / centralization. That's why
> I'd find it helpful to handle ArmVirtMemInfoLib right after the
> introduction of the helper.
>
> The actual consumers (in need of functional updates) are the page tables
> and the GCD memory space map (two concepts, not three).
>
> If you think this would be an improvement, please consider the
> reordering. No need to repost just for this.
>

Yes, I think that makes sense.