On Wed, Oct 1, 2025 at 5:43 PM 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.
If MXLEN == 32, upper_half == false then we want to read mcyclecfg, which the code today seems to be doing correctly. > > Minor simplifications are also made along with some formatting fixes. > > Signed-off-by: Anton Johansson <[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 3c8989f522..859f89aedd 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" > @@ -1241,18 +1242,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) : This doesn't look right. RV32 will want to access both mcyclecfgh and mcyclecfg, but this change restricts access to mcyclecfg as rv32 will always be true. I don't think there is anything wrong with the current code. Alistair > 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; > @@ -1260,7 +1264,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(); > } > @@ -1292,13 +1296,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; > } > > static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong > val, > -- > 2.51.0 > >
