| Message ID | 20231018154434.17367-5-rbradford@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support discontinuous PMU counters | expand |
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); > + } 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. Does setting pmu_mask directly in prop_pmu_num_set() work? Then we can drop pmu_num completely. 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 > >
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 > > > >
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) { + cpu->cfg.pmu_mask = MAKE_32BIT_MASK(3, pmu_num); + } + + 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);
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(-)