On 03/10/25, Alistair Francis wrote: > 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]> >
Did I miss something here? Isn't writing the lower 32 bits correct? > > 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 I'll take a look at the flags, thanks!:)
