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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] riscv: missing RISCV_FULL_BARRIER in LR/SC value-returning atomics
- [High] riscv: bitops: broken RVWMO ordering and leaked macros in LR/SC bitops
- [Medium] riscv: missing Zaamo dependency for Zicfiss when RISCV_ISA_ZAAMO=n
- [Low] riscv: unreachable _zaamo march rule and unused CONFIG_RISCV_ISA_ZALRSC
- [Low] riscv: missing workload benchmarks and userspace slow-AMO reporting for Zaamo
--

commit 0594932503c2ca9d6aebe33d3c6b28a68cd3db5e
Author: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

riscv: build kernel atomics from LR/SC when Zaamo is disabled

This commit adds CONFIG_RISCV_ISA_ZAAMO (and CONFIG_RISCV_ISA_ZALRSC /
CONFIG_RISCV_ISA_A) so kernels can be built using LR/SC loops instead of AMO
instructions on CPUs where Zaamo is trap-emulated in M-mode.

> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 5874ca30f4f6d..07d15a0bb3799 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -730,6 +730,59 @@ config RISCV_ISA_ZACAS
>  
>  	  If you don't know what to do here, say Y.
>  
> +config TOOLCHAIN_HAS_ZALRSC_ZAAMO
> +	bool
> +	default y
> +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64im_zaamo_zalrsc)
> +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32im_zaamo_zalrsc)
> +
> +config RISCV_ISA_ZAAMO
> +	bool "Zaamo extension support for AMO atomics" if TOOLCHAIN_HAS_ZALRSC_ZAAMO
> +	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 for a CPU where AMO is much slower than LR/SC. That
> +	  is the case when AMO instructions are emulated by a trap into
> +	  M-mode, as on the MIPS P8700: an emulated atomic costs hundreds
> +	  of cycles where LR/SC costs tens. Such a CPU still advertises
> +	  the A extension, and userspace still sees it.

[Severity: Low]
As reported by Palmer Dabbelt on v4 [1], when treating M-mode trap-emulated
AMOs on hardware like MIPS P8700 as implemented-but-slow and continuing to
advertise the A extension to userspace in CONFIG_RISCV_ISA_ZAAMO, could
real-world application performance numbers be provided (rather than only the
40-cycle vs 600-cycle microbenchmark), along with a mechanism such as hwprobe
to report slow AMO behavior to userspace?

> +
> +	  A kernel built without Zaamo runs on any CPU that implements the
> +	  A extension; it only does not use AMO.
> +
> +	  The compiler must accept _zaamo and _zalrsc in -march to make
> +	  this choice. GCC 15.1 is the first release that does. With an
> +	  older compiler the option stays enabled and -march keeps the A
> +	  extension.
> +
> +	  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 are
> +	  LR/SC sequences, so this is always enabled.
> +
> +	  It exists because asm/hwcap.h already has RISCV_ISA_EXT_ZALRSC
> +	  next to RISCV_ISA_EXT_ZAAMO, and the kernel pairs each
> +	  RISCV_ISA_EXT_<ext> it can be built for with a
> +	  CONFIG_RISCV_ISA_<ext>; rv_ext_enabled() in the BPF JIT, for
> +	  example, tests both.
> +
> +	  Zaamo plus Zacas could provide every atomic as well, with CAS
> +	  loops in place of LR/SC. This option is where that would go,
> +	  by allowing it to be disabled.
> +
> +config RISCV_ISA_A
> +	def_bool RISCV_ISA_ZAAMO && RISCV_ISA_ZALRSC
> +	help
> +	  The A extension is Zaamo plus Zalrsc. This is the Kconfig
> +	  counterpart of RISCV_ISA_EXT_A, for the same reason as
> +	  RISCV_ISA_ZALRSC above, and puts "a" in -march.
> +
>  config TOOLCHAIN_HAS_ZBB
>  	bool
>  	default y
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 44f5a439741a9..5cdc9cc017bb1 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)	:= rv32im
> -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,16 @@ 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
>  
> +# Without "a", name the sub-extensions. Zaamo can be off only when the
> +# compiler accepts _zaamo and _zalrsc in -march.
> +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

[Severity: Medium]
As reported by Sashiko on v4 [2], does disabling CONFIG_RISCV_ISA_ZAAMO break
the build when CONFIG_RISCV_USER_CFI=y (which defaults to y on 64-bit MMU
kernels)?

When CONFIG_RISCV_ISA_ZAAMO=n, riscv-march-y and KBUILD_BASE_ISA are built with
_zalrsc and omit both a and _zaamo. However, CONFIG_RISCV_USER_CFI in
arch/riscv/Kconfig checks:

	depends on 64BIT && MMU && \
		$(cc-option,-mabi=lp64 -march=rv64ima_zicfiss_zicfilp -fcf-protection=full)

without depending on RISCV_ISA_ZAAMO.

Because the Zicfiss extension requires Zaamo for ssamoswap, two build failures
follow:

1. In arch/riscv/kernel/usercfi.c, amo_user_shstk() emits:

	asm goto(".option push\n"
		".option arch, +zicfiss\n"
		"1: ssamoswap.d %[swap], %[val], %[addr]\n"

In both GNU binutils and LLVM, .option arch, +zicfiss does not enable +zaamo,
so assembling ssamoswap.d fails when KBUILD_CFLAGS lacks a and _zaamo.

2. In arch/riscv/kernel/vdso/Makefile (included by
arch/riscv/kernel/vdso_cfi/Makefile with VDSO_CFI_BUILD := 1), CFI_MARCH
appends _zicfilp_zicfiss to $(KBUILD_BASE_ISA), which fails when the base
-march has _zalrsc without _zaamo.

Should RISCV_USER_CFI depend on RISCV_ISA_ZAAMO, or should +zaamo be enabled
where Zicfiss is used?

[Severity: Low]
As reported by Conor Dooley on v3 [3], since CONFIG_RISCV_ISA_ZALRSC is
unconditionally def_bool y and CONFIG_RISCV_ISA_A is defined as
RISCV_ISA_ZAAMO && RISCV_ISA_ZALRSC, CONFIG_RISCV_ISA_A is always identical to
CONFIG_RISCV_ISA_ZAAMO.

Doesn't that make the _zaamo assignment inside ifneq ($(CONFIG_RISCV_ISA_A),y)
unreachable dead code, and could the unused CONFIG_RISCV_ISA_ZALRSC and
CONFIG_RISCV_ISA_A options be dropped in favor of using CONFIG_RISCV_ISA_ZAAMO
directly?

[ ... ]

> diff --git a/arch/riscv/include/asm/atomic.h b/arch/riscv/include/asm/atomic.h
> index 616b8b332ac56..3b189bea1d08c 100644
> --- a/arch/riscv/include/asm/atomic.h
> +++ b/arch/riscv/include/asm/atomic.h

[ ... ]

> @@ -108,6 +126,38 @@ c_type arch_atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v)	\
>  		: "memory");						\
>  	return ret;							\
>  }
> +#else
> +#define ATOMIC_FETCH_OP(op, asm_op, I, asm_type, c_type, prefix)	\
> +static __always_inline							\
> +c_type arch_atomic##prefix##_fetch_##op##_relaxed(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");						\
> +	return ret;							\
> +}									\
> +static __always_inline							\
> +c_type arch_atomic##prefix##_fetch_##op(c_type i, atomic##prefix##_t *v)	\
> +{									\
> +	register c_type ret, temp;					\
> +	__asm__ __volatile__ (						\
> +		"1:	lr." #asm_type ".aqrl %1, %0\n"			\
> +		"	" #asm_op " %2, %1, %3\n"			\
> +		"	sc." #asm_type ".aqrl %2, %2, %0\n"		\
> +		"	bnez %2, 1b\n"					\
> +		: "+A" (v->counter), "=&r" (ret), "=&r" (temp)		\
> +		: "r" (I)						\
> +		: "memory");						\
> +	return ret;							\
> +}
> +#endif

[Severity: Critical]
As reported by Sashiko on v4 [2], does pairing lr.aqrl with sc.aqrl without a
trailing RISCV_FULL_BARRIER violate the LKMM full-barrier requirement for
fully-ordered value-returning atomic RMW primitives when
CONFIG_RISCV_ISA_ZAAMO=n?

Under RVWMO, a single AMO instruction with .aqrl is both a load and a store, so
PPO Rule 6 (rw -> store.rl) orders prior accesses before the AMO and PPO Rule 5
(load.aq -> rw) orders the AMO before subsequent accesses. In an LR/SC loop,
lr.aqrl is only a load and sc.aqrl is only a store: prior memory accesses are
ordered before sc.aqrl, and subsequent memory accesses are ordered after
lr.aqrl, but because lr.aqrl executes before sc.aqrl, a prior plain store and a
subsequent plain load to different addresses can be reordered into:

	lr.aqrl -> subsequent load -> prior store -> sc.aqrl

As established in commit 5ce6c1f3535f ("riscv/atomic: Strengthen
implementations with fences") and commit 1d84afaf0252 ("riscv: Fix fully
ordered LR/SC xchg[8|16]() implementations"), fully-ordered LR/SC sequences in
RISC-V Linux must use plain lr.{w,d}, sc.{w,d}.rl, and a trailing
RISCV_FULL_BARRIER (fence rw, rw).

This affects four places when CONFIG_RISCV_ISA_ZAAMO is disabled:
- ATOMIC_FETCH_OP() in arch/riscv/include/asm/atomic.h (and ATOMIC_OP_RETURN()
  built on top of it)
- __test_and_op_bit_ord() / __test_and_op_bit() in
  arch/riscv/include/asm/bitops.h
- __arch_xchg() and _arch_xchg() in arch/riscv/include/asm/cmpxchg.h (where
  cases 4 and 8 pass swap_sfx = ".aqrl" and swap_append = "" instead of
  sc_sfx = ".rl" and sc_append = RISCV_FULL_BARRIER, and where __arch_xchg()
  declares __typeof__(*(__ptr)) temp; using caller variable __ptr rather than
  macro parameter p)
- __futex_atomic_op() in arch/riscv/include/asm/futex.h

Should these four LR/SC sequences use plain lr, sc.rl, and a trailing
RISCV_FULL_BARRIER instead of lr.aqrl + sc.aqrl?

> diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
> index 3c1a15be54d80..ab3ffc3bfcfcb 100644
> --- a/arch/riscv/include/asm/bitops.h
> +++ b/arch/riscv/include/asm/bitops.h
> @@ -170,12 +170,17 @@ static __always_inline int variable_fls(unsigned int x)
>  
>  #if (BITS_PER_LONG == 64)
>  #define __AMO(op)	"amo" #op ".d"
> +#define __LR	"lr.d"
> +#define __SC	"sc.d"
>  #elif (BITS_PER_LONG == 32)
>  #define __AMO(op)	"amo" #op ".w"
> +#define __LR	"lr.w"
> +#define __SC	"sc.w"
>  #else
>  #error "Unexpected BITS_PER_LONG"
>  #endif
>  
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  #define __test_and_op_bit_ord(op, mod, nr, addr, ord)		\
>  ({								\
>  	unsigned long __res, __mask;				\
> @@ -194,6 +199,35 @@ static __always_inline int variable_fls(unsigned int x)
>  		: "+A" (addr[BIT_WORD(nr)])			\
>  		: "r" (mod(BIT_MASK(nr)))			\
>  		: "memory");
> +#else
> +#define __test_and_op_bit_ord(op, mod, nr, addr, ord)		\
> +({								\
> +	unsigned long __res, __mask, __temp;			\
> +	__mask = BIT_MASK(nr);					\
> +	__asm__ __volatile__ (					\
> +		"1: " __LR #ord " %0, %1\n"			\
> +		#op " %2, %0, %3\n"				\
> +		__SC #ord " %2, %2, %1\n"			\
> +		"bnez %2, 1b\n"					\
> +		: "=&r" (__res), "+A" (addr[BIT_WORD(nr)]), "=&r" (__temp)	\
> +		: "r" (mod(__mask))				\
> +		: "memory");					\
> +	((__res & __mask) != 0);				\
> +})
> +
> +#define __op_bit_ord(op, mod, nr, addr, ord)			\
> +do {								\
> +	unsigned long __res, __temp;				\
> +	__asm__ __volatile__ (					\
> +		"1: " __LR #ord " %0, %1\n"			\
> +		#op " %2, %0, %3\n"				\
> +		__SC #ord " %2, %2, %1\n"			\
> +		"bnez %2, 1b\n"					\
> +		: "=&r" (__res), "+A" (addr[BIT_WORD(nr)]), "=&r" (__temp)	\
> +		: "r" (mod(BIT_MASK(nr)))			\
> +		: "memory");					\
> +} while (0)
> +#endif
>  
>  #define __test_and_op_bit(op, mod, nr, addr) 			\
>  	__test_and_op_bit_ord(op, mod, nr, addr, .aqrl)
> @@ -337,12 +371,25 @@ static __always_inline void arch___clear_bit_unlock(
>  static __always_inline bool arch_xor_unlock_is_negative_byte(unsigned long mask,
>  		volatile unsigned long *addr)
>  {
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  	unsigned long res;
>  	__asm__ __volatile__ (
>  		__AMO(xor) ".rl %0, %2, %1"
>  		: "=r" (res), "+A" (*addr)
>  		: "r" (__NOP(mask))
>  		: "memory");
> +#else
> +	unsigned long res, temp;
> +
> +	__asm__ __volatile__ (
> +		"1: " __LR ".rl %0, %1\n"
> +		"xor %2, %0, %3\n"
> +		__SC ".rl %2, %2, %1\n"
> +		"bnez %2, 1b\n"
> +		: "=&r" (res), "+A" (*addr), "=&r" (temp)
> +		: "r" (__NOP(mask))
> +		: "memory");
> +#endif
>  	return (res & BIT(7)) != 0;
>  }

[Severity: High]
As reported by Sashiko on v4 [2], in arch/riscv/include/asm/bitops.h when
CONFIG_RISCV_ISA_ZAAMO=n, __test_and_op_bit_ord() and __op_bit_ord() paste
#ord onto both __LR and __SC, and arch_xor_unlock_is_negative_byte() emits
__LR ".rl".

Doesn't this produce reserved/ineffective Zalrsc ordering combinations and
weaken release ordering in arch_xor_unlock_is_negative_byte()?

1. When arch_test_and_set_bit_lock() calls __test_and_op_bit_ord() with
ord = .aq, it emits sc.{d,w}.aq, which has aq=1, rl=0 on a store-conditional.
2. When arch_clear_bit_unlock() calls __op_bit_ord() with ord = .rl, and in
arch_xor_unlock_is_negative_byte(), it emits lr.{d,w}.rl, which has aq=0, rl=1
on a load-reserved.
3. In arch_xor_unlock_is_negative_byte(), callers such as
folio_xor_flags_has_waiters() unlock PG_locked with release semantics while
checking BIT(7) (PG_waiters) in the old value res. Because .rl on lr has no
RVWMO release effect (PPO Rule 6 only orders prior accesses before stores with
.rl), memory accesses inside the critical section are only ordered before
sc.rl and can be reordered after the lr load that samples BIT(7) into res.
4. For __test_and_op_bit() (used by arch_test_and_set_bit(),
arch_test_and_clear_bit(), and arch_test_and_change_bit()), ord = .aqrl emits
lr.aqrl + sc.aqrl without a trailing RISCV_FULL_BARRIER.
5. Finally, at the bottom of arch/riscv/include/asm/bitops.h:

	#undef __test_and_op_bit
	#undef __op_bit
	#undef __NOP
	#undef __NOT
	#undef __AMO

should __LR and __SC also be undefined here so they do not leak into
translation units that include asm/bitops.h?

[1] https://lore.kernel.org/r/mhng-6BAAFBCB-95B3-45AC-9087-C00241996255@Palmers-Mini.rwc.dabbelt.com
[2] https://lore.kernel.org/r/20261101-lrsc-only-v4-1-4e1d010463b3@mobileye.com
[3] https://lore.kernel.org/r/20261102-persuader-catering-5ed81c35ae0e@spud

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-lrsc-only-v5-0-f0648f57b2e1@mobileye.com?part=1