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;
+ }
+ 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 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
© 2016 - 2026 Red Hat, Inc.