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

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

On 11/3/26 23:09, Conor Dooley wrote:
>> +config TOOLCHAIN_HAS_ZALRSC
>> +	bool
>> +	default y
>> +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64im_zalrsc)
>> +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32im_zalrsc)
>> +
>> +config RISCV_ISA_ZAAMO
>> +	bool "Zaamo extension support for AMO atomics" if NONPORTABLE && TOOLCHAIN_HAS_ZALRSC
> 
> Probably this option should be called something like
> TOOLCHAIN_HAS_ZALRSC_ZAAMO if you're going to use it like this, because
> it is confusing why ZALRSC gates a ZAAMO option.
> 
> I'm curious actually, does every toolchain provide zalrsc and zaamo as a
> pair, or should you check for both here? I'd be surprised if there was a
> toolchain with one but not the other but worth checking to avoid some
> bizarre toolchain bug..

Agree, will be in next version.

> Ah, right. But that kinda goes against how NONPORTABLE is meant to work
> since the default cannot run on all platforms? At that point, just get
> rid of NONPORTABLE?

Agree, dropping NONPORTABLE

> | My logic when adding it:
> | There are 2 sets of extensions related defines: one is Kconfig ones,
> | CONFIG_RISCV_ISA_<ext>; and another one found in the
> | arch/riscv/include/asm/hwcap.h, RISCV_ISA_EXT_<ext>
> | 
> | In the hwcap.h, there's already
> | 
> | #define RISCV_ISA_EXT_ZAAMO		97
> | #define RISCV_ISA_EXT_ZALRSC		98
> | 
> | added by commit 35173b666a16 (riscv: add parsing for Zaamo and Zalrsc
> | extensions, 2024-06-19)
> | 
> | These 2 sets are logically coupled, and used in the BPF JUT code like
> | 
> | #define rv_ext_enabled(ext) \
> | 	(IS_ENABLED(CONFIG_RISCV_ISA_##ext) &&
> | riscv_has_extension_likely(RISCV_ISA_EXT_##ext))
> | 
> | So I thought if RISCV_ISA_EXT_ZALRSC exists although it is mandatory,
> | CONFIG_RISCV_ISA_ZALRSC counterpart should exist for the same reason.
> | 
> | Do you suggest this is different case and it is worth to remove
> | CONFIG_RISCV_ISA_ZALRSC anyway?
> 
> If this is the justification for it, then you need to provide this
> justification in your commit message - but if zalrsc is always enabled
> then there's no need for any code like that in the bpf jit in the first
> place, once more making this dead code that should be deleted?

Adding justification. Also, it is possible to have zaamo+zacas and not
support zalrsc; in this case CONFIG_RISCV_ISA_ZALRSC will be "n"; adding
clause explaining this theoretical option.

>> +# Zaamo is off only when the compiler accepts _zalrsc in -march.
> 
> I think the comment here is not quite right, it should be "can only be
> off", no?

Sure, fixed

>> +ifneq ($(CONFIG_RISCV_ISA_A),y)
>> +riscv-march-$(CONFIG_RISCV_ISA_ZAAMO) := $(riscv-march-y)_zaamo
> 
> Isn't this dead code? The logic for RISCV_ISA_A is:
> config RISCV_ISA_A
> 	def_bool RISCV_ISA_ZAAMO && RISCV_ISA_ZALRSC
> but ZALRSC is always set, so RISCV_ISA_A is equivalent to
> RISCV_ISA_ZAAMO so there can't be a case where RISCV_ISA_A is not set
> but RISCV_ISA_ZAAMO is?
> 
>> +riscv-march-$(CONFIG_RISCV_ISA_ZALRSC) := $(riscv-march-y)_zalrsc
> 
> And here, this is effectively just riscv-march-y, since if you get as
> far as here, it must be true.

See above, in theory one can have zaamo+zacas but not 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
> | Suspicious Link - Removed
> | Then I converted it to the static compile-time decision following input
> | from Gary Guo that I think is valid, here:
> | Suspicious Link - Removed
> 
> 
> Okay, that logic from Gary does make some sense. I'll accept that
> argument, but again this needs to be communicated in the commit message.

Done

> | There's BPF JIT that emits AMO instructions, patch for this is not ready
> | yet. These AMO instructions will be emulated and work correctly but very
> | slowly.
> 
> I think that needs to be part of this series. You're far too focused on
> your individual platform where the instructions work, rather than the
> holistic solution of lowering our minimum requirements so that platforms
> that do not have any AMO support work. The only way that it'd make sense
> to move forward with this without the bpf jit patch is if you disabled
> bpf entirely when RISCV_ISA_ZAAMO is disabled.

Added patch for BPF JIT.

Also, added is patch for kprobes/uprobes, to reject probes inside lr/sc
sequence. This is needed anyway, having lr/sc blocks for any atomics
only increase chances but we do have lr/sc blocks in code anyway and
need to protect it.

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

Agree, replacing with #ifdef CONFIG_RISCV_ISA_ZAAMO>> +#else
>> +#error "Need AMO or LR/SC atomics"
> 
> And this error is actually unreachable?
> 
> (and so on in every other place that this pattern is repeated)

Agree, removed
Thanks, Vladimir