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

[Vladimir Kondratiev <Vladimir.Kondratiev@mobileye.com>]

Some of issues already fixed on v4.
- Link to v4:
https://patch.msgid.link/20261101-lrsc-only-v4-0-4e1d010463b3@mobileye.com

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

I fixed these lkp reported bugs in v4 by TOOLCHAIN_HAS_ZALRSC that test
compiler support of _zalrsc in -march

>> +config RISCV_ISA_ZAAMO
>> +	bool "Zaamo extension support for AMO atomics" if NONPORTABLE
>> +	default y
>> +	help
>> +	  Use AMO instructions for kernel atomic memory operations. When
>> +	  disabled, those operations are built from LR/SC and the kernel
>> +	  is compiled with zalrsc instead of the A extension.
>> +
>> +	  Disable this only for a hart that implements Zalrsc but not
>> +	  Zaamo, such as the MIPS P8700. The resulting kernel cannot run
>> +	  on a hart without LR/SC.
> 
> The text here doesn't really make much sense, given it is default
> disabled as it depends on NONPORTABLE that is default disabled.
> If this is retained, it should be default n and the text reworded.

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

>> +
>> +	  If you don't know what to do here, say Y.
>> +
>> +config RISCV_ISA_ZALRSC
>> +	def_bool y
>> +	help
>> +	  Zalrsc provides LR/SC. cmpxchg and the conditional atomics
>> +	  require it, so it is always enabled.
> 
> So why have a kconfig option for it then? It's pointless if always
> enabled.

My logic when adding it:
There are 2 sets of extensions related defines: one is Kconfig ones,
CONFIG_RISCV_ISA_<ext>; and another one found in the
arch/riscv/include/asm/hwcap.h, RISCV_ISA_EXT_<ext>

In the hwcap.h, there's already

#define RISCV_ISA_EXT_ZAAMO		97
#define RISCV_ISA_EXT_ZALRSC		98

added by commit 35173b666a16 (riscv: add parsing for Zaamo and Zalrsc
extensions, 2024-06-19)

These 2 sets are logically coupled, and used in the BPF JUT code like

#define rv_ext_enabled(ext) \
	(IS_ENABLED(CONFIG_RISCV_ISA_##ext) &&
riscv_has_extension_likely(RISCV_ISA_EXT_##ext))

So I thought if RISCV_ISA_EXT_ZALRSC exists although it is mandatory,
CONFIG_RISCV_ISA_ZALRSC counterpart should exist for the same reason.

Do you suggest this is different case and it is worth to remove
CONFIG_RISCV_ISA_ZALRSC anyway?

>> +
>> +config RISCV_ISA_A
>> +	def_bool RISCV_ISA_ZAAMO && RISCV_ISA_ZALRSC
> 
> I think this all needs a redo...
> 
>> diff --git a/arch/riscv/include/asm/atomic.h b/arch/riscv/include/asm/atomic.h
>> index 616b8b332ac5..dc6a8c95c975 100644
>> --- a/arch/riscv/include/asm/atomic.h
>> +++ b/arch/riscv/include/asm/atomic.h
>> @@ -50,6 +50,7 @@ static __always_inline void arch_atomic64_set(atomic64_t *v, s64 i)
>>   * have the AQ or RL bits set.  These don't return anything, so there's only
>>   * one version to worry about.
>>   */
>> +#if defined(__riscv_atomic) || defined(__riscv_zaamo)
>>  #define ATOMIC_OP(op, asm_op, I, asm_type, c_type, prefix)		\
>>  static __always_inline							\
>>  void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
>> @@ -59,7 +60,25 @@ void arch_atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)	\
>>  		: "+A" (v->counter)					\
>>  		: "r" (I)						\
>>  		: "memory");						\
>> -}									\
>> +}
>> +#elif defined(__riscv_zalrsc)
> 
> ...why are we making this a compiletime decision in the first place?
> Why can't this be an alternative, starting with the zalrsc stuff and
> moving to the better implementation if available?

I started with alternatives approach, see
https://lore.kernel.org/all/20260114072552.731974-1-vladimir.kondratiev@mobileye.com/
Then I converted it to the static compile-time decision following input
from Gary Guo that I think is valid, here:
https://lore.kernel.org/all/DFSQIY6WV4U7.3B3RY31KE739B@garyguo.net/

> Is there some zaamo code that gets generated automatically that we lose
> out on by always building the kernel with zalrsc instead of with all of a?

There's BPF JIT that emits AMO instructions, patch for this is not ready
yet. These AMO instructions will be emulated and work correctly but very
slowly.

>> -KBUILD_BASE_ISA = -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)fd([^v_]*)v?/\1\2/')
>> +KBUILD_BASE_ISA = -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima?|rv64ima?)fd([^v_]*)v?/\1\2/')
>>  export KBUILD_BASE_ISA
>>  
>> +# if "A" not supported, toolchain shall support zaamo/zalrsc
> 
> Obviously this is not true, disabling some Kconfig option has no bearing
> on what a toolchain is capable of. You cannot put _foo in here without
> checking if the toolchain actually supports it.
> If no zaamo instructions are automatically generated, this dance isn't
> needed, so my suggestions for how to proceed depend on the answer to
> that question.

Fixed in v4

Thanks, Vladimir