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

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

On Wed, 28 Nov 2018 at 19:41, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/28/18 15:33, Ard Biesheuvel wrote:
> > AArch64 supports the use of more than 48 bits for physical and/or
> > virtual addressing, but only if the page size is set to 64 KB,
> > which is not supported by UEFI. So redefine MAX_ADDRESS to cover
> > only 48 address bits.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Reviewed-by: Leif Lindholm <leif.lindholm@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 968c18f915ae..dad75df1c579 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
> > +/// Maximum legal AARCH64  address (48 bits for 4 KB page size)
> >  ///
> > -#define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
> > +#define MAX_ADDRESS   0xFFFFFFFFFFFFULL
> >
> >  ///
> >  /// Maximum legal AArch64 INTN and UINTN values.
> >
>
> Hmmm.
>
> I bit the bullet and grepped the tree for MAX_ADDRESS.
>
> The amount of hits is staggering. I can't audit all of them.
>
> Generally, MAX_ADDRESS seems to be used in checks that prevent address
> wrap-around. In that regard, this change looks valid.
>
> I can't guarantee this change won't regress anything though. In the
> previous posting of this patch, I asked Liming some questions (IIRC):
>
> http://mid.mail-archive.com/6f1209fb-bb89-a70f-ba0e-3ebf2e12e459@redhat.com
>
> It would be nice to see answers. :)
>

Yep

> In addition:
>
> (a) in "BaseTools/Source/C/Include/AArch64/ProcessorBind.h", we have
> another instance of the macro definition. I suspect it should be kept in
> sync.
>

Indeed.

> (b) in "BaseTools/Source/C/Common/CommonLib.h", we have:
>
> #define MAX_UINTN MAX_ADDRESS
>
> which I think relies on (a), and hence it will be amusingly wrong after
> we synchronize (a) with MdePkg.
>
> (BTW, (b) is exactly the kind of assumption that scares me about this
> patch.)
>

That doesn't make any sense at all. What does 'native' mean in the
context of BaseTools anyway?

> We're not much past the last stable tag (edk2-stable201811), so let's
> hope there's going to be enough time to catch any regressions.
>
> With (a) and (b) investigated / fixed up, I'd be willing to A-b this.
> Cautiously :)
>

Thanks

> Anyway, this is for MdePkg, so my review is not required. (I certainly
> do not intend to *oppose* this patch.)
>
> Thanks
> Laszlo