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

[Drew Fustini <fustini@kernel.org>]

On Thu, Nov 20, 2025 at 05:07:03PM +0100, Radim Krčmář wrote:

Thanks for the review!

> 2025-11-19T16:42:17-08:00, Drew Fustini <fustini@kernel.org>:
> > 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 but accessing it when V=1 shall cause a
> > virtual instruction exception. Implement this behavior by calling the
> > hmode predicate.
> >
> > Link: https://github.com/riscv-non-isa/riscv-cbqri/releases/download/v1.0/riscv-cbqri.pdf
> > Signed-off-by: Kornel Dulęba <mindal@semihalf.com>
> > [fustini: rebase on v10.1.50, fix check_srmcfg]
> > Signed-off-by: Drew Fustini <fustini@kernel.org>
> > ---
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > @@ -216,6 +216,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> >      ISA_EXT_DATA_ENTRY(ssdbltrp, PRIV_VERSION_1_13_0, ext_ssdbltrp),
> >      ISA_EXT_DATA_ENTRY(ssnpm, PRIV_VERSION_1_13_0, ext_ssnpm),
> >      ISA_EXT_DATA_ENTRY(sspm, PRIV_VERSION_1_13_0, ext_sspm),
> > +    ISA_EXT_DATA_ENTRY(ssqosid, PRIV_VERSION_1_12_0, ext_ssqosid),
> 
> Just out of curiosity, where does PRIV_VERSION_1_12_0 come from?

Ssqosid was originally ratified as a separate document but it since was
add to the Privileged Architecture document [1]. However, the Supervisor
Level ISA was version 1.13 when Ssqosid was added, so I should change
the above to PRIV_VERSION_1_13_0.

> 
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > @@ -1759,6 +1759,37 @@ static RISCVException write_stimecmph(CPURISCVState *env, int csrno,
> > +static RISCVException check_srmcfg(CPURISCVState *env, int csrno)
> > +{
> > +    RISCVCPU *cpu = env_archcpu(env);
> > +
> > +    if (!cpu->cfg.ext_ssqosid) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    /*
> > +     * Even though this is an S-mode CSR the spec says that we need to throw
> > +     * and virt instruction fault if a guest tries to access it.
> > +     */
> > +    return env->virt_enabled ?
> > +           RISCV_EXCP_VIRT_INSTRUCTION_FAULT : smode(env, csrno);
> > +}
> 
> The check is missing interaction with mstateen0.SRMCFG.

Good point. It looks like I should check mstateen0.SRMCFG which it bit
55. I'll add something like the following to the next revision:

---------------------------------------------------------------------
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index ebb400bf6f2c..bd73f9232d70 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -359,6 +359,7 @@
 #define SMSTATEEN0_FCSR     (1ULL << 1)
 #define SMSTATEEN0_JVT      (1ULL << 2)
 #define SMSTATEEN0_CTR      (1ULL << 54)
+#define SMSTATEEN0_SRMCFG   (1ULL << 55)
 #define SMSTATEEN0_P1P13    (1ULL << 56)
 #define SMSTATEEN0_HSCONTXT (1ULL << 57)
 #define SMSTATEEN0_IMSIC    (1ULL << 58)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 06a6212c672d..8f609e826fcd 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1767,6 +1767,11 @@ static RISCVException check_srmcfg(CPURISCVState *env, int csrno)
         return RISCV_EXCP_ILLEGAL_INST;
     }

+    RISCVException ret = smstateen_acc_ok(env, 0, SMSTATEEN0_SRMCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     /*
      * Even though this is an S-mode CSR the spec says that we need to throw
      * and virt instruction fault if a guest tries to access it.
---------------------------------------------------------------------

Thanks
Drew

[1] https://docs.riscv.org/reference/isa/priv/supervisor.html#ssqosid
[2] https://docs.riscv.org/reference/isa/priv/smstateen.html#state-enable-0-registersmstateen0.SRMCFG