On 10/2/25 12:08 PM, Pierrick Bouvier wrote:
On 10/1/25 12:32 AM, Anton Johansson wrote:
According to version 20250508 of the privileged specification,
mhpmeventn is 64 bits in size and mhpmeventnh is only ever used
when XLEN == 32 and accesses the top 32 bits of the 64-bit
mhpmeventn registers. Combine the two arrays of target_ulong
mhpmeventh[] and mhpmevent[] to a single array of uint64_t.

This also allows for some minor code simplification where branches
handling either mhpmeventh[] or mhpmevent[] could be combined.

Signed-off-by: Anton Johansson <[email protected]>
---
   target/riscv/cpu.h     | 10 +++----
   target/riscv/csr.c     | 67 +++++++++++++++---------------------------
   target/riscv/machine.c |  3 +-
   target/riscv/pmu.c     | 53 ++++++++-------------------------
   4 files changed, 42 insertions(+), 91 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 3235108112..64b9964028 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -427,11 +427,11 @@ struct CPUArchState {
       /* PMU counter state */
       PMUCTRState pmu_ctrs[RV_MAX_MHPMCOUNTERS];
- /* PMU event selector configured values. First three are unused */
-    target_ulong mhpmevent_val[RV_MAX_MHPMEVENTS];
-
-    /* PMU event selector configured values for RV32 */
-    target_ulong mhpmeventh_val[RV_MAX_MHPMEVENTS];
+    /*
+     * PMU event selector configured values. First three are unused.
+     * For RV32 top 32 bits are accessed via the mhpmeventh CSR.
+     */
+    uint64_t mhpmevent_val[RV_MAX_MHPMEVENTS];
PMUFixedCtrState pmu_fixed_ctrs[2]; diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 859f89aedd..2d8916ee40 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1166,8 +1166,9 @@ static RISCVException read_mhpmevent(CPURISCVState *env, 
int csrno,
                                        target_ulong *val)
   {
       int evt_index = csrno - CSR_MCOUNTINHIBIT;
+    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32;
- *val = env->mhpmevent_val[evt_index];
+    *val = extract64(env->mhpmevent_val[evt_index], 0, rv32 ? 32 : 64);
return RISCV_EXCP_NONE;
   }
@@ -1176,13 +1177,11 @@ static RISCVException write_mhpmevent(CPURISCVState 
*env, int csrno,
                                         target_ulong val, uintptr_t ra)
   {
       int evt_index = csrno - CSR_MCOUNTINHIBIT;
-    uint64_t mhpmevt_val = val;
+    uint64_t mhpmevt_val;
       uint64_t inh_avail_mask;
if (riscv_cpu_mxl(env) == MXL_RV32) {
-        env->mhpmevent_val[evt_index] = val;
-        mhpmevt_val = mhpmevt_val |
-                      ((uint64_t)env->mhpmeventh_val[evt_index] << 32);
+        mhpmevt_val = deposit64(env->mhpmevent_val[evt_index], 0, 32, val);

Maybe I missed something, but should it be:
deposit64(env->mhpmevent_val[evt_index], 32, 32, val)
instead?

Reading the rest of the patch, I'm a bit confused about which bits are
supposed to be used in 32/64 mode.

Indeed I missed something, it's more clear with next patchs combining low/high parts. The concern I have that is left is regarding the definition of MHPMEVENT_BIT_OF. It seems to be out of sync with what we have now given that we now keep lower part in lower bits.
Existing implementation is quite confusing to be honest.

Reply via email to