Hi James,
On 11/10/2017 04:24 AM, James Morse wrote:
> 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?
>
Yes, It's not required in this case. I'll post a v2 patch and apply the workaround
where it's absolutely required. Seems handling a workaround inside helper macros
causing confusion.
>
>>> 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
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
