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 <[email protected]>: > > From: Kornel Dulęba <[email protected]> > > > > 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 <[email protected]> > > [fustini: rebase on v10.1.50, fix check_srmcfg] > > Signed-off-by: Drew Fustini <[email protected]> > > --- > > 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
