| Message ID | 20211025195601.245631-3-atish.patra@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve PMU support | expand |
On Tue, Oct 26, 2021 at 5:56 AM Atish Patra <atish.patra@wdc.com> wrote: > > 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> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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; > + } > + 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; > + } > + break; > + } > + } > + } > + > if (riscv_cpu_virt_enabled(env)) { > switch (csrno) { > case CSR_CYCLE: > -- > 2.31.1 > >
On Tue, Oct 26, 2021 at 3:56 AM Atish Patra <atish.patra@wdc.com> wrote: > The commit title is incomplete > 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. > + } > + 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
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
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; + } + 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; + } + break; + } + } + } + if (riscv_cpu_virt_enabled(env)) { switch (csrno) { case CSR_CYCLE:
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(+)