[PATCH 0/3] Support discontinuous PMU counters

[PATCH 0/3] Support discontinuous PMU counters

[Rob Bradford]

Currently the available PMU counters start at HPM3 and run through to
the number specified by the "pmu-num" property. There is no
requirement in the specification that the available counters be
continously numbered. This series add suppport for specifying a
discountinuous range of counters though a "pmu-mask" property.

Rob Bradford (3):
  target/riscv: Propagate error from PMU setup
  target/riscv: Support discontinuous PMU counters
  target/riscv: Don't assume PMU counters are continuous

 target/riscv/cpu.c     |  9 ++++++++-
 target/riscv/cpu_cfg.h |  1 +
 target/riscv/csr.c     |  5 +++--
 target/riscv/pmu.c     | 32 +++++++++++++++++++++-----------
 target/riscv/pmu.h     |  3 ++-
 5 files changed, 35 insertions(+), 15 deletions(-)

-- 
2.41.0

[PATCH 1/3] target/riscv: Propagate error from PMU setup

[Rob Bradford]

More closely follow the QEMU style by returning an Error and propagating
it there is an error relating to the PMU setup.

Further simplify the function by removing the num_counters parameter as
this is available from the passed in cpu pointer.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 target/riscv/cpu.c |  8 +++++++-
 target/riscv/pmu.c | 19 +++++++++----------
 target/riscv/pmu.h |  3 ++-
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 4140899c52..9d79c20c1a 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1488,7 +1488,13 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
     }
 
     if (cpu->cfg.pmu_num) {
-        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
+        riscv_pmu_init(cpu, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        if (cpu->cfg.ext_sscofpmf) {
             cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                           riscv_pmu_timer_cb, cpu);
         }
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 36f6307d28..13801ccb78 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -434,22 +434,21 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
 }
 
 
-int riscv_pmu_init(RISCVCPU *cpu, int num_counters)
+void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
 {
-    if (num_counters > (RV_MAX_MHPMCOUNTERS - 3)) {
-        return -1;
+    uint8_t pmu_num = cpu->cfg.pmu_num;
+
+    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
+        error_setg(errp, "Number of counters exceeds maximum available");
+        return;
     }
 
     cpu->pmu_event_ctr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
     if (!cpu->pmu_event_ctr_map) {
-        /* PMU support can not be enabled */
-        qemu_log_mask(LOG_UNIMP, "PMU events can't be supported\n");
-        cpu->cfg.pmu_num = 0;
-        return -1;
+        error_setg(errp, "Unable to allocate PMU event hash table");
+        return;
     }
 
     /* Create a bitmask of available programmable counters */
-    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, num_counters);
-
-    return 0;
+    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
 }
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 2bfb71ba87..88e0713296 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -17,13 +17,14 @@
  */
 
 #include "cpu.h"
+#include "qapi/error.h"
 
 bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
                                         uint32_t target_ctr);
 bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env,
                                   uint32_t target_ctr);
 void riscv_pmu_timer_cb(void *priv);
-int riscv_pmu_init(RISCVCPU *cpu, int num_counters);
+void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
 int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
                                uint32_t ctr_idx);
 int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
-- 
2.41.0

[PATCH 2/3] target/riscv: Support discontinuous PMU counters

[Rob Bradford]

There is no requirement that the enabled counters in the platform are
continuously numbered. Add a "pmu-mask" property that, if specified, can
be used to specify the enabled PMUs. In order to avoid ambiguity if
"pmu-mask" is specified then "pmu-num" must also match the number of
bits set in the mask.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 target/riscv/cpu.c     |  1 +
 target/riscv/cpu_cfg.h |  1 +
 target/riscv/pmu.c     | 15 +++++++++++++--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9d79c20c1a..b89b006a76 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1817,6 +1817,7 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
 static Property riscv_cpu_extensions[] = {
     /* Defaults for standard extensions */
     DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
+    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, 0),
     DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
     DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
     DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 0e6a0f245c..40f7d970bc 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -124,6 +124,7 @@ struct RISCVCPUConfig {
     bool ext_XVentanaCondOps;
 
     uint8_t pmu_num;
+    uint32_t pmu_mask;
     char *priv_spec;
     char *user_spec;
     char *bext_spec;
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 13801ccb78..f97e25a1f6 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
 void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
 {
     uint8_t pmu_num = cpu->cfg.pmu_num;
+    uint32_t pmu_mask = cpu->cfg.pmu_mask;
+
+    if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
+        error_setg(errp, "Mismatch between number of enabled counters in "
+                         "\"pmu-mask\" and \"pmu-num\"");
+        return;
+    }
 
     if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
         error_setg(errp, "Number of counters exceeds maximum available");
@@ -449,6 +456,10 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
         return;
     }
 
-    /* Create a bitmask of available programmable counters */
-    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
+    /* Create a bitmask of available programmable counters if none supplied */
+    if (pmu_mask) {
+        cpu->pmu_avail_ctrs = pmu_mask;
+    } else {
+        cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
+    }
 }
-- 
2.41.0

[PATCH 3/3] target/riscv: Don't assume PMU counters are continuous

[Rob Bradford]

Check the PMU available bitmask when checking if a counter is valid
rather than comparing the index against the number of PMUs.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 target/riscv/csr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 85a31dc420..3e126219ba 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -182,7 +182,8 @@ static RISCVException zcmt(CPURISCVState *env, int csrno)
 #if !defined(CONFIG_USER_ONLY)
 static RISCVException mctr(CPURISCVState *env, int csrno)
 {
-    int pmu_num = riscv_cpu_cfg(env)->pmu_num;
+    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
+    uint32_t pmu_avail_ctrs = cpu->pmu_avail_ctrs;
     int ctr_index;
     int base_csrno = CSR_MHPMCOUNTER3;
 
@@ -191,7 +192,7 @@ static RISCVException mctr(CPURISCVState *env, int csrno)
         base_csrno += 0x80;
     }
     ctr_index = csrno - base_csrno;
-    if (!pmu_num || ctr_index >= pmu_num) {
+    if ((BIT(ctr_index) & pmu_avail_ctrs >> 3) == 0) {
         /* The PMU is not enabled or counter is out of range */
         return RISCV_EXCP_ILLEGAL_INST;
     }
-- 
2.41.0

Re: [PATCH 2/3] target/riscv: Support discontinuous PMU counters

[Atish Kumar Patra]

On Tue, Oct 3, 2023 at 5:51 AM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> There is no requirement that the enabled counters in the platform are
> continuously numbered. Add a "pmu-mask" property that, if specified, can
> be used to specify the enabled PMUs. In order to avoid ambiguity if
> "pmu-mask" is specified then "pmu-num" must also match the number of
> bits set in the mask.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>  target/riscv/cpu.c     |  1 +
>  target/riscv/cpu_cfg.h |  1 +
>  target/riscv/pmu.c     | 15 +++++++++++++--
>  3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9d79c20c1a..b89b006a76 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1817,6 +1817,7 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>  static Property riscv_cpu_extensions[] = {
>      /* Defaults for standard extensions */
>      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, 0),
>      DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
>      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
>      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 0e6a0f245c..40f7d970bc 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -124,6 +124,7 @@ struct RISCVCPUConfig {
>      bool ext_XVentanaCondOps;
>
>      uint8_t pmu_num;
> +    uint32_t pmu_mask;
>      char *priv_spec;
>      char *user_spec;
>      char *bext_spec;
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 13801ccb78..f97e25a1f6 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
>  void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
>  {
>      uint8_t pmu_num = cpu->cfg.pmu_num;
> +    uint32_t pmu_mask = cpu->cfg.pmu_mask;
> +
> +    if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
> +        error_setg(errp, "Mismatch between number of enabled counters in "
> +                         "\"pmu-mask\" and \"pmu-num\"");
> +        return;
> +    }
>

Is that necessary for the default case? I am thinking of marking
pmu-num as deprecated and pmu-mask
as the preferred way of doing things as it is more flexible. There is
no real benefit carrying both.
The default pmu-mask value will change in that case.
We can just overwrite pmu-num with ctpop32(pmu_mask) if pmu-mask is
available. Thoughts ?

>      if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
>          error_setg(errp, "Number of counters exceeds maximum available");
> @@ -449,6 +456,10 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
>          return;
>      }
>
> -    /* Create a bitmask of available programmable counters */
> -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> +    /* Create a bitmask of available programmable counters if none supplied */
> +    if (pmu_mask) {
> +        cpu->pmu_avail_ctrs = pmu_mask;
> +    } else {
> +        cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> +    }
>  }
> --
> 2.41.0
>

Re: [PATCH 2/3] target/riscv: Support discontinuous PMU counters

[Rob Bradford]

Hi Atish,

On Tue, 2023-10-03 at 13:25 -0700, Atish Kumar Patra wrote:
> On Tue, Oct 3, 2023 at 5:51 AM Rob Bradford <rbradford@rivosinc.com>
> wrote:
> > 
> > There is no requirement that the enabled counters in the platform
> > are
> > continuously numbered. Add a "pmu-mask" property that, if
> > specified, can
> > be used to specify the enabled PMUs. In order to avoid ambiguity if
> > "pmu-mask" is specified then "pmu-num" must also match the number
> > of
> > bits set in the mask.
> > 
> > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > ---
> >  target/riscv/cpu.c     |  1 +
> >  target/riscv/cpu_cfg.h |  1 +
> >  target/riscv/pmu.c     | 15 +++++++++++++--
> >  3 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 9d79c20c1a..b89b006a76 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1817,6 +1817,7 @@ static void
> > riscv_cpu_add_misa_properties(Object *cpu_obj)
> >  static Property riscv_cpu_extensions[] = {
> >      /* Defaults for standard extensions */
> >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, 0),
> >      DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf,
> > false),
> >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > index 0e6a0f245c..40f7d970bc 100644
> > --- a/target/riscv/cpu_cfg.h
> > +++ b/target/riscv/cpu_cfg.h
> > @@ -124,6 +124,7 @@ struct RISCVCPUConfig {
> >      bool ext_XVentanaCondOps;
> > 
> >      uint8_t pmu_num;
> > +    uint32_t pmu_mask;
> >      char *priv_spec;
> >      char *user_spec;
> >      char *bext_spec;
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index 13801ccb78..f97e25a1f6 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState *env,
> > uint64_t value, uint32_t ctr_idx)
> >  void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> >  {
> >      uint8_t pmu_num = cpu->cfg.pmu_num;
> > +    uint32_t pmu_mask = cpu->cfg.pmu_mask;
> > +
> > +    if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
> > +        error_setg(errp, "Mismatch between number of enabled
> > counters in "
> > +                         "\"pmu-mask\" and \"pmu-num\"");
> > +        return;
> > +    }
> > 
> 
> Is that necessary for the default case? I am thinking of marking
> pmu-num as deprecated and pmu-mask
> as the preferred way of doing things as it is more flexible. There is
> no real benefit carrying both.
> The default pmu-mask value will change in that case.
> We can just overwrite pmu-num with ctpop32(pmu_mask) if pmu-mask is
> available. Thoughts ?
> 

I agree it makes sense to me that there is only one way for the user to
adjust the PMU count. However i'm not sure how we can handle the
transition if we choose to deprecate "pmu-num".

If we change the default "pmu-mask" to MAKE_32BIT_MASK(3, 16) then that
value in the config will always be set - you propose that we overwrite
"pmu-num" with the popcount of that property. But that will break if
the user has an existing setup that changes the value of "pmu-num"
(either as a property at runtime or in the CPU init code).

One option would be to not make the mask configurable as property and
make choosing the layout of the counters something that the specialised
CPU init can choose to do.

Cheers,

Rob

> >      if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> >          error_setg(errp, "Number of counters exceeds maximum
> > available");
> > @@ -449,6 +456,10 @@ void riscv_pmu_init(RISCVCPU *cpu, Error
> > **errp)
> >          return;
> >      }
> > 
> > -    /* Create a bitmask of available programmable counters */
> > -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > +    /* Create a bitmask of available programmable counters if none
> > supplied */
> > +    if (pmu_mask) {
> > +        cpu->pmu_avail_ctrs = pmu_mask;
> > +    } else {
> > +        cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > +    }
> >  }
> > --
> > 2.41.0
> > 

Re: [PATCH 1/3] target/riscv: Propagate error from PMU setup

[Alistair Francis]

On Tue, Oct 3, 2023 at 10:53 PM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> More closely follow the QEMU style by returning an Error and propagating
> it there is an error relating to the PMU setup.
>
> Further simplify the function by removing the num_counters parameter as
> this is available from the passed in cpu pointer.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c |  8 +++++++-
>  target/riscv/pmu.c | 19 +++++++++----------
>  target/riscv/pmu.h |  3 ++-
>  3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4140899c52..9d79c20c1a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1488,7 +1488,13 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
>      }
>
>      if (cpu->cfg.pmu_num) {
> -        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> +        riscv_pmu_init(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +
> +        if (cpu->cfg.ext_sscofpmf) {
>              cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                            riscv_pmu_timer_cb, cpu);
>          }
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 36f6307d28..13801ccb78 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -434,22 +434,21 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
>  }
>
>
> -int riscv_pmu_init(RISCVCPU *cpu, int num_counters)
> +void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
>  {
> -    if (num_counters > (RV_MAX_MHPMCOUNTERS - 3)) {
> -        return -1;
> +    uint8_t pmu_num = cpu->cfg.pmu_num;
> +
> +    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> +        error_setg(errp, "Number of counters exceeds maximum available");
> +        return;
>      }
>
>      cpu->pmu_event_ctr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
>      if (!cpu->pmu_event_ctr_map) {
> -        /* PMU support can not be enabled */
> -        qemu_log_mask(LOG_UNIMP, "PMU events can't be supported\n");
> -        cpu->cfg.pmu_num = 0;
> -        return -1;
> +        error_setg(errp, "Unable to allocate PMU event hash table");
> +        return;
>      }
>
>      /* Create a bitmask of available programmable counters */
> -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, num_counters);
> -
> -    return 0;
> +    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
>  }
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 2bfb71ba87..88e0713296 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -17,13 +17,14 @@
>   */
>
>  #include "cpu.h"
> +#include "qapi/error.h"
>
>  bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
>                                          uint32_t target_ctr);
>  bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env,
>                                    uint32_t target_ctr);
>  void riscv_pmu_timer_cb(void *priv);
> -int riscv_pmu_init(RISCVCPU *cpu, int num_counters);
> +void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
>  int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>                                 uint32_t ctr_idx);
>  int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
> --
> 2.41.0
>
>

Re: [PATCH 2/3] target/riscv: Support discontinuous PMU counters

[Alistair Francis]

On Wed, Oct 4, 2023 at 7:36 PM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> Hi Atish,
>
> On Tue, 2023-10-03 at 13:25 -0700, Atish Kumar Patra wrote:
> > On Tue, Oct 3, 2023 at 5:51 AM Rob Bradford <rbradford@rivosinc.com>
> > wrote:
> > >
> > > There is no requirement that the enabled counters in the platform
> > > are
> > > continuously numbered. Add a "pmu-mask" property that, if
> > > specified, can
> > > be used to specify the enabled PMUs. In order to avoid ambiguity if
> > > "pmu-mask" is specified then "pmu-num" must also match the number
> > > of
> > > bits set in the mask.
> > >
> > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > ---
> > >  target/riscv/cpu.c     |  1 +
> > >  target/riscv/cpu_cfg.h |  1 +
> > >  target/riscv/pmu.c     | 15 +++++++++++++--
> > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 9d79c20c1a..b89b006a76 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1817,6 +1817,7 @@ static void
> > > riscv_cpu_add_misa_properties(Object *cpu_obj)
> > >  static Property riscv_cpu_extensions[] = {
> > >      /* Defaults for standard extensions */
> > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, 0),
> > >      DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf,
> > > false),
> > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > index 0e6a0f245c..40f7d970bc 100644
> > > --- a/target/riscv/cpu_cfg.h
> > > +++ b/target/riscv/cpu_cfg.h
> > > @@ -124,6 +124,7 @@ struct RISCVCPUConfig {
> > >      bool ext_XVentanaCondOps;
> > >
> > >      uint8_t pmu_num;
> > > +    uint32_t pmu_mask;
> > >      char *priv_spec;
> > >      char *user_spec;
> > >      char *bext_spec;
> > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > > index 13801ccb78..f97e25a1f6 100644
> > > --- a/target/riscv/pmu.c
> > > +++ b/target/riscv/pmu.c
> > > @@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState *env,
> > > uint64_t value, uint32_t ctr_idx)
> > >  void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> > >  {
> > >      uint8_t pmu_num = cpu->cfg.pmu_num;
> > > +    uint32_t pmu_mask = cpu->cfg.pmu_mask;
> > > +
> > > +    if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
> > > +        error_setg(errp, "Mismatch between number of enabled
> > > counters in "
> > > +                         "\"pmu-mask\" and \"pmu-num\"");
> > > +        return;
> > > +    }
> > >
> >
> > Is that necessary for the default case? I am thinking of marking
> > pmu-num as deprecated and pmu-mask
> > as the preferred way of doing things as it is more flexible. There is
> > no real benefit carrying both.
> > The default pmu-mask value will change in that case.
> > We can just overwrite pmu-num with ctpop32(pmu_mask) if pmu-mask is
> > available. Thoughts ?
> >
>
> I agree it makes sense to me that there is only one way for the user to
> adjust the PMU count. However i'm not sure how we can handle the
> transition if we choose to deprecate "pmu-num".
>
> If we change the default "pmu-mask" to MAKE_32BIT_MASK(3, 16) then that
> value in the config will always be set - you propose that we overwrite
> "pmu-num" with the popcount of that property. But that will break if

Couldn't we deprecate "pmu-num" and then throw an error if both are
set? Then we can migrate away from "pmu-num"

Alistair

> the user has an existing setup that changes the value of "pmu-num"
> (either as a property at runtime or in the CPU init code).
>
> One option would be to not make the mask configurable as property and
> make choosing the layout of the counters something that the specialised
> CPU init can choose to do.
>
> Cheers,
>
> Rob
>
> > >      if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> > >          error_setg(errp, "Number of counters exceeds maximum
> > > available");
> > > @@ -449,6 +456,10 @@ void riscv_pmu_init(RISCVCPU *cpu, Error
> > > **errp)
> > >          return;
> > >      }
> > >
> > > -    /* Create a bitmask of available programmable counters */
> > > -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > +    /* Create a bitmask of available programmable counters if none
> > > supplied */
> > > +    if (pmu_mask) {
> > > +        cpu->pmu_avail_ctrs = pmu_mask;
> > > +    } else {
> > > +        cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > +    }
> > >  }
> > > --
> > > 2.41.0
> > >
>
>

Re: [PATCH 2/3] target/riscv: Support discontinuous PMU counters

[Atish Kumar Patra]

On Sun, Oct 8, 2023 at 5:58 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Oct 4, 2023 at 7:36 PM Rob Bradford <rbradford@rivosinc.com> wrote:
> >
> > Hi Atish,
> >
> > On Tue, 2023-10-03 at 13:25 -0700, Atish Kumar Patra wrote:
> > > On Tue, Oct 3, 2023 at 5:51 AM Rob Bradford <rbradford@rivosinc.com>
> > > wrote:
> > > >
> > > > There is no requirement that the enabled counters in the platform
> > > > are
> > > > continuously numbered. Add a "pmu-mask" property that, if
> > > > specified, can
> > > > be used to specify the enabled PMUs. In order to avoid ambiguity if
> > > > "pmu-mask" is specified then "pmu-num" must also match the number
> > > > of
> > > > bits set in the mask.
> > > >
> > > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > > ---
> > > >  target/riscv/cpu.c     |  1 +
> > > >  target/riscv/cpu_cfg.h |  1 +
> > > >  target/riscv/pmu.c     | 15 +++++++++++++--
> > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index 9d79c20c1a..b89b006a76 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -1817,6 +1817,7 @@ static void
> > > > riscv_cpu_add_misa_properties(Object *cpu_obj)
> > > >  static Property riscv_cpu_extensions[] = {
> > > >      /* Defaults for standard extensions */
> > > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > > +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, 0),
> > > >      DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf,
> > > > false),
> > > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > > index 0e6a0f245c..40f7d970bc 100644
> > > > --- a/target/riscv/cpu_cfg.h
> > > > +++ b/target/riscv/cpu_cfg.h
> > > > @@ -124,6 +124,7 @@ struct RISCVCPUConfig {
> > > >      bool ext_XVentanaCondOps;
> > > >
> > > >      uint8_t pmu_num;
> > > > +    uint32_t pmu_mask;
> > > >      char *priv_spec;
> > > >      char *user_spec;
> > > >      char *bext_spec;
> > > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > > > index 13801ccb78..f97e25a1f6 100644
> > > > --- a/target/riscv/pmu.c
> > > > +++ b/target/riscv/pmu.c
> > > > @@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState *env,
> > > > uint64_t value, uint32_t ctr_idx)
> > > >  void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> > > >  {
> > > >      uint8_t pmu_num = cpu->cfg.pmu_num;
> > > > +    uint32_t pmu_mask = cpu->cfg.pmu_mask;
> > > > +
> > > > +    if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
> > > > +        error_setg(errp, "Mismatch between number of enabled
> > > > counters in "
> > > > +                         "\"pmu-mask\" and \"pmu-num\"");
> > > > +        return;
> > > > +    }
> > > >
> > >
> > > Is that necessary for the default case? I am thinking of marking
> > > pmu-num as deprecated and pmu-mask
> > > as the preferred way of doing things as it is more flexible. There is
> > > no real benefit carrying both.
> > > The default pmu-mask value will change in that case.
> > > We can just overwrite pmu-num with ctpop32(pmu_mask) if pmu-mask is
> > > available. Thoughts ?
> > >
> >
> > I agree it makes sense to me that there is only one way for the user to
> > adjust the PMU count. However i'm not sure how we can handle the
> > transition if we choose to deprecate "pmu-num".
> >
> > If we change the default "pmu-mask" to MAKE_32BIT_MASK(3, 16) then that
> > value in the config will always be set - you propose that we overwrite
> > "pmu-num" with the popcount of that property. But that will break if
>
> Couldn't we deprecate "pmu-num" and then throw an error if both are
> set? Then we can migrate away from "pmu-num"
>

Yeah. pmu-num should be only available as a command line property and
marked deprecated.
If only pmu-num is set, it gets converted to a mask and throws a warning
that this is a deprecated property.
If only the pmu-mask is set, nothing additional is needed. These
patches are sufficient.
If nothing is set, the pmu-mask will be set to MAKE_32BIT_MASK(3, 16).
If a CPU init code uses pmu-num, we should change it to mask. The upstream code
doesn't have any other usage. Any downstream user will have to move
away from pmu-num
once this series is merged.

> Alistair
>
> > the user has an existing setup that changes the value of "pmu-num"
> > (either as a property at runtime or in the CPU init code).
> >
> > One option would be to not make the mask configurable as property and
> > make choosing the layout of the counters something that the specialised
> > CPU init can choose to do.
> >
> > Cheers,
> >
> > Rob
> >
> > > >      if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> > > >          error_setg(errp, "Number of counters exceeds maximum
> > > > available");
> > > > @@ -449,6 +456,10 @@ void riscv_pmu_init(RISCVCPU *cpu, Error
> > > > **errp)
> > > >          return;
> > > >      }
> > > >
> > > > -    /* Create a bitmask of available programmable counters */
> > > > -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > > +    /* Create a bitmask of available programmable counters if none
> > > > supplied */
> > > > +    if (pmu_mask) {
> > > > +        cpu->pmu_avail_ctrs = pmu_mask;
> > > > +    } else {
> > > > +        cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > > +    }
> > > >  }
> > > > --
> > > > 2.41.0
> > > >
> >
> >

Re: [PATCH 2/3] target/riscv: Support discontinuous PMU counters

[Alistair Francis]

On Tue, Oct 10, 2023 at 4:00 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
> On Sun, Oct 8, 2023 at 5:58 PM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Oct 4, 2023 at 7:36 PM Rob Bradford <rbradford@rivosinc.com> wrote:
> > >
> > > Hi Atish,
> > >
> > > On Tue, 2023-10-03 at 13:25 -0700, Atish Kumar Patra wrote:
> > > > On Tue, Oct 3, 2023 at 5:51 AM Rob Bradford <rbradford@rivosinc.com>
> > > > wrote:
> > > > >
> > > > > There is no requirement that the enabled counters in the platform
> > > > > are
> > > > > continuously numbered. Add a "pmu-mask" property that, if
> > > > > specified, can
> > > > > be used to specify the enabled PMUs. In order to avoid ambiguity if
> > > > > "pmu-mask" is specified then "pmu-num" must also match the number
> > > > > of
> > > > > bits set in the mask.
> > > > >
> > > > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > > > ---
> > > > >  target/riscv/cpu.c     |  1 +
> > > > >  target/riscv/cpu_cfg.h |  1 +
> > > > >  target/riscv/pmu.c     | 15 +++++++++++++--
> > > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > index 9d79c20c1a..b89b006a76 100644
> > > > > --- a/target/riscv/cpu.c
> > > > > +++ b/target/riscv/cpu.c
> > > > > @@ -1817,6 +1817,7 @@ static void
> > > > > riscv_cpu_add_misa_properties(Object *cpu_obj)
> > > > >  static Property riscv_cpu_extensions[] = {
> > > > >      /* Defaults for standard extensions */
> > > > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > > > +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, 0),
> > > > >      DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf,
> > > > > false),
> > > > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > > > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > > > index 0e6a0f245c..40f7d970bc 100644
> > > > > --- a/target/riscv/cpu_cfg.h
> > > > > +++ b/target/riscv/cpu_cfg.h
> > > > > @@ -124,6 +124,7 @@ struct RISCVCPUConfig {
> > > > >      bool ext_XVentanaCondOps;
> > > > >
> > > > >      uint8_t pmu_num;
> > > > > +    uint32_t pmu_mask;
> > > > >      char *priv_spec;
> > > > >      char *user_spec;
> > > > >      char *bext_spec;
> > > > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > > > > index 13801ccb78..f97e25a1f6 100644
> > > > > --- a/target/riscv/pmu.c
> > > > > +++ b/target/riscv/pmu.c
> > > > > @@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState *env,
> > > > > uint64_t value, uint32_t ctr_idx)
> > > > >  void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> > > > >  {
> > > > >      uint8_t pmu_num = cpu->cfg.pmu_num;
> > > > > +    uint32_t pmu_mask = cpu->cfg.pmu_mask;
> > > > > +
> > > > > +    if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
> > > > > +        error_setg(errp, "Mismatch between number of enabled
> > > > > counters in "
> > > > > +                         "\"pmu-mask\" and \"pmu-num\"");
> > > > > +        return;
> > > > > +    }
> > > > >
> > > >
> > > > Is that necessary for the default case? I am thinking of marking
> > > > pmu-num as deprecated and pmu-mask
> > > > as the preferred way of doing things as it is more flexible. There is
> > > > no real benefit carrying both.
> > > > The default pmu-mask value will change in that case.
> > > > We can just overwrite pmu-num with ctpop32(pmu_mask) if pmu-mask is
> > > > available. Thoughts ?
> > > >
> > >
> > > I agree it makes sense to me that there is only one way for the user to
> > > adjust the PMU count. However i'm not sure how we can handle the
> > > transition if we choose to deprecate "pmu-num".
> > >
> > > If we change the default "pmu-mask" to MAKE_32BIT_MASK(3, 16) then that
> > > value in the config will always be set - you propose that we overwrite
> > > "pmu-num" with the popcount of that property. But that will break if
> >
> > Couldn't we deprecate "pmu-num" and then throw an error if both are
> > set? Then we can migrate away from "pmu-num"
> >
>
> Yeah. pmu-num should be only available as a command line property and
> marked deprecated.
> If only pmu-num is set, it gets converted to a mask and throws a warning
> that this is a deprecated property.
> If only the pmu-mask is set, nothing additional is needed. These
> patches are sufficient.
> If nothing is set, the pmu-mask will be set to MAKE_32BIT_MASK(3, 16).

That all sounds good to me, and if both are set we can throw an error.

Alistair

> If a CPU init code uses pmu-num, we should change it to mask. The upstream code
> doesn't have any other usage. Any downstream user will have to move
> away from pmu-num
> once this series is merged.
>
> > Alistair
> >
> > > the user has an existing setup that changes the value of "pmu-num"
> > > (either as a property at runtime or in the CPU init code).
> > >
> > > One option would be to not make the mask configurable as property and
> > > make choosing the layout of the counters something that the specialised
> > > CPU init can choose to do.
> > >
> > > Cheers,
> > >
> > > Rob
> > >
> > > > >      if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> > > > >          error_setg(errp, "Number of counters exceeds maximum
> > > > > available");
> > > > > @@ -449,6 +456,10 @@ void riscv_pmu_init(RISCVCPU *cpu, Error
> > > > > **errp)
> > > > >          return;
> > > > >      }
> > > > >
> > > > > -    /* Create a bitmask of available programmable counters */
> > > > > -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > > > +    /* Create a bitmask of available programmable counters if none
> > > > > supplied */
> > > > > +    if (pmu_mask) {
> > > > > +        cpu->pmu_avail_ctrs = pmu_mask;
> > > > > +    } else {
> > > > > +        cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > > > +    }
> > > > >  }
> > > > > --
> > > > > 2.41.0
> > > > >
> > >
> > >

Re: [PATCH 2/3] target/riscv: Support discontinuous PMU counters

[Rob Bradford]

On Mon, 2023-10-09 at 11:00 -0700, Atish Kumar Patra wrote:
> On Sun, Oct 8, 2023 at 5:58 PM Alistair Francis
> <alistair23@gmail.com> wrote:
> > 
> > On Wed, Oct 4, 2023 at 7:36 PM Rob Bradford
> > <rbradford@rivosinc.com> wrote:
> > > 
> > > Hi Atish,
> > > 
> > > On Tue, 2023-10-03 at 13:25 -0700, Atish Kumar Patra wrote:
> > > > On Tue, Oct 3, 2023 at 5:51 AM Rob Bradford
> > > > <rbradford@rivosinc.com>
> > > > wrote:
> > > > > 
> > > > > There is no requirement that the enabled counters in the
> > > > > platform
> > > > > are
> > > > > continuously numbered. Add a "pmu-mask" property that, if
> > > > > specified, can
> > > > > be used to specify the enabled PMUs. In order to avoid
> > > > > ambiguity if
> > > > > "pmu-mask" is specified then "pmu-num" must also match the
> > > > > number
> > > > > of
> > > > > bits set in the mask.
> > > > > 
> > > > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > > > ---
> > > > >  target/riscv/cpu.c     |  1 +
> > > > >  target/riscv/cpu_cfg.h |  1 +
> > > > >  target/riscv/pmu.c     | 15 +++++++++++++--
> > > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > index 9d79c20c1a..b89b006a76 100644
> > > > > --- a/target/riscv/cpu.c
> > > > > +++ b/target/riscv/cpu.c
> > > > > @@ -1817,6 +1817,7 @@ static void
> > > > > riscv_cpu_add_misa_properties(Object *cpu_obj)
> > > > >  static Property riscv_cpu_extensions[] = {
> > > > >      /* Defaults for standard extensions */
> > > > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > > > +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask,
> > > > > 0),
> > > > >      DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf,
> > > > > false),
> > > > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei,
> > > > > true),
> > > > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > > > index 0e6a0f245c..40f7d970bc 100644
> > > > > --- a/target/riscv/cpu_cfg.h
> > > > > +++ b/target/riscv/cpu_cfg.h
> > > > > @@ -124,6 +124,7 @@ struct RISCVCPUConfig {
> > > > >      bool ext_XVentanaCondOps;
> > > > > 
> > > > >      uint8_t pmu_num;
> > > > > +    uint32_t pmu_mask;
> > > > >      char *priv_spec;
> > > > >      char *user_spec;
> > > > >      char *bext_spec;
> > > > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > > > > index 13801ccb78..f97e25a1f6 100644
> > > > > --- a/target/riscv/pmu.c
> > > > > +++ b/target/riscv/pmu.c
> > > > > @@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState
> > > > > *env,
> > > > > uint64_t value, uint32_t ctr_idx)
> > > > >  void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> > > > >  {
> > > > >      uint8_t pmu_num = cpu->cfg.pmu_num;
> > > > > +    uint32_t pmu_mask = cpu->cfg.pmu_mask;
> > > > > +
> > > > > +    if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
> > > > > +        error_setg(errp, "Mismatch between number of enabled
> > > > > counters in "
> > > > > +                         "\"pmu-mask\" and \"pmu-num\"");
> > > > > +        return;
> > > > > +    }
> > > > > 
> > > > 
> > > > Is that necessary for the default case? I am thinking of
> > > > marking
> > > > pmu-num as deprecated and pmu-mask
> > > > as the preferred way of doing things as it is more flexible.
> > > > There is
> > > > no real benefit carrying both.
> > > > The default pmu-mask value will change in that case.
> > > > We can just overwrite pmu-num with ctpop32(pmu_mask) if pmu-
> > > > mask is
> > > > available. Thoughts ?
> > > > 
> > > 
> > > I agree it makes sense to me that there is only one way for the
> > > user to
> > > adjust the PMU count. However i'm not sure how we can handle the
> > > transition if we choose to deprecate "pmu-num".
> > > 
> > > If we change the default "pmu-mask" to MAKE_32BIT_MASK(3, 16)
> > > then that
> > > value in the config will always be set - you propose that we
> > > overwrite
> > > "pmu-num" with the popcount of that property. But that will break
> > > if
> > 
> > Couldn't we deprecate "pmu-num" and then throw an error if both are
> > set? Then we can migrate away from "pmu-num"
> > 
> 
> Yeah. pmu-num should be only available as a command line property and
> marked deprecated.
> If only pmu-num is set, it gets converted to a mask and throws a
> warning
> that this is a deprecated property.

Is there a way to know the property has been set by the user? I
couldn't see anything in the API - do we just have to assume that if
the value is not the default then it has been changed by the user?

Cheers,

Rob

> If only the pmu-mask is set, nothing additional is needed. These
> patches are sufficient.
> If nothing is set, the pmu-mask will be set to MAKE_32BIT_MASK(3,
> 16).
> If a CPU init code uses pmu-num, we should change it to mask. The
> upstream code
> doesn't have any other usage. Any downstream user will have to move
> away from pmu-num
> once this series is merged.
> 
> > Alistair
> > 
> > > the user has an existing setup that changes the value of "pmu-
> > > num"
> > > (either as a property at runtime or in the CPU init code).
> > > 
> > > One option would be to not make the mask configurable as property
> > > and
> > > make choosing the layout of the counters something that the
> > > specialised
> > > CPU init can choose to do.
> > > 
> > > Cheers,
> > > 
> > > Rob
> > > 
> > > > >      if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> > > > >          error_setg(errp, "Number of counters exceeds maximum
> > > > > available");
> > > > > @@ -449,6 +456,10 @@ void riscv_pmu_init(RISCVCPU *cpu, Error
> > > > > **errp)
> > > > >          return;
> > > > >      }
> > > > > 
> > > > > -    /* Create a bitmask of available programmable counters
> > > > > */
> > > > > -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > > > +    /* Create a bitmask of available programmable counters
> > > > > if none
> > > > > supplied */
> > > > > +    if (pmu_mask) {
> > > > > +        cpu->pmu_avail_ctrs = pmu_mask;
> > > > > +    } else {
> > > > > +        cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > > > +    }
> > > > >  }
> > > > > --
> > > > > 2.41.0
> > > > > 
> > > 
> > > 

Re: [PATCH 3/3] target/riscv: Don't assume PMU counters are continuous

[LIU Zhiwei]

On 2023/10/3 20:49, Rob Bradford wrote:
> Check the PMU available bitmask when checking if a counter is valid
> rather than comparing the index against the number of PMUs.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>   target/riscv/csr.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 85a31dc420..3e126219ba 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -182,7 +182,8 @@ static RISCVException zcmt(CPURISCVState *env, int csrno)
>   #if !defined(CONFIG_USER_ONLY)
>   static RISCVException mctr(CPURISCVState *env, int csrno)
>   {
> -    int pmu_num = riscv_cpu_cfg(env)->pmu_num;
> +    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
Use env_archcpu(env) instead of RISCV_CPU(env_cpu(env)) macro.
> +    uint32_t pmu_avail_ctrs = cpu->pmu_avail_ctrs;
>       int ctr_index;
>       int base_csrno = CSR_MHPMCOUNTER3;
>   
> @@ -191,7 +192,7 @@ static RISCVException mctr(CPURISCVState *env, int csrno)
>           base_csrno += 0x80;
>       }
>       ctr_index = csrno - base_csrno;
> -    if (!pmu_num || ctr_index >= pmu_num) {
> +    if ((BIT(ctr_index) & pmu_avail_ctrs >> 3) == 0) {

Otherwise,

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           /* The PMU is not enabled or counter is out of range */
>           return RISCV_EXCP_ILLEGAL_INST;
>       }

Re: [PATCH 2/3] target/riscv: Support discontinuous PMU counters

[Alistair Francis]

On Wed, Oct 11, 2023 at 7:51 PM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> On Mon, 2023-10-09 at 11:00 -0700, Atish Kumar Patra wrote:
> > On Sun, Oct 8, 2023 at 5:58 PM Alistair Francis
> > <alistair23@gmail.com> wrote:
> > >
> > > On Wed, Oct 4, 2023 at 7:36 PM Rob Bradford
> > > <rbradford@rivosinc.com> wrote:
> > > >
> > > > Hi Atish,
> > > >
> > > > On Tue, 2023-10-03 at 13:25 -0700, Atish Kumar Patra wrote:
> > > > > On Tue, Oct 3, 2023 at 5:51 AM Rob Bradford
> > > > > <rbradford@rivosinc.com>
> > > > > wrote:
> > > > > >
> > > > > > There is no requirement that the enabled counters in the
> > > > > > platform
> > > > > > are
> > > > > > continuously numbered. Add a "pmu-mask" property that, if
> > > > > > specified, can
> > > > > > be used to specify the enabled PMUs. In order to avoid
> > > > > > ambiguity if
> > > > > > "pmu-mask" is specified then "pmu-num" must also match the
> > > > > > number
> > > > > > of
> > > > > > bits set in the mask.
> > > > > >
> > > > > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > > > > ---
> > > > > >  target/riscv/cpu.c     |  1 +
> > > > > >  target/riscv/cpu_cfg.h |  1 +
> > > > > >  target/riscv/pmu.c     | 15 +++++++++++++--
> > > > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > > index 9d79c20c1a..b89b006a76 100644
> > > > > > --- a/target/riscv/cpu.c
> > > > > > +++ b/target/riscv/cpu.c
> > > > > > @@ -1817,6 +1817,7 @@ static void
> > > > > > riscv_cpu_add_misa_properties(Object *cpu_obj)
> > > > > >  static Property riscv_cpu_extensions[] = {
> > > > > >      /* Defaults for standard extensions */
> > > > > >      DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > > > > +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask,
> > > > > > 0),
> > > > > >      DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf,
> > > > > > false),
> > > > > >      DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei,
> > > > > > true),
> > > > > >      DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > > > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > > > > index 0e6a0f245c..40f7d970bc 100644
> > > > > > --- a/target/riscv/cpu_cfg.h
> > > > > > +++ b/target/riscv/cpu_cfg.h
> > > > > > @@ -124,6 +124,7 @@ struct RISCVCPUConfig {
> > > > > >      bool ext_XVentanaCondOps;
> > > > > >
> > > > > >      uint8_t pmu_num;
> > > > > > +    uint32_t pmu_mask;
> > > > > >      char *priv_spec;
> > > > > >      char *user_spec;
> > > > > >      char *bext_spec;
> > > > > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > > > > > index 13801ccb78..f97e25a1f6 100644
> > > > > > --- a/target/riscv/pmu.c
> > > > > > +++ b/target/riscv/pmu.c
> > > > > > @@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState
> > > > > > *env,
> > > > > > uint64_t value, uint32_t ctr_idx)
> > > > > >  void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> > > > > >  {
> > > > > >      uint8_t pmu_num = cpu->cfg.pmu_num;
> > > > > > +    uint32_t pmu_mask = cpu->cfg.pmu_mask;
> > > > > > +
> > > > > > +    if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
> > > > > > +        error_setg(errp, "Mismatch between number of enabled
> > > > > > counters in "
> > > > > > +                         "\"pmu-mask\" and \"pmu-num\"");
> > > > > > +        return;
> > > > > > +    }
> > > > > >
> > > > >
> > > > > Is that necessary for the default case? I am thinking of
> > > > > marking
> > > > > pmu-num as deprecated and pmu-mask
> > > > > as the preferred way of doing things as it is more flexible.
> > > > > There is
> > > > > no real benefit carrying both.
> > > > > The default pmu-mask value will change in that case.
> > > > > We can just overwrite pmu-num with ctpop32(pmu_mask) if pmu-
> > > > > mask is
> > > > > available. Thoughts ?
> > > > >
> > > >
> > > > I agree it makes sense to me that there is only one way for the
> > > > user to
> > > > adjust the PMU count. However i'm not sure how we can handle the
> > > > transition if we choose to deprecate "pmu-num".
> > > >
> > > > If we change the default "pmu-mask" to MAKE_32BIT_MASK(3, 16)
> > > > then that
> > > > value in the config will always be set - you propose that we
> > > > overwrite
> > > > "pmu-num" with the popcount of that property. But that will break
> > > > if
> > >
> > > Couldn't we deprecate "pmu-num" and then throw an error if both are
> > > set? Then we can migrate away from "pmu-num"
> > >
> >
> > Yeah. pmu-num should be only available as a command line property and
> > marked deprecated.
> > If only pmu-num is set, it gets converted to a mask and throws a
> > warning
> > that this is a deprecated property.
>
> Is there a way to know the property has been set by the user? I
> couldn't see anything in the API - do we just have to assume that if
> the value is not the default then it has been changed by the user?

You should be able to use riscv_cpu_deprecated_exts as a starting point

Alistair