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

[Conor Dooley <conor@kernel.org>]

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

On Wed, Nov 04, 2026 at 01:41:44PM +0200, Vladimir Kondratiev wrote:
> 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.

Key word, theoretical. We do not support zaamo + zacas only systems.
Please do not add "code" that a) has no actual user and b) would only be
useful if we supported something that we do not support.

Thanks,
Conor.

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

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