mbox series

[v6,00/22] gdbstub refactor and SVE support

Message ID 20200205171031.22582-1-alex.bennee@linaro.org (mailing list archive)
Headers show
Series gdbstub refactor and SVE support | expand

Message

Alex Bennée Feb. 5, 2020, 5:10 p.m. UTC 
Hi,

The main change for the last iteration is the re-introduction of the
VG pseudo register. I've also applied review tags from the last post.
All that really needs looking over is the tests.

Please note that if your local GDB doesn't work the test will skip if
it fails to connect. I have to build --with-gdb pointing at a
relatively recent multiarch build.

This series is available from:

  github.com:stsquad/qemu.git gdbstub/sve-registers-v6

and is currently based of the in-flight testing PR.

The following patches need review
  08 - target i386 use gdb_get_reg helpers
  10 - target arm prepare for multiple dynamic XMLs (1 ack)
  13 - target arm generate xml description of our SVE re (1 ack)
  18 - tests tcg aarch64 add a gdbstub testcase for SVE 
  19 - tests tcg aarch64 add SVE iotcl test
  20 - tests tcg aarch64 add test sve ioctl guest debug 

Alex Bennée (20):
  gdbstub: make GDBState static and have common init function
  gdbstub: stop passing GDBState * around and use global
  gdbstub: move str_buf to GDBState and use GString
  gdbstub: move mem_buf to GDBState and use GByteArray
  gdbstub: add helper for 128 bit registers
  target/arm: use gdb_get_reg helpers
  target/m68k: use gdb_get_reg helpers
  target/i386: use gdb_get_reg helpers
  gdbstub: extend GByteArray to read register helpers
  target/arm: prepare for multiple dynamic XMLs
  target/arm: explicitly encode regnum in our XML
  target/arm: default SVE length to 64 bytes for linux-user
  target/arm: generate xml description of our SVE registers
  target/arm: don't bother with id_aa64pfr0_read for USER_ONLY
  tests/tcg/aarch64: userspace system register test
  configure: allow user to specify what gdb to use
  tests/guest-debug: add a simple test runner
  tests/tcg/aarch64: add a gdbstub testcase for SVE registers
  tests/tcg/aarch64: add SVE iotcl test
  tests/tcg/aarch64: add test-sve-ioctl guest-debug test

Damien Hedde (2):
  gdbstub: change GDBState.last_packet to GByteArray
  gdbstub: do not split gdb_monitor_write payload

 configure                                   |   9 +
 include/exec/gdbstub.h                      |  62 +-
 include/hw/core/cpu.h                       |   2 +-
 target/alpha/cpu.h                          |   2 +-
 target/arm/cpu.h                            |  31 +-
 target/cris/cpu.h                           |   4 +-
 target/hppa/cpu.h                           |   2 +-
 target/i386/cpu.h                           |   2 +-
 target/lm32/cpu.h                           |   2 +-
 target/m68k/cpu.h                           |   2 +-
 target/microblaze/cpu.h                     |   2 +-
 target/mips/internal.h                      |   2 +-
 target/openrisc/cpu.h                       |   2 +-
 target/ppc/cpu.h                            |   4 +-
 target/riscv/cpu.h                          |   2 +-
 target/s390x/internal.h                     |   2 +-
 target/sh4/cpu.h                            |   2 +-
 target/sparc/cpu.h                          |   2 +-
 target/xtensa/cpu.h                         |   2 +-
 gdbstub.c                                   | 936 ++++++++++----------
 hw/core/cpu.c                               |   2 +-
 target/alpha/gdbstub.c                      |   2 +-
 target/arm/cpu.c                            |   4 +-
 target/arm/gdbstub.c                        | 173 +++-
 target/arm/gdbstub64.c                      |   2 +-
 target/arm/helper.c                         | 180 +++-
 target/cris/gdbstub.c                       |   4 +-
 target/hppa/gdbstub.c                       |   2 +-
 target/i386/gdbstub.c                       |  24 +-
 target/lm32/gdbstub.c                       |   2 +-
 target/m68k/gdbstub.c                       |   2 +-
 target/m68k/helper.c                        |  33 +-
 target/microblaze/gdbstub.c                 |   2 +-
 target/mips/gdbstub.c                       |   2 +-
 target/nios2/cpu.c                          |   2 +-
 target/openrisc/gdbstub.c                   |   2 +-
 target/ppc/gdbstub.c                        |  48 +-
 target/ppc/translate_init.inc.c             |  54 +-
 target/riscv/gdbstub.c                      |  18 +-
 target/s390x/gdbstub.c                      |  30 +-
 target/sh4/gdbstub.c                        |   2 +-
 target/sparc/gdbstub.c                      |   2 +-
 target/xtensa/gdbstub.c                     |   2 +-
 tests/tcg/aarch64/sve-ioctls.c              |  70 ++
 tests/tcg/aarch64/sysregs.c                 | 172 ++++
 tests/.gitignore                            |   1 +
 tests/guest-debug/run-test.py               |  57 ++
 tests/tcg/aarch64/Makefile.target           |  32 +
 tests/tcg/aarch64/gdbstub/test-sve-ioctl.py |  79 ++
 tests/tcg/aarch64/gdbstub/test-sve.py       |  81 ++
 50 files changed, 1466 insertions(+), 694 deletions(-)
 create mode 100644 tests/tcg/aarch64/sve-ioctls.c
 create mode 100644 tests/tcg/aarch64/sysregs.c
 create mode 100755 tests/guest-debug/run-test.py
 create mode 100644 tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
 create mode 100644 tests/tcg/aarch64/gdbstub/test-sve.py

Comments

no-reply@patchew.org Feb. 5, 2020, 6 p.m. UTC | #1 
Patchew URL: https://patchew.org/QEMU/20200205171031.22582-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH  v6 00/22] gdbstub refactor and SVE support
Message-id: 20200205171031.22582-1-alex.bennee@linaro.org
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200205171031.22582-1-alex.bennee@linaro.org -> patchew/20200205171031.22582-1-alex.bennee@linaro.org
Switched to a new branch 'test'
3e44888 gdbstub: do not split gdb_monitor_write payload
d33b757 gdbstub: change GDBState.last_packet to GByteArray
713dd94 tests/tcg/aarch64: add test-sve-ioctl guest-debug test
48dd504 tests/tcg/aarch64: add SVE iotcl test
b292836 tests/tcg/aarch64: add a gdbstub testcase for SVE registers
381929d tests/guest-debug: add a simple test runner
5030841 configure: allow user to specify what gdb to use
91f1dd1 tests/tcg/aarch64: userspace system register test
8253e9e target/arm: don't bother with id_aa64pfr0_read for USER_ONLY
758a44b target/arm: generate xml description of our SVE registers
d9558ba target/arm: default SVE length to 64 bytes for linux-user
abcac23 target/arm: explicitly encode regnum in our XML
b53ef36 target/arm: prepare for multiple dynamic XMLs
b088f0b gdbstub: extend GByteArray to read register helpers
54147d1 target/i386: use gdb_get_reg helpers
558521d target/m68k: use gdb_get_reg helpers
5379b08 target/arm: use gdb_get_reg helpers
f8fce05 gdbstub: add helper for 128 bit registers
f5ec073 gdbstub: move mem_buf to GDBState and use GByteArray
b6e969a gdbstub: move str_buf to GDBState and use GString
45b329f gdbstub: stop passing GDBState * around and use global
ee4bf51 gdbstub: make GDBState static and have common init function

=== OUTPUT BEGIN ===
1/22 Checking commit ee4bf51c9ade (gdbstub: make GDBState static and have common init function)
ERROR: braces {} are necessary for all arms of this statement
#128: FILE: gdbstub.c:2743:
+    if (!gdbserver_state.init)
[...]

ERROR: suspect code indent for conditional statements (2, 6)
#178: FILE: gdbstub.c:2962:
+  if (!gdbserver_state.init) {
       return;

ERROR: suspect code indent for conditional statements (2, 6)
#183: FILE: gdbstub.c:2966:
+  if (gdbserver_fd < 0 || gdbserver_state.fd < 0) {
       return;

total: 3 errors, 0 warnings, 384 lines checked

Patch 1/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/22 Checking commit 45b329f8ffd9 (gdbstub: stop passing GDBState * around and use global)
WARNING: line over 80 characters
#756: FILE: gdbstub.c:1762:
+    if (target_memory_rw_debug(gdbserver_state.g_cpu, gdb_ctx->params[0].val_ull,

WARNING: line over 80 characters
#784: FILE: gdbstub.c:1785:
+    if (target_memory_rw_debug(gdbserver_state.g_cpu, gdb_ctx->params[0].val_ull,

WARNING: line over 80 characters
#1011: FILE: gdbstub.c:2022:
+    gdbserver_state.query_cpu = gdb_next_attached_cpu(gdbserver_state.query_cpu);

ERROR: line over 90 characters
#1372: FILE: gdbstub.c:2817:
+            put_buffer((uint8_t *)gdbserver_state.last_packet, gdbserver_state.last_packet_len);

ERROR: space required before the open parenthesis '('
#1390: FILE: gdbstub.c:2836:
+        switch(gdbserver_state.state) {

ERROR: line over 90 characters
#1422: FILE: gdbstub.c:2859:
+            } else if (gdbserver_state.line_buf_index >= sizeof(gdbserver_state.line_buf) - 1) {

ERROR: line over 90 characters
#1440: FILE: gdbstub.c:2872:
+            } else if (gdbserver_state.line_buf_index >= sizeof(gdbserver_state.line_buf) - 1) {

WARNING: line over 80 characters
#1450: FILE: gdbstub.c:2878:
+                gdbserver_state.line_buf[gdbserver_state.line_buf_index++] = ch ^ 0x20;

ERROR: line over 90 characters
#1466: FILE: gdbstub.c:2895:
+                if (gdbserver_state.line_buf_index + repeat >= sizeof(gdbserver_state.line_buf) - 1) {

WARNING: line over 80 characters
#1484: FILE: gdbstub.c:2905:
+                    memset(gdbserver_state.line_buf + gdbserver_state.line_buf_index,

ERROR: line over 90 characters
#1485: FILE: gdbstub.c:2906:
+                           gdbserver_state.line_buf[gdbserver_state.line_buf_index - 1], repeat);

WARNING: line over 80 characters
#1520: FILE: gdbstub.c:2933:
+            if (gdbserver_state.line_csum != (gdbserver_state.line_sum & 0xff)) {

ERROR: line over 90 characters
#1521: FILE: gdbstub.c:2934:
+                trace_gdbstub_err_checksum_incorrect(gdbserver_state.line_sum, gdbserver_state.line_csum);

WARNING: line over 80 characters
#1534: FILE: gdbstub.c:2943:
+                gdbserver_state.state = gdb_handle_packet(gdbserver_state.line_buf);

ERROR: line over 90 characters
#1649: FILE: gdbstub.c:3305:
+        qsort(gdbserver_state.processes, gdbserver_state.process_num, sizeof(gdbserver_state.processes[0]), pid_order);

total: 8 errors, 7 warnings, 1553 lines checked

Patch 2/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/22 Checking commit b6e969a0e793 (gdbstub: move str_buf to GDBState and use GString)
WARNING: line over 80 characters
#150: FILE: gdbstub.c:1794:
+    memtohex(gdbserver_state.str_buf, gdb_ctx->mem_buf, gdb_ctx->params[1].val_ull);

WARNING: line over 80 characters
#323: FILE: gdbstub.c:2107:
+    g_string_printf(gdbserver_state.str_buf, "PacketSize=%x", MAX_PACKET_LENGTH);

total: 0 errors, 2 warnings, 422 lines checked

Patch 3/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/22 Checking commit f5ec07345221 (gdbstub: move mem_buf to GDBState and use GByteArray)
5/22 Checking commit f8fce0502866 (gdbstub: add helper for 128 bit registers)
6/22 Checking commit 5379b084fe6f (target/arm: use gdb_get_reg helpers)
ERROR: space required after that ',' (ctx:VxV)
#45: FILE: target/arm/helper.c:118:
+        return gdb_get_reg32(buf,vfp_get_fpcr(env));
                                 ^

total: 1 errors, 0 warnings, 28 lines checked

Patch 6/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/22 Checking commit 558521d56598 (target/m68k: use gdb_get_reg helpers)
8/22 Checking commit 54147d14ad8b (target/i386: use gdb_get_reg helpers)
9/22 Checking commit b088f0be4928 (gdbstub: extend GByteArray to read register helpers)
ERROR: "foo * bar" should be "foo *bar"
#198: FILE: include/exec/gdbstub.h:136:
+static inline uint8_t * gdb_get_reg_ptr(GByteArray *buf, int len)

total: 1 errors, 0 warnings, 913 lines checked

Patch 9/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

10/22 Checking commit b53ef36cf69e (target/arm: prepare for multiple dynamic XMLs)
ERROR: line over 90 characters
#128: FILE: target/arm/gdbstub.c:159:
+    cpu->dyn_sysreg_xml.data.cpregs.keys = g_new(uint32_t, g_hash_table_size(cpu->cp_regs));

total: 1 errors, 0 warnings, 136 lines checked

Patch 10/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

11/22 Checking commit abcac23e4058 (target/arm: explicitly encode regnum in our XML)
12/22 Checking commit d9558babb5b1 (target/arm: default SVE length to 64 bytes for linux-user)
13/22 Checking commit 758a44b0b5c6 (target/arm: generate xml description of our SVE registers)
WARNING: line over 80 characters
#108: FILE: target/arm/gdbstub.c:233:
+                g_string_append_printf(s, "<field name=\"%c\" type=\"vq%d%c%c\"/>",

WARNING: line over 80 characters
#323: FILE: target/arm/helper.c:7236:
+                                     arm_gen_dynamic_svereg_xml(cs, cs->gdb_num_regs),

total: 0 errors, 2 warnings, 310 lines checked

Patch 13/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
14/22 Checking commit 8253e9ecc586 (target/arm: don't bother with id_aa64pfr0_read for USER_ONLY)
15/22 Checking commit 91f1dd18eee0 (tests/tcg/aarch64: userspace system register test)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#33: 
new file mode 100644

ERROR: space required after that ',' (ctx:VxV)
#152: FILE: tests/tcg/aarch64/sysregs.c:115:
+    get_cpu_reg_check_mask(id_aa64isar0_el1, _m(00ff,ffff,f0ff,fff0));
                                                     ^

ERROR: space required after that ',' (ctx:VxV)
#152: FILE: tests/tcg/aarch64/sysregs.c:115:
+    get_cpu_reg_check_mask(id_aa64isar0_el1, _m(00ff,ffff,f0ff,fff0));
                                                          ^

ERROR: space required after that ',' (ctx:VxV)
#152: FILE: tests/tcg/aarch64/sysregs.c:115:
+    get_cpu_reg_check_mask(id_aa64isar0_el1, _m(00ff,ffff,f0ff,fff0));
                                                               ^

ERROR: space required after that ',' (ctx:VxV)
#153: FILE: tests/tcg/aarch64/sysregs.c:116:
+    get_cpu_reg_check_mask(id_aa64isar1_el1, _m(0000,00f0,ffff,ffff));
                                                     ^

ERROR: space required after that ',' (ctx:VxV)
#153: FILE: tests/tcg/aarch64/sysregs.c:116:
+    get_cpu_reg_check_mask(id_aa64isar1_el1, _m(0000,00f0,ffff,ffff));
                                                          ^

ERROR: space required after that ',' (ctx:VxV)
#153: FILE: tests/tcg/aarch64/sysregs.c:116:
+    get_cpu_reg_check_mask(id_aa64isar1_el1, _m(0000,00f0,ffff,ffff));
                                                               ^

ERROR: space required after that ',' (ctx:VxV)
#155: FILE: tests/tcg/aarch64/sysregs.c:118:
+    get_cpu_reg_check_mask(id_aa64mmfr0_el1, _m(0000,0000,ff00,0000));
                                                     ^

ERROR: space required after that ',' (ctx:VxV)
#155: FILE: tests/tcg/aarch64/sysregs.c:118:
+    get_cpu_reg_check_mask(id_aa64mmfr0_el1, _m(0000,0000,ff00,0000));
                                                          ^

ERROR: space required after that ',' (ctx:VxV)
#155: FILE: tests/tcg/aarch64/sysregs.c:118:
+    get_cpu_reg_check_mask(id_aa64mmfr0_el1, _m(0000,0000,ff00,0000));
                                                               ^

ERROR: space required after that ',' (ctx:VxV)
#158: FILE: tests/tcg/aarch64/sysregs.c:121:
+    get_cpu_reg_check_mask(id_aa64pfr0_el1,  _m(000f,000f,00ff,0011));
                                                     ^

ERROR: space required after that ',' (ctx:VxV)
#158: FILE: tests/tcg/aarch64/sysregs.c:121:
+    get_cpu_reg_check_mask(id_aa64pfr0_el1,  _m(000f,000f,00ff,0011));
                                                          ^

ERROR: space required after that ',' (ctx:VxV)
#158: FILE: tests/tcg/aarch64/sysregs.c:121:
+    get_cpu_reg_check_mask(id_aa64pfr0_el1,  _m(000f,000f,00ff,0011));
                                                               ^

ERROR: space required after that ',' (ctx:VxV)
#159: FILE: tests/tcg/aarch64/sysregs.c:122:
+    get_cpu_reg_check_mask(id_aa64pfr1_el1,  _m(0000,0000,0000,00f0));
                                                     ^

ERROR: space required after that ',' (ctx:VxV)
#159: FILE: tests/tcg/aarch64/sysregs.c:122:
+    get_cpu_reg_check_mask(id_aa64pfr1_el1,  _m(0000,0000,0000,00f0));
                                                          ^

ERROR: space required after that ',' (ctx:VxV)
#159: FILE: tests/tcg/aarch64/sysregs.c:122:
+    get_cpu_reg_check_mask(id_aa64pfr1_el1,  _m(0000,0000,0000,00f0));
                                                               ^

ERROR: space required after that ',' (ctx:VxV)
#161: FILE: tests/tcg/aarch64/sysregs.c:124:
+    get_cpu_reg_check_mask(id_aa64dfr0_el1,  _m(0000,0000,0000,0006));
                                                     ^

ERROR: space required after that ',' (ctx:VxV)
#161: FILE: tests/tcg/aarch64/sysregs.c:124:
+    get_cpu_reg_check_mask(id_aa64dfr0_el1,  _m(0000,0000,0000,0006));
                                                          ^

ERROR: space required after that ',' (ctx:VxV)
#161: FILE: tests/tcg/aarch64/sysregs.c:124:
+    get_cpu_reg_check_mask(id_aa64dfr0_el1,  _m(0000,0000,0000,0006));
                                                               ^

ERROR: space required after that ',' (ctx:VxV)
#168: FILE: tests/tcg/aarch64/sysregs.c:131:
+    get_cpu_reg_check_mask(midr_el1,         _m(0000,0000,ffff,ffff));
                                                     ^

ERROR: space required after that ',' (ctx:VxV)
#168: FILE: tests/tcg/aarch64/sysregs.c:131:
+    get_cpu_reg_check_mask(midr_el1,         _m(0000,0000,ffff,ffff));
                                                          ^

ERROR: space required after that ',' (ctx:VxV)
#168: FILE: tests/tcg/aarch64/sysregs.c:131:
+    get_cpu_reg_check_mask(midr_el1,         _m(0000,0000,ffff,ffff));
                                                               ^

ERROR: space required after that ',' (ctx:VxV)
#170: FILE: tests/tcg/aarch64/sysregs.c:133:
+    get_cpu_reg_check_mask(mpidr_el1,        _m(0000,0000,8000,0000));
                                                     ^

ERROR: space required after that ',' (ctx:VxV)
#170: FILE: tests/tcg/aarch64/sysregs.c:133:
+    get_cpu_reg_check_mask(mpidr_el1,        _m(0000,0000,8000,0000));
                                                          ^

ERROR: space required after that ',' (ctx:VxV)
#170: FILE: tests/tcg/aarch64/sysregs.c:133:
+    get_cpu_reg_check_mask(mpidr_el1,        _m(0000,0000,8000,0000));
                                                               ^

total: 24 errors, 1 warnings, 182 lines checked

Patch 15/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

16/22 Checking commit 5030841d0372 (configure: allow user to specify what gdb to use)
17/22 Checking commit 381929d9c5a2 (tests/guest-debug: add a simple test runner)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#16: 
new file mode 100755

ERROR: line over 90 characters
#73: FILE: tests/guest-debug/run-test.py:53:
+    gdb_cmd = "%s %s -ex 'target remote localhost:1234' -x %s" % (args.gdb, args.binary, args.test)

total: 1 errors, 1 warnings, 57 lines checked

Patch 17/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

18/22 Checking commit b292836e3366 (tests/tcg/aarch64: add a gdbstub testcase for SVE registers)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#54: 
new file mode 100644

total: 0 errors, 1 warnings, 109 lines checked

Patch 18/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
19/22 Checking commit 48dd5040c653 (tests/tcg/aarch64: add SVE iotcl test)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#34: 
new file mode 100644

total: 0 errors, 1 warnings, 80 lines checked

Patch 19/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
20/22 Checking commit 713dd94ce30d (tests/tcg/aarch64: add test-sve-ioctl guest-debug test)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#45: 
new file mode 100644

total: 0 errors, 1 warnings, 103 lines checked

Patch 20/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
21/22 Checking commit d33b7572398c (gdbstub: change GDBState.last_packet to GByteArray)
22/22 Checking commit 3e448886b66b (gdbstub: do not split gdb_monitor_write payload)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200205171031.22582-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com