On 07/04/2019 19:19, Ard Biesheuvel wrote: > 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? I don't think it would, at least from an architectural perspective. But any CPU that has a non RAO/WI SCTLR.CP15BEN must also support the architected barriers, otherwise it wouldn't be able to flip them back on. I believe this is the case on all ARMv6 CPUs. We could write something like: mrc p15, 0, r1, c1, c0, 0 @ read SCTLR tst r1, r1, #(1 << 5) bne 1f orr r1, r1, #(1 << 5) @ CP15 barrier instructions mcr p15, 0, r1, c1, c0, 0 @ write SCTLR isb 1: What do you think? > >> 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. Absolutely. These are two distinct issues, and the first one definitely needs addressing. Thanks, M. -- Jazz is not dead. It just smells funny...
