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

[Drew Fustini <fustini@kernel.org>]

On Mon, Jul 06, 2026 at 11:17:04PM +0000, sashiko-bot@kernel.org wrote:
> 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@kernel.org>
> 
> 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?

They are added later in the series. This patch adds only the srmcfg CSR
plumbing that those callbacks drive.

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

No, this won't happen. riscv_srmcfg_reset_cache() is the startup
callback of a CPUHP_AP_ONLINE_DYN state, which is ordered before
CPUHP_AP_ACTIVE. Only the idle task and per-cpu kthreads can run before
it, and their QoS tag does not matter.

Drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv