[PATCH RFC 00/10] Allow platform specific PMU event encoding

[PATCH RFC 00/10] Allow platform specific PMU event encoding

[Atish Patra]

Currently, the pmu implementation is virt machine specific that
implements the SBI PMU encodings. In the future, individual machines
would want to implement their own custom event encodings as there
is no standard event encoding defined by the ISA. There is a performance
events TG which is working on defining standard set of events but not
encodings. That allows flexibility for platforms to choose whatever
encoding scheme they want. However, that means the generic pmu code
should be flexible enough to allow that in Qemu as well.

This series aims to solve that problem by first disassociating the
common pmu implementation and event encoding. The event encoding is
specific to a platform and should be defined in the platform specific
machine or cpu implementation. The newly defined callbacks can be invoked
from machine specific cpu implementation or machine code itself depending
on the requirement.

The first 5 patches in the series are generic improvements and cleanups
where as the last 5 actually implements the disassociation for the virt
machine. The current series can also be found at[2].

I recently found that Alexei has done a similar effort for SiFive FU740[1]
but the implementation differs from this one based on how the cpu callbacks
are invoked. For example, Alexei's series implements a single callback for
all the events and has defined machine specific counter read/write callbacks.
However, it just defaults to get_ticks() for every event. IMO, that is
confusing to the users unless we can actually emulate more generic events or
machine specific events.

I have separate callbacks for each type of events that we currently support
in Qemu (cycle, instruction, TLB events). Separate callbacks seems a better
approach to avoid ambiguity as we have very limited event capability in qemu.
I am open to converging them to one callback as well if we think we will
be extending set of events in the future.

Once we converge on the approaches, we can consolidate the patches
so that both SiFive FU740 and virt machine can use the same abstraction.

Cc: alexei.filippov@syntacore.com

[1] https://lore.kernel.org/all/20240910174747.148141-1-alexei.filippov@syntacore.com/T/
[2] https://github.com/atishp04/qemu/tree/b4/pmu_event_machine

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (10):
      target/riscv: Fix the hpmevent mask
      target/riscv: Introduce helper functions for pmu hashtable lookup
      target/riscv: Protect the hashtable modifications with a lock
      target/riscv: Use uint64 instead of uint as key
      target/riscv: Rename the PMU events
      target/riscv: Define PMU event related structures
      hw/riscv/virt.c : Disassociate virt PMU events
      target/riscv: Update event mapping hashtable for invalid events
      target/riscv : Use the new tlb fill event functions
      hw/riscv/virt.c: Generate the PMU node from the machine

 hw/riscv/virt.c           | 102 +++++++++++++++++++++++-
 target/riscv/cpu.h        |  52 ++++++++++--
 target/riscv/cpu_bits.h   |   4 +-
 target/riscv/cpu_helper.c |  21 ++---
 target/riscv/pmu.c        | 198 +++++++++++++++++++++-------------------------
 target/riscv/pmu.h        |   3 +-
 6 files changed, 246 insertions(+), 134 deletions(-)
---
base-commit: 19a9809808a51291008f72d051d0f9b3499fc223
change-id: 20241008-pmu_event_machine-b87c58104e61
--
Regards,
Atish patra

[PATCH RFC 01/10] target/riscv: Fix the hpmevent mask

[Atish Patra]

As per the latest privilege specification v1.13[1], the sscofpmf
only reserves first 8 bits of hpmeventX. Update the corresponding
masks accordingly.

[1]https://github.com/riscv/riscv-isa-manual/issues/1578

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu_bits.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 7e3f629356ba..a7b8bcbd0148 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -938,8 +938,8 @@ typedef enum RISCVException {
                                             MHPMEVENTH_BIT_VSINH | \
                                             MHPMEVENTH_BIT_VUINH)
 
-#define MHPMEVENT_SSCOF_MASK               _ULL(0xFFFF000000000000)
-#define MHPMEVENT_IDX_MASK                 0xFFFFF
+#define MHPMEVENT_SSCOF_MASK               0xFF00000000000000ULL
+#define MHPMEVENT_IDX_MASK                 (~MHPMEVENT_SSCOF_MASK)
 #define MHPMEVENT_SSCOF_RESVD              16
 
 /* JVT CSR bits */

-- 
2.34.1

[PATCH RFC 02/10] target/riscv: Introduce helper functions for pmu hashtable lookup

[Atish Patra]

The pmu implementation requires hashtable lookup operation sprinkled
through the file. Add a helper function that allows to consolidate
the implementation and extend it in the future easily.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/pmu.c | 56 ++++++++++++++++++++++++++----------------------------
 1 file changed, 27 insertions(+), 29 deletions(-)

diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index e05ab067d2f2..a88c321a6cad 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -265,6 +265,21 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
     counter_arr[env->priv] += delta;
 }
 
+static bool riscv_pmu_htable_lookup(RISCVCPU *cpu, uint32_t key,
+                                    uint32_t *value)
+{
+    GHashTable *table = cpu->pmu_event_ctr_map;
+    gpointer val_ptr;
+
+    val_ptr = g_hash_table_lookup(table, GUINT_TO_POINTER(key));
+    if (!val_ptr) {
+        return false;
+    }
+
+    *value = GPOINTER_TO_UINT(val_ptr);
+    return true;
+}
+
 void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, target_ulong newpriv,
                                  bool new_virt)
 {
@@ -277,18 +292,15 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx)
     uint32_t ctr_idx;
     int ret;
     CPURISCVState *env = &cpu->env;
-    gpointer value;
 
     if (!cpu->cfg.pmu_mask) {
         return 0;
     }
-    value = g_hash_table_lookup(cpu->pmu_event_ctr_map,
-                                GUINT_TO_POINTER(event_idx));
-    if (!value) {
+
+    if (!riscv_pmu_htable_lookup(cpu, event_idx, &ctr_idx)) {
         return -1;
     }
 
-    ctr_idx = GPOINTER_TO_UINT(value);
     if (!riscv_pmu_counter_enabled(cpu, ctr_idx)) {
         return -1;
     }
@@ -306,7 +318,6 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
                                         uint32_t target_ctr)
 {
     RISCVCPU *cpu;
-    uint32_t event_idx;
     uint32_t ctr_idx;
 
     /* Fixed instret counter */
@@ -315,14 +326,8 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
     }
 
     cpu = env_archcpu(env);
-    if (!cpu->pmu_event_ctr_map) {
-        return false;
-    }
-
-    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) {
+    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_INSTRUCTIONS,
+                                 &ctr_idx)) {
         return false;
     }
 
@@ -332,7 +337,6 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
 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 */
@@ -341,16 +345,8 @@ bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t target_ctr)
     }
 
     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) {
+    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_CPU_CYCLES,
+                                &ctr_idx)) {
         return false;
     }
 
@@ -381,6 +377,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
 {
     uint32_t event_idx;
     RISCVCPU *cpu = env_archcpu(env);
+    uint32_t mapped_ctr_idx;
 
     if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->pmu_event_ctr_map) {
         return -1;
@@ -398,8 +395,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
     }
 
     event_idx = value & MHPMEVENT_IDX_MASK;
-    if (g_hash_table_lookup(cpu->pmu_event_ctr_map,
-                            GUINT_TO_POINTER(event_idx))) {
+    if (riscv_pmu_htable_lookup(cpu, event_idx, &mapped_ctr_idx)) {
         return 0;
     }
 
@@ -472,8 +468,10 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
         return;
     }
 
-    ctr_idx = GPOINTER_TO_UINT(g_hash_table_lookup(cpu->pmu_event_ctr_map,
-                               GUINT_TO_POINTER(evt_idx)));
+    if (!riscv_pmu_htable_lookup(cpu, evt_idx, &ctr_idx)) {
+        return;
+    }
+
     if (!riscv_pmu_counter_enabled(cpu, ctr_idx)) {
         return;
     }

-- 
2.34.1

Re: [PATCH RFC 02/10] target/riscv: Introduce helper functions for pmu hashtable lookup

[Alexei Filippov]

On 10.10.2024 02:09, Atish Patra wrote:
> The pmu implementation requires hashtable lookup operation sprinkled
> through the file. Add a helper function that allows to consolidate
> the implementation and extend it in the future easily.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>   target/riscv/pmu.c | 56 ++++++++++++++++++++++++++----------------------------
>   1 file changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index e05ab067d2f2..a88c321a6cad 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -265,6 +265,21 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
>       counter_arr[env->priv] += delta;
>   }
>   
> +static bool riscv_pmu_htable_lookup(RISCVCPU *cpu, uint32_t key,
> +                                    uint32_t *value)
> +{
> +    GHashTable *table = cpu->pmu_event_ctr_map;
> +    gpointer val_ptr;
> +
> +    val_ptr = g_hash_table_lookup(table, GUINT_TO_POINTER(key));
> +    if (!val_ptr) {
> +        return false;
> +    }
> +
> +    *value = GPOINTER_TO_UINT(val_ptr);
> +    return true;
> +}
> +
>   void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, target_ulong newpriv,
>                                    bool new_virt)
>   {
> @@ -277,18 +292,15 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx)
>       uint32_t ctr_idx;
>       int ret;
>       CPURISCVState *env = &cpu->env;
> -    gpointer value;
>   
>       if (!cpu->cfg.pmu_mask) {
>           return 0;
>       }
> -    value = g_hash_table_lookup(cpu->pmu_event_ctr_map,
> -                                GUINT_TO_POINTER(event_idx));
> -    if (!value) {
> +
> +    if (!riscv_pmu_htable_lookup(cpu, event_idx, &ctr_idx)) {
>           return -1;
>       }
>   
> -    ctr_idx = GPOINTER_TO_UINT(value);
>       if (!riscv_pmu_counter_enabled(cpu, ctr_idx)) {
>           return -1;
>       }
> @@ -306,7 +318,6 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
>                                           uint32_t target_ctr)
Not sure about this kind of functions, this hardcoded dublication aren't 
scalable, check it in my patch.
>   {
>       RISCVCPU *cpu;
> -    uint32_t event_idx;
>       uint32_t ctr_idx;
>   
>       /* Fixed instret counter */
> @@ -315,14 +326,8 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
>       }
>   
>       cpu = env_archcpu(env);
> -    if (!cpu->pmu_event_ctr_map) {
> -        return false;
> -    }
> -
> -    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) {
> +    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_INSTRUCTIONS,
> +                                 &ctr_idx)) {
>           return false;
>       }
>   
> @@ -332,7 +337,6 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
>   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 */
> @@ -341,16 +345,8 @@ bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t target_ctr)
>       }
>   
>       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) {
> +    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_CPU_CYCLES,
> +                                &ctr_idx)) {
>           return false;
>       }
>   
> @@ -381,6 +377,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>   {
>       uint32_t event_idx;
>       RISCVCPU *cpu = env_archcpu(env);
> +    uint32_t mapped_ctr_idx;
>   
>       if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->pmu_event_ctr_map) {
>           return -1;
> @@ -398,8 +395,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>       }
>   
>       event_idx = value & MHPMEVENT_IDX_MASK;
> -    if (g_hash_table_lookup(cpu->pmu_event_ctr_map,
> -                            GUINT_TO_POINTER(event_idx))) {
> +    if (riscv_pmu_htable_lookup(cpu, event_idx, &mapped_ctr_idx)) {
>           return 0;
>       }
>   
> @@ -472,8 +468,10 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
>           return;
>       }
>   
> -    ctr_idx = GPOINTER_TO_UINT(g_hash_table_lookup(cpu->pmu_event_ctr_map,
> -                               GUINT_TO_POINTER(evt_idx)));
> +    if (!riscv_pmu_htable_lookup(cpu, evt_idx, &ctr_idx)) {
> +        return;
> +    }
> +
>       if (!riscv_pmu_counter_enabled(cpu, ctr_idx)) {
>           return;
>       }
> 

[PATCH RFC 03/10] target/riscv: Protect the hashtable modifications with a lock

[Atish Patra]

Add a read/write lock to protect the hashtable access operations
in multi-threaded scenario.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.h |  1 +
 target/riscv/pmu.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index a63a29744c26..97e408b91219 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -496,6 +496,7 @@ struct ArchCPU {
     uint32_t pmu_avail_ctrs;
     /* Mapping of events to counters */
     GHashTable *pmu_event_ctr_map;
+    pthread_rwlock_t pmu_map_lock;
     const GPtrArray *decoders;
 };
 
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index a88c321a6cad..21377518f4e0 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -271,12 +271,15 @@ static bool riscv_pmu_htable_lookup(RISCVCPU *cpu, uint32_t key,
     GHashTable *table = cpu->pmu_event_ctr_map;
     gpointer val_ptr;
 
-    val_ptr = g_hash_table_lookup(table, GUINT_TO_POINTER(key));
+    pthread_rwlock_rdlock(&cpu->pmu_map_lock);
+    gpointer val_ptr = g_hash_table_lookup(table, GUINT_TO_POINTER(key));
     if (!val_ptr) {
+        pthread_rwlock_unlock(&cpu->pmu_map_lock);
         return false;
     }
 
     *value = GPOINTER_TO_UINT(val_ptr);
+    pthread_rwlock_unlock(&cpu->pmu_map_lock);
     return true;
 }
 
@@ -388,9 +391,11 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
      * mapping.
      */
     if (!value) {
+        pthread_rwlock_wrlock(&cpu->pmu_map_lock);
         g_hash_table_foreach_remove(cpu->pmu_event_ctr_map,
                                     pmu_remove_event_map,
                                     GUINT_TO_POINTER(ctr_idx));
+        pthread_rwlock_unlock(&cpu->pmu_map_lock);
         return 0;
     }
 
@@ -410,8 +415,10 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
         /* We don't support any raw events right now */
         return -1;
     }
+    pthread_rwlock_wrlock(&cpu->pmu_map_lock);
     g_hash_table_insert(cpu->pmu_event_ctr_map, GUINT_TO_POINTER(event_idx),
                         GUINT_TO_POINTER(ctr_idx));
+    pthread_rwlock_unlock(&cpu->pmu_map_lock);
 
     return 0;
 }
@@ -597,4 +604,5 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
     }
 
     cpu->pmu_avail_ctrs = cpu->cfg.pmu_mask;
+    pthread_rwlock_init(&cpu->pmu_map_lock, NULL);
 }

-- 
2.34.1

[PATCH RFC 04/10] target/riscv: Use uint64 instead of uint as key

[Atish Patra]

The event ID can be a upto 56 bit value when sscofpmf is implemented.
Change the event to counter hashtable to store the keys as 64 bit value
instead of uint.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/pmu.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 21377518f4e0..2531d4f1a9c1 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -265,14 +265,14 @@ static void riscv_pmu_cycle_update_priv(CPURISCVState *env,
     counter_arr[env->priv] += delta;
 }
 
-static bool riscv_pmu_htable_lookup(RISCVCPU *cpu, uint32_t key,
+static bool riscv_pmu_htable_lookup(RISCVCPU *cpu, uint64_t key,
                                     uint32_t *value)
 {
     GHashTable *table = cpu->pmu_event_ctr_map;
     gpointer val_ptr;
 
     pthread_rwlock_rdlock(&cpu->pmu_map_lock);
-    gpointer val_ptr = g_hash_table_lookup(table, GUINT_TO_POINTER(key));
+    val_ptr = g_hash_table_lookup(table, &key);
     if (!val_ptr) {
         pthread_rwlock_unlock(&cpu->pmu_map_lock);
         return false;
@@ -378,9 +378,10 @@ static int64_t pmu_icount_ticks_to_ns(int64_t value)
 int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
                                uint32_t ctr_idx)
 {
-    uint32_t event_idx;
+    uint64_t event_idx;
     RISCVCPU *cpu = env_archcpu(env);
     uint32_t mapped_ctr_idx;
+    gint64 *eid_ptr;
 
     if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->pmu_event_ctr_map) {
         return -1;
@@ -415,8 +416,10 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
         /* We don't support any raw events right now */
         return -1;
     }
+    eid_ptr = g_new(gint64, 1);
+    *eid_ptr = event_idx;
     pthread_rwlock_wrlock(&cpu->pmu_map_lock);
-    g_hash_table_insert(cpu->pmu_event_ctr_map, GUINT_TO_POINTER(event_idx),
+    g_hash_table_insert(cpu->pmu_event_ctr_map, eid_ptr,
                         GUINT_TO_POINTER(ctr_idx));
     pthread_rwlock_unlock(&cpu->pmu_map_lock);
 
@@ -597,7 +600,8 @@ void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
         return;
     }
 
-    cpu->pmu_event_ctr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
+    cpu->pmu_event_ctr_map = g_hash_table_new_full(g_int64_hash, g_int64_equal,
+                                                   g_free, NULL);
     if (!cpu->pmu_event_ctr_map) {
         error_setg(errp, "Unable to allocate PMU event hash table");
         return;

-- 
2.34.1

[PATCH RFC 05/10] target/riscv: Rename the PMU events

[Atish Patra]

The current PMU events are defined by SBI PMU
specification.  As there is no standard event encoding
scheme, Virt machine chooses to use the SBI PMU encoding.
A platform may choose to implement a different event
encoding scheme completely.

Rename the event names to reflect the reality.

No functional changes introduced.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.h        | 26 +++++++++++++++-----
 target/riscv/cpu_helper.c |  8 +++---
 target/riscv/pmu.c        | 62 ++++++++++++++++++-----------------------------
 target/riscv/pmu.h        |  2 +-
 4 files changed, 48 insertions(+), 50 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 97e408b91219..2ac391a7cf74 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -820,14 +820,28 @@ enum {
 /*
  * The event id are encoded based on the encoding specified in the
  * SBI specification v0.3
+ *
+ * The event encoding is specified in the SBI specification
+ * Event idx is a 20bits wide number encoded as follows:
+ * event_idx[19:16] = type
+ * event_idx[15:0] = code
+ * The code field in cache events are encoded as follows:
+ * event_idx.code[15:3] = cache_id
+ * event_idx.code[2:1] = op_id
+ * event_idx.code[0:0] = result_id
  */
 
-enum riscv_pmu_event_idx {
-    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
-    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
-    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
-    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
-    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
+enum virt_pmu_event_idx {
+    /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
+    VIRT_PMU_EVENT_HW_CPU_CYCLES = 0x01,
+    /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
+    VIRT_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
+    /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
+    VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
+    /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
+    VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
+    /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
+    VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
 };
 
 /* used by tcg/tcg-cpu.c*/
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 203c0a92ab75..0f1655a221bd 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1295,17 +1295,17 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
 
 static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
 {
-    enum riscv_pmu_event_idx pmu_event_type;
+    enum virt_pmu_event_idx pmu_event_type;
 
     switch (access_type) {
     case MMU_INST_FETCH:
-        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
+        pmu_event_type = VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
         break;
     case MMU_DATA_LOAD:
-        pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
+        pmu_event_type = VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS;
         break;
     case MMU_DATA_STORE:
-        pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS;
+        pmu_event_type = VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS;
         break;
     default:
         return;
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 2531d4f1a9c1..c436b08d1043 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -38,40 +38,24 @@ void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name)
 {
     uint32_t fdt_event_ctr_map[15] = {};
 
-   /*
-    * The event encoding is specified in the SBI specification
-    * Event idx is a 20bits wide number encoded as follows:
-    * event_idx[19:16] = type
-    * event_idx[15:0] = code
-    * The code field in cache events are encoded as follows:
-    * event_idx.code[15:3] = cache_id
-    * event_idx.code[2:1] = op_id
-    * event_idx.code[0:0] = result_id
-    */
-
-   /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
-   fdt_event_ctr_map[0] = cpu_to_be32(0x00000001);
-   fdt_event_ctr_map[1] = cpu_to_be32(0x00000001);
+   fdt_event_ctr_map[0] = cpu_to_be32(VIRT_PMU_EVENT_HW_CPU_CYCLES);
+   fdt_event_ctr_map[1] = cpu_to_be32(VIRT_PMU_EVENT_HW_CPU_CYCLES);
    fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
 
-   /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
-   fdt_event_ctr_map[3] = cpu_to_be32(0x00000002);
-   fdt_event_ctr_map[4] = cpu_to_be32(0x00000002);
+   fdt_event_ctr_map[3] = cpu_to_be32(VIRT_PMU_EVENT_HW_INSTRUCTIONS);
+   fdt_event_ctr_map[4] = cpu_to_be32(VIRT_PMU_EVENT_HW_INSTRUCTIONS);
    fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
 
-   /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
-   fdt_event_ctr_map[6] = cpu_to_be32(0x00010019);
-   fdt_event_ctr_map[7] = cpu_to_be32(0x00010019);
+   fdt_event_ctr_map[6] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS);
+   fdt_event_ctr_map[7] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS);
    fdt_event_ctr_map[8] = cpu_to_be32(cmask);
 
-   /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
-   fdt_event_ctr_map[9] = cpu_to_be32(0x0001001B);
-   fdt_event_ctr_map[10] = cpu_to_be32(0x0001001B);
+   fdt_event_ctr_map[9] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS);
+   fdt_event_ctr_map[10] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS);
    fdt_event_ctr_map[11] = cpu_to_be32(cmask);
 
-   /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
-   fdt_event_ctr_map[12] = cpu_to_be32(0x00010021);
-   fdt_event_ctr_map[13] = cpu_to_be32(0x00010021);
+   fdt_event_ctr_map[12] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS);
+   fdt_event_ctr_map[13] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS);
    fdt_event_ctr_map[14] = cpu_to_be32(cmask);
 
    /* This a OpenSBI specific DT property documented in OpenSBI docs */
@@ -290,7 +274,7 @@ void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, target_ulong newpriv,
     riscv_pmu_icount_update_priv(env, newpriv, new_virt);
 }
 
-int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx)
+int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_pmu_event_idx event_idx)
 {
     uint32_t ctr_idx;
     int ret;
@@ -329,7 +313,7 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
     }
 
     cpu = env_archcpu(env);
-    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_INSTRUCTIONS,
+    if (!riscv_pmu_htable_lookup(cpu, VIRT_PMU_EVENT_HW_INSTRUCTIONS,
                                  &ctr_idx)) {
         return false;
     }
@@ -348,7 +332,7 @@ bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t target_ctr)
     }
 
     cpu = env_archcpu(env);
-    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_CPU_CYCLES,
+    if (!riscv_pmu_htable_lookup(cpu, VIRT_PMU_EVENT_HW_CPU_CYCLES,
                                 &ctr_idx)) {
         return false;
     }
@@ -406,11 +390,11 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
     }
 
     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:
+    case VIRT_PMU_EVENT_HW_CPU_CYCLES:
+    case VIRT_PMU_EVENT_HW_INSTRUCTIONS:
+    case VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS:
+    case VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
+    case VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
         break;
     default:
         /* We don't support any raw events right now */
@@ -464,7 +448,7 @@ static bool pmu_hpmevent_set_of_if_clear(CPURISCVState *env, uint32_t ctr_idx)
 }
 
 static void pmu_timer_trigger_irq(RISCVCPU *cpu,
-                                  enum riscv_pmu_event_idx evt_idx)
+                                  enum virt_pmu_event_idx evt_idx)
 {
     uint32_t ctr_idx;
     CPURISCVState *env = &cpu->env;
@@ -473,8 +457,8 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
     uint64_t curr_ctr_val, curr_ctrh_val;
     uint64_t ctr_val;
 
-    if (evt_idx != RISCV_PMU_EVENT_HW_CPU_CYCLES &&
-        evt_idx != RISCV_PMU_EVENT_HW_INSTRUCTIONS) {
+    if (evt_idx != VIRT_PMU_EVENT_HW_CPU_CYCLES &&
+        evt_idx != VIRT_PMU_EVENT_HW_INSTRUCTIONS) {
         return;
     }
 
@@ -533,8 +517,8 @@ 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);
+    pmu_timer_trigger_irq(cpu, VIRT_PMU_EVENT_HW_CPU_CYCLES);
+    pmu_timer_trigger_irq(cpu, VIRT_PMU_EVENT_HW_INSTRUCTIONS);
 }
 
 int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 3853d0e2629e..75a22d596b69 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -30,7 +30,7 @@ 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,
                                uint32_t ctr_idx);
-int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
+int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_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);

-- 
2.34.1

Re: [PATCH RFC 05/10] target/riscv: Rename the PMU events

[Alexei Filippov]

On 10.10.2024 02:09, Atish Patra wrote:
> The current PMU events are defined by SBI PMU
> specification.  As there is no standard event encoding
> scheme, Virt machine chooses to use the SBI PMU encoding.
> A platform may choose to implement a different event
> encoding scheme completely.
> 
> Rename the event names to reflect the reality.
> 
> No functional changes introduced.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>   target/riscv/cpu.h        | 26 +++++++++++++++-----
>   target/riscv/cpu_helper.c |  8 +++---
>   target/riscv/pmu.c        | 62 ++++++++++++++++++-----------------------------
>   target/riscv/pmu.h        |  2 +-
>   4 files changed, 48 insertions(+), 50 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 97e408b91219..2ac391a7cf74 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -820,14 +820,28 @@ enum {
>   /*
>    * The event id are encoded based on the encoding specified in the
>    * SBI specification v0.3
> + *
> + * The event encoding is specified in the SBI specification
> + * Event idx is a 20bits wide number encoded as follows:
> + * event_idx[19:16] = type
> + * event_idx[15:0] = code
> + * The code field in cache events are encoded as follows:
> + * event_idx.code[15:3] = cache_id
> + * event_idx.code[2:1] = op_id
> + * event_idx.code[0:0] = result_id
>    */
>   
> -enum riscv_pmu_event_idx {
> -    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> -    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> -    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> -    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> -    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> +enum virt_pmu_event_idx {
> +    /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> +    VIRT_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> +    /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> +    VIRT_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> +    /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
> +    VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> +    /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
> +    VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> +    /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
> +    VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
Pretty sure, this is not a good idea to rename them since the generic 
event even do not include TLB_* events as far as I know. It's acctually 
better to just leave generic naming as is and let the machine handle 
machine specific events separatly.
>   };
>   
>   /* used by tcg/tcg-cpu.c*/
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 203c0a92ab75..0f1655a221bd 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1295,17 +1295,17 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>   
>   static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
>   {
> -    enum riscv_pmu_event_idx pmu_event_type;
> +    enum virt_pmu_event_idx pmu_event_type;
>   
>       switch (access_type) {
>       case MMU_INST_FETCH:
> -        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
> +        pmu_event_type = VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
>           break;
>       case MMU_DATA_LOAD:
> -        pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
> +        pmu_event_type = VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS;
>           break;
>       case MMU_DATA_STORE:
> -        pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS;
> +        pmu_event_type = VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS;
>           break;
>       default:
>           return;
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 2531d4f1a9c1..c436b08d1043 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -38,40 +38,24 @@ void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name)
>   {
>       uint32_t fdt_event_ctr_map[15] = {};
>   
> -   /*
> -    * The event encoding is specified in the SBI specification
> -    * Event idx is a 20bits wide number encoded as follows:
> -    * event_idx[19:16] = type
> -    * event_idx[15:0] = code
> -    * The code field in cache events are encoded as follows:
> -    * event_idx.code[15:3] = cache_id
> -    * event_idx.code[2:1] = op_id
> -    * event_idx.code[0:0] = result_id
> -    */
> -
> -   /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> -   fdt_event_ctr_map[0] = cpu_to_be32(0x00000001);
> -   fdt_event_ctr_map[1] = cpu_to_be32(0x00000001);
> +   fdt_event_ctr_map[0] = cpu_to_be32(VIRT_PMU_EVENT_HW_CPU_CYCLES);
> +   fdt_event_ctr_map[1] = cpu_to_be32(VIRT_PMU_EVENT_HW_CPU_CYCLES);
>      fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
>   
> -   /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> -   fdt_event_ctr_map[3] = cpu_to_be32(0x00000002);
> -   fdt_event_ctr_map[4] = cpu_to_be32(0x00000002);
> +   fdt_event_ctr_map[3] = cpu_to_be32(VIRT_PMU_EVENT_HW_INSTRUCTIONS);
> +   fdt_event_ctr_map[4] = cpu_to_be32(VIRT_PMU_EVENT_HW_INSTRUCTIONS);
>      fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
>   
> -   /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
> -   fdt_event_ctr_map[6] = cpu_to_be32(0x00010019);
> -   fdt_event_ctr_map[7] = cpu_to_be32(0x00010019);
> +   fdt_event_ctr_map[6] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS);
> +   fdt_event_ctr_map[7] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS);
>      fdt_event_ctr_map[8] = cpu_to_be32(cmask);
>   
> -   /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
> -   fdt_event_ctr_map[9] = cpu_to_be32(0x0001001B);
> -   fdt_event_ctr_map[10] = cpu_to_be32(0x0001001B);
> +   fdt_event_ctr_map[9] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS);
> +   fdt_event_ctr_map[10] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS);
>      fdt_event_ctr_map[11] = cpu_to_be32(cmask);
>   
> -   /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
> -   fdt_event_ctr_map[12] = cpu_to_be32(0x00010021);
> -   fdt_event_ctr_map[13] = cpu_to_be32(0x00010021);
> +   fdt_event_ctr_map[12] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS);
> +   fdt_event_ctr_map[13] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS);
>      fdt_event_ctr_map[14] = cpu_to_be32(cmask);
Ok, I guess it's time to do smthng generic to this function, cz if 
number of supported machines will go up it's going to be a problem I guess.
>   
>      /* This a OpenSBI specific DT property documented in OpenSBI docs */
> @@ -290,7 +274,7 @@ void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, target_ulong newpriv,
>       riscv_pmu_icount_update_priv(env, newpriv, new_virt);
>   }
>   
> -int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx)
> +int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_pmu_event_idx event_idx)
>   {
>       uint32_t ctr_idx;
>       int ret;
> @@ -329,7 +313,7 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
>       }
>   
>       cpu = env_archcpu(env);
> -    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_INSTRUCTIONS,
> +    if (!riscv_pmu_htable_lookup(cpu, VIRT_PMU_EVENT_HW_INSTRUCTIONS,
>                                    &ctr_idx)) {
>           return false;
>       }
> @@ -348,7 +332,7 @@ bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t target_ctr)
>       }
>   
>       cpu = env_archcpu(env);
> -    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_CPU_CYCLES,
> +    if (!riscv_pmu_htable_lookup(cpu, VIRT_PMU_EVENT_HW_CPU_CYCLES,
>                                   &ctr_idx)) {
>           return false;
>       }
> @@ -406,11 +390,11 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
>       }
>   
>       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:
> +    case VIRT_PMU_EVENT_HW_CPU_CYCLES:
> +    case VIRT_PMU_EVENT_HW_INSTRUCTIONS:
> +    case VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS:
> +    case VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
> +    case VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
>           break;
>       default:
>           /* We don't support any raw events right now */
> @@ -464,7 +448,7 @@ static bool pmu_hpmevent_set_of_if_clear(CPURISCVState *env, uint32_t ctr_idx)
>   }
>   
>   static void pmu_timer_trigger_irq(RISCVCPU *cpu,
> -                                  enum riscv_pmu_event_idx evt_idx)
> +                                  enum virt_pmu_event_idx evt_idx)
>   {
>       uint32_t ctr_idx;
>       CPURISCVState *env = &cpu->env;
> @@ -473,8 +457,8 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
>       uint64_t curr_ctr_val, curr_ctrh_val;
>       uint64_t ctr_val;
>   
> -    if (evt_idx != RISCV_PMU_EVENT_HW_CPU_CYCLES &&
> -        evt_idx != RISCV_PMU_EVENT_HW_INSTRUCTIONS) {
> +    if (evt_idx != VIRT_PMU_EVENT_HW_CPU_CYCLES &&
> +        evt_idx != VIRT_PMU_EVENT_HW_INSTRUCTIONS) {
>           return;
>       }
>   
> @@ -533,8 +517,8 @@ 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);
> +    pmu_timer_trigger_irq(cpu, VIRT_PMU_EVENT_HW_CPU_CYCLES);
> +    pmu_timer_trigger_irq(cpu, VIRT_PMU_EVENT_HW_INSTRUCTIONS);
>   }
>   
>   int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 3853d0e2629e..75a22d596b69 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -30,7 +30,7 @@ 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,
>                                  uint32_t ctr_idx);
> -int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
> +int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_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);
> 

Re: [PATCH RFC 05/10] target/riscv: Rename the PMU events

[Atish Kumar Patra]

On Thu, Oct 10, 2024 at 5:10 AM Alexei Filippov
<alexei.filippov@yadro.com> wrote:
>
>
>
> On 10.10.2024 02:09, Atish Patra wrote:
> > The current PMU events are defined by SBI PMU
> > specification.  As there is no standard event encoding
> > scheme, Virt machine chooses to use the SBI PMU encoding.
> > A platform may choose to implement a different event
> > encoding scheme completely.
> >
> > Rename the event names to reflect the reality.
> >
> > No functional changes introduced.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >   target/riscv/cpu.h        | 26 +++++++++++++++-----
> >   target/riscv/cpu_helper.c |  8 +++---
> >   target/riscv/pmu.c        | 62 ++++++++++++++++++-----------------------------
> >   target/riscv/pmu.h        |  2 +-
> >   4 files changed, 48 insertions(+), 50 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 97e408b91219..2ac391a7cf74 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -820,14 +820,28 @@ enum {
> >   /*
> >    * The event id are encoded based on the encoding specified in the
> >    * SBI specification v0.3
> > + *
> > + * The event encoding is specified in the SBI specification
> > + * Event idx is a 20bits wide number encoded as follows:
> > + * event_idx[19:16] = type
> > + * event_idx[15:0] = code
> > + * The code field in cache events are encoded as follows:
> > + * event_idx.code[15:3] = cache_id
> > + * event_idx.code[2:1] = op_id
> > + * event_idx.code[0:0] = result_id
> >    */
> >
> > -enum riscv_pmu_event_idx {
> > -    RISCV_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> > -    RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> > -    RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> > -    RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > -    RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> > +enum virt_pmu_event_idx {
> > +    /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> > +    VIRT_PMU_EVENT_HW_CPU_CYCLES = 0x01,
> > +    /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> > +    VIRT_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
> > +    /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
> > +    VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
> > +    /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
> > +    VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
> > +    /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
> > +    VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
> Pretty sure, this is not a good idea to rename them since the generic
> event even do not include TLB_* events as far as I know. It's acctually
> better to just leave generic naming as is and let the machine handle
> machine specific events separatly.

These event names are defined in SBI specification which virt machine
implements.
https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-pmu.adoc#event-hardware-cache-events-type-1

These are not RISC-V standard events. As I mentioned in the cover
letter, there are no standard RISC-V event names.
Adding the RISCV_PMU prefix is confusing as there is a performance
event TG which is trying to define standard events.

> >   };
> >
> >   /* used by tcg/tcg-cpu.c*/
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 203c0a92ab75..0f1655a221bd 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -1295,17 +1295,17 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> >
> >   static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
> >   {
> > -    enum riscv_pmu_event_idx pmu_event_type;
> > +    enum virt_pmu_event_idx pmu_event_type;
> >
> >       switch (access_type) {
> >       case MMU_INST_FETCH:
> > -        pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
> > +        pmu_event_type = VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
> >           break;
> >       case MMU_DATA_LOAD:
> > -        pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
> > +        pmu_event_type = VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS;
> >           break;
> >       case MMU_DATA_STORE:
> > -        pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS;
> > +        pmu_event_type = VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS;
> >           break;
> >       default:
> >           return;
> > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > index 2531d4f1a9c1..c436b08d1043 100644
> > --- a/target/riscv/pmu.c
> > +++ b/target/riscv/pmu.c
> > @@ -38,40 +38,24 @@ void riscv_pmu_generate_fdt_node(void *fdt, uint32_t cmask, char *pmu_name)
> >   {
> >       uint32_t fdt_event_ctr_map[15] = {};
> >
> > -   /*
> > -    * The event encoding is specified in the SBI specification
> > -    * Event idx is a 20bits wide number encoded as follows:
> > -    * event_idx[19:16] = type
> > -    * event_idx[15:0] = code
> > -    * The code field in cache events are encoded as follows:
> > -    * event_idx.code[15:3] = cache_id
> > -    * event_idx.code[2:1] = op_id
> > -    * event_idx.code[0:0] = result_id
> > -    */
> > -
> > -   /* SBI_PMU_HW_CPU_CYCLES: 0x01 : type(0x00) */
> > -   fdt_event_ctr_map[0] = cpu_to_be32(0x00000001);
> > -   fdt_event_ctr_map[1] = cpu_to_be32(0x00000001);
> > +   fdt_event_ctr_map[0] = cpu_to_be32(VIRT_PMU_EVENT_HW_CPU_CYCLES);
> > +   fdt_event_ctr_map[1] = cpu_to_be32(VIRT_PMU_EVENT_HW_CPU_CYCLES);
> >      fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
> >
> > -   /* SBI_PMU_HW_INSTRUCTIONS: 0x02 : type(0x00) */
> > -   fdt_event_ctr_map[3] = cpu_to_be32(0x00000002);
> > -   fdt_event_ctr_map[4] = cpu_to_be32(0x00000002);
> > +   fdt_event_ctr_map[3] = cpu_to_be32(VIRT_PMU_EVENT_HW_INSTRUCTIONS);
> > +   fdt_event_ctr_map[4] = cpu_to_be32(VIRT_PMU_EVENT_HW_INSTRUCTIONS);
> >      fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
> >
> > -   /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
> > -   fdt_event_ctr_map[6] = cpu_to_be32(0x00010019);
> > -   fdt_event_ctr_map[7] = cpu_to_be32(0x00010019);
> > +   fdt_event_ctr_map[6] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS);
> > +   fdt_event_ctr_map[7] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS);
> >      fdt_event_ctr_map[8] = cpu_to_be32(cmask);
> >
> > -   /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
> > -   fdt_event_ctr_map[9] = cpu_to_be32(0x0001001B);
> > -   fdt_event_ctr_map[10] = cpu_to_be32(0x0001001B);
> > +   fdt_event_ctr_map[9] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS);
> > +   fdt_event_ctr_map[10] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS);
> >      fdt_event_ctr_map[11] = cpu_to_be32(cmask);
> >
> > -   /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
> > -   fdt_event_ctr_map[12] = cpu_to_be32(0x00010021);
> > -   fdt_event_ctr_map[13] = cpu_to_be32(0x00010021);
> > +   fdt_event_ctr_map[12] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS);
> > +   fdt_event_ctr_map[13] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS);
> >      fdt_event_ctr_map[14] = cpu_to_be32(cmask);
> Ok, I guess it's time to do smthng generic to this function, cz if
> number of supported machines will go up it's going to be a problem I guess.
> >

We can define a generic helper function in the pmu.c. Keep in mind
this is only required for the machines
without counter delegation extensions. In a few years, every new
platform will probably implement counter delegation
which voids the requirement of SBI PMU DT node.

> >      /* This a OpenSBI specific DT property documented in OpenSBI docs */
> > @@ -290,7 +274,7 @@ void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, target_ulong newpriv,
> >       riscv_pmu_icount_update_priv(env, newpriv, new_virt);
> >   }
> >
> > -int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx)
> > +int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_pmu_event_idx event_idx)
> >   {
> >       uint32_t ctr_idx;
> >       int ret;
> > @@ -329,7 +313,7 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
> >       }
> >
> >       cpu = env_archcpu(env);
> > -    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_INSTRUCTIONS,
> > +    if (!riscv_pmu_htable_lookup(cpu, VIRT_PMU_EVENT_HW_INSTRUCTIONS,
> >                                    &ctr_idx)) {
> >           return false;
> >       }
> > @@ -348,7 +332,7 @@ bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t target_ctr)
> >       }
> >
> >       cpu = env_archcpu(env);
> > -    if (!riscv_pmu_htable_lookup(cpu, RISCV_PMU_EVENT_HW_CPU_CYCLES,
> > +    if (!riscv_pmu_htable_lookup(cpu, VIRT_PMU_EVENT_HW_CPU_CYCLES,
> >                                   &ctr_idx)) {
> >           return false;
> >       }
> > @@ -406,11 +390,11 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
> >       }
> >
> >       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:
> > +    case VIRT_PMU_EVENT_HW_CPU_CYCLES:
> > +    case VIRT_PMU_EVENT_HW_INSTRUCTIONS:
> > +    case VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS:
> > +    case VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
> > +    case VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
> >           break;
> >       default:
> >           /* We don't support any raw events right now */
> > @@ -464,7 +448,7 @@ static bool pmu_hpmevent_set_of_if_clear(CPURISCVState *env, uint32_t ctr_idx)
> >   }
> >
> >   static void pmu_timer_trigger_irq(RISCVCPU *cpu,
> > -                                  enum riscv_pmu_event_idx evt_idx)
> > +                                  enum virt_pmu_event_idx evt_idx)
> >   {
> >       uint32_t ctr_idx;
> >       CPURISCVState *env = &cpu->env;
> > @@ -473,8 +457,8 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
> >       uint64_t curr_ctr_val, curr_ctrh_val;
> >       uint64_t ctr_val;
> >
> > -    if (evt_idx != RISCV_PMU_EVENT_HW_CPU_CYCLES &&
> > -        evt_idx != RISCV_PMU_EVENT_HW_INSTRUCTIONS) {
> > +    if (evt_idx != VIRT_PMU_EVENT_HW_CPU_CYCLES &&
> > +        evt_idx != VIRT_PMU_EVENT_HW_INSTRUCTIONS) {
> >           return;
> >       }
> >
> > @@ -533,8 +517,8 @@ 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);
> > +    pmu_timer_trigger_irq(cpu, VIRT_PMU_EVENT_HW_CPU_CYCLES);
> > +    pmu_timer_trigger_irq(cpu, VIRT_PMU_EVENT_HW_INSTRUCTIONS);
> >   }
> >
> >   int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)
> > diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> > index 3853d0e2629e..75a22d596b69 100644
> > --- a/target/riscv/pmu.h
> > +++ b/target/riscv/pmu.h
> > @@ -30,7 +30,7 @@ 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,
> >                                  uint32_t ctr_idx);
> > -int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
> > +int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_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);
> >

[PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Atish Patra]

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 2ac391a7cf74..53426710f73e 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
         uint64_t counter_virt_prev[2];
 } PMUFixedCtrState;
 
+typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
+typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
+typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
+
+typedef struct PMUEventInfo {
+    /* Event ID (BIT [0:55] valid) */
+    uint64_t event_id;
+    /* Supported hpmcounters for this event */
+    uint32_t counter_mask;
+    /* Bitmask of valid event bits */
+    uint64_t event_mask;
+} PMUEventInfo;
+
+typedef struct PMUEventFunc {
+    /* Get the ID of the event that can monitor cycles */
+    PMU_EVENT_CYCLE_FUNC get_cycle_id;
+    /* Get the ID of the event that can monitor cycles */
+    PMU_EVENT_INSTRET_FUNC get_intstret_id;
+    /* Get the ID of the event that can monitor TLB events*/
+    PMU_EVENT_TLB_FUNC get_tlb_access_id;
+} PMUEventFunc;
+
 struct CPUArchState {
     target_ulong gpr[32];
     target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
@@ -386,6 +408,9 @@ struct CPUArchState {
     target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
 
     PMUFixedCtrState pmu_fixed_ctrs[2];
+    PMUEventInfo *pmu_events;
+    PMUEventFunc pmu_efuncs;
+    int num_pmu_events;
 
     target_ulong sscratch;
     target_ulong mscratch;

-- 
2.34.1

Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Alexei Filippov]

On 10.10.2024 02:09, Atish Patra wrote:
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>   target/riscv/cpu.h | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 2ac391a7cf74..53426710f73e 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
>           uint64_t counter_virt_prev[2];
>   } PMUFixedCtrState;
>   
> +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
> +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
> +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
> +
> +typedef struct PMUEventInfo {
> +    /* Event ID (BIT [0:55] valid) */
> +    uint64_t event_id;
> +    /* Supported hpmcounters for this event */
> +    uint32_t counter_mask;
> +    /* Bitmask of valid event bits */
> +    uint64_t event_mask;
> +} PMUEventInfo;
> +
> +typedef struct PMUEventFunc {
> +    /* Get the ID of the event that can monitor cycles */
> +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
> +    /* Get the ID of the event that can monitor cycles */
> +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
> +    /* Get the ID of the event that can monitor TLB events*/
> +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
Ok, this is kinda interesting decision, but it's not scalable. AFAIU 
none spec provide us full enum of existing events. So anytime when 
somebody will try to implement their own pmu events they would have to 
add additional callbacks, and this structure never will be fulled 
properly. And then we ended up with structure 1000+ callback with only 5 
machines wich supports pmu events. I suggest my approach with only 
read/write callbacks, where machine can decide by itself how to handle 
any of machine specific events.
> +} PMUEventFunc;
> +
>   struct CPUArchState {
>       target_ulong gpr[32];
>       target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> @@ -386,6 +408,9 @@ struct CPUArchState {
>       target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
>   
>       PMUFixedCtrState pmu_fixed_ctrs[2];
> +    PMUEventInfo *pmu_events;
> +    PMUEventFunc pmu_efuncs;
> +    int num_pmu_events;
>   
>       target_ulong sscratch;
>       target_ulong mscratch;
> 

Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Atish Kumar Patra]

On Thu, Oct 10, 2024 at 5:44 AM Alexei Filippov
<alexei.filippov@yadro.com> wrote:
>
>
>
> On 10.10.2024 02:09, Atish Patra wrote:
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >   target/riscv/cpu.h | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 2ac391a7cf74..53426710f73e 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
> >           uint64_t counter_virt_prev[2];
> >   } PMUFixedCtrState;
> >
> > +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
> > +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
> > +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
> > +
> > +typedef struct PMUEventInfo {
> > +    /* Event ID (BIT [0:55] valid) */
> > +    uint64_t event_id;
> > +    /* Supported hpmcounters for this event */
> > +    uint32_t counter_mask;
> > +    /* Bitmask of valid event bits */
> > +    uint64_t event_mask;
> > +} PMUEventInfo;
> > +
> > +typedef struct PMUEventFunc {
> > +    /* Get the ID of the event that can monitor cycles */
> > +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
> > +    /* Get the ID of the event that can monitor cycles */
> > +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
> > +    /* Get the ID of the event that can monitor TLB events*/
> > +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
> Ok, this is kinda interesting decision, but it's not scalable. AFAIU

Yes it is not scalable if there is a need to scale as mentioned earlier.
Do you have any other events that can be emulated in Qemu ?

Having said that, I am okay with single call back though but not too sure
about read/write callback unless there is a use case to support those.

> none spec provide us full enum of existing events. So anytime when
> somebody will try to implement their own pmu events they would have to
> add additional callbacks, and this structure never will be fulled
> properly. And then we ended up with structure 1000+ callback with only 5
> machines wich supports pmu events. I suggest my approach with only
> read/write callbacks, where machine can decide by itself how to handle
> any of machine specific events.

Lot of these events are microarchitectural events which can't be
supported in Qemu.
I don't think it's a good idea at all to add dummy values for all the
events defined in a platform
which is harder to debug for a user.


> > +} PMUEventFunc;
> > +
> >   struct CPUArchState {
> >       target_ulong gpr[32];
> >       target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> > @@ -386,6 +408,9 @@ struct CPUArchState {
> >       target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> >
> >       PMUFixedCtrState pmu_fixed_ctrs[2];
> > +    PMUEventInfo *pmu_events;
> > +    PMUEventFunc pmu_efuncs;
> > +    int num_pmu_events;
> >
> >       target_ulong sscratch;
> >       target_ulong mscratch;
> >

Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Aleksei Filippov]

> On 11 Oct 2024, at 23:45, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> 
> On Thu, Oct 10, 2024 at 5:44 AM Alexei Filippov
> <alexei.filippov@yadro.com> wrote:
>> 
>> 
>> 
>> On 10.10.2024 02:09, Atish Patra wrote:
>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>> ---
>>>  target/riscv/cpu.h | 25 +++++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>> 
>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> index 2ac391a7cf74..53426710f73e 100644
>>> --- a/target/riscv/cpu.h
>>> +++ b/target/riscv/cpu.h
>>> @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
>>>          uint64_t counter_virt_prev[2];
>>>  } PMUFixedCtrState;
>>> 
>>> +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
>>> +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
>>> +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
>>> +
>>> +typedef struct PMUEventInfo {
>>> +    /* Event ID (BIT [0:55] valid) */
>>> +    uint64_t event_id;
>>> +    /* Supported hpmcounters for this event */
>>> +    uint32_t counter_mask;
>>> +    /* Bitmask of valid event bits */
>>> +    uint64_t event_mask;
>>> +} PMUEventInfo;
>>> +
>>> +typedef struct PMUEventFunc {
>>> +    /* Get the ID of the event that can monitor cycles */
>>> +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
>>> +    /* Get the ID of the event that can monitor cycles */
>>> +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
>>> +    /* Get the ID of the event that can monitor TLB events*/
>>> +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
>> Ok, this is kinda interesting decision, but it's not scalable. AFAIU
> 
> Yes it is not scalable if there is a need to scale as mentioned earlier.
> Do you have any other events that can be emulated in Qemu ?
> 
> Having said that, I am okay with single call back though but not too sure
> about read/write callback unless there is a use case to support those.
> 
>> none spec provide us full enum of existing events. So anytime when
>> somebody will try to implement their own pmu events they would have to
>> add additional callbacks, and this structure never will be fulled
>> properly. And then we ended up with structure 1000+ callback with only 5
>> machines wich supports pmu events. I suggest my approach with only
>> read/write callbacks, where machine can decide by itself how to handle
>> any of machine specific events.
> 
> Lot of these events are microarchitectural events which can't be
> supported in Qemu.
> I don't think it's a good idea at all to add dummy values for all the
> events defined in a platform
> which is harder to debug for a user.

Yes, you're right that the rest of the events are microarchitectural and that they can't be properly simulated in QEMU at the moment, but it seems to me that's not really the point here. The point is how elastic this solution can be - that is, whether to do any events or not and how exactly they should be counted should be decided by the vendor of a particular machine, and not by the simulator in general. Plus, I have a very real use case where it will come in handy - debugging perf. Support the possibility of simulating events on QEMU side will make the life of t perf folks much easier. I do not insist specifically on my implementation of this solution, but I think that the solution with the creation of a callback for each of the events will significantly complicate the porting of the PMU for machine vendors.
> 
> 
>>> +} PMUEventFunc;
>>> +
>>>  struct CPUArchState {
>>>      target_ulong gpr[32];
>>>      target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
>>> @@ -386,6 +408,9 @@ struct CPUArchState {
>>>      target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
>>> 
>>>      PMUFixedCtrState pmu_fixed_ctrs[2];
>>> +    PMUEventInfo *pmu_events;
>>> +    PMUEventFunc pmu_efuncs;
>>> +    int num_pmu_events;
>>> 
>>>      target_ulong sscratch;
>>>      target_ulong mscratch;

Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Atish Kumar Patra]

On Mon, Oct 21, 2024 at 6:45 AM Aleksei Filippov
<alexei.filippov@syntacore.com> wrote:
>
>
>
> > On 11 Oct 2024, at 23:45, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >
> > On Thu, Oct 10, 2024 at 5:44 AM Alexei Filippov
> > <alexei.filippov@yadro.com> wrote:
> >>
> >>
> >>
> >> On 10.10.2024 02:09, Atish Patra wrote:
> >>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>> ---
> >>>  target/riscv/cpu.h | 25 +++++++++++++++++++++++++
> >>>  1 file changed, 25 insertions(+)
> >>>
> >>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>> index 2ac391a7cf74..53426710f73e 100644
> >>> --- a/target/riscv/cpu.h
> >>> +++ b/target/riscv/cpu.h
> >>> @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
> >>>          uint64_t counter_virt_prev[2];
> >>>  } PMUFixedCtrState;
> >>>
> >>> +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
> >>> +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
> >>> +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
> >>> +
> >>> +typedef struct PMUEventInfo {
> >>> +    /* Event ID (BIT [0:55] valid) */
> >>> +    uint64_t event_id;
> >>> +    /* Supported hpmcounters for this event */
> >>> +    uint32_t counter_mask;
> >>> +    /* Bitmask of valid event bits */
> >>> +    uint64_t event_mask;
> >>> +} PMUEventInfo;
> >>> +
> >>> +typedef struct PMUEventFunc {
> >>> +    /* Get the ID of the event that can monitor cycles */
> >>> +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
> >>> +    /* Get the ID of the event that can monitor cycles */
> >>> +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
> >>> +    /* Get the ID of the event that can monitor TLB events*/
> >>> +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
> >> Ok, this is kinda interesting decision, but it's not scalable. AFAIU
> >
> > Yes it is not scalable if there is a need to scale as mentioned earlier.
> > Do you have any other events that can be emulated in Qemu ?
> >
> > Having said that, I am okay with single call back though but not too sure
> > about read/write callback unless there is a use case to support those.
> >
> >> none spec provide us full enum of existing events. So anytime when
> >> somebody will try to implement their own pmu events they would have to
> >> add additional callbacks, and this structure never will be fulled
> >> properly. And then we ended up with structure 1000+ callback with only 5
> >> machines wich supports pmu events. I suggest my approach with only
> >> read/write callbacks, where machine can decide by itself how to handle
> >> any of machine specific events.
> >
> > Lot of these events are microarchitectural events which can't be
> > supported in Qemu.
> > I don't think it's a good idea at all to add dummy values for all the
> > events defined in a platform
> > which is harder to debug for a user.
>
> Yes, you're right that the rest of the events are microarchitectural and that they can't be properly simulated in QEMU at the moment, but it seems to me that's not really the point here. The point is how elastic this solution can be - that is, whether to do any events or not and how exactly they should be counted should be decided by the vendor of a particular machine, and not by the simulator in general. Plus, I have a very real use case where it will come in handy - debugging perf. Support the possibility of simulating events on QEMU side will make the life of t perf folks much easier. I do not insist specifically on my implementation of this solution, but I think that the solution with the creation of a callback for each of the events will significantly complicate the porting of the PMU for machine vendors.
> >

As I mentioned in other threads, I am absolutely okay with single call
backs. So Let's do that. However, I am not in favor of adding
microarchitectural events that we can't support in Qemu with
completely bogus data. Debugging perf in Qemu can be easily done with
the current set of events supported. Those are not the most accurate
but it's a representation of what Qemu is running. Do you foresee any
debugging issue if we don't support all the events a platform
advertises ?

> >
> >>> +} PMUEventFunc;
> >>> +
> >>>  struct CPUArchState {
> >>>      target_ulong gpr[32];
> >>>      target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> >>> @@ -386,6 +408,9 @@ struct CPUArchState {
> >>>      target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> >>>
> >>>      PMUFixedCtrState pmu_fixed_ctrs[2];
> >>> +    PMUEventInfo *pmu_events;
> >>> +    PMUEventFunc pmu_efuncs;
> >>> +    int num_pmu_events;
> >>>
> >>>      target_ulong sscratch;
> >>>      target_ulong mscratch;
>
>

Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Aleksei Filippov]

> On 22 Oct 2024, at 15:58, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> 
> On Mon, Oct 21, 2024 at 6:45 AM Aleksei Filippov
> <alexei.filippov@syntacore.com> wrote:
>> 
>> 
>> 
>>> On 11 Oct 2024, at 23:45, Atish Kumar Patra <atishp@rivosinc.com> wrote:
>>> 
>>> On Thu, Oct 10, 2024 at 5:44 AM Alexei Filippov
>>> <alexei.filippov@yadro.com> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 10.10.2024 02:09, Atish Patra wrote:
>>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>>> ---
>>>>> target/riscv/cpu.h | 25 +++++++++++++++++++++++++
>>>>> 1 file changed, 25 insertions(+)
>>>>> 
>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>>> index 2ac391a7cf74..53426710f73e 100644
>>>>> --- a/target/riscv/cpu.h
>>>>> +++ b/target/riscv/cpu.h
>>>>> @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
>>>>>       uint64_t counter_virt_prev[2];
>>>>> } PMUFixedCtrState;
>>>>> 
>>>>> +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
>>>>> +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
>>>>> +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
>>>>> +
>>>>> +typedef struct PMUEventInfo {
>>>>> +    /* Event ID (BIT [0:55] valid) */
>>>>> +    uint64_t event_id;
>>>>> +    /* Supported hpmcounters for this event */
>>>>> +    uint32_t counter_mask;
>>>>> +    /* Bitmask of valid event bits */
>>>>> +    uint64_t event_mask;
>>>>> +} PMUEventInfo;
>>>>> +
>>>>> +typedef struct PMUEventFunc {
>>>>> +    /* Get the ID of the event that can monitor cycles */
>>>>> +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
>>>>> +    /* Get the ID of the event that can monitor cycles */
>>>>> +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
>>>>> +    /* Get the ID of the event that can monitor TLB events*/
>>>>> +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
>>>> Ok, this is kinda interesting decision, but it's not scalable. AFAIU
>>> 
>>> Yes it is not scalable if there is a need to scale as mentioned earlier.
>>> Do you have any other events that can be emulated in Qemu ?
>>> 
>>> Having said that, I am okay with single call back though but not too sure
>>> about read/write callback unless there is a use case to support those.
>>> 
>>>> none spec provide us full enum of existing events. So anytime when
>>>> somebody will try to implement their own pmu events they would have to
>>>> add additional callbacks, and this structure never will be fulled
>>>> properly. And then we ended up with structure 1000+ callback with only 5
>>>> machines wich supports pmu events. I suggest my approach with only
>>>> read/write callbacks, where machine can decide by itself how to handle
>>>> any of machine specific events.
>>> 
>>> Lot of these events are microarchitectural events which can't be
>>> supported in Qemu.
>>> I don't think it's a good idea at all to add dummy values for all the
>>> events defined in a platform
>>> which is harder to debug for a user.
>> 
>> Yes, you're right that the rest of the events are microarchitectural and that they can't be properly simulated in QEMU at the moment, but it seems to me that's not really the point here. The point is how elastic this solution can be - that is, whether to do any events or not and how exactly they should be counted should be decided by the vendor of a particular machine, and not by the simulator in general. Plus, I have a very real use case where it will come in handy - debugging perf. Support the possibility of simulating events on QEMU side will make the life of t perf folks much easier. I do not insist specifically on my implementation of this solution, but I think that the solution with the creation of a callback for each of the events will significantly complicate the porting of the PMU for machine vendors.
>>> 
> 
> As I mentioned in other threads, I am absolutely okay with single call
> backs. So Let's do that. However, I am not in favor of adding
> microarchitectural events that we can't support in Qemu with
> completely bogus data. Debugging perf in Qemu can be easily done with
> the current set of events supported. Those are not the most accurate
> but it's a representation of what Qemu is running. Do you foresee any
> debugging issue if we don't support all the events a platform
> advertises ?
In general - there is only one potential problem. When perf would try to get event by the wrong id. The main algorithm indeed could be tested with limited  quantities of events. But this gonna be a real pain for the testers which gonna deduce and remember what particular event can/can’t be counted in QEMU and why does he gets 0 there and not 0 here. Moreover, if we would allow events with even complete bogus data this would works perfectly for CI stuff for the benchmark + perf ppl, and they wouldn’t restrict their CI to that. I really do  not see any problem to let the vendor handle this situation. At least vendor can decide by his own to count/not to count some types of event, this gonna bring flexibility and the transparency of the solution and, in general, if we’ll bring some rational reason why we can't add such events we can always forbid to do such thing.
> 
>>> 
>>>>> +} PMUEventFunc;
>>>>> +
>>>>> struct CPUArchState {
>>>>>   target_ulong gpr[32];
>>>>>   target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
>>>>> @@ -386,6 +408,9 @@ struct CPUArchState {
>>>>>   target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
>>>>> 
>>>>>   PMUFixedCtrState pmu_fixed_ctrs[2];
>>>>> +    PMUEventInfo *pmu_events;
>>>>> +    PMUEventFunc pmu_efuncs;
>>>>> +    int num_pmu_events;
>>>>> 
>>>>>   target_ulong sscratch;
>>>>>   target_ulong mscratch;
>> 
>> 

Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Atish Kumar Patra]

On Wed, Nov 20, 2024 at 6:25 AM Aleksei Filippov
<alexei.filippov@syntacore.com> wrote:
>
>
>
> > On 22 Oct 2024, at 15:58, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >
> > On Mon, Oct 21, 2024 at 6:45 AM Aleksei Filippov
> > <alexei.filippov@syntacore.com> wrote:
> >>
> >>
> >>
> >>> On 11 Oct 2024, at 23:45, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >>>
> >>> On Thu, Oct 10, 2024 at 5:44 AM Alexei Filippov
> >>> <alexei.filippov@yadro.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 10.10.2024 02:09, Atish Patra wrote:
> >>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>>>> ---
> >>>>> target/riscv/cpu.h | 25 +++++++++++++++++++++++++
> >>>>> 1 file changed, 25 insertions(+)
> >>>>>
> >>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>>>> index 2ac391a7cf74..53426710f73e 100644
> >>>>> --- a/target/riscv/cpu.h
> >>>>> +++ b/target/riscv/cpu.h
> >>>>> @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
> >>>>>       uint64_t counter_virt_prev[2];
> >>>>> } PMUFixedCtrState;
> >>>>>
> >>>>> +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
> >>>>> +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
> >>>>> +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
> >>>>> +
> >>>>> +typedef struct PMUEventInfo {
> >>>>> +    /* Event ID (BIT [0:55] valid) */
> >>>>> +    uint64_t event_id;
> >>>>> +    /* Supported hpmcounters for this event */
> >>>>> +    uint32_t counter_mask;
> >>>>> +    /* Bitmask of valid event bits */
> >>>>> +    uint64_t event_mask;
> >>>>> +} PMUEventInfo;
> >>>>> +
> >>>>> +typedef struct PMUEventFunc {
> >>>>> +    /* Get the ID of the event that can monitor cycles */
> >>>>> +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
> >>>>> +    /* Get the ID of the event that can monitor cycles */
> >>>>> +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
> >>>>> +    /* Get the ID of the event that can monitor TLB events*/
> >>>>> +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
> >>>> Ok, this is kinda interesting decision, but it's not scalable. AFAIU
> >>>
> >>> Yes it is not scalable if there is a need to scale as mentioned earlier.
> >>> Do you have any other events that can be emulated in Qemu ?
> >>>
> >>> Having said that, I am okay with single call back though but not too sure
> >>> about read/write callback unless there is a use case to support those.
> >>>
> >>>> none spec provide us full enum of existing events. So anytime when
> >>>> somebody will try to implement their own pmu events they would have to
> >>>> add additional callbacks, and this structure never will be fulled
> >>>> properly. And then we ended up with structure 1000+ callback with only 5
> >>>> machines wich supports pmu events. I suggest my approach with only
> >>>> read/write callbacks, where machine can decide by itself how to handle
> >>>> any of machine specific events.
> >>>
> >>> Lot of these events are microarchitectural events which can't be
> >>> supported in Qemu.
> >>> I don't think it's a good idea at all to add dummy values for all the
> >>> events defined in a platform
> >>> which is harder to debug for a user.
> >>
> >> Yes, you're right that the rest of the events are microarchitectural and that they can't be properly simulated in QEMU at the moment, but it seems to me that's not really the point here. The point is how elastic this solution can be - that is, whether to do any events or not and how exactly they should be counted should be decided by the vendor of a particular machine, and not by the simulator in general. Plus, I have a very real use case where it will come in handy - debugging perf. Support the possibility of simulating events on QEMU side will make the life of t perf folks much easier. I do not insist specifically on my implementation of this solution, but I think that the solution with the creation of a callback for each of the events will significantly complicate the porting of the PMU for machine vendors.
> >>>
> >
> > As I mentioned in other threads, I am absolutely okay with single call
> > backs. So Let's do that. However, I am not in favor of adding
> > microarchitectural events that we can't support in Qemu with
> > completely bogus data. Debugging perf in Qemu can be easily done with
> > the current set of events supported. Those are not the most accurate
> > but it's a representation of what Qemu is running. Do you foresee any
> > debugging issue if we don't support all the events a platform
> > advertises ?
> In general - there is only one potential problem. When perf would try to get event by the wrong id. The main algorithm indeed could be tested with limited  quantities of events. But this

It won't get a wrong id as the Qemu platform won't support those.
Hence, you can not run perf for those events to begin with.

> gonna be a real pain for the testers which gonna deduce and remember what particular event can/can’t be counted in QEMU and why does he gets 0 there and not 0 here. Moreover,

> perf list will show which events are supported on a particular platform. So user won't be confused. We

> we would allow events with even complete bogus data this would works perfectly for CI stuff for the benchmark + perf ppl, and they wouldn’t restrict their CI to that. I really do  not see

IMO, it is more confusing to show bogus data rather than restricting
the number of events an user can run on Qemu platforms. Clearly, you
think otherwise. I think we can agree to disagree here. Let's
consolidate our patches to provide the infrastructure for the actual
events. The bogus event support can be a separate series(per vendor)
as that warrants a different discussion whether it is useful for users
or not.

Sounds good ?

any problem to let the vendor handle this situation. At least vendor
can decide by his own to count/not to count some types of event, this
gonna bring flexibility and the transparency of the solution and, in
general, if we’ll bring some rational reason why we can't add such
events we can always forbid to do such thing.
> >
> >>>
> >>>>> +} PMUEventFunc;
> >>>>> +
> >>>>> struct CPUArchState {
> >>>>>   target_ulong gpr[32];
> >>>>>   target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> >>>>> @@ -386,6 +408,9 @@ struct CPUArchState {
> >>>>>   target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> >>>>>
> >>>>>   PMUFixedCtrState pmu_fixed_ctrs[2];
> >>>>> +    PMUEventInfo *pmu_events;
> >>>>> +    PMUEventFunc pmu_efuncs;
> >>>>> +    int num_pmu_events;
> >>>>>
> >>>>>   target_ulong sscratch;
> >>>>>   target_ulong mscratch;
> >>
> >>
>

Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Aleksei Filippov]

> On 21 Nov 2024, at 22:54, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> 
> On Wed, Nov 20, 2024 at 6:25 AM Aleksei Filippov
> <alexei.filippov@syntacore.com> wrote:
>> 
>> 
>> 
>>> On 22 Oct 2024, at 15:58, Atish Kumar Patra <atishp@rivosinc.com> wrote:
>>> 
>>> On Mon, Oct 21, 2024 at 6:45 AM Aleksei Filippov
>>> <alexei.filippov@syntacore.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On 11 Oct 2024, at 23:45, Atish Kumar Patra <atishp@rivosinc.com> wrote:
>>>>> 
>>>>> On Thu, Oct 10, 2024 at 5:44 AM Alexei Filippov
>>>>> <alexei.filippov@yadro.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 10.10.2024 02:09, Atish Patra wrote:
>>>>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>>>>> ---
>>>>>>> target/riscv/cpu.h | 25 +++++++++++++++++++++++++
>>>>>>> 1 file changed, 25 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>>>>> index 2ac391a7cf74..53426710f73e 100644
>>>>>>> --- a/target/riscv/cpu.h
>>>>>>> +++ b/target/riscv/cpu.h
>>>>>>> @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
>>>>>>>      uint64_t counter_virt_prev[2];
>>>>>>> } PMUFixedCtrState;
>>>>>>> 
>>>>>>> +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
>>>>>>> +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
>>>>>>> +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
>>>>>>> +
>>>>>>> +typedef struct PMUEventInfo {
>>>>>>> +    /* Event ID (BIT [0:55] valid) */
>>>>>>> +    uint64_t event_id;
>>>>>>> +    /* Supported hpmcounters for this event */
>>>>>>> +    uint32_t counter_mask;
>>>>>>> +    /* Bitmask of valid event bits */
>>>>>>> +    uint64_t event_mask;
>>>>>>> +} PMUEventInfo;
>>>>>>> +
>>>>>>> +typedef struct PMUEventFunc {
>>>>>>> +    /* Get the ID of the event that can monitor cycles */
>>>>>>> +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
>>>>>>> +    /* Get the ID of the event that can monitor cycles */
>>>>>>> +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
>>>>>>> +    /* Get the ID of the event that can monitor TLB events*/
>>>>>>> +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
>>>>>> Ok, this is kinda interesting decision, but it's not scalable. AFAIU
>>>>> 
>>>>> Yes it is not scalable if there is a need to scale as mentioned earlier.
>>>>> Do you have any other events that can be emulated in Qemu ?
>>>>> 
>>>>> Having said that, I am okay with single call back though but not too sure
>>>>> about read/write callback unless there is a use case to support those.
>>>>> 
>>>>>> none spec provide us full enum of existing events. So anytime when
>>>>>> somebody will try to implement their own pmu events they would have to
>>>>>> add additional callbacks, and this structure never will be fulled
>>>>>> properly. And then we ended up with structure 1000+ callback with only 5
>>>>>> machines wich supports pmu events. I suggest my approach with only
>>>>>> read/write callbacks, where machine can decide by itself how to handle
>>>>>> any of machine specific events.
>>>>> 
>>>>> Lot of these events are microarchitectural events which can't be
>>>>> supported in Qemu.
>>>>> I don't think it's a good idea at all to add dummy values for all the
>>>>> events defined in a platform
>>>>> which is harder to debug for a user.
>>>> 
>>>> Yes, you're right that the rest of the events are microarchitectural and that they can't be properly simulated in QEMU at the moment, but it seems to me that's not really the point here. The point is how elastic this solution can be - that is, whether to do any events or not and how exactly they should be counted should be decided by the vendor of a particular machine, and not by the simulator in general. Plus, I have a very real use case where it will come in handy - debugging perf. Support the possibility of simulating events on QEMU side will make the life of t perf folks much easier. I do not insist specifically on my implementation of this solution, but I think that the solution with the creation of a callback for each of the events will significantly complicate the porting of the PMU for machine vendors.
>>>>> 
>>> 
>>> As I mentioned in other threads, I am absolutely okay with single call
>>> backs. So Let's do that. However, I am not in favor of adding
>>> microarchitectural events that we can't support in Qemu with
>>> completely bogus data. Debugging perf in Qemu can be easily done with
>>> the current set of events supported. Those are not the most accurate
>>> but it's a representation of what Qemu is running. Do you foresee any
>>> debugging issue if we don't support all the events a platform
>>> advertises ?
>> In general - there is only one potential problem. When perf would try to get event by the wrong id. The main algorithm indeed could be tested with limited  quantities of events. But this
> 
> It won't get a wrong id as the Qemu platform won't support those.
> Hence, you can not run perf for those events to begin with.
> 
>> gonna be a real pain for the testers which gonna deduce and remember what particular event can/can’t be counted in QEMU and why does he gets 0 there and not 0 here. Moreover,
> 
>> perf list will show which events are supported on a particular platform. So user won't be confused. We
> 
>> we would allow events with even complete bogus data this would works perfectly for CI stuff for the benchmark + perf ppl, and they wouldn’t restrict their CI to that. I really do  not see
> 
> IMO, it is more confusing to show bogus data rather than restricting
> the number of events an user can run on Qemu platforms. Clearly, you
> think otherwise. I think we can agree to disagree here. Let's
> consolidate our patches to provide the infrastructure for the actual
> events. The bogus event support can be a separate series(per vendor)
> as that warrants a different discussion whether it is useful for users
> or not.
> 
> Sounds good ?
Yeah, it’s even better to do it separately, agreed. Do you want me to do
 general part? Or we gonna split it in some way?
> 
> any problem to let the vendor handle this situation. At least vendor
> can decide by his own to count/not to count some types of event, this
> gonna bring flexibility and the transparency of the solution and, in
> general, if we’ll bring some rational reason why we can't add such
> events we can always forbid to do such thing.
>>> 
>>>>> 
>>>>>>> +} PMUEventFunc;
>>>>>>> +
>>>>>>> struct CPUArchState {
>>>>>>>  target_ulong gpr[32];
>>>>>>>  target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
>>>>>>> @@ -386,6 +408,9 @@ struct CPUArchState {
>>>>>>>  target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
>>>>>>> 
>>>>>>>  PMUFixedCtrState pmu_fixed_ctrs[2];
>>>>>>> +    PMUEventInfo *pmu_events;
>>>>>>> +    PMUEventFunc pmu_efuncs;
>>>>>>> +    int num_pmu_events;
>>>>>>> 
>>>>>>>  target_ulong sscratch;
>>>>>>>  target_ulong mscratch;

Re: [PATCH RFC 06/10] target/riscv: Define PMU event related structures

[Atish Kumar Patra]

On Fri, Nov 22, 2024 at 3:43 AM Aleksei Filippov
<alexei.filippov@syntacore.com> wrote:
>
>
>
> > On 21 Nov 2024, at 22:54, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >
> > On Wed, Nov 20, 2024 at 6:25 AM Aleksei Filippov
> > <alexei.filippov@syntacore.com> wrote:
> >>
> >>
> >>
> >>> On 22 Oct 2024, at 15:58, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >>>
> >>> On Mon, Oct 21, 2024 at 6:45 AM Aleksei Filippov
> >>> <alexei.filippov@syntacore.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On 11 Oct 2024, at 23:45, Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >>>>>
> >>>>> On Thu, Oct 10, 2024 at 5:44 AM Alexei Filippov
> >>>>> <alexei.filippov@yadro.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 10.10.2024 02:09, Atish Patra wrote:
> >>>>>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>>>>>> ---
> >>>>>>> target/riscv/cpu.h | 25 +++++++++++++++++++++++++
> >>>>>>> 1 file changed, 25 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>>>>>> index 2ac391a7cf74..53426710f73e 100644
> >>>>>>> --- a/target/riscv/cpu.h
> >>>>>>> +++ b/target/riscv/cpu.h
> >>>>>>> @@ -189,6 +189,28 @@ typedef struct PMUFixedCtrState {
> >>>>>>>      uint64_t counter_virt_prev[2];
> >>>>>>> } PMUFixedCtrState;
> >>>>>>>
> >>>>>>> +typedef uint64_t (*PMU_EVENT_CYCLE_FUNC)(RISCVCPU *);
> >>>>>>> +typedef uint64_t (*PMU_EVENT_INSTRET_FUNC)(RISCVCPU *);
> >>>>>>> +typedef uint64_t (*PMU_EVENT_TLB_FUNC)(RISCVCPU *, MMUAccessType access_type);
> >>>>>>> +
> >>>>>>> +typedef struct PMUEventInfo {
> >>>>>>> +    /* Event ID (BIT [0:55] valid) */
> >>>>>>> +    uint64_t event_id;
> >>>>>>> +    /* Supported hpmcounters for this event */
> >>>>>>> +    uint32_t counter_mask;
> >>>>>>> +    /* Bitmask of valid event bits */
> >>>>>>> +    uint64_t event_mask;
> >>>>>>> +} PMUEventInfo;
> >>>>>>> +
> >>>>>>> +typedef struct PMUEventFunc {
> >>>>>>> +    /* Get the ID of the event that can monitor cycles */
> >>>>>>> +    PMU_EVENT_CYCLE_FUNC get_cycle_id;
> >>>>>>> +    /* Get the ID of the event that can monitor cycles */
> >>>>>>> +    PMU_EVENT_INSTRET_FUNC get_intstret_id;
> >>>>>>> +    /* Get the ID of the event that can monitor TLB events*/
> >>>>>>> +    PMU_EVENT_TLB_FUNC get_tlb_access_id;
> >>>>>> Ok, this is kinda interesting decision, but it's not scalable. AFAIU
> >>>>>
> >>>>> Yes it is not scalable if there is a need to scale as mentioned earlier.
> >>>>> Do you have any other events that can be emulated in Qemu ?
> >>>>>
> >>>>> Having said that, I am okay with single call back though but not too sure
> >>>>> about read/write callback unless there is a use case to support those.
> >>>>>
> >>>>>> none spec provide us full enum of existing events. So anytime when
> >>>>>> somebody will try to implement their own pmu events they would have to
> >>>>>> add additional callbacks, and this structure never will be fulled
> >>>>>> properly. And then we ended up with structure 1000+ callback with only 5
> >>>>>> machines wich supports pmu events. I suggest my approach with only
> >>>>>> read/write callbacks, where machine can decide by itself how to handle
> >>>>>> any of machine specific events.
> >>>>>
> >>>>> Lot of these events are microarchitectural events which can't be
> >>>>> supported in Qemu.
> >>>>> I don't think it's a good idea at all to add dummy values for all the
> >>>>> events defined in a platform
> >>>>> which is harder to debug for a user.
> >>>>
> >>>> Yes, you're right that the rest of the events are microarchitectural and that they can't be properly simulated in QEMU at the moment, but it seems to me that's not really the point here. The point is how elastic this solution can be - that is, whether to do any events or not and how exactly they should be counted should be decided by the vendor of a particular machine, and not by the simulator in general. Plus, I have a very real use case where it will come in handy - debugging perf. Support the possibility of simulating events on QEMU side will make the life of t perf folks much easier. I do not insist specifically on my implementation of this solution, but I think that the solution with the creation of a callback for each of the events will significantly complicate the porting of the PMU for machine vendors.
> >>>>>
> >>>
> >>> As I mentioned in other threads, I am absolutely okay with single call
> >>> backs. So Let's do that. However, I am not in favor of adding
> >>> microarchitectural events that we can't support in Qemu with
> >>> completely bogus data. Debugging perf in Qemu can be easily done with
> >>> the current set of events supported. Those are not the most accurate
> >>> but it's a representation of what Qemu is running. Do you foresee any
> >>> debugging issue if we don't support all the events a platform
> >>> advertises ?
> >> In general - there is only one potential problem. When perf would try to get event by the wrong id. The main algorithm indeed could be tested with limited  quantities of events. But this
> >
> > It won't get a wrong id as the Qemu platform won't support those.
> > Hence, you can not run perf for those events to begin with.
> >
> >> gonna be a real pain for the testers which gonna deduce and remember what particular event can/can’t be counted in QEMU and why does he gets 0 there and not 0 here. Moreover,
> >
> >> perf list will show which events are supported on a particular platform. So user won't be confused. We
> >
> >> we would allow events with even complete bogus data this would works perfectly for CI stuff for the benchmark + perf ppl, and they wouldn’t restrict their CI to that. I really do  not see
> >
> > IMO, it is more confusing to show bogus data rather than restricting
> > the number of events an user can run on Qemu platforms. Clearly, you
> > think otherwise. I think we can agree to disagree here. Let's
> > consolidate our patches to provide the infrastructure for the actual
> > events. The bogus event support can be a separate series(per vendor)
> > as that warrants a different discussion whether it is useful for users
> > or not.
> >
> > Sounds good ?
> Yeah, it’s even better to do it separately, agreed. Do you want me to do
>  general part? Or we gonna split it in some way?
> >

Sure, go ahead. If you can include my first few patches that modify
the key to 64 bit
and other fixes/helpers before your patches that would be great.

> > any problem to let the vendor handle this situation. At least vendor
> > can decide by his own to count/not to count some types of event, this
> > gonna bring flexibility and the transparency of the solution and, in
> > general, if we’ll bring some rational reason why we can't add such
> > events we can always forbid to do such thing.
> >>>
> >>>>>
> >>>>>>> +} PMUEventFunc;
> >>>>>>> +
> >>>>>>> struct CPUArchState {
> >>>>>>>  target_ulong gpr[32];
> >>>>>>>  target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> >>>>>>> @@ -386,6 +408,9 @@ struct CPUArchState {
> >>>>>>>  target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
> >>>>>>>
> >>>>>>>  PMUFixedCtrState pmu_fixed_ctrs[2];
> >>>>>>> +    PMUEventInfo *pmu_events;
> >>>>>>> +    PMUEventFunc pmu_efuncs;
> >>>>>>> +    int num_pmu_events;
> >>>>>>>
> >>>>>>>  target_ulong sscratch;
> >>>>>>>  target_ulong mscratch;
>
>

[PATCH RFC 07/10] hw/riscv/virt.c : Disassociate virt PMU events

[Atish Patra]

The virt PMU related implemention should belong to virt
machine file rather than common pmu.c which can be used
for other implementations.

Make pmu.c generic by moving all the virt PMU event related
structures to it's appropriate place.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 hw/riscv/virt.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 target/riscv/pmu.c | 73 ++++++++++++++++++++++++++++++------------------
 2 files changed, 128 insertions(+), 26 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ee3129f3b314..ffda6d65d673 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -56,6 +56,61 @@
 #include "qapi/qapi-visit-common.h"
 #include "hw/virtio/virtio-iommu.h"
 
+static PMUEventInfo pmu_events_arr[] = {
+    {
+        .event_id = VIRT_PMU_EVENT_HW_CPU_CYCLES,
+        .counter_mask = 0x01,
+    },
+    {
+        .event_id = VIRT_PMU_EVENT_HW_INSTRUCTIONS,
+        .counter_mask = 0x04,
+    },
+    {
+        .event_id = VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS,
+        .counter_mask = 0,
+    },
+    {
+        .event_id = VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS,
+        .counter_mask = 0,
+    },
+    {
+        .event_id = VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS,
+        .counter_mask = 0,
+    },
+};
+
+static inline uint64_t virt_pmu_get_cycle_event_id(RISCVCPU *cpu)
+{
+    return VIRT_PMU_EVENT_HW_CPU_CYCLES;
+}
+
+static inline uint64_t virt_pmu_get_instret_event_id(RISCVCPU *cpu)
+{
+    return VIRT_PMU_EVENT_HW_INSTRUCTIONS;
+}
+
+static uint64_t virt_pmu_get_tlb_event_id(RISCVCPU *cpu,
+                                          MMUAccessType access_type)
+{
+    uint64_t tlb_event_type = ULONG_MAX;
+
+    switch (access_type) {
+    case MMU_INST_FETCH:
+        tlb_event_type = VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
+        break;
+    case MMU_DATA_LOAD:
+        tlb_event_type = VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS;
+        break;
+    case MMU_DATA_STORE:
+        tlb_event_type = VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS;
+        break;
+    default:
+        break;
+    }
+
+    return tlb_event_type;
+}
+
 /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
 static bool virt_use_kvm_aia(RISCVVirtState *s)
 {
@@ -710,6 +765,29 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
     aplic_phandles[socket] = aplic_s_phandle;
 }
 
+static void virt_pmu_events_init(RISCVVirtState *s)
+{
+    int cpu, socket, i;
+    MachineState *ms = MACHINE(s);
+    int num_sockets = riscv_socket_count(ms);
+    RISCVCPU *hart;
+
+    for (socket = 0 ; socket < num_sockets; socket++) {
+        for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
+            hart = &s->soc[socket].harts[cpu];
+            hart->env.num_pmu_events = 5;
+            /* All hpmcounters can monitor all supported events */
+            for (i = 0; i < ARRAY_SIZE(pmu_events_arr); i++) {
+                pmu_events_arr[i].counter_mask |= hart->cfg.pmu_mask;
+            }
+            hart->env.pmu_events = pmu_events_arr;
+            hart->env.pmu_efuncs.get_cycle_id = virt_pmu_get_cycle_event_id;
+            hart->env.pmu_efuncs.get_intstret_id = virt_pmu_get_instret_event_id;
+            hart->env.pmu_efuncs.get_tlb_access_id = virt_pmu_get_tlb_event_id;
+        }
+    }
+}
+
 static void create_fdt_pmu(RISCVVirtState *s)
 {
     g_autofree char *pmu_name = g_strdup_printf("/pmu");
@@ -1614,6 +1692,9 @@ static void virt_machine_init(MachineState *machine)
     }
     virt_flash_map(s, system_memory);
 
+    /* Setup the PMU Event details. This must happen before fdt setup */
+    virt_pmu_events_init(s);
+
     /* load/create device tree */
     if (machine->dtb) {
         machine->fdt = load_device_tree(machine->dtb, &s->fdt_size);
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index c436b08d1043..3235388c66e4 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -304,7 +304,8 @@ int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_pmu_event_idx event_idx)
 bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
                                         uint32_t target_ctr)
 {
-    RISCVCPU *cpu;
+    uint64_t event_idx = ULONG_MAX;
+    RISCVCPU *cpu = env_archcpu(env);
     uint32_t ctr_idx;
 
     /* Fixed instret counter */
@@ -312,9 +313,15 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
         return true;
     }
 
-    cpu = env_archcpu(env);
-    if (!riscv_pmu_htable_lookup(cpu, VIRT_PMU_EVENT_HW_INSTRUCTIONS,
-                                 &ctr_idx)) {
+    if (env->pmu_efuncs.get_intstret_id) {
+        event_idx = env->pmu_efuncs.get_intstret_id(cpu);
+    }
+
+    if (event_idx == ULONG_MAX) {
+        return false;
+    }
+
+    if (!riscv_pmu_htable_lookup(cpu, event_idx, &ctr_idx)) {
         return false;
     }
 
@@ -323,7 +330,8 @@ bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
 
 bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t target_ctr)
 {
-    RISCVCPU *cpu;
+    uint64_t event_idx = ULONG_MAX;
+    RISCVCPU *cpu = env_archcpu(env);
     uint32_t ctr_idx;
 
     /* Fixed mcycle counter */
@@ -331,9 +339,15 @@ bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env, uint32_t target_ctr)
         return true;
     }
 
-    cpu = env_archcpu(env);
-    if (!riscv_pmu_htable_lookup(cpu, VIRT_PMU_EVENT_HW_CPU_CYCLES,
-                                &ctr_idx)) {
+    if (env->pmu_efuncs.get_cycle_id) {
+        event_idx = env->pmu_efuncs.get_cycle_id(cpu);
+    }
+
+    if (event_idx == ULONG_MAX) {
+        return false;
+    }
+
+    if (!riscv_pmu_htable_lookup(cpu, event_idx, &ctr_idx)) {
         return false;
     }
 
@@ -366,6 +380,8 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
     RISCVCPU *cpu = env_archcpu(env);
     uint32_t mapped_ctr_idx;
     gint64 *eid_ptr;
+    bool valid_event = false;
+    int i;
 
     if (!riscv_pmu_counter_valid(cpu, ctr_idx) || !cpu->pmu_event_ctr_map) {
         return -1;
@@ -389,15 +405,14 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
         return 0;
     }
 
-    switch (event_idx) {
-    case VIRT_PMU_EVENT_HW_CPU_CYCLES:
-    case VIRT_PMU_EVENT_HW_INSTRUCTIONS:
-    case VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS:
-    case VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
-    case VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
-        break;
-    default:
-        /* We don't support any raw events right now */
+    for (i = 0; i < env->num_pmu_events; i++) {
+        if (event_idx == env->pmu_events[i].event_id) {
+            valid_event = true;
+            break;
+        }
+    }
+
+    if (!valid_event) {
         return -1;
     }
     eid_ptr = g_new(gint64, 1);
@@ -447,8 +462,7 @@ static bool pmu_hpmevent_set_of_if_clear(CPURISCVState *env, uint32_t ctr_idx)
     return false;
 }
 
-static void pmu_timer_trigger_irq(RISCVCPU *cpu,
-                                  enum virt_pmu_event_idx evt_idx)
+static void pmu_timer_trigger_irq(RISCVCPU *cpu, uint64_t evt_idx)
 {
     uint32_t ctr_idx;
     CPURISCVState *env = &cpu->env;
@@ -457,11 +471,6 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
     uint64_t curr_ctr_val, curr_ctrh_val;
     uint64_t ctr_val;
 
-    if (evt_idx != VIRT_PMU_EVENT_HW_CPU_CYCLES &&
-        evt_idx != VIRT_PMU_EVENT_HW_INSTRUCTIONS) {
-        return;
-    }
-
     if (!riscv_pmu_htable_lookup(cpu, evt_idx, &ctr_idx)) {
         return;
     }
@@ -515,10 +524,22 @@ static void pmu_timer_trigger_irq(RISCVCPU *cpu,
 void riscv_pmu_timer_cb(void *priv)
 {
     RISCVCPU *cpu = priv;
+    uint64_t event_idx;
+    CPURISCVState *env = &cpu->env;
 
     /* Timer event was triggered only for these events */
-    pmu_timer_trigger_irq(cpu, VIRT_PMU_EVENT_HW_CPU_CYCLES);
-    pmu_timer_trigger_irq(cpu, VIRT_PMU_EVENT_HW_INSTRUCTIONS);
+    if (env->pmu_efuncs.get_cycle_id) {
+        event_idx = env->pmu_efuncs.get_cycle_id(cpu);
+        if (event_idx != ULONG_MAX) {
+            pmu_timer_trigger_irq(cpu, event_idx);
+        }
+    }
+    if (env->pmu_efuncs.get_intstret_id) {
+        event_idx = env->pmu_efuncs.get_intstret_id(cpu);
+        if (event_idx != ULONG_MAX) {
+            pmu_timer_trigger_irq(cpu, event_idx);
+        }
+    }
 }
 
 int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t value, uint32_t ctr_idx)

-- 
2.34.1

[PATCH RFC 08/10] target/riscv: Update event mapping hashtable for invalid events

[Atish Patra]

If the software programs an invalid hpmevent or selects a invalid
counter mapping, the hashtable entry should be updated accordingly.

Otherwise, the user may get stale value from the old mapped counter.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/pmu.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 3235388c66e4..24c2fe82c247 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -387,39 +387,42 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
         return -1;
     }
 
-    /*
-     * Expected mhpmevent value is zero for reset case. Remove the current
-     * mapping.
-     */
-    if (!value) {
-        pthread_rwlock_wrlock(&cpu->pmu_map_lock);
-        g_hash_table_foreach_remove(cpu->pmu_event_ctr_map,
-                                    pmu_remove_event_map,
-                                    GUINT_TO_POINTER(ctr_idx));
-        pthread_rwlock_unlock(&cpu->pmu_map_lock);
-        return 0;
-    }
-
     event_idx = value & MHPMEVENT_IDX_MASK;
     if (riscv_pmu_htable_lookup(cpu, event_idx, &mapped_ctr_idx)) {
         return 0;
     }
 
     for (i = 0; i < env->num_pmu_events; i++) {
-        if (event_idx == env->pmu_events[i].event_id) {
+        if ((event_idx == env->pmu_events[i].event_id) &&
+            (BIT(ctr_idx) & env->pmu_events[i].counter_mask)) {
             valid_event = true;
             break;
         }
     }
 
-    if (!valid_event) {
-        return -1;
+    pthread_rwlock_wrlock(&cpu->pmu_map_lock);
+    /*
+     * Remove the current mapping in the following cases:
+     * 1. mhpmevent value is zero which indicates a reset case.
+     * 2. An invalid event is programmed for mapping to a counter.
+     */
+    if (!value || !valid_event) {
+        g_hash_table_foreach_remove(cpu->pmu_event_ctr_map,
+                                    pmu_remove_event_map,
+                                    GUINT_TO_POINTER(ctr_idx));
+        pthread_rwlock_unlock(&cpu->pmu_map_lock);
+        return 0;
     }
+
     eid_ptr = g_new(gint64, 1);
     *eid_ptr = event_idx;
-    pthread_rwlock_wrlock(&cpu->pmu_map_lock);
+    /*
+     * Insert operation will replace the value if the key exists
+     * As per the documentation, it will free the passed key is freed as well.
+     * No special handling is required for replace or key management.
+     */
     g_hash_table_insert(cpu->pmu_event_ctr_map, eid_ptr,
-                        GUINT_TO_POINTER(ctr_idx));
+                GUINT_TO_POINTER(ctr_idx));
     pthread_rwlock_unlock(&cpu->pmu_map_lock);
 
     return 0;

-- 
2.34.1

[PATCH RFC 09/10] target/riscv : Use the new tlb fill event functions

[Atish Patra]

We have TLB related event call back available now. Invoke
them from generic cpu helper code so that other machines can
implement those as well in the future. The virt machine is
the only user for now though.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 target/riscv/cpu_helper.c | 21 +++++++--------------
 target/riscv/pmu.c        |  2 +-
 target/riscv/pmu.h        |  2 +-
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 0f1655a221bd..5161fc86dbfe 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1295,23 +1295,16 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
 
 static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
 {
-    enum virt_pmu_event_idx pmu_event_type;
+    uint64_t event_type = ULONG_MAX;
+    CPURISCVState *env = &cpu->env;
 
-    switch (access_type) {
-    case MMU_INST_FETCH:
-        pmu_event_type = VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
-        break;
-    case MMU_DATA_LOAD:
-        pmu_event_type = VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS;
-        break;
-    case MMU_DATA_STORE:
-        pmu_event_type = VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS;
-        break;
-    default:
-        return;
+    if (env->pmu_efuncs.get_tlb_access_id) {
+        event_type = env->pmu_efuncs.get_tlb_access_id(cpu, access_type);
     }
 
-    riscv_pmu_incr_ctr(cpu, pmu_event_type);
+    if (event_type != ULONG_MAX) {
+        riscv_pmu_incr_ctr(cpu, event_type);
+    }
 }
 
 bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index 24c2fe82c247..e80f0f911fa3 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -274,7 +274,7 @@ void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, target_ulong newpriv,
     riscv_pmu_icount_update_priv(env, newpriv, new_virt);
 }
 
-int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_pmu_event_idx event_idx)
+int riscv_pmu_incr_ctr(RISCVCPU *cpu, uint64_t event_idx)
 {
     uint32_t ctr_idx;
     int ret;
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 75a22d596b69..810ac2fae797 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -30,7 +30,7 @@ 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,
                                uint32_t ctr_idx);
-int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum virt_pmu_event_idx event_idx);
+int riscv_pmu_incr_ctr(RISCVCPU *cpu, uint64_t 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);

-- 
2.34.1

[PATCH RFC 10/10] hw/riscv/virt.c: Generate the PMU node from the machine

[Atish Patra]

The virt machine implementation relies on the SBI PMU extension.
The OpenSBI implementation requires a PMU specific DT node that
is currently encodes the counter and PMU events mapping.
As the PMU DT node encodes the platform specific event encodings,
it should be implement in platform specific code instead of generic
PMU code.

Move the PMU DT node generation code from virt.c from common pmu
code.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 hw/riscv/virt.c    | 21 +++++++++++++++++++--
 target/riscv/pmu.c | 36 ------------------------------------
 target/riscv/pmu.h |  1 -
 3 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ffda6d65d673..056afe6a6ceb 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -792,11 +792,28 @@ static void create_fdt_pmu(RISCVVirtState *s)
 {
     g_autofree char *pmu_name = g_strdup_printf("/pmu");
     MachineState *ms = MACHINE(s);
-    RISCVCPU hart = s->soc[0].harts[0];
+    uint32_t fdt_event_ctr_map[15] = {};
+    int i;
 
     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.pmu_avail_ctrs, pmu_name);
+
+    /*
+     * To keep it simple, any event can be mapped to any programmable counters
+     * in QEMU. The generic cycle & instruction count events can also be
+     * monitored using programmable counters. In that case, mcycle & minstret
+     * must continue to provide the correct value as well. Heterogeneous PMU per
+     * hart is not supported yet. Thus, number of counters are same across all
+     * harts.
+     */
+    for (i = 0; i < ARRAY_SIZE(pmu_events_arr); i++) {
+        fdt_event_ctr_map[0 + i * 3] = cpu_to_be32(pmu_events_arr[i].event_id);
+        fdt_event_ctr_map[1 + i * 3] = cpu_to_be32(pmu_events_arr[i].event_id);
+        fdt_event_ctr_map[2 + i * 3] = cpu_to_be32(pmu_events_arr[i].counter_mask);
+    }
+    /* This a OpenSBI specific DT property documented in OpenSBI docs */
+    qemu_fdt_setprop(ms->fdt, pmu_name, "riscv,event-to-mhpmcounters",
+                     fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
 }
 
 static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index e80f0f911fa3..dd0a18ae3dc1 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -27,42 +27,6 @@
 
 #define RISCV_TIMEBASE_FREQ 1000000000 /* 1Ghz */
 
-/*
- * To keep it simple, any event can be mapped to any programmable counters in
- * QEMU. The generic cycle & instruction count events can also be monitored
- * using programmable counters. In that case, mcycle & minstret must continue
- * 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, uint32_t cmask, char *pmu_name)
-{
-    uint32_t fdt_event_ctr_map[15] = {};
-
-   fdt_event_ctr_map[0] = cpu_to_be32(VIRT_PMU_EVENT_HW_CPU_CYCLES);
-   fdt_event_ctr_map[1] = cpu_to_be32(VIRT_PMU_EVENT_HW_CPU_CYCLES);
-   fdt_event_ctr_map[2] = cpu_to_be32(cmask | 1 << 0);
-
-   fdt_event_ctr_map[3] = cpu_to_be32(VIRT_PMU_EVENT_HW_INSTRUCTIONS);
-   fdt_event_ctr_map[4] = cpu_to_be32(VIRT_PMU_EVENT_HW_INSTRUCTIONS);
-   fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
-
-   fdt_event_ctr_map[6] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS);
-   fdt_event_ctr_map[7] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_READ_MISS);
-   fdt_event_ctr_map[8] = cpu_to_be32(cmask);
-
-   fdt_event_ctr_map[9] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS);
-   fdt_event_ctr_map[10] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_DTLB_WRITE_MISS);
-   fdt_event_ctr_map[11] = cpu_to_be32(cmask);
-
-   fdt_event_ctr_map[12] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS);
-   fdt_event_ctr_map[13] = cpu_to_be32(VIRT_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS);
-   fdt_event_ctr_map[14] = cpu_to_be32(cmask);
-
-   /* This a OpenSBI specific DT property documented in OpenSBI docs */
-   qemu_fdt_setprop(fdt, pmu_name, "riscv,event-to-mhpmcounters",
-                    fdt_event_ctr_map, sizeof(fdt_event_ctr_map));
-}
-
 static bool riscv_pmu_counter_valid(RISCVCPU *cpu, uint32_t ctr_idx)
 {
     if (ctr_idx < 3 || ctr_idx >= RV_MAX_MHPMCOUNTERS ||
diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
index 810ac2fae797..10505040d9e5 100644
--- a/target/riscv/pmu.h
+++ b/target/riscv/pmu.h
@@ -31,7 +31,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, uint64_t 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);
 void riscv_pmu_update_fixed_ctrs(CPURISCVState *env, target_ulong newpriv,

-- 
2.34.1

Re: [PATCH RFC 00/10] Allow platform specific PMU event encoding

[Alexei Filippov]

On 10.10.2024 02:08, Atish Patra wrote:
> Currently, the pmu implementation is virt machine specific that
> implements the SBI PMU encodings. In the future, individual machines
> would want to implement their own custom event encodings as there
> is no standard event encoding defined by the ISA. There is a performance
> events TG which is working on defining standard set of events but not
> encodings. That allows flexibility for platforms to choose whatever
> encoding scheme they want. However, that means the generic pmu code
> should be flexible enough to allow that in Qemu as well.
> 
> This series aims to solve that problem by first disassociating the
> common pmu implementation and event encoding. The event encoding is
> specific to a platform and should be defined in the platform specific
> machine or cpu implementation. The newly defined callbacks can be invoked
> from machine specific cpu implementation or machine code itself depending
> on the requirement.
> 
> The first 5 patches in the series are generic improvements and cleanups
> where as the last 5 actually implements the disassociation for the virt
> machine. The current series can also be found at[2].
> 
> I recently found that Alexei has done a similar effort for SiFive FU740[1]
> but the implementation differs from this one based on how the cpu callbacks
> are invoked. For example, Alexei's series implements a single callback for
> all the events and has defined machine specific counter read/write callbacks.
> However, it just defaults to get_ticks() for every event. IMO, that is
> confusing to the users unless we can actually emulate more generic events or
> machine specific events.
> 
> I have separate callbacks for each type of events that we currently support
> in Qemu (cycle, instruction, TLB events). Separate callbacks seems a better
> approach to avoid ambiguity as we have very limited event capability in qemu.
> I am open to converging them to one callback as well if we think we will
> be extending set of events in the future.
> 
> Once we converge on the approaches, we can consolidate the patches
> so that both SiFive FU740 and virt machine can use the same abstraction.
> 
> Cc: alexei.filippov@syntacore.com
>
Thanks for CCing me and your patch. Your done a great work, but still I 
do not think this approach with per event callback are scalable enough. 
I'll suggest to collaborate to work your and mine solution to unite them 
to one patch set. Let me know what do you think.
> [1] https://lore.kernel.org/all/20240910174747.148141-1-alexei.filippov@syntacore.com/T/
> [2] https://github.com/atishp04/qemu/tree/b4/pmu_event_machine
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
> Atish Patra (10):
>        target/riscv: Fix the hpmevent mask
>        target/riscv: Introduce helper functions for pmu hashtable lookup
>        target/riscv: Protect the hashtable modifications with a lock
>        target/riscv: Use uint64 instead of uint as key
>        target/riscv: Rename the PMU events
>        target/riscv: Define PMU event related structures
>        hw/riscv/virt.c : Disassociate virt PMU events
>        target/riscv: Update event mapping hashtable for invalid events
>        target/riscv : Use the new tlb fill event functions
>        hw/riscv/virt.c: Generate the PMU node from the machine
> 
>   hw/riscv/virt.c           | 102 +++++++++++++++++++++++-
>   target/riscv/cpu.h        |  52 ++++++++++--
>   target/riscv/cpu_bits.h   |   4 +-
>   target/riscv/cpu_helper.c |  21 ++---
>   target/riscv/pmu.c        | 198 +++++++++++++++++++++-------------------------
>   target/riscv/pmu.h        |   3 +-
>   6 files changed, 246 insertions(+), 134 deletions(-)
> ---
> base-commit: 19a9809808a51291008f72d051d0f9b3499fc223
> change-id: 20241008-pmu_event_machine-b87c58104e61
> --
> Regards,
> Atish patra
> 

Re: [PATCH RFC 00/10] Allow platform specific PMU event encoding

[Atish Kumar Patra]

On Thu, Oct 10, 2024 at 5:51 AM Alexei Filippov
<alexei.filippov@yadro.com> wrote:
>
>
>
> On 10.10.2024 02:08, Atish Patra wrote:
> > Currently, the pmu implementation is virt machine specific that
> > implements the SBI PMU encodings. In the future, individual machines
> > would want to implement their own custom event encodings as there
> > is no standard event encoding defined by the ISA. There is a performance
> > events TG which is working on defining standard set of events but not
> > encodings. That allows flexibility for platforms to choose whatever
> > encoding scheme they want. However, that means the generic pmu code
> > should be flexible enough to allow that in Qemu as well.
> >
> > This series aims to solve that problem by first disassociating the
> > common pmu implementation and event encoding. The event encoding is
> > specific to a platform and should be defined in the platform specific
> > machine or cpu implementation. The newly defined callbacks can be invoked
> > from machine specific cpu implementation or machine code itself depending
> > on the requirement.
> >
> > The first 5 patches in the series are generic improvements and cleanups
> > where as the last 5 actually implements the disassociation for the virt
> > machine. The current series can also be found at[2].
> >
> > I recently found that Alexei has done a similar effort for SiFive FU740[1]
> > but the implementation differs from this one based on how the cpu callbacks
> > are invoked. For example, Alexei's series implements a single callback for
> > all the events and has defined machine specific counter read/write callbacks.
> > However, it just defaults to get_ticks() for every event. IMO, that is
> > confusing to the users unless we can actually emulate more generic events or
> > machine specific events.
> >
> > I have separate callbacks for each type of events that we currently support
> > in Qemu (cycle, instruction, TLB events). Separate callbacks seems a better
> > approach to avoid ambiguity as we have very limited event capability in qemu.
> > I am open to converging them to one callback as well if we think we will
> > be extending set of events in the future.
> >
> > Once we converge on the approaches, we can consolidate the patches
> > so that both SiFive FU740 and virt machine can use the same abstraction.
> >
> > Cc: alexei.filippov@syntacore.com
> >
> Thanks for CCing me and your patch. Your done a great work, but still I
> do not think this approach with per event callback are scalable enough.
> I'll suggest to collaborate to work your and mine solution to unite them
> to one patch set. Let me know what do you think.

Yes. We should definitely collaborate and send a single series to support both
virt and sifive machines. You had a question about widening the
hpmevent in your series.

(Answering here to keep the discussion in 1 place)

As per the section 18.1. Count Overflow Control, only top 8 bits are reserved.
Thus, a platform can implement their event encoding to upto 56 bit wide.

> > [1] https://lore.kernel.org/all/20240910174747.148141-1-alexei.filippov@syntacore.com/T/
> > [2] https://github.com/atishp04/qemu/tree/b4/pmu_event_machine
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> > Atish Patra (10):
> >        target/riscv: Fix the hpmevent mask
> >        target/riscv: Introduce helper functions for pmu hashtable lookup
> >        target/riscv: Protect the hashtable modifications with a lock
> >        target/riscv: Use uint64 instead of uint as key
> >        target/riscv: Rename the PMU events
> >        target/riscv: Define PMU event related structures
> >        hw/riscv/virt.c : Disassociate virt PMU events
> >        target/riscv: Update event mapping hashtable for invalid events
> >        target/riscv : Use the new tlb fill event functions
> >        hw/riscv/virt.c: Generate the PMU node from the machine
> >
> >   hw/riscv/virt.c           | 102 +++++++++++++++++++++++-
> >   target/riscv/cpu.h        |  52 ++++++++++--
> >   target/riscv/cpu_bits.h   |   4 +-
> >   target/riscv/cpu_helper.c |  21 ++---
> >   target/riscv/pmu.c        | 198 +++++++++++++++++++++-------------------------
> >   target/riscv/pmu.h        |   3 +-
> >   6 files changed, 246 insertions(+), 134 deletions(-)
> > ---
> > base-commit: 19a9809808a51291008f72d051d0f9b3499fc223
> > change-id: 20241008-pmu_event_machine-b87c58104e61
> > --
> > Regards,
> > Atish patra
> >