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

[Laszlo Ersek <lersek@redhat.com>]

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.

Thanks
Laszlo