[PATCH v2 0/6] Support discontinuous PMU counters

[PATCH v2 0/6] Support discontinuous PMU counters

[Rob Bradford]

Currently the available PMU counters start at HPM3 and run through to
the number specified by the "pmu-num" property. There is no
requirement in the specification that the available counters be
continously numbered. This series add suppport for specifying a
discountinuous range of counters though a "pmu-mask" property.

v2:

* Use cfg.pmu_mask wherever cfg.pmu_num was used previously
* Deprecate pmu_num property (warning, comment & updated documentation)
* Override default pmu_mask value iff pmu_num changed from default

Rob Bradford (6):
  target/riscv: Propagate error from PMU setup
  target/riscv: Don't assume PMU counters are continuous
  target/riscv: Use existing PMU counter mask in FDT generation
  qemu/bitops.h: Add MAKE_32BIT_MASK macro
  target/riscv: Add "pmu-mask" property to replace "pmu-num"
  docs/about/deprecated: Document RISC-V "pmu-num" deprecation

 docs/about/deprecated.rst | 10 ++++++++++
 hw/riscv/virt.c           |  2 +-
 include/qemu/bitops.h     |  3 +++
 target/riscv/cpu.c        | 13 ++++++++++---
 target/riscv/cpu_cfg.h    |  3 ++-
 target/riscv/csr.c        |  5 +++--
 target/riscv/machine.c    |  2 +-
 target/riscv/pmu.c        | 35 +++++++++++++++++------------------
 target/riscv/pmu.h        |  5 +++--
 9 files changed, 50 insertions(+), 28 deletions(-)

-- 
2.41.0

[PATCH v2 1/6] target/riscv: Propagate error from PMU setup

[Rob Bradford]

More closely follow the QEMU style by returning an Error and propagating
it there is an error relating to the PMU setup.

Further simplify the function by removing the num_counters parameter as
this is available from the passed in cpu pointer.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c |  8 +++++++-
 target/riscv/pmu.c | 19 +++++++++----------
 target/riscv/pmu.h |  3 ++-
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ac2b94b6a6..c9d8fc12fe 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1488,7 +1488,13 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
     }
 
     if (cpu->cfg.pmu_num) {
-        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
+        riscv_pmu_init(cpu, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        if (cpu->cfg.ext_sscofpmf) {
             cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                           riscv_pmu_timer_cb, cpu);
         }
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 36f6307d28..13801ccb78 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -434,22 +434,21 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
 }
 
 
-int riscv_pmu_init(RISCVCPU *cpu, int num_counters)
+void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
 {
-    if (num_counters > (RV_MAX_MHPMCOUNTERS - 3)) {
-        return -1;
+    uint8_t pmu_num = cpu->cfg.pmu_num;
+
+    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
+        error_setg(errp, "Number of counters exceeds maximum available");
+        return;
     }
 
     cpu->pmu_event_ctr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
     if (!cpu->pmu_event_ctr_map) {
-        /* PMU support can not be enabled */
-        qemu_log_mask(LOG_UNIMP, "PMU events can't be supported\n");
-        cpu->cfg.pmu_num = 0;
-        return -1;
+        error_setg(errp, "Unable to allocate PMU event hash table");
+        return;
     }
 
     /* Create a bitmask of available programmable counters */
-    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, num_counters);
-
-    return 0;
+    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
 }
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 2bfb71ba87..88e0713296 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -17,13 +17,14 @@
  */
 
 #include "cpu.h"
+#include "qapi/error.h"
 
 bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
                                         uint32_t target_ctr);
 bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env,
                                   uint32_t target_ctr);
 void riscv_pmu_timer_cb(void *priv);
-int riscv_pmu_init(RISCVCPU *cpu, int num_counters);
+void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
 int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
                                uint32_t ctr_idx);
 int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
-- 
2.41.0

[PATCH v2 2/6] target/riscv: Don't assume PMU counters are continuous

[Rob Bradford]

Check the PMU available bitmask when checking if a counter is valid
rather than comparing the index against the number of PMUs.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 target/riscv/csr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 85a31dc420..3e126219ba 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -182,7 +182,8 @@ static RISCVException zcmt(CPURISCVState *env, int csrno)
 #if !defined(CONFIG_USER_ONLY)
 static RISCVException mctr(CPURISCVState *env, int csrno)
 {
-    int pmu_num = riscv_cpu_cfg(env)->pmu_num;
+    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
+    uint32_t pmu_avail_ctrs = cpu->pmu_avail_ctrs;
     int ctr_index;
     int base_csrno = CSR_MHPMCOUNTER3;
 
@@ -191,7 +192,7 @@ static RISCVException mctr(CPURISCVState *env, int csrno)
         base_csrno += 0x80;
     }
     ctr_index = csrno - base_csrno;
-    if (!pmu_num || ctr_index >= pmu_num) {
+    if ((BIT(ctr_index) & pmu_avail_ctrs >> 3) == 0) {
         /* The PMU is not enabled or counter is out of range */
         return RISCV_EXCP_ILLEGAL_INST;
     }
-- 
2.41.0

[PATCH v2 3/6] target/riscv: Use existing PMU counter mask in FDT generation

[Rob Bradford]

During the FDT generation use the existing mask containing the enabled
counters rather then generating a new one. Using the existing mask will
support the use of discontinuous counters.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 hw/riscv/virt.c    | 2 +-
 target/riscv/pmu.c | 6 +-----
 target/riscv/pmu.h | 2 +-
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 5edc1d98d2..acdbaf9da5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -722,7 +722,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
     pmu_name = g_strdup_printf("/pmu");
     qemu_fdt_add_subnode(ms->fdt, pmu_name);
     qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
-    riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
+    riscv_pmu_generate_fdt_node(ms->fdt, hart.pmu_avail_ctrs, pmu_name);
 
     g_free(pmu_name);
 }
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 13801ccb78..7ddf4977b1 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -34,13 +34,9 @@
  * to provide the correct value as well. Heterogeneous PMU per hart is not
  * supported yet. Thus, number of counters are same across all harts.
  */
-void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
+void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name)
 {
     uint32_t fdt_event_ctr_map[15] = {};
-    uint32_t cmask;
-
-    /* All the programmable counters can map to any event */
-    cmask = MAKE_32BIT_MASK(3, num_ctrs);
 
    /*
     * The event encoding is specified in the SBI specification
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 88e0713296..505fc850d3 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -28,6 +28,6 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
 int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
                                uint32_t ctr_idx);
 int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
-void riscv_pmu_generate_fdt_node(void *fdt, int num_counters, char *pmu_name);
+void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name);
 int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value,
                           uint32_t ctr_idx);
-- 
2.41.0

[PATCH v2 4/6] qemu/bitops.h: Add MAKE_32BIT_MASK macro

[Rob Bradford]

Add 32-bit version of mask generating macro and use it in the RISC-V PMU
code.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 include/qemu/bitops.h | 3 +++
 target/riscv/pmu.c    | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index cb3526d1f4..9b25b2d5e4 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -25,6 +25,9 @@
 #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
 #define BITS_TO_LONGS(nr)       DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
 
+#define MAKE_32BIT_MASK(shift, length) \
+    (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
+
 #define MAKE_64BIT_MASK(shift, length) \
     (((~0ULL) >> (64 - (length))) << (shift))
 
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 7ddf4977b1..360c76f63e 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -24,8 +24,6 @@
 #include "sysemu/device_tree.h"
 
 #define RISCV_TIMEBASE_FREQ 1000000000 /* 1Ghz */
-#define MAKE_32BIT_MASK(shift, length) \
-        (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
 
 /*
  * To keep it simple, any event can be mapped to any programmable counters in
-- 
2.41.0

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

[Rob Bradford]

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;
 }
-- 
2.41.0

[PATCH v2 6/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation

[Rob Bradford]

This has been replaced by a "pmu-mask" property that provides much more
flexibility.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 docs/about/deprecated.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 8b136320e2..37f3414ef8 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -361,6 +361,16 @@ Specifying the iSCSI password in plain text on the command line using the
 used instead, to refer to a ``--object secret...`` instance that provides
 a password via a file, or encrypted.
 
+CPU device properties
+'''''''''''''''''''''
+
+``pmu-num=x`` on RISC-V CPUs (since 8.2)
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+In order to support more flexible counter configurations this has been
+replaced by a ``pmu-mask`` property
+
+
 Backwards compatibility
 -----------------------
 
-- 
2.41.0

Re: [PATCH v2 1/6] target/riscv: Propagate error from PMU setup

[LIU Zhiwei]

On 2023/10/11 22:45, Rob Bradford wrote:
> More closely follow the QEMU style by returning an Error and propagating
> it there is an error relating to the PMU setup.
>
> Further simplify the function by removing the num_counters parameter as
> this is available from the passed in cpu pointer.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/cpu.c |  8 +++++++-
>   target/riscv/pmu.c | 19 +++++++++----------
>   target/riscv/pmu.h |  3 ++-
>   3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ac2b94b6a6..c9d8fc12fe 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1488,7 +1488,13 @@ static void riscv_cpu_realize_tcg(DeviceState *dev, Error **errp)
>       }
>   
>       if (cpu->cfg.pmu_num) {
> -        if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
> +        riscv_pmu_init(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +
> +        if (cpu->cfg.ext_sscofpmf) {
>               cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                             riscv_pmu_timer_cb, cpu);
>           }
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 36f6307d28..13801ccb78 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -434,22 +434,21 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
>   }
>   
>   
> -int riscv_pmu_init(RISCVCPU *cpu, int num_counters)
> +void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
>   {
> -    if (num_counters > (RV_MAX_MHPMCOUNTERS - 3)) {
> -        return -1;
> +    uint8_t pmu_num = cpu->cfg.pmu_num;
> +
> +    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> +        error_setg(errp, "Number of counters exceeds maximum available");
> +        return;
>       }
>   
>       cpu->pmu_event_ctr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
>       if (!cpu->pmu_event_ctr_map) {
> -        /* PMU support can not be enabled */
> -        qemu_log_mask(LOG_UNIMP, "PMU events can't be supported\n");
> -        cpu->cfg.pmu_num = 0;
> -        return -1;
> +        error_setg(errp, "Unable to allocate PMU event hash table");
> +        return;
>       }
>   
>       /* Create a bitmask of available programmable counters */
> -    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, num_counters);
> -
> -    return 0;
> +    cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
>   }
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 2bfb71ba87..88e0713296 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -17,13 +17,14 @@
>    */
>   
>   #include "cpu.h"
> +#include "qapi/error.h"
>   
>   bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
>                                           uint32_t target_ctr);
>   bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env,
>                                     uint32_t target_ctr);
>   void riscv_pmu_timer_cb(void *priv);
> -int riscv_pmu_init(RISCVCPU *cpu, int num_counters);
> +void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>                                  uint32_t ctr_idx);
>   int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);

Re: [PATCH v2 3/6] target/riscv: Use existing PMU counter mask in FDT generation

[LIU Zhiwei]

On 2023/10/11 22:45, Rob Bradford wrote:
> During the FDT generation use the existing mask containing the enabled
> counters rather then generating a new one. Using the existing mask will
> support the use of discontinuous counters.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>   hw/riscv/virt.c    | 2 +-
>   target/riscv/pmu.c | 6 +-----
>   target/riscv/pmu.h | 2 +-
>   3 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 5edc1d98d2..acdbaf9da5 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -722,7 +722,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
>       pmu_name = g_strdup_printf("/pmu");
>       qemu_fdt_add_subnode(ms->fdt, pmu_name);
>       qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
> -    riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
> +    riscv_pmu_generate_fdt_node(ms->fdt, hart.pmu_avail_ctrs, pmu_name);
>   
>       g_free(pmu_name);
>   }
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 13801ccb78..7ddf4977b1 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -34,13 +34,9 @@
>    * to provide the correct value as well. Heterogeneous PMU per hart is not
>    * supported yet. Thus, number of counters are same across all harts.
>    */
> -void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
> +void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name)
>   {
>       uint32_t fdt_event_ctr_map[15] = {};
> -    uint32_t cmask;
> -
> -    /* All the programmable counters can map to any event */
> -    cmask = MAKE_32BIT_MASK(3, num_ctrs);
>   
>      /*
>       * The event encoding is specified in the SBI specification
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 88e0713296..505fc850d3 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -28,6 +28,6 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
>   int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>                                  uint32_t ctr_idx);
>   int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
> -void riscv_pmu_generate_fdt_node(void *fdt, int num_counters, char *pmu_name);
> +void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name);

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value,
>                             uint32_t ctr_idx);

Re: [PATCH v2 4/6] qemu/bitops.h: Add MAKE_32BIT_MASK macro

[LIU Zhiwei]

On 2023/10/11 22:45, Rob Bradford wrote:
> Add 32-bit version of mask generating macro and use it in the RISC-V PMU
> code.
CC Richard
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>   include/qemu/bitops.h | 3 +++
>   target/riscv/pmu.c    | 2 --
>   2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index cb3526d1f4..9b25b2d5e4 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h
> @@ -25,6 +25,9 @@
>   #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
>   #define BITS_TO_LONGS(nr)       DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
>   
> +#define MAKE_32BIT_MASK(shift, length) \
> +    (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
> +
>   #define MAKE_64BIT_MASK(shift, length) \
>       (((~0ULL) >> (64 - (length))) << (shift))
>   
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 7ddf4977b1..360c76f63e 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -24,8 +24,6 @@
>   #include "sysemu/device_tree.h"
>   
>   #define RISCV_TIMEBASE_FREQ 1000000000 /* 1Ghz */
> -#define MAKE_32BIT_MASK(shift, length) \
> -        (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
>   

We can always use the MAKE_64BIT_MASK instead of MAKE_32BIT_MASK.  And 
MAKE_32BIT_MASK only used in target/riscv. I am not sure  whether this 
patch will be accepted.

Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   /*
>    * To keep it simple, any event can be mapped to any programmable counters in

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

[LIU Zhiwei]

[-- Attachment #1: Type: text/plain, Size: 4229 bytes --]


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.

Zhiwei

>   }

[-- Attachment #2: Type: text/html, Size: 4921 bytes --]

Re: [PATCH v2 6/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation

[LIU Zhiwei]

On 2023/10/11 22:45, Rob Bradford wrote:
> This has been replaced by a "pmu-mask" property that provides much more
> flexibility.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>   docs/about/deprecated.rst | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 8b136320e2..37f3414ef8 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -361,6 +361,16 @@ Specifying the iSCSI password in plain text on the command line using the
>   used instead, to refer to a ``--object secret...`` instance that provides
>   a password via a file, or encrypted.
>   
> +CPU device properties
> +'''''''''''''''''''''
> +
> +``pmu-num=x`` on RISC-V CPUs (since 8.2)
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +In order to support more flexible counter configurations this has been
> +replaced by a ``pmu-mask`` property
> +
> +

Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   Backwards compatibility
>   -----------------------
>   

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

[Rob Bradford]

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.

Cheers,

Rob

> Zhiwei
>  
>  
> >  
> >  }
> >  
>  

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

Re: [PATCH v2 2/6] target/riscv: Don't assume PMU counters are continuous

[Alistair Francis]

On Thu, Oct 12, 2023 at 12:52 AM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> Check the PMU available bitmask when checking if a counter is valid
> rather than comparing the index against the number of PMUs.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/csr.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 85a31dc420..3e126219ba 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -182,7 +182,8 @@ static RISCVException zcmt(CPURISCVState *env, int csrno)
>  #if !defined(CONFIG_USER_ONLY)
>  static RISCVException mctr(CPURISCVState *env, int csrno)
>  {
> -    int pmu_num = riscv_cpu_cfg(env)->pmu_num;
> +    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
> +    uint32_t pmu_avail_ctrs = cpu->pmu_avail_ctrs;
>      int ctr_index;
>      int base_csrno = CSR_MHPMCOUNTER3;
>
> @@ -191,7 +192,7 @@ static RISCVException mctr(CPURISCVState *env, int csrno)
>          base_csrno += 0x80;
>      }
>      ctr_index = csrno - base_csrno;
> -    if (!pmu_num || ctr_index >= pmu_num) {
> +    if ((BIT(ctr_index) & pmu_avail_ctrs >> 3) == 0) {
>          /* The PMU is not enabled or counter is out of range */
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
> --
> 2.41.0
>
>

Re: [PATCH v2 3/6] target/riscv: Use existing PMU counter mask in FDT generation

[Alistair Francis]

On Thu, Oct 12, 2023 at 2:37 AM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> During the FDT generation use the existing mask containing the enabled
> counters rather then generating a new one. Using the existing mask will
> support the use of discontinuous counters.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c    | 2 +-
>  target/riscv/pmu.c | 6 +-----
>  target/riscv/pmu.h | 2 +-
>  3 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 5edc1d98d2..acdbaf9da5 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -722,7 +722,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
>      pmu_name = g_strdup_printf("/pmu");
>      qemu_fdt_add_subnode(ms->fdt, pmu_name);
>      qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
> -    riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
> +    riscv_pmu_generate_fdt_node(ms->fdt, hart.pmu_avail_ctrs, pmu_name);
>
>      g_free(pmu_name);
>  }
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 13801ccb78..7ddf4977b1 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -34,13 +34,9 @@
>   * to provide the correct value as well. Heterogeneous PMU per hart is not
>   * supported yet. Thus, number of counters are same across all harts.
>   */
> -void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs, char *pmu_name)
> +void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name)
>  {
>      uint32_t fdt_event_ctr_map[15] = {};
> -    uint32_t cmask;
> -
> -    /* All the programmable counters can map to any event */
> -    cmask = MAKE_32BIT_MASK(3, num_ctrs);
>
>     /*
>      * The event encoding is specified in the SBI specification
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 88e0713296..505fc850d3 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -28,6 +28,6 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
>  int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>                                 uint32_t ctr_idx);
>  int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
> -void riscv_pmu_generate_fdt_node(void *fdt, int num_counters, char *pmu_name);
> +void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name);
>  int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value,
>                            uint32_t ctr_idx);
> --
> 2.41.0
>
>

Re: [PATCH v2 4/6] qemu/bitops.h: Add MAKE_32BIT_MASK macro

[Alistair Francis]

On Thu, Oct 12, 2023 at 6:53 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
>
> On 2023/10/11 22:45, Rob Bradford wrote:
> > Add 32-bit version of mask generating macro and use it in the RISC-V PMU
> > code.
> CC Richard
> > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > ---
> >   include/qemu/bitops.h | 3 +++
> >   target/riscv/pmu.c    | 2 --
> >   2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> > index cb3526d1f4..9b25b2d5e4 100644
> > --- a/include/qemu/bitops.h
> > +++ b/include/qemu/bitops.h
> > @@ -25,6 +25,9 @@
> >   #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
> >   #define BITS_TO_LONGS(nr)       DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
> >
> > +#define MAKE_32BIT_MASK(shift, length) \
> > +    (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
> > +
> >   #define MAKE_64BIT_MASK(shift, length) \
> >       (((~0ULL) >> (64 - (length))) << (shift))
> >
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index 7ddf4977b1..360c76f63e 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -24,8 +24,6 @@
> >   #include "sysemu/device_tree.h"
> >
> >   #define RISCV_TIMEBASE_FREQ 1000000000 /* 1Ghz */
> > -#define MAKE_32BIT_MASK(shift, length) \
> > -        (((uint32_t)(~0UL) >> (32 - (length))) << (shift))
> >
>
> We can always use the MAKE_64BIT_MASK instead of MAKE_32BIT_MASK.  And
> MAKE_32BIT_MASK only used in target/riscv. I am not sure  whether this
> patch will be accepted.

Good point, can we use MAKE_64BIT_MASK instead?

Alistair

>
> Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>
> Zhiwei
>
> >   /*
> >    * To keep it simple, any event can be mapped to any programmable counters in
>

Re: [PATCH v2 6/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation

[Alistair Francis]

On Thu, Oct 12, 2023 at 12:52 AM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> This has been replaced by a "pmu-mask" property that provides much more
> flexibility.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>  docs/about/deprecated.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 8b136320e2..37f3414ef8 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -361,6 +361,16 @@ Specifying the iSCSI password in plain text on the command line using the
>  used instead, to refer to a ``--object secret...`` instance that provides
>  a password via a file, or encrypted.
>
> +CPU device properties
> +'''''''''''''''''''''
> +
> +``pmu-num=x`` on RISC-V CPUs (since 8.2)
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +In order to support more flexible counter configurations this has been
> +replaced by a ``pmu-mask`` property

Can we give an example of how to migrate to pmu-mask?

Alistair

> +
> +
>  Backwards compatibility
>  -----------------------
>
> --
> 2.41.0
>
>