Re: [PATCH v4 2/2] riscv: accept the atomic extensions this kernel was built with

[Conor Dooley <conor@kernel.org>]

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

On Sun, Nov 01, 2026 at 02:51:40PM +0200, Vladimir Kondratiev wrote:
> riscv_early_of_processor_hartid() rejects a hart unless its
> riscv,isa-extensions property contains "a". That rejects a Zalrsc-only
> hart even when the kernel was built to use only LR/SC.
> 
> Require "a" or "zaamo" when the kernel emits AMO, and "a" or "zalrsc"
> when it emits LR/SC. A normal build defines both toolchain macros and
> still requires the full A extension. The legacy "riscv,isa" fallback
> keeps its rv{32,64}ima check.
> 
> Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> ---
>  arch/riscv/kernel/cpu.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 3dbc8cc557dd..5d1e39d16e59 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -82,12 +82,32 @@ int __init riscv_early_of_processor_hartid(struct device_node *node, unsigned lo
>  		return -ENODEV;
>  
>  	if (of_property_match_string(node, "riscv,isa-extensions", "i") < 0 ||
> -	    of_property_match_string(node, "riscv,isa-extensions", "m") < 0 ||
> -	    of_property_match_string(node, "riscv,isa-extensions", "a") < 0) {
> -		pr_warn("CPU with hartid=%lu does not support ima", *hart);
> +	    of_property_match_string(node, "riscv,isa-extensions", "m") < 0) {
> +		pr_warn("CPU with hartid=%lu does not support im", *hart);
>  		return -ENODEV;
>  	}
> -
> +#if !defined(__riscv_atomic) && !defined(__riscv_zaamo) && !defined(__riscv_zalrsc)
> +#error "Need AMO or LR/SC atomics"
> +#endif
> +	/*
> +	 * Require each atomic form this kernel emits. The single-letter
> +	 * "a" extension satisfies either check. A Zalrsc-only build defines
> +	 * __riscv_zalrsc and checks that extension only.
> +	 */
> +#if defined(__riscv_atomic) || defined(__riscv_zaamo)
> +	if (of_property_match_string(node, "riscv,isa-extensions", "a") < 0 &&
> +	    of_property_match_string(node, "riscv,isa-extensions", "zaamo") < 0) {
> +		pr_warn("CPU with hartid=%lu does not support AMO atomics", *hart);
> +		return -ENODEV;
> +	}
> +#endif
> +#if defined(__riscv_atomic) || defined(__riscv_zalrsc)
> +	if (of_property_match_string(node, "riscv,isa-extensions", "a") < 0 &&
> +	    of_property_match_string(node, "riscv,isa-extensions", "zalrsc") < 0) {
> +		pr_warn("CPU with hartid=%lu does not support LR/SC atomics", *hart);
> +		return -ENODEV;
> +	}
> +#endif
>  	return 0;

I really hate this in particular. Please make use of IS_ENABLED() to
simplify this and enable writing it without #if stuff.

It's effectively just:

	if (IS_ENABLED(RISCV_ISA_A) && of_property_match_string(node, "riscv,isa-extensions", "a") < 0 &&
	    of_property_match_string(node, "riscv,isa-extensions", "zaamo") < 0) {
		pr_warn("CPU with hartid=%lu does not support AMO atomics", *hart);
		return -ENODEV;
	}

	if (of_property_match_string(node, "riscv,isa-extensions", "a") < 0 &&
	    of_property_match_string(node, "riscv,isa-extensions", "zalrsc") < 0) {
		pr_warn("CPU with hartid=%lu does not support LR/SC atomics", *hart);
		return -ENODEV;
	}

but I would probably opt to write this as:

	/*
	 * The hart must support lr/sc in all configurations, and AMOs if the
	 * kernel was built with the full A extension.
	 */
	if (of_property_match_string(node, "riscv,isa-extensions", "a") >= 0)
		return 0;

	if (of_property_match_string(node, "riscv,isa-extensions", "zalrsc") < 0) {
		pr_warn("CPU with hartid=%lu does not support LR/SC atomics", *hart);
		return -ENODEV;
	}

	if (IS_ENABLED(RISCV_ISA_A) && of_property_match_string(node, "riscv,isa-extensions", "zaamo") < 0) {
		pr_warn("CPU with hartid=%lu does not support AMO atomics", *hart);
		return -ENODEV;
	}

That works, right?

That got me thinking though, I feel like this breaks some of the
extension parsing logic in other ways.

If you putting zalrsc and zaamo in your devicetree, we never do any
backwards processing to set the a extension (we don't do this for any
extensions by design, we only populate "child" extensions based on their
"parent"). If there's some userspace thing that expects to see a in
cpufeature or w/e it'd break. There shouldn't be since a has always been
supported but who knows. You need custom userspace anyway so I honestly
don't give that much of a shit about this going wrong.

What does "worry" me is hwprobe. Currently you've not modified hwprobe
at all to correctly state that you do not support
RISCV_HWPROBE_BASE_BEHAVIOR_IMA. (and brings up the question of whether
you support RISCV_HWPROBE_BASE_BEHAVIOR_IMA_FD etc!)

Maybe, again you don't care cos of your custom userspace, but this needs
to be holistic if it is being done at all.
Personally, I don't want to lower the minimum requirements - the people
designing this stuff to run linux should have checked to see what our
requirements were (and have always been...).

Thanks,
Conor.


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