On Thu, Nov 4, 2021 at 4:08 AM Bin Meng <bmeng...@gmail.com> wrote:
>
> On Tue, Oct 26, 2021 at 3:56 AM Atish Patra <atish.pa...@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.pa...@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
