* [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" @ 2018-12-06 21:37 Ard Biesheuvel 2018-12-07 0:03 ` Gao, Liming 2018-12-07 10:41 ` Laszlo Ersek 0 siblings, 2 replies; 6+ messages in thread From: Ard Biesheuvel @ 2018-12-06 21:37 UTC (permalink / raw) To: edk2-devel Cc: Ard Biesheuvel, Leif Lindholm, Liming Gao, Laszlo Ersek, Eric Auger, Andrew Jones, Philippe Mathieu-Daude This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286. On AArch64, we can only use 48 address bits while running in UEFI, while the GCD and UEFI memory maps may describe up to 52 bits of physical address space. For this reason, MAX_ADDRESS was reduced to 48 bits, to ensure that the firmware does not inadvertently attempt to allocate memory that we cannot access. However, MAX_ADDRESS is used in runtime drivers as well, and runtime drivers may deal with kernel virtual addresses, which have bits [63:48] set. In fact, the OS may be running with 64 KB pages and pass addresses into the runtime services that use up to 52 bits of address space, either with the top bits set or cleared, even if the physical address space does not extend beyond 48 bits. In summary, changing MAX_ADDRESS is a mistake, and needs to be reverted. Cc: Leif Lindholm <leif.lindholm@linaro.org> Cc: Liming Gao <liming.gao@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Eric Auger <eric.auger@redhat.com> Cc: Andrew Jones <drjones@redhat.com> Cc: Philippe Mathieu-Daude <philmd@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@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 dad75df1c579..968c18f915ae 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 (48 bits for 4 KB page size) +/// Maximum legal AARCH64 address /// -#define MAX_ADDRESS 0xFFFFFFFFFFFFULL +#define MAX_ADDRESS 0xFFFFFFFFFFFFFFFFULL /// /// Maximum legal AArch64 INTN and UINTN values. -- 2.19.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" 2018-12-06 21:37 [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" Ard Biesheuvel @ 2018-12-07 0:03 ` Gao, Liming 2018-12-07 10:41 ` Laszlo Ersek 1 sibling, 0 replies; 6+ messages in thread From: Gao, Liming @ 2018-12-07 0:03 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel@lists.01.org Cc: Leif Lindholm, Laszlo Ersek, Eric Auger, Andrew Jones, Philippe Mathieu-Daude Oh, this is a good point of runtime service in OS part. I agree to revert it. Reviewed-by: Liming Gao <liming.gao@intel.com> Thanks Liming >-----Original Message----- >From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >Sent: Friday, December 07, 2018 5:37 AM >To: edk2-devel@lists.01.org >Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm ><leif.lindholm@linaro.org>; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek ><lersek@redhat.com>; Eric Auger <eric.auger@redhat.com>; Andrew Jones ><drjones@redhat.com>; Philippe Mathieu-Daude <philmd@redhat.com> >Subject: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit >MAX_ADDRESS to 48 bits" > >This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286. > >On AArch64, we can only use 48 address bits while running in UEFI, >while the GCD and UEFI memory maps may describe up to 52 bits of >physical address space. For this reason, MAX_ADDRESS was reduced >to 48 bits, to ensure that the firmware does not inadvertently >attempt to allocate memory that we cannot access. > >However, MAX_ADDRESS is used in runtime drivers as well, and >runtime drivers may deal with kernel virtual addresses, which have >bits [63:48] set. In fact, the OS may be running with 64 KB pages >and pass addresses into the runtime services that use up to 52 >bits of address space, either with the top bits set or cleared, >even if the physical address space does not extend beyond 48 bits. > >In summary, changing MAX_ADDRESS is a mistake, and needs to be >reverted. > >Cc: Leif Lindholm <leif.lindholm@linaro.org> >Cc: Liming Gao <liming.gao@intel.com> >Cc: Laszlo Ersek <lersek@redhat.com> >Cc: Eric Auger <eric.auger@redhat.com> >Cc: Andrew Jones <drjones@redhat.com> >Cc: Philippe Mathieu-Daude <philmd@redhat.com> >Contributed-under: TianoCore Contribution Agreement 1.1 >Signed-off-by: Ard Biesheuvel <ard.biesheuvel@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 dad75df1c579..968c18f915ae 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 (48 bits for 4 KB page size) >+/// Maximum legal AARCH64 address > /// >-#define MAX_ADDRESS 0xFFFFFFFFFFFFULL >+#define MAX_ADDRESS 0xFFFFFFFFFFFFFFFFULL > > /// > /// Maximum legal AArch64 INTN and UINTN values. >-- >2.19.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" 2018-12-06 21:37 [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" Ard Biesheuvel 2018-12-07 0:03 ` Gao, Liming @ 2018-12-07 10:41 ` Laszlo Ersek 2018-12-07 10:43 ` Ard Biesheuvel 1 sibling, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2018-12-07 10:41 UTC (permalink / raw) To: Ard Biesheuvel, edk2-devel Cc: Leif Lindholm, Liming Gao, Eric Auger, Andrew Jones, Philippe Mathieu-Daude On 12/06/18 22:37, Ard Biesheuvel wrote: > This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286. > > On AArch64, we can only use 48 address bits while running in UEFI, > while the GCD and UEFI memory maps may describe up to 52 bits of > physical address space. For this reason, MAX_ADDRESS was reduced > to 48 bits, to ensure that the firmware does not inadvertently > attempt to allocate memory that we cannot access. > > However, MAX_ADDRESS is used in runtime drivers as well, and > runtime drivers may deal with kernel virtual addresses, which have > bits [63:48] set. In fact, the OS may be running with 64 KB pages > and pass addresses into the runtime services that use up to 52 > bits of address space, either with the top bits set or cleared, > even if the physical address space does not extend beyond 48 bits. > > In summary, changing MAX_ADDRESS is a mistake, and needs to be > reverted. > > Cc: Leif Lindholm <leif.lindholm@linaro.org> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Eric Auger <eric.auger@redhat.com> > Cc: Andrew Jones <drjones@redhat.com> > Cc: Philippe Mathieu-Daude <philmd@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@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 dad75df1c579..968c18f915ae 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 (48 bits for 4 KB page size) > +/// Maximum legal AARCH64 address > /// > -#define MAX_ADDRESS 0xFFFFFFFFFFFFULL > +#define MAX_ADDRESS 0xFFFFFFFFFFFFFFFFULL > > /// > /// Maximum legal AArch64 INTN and UINTN values. > I was worried the patch could regress some things, but unfortunately, I couldn't name any specific area of concern. Sorry about that. For this change: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Topic change: the patch that's being reverted was originally posted as: [edk2] [PATCH v3 05/16] MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits in the series [edk2] [PATCH v3 00/16] [Arm|ArmVirt|MdePkg|Embedded]Pkg: lift 40-bit IPA space limit In further patches of that series, we depended on the lowered limit of MAX_ADDRESS. Given that MAX_ADDRESS is being raised back to its original value, I think those dependent locations should be re-checked. For example, in [edk2] [PATCH v3 08/16] ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account (commit e36b243c7178), we added // // Limit the virtual address space to what we can actually use: UEFI // mandates a 1:1 mapping, so no point in making the virtual address // space larger than the physical address space. We also have to take // into account the architectural limitations that result from UEFI's // use of 4 KB pages. // MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1, MAX_ADDRESS); Presumably, we should now replace MAX_ADDRESS with 0xFFFFFFFFFFFFULL. (I'm unsure if other modules updated by the rest of the patches are affected -- I tried to grep them for MAX_ADDRESS, and I couldn't find any (obvious) matches.) Thanks, Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" 2018-12-07 10:41 ` Laszlo Ersek @ 2018-12-07 10:43 ` Ard Biesheuvel 2018-12-07 11:26 ` Ard Biesheuvel 2018-12-07 11:54 ` Leif Lindholm 0 siblings, 2 replies; 6+ messages in thread From: Ard Biesheuvel @ 2018-12-07 10:43 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Leif Lindholm, Gao, Liming, Auger Eric, Andrew Jones, Philippe Mathieu-Daudé On Fri, 7 Dec 2018 at 11:41, Laszlo Ersek <lersek@redhat.com> wrote: > > On 12/06/18 22:37, Ard Biesheuvel wrote: > > This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286. > > > > On AArch64, we can only use 48 address bits while running in UEFI, > > while the GCD and UEFI memory maps may describe up to 52 bits of > > physical address space. For this reason, MAX_ADDRESS was reduced > > to 48 bits, to ensure that the firmware does not inadvertently > > attempt to allocate memory that we cannot access. > > > > However, MAX_ADDRESS is used in runtime drivers as well, and > > runtime drivers may deal with kernel virtual addresses, which have > > bits [63:48] set. In fact, the OS may be running with 64 KB pages > > and pass addresses into the runtime services that use up to 52 > > bits of address space, either with the top bits set or cleared, > > even if the physical address space does not extend beyond 48 bits. > > > > In summary, changing MAX_ADDRESS is a mistake, and needs to be > > reverted. > > > > Cc: Leif Lindholm <leif.lindholm@linaro.org> > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Eric Auger <eric.auger@redhat.com> > > Cc: Andrew Jones <drjones@redhat.com> > > Cc: Philippe Mathieu-Daude <philmd@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@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 dad75df1c579..968c18f915ae 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 (48 bits for 4 KB page size) > > +/// Maximum legal AARCH64 address > > /// > > -#define MAX_ADDRESS 0xFFFFFFFFFFFFULL > > +#define MAX_ADDRESS 0xFFFFFFFFFFFFFFFFULL > > > > /// > > /// Maximum legal AArch64 INTN and UINTN values. > > > > I was worried the patch could regress some things, but unfortunately, I > couldn't name any specific area of concern. Sorry about that. > Sometimes a hunch should be taken more seriously, I suppose :-) > For this change: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > Thanks > Topic change: the patch that's being reverted was originally posted as: > > [edk2] [PATCH v3 05/16] > MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits > > in the series > > [edk2] [PATCH v3 00/16] > [Arm|ArmVirt|MdePkg|Embedded]Pkg: lift 40-bit IPA space limit > > In further patches of that series, we depended on the lowered limit of > MAX_ADDRESS. Given that MAX_ADDRESS is being raised back to its original > value, I think those dependent locations should be re-checked. > > For example, in > > [edk2] [PATCH v3 08/16] > ArmPkg/ArmMmuLib: take the CPU supported maximum PA space into account > > (commit e36b243c7178), we added > > // > // Limit the virtual address space to what we can actually use: UEFI > // mandates a 1:1 mapping, so no point in making the virtual address > // space larger than the physical address space. We also have to take > // into account the architectural limitations that result from UEFI's > // use of 4 KB pages. > // > MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1, > MAX_ADDRESS); > > Presumably, we should now replace MAX_ADDRESS with 0xFFFFFFFFFFFFULL. > > (I'm unsure if other modules updated by the rest of the patches are > affected -- I tried to grep them for MAX_ADDRESS, and I couldn't find > any (obvious) matches.) > I am looking into an alternative approach, involving the introduction of MAX_ALLOC_ADDRESS, and updating the page allocator to honour it. The MMU library should be updated to refer to it as well, thanks for reminding me. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" 2018-12-07 10:43 ` Ard Biesheuvel @ 2018-12-07 11:26 ` Ard Biesheuvel 2018-12-07 11:54 ` Leif Lindholm 1 sibling, 0 replies; 6+ messages in thread From: Ard Biesheuvel @ 2018-12-07 11:26 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Leif Lindholm, Gao, Liming, Auger Eric, Andrew Jones, Philippe Mathieu-Daudé On Fri, 7 Dec 2018 at 11:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Fri, 7 Dec 2018 at 11:41, Laszlo Ersek <lersek@redhat.com> wrote: > > > > On 12/06/18 22:37, Ard Biesheuvel wrote: > > > This reverts commit 82379bf6603274e81604d5a6f6bb14bdde616286. > > > > > > On AArch64, we can only use 48 address bits while running in UEFI, > > > while the GCD and UEFI memory maps may describe up to 52 bits of > > > physical address space. For this reason, MAX_ADDRESS was reduced > > > to 48 bits, to ensure that the firmware does not inadvertently > > > attempt to allocate memory that we cannot access. > > > > > > However, MAX_ADDRESS is used in runtime drivers as well, and > > > runtime drivers may deal with kernel virtual addresses, which have > > > bits [63:48] set. In fact, the OS may be running with 64 KB pages > > > and pass addresses into the runtime services that use up to 52 > > > bits of address space, either with the top bits set or cleared, > > > even if the physical address space does not extend beyond 48 bits. > > > > > > In summary, changing MAX_ADDRESS is a mistake, and needs to be > > > reverted. > > > > > > Cc: Leif Lindholm <leif.lindholm@linaro.org> > > > Cc: Liming Gao <liming.gao@intel.com> > > > Cc: Laszlo Ersek <lersek@redhat.com> > > > Cc: Eric Auger <eric.auger@redhat.com> > > > Cc: Andrew Jones <drjones@redhat.com> > > > Cc: Philippe Mathieu-Daude <philmd@redhat.com> > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@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 dad75df1c579..968c18f915ae 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 (48 bits for 4 KB page size) > > > +/// Maximum legal AARCH64 address > > > /// > > > -#define MAX_ADDRESS 0xFFFFFFFFFFFFULL > > > +#define MAX_ADDRESS 0xFFFFFFFFFFFFFFFFULL > > > > > > /// > > > /// Maximum legal AArch64 INTN and UINTN values. > > > > > > > I was worried the patch could regress some things, but unfortunately, I > > couldn't name any specific area of concern. Sorry about that. > > > > Sometimes a hunch should be taken more seriously, I suppose :-) > > > For this change: > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > > Pushed as 9601046bf459..a5274cdc87d9 Thanks all ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" 2018-12-07 10:43 ` Ard Biesheuvel 2018-12-07 11:26 ` Ard Biesheuvel @ 2018-12-07 11:54 ` Leif Lindholm 1 sibling, 0 replies; 6+ messages in thread From: Leif Lindholm @ 2018-12-07 11:54 UTC (permalink / raw) To: Ard Biesheuvel Cc: Laszlo Ersek, edk2-devel@lists.01.org, Gao, Liming, Auger Eric, Andrew Jones, Philippe Mathieu-Daudé On Fri, Dec 07, 2018 at 11:43:14AM +0100, Ard Biesheuvel wrote: > > I was worried the patch could regress some things, but unfortunately, I > > couldn't name any specific area of concern. Sorry about that. > > > > Sometimes a hunch should be taken more seriously, I suppose :-) Nah, that's what the period right after the stable tag is for. And git revert :) Have a retroactive Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> for the list archive. / Leif ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-07 11:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-06 21:37 [PATCH] Revert "MdePkg/ProcessorBind.h AARCH64: limit MAX_ADDRESS to 48 bits" Ard Biesheuvel 2018-12-07 0:03 ` Gao, Liming 2018-12-07 10:41 ` Laszlo Ersek 2018-12-07 10:43 ` Ard Biesheuvel 2018-12-07 11:26 ` Ard Biesheuvel 2018-12-07 11:54 ` Leif Lindholm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox