[PATCH] armv8: Fix TCR 64-bit writes

[PATCH] armv8: Fix TCR 64-bit writes

[Andre Przywara]

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(-)

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

RE: [PATCH] armv8: Fix TCR 64-bit writes

[Peng Fan]

> 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

Re: [PATCH] armv8: Fix TCR 64-bit writes

[Peter Collingbourne]

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

Re: [PATCH] armv8: Fix TCR 64-bit writes

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] armv8: Fix TCR 64-bit writes

[Mark Kettenis]

> 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);
 	}

Re: [PATCH] armv8: Fix TCR 64-bit writes

[Andre Przywara]

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);
>  	}
> 
>