Re: [PATCH v7 05/11] target/riscv: Add cycle & instret privilege mode filtering support

[Atish Kumar Patra <atishp@rivosinc.com>]

On Tue, Jul 2, 2024 at 6:19 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Jun 27, 2024 at 10:00 AM Atish Patra <atishp@rivosinc.com> wrote:
> >
> > From: Kaiwen Xue <kaiwenx@rivosinc.com>
> >
> > QEMU only calculates dummy cycles and instructions, so there is no
> > actual means to stop the icount in QEMU. Hence this patch merely adds
> > the functionality of accessing the cfg registers, and cause no actual
> > effects on the counting of cycle and instret counters.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> > ---
> >  target/riscv/csr.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 3ad851707e5c..665c534db1a0 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -236,6 +236,24 @@ static RISCVException sscofpmf_32(CPURISCVState *env, int csrno)
> >      return sscofpmf(env, csrno);
> >  }
> >
> > +static RISCVException smcntrpmf(CPURISCVState *env, int csrno)
> > +{
> > +    if (!riscv_cpu_cfg(env)->ext_smcntrpmf) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException smcntrpmf_32(CPURISCVState *env, int csrno)
> > +{
> > +    if (riscv_cpu_mxl(env) != MXL_RV32) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return smcntrpmf(env, csrno);
> > +}
> > +
> >  static RISCVException any(CPURISCVState *env, int csrno)
> >  {
> >      return RISCV_EXCP_NONE;
> > @@ -830,6 +848,62 @@ static RISCVException read_hpmcounterh(CPURISCVState *env, int csrno,
> >
> >  #else /* CONFIG_USER_ONLY */
> >
> > +static RISCVException read_mcyclecfg(CPURISCVState *env, int csrno,
> > +                                     target_ulong *val)
> > +{
> > +    *val = env->mcyclecfg;
>
> We don't do a good job of this in other places, but we should check
> for RVU and RVS to determine if the bits can actually be set.
>
> This is especially important for Hypervisor support (VS/VU-modes), as
> that is often not supported so we should report that here
>

Agreed. I have fixed that here and added a patch for checking that
while updating
the INH bits for mhpmevent as well.

> Alistair
>
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_mcyclecfg(CPURISCVState *env, int csrno,
> > +                                      target_ulong val)
> > +{
> > +    env->mcyclecfg = val;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException read_mcyclecfgh(CPURISCVState *env, int csrno,
> > +                                      target_ulong *val)
> > +{
> > +    *val = env->mcyclecfgh;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_mcyclecfgh(CPURISCVState *env, int csrno,
> > +                                       target_ulong val)
> > +{
> > +    env->mcyclecfgh = val;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException read_minstretcfg(CPURISCVState *env, int csrno,
> > +                                       target_ulong *val)
> > +{
> > +    *val = env->minstretcfg;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_minstretcfg(CPURISCVState *env, int csrno,
> > +                                        target_ulong val)
> > +{
> > +    env->minstretcfg = val;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException read_minstretcfgh(CPURISCVState *env, int csrno,
> > +                                        target_ulong *val)
> > +{
> > +    *val = env->minstretcfgh;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_minstretcfgh(CPURISCVState *env, int csrno,
> > +                                         target_ulong val)
> > +{
> > +    env->minstretcfgh = val;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >  static RISCVException read_mhpmevent(CPURISCVState *env, int csrno,
> >                                       target_ulong *val)
> >  {
> > @@ -5051,6 +5125,13 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >                               write_mcountinhibit,
> >                               .min_priv_ver = PRIV_VERSION_1_11_0       },
> >
> > +    [CSR_MCYCLECFG]      = { "mcyclecfg",   smcntrpmf, read_mcyclecfg,
> > +                             write_mcyclecfg,
> > +                             .min_priv_ver = PRIV_VERSION_1_12_0       },
> > +    [CSR_MINSTRETCFG]    = { "minstretcfg", smcntrpmf, read_minstretcfg,
> > +                             write_minstretcfg,
> > +                             .min_priv_ver = PRIV_VERSION_1_12_0       },
> > +
> >      [CSR_MHPMEVENT3]     = { "mhpmevent3",     any,    read_mhpmevent,
> >                               write_mhpmevent                           },
> >      [CSR_MHPMEVENT4]     = { "mhpmevent4",     any,    read_mhpmevent,
> > @@ -5110,6 +5191,13 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >      [CSR_MHPMEVENT31]    = { "mhpmevent31",    any,    read_mhpmevent,
> >                               write_mhpmevent                           },
> >
> > +    [CSR_MCYCLECFGH]     = { "mcyclecfgh",   smcntrpmf_32, read_mcyclecfgh,
> > +                             write_mcyclecfgh,
> > +                             .min_priv_ver = PRIV_VERSION_1_12_0        },
> > +    [CSR_MINSTRETCFGH]   = { "minstretcfgh", smcntrpmf_32, read_minstretcfgh,
> > +                             write_minstretcfgh,
> > +                             .min_priv_ver = PRIV_VERSION_1_12_0        },
> > +
> >      [CSR_MHPMEVENT3H]    = { "mhpmevent3h",    sscofpmf_32,  read_mhpmeventh,
> >                               write_mhpmeventh,
> >                               .min_priv_ver = PRIV_VERSION_1_12_0        },
> >
> > --
> > 2.34.1
> >
> >