[PATCH] arm64: errata: Add Ampere erratum AC04_CPU_50 workaround alternative

[PATCH] arm64: errata: Add Ampere erratum AC04_CPU_50 workaround alternative

[Zaid Alali]

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(-)

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
-- 
2.34.1

Re: [PATCH] arm64: errata: Add Ampere erratum AC04_CPU_50 workaround alternative

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] arm64: errata: Add Ampere erratum AC04_CPU_50 workaround alternative

[Oliver Upton]

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

-- 
Thanks,
Oliver

Re: [PATCH] arm64: errata: Add Ampere erratum AC04_CPU_50 workaround alternative

[Zaid Alali]

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.

Re: [PATCH] arm64: errata: Add Ampere erratum AC04_CPU_50 workaround alternative

[Zaid Alali]

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

Re: [PATCH] arm64: errata: Add Ampere erratum AC04_CPU_50 workaround alternative

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.