Re: [PATCH v2 05/33] target/riscv: Combine mhpmevent and mhpmeventh

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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 <anjo@rev.ng>
> ---
>   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.