Re: [ PATCH v3 02/10] target/riscv: Implement PMU CSR predicate function for

[Atish Patra]

On Thu, Nov 4, 2021 at 4:08 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Oct 26, 2021 at 3:56 AM Atish Patra <atish.patra@wdc.com> wrote:
> >
>
> The commit title is incomplete
>

Oops. Fixed it.

>
> > Currently, the predicate function for PMU related CSRs only works if
> > virtualization is enabled. It also does not check mcounteren bits before
> > before cycle/minstret/hpmcounterx access.
> >
> > Support supervisor mode access in the predicate function as well.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> >  target/riscv/csr.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 1ec776013435..de484c74d3b4 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -62,12 +62,64 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >  #if !defined(CONFIG_USER_ONLY)
> >      CPUState *cs = env_cpu(env);
> >      RISCVCPU *cpu = RISCV_CPU(cs);
> > +    int ctr_index;
> >
> >      if (!cpu->cfg.ext_counters) {
> >          /* The Counters extensions is not enabled */
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> >
> > +    if (env->priv == PRV_S) {
> > +        switch (csrno) {
> > +        case CSR_CYCLE:
> > +            if (!get_field(env->mcounteren, COUNTEREN_CY)) {
> > +                return RISCV_EXCP_ILLEGAL_INST;
> > +            }
> > +            break;
> > +        case CSR_TIME:
> > +            if (!get_field(env->mcounteren, COUNTEREN_TM)) {
> > +                return RISCV_EXCP_ILLEGAL_INST;
> > +            }
> > +            break;
> > +        case CSR_INSTRET:
> > +            if (!get_field(env->mcounteren, COUNTEREN_IR)) {
> > +                return RISCV_EXCP_ILLEGAL_INST;
> > +            }
> > +            break;
> > +        case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > +            ctr_index = csrno - CSR_CYCLE;
> > +            if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > +                return RISCV_EXCP_ILLEGAL_INST;
> > +            }
> > +            break;
>
> The above switch..case can be simplified with the logic of last case
> as the same one applies to all cases.
>

I did not condense it because I wanted to keep it analogous to the
virtualization case.
But I agree that it should be much shorter in both cases. I will add a
separate patch to simplify the entire function for
all the cases.


> > +        }
> > +        if (riscv_cpu_is_32bit(env)) {
> > +            switch (csrno) {
> > +            case CSR_CYCLEH:
> > +                if (!get_field(env->mcounteren, COUNTEREN_CY)) {
> > +                    return RISCV_EXCP_ILLEGAL_INST;
> > +                }
> > +                break;
> > +            case CSR_TIMEH:
> > +                if (!get_field(env->mcounteren, COUNTEREN_TM)) {
> > +                    return RISCV_EXCP_ILLEGAL_INST;
> > +                }
> > +                break;
> > +            case CSR_INSTRETH:
> > +                if (!get_field(env->mcounteren, COUNTEREN_IR)) {
> > +                    return RISCV_EXCP_ILLEGAL_INST;
> > +                }
> > +                break;
> > +            case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > +                ctr_index = csrno - CSR_CYCLEH;
> > +                if (!get_field(env->mcounteren, 1 << ctr_index)) {
> > +                    return RISCV_EXCP_ILLEGAL_INST;
> > +                }
>
> ditto
>
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> >      if (riscv_cpu_virt_enabled(env)) {
> >          switch (csrno) {
> >          case CSR_CYCLE:
> >
>
> Regards,
> Bin
>


--
Regards,
Atish