On Mon, 6 Jun 2022 23:00:08 +0200 (CEST)
Mark Kettenis <mark.kette...@xs4all.nl> wrote:
Hi Mark,
> > From: Andre Przywara <andre.przyw...@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);
> }
>
>
