Re: [PATCH v6 1/6] riscv: implement Ssqosid extension and srmcfg CSR

[Drew Fustini <fustini@kernel.org>]

On Tue, Oct 27, 2026 at 12:22:02AM +0000, Alistair Francis wrote:
> On Mon, 2026-10-26 at 10:47 -0700, Drew Fustini wrote:
> > From: Kornel Dulęba <mindal@semihalf.com>
> > 
> > Implement the srmcfg CSR defined by the Ssqosid ISA extension
> > (Supervisor-mode Quality of Service ID). The CSR contains two fields:
> > 
> >   - Resource Control ID (RCID) used determine resource allocation
> >   - Monitoring Counter ID (MCID) used to track resource usage
> > 
> > The CSR is defined for S-mode, so check mstateen0.srmcfg to determine
> > if s-mode is allowed to access it. In addition, accessing it when V=1
> > shall cause a virtual instruction exception.
> > 
> > Link:
> > https://github.com/riscv-non-isa/riscv-cbqri/releases/download/v1.0/riscv-cbqri.pdf
> > Link: https://docs.riscv.org/reference/isa/priv/smstateen.html
> > Signed-off-by: Kornel Dulęba <mindal@semihalf.com>
> > [fustini: rebase on v10.1.50, fix check_srmcfg]
> > Reviewed-by: Radim Krčmář <radim.krcmar@oss.qualcomm.com>
> > Signed-off-by: Drew Fustini <fustini@kernel.org>
> 
> I'm just meat proxying this one, but it looks believable, let me know
> if it's wrong though:

Is this something I can run myself in the future?

I can run local sashiko for my linux patches but I wasn't sure what to
do for qemu.

> - [HIGH] In target/riscv/tcg/csr.c, none of write_mstateen0,
> write_mstateen0h, write_hstateen0, or write_hstateen0h ever ORs
> SMSTATEEN0_SRMCFG into wr_mask when ext_ssqosid is set, so bit 55 of
> mstateen0/hstateen0 stays read-only zero and M-mode firmware writes
> that enable srmcfg access are silently dropped, violating the
> Smstateen spec (a bit for an implemented extension must not be
> read-only zero). check_srmcfg gates the new CSR on
> smstateen_acc_ok(..., SMSTATEEN0_SRMCFG), and Smstateen is enabled by
> default on the same dynamic CPUs that get ext_ssqosid by default, so
> on the stock virt machine every S-mode (V=0) srmcfg access traps
> illegal instruction and VS-mode additionally takes a virtual
> instruction fault: the QoS feature this series exists to expose is
> systematically unusable. Fix by ORing the bit into the stateen write
> masks like the existing CTR, P1P13, and SVSLCT cases.

This is a real issue for mstateen0 and mstateen0h. I will add
SMSTATEEN0_SRMCFG to wr_mask in write_mstateen0() and
write_mstateen0h(). However, I believe hstateen0 bit 55 is WPRI so I
think the current behavior is correct.

> - [MEDIUM] In target/riscv/cpu.h, env->srmcfg is new guest-writable
>   architected state (written via write_srmcfg in
>   target/riscv/tcg/csr.c) but target/riscv/machine.c never migrates
>   it: vmstate_riscv_cpu has no ssqosid/srmcfg field or subsection.
>   After live migration, savevm/loadvm, or snapshot restore the
>   destination CPU resumes with srmcfg = 0 while the guest believes its
>   programmed RCID/MCID are still in effect. Add a versioned
>   vmstate_ssqosid subsection with a .needed predicate on ext_ssqosid,
>   following the vmstate_smstateen/vmstate_envcfg pattern.

I'll add a VMStateDescription for Ssqosid.

> - [MEDIUM] In target/riscv/cpu.c (isa_edata_arr), Ssqosid is
> registered as a user-facing extension and enabled by default on
> BASE32/BASE64 with no tests/tcg coverage anywhere in the tree. The
> RISC-V upstreaming rules require unit tests for new extensions' CSR
> accesses, and the stateen gating and V=1 trap paths added in
> check_srmcfg are exactly what such tests would exercise -- they would
> have caught the broken SRMCFG stateen bit above. Add tests/tcg cases
> for srmcfg read/write, WARL masking, and the
> stateen/virtual-instruction-fault gating.

I'll add tests for srmcfg.

> - [LOW] nit: in target/riscv/tcg/csr.c, the check_srmcfg comment says
>   "we need to throw and virt instruction fault"; s/and virt/a virtual/.

I'll fix.

Thanks,
Drew