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

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

Message

Ard Biesheuvel Nov. 23, 2018, 12:14 p.m.
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

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

Comments

Laszlo Ersek Nov. 25, 2018, 5:23 p.m. | #1
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 26, 2018, 9:35 a.m. | #2
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 26, 2018, 10:22 a.m. | #3
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

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 26, 2018, 11:31 a.m. | #4
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel