[Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler

Jon Doron posted 27 patches 4 years, 12 months ago
Test asan failed
Test docker-clang@ubuntu failed
Test docker-mingw@fedora failed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190502081554.5521-1-arilou@gmail.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
accel/kvm/kvm-all.c  |   39 +
gdbstub.c            | 1807 ++++++++++++++++++++++++++++++------------
include/sysemu/kvm.h |    2 +
3 files changed, 1359 insertions(+), 489 deletions(-)
[Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler
Posted by Jon Doron 4 years, 12 months ago
This patch series refactors the old gdbstub command packets handler
with a new infrastructure which should ease extending and adding new
and missing gdb command packets.

version 9 changes:
- checkpatch fixes

version 8 changes:
- Add new command to display the Supported qemu generic query/sets
- kvm: Add API to read/write a MSR
- Add new commands specific for qemu:
  * Command to swap the memory GDB sees to be the physical memory
  * Commands to read and write a MSR

version 7 changes:
- Fixed few checkpatch complaints
- Feedback from Alex Bennee

version 4-6 changes:
- mostly feedback from Richard Henderson

version 3 changes
- Split the single patch to many individual patches for easier reviewing

version 2 changes
- Code convention fixes

Jon Doron (27):
  gdbstub: Add infrastructure to parse cmd packets
  gdbstub: Implement deatch (D pkt) with new infra
  gdbstub: Implement thread_alive (T pkt) with new infra
  gdbstub: Implement continue (c pkt) with new infra
  gdbstub: Implement continue with signal (C pkt) with new infra
  gdbstub: Implement set_thread (H pkt) with new infra
  gdbstub: Implement insert breakpoint (Z pkt) with new infra
  gdbstub: Implement remove breakpoint (z pkt) with new infra
  gdbstub: Implement set register (P pkt) with new infra
  gdbstub: Implement get register (p pkt) with new infra
  gdbstub: Implement write memory (M pkt) with new infra
  gdbstub: Implement read memory (m pkt) with new infra
  gdbstub: Implement write all registers (G pkt) with new infra
  gdbstub: Implement read all registers (g pkt) with new infra
  gdbstub: Implement file io (F pkt) with new infra
  gdbstub: Implement step (s pkt) with new infra
  gdbstub: Implement v commands with new infra
  gdbstub: Implement generic query (q pkt) with new infra
  gdbstub: Implement generic set (Q pkt) with new infra
  gdbstub: Implement target halted (? pkt) with new infra
  gdbstub: Clear unused variables in gdb_handle_packet
  gdbstub: Implement generic query qemu.Supported
  gdbstub: Implement qemu physical memory mode
  gdbstub: Add another handler for setting qemu.sstep
  kvm: Add API to read/write a CPU MSR value
  gdbstub: Add support to read a MSR for KVM target
  gdbstub: Add support to write a MSR for KVM target

 accel/kvm/kvm-all.c  |   39 +
 gdbstub.c            | 1807 ++++++++++++++++++++++++++++++------------
 include/sysemu/kvm.h |    2 +
 3 files changed, 1359 insertions(+), 489 deletions(-)

-- 
2.20.1


Re: [Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler
Posted by Alex Bennée 4 years, 11 months ago
Jon Doron <arilou@gmail.com> writes:

> This patch series refactors the old gdbstub command packets handler
> with a new infrastructure which should ease extending and adding new
> and missing gdb command packets.

There seems to be some compiler errors and variants that this breaks the
CI on:

  https://travis-ci.org/stsquad/qemu/builds/532410263
  https://app.shippable.com/github/stsquad/qemu/runs/822/summary/console
  https://gitlab.com/stsquad/qemu/pipelines/61291132

You might want to consider setting up CI checks on your system and
running your next series through them before posting:

  https://wiki.qemu.org/Testing/CI

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler
Posted by Alex Bennée 4 years, 11 months ago
Jon Doron <arilou@gmail.com> writes:

> This patch series refactors the old gdbstub command packets handler
> with a new infrastructure which should ease extending and adding new
> and missing gdb command packets.

Jon,

I've finished my review and things are looking pretty good. The code is
a good clean-up and makes adding new features a lot easier. Thanks for
the examples of extensions - they were worth it to see how this might be
used although we shouldn't include them in the first merge. As they
extend the gdbserver ABI we'll want to think carefully about exactly
what we want to expose before we include it in master.

Going forwards aside from the various comments on each patch it would be
worth making sure the branch has gone through at least one CI run to
make sure the non-x86 builds (and disable-tcg and other exotica) haven't
been broken.

It would be nice if we could extend the testing of the gdbserver. Have
you been testing this with the gdb test suite or just manually? Now we
have system test and linux-user binaries being built we could probably
do better than the manually run tests/guest-debug/test-gdbstub.py.

Finally it would be nice if we could modernise the membuf and strbuf
handling with a more robust glib based approach. I understand if you
don't want to do that now and I'll happily accept the patches without it
but I did notice you can send the gdbserver a bit loopy if you send it
some very long maint packets so it would be nice to have that a bit
safer.

--
Alex Bennée

Re: [Qemu-devel] [PATCH v9 00/27] gdbstub: Refactor command packets handler
Posted by Alex Bennée 4 years, 11 months ago
Alex Bennée <alex.bennee@linaro.org> writes:

> Jon Doron <arilou@gmail.com> writes:
>
>> This patch series refactors the old gdbstub command packets handler
>> with a new infrastructure which should ease extending and adding new
>> and missing gdb command packets.
>
> Jon,
>
<snip>
>
> Finally it would be nice if we could modernise the membuf and strbuf
> handling with a more robust glib based approach. I understand if you
> don't want to do that now and I'll happily accept the patches without it
> but I did notice you can send the gdbserver a bit loopy if you send it
> some very long maint packets so it would be nice to have that a bit
> safer.

Actually I had a go at this this morning and it turned out to be quite
fiddly so perhaps this is something best left to a follow-up series once
the re-factoring is in.

--
Alex Bennée