target/riscv/cpu.h | 9 +++ target/riscv/csr.c | 43 +++++++++----- target/riscv/pmu.c | 139 ++++++++++++++++++++++----------------------- target/riscv/pmu.h | 11 ++-- 4 files changed, 115 insertions(+), 87 deletions(-)
Was added call backs for machine specific pmu events.
Simplify monitor functions by adding new hash table, which going to map
counter number and event index.
Was added read/write callbacks which going to simplify support for events,
which expected to have different behavior.
Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com>
---
Changes since v2:
-rebased to latest master
target/riscv/cpu.h | 9 +++
target/riscv/csr.c | 43 +++++++++-----
target/riscv/pmu.c | 139 ++++++++++++++++++++++-----------------------
target/riscv/pmu.h | 11 ++--
4 files changed, 115 insertions(+), 87 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 6fe0d712b4..fbf82b050b 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -374,6 +374,13 @@ struct CPUArchState {
uint64_t (*rdtime_fn)(void *);
void *rdtime_fn_arg;
+ /*machine specific pmu callback */
+ void (*pmu_ctr_write)(PMUCTRState *counter, uint32_t event_idx,
+ target_ulong val, bool high_half);
+ target_ulong (*pmu_ctr_read)(PMUCTRState *counter, uint32_t event_idx,
+ bool high_half);
+ bool (*pmu_vendor_support)(uint32_t event_idx);
+
/* machine specific AIA ireg read-modify-write callback */
#define AIA_MAKE_IREG(__isel, __priv, __virt, __vgein, __xlen) \
((((__xlen) & 0xff) << 24) | \
@@ -455,6 +462,8 @@ struct ArchCPU {
uint32_t pmu_avail_ctrs;
/* Mapping of events to counters */
GHashTable *pmu_event_ctr_map;
+ /* Mapping of counters to events */
+ GHashTable *pmu_ctr_event_map;
const GPtrArray *decoders;
};
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 58ef7079dc..b541852c84 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -875,20 +875,25 @@ static RISCVException write_mhpmcounter(CPURISCVState *env, int csrno,
int ctr_idx = csrno - CSR_MCYCLE;
PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
uint64_t mhpmctr_val = val;
+ int event_idx;
counter->mhpmcounter_val = val;
- if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
- riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
- counter->mhpmcounter_prev = get_ticks(false);
- if (ctr_idx > 2) {
+ event_idx = riscv_pmu_get_event_by_ctr(env, ctr_idx);
+
+ if (event_idx != RISCV_PMU_EVENT_NOT_PRESENTED) {
+ if (RISCV_PMU_CTR_IS_HPM(ctr_idx) && env->pmu_ctr_write) {
+ env->pmu_ctr_write(counter, event_idx, val, false);
+ } else {
+ counter->mhpmcounter_prev = get_ticks(false);
+ }
+ if (RISCV_PMU_CTR_IS_HPM(ctr_idx)) {
if (riscv_cpu_mxl(env) == MXL_RV32) {
mhpmctr_val = mhpmctr_val |
((uint64_t)counter->mhpmcounterh_val << 32);
}
riscv_pmu_setup_timer(env, mhpmctr_val, ctr_idx);
}
- } else {
- /* Other counters can keep incrementing from the given value */
+ } else {
counter->mhpmcounter_prev = val;
}
@@ -902,13 +907,19 @@ static RISCVException write_mhpmcounterh(CPURISCVState *env, int csrno,
PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
uint64_t mhpmctr_val = counter->mhpmcounter_val;
uint64_t mhpmctrh_val = val;
+ int event_idx;
counter->mhpmcounterh_val = val;
mhpmctr_val = mhpmctr_val | (mhpmctrh_val << 32);
- if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
- riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
- counter->mhpmcounterh_prev = get_ticks(true);
- if (ctr_idx > 2) {
+ event_idx = riscv_pmu_get_event_by_ctr(env, ctr_idx);
+
+ if (event_idx != RISCV_PMU_EVENT_NOT_PRESENTED) {
+ if (RISCV_PMU_CTR_IS_HPM(ctr_idx) && env->pmu_ctr_write) {
+ env->pmu_ctr_write(counter, event_idx, val, true);
+ } else {
+ counter->mhpmcounterh_prev = get_ticks(true);
+ }
+ if (RISCV_PMU_CTR_IS_HPM(ctr_idx)) {
riscv_pmu_setup_timer(env, mhpmctr_val, ctr_idx);
}
} else {
@@ -926,6 +937,7 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
counter->mhpmcounter_prev;
target_ulong ctr_val = upper_half ? counter->mhpmcounterh_val :
counter->mhpmcounter_val;
+ int event_idx;
if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
/*
@@ -946,9 +958,14 @@ static RISCVException riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
* The kernel computes the perf delta by subtracting the current value from
* the value it initialized previously (ctr_val).
*/
- if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
- riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
- *val = get_ticks(upper_half) - ctr_prev + ctr_val;
+ event_idx = riscv_pmu_get_event_by_ctr(env, ctr_idx);
+ if (event_idx != RISCV_PMU_EVENT_NOT_PRESENTED) {
+ if (RISCV_PMU_CTR_IS_HPM(ctr_idx) && env->pmu_ctr_read) {
+ *val = env->pmu_ctr_read(counter, event_idx,
+ upper_half);
+ } else {
+ *val = get_ticks(upper_half) - ctr_prev + ctr_val;
+ }
} else {
*val = ctr_val;
}
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 0e7d58b8a5..c3b6b20337 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -88,7 +88,7 @@ static bool riscv_pmu_counter_valid(RISCVCPU *cpu, uint32_t ctr_idx)
}
}
-static bool riscv_pmu_counter_enabled(RISCVCPU *cpu, uint32_t ctr_idx)
+bool riscv_pmu_counter_enabled(RISCVCPU *cpu, uint32_t ctr_idx)
{
CPURISCVState *env = &cpu->env;
@@ -207,59 +207,28 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx)
return ret;
}
-bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
- uint32_t target_ctr)
+int riscv_pmu_get_event_by_ctr(CPURISCVState *env,
+ uint32_t target_ctr)
{
RISCVCPU *cpu;
uint32_t event_idx;
- uint32_t ctr_idx;
- /* Fixed instret counter */
- if (target_ctr == 2) {
- return true;
+ if (target_ctr < 3) {
+ return target_ctr;
}
cpu = env_archcpu(env);
- if (!cpu->pmu_event_ctr_map) {
- return false;
+ if (!cpu->pmu_ctr_event_map || !cpu->pmu_event_ctr_map) {
+ return RISCV_PMU_EVENT_NOT_PRESENTED;
}
- event_idx = RISCV_PMU_EVENT_HW_INSTRUCTIONS;
- ctr_idx = GPOINTER_TO_UINT(g_hash_table_lookup(cpu->pmu_event_ctr_map,
- GUINT_TO_POINTER(event_idx)));
- if (!ctr_idx) {
- return false;
+ event_idx = GPOINTER_TO_UINT(g_hash_table_lookup(cpu->pmu_ctr_event_map,
+ GUINT_TO_POINTER(target_ctr)));
+ if (!event_idx) {
+ return RISCV_PMU_EVENT_NOT_PRESENTED;
}
- return target_ctr == ctr_idx ? true : false;
-}
-
-bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t target_ctr)
-{
- RISCVCPU *cpu;
- uint32_t event_idx;
- uint32_t ctr_idx;
-
- /* Fixed mcycle counter */
- if (target_ctr == 0) {
- return true;
- }
-
- cpu = env_archcpu(env);
- if (!cpu->pmu_event_ctr_map) {
- return false;
- }
-
- event_idx = RISCV_PMU_EVENT_HW_CPU_CYCLES;
- ctr_idx = GPOINTER_TO_UINT(g_hash_table_lookup(cpu->pmu_event_ctr_map,
- GUINT_TO_POINTER(event_idx)));
-
- /* Counter zero is not used for event_ctr_map */
- if (!ctr_idx) {
- return false;
- }
-
- return (target_ctr == ctr_idx) ? true : false;
+ return event_idx;
}
static gboolean pmu_remove_event_map(gpointer key, gpointer value,
@@ -268,6 +237,12 @@ static gboolean pmu_remove_event_map(gpointer key, gpointer value,
return (GPOINTER_TO_UINT(value) == GPOINTER_TO_UINT(udata)) ? true : false;
}
+static gboolean pmu_remove_ctr_map(gpointer key, gpointer value,
+ gpointer udata)
+{
+ return (GPOINTER_TO_UINT(key) == GPOINTER_TO_UINT(udata)) ? true : false;
+}
+
static int64_t pmu_icount_ticks_to_ns(int64_t value)
{
int64_t ret = 0;
@@ -286,8 +261,11 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
{
uint32_t event_idx;
RISCVCPU *cpu = env_archcpu(env);
+ bool machine_specific = false;
- if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->pmu_event_ctr_map) {
+ if (!riscv_pmu_counter_valid(cpu, ctr_idx) ||
+ !cpu->pmu_event_ctr_map ||
+ !cpu->pmu_ctr_event_map) {
return -1;
}
@@ -299,6 +277,9 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
g_hash_table_foreach_remove(cpu->pmu_event_ctr_map,
pmu_remove_event_map,
GUINT_TO_POINTER(ctr_idx));
+ g_hash_table_foreach_remove(cpu->pmu_ctr_event_map,
+ pmu_remove_ctr_map,
+ GUINT_TO_POINTER(ctr_idx));
return 0;
}
@@ -308,40 +289,39 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
return 0;
}
- switch (event_idx) {
- case RISCV_PMU_EVENT_HW_CPU_CYCLES:
- case RISCV_PMU_EVENT_HW_INSTRUCTIONS:
- case RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS:
- case RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
- case RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
- break;
- default:
- /* We don't support any raw events right now */
- return -1;
+ if (RISCV_PMU_CTR_IS_HPM(ctr_idx) && env->pmu_vendor_support) {
+ machine_specific = env->pmu_vendor_support(event_idx);
+ }
+
+ if (!machine_specific) {
+ switch (event_idx) {
+ case RISCV_PMU_EVENT_HW_CPU_CYCLES:
+ case RISCV_PMU_EVENT_HW_INSTRUCTIONS:
+ case RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS:
+ case RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
+ case RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
+ break;
+ default:
+ return -1;
+ }
}
g_hash_table_insert(cpu->pmu_event_ctr_map, GUINT_TO_POINTER(event_idx),
GUINT_TO_POINTER(ctr_idx));
+ g_hash_table_insert(cpu->pmu_ctr_event_map, GUINT_TO_POINTER(ctr_idx),
+ GUINT_TO_POINTER(event_idx));
return 0;
}
static void pmu_timer_trigger_irq(RISCVCPU *cpu,
- enum riscv_pmu_event_idx evt_idx)
+ uint32_t ctr_idx)
{
- uint32_t ctr_idx;
CPURISCVState *env = &cpu->env;
PMUCTRState *counter;
target_ulong *mhpmevent_val;
uint64_t of_bit_mask;
int64_t irq_trigger_at;
- if (evt_idx != RISCV_PMU_EVENT_HW_CPU_CYCLES &&
- evt_idx != RISCV_PMU_EVENT_HW_INSTRUCTIONS) {
- return;
- }
-
- ctr_idx = GPOINTER_TO_UINT(g_hash_table_lookup(cpu->pmu_event_ctr_map,
- GUINT_TO_POINTER(evt_idx)));
if (!riscv_pmu_counter_enabled(cpu, ctr_idx)) {
return;
}
@@ -349,7 +329,7 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
if (riscv_cpu_mxl(env) == MXL_RV32) {
mhpmevent_val = &env->mhpmeventh_val[ctr_idx];
of_bit_mask = MHPMEVENTH_BIT_OF;
- } else {
+ } else {
mhpmevent_val = &env->mhpmevent_val[ctr_idx];
of_bit_mask = MHPMEVENT_BIT_OF;
}
@@ -372,14 +352,25 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
}
}
+static void riscv_pmu_timer_trigger_irq(gpointer ctr, gpointer event_idx,
+ gpointer opaque)
+{
+ RISCVCPU *cpu = opaque;
+
+ pmu_timer_trigger_irq(cpu, GPOINTER_TO_UINT(ctr));
+}
+
/* Timer callback for instret and cycle counter overflow */
void riscv_pmu_timer_cb(void *priv)
{
RISCVCPU *cpu = priv;
- /* Timer event was triggered only for these events */
- pmu_timer_trigger_irq(cpu, RISCV_PMU_EVENT_HW_CPU_CYCLES);
- pmu_timer_trigger_irq(cpu, RISCV_PMU_EVENT_HW_INSTRUCTIONS);
+ if (!cpu->pmu_ctr_event_map || !cpu->pmu_event_ctr_map) {
+ return;
+ }
+ g_hash_table_foreach(cpu->pmu_ctr_event_map,
+ riscv_pmu_timer_trigger_irq,
+ cpu);
}
int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
@@ -388,6 +379,7 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
int64_t overflow_ns, overflow_left = 0;
RISCVCPU *cpu = env_archcpu(env);
PMUCTRState *counter = &env->pmu_ctrs[ctr_idx];
+ uint32_t event_idx;
if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->cfg.ext_sscofpmf) {
return -1;
@@ -408,8 +400,9 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
overflow_left = overflow_delta - INT64_MAX;
}
- if (riscv_pmu_ctr_monitor_cycles(env, ctr_idx) ||
- riscv_pmu_ctr_monitor_instructions(env, ctr_idx)) {
+ event_idx = riscv_pmu_get_event_by_ctr(env, ctr_idx);
+
+ if (event_idx != RISCV_PMU_EVENT_NOT_PRESENTED) {
overflow_ns = pmu_icount_ticks_to_ns((int64_t)overflow_delta);
overflow_left = pmu_icount_ticks_to_ns(overflow_left) ;
} else {
@@ -443,7 +436,13 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
cpu->pmu_event_ctr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
if (!cpu->pmu_event_ctr_map) {
- error_setg(errp, "Unable to allocate PMU event hash table");
+ error_setg(errp, "Unable to allocate first PMU event hash table");
+ return;
+ }
+
+ cpu->pmu_ctr_event_map = g_hash_table_new(g_direct_hash, g_direct_equal);
+ if (!cpu->pmu_ctr_event_map) {
+ error_setg(errp, "Unable to allocate second PMU event hash table");
return;
}
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 7c0ad661e0..b99a5f58d4 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -22,10 +22,12 @@
#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);
+#define RISCV_PMU_EVENT_NOT_PRESENTED -1
+
+#define RISCV_PMU_CTR_IS_HPM(x) (x > 2)
+
+int riscv_pmu_get_event_by_ctr(CPURISCVState *env,
+ uint32_t target_ctr);
void riscv_pmu_timer_cb(void *priv);
void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
@@ -34,5 +36,6 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
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);
+bool riscv_pmu_counter_enabled(RISCVCPU *cpu, uint32_t ctr_idx);
#endif /* RISCV_PMU_H */
--
2.34.1
On 6/25/24 07:46, Alexei Filippov wrote: > Was added call backs for machine specific pmu events. > Simplify monitor functions by adding new hash table, which going to map > counter number and event index. > Was added read/write callbacks which going to simplify support for events, > which expected to have different behavior. > > Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com> > --- > Changes since v2: > -rebased to latest master > target/riscv/cpu.h | 9 +++ > target/riscv/csr.c | 43 +++++++++----- > target/riscv/pmu.c | 139 ++++++++++++++++++++++----------------------- > target/riscv/pmu.h | 11 ++-- > 4 files changed, 115 insertions(+), 87 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 6fe0d712b4..fbf82b050b 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -374,6 +374,13 @@ struct CPUArchState { > uint64_t (*rdtime_fn)(void *); > void *rdtime_fn_arg; > > + /*machine specific pmu callback */ > + void (*pmu_ctr_write)(PMUCTRState *counter, uint32_t event_idx, > + target_ulong val, bool high_half); > + target_ulong (*pmu_ctr_read)(PMUCTRState *counter, uint32_t event_idx, > + bool high_half); > + bool (*pmu_vendor_support)(uint32_t event_idx); Do these really belong in CPUArchState, rather than RISCVCPUClass? Surely there's more to this series, since these fields are never set... r~
On 25.06.2024 21:18, Richard Henderson wrote: > On 6/25/24 07:46, Alexei Filippov wrote: >> Was added call backs for machine specific pmu events. >> Simplify monitor functions by adding new hash table, which going to map >> counter number and event index. >> Was added read/write callbacks which going to simplify support for events, >> which expected to have different behavior. >> >> Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com> >> --- >> Changes since v2: >> -rebased to latest master >> target/riscv/cpu.h | 9 +++ >> target/riscv/csr.c | 43 +++++++++----- >> target/riscv/pmu.c | 139 ++++++++++++++++++++++----------------------- >> target/riscv/pmu.h | 11 ++-- >> 4 files changed, 115 insertions(+), 87 deletions(-) >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 6fe0d712b4..fbf82b050b 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -374,6 +374,13 @@ struct CPUArchState { >> uint64_t (*rdtime_fn)(void *); >> void *rdtime_fn_arg; >> + /*machine specific pmu callback */ >> + void (*pmu_ctr_write)(PMUCTRState *counter, uint32_t event_idx, >> + target_ulong val, bool high_half); >> + target_ulong (*pmu_ctr_read)(PMUCTRState *counter, uint32_t event_idx, >> + bool high_half); >> + bool (*pmu_vendor_support)(uint32_t event_idx); > > Do these really belong in CPUArchState, rather than RISCVCPUClass? > > Surely there's more to this series, since these fields are never set... > > > r~ Initially this callbacks was added to CPUArchState just to be along with similar implementation with rdtime_fn*. Yes, you're right, there are more series to this, but, it can't be separated from syntacore specific parts, which is unfortunately not ready yet to be published. So, I can prepare second patch to implement PMU subsystem for virt device. What do you think about it? (I'll send it in the few days). -- Sincerely, Aleksei Filippov
Hi Aleksei, On 8/7/24 11:46, Aleksei Filippov wrote: > On 25.06.2024 21:18, Richard Henderson wrote: >> On 6/25/24 07:46, Alexei Filippov wrote: >>> Was added call backs for machine specific pmu events. >>> Simplify monitor functions by adding new hash table, which going to map >>> counter number and event index. >>> Was added read/write callbacks which going to simplify support for >>> events, >>> which expected to have different behavior. >>> >>> Signed-off-by: Alexei Filippov <alexei.filippov@syntacore.com> >>> --- >>> Changes since v2: >>> -rebased to latest master >>> target/riscv/cpu.h | 9 +++ >>> target/riscv/csr.c | 43 +++++++++----- >>> target/riscv/pmu.c | 139 ++++++++++++++++++++++----------------------- >>> target/riscv/pmu.h | 11 ++-- >>> 4 files changed, 115 insertions(+), 87 deletions(-) >>> >>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>> index 6fe0d712b4..fbf82b050b 100644 >>> --- a/target/riscv/cpu.h >>> +++ b/target/riscv/cpu.h >>> @@ -374,6 +374,13 @@ struct CPUArchState { >>> uint64_t (*rdtime_fn)(void *); >>> void *rdtime_fn_arg; >>> + /*machine specific pmu callback */ >>> + void (*pmu_ctr_write)(PMUCTRState *counter, uint32_t event_idx, >>> + target_ulong val, bool high_half); >>> + target_ulong (*pmu_ctr_read)(PMUCTRState *counter, uint32_t >>> event_idx, >>> + bool high_half); >>> + bool (*pmu_vendor_support)(uint32_t event_idx); >> >> Do these really belong in CPUArchState, rather than RISCVCPUClass? >> >> Surely there's more to this series, since these fields are never set... >> >> >> r~ > > Initially this callbacks was added to CPUArchState just to be along with > similar implementation with rdtime_fn*. > > Yes, you're right, there are more series to this, but, it can't be > separated from syntacore specific parts, which is unfortunately not > ready yet to be published. So, I can prepare second patch to implement > PMU subsystem for virt device. What do you think about it? (I'll send it > in the few days). How can we test your patch meanwhile? Thanks, Phil.
© 2016 - 2024 Red Hat, Inc.