Re: [PATCH v4 4/6] target/riscv: Add "pmu-mask" property to replace "pmu-num"

[Rob Bradford]

On Mon, 2023-10-23 at 12:02 +1000, Alistair Francis wrote:
> On Thu, Oct 19, 2023 at 1:45 AM Rob Bradford <rbradford@rivosinc.com>
> wrote:
> > 
> > Using a mask instead of the number of PMU devices supports the
> > accurate
> > emulation of platforms that have a discontinuous set of PMU
> > counters.
> > 
> > The "pmu-num" property now generates a warning when used by the
> > user on
> > the command line. In order to avoid warning for the default value
> > when
> > the property is not explicitly by the user the property default
> > value
> > cannot be used so the default value must be set during the CPU
> > object
> > initialisation.
> > 
> > If the "pmu-num" value is changed from the default then the mask
> > will be
> > generated from that to support the transition to "pmu-mask".
> > 
> > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > ---
> >  target/riscv/cpu.c         | 31 ++++++++++++++++++++++++++++++-
> >  target/riscv/cpu_cfg.h     |  3 ++-
> >  target/riscv/machine.c     |  2 +-
> >  target/riscv/pmu.c         | 20 ++++++++++++++++----
> >  target/riscv/tcg/tcg-cpu.c |  2 +-
> >  5 files changed, 50 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index ac4a6c7eec..1b734d1dde 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1169,6 +1169,11 @@ static void riscv_cpu_post_init(Object *obj)
> > 
> >  static void riscv_cpu_init(Object *obj)
> >  {
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> > +    /* Using property default value would spam deprecation warning
> > */
> > +    cpu->cfg.pmu_num = 16;
> > +
> >  #ifndef CONFIG_USER_ONLY
> >      qdev_init_gpio_in(DEVICE(obj), riscv_cpu_set_irq,
> >                        IRQ_LOCAL_MAX + IRQ_LOCAL_GUEST_MAX);
> > @@ -1362,8 +1367,32 @@ const RISCVCPUMultiExtConfig
> > riscv_cpu_deprecated_exts[] = {
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> > 
> > +static void prop_pmu_num_set(Object *obj, Visitor *v, const char
> > *name,
> > +                             void *opaque, Error **errp)
> > +{
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> > +    visit_type_uint8(v, name, &cpu->cfg.pmu_num, errp);
> > +    warn_report("\"pmu-num\" property is deprecated; use \"pmu-
> > mask\"");
> > +}
> > +
> > +static void prop_pmu_num_get(Object *obj, Visitor *v, const char
> > *name,
> > +                             void *opaque, Error **errp)
> > +{
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> > +    visit_type_uint8(v, name, &cpu->cfg.pmu_num, errp);
> > +}
> > +
> > +const PropertyInfo prop_pmu_num = {
> > +    .name = "pmu-num",
> > +    .get = prop_pmu_num_get,
> > +    .set = prop_pmu_num_set,
> > +};
> > +
> >  Property riscv_cpu_options[] = {
> > -    DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > +    DEFINE_PROP("pmu-num", RISCVCPU, cfg.pmu_num, prop_pmu_num,
> > uint8_t), /* Deprecated */
> > +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask,
> > MAKE_64BIT_MASK(3, 16)),
> > 
> >      DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
> >      DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > index 0e6a0f245c..d273487040 100644
> > --- a/target/riscv/cpu_cfg.h
> > +++ b/target/riscv/cpu_cfg.h
> > @@ -123,7 +123,8 @@ struct RISCVCPUConfig {
> >      bool ext_xtheadsync;
> >      bool ext_XVentanaCondOps;
> > 
> > -    uint8_t pmu_num;
> > +    uint8_t pmu_num; /* Deprecated */
> > +    uint32_t pmu_mask;
> >      char *priv_spec;
> >      char *user_spec;
> >      char *bext_spec;
> > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > index c7c862cdd3..9f6e3f7a6d 100644
> > --- a/target/riscv/machine.c
> > +++ b/target/riscv/machine.c
> > @@ -313,7 +313,7 @@ static bool pmu_needed(void *opaque)
> >  {
> >      RISCVCPU *cpu = opaque;
> > 
> > -    return cpu->cfg.pmu_num;
> > +    return (cpu->cfg.pmu_mask > 0);
> >  }
> > 
> >  static const VMStateDescription vmstate_pmu_ctr_state = {
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index 7ddf4977b1..5e89354bb9 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -18,6 +18,7 @@
> > 
> >  #include "qemu/osdep.h"
> >  #include "qemu/log.h"
> > +#include "qemu/error-report.h"
> >  #include "cpu.h"
> >  #include "pmu.h"
> >  #include "sysemu/cpu-timers.h"
> > @@ -184,7 +185,7 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum
> > riscv_pmu_event_idx event_idx)
> >      CPURISCVState *env = &cpu->env;
> >      gpointer value;
> > 
> > -    if (!cpu->cfg.pmu_num) {
> > +    if (!cpu->cfg.pmu_mask) {
> >          return 0;
> >      }
> >      value = g_hash_table_lookup(cpu->pmu_event_ctr_map,
> > @@ -434,7 +435,13 @@ void riscv_pmu_init(RISCVCPU *cpu, Error
> > **errp)
> >  {
> >      uint8_t pmu_num = cpu->cfg.pmu_num;
> > 
> > -    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> > +    if (cpu->cfg.pmu_mask & (COUNTEREN_CY | COUNTEREN_TM |
> > COUNTEREN_IR)) {
> > +        error_setg(errp, "\"pmu-mask\" contains invalid bits (0-2)
> > set");
> > +        return;
> > +    }
> > +
> > +    if (ctpop32(cpu->cfg.pmu_mask) > (RV_MAX_MHPMCOUNTERS - 3) ||
> > +        (pmu_num > RV_MAX_MHPMCOUNTERS - 3)) {
> >          error_setg(errp, "Number of counters exceeds maximum
> > available");
> >          return;
> >      }
> > @@ -445,6 +452,11 @@ 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);
> > +    if (pmu_num == 0) {
> > +        cpu->cfg.pmu_mask = 0;
> > +    } else if (pmu_num != 16) {
> 
> I think it's clearer if this is just an else instead of if not 16
> 
> > +        cpu->cfg.pmu_mask = MAKE_32BIT_MASK(3, pmu_num);
> > +    }
> 

Hi Alistair,

> Also doesn't think mean that the pmu-num prop will override the
> pmu-mask prop? We probably want to do it the other way around.

For transition it is necessary to override the "pmu-mask" value if the
user specifies "pmu-num" on the command line as the "pmu-mask" has a
default value set.

> Does setting pmu_mask directly in prop_pmu_num_set() work? Then we
> can
> drop pmu_num completely.
> 

This was a great suggestion and simplified the code somewhat - i've
done this in the latest version sent to the list.

Cheers,

Rob

> Alistair
> 
> > +
> > +    cpu->pmu_avail_ctrs = cpu->cfg.pmu_mask;
> >  }
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-
> > cpu.c
> > index ed3eb991c0..53c52389b9 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -613,7 +613,7 @@ static bool tcg_cpu_realize(CPUState *cs, Error
> > **errp)
> >          riscv_timer_init(cpu);
> >      }
> > 
> > -    if (cpu->cfg.pmu_num) {
> > +    if (cpu->cfg.pmu_mask) {
> >          riscv_pmu_init(cpu, &local_err);
> >          if (local_err != NULL) {
> >              error_propagate(errp, local_err);
> > --
> > 2.41.0
> > 
> >