From: Brian Cain <bcain@quicinc.com>

Co-authored-by: Sid Manning <sidneym@quicinc.com>
Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/cpu_helper.h |   8 +++
 target/hexagon/cpu.c        |   1 +
 target/hexagon/cpu_helper.c |  37 ++++++++++++
 target/hexagon/op_helper.c  | 114 ++++++++++++++++++++++++++++++++++--
 4 files changed, 156 insertions(+), 4 deletions(-)

diff --git a/target/hexagon/cpu_helper.h b/target/hexagon/cpu_helper.h
index 194bcbf451..5f5f15149a 100644
--- a/target/hexagon/cpu_helper.h
+++ b/target/hexagon/cpu_helper.h
@@ -7,6 +7,14 @@
 #ifndef HEXAGON_CPU_HELPER_H
 #define HEXAGON_CPU_HELPER_H
 
+uint32_t hexagon_get_pmu_counter(CPUHexagonState *cur_env, int index);
+uint64_t hexagon_get_sys_pcycle_count(CPUHexagonState *env);
+uint32_t hexagon_get_sys_pcycle_count_low(CPUHexagonState *env);
+uint32_t hexagon_get_sys_pcycle_count_high(CPUHexagonState *env);
+void hexagon_set_sys_pcycle_count(CPUHexagonState *env, uint64_t);
+void hexagon_set_sys_pcycle_count_low(CPUHexagonState *env, uint32_t);
+void hexagon_set_sys_pcycle_count_high(CPUHexagonState *env, uint32_t);
+
 static inline void arch_set_thread_reg(CPUHexagonState *env, uint32_t reg,
                                        uint32_t val)
 {
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 0db91a936a..36a93cc22f 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -322,6 +322,7 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
     qemu_init_vcpu(cs);
     cpu_reset(cs);
 #ifndef CONFIG_USER_ONLY
+    CPUHexagonState *env = cpu_env(cs);
     if (cs->cpu_index == 0) {
         env->g_sreg = g_new0(target_ulong, NUM_SREGS);
     } else {
diff --git a/target/hexagon/cpu_helper.c b/target/hexagon/cpu_helper.c
index 6e4bc85580..0062253176 100644
--- a/target/hexagon/cpu_helper.c
+++ b/target/hexagon/cpu_helper.c
@@ -27,10 +27,47 @@
 
 #ifndef CONFIG_USER_ONLY
 
+uint32_t hexagon_get_pmu_counter(CPUHexagonState *cur_env, int index)
+{
+    g_assert_not_reached();
+}
+
 uint32_t arch_get_system_reg(CPUHexagonState *env, uint32_t reg)
 {
     g_assert_not_reached();
 }
 
+uint64_t hexagon_get_sys_pcycle_count(CPUHexagonState *env)
+{
+    g_assert_not_reached();
+}
+
+uint32_t hexagon_get_sys_pcycle_count_high(CPUHexagonState *env)
+{
+    g_assert_not_reached();
+}
+
+uint32_t hexagon_get_sys_pcycle_count_low(CPUHexagonState *env)
+{
+    g_assert_not_reached();
+}
+
+void hexagon_set_sys_pcycle_count_high(CPUHexagonState *env,
+        uint32_t cycles_hi)
+{
+    g_assert_not_reached();
+}
+
+void hexagon_set_sys_pcycle_count_low(CPUHexagonState *env,
+        uint32_t cycles_lo)
+{
+    g_assert_not_reached();
+}
+
+void hexagon_set_sys_pcycle_count(CPUHexagonState *env, uint64_t cycles)
+{
+    g_assert_not_reached();
+}
+
 
 #endif
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 139a0b5ab2..76b2475d88 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -17,6 +17,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
+#include "qemu/main-loop.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "exec/helper-proto.h"
@@ -1397,25 +1398,130 @@ void HELPER(setimask)(CPUHexagonState *env, uint32_t pred, uint32_t imask)
     g_assert_not_reached();
 }
 
+static bool handle_pmu_sreg_write(CPUHexagonState *env, uint32_t reg,
+                                  uint32_t val)
+{
+    if (reg == HEX_SREG_PMUSTID0 || reg == HEX_SREG_PMUSTID1
+        || reg == HEX_SREG_PMUCFG || reg == HEX_SREG_PMUEVTCFG
+        || reg == HEX_SREG_PMUEVTCFG1
+        || (reg >= HEX_SREG_PMUCNT4 && reg <= HEX_SREG_PMUCNT3)) {
+        qemu_log_mask(LOG_UNIMP, "PMU registers not yet implemented");
+        return true;
+    }
+    return false;
+}
+
+static void modify_syscfg(CPUHexagonState *env, uint32_t val)
+{
+    g_assert_not_reached();
+}
+
+static void hexagon_set_vid(CPUHexagonState *env, uint32_t offset, int val)
+{
+    g_assert_not_reached();
+}
+
+static uint32_t hexagon_find_last_irq(CPUHexagonState *env, uint32_t vid)
+{
+    g_assert_not_reached();
+}
+
+static void hexagon_read_timer(CPUHexagonState *env, uint32_t *low,
+                               uint32_t *high)
+{
+    qemu_log_mask(LOG_UNIMP, "reading timer_hi/lo not yet supported\n");
+}
+
+static inline QEMU_ALWAYS_INLINE void sreg_write(CPUHexagonState *env,
+                                                 uint32_t reg, uint32_t val)
+
+{
+    g_assert(bql_locked());
+    if ((reg == HEX_SREG_VID) || (reg == HEX_SREG_VID1)) {
+        hexagon_set_vid(env, (reg == HEX_SREG_VID) ? L2VIC_VID_0 : L2VIC_VID_1,
+                        val);
+        arch_set_system_reg(env, reg, val);
+    } else if (reg == HEX_SREG_SYSCFG) {
+        modify_syscfg(env, val);
+    } else if (reg == HEX_SREG_IMASK) {
+        val = GET_FIELD(IMASK_MASK, val);
+        arch_set_system_reg(env, reg, val);
+    } else if (reg == HEX_SREG_PCYCLELO) {
+        hexagon_set_sys_pcycle_count_low(env, val);
+    } else if (reg == HEX_SREG_PCYCLEHI) {
+        hexagon_set_sys_pcycle_count_high(env, val);
+    } else if (!handle_pmu_sreg_write(env, reg, val)) {
+        if (reg >= HEX_SREG_GLB_START) {
+            arch_set_system_reg(env, reg, val);
+        } else {
+            arch_set_system_reg(env, reg, val);
+        }
+    }
+}
+
 void HELPER(sreg_write)(CPUHexagonState *env, uint32_t reg, uint32_t val)
 {
-    g_assert_not_reached();
+    BQL_LOCK_GUARD();
+    sreg_write(env, reg, val);
 }
 
 void HELPER(sreg_write_pair)(CPUHexagonState *env, uint32_t reg, uint64_t val)
+{
+    BQL_LOCK_GUARD();
+    sreg_write(env, reg, val & 0xFFFFFFFF);
+    sreg_write(env, reg + 1, val >> 32);
+}
 
+static inline QEMU_ALWAYS_INLINE uint32_t sreg_read(CPUHexagonState *env,
+                                                    uint32_t reg)
 {
-    g_assert_not_reached();
+    g_assert(bql_locked());
+    if (reg == HEX_SREG_PMUSTID0 || reg == HEX_SREG_PMUSTID1
+        || reg == HEX_SREG_PMUCFG || reg == HEX_SREG_PMUEVTCFG
+        || reg == HEX_SREG_PMUEVTCFG1
+        || (reg >= HEX_SREG_PMUCNT4 && reg <= HEX_SREG_PMUCNT3)) {
+        qemu_log_mask(LOG_UNIMP, "PMU registers not yet implemented");
+        return 0;
+    }
+    if ((reg == HEX_SREG_VID) || (reg == HEX_SREG_VID1)) {
+        const uint32_t vid = hexagon_find_last_irq(env, reg);
+        arch_set_system_reg(env, reg, vid);
+    } else if ((reg == HEX_SREG_TIMERLO) || (reg == HEX_SREG_TIMERHI)) {
+        uint32_t low = 0;
+        uint32_t high = 0;
+        hexagon_read_timer(env, &low, &high);
+        arch_set_system_reg(env, HEX_SREG_TIMERLO, low);
+        arch_set_system_reg(env, HEX_SREG_TIMERHI, high);
+    } else if (reg == HEX_SREG_BADVA) {
+        target_ulong ssr = arch_get_system_reg(env, HEX_SREG_SSR);
+        if (GET_SSR_FIELD(SSR_BVS, ssr)) {
+            return arch_get_system_reg(env, HEX_SREG_BADVA1);
+        }
+        return arch_get_system_reg(env, HEX_SREG_BADVA0);
+    }
+    return arch_get_system_reg(env, reg);
 }
 
 uint32_t HELPER(sreg_read)(CPUHexagonState *env, uint32_t reg)
 {
-    g_assert_not_reached();
+    BQL_LOCK_GUARD();
+    return sreg_read(env, reg);
 }
 
 uint64_t HELPER(sreg_read_pair)(CPUHexagonState *env, uint32_t reg)
 {
-    g_assert_not_reached();
+    BQL_LOCK_GUARD();
+    if (reg == HEX_SREG_TIMERLO) {
+        uint32_t low = 0;
+        uint32_t high = 0;
+        hexagon_read_timer(env, &low, &high);
+        arch_set_system_reg(env, HEX_SREG_TIMERLO, low);
+        arch_set_system_reg(env, HEX_SREG_TIMERHI, high);
+    } else if (reg == HEX_SREG_PCYCLELO) {
+        return hexagon_get_sys_pcycle_count(env);
+    }
+    return   (uint64_t)sreg_read(env, reg) |
+           (((uint64_t)sreg_read(env, reg + 1)) << 32);
 }
 
 uint32_t HELPER(greg_read)(CPUHexagonState *env, uint32_t reg)
-- 
2.34.1

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Friday, February 28, 2025 11:26 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; quic_mathbern@quicinc.com; ale@rev.ng; anjo@rev.ng;
> quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com; Brian Cain <bcain@quicinc.com>
> Subject: [PATCH 27/38] target/hexagon: Add sreg_{read,write} helpers
> 
> From: Brian Cain <bcain@quicinc.com>
> 
> Co-authored-by: Sid Manning <sidneym@quicinc.com>
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/cpu_helper.h |   8 +++
>  target/hexagon/cpu.c        |   1 +
>  target/hexagon/cpu_helper.c |  37 ++++++++++++
> target/hexagon/op_helper.c  | 114
> ++++++++++++++++++++++++++++++++++--
>  4 files changed, 156 insertions(+), 4 deletions(-)
> 


diff --git a/target/hexagon/cpu.c
> b/target/hexagon/cpu.c index 0db91a936a..36a93cc22f 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -322,6 +322,7 @@ static void hexagon_cpu_realize(DeviceState *dev,
> Error **errp)
>      qemu_init_vcpu(cs);
>      cpu_reset(cs);
>  #ifndef CONFIG_USER_ONLY
> +    CPUHexagonState *env = cpu_env(cs);

Is there a use for this?  If it's in a later patch, move this declaration there.

>      if (cs->cpu_index == 0) {
>          env->g_sreg = g_new0(target_ulong, NUM_SREGS);
>      } else {


> diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
> index 139a0b5ab2..76b2475d88 100644
> --- a/target/hexagon/op_helper.c
> +++ b/target/hexagon/op_helper.c
> @@ -17,6 +17,7 @@
> 
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
> +#include "qemu/main-loop.h"
>  #include "exec/exec-all.h"
>  #include "exec/cpu_ldst.h"
>  #include "exec/helper-proto.h"
> @@ -1397,25 +1398,130 @@ void HELPER(setimask)(CPUHexagonState *env,
> uint32_t pred, uint32_t imask)
>      g_assert_not_reached();
>  }
> 
> +static bool handle_pmu_sreg_write(CPUHexagonState *env, uint32_t reg,
> +                                  uint32_t val) {
> +    if (reg == HEX_SREG_PMUSTID0 || reg == HEX_SREG_PMUSTID1
> +        || reg == HEX_SREG_PMUCFG || reg == HEX_SREG_PMUEVTCFG
> +        || reg == HEX_SREG_PMUEVTCFG1
> +        || (reg >= HEX_SREG_PMUCNT4 && reg <= HEX_SREG_PMUCNT3)) {
> +        qemu_log_mask(LOG_UNIMP, "PMU registers not yet implemented");
> +        return true;
> +    }
> +    return false;
> +}
> +

Poor name for this function.  It's not *handling* the write, it's checking for a set of registers.  Until PMU registers are implemented, it's hard to comment on the correctness of the check.

> +static inline QEMU_ALWAYS_INLINE void sreg_write(CPUHexagonState
> *env,
> +                                                 uint32_t reg, uint32_t
> +val)
> +
> +{
> +    g_assert(bql_locked());
> +    if ((reg == HEX_SREG_VID) || (reg == HEX_SREG_VID1)) {
> +        hexagon_set_vid(env, (reg == HEX_SREG_VID) ? L2VIC_VID_0 :
> L2VIC_VID_1,
> +                        val);
> +        arch_set_system_reg(env, reg, val);
> +    } else if (reg == HEX_SREG_SYSCFG) {
> +        modify_syscfg(env, val);
> +    } else if (reg == HEX_SREG_IMASK) {
> +        val = GET_FIELD(IMASK_MASK, val);
> +        arch_set_system_reg(env, reg, val);
> +    } else if (reg == HEX_SREG_PCYCLELO) {
> +        hexagon_set_sys_pcycle_count_low(env, val);
> +    } else if (reg == HEX_SREG_PCYCLEHI) {
> +        hexagon_set_sys_pcycle_count_high(env, val);
> +    } else if (!handle_pmu_sreg_write(env, reg, val)) {

This should be
    } else if (handle_pmu_sreg_write(...)) {
        qemu_log_mask(LOG_UNIMP, ...);
    } else {
That leaves a better spot for you to come back in the future and add the implementation.

> +        if (reg >= HEX_SREG_GLB_START) {
> +            arch_set_system_reg(env, reg, val);
> +        } else {
> +            arch_set_system_reg(env, reg, val);
> +        }

Why the check when the two conditions do the same thing?

> +    }
> +}
> +
>  void HELPER(sreg_write)(CPUHexagonState *env, uint32_t reg, uint32_t val)
> {
> -    g_assert_not_reached();
> +    BQL_LOCK_GUARD();
> +    sreg_write(env, reg, val);
>  }
> 
>  void HELPER(sreg_write_pair)(CPUHexagonState *env, uint32_t reg,
> uint64_t val)
> +{
> +    BQL_LOCK_GUARD();
> +    sreg_write(env, reg, val & 0xFFFFFFFF);
> +    sreg_write(env, reg + 1, val >> 32); }
> 
> +static inline QEMU_ALWAYS_INLINE uint32_t sreg_read(CPUHexagonState
> *env,
> +                                                    uint32_t reg)
>  {
> -    g_assert_not_reached();
> +    g_assert(bql_locked());
> +    if (reg == HEX_SREG_PMUSTID0 || reg == HEX_SREG_PMUSTID1
> +        || reg == HEX_SREG_PMUCFG || reg == HEX_SREG_PMUEVTCFG
> +        || reg == HEX_SREG_PMUEVTCFG1
> +        || (reg >= HEX_SREG_PMUCNT4 && reg <= HEX_SREG_PMUCNT3)) {
> +        qemu_log_mask(LOG_UNIMP, "PMU registers not yet implemented");
> +        return 0;
> +    }
> +    if ((reg == HEX_SREG_VID) || (reg == HEX_SREG_VID1)) {
> +        const uint32_t vid = hexagon_find_last_irq(env, reg);
> +        arch_set_system_reg(env, reg, vid);
> +    } else if ((reg == HEX_SREG_TIMERLO) || (reg == HEX_SREG_TIMERHI)) {
> +        uint32_t low = 0;
> +        uint32_t high = 0;
> +        hexagon_read_timer(env, &low, &high);
> +        arch_set_system_reg(env, HEX_SREG_TIMERLO, low);
> +        arch_set_system_reg(env, HEX_SREG_TIMERHI, high);
> +    } else if (reg == HEX_SREG_BADVA) {
> +        target_ulong ssr = arch_get_system_reg(env, HEX_SREG_SSR);
> +        if (GET_SSR_FIELD(SSR_BVS, ssr)) {
> +            return arch_get_system_reg(env, HEX_SREG_BADVA1);
> +        }
> +        return arch_get_system_reg(env, HEX_SREG_BADVA0);
> +    }
> +    return arch_get_system_reg(env, reg);
>  }
> 
>  uint32_t HELPER(sreg_read)(CPUHexagonState *env, uint32_t reg)  {
> -    g_assert_not_reached();
> +    BQL_LOCK_GUARD();
> +    return sreg_read(env, reg);
>  }
> 
>  uint64_t HELPER(sreg_read_pair)(CPUHexagonState *env, uint32_t reg)  {
> -    g_assert_not_reached();
> +    BQL_LOCK_GUARD();
> +    if (reg == HEX_SREG_TIMERLO) {
> +        uint32_t low = 0;
> +        uint32_t high = 0;
> +        hexagon_read_timer(env, &low, &high);
> +        arch_set_system_reg(env, HEX_SREG_TIMERLO, low);
> +        arch_set_system_reg(env, HEX_SREG_TIMERHI, high);

Why handle this here instead of relying on sreg_read?

> +    } else if (reg == HEX_SREG_PCYCLELO) {
> +        return hexagon_get_sys_pcycle_count(env);

Why isn't this handled in sreg_read?

> +    }
> +    return   (uint64_t)sreg_read(env, reg) |
> +           (((uint64_t)sreg_read(env, reg + 1)) << 32);
>  }
> 
>  uint32_t HELPER(greg_read)(CPUHexagonState *env, uint32_t reg)
> --
> 2.34.1