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

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

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 <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.

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.