On Fri, Oct 3, 2025 at 5:37 AM Pierrick Bouvier <[email protected]> wrote: > > 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.
Besides this missed part the patch looks good. Reviewed-by: Alistair Francis <[email protected]> > 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. That might be a bug, it should be using MHPMEVENTH_BIT_OF with mhpmeventh_val Alistair > Existing implementation is quite confusing to be honest. >
