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

[LIU Zhiwei]

On 2023/10/12 20:38, Rob Bradford wrote:
> On Thu, 2023-10-12 at 17:05 +0800, LIU Zhiwei wrote:
> >   
> >
> >   
> >   
> > On 2023/10/11 22:45, Rob Bradford 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.
> > >
> > > Generate a warning if the old property changed from the default but
> > > still go ahead and use it to generate the mask if the user has
> > > changed
> > > it from the default
> > >
> > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > ---
> > >   target/riscv/cpu.c     |  5 +++--
> > >   target/riscv/cpu_cfg.h |  3 ++-
> > >   target/riscv/machine.c |  2 +-
> > >   target/riscv/pmu.c     | 14 ++++++++++----
> > >   4 files changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index c9d8fc12fe..4d2987e568 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1487,7 +1487,7 @@ static void riscv_cpu_realize_tcg(DeviceState
> > > *dev, 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);
> > > @@ -1812,7 +1812,8 @@ 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_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16), /*
> > > Deprecated */
> > > +    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask,
> > > MAKE_32BIT_MASK(3, 16)),
> > >       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..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 360c76f63e..f2d35b4d3b 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"
> > > @@ -182,7 +183,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,
> > > @@ -432,7 +433,7 @@ void riscv_pmu_init(RISCVCPU *cpu, Error
> > > **errp)
> > >   {
> > >       uint8_t pmu_num = cpu->cfg.pmu_num;
> > >   
> > > -    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> > > +    if (ctpop32(cpu->cfg.pmu_mask) > (RV_MAX_MHPMCOUNTERS - 3)) {
> > >   
> >   
> >   
> > >   
> > >           error_setg(errp, "Number of counters exceeds maximum
> > > available");
> > >           return;
> > >       }
> > > @@ -443,6 +444,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);
> > > +    /* Check if user set it by comparing against default */
> > > +    if (pmu_num != 16) {
> > > +        warn_report("\"pmu-num\" property is deprecated; use
> > > \"pmu-mask\"");
> > > +        cpu->cfg.pmu_mask = MAKE_32BIT_MASK(3, pmu_num);
> > > +    }
> > > +
> > > +    cpu->pmu_avail_ctrs = cpu->cfg.pmu_mask;
> > >   
> >   
> > How to process the pmu_mask[0:2] no-zero bits? They should not
> > included into pmu_avail_ctrs.
> >   
> Good point, thanks Zhiwei. I think rather than masking those bits it is
> better to add a check and error out if those bits are set.

Agree.

Thanks,
Zhiwei

> Cheers,
>
> Rob
>
> > Zhiwei
> >   
> >   
> > >   
> > >   }
> > >   
> >