| Message ID | 20181128143357.991-6-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 82379bf6603274e81604d5a6f6bb14bdde616286 |
| Headers | show |
| Series | Pkg: lift 40-bit IPA space limit | expand |
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
> -----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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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.