Hi,
Regarding having the SME check inside the "if we have SVE" check, we were
looking the the
Arm ARM, specifically the following excerpt from section A1.4:
The architecture provides the following:
- A general-purpose register file.
- A SIMD&FP register file.
- If FEAT_SVE or FEAT_SME is implemented, an SVE scalable vector register
file and an
SVE scalable predicate register file.
- if FEAT_SME is implemented, the scalable ZA storage.
Based on this, we were considering the following update to the change in
gdbstub64.c and
we wanted to get your input.
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);
} else {
gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
aarch64_gdb_set_fpu_reg,
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);
gdb_register_coprocessor(cs, aarch64_gdb_get_sme_reg,
aarch64_gdb_set_sme_reg, sme_feature, 0);
}
Thanks,
Vacha
On Tue, Aug 19, 2025 at 5:05 AM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Mon, 11 Aug 2025 at 20:37, Vacha Bhavsar
> <vacha.bhav...@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.bhav...@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..4371a367a0 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);
> > + }
>
> It's possible to have SME without SVE, so this should not be
> inside the "if we have SVE" check.
>
> > } 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 08e2858539..047b1f8133 100644
> > --- a/target/arm/gdbstub64.c
> > +++ b/target/arm/gdbstub64.c
> > @@ -249,6 +249,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:
>
> We should comment what all the cases here are; the usual ploce to
> put such a comment is either on the same line as the case, or else
> on the line immediately after the case if the comment would be
> too long to put on the same line.
>
> The capitalization here is odd, too: the register is either "SVG" or "svg".
>
> > + {
> > + 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;
> > + }
>
> You don't need braces here, because there are no local variables
> in this arm of the switch statement.
>
> > + 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;
>
> But you do need braces here, because there are local variables.
> This is (a) because our coding style requires that declarations
> go at the top of a block and (b) because clang will complain:
>
> ../../target/arm/gdbstub64.c:310:9: error: label followed by a
> declaration is a C23 extension [-Werror,-Wc23-extensions]
> 310 | int len = 0;
> | ^
>
> > + for (int i = 0; i < svl; i++) {
> > + for (int q = 0; q < vq; q++) {
> > + if (target_big_endian()){
>
> Missing space before the {
>
> > + 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 +498,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 1b3d0244fd..41e05066b9 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -1802,8 +1802,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);
> > --
>
> thanks
> -- PMM
>
