[PATCH] arm64: errata: Add NXP iMX8QM workaround for A53 Cache coherency issue
Ivan T. Ivanov
iivanov at suse.de
Tue Apr 18 06:25:00 PDT 2023
On 04-13 12:19, Mark Rutland wrote:
> >
> > Unfortunately I was unable to find a way to identify SoC ID using
> > registers. Boot CPU MIDR_EL1 is equal to 0x410fd034. So I fallback to
> > using devicetree compatible strings for this.
>
> How does this work with KVM?
>
> VMs have no idea that the host platform is, and so will have no idea that this
> erratum applies, so they're going to blow up spectacularly.
I don't think this platform it is supposed to run KVM guests, but
who knows?! Someone suggested using SMBIOS tables or something around
this, but I am not sure how reliable way this is.
>
> So we should probably be disabling KVM (or at the very least, printing a
> gigantic warning).
I will see if I can come up with something. Suggestions are of course
welcomed.
>
> >
> > +config NXP_IMX8QM_ERRATUM_ERR050104
> > + bool "NXP iMX8QM: Workaround for Arm/A53 Cache coherency issue"
>
> Please use the erratum number, e.g.
>
Ok.
> bool "NXP iMX8QM ERR050104: broken cache/tlb invalidation"
>
> > + default n
> > + help
> > + Some maintenance operations exchanged between the A53 and A72 core
> > + clusters, involving some Translation Look-aside Buffer Invalidate
> > + (TLBI) and Instruction Cache (IC) instructions can be corrupted.
>
> Likewise, please add a more compelte description here, e.g.
>
> help
> On iMX8QM, addresses above bit 35 are not broadcast correctly for
> TLBI or IC operations, making TLBI and IC unreliable.
>
> Work around this erratum by using TLBI *ALL*IS and IC IALLUIS
> operations. EL0 use of IC IVAU is trapped and upgraded to IC IALLUIS.
>
Ok.
> > +
> > + If unsure, say N.
> > +
> > endmenu # "ARM errata workarounds via the alternatives framework"
> >
> > choice
> > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> > index 6bf013fb110d..1ed648f7f29a 100644
> > --- a/arch/arm64/include/asm/cpufeature.h
> > +++ b/arch/arm64/include/asm/cpufeature.h
> > @@ -835,7 +835,8 @@ static inline bool system_supports_bti(void)
> > static inline bool system_supports_tlb_range(void)
> > {
> > return IS_ENABLED(CONFIG_ARM64_TLB_RANGE) &&
> > - cpus_have_const_cap(ARM64_HAS_TLB_RANGE);
> > + cpus_have_const_cap(ARM64_HAS_TLB_RANGE) &&
> > + !cpus_have_const_cap(ARM64_WORKAROUND_NXP_ERR050104);
> > }
>
> It'd be better to handle this in the detection of ARM64_HAS_TLB_RANGE, as we
> have for CNP where has_useable_cnp() checks for ARM64_WORKAROUND_NVIDIA_CARMEL_CNP.
Thanks, I will rework this.
>
> >
> > int do_emulate_mrs(struct pt_regs *regs, u32 sys_reg, u32 rt);
> > diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
> > index 412a3b9a3c25..12055b859ce3 100644
> > --- a/arch/arm64/include/asm/tlbflush.h
> > +++ b/arch/arm64/include/asm/tlbflush.h
> > @@ -37,7 +37,11 @@
> > : : )
> >
> > #define __TLBI_1(op, arg) asm (ARM64_ASM_PREAMBLE \
> > - "tlbi " #op ", %0\n" \
> > + ALTERNATIVE("nop\n nop\n tlbi " #op ", %0", \
> > + "tlbi vmalle1is\n dsb ish\n isb", \
> > + ARM64_WORKAROUND_NXP_ERR050104) \
> > + : : "r" (arg)); \
>
> Why do you need the DSB ISH + ISB here? It's up to the caller to issue those,
> and the ARM64_WORKAROUND_REPEAT_TLBI workaround only has DSB ISH to ensure that
> the first op completes before the second is issued.
Right, perhaps I was gone too far here :-)
>
> > + asm (ARM64_ASM_PREAMBLE \
> > ALTERNATIVE("nop\n nop", \
> > "dsb ish\n tlbi " #op ", %0", \
> > ARM64_WORKAROUND_REPEAT_TLBI, \
> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> > index 307faa2b4395..7b702a79bf60 100644
> > --- a/arch/arm64/kernel/cpu_errata.c
> > +++ b/arch/arm64/kernel/cpu_errata.c
> > @@ -8,6 +8,7 @@
> > #include <linux/arm-smccc.h>
> > #include <linux/types.h>
> > #include <linux/cpu.h>
> > +#include <linux/of.h>
> > #include <asm/cpu.h>
> > #include <asm/cputype.h>
> > #include <asm/cpufeature.h>
> > @@ -55,6 +56,14 @@ is_kryo_midr(const struct arm64_cpu_capabilities *entry, int scope)
> > return model == entry->midr_range.model;
> > }
> >
> > +static bool __maybe_unused
> > +is_imx8qm_soc(const struct arm64_cpu_capabilities *entry, int scope)
> > +{
> > + WARN_ON(preemptible());
> > +
> > + return of_machine_is_compatible("fsl,imx8qm");
> > +}
>
> As above, what is going to be done for VMs, where this won't be present?
As I said, not sure this is usable for running VM's, but I am open for
suggestions. Maybe NXP has some register which could be used for SoC
identification, but judging by downstream implementation there is none.
>
> > +
> > static bool
> > has_mismatched_cache_type(const struct arm64_cpu_capabilities *entry,
> > int scope)
> > @@ -729,6 +738,15 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
> > MIDR_FIXED(MIDR_CPU_VAR_REV(1,1), BIT(25)),
> > .cpu_enable = cpu_clear_bf16_from_user_emulation,
> > },
> > +#endif
> > +#ifdef CONFIG_NXP_IMX8QM_ERRATUM_ERR050104
> > + {
> > + .desc = "NXP A53 cache coherency issue",
>
> Please use the erratum number, i.e.
>
> .desc = "NXP erratum ERR050104",
Ok.
>
> > + .capability = ARM64_WORKAROUND_NXP_ERR050104,
> > + .type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
> > + .matches = is_imx8qm_soc,
> > + .cpu_enable = cpu_enable_cache_maint_trap,
> > + },
> > #endif
> > {
> > }
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index 4a79ba100799..4858f8c86fd5 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -529,6 +529,26 @@ void do_el1_fpac(struct pt_regs *regs, unsigned long esr)
> > uaccess_ttbr0_disable(); \
> > }
> >
> > +#define __user_instruction_cache_maint(address, res) \
> > +do { \
> > + if (address >= TASK_SIZE_MAX) { \
> > + res = -EFAULT; \
> > + } else { \
> > + uaccess_ttbr0_enable(); \
> > + asm volatile ( \
> > + "1:\n" \
> > + ALTERNATIVE(" ic ivau, %1\n", \
> > + " ic ialluis\n", \
> > + ARM64_WORKAROUND_NXP_ERR050104) \
> > + " mov %w0, #0\n" \
> > + "2:\n" \
> > + _ASM_EXTABLE_UACCESS_ERR(1b, 2b, %w0) \
> > + : "=r" (res) \
> > + : "r" (address)); \
> > + uaccess_ttbr0_disable(); \
> > + } \
> > +} while (0)
> > +
> > static void user_cache_maint_handler(unsigned long esr, struct pt_regs *regs)
> > {
> > unsigned long tagged_address, address;
> > @@ -556,7 +576,7 @@ static void user_cache_maint_handler(unsigned long esr, struct pt_regs *regs)
> > __user_cache_maint("dc civac", address, ret);
> > break;
> > case ESR_ELx_SYS64_ISS_CRM_IC_IVAU: /* IC IVAU */
> > - __user_cache_maint("ic ivau", address, ret);
> > + __user_instruction_cache_maint(address, ret);
> > break;
>
> Hmm... this will silently change any 'IC IVAU' to never fault. That's probably
> not the end of the world, since it's IMP-DEF whether IC would raise a
> permission fault, but it is a change of behaviour.
>
> It would be a bit simpler to handle this inline within the switch, e.g.
>
> case ESR_ELx_SYS64_ISS_CRM_IC_IVAU: /* IC IVAU */
> if (cpus_have_final_cap(ARM64_WORKAROUND_NXP_ERR050104)) {
> asm volatile("ic ialluis");
> ret = 0;
> break;
> }
> __user_instruction_cache_maint(address, ret);
> break;
>
> ... as that would avoid duplicating the bulk of the __user_cache_maint() macro.
Ok.
>
>
> > default:
> > force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc, 0);
> > diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
> > index 37b1340e9646..e225f1cd1005 100644
> > --- a/arch/arm64/tools/cpucaps
> > +++ b/arch/arm64/tools/cpucaps
> > @@ -90,3 +90,4 @@ WORKAROUND_NVIDIA_CARMEL_CNP
> > WORKAROUND_QCOM_FALKOR_E1003
> > WORKAROUND_REPEAT_TLBI
> > WORKAROUND_SPECULATIVE_AT
> > +WORKAROUND_NXP_ERR050104
>
> These definitions are expected to be ordered alphabetically, so this should be
> earlier in the list.
>
Ok. Thanks!
Ivan
More information about the linux-arm-kernel
mailing list