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

[PATCH v2 00/11] 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 but I've spun out v2 for further discussion.

In v2:

  - drop uint8_t casting and use void as C intended ;-)
  - add specific 32/64 bit helpers with type checking an assertion
  - various tweaks and fixes (see individual commits)

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 Bennée (11):
  include/exec: fix assert in size_memop
  include/gdbstub: fix include guard in commands.h
  gdbstub: assert earlier in handle_read_all_regs
  gdbstub: introduce target independent gdb register helper
  target/arm: convert 32 bit gdbstub to new helpers
  target/arm: convert 64 bit gdbstub to new helpers
  target/ppc: expand comment on FP/VMX/VSX access functions
  target/ppc: make ppc_maybe_bswap_register static
  target/ppc: convert gdbstub to new helpers
  target/microblaze: convert gdbstub to new helper
  include/gdbstub: add note to helpers.h

 include/exec/memop.h        |   4 +-
 include/gdbstub/commands.h  |   2 +-
 include/gdbstub/helpers.h   |   4 +-
 include/gdbstub/registers.h |  55 ++++++++++
 target/ppc/cpu.h            |   8 +-
 gdbstub/gdbstub.c           |  25 ++++-
 target/arm/gdbstub.c        |  55 ++++++----
 target/arm/gdbstub64.c      |  53 ++++++----
 target/microblaze/gdbstub.c |  49 ++++-----
 target/ppc/gdbstub.c        | 197 ++++++++++++++++++++----------------
 10 files changed, 292 insertions(+), 160 deletions(-)
 create mode 100644 include/gdbstub/registers.h

-- 
2.39.5

[PATCH v2 01/11] 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>

---
v2
  - instead of 128 use 1 << MO_SIZE for future proofing
---
 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..6afe50a7d0 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 <= (1 << MO_SIZE));
 #endif
     return (MemOp)ctz32(size);
 }
-- 
2.39.5

[PATCH v2 02/11] include/gdbstub: fix include guard in commands.h

[Alex Bennée]

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 v2 03/11] 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 282e13e163..b6d5e11e03 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1328,8 +1328,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 v2 04/11] 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.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use unsigned consistently
  - fix some rouge whitespace
  - add typed reg32/64 wrappers
  - pass void * to underlying helper to avoid casting
---
 include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
 gdbstub/gdbstub.c           | 23 ++++++++++++++++
 2 files changed, 78 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..2220f58efe
--- /dev/null
+++ b/include/gdbstub/registers.h
@@ -0,0 +1,55 @@
+/*
+ * 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.
+ *
+ * There are wrapper functions for the common sizes you can use to
+ * keep type checking.
+ *
+ * Returns the number of bytes written to the array.
+ */
+int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
+
+/**
+ * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
+    g_assert((op & MO_SIZE) == MO_32);
+    return gdb_get_register_value(op, buf, val);
+}
+
+/**
+ * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
+ * mo: size and endian MemOp
+ * buf: GByteArray to store in target order
+ * val: pointer to uint32_t value in host order
+ */
+static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
+    g_assert((op & MO_SIZE) == MO_64);
+    return gdb_get_register_value(op, buf, val);
+}
+
+#endif /* GDB_REGISTERS_H */
+
+
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
+{
+    unsigned bytes = memop_size(op);
+
+    if (op & MO_BSWAP) {
+        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
+        for (unsigned i = bytes; i > 0; i--) {
+            g_byte_array_append(buf, ptr--, 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 v2 05/11] target/arm: convert 32 bit gdbstub to new helpers

[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.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use new wrappers
  - explicit MO_32 usage and reg32/64 helpers
---
 target/arm/gdbstub.c | 55 +++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 30068c2262..71d672ace5 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_reg32_value(MO_TE | MO_32, mem_buf, &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_reg32_value(MO_TE | MO_32, mem_buf, &reg);
     }
     /* Unknown register.  */
     return 0;
@@ -115,19 +121,21 @@ 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_reg64_value(MO_TE | MO_64, buf, 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_reg64_value(MO_TE | MO_64, buf, 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_reg32_value(MO_TE | MO_32, buf, &fpcr);
     }
     return 0;
 }
@@ -166,9 +174,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_reg32_value(MO_TE | MO_32, buf,
+                                   &env->vfp.xregs[ARM_VFP_FPSID]);
     case 1:
-        return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]);
+        return gdb_get_reg32_value(MO_TE | MO_32, buf,
+                                   &env->vfp.xregs[ARM_VFP_FPEXC]);
     }
     return 0;
 }
@@ -196,7 +206,7 @@ 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_reg32_value(MO_TE | MO_32, buf, &env->v7m.vpr);
     default:
         return 0;
     }
@@ -236,9 +246,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 +387,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 +405,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 v2 06/11] target/arm: convert 64 bit gdbstub to new helpers

[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>

---
v2
  - use MO32/MO64 varients for reg sizes
  - use wrappers for 32/64 bit regs
  - do SVE Z registers in 128bit chunks
---
 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..6ad10368e8 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)
@@ -32,18 +32,21 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
+    MemOp mop = MO_TE; /* TE = LE for registers despite SCTLR.EE/E0E */
+    uint32_t pstate;
 
     if (n < 31) {
         /* Core integer register.  */
-        return gdb_get_reg64(mem_buf, env->xregs[n]);
+        return gdb_get_reg64_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_reg64_value(mop | MO_64, mem_buf, &env->xregs[31]);
     case 32:
-        return gdb_get_reg64(mem_buf, env->pc);
+        return gdb_get_reg64_value(mop | MO_64, mem_buf, &env->pc);
     case 33:
-        return gdb_get_reg32(mem_buf, pstate_read(env));
+        pstate = pstate_read(env);
+        return gdb_get_reg32_value(mop | MO_32, mem_buf, &pstate);
     }
     /* Unknown register.  */
     return 0;
@@ -82,23 +85,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_TE | MO_128, buf, 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_reg32_value(MO_TE | MO_32, buf, &fpr);
 }
 
 int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg)
@@ -132,30 +139,35 @@ int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
+    MemOp mop = MO_TE; /* TE = LE for registers despite SCTLR.EE/E0E */
+    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(mop | MO_128, buf, &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_reg32_value(mop | MO_32, buf, &fpr);
     case 33:
-        return gdb_get_reg32(buf, vfp_get_fpcr(env));
+        fpr = vfp_get_fpcr(env);
+        return gdb_get_reg32_value(mop | MO_32, buf, &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_reg64_value(mop | MO_64, buf,
+                                       &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_reg64_value(mop | MO_64, buf, &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_reg64_value(MO_TE | MO_64, buf, &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_reg64_value(MO_TE | MO_64, buf, &tcf0);
 }
 
 int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)
-- 
2.39.5

[PATCH v2 07/11] 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.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
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 v2 08/11] target/ppc: make ppc_maybe_bswap_register static

[Alex Bennée]

It's not used outside of the gdbstub code.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 v2 09/11] target/ppc: convert gdbstub to new helpers

[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.

We also have to deal with heavy usage of target_ulong so we copy some
macro stuff from the old helpers.h and add a gdb_get_regl_value()
helper.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use new helpers
  - fix bunch of target_ulong cases
---
 target/ppc/gdbstub.c | 195 ++++++++++++++++++++++++-------------------
 1 file changed, 111 insertions(+), 84 deletions(-)

diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index c09e93abaf..b96c3ac5b8 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,41 @@ 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 handling target_ulong values
+ * from gdbstub's GByteArray based on what the build config is. This
+ * will need fixing for single-binary.
+ */
+
+#if TARGET_LONG_BITS == 64
+#define ldtul_p(addr) ldq_p(addr)
+#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v)
+#else
+#define ldtul_p(addr) ldl_p(addr)
+#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v)
+#endif
+
 /*
  * Old gdb always expects FP registers.  Newer (xml-aware) gdb only
  * expects whatever the target description contains.  Due to a
@@ -109,51 +144,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_regl_value(mo, buf, &env->gpr[n]);
     } else {
         switch (n) {
         case 64:
-            gdb_get_regl(buf, env->nip);
-            break;
+            return gdb_get_regl_value(mo, buf, &env->nip);
         case 65:
-            gdb_get_regl(buf, env->msr);
-            break;
+            return gdb_get_regl_value(mo, buf, &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, &cr);
             }
         case 67:
-            gdb_get_regl(buf, env->lr);
+            return gdb_get_regl_value(mo, buf, &env->lr);
             break;
         case 68:
-            gdb_get_regl(buf, env->ctr);
+            return gdb_get_regl_value(mo, buf, &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, &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 +195,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_regl_value(mo, buf, &env->gpr[n]);
     } else if (n < 64) {
         /* fprs */
-        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
+        actual = gdb_get_reg64_value(mo, buf, 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_reg64_value(mo, buf, &empty);
+        actual = gdb_get_reg64_value(mo, buf, &empty);
     } else {
         switch (n) {
         case 64 + 32:
-            gdb_get_reg64(buf, env->nip);
+            actual = gdb_get_regl_value(mo, buf, &env->nip);
             break;
         case 65 + 32:
-            gdb_get_reg64(buf, env->msr);
+            actual = gdb_get_regl_value(mo, buf, &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_reg32_value(mo, buf, &cr);
+            break;
+        }
         case 67 + 32:
-            gdb_get_reg64(buf, env->lr);
+            actual = gdb_get_regl_value(mo, buf, &env->lr);
             break;
         case 68 + 32:
-            gdb_get_reg64(buf, env->ctr);
+            actual = gdb_get_regl_value(mo, buf, &env->ctr);
             break;
         case 69 + 32:
-            gdb_get_reg32(buf, cpu_read_xer(env));
+        {
+            uint32_t xer = cpu_read_xer(env);
+            actual = gdb_get_reg32_value(mo, buf, &xer);
             break;
+        }
         case 70 + 32:
-            gdb_get_reg64(buf, env->fpscr);
+            actual = gdb_get_regl_value(mo, buf, &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 +248,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 +408,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 +439,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_regl_value(mo, buf, &val);
 }
 
 static int gdb_set_spr_reg(CPUState *cs, uint8_t *mem_buf, int n)
@@ -441,18 +477,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_reg64_value(mo, buf, 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, TARGET_LONG_SIZE);
+        return gdb_get_regl_value(mo, buf, &env->fpscr);
     }
     return 0;
 }
@@ -479,26 +511,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, 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_reg32_value(mo, buf, &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, TARGET_LONG_SIZE);
+        return gdb_get_regl_value(mo, buf, &env->spr[SPR_VRSAVE]);
     }
     return 0;
 }
@@ -532,25 +559,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_reg32_value(mo, buf, &low);
 #else
-        gdb_get_reg32(buf, env->gprh[n]);
+        mo = ppc_gdb_memop(env, 4);
+        return gdb_get_reg32_value(mo, buf, &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_reg64_value(mo, buf, &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_reg32_value(mo, buf, &env->spe_fscr);
     }
     return 0;
 }
@@ -593,9 +620,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_reg64_value(ppc_gdb_memop(env, 8),
+                                   buf,
+                                   cpu_vsrl_ptr(env, n));
     }
     return 0;
 }
-- 
2.39.5

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

[Alex Bennée]

This is a pretty simple conversion given a single set of registers and
an existing helper to probe endianess.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use mb_cpu_is_big_endian
  - use explicit MO_32 size
  - handle differing size of env->ear between user/system
---
 target/microblaze/gdbstub.c | 49 +++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
index d493681d38..dbaf7ecb9c 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,57 @@ 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 = mb_cpu_is_big_endian(cs) ? MO_BE : MO_LE;
+    uint32_t msr;
 
     switch (n) {
     case 1 ... 31:
-        val = env->regs[n];
-        break;
+        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->regs[n]);
     case GDB_PC:
-        val = env->pc;
-        break;
+        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->pc);
     case GDB_MSR:
-        val = mb_cpu_read_msr(env);
-        break;
+        msr = mb_cpu_read_msr(env);
+        return gdb_get_reg32_value(mo | MO_32, mem_buf, &msr);
     case GDB_EAR:
-        val = env->ear;
-        break;
+#if TARGET_LONG_BITS == 64
+        return gdb_get_reg64_value(mo | MO_64, mem_buf, &env->ear);
+#else
+        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->ear);
+#endif
     case GDB_ESR:
-        val = env->esr;
-        break;
+        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->esr);
     case GDB_FSR:
-        val = env->fsr;
-        break;
+        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->fsr);
     case GDB_BTR:
-        val = env->btr;
-        break;
+        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->btr);
     case GDB_PVR0 ... GDB_PVR11:
         /* PVR12 is intentionally skipped */
-        val = cpu->cfg.pvr_regs[n - GDB_PVR0];
-        break;
+        return gdb_get_reg32_value(mo | MO_32, mem_buf,
+                                      &cpu->cfg.pvr_regs[n - GDB_PVR0]);
     case GDB_EDR:
-        val = env->edr;
-        break;
+        return gdb_get_reg32_value(mo | MO_32, mem_buf, &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_reg32_value(mo, mem_buf, &env->slr);
         break;
     case GDB_SP_SHR:
-        val = env->shr;
+        return gdb_get_reg32_value(mo, mem_buf, &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

[PATCH v2 11/11] include/gdbstub: add note to helpers.h

[Alex Bennée]

We've not yet deprecated but we should steer users away from these
helpers if they want to be in a single/heterogeneous binary.

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

diff --git a/include/gdbstub/helpers.h b/include/gdbstub/helpers.h
index 6f7cc48adc..9b3a535b03 100644
--- a/include/gdbstub/helpers.h
+++ b/include/gdbstub/helpers.h
@@ -2,7 +2,9 @@
  * gdbstub helpers
  *
  * These are all used by the various frontends and have to be host
- * aware to ensure things are store in target order.
+ * aware to ensure things are store in target order. Consider using
+ * the endian neutral registers.h if you want the architecture to be
+ * included in an eventual single QEMU binary.
  *
  * Copyright (c) 2022 Linaro Ltd
  *
-- 
2.39.5

Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> 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.
>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - use unsigned consistently
>   - fix some rouge whitespace
>   - add typed reg32/64 wrappers
>   - pass void * to underlying helper to avoid casting
> ---
>  include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>  gdbstub/gdbstub.c           | 23 ++++++++++++++++
>  2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 1);
> +        };

I forgot to fix this up. Hopefully the following seems a little less
like pointer abuse:

/*
 * 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, void *val)
{
    unsigned bytes = memop_size(op);
    uint8_t *data = val;

    if (op & MO_BSWAP) {
        uint8_t *ptr = &data[bytes - 1];
        do {
            g_byte_array_append(buf, ptr--, 1);
        } while (ptr >= data);
    } else {
        g_byte_array_append(buf, val, bytes);
    }

    return bytes;
}


> +    } 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)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 01/11] include/exec: fix assert in size_memop

[Philippe Mathieu-Daudé]

On 24/3/25 11:21, 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>
> 
> ---
> v2
>    - instead of 128 use 1 << MO_SIZE for future proofing
> ---
>   include/exec/memop.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Philippe Mathieu-Daudé]

On 24/3/25 11:21, Alex Bennée wrote:
> This is a pretty simple conversion given a single set of registers and
> an existing helper to probe endianess.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use mb_cpu_is_big_endian
>    - use explicit MO_32 size
>    - handle differing size of env->ear between user/system
> ---
>   target/microblaze/gdbstub.c | 49 +++++++++++++++++--------------------
>   1 file changed, 22 insertions(+), 27 deletions(-)


> @@ -50,62 +50,57 @@ 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 = mb_cpu_is_big_endian(cs) ? MO_BE : MO_LE;
> +    uint32_t msr;
>   
>       switch (n) {
>       case 1 ... 31:
> -        val = env->regs[n];
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->regs[n]);
>       case GDB_PC:
> -        val = env->pc;
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->pc);
>       case GDB_MSR:
> -        val = mb_cpu_read_msr(env);
> -        break;
> +        msr = mb_cpu_read_msr(env);
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &msr);
>       case GDB_EAR:
> -        val = env->ear;
> -        break;
> +#if TARGET_LONG_BITS == 64

Not necessary if basing on top of:
https://lore.kernel.org/qemu-devel/20250212220155.1147144-5-richard.henderson@linaro.org/

> +        return gdb_get_reg64_value(mo | MO_64, mem_buf, &env->ear);
> +#else
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->ear);
> +#endif

Re: [PATCH v2 11/11] include/gdbstub: add note to helpers.h

[Philippe Mathieu-Daudé]

On 24/3/25 11:21, Alex Bennée wrote:
> We've not yet deprecated but we should steer users away from these
> helpers if they want to be in a single/heterogeneous binary.

Why not deprecate?

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/gdbstub/helpers.h | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/gdbstub/helpers.h b/include/gdbstub/helpers.h
> index 6f7cc48adc..9b3a535b03 100644
> --- a/include/gdbstub/helpers.h
> +++ b/include/gdbstub/helpers.h
> @@ -2,7 +2,9 @@
>    * gdbstub helpers
>    *
>    * These are all used by the various frontends and have to be host
> - * aware to ensure things are store in target order.
> + * aware to ensure things are store in target order. Consider using
> + * the endian neutral registers.h if you want the architecture to be
> + * included in an eventual single QEMU binary.
>    *
>    * Copyright (c) 2022 Linaro Ltd
>    *

Re: [PATCH v2 01/11] include/exec: fix assert in size_memop

[Richard Henderson]

On 3/24/25 03:21, 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>
> 
> ---
> v2
>    - instead of 128 use 1 << MO_SIZE for future proofing
> ---
>   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..6afe50a7d0 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 <= (1 << MO_SIZE));

1 << MO_SIZE is 1024, not 128.

So the patch is correct, but the comment above is not.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 11/11] include/gdbstub: add note to helpers.h

[Alex Bennée]

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

> On 24/3/25 11:21, Alex Bennée wrote:
>> We've not yet deprecated but we should steer users away from these
>> helpers if they want to be in a single/heterogeneous binary.
>
> Why not deprecate?

I guess philosophically do we expect to eventually convert all frontends
to the new API or only those that want to be in the single binary?
Should I just be more explicit:

>>    *
>>    * These are all used by the various frontends and have to be host
>> - * aware to ensure things are store in target order.
>> + * aware to ensure things are store in target order. Consider using
>> + * the endian neutral registers.h if you want the architecture to be
>> + * included in an eventual single QEMU binary.
>>    *
>>    * Copyright (c) 2022 Linaro Ltd
>>    *


 These are all used by the various frontends and have to be host aware
 to ensure things are store in target order.

 New front-ends should not be using these APIs at all. They should be
 using the endian neutral registers.h as should any architecture that
 intends to be included in an eventual single QEMU binary.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper

[Richard Henderson]

On 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}

Passing the value by address is crazy.

> @@ -551,6 +553,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 1);
> +        };

I think you're still wed to this supposedly generic way to handle values, and this method 
of bswap is really silly.

Better, I think is

int gdb_get_reg32_value(MemOp endian, GByteArray *buf, uint32_t val)
{
     /* We only expect MO_BE or MO_LE, one of which is 0 depending on host. */
     if (endian) {
         assert(endian == MO_BSWAP);
         val = bswap32(val);
     }

     g_byte_array_append(buf, &val, sizeof(val));
     return sizeof(val);
}


I'll also mention that the return value is now mostly useless: I think you should just 
rely on buf->len.  This is something that we could have cleaned up when converting from 
uint8_t *buf to GByteArray in the first place.  But changing the interface of all of the 
gdb_read_register hooks is a larger job.


r~

Re: [PATCH v2 08/11] target/ppc: make ppc_maybe_bswap_register static

[Richard Henderson]

On 3/24/25 03:21, Alex Bennée wrote:
> It's not used outside of the gdbstub code.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 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: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 07/11] target/ppc: expand comment on FP/VMX/VSX access functions

[Richard Henderson]

On 3/24/25 03:21, Alex Bennée wrote:
> Mainly as an aid to myself getting confused too many bswaps deep into
> the code.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/ppc/cpu.h | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 09/11] target/ppc: convert gdbstub to new helpers

[Richard Henderson]

On 3/24/25 03:21, Alex Bennée wrote:
> +    #ifdef TARGET_BIG_ENDIAN
> +    MemOp end = MO_BE;
> +    #else
> +    MemOp end = MO_LE;
> +    #endif
> +#endif

That's what MO_TE is for.

> +/*
> + * Helpers copied from helpers.h just for handling target_ulong values
> + * from gdbstub's GByteArray based on what the build config is. This
> + * will need fixing for single-binary.
> + */
> +
> +#if TARGET_LONG_BITS == 64
> +#define ldtul_p(addr) ldq_p(addr)
> +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v)
> +#else
> +#define ldtul_p(addr) ldl_p(addr)
> +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v)
> +#endif

Surely you're not intending to replicate this in any target that can have multiple sizes? 
This is not an improvement.


r~

Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper

[Pierrick Bouvier]

On 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +

Could it be made static, so it's hidden from the public interface?
You'll need to un-inline gdb_get_reg*_value functions. I doubt it's 
performance critical and need to be inline.

> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 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 v2 01/11] include/exec: fix assert in size_memop

[Pierrick Bouvier]

On 3/24/25 03:21, 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>
> 
> ---
> v2
>    - instead of 128 use 1 << MO_SIZE for future proofing
> ---
>   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..6afe50a7d0 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 <= (1 << MO_SIZE));
>   #endif
>       return (MemOp)ctz32(size);
>   }

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

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

[Pierrick Bouvier]

On 3/24/25 03:21, 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 but I've spun out v2 for further discussion.
> 
> In v2:
> 
>    - drop uint8_t casting and use void as C intended ;-)

C allows that permissively, and it can be convenient when underlying 
type is really unknown. But in our case, it's only two variants.

If really it hurts to replace callers, _Generic is at least a better 
solution than invoking the void* hammer.

>    - add specific 32/64 bit helpers with type checking an assertion
>    - various tweaks and fixes (see individual commits)
> 
> 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 Bennée (11):
>    include/exec: fix assert in size_memop
>    include/gdbstub: fix include guard in commands.h
>    gdbstub: assert earlier in handle_read_all_regs
>    gdbstub: introduce target independent gdb register helper
>    target/arm: convert 32 bit gdbstub to new helpers
>    target/arm: convert 64 bit gdbstub to new helpers
>    target/ppc: expand comment on FP/VMX/VSX access functions
>    target/ppc: make ppc_maybe_bswap_register static
>    target/ppc: convert gdbstub to new helpers
>    target/microblaze: convert gdbstub to new helper
>    include/gdbstub: add note to helpers.h
> 
>   include/exec/memop.h        |   4 +-
>   include/gdbstub/commands.h  |   2 +-
>   include/gdbstub/helpers.h   |   4 +-
>   include/gdbstub/registers.h |  55 ++++++++++
>   target/ppc/cpu.h            |   8 +-
>   gdbstub/gdbstub.c           |  25 ++++-
>   target/arm/gdbstub.c        |  55 ++++++----
>   target/arm/gdbstub64.c      |  53 ++++++----
>   target/microblaze/gdbstub.c |  49 ++++-----
>   target/ppc/gdbstub.c        | 197 ++++++++++++++++++++----------------
>   10 files changed, 292 insertions(+), 160 deletions(-)
>   create mode 100644 include/gdbstub/registers.h
> 

Re: [PATCH v2 02/11] include/gdbstub: fix include guard in commands.h

[Pierrick Bouvier]

On 3/24/25 03:21, Alex Bennée wrote:
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 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 v2 03/11] gdbstub: assert earlier in handle_read_all_regs

[Pierrick Bouvier]

On 3/24/25 03:21, 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.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 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 282e13e163..b6d5e11e03 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1328,8 +1328,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();

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

Re: [PATCH v2 05/11] target/arm: convert 32 bit gdbstub to new helpers

[Pierrick Bouvier]

On 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use new wrappers
>    - explicit MO_32 usage and reg32/64 helpers
> ---
>   target/arm/gdbstub.c | 55 +++++++++++++++++++++++++++-----------------
>   1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 30068c2262..71d672ace5 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_reg32_value(MO_TE | MO_32, mem_buf, &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_reg32_value(MO_TE | MO_32, mem_buf, &reg);
>       }
>       /* Unknown register.  */
>       return 0;
> @@ -115,19 +121,21 @@ 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_reg64_value(MO_TE | MO_64, buf, 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_reg64_value(MO_TE | MO_64, buf, q);
>           }

What about upper part of register? s/128/64 seems unintended, and buf 
will only contain lower part.
Probably needed to introduce gdb_get_reg128_value helper as well.

>       }
>       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_reg32_value(MO_TE | MO_32, buf, &fpcr);
>       }
>       return 0;
>   }
> @@ -166,9 +174,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_reg32_value(MO_TE | MO_32, buf,
> +                                   &env->vfp.xregs[ARM_VFP_FPSID]);
>       case 1:
> -        return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]);
> +        return gdb_get_reg32_value(MO_TE | MO_32, buf,
> +                                   &env->vfp.xregs[ARM_VFP_FPEXC]);
>       }
>       return 0;
>   }
> @@ -196,7 +206,7 @@ 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_reg32_value(MO_TE | MO_32, buf, &env->v7m.vpr);
>       default:
>           return 0;
>       }
> @@ -236,9 +246,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 +387,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 +405,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 v2 06/11] target/arm: convert 64 bit gdbstub to new helpers

[Pierrick Bouvier]

On 3/24/25 03:21, 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>
> 
> ---
> v2
>    - use MO32/MO64 varients for reg sizes
>    - use wrappers for 32/64 bit regs
>    - do SVE Z registers in 128bit chunks
> ---
>   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..6ad10368e8 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)
> @@ -32,18 +32,21 @@ int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> +    MemOp mop = MO_TE; /* TE = LE for registers despite SCTLR.EE/E0E */
> +    uint32_t pstate;
>   
>       if (n < 31) {
>           /* Core integer register.  */
> -        return gdb_get_reg64(mem_buf, env->xregs[n]);
> +        return gdb_get_reg64_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_reg64_value(mop | MO_64, mem_buf, &env->xregs[31]);
>       case 32:
> -        return gdb_get_reg64(mem_buf, env->pc);
> +        return gdb_get_reg64_value(mop | MO_64, mem_buf, &env->pc);
>       case 33:
> -        return gdb_get_reg32(mem_buf, pstate_read(env));
> +        pstate = pstate_read(env);
> +        return gdb_get_reg32_value(mop | MO_32, mem_buf, &pstate);
>       }
>       /* Unknown register.  */
>       return 0;
> @@ -82,23 +85,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_TE | MO_128, buf, q);
>       }

Yes, please add a helper for 128 as well, and having an assert on size.

>       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_reg32_value(MO_TE | MO_32, buf, &fpr);
>   }
>   
>   int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg)
> @@ -132,30 +139,35 @@ int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg)
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> +    MemOp mop = MO_TE; /* TE = LE for registers despite SCTLR.EE/E0E */
> +    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(mop | MO_128, buf, &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_reg32_value(mop | MO_32, buf, &fpr);
>       case 33:
> -        return gdb_get_reg32(buf, vfp_get_fpcr(env));
> +        fpr = vfp_get_fpcr(env);
> +        return gdb_get_reg32_value(mop | MO_32, buf, &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_reg64_value(mop | MO_64, buf,
> +                                       &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_reg64_value(mop | MO_64, buf, &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_reg64_value(MO_TE | MO_64, buf, &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_reg64_value(MO_TE | MO_64, buf, &tcf0);
>   }
>   
>   int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)

Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper

[Pierrick Bouvier]

On 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +

Maybe we could simply expect endian information from MemOp, and add 
required size here.
It clutters call sites to have gdb_get_regX_value(MO_X | ...).
We can eventually assert no size was set, so it's not added by mistake 
from callers.

> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 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 v2 07/11] target/ppc: expand comment on FP/VMX/VSX access functions

[Pierrick Bouvier]

On 3/24/25 03:21, Alex Bennée wrote:
> Mainly as an aid to myself getting confused too many bswaps deep into
> the code.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 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 v2 08/11] target/ppc: make ppc_maybe_bswap_register static

[Pierrick Bouvier]

On 3/24/25 03:21, Alex Bennée wrote:
> It's not used outside of the gdbstub code.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 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 v2 09/11] target/ppc: convert gdbstub to new helpers

[Pierrick Bouvier]

On 3/24/25 10:39, Richard Henderson wrote:
> On 3/24/25 03:21, Alex Bennée wrote:
>> +    #ifdef TARGET_BIG_ENDIAN
>> +    MemOp end = MO_BE;
>> +    #else
>> +    MemOp end = MO_LE;
>> +    #endif
>> +#endif
> 
> That's what MO_TE is for.
> 
>> +/*
>> + * Helpers copied from helpers.h just for handling target_ulong values
>> + * from gdbstub's GByteArray based on what the build config is. This
>> + * will need fixing for single-binary.
>> + */
>> +
>> +#if TARGET_LONG_BITS == 64
>> +#define ldtul_p(addr) ldq_p(addr)
>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v)
>> +#else
>> +#define ldtul_p(addr) ldl_p(addr)
>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v)
>> +#endif
> 
> Surely you're not intending to replicate this in any target that can have multiple sizes?
> This is not an improvement.
> 

If I'm correct (and from v1), ppc is the only architecture having 
registers defined with target_long types.

So at the time I suggested to either move the macro definition in a 
header (helpers-target.h) and deal with ppc later, or replace ppc 
registers definition, which is surely more complicated to do at the moment.

Moving macro definition directly in this file (if it's really the only 
one needing it) is ok for me as well.

> 
> r~

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

[Pierrick Bouvier]

On 3/24/25 03:21, Alex Bennée wrote:
> This is a pretty simple conversion given a single set of registers and
> an existing helper to probe endianess.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use mb_cpu_is_big_endian
>    - use explicit MO_32 size
>    - handle differing size of env->ear between user/system
> ---
>   target/microblaze/gdbstub.c | 49 +++++++++++++++++--------------------
>   1 file changed, 22 insertions(+), 27 deletions(-)
> 
> diff --git a/target/microblaze/gdbstub.c b/target/microblaze/gdbstub.c
> index d493681d38..dbaf7ecb9c 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,57 @@ 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 = mb_cpu_is_big_endian(cs) ? MO_BE : MO_LE;
> +    uint32_t msr;
>   
>       switch (n) {
>       case 1 ... 31:
> -        val = env->regs[n];
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->regs[n]);
>       case GDB_PC:
> -        val = env->pc;
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->pc);
>       case GDB_MSR:
> -        val = mb_cpu_read_msr(env);
> -        break;
> +        msr = mb_cpu_read_msr(env);
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &msr);
>       case GDB_EAR:
> -        val = env->ear;
> -        break;
> +#if TARGET_LONG_BITS == 64
> +        return gdb_get_reg64_value(mo | MO_64, mem_buf, &env->ear);
> +#else
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->ear);
> +#endif
>       case GDB_ESR:
> -        val = env->esr;
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->esr);
>       case GDB_FSR:
> -        val = env->fsr;
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->fsr);
>       case GDB_BTR:
> -        val = env->btr;
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &env->btr);
>       case GDB_PVR0 ... GDB_PVR11:
>           /* PVR12 is intentionally skipped */
> -        val = cpu->cfg.pvr_regs[n - GDB_PVR0];
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf,
> +                                      &cpu->cfg.pvr_regs[n - GDB_PVR0]);
>       case GDB_EDR:
> -        val = env->edr;
> -        break;
> +        return gdb_get_reg32_value(mo | MO_32, mem_buf, &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_reg32_value(mo, mem_buf, &env->slr);
>           break;
>       case GDB_SP_SHR:
> -        val = env->shr;
> +        return gdb_get_reg32_value(mo, mem_buf, &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)

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

Re: [PATCH v2 11/11] include/gdbstub: add note to helpers.h

[Pierrick Bouvier]

On 3/24/25 10:29, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 24/3/25 11:21, Alex Bennée wrote:
>>> We've not yet deprecated but we should steer users away from these
>>> helpers if they want to be in a single/heterogeneous binary.
>>
>> Why not deprecate?
> 
> I guess philosophically do we expect to eventually convert all frontends
> to the new API or only those that want to be in the single binary?
> Should I just be more explicit:
> 
>>>     *
>>>     * These are all used by the various frontends and have to be host
>>> - * aware to ensure things are store in target order.
>>> + * aware to ensure things are store in target order. Consider using
>>> + * the endian neutral registers.h if you want the architecture to be
>>> + * included in an eventual single QEMU binary.
>>>     *
>>>     * Copyright (c) 2022 Linaro Ltd
>>>     *
> 
> 
>   These are all used by the various frontends and have to be host aware
>   to ensure things are store in target order.
> 

If the fix is an easy "sed like" with static typing guarantee (so no bug 
can be introduced), we can probably just do it on all existing targets, 
and remove this.

If it's hard or error prone, then I would say it's ok to use a "one 
target at a time" approach.

>   New front-ends should not be using these APIs at all. They should be
>   using the endian neutral registers.h as should any architecture that
>   intends to be included in an eventual single QEMU binary.
> 

Re: [PATCH v2 11/11] include/gdbstub: add note to helpers.h

[Pierrick Bouvier]

On 3/24/25 10:29, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 24/3/25 11:21, Alex Bennée wrote:
>>> We've not yet deprecated but we should steer users away from these
>>> helpers if they want to be in a single/heterogeneous binary.
>>
>> Why not deprecate?
> 
> I guess philosophically do we expect to eventually convert all frontends
> to the new API or only those that want to be in the single binary?
> Should I just be more explicit:
> 
>>>     *
>>>     * These are all used by the various frontends and have to be host
>>> - * aware to ensure things are store in target order.
>>> + * aware to ensure things are store in target order. Consider using
>>> + * the endian neutral registers.h if you want the architecture to be
>>> + * included in an eventual single QEMU binary.
>>>     *
>>>     * Copyright (c) 2022 Linaro Ltd
>>>     *
> 
> 
>   These are all used by the various frontends and have to be host aware
>   to ensure things are store in target order.
> 

If the fix is an easy "sed like" with static typing guarantee (so no bug 
can be introduced), we can probably just do it on all existing targets, 
and remove this.

If it's hard or error prone, then I would say it's ok to use a "one 
target at a time" approach.

>   New front-ends should not be using these APIs at all. They should be
>   using the endian neutral registers.h as should any architecture that
>   intends to be included in an eventual single QEMU binary.
> 

Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper

[Pierrick Bouvier]

On 3/24/25 03:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +

After reading the whole series, I think I am missing the point of 
introduce op here.
If what we really want is just the target endianness, why not add the 
"if target_words_bigendian()" in the wrapper here?

Are there cases where we want another endianness than target one?

> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 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 v2 09/11] target/ppc: convert gdbstub to new helpers

[Richard Henderson]

On 3/24/25 12:29, Pierrick Bouvier wrote:
> On 3/24/25 10:39, Richard Henderson wrote:
>> On 3/24/25 03:21, Alex Bennée wrote:
>>> +    #ifdef TARGET_BIG_ENDIAN
>>> +    MemOp end = MO_BE;
>>> +    #else
>>> +    MemOp end = MO_LE;
>>> +    #endif
>>> +#endif
>>
>> That's what MO_TE is for.
>>
>>> +/*
>>> + * Helpers copied from helpers.h just for handling target_ulong values
>>> + * from gdbstub's GByteArray based on what the build config is. This
>>> + * will need fixing for single-binary.
>>> + */
>>> +
>>> +#if TARGET_LONG_BITS == 64
>>> +#define ldtul_p(addr) ldq_p(addr)
>>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v)
>>> +#else
>>> +#define ldtul_p(addr) ldl_p(addr)
>>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v)
>>> +#endif
>>
>> Surely you're not intending to replicate this in any target that can have multiple sizes?
>> This is not an improvement.
>>
> 
> If I'm correct (and from v1), ppc is the only architecture having registers defined with 
> target_long types.

With a quick check, mips, riscv, sparc do as well.


r~

Re: [PATCH v2 09/11] target/ppc: convert gdbstub to new helpers

[Pierrick Bouvier]

On 3/24/25 13:04, Richard Henderson wrote:
> On 3/24/25 12:29, Pierrick Bouvier wrote:
>> On 3/24/25 10:39, Richard Henderson wrote:
>>> On 3/24/25 03:21, Alex Bennée wrote:
>>>> +    #ifdef TARGET_BIG_ENDIAN
>>>> +    MemOp end = MO_BE;
>>>> +    #else
>>>> +    MemOp end = MO_LE;
>>>> +    #endif
>>>> +#endif
>>>
>>> That's what MO_TE is for.
>>>
>>>> +/*
>>>> + * Helpers copied from helpers.h just for handling target_ulong values
>>>> + * from gdbstub's GByteArray based on what the build config is. This
>>>> + * will need fixing for single-binary.
>>>> + */
>>>> +
>>>> +#if TARGET_LONG_BITS == 64
>>>> +#define ldtul_p(addr) ldq_p(addr)
>>>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v)
>>>> +#else
>>>> +#define ldtul_p(addr) ldl_p(addr)
>>>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v)
>>>> +#endif
>>>
>>> Surely you're not intending to replicate this in any target that can have multiple sizes?
>>> This is not an improvement.
>>>
>>
>> If I'm correct (and from v1), ppc is the only architecture having registers defined with
>> target_long types.
> 
> With a quick check, mips, riscv, sparc do as well.
>

Right, I should have run the simple grep :)
So it's better to keep those macros defined in a separate header (out of 
helpers.h, like helpers-target.h), so we can already start to cleanup 
some targets, and do the rest of the work for the ones using 
target_ulong type for later.

> 
> r~

Re: [PATCH v2 09/11] target/ppc: convert gdbstub to new helpers

[Philippe Mathieu-Daudé]

On 24/3/25 21:49, Pierrick Bouvier wrote:
> On 3/24/25 13:04, Richard Henderson wrote:
>> On 3/24/25 12:29, Pierrick Bouvier wrote:
>>> On 3/24/25 10:39, Richard Henderson wrote:
>>>> On 3/24/25 03:21, Alex Bennée wrote:
>>>>> +    #ifdef TARGET_BIG_ENDIAN
>>>>> +    MemOp end = MO_BE;
>>>>> +    #else
>>>>> +    MemOp end = MO_LE;
>>>>> +    #endif
>>>>> +#endif
>>>>
>>>> That's what MO_TE is for.
>>>>
>>>>> +/*
>>>>> + * Helpers copied from helpers.h just for handling target_ulong 
>>>>> values
>>>>> + * from gdbstub's GByteArray based on what the build config is. This
>>>>> + * will need fixing for single-binary.
>>>>> + */
>>>>> +
>>>>> +#if TARGET_LONG_BITS == 64
>>>>> +#define ldtul_p(addr) ldq_p(addr)
>>>>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v)
>>>>> +#else
>>>>> +#define ldtul_p(addr) ldl_p(addr)
>>>>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v)
>>>>> +#endif
>>>>
>>>> Surely you're not intending to replicate this in any target that can 
>>>> have multiple sizes?
>>>> This is not an improvement.
>>>>
>>>
>>> If I'm correct (and from v1), ppc is the only architecture having 
>>> registers defined with
>>> target_long types.
>>
>> With a quick check, mips, riscv, sparc do as well.
>>
> 
> Right, I should have run the simple grep :)
> So it's better to keep those macros defined in a separate header (out of 
> helpers.h, like helpers-target.h), so we can already start to cleanup 
> some targets, and do the rest of the work for the ones using 
> target_ulong type for later.

See also
https://lore.kernel.org/qemu-devel/f9131f6e-e843-48be-b85f-c341ec198154@linaro.org/
considering s/TARGET_LONG_SIZE/target_info_long_size()/

Re: [PATCH v2 09/11] target/ppc: convert gdbstub to new helpers

[Pierrick Bouvier]

On 3/25/25 02:22, Philippe Mathieu-Daudé wrote:
> On 24/3/25 21:49, Pierrick Bouvier wrote:
>> On 3/24/25 13:04, Richard Henderson wrote:
>>> On 3/24/25 12:29, Pierrick Bouvier wrote:
>>>> On 3/24/25 10:39, Richard Henderson wrote:
>>>>> On 3/24/25 03:21, Alex Bennée wrote:
>>>>>> +    #ifdef TARGET_BIG_ENDIAN
>>>>>> +    MemOp end = MO_BE;
>>>>>> +    #else
>>>>>> +    MemOp end = MO_LE;
>>>>>> +    #endif
>>>>>> +#endif
>>>>>
>>>>> That's what MO_TE is for.
>>>>>
>>>>>> +/*
>>>>>> + * Helpers copied from helpers.h just for handling target_ulong
>>>>>> values
>>>>>> + * from gdbstub's GByteArray based on what the build config is. This
>>>>>> + * will need fixing for single-binary.
>>>>>> + */
>>>>>> +
>>>>>> +#if TARGET_LONG_BITS == 64
>>>>>> +#define ldtul_p(addr) ldq_p(addr)
>>>>>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg64_value(m, b, v)
>>>>>> +#else
>>>>>> +#define ldtul_p(addr) ldl_p(addr)
>>>>>> +#define gdb_get_regl_value(m, b, v) gdb_get_reg32_value(m, b, v)
>>>>>> +#endif
>>>>>
>>>>> Surely you're not intending to replicate this in any target that can
>>>>> have multiple sizes?
>>>>> This is not an improvement.
>>>>>
>>>>
>>>> If I'm correct (and from v1), ppc is the only architecture having
>>>> registers defined with
>>>> target_long types.
>>>
>>> With a quick check, mips, riscv, sparc do as well.
>>>
>>
>> Right, I should have run the simple grep :)
>> So it's better to keep those macros defined in a separate header (out of
>> helpers.h, like helpers-target.h), so we can already start to cleanup
>> some targets, and do the rest of the work for the ones using
>> target_ulong type for later.
> 
> See also
> https://lore.kernel.org/qemu-devel/f9131f6e-e843-48be-b85f-c341ec198154@linaro.org/
> considering s/TARGET_LONG_SIZE/target_info_long_size()/
> 

The problem is that both macros (ldtul, gdb_get_regl) have different 
signatures. For gdb_get_regl, it could be solved by casting the pointer 
to the right type.

However, for ldtul, it's much more dangerous, as ldl returns a signed 
value, and ldq, an unsigned one. I'm not sure which signature we should 
adopt for this.

That's why I proposed in the last series to replace ldtul_p with another 
function returning value by pointer, so we can easily replace call sites 
without the risk of truncating something.

Re: [PATCH v2 01/11] include/exec: fix assert in size_memop

[Akihiko Odaki]

On 2025/03/24 19:21, 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>
> 
> ---
> v2
>    - instead of 128 use 1 << MO_SIZE for future proofing
> ---
>   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..6afe50a7d0 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.  */

It may be better to avoid writing the literal number (128) in the 
comment too.

Perhaps it is easier to simply remove the comment instead of updating it 
to explain the assertion without the literal number.
(size & (size - 1)) == 0 looks cryptic, but it can be replaced with 
is_power_of_2(), which is more obvious and will not need an explanation.

Regards,
Akihiko Odaki

> +    assert((size & (size - 1)) == 0 && size >= 1 && size <= (1 << MO_SIZE));
>   #endif
>       return (MemOp)ctz32(size);
>   }

Re: [PATCH v2 04/11] gdbstub: introduce target independent gdb register helper

[Akihiko Odaki]

On 2025/03/24 19:21, 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.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use unsigned consistently
>    - fix some rouge whitespace
>    - add typed reg32/64 wrappers
>    - pass void * to underlying helper to avoid casting
> ---
>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>   2 files changed, 78 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..2220f58efe
> --- /dev/null
> +++ b/include/gdbstub/registers.h
> @@ -0,0 +1,55 @@
> +/*
> + * 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.
> + *
> + * There are wrapper functions for the common sizes you can use to
> + * keep type checking.
> + *
> + * Returns the number of bytes written to the array.
> + */
> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
> +
> +/**
> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
> +    g_assert((op & MO_SIZE) == MO_32);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +/**
> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
> + * mo: size and endian MemOp
> + * buf: GByteArray to store in target order
> + * val: pointer to uint32_t value in host order
> + */
> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
> +    g_assert((op & MO_SIZE) == MO_64);
> +    return gdb_get_register_value(op, buf, val);
> +}
> +
> +#endif /* GDB_REGISTERS_H */
> +
> +

Nitpick: extra newlines here.

> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
> +{
> +    unsigned bytes = memop_size(op);
> +
> +    if (op & MO_BSWAP) {
> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
> +        for (unsigned i = bytes; i > 0; i--) {
> +            g_byte_array_append(buf, ptr--, 1);
> +        };
> +    } else {
> +        g_byte_array_append(buf, val, bytes);
> +    }
> +
> +    return bytes;
> +}
> +
> +

An extra blank line here too; each of the other functions in this file 
has only one blank line following.

>   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 v2 04/11] gdbstub: introduce target independent gdb register helper

[Akihiko Odaki]

On 2025/03/24 19:32, Alex Bennée wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
>> 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.
>>
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> ---
>> v2
>>    - use unsigned consistently
>>    - fix some rouge whitespace
>>    - add typed reg32/64 wrappers
>>    - pass void * to underlying helper to avoid casting
>> ---
>>   include/gdbstub/registers.h | 55 +++++++++++++++++++++++++++++++++++++
>>   gdbstub/gdbstub.c           | 23 ++++++++++++++++
>>   2 files changed, 78 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..2220f58efe
>> --- /dev/null
>> +++ b/include/gdbstub/registers.h
>> @@ -0,0 +1,55 @@
>> +/*
>> + * 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.
>> + *
>> + * There are wrapper functions for the common sizes you can use to
>> + * keep type checking.
>> + *
>> + * Returns the number of bytes written to the array.
>> + */
>> +int gdb_get_register_value(MemOp op, GByteArray *buf, void *val);
>> +
>> +/**
>> + * gdb_get_reg32_value() - type checked wrapper for gdb_get_register_value()
>> + * mo: size and endian MemOp
>> + * buf: GByteArray to store in target order
>> + * val: pointer to uint32_t value in host order
>> + */
>> +static inline int gdb_get_reg32_value(MemOp op, GByteArray *buf, uint32_t *val) {
>> +    g_assert((op & MO_SIZE) == MO_32);
>> +    return gdb_get_register_value(op, buf, val);
>> +}
>> +
>> +/**
>> + * gdb_get_reg64_value() - type checked wrapper for gdb_get_register_value()
>> + * mo: size and endian MemOp
>> + * buf: GByteArray to store in target order
>> + * val: pointer to uint32_t value in host order
>> + */
>> +static inline int gdb_get_reg64_value(MemOp op, GByteArray *buf, uint64_t *val) {
>> +    g_assert((op & MO_SIZE) == MO_64);
>> +    return gdb_get_register_value(op, buf, val);
>> +}
>> +
>> +#endif /* GDB_REGISTERS_H */
>> +
>> +
>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> index b6d5e11e03..e799fdc019 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,27 @@ 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, void *val)
>> +{
>> +    unsigned bytes = memop_size(op);
>> +
>> +    if (op & MO_BSWAP) {
>> +        uint8_t *ptr = &((uint8_t *) val)[bytes - 1];
>> +        for (unsigned i = bytes; i > 0; i--) {
>> +            g_byte_array_append(buf, ptr--, 1);
>> +        };
> 
> I forgot to fix this up. Hopefully the following seems a little less
> like pointer abuse:
> 
> /*
>   * 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, void *val)
> {
>      unsigned bytes = memop_size(op);

Nitpick: I would name it "size" instead of "bytes" for consistency.

>      uint8_t *data = val;

It's unclear what "data" means. This can be named "bytes", "val8", 
"octets", etc to differentiate from "val".

> 
>      if (op & MO_BSWAP) {
>          uint8_t *ptr = &data[bytes - 1];
>          do {
>              g_byte_array_append(buf, ptr--, 1);
>          } while (ptr >= data);

It may be better to initialize ptr to point the position after the last 
element:

uint8_t *ptr = &data[bytes];

while (ptr > data) {
   g_byte_array_append(buf, --ptr, 1);
}

Strictly speaking, this is well-defined when bytes == 0, which is not in 
the original version, and you don't need to write decrements at two 
places (initialization and update).

>      } else {
>          g_byte_array_append(buf, val, bytes);
>      }
> 
>      return bytes;
> }
> 
> 
>> +    } 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 v2 01/11] include/exec: fix assert in size_memop

[Philippe Mathieu-Daudé]

On 29/3/25 08:51, Akihiko Odaki wrote:
> On 2025/03/24 19:21, 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>
>>
>> ---
>> v2
>>    - instead of 128 use 1 << MO_SIZE for future proofing
>> ---
>>   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..6afe50a7d0 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.  */
> 
> It may be better to avoid writing the literal number (128) in the 
> comment too.
> 
> Perhaps it is easier to simply remove the comment instead of updating it 
> to explain the assertion without the literal number.
> (size & (size - 1)) == 0 looks cryptic, but it can be replaced with 
> is_power_of_2(), which is more obvious and will not need an explanation.

+1 for is_power_of_2()

> 
> Regards,
> Akihiko Odaki
> 
>> +    assert((size & (size - 1)) == 0 && size >= 1 && size <= (1 << 
>> MO_SIZE));
>>   #endif
>>       return (MemOp)ctz32(size);
>>   }
>