Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[James Morse]

Hi Shanker,

On 09/11/17 15:22, Shanker Donthineni wrote:
> On 11/09/2017 05:08 AM, James Morse wrote:
>> On 04/11/17 21:43, Shanker Donthineni wrote:
>>> On 11/03/2017 10:11 AM, Robin Murphy wrote:
>>>> On 03/11/17 03:27, Shanker Donthineni wrote:
>>>>> The ARM architecture defines the memory locations that are permitted
>>>>> to be accessed as the result of a speculative instruction fetch from
>>>>> an exception level for which all stages of translation are disabled.
>>>>> Specifically, the core is permitted to speculatively fetch from the
>>>>> 4KB region containing the current program counter and next 4KB.
>>>>>
>>>>> When translation is changed from enabled to disabled for the running
>>>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>>>> Falkor core may errantly speculatively access memory locations outside
>>>>> of the 4KB region permitted by the architecture. The errant memory
>>>>> access may lead to one of the following unexpected behaviors.
>>>>>
>>>>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>>>>    to the errant memory access attempting to access a region of memory
>>>>>    that is protected by a slave-side memory protection unit.
>>>>> 2) Unpredictable device behavior due to a speculative read from device
>>>>>    memory. This behavior may only occur if the instruction cache is
>>>>>    disabled prior to or coincident with translation being changed from
>>>>>    enabled to disabled.
>>>>>
>>>>> To avoid the errant behavior, software must execute an ISB immediately
>>>>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.

>>>>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>>>>> index b6dfb4f..4c91efb 100644
>>>>> --- a/arch/arm64/include/asm/assembler.h
>>>>> +++ b/arch/arm64/include/asm/assembler.h
>>>>> @@ -514,6 +515,22 @@
>>>>>   *   reg: the value to be written.
>>>>>   */
>>>>>  	.macro	write_sctlr, eln, reg
>>>>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>>>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>>>>> +	tbnz    \reg, #0, 8000f          // enable MMU?
>>
>> Won't this match any change that leaves the MMU enabled?
> 
> Yes. No need to apply workaround if the MMU is going to be enabled.

(Sorry, looks like I had this upside down)

My badly-made-point is you can't know if the MMU is being disabled unless you
have both the old and new values.

As an example, in el2_setup, (where the MMU is disabled), we set the EE/E0E bits
to match the kernel's endianness. Won't your macro will insert an unnecessary
isb? Is this needed for the errata workaround?


>> I think the macro is making this more confusing. Disabling the MMU is obvious
>> from the call-site, (and really rare!). Trying to work it out from a macro makes
>> it more complicated than necessary.

> Not clear, are you suggesting not to use read{write}_sctlr() macros instead apply 
> the workaround from the call-site based on the MMU-on status?

Yes. This is the only way to patch only the locations that turn the MMU off.


> If yes, It simplifies
> the code logic but CONFIG_QCOM_FALKOR_ERRATUM_1041 references are scatter everywhere. 

Wouldn't they only appear in the places that are affected by the errata?
This is exactly what we want, anyone touching that code now knows they need to
double check this behaviour, (and ask you to test it!).

Otherwise we have a macro second guessing what is happening, if its not quite
right (because some information has been lost), we're now not sure what we need
to do if we ever refactor any of this code.

[...]

>>> I'll prefer alternatives
>>> just to avoid the unnecessary overhead on future Qualcomm Datacenter
>>> server CPUs and regression on other CPUs because of inserting an ISB
>>
>> I think hiding errata on other CPUs is a good argument.
>>
>> My suggestion would be:
>>> #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>> 	isb
>>> #endif
>>
>> In head.S and efi-entry.S, as these run before alternatives.
>> Then use alternatives to add just the isb in the mmu-off path for the other callers.

> Thanks for your opinion on this one, I'll change to an unconditional ISB in v2 patch.
> After this change the enable_mmu() issues two ISBs before writing to SCTLR_EL1.

Another great reason not to wrap this in a macro, there may already be a
suitable isb, in which case a comment will suffice.


> Are you okay with this behavior?

Back-to-back isb doesn't sound like a good idea.


>  ENTRY(__enable_mmu)
>         mrs     x1, ID_AA64MMFR0_EL1
>         ubfx    x2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
>         cmp     x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
>         b.ne    __no_granule_support
>         update_early_cpu_boot_status 0, x1, x2
>         adrp    x1, idmap_pg_dir
>         adrp    x2, swapper_pg_dir
>         msr     ttbr0_el1, x1                   // load TTBR0
>         msr     ttbr1_el1, x2                   // load TTBR1
>         isb
>         write_sctlr el1, x0
>         isb

Now I'm thoroughly confused. Isn't this one of the sequences that doesn't hit
the issue? Here we're switching SCTLR.M from 0 to 1.


Thanks,

James