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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing explicit restoration of the CSR_SRMCFG hardware register for the currently executing task after PM resume.
- [High] Stale `cpu_srmcfg` cache is not invalidated during CPU offline, leading to Resource Control bypass during CPU bring-up.
--

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

riscv: add support for srmcfg CSR from Ssqosid extension

This commit adds support for the srmcfg CSR defined in the Ssqosid ISA
extension, allowing tasks to be tagged with Resource Control ID (RCID) and
Monitoring Counter ID (MCID). It handles context switching by saving and
restoring the srmcfg value per task, while caching it per-cpu to avoid
redundant writes.

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..2f3fbb08dcc97
> --- /dev/null
> +++ b/arch/riscv/kernel/qos.c

[ ... ]

> +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);
> +		break;
> +	}
> +	return NOTIFY_OK;
> +}

[Severity: High]
Does this leave the currently executing task with an incorrect hardware CSR
state?

While the software cache is invalidated by setting cpu_srmcfg to U32_MAX,
the hardware CSR_SRMCFG register is not explicitly reprogrammed during
CPU_PM_EXIT.

Since the hardware state may be lost or reset to an implementation-defined
value upon resume, wouldn't the task running at resume time continue executing
with the wrong QoS tag until it is eventually preempted?

[ ... ]

> +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, NULL);

[Severity: High]
Should a teardown callback be provided to cpuhp_setup_state() to invalidate
the cpu_srmcfg cache when a CPU goes offline?

Without a teardown callback, a CPU going offline retains its previous
cpu_srmcfg value. When brought back online, its hardware CSR is reset.

Tasks scheduling before the CPU reaches the CPUHP_AP_ONLINE_DYN state might
skip the CSR write if their thread srmcfg tag matches the stale cache. Could
this allow those early context switches to bypass resource constraints by
running with the hardware-reset CSR?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-ssqosid-cbqri-rqsc-v7-0-v5-0-78d3a7ba9dbe@kernel.org?part=3