On 07/04/2019 19:19, Ard Biesheuvel wrote:
> On Sun, 31 Mar 2019 at 10:47, Marc Zyngier <[email protected]> wrote:
>>
>> On Sat, 30 Mar 2019 13:10:58 +0000,
>> Ard Biesheuvel <[email protected]> wrote:
>>>
>>> On Sat, 30 Mar 2019 at 10:50, Marc Zyngier <[email protected]> wrote:
>>>>
>>>> Hi Ard,
>>>>
>>>> On Fri, 29 Mar 2019 18:24:18 +0000,
>>>> Ard Biesheuvel <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: Ard Biesheuvel <[email protected]>
>>>>> ---
>>>>> 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...
