Re: [PATCH RFC v6 03/18] riscv: add support for srmcfg CSR from Ssqosid extension

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] CPU hotplug cache invalidation in riscv_srmcfg_offline() is overwritten before the CPU halts.
- [High] PM exit notifier invalidates software cache but fails to eagerly restore the hardware CSR.
- [Medium] Failure in cpuhp_setup_state() skips PM notifier registration, silently breaking suspend/resume QoS tracking.
--

commit bc4841378eac220040a12e4f7c694dde12285eb1
Author: Drew Fustini <fustini@kernel.org>

riscv: add support for srmcfg CSR from Ssqosid extension

This patch adds support for the srmcfg CSR defined in the Ssqosid ISA
extension, tracking resource allocation (RCID) and usage (MCID). It adds
a per-task thread_struct field and per-CPU cached variables to update the
CSR during context switches.

Link: https://github.com/riscv/riscv-ssqosid/releases/tag/v1.0

[ ... ]

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

[ ... ]

> +/*
> + * Invalidate the cache on offline too. The sentinel persists across the
> + * offline period, so a CPU brought back online finds the cache already
> + * invalidated before it is schedulable. This closes the window where a
> + * task scheduled before riscv_srmcfg_online() runs could match a stale
> + * cache and skip the CSR write while the hardware CSR was reset across
> + * hart stop/start.
> + */
> +static int riscv_srmcfg_offline(unsigned int cpu)
> +{
> +	per_cpu(cpu_srmcfg, cpu) = U32_MAX;

[Severity: High]
Will this U32_MAX sentinel be overwritten before the CPU actually halts?

Because riscv_srmcfg_offline() executes at CPUHP_AP_ONLINE_DYN, the dying CPU
continues tearing down and eventually context switches to the idle thread to
physically offline.

This context switch invokes __switch_to_srmcfg(), which writes the hardware
CSR and overwrites the cpu_srmcfg cache with the idle task's value,
permanently losing the U32_MAX sentinel.

When the CPU comes back online, its hardware CSR is in an implementation
defined reset state, but the software cache holds a valid value. Any early
tasks sharing this stale value might skip the CSR write and execute with
incorrect QoS tags until riscv_srmcfg_online() is reached.

> +	return 0;
> +}
> +
> +/*
> + * CPU PM notifier: invalidate the cached srmcfg on resume from a deep
> + * idle / suspend. Ssqosid v1.0 leaves CSR_SRMCFG state across low-power
> + * transitions implementation-defined, and the boot CPU never goes
> + * through the cpuhp online callback during system suspend, so without
> + * this hook __switch_to_srmcfg() would skip the CSR write when the
> + * outgoing task happens to share its srmcfg with the pre-suspend cache.
> + */
> +static int riscv_srmcfg_pm_notify(struct notifier_block *nb,
> +				  unsigned long action, void *unused)
> +{
> +	switch (action) {
> +	case CPU_PM_EXIT:
> +	case CPU_PM_ENTER_FAILED:
> +		__this_cpu_write(cpu_srmcfg, U32_MAX);

[Severity: High]
Does this notifier need to eagerly restore the hardware CSR for the currently
executing thread?

It sets cpu_srmcfg to U32_MAX to force a CSR write on the next context
switch. However, upon waking from low-power states like suspend-to-RAM, the
CPU does not immediately context switch.

It unwinds the suspend stack, resumes executing the interrupted thread, and
services pending interrupts. 

Because the hardware CSR is not explicitly restored here, all code executing
between wakeup and the first context switch might run with the hardware's
implementation defined reset value for CSR_SRMCFG instead of the task's
assigned QoS tags.

> +		break;
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block riscv_srmcfg_pm_nb = {
> +	.notifier_call = riscv_srmcfg_pm_notify,
> +};
> +
> +static int __init riscv_srmcfg_init(void)
> +{
> +	int err;
> +
> +	if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SSQOSID))
> +		return 0;
> +
> +	/*
> +	 * cpuhp_setup_state() invokes the startup callback locally on every
> +	 * already-online CPU, so no separate seed loop is needed here.
> +	 */
> +	err = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "riscv/srmcfg:online",
> +				riscv_srmcfg_online, riscv_srmcfg_offline);
> +	if (err < 0) {
> +		pr_warn("srmcfg cpuhp registration failed (%d), cpus brought online after boot will not invalidate the CSR_SRMCFG cache\n",
> +			err);
> +		return err;

[Severity: Medium]
Is it intentional to skip PM notifier registration if CPU hotplug setup fails?

This early return skips cpu_pm_register_notifier() entirely. 

While cpuhp_setup_state() failures are rare, PM notifiers do not strictly
depend on CPU hotplug registration. If this error path is taken, tasks waking
from suspend could run with incorrect QoS tags because the PM notifier is never
registered.

> +	}
> +
> +	cpu_pm_register_notifier(&riscv_srmcfg_pm_nb);
> +	return 0;
> +}
> +arch_initcall(riscv_srmcfg_init);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-ssqosid-cbqri-rqsc-v7-0-v6-0-baf00f50028a@kernel.org?part=3