| Message ID | 20250127201829.209258-1-zaidal@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: errata: Add Ampere erratum AC04_CPU_50 workaround alternative | expand |
On Mon, 27 Jan 2025 20:18:29 +0000, Zaid Alali <zaidal@os.amperecomputing.com> wrote: > > Add an alternative code sequence to work around Ampere erratum > AC03_CPU_50 on AmpereOne and Ampere1A. > > Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a > direct read of the register will return a value of 0xf8. An incorrect > value from a direct read can only happen with the value 0xf0. Under which precise conditions? Does it equally apply to virtual interrupts or SCR_EL3.FIQ==0, for which there is no non-secure shift (which I can only assume is the source of the erratum)? Does it equally affect G0 and G1 interrupts? > > Note: Currently there are no checks against a value of 0xf0, and that > save restore of 0xf8 -> 0xf0 is fine, so this is all future proofing. > > Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com> > --- > arch/arm64/Kconfig | 16 ++++++++++++++++ > arch/arm64/include/asm/arch_gicv3.h | 2 +- > arch/arm64/include/asm/daifflags.h | 4 ++-- > arch/arm64/include/asm/irqflags.h | 6 +++--- > arch/arm64/include/asm/sysreg.h | 9 +++++++++ > arch/arm64/kernel/cpu_errata.c | 15 +++++++++++++++ > arch/arm64/kernel/entry.S | 4 ++++ > arch/arm64/tools/cpucaps | 1 + Please add an entry to Documentation/arch/arm64/silicon-errata.txt. > 8 files changed, 51 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index fcdd0ed3eca8..8d6e263d66c7 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -461,6 +461,22 @@ config AMPERE_ERRATUM_AC03_CPU_38 > > If unsure, say Y. > > +config AMPERE_ERRATUM_AC03_CPU_50 > + bool "AmpereOne: AC03_CPU_50: Certain checks for ICC_PMR_EL1 that expects the value 0xf0 may read 0xf8 instead" > + default y > + help > + This option adds an alternative code sequence to work around Ampere > + erratum AC03_CPU_50 on AmpereOne and Ampere1A. > + > + Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a > + direct read of the register will return a value of 0xf8. An incorrect > + value from a direct read can only happen with the value 0xf0. > + > + The workaround for the erratum will do logical AND 0xf0 to the > + value read from ICC_PMR_EL1 register before returning the value. > + > + If unsure, say Y. > + An alternative for this would simply to prevent the enabling of pNMI on this platform. M.
On Tue, Jan 28, 2025 at 08:34:47AM +0000, Marc Zyngier wrote: > > +config AMPERE_ERRATUM_AC03_CPU_50 > > + bool "AmpereOne: AC03_CPU_50: Certain checks for ICC_PMR_EL1 that expects the value 0xf0 may read 0xf8 instead" > > + default y > > + help > > + This option adds an alternative code sequence to work around Ampere > > + erratum AC03_CPU_50 on AmpereOne and Ampere1A. > > + > > + Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a > > + direct read of the register will return a value of 0xf8. An incorrect > > + value from a direct read can only happen with the value 0xf0. > > + > > + The workaround for the erratum will do logical AND 0xf0 to the > > + value read from ICC_PMR_EL1 register before returning the value. > > + > > + If unsure, say Y. > > + > > An alternative for this would simply to prevent the enabling of pNMI > on this platform. There's also AC03_CPU_36, where the CPU goes into the weeds if you take an asynchronous exception while fiddling with HCR_EL2. We don't have a mitigation for it, and it can be pretty easily reproduced by using pNMIs while running VMs. So I agree, disabling pNMIs might be the easier way out. [*] https://amperecomputing.com/assets/AmpereOne_Developer_ER_v0_80_20240823_28945022f4.pdf
On Tue, Jan 28, 2025 at 08:34:47AM +0000, Marc Zyngier wrote: > On Mon, 27 Jan 2025 20:18:29 +0000, > Zaid Alali <zaidal@os.amperecomputing.com> wrote: > > > > Add an alternative code sequence to work around Ampere erratum > > AC03_CPU_50 on AmpereOne and Ampere1A. > > > > Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a > > direct read of the register will return a value of 0xf8. An incorrect > > value from a direct read can only happen with the value 0xf0. > > Under which precise conditions? Does it equally apply to virtual > interrupts or SCR_EL3.FIQ==0, for which there is no non-secure shift > (which I can only assume is the source of the erratum)? Does it > equally affect G0 and G1 interrupts? > Hi Marc, This only occurs when: When SCR_EL3.FIQ==1 and PE is NOT in EL3/Secure State, and ICC_PMR_EL1.Priority==LowestPriorityImplemented==0xf8 (highest priority is 0x00). Does it equally apply to virtual interrupts or SCR_EL3.FIQ==0? Based on this Defect (AArch-21735) and our implementation, it only affected ICC_PMR_EL1, therefore does not apply to virtual interrupts. It also does not apply when SCR_EL3.FIQ==0, as no modification of ICC_PMR_EL1 is required. Automatically sends the unshifted version. For which there is no non-secure shift, does it equally affect G0 and G1 interrupts? When SCR_EL3.FIQ==1 and PE is NOT in EL3/Secure State, and ICC_PMR_EL1.Priority==LowestPriorityImplemented==0xf8 (highest priority is 0x00): The ICC_PMR_EL1.Priority read returns an unshifted version (0xf8). It should have returned 0xf0. HW shifts the priority and inserts a 1b1 ensure the priority is a Grp1 priority. Should only be visible in HW. When SCR_EL3.FIQ==1 and PE is NOT in EL3/Secure State, and ICC_PMR_EL1.Priority==Grp0: This issue would not occur. Returned value would be the highest priority, 0x00. The returned value is correct as per ARM. When SCR_EL3.FIQ==1 and PE is NOT in EL3/Secure State, and ICC_PMR_EL1.Priority==Grp1 (but not the lowest priority): This issue would not occur. Returned value would be the shifted version. The returned value is correct as per ARM. > > > > Note: Currently there are no checks against a value of 0xf0, and that > > save restore of 0xf8 -> 0xf0 is fine, so this is all future proofing. > > > > Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com> > > --- > > arch/arm64/Kconfig | 16 ++++++++++++++++ > > arch/arm64/include/asm/arch_gicv3.h | 2 +- > > arch/arm64/include/asm/daifflags.h | 4 ++-- > > arch/arm64/include/asm/irqflags.h | 6 +++--- > > arch/arm64/include/asm/sysreg.h | 9 +++++++++ > > arch/arm64/kernel/cpu_errata.c | 15 +++++++++++++++ > > arch/arm64/kernel/entry.S | 4 ++++ > > arch/arm64/tools/cpucaps | 1 + > > Please add an entry to Documentation/arch/arm64/silicon-errata.txt. > > > 8 files changed, 51 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > index fcdd0ed3eca8..8d6e263d66c7 100644 > > --- a/arch/arm64/Kconfig > > +++ b/arch/arm64/Kconfig > > @@ -461,6 +461,22 @@ config AMPERE_ERRATUM_AC03_CPU_38 > > > > If unsure, say Y. > > > > +config AMPERE_ERRATUM_AC03_CPU_50 > > + bool "AmpereOne: AC03_CPU_50: Certain checks for ICC_PMR_EL1 that expects the value 0xf0 may read 0xf8 instead" > > + default y > > + help > > + This option adds an alternative code sequence to work around Ampere > > + erratum AC03_CPU_50 on AmpereOne and Ampere1A. > > + > > + Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a > > + direct read of the register will return a value of 0xf8. An incorrect > > + value from a direct read can only happen with the value 0xf0. > > + > > + The workaround for the erratum will do logical AND 0xf0 to the > > + value read from ICC_PMR_EL1 register before returning the value. > > + > > + If unsure, say Y. > > + > > An alternative for this would simply to prevent the enabling of pNMI > on this platform. > > M. > > -- > Without deviation from the norm, progress is not possible.
On Tue, Jan 28, 2025 at 12:54:26PM -0800, Oliver Upton wrote: > On Tue, Jan 28, 2025 at 08:34:47AM +0000, Marc Zyngier wrote: > > > +config AMPERE_ERRATUM_AC03_CPU_50 > > > + bool "AmpereOne: AC03_CPU_50: Certain checks for ICC_PMR_EL1 that expects the value 0xf0 may read 0xf8 instead" > > > + default y > > > + help > > > + This option adds an alternative code sequence to work around Ampere > > > + erratum AC03_CPU_50 on AmpereOne and Ampere1A. > > > + > > > + Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a > > > + direct read of the register will return a value of 0xf8. An incorrect > > > + value from a direct read can only happen with the value 0xf0. > > > + > > > + The workaround for the erratum will do logical AND 0xf0 to the > > > + value read from ICC_PMR_EL1 register before returning the value. > > > + > > > + If unsure, say Y. > > > + > > > > An alternative for this would simply to prevent the enabling of pNMI > > on this platform. > > There's also AC03_CPU_36, where the CPU goes into the weeds if you take > an asynchronous exception while fiddling with HCR_EL2. We don't have a > mitigation for it, and it can be pretty easily reproduced by using pNMIs > while running VMs. > > So I agree, disabling pNMIs might be the easier way out. > > [*] https://amperecomputing.com/assets/AmpereOne_Developer_ER_v0_80_20240823_28945022f4.pdf > We prefer to keep pNMIs enabled if possible since it is useful, and we will send another patch for AC03_CPU_36 to fix that. If this is not feasible we can send another patch to disable it in both AC03_CPU_36 and AC03_CPU_50. > -- > Thanks, > Oliver
On Tue, 04 Feb 2025 21:46:06 +0000, Zaid Alali <zaidal@os.amperecomputing.com> wrote: > > On Tue, Jan 28, 2025 at 08:34:47AM +0000, Marc Zyngier wrote: > > On Mon, 27 Jan 2025 20:18:29 +0000, > > Zaid Alali <zaidal@os.amperecomputing.com> wrote: > > > > > > Add an alternative code sequence to work around Ampere erratum > > > AC03_CPU_50 on AmpereOne and Ampere1A. > > > > > > Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a > > > direct read of the register will return a value of 0xf8. An incorrect > > > value from a direct read can only happen with the value 0xf0. > > > > Under which precise conditions? Does it equally apply to virtual > > interrupts or SCR_EL3.FIQ==0, for which there is no non-secure shift > > (which I can only assume is the source of the erratum)? Does it > > equally affect G0 and G1 interrupts? > > > > Hi Marc, > > This only occurs when: > When SCR_EL3.FIQ==1 and PE is NOT in EL3/Secure State, > and ICC_PMR_EL1.Priority==LowestPriorityImplemented==0xf8 (highest priority is 0x00). > > Does it equally apply to virtual interrupts or SCR_EL3.FIQ==0? > > Based on this Defect (AArch-21735) and our implementation, it only > affected ICC_PMR_EL1, therefore does not apply to virtual interrupts. Are you saying that this is erratum is *strictly* AARCH-21735? M.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index fcdd0ed3eca8..8d6e263d66c7 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -461,6 +461,22 @@ config AMPERE_ERRATUM_AC03_CPU_38 If unsure, say Y. +config AMPERE_ERRATUM_AC03_CPU_50 + bool "AmpereOne: AC03_CPU_50: Certain checks for ICC_PMR_EL1 that expects the value 0xf0 may read 0xf8 instead" + default y + help + This option adds an alternative code sequence to work around Ampere + erratum AC03_CPU_50 on AmpereOne and Ampere1A. + + Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a + direct read of the register will return a value of 0xf8. An incorrect + value from a direct read can only happen with the value 0xf0. + + The workaround for the erratum will do logical AND 0xf0 to the + value read from ICC_PMR_EL1 register before returning the value. + + If unsure, say Y. + config ARM64_WORKAROUND_CLEAN_CACHE bool diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h index 9e96f024b2f1..299d7e17abdf 100644 --- a/arch/arm64/include/asm/arch_gicv3.h +++ b/arch/arm64/include/asm/arch_gicv3.h @@ -127,7 +127,7 @@ static inline void gic_write_bpr1(u32 val) static inline u32 gic_read_pmr(void) { - return read_sysreg_s(SYS_ICC_PMR_EL1); + return read_sysreg_pmr(); } static __always_inline void gic_write_pmr(u32 val) diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h index fbb5c99eb2f9..2abea378ebd8 100644 --- a/arch/arm64/include/asm/daifflags.h +++ b/arch/arm64/include/asm/daifflags.h @@ -22,7 +22,7 @@ static inline void local_daif_mask(void) { WARN_ON(system_has_prio_mask_debugging() && - (read_sysreg_s(SYS_ICC_PMR_EL1) == (GIC_PRIO_IRQOFF | + (read_sysreg_pmr() == (GIC_PRIO_IRQOFF | GIC_PRIO_PSR_I_SET))); asm volatile( @@ -46,7 +46,7 @@ static inline unsigned long local_daif_save_flags(void) if (system_uses_irq_prio_masking()) { /* If IRQs are masked with PMR, reflect it in the flags */ - if (read_sysreg_s(SYS_ICC_PMR_EL1) != GIC_PRIO_IRQON) + if (read_sysreg_pmr() != GIC_PRIO_IRQON) flags |= PSR_I_BIT | PSR_F_BIT; } diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h index d4d7451c2c12..757e7e837992 100644 --- a/arch/arm64/include/asm/irqflags.h +++ b/arch/arm64/include/asm/irqflags.h @@ -30,7 +30,7 @@ static __always_inline void __daif_local_irq_enable(void) static __always_inline void __pmr_local_irq_enable(void) { if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING)) { - u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1); + u32 pmr = read_sysreg_pmr(); WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF); } @@ -59,7 +59,7 @@ static __always_inline void __daif_local_irq_disable(void) static __always_inline void __pmr_local_irq_disable(void) { if (IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING)) { - u32 pmr = read_sysreg_s(SYS_ICC_PMR_EL1); + u32 pmr = read_sysreg_pmr(); WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF); } @@ -84,7 +84,7 @@ static __always_inline unsigned long __daif_local_save_flags(void) static __always_inline unsigned long __pmr_local_save_flags(void) { - return read_sysreg_s(SYS_ICC_PMR_EL1); + return read_sysreg_pmr(); } /* diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index b8303a83c0bf..190409fff3b3 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -1226,6 +1226,15 @@ par; \ }) +#define read_sysreg_pmr() ({ \ + u64 pmr = read_sysreg_s(SYS_ICC_PMR_EL1); \ + asm(ALTERNATIVE("nop", "and %0, %0, #0xf0", \ + ARM64_WORKAROUND_AMPERE_AC03_CPU_50) \ + : "+r" (pmr) \ + ); \ + pmr; \ +}) + #define SYS_FIELD_VALUE(reg, field, val) reg##_##field##_##val #define SYS_FIELD_GET(reg, field, val) \ diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c index a78f247029ae..469f778228c8 100644 --- a/arch/arm64/kernel/cpu_errata.c +++ b/arch/arm64/kernel/cpu_errata.c @@ -467,6 +467,14 @@ static const struct midr_range erratum_ac03_cpu_38_list[] = { }; #endif +#ifdef CONFIG_AMPERE_ERRATUM_AC03_CPU_50 +static const struct midr_range erratum_ac03_cpu_50_list[] = { + MIDR_ALL_VERSIONS(MIDR_AMPERE1), + MIDR_ALL_VERSIONS(MIDR_AMPERE1A), + {}, +}; +#endif + const struct arm64_cpu_capabilities arm64_errata[] = { #ifdef CONFIG_ARM64_WORKAROUND_CLEAN_CACHE { @@ -785,6 +793,13 @@ const struct arm64_cpu_capabilities arm64_errata[] = { .capability = ARM64_WORKAROUND_AMPERE_AC03_CPU_38, ERRATA_MIDR_RANGE_LIST(erratum_ac03_cpu_38_list), }, +#endif +#ifdef CONFIG_AMPERE_ERRATUM_AC03_CPU_50 + { + .desc = "AmpereOne erratum AC03_CPU_50", + .capability = ARM64_WORKAROUND_AMPERE_AC03_CPU_50, + ERRATA_MIDR_RANGE_LIST(erratum_ac03_cpu_50_list), + }, #endif { } diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 5ae2a34b50bd..aed01144a351 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -318,6 +318,10 @@ alternative_else_nop_endif mrs_s x20, SYS_ICC_PMR_EL1 str w20, [sp, #S_PMR] +alternative_if ARM64_WORKAROUND_AMPERE_AC03_CPU_50 + and x20, x20, #0xf0 +alternative_else_nop_endif + str x20, [sp, #S_PMR_SAVE] mov x20, #GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET msr_s SYS_ICC_PMR_EL1, x20 diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps index eb17f59e543c..9e0776bb8c3b 100644 --- a/arch/arm64/tools/cpucaps +++ b/arch/arm64/tools/cpucaps @@ -93,6 +93,7 @@ WORKAROUND_2457168 WORKAROUND_2645198 WORKAROUND_2658417 WORKAROUND_AMPERE_AC03_CPU_38 +WORKAROUND_AMPERE_AC03_CPU_50 WORKAROUND_TRBE_OVERWRITE_FILL_MODE WORKAROUND_TSB_FLUSH_FAILURE WORKAROUND_TRBE_WRITE_OUT_OF_RANGE
Add an alternative code sequence to work around Ampere erratum AC03_CPU_50 on AmpereOne and Ampere1A. Due to AC03_CPU_50, when ICC_PMR_EL1 should have a value of 0xf0 a direct read of the register will return a value of 0xf8. An incorrect value from a direct read can only happen with the value 0xf0. Note: Currently there are no checks against a value of 0xf0, and that save restore of 0xf8 -> 0xf0 is fine, so this is all future proofing. Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com> --- arch/arm64/Kconfig | 16 ++++++++++++++++ arch/arm64/include/asm/arch_gicv3.h | 2 +- arch/arm64/include/asm/daifflags.h | 4 ++-- arch/arm64/include/asm/irqflags.h | 6 +++--- arch/arm64/include/asm/sysreg.h | 9 +++++++++ arch/arm64/kernel/cpu_errata.c | 15 +++++++++++++++ arch/arm64/kernel/entry.S | 4 ++++ arch/arm64/tools/cpucaps | 1 + 8 files changed, 51 insertions(+), 6 deletions(-)