Re: [PATCH v3 1/2] riscv: build kernel atomics from LR/SC when Zaamo is disabled

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7231 bytes --]

On Mon, Oct 26, 2026 at 11:20:02AM +0200, Vladimir Kondratiev wrote:
> The A extension is Zaamo plus Zalrsc. MIPS P8700 implements only
> Zalrsc, and its AMO instructions trap to SBI. A typical atomic is
> about 40 cycles with LR/SC and about 600 cycles with an emulated AMO,
> so keep the existing AMO sequences when the toolchain defines
> __riscv_zaamo or __riscv_atomic, and use an LR/SC loop otherwise.
> 
> Add RISCV_ISA_ZAAMO and RISCV_ISA_ZALRSC. Both default on, and that
> selection still passes "a" in -march. Zaamo can be turned off only
> with CONFIG_NONPORTABLE; Zalrsc remains required because cmpxchg and
> the conditional atomics are LR/SC sequences. An LR/SC-only build
> passes _zalrsc instead of "a".
> 
> Suggested-by: Chao-ying Fu <cfu@wavecomp.com>
> Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

As you can see by the lkp reports, this is not the right way to do this.

> ---
>  arch/riscv/Kconfig               | 23 +++++++++++++++++
>  arch/riscv/Makefile              | 15 ++++++++---
>  arch/riscv/include/asm/atomic.h  | 56 +++++++++++++++++++++++++++++++++++++++-
>  arch/riscv/include/asm/bitops.h  | 51 ++++++++++++++++++++++++++++++++++++
>  arch/riscv/include/asm/cmpxchg.h | 18 +++++++++++++
>  arch/riscv/include/asm/futex.h   | 53 +++++++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/entry.S        |  9 +++++++
>  arch/riscv/kernel/head.S         | 14 ++++++++++
>  8 files changed, 234 insertions(+), 5 deletions(-)
> +
>  config TOOLCHAIN_HAS_ZBB
>  	bool
>  	default y
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 5874ca30f4f6..1403445482b9 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -730,6 +730,29 @@ config RISCV_ISA_ZACAS
>  
>  	  If you don't know what to do here, say Y.
>  
> +config RISCV_ISA_ZAAMO
> +	bool "Zaamo extension support for AMO atomics" if NONPORTABLE
> +	default y
> +	help
> +	  Use AMO instructions for kernel atomic memory operations. When
> +	  disabled, those operations are built from LR/SC and the kernel
> +	  is compiled with zalrsc instead of the A extension.
> +
> +	  Disable this only for a hart that implements Zalrsc but not
> +	  Zaamo, such as the MIPS P8700. The resulting kernel cannot run
> +	  on a hart without LR/SC.

The text here doesn't really make much sense, given it is default
disabled as it depends on NONPORTABLE that is default disabled.
If this is retained, it should be default n and the text reworded.

> +
> +	  If you don't know what to do here, say Y.
> +
> +config RISCV_ISA_ZALRSC
> +	def_bool y
> +	help
> +	  Zalrsc provides LR/SC. cmpxchg and the conditional atomics
> +	  require it, so it is always enabled.

So why have a kconfig option for it then? It's pointless if always
enabled.

> +
> +config RISCV_ISA_A
> +	def_bool RISCV_ISA_ZAAMO && RISCV_ISA_ZALRSC

I think this all needs a redo...

> diff --git a/arch/riscv/include/asm/atomic.h b/arch/riscv/include/asm/atomic.h
> index 616b8b332ac5..dc6a8c95c975 100644
> --- a/arch/riscv/include/asm/atomic.h
> +++ b/arch/riscv/include/asm/atomic.h
> @@ -50,6 +50,7 @@ static __always_inline void arch_atomic64_set(atomic64_t *v, s64 i)
>   * have the AQ or RL bits set.  These don't return anything, so there's only
>   * one version to worry about.
>   */
> +#if defined(__riscv_atomic) || defined(__riscv_zaamo)
>  #define ATOMIC_OP(op, asm_op, I, asm_type, c_type, prefix)		\
>  static __always_inline							\
>  void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
> @@ -59,7 +60,25 @@ void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
>  		: "+A" (v->counter)					\
>  		: "r" (I)						\
>  		: "memory");						\
> -}									\
> +}
> +#elif defined(__riscv_zalrsc)

...why are we making this a compiletime decision in the first place?
Why can't this be an alternative, starting with the zalrsc stuff and
moving to the better implementation if available?
Is there some zaamo code that gets generated automatically that we lose
out on by always building the kernel with zalrsc instead of with all of a?

> +#define ATOMIC_OP(op, asm_op, I, asm_type, c_type, prefix)		\
> +static __always_inline							\
> +void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
> +{									\
> +	register c_type ret, temp;					\
> +	__asm__ __volatile__ (						\
> +		"1:	lr." #asm_type " %1, %0\n"			\
> +		"	" #asm_op " %2, %1, %3\n"			\
> +		"	sc." #asm_type " %2, %2, %0\n"			\
> +		"	bnez %2, 1b\n"					\
> +		: "+A" (v->counter), "=&r" (ret), "=&r" (temp)		\
> +		: "r" (I)						\
> +		: "memory");						\
> +}
> +#else
> +#error "Need AMO or LR/SC atomics"
> +#endif
>  
>  #ifdef CONFIG_GENERIC_ATOMIC64
>  #define ATOMIC_OPS(op, asm_op, I)					\
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 44f5a439741a..23ecf68a761d 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -58,8 +58,9 @@ ifeq ($(CONFIG_SHADOW_CALL_STACK),y)
>  endif
>  
>  # ISA string setting
> -riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32ima
> -riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64ima
> +riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32im
> +riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64im
> +riscv-march-$(CONFIG_RISCV_ISA_A)	:= $(riscv-march-y)a
>  riscv-march-$(CONFIG_FPU)		:= $(riscv-march-y)fd
>  riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
>  riscv-march-$(CONFIG_RISCV_ISA_V)	:= $(riscv-march-y)v
> @@ -75,9 +76,15 @@ else
>  riscv-march-$(CONFIG_TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI) := $(riscv-march-y)_zicsr_zifencei
>  endif
>  
> -KBUILD_BASE_ISA = -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)fd([^v_]*)v?/\1\2/')
> +KBUILD_BASE_ISA = -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima?|rv64ima?)fd([^v_]*)v?/\1\2/')
>  export KBUILD_BASE_ISA
>  
> +# if "A" not supported, toolchain shall support zaamo/zalrsc

Obviously this is not true, disabling some Kconfig option has no bearing
on what a toolchain is capable of. You cannot put _foo in here without
checking if the toolchain actually supports it.
If no zaamo instructions are automatically generated, this dance isn't
needed, so my suggestions for how to proceed depend on the answer to
that question.

pw-bot: changes-requested

Thanks,
Conor.

> +ifneq ($(CONFIG_RISCV_ISA_A),y)
> +riscv-march-$(CONFIG_RISCV_ISA_ZAAMO) := $(riscv-march-y)_zaamo
> +riscv-march-$(CONFIG_RISCV_ISA_ZALRSC) := $(riscv-march-y)_zalrsc
> +endif
> +
>  # Remove F,D,V from isa string for all. Keep extensions between "fd" and "v" by
>  # matching non-v and non-multi-letter extensions out with the filter ([^v_]*)
>  KBUILD_CFLAGS += $(KBUILD_BASE_ISA)
> @@ -85,7 +92,7 @@ KBUILD_CFLAGS += $(KBUILD_BASE_ISA)
>  KBUILD_AFLAGS += -march=$(riscv-march-y)
>  
>  # For C code built with floating-point support, exclude V but keep F and D.
> -CC_FLAGS_FPU  := -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)([^v_]*)v?/\1\2/')
> +CC_FLAGS_FPU  := -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima?|rv64ima?)([^v_]*)v?/\1\2/')
>  
>  KBUILD_CFLAGS += -mno-save-restore
>  


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]