From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=liming.gao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2897421196808 for ; Thu, 29 Nov 2018 07:19:36 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Nov 2018 07:19:35 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,295,1539673200"; d="scan'208";a="95765589" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga006.jf.intel.com with ESMTP; 29 Nov 2018 07:19:35 -0800 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 29 Nov 2018 07:19:35 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 29 Nov 2018 07:19:34 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.203]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.46]) with mapi id 14.03.0415.000; Thu, 29 Nov 2018 23:19:32 +0800 From: "Gao, Liming" To: Laszlo Ersek , Ard Biesheuvel CC: Andrew Jones , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH v3 05/16] MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits Thread-Index: AQHUhyeC3oYbikQkNEe+FWStcXnWVaVk/24AgAEMIoCAAA7vAIAAw3rQ Date: Thu, 29 Nov 2018 15:19:32 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E380C77@SHSMSX104.ccr.corp.intel.com> References: <20181128143357.991-1-ard.biesheuvel@linaro.org> <20181128143357.991-6-ard.biesheuvel@linaro.org> <8ad971d4-c8d8-2f6e-0d60-c61e4ed362d7@redhat.com> <4806d353-eeed-88a2-55ba-18cace17e281@redhat.com> In-Reply-To: <4806d353-eeed-88a2-55ba-18cace17e281@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiN2NlY2Y4MjMtMmMwMi00NWEzLTlmYjktMWU5ZDZkNzU0YTg4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiU2ZTZ2VkVnRQcjNuZWJWOWRyK1ErWncraFFMZDExdnNVVWdDQ3JOODV0K3VuYWhMWXZMNXNvclVodnlrTlwva1EifQ== dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v3 05/16] MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Nov 2018 15:19:36 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of La= szlo Ersek > Sent: Thursday, November 29, 2018 7:34 PM > To: Ard Biesheuvel > Cc: Andrew Jones ; edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH v3 05/16] MdePkg/ProcessorBind.h AARCH64: limi= t MAX_ADDRESS to 48 bits >=20 > On 11/29/18 11:40, Ard Biesheuvel wrote: > > On Wed, 28 Nov 2018 at 19:41, Laszlo Ersek 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 > >>> Reviewed-by: Leif Lindholm > >>> --- > >>> 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@redha= t.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 afte= r > >> 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? >=20 > I can't tell whether this MAX_UINTN definition is for BaseTools itself > (i.e., "native") or for the build target arch. >=20 > "CommonLib.c" has a number of instances of MAX_UINTN... Hm, they are in > the following two functions: >=20 > - StrDecimalToUintnS() -- wrapped by StrDecimalToUintn(), > StrToIpv4Address(), and StrToIpv6Address()) >=20 > - StrHexToUintnS() -- wrapped by StrHexToUintn(), and > StrToIpv6Address(). >=20 > I tried to look at where those are called from, and the picture remains > muddled. >=20 > 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 r= esult no matter on the host with the different arch.=20 For this change in MdePkg, it makes sense to me. Reviewed-by: Liming Gao >=20 > Thanks > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel