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

["Gao, Liming" <liming.gao@intel.com>]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, November 29, 2018 7:34 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Andrew Jones <drjones@redhat.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v3 05/16] MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits
> 
> On 11/29/18 11:40, Ard Biesheuvel wrote:
> > 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?
> 
> I can't tell whether this MAX_UINTN definition is for BaseTools itself
> (i.e., "native") or for the build target arch.
> 
> "CommonLib.c" has a number of instances of MAX_UINTN... Hm, they are in
> the following two functions:
> 
> - StrDecimalToUintnS() -- wrapped by StrDecimalToUintn(),
>   StrToIpv4Address(), and StrToIpv6Address())
> 
> - StrHexToUintnS() -- wrapped by StrHexToUintn(), and
>   StrToIpv6Address().
> 
> I tried to look at where those are called from, and the picture remains
> muddled.
> 
> For example, StrHexToUintn() is called from
> "BaseTools/Source/C/DevicePath/DevicePathFromText.c", functions
> EisaIdFromText() and DevPathFromTextNVMe(). These functions seem to
> compose binary devpath nodes from text *during the build*, likely for
> the firmware-under-build to consume as-is. Hence, this use of MAX_UINTN
> -- through StrHexToUintn() -- qualifies as *native* use. And for that,
> the MAX_UINTN definition in "CommonLib.h" looks wrong, independently of
> your patch series.

I agree this is an issue in BaseTools. BaseTools should generate the same result no matter on the host with the different arch. 

For this change in MdePkg, it makes sense to me. Reviewed-by: Liming Gao <liming.gao@intel.com>
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel