Re: [PATCH v6 6/6] riscv: hwprobe: report whether AMO is faster than LR/SC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] riscv: hwprobe: stale vDSO cache for AMO_PERF in
  check_amo_speed_all_cpus()
- [Low] riscv: hwprobe: set_amo_speed_param() returns -EINVAL in
  __setup() handler
- [Low] riscv: hwprobe: amo_speed= override skips Zaamo check in
  check_amo_speed_all_cpus()
--

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

riscv: hwprobe: report whether AMO is faster than LR/SC

This commit measures the relative speed of uncontended AMO instructions
versus LR/SC loops on each CPU at boot and exposes the result to userspace
via the RISCV_HWPROBE_KEY_AMO_PERF hwprobe key.

> diff --git a/arch/riscv/kernel/amo_speed.c b/arch/riscv/kernel/amo_speed.c
> new file mode 100644
> index 0000000000000..933891b69db62
> --- /dev/null
> +++ b/arch/riscv/kernel/amo_speed.c

[ ... ]

> +static int amo_speed_online_cpu(unsigned int cpu)
> +{
> +	if (per_cpu(amo_speed, cpu) != RISCV_HWPROBE_AMO_UNKNOWN)
> +		return 0;
> +
> +	if (amo_speed_param != RISCV_HWPROBE_AMO_UNKNOWN)
> +		per_cpu(amo_speed, cpu) = amo_speed_param;
> +	else
> +		check_amo_speed(NULL);
> +
> +	return 0;
> +}
> +
> +static int __init set_amo_speed_param(char *str)
> +{
> +	if (!strcmp(str, "slow"))
> +		amo_speed_param = RISCV_HWPROBE_AMO_SLOW;
> +	else if (!strcmp(str, "fast"))
> +		amo_speed_param = RISCV_HWPROBE_AMO_FAST;
> +	else
> +		return -EINVAL;
> +
> +	return 1;
> +}
> +__setup("amo_speed=", set_amo_speed_param);

[Severity: Low]
Does returning -EINVAL from a __setup() callback cause invalid amo_speed=
values to be treated as handled?  obsolete_checksetup() treats any
non-zero return value from setup_func() as success, so an unrecognized
argument is silently accepted instead of being reported as an unknown
boot option.  __setup() handlers return 0 on failure.

> +static int __init check_amo_speed_all_cpus(void)
> +{
> +	int cpu;
> +
> +	if (amo_speed_param != RISCV_HWPROBE_AMO_UNKNOWN) {
> +		pr_info("AMO speed set to '%s' (%lu) by command line\n",
> +			amo_speed_param == RISCV_HWPROBE_AMO_FAST ? "fast" : "slow",
> +			amo_speed_param);
> +		for_each_online_cpu(cpu)
> +			per_cpu(amo_speed, cpu) = amo_speed_param;

[Severity: Low]
Does the amo_speed= command-line override bypass the Zaamo extension check
in check_amo_speed_all_cpus() and amo_speed_online_cpu()?  If amo_speed=
is passed on a CPU without RISCV_ISA_EXT_ZAAMO, per_cpu(amo_speed, cpu) is
set to RISCV_HWPROBE_AMO_FAST or RISCV_HWPROBE_AMO_SLOW instead of
RISCV_HWPROBE_AMO_UNSUPPORTED, reporting to userspace that AMO
instructions are usable on hardware where they trap as illegal
instructions.

> +	} else {
> +		on_each_cpu(check_amo_speed, NULL, 1);
> +	}
> +
> +	cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "riscv:amo_speed",
> +				  amo_speed_online_cpu, NULL);
> +
> +	return 0;
> +}
> +
> +/*
> + * Same level as the unaligned access probe: after clocksource_done_booting()
> + * so that amo_speed_measure() uses a stable clocksource, but before
> + * rootfs_initcall() enables usermode helpers that can reach hwprobe.
> + */
> +fs_initcall_sync(check_amo_speed_all_cpus);

[Severity: High]
Does running check_amo_speed_all_cpus() at fs_initcall_sync leave the vDSO
hwprobe table with a stale RISCV_HWPROBE_AMO_UNKNOWN value for
RISCV_HWPROBE_KEY_AMO_PERF?

init_hwprobe_vdso_data() populates avd->all_cpu_hwprobe_values[] earlier
at arch_initcall_sync:

arch/riscv/kernel/sys_hwprobe.c:init_hwprobe_vdso_data() {
    ...
	for (key = 0; key <= RISCV_HWPROBE_MAX_KEY; key++) {
		pair.key = key;
		hwprobe_one_pair(&pair, cpu_online_mask);

		WARN_ON_ONCE(pair.key < 0);

		avd->all_cpu_hwprobe_values[key] = pair.value;
    ...
}

Because per_cpu(amo_speed, cpu) is still RISCV_HWPROBE_AMO_UNKNOWN when
init_hwprobe_vdso_data() runs, and check_amo_speed_all_cpus() does not
update vdso_k_arch_data->all_cpu_hwprobe_values[RISCV_HWPROBE_KEY_AMO_PERF]
after probing, userspace queries routed through __vdso_riscv_hwprobe()
always receive RISCV_HWPROBE_AMO_UNKNOWN.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-lrsc-only-v6-0-bafc3cc0fcae@mobileye.com?part=6