[PATCH v5 0/3] riscv: support CPUs with slow Zaamo

[PATCH v5 0/3] riscv: support CPUs with slow Zaamo

[Vladimir Kondratiev]

RISC-V split the A extension into Zaamo (AMO*.w/AMO*.d) and Zalrsc
(LR/SC). The kernel puts "a" in -march and emits AMO for atomic_add,
bitops, xchg and the like, and LR/SC for cmpxchg.

Zaamo is not always native. On MIPS P8700 [1] every AMO instruction
traps into M-mode and is emulated there, so the extension is formally
supported, and advertised to userspace, but very slow. A typical
uncontended atomic increment costs:

  native AMO                                     2..5 cycles
  LR/SC, private L1 reservation monitor         6..12 cycles
  LR/SC, PoC (L2) reservation monitor       about 40 cycles
  AMO emulated by an M-mode trap           about 600 cycles

The last two were measured on the eyeq7h hardware. On such a CPU the
kernel has to use LR/SC for every atomic.

Mobileye is going to use the "eyeq7h" platform, built on this CPU, for
ADAS and autonomous driving. Kernel atomics are on the scheduling,
memory-management and driver paths of that product, so the emulated
AMO cost is not acceptable there.

Patch 1 adds RISCV_ISA_ZAAMO. It defaults to y and nothing changes:
-march keeps "a" and the generated code is the same. When it is turned
off, -march carries _zalrsc instead of "a" and every kernel atomic is
an LR/SC loop. Such a kernel runs on any hart that implements A, it
just does not use AMO, so the option does not depend on NONPORTABLE.
Turning Zaamo off needs a compiler that accepts _zaamo and _zalrsc in
-march; GCC 15.1 is the first release that does.

The choice is made at compile time rather than with alternatives.
Nearly every CPU has a native A extension, and an alternative would
inflate all those kernels with the nops needed to pad one AMO to the
size of an LR/SC loop, at every atomic site, for the benefit of a few.

Nothing changes for userspace. The hart still advertises "a", which is
true. Userspace that cares can know about the slow AMO upfront, or
measure it.

Zalrsc stays mandatory. cmpxchg, conditional atomics and futex CAS are
already LR/SC sequences; Zaamo cannot replace them. RISCV_ISA_ZALRSC
is always y and exists as the Kconfig counterpart of
RISCV_ISA_EXT_ZALRSC, which hwcap.h already has next to
RISCV_ISA_EXT_ZAAMO; the two sets are tested together, for example by
rv_ext_enabled() in the BPF JIT. RISCV_ISA_A exists for the same
reason, paired with RISCV_ISA_EXT_A.

Three sets of extensions provide every atomic Linux needs: "a", which
is what the kernel supports today; Zalrsc plus Ziccrse, which this
series adds, with Ziccrse as the forward-progress guarantee for the
constrained LR/SC loops; and Zaamo plus Zacas, with CAS loops in place
of LR/SC as on x86. The series leaves the last one open:
RISCV_ISA_ZALRSC is where it would go, by allowing "n" there and making
amocas the base implementation.

The BPF JIT encodes its own atomics. Patch 2 makes it emit LR/SC loops
when Zaamo is disabled, so BPF programs do not trap either. The Zabha
and Zacas paths in the JIT and in cmpxchg.h are selected at runtime
from the ISA string; firmware for a CPU that emulates AMO should not
advertise them, as both extensions are AMO.

A breakpoint inside an LR/SC sequence drops the reservation and the
loop never completes. With every atomic an LR/SC loop that is easy to
hit, so patch 3 makes kprobes and uprobes reject such addresses.
kprobes walk from the enclosing symbol; uprobes scan forward using the
constrained-sequence rules from the ISA. This patch fixes a problem
that exists today, since cmpxchg is already an LR/SC loop. It does not
depend on the rest of the series and can be merged on its own.

[1] https://mips.com/products/hardware/p8700/

Changes in v5:
- Treat Zaamo as supported but slow rather than absent. The hart keeps
  advertising "a" and the kernel keeps accepting it, so the boot-time
  extension check patch is dropped.
- Drop the NONPORTABLE dependency. A kernel built without Zaamo runs
  on any hart that implements A.
- Rename TOOLCHAIN_HAS_ZALRSC to TOOLCHAIN_HAS_ZALRSC_ZAAMO and test
  both _zaamo and _zalrsc in -march.
- Explain why RISCV_ISA_ZALRSC and RISCV_ISA_A exist, and why the
  choice is made at compile time rather than with alternatives.
- Add the BPF JIT patch. Without Zaamo the JIT emits LR/SC loops for
  the BPF read-modify-write atomics on RV32 and RV64, and reserves a
  second exception-table entry for the sc of an arena atomic.
- Reject kprobes and uprobes on lr and sc, and on any instruction
  between them.
- Link to v4: https://patch.msgid.link/20261101-lrsc-only-v4-0-4e1d010463b3@mobileye.com

Changes in v4:
- Require a compiler that accepts _zalrsc in -march before Zaamo can
  be disabled. GCC 15.1 introduces that support. Older compilers keep
  Zaamo enabled and -march keeps "a".
- Link to v3: https://patch.msgid.link/20261026-lrsc-only-v3-0-ec29c8190fb1@mobileye.com

Changes in v3:
- Make Zalrsc mandatory. cmpxchg and the conditional atomics already
  use LR/SC, so RISCV_ISA_ZALRSC can no longer be turned off.
- Check AMO and LR/SC support independently. A kernel that emits both
  requires "a", or both "zaamo" and "zalrsc".
- Put the LR/SC bitop sequence in its own block so two inlined
  set_bit or clear_bit calls compile.
- Remove the LR/SC loop that ran after the hart lottery and accessed
  address 1. Build the lottery increment with add so RV32 assembles.
- Add an exception-table entry for sc as well as lr in the futex
  LR/SC helper.

Changes in v2:
- Switch from a runtime choice of atomic implementation to a
  compile-time one, gated by NONPORTABLE.

Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
---
Vladimir Kondratiev (3):
      riscv: build kernel atomics from LR/SC when Zaamo is disabled
      riscv, bpf: emit LR/SC loops when Zaamo is disabled
      riscv: probes: reject probes inside an LR/SC sequence

 arch/riscv/Kconfig                     |  53 ++++++++++++++++
 arch/riscv/Makefile                    |  16 +++--
 arch/riscv/include/asm/atomic.h        |  52 +++++++++++++++-
 arch/riscv/include/asm/bitops.h        |  47 ++++++++++++++
 arch/riscv/include/asm/cmpxchg.h       |  16 +++++
 arch/riscv/include/asm/futex.h         |  45 ++++++++++++++
 arch/riscv/include/asm/insn.h          |  28 +++++++++
 arch/riscv/kernel/entry.S              |   7 +++
 arch/riscv/kernel/head.S               |  12 ++++
 arch/riscv/kernel/probes/decode-insn.c |  94 ++++++++++++++++++++++++++++
 arch/riscv/kernel/probes/decode-insn.h |   7 +++
 arch/riscv/kernel/probes/kprobes.c     |  22 ++++++-
 arch/riscv/kernel/probes/uprobes.c     |  31 ++++++++++
 arch/riscv/net/bpf_jit.h               |   1 +
 arch/riscv/net/bpf_jit_comp32.c        |  81 +++++++++++++++++++++++--
 arch/riscv/net/bpf_jit_comp64.c        | 108 ++++++++++++++++++++++++++++++++-
 arch/riscv/net/bpf_jit_core.c          |  29 +++++++++
 17 files changed, 636 insertions(+), 13 deletions(-)
---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20260120-lrsc-only-cc06422f5bb1

Best regards,
--  
Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

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

[Vladimir Kondratiev]

The A extension is Zaamo plus Zalrsc. On some CPUs Zaamo is present
only formally. MIPS P8700 traps every AMO instruction into M-mode and
emulates it there, so the extension is supported but very slow. A
typical uncontended atomic increment costs:

  native AMO                                     2..5 cycles
  LR/SC, private L1 reservation monitor         6..12 cycles
  LR/SC, PoC (L2) reservation monitor       about 40 cycles
  AMO emulated by an M-mode trap           about 600 cycles

The last two were measured on the eyeq7h hardware. On such a CPU the
kernel must use LR/SC for every atomic.

Add RISCV_ISA_ZAAMO, default y. The default keeps "a" in -march and
the generated code unchanged. When it is disabled, -march carries
_zalrsc instead of "a", and atomic_*, bitops, xchg, futex and the
early boot code use LR/SC loops. The sequence is chosen with the
preprocessor, on CONFIG_RISCV_ISA_ZAAMO like the other ISA options in
the headers, because the assembler rejects AMO mnemonics once -march
lacks Zaamo. A kernel built this way runs on any hart that implements A; it just
does not use AMO. It is therefore a plain option, not a NONPORTABLE
one, and the hart keeps advertising "a" to userspace, which is true.

The choice is made at compile time rather than with alternatives.
Nearly every CPU has a native A extension, and an alternative would
inflate all those kernels with the nops needed to pad one AMO to the
size of an LR/SC loop, at every atomic site, for the benefit of a few.

Turning Zaamo off needs a compiler that accepts _zaamo and _zalrsc in
-march. GCC 15.1 is the first release that does.
TOOLCHAIN_HAS_ZALRSC_ZAAMO checks for it; with an older compiler the
option stays at y.

Add RISCV_ISA_ZALRSC as well, always y, because cmpxchg and the
conditional atomics are LR/SC sequences that Zaamo cannot replace. It
exists as the Kconfig counterpart of RISCV_ISA_EXT_ZALRSC in hwcap.h,
added by commit 35173b666a16 ("riscv: add parsing for Zaamo and Zalrsc
extensions"). The two sets are used together, for instance by
rv_ext_enabled() in the BPF JIT, so an extension the kernel can be
built for has both. RISCV_ISA_A, the counterpart of RISCV_ISA_EXT_A,
follows the same rule and puts "a" in -march.

Three sets of extensions provide every atomic Linux needs. "a" is what
the kernel supports today. Zalrsc plus Ziccrse is what this patch adds:
every atomic is a constrained LR/SC loop, and Ziccrse is the
forward-progress guarantee for such loops. Zaamo plus Zacas would do as
well, with CAS loops in place of LR/SC as on x86. This patch leaves
that option open: RISCV_ISA_ZALRSC is where it would go, by allowing
"n" there and making amocas the base implementation.

Suggested-by: Chao-ying Fu <chao-ying.fu@globalfoundries.com>
Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
---
 arch/riscv/Kconfig               | 53 ++++++++++++++++++++++++++++++++++++++++
 arch/riscv/Makefile              | 16 +++++++++---
 arch/riscv/include/asm/atomic.h  | 52 ++++++++++++++++++++++++++++++++++++++-
 arch/riscv/include/asm/bitops.h  | 47 +++++++++++++++++++++++++++++++++++
 arch/riscv/include/asm/cmpxchg.h | 16 ++++++++++++
 arch/riscv/include/asm/futex.h   | 45 ++++++++++++++++++++++++++++++++++
 arch/riscv/kernel/entry.S        |  7 ++++++
 arch/riscv/kernel/head.S         | 12 +++++++++
 8 files changed, 243 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 5874ca30f4f6..07d15a0bb379 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.
+
+	  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 44f5a439741a..5cdc9cc017bb 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,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
+
 # 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 +93,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..3b189bea1d08 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.
  */
+#ifdef CONFIG_RISCV_ISA_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,23 @@ void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
 		: "+A" (v->counter)					\
 		: "r" (I)						\
 		: "memory");						\
-}									\
+}
+#else
+#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");						\
+}
+#endif
 
 #ifdef CONFIG_GENERIC_ATOMIC64
 #define ATOMIC_OPS(op, asm_op, I)					\
@@ -84,6 +101,7 @@ ATOMIC_OPS(xor, xor,  i)
  * There's two flavors of these: the arithmetic ops have both fetch and return
  * versions, while the logical ops only have fetch versions.
  */
+#ifdef CONFIG_RISCV_ISA_ZAAMO
 #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,		\
@@ -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
 
 #define ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_type, c_type, prefix)	\
 static __always_inline							\
diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
index 3c1a15be54d8..ab3ffc3bfcfc 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;
 }
 
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index 662e160b0522..883dc4aa1dc3 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -57,6 +57,7 @@
 	}									\
 })
 
+#ifdef CONFIG_RISCV_ISA_ZAAMO
 #define __arch_xchg(sfx, prepend, append, r, p, n)			\
 ({									\
 	__asm__ __volatile__ (						\
@@ -67,6 +68,21 @@
 		: "r" (n)						\
 		: "memory");						\
 })
+#else
+#define __arch_xchg(sfx, prepend, append, r, p, n)			\
+({									\
+	__typeof__(*(__ptr)) temp;					\
+	__asm__ __volatile__ (						\
+		prepend							\
+		"1:	lr" sfx " %0, %1\n"				\
+		"	sc" sfx " %2, %3, %1\n"				\
+		"	bnez %2, 1b\n"					\
+		append							\
+		: "=&r" (r), "+A" (*(p)), "=&r" (temp)			\
+		: "r" (n)						\
+		: "memory");						\
+})
+#endif
 
 #define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend,			\
 		   sc_append, swap_append)				\
diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
index 6f3bc9bdb85e..58f12bf2af68 100644
--- a/arch/riscv/include/asm/futex.h
+++ b/arch/riscv/include/asm/futex.h
@@ -19,6 +19,7 @@
 #define __disable_user_access()		do { } while (0)
 #endif
 
+#ifdef CONFIG_RISCV_ISA_ZAAMO
 #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)	\
 {								\
 	__enable_user_access();					\
@@ -32,6 +33,27 @@
 	: "memory");						\
 	__disable_user_access();				\
 }
+#else
+#define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)	\
+{								\
+	int temp;						\
+								\
+	__enable_user_access();					\
+	__asm__ __volatile__ (					\
+	"1:	lr.w.aqrl %[ov], %[u]			\n"	\
+	"	" insn "				\n"	\
+	"2:	sc.w.aqrl %[t], %[t], %[u]		\n"	\
+	"	bnez %[t], 1b				\n"	\
+	"3:						\n"	\
+	_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r])			\
+	_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r])			\
+	: [r] "+r" (ret), [ov] "=&r" (oldval),			\
+	  [t] "=&r" (temp), [u] "+m" (*(uaddr))			\
+	: [op] "Jr" (oparg)					\
+	: "memory");						\
+	__disable_user_access();				\
+}
+#endif
 
 static inline int
 arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
@@ -43,6 +65,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
 	uaddr = untagged_addr(uaddr);
 
 	switch (op) {
+#ifdef CONFIG_RISCV_ISA_ZAAMO
 	case FUTEX_OP_SET:
 		__futex_atomic_op("amoswap.w.aqrl %[ov],%z[op],%[u]",
 				  ret, oldval, uaddr, oparg);
@@ -63,6 +86,28 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
 		__futex_atomic_op("amoxor.w.aqrl %[ov],%z[op],%[u]",
 				  ret, oldval, uaddr, oparg);
 		break;
+#else
+	case FUTEX_OP_SET:
+		__futex_atomic_op("mv %[t], %z[op]",
+				  ret, oldval, uaddr, oparg);
+		break;
+	case FUTEX_OP_ADD:
+		__futex_atomic_op("add %[t], %[ov], %z[op]",
+				  ret, oldval, uaddr, oparg);
+		break;
+	case FUTEX_OP_OR:
+		__futex_atomic_op("or %[t], %[ov], %z[op]",
+				  ret, oldval, uaddr, oparg);
+		break;
+	case FUTEX_OP_ANDN:
+		__futex_atomic_op("and %[t], %[ov], %z[op]",
+				  ret, oldval, uaddr, ~oparg);
+		break;
+	case FUTEX_OP_XOR:
+		__futex_atomic_op("xor %[t], %[ov], %z[op]",
+				  ret, oldval, uaddr, oparg);
+		break;
+#endif
 	default:
 		ret = -ENOSYS;
 	}
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 61b7a7716697..ba2db6804a6c 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -73,7 +73,14 @@
 	beq	a2, zero, .Lnew_valid_map_cpus_restore_context
 
 	/* Atomically reset the current cpu bit in new_valid_map_cpus */
+#ifdef CONFIG_RISCV_ISA_ZAAMO
 	amoxor.d	a0, a1, (a0)
+#else
+1:	lr.d	a2, (a0)
+	xor	a2, a1, a2
+	sc.d	a2, a2, (a0)
+	bnez	a2, 1b
+#endif
 
 	/*
 	 * A sfence.vma is required here. Even if we had Svvptc, there's no
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index f6a8ca49e627..dc3862ed29c4 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -265,7 +265,19 @@ SYM_CODE_START(_start_kernel)
 	/* Pick one hart to run the main boot sequence */
 	la a3, hart_lottery
 	li a2, 1
+#ifdef CONFIG_RISCV_ISA_ZAAMO
 	amoadd.w a3, a2, (a3)
+#else
+	/*
+	 * hart_lottery is a small 32-bit counter. add is valid on RV32
+	 * and RV64; sc.w stores the low 32 bits.
+	 */
+1:	lr.w t0, (a3)
+	add t1, t0, a2
+	sc.w t1, t1, (a3)
+	bnez t1, 1b
+	mv a3, t0
+#endif
 	bnez a3, .Lsecondary_start
 #endif /* CONFIG_RISCV_BOOT_SPINWAIT */
 

-- 
2.43.0

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

[sashiko-bot]

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

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

[bot+bpf-ci]

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

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

TL;DR:
- Unbounded futex LR/SC loop may starve, spinning with hash locks held
- kprobe on LR/SC atomics may livelock
- Zacas/Zabha still emit AMOs, contradicting docs
- redundant lr/sc ordering suffixes

> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig

[ ... ]

> +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.
> +
> [ ... ]
> +	  A kernel built without Zaamo runs on any CPU that implements the
> +	  A extension; it only does not use AMO.

The help text says a kernel built without Zaamo "only does not use AMO", and
the commit message says "On such a CPU the kernel must use LR/SC for every
atomic" and "every atomic is a constrained LR/SC loop".

Is that accurate when the hart reports Zacas or Zabha?  RISCV_ISA_ZACAS and
RISCV_ISA_ZABHA stay default y and do not depend on RISCV_ISA_ZAAMO, so a
ZAAMO=n kernel still issues AMO-class instructions on those harts:

__arch_cmpxchg():
    if (IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) && ...
        riscv_has_extension_unlikely(RISCV_ISA_EXT_ZACAS))
    -> amocas.w / amocas.d

__arch_cmpxchg_masked() -> amocas.b / amocas.h
__arch_xchg_masked()    -> amoswap.b / amoswap.h (Zabha)

riscv_spinlock_init() also selects the queued spinlock using Zabha when
Zabha and Zacas are both present, so the spinlock fast paths would run on
amocas/amoswap.h.

On a CPU that emulates AMOs with an M-mode trap and also reports
Zacas/Zabha, the hottest atomics would still trap.

Should RISCV_ISA_ZACAS and RISCV_ISA_ZABHA depend on RISCV_ISA_ZAAMO?  If
not, should the help text and changelog say that Zacas/Zabha AMOs are still
used when the hart reports them?

[ ... ]

> diff --git a/arch/riscv/include/asm/atomic.h b/arch/riscv/include/asm/atomic.h
> --- a/arch/riscv/include/asm/atomic.h
> +++ b/arch/riscv/include/asm/atomic.h

[ ... ]

> +#else
> +#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");						\
> +}
> +#endif

Can a kprobe placed on one of these lr/sc loops livelock the CPU?  With
CONFIG_RISCV_ISA_ZAAMO=n, every inlined arch_atomic{,64}_<op>() and
arch_atomic{,64}_fetch_<op>{,_relaxed}() becomes an lr / op / sc / bnez
loop, where before it was a single amo<op> instruction that is safe to
probe.

arch_check_kprobe() only checks that the address is on an instruction
boundary, and riscv_probe_decode_insn() rejects only system, fence and
c.ebreak instructions, so a probe on the lr, the sc, or an instruction
between them is accepted.  The exception return path in entry.S then clears
the reservation on purpose:

    REG_L  a2, PT_EPC(sp)
    REG_SC x0, a2, PT_EPC(sp)

The trap for the single-step slot does the same, so the following sc always
fails, bnez goes back to lr, the probe fires again, and the loop never
completes.

This was already possible for the existing cmpxchg and conditional-atomic
LR/SC loops, but with ZAAMO=n it now reaches every atomic_inc(),
atomic_add() and atomic_fetch_*() site, including small out-of-line
functions whose first instruction is the lr.

A later commit in this series, 01f0e5fb499d ("riscv: probes: reject probes
inside an LR/SC sequence"), fixes this, but it is still reachable at this
commit, which matters for bisection and backports.

[ ... ]

> diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
> --- a/arch/riscv/include/asm/bitops.h
> +++ b/arch/riscv/include/asm/bitops.h

[ ... ]

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

Both macros put the same ordering suffix on the lr and on the sc.  That
gives:

arch_test_and_set_bit_lock()   (ord = .aq)  -> lr.d.aq  ... sc.d.aq
arch_clear_bit_unlock()        (ord = .rl)  -> lr.d.rl  ... sc.d.rl
arch___clear_bit_unlock()      (ord = .rl)  -> lr.d.rl  ... sc.d.rl

arch_xor_unlock_is_negative_byte() hard-codes the same pattern with
__LR ".rl" and __SC ".rl".

The A extension spec says software should not set rl on an LR unless aq is
also set, and should not set aq on an SC unless rl is also set, and that
LR.rl and SC.aq are not guaranteed to provide any stronger ordering than
having both bits clear but may result in lower performance.

The ordering that is wanted still comes from lr.aq and sc.rl, so the
semantics hold, but the other annotations do nothing useful and may be
slower on the hardware this option targets.  They also differ from the rest
of arch/riscv, where LR/SC ordering is a plain lr plus sc.rl and fences (see
arch_cmpxchg_acquire/_release and __arch_cmpxchg(), whose lr has no
ordering suffix).

Could these macros take separate lr and sc suffixes, as the cmpxchg macros
do with lr_sfx/sc_sfx?

[ ... ]

> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h

[ ... ]

This is the same issue as the Kconfig comment above.  The #ifdef
CONFIG_RISCV_ISA_ZAAMO in this header does not touch these paths, which
still emit AMO instructions when the hart advertises Zabha or Zacas:

__arch_xchg_masked()      -> amoswap.b/.h (Zabha), including xchg_relaxed()
                             on the 16-bit qspinlock tail
__arch_cmpxchg()          -> amocas.w/.d (Zacas), i.e. every cmpxchg and
                             try_cmpxchg, including the spinlock fast paths
__arch_cmpxchg_masked()   -> amocas.b/.h (Zabha + Zacas)
__arch_cmpxchg128()       -> amocas.q

These still build because each one enables the extension with
".option arch, +zabha" or ".option arch, +zacas".

This contradicts the commit message statement "A kernel built this way runs
on any hart that implements A; it just does not use AMO".  Should
RISCV_ISA_ZACAS and RISCV_ISA_ZABHA depend on RISCV_ISA_ZAAMO?

[ ... ]

> diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
> --- a/arch/riscv/include/asm/futex.h
> +++ b/arch/riscv/include/asm/futex.h

[ ... ]

> +#else
> +#define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)	\
> +{								\
> +	int temp;						\
> +								\
> +	__enable_user_access();					\
> +	__asm__ __volatile__ (					\
> +	"1:	lr.w.aqrl %[ov], %[u]			\n"	\
> +	"	" insn "				\n"	\
> +	"2:	sc.w.aqrl %[t], %[t], %[u]		\n"	\
> +	"	bnez %[t], 1b				\n"	\

Can this loop run unbounded while the futex hash-bucket locks are held?
With CONFIG_RISCV_ISA_ZAAMO=n, FUTEX_WAKE_OP now runs an LR/SC loop on user
memory:

futex(FUTEX_WAKE_OP)
  futex_wake_op()                    /* kernel/futex/waitwake.c */
    double_lock_hb(hb1, hb2)
    futex_atomic_op_inuser(op, uaddr2)
      pagefault_disable()
      arch_futex_atomic_op_inuser()
        __futex_atomic_op()

The loop exits only when sc.w.aqrl succeeds or faults.  The RISC-V
eventual-success guarantee for constrained LR/SC loops (the one Ziccrse
provides) does not hold while other harts keep writing to the reservation
set; the spec says that in that case no hart is guaranteed to leave its
LR/SC loop.

A process can do exactly that: one thread calls FUTEX_WAKE_OP on uaddr2
while a second thread on another hart stores to *uaddr2, or anything else
in the same reservation granule, in a tight loop.  The kernel then spins
with preemption disabled and both hash-bucket locks held.  The old
amo*.w.aqrl version cannot be starved this way because an AMO always
completes.

arm64 bounds its LDXR/STXR futex loop: LLSC_FUTEX_ATOMIC_OP() in
arch/arm64/include/asm/futex.h counts down FUTEX_MAX_LOOPS and returns
-EAGAIN, and futex_wake_op() already handles that:

    if (!IS_ENABLED(CONFIG_MMU) ||
        unlikely(op_ret != -EFAULT && op_ret != -EAGAIN))

by dropping the locks, calling cond_resched() and retrying.

The commit message says Ziccrse is "the forward-progress guarantee for such
loops", which does not cover this case.  The existing riscv
futex_atomic_cmpxchg_inatomic() has the same unbounded pattern, but that
does not make the new instance correct.

Should both loops be bounded the way arm64 does it?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42069736200

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

[Jisheng Zhang]

On Thu, Nov 05, 2026 at 06:37:36PM +0200, Vladimir Kondratiev wrote:
> The A extension is Zaamo plus Zalrsc. On some CPUs Zaamo is present
> only formally. MIPS P8700 traps every AMO instruction into M-mode and
> emulates it there, so the extension is supported but very slow. A
> typical uncontended atomic increment costs:
> 
>   native AMO                                     2..5 cycles
>   LR/SC, private L1 reservation monitor         6..12 cycles
>   LR/SC, PoC (L2) reservation monitor       about 40 cycles
>   AMO emulated by an M-mode trap           about 600 cycles
> 
> The last two were measured on the eyeq7h hardware. On such a CPU the
> kernel must use LR/SC for every atomic.
> 
> Add RISCV_ISA_ZAAMO, default y. The default keeps "a" in -march and
> the generated code unchanged. When it is disabled, -march carries
> _zalrsc instead of "a", and atomic_*, bitops, xchg, futex and the
> early boot code use LR/SC loops. The sequence is chosen with the
> preprocessor, on CONFIG_RISCV_ISA_ZAAMO like the other ISA options in
> the headers, because the assembler rejects AMO mnemonics once -march
> lacks Zaamo. A kernel built this way runs on any hart that implements A; it just
> does not use AMO. It is therefore a plain option, not a NONPORTABLE
> one, and the hart keeps advertising "a" to userspace, which is true.
> 
> The choice is made at compile time rather than with alternatives.
> Nearly every CPU has a native A extension, and an alternative would
> inflate all those kernels with the nops needed to pad one AMO to the
> size of an LR/SC loop, at every atomic site, for the benefit of a few.
> 
> Turning Zaamo off needs a compiler that accepts _zaamo and _zalrsc in
> -march. GCC 15.1 is the first release that does.
> TOOLCHAIN_HAS_ZALRSC_ZAAMO checks for it; with an older compiler the
> option stays at y.
> 
> Add RISCV_ISA_ZALRSC as well, always y, because cmpxchg and the
> conditional atomics are LR/SC sequences that Zaamo cannot replace. It
> exists as the Kconfig counterpart of RISCV_ISA_EXT_ZALRSC in hwcap.h,
> added by commit 35173b666a16 ("riscv: add parsing for Zaamo and Zalrsc
> extensions"). The two sets are used together, for instance by
> rv_ext_enabled() in the BPF JIT, so an extension the kernel can be
> built for has both. RISCV_ISA_A, the counterpart of RISCV_ISA_EXT_A,
> follows the same rule and puts "a" in -march.
> 
> Three sets of extensions provide every atomic Linux needs. "a" is what
> the kernel supports today. Zalrsc plus Ziccrse is what this patch adds:
> every atomic is a constrained LR/SC loop, and Ziccrse is the
> forward-progress guarantee for such loops. Zaamo plus Zacas would do as
> well, with CAS loops in place of LR/SC as on x86. This patch leaves
> that option open: RISCV_ISA_ZALRSC is where it would go, by allowing
> "n" there and making amocas the base implementation.

'A' is part of linux's existing RISC-V minimum ISA from day1. So every
piece of riscv arch code and linux userspace is written with that in mind.
Making an im_zalrsc kernel work addresses the implemenation gaps(though I
doubt current series close all gaps, e.g what about rust? rust part still
assumes IMA), but would not by itself justify changing that minimum

Your system already implements 'A' through firmware, the motivation
of this series is performance. As Palmer pointed out, could you plz
provide representative workload results showing the benefit warrants
another supported ISA configuration and its maintenance cost?

I'm not in favour of lowering the baseline which was set up from
day1 solely to accommodate this implementation's AMO performance.
Until that broader policy change is justified and determined by
the riscv maintainers, I think retaining A as a mandatory and keep
this serie out of tree is the best.

> 
> Suggested-by: Chao-ying Fu <chao-ying.fu@globalfoundries.com>
> Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> ---
>  arch/riscv/Kconfig               | 53 ++++++++++++++++++++++++++++++++++++++++
>  arch/riscv/Makefile              | 16 +++++++++---
>  arch/riscv/include/asm/atomic.h  | 52 ++++++++++++++++++++++++++++++++++++++-
>  arch/riscv/include/asm/bitops.h  | 47 +++++++++++++++++++++++++++++++++++
>  arch/riscv/include/asm/cmpxchg.h | 16 ++++++++++++
>  arch/riscv/include/asm/futex.h   | 45 ++++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/entry.S        |  7 ++++++
>  arch/riscv/kernel/head.S         | 12 +++++++++
>  8 files changed, 243 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 5874ca30f4f6..07d15a0bb379 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.
> +
> +	  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 44f5a439741a..5cdc9cc017bb 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,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
> +
>  # 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 +93,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..3b189bea1d08 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.
>   */
> +#ifdef CONFIG_RISCV_ISA_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,23 @@ void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
>  		: "+A" (v->counter)					\
>  		: "r" (I)						\
>  		: "memory");						\
> -}									\
> +}
> +#else
> +#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");						\
> +}
> +#endif
>  
>  #ifdef CONFIG_GENERIC_ATOMIC64
>  #define ATOMIC_OPS(op, asm_op, I)					\
> @@ -84,6 +101,7 @@ ATOMIC_OPS(xor, xor,  i)
>   * There's two flavors of these: the arithmetic ops have both fetch and return
>   * versions, while the logical ops only have fetch versions.
>   */
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  #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,		\
> @@ -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
>  
>  #define ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_type, c_type, prefix)	\
>  static __always_inline							\
> diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
> index 3c1a15be54d8..ab3ffc3bfcfc 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;
>  }
>  
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 662e160b0522..883dc4aa1dc3 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -57,6 +57,7 @@
>  	}									\
>  })
>  
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  #define __arch_xchg(sfx, prepend, append, r, p, n)			\
>  ({									\
>  	__asm__ __volatile__ (						\
> @@ -67,6 +68,21 @@
>  		: "r" (n)						\
>  		: "memory");						\
>  })
> +#else
> +#define __arch_xchg(sfx, prepend, append, r, p, n)			\
> +({									\
> +	__typeof__(*(__ptr)) temp;					\
> +	__asm__ __volatile__ (						\
> +		prepend							\
> +		"1:	lr" sfx " %0, %1\n"				\
> +		"	sc" sfx " %2, %3, %1\n"				\
> +		"	bnez %2, 1b\n"					\
> +		append							\
> +		: "=&r" (r), "+A" (*(p)), "=&r" (temp)			\
> +		: "r" (n)						\
> +		: "memory");						\
> +})
> +#endif
>  
>  #define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend,			\
>  		   sc_append, swap_append)				\
> diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
> index 6f3bc9bdb85e..58f12bf2af68 100644
> --- a/arch/riscv/include/asm/futex.h
> +++ b/arch/riscv/include/asm/futex.h
> @@ -19,6 +19,7 @@
>  #define __disable_user_access()		do { } while (0)
>  #endif
>  
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)	\
>  {								\
>  	__enable_user_access();					\
> @@ -32,6 +33,27 @@
>  	: "memory");						\
>  	__disable_user_access();				\
>  }
> +#else
> +#define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)	\
> +{								\
> +	int temp;						\
> +								\
> +	__enable_user_access();					\
> +	__asm__ __volatile__ (					\
> +	"1:	lr.w.aqrl %[ov], %[u]			\n"	\
> +	"	" insn "				\n"	\
> +	"2:	sc.w.aqrl %[t], %[t], %[u]		\n"	\
> +	"	bnez %[t], 1b				\n"	\
> +	"3:						\n"	\
> +	_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r])			\
> +	_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r])			\
> +	: [r] "+r" (ret), [ov] "=&r" (oldval),			\
> +	  [t] "=&r" (temp), [u] "+m" (*(uaddr))			\
> +	: [op] "Jr" (oparg)					\
> +	: "memory");						\
> +	__disable_user_access();				\
> +}
> +#endif
>  
>  static inline int
>  arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
> @@ -43,6 +65,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  	uaddr = untagged_addr(uaddr);
>  
>  	switch (op) {
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  	case FUTEX_OP_SET:
>  		__futex_atomic_op("amoswap.w.aqrl %[ov],%z[op],%[u]",
>  				  ret, oldval, uaddr, oparg);
> @@ -63,6 +86,28 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  		__futex_atomic_op("amoxor.w.aqrl %[ov],%z[op],%[u]",
>  				  ret, oldval, uaddr, oparg);
>  		break;
> +#else
> +	case FUTEX_OP_SET:
> +		__futex_atomic_op("mv %[t], %z[op]",
> +				  ret, oldval, uaddr, oparg);
> +		break;
> +	case FUTEX_OP_ADD:
> +		__futex_atomic_op("add %[t], %[ov], %z[op]",
> +				  ret, oldval, uaddr, oparg);
> +		break;
> +	case FUTEX_OP_OR:
> +		__futex_atomic_op("or %[t], %[ov], %z[op]",
> +				  ret, oldval, uaddr, oparg);
> +		break;
> +	case FUTEX_OP_ANDN:
> +		__futex_atomic_op("and %[t], %[ov], %z[op]",
> +				  ret, oldval, uaddr, ~oparg);
> +		break;
> +	case FUTEX_OP_XOR:
> +		__futex_atomic_op("xor %[t], %[ov], %z[op]",
> +				  ret, oldval, uaddr, oparg);
> +		break;
> +#endif
>  	default:
>  		ret = -ENOSYS;
>  	}
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 61b7a7716697..ba2db6804a6c 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -73,7 +73,14 @@
>  	beq	a2, zero, .Lnew_valid_map_cpus_restore_context
>  
>  	/* Atomically reset the current cpu bit in new_valid_map_cpus */
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  	amoxor.d	a0, a1, (a0)
> +#else
> +1:	lr.d	a2, (a0)
> +	xor	a2, a1, a2
> +	sc.d	a2, a2, (a0)
> +	bnez	a2, 1b
> +#endif
>  
>  	/*
>  	 * A sfence.vma is required here. Even if we had Svvptc, there's no
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index f6a8ca49e627..dc3862ed29c4 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -265,7 +265,19 @@ SYM_CODE_START(_start_kernel)
>  	/* Pick one hart to run the main boot sequence */
>  	la a3, hart_lottery
>  	li a2, 1
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  	amoadd.w a3, a2, (a3)
> +#else
> +	/*
> +	 * hart_lottery is a small 32-bit counter. add is valid on RV32
> +	 * and RV64; sc.w stores the low 32 bits.
> +	 */
> +1:	lr.w t0, (a3)
> +	add t1, t0, a2
> +	sc.w t1, t1, (a3)
> +	bnez t1, 1b
> +	mv a3, t0
> +#endif
>  	bnez a3, .Lsecondary_start
>  #endif /* CONFIG_RISCV_BOOT_SPINWAIT */
>  
> 
> -- 
> 2.43.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

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

On Thu, Nov 05, 2026 at 06:37:36PM +0200, Vladimir Kondratiev wrote:
> The A extension is Zaamo plus Zalrsc. On some CPUs Zaamo is present
> only formally. MIPS P8700 traps every AMO instruction into M-mode and
> emulates it there, so the extension is supported but very slow. A
> typical uncontended atomic increment costs:
> 
>   native AMO                                     2..5 cycles
>   LR/SC, private L1 reservation monitor         6..12 cycles
>   LR/SC, PoC (L2) reservation monitor       about 40 cycles
>   AMO emulated by an M-mode trap           about 600 cycles
> 
> The last two were measured on the eyeq7h hardware. On such a CPU the
> kernel must use LR/SC for every atomic.
> 
> Add RISCV_ISA_ZAAMO, default y. The default keeps "a" in -march and
> the generated code unchanged. When it is disabled, -march carries
> _zalrsc instead of "a", and atomic_*, bitops, xchg, futex and the
> early boot code use LR/SC loops. The sequence is chosen with the
> preprocessor, on CONFIG_RISCV_ISA_ZAAMO like the other ISA options in
> the headers, because the assembler rejects AMO mnemonics once -march
> lacks Zaamo. A kernel built this way runs on any hart that implements A; it just
> does not use AMO. It is therefore a plain option, not a NONPORTABLE
> one, and the hart keeps advertising "a" to userspace, which is true.
> 
> The choice is made at compile time rather than with alternatives.
> Nearly every CPU has a native A extension, and an alternative would
> inflate all those kernels with the nops needed to pad one AMO to the
> size of an LR/SC loop, at every atomic site, for the benefit of a few.
> 
> Turning Zaamo off needs a compiler that accepts _zaamo and _zalrsc in
> -march. GCC 15.1 is the first release that does.
> TOOLCHAIN_HAS_ZALRSC_ZAAMO checks for it; with an older compiler the
> option stays at y.
> 
> Add RISCV_ISA_ZALRSC as well, always y, because cmpxchg and the
> conditional atomics are LR/SC sequences that Zaamo cannot replace. It
> exists as the Kconfig counterpart of RISCV_ISA_EXT_ZALRSC in hwcap.h,
> added by commit 35173b666a16 ("riscv: add parsing for Zaamo and Zalrsc
> extensions"). The two sets are used together, for instance by
> rv_ext_enabled() in the BPF JIT, so an extension the kernel can be
> built for has both. RISCV_ISA_A, the counterpart of RISCV_ISA_EXT_A,
> follows the same rule and puts "a" in -march.
> 
> Three sets of extensions provide every atomic Linux needs. "a" is what
> the kernel supports today. Zalrsc plus Ziccrse is what this patch adds:
> every atomic is a constrained LR/SC loop, and Ziccrse is the
> forward-progress guarantee for such loops. Zaamo plus Zacas would do as
> well, with CAS loops in place of LR/SC as on x86. This patch leaves
> that option open: RISCV_ISA_ZALRSC is where it would go, by allowing
> "n" there and making amocas the base implementation.
> 
> Suggested-by: Chao-ying Fu <chao-ying.fu@globalfoundries.com>
> Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

NAK, on the basis laid out in
https://lore.kernel.org/linux-riscv/20261106-smock-headband-d6a7d568c228@spud/

On any future interations please include:
NAKed-by: Conor Dooley <conor.dooley@microchip.com>
Link: https://lore.kernel.org/linux-riscv/20261106-smock-headband-d6a7d568c228@spud/

> ---
>  arch/riscv/Kconfig               | 53 ++++++++++++++++++++++++++++++++++++++++
>  arch/riscv/Makefile              | 16 +++++++++---
>  arch/riscv/include/asm/atomic.h  | 52 ++++++++++++++++++++++++++++++++++++++-
>  arch/riscv/include/asm/bitops.h  | 47 +++++++++++++++++++++++++++++++++++
>  arch/riscv/include/asm/cmpxchg.h | 16 ++++++++++++
>  arch/riscv/include/asm/futex.h   | 45 ++++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/entry.S        |  7 ++++++
>  arch/riscv/kernel/head.S         | 12 +++++++++
>  8 files changed, 243 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 5874ca30f4f6..07d15a0bb379 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.
> +
> +	  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.

https://lore.kernel.org/linux-riscv/20261104-headlamp-improve-21863d9ca13e@spud/

I've also repeated asked for this to be deleted.

Thanks,
Conor.

> +
> +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 44f5a439741a..5cdc9cc017bb 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,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
> +
>  # 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 +93,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..3b189bea1d08 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.
>   */
> +#ifdef CONFIG_RISCV_ISA_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,23 @@ void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
>  		: "+A" (v->counter)					\
>  		: "r" (I)						\
>  		: "memory");						\
> -}									\
> +}
> +#else
> +#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");						\
> +}
> +#endif
>  
>  #ifdef CONFIG_GENERIC_ATOMIC64
>  #define ATOMIC_OPS(op, asm_op, I)					\
> @@ -84,6 +101,7 @@ ATOMIC_OPS(xor, xor,  i)
>   * There's two flavors of these: the arithmetic ops have both fetch and return
>   * versions, while the logical ops only have fetch versions.
>   */
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  #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,		\
> @@ -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
>  
>  #define ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_type, c_type, prefix)	\
>  static __always_inline							\
> diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
> index 3c1a15be54d8..ab3ffc3bfcfc 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;
>  }
>  
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 662e160b0522..883dc4aa1dc3 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -57,6 +57,7 @@
>  	}									\
>  })
>  
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  #define __arch_xchg(sfx, prepend, append, r, p, n)			\
>  ({									\
>  	__asm__ __volatile__ (						\
> @@ -67,6 +68,21 @@
>  		: "r" (n)						\
>  		: "memory");						\
>  })
> +#else
> +#define __arch_xchg(sfx, prepend, append, r, p, n)			\
> +({									\
> +	__typeof__(*(__ptr)) temp;					\
> +	__asm__ __volatile__ (						\
> +		prepend							\
> +		"1:	lr" sfx " %0, %1\n"				\
> +		"	sc" sfx " %2, %3, %1\n"				\
> +		"	bnez %2, 1b\n"					\
> +		append							\
> +		: "=&r" (r), "+A" (*(p)), "=&r" (temp)			\
> +		: "r" (n)						\
> +		: "memory");						\
> +})
> +#endif
>  
>  #define _arch_xchg(ptr, new, sc_sfx, swap_sfx, prepend,			\
>  		   sc_append, swap_append)				\
> diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
> index 6f3bc9bdb85e..58f12bf2af68 100644
> --- a/arch/riscv/include/asm/futex.h
> +++ b/arch/riscv/include/asm/futex.h
> @@ -19,6 +19,7 @@
>  #define __disable_user_access()		do { } while (0)
>  #endif
>  
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)	\
>  {								\
>  	__enable_user_access();					\
> @@ -32,6 +33,27 @@
>  	: "memory");						\
>  	__disable_user_access();				\
>  }
> +#else
> +#define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)	\
> +{								\
> +	int temp;						\
> +								\
> +	__enable_user_access();					\
> +	__asm__ __volatile__ (					\
> +	"1:	lr.w.aqrl %[ov], %[u]			\n"	\
> +	"	" insn "				\n"	\
> +	"2:	sc.w.aqrl %[t], %[t], %[u]		\n"	\
> +	"	bnez %[t], 1b				\n"	\
> +	"3:						\n"	\
> +	_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %[r])			\
> +	_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %[r])			\
> +	: [r] "+r" (ret), [ov] "=&r" (oldval),			\
> +	  [t] "=&r" (temp), [u] "+m" (*(uaddr))			\
> +	: [op] "Jr" (oparg)					\
> +	: "memory");						\
> +	__disable_user_access();				\
> +}
> +#endif
>  
>  static inline int
>  arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
> @@ -43,6 +65,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  	uaddr = untagged_addr(uaddr);
>  
>  	switch (op) {
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  	case FUTEX_OP_SET:
>  		__futex_atomic_op("amoswap.w.aqrl %[ov],%z[op],%[u]",
>  				  ret, oldval, uaddr, oparg);
> @@ -63,6 +86,28 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  		__futex_atomic_op("amoxor.w.aqrl %[ov],%z[op],%[u]",
>  				  ret, oldval, uaddr, oparg);
>  		break;
> +#else
> +	case FUTEX_OP_SET:
> +		__futex_atomic_op("mv %[t], %z[op]",
> +				  ret, oldval, uaddr, oparg);
> +		break;
> +	case FUTEX_OP_ADD:
> +		__futex_atomic_op("add %[t], %[ov], %z[op]",
> +				  ret, oldval, uaddr, oparg);
> +		break;
> +	case FUTEX_OP_OR:
> +		__futex_atomic_op("or %[t], %[ov], %z[op]",
> +				  ret, oldval, uaddr, oparg);
> +		break;
> +	case FUTEX_OP_ANDN:
> +		__futex_atomic_op("and %[t], %[ov], %z[op]",
> +				  ret, oldval, uaddr, ~oparg);
> +		break;
> +	case FUTEX_OP_XOR:
> +		__futex_atomic_op("xor %[t], %[ov], %z[op]",
> +				  ret, oldval, uaddr, oparg);
> +		break;
> +#endif
>  	default:
>  		ret = -ENOSYS;
>  	}
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 61b7a7716697..ba2db6804a6c 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -73,7 +73,14 @@
>  	beq	a2, zero, .Lnew_valid_map_cpus_restore_context
>  
>  	/* Atomically reset the current cpu bit in new_valid_map_cpus */
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  	amoxor.d	a0, a1, (a0)
> +#else
> +1:	lr.d	a2, (a0)
> +	xor	a2, a1, a2
> +	sc.d	a2, a2, (a0)
> +	bnez	a2, 1b
> +#endif
>  
>  	/*
>  	 * A sfence.vma is required here. Even if we had Svvptc, there's no
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index f6a8ca49e627..dc3862ed29c4 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -265,7 +265,19 @@ SYM_CODE_START(_start_kernel)
>  	/* Pick one hart to run the main boot sequence */
>  	la a3, hart_lottery
>  	li a2, 1
> +#ifdef CONFIG_RISCV_ISA_ZAAMO
>  	amoadd.w a3, a2, (a3)
> +#else
> +	/*
> +	 * hart_lottery is a small 32-bit counter. add is valid on RV32
> +	 * and RV64; sc.w stores the low 32 bits.
> +	 */
> +1:	lr.w t0, (a3)
> +	add t1, t0, a2
> +	sc.w t1, t1, (a3)
> +	bnez t1, 1b
> +	mv a3, t0
> +#endif
>  	bnez a3, .Lsecondary_start
>  #endif /* CONFIG_RISCV_BOOT_SPINWAIT */
>  
> 
> -- 
> 2.43.0
> 

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

[PATCH v5 2/3] riscv, bpf: emit LR/SC loops when Zaamo is disabled

[Vladimir Kondratiev]

A kernel built with CONFIG_RISCV_ISA_ZAAMO disabled is meant for a
hart where AMO instructions are emulated by an M-mode trap. The BPF JIT
still encodes amo* instructions for the BPF atomic read-modify-write
operations, so every such operation takes that trap.

Emit an LR/SC loop for BPF_ADD, BPF_AND, BPF_OR, BPF_XOR, their
BPF_FETCH forms and BPF_XCHG when Zaamo is disabled, on RV32 and RV64.
The sequences follow the Zalrsc variants in
arch/riscv/include/asm/atomic.h: a relaxed op discards the old value,
a fetch op uses .aqrl and returns the old value. BPF_CMPXCHG is
unchanged; it already uses amocas with Zacas and an LR/SC loop
otherwise.

The JIT encodes instructions as integers, so no assembler constraint
applies and both paths stay compiled. The choice is a plain
IS_ENABLED() test, the same compile-time decision the rest of the
kernel makes. The hart advertises "a" either way, so a runtime
extension check would not tell the two cases apart.

An arena (BPF_PROBE_ATOMIC) access needs an exception table entry for
each instruction that may fault. The verifier reserves one entry per
atomic instruction, but an LR/SC loop can fault on either the lr or the
sc. Count the extra entries once the instruction offsets converge,
before the image is allocated, and add an entry for the sc that shares
the fixup of the lr, so a fault skips the retry and the result move.

Co-developed-by: Chao-ying Fu <chao-ying.fu@globalfoundries.com>
Signed-off-by: Chao-ying Fu <chao-ying.fu@globalfoundries.com>
Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
---
 arch/riscv/net/bpf_jit.h        |   1 +
 arch/riscv/net/bpf_jit_comp32.c |  81 ++++++++++++++++++++++++++++--
 arch/riscv/net/bpf_jit_comp64.c | 108 +++++++++++++++++++++++++++++++++++++++-
 arch/riscv/net/bpf_jit_core.c   |  29 +++++++++++
 4 files changed, 213 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index 419b9d795f2a..8c1301289ea8 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -79,6 +79,7 @@ struct rv_jit_context {
 	int nexentries;
 	int ex_insn_off;
 	int ex_jmp_off;
+	int ex_sc_off;		/* sc in an lr/sc pair; 0 if unused */
 	unsigned long flags;
 	int stack_size;
 	int tcc_offset;
diff --git a/arch/riscv/net/bpf_jit_comp32.c b/arch/riscv/net/bpf_jit_comp32.c
index a9e0bd5cc81d..ea1779a23634 100644
--- a/arch/riscv/net/bpf_jit_comp32.c
+++ b/arch/riscv/net/bpf_jit_comp32.c
@@ -874,11 +874,67 @@ static int emit_load_r64(const s8 *dst, const s8 *src, s16 off,
 	return 0;
 }
 
-static int emit_bpf_atomic(s8 dst, const s8 *src, const s8 *rs,
-			   struct rv_jit_context *ctx,
-			   const struct bpf_insn *insn)
+/*
+ * Read-modify-write via lr/sc for kernels built without Zaamo. T0 holds
+ * the address. T1/T2 are not BPF registers; the address was copied out of
+ * lo(rd) before this is called, so clobbering T2 is fine.
+ */
+static int emit_bpf_atomic_lrsc(s8 dst, const s8 *rs, s32 imm,
+				struct rv_jit_context *ctx)
+{
+	bool is_fetch = (imm & BPF_FETCH) || (imm == BPF_XCHG);
+	bool xchg = imm == BPF_XCHG;
+	u8 aq = is_fetch ? 1 : 0;
+	u8 rl = is_fetch ? 1 : 0;
+	u8 old = RV_REG_T1;
+	u8 tmp = RV_REG_T2;
+	int lr_off, jmp_off;
+	s32 op = xchg ? BPF_XCHG : imm & ~BPF_FETCH;
+
+	switch (op) {
+	case BPF_ADD:
+	case BPF_AND:
+	case BPF_OR:
+	case BPF_XOR:
+	case BPF_XCHG:
+		break;
+	default:
+		return -1;
+	}
+
+	lr_off = ctx->ninsns;
+	emit(rv_lr_w(old, 0, dst, aq, rl), ctx);
+
+	switch (op) {
+	case BPF_ADD:
+		emit(rv_add(tmp, old, lo(rs)), ctx);
+		break;
+	case BPF_AND:
+		emit(rv_and(tmp, old, lo(rs)), ctx);
+		break;
+	case BPF_OR:
+		emit(rv_or(tmp, old, lo(rs)), ctx);
+		break;
+	case BPF_XOR:
+		emit(rv_xor(tmp, old, lo(rs)), ctx);
+		break;
+	}
+
+	if (xchg)
+		emit(rv_sc_w(tmp, lo(rs), dst, aq, rl), ctx);
+	else
+		emit(rv_sc_w(tmp, tmp, dst, aq, rl), ctx);
+
+	jmp_off = ninsns_rvoff(lr_off - ctx->ninsns);
+	emit(rv_bne(tmp, RV_REG_ZERO, jmp_off >> 1), ctx);
+	if (is_fetch)
+		emit(rv_addi(lo(rs), old, 0), ctx);
+	return 0;
+}
+
+static int emit_bpf_atomic_amo(s8 dst, const s8 *rs, s32 imm,
+			       struct rv_jit_context *ctx)
 {
-	s32 imm = insn->imm;
 	bool is_fetch = (imm & BPF_FETCH) || (imm == BPF_XCHG);
 	s8 fetch_reg = is_fetch ? lo(rs) : RV_REG_ZERO;
 	int aq = is_fetch ? 1 : 0;
@@ -907,6 +963,23 @@ static int emit_bpf_atomic(s8 dst, const s8 *src, const s8 *rs,
 	default:
 		return -1;
 	}
+	return 0;
+}
+
+static int emit_bpf_atomic(s8 dst, const s8 *src, const s8 *rs,
+			   struct rv_jit_context *ctx,
+			   const struct bpf_insn *insn)
+{
+	s32 imm = insn->imm;
+	bool is_fetch = (imm & BPF_FETCH) || (imm == BPF_XCHG);
+	int ret;
+
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZAAMO))
+		ret = emit_bpf_atomic_amo(dst, rs, imm, ctx);
+	else
+		ret = emit_bpf_atomic_lrsc(dst, rs, imm, ctx);
+	if (ret)
+		return ret;
 
 	if (is_fetch) {
 		emit(rv_addi(hi(rs), RV_REG_ZERO, 0), ctx);
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 01fe66774f02..94e4ddc6d965 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -573,6 +573,79 @@ static int emit_atomic_ld_st(u8 rd, u8 rs, const struct bpf_insn *insn,
 	return 0;
 }
 
+/*
+ * Read-modify-write via lr/sc for kernels built without Zaamo, matching
+ * the Zalrsc sequences in arch/riscv/include/asm/atomic.h. A relaxed op
+ * discards the old value. A fetch op (including XCHG) uses .aqrl and
+ * writes that value back to val. T2/T3 are not BPF registers; addr may
+ * already be T1.
+ */
+static void emit_atomic_lrsc(u8 addr, u8 val, bool is64, s32 imm,
+			     struct rv_jit_context *ctx)
+{
+	bool fetch = imm & BPF_FETCH;
+	u8 aq = fetch ? 1 : 0;
+	u8 rl = fetch ? 1 : 0;
+	u8 old = RV_REG_T2;
+	u8 tmp = RV_REG_T3;
+	int lr_off, jmp_off;
+	s32 op = imm == BPF_XCHG ? BPF_XCHG : imm & ~BPF_FETCH;
+
+	lr_off = ctx->ninsns;
+	ctx->ex_insn_off = ctx->ninsns;
+	emit(is64 ? rv_lr_d(old, 0, addr, aq, rl) :
+	     rv_lr_w(old, 0, addr, aq, rl), ctx);
+
+	if (op != BPF_XCHG) {
+		if (!is64 && op == BPF_ADD)
+			emit(rv_addw(tmp, old, val), ctx);
+		else if (op == BPF_ADD)
+			emit_add(tmp, old, val, ctx);
+		else if (op == BPF_AND)
+			emit_and(tmp, old, val, ctx);
+		else if (op == BPF_OR)
+			emit_or(tmp, old, val, ctx);
+		else if (op == BPF_XOR)
+			emit_xor(tmp, old, val, ctx);
+
+		ctx->ex_sc_off = ctx->ninsns;
+		emit(is64 ? rv_sc_d(tmp, tmp, addr, aq, rl) :
+		     rv_sc_w(tmp, tmp, addr, aq, rl), ctx);
+	} else {
+		ctx->ex_sc_off = ctx->ninsns;
+		emit(is64 ? rv_sc_d(tmp, val, addr, aq, rl) :
+		     rv_sc_w(tmp, val, addr, aq, rl), ctx);
+	}
+
+	/* Branch offset is relative to this instruction, back to the lr. */
+	jmp_off = ninsns_rvoff(lr_off - ctx->ninsns);
+	emit(rv_bne(tmp, RV_REG_ZERO, jmp_off >> 1), ctx);
+
+	if (fetch) {
+		emit_mv(val, old, ctx);
+		if (!is64)
+			emit_zextw(val, val, ctx);
+	}
+	ctx->ex_jmp_off = ctx->ninsns;
+}
+
+static bool is_atomic_lrsc_op(s32 imm)
+{
+	switch (imm) {
+	case BPF_ADD:
+	case BPF_AND:
+	case BPF_OR:
+	case BPF_XOR:
+	case BPF_ADD | BPF_FETCH:
+	case BPF_AND | BPF_FETCH:
+	case BPF_OR | BPF_FETCH:
+	case BPF_XOR | BPF_FETCH:
+	case BPF_XCHG:
+		return true;
+	}
+	return false;
+}
+
 static int emit_atomic_rmw(u8 rd, u8 rs, const struct bpf_insn *insn,
 			   struct rv_jit_context *ctx)
 {
@@ -601,6 +674,12 @@ static int emit_atomic_rmw(u8 rd, u8 rs, const struct bpf_insn *insn,
 		rd = RV_REG_T1;
 	}
 
+	/* Without Zaamo, every op except cmpxchg is an lr/sc loop. */
+	if (!IS_ENABLED(CONFIG_RISCV_ISA_ZAAMO) && is_atomic_lrsc_op(imm)) {
+		emit_atomic_lrsc(rd, rs, is64, imm, ctx);
+		return 0;
+	}
+
 	switch (imm) {
 	/* lock *(u32/u64 *)(dst_reg + off16) <op>= src_reg */
 	case BPF_ADD:
@@ -732,8 +811,8 @@ bool ex_handler_bpf(const struct exception_table_entry *ex,
 }
 
 /* For accesses to BTF pointers, add an entry to the exception table */
-static int add_exception_handler(const struct bpf_insn *insn, int dst_reg,
-				 struct rv_jit_context *ctx)
+static int add_exception_handler_one(const struct bpf_insn *insn, int dst_reg,
+				     struct rv_jit_context *ctx)
 {
 	struct exception_table_entry *ex;
 	unsigned long pc;
@@ -804,6 +883,31 @@ static int add_exception_handler(const struct bpf_insn *insn, int dst_reg,
 	return 0;
 }
 
+/*
+ * An lr/sc pair faults on either instruction. Both entries share the fixup
+ * that follows the loop, so a probe fault skips the retry and the result move.
+ */
+static int add_exception_handler(const struct bpf_insn *insn, int dst_reg,
+				 struct rv_jit_context *ctx)
+{
+	int sc_off = ctx->ex_sc_off;
+	int jmp_off = ctx->ex_jmp_off;
+	int ret;
+
+	ctx->ex_sc_off = 0;
+	/* Offset discovery runs before the image exists. Entries are added later. */
+	if (!ctx->insns)
+		return 0;
+
+	ret = add_exception_handler_one(insn, dst_reg, ctx);
+	if (ret || sc_off <= 0)
+		return ret;
+
+	ctx->ex_insn_off = sc_off;
+	ctx->ex_jmp_off = jmp_off;
+	return add_exception_handler_one(insn, dst_reg, ctx);
+}
+
 static int gen_jump_or_nops(void *target, void *ip, u32 *insns, bool is_call)
 {
 	s64 rvoff;
diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
index cbfcd287ea16..cba0d34c3e6f 100644
--- a/arch/riscv/net/bpf_jit_core.c
+++ b/arch/riscv/net/bpf_jit_core.c
@@ -16,6 +16,34 @@
 /* Number of iterations to try until offsets converge. */
 #define NR_JIT_ITERATIONS	32
 
+/*
+ * The verifier reserves one extable slot per BPF_PROBE_ATOMIC. Without
+ * Zaamo a read-modify-write atomic is an lr/sc loop that can fault on
+ * either instruction, so reserve a second slot for the sc.
+ */
+static void rv_jit_add_lrsc_exentries(struct bpf_prog *prog)
+{
+	const struct bpf_insn *insn = prog->insnsi;
+	int i;
+
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZAAMO) || __riscv_xlen != 64)
+		return;
+
+	for (i = 0; i < prog->len; i++, insn++) {
+		if (BPF_CLASS(insn->code) != BPF_STX ||
+		    BPF_MODE(insn->code) != BPF_PROBE_ATOMIC ||
+		    bpf_atomic_is_load_store(insn))
+			continue;
+		if (BPF_SIZE(insn->code) != BPF_W &&
+		    BPF_SIZE(insn->code) != BPF_DW)
+			continue;
+		/* amocas is a single faulting instruction. */
+		if (insn->imm == BPF_CMPXCHG && rv_ext_enabled(ZACAS))
+			continue;
+		prog->aux->num_exentries++;
+	}
+}
+
 static int build_body(struct rv_jit_context *ctx, bool extra_pass, int *offset)
 {
 	const struct bpf_prog *prog = ctx->prog;
@@ -100,6 +128,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 		if (ctx->ninsns == prev_ninsns) {
 			if (jit_data->header)
 				break;
+			rv_jit_add_lrsc_exentries(prog);
 			/* obtain the actual image size */
 			extable_size = prog->aux->num_exentries *
 				sizeof(struct exception_table_entry);

-- 
2.43.0

[PATCH v5 3/3] riscv: probes: reject probes inside an LR/SC sequence

[Vladimir Kondratiev]

A kprobe or uprobe replaces an instruction with a breakpoint. The trap
clears the hart's reservation, so if the breakpoint sits on the lr, on
the sc, or on any instruction between them, the sc fails, the loop
branches back to the lr and the probe fires again. The sequence never
completes.

This has always been possible on RISC-V, because cmpxchg and the
conditional atomics are LR/SC loops. Once Zaamo is disabled every
kernel atomic is such a loop, and so is every atomic in userspace built
for the same hart, so the chance of a probe landing in one grows
accordingly. arm64 has the same problem and rejects probes inside an
exclusive-load/store-exclusive region.

Teach the shared instruction decoder to reject lr and sc themselves.
This covers both kprobes and uprobes.

For kprobes extend arch_check_kprobe(), which already walks the
instructions from the enclosing symbol up to the probe address, to
track whether the address lies between an lr and the following sc, and
reject it if so. The walk goes forward, so compressed instructions do
not make it ambiguous.

uprobes have no symbol to start from and only the probed instruction
is copied. Use the rules for a constrained LR/SC sequence instead: at
most 16 instructions between lr and sc, all base integer instructions
other than loads, stores, JALR, FENCE, SYSTEM and backward jumps or
branches, or their compressed forms. Read the text that follows the
probe from the task, which the caller's mmap_lock allows, and walk
forward. Reaching an sc through permitted instructions only means the
probe sits inside a sequence; any other instruction, an lr, the end of
the window or unreadable text means it does not. Only a sequence that
follows these rules is guaranteed to make progress, so a probe inside
any other one is already on unsupported ground.

Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
---
 arch/riscv/include/asm/insn.h          | 28 ++++++++++
 arch/riscv/kernel/probes/decode-insn.c | 94 ++++++++++++++++++++++++++++++++++
 arch/riscv/kernel/probes/decode-insn.h |  7 +++
 arch/riscv/kernel/probes/kprobes.c     | 22 +++++++-
 arch/riscv/kernel/probes/uprobes.c     | 31 +++++++++++
 5 files changed, 180 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
index c3005573e8c9..f5de81753bec 100644
--- a/arch/riscv/include/asm/insn.h
+++ b/arch/riscv/include/asm/insn.h
@@ -10,11 +10,16 @@
 
 #define RV_INSN_FUNCT3_MASK	GENMASK(14, 12)
 #define RV_INSN_FUNCT3_OPOFF	12
+#define RV_INSN_FUNCT5_MASK	GENMASK(31, 27)
+#define RV_INSN_FUNCT5_OPOFF	27
+#define RV_INSN_FUNCT7_MASK	GENMASK(31, 25)
+#define RV_INSN_FUNCT7_OPOFF	25
 #define RV_INSN_OPCODE_MASK	GENMASK(6, 0)
 #define RV_INSN_OPCODE_OPOFF	0
 #define RV_INSN_FUNCT12_OPOFF	20
 
 #define RV_ENCODE_FUNCT3(f_)	(RVG_FUNCT3_##f_ << RV_INSN_FUNCT3_OPOFF)
+#define RV_ENCODE_FUNCT5(f_)	(RVG_FUNCT5_##f_ << RV_INSN_FUNCT5_OPOFF)
 #define RV_ENCODE_FUNCT12(f_)	(RVG_FUNCT12_##f_ << RV_INSN_FUNCT12_OPOFF)
 
 /* The bit field of immediate value in I-type instruction */
@@ -136,7 +141,13 @@
 
 /* parts of opcode for RVG*/
 #define RVG_OPCODE_FENCE	0x0f
+#define RVG_OPCODE_OP_IMM	0x13
 #define RVG_OPCODE_AUIPC	0x17
+#define RVG_OPCODE_OP_IMM_32	0x1b
+#define RVG_OPCODE_AMO		0x2f
+#define RVG_OPCODE_OP		0x33
+#define RVG_OPCODE_LUI		0x37
+#define RVG_OPCODE_OP_32	0x3b
 #define RVG_OPCODE_BRANCH	0x63
 #define RVG_OPCODE_JALR		0x67
 #define RVG_OPCODE_JAL		0x6f
@@ -188,6 +199,17 @@
 #define RVG_FUNCT12_EBREAK	0x1
 #define RVG_FUNCT12_SRET	0x102
 
+/* parts of funct5 code for A extension */
+#define RVG_FUNCT5_LR		0x02
+#define RVG_FUNCT5_SC		0x03
+
+/*
+ * The AMO funct3 field holds the access width: 010 for .w, 011 for .d.
+ * Matching bits 14:13 alone accepts both and nothing else.
+ */
+#define RVG_AMO_WIDTH_MASK	GENMASK(14, 13)
+#define RVG_AMO_WIDTH_WD	BIT(13)
+
 #define RVG_MATCH_AUIPC		(RVG_OPCODE_AUIPC)
 #define RVG_MATCH_JALR		(RV_ENCODE_FUNCT3(JALR) | RVG_OPCODE_JALR)
 #define RVG_MATCH_JAL		(RVG_OPCODE_JAL)
@@ -200,6 +222,8 @@
 #define RVG_MATCH_BGEU		(RV_ENCODE_FUNCT3(BGEU) | RVG_OPCODE_BRANCH)
 #define RVG_MATCH_EBREAK	(RV_ENCODE_FUNCT12(EBREAK) | RVG_OPCODE_SYSTEM)
 #define RVG_MATCH_SRET		(RV_ENCODE_FUNCT12(SRET) | RVG_OPCODE_SYSTEM)
+#define RVG_MATCH_LR		(RV_ENCODE_FUNCT5(LR) | RVG_AMO_WIDTH_WD | RVG_OPCODE_AMO)
+#define RVG_MATCH_SC		(RV_ENCODE_FUNCT5(SC) | RVG_AMO_WIDTH_WD | RVG_OPCODE_AMO)
 #define RVC_MATCH_C_BEQZ	(RVC_ENCODE_FUNCT3(C_BEQZ) | RVC_OPCODE_C1)
 #define RVC_MATCH_C_BNEZ	(RVC_ENCODE_FUNCT3(C_BNEZ) | RVC_OPCODE_C1)
 #define RVC_MATCH_C_J		(RVC_ENCODE_FUNCT3(C_J) | RVC_OPCODE_C1)
@@ -227,6 +251,8 @@
 #define RVC_MASK_C_EBREAK	0xffff
 #define RVG_MASK_EBREAK		0xffffffff
 #define RVG_MASK_SRET		0xffffffff
+#define RVG_MASK_LR		(RV_INSN_FUNCT5_MASK | RVG_AMO_WIDTH_MASK | RV_INSN_OPCODE_MASK)
+#define RVG_MASK_SC		(RV_INSN_FUNCT5_MASK | RVG_AMO_WIDTH_MASK | RV_INSN_OPCODE_MASK)
 
 #define __INSN_LENGTH_MASK	_UL(0x3)
 #define __INSN_LENGTH_GE_32	_UL(0x3)
@@ -262,6 +288,8 @@ __RISCV_INSN_FUNCS(c_ebreak, RVC_MASK_C_EBREAK, RVC_MATCH_C_EBREAK)
 __RISCV_INSN_FUNCS(ebreak, RVG_MASK_EBREAK, RVG_MATCH_EBREAK)
 __RISCV_INSN_FUNCS(sret, RVG_MASK_SRET, RVG_MATCH_SRET)
 __RISCV_INSN_FUNCS(fence, RVG_MASK_FENCE, RVG_MATCH_FENCE);
+__RISCV_INSN_FUNCS(lr, RVG_MASK_LR, RVG_MATCH_LR)
+__RISCV_INSN_FUNCS(sc, RVG_MASK_SC, RVG_MATCH_SC)
 
 /* special case to catch _any_ system instruction */
 static __always_inline bool riscv_insn_is_system(u32 code)
diff --git a/arch/riscv/kernel/probes/decode-insn.c b/arch/riscv/kernel/probes/decode-insn.c
index 433d9035b888..6a4f55b07d04 100644
--- a/arch/riscv/kernel/probes/decode-insn.c
+++ b/arch/riscv/kernel/probes/decode-insn.c
@@ -4,11 +4,99 @@
 #include <linux/kprobes.h>
 #include <linux/module.h>
 #include <linux/kallsyms.h>
+#include <linux/bug.h>
 #include <asm/sections.h>
 
 #include "decode-insn.h"
 #include "simulate-insn.h"
 
+/*
+ * Can insn sit between the lr and the sc of a constrained LR/SC sequence?
+ * The ISA allows base integer instructions there, except loads, stores,
+ * JALR, FENCE, SYSTEM, backward jumps and taken backward branches, plus
+ * their compressed forms. Backward branches are treated as not allowed:
+ * the retry branch follows the sc, so one inside the body is not a
+ * constrained sequence.
+ */
+static bool __kprobes riscv_insn_is_lrsc_body(u32 insn)
+{
+	u32 funct3;
+
+	if (GET_INSN_LENGTH(insn) == 2) {
+		switch (insn & RVC_INSN_OPCODE_MASK) {
+		case RVC_OPCODE_C0:
+			/* c.addi4spn; the rest of the quadrant is loads and stores */
+			return (insn & RVC_INSN_FUNCT3_MASK) == 0;
+		case RVC_OPCODE_C1:
+			/* Arithmetic, c.j, c.jal and branches, bit 12 is the offset sign */
+			if (riscv_insn_is_c_j(insn) || riscv_insn_is_c_jal(insn) ||
+			    riscv_insn_is_c_beqz(insn) || riscv_insn_is_c_bnez(insn))
+				return !(insn & BIT(12));
+			return true;
+		case RVC_OPCODE_C2:
+			funct3 = (insn & RVC_INSN_FUNCT3_MASK) >> RVC_INSN_FUNCT3_OPOFF;
+			if (funct3 == 0)	/* c.slli */
+				return true;
+			if (funct3 == 4)	/* c.mv, c.add, but not c.jr, c.jalr, c.ebreak */
+				return !riscv_insn_is_c_jr(insn) &&
+				       !riscv_insn_is_c_jalr(insn) &&
+				       !riscv_insn_is_c_ebreak(insn);
+			return false;	/* stack loads and stores */
+		}
+		return false;
+	}
+
+	switch (insn & RV_INSN_OPCODE_MASK) {
+	case RVG_OPCODE_OP_IMM:
+	case RVG_OPCODE_OP_IMM_32:
+	case RVG_OPCODE_LUI:
+	case RVG_OPCODE_AUIPC:
+		return true;
+	case RVG_OPCODE_OP:
+	case RVG_OPCODE_OP_32:
+		/* funct7 0 or 0x20 is base I; anything else is M, B or another extension */
+		return !((insn & RV_INSN_FUNCT7_MASK) & ~(0x20U << RV_INSN_FUNCT7_OPOFF));
+	case RVG_OPCODE_BRANCH:
+	case RVG_OPCODE_JAL:
+		/* forward only, bit 31 is the offset sign */
+		return !(insn & BIT(31));
+	}
+	return false;
+}
+
+/*
+ * Does the instruction at the start of buf lie inside a constrained LR/SC
+ * sequence? Such a sequence holds at most RISCV_LRSC_MAX_BODY instructions
+ * between the lr and the sc, all from the set above. Walk forward from the
+ * probed instruction: reaching an sc through permitted instructions only
+ * means the probe sits in the body; anything else, including an lr that
+ * opens a new sequence, means it does not. The walk stops at the end of
+ * buf, so a short buffer only makes the answer more permissive.
+ */
+bool __kprobes riscv_probe_in_lrsc_body(const u16 *buf, size_t len)
+{
+	size_t off = 0;
+	u32 insn;
+	int n;
+
+	for (n = 0; n <= RISCV_LRSC_MAX_BODY; n++) {
+		if (off + 2 > len)
+			return false;
+		insn = buf[off / 2];
+		if (GET_INSN_LENGTH(insn) == 4) {
+			if (off + 4 > len)
+				return false;
+			insn |= (u32)buf[off / 2 + 1] << 16;
+		}
+		if (riscv_insn_is_sc(insn))
+			return true;
+		if (!riscv_insn_is_lrsc_body(insn))
+			return false;
+		off += GET_INSN_LENGTH(insn);
+	}
+	return false;
+}
+
 /* Return:
  *   INSN_REJECTED     If instruction is one not allowed to kprobe,
  *   INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot.
@@ -23,6 +111,12 @@ riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *api)
 	 */
 	RISCV_INSN_REJECTED(system,		insn);
 	RISCV_INSN_REJECTED(fence,		insn);
+	/*
+	 * The breakpoint trap drops the reservation, so a probe on either
+	 * end of an LR/SC sequence makes the SC fail on every retry.
+	 */
+	RISCV_INSN_REJECTED(lr,			insn);
+	RISCV_INSN_REJECTED(sc,			insn);
 
 	/*
 	 * Simulate instructions list:
diff --git a/arch/riscv/kernel/probes/decode-insn.h b/arch/riscv/kernel/probes/decode-insn.h
index 42269a7d676d..aa1b0d363545 100644
--- a/arch/riscv/kernel/probes/decode-insn.h
+++ b/arch/riscv/kernel/probes/decode-insn.h
@@ -15,4 +15,11 @@ enum probe_insn {
 enum probe_insn __kprobes
 riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *asi);
 
+/* A constrained LR/SC sequence has at most this many instructions between lr and sc */
+#define RISCV_LRSC_MAX_BODY	16
+/* Bytes from the probed instruction through the farthest possible sc */
+#define RISCV_LRSC_SCAN_BYTES	((RISCV_LRSC_MAX_BODY + 1) * 4)
+
+bool __kprobes riscv_probe_in_lrsc_body(const u16 *buf, size_t len);
+
 #endif /* _RISCV_KERNEL_KPROBES_DECODE_INSN_H */
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 9e2afabf94e8..9c2284eafae9 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -12,6 +12,7 @@
 #include <asm/sections.h>
 #include <asm/cacheflush.h>
 #include <asm/bug.h>
+#include <asm/insn.h>
 #include <asm/text-patching.h>
 
 #include "decode-insn.h"
@@ -49,9 +50,17 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
 	post_kprobe_handler(p, kcb, regs);
 }
 
+/*
+ * Walk the instructions from the enclosing symbol up to addr. This checks
+ * that addr is an instruction boundary and that it does not lie inside an
+ * LR/SC sequence: the breakpoint trap drops the reservation, so the SC
+ * would fail and the loop would hit the probe again on every retry.
+ */
 static bool __kprobes arch_check_kprobe(unsigned long addr)
 {
 	unsigned long tmp, offset;
+	bool in_lrsc = false;
+	u32 insn;
 
 	/* start iterating at the closest preceding symbol */
 	if (!kallsyms_lookup_size_offset(addr, NULL, &offset))
@@ -61,9 +70,18 @@ static bool __kprobes arch_check_kprobe(unsigned long addr)
 
 	while (tmp <= addr) {
 		if (tmp == addr)
-			return true;
+			return !in_lrsc;
+
+		insn = *(u16 *)tmp;
+		if (GET_INSN_LENGTH(insn) == 4) {
+			insn |= (u32)*(u16 *)(tmp + 2) << 16;
+			if (riscv_insn_is_lr(insn))
+				in_lrsc = true;
+			else if (riscv_insn_is_sc(insn))
+				in_lrsc = false;
+		}
 
-		tmp += GET_INSN_LENGTH(*(u16 *)tmp);
+		tmp += GET_INSN_LENGTH(insn);
 	}
 
 	return false;
diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c
index eb177d0ce8ab..d97a10262166 100644
--- a/arch/riscv/kernel/probes/uprobes.c
+++ b/arch/riscv/kernel/probes/uprobes.c
@@ -28,6 +28,34 @@ unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
 	return instruction_pointer(regs);
 }
 
+/*
+ * Is the instruction at addr inside a constrained LR/SC sequence? Only the
+ * probed instruction was copied, so read the text that follows it from the
+ * task. The caller holds mmap_lock, as uprobe_write_opcode() requires too.
+ * Text that cannot be read ends the scan early, which accepts the probe.
+ */
+static bool uprobe_in_lrsc_body(struct mm_struct *mm, unsigned long addr)
+{
+	u16 buf[RISCV_LRSC_SCAN_BYTES / sizeof(u16)];
+	struct vm_area_struct *vma;
+	struct page *page;
+	size_t got = 0;
+	int len;
+
+	while (got < sizeof(buf)) {
+		len = min_t(size_t, sizeof(buf) - got,
+			    PAGE_SIZE - offset_in_page(addr + got));
+		page = get_user_page_vma_remote(mm, addr + got, FOLL_FORCE, &vma);
+		if (IS_ERR(page))
+			break;
+		uprobe_copy_from_page(page, addr + got, (void *)buf + got, len);
+		put_page(page);
+		got += len;
+	}
+
+	return riscv_probe_in_lrsc_body(buf, got);
+}
+
 int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 			     unsigned long addr)
 {
@@ -53,6 +81,9 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 		return -EINVAL;
 	}
 
+	if (uprobe_in_lrsc_body(mm, addr))
+		return -EINVAL;
+
 	return 0;
 }
 

-- 
2.43.0

Re: [PATCH v5 3/3] riscv: probes: reject probes inside an LR/SC sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] riscv: probes: arch_check_kprobe() falsely rejects probes after __cmpwait()
--

commit 4dd5cb2e2ba32786ac861a461730851e4daa6b77
Author: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

riscv: probes: reject probes inside an LR/SC sequence

This commit updates the RISC-V kprobes and uprobes instruction decoders and
validators to reject probes placed on or between load-reserved (lr) and
store-conditional (sc) instructions, preventing infinite retry loops caused
by breakpoint traps clearing the hart's reservation.

> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index 9e2afabf94e80..9c2284eafae90 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
[ ... ]
> @@ -61,9 +70,18 @@ static bool __kprobes arch_check_kprobe(unsigned long addr)
>  
>  	while (tmp <= addr) {
>  		if (tmp == addr)
> -			return true;
> +			return !in_lrsc;
> +
> +		insn = *(u16 *)tmp;
> +		if (GET_INSN_LENGTH(insn) == 4) {
> +			insn |= (u32)*(u16 *)(tmp + 2) << 16;
> +			if (riscv_insn_is_lr(insn))
> +				in_lrsc = true;
> +			else if (riscv_insn_is_sc(insn))
> +				in_lrsc = false;
> +		}
>  
> -		tmp += GET_INSN_LENGTH(*(u16 *)tmp);
> +		tmp += GET_INSN_LENGTH(insn);
>  	}

[Severity: Medium]
Can this leave in_lrsc stuck at true for the rest of a function after an
inlined __cmpwait() sequence?

In arch_check_kprobe(), in_lrsc is set to true whenever riscv_insn_is_lr()
matches an lr.w or lr.d instruction, and is only cleared when
riscv_insn_is_sc() matches an sc.w or sc.d instruction.

However, when CONFIG_RISCV_ISA_ZAWRS is enabled (which is default y),
__cmpwait() in arch/riscv/include/asm/cmpxchg.h uses lr.w or lr.d paired with
wrs.nto and no sc instruction to implement smp_cond_load_relaxed(),
smp_cond_load_acquire(), and atomic_cond_read_*():

	case 4:
		asm volatile(
		"	lr.w	%0, %1\n"
		"	xor	%0, %0, %2\n"
		"	bnez	%0, 1f\n"
			ZAWRS_WRS_NTO "\n"
		"1:"
		: "=&r" (tmp), "+A" (*(u32 *)ptr)
		: "r" (val));
		break;

When register_kprobe() -> arch_prepare_kprobe() -> arch_check_kprobe() walks
past an inlined __cmpwait() in functions such as try_to_wake_up(),
queued_spin_lock_slowpath(), or osq_lock(), won't in_lrsc remain true after
the wrs.nto and cause arch_check_kprobe() to falsely reject kprobes on all
subsequent instructions in that function (unless an unrelated sc happens to
appear before addr)?

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