Re: [PATCH v4 3/8] riscv: Add support for srmcfg CSR from Ssqosid extension

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Late CPU hotplug cache invalidation allows newly onlined CPUs to execute with implementation-defined CSR values and potentially bypass the CSR update.
- [Medium] Missing resctrl architecture callbacks and generic subsystem integration, rendering the srmcfg context-switch logic functionally dead code.
--

--- Patch [3]: [PATCH v4 3/8] riscv: Add support for srmcfg CSR from Ssqosid extension ---
commit 5baf8388a55d26b8cdc1b9f1025583f7b6a3e4f3
Author: Drew Fustini <fustini@xxxxxxxxxx>

riscv: Add support for srmcfg CSR from Ssqosid extension

Add support for the srmcfg CSR defined in the Ssqosid ISA extension.
The CSR contains two fields: Resource Control ID (RCID) and Monitoring
Counter ID (MCID) for tracking resource usage.

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

>     resctrl_arch_set_closid_rmid() updates it when a task is assigned to a
>     resctrl control or monitoring group.
>     
>     A per-cpu cpu_srmcfg_default holds the default srmcfg for each CPU, set
>     by resctrl_arch_set_cpu_default_closid_rmid() on CPU group assignment.

[Severity: Medium]
Are these resctrl architecture callbacks missing from this patch series?

The commit message mentions resctrl_arch_set_closid_rmid() and
resctrl_arch_set_cpu_default_closid_rmid(), but they do not appear to be
implemented.

Without these integration functions, the new srmcfg field in thread_struct
and the cpu_srmcfg_default per-CPU variable are never updated.

Does this leave the context-switch tracking as functionally dead code that
only ever zeroes the CSR?

> diff --git a/arch/riscv/kernel/qos.c b/arch/riscv/kernel/qos.c
> new file mode 100644
> index 0000000000000..99925c0e93dd8
> --- /dev/null
> +++ b/arch/riscv/kernel/qos.c
[ ... ]
> +static int riscv_srmcfg_reset_cache(unsigned int cpu)
> +{
> +	per_cpu(cpu_srmcfg, cpu) = U32_MAX;
> +	return 0;
> +}
[ ... ]
> +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_reset_cache, riscv_srmcfg_reset_cache);

[Severity: High]
Does this late CPU hotplug cache invalidation allow newly onlined CPUs to
execute with implementation-defined CSR values?

The CPU cache invalidation happens at CPUHP_AP_ONLINE_DYN, which runs on the
target CPU itself. This means the CPU has already booted and context-switched
into the hotplug thread before riscv_srmcfg_reset_cache() is called.

If a CPU is offlined while its software cpu_srmcfg cache happens to be 0, and
then powered back on, its hardware SRMCFG register initializes to an
implementation-defined value.

When the CPU context-switches to the cpuhp thread (which has a default
srmcfg of 0), __switch_to_srmcfg() might see that the incoming thread's 0
matches the stale software cache's 0.

Could this bypass the necessary CSR write, leaving the CPU to run with
garbage values in the SRMCFG hardware CSR until the next context switch?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-dfustini-atl-sc-cbqri-dt-v4-0-e75c20201d64@xxxxxxxxxx?part=3