>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]> --- NOTE: I've not included any reviewed-bys or modified this patch as it's still unclear to me whether this change is correct or not. Alistair mentioned that this can get called for MXLEN == 32 and upper_half == false, meaning the lower field would be accessed. I'm sure I'm missing something but this is still not clear to me, it seems to me like we always want to access the upper half for MXLEN == 32 since that's were the inhibit flags are stored. --- 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; } static RISCVException riscv_pmu_write_ctr(CPURISCVState *env, target_ulong val, -- 2.51.0
