On 31/10/25, Alistair Francis wrote: > On Tue, Oct 28, 2025 at 4:23 AM Anton Johansson via > <[email protected]> wrote: > > > > From my understanding the upper_half argument only indicates whether the > > upper or lower 32 bits should be returned, and upper_half will only ever > > be set when MXLEN == 32. However, the function also uses upper_half to > > determine whether the inhibit flags are located in mcyclecfgh or > > mcyclecfg, but this misses the case where MXLEN == 32, upper_half == false > > for TARGET_RISCV32 where we would also need to read the upper half field. > > > > Minor simplifications are also made along with some formatting fixes. > > > > Signed-off-by: Anton Johansson <[email protected]> > > Reviewed-by: Pierrick Bouvier <[email protected]> > > --- > > target/riscv/csr.c | 22 ++++++++++------------ > > 1 file changed, 10 insertions(+), 12 deletions(-) > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index 5c91658c3d..657179a983 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -17,6 +17,7 @@ > > * this program. If not, see <http://www.gnu.org/licenses/>. > > */ > > > > +#include "cpu_bits.h" > > #include "qemu/osdep.h" > > #include "qemu/log.h" > > #include "qemu/timer.h" > > @@ -1243,18 +1244,21 @@ static target_ulong > > riscv_pmu_ctr_get_fixed_counters_val(CPURISCVState *env, > > int inst = riscv_pmu_ctr_monitor_instructions(env, counter_idx); > > uint64_t *counter_arr_virt = env->pmu_fixed_ctrs[inst].counter_virt; > > uint64_t *counter_arr = env->pmu_fixed_ctrs[inst].counter; > > - target_ulong result = 0; > > uint64_t curr_val = 0; > > uint64_t cfg_val = 0; > > + bool rv32 = riscv_cpu_mxl(env) == MXL_RV32; > > + > > + /* Ensure upper_half is only set for MXL_RV32 */ > > + g_assert(rv32 || !upper_half); > > > > if (counter_idx == 0) { > > - cfg_val = upper_half ? ((uint64_t)env->mcyclecfgh << 32) : > > + cfg_val = rv32 ? ((uint64_t)env->mcyclecfgh << 32) : > > env->mcyclecfg; > > } else if (counter_idx == 2) { > > - cfg_val = upper_half ? ((uint64_t)env->minstretcfgh << 32) : > > + cfg_val = rv32 ? ((uint64_t)env->minstretcfgh << 32) : > > env->minstretcfg; > > } else { > > - cfg_val = upper_half ? > > + cfg_val = rv32 ? > > ((uint64_t)env->mhpmeventh_val[counter_idx] << 32) : > > env->mhpmevent_val[counter_idx]; > > cfg_val &= MHPMEVENT_FILTER_MASK; > > @@ -1262,7 +1266,7 @@ static target_ulong > > riscv_pmu_ctr_get_fixed_counters_val(CPURISCVState *env, > > > > if (!cfg_val) { > > if (icount_enabled()) { > > - curr_val = inst ? icount_get_raw() : icount_get(); > > + curr_val = inst ? icount_get_raw() : icount_get(); > > } else { > > curr_val = cpu_get_host_ticks(); > > } > > @@ -1294,13 +1298,7 @@ static target_ulong > > riscv_pmu_ctr_get_fixed_counters_val(CPURISCVState *env, > > } > > > > done: > > - if (riscv_cpu_mxl(env) == MXL_RV32) { > > - result = upper_half ? curr_val >> 32 : curr_val; > > - } else { > > - result = curr_val; > > - } > > - > > - return result; > > + return upper_half ? curr_val >> 32 : curr_val; > > This isn't right, why shift this back if it potentially wasn't shifted > in the first place.
Ah right, I was assuming the value would always be shifted for rv32. > > This patch should be dropped from the series. If you want I'm happy to > rebase the followup patches > > target/riscv: Combine mhpmevent and mhpmeventh > target/riscv: Combine mcyclecfg and mcyclecfgh > target/riscv: Combine minstretcfg and minstretcfgh > target/riscv: Combine mhpmcounter and mhpmcounterh > > without this patch and send them? Thank you, that would be much appreciated!:)
