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

[Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>]

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

Hi,

Regarding the definition of ZA as a 'vector of bytes' in the gdb
documentation, the choice that we have made in representing it
as a vector of vectors of bytes is based on the xml retrieved by
the native gdb client when run on a host with SME capabilities.

Is it sufficient to document this discrepancy in the commit message?

Thanks,
Vacha



On Tue, Sep 2, 2025 at 6:45 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 26 Aug 2025 at 19:50, Vacha Bhavsar
> <vacha.bhavsar@oss.qualcomm.com> 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.
> >
> > Signed-off-by: Vacha Bhavsar <vacha.bhavsar@oss.qualcomm.com>
> > ---
> > Changes since v5:
> > - added code to handle the case when we have SME without SVE
> > - added comments to indicate th cases in aarch64_gdb_get/set_sme_reg
> > - added/removed braces where necessary
> > - corrected capitalization in comments
> > ---
> >  target/arm/cpu.h       |   1 +
> >  target/arm/gdbstub.c   |   9 ++-
> >  target/arm/gdbstub64.c | 121 +++++++++++++++++++++++++++++++++++++++++
> >  target/arm/internals.h |   3 +
> >  4 files changed, 133 insertions(+), 1 deletion(-)
> >
> > 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..110258ec18 100644
> > --- a/target/arm/gdbstub.c
> > +++ b/target/arm/gdbstub.c
> > @@ -527,7 +527,7 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU
> *cpu)
> >           * registers so we don't need to include both.
> >           */
> >  #ifdef TARGET_AARCH64
> > -        if (isar_feature_aa64_sve(&cpu->isar)) {
> > +        if (isar_feature_aa64_sve(&cpu->isar) ||
> isar_feature_aa64_sme(&cpu->isar)) {
> >              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);
> > @@ -537,6 +537,13 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU
> *cpu)
> >
>  gdb_find_static_feature("aarch64-fpu.xml"),
> >                                       0);
> >          }
> > +
> > +        if (isar_feature_aa64_sme(&cpu->isar)) {
> > +            GDBFeature *sme_feature = arm_gen_dynamic_smereg_feature(cs,
> > +                cs->gdb_num_regs);
>
> Your indent here and below for function calls on multiple
> lines is wrong -- follow the way the existing code does it,
> where the second line lines up with the first argument
> after the '('.  (We sometimes make an exception where the wrapping
> would look terrible, but this is the usual approach.)
>
> > +            gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
> > +                aarch64_gdb_set_sme_reg, sme_feature, 0);
> > +        }
> >          /*
> >           * Note that we report pauth information via the feature name
> >           * org.gnu.gdb.aarch64.pauth_v2, not org.gnu.gdb.aarch64.pauth.
> > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> > index 08e2858539..d3fd94b93d 100644
> > --- a/target/arm/gdbstub64.c
> > +++ b/target/arm/gdbstub64.c
> > @@ -249,6 +249,90 @@ 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) {
> > +    case 0: /* svg register */
> > +    {
> > +        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: /* svcr register */
> > +        return gdb_get_reg64(buf, env->svcr);
> > +    case 2: /* za register */
> > +    {
> > +        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: /* svg register */
> > +        /* cannot set svg via gdbstub */
> > +        return 8;
> > +    case 1: /* svcr register */
> > +        aarch64_set_svcr(env, ldq_le_p(buf),
> > +            R_SVCR_SM_MASK | R_SVCR_ZA_MASK);
> > +        return 8;
> > +    case 2: /* za register */
> > +    {
> > +        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);
> > @@ -413,6 +497,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);
>
>
> https://sourceware.org/gdb/current/onlinedocs/gdb.html/AArch64-Features.html#AArch64-Features
>
> says ZA should be a vector of bytes, not a vector of a vector of bytes.
> Is it wrong ?
>
> > +
> > +    /* Define the svg, svcr, and za registers. */
> > +
> > +    /* fpscr & status registers */
>
> This comment seems to be wrong and can be deleted.
>
> > +    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);
>
> We will also want to have support for the org.gnu.gdb.aarch64.sme2
> feature (which has the ZT0 register), but we can add that as
> a separate patch later.
>
> > +
> > +    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)
> >  {
>
> thanks
> -- PMM
>

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