On 31/10/25, Alistair Francis wrote:
> On Tue, Oct 28, 2025 at 4:23 AM Anton Johansson via
> <qemu-devel@nongnu.org> 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 <anjo@rev.ng>
> > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > ---
> > 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!:)