Hi,
Thank you for the feedback! Regarding the following: >So the only difference between these two branches is that we are >checking "int(v) == MAGIC" rather than "v == MAGIC" ? >Is this a "one GDB only works one way, and the other GDB only >works the other way" case? Or is there a real interesting thing >we'd like to test involving the cast ? Yes, the only difference between the two branches is the presence of the int cast. This seems to be an issue limited to specific versions of gdb. This has been discussed with the gdb team (https://sourceware.org/pipermail/gdb/2025-August/051868.html, https://sourceware.org/pipermail/gdb/2025-August/051878.html) and a bug has been filed. With additional tests I have found that the int cast causes no issues with the testcase when running gdb16.1 or newer. Other than this issue there is no intention on our end of testing anything interesting regarding casting as the int cast was included to stay aligned with the existing SVE test. I have sent a new patch version in which we have opted to go with your suggestion of reporting the test as SKIPPED when the detected gdb version does not allow for int casting of integers greater than 8 bytes (or does not have SME tile support). To accomplish this, we have split the test into 3: - one to test basic access to za (always run regardless of gdb version) - one to test access to doubleword tile slices (run only with gdb14.1 or newer) - one to test access to quadword tile slices (run only with gdb16.1 or newer) I have also restructured the test file to make the code more compact and added additional comments. Looking forward to your input on this approach. Thanks, Vacha On Tue, Sep 2, 2025 at 6:29 AM Peter Maydell <peter.mayd...@linaro.org> wrote: > On Tue, 26 Aug 2025 at 19:45, Vacha Bhavsar > <vacha.bhav...@oss.qualcomm.com> wrote: > > > > This patch adds a test case to test SME register exposure to > > a remote gdb debugging session. This test simply sets and > > reads SME registers. > > > > Signed-off-by: Vacha Bhavsar <vacha.bhav...@oss.qualcomm.com> > > --- > > Changes since v5: > > - added copyright and SPDX line > > - added functionality to avoid casting a gdb.Value object > > to int when testing the za quadwords to address bug found > > during review, this change is declared to users via a > > warning message included in the test results file > > run-gdbstub-sysregs-sme.out > > --- > > configure | 11 ++ > > tests/tcg/aarch64/Makefile.target | 33 +++++- > > tests/tcg/aarch64/gdbstub/test-sme.py | 165 ++++++++++++++++++++++++++ > > 3 files changed, 208 insertions(+), 1 deletion(-) > > create mode 100644 tests/tcg/aarch64/gdbstub/test-sme.py > > > > diff --git a/configure b/configure > > index 274a778764..9e2ae174dc 100755 > > --- a/configure > > +++ b/configure > > @@ -1839,6 +1839,17 @@ for target in $target_list; do > > echo "GDB=$gdb_bin" >> $config_target_mak > > fi > > > > + if test "${gdb_arches#*$arch}" != "$gdb_arches" && version_ge > $gdb_version 14.1; then > > + echo "GDB_HAS_SME_TILES=y" >> $config_target_mak > > + if test "$gdb_version" = "15.0.50.20240403-git"; then > > + echo "GDB_HAS_INT_CAST_SUPPORT=n" >> $config_target_mak > > + else > > + echo "GDB_HAS_INT_CAST_SUPPORT=y" >> $config_target_mak > > + fi > > + else > > + echo "GDB_HAS_SME_TILES=n" >> $config_target_mak > > + fi > > + > > if test "${gdb_arches#*aarch64}" != "$gdb_arches" && version_ge > $gdb_version 15.1; then > > echo "GDB_HAS_MTE=y" >> $config_target_mak > > fi > > diff --git a/tests/tcg/aarch64/Makefile.target > b/tests/tcg/aarch64/Makefile.target > > index 16ddcf4f88..f9304d29cf 100644 > > --- a/tests/tcg/aarch64/Makefile.target > > +++ b/tests/tcg/aarch64/Makefile.target > > @@ -132,7 +132,38 @@ run-gdbstub-sve-ioctls: sve-ioctls > > --bin $< --test > $(AARCH64_SRC)/gdbstub/test-sve-ioctl.py, \ > > basic gdbstub SVE ZLEN support) > > > > -EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls > > +ifneq ($(CROSS_AS_HAS_ARMV9_SME),) > > +# SME gdbstub test > > +ifeq ($(GDB_HAS_SME_TILES),y) > > +ifeq ($(GDB_HAS_INT_CAST_SUPPORT),y) > > +run-gdbstub-sysregs-sme: sysregs > > + $(call run-test, $@, $(GDB_SCRIPT) \ > > + --gdb $(GDB) \ > > + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ > > + --bin $< --test $(AARCH64_SRC)/gdbstub/test-sme.py \ > > + -- test_sme --gdb_sme_tile_support > --gdb_int_cast_support, \ > > + basic gdbstub SME support) > > +else > > +run-gdbstub-sysregs-sme: sysregs > > + $(call run-test, $@, $(GDB_SCRIPT) \ > > + --gdb $(GDB) \ > > + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ > > + --bin $< --test $(AARCH64_SRC)/gdbstub/test-sme.py \ > > + -- test_sme --gdb_sme_tile_support, \ > > + basic gdbstub SME support) > > +endif > > +else > > +run-gdbstub-sysregs-sme: sysregs > > + $(call run-test, $@, $(GDB_SCRIPT) \ > > + --gdb $(GDB) \ > > + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ > > + --bin $< --test $(AARCH64_SRC)/gdbstub/test-sme.py, \ > > + basic gdbstub SME support) > > + > > +endif > > +endif > > + > > +EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls > run-gdbstub-sysregs-sme > > > > ifeq ($(GDB_HAS_MTE),y) > > run-gdbstub-mte: mte-8 > > diff --git a/tests/tcg/aarch64/gdbstub/test-sme.py > b/tests/tcg/aarch64/gdbstub/test-sme.py > > new file mode 100644 > > index 0000000000..e27a37631b > > --- /dev/null > > +++ b/tests/tcg/aarch64/gdbstub/test-sme.py > > @@ -0,0 +1,165 @@ > > +# > > +# Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. > > +# > > +# SPDX-License-Identifier: GPL-2.0-or-later > > + > > +from __future__ import print_function > > +# > > +# Test the SME registers are visible and changeable via gdbstub > > +# > > +# This is launched via tests/guest-debug/run-test.py > > +# > > + > > +import argparse > > +import gdb > > +from test_gdbstub import main, report > > + > > +MAGIC = 0x01020304 > > +INT_CAST_SUPPORT = 0 > > + > > +def run_test(): > > + "Run through the tests one by one" > > This is a very unhelpful name and comment for this function. > > > + > > + frame = gdb.selected_frame() > > + rname = "za" > > + za = frame.read_register(rname) > > + report(True, "Reading %s" % rname) > > + > > + for i in range(0, 16): > > + for j in range(0, 16): > > + cmd = "set $za[%d][%d] = 0x01" % (i, j) > > + gdb.execute(cmd) > > + report(True, "%s" % cmd) > > + for i in range(0, 16): > > + for j in range(0, 16): > > + reg = "$za[%d][%d]" % (i, j) > > + v = gdb.parse_and_eval(reg) > > + report(str(v.type) == "uint8_t", > > + "size of %s" % (reg)) > > + report(int(v) == 0x1, "%s is 0x%x" % (reg, 0x1)) > > + > > +def run_test_slices(): > > + "Run through the tests one by one" > > + > > + frame = gdb.selected_frame() > > + rname = "za" > > + za = frame.read_register(rname) > > + report(True, "Reading %s" % rname) > > + > > + for i in range(0, 16): > > + for j in range(0, 16): > > + cmd = "set $za[%d][%d] = 0x01" % (i, j) > > + gdb.execute(cmd) > > + report(True, "%s" % cmd) > > + for i in range(0, 16): > > + for j in range(0, 16): > > + reg = "$za[%d][%d]" % (i, j) > > + v = gdb.parse_and_eval(reg) > > + report(str(v.type) == "uint8_t", > > + "size of %s" % (reg)) > > + report(int(v) == 0x1, "%s is 0x%x" % (reg, 0x1)) > > This first part is exactly the same as the run_test() > function is testing, isn't it? > > > + > > + if INT_CAST_SUPPORT: > > + for i in range(0, 4): > > + for j in range(0, 4): > > + for k in range(0, 4): > > + cmd = "set $za%dhq%d[%d] = 0x%x" % (i, j, k, MAGIC) > > + gdb.execute(cmd) > > + report(True, "%s" % cmd) > > + for j in range(0, 4): > > + for k in range(0, 4): > > + reg = "$za%dhq%d[%d]" % (i, j, k) > > + v = gdb.parse_and_eval(reg) > > + report(str(v.type) == "uint128_t", > > + "size of %s" % (reg)) > > + report(int(v) == MAGIC, "%s is 0x%x" % (reg, MAGIC)) > > + > > + for j in range(0, 4): > > + for k in range(0, 4): > > + cmd = "set $za%dvq%d[%d] = 0x%x" % (i, j, k, MAGIC) > > + gdb.execute(cmd) > > + report(True, "%s" % cmd) > > + for j in range(0, 4): > > + for k in range(0, 4): > > + reg = "$za%dvq%d[%d]" % (i, j, k) > > + v = gdb.parse_and_eval(reg) > > + report(str(v.type) == "uint128_t", > > + "size of %s" % (reg)) > > + report(int(v) == MAGIC, "%s is 0x%x" % (reg, MAGIC)) > > + > > + else: > > + for i in range(0, 4): > > + for j in range(0, 4): > > + for k in range(0, 4): > > + cmd = "set $za%dhq%d[%d] = 0x%x" % (i, j, k, MAGIC) > > + gdb.execute(cmd) > > + report(True, "%s" % cmd) > > + for j in range(0, 4): > > + for k in range(0, 4): > > + reg = "$za%dhq%d[%d]" % (i, j, k) > > + v = gdb.parse_and_eval(reg) > > + report(str(v.type) == "uint128_t", > > + "size of %s" % (reg)) > > + report(v == MAGIC, "%s is 0x%x" % (reg, MAGIC)) > > So the only difference between these two branches is that we are > checking "int(v) == MAGIC" rather than "v == MAGIC" ? > > Is this a "one GDB only works one way, and the other GDB only > works the other way" case? Or is there a real interesting thing > we'd like to test involving the cast ? > > Either way, this code is way too repetitive. Don't copy-and-paste > like this, it's very hard to review. > > > + > > + for j in range(0, 4): > > + for k in range(0, 4): > > + cmd = "set $za%dvq%d[%d] = 0x%x" % (i, j, k, MAGIC) > > + gdb.execute(cmd) > > + report(True, "%s" % cmd) > > + for j in range(0, 4): > > + for k in range(0, 4): > > + reg = "$za%dvq%d[%d]" % (i, j, k) > > + v = gdb.parse_and_eval(reg) > > + report(str(v.type) == "uint128_t", > > + "size of %s" % (reg)) > > + report(v == MAGIC, "%s is 0x%x" % (reg, MAGIC)) > > + > > + for i in range(0, 4): > > + for j in range(0, 4): > > + for k in range(0, 4): > > + cmd = "set $za%dhd%d[%d] = 0x%x" % (i, j, k, MAGIC) > > + gdb.execute(cmd) > > + report(True, "%s" % cmd) > > + for j in range(0, 4): > > + for k in range(0, 4): > > + reg = "$za%dhd%d[%d]" % (i, j, k) > > + v = gdb.parse_and_eval(reg) > > + report(str(v.type) == "uint64_t", > > + "size of %s" % (reg)) > > + report(int(v) == MAGIC, "%s is 0x%x" % (reg, MAGIC)) > > + > > + for j in range(0, 4): > > + for k in range(0, 4): > > + cmd = "set $za%dvd%d[%d] = 0x%x" % (i, j, k, MAGIC) > > + gdb.execute(cmd) > > + report(True, "%s" % cmd) > > + for j in range(0, 4): > > + for k in range(0, 4): > > + reg = "$za%dvd%d[%d]" % (i, j, k) > > + v = gdb.parse_and_eval(reg) > > + report(str(v.type) == "uint64_t", > > + "size of %s" % (reg)) > > + report(int(v) == MAGIC, "%s is 0x%x" % (reg, MAGIC)) > > + > > + > > +parser = argparse.ArgumentParser(description="A gdbstub test for SME > support") > > +parser.add_argument("--gdb_sme_tile_support", help="GDB support for SME > tiles", \ > > + action="store_true") > > +parser.add_argument("--gdb_int_cast_support", > > + help="GDB support for 128bit int cast", \ > > + action="store_true") > > +args = parser.parse_args() > > + > > +if args.gdb_sme_tile_support: > > + if args.gdb_int_cast_support: > > + INT_CAST_SUPPORT = 1 > > + else: > > + print("WARNING: The version of gdb used > (15.0.50.20240403-git)\n" > > This is super misleading: it looks like it's printing out what > gdb version we ran the tests with, but actually this version > string is just hardcoded ! > > > + "does not support casting a gdb.Value object to 128 bit > python\n" > > + "integer. Thus, the testing for the ZA quadwords will be done\n" > > + "without int casting. Refer to > tests/tcg/aarch64/gdbstub/test-sme.py\n" > > + "for details.") > > This is a big warning that doesn't convey anything useful > to the reader (why do I care whether your test case > is using int casting or not?). > > If there is part of the gdb view of the register that we > can't test on some gdb versions, that's fine. To handle > that, split the test case into two, so that for the > test that needs a newer gdb we report the test as SKIPPED > because gdb is too old. > > Please also write some comments that tell me what your > test is testing. I should not have to reverse-engineer > your intentions by reading the code... > > thanks > -- PMM >
