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

[Conor Dooley]

Hey,

Sorry about not noticing the v4, I had the v3 on my todo list for a few
days and didn't check for an update.

On Sun, Nov 01, 2026 at 02:51:39PM +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. Zalrsc remains required because
> cmpxchg and the conditional atomics are LR/SC sequences. Zaamo can be
> turned off only with CONFIG_NONPORTABLE, and only when the compiler
> accepts _zalrsc in -march. GCC 15.1 introduces that support. Older compilers keep Zaamo
> enabled and -march keeps "a". An LR/SC-only build passes _zalrsc
> instead of "a".
> 
> Suggested-by: Chao-ying Fu <cfu@xxxxxxxxxxxx>
> Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@xxxxxxxxxxxx>
> ---
>  arch/riscv/Kconfig               | 32 +++++++++++++++++++++++
>  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, 243 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 5874ca30f4f6..925963a61f62 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -730,6 +730,38 @@ config RISCV_ISA_ZACAS
>  
>  	  If you don't know what to do here, say Y.
>  
> +config TOOLCHAIN_HAS_ZALRSC
> +	bool
> +	default y
> +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64im_zalrsc)
> +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32im_zalrsc)
> +
> +config RISCV_ISA_ZAAMO
> +	bool "Zaamo extension support for AMO atomics" if NONPORTABLE && TOOLCHAIN_HAS_ZALRSC

Probably this option should be called something like
TOOLCHAIN_HAS_ZALRSC_ZAAMO if you're going to use it like this, because
it is confusing why ZALRSC gates a ZAAMO option.

I'm curious actually, does every toolchain provide zalrsc and zaamo as a
pair, or should you check for both here? I'd be surprised if there was a
toolchain with one but not the other but worth checking to avoid some
bizarre toolchain bug..

| > 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.
| 
| Not exactly. RISCV_ISA_ZAAMO is visible if NONPORTABLE &&
| TOOLCHAIN_HAS_ZALRSC (later was added in v4). Otherwise it is not
| visible and set to "y". If one disables NONPORTABLE, RISCV_ISA_ZAAMO set
| to "y" with no option to change it. Same for added in v4
| TOOLCHAIN_HAS_ZALRSC

Ah, right. But that kinda goes against how NONPORTABLE is meant to work
since the default cannot run on all platforms? At that point, just get
rid of 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, and only when the compiler accepts
> +	  _zalrsc in -march. GCC 15.1 introduces that support. Without it this
> +	  option stays enabled and -march keeps the A extension.
> +
> +	  The resulting kernel cannot run on a hart without LR/SC.
> +
> +	  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.
| 
| My logic when adding it:
| There are 2 sets of extensions related defines: one is Kconfig ones,
| CONFIG_RISCV_ISA_<ext>; and another one found in the
| arch/riscv/include/asm/hwcap.h, RISCV_ISA_EXT_<ext>
| 
| In the hwcap.h, there's already
| 
| #define RISCV_ISA_EXT_ZAAMO		97
| #define RISCV_ISA_EXT_ZALRSC		98
| 
| added by commit 35173b666a16 (riscv: add parsing for Zaamo and Zalrsc
| extensions, 2024-06-19)
| 
| These 2 sets are logically coupled, and used in the BPF JUT code like
| 
| #define rv_ext_enabled(ext) \
| 	(IS_ENABLED(CONFIG_RISCV_ISA_##ext) &&
| riscv_has_extension_likely(RISCV_ISA_EXT_##ext))
| 
| So I thought if RISCV_ISA_EXT_ZALRSC exists although it is mandatory,
| CONFIG_RISCV_ISA_ZALRSC counterpart should exist for the same reason.
| 
| Do you suggest this is different case and it is worth to remove
| CONFIG_RISCV_ISA_ZALRSC anyway?

If this is the justification for it, then you need to provide this
justification in your commit message - but if zalrsc is always enabled
then there's no need for any code like that in the bpf jit in the first
place, once more making this dead code that should be deleted?

> +
> +config RISCV_ISA_A
> +	def_bool RISCV_ISA_ZAAMO && RISCV_ISA_ZALRSC
> +
>  config TOOLCHAIN_HAS_ZBB
>  	bool
>  	default y
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 44f5a439741a..1b0a459a0f74 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
>  
> +# Zaamo is off only when the compiler accepts _zalrsc in -march.

I think the comment here is not quite right, it should be "can only be
off", no?

> +ifneq ($(CONFIG_RISCV_ISA_A),y)
> +riscv-march-$(CONFIG_RISCV_ISA_ZAAMO) := $(riscv-march-y)_zaamo

Isn't this dead code? The logic for RISCV_ISA_A is:
config RISCV_ISA_A
	def_bool RISCV_ISA_ZAAMO && RISCV_ISA_ZALRSC
but ZALRSC is always set, so RISCV_ISA_A is equivalent to
RISCV_ISA_ZAAMO so there can't be a case where RISCV_ISA_A is not set
but RISCV_ISA_ZAAMO is?

> +riscv-march-$(CONFIG_RISCV_ISA_ZALRSC) := $(riscv-march-y)_zalrsc

And here, this is effectively just riscv-march-y, since if you get as
far as here, it must be true.

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

| > ...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?
| 
| I started with alternatives approach, see
| https://lore.kernel.org/all/20260114072552.731974-1-vladimir.kondratiev@xxxxxxxxxxxx/
| Then I converted it to the static compile-time decision following input
| from Gary Guo that I think is valid, here:
| https://lore.kernel.org/all/DFSQIY6WV4U7.3B3RY31KE739B@xxxxxxxxxxx/


Okay, that logic from Gary does make some sense. I'll accept that
argument, but again this needs to be communicated in the commit message.

| > 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?
| 
| There's BPF JIT that emits AMO instructions, patch for this is not ready
| yet. These AMO instructions will be emulated and work correctly but very
| slowly.

I think that needs to be part of this series. You're far too focused on
your individual platform where the instructions work, rather than the
holistic solution of lowering our minimum requirements so that platforms
that do not have any AMO support work. The only way that it'd make sense
to move forward with this without the bpf jit patch is if you disabled
bpf entirely when RISCV_ISA_ZAAMO is disabled.

> +#if defined(__riscv_atomic) || defined(__riscv_zaamo)

Something else to note is that we do not use this pattern anywhere else
in arch code as far as I know? We always use the RISC_ISA_WHATEVER
config option to gate conditional stuff. Since RISCV_ISA_A is equivalent
to RISCV_ISA_ZAAMO (unless I am mistaken there somehow) this just collapses
to a single check?

>  #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)
> +#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"

And this error is actually unreachable?

(and so on in every other place that this pattern is repeated)

Cheers,
Conor.

> +#endif

Attachment: signature.asc
Description: PGP signature