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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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