[PATCH v4 0/2] target/arm: Added support for SME register exposure to GDB

[PATCH v4 0/2] target/arm: Added support for SME register exposure to GDB

[Vacha Bhavsar]

The QEMU GDB stub does not expose the ZA storage SME register to GDB via
the remote serial protocol, which can be a useful functionality to debug SME
code. To provide this functionality in Aarch64 target, this patch registers the
SME register set with the GDB stub. To do so, this patch implements the
aarch64_gdb_get_sme_reg() and aarch64_gdb_set_sme_reg() functions to
specify how to get and set the SME registers, and the
arm_gen_dynamic_smereg_feature() function to generate the target
description in XML format to indicate the target architecture supports SME.
Finally, this patch includes a dyn_smereg_feature structure to hold this
GDB XML description of the SME registers for each CPU.

Furthermore, this patch series increases the value of MAX_PACKET_LENGTH
to allow for remote GDB debugging of the ZA register when the vector
length is maximal.

Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>

Vacha Bhavsar (2):
  target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging
  target/arm: Added support for SME register exposure to GDB

-- 
2.34.1

[PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Vacha Bhavsar]

This patch increases the value of the MAX_PACKET_LEGNTH to
131100 from 4096 to allow the GDBState.line_buf to be large enough
to accommodate the full contents of the SME ZA storage when the
vector length is maximal. This is in preparation for a related
patch that allows SME register visibility through remote GDB
debugging.

Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
---
Changes since v3:
- this patch was not present in version 3

 gdbstub/internals.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index bf5a5c6302..b58a66c201 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -11,7 +11,7 @@
 
 #include "exec/cpu-common.h"
 
-#define MAX_PACKET_LENGTH 4096
+#define MAX_PACKET_LENGTH 131100
 
 /*
  * Shared structures and definitions
-- 
2.34.1

[PATCH v4 2/2] target/arm: Added support for SME register exposure to GDB

[Vacha Bhavsar]

The QEMU GDB stub does not expose the ZA storage SME register to GDB via
the remote serial protocol, which can be a useful functionality to debug SME
code. To provide this functionality in Aarch64 target, this patch registers the
SME register set with the GDB stub. To do so, this patch implements the
aarch64_gdb_get_sme_reg() and aarch64_gdb_set_sme_reg() functions to
specify how to get and set the SME registers, and the
arm_gen_dynamic_smereg_feature() function to generate the target
description in XML format to indicate the target architecture supports SME.
Finally, this patch includes a dyn_smereg_feature structure to hold this
GDB XML description of the SME registers for each CPU.

Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
---
Changes since v3:
- added changes to aarch64_gdb_set_sme_reg() to address the concerns 
brought up in review regarding endianness

 target/arm/cpu.h       |   1 +
 target/arm/gdbstub.c   |   6 ++
 target/arm/gdbstub64.c | 122 +++++++++++++++++++++++++++++++++++++++++
 target/arm/internals.h |   3 +
 4 files changed, 132 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index dc9b6dce4c..8bd66d7049 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -933,6 +933,7 @@ struct ArchCPU {
 
     DynamicGDBFeatureInfo dyn_sysreg_feature;
     DynamicGDBFeatureInfo dyn_svereg_feature;
+    DynamicGDBFeatureInfo dyn_smereg_feature;
     DynamicGDBFeatureInfo dyn_m_systemreg_feature;
     DynamicGDBFeatureInfo dyn_m_secextreg_feature;
 
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index ce4497ad7c..9c942c77cc 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -531,6 +531,12 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
             GDBFeature *feature = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs);
             gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
                                      aarch64_gdb_set_sve_reg, feature, 0);
+            if (isar_feature_aa64_sme(&cpu->isar)) {
+                GDBFeature *sme_feature = arm_gen_dynamic_smereg_feature(cs,
+                                             cs->gdb_num_regs);
+                gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
+                    aarch64_gdb_set_sme_reg, sme_feature, 0);
+            }
         } else {
             gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
                                      aarch64_gdb_set_fpu_reg,
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 64ee9b3b56..3d86980bc9 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -228,6 +228,91 @@ int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg)
     return 0;
 }
 
+int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    switch (reg) {
+    /* Svg register */
+    case 0:
+    {
+        int vq = 0;
+        if (FIELD_EX64(env->svcr, SVCR, SM)) {
+            vq = sve_vqm1_for_el_sm(env, arm_current_el(env),
+                     FIELD_EX64(env->svcr, SVCR, SM)) + 1;
+        }
+        /* svg = vector granules (2 * vector quardwords) in streaming mode */
+        return gdb_get_reg64(buf, vq * 2);
+    }
+    case 1:
+        return gdb_get_reg64(buf, env->svcr);
+    case 2:
+    {
+        int len = 0;
+        int vq = cpu->sme_max_vq;
+        int svl = vq * 16;
+        for (int i = 0; i < svl; i++) {
+            for (int q = 0; q < vq; q++) {
+                len += gdb_get_reg128(buf,
+                           env->za_state.za[i].d[q * 2 + 1],
+                           env->za_state.za[i].d[q * 2]);
+            }
+        }
+        return len;
+    }
+    default:
+        /* gdbstub asked for something out of range */
+        qemu_log_mask(LOG_UNIMP, "%s: out of range register %d", __func__, reg);
+        break;
+    }
+
+    return 0;
+}
+
+int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    switch (reg) {
+    case 0:
+    {
+        /* cannot set svg via gdbstub */
+        return 8;
+    }
+    case 1:
+        aarch64_set_svcr(env, ldq_le_p(buf),
+            R_SVCR_SM_MASK | R_SVCR_ZA_MASK);
+        return 8;
+    case 2:
+        int len = 0;
+        int vq = cpu->sme_max_vq;
+        int svl = vq * 16;
+        for (int i = 0; i < svl; i++) {
+            for (int q = 0; q < vq; q++) {
+                if (target_big_endian()){
+                    env->za_state.za[i].d[q * 2 + 1] = ldq_p(buf);
+                    buf += 8;
+                    env->za_state.za[i].d[q * 2] = ldq_p(buf);
+                } else{
+                    env->za_state.za[i].d[q * 2] = ldq_p(buf);
+                    buf += 8;
+                    env->za_state.za[i].d[q * 2 + 1] = ldq_p(buf);
+                }
+                buf += 8;
+                len += 16;
+            }
+        }
+        return len;
+    default:
+        /* gdbstub asked for something out of range */
+        break;
+    }
+
+    return 0;
+}
+
 int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -392,6 +477,43 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg)
     return &cpu->dyn_svereg_feature.desc;
 }
 
+GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cs, int base_reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    int vq = cpu->sme_max_vq;
+    int svl = vq * 16;
+    GDBFeatureBuilder builder;
+    int reg = 0;
+
+    gdb_feature_builder_init(&builder, &cpu->dyn_smereg_feature.desc,
+        "org.gnu.gdb.aarch64.sme", "sme-registers.xml", base_reg);
+
+
+    /* Create the sme_bv vector type. */
+    gdb_feature_builder_append_tag(&builder,
+        "<vector id=\"sme_bv\" type=\"uint8\" count=\"%d\"/>",
+        svl);
+
+    /* Create the sme_bvv vector type. */
+    gdb_feature_builder_append_tag(
+        &builder, "<vector id=\"sme_bvv\" type=\"sme_bv\" count=\"%d\"/>",
+        svl);
+
+    /* Define the svg, svcr, and za registers. */
+
+    /* fpscr & status registers */
+    gdb_feature_builder_append_reg(&builder, "svg", 64, reg++,
+        "int", NULL);
+    gdb_feature_builder_append_reg(&builder, "svcr", 64, reg++,
+        "int", NULL);
+    gdb_feature_builder_append_reg(&builder, "za", svl * svl * 8, reg++,
+        "sme_bvv", NULL);
+
+    gdb_feature_builder_end(&builder);
+
+    return &cpu->dyn_smereg_feature.desc;
+}
+
 #ifdef CONFIG_USER_ONLY
 int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
 {
diff --git a/target/arm/internals.h b/target/arm/internals.h
index c4765e4489..760e1c6490 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1808,8 +1808,11 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
 }
 
 GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int base_reg);
+GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cpu, int base_reg);
 int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg);
 int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg);
+int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg);
+int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg);
 int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg);
 int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg);
 int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg);
-- 
2.34.1

[PATCH v4 2/2] target/arm: Added support for SME register exposure to GDB

[Vacha Bhavsar]

The QEMU GDB stub does not expose the ZA storage SME register to GDB via
the remote serial protocol, which can be a useful functionality to debug SME
code. To provide this functionality in Aarch64 target, this patch registers the
SME register set with the GDB stub. To do so, this patch implements the
aarch64_gdb_get_sme_reg() and aarch64_gdb_set_sme_reg() functions to
specify how to get and set the SME registers, and the
arm_gen_dynamic_smereg_feature() function to generate the target
description in XML format to indicate the target architecture supports SME.
Finally, this patch includes a dyn_smereg_feature structure to hold this
GDB XML description of the SME registers for each CPU.

Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
---
 target/arm/cpu.h       |   1 +
 target/arm/gdbstub.c   |   6 ++
 target/arm/gdbstub64.c | 122 +++++++++++++++++++++++++++++++++++++++++
 target/arm/internals.h |   3 +
 4 files changed, 132 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index dc9b6dce4c..8bd66d7049 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -933,6 +933,7 @@ struct ArchCPU {
 
     DynamicGDBFeatureInfo dyn_sysreg_feature;
     DynamicGDBFeatureInfo dyn_svereg_feature;
+    DynamicGDBFeatureInfo dyn_smereg_feature;
     DynamicGDBFeatureInfo dyn_m_systemreg_feature;
     DynamicGDBFeatureInfo dyn_m_secextreg_feature;
 
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index ce4497ad7c..9c942c77cc 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -531,6 +531,12 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
             GDBFeature *feature = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs);
             gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
                                      aarch64_gdb_set_sve_reg, feature, 0);
+            if (isar_feature_aa64_sme(&cpu->isar)) {
+                GDBFeature *sme_feature = arm_gen_dynamic_smereg_feature(cs,
+                                             cs->gdb_num_regs);
+                gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
+                    aarch64_gdb_set_sme_reg, sme_feature, 0);
+            }
         } else {
             gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
                                      aarch64_gdb_set_fpu_reg,
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 64ee9b3b56..3d86980bc9 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -228,6 +228,91 @@ int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg)
     return 0;
 }
 
+int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    switch (reg) {
+    /* Svg register */
+    case 0:
+    {
+        int vq = 0;
+        if (FIELD_EX64(env->svcr, SVCR, SM)) {
+            vq = sve_vqm1_for_el_sm(env, arm_current_el(env),
+                     FIELD_EX64(env->svcr, SVCR, SM)) + 1;
+        }
+        /* svg = vector granules (2 * vector quardwords) in streaming mode */
+        return gdb_get_reg64(buf, vq * 2);
+    }
+    case 1:
+        return gdb_get_reg64(buf, env->svcr);
+    case 2:
+    {
+        int len = 0;
+        int vq = cpu->sme_max_vq;
+        int svl = vq * 16;
+        for (int i = 0; i < svl; i++) {
+            for (int q = 0; q < vq; q++) {
+                len += gdb_get_reg128(buf,
+                           env->za_state.za[i].d[q * 2 + 1],
+                           env->za_state.za[i].d[q * 2]);
+            }
+        }
+        return len;
+    }
+    default:
+        /* gdbstub asked for something out of range */
+        qemu_log_mask(LOG_UNIMP, "%s: out of range register %d", __func__, reg);
+        break;
+    }
+
+    return 0;
+}
+
+int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+
+    switch (reg) {
+    case 0:
+    {
+        /* cannot set svg via gdbstub */
+        return 8;
+    }
+    case 1:
+        aarch64_set_svcr(env, ldq_le_p(buf),
+            R_SVCR_SM_MASK | R_SVCR_ZA_MASK);
+        return 8;
+    case 2:
+        int len = 0;
+        int vq = cpu->sme_max_vq;
+        int svl = vq * 16;
+        for (int i = 0; i < svl; i++) {
+            for (int q = 0; q < vq; q++) {
+                if (target_big_endian()){
+                    env->za_state.za[i].d[q * 2 + 1] = ldq_p(buf);
+                    buf += 8;
+                    env->za_state.za[i].d[q * 2] = ldq_p(buf);
+                } else{
+                    env->za_state.za[i].d[q * 2] = ldq_p(buf);
+                    buf += 8;
+                    env->za_state.za[i].d[q * 2 + 1] = ldq_p(buf);
+                }
+                buf += 8;
+                len += 16;
+            }
+        }
+        return len;
+    default:
+        /* gdbstub asked for something out of range */
+        break;
+    }
+
+    return 0;
+}
+
 int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -392,6 +477,43 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg)
     return &cpu->dyn_svereg_feature.desc;
 }
 
+GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cs, int base_reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    int vq = cpu->sme_max_vq;
+    int svl = vq * 16;
+    GDBFeatureBuilder builder;
+    int reg = 0;
+
+    gdb_feature_builder_init(&builder, &cpu->dyn_smereg_feature.desc,
+        "org.gnu.gdb.aarch64.sme", "sme-registers.xml", base_reg);
+
+
+    /* Create the sme_bv vector type. */
+    gdb_feature_builder_append_tag(&builder,
+        "<vector id=\"sme_bv\" type=\"uint8\" count=\"%d\"/>",
+        svl);
+
+    /* Create the sme_bvv vector type. */
+    gdb_feature_builder_append_tag(
+        &builder, "<vector id=\"sme_bvv\" type=\"sme_bv\" count=\"%d\"/>",
+        svl);
+
+    /* Define the svg, svcr, and za registers. */
+
+    /* fpscr & status registers */
+    gdb_feature_builder_append_reg(&builder, "svg", 64, reg++,
+        "int", NULL);
+    gdb_feature_builder_append_reg(&builder, "svcr", 64, reg++,
+        "int", NULL);
+    gdb_feature_builder_append_reg(&builder, "za", svl * svl * 8, reg++,
+        "sme_bvv", NULL);
+
+    gdb_feature_builder_end(&builder);
+
+    return &cpu->dyn_smereg_feature.desc;
+}
+
 #ifdef CONFIG_USER_ONLY
 int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
 {
diff --git a/target/arm/internals.h b/target/arm/internals.h
index c4765e4489..760e1c6490 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1808,8 +1808,11 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
 }
 
 GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int base_reg);
+GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cpu, int base_reg);
 int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg);
 int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg);
+int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg);
+int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg);
 int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg);
 int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg);
 int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg);
-- 
2.34.1

Re: [PATCH v4 0/2] target/arm: Added support for SME register exposure to GDB

[Philippe Mathieu-Daudé]

Cc'ing Gustavo.

On 22/7/25 22:14, Vacha Bhavsar wrote:
> The QEMU GDB stub does not expose the ZA storage SME register to GDB via
> the remote serial protocol, which can be a useful functionality to debug SME
> code. To provide this functionality in Aarch64 target, this patch registers the
> SME register set with the GDB stub. To do so, this patch implements the
> aarch64_gdb_get_sme_reg() and aarch64_gdb_set_sme_reg() functions to
> specify how to get and set the SME registers, and the
> arm_gen_dynamic_smereg_feature() function to generate the target
> description in XML format to indicate the target architecture supports SME.
> Finally, this patch includes a dyn_smereg_feature structure to hold this
> GDB XML description of the SME registers for each CPU.
> 
> Furthermore, this patch series increases the value of MAX_PACKET_LENGTH
> to allow for remote GDB debugging of the ZA register when the vector
> length is maximal.
> 
> Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> 
> Vacha Bhavsar (2):
>    target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging
>    target/arm: Added support for SME register exposure to GDB
> 

Re: [PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Alex Bennée]

Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:

> This patch increases the value of the MAX_PACKET_LEGNTH to
> 131100 from 4096 to allow the GDBState.line_buf to be large enough
> to accommodate the full contents of the SME ZA storage when the
> vector length is maximal. This is in preparation for a related
> patch that allows SME register visibility through remote GDB
> debugging.
>
> Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> ---
> Changes since v3:
> - this patch was not present in version 3
>
>  gdbstub/internals.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index bf5a5c6302..b58a66c201 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -11,7 +11,7 @@
>  
>  #include "exec/cpu-common.h"
>  
> -#define MAX_PACKET_LENGTH 4096
> +#define MAX_PACKET_LENGTH 131100

This is a rather large expansion for something that ends up in a static at:

    char line_buf[MAX_PACKET_LENGTH];

I think maybe its time to get rid of this hardcoded define and make line_buf a
dynamically re-sizeable buffer along the lines of str_buf and mem_buf.
In fact make it a GString and we can get rid of line_buf_index as well.

>  
>  /*
>   * Shared structures and definitions

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v4 2/2] target/arm: Added support for SME register exposure to GDB

[Alex Bennée]

Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:

> The QEMU GDB stub does not expose the ZA storage SME register to GDB via
> the remote serial protocol, which can be a useful functionality to debug SME
> code. To provide this functionality in Aarch64 target, this patch registers the
> SME register set with the GDB stub. To do so, this patch implements the
> aarch64_gdb_get_sme_reg() and aarch64_gdb_set_sme_reg() functions to
> specify how to get and set the SME registers, and the
> arm_gen_dynamic_smereg_feature() function to generate the target
> description in XML format to indicate the target architecture supports SME.
> Finally, this patch includes a dyn_smereg_feature structure to hold this
> GDB XML description of the SME registers for each CPU.
>
> Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> ---
> Changes since v3:
> - added changes to aarch64_gdb_set_sme_reg() to address the concerns 
> brought up in review regarding endianness
>
>  target/arm/cpu.h       |   1 +
>  target/arm/gdbstub.c   |   6 ++
>  target/arm/gdbstub64.c | 122 +++++++++++++++++++++++++++++++++++++++++
>  target/arm/internals.h |   3 +
>  4 files changed, 132 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index dc9b6dce4c..8bd66d7049 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -933,6 +933,7 @@ struct ArchCPU {
>  
>      DynamicGDBFeatureInfo dyn_sysreg_feature;
>      DynamicGDBFeatureInfo dyn_svereg_feature;
> +    DynamicGDBFeatureInfo dyn_smereg_feature;
>      DynamicGDBFeatureInfo dyn_m_systemreg_feature;
>      DynamicGDBFeatureInfo dyn_m_secextreg_feature;
>  
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index ce4497ad7c..9c942c77cc 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -531,6 +531,12 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>              GDBFeature *feature = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs);
>              gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
>                                       aarch64_gdb_set_sve_reg, feature, 0);
> +            if (isar_feature_aa64_sme(&cpu->isar)) {
> +                GDBFeature *sme_feature = arm_gen_dynamic_smereg_feature(cs,
> +                                             cs->gdb_num_regs);
> +                gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
> +                    aarch64_gdb_set_sme_reg, sme_feature, 0);
> +            }
>          } else {
>              gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
>                                       aarch64_gdb_set_fpu_reg,
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index 64ee9b3b56..3d86980bc9 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -228,6 +228,91 @@ int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg)
>      return 0;
>  }
>  
> +int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    switch (reg) {
> +    /* Svg register */
> +    case 0:
> +    {
> +        int vq = 0;
> +        if (FIELD_EX64(env->svcr, SVCR, SM)) {
> +            vq = sve_vqm1_for_el_sm(env, arm_current_el(env),
> +                     FIELD_EX64(env->svcr, SVCR, SM)) + 1;
> +        }
> +        /* svg = vector granules (2 * vector quardwords) in streaming mode */
> +        return gdb_get_reg64(buf, vq * 2);
> +    }
> +    case 1:
> +        return gdb_get_reg64(buf, env->svcr);
> +    case 2:
> +    {
> +        int len = 0;
> +        int vq = cpu->sme_max_vq;
> +        int svl = vq * 16;
> +        for (int i = 0; i < svl; i++) {
> +            for (int q = 0; q < vq; q++) {
> +                len += gdb_get_reg128(buf,
> +                           env->za_state.za[i].d[q * 2 + 1],
> +                           env->za_state.za[i].d[q * 2]);
> +            }
> +        }
> +        return len;
> +    }
> +    default:
> +        /* gdbstub asked for something out of range */
> +        qemu_log_mask(LOG_UNIMP, "%s: out of range register %d", __func__, reg);
> +        break;
> +    }
> +
> +    return 0;
> +}
> +
> +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    switch (reg) {
> +    case 0:
> +    {
> +        /* cannot set svg via gdbstub */
> +        return 8;
> +    }
> +    case 1:
> +        aarch64_set_svcr(env, ldq_le_p(buf),
> +            R_SVCR_SM_MASK | R_SVCR_ZA_MASK);
> +        return 8;
> +    case 2:
> +        int len = 0;
> +        int vq = cpu->sme_max_vq;
> +        int svl = vq * 16;
> +        for (int i = 0; i < svl; i++) {
> +            for (int q = 0; q < vq; q++) {
> +                if (target_big_endian()){
> +                    env->za_state.za[i].d[q * 2 + 1] = ldq_p(buf);
> +                    buf += 8;
> +                    env->za_state.za[i].d[q * 2] = ldq_p(buf);
> +                } else{
> +                    env->za_state.za[i].d[q * 2] = ldq_p(buf);
> +                    buf += 8;
> +                    env->za_state.za[i].d[q * 2 + 1] = ldq_p(buf);
> +                }
> +                buf += 8;
> +                len += 16;
> +            }
> +        }
> +        return len;
> +    default:
> +        /* gdbstub asked for something out of range */
> +        break;
> +    }
> +
> +    return 0;
> +}
> +
>  int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -392,6 +477,43 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg)
>      return &cpu->dyn_svereg_feature.desc;
>  }
>  
> +GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cs, int base_reg)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    int vq = cpu->sme_max_vq;
> +    int svl = vq * 16;
> +    GDBFeatureBuilder builder;
> +    int reg = 0;
> +
> +    gdb_feature_builder_init(&builder, &cpu->dyn_smereg_feature.desc,
> +        "org.gnu.gdb.aarch64.sme", "sme-registers.xml", base_reg);
> +
> +
> +    /* Create the sme_bv vector type. */
> +    gdb_feature_builder_append_tag(&builder,
> +        "<vector id=\"sme_bv\" type=\"uint8\" count=\"%d\"/>",
> +        svl);
> +
> +    /* Create the sme_bvv vector type. */
> +    gdb_feature_builder_append_tag(
> +        &builder, "<vector id=\"sme_bvv\" type=\"sme_bv\" count=\"%d\"/>",
> +        svl);
> +
> +    /* Define the svg, svcr, and za registers. */
> +
> +    /* fpscr & status registers */
> +    gdb_feature_builder_append_reg(&builder, "svg", 64, reg++,
> +        "int", NULL);
> +    gdb_feature_builder_append_reg(&builder, "svcr", 64, reg++,
> +        "int", NULL);
> +    gdb_feature_builder_append_reg(&builder, "za", svl * svl * 8, reg++,
> +        "sme_bvv", NULL);
> +
> +    gdb_feature_builder_end(&builder);
> +
> +    return &cpu->dyn_smereg_feature.desc;
> +}
> +
>  #ifdef CONFIG_USER_ONLY
>  int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
>  {
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index c4765e4489..760e1c6490 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1808,8 +1808,11 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
>  }
>  
>  GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int base_reg);
> +GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cpu, int base_reg);
>  int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg);
>  int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg);
> +int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg);
> +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg);
>  int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg);
>  int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg);
>  int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg);

It would also be nice to add a test for this, see tests/tcg/aarch64/gdbstub/test-sve.py

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Peter Maydell]

On Mon, 4 Aug 2025 at 16:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
>
> > This patch increases the value of the MAX_PACKET_LEGNTH to
> > 131100 from 4096 to allow the GDBState.line_buf to be large enough
> > to accommodate the full contents of the SME ZA storage when the
> > vector length is maximal. This is in preparation for a related
> > patch that allows SME register visibility through remote GDB
> > debugging.
> >
> > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> > ---
> > Changes since v3:
> > - this patch was not present in version 3
> >
> >  gdbstub/internals.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> > index bf5a5c6302..b58a66c201 100644
> > --- a/gdbstub/internals.h
> > +++ b/gdbstub/internals.h
> > @@ -11,7 +11,7 @@
> >
> >  #include "exec/cpu-common.h"
> >
> > -#define MAX_PACKET_LENGTH 4096
> > +#define MAX_PACKET_LENGTH 131100
>
> This is a rather large expansion for something that ends up in a static at:
>
>     char line_buf[MAX_PACKET_LENGTH];
>
> I think maybe its time to get rid of this hardcoded define and make line_buf a
> dynamically re-sizeable buffer along the lines of str_buf and mem_buf.
> In fact make it a GString and we can get rid of line_buf_index as well.

What exactly is the packet/response where MAX_PACKET_LENGTH is
causing problems? The commit message doesn't say.

In general I thought the gdbstub protocol was supposed to handle a
fixed packet length (e.g. in handle_query_xfer_features() the response
packet indicates truncation via "l" vs "m" so the gdb end knows it needs
to send another request to get the rest of the data). So if we run
into something which seems to be fixed by raising MAX_PACKET_LENGTH
I would first want to look at whether the underlying problem is
that we're not indicating to gdb "this data is incomplete, you'll
need to ask for more of it" or something of that nature.

thanks
-- PMM

Re: [PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 4 Aug 2025 at 16:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
>>
>> > This patch increases the value of the MAX_PACKET_LEGNTH to
>> > 131100 from 4096 to allow the GDBState.line_buf to be large enough
>> > to accommodate the full contents of the SME ZA storage when the
>> > vector length is maximal. This is in preparation for a related
>> > patch that allows SME register visibility through remote GDB
>> > debugging.
>> >
>> > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
>> > ---
>> > Changes since v3:
>> > - this patch was not present in version 3
>> >
>> >  gdbstub/internals.h | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/gdbstub/internals.h b/gdbstub/internals.h
>> > index bf5a5c6302..b58a66c201 100644
>> > --- a/gdbstub/internals.h
>> > +++ b/gdbstub/internals.h
>> > @@ -11,7 +11,7 @@
>> >
>> >  #include "exec/cpu-common.h"
>> >
>> > -#define MAX_PACKET_LENGTH 4096
>> > +#define MAX_PACKET_LENGTH 131100
>>
>> This is a rather large expansion for something that ends up in a static at:
>>
>>     char line_buf[MAX_PACKET_LENGTH];
>>
>> I think maybe its time to get rid of this hardcoded define and make line_buf a
>> dynamically re-sizeable buffer along the lines of str_buf and mem_buf.
>> In fact make it a GString and we can get rid of line_buf_index as well.
>
> What exactly is the packet/response where MAX_PACKET_LENGTH is
> causing problems? The commit message doesn't say.

I assume it would be the g/G or p/P packets. The docs don't seem to say
anything about them splitting them across multiple packets.

> In general I thought the gdbstub protocol was supposed to handle a
> fixed packet length (e.g. in handle_query_xfer_features() the response
> packet indicates truncation via "l" vs "m" so the gdb end knows it needs
> to send another request to get the rest of the data). So if we run
> into something which seems to be fixed by raising MAX_PACKET_LENGTH
> I would first want to look at whether the underlying problem is
> that we're not indicating to gdb "this data is incomplete, you'll
> need to ask for more of it" or something of that nature.

The docs reference "bulk transfers":

  ‘PacketSize=bytes’

      The remote stub can accept packets up to at least bytes in length.
      GDB will send packets up to this size for bulk transfers, and will
      never send larger packets. This is a limit on the data characters
      in the packet, not including the frame and checksum. There is no
      trailing NUL byte in a remote protocol packet; if the stub stores
      packets in a NUL-terminated format, it should allow an extra byte
      in its buffer for the NUL. If this stub feature is not supported,
      GDB guesses based on the size of the ‘g’ packet response.


>
> thanks
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Peter Maydell]

On Mon, 4 Aug 2025 at 19:32, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Mon, 4 Aug 2025 at 16:34, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
> >>
> >> > This patch increases the value of the MAX_PACKET_LEGNTH to
> >> > 131100 from 4096 to allow the GDBState.line_buf to be large enough
> >> > to accommodate the full contents of the SME ZA storage when the
> >> > vector length is maximal. This is in preparation for a related
> >> > patch that allows SME register visibility through remote GDB
> >> > debugging.
> >> >
> >> > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> >> > ---
> >> > Changes since v3:
> >> > - this patch was not present in version 3
> >> >
> >> >  gdbstub/internals.h | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> >> > index bf5a5c6302..b58a66c201 100644
> >> > --- a/gdbstub/internals.h
> >> > +++ b/gdbstub/internals.h
> >> > @@ -11,7 +11,7 @@
> >> >
> >> >  #include "exec/cpu-common.h"
> >> >
> >> > -#define MAX_PACKET_LENGTH 4096
> >> > +#define MAX_PACKET_LENGTH 131100
> >>
> >> This is a rather large expansion for something that ends up in a static at:
> >>
> >>     char line_buf[MAX_PACKET_LENGTH];
> >>
> >> I think maybe its time to get rid of this hardcoded define and make line_buf a
> >> dynamically re-sizeable buffer along the lines of str_buf and mem_buf.
> >> In fact make it a GString and we can get rid of line_buf_index as well.
> >
> > What exactly is the packet/response where MAX_PACKET_LENGTH is
> > causing problems? The commit message doesn't say.
>
> I assume it would be the g/G or p/P packets. The docs don't seem to say
> anything about them splitting them across multiple packets.

Probably because nobody thought about the possibility of enormous
registers. This sounds like something to query with the gdb devs
about what they expect the handling of the SME ZA storage should be.

> > In general I thought the gdbstub protocol was supposed to handle a
> > fixed packet length (e.g. in handle_query_xfer_features() the response
> > packet indicates truncation via "l" vs "m" so the gdb end knows it needs
> > to send another request to get the rest of the data). So if we run
> > into something which seems to be fixed by raising MAX_PACKET_LENGTH
> > I would first want to look at whether the underlying problem is
> > that we're not indicating to gdb "this data is incomplete, you'll
> > need to ask for more of it" or something of that nature.
>
> The docs reference "bulk transfers":
>
>   ‘PacketSize=bytes’
>
>       The remote stub can accept packets up to at least bytes in length.
>       GDB will send packets up to this size for bulk transfers, and will
>       never send larger packets. This is a limit on the data characters
>       in the packet, not including the frame and checksum. There is no
>       trailing NUL byte in a remote protocol packet; if the stub stores
>       packets in a NUL-terminated format, it should allow an extra byte
>       in its buffer for the NUL. If this stub feature is not supported,
>       GDB guesses based on the size of the ‘g’ packet response.

We do advertise this.

-- PMM

Re: [PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Vacha Bhavsar]

[-- Attachment #1: Type: text/plain, Size: 5201 bytes --]

Hi,



Thank you for your comments!



What exactly is the packet/response where MAX_PACKET_LENGTH is
causing problems? The commit message doesn't say.



The issue is that when we run something like set $za[0][0] = 0x01
in the gdb client, the client sends the entire contents on the new
expected za register value, at which point the client side gets stuck
and does not return the gdb prompt. The issue is found to be the following
code (line 2396 of gdbstub/gdbstub.c):



else if (gdbserver_state.line_buf_index >= sizeof(gdbserver_state.line_buf)
- 1) {
                trace_gdbstub_err_overrun();
                gdbserver_state.state = RS_IDLE;



Since the current value of sizeof(gdbserver_state.line_buf) is 4096 whereas
the
entire contents of the P packet coming in from the gdb client is at least
131072
(twice the number of bytes in the za storage at max svl), the above
statement
eventually evaluates to true, causing the state machine to reset to RS_IDLE
and
treat the rest of the packet as if it's looking for a new command. This is
why
the client side gets stuck until there is a timeout and then debugging
continues
as usual.



For this reason, the MAX_PACKET_LENGTH value was increased in an effort to
increase
the size of gdbserver_state.line_buf and avoid entering the above mentioned
clause.





This sounds like something to query with the gdb devs
about what they expect the handling of the SME ZA storage should be.



Will do!



Thanks,
Vacha

On Mon, Aug 4, 2025 at 2:38 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 4 Aug 2025 at 19:32, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > On Mon, 4 Aug 2025 at 16:34, Alex Bennée <alex.bennee@linaro.org>
> wrote:
> > >>
> > >> Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
> > >>
> > >> > This patch increases the value of the MAX_PACKET_LEGNTH to
> > >> > 131100 from 4096 to allow the GDBState.line_buf to be large enough
> > >> > to accommodate the full contents of the SME ZA storage when the
> > >> > vector length is maximal. This is in preparation for a related
> > >> > patch that allows SME register visibility through remote GDB
> > >> > debugging.
> > >> >
> > >> > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> > >> > ---
> > >> > Changes since v3:
> > >> > - this patch was not present in version 3
> > >> >
> > >> >  gdbstub/internals.h | 2 +-
> > >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> > >> > index bf5a5c6302..b58a66c201 100644
> > >> > --- a/gdbstub/internals.h
> > >> > +++ b/gdbstub/internals.h
> > >> > @@ -11,7 +11,7 @@
> > >> >
> > >> >  #include "exec/cpu-common.h"
> > >> >
> > >> > -#define MAX_PACKET_LENGTH 4096
> > >> > +#define MAX_PACKET_LENGTH 131100
> > >>
> > >> This is a rather large expansion for something that ends up in a
> static at:
> > >>
> > >>     char line_buf[MAX_PACKET_LENGTH];
> > >>
> > >> I think maybe its time to get rid of this hardcoded define and make
> line_buf a
> > >> dynamically re-sizeable buffer along the lines of str_buf and mem_buf.
> > >> In fact make it a GString and we can get rid of line_buf_index as
> well.
> > >
> > > What exactly is the packet/response where MAX_PACKET_LENGTH is
> > > causing problems? The commit message doesn't say.
> >
> > I assume it would be the g/G or p/P packets. The docs don't seem to say
> > anything about them splitting them across multiple packets.
>
> Probably because nobody thought about the possibility of enormous
> registers. This sounds like something to query with the gdb devs
> about what they expect the handling of the SME ZA storage should be.
>
> > > In general I thought the gdbstub protocol was supposed to handle a
> > > fixed packet length (e.g. in handle_query_xfer_features() the response
> > > packet indicates truncation via "l" vs "m" so the gdb end knows it
> needs
> > > to send another request to get the rest of the data). So if we run
> > > into something which seems to be fixed by raising MAX_PACKET_LENGTH
> > > I would first want to look at whether the underlying problem is
> > > that we're not indicating to gdb "this data is incomplete, you'll
> > > need to ask for more of it" or something of that nature.
> >
> > The docs reference "bulk transfers":
> >
> >   ‘PacketSize=bytes’
> >
> >       The remote stub can accept packets up to at least bytes in length.
> >       GDB will send packets up to this size for bulk transfers, and will
> >       never send larger packets. This is a limit on the data characters
> >       in the packet, not including the frame and checksum. There is no
> >       trailing NUL byte in a remote protocol packet; if the stub stores
> >       packets in a NUL-terminated format, it should allow an extra byte
> >       in its buffer for the NUL. If this stub feature is not supported,
> >       GDB guesses based on the size of the ‘g’ packet response.
>
> We do advertise this.
>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 6804 bytes --]

Re: [PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Vacha Bhavsar]

[-- Attachment #1: Type: text/plain, Size: 5961 bytes --]

Hi,

I reached out to the gdb dev team regarding these questions
on how to correctly handle the SME za storage. This discussion
can be found here:

https://sourceware.org/pipermail/gdb/2025-August/051858.html
https://sourceware.org/pipermail/gdb/2025-August/051860.html

It seems this approach of increasing the buffer size to above
131100 has also been applied to gdbserver as there currently
isn't support for partial data transfers.



Thanks,

Vacha

On Tue, Aug 5, 2025 at 5:21 PM Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
wrote:

> Hi,
>
>
>
> Thank you for your comments!
>
>
>
> What exactly is the packet/response where MAX_PACKET_LENGTH is
> causing problems? The commit message doesn't say.
>
>
>
> The issue is that when we run something like set $za[0][0] = 0x01
> in the gdb client, the client sends the entire contents on the new
> expected za register value, at which point the client side gets stuck
> and does not return the gdb prompt. The issue is found to be the following
> code (line 2396 of gdbstub/gdbstub.c):
>
>
>
> else if (gdbserver_state.line_buf_index >=
> sizeof(gdbserver_state.line_buf) - 1) {
>                 trace_gdbstub_err_overrun();
>                 gdbserver_state.state = RS_IDLE;
>
>
>
> Since the current value of sizeof(gdbserver_state.line_buf) is 4096
> whereas the
> entire contents of the P packet coming in from the gdb client is at least
> 131072
> (twice the number of bytes in the za storage at max svl), the above
> statement
> eventually evaluates to true, causing the state machine to reset to
> RS_IDLE and
> treat the rest of the packet as if it's looking for a new command. This is
> why
> the client side gets stuck until there is a timeout and then debugging
> continues
> as usual.
>
>
>
> For this reason, the MAX_PACKET_LENGTH value was increased in an effort to
> increase
> the size of gdbserver_state.line_buf and avoid entering the above
> mentioned clause.
>
>
>
>
>
> This sounds like something to query with the gdb devs
> about what they expect the handling of the SME ZA storage should be.
>
>
>
> Will do!
>
>
>
> Thanks,
> Vacha
>
> On Mon, Aug 4, 2025 at 2:38 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On Mon, 4 Aug 2025 at 19:32, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >
>> > Peter Maydell <peter.maydell@linaro.org> writes:
>> >
>> > > On Mon, 4 Aug 2025 at 16:34, Alex Bennée <alex.bennee@linaro.org>
>> wrote:
>> > >>
>> > >> Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
>> > >>
>> > >> > This patch increases the value of the MAX_PACKET_LEGNTH to
>> > >> > 131100 from 4096 to allow the GDBState.line_buf to be large enough
>> > >> > to accommodate the full contents of the SME ZA storage when the
>> > >> > vector length is maximal. This is in preparation for a related
>> > >> > patch that allows SME register visibility through remote GDB
>> > >> > debugging.
>> > >> >
>> > >> > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
>> > >> > ---
>> > >> > Changes since v3:
>> > >> > - this patch was not present in version 3
>> > >> >
>> > >> >  gdbstub/internals.h | 2 +-
>> > >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >> >
>> > >> > diff --git a/gdbstub/internals.h b/gdbstub/internals.h
>> > >> > index bf5a5c6302..b58a66c201 100644
>> > >> > --- a/gdbstub/internals.h
>> > >> > +++ b/gdbstub/internals.h
>> > >> > @@ -11,7 +11,7 @@
>> > >> >
>> > >> >  #include "exec/cpu-common.h"
>> > >> >
>> > >> > -#define MAX_PACKET_LENGTH 4096
>> > >> > +#define MAX_PACKET_LENGTH 131100
>> > >>
>> > >> This is a rather large expansion for something that ends up in a
>> static at:
>> > >>
>> > >>     char line_buf[MAX_PACKET_LENGTH];
>> > >>
>> > >> I think maybe its time to get rid of this hardcoded define and make
>> line_buf a
>> > >> dynamically re-sizeable buffer along the lines of str_buf and
>> mem_buf.
>> > >> In fact make it a GString and we can get rid of line_buf_index as
>> well.
>> > >
>> > > What exactly is the packet/response where MAX_PACKET_LENGTH is
>> > > causing problems? The commit message doesn't say.
>> >
>> > I assume it would be the g/G or p/P packets. The docs don't seem to say
>> > anything about them splitting them across multiple packets.
>>
>> Probably because nobody thought about the possibility of enormous
>> registers. This sounds like something to query with the gdb devs
>> about what they expect the handling of the SME ZA storage should be.
>>
>> > > In general I thought the gdbstub protocol was supposed to handle a
>> > > fixed packet length (e.g. in handle_query_xfer_features() the response
>> > > packet indicates truncation via "l" vs "m" so the gdb end knows it
>> needs
>> > > to send another request to get the rest of the data). So if we run
>> > > into something which seems to be fixed by raising MAX_PACKET_LENGTH
>> > > I would first want to look at whether the underlying problem is
>> > > that we're not indicating to gdb "this data is incomplete, you'll
>> > > need to ask for more of it" or something of that nature.
>> >
>> > The docs reference "bulk transfers":
>> >
>> >   ‘PacketSize=bytes’
>> >
>> >       The remote stub can accept packets up to at least bytes in length.
>> >       GDB will send packets up to this size for bulk transfers, and will
>> >       never send larger packets. This is a limit on the data characters
>> >       in the packet, not including the frame and checksum. There is no
>> >       trailing NUL byte in a remote protocol packet; if the stub stores
>> >       packets in a NUL-terminated format, it should allow an extra byte
>> >       in its buffer for the NUL. If this stub feature is not supported,
>> >       GDB guesses based on the size of the ‘g’ packet response.
>>
>> We do advertise this.
>>
>> -- PMM
>>
>

[-- Attachment #2: Type: text/html, Size: 9249 bytes --]

Re: [PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Peter Maydell]

On Wed, 6 Aug 2025 at 22:18, Vacha Bhavsar
<vacha.bhavsar@oss.qualcomm.com> wrote:
>
> Hi,
>
> I reached out to the gdb dev team regarding these questions
> on how to correctly handle the SME za storage. This discussion
> can be found here:
>
> https://sourceware.org/pipermail/gdb/2025-August/051858.html
> https://sourceware.org/pipermail/gdb/2025-August/051860.html
>
> It seems this approach of increasing the buffer size to above
> 131100 has also been applied to gdbserver as there currently
> isn't support for partial data transfers.

Thanks for chasing that up.

(1) I note that gdb bumped this to 131104, and we only
have 131100. We should check what the worst-case is and
make sure our number is big enough (maybe we count different
things in our buffer size than gdbstub does?)

(2) We should add a comment to MAX_PACKET_LENGTH, something
like:

/*
 * Most "large" transfers (e.g. memory reads, feature XML
 * transfer) have mechanisms in the gdb protocol for splitting
 * them. However, register values in particular cannot currently
 * be split. This packet size must therefore be at least big enough
 * for the worst-case register size. Currently that is Arm SME
 * ZA storage with a 256x256 byte value. We also must account
 * for the conversion from raw data to hex in gdb_memtohex(),
 * which writes 2*size + 1 bytes, and for other overhead like
 * the command itself or the checksum.
 */

(but ideally check the figures and be a bit less vague about
the "other overhead" :-))

-- PMM

Re: [PATCH v4 2/2] target/arm: Added support for SME register exposure to GDB

[Vacha Bhavsar]

[-- Attachment #1: Type: text/plain, Size: 9736 bytes --]

Hi,



I've built a testcase for this similar to the one you suggested. This test

checks both reading and writing the za register via $za and via the tiles

and tiles slices that gdb produces (i.e., za0hb0). However, these tiles and

slices are generated from the gdb side, they're not made available by any

of the changes that I have implemented. But this feature of gdb's kicks in

when using gdb14.1 or newer. Due to this, the testcase works correctly when

used with gdb14.1 and above, and fails on any gdb version older than that

as the tiles/slices are not made available by gdb.



I was wondering if there is any way to set a requirement on this testcase

which specifies it needs to be run with minimum of gdb version 14.1

which has the functionality to break down the ZA storage into tiles and

slices?





Thanks,

Vacha

On Mon, Aug 4, 2025 at 11:35 AM Alex Bennée <alex.bennee@linaro.org> wrote:

> Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
>
> > The QEMU GDB stub does not expose the ZA storage SME register to GDB via
> > the remote serial protocol, which can be a useful functionality to debug
> SME
> > code. To provide this functionality in Aarch64 target, this patch
> registers the
> > SME register set with the GDB stub. To do so, this patch implements the
> > aarch64_gdb_get_sme_reg() and aarch64_gdb_set_sme_reg() functions to
> > specify how to get and set the SME registers, and the
> > arm_gen_dynamic_smereg_feature() function to generate the target
> > description in XML format to indicate the target architecture supports
> SME.
> > Finally, this patch includes a dyn_smereg_feature structure to hold this
> > GDB XML description of the SME registers for each CPU.
> >
> > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> > ---
> > Changes since v3:
> > - added changes to aarch64_gdb_set_sme_reg() to address the concerns
> > brought up in review regarding endianness
> >
> >  target/arm/cpu.h       |   1 +
> >  target/arm/gdbstub.c   |   6 ++
> >  target/arm/gdbstub64.c | 122 +++++++++++++++++++++++++++++++++++++++++
> >  target/arm/internals.h |   3 +
> >  4 files changed, 132 insertions(+)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index dc9b6dce4c..8bd66d7049 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -933,6 +933,7 @@ struct ArchCPU {
> >
> >      DynamicGDBFeatureInfo dyn_sysreg_feature;
> >      DynamicGDBFeatureInfo dyn_svereg_feature;
> > +    DynamicGDBFeatureInfo dyn_smereg_feature;
> >      DynamicGDBFeatureInfo dyn_m_systemreg_feature;
> >      DynamicGDBFeatureInfo dyn_m_secextreg_feature;
> >
> > diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> > index ce4497ad7c..9c942c77cc 100644
> > --- a/target/arm/gdbstub.c
> > +++ b/target/arm/gdbstub.c
> > @@ -531,6 +531,12 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU
> *cpu)
> >              GDBFeature *feature = arm_gen_dynamic_svereg_feature(cs,
> cs->gdb_num_regs);
> >              gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
> >                                       aarch64_gdb_set_sve_reg, feature,
> 0);
> > +            if (isar_feature_aa64_sme(&cpu->isar)) {
> > +                GDBFeature *sme_feature =
> arm_gen_dynamic_smereg_feature(cs,
> > +                                             cs->gdb_num_regs);
> > +                gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
> > +                    aarch64_gdb_set_sme_reg, sme_feature, 0);
> > +            }
> >          } else {
> >              gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
> >                                       aarch64_gdb_set_fpu_reg,
> > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> > index 64ee9b3b56..3d86980bc9 100644
> > --- a/target/arm/gdbstub64.c
> > +++ b/target/arm/gdbstub64.c
> > @@ -228,6 +228,91 @@ int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t
> *buf, int reg)
> >      return 0;
> >  }
> >
> > +int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    CPUARMState *env = &cpu->env;
> > +
> > +    switch (reg) {
> > +    /* Svg register */
> > +    case 0:
> > +    {
> > +        int vq = 0;
> > +        if (FIELD_EX64(env->svcr, SVCR, SM)) {
> > +            vq = sve_vqm1_for_el_sm(env, arm_current_el(env),
> > +                     FIELD_EX64(env->svcr, SVCR, SM)) + 1;
> > +        }
> > +        /* svg = vector granules (2 * vector quardwords) in streaming
> mode */
> > +        return gdb_get_reg64(buf, vq * 2);
> > +    }
> > +    case 1:
> > +        return gdb_get_reg64(buf, env->svcr);
> > +    case 2:
> > +    {
> > +        int len = 0;
> > +        int vq = cpu->sme_max_vq;
> > +        int svl = vq * 16;
> > +        for (int i = 0; i < svl; i++) {
> > +            for (int q = 0; q < vq; q++) {
> > +                len += gdb_get_reg128(buf,
> > +                           env->za_state.za[i].d[q * 2 + 1],
> > +                           env->za_state.za[i].d[q * 2]);
> > +            }
> > +        }
> > +        return len;
> > +    }
> > +    default:
> > +        /* gdbstub asked for something out of range */
> > +        qemu_log_mask(LOG_UNIMP, "%s: out of range register %d",
> __func__, reg);
> > +        break;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    CPUARMState *env = &cpu->env;
> > +
> > +    switch (reg) {
> > +    case 0:
> > +    {
> > +        /* cannot set svg via gdbstub */
> > +        return 8;
> > +    }
> > +    case 1:
> > +        aarch64_set_svcr(env, ldq_le_p(buf),
> > +            R_SVCR_SM_MASK | R_SVCR_ZA_MASK);
> > +        return 8;
> > +    case 2:
> > +        int len = 0;
> > +        int vq = cpu->sme_max_vq;
> > +        int svl = vq * 16;
> > +        for (int i = 0; i < svl; i++) {
> > +            for (int q = 0; q < vq; q++) {
> > +                if (target_big_endian()){
> > +                    env->za_state.za[i].d[q * 2 + 1] = ldq_p(buf);
> > +                    buf += 8;
> > +                    env->za_state.za[i].d[q * 2] = ldq_p(buf);
> > +                } else{
> > +                    env->za_state.za[i].d[q * 2] = ldq_p(buf);
> > +                    buf += 8;
> > +                    env->za_state.za[i].d[q * 2 + 1] = ldq_p(buf);
> > +                }
> > +                buf += 8;
> > +                len += 16;
> > +            }
> > +        }
> > +        return len;
> > +    default:
> > +        /* gdbstub asked for something out of range */
> > +        break;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
> >  {
> >      ARMCPU *cpu = ARM_CPU(cs);
> > @@ -392,6 +477,43 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState
> *cs, int base_reg)
> >      return &cpu->dyn_svereg_feature.desc;
> >  }
> >
> > +GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cs, int base_reg)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    int vq = cpu->sme_max_vq;
> > +    int svl = vq * 16;
> > +    GDBFeatureBuilder builder;
> > +    int reg = 0;
> > +
> > +    gdb_feature_builder_init(&builder, &cpu->dyn_smereg_feature.desc,
> > +        "org.gnu.gdb.aarch64.sme", "sme-registers.xml", base_reg);
> > +
> > +
> > +    /* Create the sme_bv vector type. */
> > +    gdb_feature_builder_append_tag(&builder,
> > +        "<vector id=\"sme_bv\" type=\"uint8\" count=\"%d\"/>",
> > +        svl);
> > +
> > +    /* Create the sme_bvv vector type. */
> > +    gdb_feature_builder_append_tag(
> > +        &builder, "<vector id=\"sme_bvv\" type=\"sme_bv\"
> count=\"%d\"/>",
> > +        svl);
> > +
> > +    /* Define the svg, svcr, and za registers. */
> > +
> > +    /* fpscr & status registers */
> > +    gdb_feature_builder_append_reg(&builder, "svg", 64, reg++,
> > +        "int", NULL);
> > +    gdb_feature_builder_append_reg(&builder, "svcr", 64, reg++,
> > +        "int", NULL);
> > +    gdb_feature_builder_append_reg(&builder, "za", svl * svl * 8, reg++,
> > +        "sme_bvv", NULL);
> > +
> > +    gdb_feature_builder_end(&builder);
> > +
> > +    return &cpu->dyn_smereg_feature.desc;
> > +}
> > +
> >  #ifdef CONFIG_USER_ONLY
> >  int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
> >  {
> > diff --git a/target/arm/internals.h b/target/arm/internals.h
> > index c4765e4489..760e1c6490 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -1808,8 +1808,11 @@ static inline uint64_t
> pmu_counter_mask(CPUARMState *env)
> >  }
> >
> >  GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int base_reg);
> > +GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cpu, int base_reg);
> >  int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg);
> >  int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg);
> > +int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg);
> > +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg);
> >  int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg);
> >  int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg);
> >  int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg);
>
> It would also be nice to add a test for this, see
> tests/tcg/aarch64/gdbstub/test-sve.py
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

[-- Attachment #2: Type: text/html, Size: 12616 bytes --]

Re: [PATCH v4 2/2] target/arm: Added support for SME register exposure to GDB

[Alex Bennée]

Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:

> Hi,
>
>  
>
> I've built a testcase for this similar to the one you suggested. This test 
>
> checks both reading and writing the za register via $za and via the tiles
>
> and tiles slices that gdb produces (i.e., za0hb0). However, these tiles and 
>
> slices are generated from the gdb side, they're not made available by any
>
> of the changes that I have implemented. But this feature of gdb's kicks in
>
> when using gdb14.1 or newer. Due to this, the testcase works correctly when
>
> used with gdb14.1 and above, and fails on any gdb version older than that
>
> as the tiles/slices are not made available by gdb.
>
>  
>
> I was wondering if there is any way to set a requirement on this testcase
>
> which specifies it needs to be run with minimum of gdb version 14.1
>
> which has the functionality to break down the ZA storage into tiles and 
>
> slices?

We have tests in configure to probe the gdbversion, e.g:

  if test "${gdb_arches#*aarch64}" != "$gdb_arches" && version_ge $gdb_version 15.1; then
      echo "GDB_HAS_MTE=y" >> $config_target_mak
  fi

which can then wrap the test in the Makefile, e.g.:

  ifeq ($(GDB_HAS_MTE),y)
  run-gdbstub-mte: mte-8
          $(call run-test, $@, $(GDB_SCRIPT) \
                  --gdb $(GDB) \
                  --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
                  --bin $< --test $(AARCH64_SRC)/gdbstub/test-mte.py \
                  -- --mode=user, \
          gdbstub MTE support)

  EXTRA_RUNS += run-gdbstub-mte
  endif


>
>  
>
>  
>
> Thanks,
>
> Vacha
>
> On Mon, Aug 4, 2025 at 11:35 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
>
>  > The QEMU GDB stub does not expose the ZA storage SME register to GDB via
>  > the remote serial protocol, which can be a useful functionality to debug SME
>  > code. To provide this functionality in Aarch64 target, this patch registers the
>  > SME register set with the GDB stub. To do so, this patch implements the
>  > aarch64_gdb_get_sme_reg() and aarch64_gdb_set_sme_reg() functions to
>  > specify how to get and set the SME registers, and the
>  > arm_gen_dynamic_smereg_feature() function to generate the target
>  > description in XML format to indicate the target architecture supports SME.
>  > Finally, this patch includes a dyn_smereg_feature structure to hold this
>  > GDB XML description of the SME registers for each CPU.
>  >
>  > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
>  > ---
>  > Changes since v3:
>  > - added changes to aarch64_gdb_set_sme_reg() to address the concerns 
>  > brought up in review regarding endianness
>  >
>  >  target/arm/cpu.h       |   1 +
>  >  target/arm/gdbstub.c   |   6 ++
>  >  target/arm/gdbstub64.c | 122 +++++++++++++++++++++++++++++++++++++++++
>  >  target/arm/internals.h |   3 +
>  >  4 files changed, 132 insertions(+)
>  >
>  > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>  > index dc9b6dce4c..8bd66d7049 100644
>  > --- a/target/arm/cpu.h
>  > +++ b/target/arm/cpu.h
>  > @@ -933,6 +933,7 @@ struct ArchCPU {
>  >  
>  >      DynamicGDBFeatureInfo dyn_sysreg_feature;
>  >      DynamicGDBFeatureInfo dyn_svereg_feature;
>  > +    DynamicGDBFeatureInfo dyn_smereg_feature;
>  >      DynamicGDBFeatureInfo dyn_m_systemreg_feature;
>  >      DynamicGDBFeatureInfo dyn_m_secextreg_feature;
>  >  
>  > diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
>  > index ce4497ad7c..9c942c77cc 100644
>  > --- a/target/arm/gdbstub.c
>  > +++ b/target/arm/gdbstub.c
>  > @@ -531,6 +531,12 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>  >              GDBFeature *feature = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs);
>  >              gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
>  >                                       aarch64_gdb_set_sve_reg, feature, 0);
>  > +            if (isar_feature_aa64_sme(&cpu->isar)) {
>  > +                GDBFeature *sme_feature = arm_gen_dynamic_smereg_feature(cs,
>  > +                                             cs->gdb_num_regs);
>  > +                gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
>  > +                    aarch64_gdb_set_sme_reg, sme_feature, 0);
>  > +            }
>  >          } else {
>  >              gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
>  >                                       aarch64_gdb_set_fpu_reg,
>  > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
>  > index 64ee9b3b56..3d86980bc9 100644
>  > --- a/target/arm/gdbstub64.c
>  > +++ b/target/arm/gdbstub64.c
>  > @@ -228,6 +228,91 @@ int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg)
>  >      return 0;
>  >  }
>  >  
>  > +int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg)
>  > +{
>  > +    ARMCPU *cpu = ARM_CPU(cs);
>  > +    CPUARMState *env = &cpu->env;
>  > +
>  > +    switch (reg) {
>  > +    /* Svg register */
>  > +    case 0:
>  > +    {
>  > +        int vq = 0;
>  > +        if (FIELD_EX64(env->svcr, SVCR, SM)) {
>  > +            vq = sve_vqm1_for_el_sm(env, arm_current_el(env),
>  > +                     FIELD_EX64(env->svcr, SVCR, SM)) + 1;
>  > +        }
>  > +        /* svg = vector granules (2 * vector quardwords) in streaming mode */
>  > +        return gdb_get_reg64(buf, vq * 2);
>  > +    }
>  > +    case 1:
>  > +        return gdb_get_reg64(buf, env->svcr);
>  > +    case 2:
>  > +    {
>  > +        int len = 0;
>  > +        int vq = cpu->sme_max_vq;
>  > +        int svl = vq * 16;
>  > +        for (int i = 0; i < svl; i++) {
>  > +            for (int q = 0; q < vq; q++) {
>  > +                len += gdb_get_reg128(buf,
>  > +                           env->za_state.za⚠️[i].d[q * 2 + 1],
>  > +                           env->za_state.za⚠️[i].d[q * 2]);
>  > +            }
>  > +        }
>  > +        return len;
>  > +    }
>  > +    default:
>  > +        /* gdbstub asked for something out of range */
>  > +        qemu_log_mask(LOG_UNIMP, "%s: out of range register %d", __func__, reg);
>  > +        break;
>  > +    }
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg)
>  > +{
>  > +    ARMCPU *cpu = ARM_CPU(cs);
>  > +    CPUARMState *env = &cpu->env;
>  > +
>  > +    switch (reg) {
>  > +    case 0:
>  > +    {
>  > +        /* cannot set svg via gdbstub */
>  > +        return 8;
>  > +    }
>  > +    case 1:
>  > +        aarch64_set_svcr(env, ldq_le_p(buf),
>  > +            R_SVCR_SM_MASK | R_SVCR_ZA_MASK);
>  > +        return 8;
>  > +    case 2:
>  > +        int len = 0;
>  > +        int vq = cpu->sme_max_vq;
>  > +        int svl = vq * 16;
>  > +        for (int i = 0; i < svl; i++) {
>  > +            for (int q = 0; q < vq; q++) {
>  > +                if (target_big_endian()){
>  > +                    env->za_state.za⚠️[i].d[q * 2 + 1] = ldq_p(buf);
>  > +                    buf += 8;
>  > +                    env->za_state.za⚠️[i].d[q * 2] = ldq_p(buf);
>  > +                } else{
>  > +                    env->za_state.za⚠️[i].d[q * 2] = ldq_p(buf);
>  > +                    buf += 8;
>  > +                    env->za_state.za⚠️[i].d[q * 2 + 1] = ldq_p(buf);
>  > +                }
>  > +                buf += 8;
>  > +                len += 16;
>  > +            }
>  > +        }
>  > +        return len;
>  > +    default:
>  > +        /* gdbstub asked for something out of range */
>  > +        break;
>  > +    }
>  > +
>  > +    return 0;
>  > +}
>  > +
>  >  int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
>  >  {
>  >      ARMCPU *cpu = ARM_CPU(cs);
>  > @@ -392,6 +477,43 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg)
>  >      return &cpu->dyn_svereg_feature.desc;
>  >  }
>  >  
>  > +GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cs, int base_reg)
>  > +{
>  > +    ARMCPU *cpu = ARM_CPU(cs);
>  > +    int vq = cpu->sme_max_vq;
>  > +    int svl = vq * 16;
>  > +    GDBFeatureBuilder builder;
>  > +    int reg = 0;
>  > +
>  > +    gdb_feature_builder_init(&builder, &cpu->dyn_smereg_feature.desc,
>  > +        "org.gnu.gdb.aarch64.sme", "sme-registers.xml", base_reg);
>  > +
>  > +
>  > +    /* Create the sme_bv vector type. */
>  > +    gdb_feature_builder_append_tag(&builder,
>  > +        "<vector id=\"sme_bv\" type=\"uint8\" count=\"%d\"/>",
>  > +        svl);
>  > +
>  > +    /* Create the sme_bvv vector type. */
>  > +    gdb_feature_builder_append_tag(
>  > +        &builder, "<vector id=\"sme_bvv\" type=\"sme_bv\" count=\"%d\"/>",
>  > +        svl);
>  > +
>  > +    /* Define the svg, svcr, and za registers. */
>  > +
>  > +    /* fpscr & status registers */
>  > +    gdb_feature_builder_append_reg(&builder, "svg", 64, reg++,
>  > +        "int", NULL);
>  > +    gdb_feature_builder_append_reg(&builder, "svcr", 64, reg++,
>  > +        "int", NULL);
>  > +    gdb_feature_builder_append_reg(&builder, "za", svl * svl * 8, reg++,
>  > +        "sme_bvv", NULL);
>  > +
>  > +    gdb_feature_builder_end(&builder);
>  > +
>  > +    return &cpu->dyn_smereg_feature.desc;
>  > +}
>  > +
>  >  #ifdef CONFIG_USER_ONLY
>  >  int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int reg)
>  >  {
>  > diff --git a/target/arm/internals.h b/target/arm/internals.h
>  > index c4765e4489..760e1c6490 100644
>  > --- a/target/arm/internals.h
>  > +++ b/target/arm/internals.h
>  > @@ -1808,8 +1808,11 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
>  >  }
>  >  
>  >  GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int base_reg);
>  > +GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cpu, int base_reg);
>  >  int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg);
>  >  int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg);
>  > +int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg);
>  > +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg);
>  >  int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg);
>  >  int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg);
>  >  int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg);
>
>  It would also be nice to add a test for this, see tests/tcg/aarch64/gdbstub/test-sve.py
>
>  -- 
>  Alex Bennée
>  Virtualisation Tech Lead @ Linaro

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v4 1/2] target/arm: Increase MAX_PACKET_LENGTH for SME ZA remote gdb debugging

[Vacha Bhavsar]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

Hi,

From testing with a max vector length, the minimum size the buffer needs to
be is 131077 to survive the worst case of a P packet with the ZA register.
This is enough to fit the whole ZA register plus the overhead for the P
packet (command + register number, the checksum is not stored in line_buf)
and the null terminator. I had overshot and rounded it safely to 131100,
and it seems gdbserver has done the same thing but they specifically
counted 32 bytes for overhead, it's not specified why. So the 131100 is
large enough, however I have changed it to 131104 just to stay consistent
with gdbserver and have added this note to the comment you've suggested to
be placed above MAX_PACKET_LENGTH. I've also changed line_buf to be a
GString as Alex suggested.

I have sent a new version with these changes. Looking forward to your
feedback!

Thanks,
Vacha

[-- Attachment #2: Type: text/html, Size: 957 bytes --]

Re: [PATCH v4 2/2] target/arm: Added support for SME register exposure to GDB

[Vacha Bhavsar]

[-- Attachment #1: Type: text/plain, Size: 11514 bytes --]

Got it, thank you!

On Mon, Aug 11, 2025 at 3:58 AM Alex Bennée <alex.bennee@linaro.org> wrote:

> Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
>
> > Hi,
> >
> >
> >
> > I've built a testcase for this similar to the one you suggested. This
> test
> >
> > checks both reading and writing the za register via $za and via the tiles
> >
> > and tiles slices that gdb produces (i.e., za0hb0). However, these tiles
> and
> >
> > slices are generated from the gdb side, they're not made available by any
> >
> > of the changes that I have implemented. But this feature of gdb's kicks
> in
> >
> > when using gdb14.1 or newer. Due to this, the testcase works correctly
> when
> >
> > used with gdb14.1 and above, and fails on any gdb version older than that
> >
> > as the tiles/slices are not made available by gdb.
> >
> >
> >
> > I was wondering if there is any way to set a requirement on this testcase
> >
> > which specifies it needs to be run with minimum of gdb version 14.1
> >
> > which has the functionality to break down the ZA storage into tiles and
> >
> > slices?
>
> We have tests in configure to probe the gdbversion, e.g:
>
>   if test "${gdb_arches#*aarch64}" != "$gdb_arches" && version_ge
> $gdb_version 15.1; then
>       echo "GDB_HAS_MTE=y" >> $config_target_mak
>   fi
>
> which can then wrap the test in the Makefile, e.g.:
>
>   ifeq ($(GDB_HAS_MTE),y)
>   run-gdbstub-mte: mte-8
>           $(call run-test, $@, $(GDB_SCRIPT) \
>                   --gdb $(GDB) \
>                   --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>                   --bin $< --test $(AARCH64_SRC)/gdbstub/test-mte.py \
>                   -- --mode=user, \
>           gdbstub MTE support)
>
>   EXTRA_RUNS += run-gdbstub-mte
>   endif
>
>
> >
> >
> >
> >
> >
> > Thanks,
> >
> > Vacha
> >
> > On Mon, Aug 4, 2025 at 11:35 AM Alex Bennée <alex.bennee@linaro.org>
> wrote:
> >
> >  Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com> writes:
> >
> >  > The QEMU GDB stub does not expose the ZA storage SME register to GDB
> via
> >  > the remote serial protocol, which can be a useful functionality to
> debug SME
> >  > code. To provide this functionality in Aarch64 target, this patch
> registers the
> >  > SME register set with the GDB stub. To do so, this patch implements
> the
> >  > aarch64_gdb_get_sme_reg() and aarch64_gdb_set_sme_reg() functions to
> >  > specify how to get and set the SME registers, and the
> >  > arm_gen_dynamic_smereg_feature() function to generate the target
> >  > description in XML format to indicate the target architecture
> supports SME.
> >  > Finally, this patch includes a dyn_smereg_feature structure to hold
> this
> >  > GDB XML description of the SME registers for each CPU.
> >  >
> >  > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> >  > ---
> >  > Changes since v3:
> >  > - added changes to aarch64_gdb_set_sme_reg() to address the concerns
> >  > brought up in review regarding endianness
> >  >
> >  >  target/arm/cpu.h       |   1 +
> >  >  target/arm/gdbstub.c   |   6 ++
> >  >  target/arm/gdbstub64.c | 122
> +++++++++++++++++++++++++++++++++++++++++
> >  >  target/arm/internals.h |   3 +
> >  >  4 files changed, 132 insertions(+)
> >  >
> >  > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >  > index dc9b6dce4c..8bd66d7049 100644
> >  > --- a/target/arm/cpu.h
> >  > +++ b/target/arm/cpu.h
> >  > @@ -933,6 +933,7 @@ struct ArchCPU {
> >  >
> >  >      DynamicGDBFeatureInfo dyn_sysreg_feature;
> >  >      DynamicGDBFeatureInfo dyn_svereg_feature;
> >  > +    DynamicGDBFeatureInfo dyn_smereg_feature;
> >  >      DynamicGDBFeatureInfo dyn_m_systemreg_feature;
> >  >      DynamicGDBFeatureInfo dyn_m_secextreg_feature;
> >  >
> >  > diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> >  > index ce4497ad7c..9c942c77cc 100644
> >  > --- a/target/arm/gdbstub.c
> >  > +++ b/target/arm/gdbstub.c
> >  > @@ -531,6 +531,12 @@ void
> arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
> >  >              GDBFeature *feature = arm_gen_dynamic_svereg_feature(cs,
> cs->gdb_num_regs);
> >  >              gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
> >  >                                       aarch64_gdb_set_sve_reg,
> feature, 0);
> >  > +            if (isar_feature_aa64_sme(&cpu->isar)) {
> >  > +                GDBFeature *sme_feature =
> arm_gen_dynamic_smereg_feature(cs,
> >  > +                                             cs->gdb_num_regs);
> >  > +                gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
> >  > +                    aarch64_gdb_set_sme_reg, sme_feature, 0);
> >  > +            }
> >  >          } else {
> >  >              gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
> >  >                                       aarch64_gdb_set_fpu_reg,
> >  > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> >  > index 64ee9b3b56..3d86980bc9 100644
> >  > --- a/target/arm/gdbstub64.c
> >  > +++ b/target/arm/gdbstub64.c
> >  > @@ -228,6 +228,91 @@ int aarch64_gdb_set_sve_reg(CPUState *cs,
> uint8_t *buf, int reg)
> >  >      return 0;
> >  >  }
> >  >
> >  > +int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg)
> >  > +{
> >  > +    ARMCPU *cpu = ARM_CPU(cs);
> >  > +    CPUARMState *env = &cpu->env;
> >  > +
> >  > +    switch (reg) {
> >  > +    /* Svg register */
> >  > +    case 0:
> >  > +    {
> >  > +        int vq = 0;
> >  > +        if (FIELD_EX64(env->svcr, SVCR, SM)) {
> >  > +            vq = sve_vqm1_for_el_sm(env, arm_current_el(env),
> >  > +                     FIELD_EX64(env->svcr, SVCR, SM)) + 1;
> >  > +        }
> >  > +        /* svg = vector granules (2 * vector quardwords) in
> streaming mode */
> >  > +        return gdb_get_reg64(buf, vq * 2);
> >  > +    }
> >  > +    case 1:
> >  > +        return gdb_get_reg64(buf, env->svcr);
> >  > +    case 2:
> >  > +    {
> >  > +        int len = 0;
> >  > +        int vq = cpu->sme_max_vq;
> >  > +        int svl = vq * 16;
> >  > +        for (int i = 0; i < svl; i++) {
> >  > +            for (int q = 0; q < vq; q++) {
> >  > +                len += gdb_get_reg128(buf,
> >  > +                           env->za_state.za⚠️[i].d[q * 2 + 1],
> >  > +                           env->za_state.za⚠️[i].d[q * 2]);
> >  > +            }
> >  > +        }
> >  > +        return len;
> >  > +    }
> >  > +    default:
> >  > +        /* gdbstub asked for something out of range */
> >  > +        qemu_log_mask(LOG_UNIMP, "%s: out of range register %d",
> __func__, reg);
> >  > +        break;
> >  > +    }
> >  > +
> >  > +    return 0;
> >  > +}
> >  > +
> >  > +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg)
> >  > +{
> >  > +    ARMCPU *cpu = ARM_CPU(cs);
> >  > +    CPUARMState *env = &cpu->env;
> >  > +
> >  > +    switch (reg) {
> >  > +    case 0:
> >  > +    {
> >  > +        /* cannot set svg via gdbstub */
> >  > +        return 8;
> >  > +    }
> >  > +    case 1:
> >  > +        aarch64_set_svcr(env, ldq_le_p(buf),
> >  > +            R_SVCR_SM_MASK | R_SVCR_ZA_MASK);
> >  > +        return 8;
> >  > +    case 2:
> >  > +        int len = 0;
> >  > +        int vq = cpu->sme_max_vq;
> >  > +        int svl = vq * 16;
> >  > +        for (int i = 0; i < svl; i++) {
> >  > +            for (int q = 0; q < vq; q++) {
> >  > +                if (target_big_endian()){
> >  > +                    env->za_state.za⚠️[i].d[q * 2 + 1] = ldq_p(buf);
> >  > +                    buf += 8;
> >  > +                    env->za_state.za⚠️[i].d[q * 2] = ldq_p(buf);
> >  > +                } else{
> >  > +                    env->za_state.za⚠️[i].d[q * 2] = ldq_p(buf);
> >  > +                    buf += 8;
> >  > +                    env->za_state.za⚠️[i].d[q * 2 + 1] = ldq_p(buf);
> >  > +                }
> >  > +                buf += 8;
> >  > +                len += 16;
> >  > +            }
> >  > +        }
> >  > +        return len;
> >  > +    default:
> >  > +        /* gdbstub asked for something out of range */
> >  > +        break;
> >  > +    }
> >  > +
> >  > +    return 0;
> >  > +}
> >  > +
> >  >  int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int reg)
> >  >  {
> >  >      ARMCPU *cpu = ARM_CPU(cs);
> >  > @@ -392,6 +477,43 @@ GDBFeature
> *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg)
> >  >      return &cpu->dyn_svereg_feature.desc;
> >  >  }
> >  >
> >  > +GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cs, int
> base_reg)
> >  > +{
> >  > +    ARMCPU *cpu = ARM_CPU(cs);
> >  > +    int vq = cpu->sme_max_vq;
> >  > +    int svl = vq * 16;
> >  > +    GDBFeatureBuilder builder;
> >  > +    int reg = 0;
> >  > +
> >  > +    gdb_feature_builder_init(&builder, &cpu->dyn_smereg_feature.desc,
> >  > +        "org.gnu.gdb.aarch64.sme", "sme-registers.xml", base_reg);
> >  > +
> >  > +
> >  > +    /* Create the sme_bv vector type. */
> >  > +    gdb_feature_builder_append_tag(&builder,
> >  > +        "<vector id=\"sme_bv\" type=\"uint8\" count=\"%d\"/>",
> >  > +        svl);
> >  > +
> >  > +    /* Create the sme_bvv vector type. */
> >  > +    gdb_feature_builder_append_tag(
> >  > +        &builder, "<vector id=\"sme_bvv\" type=\"sme_bv\"
> count=\"%d\"/>",
> >  > +        svl);
> >  > +
> >  > +    /* Define the svg, svcr, and za registers. */
> >  > +
> >  > +    /* fpscr & status registers */
> >  > +    gdb_feature_builder_append_reg(&builder, "svg", 64, reg++,
> >  > +        "int", NULL);
> >  > +    gdb_feature_builder_append_reg(&builder, "svcr", 64, reg++,
> >  > +        "int", NULL);
> >  > +    gdb_feature_builder_append_reg(&builder, "za", svl * svl * 8,
> reg++,
> >  > +        "sme_bvv", NULL);
> >  > +
> >  > +    gdb_feature_builder_end(&builder);
> >  > +
> >  > +    return &cpu->dyn_smereg_feature.desc;
> >  > +}
> >  > +
> >  >  #ifdef CONFIG_USER_ONLY
> >  >  int aarch64_gdb_get_tag_ctl_reg(CPUState *cs, GByteArray *buf, int
> reg)
> >  >  {
> >  > diff --git a/target/arm/internals.h b/target/arm/internals.h
> >  > index c4765e4489..760e1c6490 100644
> >  > --- a/target/arm/internals.h
> >  > +++ b/target/arm/internals.h
> >  > @@ -1808,8 +1808,11 @@ static inline uint64_t
> pmu_counter_mask(CPUARMState *env)
> >  >  }
> >  >
> >  >  GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int
> base_reg);
> >  > +GDBFeature *arm_gen_dynamic_smereg_feature(CPUState *cpu, int
> base_reg);
> >  >  int aarch64_gdb_get_sve_reg(CPUState *cs, GByteArray *buf, int reg);
> >  >  int aarch64_gdb_set_sve_reg(CPUState *cs, uint8_t *buf, int reg);
> >  > +int aarch64_gdb_get_sme_reg(CPUState *cs, GByteArray *buf, int reg);
> >  > +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg);
> >  >  int aarch64_gdb_get_fpu_reg(CPUState *cs, GByteArray *buf, int reg);
> >  >  int aarch64_gdb_set_fpu_reg(CPUState *cs, uint8_t *buf, int reg);
> >  >  int aarch64_gdb_get_pauth_reg(CPUState *cs, GByteArray *buf, int
> reg);
> >
> >  It would also be nice to add a test for this, see
> tests/tcg/aarch64/gdbstub/test-sve.py
> >
> >  --
> >  Alex Bennée
> >  Virtualisation Tech Lead @ Linaro
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

[-- Attachment #2: Type: text/html, Size: 15642 bytes --]