* [PATCH v4 0/6] Support discontinuous PMU counters
@ 2023-10-18 15:39 Rob Bradford
2023-10-18 15:39 ` [PATCH v4 1/6] target/riscv: Propagate error from PMU setup Rob Bradford
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Rob Bradford @ 2023-10-18 15:39 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, atishp, palmer, alistair.francis, bin.meng, liweiwei,
dbarboza, zhiwei_liu, 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.
v4:
* Added more R-B tags (just missing from 4 & 5)
* Added details on how to calculate mask
* Use custom property for "pmu-num" in order to give deprecation warning
* Special case a zero value for "pmu-num"
v3:
* Use env_archcpu() in csr.c
* Re-added check to enforce deprectated "num-pmu" below limit
* Check that standard counters are not included in mask
* Remove use of MAKE_32BIT_MASK()
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
target/riscv: Add "pmu-mask" property to replace "pmu-num"
docs/about/deprecated: Document RISC-V "pmu-num" deprecation
target/riscv: Use MAKE_64BIT_MASK instead of custom macro
docs/about/deprecated.rst | 12 +++++++++++
hw/riscv/virt.c | 2 +-
target/riscv/cpu.c | 31 +++++++++++++++++++++++++++-
target/riscv/cpu_cfg.h | 3 ++-
target/riscv/csr.c | 5 +++--
target/riscv/machine.c | 2 +-
target/riscv/pmu.c | 41 +++++++++++++++++++++-----------------
target/riscv/pmu.h | 5 +++--
target/riscv/tcg/tcg-cpu.c | 10 ++++++++--
9 files changed, 83 insertions(+), 28 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 1/6] target/riscv: Propagate error from PMU setup
2023-10-18 15:39 [PATCH v4 0/6] Support discontinuous PMU counters Rob Bradford
@ 2023-10-18 15:39 ` Rob Bradford
2023-10-24 0:03 ` Atish Kumar Patra
2023-10-18 15:39 ` [PATCH v4 2/6] target/riscv: Don't assume PMU counters are continuous Rob Bradford
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Rob Bradford @ 2023-10-18 15:39 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, atishp, palmer, alistair.francis, bin.meng, liweiwei,
dbarboza, zhiwei_liu, 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>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
target/riscv/pmu.c | 19 +++++++++----------
target/riscv/pmu.h | 3 ++-
target/riscv/tcg/tcg-cpu.c | 8 +++++++-
3 files changed, 18 insertions(+), 12 deletions(-)
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);
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index a28918ab30..ed3eb991c0 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -614,7 +614,13 @@ static bool tcg_cpu_realize(CPUState *cs, 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 false;
+ }
+
+ if (cpu->cfg.ext_sscofpmf) {
cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
riscv_pmu_timer_cb, cpu);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 2/6] target/riscv: Don't assume PMU counters are continuous
2023-10-18 15:39 [PATCH v4 0/6] Support discontinuous PMU counters Rob Bradford
2023-10-18 15:39 ` [PATCH v4 1/6] target/riscv: Propagate error from PMU setup Rob Bradford
@ 2023-10-18 15:39 ` Rob Bradford
2023-10-24 0:05 ` Atish Kumar Patra
2023-10-18 15:39 ` [PATCH v4 3/6] target/riscv: Use existing PMU counter mask in FDT generation Rob Bradford
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Rob Bradford @ 2023-10-18 15:39 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, atishp, palmer, alistair.francis, bin.meng, liweiwei,
dbarboza, zhiwei_liu, 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>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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 4b4ab56c40..a6ea38e0ba 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -183,7 +183,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 = env_archcpu(env);
+ uint32_t pmu_avail_ctrs = cpu->pmu_avail_ctrs;
int ctr_index;
int base_csrno = CSR_MHPMCOUNTER3;
@@ -192,7 +193,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 3/6] target/riscv: Use existing PMU counter mask in FDT generation
2023-10-18 15:39 [PATCH v4 0/6] Support discontinuous PMU counters Rob Bradford
2023-10-18 15:39 ` [PATCH v4 1/6] target/riscv: Propagate error from PMU setup Rob Bradford
2023-10-18 15:39 ` [PATCH v4 2/6] target/riscv: Don't assume PMU counters are continuous Rob Bradford
@ 2023-10-18 15:39 ` Rob Bradford
2023-10-24 0:06 ` Atish Kumar Patra
2023-10-18 15:39 ` [PATCH v4 4/6] target/riscv: Add "pmu-mask" property to replace "pmu-num" Rob Bradford
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Rob Bradford @ 2023-10-18 15:39 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, atishp, palmer, alistair.francis, bin.meng, liweiwei,
dbarboza, zhiwei_liu, 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>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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 9de578c756..241681f98d 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 4/6] target/riscv: Add "pmu-mask" property to replace "pmu-num"
2023-10-18 15:39 [PATCH v4 0/6] Support discontinuous PMU counters Rob Bradford
` (2 preceding siblings ...)
2023-10-18 15:39 ` [PATCH v4 3/6] target/riscv: Use existing PMU counter mask in FDT generation Rob Bradford
@ 2023-10-18 15:39 ` Rob Bradford
2023-10-23 2:02 ` Alistair Francis
2023-10-18 15:39 ` [PATCH v4 5/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation Rob Bradford
2023-10-18 15:39 ` [PATCH v4 6/6] target/riscv: Use MAKE_64BIT_MASK instead of custom macro Rob Bradford
5 siblings, 1 reply; 15+ messages in thread
From: Rob Bradford @ 2023-10-18 15:39 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, atishp, palmer, alistair.francis, bin.meng, liweiwei,
dbarboza, zhiwei_liu, 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.
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) {
+ 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);
--
2.41.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 5/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation
2023-10-18 15:39 [PATCH v4 0/6] Support discontinuous PMU counters Rob Bradford
` (3 preceding siblings ...)
2023-10-18 15:39 ` [PATCH v4 4/6] target/riscv: Add "pmu-mask" property to replace "pmu-num" Rob Bradford
@ 2023-10-18 15:39 ` Rob Bradford
2023-10-23 2:04 ` Alistair Francis
2023-10-18 15:39 ` [PATCH v4 6/6] target/riscv: Use MAKE_64BIT_MASK instead of custom macro Rob Bradford
5 siblings, 1 reply; 15+ messages in thread
From: Rob Bradford @ 2023-10-18 15:39 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, atishp, palmer, alistair.francis, bin.meng, liweiwei,
dbarboza, zhiwei_liu, Rob Bradford, reviewer:Incompatible changes
This has been replaced by a "pmu-mask" property that provides much more
flexibility.
Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
docs/about/deprecated.rst | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 2febd2d12f..857b5d4fc4 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -405,6 +405,18 @@ 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=n`` on RISC-V CPUs (since 8.2)
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+In order to support more flexible counter configurations this has been replaced
+by a ``pmu-mask`` property. If set of counters is continuous then the mask can
+be calculated with ``((2 ^ n) - 1) << 3``. The least significant three bits
+must be left clear.
+
+
Backwards compatibility
-----------------------
--
2.41.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v4 6/6] target/riscv: Use MAKE_64BIT_MASK instead of custom macro
2023-10-18 15:39 [PATCH v4 0/6] Support discontinuous PMU counters Rob Bradford
` (4 preceding siblings ...)
2023-10-18 15:39 ` [PATCH v4 5/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation Rob Bradford
@ 2023-10-18 15:39 ` Rob Bradford
2023-10-24 0:31 ` Atish Kumar Patra
5 siblings, 1 reply; 15+ messages in thread
From: Rob Bradford @ 2023-10-18 15:39 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, atishp, palmer, alistair.francis, bin.meng, liweiwei,
dbarboza, zhiwei_liu, Rob Bradford
A 32-bit mask can be trivially created using the 64-bit macro so make
use of that instead.
Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/pmu.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 5e89354bb9..81b25ec11a 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -25,8 +25,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
@@ -455,7 +453,7 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
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->cfg.pmu_mask = MAKE_64BIT_MASK(3, pmu_num);
}
cpu->pmu_avail_ctrs = cpu->cfg.pmu_mask;
--
2.41.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4 4/6] target/riscv: Add "pmu-mask" property to replace "pmu-num"
2023-10-18 15:39 ` [PATCH v4 4/6] target/riscv: Add "pmu-mask" property to replace "pmu-num" Rob Bradford
@ 2023-10-23 2:02 ` Alistair Francis
2023-10-31 15:43 ` Rob Bradford
0 siblings, 1 reply; 15+ messages in thread
From: Alistair Francis @ 2023-10-23 2:02 UTC (permalink / raw)
To: Rob Bradford
Cc: qemu-devel, qemu-riscv, atishp, palmer, alistair.francis,
bin.meng, liweiwei, dbarboza, zhiwei_liu
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
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation
2023-10-18 15:39 ` [PATCH v4 5/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation Rob Bradford
@ 2023-10-23 2:04 ` Alistair Francis
2023-10-24 0:30 ` Atish Kumar Patra
0 siblings, 1 reply; 15+ messages in thread
From: Alistair Francis @ 2023-10-23 2:04 UTC (permalink / raw)
To: Rob Bradford
Cc: qemu-devel, qemu-riscv, atishp, palmer, alistair.francis,
bin.meng, liweiwei, dbarboza, zhiwei_liu,
reviewer:Incompatible changes
On Thu, Oct 19, 2023 at 1:47 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>
> Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> docs/about/deprecated.rst | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 2febd2d12f..857b5d4fc4 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -405,6 +405,18 @@ 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=n`` on RISC-V CPUs (since 8.2)
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +In order to support more flexible counter configurations this has been replaced
> +by a ``pmu-mask`` property. If set of counters is continuous then the mask can
> +be calculated with ``((2 ^ n) - 1) << 3``. The least significant three bits
> +must be left clear.
> +
> +
> Backwards compatibility
> -----------------------
>
> --
> 2.41.0
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/6] target/riscv: Propagate error from PMU setup
2023-10-18 15:39 ` [PATCH v4 1/6] target/riscv: Propagate error from PMU setup Rob Bradford
@ 2023-10-24 0:03 ` Atish Kumar Patra
0 siblings, 0 replies; 15+ messages in thread
From: Atish Kumar Patra @ 2023-10-24 0:03 UTC (permalink / raw)
To: Rob Bradford
Cc: qemu-devel, qemu-riscv, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu
On Wed, Oct 18, 2023 at 8:44 AM Rob Bradford <rbradford@rivosinc.com> 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>
> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
> target/riscv/pmu.c | 19 +++++++++----------
> target/riscv/pmu.h | 3 ++-
> target/riscv/tcg/tcg-cpu.c | 8 +++++++-
> 3 files changed, 18 insertions(+), 12 deletions(-)
>
> 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);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index a28918ab30..ed3eb991c0 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -614,7 +614,13 @@ static bool tcg_cpu_realize(CPUState *cs, 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 false;
> + }
> +
> + if (cpu->cfg.ext_sscofpmf) {
> cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> riscv_pmu_timer_cb, cpu);
> }
> --
> 2.41.0
>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 2/6] target/riscv: Don't assume PMU counters are continuous
2023-10-18 15:39 ` [PATCH v4 2/6] target/riscv: Don't assume PMU counters are continuous Rob Bradford
@ 2023-10-24 0:05 ` Atish Kumar Patra
0 siblings, 0 replies; 15+ messages in thread
From: Atish Kumar Patra @ 2023-10-24 0:05 UTC (permalink / raw)
To: Rob Bradford
Cc: qemu-devel, qemu-riscv, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu
On Wed, Oct 18, 2023 at 8:44 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: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.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 4b4ab56c40..a6ea38e0ba 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -183,7 +183,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 = env_archcpu(env);
> + uint32_t pmu_avail_ctrs = cpu->pmu_avail_ctrs;
> int ctr_index;
> int base_csrno = CSR_MHPMCOUNTER3;
>
> @@ -192,7 +193,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
>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/6] target/riscv: Use existing PMU counter mask in FDT generation
2023-10-18 15:39 ` [PATCH v4 3/6] target/riscv: Use existing PMU counter mask in FDT generation Rob Bradford
@ 2023-10-24 0:06 ` Atish Kumar Patra
0 siblings, 0 replies; 15+ messages in thread
From: Atish Kumar Patra @ 2023-10-24 0:06 UTC (permalink / raw)
To: Rob Bradford
Cc: qemu-devel, qemu-riscv, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu
On Wed, Oct 18, 2023 at 8:44 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: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.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 9de578c756..241681f98d 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
>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 5/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation
2023-10-23 2:04 ` Alistair Francis
@ 2023-10-24 0:30 ` Atish Kumar Patra
0 siblings, 0 replies; 15+ messages in thread
From: Atish Kumar Patra @ 2023-10-24 0:30 UTC (permalink / raw)
To: Alistair Francis
Cc: Rob Bradford, qemu-devel, qemu-riscv, palmer, alistair.francis,
bin.meng, liweiwei, dbarboza, zhiwei_liu,
reviewer:Incompatible changes
On Sun, Oct 22, 2023 at 7:04 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Oct 19, 2023 at 1:47 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>
> > Acked-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
> > ---
> > docs/about/deprecated.rst | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index 2febd2d12f..857b5d4fc4 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -405,6 +405,18 @@ 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=n`` on RISC-V CPUs (since 8.2)
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +In order to support more flexible counter configurations this has been replaced
> > +by a ``pmu-mask`` property. If set of counters is continuous then the mask can
> > +be calculated with ``((2 ^ n) - 1) << 3``. The least significant three bits
> > +must be left clear.
> > +
> > +
> > Backwards compatibility
> > -----------------------
> >
> > --
> > 2.41.0
> >
> >
Reviewed-by: Atish Patra <atishp@rivosinc.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 6/6] target/riscv: Use MAKE_64BIT_MASK instead of custom macro
2023-10-18 15:39 ` [PATCH v4 6/6] target/riscv: Use MAKE_64BIT_MASK instead of custom macro Rob Bradford
@ 2023-10-24 0:31 ` Atish Kumar Patra
0 siblings, 0 replies; 15+ messages in thread
From: Atish Kumar Patra @ 2023-10-24 0:31 UTC (permalink / raw)
To: Rob Bradford
Cc: qemu-devel, qemu-riscv, palmer, alistair.francis, bin.meng,
liweiwei, dbarboza, zhiwei_liu
On Wed, Oct 18, 2023 at 8:44 AM Rob Bradford <rbradford@rivosinc.com> wrote:
>
> A 32-bit mask can be trivially created using the 64-bit macro so make
> use of that instead.
>
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> target/riscv/pmu.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 5e89354bb9..81b25ec11a 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -25,8 +25,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
> @@ -455,7 +453,7 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> 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->cfg.pmu_mask = MAKE_64BIT_MASK(3, pmu_num);
> }
>
> cpu->pmu_avail_ctrs = cpu->cfg.pmu_mask;
> --
> 2.41.0
>
Reviewed-by: Atish Patra <atishp@rivosinc.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 4/6] target/riscv: Add "pmu-mask" property to replace "pmu-num"
2023-10-23 2:02 ` Alistair Francis
@ 2023-10-31 15:43 ` Rob Bradford
0 siblings, 0 replies; 15+ messages in thread
From: Rob Bradford @ 2023-10-31 15:43 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel, qemu-riscv, atishp, palmer, alistair.francis,
bin.meng, liweiwei, dbarboza, zhiwei_liu
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
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-10-31 15:43 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-18 15:39 [PATCH v4 0/6] Support discontinuous PMU counters Rob Bradford
2023-10-18 15:39 ` [PATCH v4 1/6] target/riscv: Propagate error from PMU setup Rob Bradford
2023-10-24 0:03 ` Atish Kumar Patra
2023-10-18 15:39 ` [PATCH v4 2/6] target/riscv: Don't assume PMU counters are continuous Rob Bradford
2023-10-24 0:05 ` Atish Kumar Patra
2023-10-18 15:39 ` [PATCH v4 3/6] target/riscv: Use existing PMU counter mask in FDT generation Rob Bradford
2023-10-24 0:06 ` Atish Kumar Patra
2023-10-18 15:39 ` [PATCH v4 4/6] target/riscv: Add "pmu-mask" property to replace "pmu-num" Rob Bradford
2023-10-23 2:02 ` Alistair Francis
2023-10-31 15:43 ` Rob Bradford
2023-10-18 15:39 ` [PATCH v4 5/6] docs/about/deprecated: Document RISC-V "pmu-num" deprecation Rob Bradford
2023-10-23 2:04 ` Alistair Francis
2023-10-24 0:30 ` Atish Kumar Patra
2023-10-18 15:39 ` [PATCH v4 6/6] target/riscv: Use MAKE_64BIT_MASK instead of custom macro Rob Bradford
2023-10-24 0:31 ` Atish Kumar Patra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).