Re: [PATCH 14/61] target/arm: Replace cpreg_field_is_64bit with cpreg_field_type

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Wed, 27 Aug 2025 04:04, Richard Henderson <richard.henderson@linaro.org> wrote:
>Prepare for 128-bit fields by using a better query api.
>
>Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>---
> target/arm/cpregs.h  | 10 ++++++----
> target/arm/gdbstub.c |  7 +++++--
> target/arm/helper.c  | 18 +++++++++++++-----
> 3 files changed, 24 insertions(+), 11 deletions(-)
>
>diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
>index 812fb1340a..b6c8eff0dd 100644
>--- a/target/arm/cpregs.h
>+++ b/target/arm/cpregs.h
>@@ -22,6 +22,7 @@
> #define TARGET_ARM_CPREGS_H
> 
> #include "hw/registerfields.h"
>+#include "exec/memop.h"
> #include "target/arm/kvm-consts.h"
> #include "cpu.h"
> 
>@@ -1053,12 +1054,13 @@ void raw_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value);
> void arm_cp_reset_ignore(CPUARMState *env, const ARMCPRegInfo *ri);
> 
> /*
>- * Return true if this reginfo struct's field in the cpu state struct
>- * is 64 bits wide.
>+ * Return MO_32 if the field in CPUARMState is uint32_t or
>+ * MO_64 if the field in CPUARMState is uint64_t.
>  */
>-static inline bool cpreg_field_is_64bit(const ARMCPRegInfo *ri)
>+static inline MemOp cpreg_field_type(const ARMCPRegInfo *ri)

Using MemOp is slightly confusing though I understand where you're 
coming from. Would introducing a BitWidth enum be a good idea?


diff --git a/include/exec/memop.h b/include/exec/memop.h
index cf7da3362e..9104c4f162 100644
--- a/include/exec/memop.h
+++ b/include/exec/memop.h
@@ -14,15 +14,26 @@
 
 #include "qemu/host-utils.h"
 
+typedef enum BitWidth {
+    BW_8     = 0,
+    BW_16    = 1,
+    BW_32    = 2,
+    BW_64    = 3,
+    BW_128   = 4,
+    BW_256   = 5,
+    BW_512   = 6,
+    BW_1024  = 7,
+}
+
 typedef enum MemOp {
-    MO_8     = 0,
-    MO_16    = 1,
-    MO_32    = 2,
-    MO_64    = 3,
-    MO_128   = 4,
-    MO_256   = 5,
-    MO_512   = 6,
-    MO_1024  = 7,
+    MO_8     = BW_8,
+    MO_16    = BW_16,
+    MO_32    = BW_32,
+    MO_64    = BW_64,
+    MO_128   = BW_128,
+    MO_256   = BW_256,
+    MO_512   = BW_512,
+    MO_1024  = BW_1024,
     MO_SIZE  = 0x07,   /* Mask for the above.  */

> {
>-    return (ri->state == ARM_CP_STATE_AA64) || (ri->type & ARM_CP_64BIT);
>+    return (ri->state == ARM_CP_STATE_AA64 || (ri->type & ARM_CP_64BIT)
>+            ? MO_64 : MO_32);
> }
> 
> static inline bool cp_access_ok(int current_el,
>diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
>index ce4497ad7c..e2fc389170 100644
>--- a/target/arm/gdbstub.c
>+++ b/target/arm/gdbstub.c
>@@ -247,10 +247,13 @@ static int arm_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
>     key = cpu->dyn_sysreg_feature.data.cpregs.keys[reg];
>     ri = get_arm_cp_reginfo(cpu->cp_regs, key);
>     if (ri) {
>-        if (cpreg_field_is_64bit(ri)) {
>+        switch (cpreg_field_type(ri)) {
>+        case MO_64:
>             return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri));
>-        } else {
>+        case MO_32:
>             return gdb_get_reg32(buf, (uint32_t)read_raw_cp_reg(env, ri));
>+        default:
>+            g_assert_not_reached();
>         }
>     }
>     return 0;
>diff --git a/target/arm/helper.c b/target/arm/helper.c
>index 3a9d8f0ddc..c4103d958a 100644
>--- a/target/arm/helper.c
>+++ b/target/arm/helper.c
>@@ -63,20 +63,28 @@ int compare_u64(const void *a, const void *b)
> uint64_t raw_read(CPUARMState *env, const ARMCPRegInfo *ri)
> {
>     assert(ri->fieldoffset);
>-    if (cpreg_field_is_64bit(ri)) {
>+    switch (cpreg_field_type(ri)) {
>+    case MO_64:
>         return CPREG_FIELD64(env, ri);
>-    } else {
>+    case MO_32:
>         return CPREG_FIELD32(env, ri);
>+    default:
>+        g_assert_not_reached();
>     }
> }
> 
> void raw_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
> {
>     assert(ri->fieldoffset);
>-    if (cpreg_field_is_64bit(ri)) {
>+    switch (cpreg_field_type(ri)) {
>+    case MO_64:
>         CPREG_FIELD64(env, ri) = value;
>-    } else {
>+        break;
>+    case MO_32:
>         CPREG_FIELD32(env, ri) = value;
>+        break;
>+    default:
>+        g_assert_not_reached();
>     }
> }
> 
>@@ -2748,7 +2756,7 @@ static void vmsa_ttbr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                             uint64_t value)
> {
>     /* If the ASID changes (with a 64-bit write), we must flush the TLB.  */
>-    if (cpreg_field_is_64bit(ri) &&
>+    if (cpreg_field_type(ri) == MO_64 &&
>         extract64(raw_read(env, ri) ^ value, 48, 16) != 0) {
>         ARMCPU *cpu = env_archcpu(env);
>         tlb_flush(CPU(cpu));
>-- 
>2.43.0
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>