[PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits"

[PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits"

[Ard Biesheuvel]

This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286.

On AArch64, we can only use 48 address bits while running in UEFI,
while the GCD and UEFI memory maps may describe up to 52 bits of
physical address space. For this reason, MAX_ADDRESS was reduced
to 48 bits, to ensure that the firmware does not inadvertently
attempt to allocate memory that we cannot access.

However, MAX_ADDRESS is used in runtime drivers as well, and
runtime drivers may deal with kernel virtual addresses, which have
bits [63:48] set. In fact, the OS may be running with 64 KB pages
and pass addresses into the runtime services that use up to 52
bits of address space, either with the top bits set or cleared,
even if the physical address space does not extend beyond 48 bits.

In summary, changing MAX_ADDRESS is a mistake, and needs to be
reverted.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Philippe Mathieu-Daude <philmd@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Include/AArch64/ProcessorBind.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
index dad75df1c579..968c18f915ae 100644
--- a/MdePkg/Include/AArch64/ProcessorBind.h
+++ b/MdePkg/Include/AArch64/ProcessorBind.h
@@ -138,9 +138,9 @@ typedef INT64   INTN;
 #define MAX_2_BITS  0xC000000000000000ULL
 
 ///
-/// Maximum legal AARCH64  address (48 bits for 4 KB page size)
+/// Maximum legal AARCH64  address
 ///
-#define MAX_ADDRESS   0xFFFFFFFFFFFFULL
+#define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
 
 ///
 /// Maximum legal AArch64 INTN and UINTN values.
-- 
2.19.2

Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits"

[Gao, Liming]

Oh, this is a good point of runtime service in OS part. I agree to revert it. 

Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Friday, December 07, 2018 5:37 AM
>To: edk2-devel@lists.01.org
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm
><leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek
><lersek@redhat.com>; Eric Auger <eric.auger@redhat.com>; Andrew Jones
><drjones@redhat.com>; Philippe Mathieu-Daude <philmd@redhat.com>
>Subject: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit
>MAX_ADDRESS to 48 bits"
>
>This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286.
>
>On AArch64, we can only use 48 address bits while running in UEFI,
>while the GCD and UEFI memory maps may describe up to 52 bits of
>physical address space. For this reason, MAX_ADDRESS was reduced
>to 48 bits, to ensure that the firmware does not inadvertently
>attempt to allocate memory that we cannot access.
>
>However, MAX_ADDRESS is used in runtime drivers as well, and
>runtime drivers may deal with kernel virtual addresses, which have
>bits [63:48] set. In fact, the OS may be running with 64 KB pages
>and pass addresses into the runtime services that use up to 52
>bits of address space, either with the top bits set or cleared,
>even if the physical address space does not extend beyond 48 bits.
>
>In summary, changing MAX_ADDRESS is a mistake, and needs to be
>reverted.
>
>Cc: Leif Lindholm <leif.lindholm@linaro.org>
>Cc: Liming Gao <liming.gao@intel.com>
>Cc: Laszlo Ersek <lersek@redhat.com>
>Cc: Eric Auger <eric.auger@redhat.com>
>Cc: Andrew Jones <drjones@redhat.com>
>Cc: Philippe Mathieu-Daude <philmd@redhat.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
> MdePkg/Include/AArch64/ProcessorBind.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/MdePkg/Include/AArch64/ProcessorBind.h
>b/MdePkg/Include/AArch64/ProcessorBind.h
>index dad75df1c579..968c18f915ae 100644
>--- a/MdePkg/Include/AArch64/ProcessorBind.h
>+++ b/MdePkg/Include/AArch64/ProcessorBind.h
>@@ -138,9 +138,9 @@ typedef INT64   INTN;
> #define MAX_2_BITS  0xC000000000000000ULL
>
> ///
>-/// Maximum legal AARCH64  address (48 bits for 4 KB page size)
>+/// Maximum legal AARCH64  address
> ///
>-#define MAX_ADDRESS   0xFFFFFFFFFFFFULL
>+#define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
>
> ///
> /// Maximum legal AArch64 INTN and UINTN values.
>--
>2.19.2

Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits"

[Laszlo Ersek]

On 12/06/18 22:37, Ard Biesheuvel wrote:
> This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286.
> 
> On AArch64, we can only use 48 address bits while running in UEFI,
> while the GCD and UEFI memory maps may describe up to 52 bits of
> physical address space. For this reason, MAX_ADDRESS was reduced
> to 48 bits, to ensure that the firmware does not inadvertently
> attempt to allocate memory that we cannot access.
> 
> However, MAX_ADDRESS is used in runtime drivers as well, and
> runtime drivers may deal with kernel virtual addresses, which have
> bits [63:48] set. In fact, the OS may be running with 64 KB pages
> and pass addresses into the runtime services that use up to 52
> bits of address space, either with the top bits set or cleared,
> even if the physical address space does not extend beyond 48 bits.
> 
> In summary, changing MAX_ADDRESS is a mistake, and needs to be
> reverted.
> 
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: Andrew Jones <drjones@redhat.com>
> Cc: Philippe Mathieu-Daude <philmd@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdePkg/Include/AArch64/ProcessorBind.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> index dad75df1c579..968c18f915ae 100644
> --- a/MdePkg/Include/AArch64/ProcessorBind.h
> +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> @@ -138,9 +138,9 @@ typedef INT64   INTN;
>  #define MAX_2_BITS  0xC000000000000000ULL
>  
>  ///
> -/// Maximum legal AARCH64  address (48 bits for 4 KB page size)
> +/// Maximum legal AARCH64  address
>  ///
> -#define MAX_ADDRESS   0xFFFFFFFFFFFFULL
> +#define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
>  
>  ///
>  /// Maximum legal AArch64 INTN and UINTN values.
> 

I was worried the patch could regress some things, but unfortunately, I
couldn't name any specific area of concern. Sorry about that.

For this change:

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

Topic change: the patch that's being reverted was originally posted as:

  [edk2] [PATCH v3 05/16]
  MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits

in the series

  [edk2] [PATCH v3 00/16]
  [Arm|ArmVirt|MdePkg|Embedded]Pkg: lift 40-bit IPA space limit

In further patches of that series, we depended on the lowered limit of
MAX_ADDRESS. Given that MAX_ADDRESS is being raised back to its original
value, I think those dependent locations should be re-checked.

For example, in

  [edk2] [PATCH v3 08/16]
  ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account

(commit e36b243c7178), we added

  //
  // Limit the virtual address space to what we can actually use: UEFI
  // mandates a 1:1 mapping, so no point in making the virtual address
  // space larger than the physical address space. We also have to take
  // into account the architectural limitations that result from UEFI's
  // use of 4 KB pages.
  //
  MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1,
                    MAX_ADDRESS);

Presumably, we should now replace MAX_ADDRESS with 0xFFFFFFFFFFFFULL.

(I'm unsure if other modules updated by the rest of the patches are
affected -- I tried to grep them for MAX_ADDRESS, and I couldn't find
any (obvious) matches.)

Thanks,
Laszlo

Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits"

[Ard Biesheuvel]

On Fri, 7 Dec 2018 at 11:41, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/06/18 22:37, Ard Biesheuvel wrote:
> > This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286.
> >
> > On AArch64, we can only use 48 address bits while running in UEFI,
> > while the GCD and UEFI memory maps may describe up to 52 bits of
> > physical address space. For this reason, MAX_ADDRESS was reduced
> > to 48 bits, to ensure that the firmware does not inadvertently
> > attempt to allocate memory that we cannot access.
> >
> > However, MAX_ADDRESS is used in runtime drivers as well, and
> > runtime drivers may deal with kernel virtual addresses, which have
> > bits [63:48] set. In fact, the OS may be running with 64 KB pages
> > and pass addresses into the runtime services that use up to 52
> > bits of address space, either with the top bits set or cleared,
> > even if the physical address space does not extend beyond 48 bits.
> >
> > In summary, changing MAX_ADDRESS is a mistake, and needs to be
> > reverted.
> >
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Eric Auger <eric.auger@redhat.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > Cc: Philippe Mathieu-Daude <philmd@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  MdePkg/Include/AArch64/ProcessorBind.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > index dad75df1c579..968c18f915ae 100644
> > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > @@ -138,9 +138,9 @@ typedef INT64   INTN;
> >  #define MAX_2_BITS  0xC000000000000000ULL
> >
> >  ///
> > -/// Maximum legal AARCH64  address (48 bits for 4 KB page size)
> > +/// Maximum legal AARCH64  address
> >  ///
> > -#define MAX_ADDRESS   0xFFFFFFFFFFFFULL
> > +#define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
> >
> >  ///
> >  /// Maximum legal AArch64 INTN and UINTN values.
> >
>
> I was worried the patch could regress some things, but unfortunately, I
> couldn't name any specific area of concern. Sorry about that.
>

Sometimes a hunch should be taken more seriously, I suppose :-)

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

Thanks

> Topic change: the patch that's being reverted was originally posted as:
>
>   [edk2] [PATCH v3 05/16]
>   MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits
>
> in the series
>
>   [edk2] [PATCH v3 00/16]
>   [Arm|ArmVirt|MdePkg|Embedded]Pkg: lift 40-bit IPA space limit
>
> In further patches of that series, we depended on the lowered limit of
> MAX_ADDRESS. Given that MAX_ADDRESS is being raised back to its original
> value, I think those dependent locations should be re-checked.
>
> For example, in
>
>   [edk2] [PATCH v3 08/16]
>   ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account
>
> (commit e36b243c7178), we added
>
>   //
>   // Limit the virtual address space to what we can actually use: UEFI
>   // mandates a 1:1 mapping, so no point in making the virtual address
>   // space larger than the physical address space. We also have to take
>   // into account the architectural limitations that result from UEFI's
>   // use of 4 KB pages.
>   //
>   MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1,
>                     MAX_ADDRESS);
>
> Presumably, we should now replace MAX_ADDRESS with 0xFFFFFFFFFFFFULL.
>
> (I'm unsure if other modules updated by the rest of the patches are
> affected -- I tried to grep them for MAX_ADDRESS, and I couldn't find
> any (obvious) matches.)
>

I am looking into an alternative approach, involving the introduction
of MAX_ALLOC_ADDRESS, and updating the page allocator to honour it.
The MMU library should be updated to refer to it as well, thanks for
reminding me.

Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits"

[Ard Biesheuvel]

On Fri, 7 Dec 2018 at 11:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 7 Dec 2018 at 11:41, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 12/06/18 22:37, Ard Biesheuvel wrote:
> > > This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286.
> > >
> > > On AArch64, we can only use 48 address bits while running in UEFI,
> > > while the GCD and UEFI memory maps may describe up to 52 bits of
> > > physical address space. For this reason, MAX_ADDRESS was reduced
> > > to 48 bits, to ensure that the firmware does not inadvertently
> > > attempt to allocate memory that we cannot access.
> > >
> > > However, MAX_ADDRESS is used in runtime drivers as well, and
> > > runtime drivers may deal with kernel virtual addresses, which have
> > > bits [63:48] set. In fact, the OS may be running with 64 KB pages
> > > and pass addresses into the runtime services that use up to 52
> > > bits of address space, either with the top bits set or cleared,
> > > even if the physical address space does not extend beyond 48 bits.
> > >
> > > In summary, changing MAX_ADDRESS is a mistake, and needs to be
> > > reverted.
> > >
> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Eric Auger <eric.auger@redhat.com>
> > > Cc: Andrew Jones <drjones@redhat.com>
> > > Cc: Philippe Mathieu-Daude <philmd@redhat.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  MdePkg/Include/AArch64/ProcessorBind.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> > > index dad75df1c579..968c18f915ae 100644
> > > --- a/MdePkg/Include/AArch64/ProcessorBind.h
> > > +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> > > @@ -138,9 +138,9 @@ typedef INT64   INTN;
> > >  #define MAX_2_BITS  0xC000000000000000ULL
> > >
> > >  ///
> > > -/// Maximum legal AARCH64  address (48 bits for 4 KB page size)
> > > +/// Maximum legal AARCH64  address
> > >  ///
> > > -#define MAX_ADDRESS   0xFFFFFFFFFFFFULL
> > > +#define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
> > >
> > >  ///
> > >  /// Maximum legal AArch64 INTN and UINTN values.
> > >
> >
> > I was worried the patch could regress some things, but unfortunately, I
> > couldn't name any specific area of concern. Sorry about that.
> >
>
> Sometimes a hunch should be taken more seriously, I suppose :-)
>
> > For this change:
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
>

Pushed as 9601046bf459..a5274cdc87d9

Thanks all

Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits"

[Leif Lindholm]

On Fri, Dec 07, 2018 at 11:43:14AM +0100, Ard Biesheuvel wrote:
> > I was worried the patch could regress some things, but unfortunately, I
> > couldn't name any specific area of concern. Sorry about that.
> >
> 
> Sometimes a hunch should be taken more seriously, I suppose :-)

Nah, that's what the period right after the stable tag is for. And git
revert :)

Have a retroactive
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
for the list archive.

/
    Leif