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

[Kevin Xue]

On Tue, Jul 18, 2023 at 6:25 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> On 2023/7/19 06:47, Kaiwen Xue wrote:
> > 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.
> Maybe you can record/accumulate them when privilege mode changes/switchs.

Good idea. Will do in the next version as well.

- Kevin

> >
> > Signed-off-by: Kaiwen Xue <kaiwenx@andrew.cmu.edu>
> > Signed-off-by: Kaiwen Xue <kaiwenx@rivosinc.com>
> > ---
> >   target/riscv/csr.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 73 insertions(+)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index ea7585329e..b1d5e85a79 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -218,6 +218,17 @@ static RISCVException sscofpmf(CPURISCVState *env, int 
> > csrno)
> >       return RISCV_EXCP_NONE;
> >   }
> >
> > +static RISCVException smcntrpmf(CPURISCVState *env, int csrno)
> > +{
> > +    RISCVCPU *cpu = env_archcpu(env);
> > +
> > +    if (!cpu->cfg.ext_smcntrpmf) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >   static RISCVException any(CPURISCVState *env, int csrno)
> >   {
> >       return RISCV_EXCP_NONE;
> > @@ -800,6 +811,54 @@ static int read_hpmcounterh(CPURISCVState *env, int 
> > csrno, target_ulong *val)
> >
> >   #else /* CONFIG_USER_ONLY */
> >
> > +static int read_mcyclecfg(CPURISCVState *env, int csrno, target_ulong *val)
> > +{
> > +    *val = env->mcyclecfg;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int write_mcyclecfg(CPURISCVState *env, int csrno, target_ulong val)
> > +{
> > +    env->mcyclecfg = val;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int read_mcyclecfgh(CPURISCVState *env, int csrno, target_ulong 
> > *val)
> > +{
> > +    *val = env->mcyclecfgh;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int write_mcyclecfgh(CPURISCVState *env, int csrno, target_ulong 
> > val)
> > +{
> > +    env->mcyclecfgh = val;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int read_minstretcfg(CPURISCVState *env, int csrno, target_ulong 
> > *val)
> > +{
> > +    *val = env->minstretcfg;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int write_minstretcfg(CPURISCVState *env, int csrno, target_ulong 
> > val)
> > +{
> > +    env->minstretcfg = val;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int read_minstretcfgh(CPURISCVState *env, int csrno, target_ulong 
> > *val)
> > +{
> > +    *val = env->minstretcfgh;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static int write_minstretcfgh(CPURISCVState *env, int csrno, target_ulong 
> > val)
> > +{
> > +    env->minstretcfgh = val;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >   static int read_mhpmevent(CPURISCVState *env, int csrno, target_ulong 
> > *val)
> >   {
> >       int evt_index = csrno - CSR_MCOUNTINHIBIT;
> > @@ -4506,6 +4565,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,
> > @@ -4565,6 +4631,13 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >       [CSR_MHPMEVENT31]    = { "mhpmevent31",    any,    read_mhpmevent,
> >                                write_mhpmevent                           },
> >
> > +    [CSR_MCYCLECFGH]     = { "mcyclecfgh",   smcntrpmf, read_mcyclecfgh,
> > +                             write_mcyclecfgh,
> > +                             .min_priv_ver = PRIV_VERSION_1_12_0        },
> > +    [CSR_MINSTRETCFGH]   = { "minstretcfgh", smcntrpmf, read_minstretcfgh,
> > +                             write_minstretcfgh,
> > +                             .min_priv_ver = PRIV_VERSION_1_12_0        },
>
> This two CSRs are RV32-only, they cannot directly share the same
> predicate as MCYCLECFG/MINSTRETCFG.
>
> Regards,
>
> Weiwei Li

Thanks for catching this! Seems sscofpmf also reused the sscofpmf()
predicate for scountovf
and mhpmeventXh. I'll probably turn in another patch for that later on.

- Kevin

>
> > +
> >       [CSR_MHPMEVENT3H]    = { "mhpmevent3h",    sscofpmf,  read_mhpmeventh,
> >                                write_mhpmeventh,
> >                                .min_priv_ver = PRIV_VERSION_1_12_0        },
>