Hi Peter, Thank you for the feedback! I have sent over a revised version of this patch with appropriate changes. I have removed the unnecessary comments regarding the zregs, and also corrected the typo, "out our" to "out of" in the comments. Additionally, I have used aarch64_set_svcr to correctly set the 64 bit value of SVCR.
Regarding your question on whether the code handling the reading of the ZA storage would work on a big-endian host, I believe it would. I think this also applies to the pre-existing SVE code as well, based off of what I have read from this GDB documentation ( https://sourceware.org/gdb/download/onlinedocs/gdb#Packets) which states the following: ‘P n…=r…’ Write register n… with value r…. The register number n is in hexadecimal, and r… contains two hex digits for each byte in the register (target byte order). >From this, I believe the QEMU GDB stub is already expecting the register content in target byte order, leaving this responsibility to the client. Based on this, I believe the pre-existing SVE code and my SME code should correctly work on big-endian hosts. Looking forward to your feedback! Thanks, Vacha On Mon, Jul 14, 2025 at 6:30 AM Peter Maydell <peter.mayd...@linaro.org> wrote: > On Tue, 8 Jul 2025 at 23:14, 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> > > > > > +int aarch64_gdb_set_sme_reg(CPUState *cs, uint8_t *buf, int reg) > > +{ > > + ARMCPU *cpu = ARM_CPU(cs); > > + CPUARMState *env = &cpu->env; > > + > > + /* The first 32 registers are the zregs */ > > + switch (reg) { > > + /* The first 32 registers are the zregs */ > > You don't need the same comment twice, and it also doesn't > look like it makes sense here, because the zregs are > SVE regs, not SME regs. > > > + case 0: > > + { > > + /* cannot set svg via gdbstub */ > > + return 0; > > + } > > + case 1: > > + env->svcr = *buf & 0x03; > > This will update env->svcr but won't have the correct effects > on the CPU (e.g. zeroing the ZA storage); I think you need to > call aarch64_set_svcr() here. Also you've declared > SVCR in the XML as a 64-bit register, so you should read it out > of the buffer as a 64-bit value, not short-cut by reading just > one byte. > > > + return 8; > > + case 2: > > + int vq, len = 0; > > + int svl = cpu->sve_max_vq * 16; > > + uint64_t *p = (uint64_t *) buf; > > I know this is what the existing SVE code does, but does > this really do the right thing on a big-endian host ? > > > + for (int i = 0; i < svl; i++) { > > + for (vq = 0; vq < cpu->sve_max_vq; vq++) { > > + env->za_state.za[i].d[vq * 2 + 1] = *p++; > > + env->za_state.za[i].d[vq * 2] = *p++; > > + len += 16; > > + } > > + } > > + return len; > > + default: > > + /* gdbstub asked for something out our range */ > > "out of" > > > + break; > > + } > > + > > + return 0; > > +} > > (PS: I would consider this close enough to being a bugfix to be > OK with putting it into 10.1 in the first rc cycle or so even > if it misses softfreeze.) > > thanks > -- PMM >
