[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <86a57w9x7i.fsf@scott-ph-mail.amperecomputing.com>
Date: Thu, 01 May 2025 08:17:53 -0700
From: D Scott Phillips <scott@...amperecomputing.com>
To: Marc Zyngier <maz@...nel.org>
Cc: Catalin Marinas <catalin.marinas@....com>, James Clark
<james.clark@...aro.org>, James Morse <james.morse@....com>, Joey Gouly
<joey.gouly@....com>, Kevin Brodsky <kevin.brodsky@....com>, Mark Brown
<broonie@...nel.org>, Mark Rutland <mark.rutland@....com>, Oliver Upton
<oliver.upton@...ux.dev>, "Rob Herring (Arm)" <robh@...nel.org>, Shameer
Kolothum <shameerali.kolothum.thodi@...wei.com>, Shiqi Liu
<shiqiliu@...t.edu.cn>, Will Deacon <will@...nel.org>, Yicong Yang
<yangyicong@...ilicon.com>, kvmarm@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64: errata: Work around AmpereOne's erratum
AC04_CPU_23
Marc Zyngier <maz@...nel.org> writes:
> On Fri, 25 Apr 2025 03:47:41 +0100,
> D Scott Phillips <scott@...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@...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.
Powered by blists - more mailing lists