Re: [PATCH v6 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] The `srmcfg` CSR is not initialized during early boot or CPU hotplug bringup, leaving CPUs running with undefined QoS tags.
- [Medium] The architecture-specific resctrl setters `resctrl_arch_set_closid_rmid()` and `resctrl_arch_set_cpu_default_closid_rmid()` are missing.
--

--- Patch [3]: [PATCH v6 3/8] riscv: Add support for srmcfg CSR from Ssqosid extension ---
commit a02eee11a5e49e74d1e00459d9acc1ec4a1982fa
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

> The context-switch path writes the field to the CSR, and
> 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]
Neither resctrl_arch_set_closid_rmid() nor
resctrl_arch_set_cpu_default_closid_rmid() appear to be implemented in this
patch. 

Since they are missing, are thread_struct.srmcfg and cpu_srmcfg_default ever
actually modified from their initial zero values?

> diff --git a/arch/riscv/kernel/qos.c b/arch/riscv/kernel/qos.c
> index 0000000000000..99925c0e93dd8
> --- /dev/null
> +++ b/arch/riscv/kernel/qos.c
> @@ -0,0 +1,101 @@

[ ... ]

> +	/*
> +	 * 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 software cache correctly handle CPU hotplug and early boot?

When a CPU is offlined, it continues context-switching as it goes down, which
overwrites the U32_MAX cache sentinel with a valid tag (such as 0 for the idle
thread).

When the CPU is brought back online, its hardware srmcfg CSR is reset to an
implementation-defined value. If the software cache cpu_srmcfg is still 0, the
first context switch to a task with a 0 tag in __switch_to_srmcfg() will see
thread_srmcfg == cpu_srmcfg and skip the CSR write:

arch/riscv/include/asm/qos.h:__switch_to_srmcfg() {
    ...
    if (thread_srmcfg != __this_cpu_read(cpu_srmcfg)) {
        __this_cpu_write(cpu_srmcfg, thread_srmcfg);
        csr_write(CSR_SRMCFG, thread_srmcfg);
    }
    ...
}

This leaves the hardware CSR undefined until this CPUHP_AP_ONLINE_DYN startup
callback runs. Is CPUHP_AP_ONLINE_DYN too late to prevent the CPU from running
with undefined QoS tags?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-dfustini-atl-sc-cbqri-dt-v6-0-7c22b05d461b@xxxxxxxxxx?part=3