Hi Shanker, Robin, 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 >>> @@ -30,6 +30,7 @@ >>> #include <asm/pgtable-hwdef.h> >>> #include <asm/ptrace.h> >>> #include <asm/thread_info.h> >>> +#include <asm/alternative.h> >>> >>> /* >>> * Enable and disable interrupts. >>> @@ -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? 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. >> Do we really need the branch here? It's not like enabling the MMU is >> something we do on the syscall fastpath, and I can't imagine an extra >> ISB hurts much (and is probably comparable to a mispredicted branch > I don't have any strong opinion on whether to use an ISB conditionally > or unconditionally. Yes, the current kernel code is not touching > SCTLR_ELn register on the system call fast path. I would like to keep > it as a conditional ISB in case if the future kernel accesses the > SCTLR_ELn on the fast path. An extra ISB should not hurt a lot but I > believe it has more overhead than the TBZ+branch mis-prediction on Falkor > CPU. This patch has been tested on the real hardware to fix the problem. > I'm open to change to an unconditional ISB if it's the better fix. > >> anyway). In fact, is there any noticeable hit on other >> microarchitectures if we save the alternative bother and just do it >> unconditionally always? >> > > I can't comment on the performance impacts of other CPUs since I don't > have access to their development platforms. 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. > prior to SCTLR_ELn register update. Let's see what rest of the ARM > maintainers think about always using an ISB instead of alternatives. Thanks, James -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
