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

[Laszlo Ersek <lersek@redhat.com>]

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. :)

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.

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


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

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

Thanks
Laszlo