[PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Alex Bennée]

The aim of this work is to get rid of the endian aware helpers in
gdbstub/helpers.h which due to their use of tswap() mean target
gdbstubs need to be built multiple times. While this series doesn't
actually build each stub once it introduces a new helper -
gdb_get_register_value() which takes a MemOp which can describe the
current endian state of the system. This will be a lot easier to
dynamically feed from a helper function.

The most complex example is PPC which has a helper called
ppc_maybe_bswap_register() which was doing this.

This is still an RFC so I'm interested in feedback:

  - is the API sane
  - can we avoid lots of (uint8_t *) casting?
  - should we have a reverse helper for setting registers

If this seems like the right approach I can have a go at more of the
frontends later.

There are a few other misc clean-ups I did on the way which might be
worth cherry picking for 10.0 but I'll leave that up to maintainers.

Alex.

Alex Bennée (10):
  include/gdbstub: fix include guard in commands.h
  gdbstub: introduce target independent gdb register helper
  target/arm: convert 32 bit gdbstub to new helper
  target/arm: convert 64 bit gdbstub to new helper
  target/ppc: expand comment on FP/VMX/VSX access functions
  target/ppc: make ppc_maybe_bswap_register static
  target/ppc: convert gdbstub to new helper (!hacky)
  gdbstub: assert earlier in handle_read_all_regs
  include/exec: fix assert in size_memop
  target/microblaze: convert gdbstub to new helper

 include/exec/memop.h        |   4 +-
 include/gdbstub/commands.h  |   2 +-
 include/gdbstub/registers.h |  30 ++++++
 target/ppc/cpu.h            |   8 +-
 gdbstub/gdbstub.c           |  24 ++++-
 target/arm/gdbstub.c        |  57 +++++++----
 target/arm/gdbstub64.c      |  53 ++++++----
 target/microblaze/gdbstub.c |  44 ++++----
 target/ppc/gdbstub.c        | 194 ++++++++++++++++++++----------------
 9 files changed, 257 insertions(+), 159 deletions(-)
 create mode 100644 include/gdbstub/registers.h

-- 
2.39.5

[PATCH 01/10] include/gdbstub: fix include guard in commands.h

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/gdbstub/commands.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
index 40f0514fe9..bff3674872 100644
--- a/include/gdbstub/commands.h
+++ b/include/gdbstub/commands.h
@@ -1,5 +1,5 @@
 #ifndef GDBSTUB_COMMANDS_H
-#define GDBSTUB
+#define GDBSTUB_COMMANDS_H
 
 typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
 
-- 
2.39.5

[PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Alex Bennée]

The current helper.h functions rely on hard coded assumptions about
target endianess to use the tswap macros. We also end up double
swapping a bunch of values if the target can run in multiple endianess
modes. Avoid this by getting the target to pass the endianess and size
via a MemOp and fixing up appropriately.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
 gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
 2 files changed, 52 insertions(+)
 create mode 100644 include/gdbstub/registers.h

diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
new file mode 100644
index 0000000000..4abc7a6ae7
--- /dev/null
+++ b/include/gdbstub/registers.h
@@ -0,0 +1,30 @@
+/*
+ * GDB Common Register Helpers
+ *
+ * Copyright (c) 2025 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef GDB_REGISTERS_H
+#define GDB_REGISTERS_H
+
+#include "exec/memop.h"
+
+/**
+ * gdb_get_register_value() - get register value for gdb
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to value in host order
+ *
+ * This replaces the previous legacy read functions with a single
+ * function to handle all sizes. Passing @mo allows the target mode to
+ * be taken into account and avoids using hard coded tswap() macros.
+ *
+ * Returns the number of bytes written to the array.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
+
+#endif /* GDB_REGISTERS_H */
+
+
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 282e13e163..3d7b1028e4 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -32,6 +32,7 @@
 #include "exec/gdbstub.h"
 #include "gdbstub/commands.h"
 #include "gdbstub/syscalls.h"
+#include "gdbstub/registers.h"
 #ifdef CONFIG_USER_ONLY
 #include "accel/tcg/vcpu-state.h"
 #include "gdbstub/user.h"
@@ -45,6 +46,7 @@
 #include "system/runstate.h"
 #include "exec/replay-core.h"
 #include "exec/hwaddr.h"
+#include "exec/memop.h"
 
 #include "internals.h"
 
@@ -551,6 +553,26 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
     return 0;
 }
 
+/*
+ * Target helper function to read value into GByteArray, target
+ * supplies the size and target endianess via the MemOp.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
+{
+    size_t bytes = memop_size(op);
+
+    if (op & MO_BSWAP) {
+        for ( int i = bytes ; i > 0; i--) {
+            g_byte_array_append(buf, &val[i - 1], 1);
+        };
+    } else {
+        g_byte_array_append(buf, val, bytes);
+    }
+
+    return bytes;
+}
+
+
 static void gdb_register_feature(CPUState *cpu, int base_reg,
                                  gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
                                  const GDBFeature *feature)
-- 
2.39.5

[PATCH 03/10] target/arm: convert 32 bit gdbstub to new helper

[Alex Bennée]

For some of the helpers we need a temporary variable to copy from
although we could add some helpers to return pointers into env in
those cases if we wanted to.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/gdbstub.c | 57 ++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 21 deletions(-)

diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 30068c2262..14d931b0bf 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -20,7 +20,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "exec/gdbstub.h"
-#include "gdbstub/helpers.h"
+#include "gdbstub/registers.h"
 #include "gdbstub/commands.h"
 #include "system/tcg.h"
 #include "internals.h"
@@ -33,12 +33,16 @@ typedef struct RegisterSysregFeatureParam {
     int n;
 } RegisterSysregFeatureParam;
 
-/* Old gdb always expect FPA registers.  Newer (xml-aware) gdb only expect
-   whatever the target description contains.  Due to a historical mishap
-   the FPA registers appear in between core integer regs and the CPSR.
-   We hack round this by giving the FPA regs zero size when talking to a
-   newer gdb.  */
-
+/*
+ * Old gdb always expect FPA registers. Newer (xml-aware) gdb only
+ * expect whatever the target description contains. Due to a
+ * historical mishap the FPA registers appear in between core integer
+ * regs and the CPSR. We hack round this by giving the FPA regs zero
+ * size when talking to a newer gdb.
+ *
+ * While gdb cares about the memory endianess of the target all
+ * registers are passed in little-endian mode.
+ */
 int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -46,15 +50,17 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
 
     if (n < 16) {
         /* Core integer register.  */
-        return gdb_get_reg32(mem_buf, env->regs[n]);
+        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &env->regs[n]);
     }
     if (n == 25) {
         /* CPSR, or XPSR for M-profile */
+        uint32_t reg;
         if (arm_feature(env, ARM_FEATURE_M)) {
-            return gdb_get_reg32(mem_buf, xpsr_read(env));
+            reg = xpsr_read(env);
         } else {
-            return gdb_get_reg32(mem_buf, cpsr_read(env));
+            reg = cpsr_read(env);
         }
+        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &reg);
     }
     /* Unknown register.  */
     return 0;
@@ -115,19 +121,22 @@ static int vfp_gdb_get_reg(CPUState *cs, GByteArray *buf, int reg)
 
     /* VFP data registers are always little-endian.  */
     if (reg < nregs) {
-        return gdb_get_reg64(buf, *aa32_vfp_dreg(env, reg));
+        return gdb_get_register_value(MO_TEUQ, buf,
+                                      (uint8_t *) aa32_vfp_dreg(env, reg));
     }
     if (arm_feature(env, ARM_FEATURE_NEON)) {
         /* Aliases for Q regs.  */
         nregs += 16;
         if (reg < nregs) {
             uint64_t *q = aa32_vfp_qreg(env, reg - 32);
-            return gdb_get_reg128(buf, q[0], q[1]);
+            return gdb_get_register_value(MO_TEUO, buf, (uint8_t *) q);
         }
     }
     switch (reg - nregs) {
+        uint32_t fpcr;
     case 0:
-        return gdb_get_reg32(buf, vfp_get_fpscr(env));
+        fpcr = vfp_get_fpscr(env);
+        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpcr);
     }
     return 0;
 }
@@ -166,9 +175,11 @@ static int vfp_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
 
     switch (reg) {
     case 0:
-        return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]);
+        return gdb_get_register_value(MO_TEUL, buf,
+                                      (uint8_t *) &env->vfp.xregs[ARM_VFP_FPSID]);
     case 1:
-        return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]);
+        return gdb_get_register_value(MO_TEUL, buf,
+                                      (uint8_t *) &env->vfp.xregs[ARM_VFP_FPEXC]);
     }
     return 0;
 }
@@ -196,7 +207,8 @@ static int mve_gdb_get_reg(CPUState *cs, GByteArray *buf, int reg)
 
     switch (reg) {
     case 0:
-        return gdb_get_reg32(buf, env->v7m.vpr);
+        return gdb_get_register_value(MO_TEUL, buf,
+                                      (uint8_t *) &env->v7m.vpr);
     default:
         return 0;
     }
@@ -236,9 +248,11 @@ static int arm_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
     ri = get_arm_cp_reginfo(cpu->cp_regs, key);
     if (ri) {
         if (cpreg_field_is_64bit(ri)) {
-            return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri));
+            uint64_t cpreg = read_raw_cp_reg(env, ri);
+            return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &cpreg);
         } else {
-            return gdb_get_reg32(buf, (uint32_t)read_raw_cp_reg(env, ri));
+            uint32_t cpreg = (uint32_t) read_raw_cp_reg(env, ri);
+            return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &cpreg);
         }
     }
     return 0;
@@ -375,12 +389,12 @@ static uint32_t *m_sysreg_ptr(CPUARMState *env, MProfileSysreg reg, bool sec)
 static int m_sysreg_get(CPUARMState *env, GByteArray *buf,
                         MProfileSysreg reg, bool secure)
 {
-    uint32_t *ptr = m_sysreg_ptr(env, reg, secure);
+    uint8_t *ptr = (uint8_t *) m_sysreg_ptr(env, reg, secure);
 
     if (ptr == NULL) {
         return 0;
     }
-    return gdb_get_reg32(buf, *ptr);
+    return gdb_get_register_value(MO_TEUL, buf, ptr);
 }
 
 static int arm_gdb_get_m_systemreg(CPUState *cs, GByteArray *buf, int reg)
@@ -393,7 +407,8 @@ static int arm_gdb_get_m_systemreg(CPUState *cs, GByteArray *buf, int reg)
      * banked and non-banked bits.
      */
     if (reg == M_SYSREG_CONTROL) {
-        return gdb_get_reg32(buf, arm_v7m_mrs_control(env, env->v7m.secure));
+        uint32_t reg32 = arm_v7m_mrs_control(env, env->v7m.secure);
+        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &reg32);
     }
     return m_sysreg_get(env, buf, reg, env->v7m.secure);
 }
-- 
2.39.5

[PATCH 04/10] target/arm: convert 64 bit gdbstub to new helper

[Alex Bennée]

For some of the helpers we need a temporary variable to copy from
although we could add some helpers to return pointers into env in
those cases if we wanted to.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/gdbstub64.c | 53 ++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 1a4dbec567..793332af31 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -20,7 +20,7 @@
 #include "qemu/log.h"
 #include "cpu.h"
 #include "internals.h"
-#include "gdbstub/helpers.h"
+#include "gdbstub/registers.h"
 #include "gdbstub/commands.h"
 #include "tcg/mte_helper.h"
 #if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
@@ -35,15 +35,16 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
 
     if (n < 31) {
         /* Core integer register.  */
-        return gdb_get_reg64(mem_buf, env->xregs[n]);
+        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->xregs[n]);
     }
     switch (n) {
     case 31:
-        return gdb_get_reg64(mem_buf, env->xregs[31]);
+        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->xregs[31]);
     case 32:
-        return gdb_get_reg64(mem_buf, env->pc);
+        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->pc);
     case 33:
-        return gdb_get_reg32(mem_buf, pstate_read(env));
+        uint32_t pstate = pstate_read(env);
+        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &pstate);
     }
     /* Unknown register.  */
     return 0;
@@ -82,23 +83,27 @@ int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
+    uint32_t fpr;
 
     switch (reg) {
     case 0 ... 31:
     {
         /* 128 bit FP register - quads are in LE order */
         uint64_t *q = aa64_vfp_qreg(env, reg);
-        return gdb_get_reg128(buf, q[1], q[0]);
+        return gdb_get_register_value(MO_TEUO, buf, (uint8_t *) q);
     }
     case 32:
         /* FPSR */
-        return gdb_get_reg32(buf, vfp_get_fpsr(env));
+        fpr = vfp_get_fpsr(env);
+        break;
     case 33:
         /* FPCR */
-        return gdb_get_reg32(buf, vfp_get_fpcr(env));
+        fpr = vfp_get_fpcr(env);
+        break;
     default:
         return 0;
     }
+    return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
 }
 
 int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg)
@@ -132,30 +137,37 @@ int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
+    uint32_t fpr;
 
     switch (reg) {
     /* The first 32 registers are the zregs */
     case 0 ... 31:
     {
         int vq, len = 0;
+        ARMVectorReg *zreg = &env->vfp.zregs[reg];
+
         for (vq = 0; vq < cpu->sve_max_vq; vq++) {
-            len += gdb_get_reg128(buf,
-                                  env->vfp.zregs[reg].d[vq * 2 + 1],
-                                  env->vfp.zregs[reg].d[vq * 2]);
+            len += gdb_get_register_value(MO_TEUQ, buf,
+                                          (uint8_t *) &zreg->d[vq * 2 + 1]);
+            len += gdb_get_register_value(MO_TEUQ, buf,
+                                          (uint8_t *) &zreg->d[vq * 2]);
         }
         return len;
     }
     case 32:
-        return gdb_get_reg32(buf, vfp_get_fpsr(env));
+        fpr = vfp_get_fpsr(env);
+        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
     case 33:
-        return gdb_get_reg32(buf, vfp_get_fpcr(env));
+        fpr = vfp_get_fpcr(env);
+        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
     /* then 16 predicates and the ffr */
     case 34 ... 50:
     {
         int preg = reg - 34;
         int vq, len = 0;
         for (vq = 0; vq < cpu->sve_max_vq; vq = vq + 4) {
-            len += gdb_get_reg64(buf, env->vfp.pregs[preg].p[vq / 4]);
+            len += gdb_get_register_value(MO_TEUQ, buf,
+                                          (uint8_t *) &env->vfp.pregs[preg].p[vq / 4]);
         }
         return len;
     }
@@ -165,8 +177,8 @@ int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
          * We report in Vector Granules (VG) which is 64bit in a Z reg
          * while the ZCR works in Vector Quads (VQ) which is 128bit chunks.
          */
-        int vq = sve_vqm1_for_el(env, arm_current_el(env)) + 1;
-        return gdb_get_reg64(buf, vq * 2);
+        uint64_t vq = (sve_vqm1_for_el(env, arm_current_el(env)) + 1) * 2;
+        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &vq);
     }
     default:
         /* gdbstub asked for something out our range */
@@ -248,10 +260,11 @@ int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
             bool is_data = !(reg & 1);
             bool is_high = reg & 2;
             ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
-            ARMVAParameters param;
+            ARMVAParameters param = aa64_va_parameters(env, -is_high, mmu_idx,
+                                                       is_data, false);
+            uint64_t pauth_mask = pauth_ptr_mask(param);
 
-            param = aa64_va_parameters(env, -is_high, mmu_idx, is_data, false);
-            return gdb_get_reg64(buf, pauth_ptr_mask(param));
+            return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &pauth_mask);
         }
     default:
         return 0;
@@ -399,7 +412,7 @@ int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
 
     tcf0 = extract64(env->cp15.sctlr_el[1], 38, 2);
 
-    return gdb_get_reg64(buf, tcf0);
+    return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &tcf0);
 }
 
 int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)
-- 
2.39.5

[PATCH 05/10] target/ppc: expand comment on FP/VMX/VSX access functions

[Alex Bennée]

Mainly as an aid to myself getting confused too many bswaps deep into
the code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/ppc/cpu.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index efab54a068..1e833ade04 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2906,7 +2906,12 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx)
            (start + nregs > 32 && (rx >= start || rx < start + nregs - 32));
 }
 
-/* Accessors for FP, VMX and VSX registers */
+/*
+ * Access functions for FP, VMX and VSX registers
+ *
+ * The register is stored as a 128 bit host endian value so we need to
+ * take that into account when accessing smaller parts of it.
+ */
 #if HOST_BIG_ENDIAN
 #define VsrB(i) u8[i]
 #define VsrSB(i) s8[i]
-- 
2.39.5

[PATCH 06/10] target/ppc: make ppc_maybe_bswap_register static

[Alex Bennée]

It's not used outside of the gdbstub code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/ppc/cpu.h     | 1 -
 target/ppc/gdbstub.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 1e833ade04..950bb6e06c 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -3016,7 +3016,6 @@ static inline bool ppc_interrupts_little_endian(PowerPCCPU *cpu, bool hv)
 
 void dump_mmu(CPUPPCState *env);
 
-void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
 void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
 uint32_t ppc_get_vscr(CPUPPCState *env);
 void ppc_set_cr(CPUPPCState *env, uint64_t cr);
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 3b28d4e21c..c09e93abaf 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -81,7 +81,7 @@ static int ppc_gdb_register_len(int n)
  * TARGET_BIG_ENDIAN is always set, and we must check the current
  * mode of the chip to see if we're running in little-endian.
  */
-void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
+static void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
 {
 #ifndef CONFIG_USER_ONLY
     if (!FIELD_EX64(env->msr, MSR, LE)) {
-- 
2.39.5

[PATCH 07/10] target/ppc: convert gdbstub to new helper (!hacky)

[Alex Bennée]

By passing the explicit state of LE/BE via the memop we can avoid the
messing about we do with ppc_maybe_bswap_register() at least for
supplying register values to gdbstub.

The fact we still need the helper for setting the values probably
indicates we could do with a reverse helper, possibly to setting env
vars directly? This is complicated by aliasing though.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/ppc/gdbstub.c | 192 ++++++++++++++++++++++++-------------------
 1 file changed, 108 insertions(+), 84 deletions(-)

diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index c09e93abaf..663a97d986 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -20,7 +20,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "exec/gdbstub.h"
-#include "gdbstub/helpers.h"
+#include "gdbstub/registers.h"
 #include "internal.h"
 
 static int ppc_gdb_register_len_apple(int n)
@@ -74,12 +74,12 @@ static int ppc_gdb_register_len(int n)
 }
 
 /*
- * We need to present the registers to gdb in the "current" memory
- * ordering.  For user-only mode we get this for free;
- * TARGET_BIG_ENDIAN is set to the proper ordering for the
- * binary, and cannot be changed.  For system mode,
- * TARGET_BIG_ENDIAN is always set, and we must check the current
- * mode of the chip to see if we're running in little-endian.
+ * We need to map the target endian registers from gdb in the
+ * "current" memory ordering. For user-only mode we get this for free;
+ * TARGET_BIG_ENDIAN is set to the proper ordering for the binary, and
+ * cannot be changed. For system mode, TARGET_BIG_ENDIAN is always
+ * set, and we must check the current mode of the chip to see if we're
+ * running in little-endian.
  */
 static void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
 {
@@ -98,6 +98,38 @@ static void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len
 #endif
 }
 
+/*
+ * We need to present the registers to gdb in the "current" memory
+ * ordering. For user-only mode this is hardwired by TARGET_BIG_ENDIAN
+ * and cannot be changed. For system mode we must check the current
+ * mode of the chip to see if we're running in little-endian.
+ */
+static MemOp ppc_gdb_memop(CPUPPCState *env, int len)
+{
+#ifndef CONFIG_USER_ONLY
+    MemOp end = FIELD_EX64(env->msr, MSR, LE) ? MO_LE : MO_BE;
+#else
+    #ifdef TARGET_BIG_ENDIAN
+    MemOp end = MO_BE;
+    #else
+    MemOp end = MO_LE;
+    #endif
+#endif
+
+    return size_memop(len) | end;
+}
+
+/*
+ * Helpers copied from helpers.h just for loading target_ulong values
+ * from gdbstub's GByteArray
+ */
+
+#if TARGET_LONG_BITS == 64
+#define ldtul_p(addr) ldq_p(addr)
+#else
+#define ldtul_p(addr) ldl_p(addr)
+#endif
+
 /*
  * Old gdb always expects FP registers.  Newer (xml-aware) gdb only
  * expects whatever the target description contains.  Due to a
@@ -109,51 +141,50 @@ static void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len
 int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
 {
     CPUPPCState *env = cpu_env(cs);
-    uint8_t *mem_buf;
     int r = ppc_gdb_register_len(n);
+    MemOp mo;
 
     if (!r) {
         return r;
     }
 
+    mo = ppc_gdb_memop(env, r);
+
     if (n < 32) {
         /* gprs */
-        gdb_get_regl(buf, env->gpr[n]);
+        return gdb_get_register_value(mo, buf, (uint8_t *) &env->gpr[n]);
     } else {
         switch (n) {
         case 64:
-            gdb_get_regl(buf, env->nip);
-            break;
+            return gdb_get_register_value(mo, buf, (uint8_t *) &env->nip);
         case 65:
-            gdb_get_regl(buf, env->msr);
-            break;
+            return gdb_get_register_value(mo, buf, (uint8_t *) &env->msr);
         case 66:
             {
                 uint32_t cr = ppc_get_cr(env);
-                gdb_get_reg32(buf, cr);
-                break;
+                return gdb_get_register_value(ppc_gdb_memop(env, 4), buf, (uint8_t *) &cr);
             }
         case 67:
-            gdb_get_regl(buf, env->lr);
+            return gdb_get_register_value(mo, buf, (uint8_t *) &env->lr);
             break;
         case 68:
-            gdb_get_regl(buf, env->ctr);
+            return gdb_get_register_value(mo, buf, (uint8_t *) &env->ctr);
             break;
         case 69:
-            gdb_get_reg32(buf, cpu_read_xer(env));
-            break;
+            uint32_t val = cpu_read_xer(env);
+            return gdb_get_register_value(ppc_gdb_memop(env, 4), buf, (uint8_t *) &val);
         }
     }
-    mem_buf = buf->data + buf->len - r;
-    ppc_maybe_bswap_register(env, mem_buf, r);
-    return r;
+
+    return 0;
 }
 
 int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
 {
     CPUPPCState *env = cpu_env(cs);
-    uint8_t *mem_buf;
     int r = ppc_gdb_register_len_apple(n);
+    MemOp mo = ppc_gdb_memop(env, r);
+    int actual = 0;
 
     if (!r) {
         return r;
@@ -161,44 +192,48 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
 
     if (n < 32) {
         /* gprs */
-        gdb_get_reg64(buf, env->gpr[n]);
+        actual = gdb_get_register_value(mo, buf, (uint8_t *) &env->gpr[n]);
     } else if (n < 64) {
         /* fprs */
-        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
+        actual = gdb_get_register_value(mo, buf, (uint8_t *) cpu_fpr_ptr(env, n - 32));
     } else if (n < 96) {
-        /* Altivec */
-        gdb_get_reg64(buf, n - 64);
-        gdb_get_reg64(buf, 0);
+        /* Altivec - where are they? ppc_vsr_t vsr[64]? */
+        uint64_t empty = 0;
+        actual = gdb_get_register_value(mo, buf, (uint8_t *) &empty);
+        actual = gdb_get_register_value(mo, buf, (uint8_t *) &empty);
     } else {
         switch (n) {
         case 64 + 32:
-            gdb_get_reg64(buf, env->nip);
+            actual = gdb_get_register_value(mo, buf, (uint8_t *) &env->nip);
             break;
         case 65 + 32:
-            gdb_get_reg64(buf, env->msr);
+            actual = gdb_get_register_value(mo, buf, (uint8_t *) &env->msr);
             break;
         case 66 + 32:
-            {
-                uint32_t cr = ppc_get_cr(env);
-                gdb_get_reg32(buf, cr);
-                break;
-            }
+        {
+            uint32_t cr = ppc_get_cr(env);
+            actual = gdb_get_register_value(mo, buf, (uint8_t *) &cr);
+            break;
+        }
         case 67 + 32:
-            gdb_get_reg64(buf, env->lr);
+            actual = gdb_get_register_value(mo, buf, (uint8_t *) &env->lr);
             break;
         case 68 + 32:
-            gdb_get_reg64(buf, env->ctr);
+            actual = gdb_get_register_value(mo, buf, (uint8_t *) &env->ctr);
             break;
         case 69 + 32:
-            gdb_get_reg32(buf, cpu_read_xer(env));
+        {
+            uint32_t xer = cpu_read_xer(env);
+            actual = gdb_get_register_value(mo, buf, (uint8_t *) &xer);
             break;
+        }
         case 70 + 32:
-            gdb_get_reg64(buf, env->fpscr);
+            actual = gdb_get_register_value(mo, buf, (uint8_t *) &env->fpscr);
             break;
         }
     }
-    mem_buf = buf->data + buf->len - r;
-    ppc_maybe_bswap_register(env, mem_buf, r);
+
+    g_assert(r == actual);
     return r;
 }
 
@@ -210,6 +245,9 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     if (!r) {
         return r;
     }
+
+    g_assert(r == n);
+    
     ppc_maybe_bswap_register(env, mem_buf, r);
     if (n < 32) {
         /* gprs */
@@ -367,18 +405,16 @@ static int gdb_get_spr_reg(CPUState *cs, GByteArray *buf, int n)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
+    MemOp mo = ppc_gdb_memop(env, TARGET_LONG_SIZE);
+    target_ulong val;
     int reg;
-    int len;
 
     reg = gdb_find_spr_idx(env, n);
     if (reg < 0) {
         return 0;
     }
 
-    len = TARGET_LONG_SIZE;
-
     /* Handle those SPRs that are not part of the env->spr[] array */
-    target_ulong val;
     switch (reg) {
 #if defined(TARGET_PPC64)
     case SPR_CFAR:
@@ -400,10 +436,7 @@ static int gdb_get_spr_reg(CPUState *cs, GByteArray *buf, int n)
     default:
         val = env->spr[reg];
     }
-    gdb_get_regl(buf, val);
-
-    ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, len), len);
-    return len;
+    return gdb_get_register_value(mo, buf, (uint8_t *) &val);
 }
 
 static int gdb_set_spr_reg(CPUState *cs, uint8_t *mem_buf, int n)
@@ -441,18 +474,14 @@ static int gdb_get_float_reg(CPUState *cs, GByteArray *buf, int n)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
-    uint8_t *mem_buf;
+    MemOp mo;
     if (n < 32) {
-        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n));
-        mem_buf = gdb_get_reg_ptr(buf, 8);
-        ppc_maybe_bswap_register(env, mem_buf, 8);
-        return 8;
+        mo = ppc_gdb_memop(env, 8);
+        return gdb_get_register_value(mo, buf, (uint8_t *)cpu_fpr_ptr(env, n));
     }
     if (n == 32) {
-        gdb_get_reg32(buf, env->fpscr);
-        mem_buf = gdb_get_reg_ptr(buf, 4);
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        return 4;
+        mo = ppc_gdb_memop(env, 4);
+        return gdb_get_register_value(mo, buf, (uint8_t *) &env->fpscr);
     }
     return 0;
 }
@@ -479,26 +508,21 @@ static int gdb_get_avr_reg(CPUState *cs, GByteArray *buf, int n)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
-    uint8_t *mem_buf;
+    MemOp mo;
 
     if (n < 32) {
         ppc_avr_t *avr = cpu_avr_ptr(env, n);
-        gdb_get_reg128(buf, avr->VsrD(0), avr->VsrD(1));
-        mem_buf = gdb_get_reg_ptr(buf, 16);
-        ppc_maybe_bswap_register(env, mem_buf, 16);
-        return 16;
+        mo = ppc_gdb_memop(env, 16);
+        return gdb_get_register_value(mo, buf, (uint8_t *) avr);
     }
     if (n == 32) {
-        gdb_get_reg32(buf, ppc_get_vscr(env));
-        mem_buf = gdb_get_reg_ptr(buf, 4);
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        return 4;
+        uint32_t vscr = ppc_get_vscr(env);
+        mo = ppc_gdb_memop(env, 4);
+        return gdb_get_register_value(mo, buf, (uint8_t *) &vscr);
     }
     if (n == 33) {
-        gdb_get_reg32(buf, (uint32_t)env->spr[SPR_VRSAVE]);
-        mem_buf = gdb_get_reg_ptr(buf, 4);
-        ppc_maybe_bswap_register(env, mem_buf, 4);
-        return 4;
+        mo = ppc_gdb_memop(env, 4);
+        return gdb_get_register_value(mo, buf, (uint8_t *) &env->spr[SPR_VRSAVE]);
     }
     return 0;
 }
@@ -532,25 +556,25 @@ static int gdb_get_spe_reg(CPUState *cs, GByteArray *buf, int n)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
+    MemOp mo;
 
     if (n < 32) {
 #if defined(TARGET_PPC64)
-        gdb_get_reg32(buf, env->gpr[n] >> 32);
-        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 4), 4);
+        uint32_t low = env->gpr[n] >> 32;
+        mo = ppc_gdb_memop(env, 4);
+        return gdb_get_register_value(mo, buf, (uint8_t *) &low);
 #else
-        gdb_get_reg32(buf, env->gprh[n]);
+        mo = ppc_gdb_memop(env, 4);
+        return gdb_get_register_value(mo, buf, (uint8_t *) &env->gprh[n]);
 #endif
-        return 4;
     }
     if (n == 32) {
-        gdb_get_reg64(buf, env->spe_acc);
-        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 8), 8);
-        return 8;
+        mo = ppc_gdb_memop(env, 8);
+        return gdb_get_register_value(mo, buf, (uint8_t *) &env->spe_acc);
     }
     if (n == 33) {
-        gdb_get_reg32(buf, env->spe_fscr);
-        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 4), 4);
-        return 4;
+        mo = ppc_gdb_memop(env, 4);
+        return gdb_get_register_value(mo, buf, (uint8_t *) &env->spe_fscr);
     }
     return 0;
 }
@@ -593,9 +617,9 @@ static int gdb_get_vsx_reg(CPUState *cs, GByteArray *buf, int n)
     CPUPPCState *env = &cpu->env;
 
     if (n < 32) {
-        gdb_get_reg64(buf, *cpu_vsrl_ptr(env, n));
-        ppc_maybe_bswap_register(env, gdb_get_reg_ptr(buf, 8), 8);
-        return 8;
+        return gdb_get_register_value(ppc_gdb_memop(env, 8),
+                                      buf,
+                                      (uint8_t *)cpu_vsrl_ptr(env, n));
     }
     return 0;
 }
-- 
2.39.5

[PATCH 08/10] gdbstub: assert earlier in handle_read_all_regs

[Alex Bennée]

When things go wrong we want to assert on the register that failed to
be able to figure out what went wrong.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub/gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 3d7b1028e4..8561d9d527 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1350,8 +1350,8 @@ static void handle_read_all_regs(GArray *params, void *user_ctx)
         len += gdb_read_register(gdbserver_state.g_cpu,
                                  gdbserver_state.mem_buf,
                                  reg_id);
+        g_assert(len == gdbserver_state.mem_buf->len);
     }
-    g_assert(len == gdbserver_state.mem_buf->len);
 
     gdb_memtohex(gdbserver_state.str_buf, gdbserver_state.mem_buf->data, len);
     gdb_put_strbuf();
-- 
2.39.5

[PATCH 09/10] include/exec: fix assert in size_memop

[Alex Bennée]

We can handle larger sized memops now, expand the range of the assert.

Fixes: 4b473e0c60 (tcg: Expand MO_SIZE to 3 bits)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/memop.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/exec/memop.h b/include/exec/memop.h
index 407a47d82c..faed3ff902 100644
--- a/include/exec/memop.h
+++ b/include/exec/memop.h
@@ -162,8 +162,8 @@ static inline unsigned memop_size(MemOp op)
 static inline MemOp size_memop(unsigned size)
 {
 #ifdef CONFIG_DEBUG_TCG
-    /* Power of 2 up to 8.  */
-    assert((size & (size - 1)) == 0 && size >= 1 && size <= 8);
+    /* Power of 2 up to 128.  */
+    assert((size & (size - 1)) == 0 && size >= 1 && size <= 128);
 #endif
     return (MemOp)ctz32(size);
 }
-- 
2.39.5

[PATCH 10/10] target/microblaze: convert gdbstub to new helper

[Alex Bennée]

This is a pretty simple conversion which will be trivial to update
once TARGET_BIGENDIAN is replaced with a helper function.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/microblaze/gdbstub.c | 44 ++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
index d493681d38..106d6efbab 100644
--- a/target/microblaze/gdbstub.c
+++ b/target/microblaze/gdbstub.c
@@ -19,7 +19,7 @@
  */
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "gdbstub/helpers.h"
+#include "gdbstub/registers.h"
 
 /*
  * GDB expects SREGs in the following order:
@@ -50,62 +50,52 @@ int mb_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
 {
     MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
     CPUMBState *env = &cpu->env;
-    uint32_t val;
+    MemOp mo = TARGET_BIG_ENDIAN ? MO_BEUL : MO_LEUL;
 
     switch (n) {
     case 1 ... 31:
-        val = env->regs[n];
-        break;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->regs[n]);
     case GDB_PC:
-        val = env->pc;
-        break;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->pc);
     case GDB_MSR:
-        val = mb_cpu_read_msr(env);
-        break;
+        uint32_t msr = mb_cpu_read_msr(env);
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &msr);
     case GDB_EAR:
-        val = env->ear;
-        break;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->ear);
     case GDB_ESR:
-        val = env->esr;
-        break;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->esr);
     case GDB_FSR:
-        val = env->fsr;
-        break;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->fsr);
     case GDB_BTR:
-        val = env->btr;
-        break;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->btr);
     case GDB_PVR0 ... GDB_PVR11:
         /* PVR12 is intentionally skipped */
-        val = cpu->cfg.pvr_regs[n - GDB_PVR0];
-        break;
+        return gdb_get_register_value(mo, mem_buf,
+                                      (uint8_t *) &cpu->cfg.pvr_regs[n - GDB_PVR0]);
     case GDB_EDR:
-        val = env->edr;
-        break;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->edr);
     default:
         /* Other SRegs aren't modeled, so report a value of 0 */
-        val = 0;
-        break;
+        return 0;
     }
-    return gdb_get_reg32(mem_buf, val);
 }
 
 int mb_cpu_gdb_read_stack_protect(CPUState *cs, GByteArray *mem_buf, int n)
 {
     MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
     CPUMBState *env = &cpu->env;
-    uint32_t val;
+    MemOp mo = TARGET_BIG_ENDIAN ? MO_BEUL : MO_LEUL;
 
     switch (n) {
     case GDB_SP_SHL:
-        val = env->slr;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->slr);
         break;
     case GDB_SP_SHR:
-        val = env->shr;
+        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->shr);
         break;
     default:
         return 0;
     }
-    return gdb_get_reg32(mem_buf, val);
 }
 
 int mb_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
-- 
2.39.5

Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Akihiko Odaki]

On 2025/03/20 3:22, Alex Bennée wrote:
> The current helper.h functions rely on hard coded assumptions about
> target endianess to use the tswap macros. We also end up double
> swapping a bunch of values if the target can run in multiple endianess
> modes. Avoid this by getting the target to pass the endianess and size
> via a MemOp and fixing up appropriately.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

The overall idea looks good to me. I have a few nitpicks:

> ---
>   include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>   2 files changed, 52 insertions(+)
>   create mode 100644 include/gdbstub/registers.h
> 
> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
> new file mode 100644
> index 0000000000..4abc7a6ae7
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,30 @@
> +/*
> + * GDB Common Register Helpers
> + *
> + * Copyright (c) 2025 Linaro Ltd
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef GDB_REGISTERS_H
> +#define GDB_REGISTERS_H
> +
> +#include "exec/memop.h"
> +
> +/**
> + * gdb_get_register_value() - get register value for gdb
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to value in host order
> + *
> + * This replaces the previous legacy read functions with a single
> + * function to handle all sizes. Passing @mo allows the target mode to
> + * be taken into account and avoids using hard coded tswap() macros.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 282e13e163..3d7b1028e4 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -32,6 +32,7 @@
>   #include "exec/gdbstub.h"
>   #include "gdbstub/commands.h"
>   #include "gdbstub/syscalls.h"
> +#include "gdbstub/registers.h"
>   #ifdef CONFIG_USER_ONLY
>   #include "accel/tcg/vcpu-state.h"
>   #include "gdbstub/user.h"
> @@ -45,6 +46,7 @@
>   #include "system/runstate.h"
>   #include "exec/replay-core.h"
>   #include "exec/hwaddr.h"
> +#include "exec/memop.h"
>   
>   #include "internals.h"
>   
> @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>       return 0;
>   }
>   
> +/*
> + * Target helper function to read value into GByteArray, target
> + * supplies the size and target endianess via the MemOp.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
> +{
> +    size_t bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        for ( int i = bytes ; i > 0; i--) {

memop_size() returns unsigned, but bytes is size_t and i is int, and 
this function returns int. Let's keep them consistent.

There is an extra whitespace between "(" and "int".

Regards,
Akihiko Odaki

> +            g_byte_array_append(buf, &val[i - 1], 1);
> +        };
> +    } else {
> +        g_byte_array_append(buf, val, bytes);
> +    }
> +
> +    return bytes;
> +}
> +
> +
>   static void gdb_register_feature(CPUState *cpu, int base_reg,
>                                    gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>                                    const GDBFeature *feature)

Re: [PATCH 03/10] target/arm: convert 32 bit gdbstub to new helper

[Akihiko Odaki]

On 2025/03/20 3:22, Alex Bennée wrote:
> For some of the helpers we need a temporary variable to copy from
> although we could add some helpers to return pointers into env in
> those cases if we wanted to.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/arm/gdbstub.c | 57 ++++++++++++++++++++++++++++----------------
>   1 file changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 30068c2262..14d931b0bf 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -20,7 +20,7 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "exec/gdbstub.h"
> -#include "gdbstub/helpers.h"
> +#include "gdbstub/registers.h"
>   #include "gdbstub/commands.h"
>   #include "system/tcg.h"
>   #include "internals.h"
> @@ -33,12 +33,16 @@ typedef struct RegisterSysregFeatureParam {
>       int n;
>   } RegisterSysregFeatureParam;
>   
> -/* Old gdb always expect FPA registers.  Newer (xml-aware) gdb only expect
> -   whatever the target description contains.  Due to a historical mishap
> -   the FPA registers appear in between core integer regs and the CPSR.
> -   We hack round this by giving the FPA regs zero size when talking to a
> -   newer gdb.  */
> -
> +/*
> + * Old gdb always expect FPA registers. Newer (xml-aware) gdb only
> + * expect whatever the target description contains. Due to a
> + * historical mishap the FPA registers appear in between core integer
> + * regs and the CPSR. We hack round this by giving the FPA regs zero
> + * size when talking to a newer gdb.
> + *
> + * While gdb cares about the memory endianess of the target all
> + * registers are passed in little-endian mode.
> + */
>   int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
> @@ -46,15 +50,17 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   
>       if (n < 16) {
>           /* Core integer register.  */
> -        return gdb_get_reg32(mem_buf, env->regs[n]);
> +        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &env->regs[n]);

Having a whitespace between the cast and the later expression is unusual 
though checkpatch.pl doesn't complain.

>       }
>       if (n == 25) {
>           /* CPSR, or XPSR for M-profile */
> +        uint32_t reg;
>           if (arm_feature(env, ARM_FEATURE_M)) {
> -            return gdb_get_reg32(mem_buf, xpsr_read(env));
> +            reg = xpsr_read(env);
>           } else {
> -            return gdb_get_reg32(mem_buf, cpsr_read(env));
> +            reg = cpsr_read(env);
>           }
> +        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &reg);
>       }
>       /* Unknown register.  */
>       return 0;
> @@ -115,19 +121,22 @@ static int vfp_gdb_get_reg(CPUState *cs, GByteArray *buf, int reg)
>   
>       /* VFP data registers are always little-endian.  */
>       if (reg < nregs) {
> -        return gdb_get_reg64(buf, *aa32_vfp_dreg(env, reg));
> +        return gdb_get_register_value(MO_TEUQ, buf,
> +                                      (uint8_t *) aa32_vfp_dreg(env, reg));
>       }
>       if (arm_feature(env, ARM_FEATURE_NEON)) {
>           /* Aliases for Q regs.  */
>           nregs += 16;
>           if (reg < nregs) {
>               uint64_t *q = aa32_vfp_qreg(env, reg - 32);
> -            return gdb_get_reg128(buf, q[0], q[1]);
> +            return gdb_get_register_value(MO_TEUO, buf, (uint8_t *) q);
>           }
>       }
>       switch (reg - nregs) {
> +        uint32_t fpcr;
>       case 0:
> -        return gdb_get_reg32(buf, vfp_get_fpscr(env));
> +        fpcr = vfp_get_fpscr(env);
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpcr);
>       }
>       return 0;
>   }
> @@ -166,9 +175,11 @@ static int vfp_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
>   
>       switch (reg) {
>       case 0:
> -        return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]);
> +        return gdb_get_register_value(MO_TEUL, buf,
> +                                      (uint8_t *) &env->vfp.xregs[ARM_VFP_FPSID]);
>       case 1:
> -        return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]);
> +        return gdb_get_register_value(MO_TEUL, buf,
> +                                      (uint8_t *) &env->vfp.xregs[ARM_VFP_FPEXC]);
>       }
>       return 0;
>   }
> @@ -196,7 +207,8 @@ static int mve_gdb_get_reg(CPUState *cs, GByteArray *buf, int reg)
>   
>       switch (reg) {
>       case 0:
> -        return gdb_get_reg32(buf, env->v7m.vpr);
> +        return gdb_get_register_value(MO_TEUL, buf,
> +                                      (uint8_t *) &env->v7m.vpr);
>       default:
>           return 0;
>       }
> @@ -236,9 +248,11 @@ static int arm_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
>       ri = get_arm_cp_reginfo(cpu->cp_regs, key);
>       if (ri) {
>           if (cpreg_field_is_64bit(ri)) {
> -            return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri));
> +            uint64_t cpreg = read_raw_cp_reg(env, ri);
> +            return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &cpreg);
>           } else {
> -            return gdb_get_reg32(buf, (uint32_t)read_raw_cp_reg(env, ri));
> +            uint32_t cpreg = (uint32_t) read_raw_cp_reg(env, ri);
> +            return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &cpreg);
>           }
>       }
>       return 0;
> @@ -375,12 +389,12 @@ static uint32_t *m_sysreg_ptr(CPUARMState *env, MProfileSysreg reg, bool sec)
>   static int m_sysreg_get(CPUARMState *env, GByteArray *buf,
>                           MProfileSysreg reg, bool secure)
>   {
> -    uint32_t *ptr = m_sysreg_ptr(env, reg, secure);
> +    uint8_t *ptr = (uint8_t *) m_sysreg_ptr(env, reg, secure);
>   
>       if (ptr == NULL) {
>           return 0;
>       }
> -    return gdb_get_reg32(buf, *ptr);
> +    return gdb_get_register_value(MO_TEUL, buf, ptr);
>   }
>   
>   static int arm_gdb_get_m_systemreg(CPUState *cs, GByteArray *buf, int reg)
> @@ -393,7 +407,8 @@ static int arm_gdb_get_m_systemreg(CPUState *cs, GByteArray *buf, int reg)
>        * banked and non-banked bits.
>        */
>       if (reg == M_SYSREG_CONTROL) {
> -        return gdb_get_reg32(buf, arm_v7m_mrs_control(env, env->v7m.secure));
> +        uint32_t reg32 = arm_v7m_mrs_control(env, env->v7m.secure);
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &reg32);
>       }
>       return m_sysreg_get(env, buf, reg, env->v7m.secure);
>   }

Re: [PATCH 09/10] include/exec: fix assert in size_memop

[Akihiko Odaki]

Updating the email address of Juan Quintela according to .mailmap.

On 2025/03/20 3:22, Alex Bennée wrote:
> We can handle larger sized memops now, expand the range of the assert.
> 
> Fixes: 4b473e0c60 (tcg: Expand MO_SIZE to 3 bits)
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/exec/memop.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/exec/memop.h b/include/exec/memop.h
> index 407a47d82c..faed3ff902 100644
> --- a/include/exec/memop.h
> +++ b/include/exec/memop.h
> @@ -162,8 +162,8 @@ static inline unsigned memop_size(MemOp op)
>   static inline MemOp size_memop(unsigned size)
>   {
>   #ifdef CONFIG_DEBUG_TCG
> -    /* Power of 2 up to 8.  */
> -    assert((size & (size - 1)) == 0 && size >= 1 && size <= 8);
> +    /* Power of 2 up to 128.  */
> +    assert((size & (size - 1)) == 0 && size >= 1 && size <= 128);

This is the minimal change to fix, but perhaps it may be also nice to 
change it to use MO_SIZE.

Regards,
Akihiko Odaki

>   #endif
>       return (MemOp)ctz32(size);
>   }

Re: [PATCH 06/10] target/ppc: make ppc_maybe_bswap_register static

[Philippe Mathieu-Daudé]

On 19/3/25 19:22, Alex Bennée wrote:
> It's not used outside of the gdbstub code.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/ppc/cpu.h     | 1 -
>   target/ppc/gdbstub.c | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 08/10] gdbstub: assert earlier in handle_read_all_regs

[Philippe Mathieu-Daudé]

On 19/3/25 19:22, Alex Bennée wrote:
> When things go wrong we want to assert on the register that failed to
> be able to figure out what went wrong.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   gdbstub/gdbstub.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 10/10] target/microblaze: convert gdbstub to new helper

[Philippe Mathieu-Daudé]

On 19/3/25 19:22, Alex Bennée wrote:
> This is a pretty simple conversion which will be trivial to update
> once TARGET_BIGENDIAN is replaced with a helper function.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/microblaze/gdbstub.c | 44 ++++++++++++++-----------------------
>   1 file changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
> index d493681d38..106d6efbab 100644
> --- a/target/microblaze/gdbstub.c
> +++ b/target/microblaze/gdbstub.c
> @@ -19,7 +19,7 @@
>    */
>   #include "qemu/osdep.h"
>   #include "cpu.h"
> -#include "gdbstub/helpers.h"
> +#include "gdbstub/registers.h"
>   
>   /*
>    * GDB expects SREGs in the following order:
> @@ -50,62 +50,52 @@ int mb_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   {
>       MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
>       CPUMBState *env = &cpu->env;
> -    uint32_t val;
> +    MemOp mo = TARGET_BIG_ENDIAN ? MO_BEUL : MO_LEUL;

Since commit 415aae543ed TARGET_BIG_ENDIAN isn't doing what you'd
expect on microblaze, I think you want:

        MemOp mo = mb_cpu_is_big_endian(cs) ? MO_BEUL : MO_LEUL;

>   
>       switch (n) {
>       case 1 ... 31:
> -        val = env->regs[n];
> -        break;
> +        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->regs[n]);
>       case GDB_PC:
> -        val = env->pc;
> -        break;
> +        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->pc);
>       case GDB_MSR:
> -        val = mb_cpu_read_msr(env);
> -        break;
> +        uint32_t msr = mb_cpu_read_msr(env);
> +        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &msr);
>       case GDB_EAR:
> -        val = env->ear;
> -        break;
> +        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->ear);
>       case GDB_ESR:
> -        val = env->esr;
> -        break;
> +        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->esr);
>       case GDB_FSR:
> -        val = env->fsr;
> -        break;
> +        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->fsr);
>       case GDB_BTR:
> -        val = env->btr;
> -        break;
> +        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->btr);
>       case GDB_PVR0 ... GDB_PVR11:
>           /* PVR12 is intentionally skipped */
> -        val = cpu->cfg.pvr_regs[n - GDB_PVR0];
> -        break;
> +        return gdb_get_register_value(mo, mem_buf,
> +                                      (uint8_t *) &cpu->cfg.pvr_regs[n - GDB_PVR0]);
>       case GDB_EDR:
> -        val = env->edr;
> -        break;
> +        return gdb_get_register_value(mo, mem_buf, (uint8_t *) &env->edr);
>       default:
>           /* Other SRegs aren't modeled, so report a value of 0 */
> -        val = 0;
> -        break;
> +        return 0;
>       }
> -    return gdb_get_reg32(mem_buf, val);
>   }

Re: [PATCH 01/10] include/gdbstub: fix include guard in commands.h

[Philippe Mathieu-Daudé]

On 19/3/25 19:22, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/gdbstub/commands.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Philippe Mathieu-Daudé]

Hi Alex,

On 19/3/25 19:22, Alex Bennée wrote:
> The current helper.h functions rely on hard coded assumptions about
> target endianess to use the tswap macros. We also end up double
> swapping a bunch of values if the target can run in multiple endianess
> modes. Avoid this by getting the target to pass the endianess and size
> via a MemOp and fixing up appropriately.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>   2 files changed, 52 insertions(+)
>   create mode 100644 include/gdbstub/registers.h


> +/**
> + * gdb_get_register_value() - get register value for gdb
> + * mo: size and endian MemOp

Since we pass the size, ...

> + * buf: GByteArray to store in target order
> + * val: pointer to value in host order

we don't know the type of the value, ...

> + *
> + * This replaces the previous legacy read functions with a single
> + * function to handle all sizes. Passing @mo allows the target mode to
> + * be taken into account and avoids using hard coded tswap() macros.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);

so I'd rather use a 'void *' type here (also const),

int gdb_get_register_value(MemOp op, GByteArray *gbuf, const void *ptr);

...

> +/*
> + * Target helper function to read value into GByteArray, target
> + * supplies the size and target endianess via the MemOp.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
> +{

using uint8_t type being one implementation choice for this method:

int gdb_get_register_value(MemOp op, GByteArray *gbuf, const void *ptr)
{
        const uint8_t *buf = ptr;

> +    size_t bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        for ( int i = bytes ; i > 0; i--) {
> +            g_byte_array_append(buf, &val[i - 1], 1);
> +        };
> +    } else {
> +        g_byte_array_append(buf, val, bytes);
> +    }
> +
> +    return bytes;
> +}

Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Philippe Mathieu-Daudé]

On 20/3/25 07:19, Akihiko Odaki wrote:
> On 2025/03/20 3:22, Alex Bennée wrote:
>> The current helper.h functions rely on hard coded assumptions about
>> target endianess to use the tswap macros. We also end up double
>> swapping a bunch of values if the target can run in multiple endianess
>> modes. Avoid this by getting the target to pass the endianess and size
>> via a MemOp and fixing up appropriately.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> The overall idea looks good to me. I have a few nitpicks:
> 
>> ---
>>   include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>>   gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>>   2 files changed, 52 insertions(+)
>>   create mode 100644 include/gdbstub/registers.h
>>
>> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
>> new file mode 100644
>> index 0000000000..4abc7a6ae7
>> --- /dev/null
>> +++ b/include/gdbstub/registers.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * GDB Common Register Helpers
>> + *
>> + * Copyright (c) 2025 Linaro Ltd
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef GDB_REGISTERS_H
>> +#define GDB_REGISTERS_H
>> +
>> +#include "exec/memop.h"
>> +
>> +/**
>> + * gdb_get_register_value() - get register value for gdb
>> + * mo: size and endian MemOp
>> + * buf: GByteArray to store in target order
>> + * val: pointer to value in host order
>> + *
>> + * This replaces the previous legacy read functions with a single
>> + * function to handle all sizes. Passing @mo allows the target mode to
>> + * be taken into account and avoids using hard coded tswap() macros.
>> + *
>> + * Returns the number of bytes written to the array.
>> + */
>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
>> +
>> +#endif /* GDB_REGISTERS_H */
>> +
>> +
>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> index 282e13e163..3d7b1028e4 100644
>> --- a/gdbstub/gdbstub.c
>> +++ b/gdbstub/gdbstub.c
>> @@ -32,6 +32,7 @@
>>   #include "exec/gdbstub.h"
>>   #include "gdbstub/commands.h"
>>   #include "gdbstub/syscalls.h"
>> +#include "gdbstub/registers.h"
>>   #ifdef CONFIG_USER_ONLY
>>   #include "accel/tcg/vcpu-state.h"
>>   #include "gdbstub/user.h"
>> @@ -45,6 +46,7 @@
>>   #include "system/runstate.h"
>>   #include "exec/replay-core.h"
>>   #include "exec/hwaddr.h"
>> +#include "exec/memop.h"
>>   #include "internals.h"
>> @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState *cpu, 
>> uint8_t *mem_buf, int reg)
>>       return 0;
>>   }
>> +/*
>> + * Target helper function to read value into GByteArray, target
>> + * supplies the size and target endianess via the MemOp.
>> + */
>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
>> +{
>> +    size_t bytes = memop_size(op);
>> +
>> +    if (op & MO_BSWAP) {
>> +        for ( int i = bytes ; i > 0; i--) {
> 
> memop_size() returns unsigned, but bytes is size_t and i is int, and 
> this function returns int. Let's keep them consistent.

I wasn't sure why this method returns any information at all, but
apparently the next patch shows some uses. Indeed as Akihiko pointed,
if we return something, let it be unsigned (possibly size_t).

Re: [PATCH 09/10] include/exec: fix assert in size_memop

[Philippe Mathieu-Daudé]

On 20/3/25 07:29, Akihiko Odaki wrote:
> Updating the email address of Juan Quintela according to .mailmap.
> 
> On 2025/03/20 3:22, Alex Bennée wrote:
>> We can handle larger sized memops now, expand the range of the assert.
>>
>> Fixes: 4b473e0c60 (tcg: Expand MO_SIZE to 3 bits)
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   include/exec/memop.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/exec/memop.h b/include/exec/memop.h
>> index 407a47d82c..faed3ff902 100644
>> --- a/include/exec/memop.h
>> +++ b/include/exec/memop.h
>> @@ -162,8 +162,8 @@ static inline unsigned memop_size(MemOp op)
>>   static inline MemOp size_memop(unsigned size)
>>   {
>>   #ifdef CONFIG_DEBUG_TCG
>> -    /* Power of 2 up to 8.  */
>> -    assert((size & (size - 1)) == 0 && size >= 1 && size <= 8);
>> +    /* Power of 2 up to 128.  */
>> +    assert((size & (size - 1)) == 0 && size >= 1 && size <= 128);
> 
> This is the minimal change to fix, but perhaps it may be also nice to 
> change it to use MO_SIZE.

I concur.

       ... && size >= 1 && size <= (1 << MO_SIZE));

> Regards,
> Akihiko Odaki
> 
>>   #endif
>>       return (MemOp)ctz32(size);
>>   }
> 

Re: [PATCH 04/10] target/arm: convert 64 bit gdbstub to new helper

[Philippe Mathieu-Daudé]

On 19/3/25 19:22, Alex Bennée wrote:
> For some of the helpers we need a temporary variable to copy from
> although we could add some helpers to return pointers into env in
> those cases if we wanted to.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/arm/gdbstub64.c | 53 ++++++++++++++++++++++++++----------------
>   1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index 1a4dbec567..793332af31 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -20,7 +20,7 @@
>   #include "qemu/log.h"
>   #include "cpu.h"
>   #include "internals.h"
> -#include "gdbstub/helpers.h"
> +#include "gdbstub/registers.h"
>   #include "gdbstub/commands.h"
>   #include "tcg/mte_helper.h"
>   #if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
> @@ -35,15 +35,16 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   
>       if (n < 31) {
>           /* Core integer register.  */
> -        return gdb_get_reg64(mem_buf, env->xregs[n]);
> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->xregs[n]);

s/MO_TEUO/MO_TEUQ/

But I think we should get MO_TE from CPUState in prologue.

   MemOp mop = MO_TE; /* FIXME get from CPUState */

   ...
   return gdb_get_register_value(mop | MO_64, mem_buf, &env->xregs[n]);

>       }
>       switch (n) {
>       case 31:
> -        return gdb_get_reg64(mem_buf, env->xregs[31]);
> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->xregs[31]);

s/MO_TEUO/MO_TEUQ/

But harder to get it wrong using the MO_$bit equivalent:

   return gdb_get_register_value(mop | MO_64, mem_buf, &env->xregs[31]);

>       case 32:
> -        return gdb_get_reg64(mem_buf, env->pc);
> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->pc);

s/MO_TEUO/MO_TEUQ/

>       case 33:
> -        return gdb_get_reg32(mem_buf, pstate_read(env));
> +        uint32_t pstate = pstate_read(env);
> +        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &pstate);
>       }
>       /* Unknown register.  */
>       return 0;
> @@ -82,23 +83,27 @@ int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> +    uint32_t fpr;
>   
>       switch (reg) {
>       case 0 ... 31:
>       {
>           /* 128 bit FP register - quads are in LE order */
>           uint64_t *q = aa64_vfp_qreg(env, reg);
> -        return gdb_get_reg128(buf, q[1], q[0]);
> +        return gdb_get_register_value(MO_TEUO, buf, (uint8_t *) q);
>       }
>       case 32:
>           /* FPSR */
> -        return gdb_get_reg32(buf, vfp_get_fpsr(env));
> +        fpr = vfp_get_fpsr(env);
> +        break;
>       case 33:
>           /* FPCR */
> -        return gdb_get_reg32(buf, vfp_get_fpcr(env));
> +        fpr = vfp_get_fpcr(env);
> +        break;
>       default:
>           return 0;
>       }
> +    return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
>   }
>   
>   int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg)
> @@ -132,30 +137,37 @@ int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> +    uint32_t fpr;
>   
>       switch (reg) {
>       /* The first 32 registers are the zregs */
>       case 0 ... 31:
>       {
>           int vq, len = 0;
> +        ARMVectorReg *zreg = &env->vfp.zregs[reg];
> +
>           for (vq = 0; vq < cpu->sve_max_vq; vq++) {
> -            len += gdb_get_reg128(buf,
> -                                  env->vfp.zregs[reg].d[vq * 2 + 1],
> -                                  env->vfp.zregs[reg].d[vq * 2]);
> +            len += gdb_get_register_value(MO_TEUQ, buf,
> +                                          (uint8_t *) &zreg->d[vq * 2 + 1]);

s/MO_TEUQ/MO_TEUO/ ?

> +            len += gdb_get_register_value(MO_TEUQ, buf,
> +                                          (uint8_t *) &zreg->d[vq * 2]);

s/MO_TEUQ/MO_TEUO/ ?

>           }
>           return len;
>       }
>       case 32:
> -        return gdb_get_reg32(buf, vfp_get_fpsr(env));
> +        fpr = vfp_get_fpsr(env);
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
>       case 33:
> -        return gdb_get_reg32(buf, vfp_get_fpcr(env));
> +        fpr = vfp_get_fpcr(env);
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
>       /* then 16 predicates and the ffr */
>       case 34 ... 50:
>       {
>           int preg = reg - 34;
>           int vq, len = 0;
>           for (vq = 0; vq < cpu->sve_max_vq; vq = vq + 4) {
> -            len += gdb_get_reg64(buf, env->vfp.pregs[preg].p[vq / 4]);
> +            len += gdb_get_register_value(MO_TEUQ, buf,
> +                                          (uint8_t *) &env->vfp.pregs[preg].p[vq / 4]);
>           }
>           return len;
>       }
> @@ -165,8 +177,8 @@ int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
>            * We report in Vector Granules (VG) which is 64bit in a Z reg
>            * while the ZCR works in Vector Quads (VQ) which is 128bit chunks.
>            */
> -        int vq = sve_vqm1_for_el(env, arm_current_el(env)) + 1;
> -        return gdb_get_reg64(buf, vq * 2);
> +        uint64_t vq = (sve_vqm1_for_el(env, arm_current_el(env)) + 1) * 2;
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &vq);

s/MO_TEUL/MO_TEUQ/

>       }
>       default:
>           /* gdbstub asked for something out our range */
> @@ -248,10 +260,11 @@ int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
>               bool is_data = !(reg & 1);
>               bool is_high = reg & 2;
>               ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
> -            ARMVAParameters param;
> +            ARMVAParameters param = aa64_va_parameters(env, -is_high, mmu_idx,
> +                                                       is_data, false);
> +            uint64_t pauth_mask = pauth_ptr_mask(param);
>   
> -            param = aa64_va_parameters(env, -is_high, mmu_idx, is_data, false);
> -            return gdb_get_reg64(buf, pauth_ptr_mask(param));
> +            return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &pauth_mask);
>           }
>       default:
>           return 0;
> @@ -399,7 +412,7 @@ int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
>   
>       tcf0 = extract64(env->cp15.sctlr_el[1], 38, 2);
>   
> -    return gdb_get_reg64(buf, tcf0);
> +    return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &tcf0);
>   }
>   
>   int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)

Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Pierrick Bouvier]

On 3/19/25 11:22, Alex Bennée wrote:
> The current helper.h functions rely on hard coded assumptions about
> target endianess to use the tswap macros. We also end up double
> swapping a bunch of values if the target can run in multiple endianess
> modes. Avoid this by getting the target to pass the endianess and size
> via a MemOp and fixing up appropriately.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>   2 files changed, 52 insertions(+)
>   create mode 100644 include/gdbstub/registers.h
> 
> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
> new file mode 100644
> index 0000000000..4abc7a6ae7
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,30 @@
> +/*
> + * GDB Common Register Helpers
> + *
> + * Copyright (c) 2025 Linaro Ltd
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef GDB_REGISTERS_H
> +#define GDB_REGISTERS_H
> +
> +#include "exec/memop.h"
> +
> +/**
> + * gdb_get_register_value() - get register value for gdb
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to value in host order
> + *
> + * This replaces the previous legacy read functions with a single
> + * function to handle all sizes. Passing @mo allows the target mode to
> + * be taken into account and avoids using hard coded tswap() macros.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 282e13e163..3d7b1028e4 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -32,6 +32,7 @@
>   #include "exec/gdbstub.h"
>   #include "gdbstub/commands.h"
>   #include "gdbstub/syscalls.h"
> +#include "gdbstub/registers.h"
>   #ifdef CONFIG_USER_ONLY
>   #include "accel/tcg/vcpu-state.h"
>   #include "gdbstub/user.h"
> @@ -45,6 +46,7 @@
>   #include "system/runstate.h"
>   #include "exec/replay-core.h"
>   #include "exec/hwaddr.h"
> +#include "exec/memop.h"
>   
>   #include "internals.h"
>   
> @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>       return 0;
>   }
>   
> +/*
> + * Target helper function to read value into GByteArray, target
> + * supplies the size and target endianess via the MemOp.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
> +{
> +    size_t bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        for ( int i = bytes ; i > 0; i--) {
> +            g_byte_array_append(buf, &val[i - 1], 1);
> +        };
> +    } else {
> +        g_byte_array_append(buf, val, bytes);
> +    }
> +
> +    return bytes;
> +}
> +
> +
>   static void gdb_register_feature(CPUState *cpu, int base_reg,
>                                    gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>                                    const GDBFeature *feature)

It could be preferable to set buf with the value, instead of simply 
appending the value. This way, there is no need to return the size, as 
it's contained in buffer size itself.

If we insist on returning the size, it's better to make it a parameter 
(and use a void parameter type), because at the moment, it gives the 
impression the function itself returns the value, which may be confusing.

Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Pierrick Bouvier]

On 3/20/25 12:30, Pierrick Bouvier wrote:
> On 3/19/25 11:22, Alex Bennée wrote:
>> The current helper.h functions rely on hard coded assumptions about
>> target endianess to use the tswap macros. We also end up double
>> swapping a bunch of values if the target can run in multiple endianess
>> modes. Avoid this by getting the target to pass the endianess and size
>> via a MemOp and fixing up appropriately.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>    include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>>    gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>>    2 files changed, 52 insertions(+)
>>    create mode 100644 include/gdbstub/registers.h
>>
>> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
>> new file mode 100644
>> index 0000000000..4abc7a6ae7
>> --- /dev/null
>> +++ b/include/gdbstub/registers.h
>> @@ -0,0 +1,30 @@
>> +/*
>> + * GDB Common Register Helpers
>> + *
>> + * Copyright (c) 2025 Linaro Ltd
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef GDB_REGISTERS_H
>> +#define GDB_REGISTERS_H
>> +
>> +#include "exec/memop.h"
>> +
>> +/**
>> + * gdb_get_register_value() - get register value for gdb
>> + * mo: size and endian MemOp
>> + * buf: GByteArray to store in target order
>> + * val: pointer to value in host order
>> + *
>> + * This replaces the previous legacy read functions with a single
>> + * function to handle all sizes. Passing @mo allows the target mode to
>> + * be taken into account and avoids using hard coded tswap() macros.
>> + *
>> + * Returns the number of bytes written to the array.
>> + */
>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
>> +
>> +#endif /* GDB_REGISTERS_H */
>> +
>> +
>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> index 282e13e163..3d7b1028e4 100644
>> --- a/gdbstub/gdbstub.c
>> +++ b/gdbstub/gdbstub.c
>> @@ -32,6 +32,7 @@
>>    #include "exec/gdbstub.h"
>>    #include "gdbstub/commands.h"
>>    #include "gdbstub/syscalls.h"
>> +#include "gdbstub/registers.h"
>>    #ifdef CONFIG_USER_ONLY
>>    #include "accel/tcg/vcpu-state.h"
>>    #include "gdbstub/user.h"
>> @@ -45,6 +46,7 @@
>>    #include "system/runstate.h"
>>    #include "exec/replay-core.h"
>>    #include "exec/hwaddr.h"
>> +#include "exec/memop.h"
>>    
>>    #include "internals.h"
>>    
>> @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>>        return 0;
>>    }
>>    
>> +/*
>> + * Target helper function to read value into GByteArray, target
>> + * supplies the size and target endianess via the MemOp.
>> + */
>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
>> +{
>> +    size_t bytes = memop_size(op);
>> +
>> +    if (op & MO_BSWAP) {
>> +        for ( int i = bytes ; i > 0; i--) {
>> +            g_byte_array_append(buf, &val[i - 1], 1);
>> +        };
>> +    } else {
>> +        g_byte_array_append(buf, val, bytes);
>> +    }
>> +
>> +    return bytes;
>> +}
>> +
>> +
>>    static void gdb_register_feature(CPUState *cpu, int base_reg,
>>                                     gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>>                                     const GDBFeature *feature)
> 
> It could be preferable to set buf with the value, instead of simply
> appending the value. This way, there is no need to return the size, as
> it's contained in buffer size itself.
> 
> If we insist on returning the size, it's better to make it a parameter
> (and use a void parameter type), because at the moment, it gives the
> impression the function itself returns the value, which may be confusing.

Seems like it's the existing convention through 
gdb_set_reg_cb/gdb_get_reg_cb, so we have to follow this.

Re: [PATCH 01/10] include/gdbstub: fix include guard in commands.h

[Pierrick Bouvier]

On 3/19/25 11:22, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/gdbstub/commands.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
> index 40f0514fe9..bff3674872 100644
> --- a/include/gdbstub/commands.h
> +++ b/include/gdbstub/commands.h
> @@ -1,5 +1,5 @@
>   #ifndef GDBSTUB_COMMANDS_H
> -#define GDBSTUB
> +#define GDBSTUB_COMMANDS_H
>   
>   typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
>   

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Pierrick Bouvier]

On 3/19/25 11:22, Alex Bennée wrote:
> The current helper.h functions rely on hard coded assumptions about
> target endianess to use the tswap macros. We also end up double
> swapping a bunch of values if the target can run in multiple endianess
> modes. Avoid this by getting the target to pass the endianess and size
> via a MemOp and fixing up appropriately.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>   2 files changed, 52 insertions(+)
>   create mode 100644 include/gdbstub/registers.h
> 
> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
> new file mode 100644
> index 0000000000..4abc7a6ae7
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,30 @@
> +/*
> + * GDB Common Register Helpers
> + *
> + * Copyright (c) 2025 Linaro Ltd
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef GDB_REGISTERS_H
> +#define GDB_REGISTERS_H
> +
> +#include "exec/memop.h"
> +
> +/**
> + * gdb_get_register_value() - get register value for gdb
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to value in host order
> + *
> + * This replaces the previous legacy read functions with a single
> + * function to handle all sizes. Passing @mo allows the target mode to
> + * be taken into account and avoids using hard coded tswap() macros.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 282e13e163..3d7b1028e4 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -32,6 +32,7 @@
>   #include "exec/gdbstub.h"
>   #include "gdbstub/commands.h"
>   #include "gdbstub/syscalls.h"
> +#include "gdbstub/registers.h"
>   #ifdef CONFIG_USER_ONLY
>   #include "accel/tcg/vcpu-state.h"
>   #include "gdbstub/user.h"
> @@ -45,6 +46,7 @@
>   #include "system/runstate.h"
>   #include "exec/replay-core.h"
>   #include "exec/hwaddr.h"
> +#include "exec/memop.h"
>   
>   #include "internals.h"
>   
> @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>       return 0;
>   }
>   
> +/*
> + * Target helper function to read value into GByteArray, target
> + * supplies the size and target endianess via the MemOp.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
> +{
> +    size_t bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        for ( int i = bytes ; i > 0; i--) {
> +            g_byte_array_append(buf, &val[i - 1], 1);
> +        };
> +    } else {
> +        g_byte_array_append(buf, val, bytes);
> +    }
> +
> +    return bytes;
> +}
> +
> +
>   static void gdb_register_feature(CPUState *cpu, int base_reg,
>                                    gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>                                    const GDBFeature *feature)

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 03/10] target/arm: convert 32 bit gdbstub to new helper

[Pierrick Bouvier]

On 3/19/25 11:22, Alex Bennée wrote:
> For some of the helpers we need a temporary variable to copy from
> although we could add some helpers to return pointers into env in
> those cases if we wanted to.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/arm/gdbstub.c | 57 ++++++++++++++++++++++++++++----------------
>   1 file changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 30068c2262..14d931b0bf 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -20,7 +20,7 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "exec/gdbstub.h"
> -#include "gdbstub/helpers.h"
> +#include "gdbstub/registers.h"
>   #include "gdbstub/commands.h"
>   #include "system/tcg.h"
>   #include "internals.h"
> @@ -33,12 +33,16 @@ typedef struct RegisterSysregFeatureParam {
>       int n;
>   } RegisterSysregFeatureParam;
>   
> -/* Old gdb always expect FPA registers.  Newer (xml-aware) gdb only expect
> -   whatever the target description contains.  Due to a historical mishap
> -   the FPA registers appear in between core integer regs and the CPSR.
> -   We hack round this by giving the FPA regs zero size when talking to a
> -   newer gdb.  */
> -
> +/*
> + * Old gdb always expect FPA registers. Newer (xml-aware) gdb only
> + * expect whatever the target description contains. Due to a
> + * historical mishap the FPA registers appear in between core integer
> + * regs and the CPSR. We hack round this by giving the FPA regs zero
> + * size when talking to a newer gdb.
> + *
> + * While gdb cares about the memory endianess of the target all
> + * registers are passed in little-endian mode.
> + */
>   int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
> @@ -46,15 +50,17 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   
>       if (n < 16) {
>           /* Core integer register.  */
> -        return gdb_get_reg32(mem_buf, env->regs[n]);
> +        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &env->regs[n]);
>       }
>       if (n == 25) {
>           /* CPSR, or XPSR for M-profile */
> +        uint32_t reg;
>           if (arm_feature(env, ARM_FEATURE_M)) {
> -            return gdb_get_reg32(mem_buf, xpsr_read(env));
> +            reg = xpsr_read(env);
>           } else {
> -            return gdb_get_reg32(mem_buf, cpsr_read(env));
> +            reg = cpsr_read(env);
>           }
> +        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &reg);
>       }
>       /* Unknown register.  */
>       return 0;
> @@ -115,19 +121,22 @@ static int vfp_gdb_get_reg(CPUState *cs, GByteArray *buf, int reg)
>   
>       /* VFP data registers are always little-endian.  */
>       if (reg < nregs) {
> -        return gdb_get_reg64(buf, *aa32_vfp_dreg(env, reg));
> +        return gdb_get_register_value(MO_TEUQ, buf,
> +                                      (uint8_t *) aa32_vfp_dreg(env, reg));
>       }
>       if (arm_feature(env, ARM_FEATURE_NEON)) {
>           /* Aliases for Q regs.  */
>           nregs += 16;
>           if (reg < nregs) {
>               uint64_t *q = aa32_vfp_qreg(env, reg - 32);
> -            return gdb_get_reg128(buf, q[0], q[1]);
> +            return gdb_get_register_value(MO_TEUO, buf, (uint8_t *) q);
>           }
>       }
>       switch (reg - nregs) {
> +        uint32_t fpcr;
>       case 0:
> -        return gdb_get_reg32(buf, vfp_get_fpscr(env));
> +        fpcr = vfp_get_fpscr(env);
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpcr);
>       }
>       return 0;
>   }
> @@ -166,9 +175,11 @@ static int vfp_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
>   
>       switch (reg) {
>       case 0:
> -        return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]);
> +        return gdb_get_register_value(MO_TEUL, buf,
> +                                      (uint8_t *) &env->vfp.xregs[ARM_VFP_FPSID]);
>       case 1:
> -        return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]);
> +        return gdb_get_register_value(MO_TEUL, buf,
> +                                      (uint8_t *) &env->vfp.xregs[ARM_VFP_FPEXC]);
>       }
>       return 0;
>   }
> @@ -196,7 +207,8 @@ static int mve_gdb_get_reg(CPUState *cs, GByteArray *buf, int reg)
>   
>       switch (reg) {
>       case 0:
> -        return gdb_get_reg32(buf, env->v7m.vpr);
> +        return gdb_get_register_value(MO_TEUL, buf,
> +                                      (uint8_t *) &env->v7m.vpr);
>       default:
>           return 0;
>       }
> @@ -236,9 +248,11 @@ static int arm_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg)
>       ri = get_arm_cp_reginfo(cpu->cp_regs, key);
>       if (ri) {
>           if (cpreg_field_is_64bit(ri)) {
> -            return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri));
> +            uint64_t cpreg = read_raw_cp_reg(env, ri);
> +            return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &cpreg);
>           } else {
> -            return gdb_get_reg32(buf, (uint32_t)read_raw_cp_reg(env, ri));
> +            uint32_t cpreg = (uint32_t) read_raw_cp_reg(env, ri);
> +            return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &cpreg);
>           }
>       }
>       return 0;
> @@ -375,12 +389,12 @@ static uint32_t *m_sysreg_ptr(CPUARMState *env, MProfileSysreg reg, bool sec)
>   static int m_sysreg_get(CPUARMState *env, GByteArray *buf,
>                           MProfileSysreg reg, bool secure)
>   {
> -    uint32_t *ptr = m_sysreg_ptr(env, reg, secure);
> +    uint8_t *ptr = (uint8_t *) m_sysreg_ptr(env, reg, secure);
>   
>       if (ptr == NULL) {
>           return 0;
>       }
> -    return gdb_get_reg32(buf, *ptr);
> +    return gdb_get_register_value(MO_TEUL, buf, ptr);
>   }
>   
>   static int arm_gdb_get_m_systemreg(CPUState *cs, GByteArray *buf, int reg)
> @@ -393,7 +407,8 @@ static int arm_gdb_get_m_systemreg(CPUState *cs, GByteArray *buf, int reg)
>        * banked and non-banked bits.
>        */
>       if (reg == M_SYSREG_CONTROL) {
> -        return gdb_get_reg32(buf, arm_v7m_mrs_control(env, env->v7m.secure));
> +        uint32_t reg32 = arm_v7m_mrs_control(env, env->v7m.secure);
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &reg32);
>       }
>       return m_sysreg_get(env, buf, reg, env->v7m.secure);
>   }

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 04/10] target/arm: convert 64 bit gdbstub to new helper

[Pierrick Bouvier]

On 3/19/25 11:22, Alex Bennée wrote:
> For some of the helpers we need a temporary variable to copy from
> although we could add some helpers to return pointers into env in
> those cases if we wanted to.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/arm/gdbstub64.c | 53 ++++++++++++++++++++++++++----------------
>   1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index 1a4dbec567..793332af31 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -20,7 +20,7 @@
>   #include "qemu/log.h"
>   #include "cpu.h"
>   #include "internals.h"
> -#include "gdbstub/helpers.h"
> +#include "gdbstub/registers.h"
>   #include "gdbstub/commands.h"
>   #include "tcg/mte_helper.h"
>   #if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
> @@ -35,15 +35,16 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   
>       if (n < 31) {
>           /* Core integer register.  */
> -        return gdb_get_reg64(mem_buf, env->xregs[n]);
> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->xregs[n]);
>       }
>       switch (n) {
>       case 31:
> -        return gdb_get_reg64(mem_buf, env->xregs[31]);
> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->xregs[31]);
>       case 32:
> -        return gdb_get_reg64(mem_buf, env->pc);
> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->pc);

For the 3 occurrences, should it be MO_TEUQ instead?

>       case 33:
> -        return gdb_get_reg32(mem_buf, pstate_read(env));
> +        uint32_t pstate = pstate_read(env);
> +        return gdb_get_register_value(MO_TEUL, mem_buf, (uint8_t *) &pstate);
>       }
>       /* Unknown register.  */
>       return 0;
> @@ -82,23 +83,27 @@ int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> +    uint32_t fpr;
>   
>       switch (reg) {
>       case 0 ... 31:
>       {
>           /* 128 bit FP register - quads are in LE order */
>           uint64_t *q = aa64_vfp_qreg(env, reg);
> -        return gdb_get_reg128(buf, q[1], q[0]);
> +        return gdb_get_register_value(MO_TEUO, buf, (uint8_t *) q);
>       }
>       case 32:
>           /* FPSR */
> -        return gdb_get_reg32(buf, vfp_get_fpsr(env));
> +        fpr = vfp_get_fpsr(env);
> +        break;
>       case 33:
>           /* FPCR */
> -        return gdb_get_reg32(buf, vfp_get_fpcr(env));
> +        fpr = vfp_get_fpcr(env);
> +        break;
>       default:
>           return 0;
>       }
> +    return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
>   }
>   
>   int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg)
> @@ -132,30 +137,37 @@ int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> +    uint32_t fpr;
>   
>       switch (reg) {
>       /* The first 32 registers are the zregs */
>       case 0 ... 31:
>       {
>           int vq, len = 0;
> +        ARMVectorReg *zreg = &env->vfp.zregs[reg];
> +
>           for (vq = 0; vq < cpu->sve_max_vq; vq++) {
> -            len += gdb_get_reg128(buf,
> -                                  env->vfp.zregs[reg].d[vq * 2 + 1],
> -                                  env->vfp.zregs[reg].d[vq * 2]);
> +            len += gdb_get_register_value(MO_TEUQ, buf,
> +                                          (uint8_t *) &zreg->d[vq * 2 + 1]);
> +            len += gdb_get_register_value(MO_TEUQ, buf,
> +                                          (uint8_t *) &zreg->d[vq * 2]);
>           }
>           return len;
>       }
>       case 32:
> -        return gdb_get_reg32(buf, vfp_get_fpsr(env));
> +        fpr = vfp_get_fpsr(env);
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
>       case 33:
> -        return gdb_get_reg32(buf, vfp_get_fpcr(env));
> +        fpr = vfp_get_fpcr(env);
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &fpr);
>       /* then 16 predicates and the ffr */
>       case 34 ... 50:
>       {
>           int preg = reg - 34;
>           int vq, len = 0;
>           for (vq = 0; vq < cpu->sve_max_vq; vq = vq + 4) {
> -            len += gdb_get_reg64(buf, env->vfp.pregs[preg].p[vq / 4]);
> +            len += gdb_get_register_value(MO_TEUQ, buf,
> +                                          (uint8_t *) &env->vfp.pregs[preg].p[vq / 4]);
>           }
>           return len;
>       }
> @@ -165,8 +177,8 @@ int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
>            * We report in Vector Granules (VG) which is 64bit in a Z reg
>            * while the ZCR works in Vector Quads (VQ) which is 128bit chunks.
>            */
> -        int vq = sve_vqm1_for_el(env, arm_current_el(env)) + 1;
> -        return gdb_get_reg64(buf, vq * 2);
> +        uint64_t vq = (sve_vqm1_for_el(env, arm_current_el(env)) + 1) * 2;
> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &vq);
>       }

Should it be MO_TEUQ instead?

>       default:
>           /* gdbstub asked for something out our range */
> @@ -248,10 +260,11 @@ int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
>               bool is_data = !(reg & 1);
>               bool is_high = reg & 2;
>               ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
> -            ARMVAParameters param;
> +            ARMVAParameters param = aa64_va_parameters(env, -is_high, mmu_idx,
> +                                                       is_data, false);
> +            uint64_t pauth_mask = pauth_ptr_mask(param);
>   
> -            param = aa64_va_parameters(env, -is_high, mmu_idx, is_data, false);
> -            return gdb_get_reg64(buf, pauth_ptr_mask(param));
> +            return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &pauth_mask);
>           }
>       default:
>           return 0;
> @@ -399,7 +412,7 @@ int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
>   
>       tcf0 = extract64(env->cp15.sctlr_el[1], 38, 2);
>   
> -    return gdb_get_reg64(buf, tcf0);
> +    return gdb_get_register_value(MO_TEUQ, buf, (uint8_t *) &tcf0);
>   }
>   
>   int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)

Re: [PATCH 05/10] target/ppc: expand comment on FP/VMX/VSX access functions

[Pierrick Bouvier]

On 3/19/25 11:22, Alex Bennée wrote:
> Mainly as an aid to myself getting confused too many bswaps deep into
> the code.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/ppc/cpu.h | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index efab54a068..1e833ade04 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2906,7 +2906,12 @@ static inline bool lsw_reg_in_range(int start, int nregs, int rx)
>              (start + nregs > 32 && (rx >= start || rx < start + nregs - 32));
>   }
>   
> -/* Accessors for FP, VMX and VSX registers */
> +/*
> + * Access functions for FP, VMX and VSX registers
> + *
> + * The register is stored as a 128 bit host endian value so we need to
> + * take that into account when accessing smaller parts of it.
> + */
>   #if HOST_BIG_ENDIAN
>   #define VsrB(i) u8[i]
>   #define VsrSB(i) s8[i]

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 06/10] target/ppc: make ppc_maybe_bswap_register static

[Pierrick Bouvier]

On 3/19/25 11:22, Alex Bennée wrote:
> It's not used outside of the gdbstub code.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/ppc/cpu.h     | 1 -
>   target/ppc/gdbstub.c | 2 +-
>   2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 1e833ade04..950bb6e06c 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -3016,7 +3016,6 @@ static inline bool ppc_interrupts_little_endian(PowerPCCPU *cpu, bool hv)
>   
>   void dump_mmu(CPUPPCState *env);
>   
> -void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
>   void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
>   uint32_t ppc_get_vscr(CPUPPCState *env);
>   void ppc_set_cr(CPUPPCState *env, uint64_t cr);
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 3b28d4e21c..c09e93abaf 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -81,7 +81,7 @@ static int ppc_gdb_register_len(int n)
>    * TARGET_BIG_ENDIAN is always set, and we must check the current
>    * mode of the chip to see if we're running in little-endian.
>    */
> -void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
> +static void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
>   {
>   #ifndef CONFIG_USER_ONLY
>       if (!FIELD_EX64(env->msr, MSR, LE)) {

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Pierrick Bouvier]

On 3/19/25 11:22, Alex Bennée wrote:
> The aim of this work is to get rid of the endian aware helpers in
> gdbstub/helpers.h which due to their use of tswap() mean target
> gdbstubs need to be built multiple times. While this series doesn't
> actually build each stub once it introduces a new helper -
> gdb_get_register_value() which takes a MemOp which can describe the
> current endian state of the system. This will be a lot easier to
> dynamically feed from a helper function.
> 
> The most complex example is PPC which has a helper called
> ppc_maybe_bswap_register() which was doing this.
> 
> This is still an RFC so I'm interested in feedback:
> 
>    - is the API sane
>    - can we avoid lots of (uint8_t *) casting?

Even though the series has a good intent, the fact we make everything 
"generic" makes that we lose all guarantees we could get by relying on 
static typing, and that we had possibility of mistakes when passing size 
(which happened in patch 4 if I'm correct). And explicit casting comes 
as a *strong* warning about that.

By patch 7, I was really feeling it's not a win vs explicit functions 
per size.

If the goal of the series is to get rid of endian aware helpers, well, 
this can be fixed in the helpers themselves, without needing to 
introduce a "generic" size helper. Maybe we are trying to solve two 
different problems here?

>    - should we have a reverse helper for setting registers
> 
> If this seems like the right approach I can have a go at more of the
> frontends later.
> 
> There are a few other misc clean-ups I did on the way which might be
> worth cherry picking for 10.0 but I'll leave that up to maintainers.
> 
> Alex.
> 
> Alex Bennée (10):
>    include/gdbstub: fix include guard in commands.h
>    gdbstub: introduce target independent gdb register helper
>    target/arm: convert 32 bit gdbstub to new helper
>    target/arm: convert 64 bit gdbstub to new helper
>    target/ppc: expand comment on FP/VMX/VSX access functions
>    target/ppc: make ppc_maybe_bswap_register static
>    target/ppc: convert gdbstub to new helper (!hacky)
>    gdbstub: assert earlier in handle_read_all_regs
>    include/exec: fix assert in size_memop
>    target/microblaze: convert gdbstub to new helper
> 
>   include/exec/memop.h        |   4 +-
>   include/gdbstub/commands.h  |   2 +-
>   include/gdbstub/registers.h |  30 ++++++
>   target/ppc/cpu.h            |   8 +-
>   gdbstub/gdbstub.c           |  24 ++++-
>   target/arm/gdbstub.c        |  57 +++++++----
>   target/arm/gdbstub64.c      |  53 ++++++----
>   target/microblaze/gdbstub.c |  44 ++++----
>   target/ppc/gdbstub.c        | 194 ++++++++++++++++++++----------------
>   9 files changed, 257 insertions(+), 159 deletions(-)
>   create mode 100644 include/gdbstub/registers.h
> 

Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Pierrick Bouvier]

On 3/20/25 12:52, Pierrick Bouvier wrote:
> On 3/19/25 11:22, Alex Bennée wrote:
>> The aim of this work is to get rid of the endian aware helpers in
>> gdbstub/helpers.h which due to their use of tswap() mean target
>> gdbstubs need to be built multiple times. While this series doesn't
>> actually build each stub once it introduces a new helper -
>> gdb_get_register_value() which takes a MemOp which can describe the
>> current endian state of the system. This will be a lot easier to
>> dynamically feed from a helper function.
>>
>> The most complex example is PPC which has a helper called
>> ppc_maybe_bswap_register() which was doing this.
>>
>> This is still an RFC so I'm interested in feedback:
>>
>>     - is the API sane
>>     - can we avoid lots of (uint8_t *) casting?
> 
> Even though the series has a good intent, the fact we make everything
> "generic" makes that we lose all guarantees we could get by relying on
> static typing, and that we had possibility of mistakes when passing size
> (which happened in patch 4 if I'm correct). And explicit casting comes
> as a *strong* warning about that.
> 
> By patch 7, I was really feeling it's not a win vs explicit functions
> per size.
> 
> If the goal of the series is to get rid of endian aware helpers, well,
> this can be fixed in the helpers themselves, without needing to
> introduce a "generic" size helper. Maybe we are trying to solve two
> different problems here?
> 
>>     - should we have a reverse helper for setting registers
>>
>> If this seems like the right approach I can have a go at more of the
>> frontends later.
>>

Looking at include/gdbstub/helpers.h, gdb_get_reg128 can be solved by 
using target_words_bigendian() instead of TARGET_BIG_ENDIAN, which is 
already what tswap primitives are doing.

For gdb_get_regl, I would get rid of it completely, by editing all 
targets gdbstub.c, and replacing with gdb_get_reg32 or gdb_get_reg64 
explicit calls.
ppc is a very naughty boy, because registers are defined as 
target_ulong, while other arch use fixed types. The solution might be as 
simple as changing ppc registers definition to uint64_t.
If it's too complicated, you can postpone the problem by leaving 
gdb_get_regl defined only in ppc gdbstub, and clean up all other arch.
Thanks to static typing, it will be easy to spot a wrong gdb_get_regl 
conversion, so it's a no-risk operaton.

For ldtul_p, ldtul_le_p, and ldtul_be_p, it's a similar game. It's 
harder because only return type will differ, and you might miss occurences.
A safe way could be to replace ldtul_p by call to a function taking 
return value through pointer in parameter. This way, you can replace 
easily with l and q variants, without any risk off implicit conversion.
And for the one left depending on target_ulong/target_long, leave that 
for now, and move ldtul*_p to a target-helper.h included only for archs 
needing this.

>> There are a few other misc clean-ups I did on the way which might be
>> worth cherry picking for 10.0 but I'll leave that up to maintainers.
>>
>> Alex.
>>
>> Alex Bennée (10):
>>     include/gdbstub: fix include guard in commands.h
>>     gdbstub: introduce target independent gdb register helper
>>     target/arm: convert 32 bit gdbstub to new helper
>>     target/arm: convert 64 bit gdbstub to new helper
>>     target/ppc: expand comment on FP/VMX/VSX access functions
>>     target/ppc: make ppc_maybe_bswap_register static
>>     target/ppc: convert gdbstub to new helper (!hacky)
>>     gdbstub: assert earlier in handle_read_all_regs
>>     include/exec: fix assert in size_memop
>>     target/microblaze: convert gdbstub to new helper
>>
>>    include/exec/memop.h        |   4 +-
>>    include/gdbstub/commands.h  |   2 +-
>>    include/gdbstub/registers.h |  30 ++++++
>>    target/ppc/cpu.h            |   8 +-
>>    gdbstub/gdbstub.c           |  24 ++++-
>>    target/arm/gdbstub.c        |  57 +++++++----
>>    target/arm/gdbstub64.c      |  53 ++++++----
>>    target/microblaze/gdbstub.c |  44 ++++----
>>    target/ppc/gdbstub.c        | 194 ++++++++++++++++++++----------------
>>    9 files changed, 257 insertions(+), 159 deletions(-)
>>    create mode 100644 include/gdbstub/registers.h
>>
> 

Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 3/20/25 12:30, Pierrick Bouvier wrote:
>> On 3/19/25 11:22, Alex Bennée wrote:
>>> The current helper.h functions rely on hard coded assumptions about
>>> target endianess to use the tswap macros. We also end up double
>>> swapping a bunch of values if the target can run in multiple endianess
>>> modes. Avoid this by getting the target to pass the endianess and size
>>> via a MemOp and fixing up appropriately.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>    include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>>>    gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>>>    2 files changed, 52 insertions(+)
>>>    create mode 100644 include/gdbstub/registers.h
>>>
>>> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
>>> new file mode 100644
>>> index 0000000000..4abc7a6ae7
>>> --- /dev/null
>>> +++ b/include/gdbstub/registers.h
>>> @@ -0,0 +1,30 @@
>>> +/*
>>> + * GDB Common Register Helpers
>>> + *
>>> + * Copyright (c) 2025 Linaro Ltd
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#ifndef GDB_REGISTERS_H
>>> +#define GDB_REGISTERS_H
>>> +
>>> +#include "exec/memop.h"
>>> +
>>> +/**
>>> + * gdb_get_register_value() - get register value for gdb
>>> + * mo: size and endian MemOp
>>> + * buf: GByteArray to store in target order
>>> + * val: pointer to value in host order
>>> + *
>>> + * This replaces the previous legacy read functions with a single
>>> + * function to handle all sizes. Passing @mo allows the target mode to
>>> + * be taken into account and avoids using hard coded tswap() macros.
>>> + *
>>> + * Returns the number of bytes written to the array.
>>> + */
>>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
>>> +
>>> +#endif /* GDB_REGISTERS_H */
>>> +
>>> +
>>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>>> index 282e13e163..3d7b1028e4 100644
>>> --- a/gdbstub/gdbstub.c
>>> +++ b/gdbstub/gdbstub.c
>>> @@ -32,6 +32,7 @@
>>>    #include "exec/gdbstub.h"
>>>    #include "gdbstub/commands.h"
>>>    #include "gdbstub/syscalls.h"
>>> +#include "gdbstub/registers.h"
>>>    #ifdef CONFIG_USER_ONLY
>>>    #include "accel/tcg/vcpu-state.h"
>>>    #include "gdbstub/user.h"
>>> @@ -45,6 +46,7 @@
>>>    #include "system/runstate.h"
>>>    #include "exec/replay-core.h"
>>>    #include "exec/hwaddr.h"
>>> +#include "exec/memop.h"
>>>       #include "internals.h"
>>>    @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState
>>> *cpu, uint8_t *mem_buf, int reg)
>>>        return 0;
>>>    }
>>>    +/*
>>> + * Target helper function to read value into GByteArray, target
>>> + * supplies the size and target endianess via the MemOp.
>>> + */
>>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
>>> +{
>>> +    size_t bytes = memop_size(op);
>>> +
>>> +    if (op & MO_BSWAP) {
>>> +        for ( int i = bytes ; i > 0; i--) {
>>> +            g_byte_array_append(buf, &val[i - 1], 1);
>>> +        };
>>> +    } else {
>>> +        g_byte_array_append(buf, val, bytes);
>>> +    }
>>> +
>>> +    return bytes;
>>> +}
>>> +
>>> +
>>>    static void gdb_register_feature(CPUState *cpu, int base_reg,
>>>                                     gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>>>                                     const GDBFeature *feature)
>> It could be preferable to set buf with the value, instead of simply
>> appending the value. This way, there is no need to return the size, as
>> it's contained in buffer size itself.
>> If we insist on returning the size, it's better to make it a
>> parameter
>> (and use a void parameter type), because at the moment, it gives the
>> impression the function itself returns the value, which may be confusing.
>
> Seems like it's the existing convention through
> gdb_set_reg_cb/gdb_get_reg_cb, so we have to follow this.

For the "g" packet we append multiple registers so the buffer size grows
as we append each one.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 04/10] target/arm: convert 64 bit gdbstub to new helper

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 3/19/25 11:22, Alex Bennée wrote:
>> For some of the helpers we need a temporary variable to copy from
>> although we could add some helpers to return pointers into env in
>> those cases if we wanted to.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   target/arm/gdbstub64.c | 53 ++++++++++++++++++++++++++----------------
>>   1 file changed, 33 insertions(+), 20 deletions(-)
>> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
>> index 1a4dbec567..793332af31 100644
>> --- a/target/arm/gdbstub64.c
>> +++ b/target/arm/gdbstub64.c
>> @@ -20,7 +20,7 @@
>>   #include "qemu/log.h"
>>   #include "cpu.h"
>>   #include "internals.h"
>> -#include "gdbstub/helpers.h"
>> +#include "gdbstub/registers.h"
>>   #include "gdbstub/commands.h"
>>   #include "tcg/mte_helper.h"
>>   #if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
>> @@ -35,15 +35,16 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>>         if (n < 31) {
>>           /* Core integer register.  */
>> -        return gdb_get_reg64(mem_buf, env->xregs[n]);
>> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->xregs[n]);
>>       }
>>       switch (n) {
>>       case 31:
>> -        return gdb_get_reg64(mem_buf, env->xregs[31]);
>> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->xregs[31]);
>>       case 32:
>> -        return gdb_get_reg64(mem_buf, env->pc);
>> +        return gdb_get_register_value(MO_TEUO, mem_buf, (uint8_t *) &env->pc);
>
> For the 3 occurrences, should it be MO_TEUQ instead?

>> -        int vq = sve_vqm1_for_el(env, arm_current_el(env)) + 1;
>> -        return gdb_get_reg64(buf, vq * 2);
>> +        uint64_t vq = (sve_vqm1_for_el(env, arm_current_el(env)) + 1) * 2;
>> +        return gdb_get_register_value(MO_TEUL, buf, (uint8_t *) &vq);
>>       }
>
> Should it be MO_TEUQ instead?

I've taken Philippe's suggestion and used explicit MO_32/64 which is
easier to follow.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 3/19/25 11:22, Alex Bennée wrote:
>> The aim of this work is to get rid of the endian aware helpers in
>> gdbstub/helpers.h which due to their use of tswap() mean target
>> gdbstubs need to be built multiple times. While this series doesn't
>> actually build each stub once it introduces a new helper -
>> gdb_get_register_value() which takes a MemOp which can describe the
>> current endian state of the system. This will be a lot easier to
>> dynamically feed from a helper function.
>> The most complex example is PPC which has a helper called
>> ppc_maybe_bswap_register() which was doing this.
>> This is still an RFC so I'm interested in feedback:
>>    - is the API sane
>>    - can we avoid lots of (uint8_t *) casting?
>
> Even though the series has a good intent, the fact we make everything
> "generic" makes that we lose all guarantees we could get by relying on
> static typing, and that we had possibility of mistakes when passing
> size (which happened in patch 4 if I'm correct). And explicit casting
> comes as a *strong* warning about that.
>
> By patch 7, I was really feeling it's not a win vs explicit functions
> per size.
>
> If the goal of the series is to get rid of endian aware helpers, well,
> this can be fixed in the helpers themselves, without needing to
> introduce a "generic" size helper. Maybe we are trying to solve two
> different problems here?

It did seem natural that if you were defining a MemOp you would use all
of it rather than only its endian definition. But you are right we could
introduce the same helpers with a bool flag for endianess.

Maybe we should have fully formed mops and just assert in the helper:

  gdb_get_reg32(MemOp op, GByteArray *buf, uint32_t val) {
      g_assert(op & MO_SIZE == MO_32);
      gdb_get_register_value(op, buf, &val);
  }

I was also trying to avoid over boilerplating the code.

>
>>    - should we have a reverse helper for setting registers
>> If this seems like the right approach I can have a go at more of the
>> frontends later.
>> There are a few other misc clean-ups I did on the way which might be
>> worth cherry picking for 10.0 but I'll leave that up to maintainers.
>> Alex.
>> Alex Bennée (10):
>>    include/gdbstub: fix include guard in commands.h
>>    gdbstub: introduce target independent gdb register helper
>>    target/arm: convert 32 bit gdbstub to new helper
>>    target/arm: convert 64 bit gdbstub to new helper
>>    target/ppc: expand comment on FP/VMX/VSX access functions
>>    target/ppc: make ppc_maybe_bswap_register static
>>    target/ppc: convert gdbstub to new helper (!hacky)
>>    gdbstub: assert earlier in handle_read_all_regs
>>    include/exec: fix assert in size_memop
>>    target/microblaze: convert gdbstub to new helper
>>   include/exec/memop.h        |   4 +-
>>   include/gdbstub/commands.h  |   2 +-
>>   include/gdbstub/registers.h |  30 ++++++
>>   target/ppc/cpu.h            |   8 +-
>>   gdbstub/gdbstub.c           |  24 ++++-
>>   target/arm/gdbstub.c        |  57 +++++++----
>>   target/arm/gdbstub64.c      |  53 ++++++----
>>   target/microblaze/gdbstub.c |  44 ++++----
>>   target/ppc/gdbstub.c        | 194 ++++++++++++++++++++----------------
>>   9 files changed, 257 insertions(+), 159 deletions(-)
>>   create mode 100644 include/gdbstub/registers.h
>> 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Philippe Mathieu-Daudé]

On 20/3/25 21:16, Pierrick Bouvier wrote:
> On 3/20/25 12:52, Pierrick Bouvier wrote:
>> On 3/19/25 11:22, Alex Bennée wrote:
>>> The aim of this work is to get rid of the endian aware helpers in
>>> gdbstub/helpers.h which due to their use of tswap() mean target
>>> gdbstubs need to be built multiple times. While this series doesn't
>>> actually build each stub once it introduces a new helper -
>>> gdb_get_register_value() which takes a MemOp which can describe the
>>> current endian state of the system. This will be a lot easier to
>>> dynamically feed from a helper function.
>>>
>>> The most complex example is PPC which has a helper called
>>> ppc_maybe_bswap_register() which was doing this.
>>>
>>> This is still an RFC so I'm interested in feedback:
>>>
>>>     - is the API sane
>>>     - can we avoid lots of (uint8_t *) casting?
>>
>> Even though the series has a good intent, the fact we make everything
>> "generic" makes that we lose all guarantees we could get by relying on
>> static typing, and that we had possibility of mistakes when passing size
>> (which happened in patch 4 if I'm correct). And explicit casting comes
>> as a *strong* warning about that.
>>
>> By patch 7, I was really feeling it's not a win vs explicit functions
>> per size.
>>
>> If the goal of the series is to get rid of endian aware helpers, well,
>> this can be fixed in the helpers themselves, without needing to
>> introduce a "generic" size helper. Maybe we are trying to solve two
>> different problems here?
>>
>>>     - should we have a reverse helper for setting registers
>>>
>>> If this seems like the right approach I can have a go at more of the
>>> frontends later.
>>>
> 
> Looking at include/gdbstub/helpers.h, gdb_get_reg128 can be solved by 
> using target_words_bigendian() instead of TARGET_BIG_ENDIAN, which is 
> already what tswap primitives are doing.

We'll need to eventually remove target_words_bigendian(), so that'd just
be postponing that.

Re: [PATCH 02/10] gdbstub: introduce target independent gdb register helper

[Pierrick Bouvier]

On 3/21/25 04:36, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 3/20/25 12:30, Pierrick Bouvier wrote:
>>> On 3/19/25 11:22, Alex Bennée wrote:
>>>> The current helper.h functions rely on hard coded assumptions about
>>>> target endianess to use the tswap macros. We also end up double
>>>> swapping a bunch of values if the target can run in multiple endianess
>>>> modes. Avoid this by getting the target to pass the endianess and size
>>>> via a MemOp and fixing up appropriately.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> ---
>>>>     include/gdbstub/registers.h | 30 ++++++++++++++++++++++++++++++
>>>>     gdbstub/gdbstub.c           | 22 ++++++++++++++++++++++
>>>>     2 files changed, 52 insertions(+)
>>>>     create mode 100644 include/gdbstub/registers.h
>>>>
>>>> diff --git a/include/gdbstub/registers.h b/include/gdbstub/registers.h
>>>> new file mode 100644
>>>> index 0000000000..4abc7a6ae7
>>>> --- /dev/null
>>>> +++ b/include/gdbstub/registers.h
>>>> @@ -0,0 +1,30 @@
>>>> +/*
>>>> + * GDB Common Register Helpers
>>>> + *
>>>> + * Copyright (c) 2025 Linaro Ltd
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + */
>>>> +
>>>> +#ifndef GDB_REGISTERS_H
>>>> +#define GDB_REGISTERS_H
>>>> +
>>>> +#include "exec/memop.h"
>>>> +
>>>> +/**
>>>> + * gdb_get_register_value() - get register value for gdb
>>>> + * mo: size and endian MemOp
>>>> + * buf: GByteArray to store in target order
>>>> + * val: pointer to value in host order
>>>> + *
>>>> + * This replaces the previous legacy read functions with a single
>>>> + * function to handle all sizes. Passing @mo allows the target mode to
>>>> + * be taken into account and avoids using hard coded tswap() macros.
>>>> + *
>>>> + * Returns the number of bytes written to the array.
>>>> + */
>>>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val);
>>>> +
>>>> +#endif /* GDB_REGISTERS_H */
>>>> +
>>>> +
>>>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>>>> index 282e13e163..3d7b1028e4 100644
>>>> --- a/gdbstub/gdbstub.c
>>>> +++ b/gdbstub/gdbstub.c
>>>> @@ -32,6 +32,7 @@
>>>>     #include "exec/gdbstub.h"
>>>>     #include "gdbstub/commands.h"
>>>>     #include "gdbstub/syscalls.h"
>>>> +#include "gdbstub/registers.h"
>>>>     #ifdef CONFIG_USER_ONLY
>>>>     #include "accel/tcg/vcpu-state.h"
>>>>     #include "gdbstub/user.h"
>>>> @@ -45,6 +46,7 @@
>>>>     #include "system/runstate.h"
>>>>     #include "exec/replay-core.h"
>>>>     #include "exec/hwaddr.h"
>>>> +#include "exec/memop.h"
>>>>        #include "internals.h"
>>>>     @@ -551,6 +553,26 @@ static int gdb_write_register(CPUState
>>>> *cpu, uint8_t *mem_buf, int reg)
>>>>         return 0;
>>>>     }
>>>>     +/*
>>>> + * Target helper function to read value into GByteArray, target
>>>> + * supplies the size and target endianess via the MemOp.
>>>> + */
>>>> +int gdb_get_register_value(MemOp op, GByteArray *buf, uint8_t *val)
>>>> +{
>>>> +    size_t bytes = memop_size(op);
>>>> +
>>>> +    if (op & MO_BSWAP) {
>>>> +        for ( int i = bytes ; i > 0; i--) {
>>>> +            g_byte_array_append(buf, &val[i - 1], 1);
>>>> +        };
>>>> +    } else {
>>>> +        g_byte_array_append(buf, val, bytes);
>>>> +    }
>>>> +
>>>> +    return bytes;
>>>> +}
>>>> +
>>>> +
>>>>     static void gdb_register_feature(CPUState *cpu, int base_reg,
>>>>                                      gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>>>>                                      const GDBFeature *feature)
>>> It could be preferable to set buf with the value, instead of simply
>>> appending the value. This way, there is no need to return the size, as
>>> it's contained in buffer size itself.
>>> If we insist on returning the size, it's better to make it a
>>> parameter
>>> (and use a void parameter type), because at the moment, it gives the
>>> impression the function itself returns the value, which may be confusing.
>>
>> Seems like it's the existing convention through
>> gdb_set_reg_cb/gdb_get_reg_cb, so we have to follow this.
> 
> For the "g" packet we append multiple registers so the buffer size grows
> as we append each one.
> 

Thanks for sharing the rationale behind this.

Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Pierrick Bouvier]

On 3/21/25 06:02, Philippe Mathieu-Daudé wrote:
> On 20/3/25 21:16, Pierrick Bouvier wrote:
>> On 3/20/25 12:52, Pierrick Bouvier wrote:
>>> On 3/19/25 11:22, Alex Bennée wrote:
>>>> The aim of this work is to get rid of the endian aware helpers in
>>>> gdbstub/helpers.h which due to their use of tswap() mean target
>>>> gdbstubs need to be built multiple times. While this series doesn't
>>>> actually build each stub once it introduces a new helper -
>>>> gdb_get_register_value() which takes a MemOp which can describe the
>>>> current endian state of the system. This will be a lot easier to
>>>> dynamically feed from a helper function.
>>>>
>>>> The most complex example is PPC which has a helper called
>>>> ppc_maybe_bswap_register() which was doing this.
>>>>
>>>> This is still an RFC so I'm interested in feedback:
>>>>
>>>>      - is the API sane
>>>>      - can we avoid lots of (uint8_t *) casting?
>>>
>>> Even though the series has a good intent, the fact we make everything
>>> "generic" makes that we lose all guarantees we could get by relying on
>>> static typing, and that we had possibility of mistakes when passing size
>>> (which happened in patch 4 if I'm correct). And explicit casting comes
>>> as a *strong* warning about that.
>>>
>>> By patch 7, I was really feeling it's not a win vs explicit functions
>>> per size.
>>>
>>> If the goal of the series is to get rid of endian aware helpers, well,
>>> this can be fixed in the helpers themselves, without needing to
>>> introduce a "generic" size helper. Maybe we are trying to solve two
>>> different problems here?
>>>
>>>>      - should we have a reverse helper for setting registers
>>>>
>>>> If this seems like the right approach I can have a go at more of the
>>>> frontends later.
>>>>
>>
>> Looking at include/gdbstub/helpers.h, gdb_get_reg128 can be solved by
>> using target_words_bigendian() instead of TARGET_BIG_ENDIAN, which is
>> already what tswap primitives are doing.
> 
> We'll need to eventually remove target_words_bigendian(), so that'd just
> be postponing that.
> 

It seemed to me that one of the goal of current shared work on single 
binary is to be able to check those kind of things at runtime. So I 
don't see how we can plan to remove target_words_bigendian(), or an 
equivalent.

If you mean we'll replace this call with another name/api, I don't see 
it as a problem to use it for now. It's upstream and works.

Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Pierrick Bouvier]

On 3/21/25 04:46, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 3/19/25 11:22, Alex Bennée wrote:
>>> The aim of this work is to get rid of the endian aware helpers in
>>> gdbstub/helpers.h which due to their use of tswap() mean target
>>> gdbstubs need to be built multiple times. While this series doesn't
>>> actually build each stub once it introduces a new helper -
>>> gdb_get_register_value() which takes a MemOp which can describe the
>>> current endian state of the system. This will be a lot easier to
>>> dynamically feed from a helper function.
>>> The most complex example is PPC which has a helper called
>>> ppc_maybe_bswap_register() which was doing this.
>>> This is still an RFC so I'm interested in feedback:
>>>     - is the API sane
>>>     - can we avoid lots of (uint8_t *) casting?
>>
>> Even though the series has a good intent, the fact we make everything
>> "generic" makes that we lose all guarantees we could get by relying on
>> static typing, and that we had possibility of mistakes when passing
>> size (which happened in patch 4 if I'm correct). And explicit casting
>> comes as a *strong* warning about that.
>>
>> By patch 7, I was really feeling it's not a win vs explicit functions
>> per size.
>>
>> If the goal of the series is to get rid of endian aware helpers, well,
>> this can be fixed in the helpers themselves, without needing to
>> introduce a "generic" size helper. Maybe we are trying to solve two
>> different problems here?
> 
> It did seem natural that if you were defining a MemOp you would use all
> of it rather than only its endian definition. But you are right we could
> introduce the same helpers with a bool flag for endianess.
> 

Defining MemOp and passing is ok, but loosing static typing guarantees 
is wrong in my opinion. C is already on the weak side regarding typing, 
we don't need to "void*"ize things more instead of replacing the calls 
with correct variants.

> Maybe we should have fully formed mops and just assert in the helper:
> 
>    gdb_get_reg32(MemOp op, GByteArray *buf, uint32_t val) {
>        g_assert(op & MO_SIZE == MO_32);
>        gdb_get_register_value(op, buf, &val);
>    }
> 
> I was also trying to avoid over boilerplating the code.
>

Adding proper functions definition instead of macros, and eliminating 
ifdefs is not really boilerplate.
Adding casts to loosen type system is not a win versus that.

>>
>>>     - should we have a reverse helper for setting registers
>>> If this seems like the right approach I can have a go at more of the
>>> frontends later.
>>> There are a few other misc clean-ups I did on the way which might be
>>> worth cherry picking for 10.0 but I'll leave that up to maintainers.
>>> Alex.
>>> Alex Bennée (10):
>>>     include/gdbstub: fix include guard in commands.h
>>>     gdbstub: introduce target independent gdb register helper
>>>     target/arm: convert 32 bit gdbstub to new helper
>>>     target/arm: convert 64 bit gdbstub to new helper
>>>     target/ppc: expand comment on FP/VMX/VSX access functions
>>>     target/ppc: make ppc_maybe_bswap_register static
>>>     target/ppc: convert gdbstub to new helper (!hacky)
>>>     gdbstub: assert earlier in handle_read_all_regs
>>>     include/exec: fix assert in size_memop
>>>     target/microblaze: convert gdbstub to new helper
>>>    include/exec/memop.h        |   4 +-
>>>    include/gdbstub/commands.h  |   2 +-
>>>    include/gdbstub/registers.h |  30 ++++++
>>>    target/ppc/cpu.h            |   8 +-
>>>    gdbstub/gdbstub.c           |  24 ++++-
>>>    target/arm/gdbstub.c        |  57 +++++++----
>>>    target/arm/gdbstub64.c      |  53 ++++++----
>>>    target/microblaze/gdbstub.c |  44 ++++----
>>>    target/ppc/gdbstub.c        | 194 ++++++++++++++++++++----------------
>>>    9 files changed, 257 insertions(+), 159 deletions(-)
>>>    create mode 100644 include/gdbstub/registers.h
>>>
> 

Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Philippe Mathieu-Daudé]

On 21/3/25 18:31, Pierrick Bouvier wrote:
> On 3/21/25 04:46, Alex Bennée wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>
>>> On 3/19/25 11:22, Alex Bennée wrote:
>>>> The aim of this work is to get rid of the endian aware helpers in
>>>> gdbstub/helpers.h which due to their use of tswap() mean target
>>>> gdbstubs need to be built multiple times. While this series doesn't
>>>> actually build each stub once it introduces a new helper -
>>>> gdb_get_register_value() which takes a MemOp which can describe the
>>>> current endian state of the system. This will be a lot easier to
>>>> dynamically feed from a helper function.
>>>> The most complex example is PPC which has a helper called
>>>> ppc_maybe_bswap_register() which was doing this.
>>>> This is still an RFC so I'm interested in feedback:
>>>>     - is the API sane
>>>>     - can we avoid lots of (uint8_t *) casting?
>>>
>>> Even though the series has a good intent, the fact we make everything
>>> "generic" makes that we lose all guarantees we could get by relying on
>>> static typing, and that we had possibility of mistakes when passing
>>> size (which happened in patch 4 if I'm correct). And explicit casting
>>> comes as a *strong* warning about that.
>>>
>>> By patch 7, I was really feeling it's not a win vs explicit functions
>>> per size.
>>>
>>> If the goal of the series is to get rid of endian aware helpers, well,
>>> this can be fixed in the helpers themselves, without needing to
>>> introduce a "generic" size helper. Maybe we are trying to solve two
>>> different problems here?
>>
>> It did seem natural that if you were defining a MemOp you would use all
>> of it rather than only its endian definition. But you are right we could
>> introduce the same helpers with a bool flag for endianess.
>>
> 
> Defining MemOp and passing is ok, but loosing static typing guarantees 
> is wrong in my opinion. C is already on the weak side regarding typing, 
> we don't need to "void*"ize things more instead of replacing the calls 
> with correct variants.
> 
>> Maybe we should have fully formed mops and just assert in the helper:
>>
>>    gdb_get_reg32(MemOp op, GByteArray *buf, uint32_t val) {
>>        g_assert(op & MO_SIZE == MO_32);
>>        gdb_get_register_value(op, buf, &val);
>>    }
>>
>> I was also trying to avoid over boilerplating the code.
>>
> 
> Adding proper functions definition instead of macros, and eliminating 
> ifdefs is not really boilerplate.

In another thread Richard said for these cases we should use _Generic()
more.

> Adding casts to loosen type system is not a win versus that.

Agreed.

Re: [PATCH 00/10] gdbstub: conversion to runtime endianess helpers

[Pierrick Bouvier]

On 3/23/25 08:41, Philippe Mathieu-Daudé wrote:
> On 21/3/25 18:31, Pierrick Bouvier wrote:
>>
>> Adding proper functions definition instead of macros, and eliminating
>> ifdefs is not really boilerplate.
> 
> In another thread Richard said for these cases we should use _Generic()
> more.
> 

I was thinking about suggesting it when I first answered on this thread. 
However, I realized it doesn't really help us towards our end goal to 
unify variants of a compilation unit.

_Generic is just a way to have a compile time
switch(typeof(x)) {
   case type_a: ...
   case type_b: ...
}

This is, in some ways, what our existing macros do here, based on 
TARGET_LONG_BITS. Some C compilers targetting embedded systems support 
sizeof on integral types in the preprocessor to support this kind of 
case before _Generic existed.

It can replace macros based on static types, but it doesn't make much 
more towards eliminating target type dependency.

It could still be used here though, to avoid the need to patch all 
targets using gdb register macros, that already used fixed types.

But since our list of call sites is not so huge, I would be in favor to 
fix the code directly, instead of relying on _Generic. But I'm open to 
that if we collectively think it's a better way.

>> Adding casts to loosen type system is not a win versus that.
> 
> Agreed.