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>

---
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

On 10/14/25 1:34 PM, Anton Johansson 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>
> 
> ---
> 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.

In case there is a doubt, having an assert for this situation would make 
it future proof.
g_assert(!(rv32 && !upper_half));

As well, maybe Alistair can point a call site where this combination is 
possible.

> ---
>   target/riscv/csr.c | 22 ++++++++++------------
>   1 file changed, 10 insertions(+), 12 deletions(-)
>