Re: [PATCH v6 5/5] riscv: errata: picoheart: Add workaround for AMOCASQ errata

[Jisheng Zhang <jszhang@kernel.org>]

On Tue, Nov 10, 2026 at 05:50:46PM +0800, Yicong Yang wrote:
> Hi Jisheng,
> 
> On 11/8/26 9:33 AM, Jisheng Zhang wrote:
> > On Sat, Nov 07, 2026 at 11:10:59PM +0800, Yicong Yang wrote:
> >> On 11/7/26 9:34 PM, Jisheng Zhang wrote:
> >>> On Wed, Nov 04, 2026 at 03:57:06PM +0800, Yicong Yang wrote:
> >>>> Some Picoheart CPUs implement Zacas extension but lack support
> >>>> for AMOCASQ PMA attribute. Thus makes the CASQ instruction de
> >>>> facto unavailable. Since currently no ways to retrieve the PMA
> >>>> information and the riscv kernel will declare cmpxchg128 support
> >>>> if platform declare support of Zacas, the use of cmpxchg128 will
> >>>> lead to PMA violation and crash the kernel. Add the errata
> >>>> workaround to disable the use of cmpxchg128 on the affected CPUs.
> >>>>
> >>>> Tested-by: Han Gao <gaohan@iscas.ac.cn>
> >>>> Signed-off-by: Yicong Yang <yang.yicong@picoheart.com>
> >>>> ---
> >>>>  arch/riscv/Kconfig.errata                    | 12 ++++++++++
> >>>>  arch/riscv/errata/picoheart/errata.c         | 25 +++++++++++++++++++-
> >>>>  arch/riscv/include/asm/cmpxchg.h             | 18 +++++++++++++-
> >>>>  arch/riscv/include/asm/errata_list_vendors.h |  3 ++-
> >>>>  4 files changed, 55 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata
> >>>> index c75dec4b0ba7..4e6516ba3a42 100644
> >>>> --- a/arch/riscv/Kconfig.errata
> >>>> +++ b/arch/riscv/Kconfig.errata
> >>>> @@ -202,4 +202,16 @@ config ERRATA_PICOHEART_CBO_CLEAN
> >>>>  
> >>>>  	   If you don't know what to do here, say "Y".
> >>>>  
> >>>> +config ERRATA_PICOHEART_AMOCASQ
> >>>> +	bool "Apply Picoheart AMOCASQ errata"
> >>>> +	depends on ERRATA_PICOHEART && RISCV_ISA_ZACAS
> >>>> +	default y
> >>>> +	help
> >>>> +	  Some Picoheart CPUs implement the Zacas extension but lack support
> >>>> +	  for AMOCASQ PMA attribute. Thus makes the CASQ instruction de facto
> >>>> +	  unavailable. Enable this errata workaround to disable the use of
> >>>> +	  CASQ in the kernel (cmpxchg128).
> >>>> +
> >>>> +	  If you don't know what to do here, say "Y".
> >>>> +
> >>>>  endmenu # "CPU errata selection"
> >>>> diff --git a/arch/riscv/errata/picoheart/errata.c b/arch/riscv/errata/picoheart/errata.c
> >>>> index c945ae694396..a658f38cf4de 100644
> >>>> --- a/arch/riscv/errata/picoheart/errata.c
> >>>> +++ b/arch/riscv/errata/picoheart/errata.c
> >>>> @@ -174,11 +174,34 @@ static void picoheart_errata_probe_cbo_clean(unsigned int stage,
> >>>>  	static_branch_enable(&has_picoheart_cbo_clean_errata);
> >>>>  }
> >>>>  
> >>>> +static bool picoheart_errata_probe_amocasq(unsigned int stage,
> >>>> +					   unsigned long archid,
> >>>> +					   unsigned long impid)
> >>>> +{
> >>>> +	if (!IS_ENABLED(CONFIG_ERRATA_PICOHEART_AMOCASQ))
> >>>> +		return false;
> >>>> +
> >>>> +	if (!IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) ||
> >>>> +	    !riscv_isa_extension_available(NULL, ZACAS))
> >>>> +		return false;
> >>>> +
> >>>> +	if (archid != 0x804a555049544552 || impid != 0x100)
> >>>> +		return false;
> >>>> +
> >>>> +	return true;
> >>>> +}
> >>>> +
> >>>>  static u32 picoheart_errata_probe(unsigned int stage, unsigned long archid,
> >>>>  				  unsigned long impid)
> >>>>  {
> >>>> +	u32 cpu_req_errata = 0;
> >>>> +
> >>>>  	picoheart_errata_probe_cbo_clean(stage, archid, impid);
> >>>> -	return 0;
> >>>> +
> >>>> +	if (picoheart_errata_probe_amocasq(stage, archid, impid))
> >>>> +		cpu_req_errata |= BIT(ERRATA_PICOHEART_AMOCASQ);
> >>>> +
> >>>> +	return cpu_req_errata;
> >>>>  }
> >>>>  
> >>>>  void picoheart_errata_patch_func(struct alt_entry *begin, struct alt_entry *end,
> >>>> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> >>>> index 662e160b0522..0488b9314c13 100644
> >>>> --- a/arch/riscv/include/asm/cmpxchg.h
> >>>> +++ b/arch/riscv/include/asm/cmpxchg.h
> >>>> @@ -329,7 +329,23 @@
> >>>>  
> >>>>  #if defined(CONFIG_64BIT) && defined(CONFIG_RISCV_ISA_ZACAS) && defined(CONFIG_TOOLCHAIN_HAS_ZACAS)
> >>>>  
> >>>> -#define system_has_cmpxchg128()        riscv_has_extension_unlikely(RISCV_ISA_EXT_ZACAS)
> >>>> +#define system_has_cmpxchg128 system_has_cmpxchg128
> >>>> +static __always_inline bool system_has_cmpxchg128(void)
> >>>> +{
> >>>> +	/* The vendor erratum overrides the standard Zacas alternative. */
> >>>> +	asm goto(ALTERNATIVE_2(
> >>>> +			"nop",
> >>>> +			"j %l[l_yes]",
> >>>> +			STANDARD_EXT, RISCV_ISA_EXT_ZACAS, 1,
> >>>> +			"nop",
> >>>> +			PICOHEART_VENDOR_ID, ERRATA_PICOHEART_AMOCASQ,
> >>>> +			CONFIG_ERRATA_PICOHEART_AMOCASQ)
> >>>> +		: : : : l_yes);
> >>>
> >>> This makes the generic code ugly, adds some maintain effort.
> >>> why not discarding "zacas" in the dts? It's much simpler
> >>
> >> it'll prevent the usage of other more widely used CAS instructions
> >> in both user and kernel space. we'd like to retain the performance
> >> benefit of CAS over LR/SC for primitives such as cmpxchg() on our
> >> server as well.
> > 
> > If the Zacas can't be safely exposed on this platform, the ISA extension
> > should be disabled, rather than adding vendor specific exceptions to
> > generic cap detecion which adds unnecessary complexity and maintenance
> > burden to generic code. It also sets an undesirable precedent for
> > handling similar errata in the future. Imagine if various vendors don't
> > implement various extensions correctly, then the riscv arch code will
> > be full of this kind of specific exceptions here and there, which make
> > thes fragmentation in RISC-V land situation worse.
> > 
> 
> I totally understand the concerns. The silicon errata is unexpected but
> the riscv's errata framework and alternatives provides the means to save
> the case. I'd like to keep the erratum handling as small as possible and
> open to better solutions..
> 
> Disabling the entire extension because of the AMOCAS.Q erratum would also

I re-read the patch's commit msg, "AMOCASQ PMA attribute", it seems the PMA
attribute doesn't support 128bit, IMO, this seems not an errata but a design
choice -- decide to support subset of Zacas during the design stage. This is
just my guess though.

This reminds me a recent patch in the mailist which is to workaround a design
choice: only implement subset of 'A". IMHO this kind of workaround
should be kept out of tree. arch/riscv code fragmentation becomes non-trivial
due to various extension combinations becomes, if vendor choose to implement
subset of each extension, then it significantly increases combinations
we need to support, to test, to maintain.

> lose the benefits of the working AMOCAS.W/D instructions. Preserving their
> performance benefit in the kernel is my main motivation. Our measurements  
> show substantial gains for some cmpxchg() workloads, particularly under
> contention on our server platform. The following results compare kernel
> cmpxchg() using CAS and LR/SC:
> 
> CAS will reduce the latency by about 20~21%:
> Width       LR/SC       CAS    Improvement
> 32-bit     1.000×    1.253×         +25.3%
> 64-bit     1.000×    1.270×         +27.0%
> 
> and increase the throughput by at most 6x at high contention.
> Width Contenders       LR/SC       CAS    Improvement
> 32-bit         2      1.000×    0.982×          -1.8%
> 32-bit         4      1.000×    2.115×        +111.5%
> 32-bit         8      1.000×    4.102×        +310.2%
> 32-bit        16      1.000×    6.981×        +598.1%
> 32-bit        30      1.000×    4.808×        +380.8%
> 64-bit         2      1.000×    1.088×          +8.8%
> 64-bit         4      1.000×    1.554×         +55.4%
> 64-bit         8      1.000×    2.907×        +190.7%
> 64-bit        16      1.000×    4.970×        +397.0%
> 64-bit        30      1.000×    5.262×        +426.2%
> 
> >>
> >> since kernel already provides system_has_cmpxchg128() as a
> >> separate capability check, disabling 128-bit CAS here lets us
> >> limit the workaround to cmpxhg128 only and could be a reasonable
> > 
> > The workaround only affects kernel's cap check, but doesn't prevent
> > userspace from executing the affected instruction if userspace uses
> > hwprobe etc. to dynamically detect the underlying hw supported
> > extensions. Therefore, this workaround does not fully address the
> > errata and could leave userspace exposed to the same issue.
> 
> indeed. we initially decided to retain the advertisement of Zacas to
> userspace as AMOCAS.Q is not frequently used and the access fault by
> AMOCAS.Q is less fatal compared to the kernel. the access fault in
> kernel will crash the system.
> 
> > 
> > Disabling the affected extension at the platform level is a more
> > consistent approach.
> 
> what about retaining AMOCAS.W/D for kernel use, while keeping
> cmpxchg128() disabled and withholding Zacas from userspace capability
> advertisement? That would preserve the measured kernel performance
> benefit without advertising full Zacas support to applications.
> 

AIUI, this can't be acchived without introducing subset concept of Zacas
to the arch/riscv code

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv