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