1. Patchew
  2. QEMU
  3. [ RFC v2 0/9] Improve PMU support
  4. View patch

[ RFC v2 1/9] target/riscv: Fix PMU CSR predicate function

Atish Patra posted 9 patches 4 years, 5 months ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>
There is a newer version of this series
[ RFC v2 1/9] target/riscv: Fix PMU CSR predicate function
Posted by Atish Patra 4 years, 5 months ago 
Currently, the predicate function for PMU related CSRs only works if
virtualization is enabled. Ideally, they should check the mcountern
bits before cycle/minstret/hpmcounterx access. The predicate function
also calculates the counter index incorrectly for hpmcounterx.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 target/riscv/csr.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9a4ed18ac597..0515d851b948 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, HCOUNTEREN_CY)) {
+                return RISCV_EXCP_ILLEGAL_INST;
+            }
+            break;
+        case CSR_TIME:
+            if (!get_field(env->mcounteren, HCOUNTEREN_TM)) {
+                return RISCV_EXCP_ILLEGAL_INST;
+            }
+            break;
+        case CSR_INSTRET:
+            if (!get_field(env->mcounteren, HCOUNTEREN_IR)) {
+                return RISCV_EXCP_ILLEGAL_INST;
+            }
+            break;
+        case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
+            ctr_index = csrno - CSR_HPMCOUNTER3 + 3;
+            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, HCOUNTEREN_CY)) {
+                    return RISCV_EXCP_ILLEGAL_INST;
+                }
+                break;
+            case CSR_TIMEH:
+                if (!get_field(env->mcounteren, HCOUNTEREN_TM)) {
+                    return RISCV_EXCP_ILLEGAL_INST;
+                }
+                break;
+            case CSR_INSTRETH:
+                if (!get_field(env->mcounteren, HCOUNTEREN_IR)) {
+                    return RISCV_EXCP_ILLEGAL_INST;
+                }
+                break;
+            case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
+                ctr_index = csrno - CSR_HPMCOUNTER3H + 3;
+                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:
@@ -89,8 +141,9 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
             }
             break;
         case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
-            if (!get_field(env->hcounteren, 1 << (csrno - CSR_HPMCOUNTER3)) &&
-                get_field(env->mcounteren, 1 << (csrno - CSR_HPMCOUNTER3))) {
+            ctr_index = csrno - CSR_HPMCOUNTER3 + 3;
+            if (!get_field(env->hcounteren, 1 << ctr_index) &&
+                get_field(env->mcounteren, 1 << ctr_index)) {
                 return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
             }
             break;
@@ -116,8 +169,9 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
                 }
                 break;
             case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
-                if (!get_field(env->hcounteren, 1 << (csrno - CSR_HPMCOUNTER3H)) &&
-                    get_field(env->mcounteren, 1 << (csrno - CSR_HPMCOUNTER3H))) {
+                ctr_index = csrno - CSR_HPMCOUNTER3H + 3;
+                if (!get_field(env->hcounteren, 1 << ctr_index) &&
+                    get_field(env->mcounteren, 1 << ctr_index)) {
                     return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
                 }
                 break;
-- 
2.31.1
Re: [ RFC v2 1/9] target/riscv: Fix PMU CSR predicate function
Posted by Bin Meng 4 years, 4 months ago 
On Fri, Sep 10, 2021 at 4:27 AM Atish Patra <atish.patra@wdc.com> wrote:
>
> Currently, the predicate function for PMU related CSRs only works if
> virtualization is enabled. Ideally, they should check the mcountern
> bits before cycle/minstret/hpmcounterx access. The predicate function
> also calculates the counter index incorrectly for hpmcounterx.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  target/riscv/csr.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 9a4ed18ac597..0515d851b948 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, HCOUNTEREN_CY)) {
> +                return RISCV_EXCP_ILLEGAL_INST;
> +            }
> +            break;
> +        case CSR_TIME:
> +            if (!get_field(env->mcounteren, HCOUNTEREN_TM)) {
> +                return RISCV_EXCP_ILLEGAL_INST;
> +            }
> +            break;
> +        case CSR_INSTRET:
> +            if (!get_field(env->mcounteren, HCOUNTEREN_IR)) {
> +                return RISCV_EXCP_ILLEGAL_INST;
> +            }
> +            break;
> +        case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> +            ctr_index = csrno - CSR_HPMCOUNTER3 + 3;
> +            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, HCOUNTEREN_CY)) {
> +                    return RISCV_EXCP_ILLEGAL_INST;
> +                }
> +                break;
> +            case CSR_TIMEH:
> +                if (!get_field(env->mcounteren, HCOUNTEREN_TM)) {
> +                    return RISCV_EXCP_ILLEGAL_INST;
> +                }
> +                break;
> +            case CSR_INSTRETH:
> +                if (!get_field(env->mcounteren, HCOUNTEREN_IR)) {
> +                    return RISCV_EXCP_ILLEGAL_INST;
> +                }
> +                break;
> +            case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> +                ctr_index = csrno - CSR_HPMCOUNTER3H + 3;
> +                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:
> @@ -89,8 +141,9 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>              }
>              break;
>          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> -            if (!get_field(env->hcounteren, 1 << (csrno - CSR_HPMCOUNTER3)) &&
> -                get_field(env->mcounteren, 1 << (csrno - CSR_HPMCOUNTER3))) {
> +            ctr_index = csrno - CSR_HPMCOUNTER3 + 3;

ctr_index = csrno - CSR_CYCLE;

> +            if (!get_field(env->hcounteren, 1 << ctr_index) &&
> +                get_field(env->mcounteren, 1 << ctr_index)) {

This fix (along with the H part below) should be put in a separate patch.

>                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>              }
>              break;
> @@ -116,8 +169,9 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
>                  }
>                  break;
>              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> -                if (!get_field(env->hcounteren, 1 << (csrno - CSR_HPMCOUNTER3H)) &&
> -                    get_field(env->mcounteren, 1 << (csrno - CSR_HPMCOUNTER3H))) {
> +                ctr_index = csrno - CSR_HPMCOUNTER3H + 3;

ctr_index = csrno - CSR_CYCLEH;

> +                if (!get_field(env->hcounteren, 1 << ctr_index) &&
> +                    get_field(env->mcounteren, 1 << ctr_index)) {
>                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>                  }
>                  break;
> --

You may need to rebase the patch on:
http://patchwork.ozlabs.org/project/qemu-devel/patch/20210915084601.24304-1-bmeng.cn@gmail.com/

Regards,
Bin
Re: [ RFC v2 1/9] target/riscv: Fix PMU CSR predicate function
Posted by Atish Patra 4 years, 4 months ago 
On Wed, Sep 15, 2021 at 7:51 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Sep 10, 2021 at 4:27 AM Atish Patra <atish.patra@wdc.com> wrote:
> >
> > Currently, the predicate function for PMU related CSRs only works if
> > virtualization is enabled. Ideally, they should check the mcountern
> > bits before cycle/minstret/hpmcounterx access. The predicate function
> > also calculates the counter index incorrectly for hpmcounterx.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> >  target/riscv/csr.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 58 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 9a4ed18ac597..0515d851b948 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, HCOUNTEREN_CY)) {
> > +                return RISCV_EXCP_ILLEGAL_INST;
> > +            }
> > +            break;
> > +        case CSR_TIME:
> > +            if (!get_field(env->mcounteren, HCOUNTEREN_TM)) {
> > +                return RISCV_EXCP_ILLEGAL_INST;
> > +            }
> > +            break;
> > +        case CSR_INSTRET:
> > +            if (!get_field(env->mcounteren, HCOUNTEREN_IR)) {
> > +                return RISCV_EXCP_ILLEGAL_INST;
> > +            }
> > +            break;
> > +        case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > +            ctr_index = csrno - CSR_HPMCOUNTER3 + 3;
> > +            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, HCOUNTEREN_CY)) {
> > +                    return RISCV_EXCP_ILLEGAL_INST;
> > +                }
> > +                break;
> > +            case CSR_TIMEH:
> > +                if (!get_field(env->mcounteren, HCOUNTEREN_TM)) {
> > +                    return RISCV_EXCP_ILLEGAL_INST;
> > +                }
> > +                break;
> > +            case CSR_INSTRETH:
> > +                if (!get_field(env->mcounteren, HCOUNTEREN_IR)) {
> > +                    return RISCV_EXCP_ILLEGAL_INST;
> > +                }
> > +                break;
> > +            case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > +                ctr_index = csrno - CSR_HPMCOUNTER3H + 3;
> > +                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:
> > @@ -89,8 +141,9 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >              }
> >              break;
> >          case CSR_HPMCOUNTER3...CSR_HPMCOUNTER31:
> > -            if (!get_field(env->hcounteren, 1 << (csrno - CSR_HPMCOUNTER3)) &&
> > -                get_field(env->mcounteren, 1 << (csrno - CSR_HPMCOUNTER3))) {
> > +            ctr_index = csrno - CSR_HPMCOUNTER3 + 3;
>
> ctr_index = csrno - CSR_CYCLE;

Will update it.

>
> > +            if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > +                get_field(env->mcounteren, 1 << ctr_index)) {
>
> This fix (along with the H part below) should be put in a separate patch.

Sure.

>
> >                  return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> >              }
> >              break;
> > @@ -116,8 +169,9 @@ static RISCVException ctr(CPURISCVState *env, int csrno)
> >                  }
> >                  break;
> >              case CSR_HPMCOUNTER3H...CSR_HPMCOUNTER31H:
> > -                if (!get_field(env->hcounteren, 1 << (csrno - CSR_HPMCOUNTER3H)) &&
> > -                    get_field(env->mcounteren, 1 << (csrno - CSR_HPMCOUNTER3H))) {
> > +                ctr_index = csrno - CSR_HPMCOUNTER3H + 3;
>
> ctr_index = csrno - CSR_CYCLEH;
>
> > +                if (!get_field(env->hcounteren, 1 << ctr_index) &&
> > +                    get_field(env->mcounteren, 1 << ctr_index)) {
> >                      return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> >                  }
> >                  break;
> > --
>
> You may need to rebase the patch on:
> http://patchwork.ozlabs.org/project/qemu-devel/patch/20210915084601.24304-1-bmeng.cn@gmail.com/
>

Sure.

> Regards,
> Bin
>


-- 
Regards,
Atish

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.