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

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
>

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
> > 

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
> > >
>
>

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
> > > >
> >
> >

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
> > > > > 
> > > 
> > > 

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

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
> > > > >
> > >
> > >