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. 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? Alistair > } > > static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong > val, > -- > 2.51.0 > >
