On Sun, 31 Mar 2019 at 10:47, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > > On Sat, 30 Mar 2019 13:10:58 +0000, > Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > > > > On Sat, 30 Mar 2019 at 10:50, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > > > > > > Hi Ard, > > > > > > On Fri, 29 Mar 2019 18:24:18 +0000, > > > Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > > > > > > > > The EFI stub is entered with the caches and MMU enabled by the > > > > firmware, and once the stub is ready to hand over to the decompressor, > > > > we clean and disable the caches. > > > > > > > > The cache clean routines use CP15 barrier instructions, which can be > > > > disabled via SCTLR. Normally, when using the provided cache handling > > > > routines to enable the caches and MMU, this bit is enabled as well. > > > > However, but since we entered the stub with the caches already enabled, > > > > this routine is not executed before we call the cache clean routines, > > > > resulting in undefined instruction exceptions if the firmware never > > > > enabled this bit. > > > > > > > > So set the bit explicitly in the EFI entry code. > > > > > > > > Cc: Marc Zyngier <marc.zyngier@xxxxxxx> > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > > > > --- > > > > arch/arm/boot/compressed/head.S | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S > > > > index 6c7ccb428c07..62a49356fca3 100644 > > > > --- a/arch/arm/boot/compressed/head.S > > > > +++ b/arch/arm/boot/compressed/head.S > > > > @@ -1438,6 +1438,16 @@ ENTRY(efi_stub_entry) > > > > > > > > @ Preserve return value of efi_entry() in r4 > > > > mov r4, r0 > > > > + > > > > + @ our cache maintenance code relies on CP15 barrier instructions > > > > + @ but since we arrived here with the MMU and caches configured > > > > + @ by UEFI, we must ensure that the use of those instructions is > > > > + @ enabled in the SCTLR register, since we never executed our own > > > > + @ cache enable routine, which is normally in charge of this. > > > > + mrc p15, 0, r1, c1, c0, 0 @ read SCTLR > > > > + orr r1, r1, #(1 << 5) @ CP15 barrier instructions > > > > + mcr p15, 0, r1, c1, c0, 0 @ write SCTLR > > > > + > > > > > > To be on the safe side, you could add an isb here. I'm pretty sure it > > > is immaterial on any ARMv7 core, but hey, I'm paranoid. > > > > > > > Well, this is actually triggering now under KVM, so I wonder if this > > is a regression of some kind on the host side. But the EFI stub > > shouldn't use CP15 barriers without enabling them, so we need this > > patch in any case. > > My remark was only about the lack of an ISB instruction after the > write to SCTLR, and you're perfectly right to enable CP15BEN. > Actually, the CP15 ISB is not usable here, and using the v7 ISB breaks v6. Would reading back SCTLR suffice? > But there is something fishy. Looking at the KVM/arm64 code, > SCTLR_EL1.CP15BEN resets to 1. If the barrier undefs in the stubs, > something must have cleared it (or the reset code is busted...). > > I've dumped SCTLR from a guest running EFI (using GDB), and I do see > SCTLR.CP15EN being set... > Yeah. I won't be able to dig into this for a while, but in the mean time, I'd like to get a guest fix in in any case.
