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

On Thu, Jul 30, 2026 at 01:22:26AM +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

Both are added in patch 5 along with the rest of the resctrl glue. I
already explained that this is not a problem but sashiko doesn't read
replies.

> > 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?

This is not a problem. Only the idle task and per-cpu kthreads run in
that window, and they run with the hart's reset srmcfg value.

Drew

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv