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