Re: [PATCH v2] arm64: errata: Work around AmpereOne's erratum AC04_CPU_23

[D Scott Phillips <scott@os.amperecomputing.com>]

Marc Zyngier <maz@kernel.org> writes:

> On Fri, 25 Apr 2025 03:47:41 +0100,
> D Scott Phillips <scott@os.amperecomputing.com> wrote:
>> 
>> Updates to HCR_EL2 can rarely corrupt simultaneous translations for data
>> addresses initiated by load/store instructions. Only instruction
>> initiated translations are vulnerable, not translations from prefetches
>> for example. A DSB before the store to HCR_EL2 is sufficient to prevent older
>> instructions from hitting the window for corruption, and an ISB after
>> is sufficient to prevent younger instructions from hitting the window
>> for corruption.
>> 
>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>> ---
>> v1: https://lore.kernel.org/kvmarm/20250415154711.1698544-2-scott@os.amperecomputing.com/
>> Changes since v1:
>>  - Add a write_sysreg_hcr() helper (Oliver)
>>  - Add more specific wording in the errata description (Oliver & Marc)
>> 
>>  arch/arm64/Kconfig                      | 17 +++++++++++++++++
>>  arch/arm64/include/asm/hardirq.h        |  4 ++--
>>  arch/arm64/include/asm/sysreg.h         | 10 ++++++++++
>>  arch/arm64/kernel/cpu_errata.c          | 14 ++++++++++++++
>>  arch/arm64/kvm/at.c                     |  8 ++++----
>>  arch/arm64/kvm/hyp/include/hyp/switch.h |  2 +-
>>  arch/arm64/kvm/hyp/nvhe/mem_protect.c   |  2 +-
>>  arch/arm64/kvm/hyp/nvhe/switch.c        |  2 +-
>>  arch/arm64/kvm/hyp/vhe/switch.c         |  2 +-
>>  arch/arm64/kvm/hyp/vhe/tlb.c            |  4 ++--
>>  arch/arm64/tools/cpucaps                |  1 +
>>  11 files changed, 54 insertions(+), 12 deletions(-)
>> 
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index a182295e6f08b..3ae4e80e3002b 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -464,6 +464,23 @@ config AMPERE_ERRATUM_AC03_CPU_38
>>  
>>  	  If unsure, say Y.
>>  
>> +config AMPERE_ERRATUM_AC04_CPU_23
>> +        bool "AmpereOne: AC04_CPU_23:  Failure to synchronize writes to HCR_EL2 may corrupt address translations."
>> +	default y
>> +	help
>> +	  This option adds an alternative code sequence to work around Ampere
>> +	  errata AC04_CPU_23 on AmpereOne.
>> +
>> +	  Updates to HCR_EL2 can rarely corrupt simultaneous translations for
>> +	  data addresses initiated by load/store instructions. Only
>> +	  instruction initiated translations are vulnerable, not translations
>> +	  from prefetches for example. A DSB before the store to HCR_EL2 is
>> +	  sufficient to prevent older instructions from hitting the window
>> +	  for corruption, and an ISB after is sufficient to prevent younger
>> +	  instructions from hitting the window for corruption.
>> +
>> +	  If unsure, say Y.
>> +
>>  config ARM64_WORKAROUND_CLEAN_CACHE
>>  	bool
>>  
>> diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
>> index cbfa7b6f2e098..77d6b8c63d4e6 100644
>> --- a/arch/arm64/include/asm/hardirq.h
>> +++ b/arch/arm64/include/asm/hardirq.h
>> @@ -41,7 +41,7 @@ do {									\
>>  									\
>>  	___hcr = read_sysreg(hcr_el2);					\
>>  	if (!(___hcr & HCR_TGE)) {					\
>> -		write_sysreg(___hcr | HCR_TGE, hcr_el2);		\
>> +		write_sysreg_hcr(___hcr | HCR_TGE);			\
>>  		isb();							\
>>  	}								\
>>  	/*								\
>> @@ -82,7 +82,7 @@ do {									\
>>  	 */								\
>>  	barrier();							\
>>  	if (!___ctx->cnt && !(___hcr & HCR_TGE))			\
>> -		write_sysreg(___hcr, hcr_el2);				\
>> +		write_sysreg_hcr(___hcr);				\
>>  } while (0)
>>  
>>  static inline void ack_bad_irq(unsigned int irq)
>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> index 2639d3633073d..d41eeba7f8201 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -1185,6 +1185,16 @@
>>  		write_sysreg_s(__scs_new, sysreg);			\
>>  } while (0)
>>  
>> +#define write_sysreg_hcr(__val) do {					\
>> +	if(IS_ENABLED(CONFIG_AMPERE_ERRATUM_AC04_CPU_23) &&		\
>> +	   alternative_has_cap_unlikely(ARM64_WORKAROUND_AMPERE_AC04_CPU_23)) \
>> +		asm volatile("dsb nsh; msr hcr_el2, %x0; isb"		\
>> +			     : : "rZ" (__val));				\
>> +	else								\
>> +		asm volatile("msr hcr_el2, %x0"				\
>> +			     : : "rZ" (__val));				\
>> +} while (0)
>> +
>
> I'm worried that some of these accesses may occur before we compute
> the capabilities. I'd be more confident if the default was to have the
> workaround, and only to relax it once we know we're not on a broken
> system.

OK, will do

> But that leaves the question of the early stuff that runs before we
> get to C code. Are you sure this isn't affected by this issue (for
> example, the code in head.S, hyp-stub.S, and el2-setup.h)?

Ya, that's a good point. The corrupted translations can happen
speculatively, so there's every reason to think all those places also
need to be considered. I'll take a look everywhere now, not just at C.

>>  #define read_sysreg_par() ({						\
>>  	u64 par;							\
>>  	asm(ALTERNATIVE("nop", "dmb sy", ARM64_WORKAROUND_1508412));	\
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index b55f5f7057502..60f1b70fc0845 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -557,6 +557,13 @@ static const struct midr_range erratum_ac03_cpu_38_list[] = {
>>  };
>>  #endif
>>  
>> +#ifdef CONFIG_AMPERE_ERRATUM_AC04_CPU_23
>> +static const struct midr_range erratum_ac04_cpu_23_list[] = {
>> +	MIDR_ALL_VERSIONS(MIDR_AMPERE1A),
>> +	{},
>> +};
>> +#endif
>> +
>>  const struct arm64_cpu_capabilities arm64_errata[] = {
>>  #ifdef CONFIG_ARM64_WORKAROUND_CLEAN_CACHE
>>  	{
>> @@ -875,6 +882,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_AC04_CPU_23
>> +	{
>> +		.desc = "AmpereOne erratum AC04_CPU_23",
>> +		.capability = ARM64_WORKAROUND_AMPERE_AC04_CPU_23,
>> +		ERRATA_MIDR_RANGE_LIST(erratum_ac04_cpu_23_list),
>> +	},
>>  #endif
>
> This needs to be captured in Documentation/arch/arm64/silicon-errata.rst.

OK, will do, thanks Marc.