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

Richard Henderson posted 61 patches 1 month ago
[PATCH 14/61] target/arm: Replace cpreg_field_is_64bit with cpreg_field_type
Posted by Richard Henderson 1 month ago
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)
 {
-    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
Re: [PATCH 14/61] target/arm: Replace cpreg_field_is_64bit with cpreg_field_type
Posted by Manos Pitsidianakis 1 month ago
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>
Re: [PATCH 14/61] target/arm: Replace cpreg_field_is_64bit with cpreg_field_type
Posted by Richard Henderson 3 weeks, 4 days ago
On 8/29/25 09:13, Manos Pitsidianakis wrote:
> 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.  */

I'm not sure this is helpful.  We already (ab)use MO_n quite often as a symbolic name for 
log8(n) outside of the context of memory operations.


r~


PS. If I were to do it over I'd always count in bytes not bits, since bits are not 
addressable.  Though I suppose we might have been lead by uintN_t and friends.