On Thu, Oct 19, 2023 at 1:45 AM Rob Bradford <rbradf...@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 <rbradf...@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
>
>
