| Message ID | 20220509160849.246793-1-andre.przywara@arm.com |
|---|---|
| State | Accepted |
| Commit | 2b641211c51d5357eacadf571d2ee70da16749e1 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | armv8: Fix TCR 64-bit writes | expand |
> Subject: [PATCH] armv8: Fix TCR 64-bit writes > > The AArch64 TCR_ELx register is a 64-bit register, and many newer architecture > features use bits in the upper half. So far U-Boot was igorant of those bits, > trying to leave them alone. > However, in an effort to set bit 31 to 1, it failed doing so, because the compiler > sign-extended "1 << 31", so that all bits[63:31] got set. > > Older ARMv8.0 cores don't define anything dangerous up there, but newer > architecture revisions do, and setting all those bits will end badly: > ================= > $ qemu-system-aarch64 -cpu max .... > U-Boot 2022.07-rc1 (May 09 2022 - 15:21:00 +0100) > > DRAM: 1.5 GiB > ================= (hangs here) > > Defining TCR_ELx_RSVD to "1U << 31" avoids the sign-extension, so all upper > bits stay at a safe 0 value. This means no more surprises when U-Boot runs on a > more capable CPU core. > > Reported-by: Balaji Anandapadmanaban <Balaji.Anandapadmanaban@arm.com> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Reviewed-by: Peng Fan <peng.fan@nxp.com> > --- > arch/arm/include/asm/armv8/mmu.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/include/asm/armv8/mmu.h > b/arch/arm/include/asm/armv8/mmu.h > index fc97c55114..c36b2cf5a5 100644 > --- a/arch/arm/include/asm/armv8/mmu.h > +++ b/arch/arm/include/asm/armv8/mmu.h > @@ -99,9 +99,9 @@ > #define TCR_TG0_16K (2 << 14) > #define TCR_EPD1_DISABLE (1 << 23) > > -#define TCR_EL1_RSVD (1 << 31) > -#define TCR_EL2_RSVD (1 << 31 | 1 << 23) > -#define TCR_EL3_RSVD (1 << 31 | 1 << 23) > +#define TCR_EL1_RSVD (1U << 31) > +#define TCR_EL2_RSVD (1U << 31 | 1 << 23) > +#define TCR_EL3_RSVD (1U << 31 | 1 << 23) > > #ifndef __ASSEMBLY__ > static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr) > -- > 2.25.1
On Tue, May 10, 2022 at 12:56 AM Peng Fan <peng.fan@nxp.com> wrote: > > > Subject: [PATCH] armv8: Fix TCR 64-bit writes > > > > The AArch64 TCR_ELx register is a 64-bit register, and many newer architecture > > features use bits in the upper half. So far U-Boot was igorant of those bits, > > trying to leave them alone. > > However, in an effort to set bit 31 to 1, it failed doing so, because the compiler > > sign-extended "1 << 31", so that all bits[63:31] got set. > > > > Older ARMv8.0 cores don't define anything dangerous up there, but newer > > architecture revisions do, and setting all those bits will end badly: > > ================= > > $ qemu-system-aarch64 -cpu max .... > > U-Boot 2022.07-rc1 (May 09 2022 - 15:21:00 +0100) > > > > DRAM: 1.5 GiB > > ================= (hangs here) > > > > Defining TCR_ELx_RSVD to "1U << 31" avoids the sign-extension, so all upper > > bits stay at a safe 0 value. This means no more surprises when U-Boot runs on a > > more capable CPU core. > > > > Reported-by: Balaji Anandapadmanaban <Balaji.Anandapadmanaban@arm.com> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > Reviewed-by: Peng Fan <peng.fan@nxp.com> I ran into the same problem as you and developed the same fix. I was about to send it out before I found your patch on the list. Tested-by: Peter Collingbourne <pcc@google.com> Reviewed-by: Peter Collingbourne <pcc@google.com> You may also want to add: Fixes: ad3d6e88a1a4 ("armv8/mmu: Set bits marked RES1 in TCR") Peter
On Mon, May 09, 2022 at 05:08:49PM +0100, Andre Przywara wrote: > The AArch64 TCR_ELx register is a 64-bit register, and many newer > architecture features use bits in the upper half. So far U-Boot was > igorant of those bits, trying to leave them alone. > However, in an effort to set bit 31 to 1, it failed doing so, because > the compiler sign-extended "1 << 31", so that all bits[63:31] got set. > > Older ARMv8.0 cores don't define anything dangerous up there, but newer > architecture revisions do, and setting all those bits will end badly: > ================= > $ qemu-system-aarch64 -cpu max .... > U-Boot 2022.07-rc1 (May 09 2022 - 15:21:00 +0100) > > DRAM: 1.5 GiB > ================= (hangs here) > > Defining TCR_ELx_RSVD to "1U << 31" avoids the sign-extension, so all > upper bits stay at a safe 0 value. This means no more surprises when > U-Boot runs on a more capable CPU core. > > Reported-by: Balaji Anandapadmanaban <Balaji.Anandapadmanaban@arm.com> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > Reviewed-by: Peng Fan <peng.fan@nxp.com> > Tested-by: Peter Collingbourne <pcc@google.com> > Reviewed-by: Peter Collingbourne <pcc@google.com> Applied to u-boot/master, thanks!
> From: Andre Przywara <andre.przywara@arm.com> > Date: Mon, 9 May 2022 17:08:49 +0100 > > The AArch64 TCR_ELx register is a 64-bit register, and many newer > architecture features use bits in the upper half. So far U-Boot was > igorant of those bits, trying to leave them alone. > However, in an effort to set bit 31 to 1, it failed doing so, because > the compiler sign-extended "1 << 31", so that all bits[63:31] got set. > > Older ARMv8.0 cores don't define anything dangerous up there, but newer > architecture revisions do, and setting all those bits will end badly: > ================= > $ qemu-system-aarch64 -cpu max .... > U-Boot 2022.07-rc1 (May 09 2022 - 15:21:00 +0100) > > DRAM: 1.5 GiB > ================= (hangs here) > > Defining TCR_ELx_RSVD to "1U << 31" avoids the sign-extension, so all > upper bits stay at a safe 0 value. This means no more surprises when > U-Boot runs on a more capable CPU core. Hi Andre, This change breaks U-Boot on Apple M1 systems. Th reason is "interesting". The Apple Icestorm and Firestorm cores implement FEAT_VHE, but do so in an unusual (non-compiant?) way. the HCR_EL2.E2H bit is hardwired to 1, which means that EL2 Host is always enabled. As a consequence, TCR_EL2 has a different layout (the same as TCR_EL1). This means that the get_tcr() function in arch/arm/cpu/armv8/cache_v8.c returns the wrong value. Apparently the sign-extension that happened before accidentally set the "right" bits so I didn't notice this issue before. Below is a possible fix. This uses #ifdef CONFIG_ARCH_APPLE to avoid growing the code. A more generic way of fixing this would involve checking the value of HCR_EL2.E2H, probably in mmu_setup() and pass el=1 instead of el=2 to get_tcr() instead. Thoughts? diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c index 3de18c7675..ea3a60b2cd 100644 --- a/arch/arm/cpu/armv8/cache_v8.c +++ b/arch/arm/cpu/armv8/cache_v8.c @@ -74,7 +74,11 @@ u64 get_tcr(int el, u64 *pips, u64 *pva_bits) if (el == 1) { tcr = TCR_EL1_RSVD | (ips << 32) | TCR_EPD1_DISABLE; } else if (el == 2) { +#ifdef CONFIG_ARCH_APPLE + tcr = TCR_EL1_RSVD | (ips << 32) | TCR_EPD1_DISABLE; +#else tcr = TCR_EL2_RSVD | (ips << 16); +#endif } else { tcr = TCR_EL3_RSVD | (ips << 16); }
On Mon, 6 Jun 2022 23:00:08 +0200 (CEST) Mark Kettenis <mark.kettenis@xs4all.nl> wrote: Hi Mark, > > From: Andre Przywara <andre.przywara@arm.com> > > Date: Mon, 9 May 2022 17:08:49 +0100 > > > > The AArch64 TCR_ELx register is a 64-bit register, and many newer > > architecture features use bits in the upper half. So far U-Boot was > > igorant of those bits, trying to leave them alone. > > However, in an effort to set bit 31 to 1, it failed doing so, because > > the compiler sign-extended "1 << 31", so that all bits[63:31] got set. > > > > Older ARMv8.0 cores don't define anything dangerous up there, but newer > > architecture revisions do, and setting all those bits will end badly: > > ================= > > $ qemu-system-aarch64 -cpu max .... > > U-Boot 2022.07-rc1 (May 09 2022 - 15:21:00 +0100) > > > > DRAM: 1.5 GiB > > ================= (hangs here) > > > > Defining TCR_ELx_RSVD to "1U << 31" avoids the sign-extension, so all > > upper bits stay at a safe 0 value. This means no more surprises when > > U-Boot runs on a more capable CPU core. > > Hi Andre, > > This change breaks U-Boot on Apple M1 systems. Th reason is > "interesting". The Apple Icestorm and Firestorm cores implement > FEAT_VHE, but do so in an unusual (non-compiant?) way. Ah, the good ol' always VHE hack, biting us over and over again. > the > HCR_EL2.E2H bit is hardwired to 1, which means that EL2 Host is always > enabled. As a consequence, TCR_EL2 has a different layout (the same > as TCR_EL1). This means that the get_tcr() function in > arch/arm/cpu/armv8/cache_v8.c returns the wrong value. Apparently the > sign-extension that happened before accidentally set the "right" bits > so I didn't notice this issue before. > > Below is a possible fix. This uses #ifdef CONFIG_ARCH_APPLE to avoid > growing the code. A more generic way of fixing this would involve > checking the value of HCR_EL2.E2H, probably in mmu_setup() and pass > el=1 instead of el=2 to get_tcr() instead. > > Thoughts? I am very much in favour of fixing this properly, so by using runtime VHE detection. Looking at the code, the "el" parameter in get_tcr is actually mostly just the current EL, except in two cases, where we pass 0 (which looks wrong), and are just after the VA size bits. I will try to just determine the correct EL internally, so calling current_el() and checking HCR_EL2. Will send a patch ASAP. Cheers, Andre > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > index 3de18c7675..ea3a60b2cd 100644 > --- a/arch/arm/cpu/armv8/cache_v8.c > +++ b/arch/arm/cpu/armv8/cache_v8.c > @@ -74,7 +74,11 @@ u64 get_tcr(int el, u64 *pips, u64 *pva_bits) > if (el == 1) { > tcr = TCR_EL1_RSVD | (ips << 32) | TCR_EPD1_DISABLE; > } else if (el == 2) { > +#ifdef CONFIG_ARCH_APPLE > + tcr = TCR_EL1_RSVD | (ips << 32) | TCR_EPD1_DISABLE; > +#else > tcr = TCR_EL2_RSVD | (ips << 16); > +#endif > } else { > tcr = TCR_EL3_RSVD | (ips << 16); > } > >
diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h index fc97c55114..c36b2cf5a5 100644 --- a/arch/arm/include/asm/armv8/mmu.h +++ b/arch/arm/include/asm/armv8/mmu.h @@ -99,9 +99,9 @@ #define TCR_TG0_16K (2 << 14) #define TCR_EPD1_DISABLE (1 << 23) -#define TCR_EL1_RSVD (1 << 31) -#define TCR_EL2_RSVD (1 << 31 | 1 << 23) -#define TCR_EL3_RSVD (1 << 31 | 1 << 23) +#define TCR_EL1_RSVD (1U << 31) +#define TCR_EL2_RSVD (1U << 31 | 1 << 23) +#define TCR_EL3_RSVD (1U << 31 | 1 << 23) #ifndef __ASSEMBLY__ static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
The AArch64 TCR_ELx register is a 64-bit register, and many newer architecture features use bits in the upper half. So far U-Boot was igorant of those bits, trying to leave them alone. However, in an effort to set bit 31 to 1, it failed doing so, because the compiler sign-extended "1 << 31", so that all bits[63:31] got set. Older ARMv8.0 cores don't define anything dangerous up there, but newer architecture revisions do, and setting all those bits will end badly: ================= $ qemu-system-aarch64 -cpu max .... U-Boot 2022.07-rc1 (May 09 2022 - 15:21:00 +0100) DRAM: 1.5 GiB ================= (hangs here) Defining TCR_ELx_RSVD to "1U << 31" avoids the sign-extension, so all upper bits stay at a safe 0 value. This means no more surprises when U-Boot runs on a more capable CPU core. Reported-by: Balaji Anandapadmanaban <Balaji.Anandapadmanaban@arm.com> Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arch/arm/include/asm/armv8/mmu.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)