[RFC PATCH 05/34] target/riscv: Bugfix riscv_pmu_ctr_get_fixed_counters_val()

Anton Johansson via posted 34 patches 4 days, 8 hours ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Laurent Vivier <laurent@vivier.eu>, Christoph Muellner <christoph.muellner@vrull.eu>
[RFC PATCH 05/34] target/riscv: Bugfix riscv_pmu_ctr_get_fixed_counters_val()
Posted by Anton Johansson via 4 days, 8 hours ago
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>
---
 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 8842e07a73..5997b71b43 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"
@@ -1242,18 +1243,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;
@@ -1261,7 +1265,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();
         }
@@ -1293,13 +1297,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