[Qemu-devel] [PATCH v8 00/16] gdbstub: support for the multiprocess extension

Luc Michel posted 16 patches 5 years, 4 months ago
Test checkpatch passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181207090135.7651-1-luc.michel@greensocs.com
include/hw/arm/xlnx-zynqmp.h |   3 +
include/hw/cpu/cluster.h     |  58 +++
gdbstub.c                    | 662 +++++++++++++++++++++++++++++++----
hw/arm/xlnx-zynqmp.c         |  23 +-
hw/cpu/cluster.c             |  50 +++
MAINTAINERS                  |   2 +
hw/cpu/Makefile.objs         |   2 +-
7 files changed, 719 insertions(+), 81 deletions(-)
create mode 100644 include/hw/cpu/cluster.h
create mode 100644 hw/cpu/cluster.c
[Qemu-devel] [PATCH v8 00/16] gdbstub: support for the multiprocess extension
Posted by Luc Michel 5 years, 4 months ago
changes since v7:
  - patch 1    Add documentation about cpu-cluster [Eduardo, Peter]

  - patch 1    Remove the cluster-id auto-assign mechanism [Eduardo]

  - patch 2    Replace create_unique_process() with
               create_default_process() that is now called even if some
               clusters are found. This default process is used for
               CPUs that are not in a cluster [Eduardo, Peter]

  - patch 3    Refactor and harden gdb_get_cpu_pid() [Philippe]

  - patch 16   Go back to cluster-id manual assignment [Eduardo]

changes since v6:
  - patch 4    Fix a refactoring issue in gdb_get_cpu [Edgar]

  - patch 5    Renamed gdb_first_cpu/gdb_next_cpu to
               gdb_first_attached_cpu/gdb_next_attached_cpu [Edgar]

  - patch 7    Added the CPU name and removed the CPU index in the
               ThreadInfo packet in multiprocess mode [Edgar]

changes since v5:
  - patch 1    Rebased on top of master

  - patch 2    Cluster ID handling hardening to ensure uint32_t overflow
               won't cause PID 0 to be used [Edgar]

  - patch 2    Fix the GDB process ordering function to avoid uint32_t
               overflow when comparing two cluster_ids [Edgar]

changes since v4:
  - patch 1    add cpu_cluster.[ch] files into MAINTAINERS Machine core
               section (thanks Eduardo!) [Philippe, Eduardo]

  - patch 1    revert to uint32_t for cluster IDs [Philippe]

  - patch 1    fixed a coding style issue [patchew]

changes since v3:
  - patch 1    cpu_cluster.h: remove QEMU_ from the multiple includes
               guard #ifdef/#define [Alistair]

  - patch 1    cpu_cluster.c: include osdep.h first [Alistair]

  - patch 1    use uint64_t for cluster ID for prosperity :) [Philippe]

  - patch 1    auto-assign a cluster ID to newly created clusters [Philippe]

  - patch 2    fix mid-code variable declaration [Alistair]

  - patch 3    add a comment in gdb_get_cpu_pid() when retrieving CPU
               parent canonical path [Alistair]

  - patch 14   fix a typo in the commit message [Alistair]

changes since v2:
  - patch 1    introducing the cpu-cluster type. I didn't opt for an
               Interface, but I can add one if you think it's necessary.
               For now this class inherits from Device and has a
               cluster-id property, used by the GDB stub to compute a
               PID.

  - patch 2    removed GDB group related code as it has been replaced
               with CPU clusters

  - patch 2/8  moved GDBProcess target_xml field introduction into patch
               8 [Philippe]

  - patch 3    gdb_get_cpu_pid() now search for CPU being a child of a
               cpu-cluster object. Use the cluster-id to compute the PID.

  - patch 4    gdb_get_process() does not rely on s->processes array
               indices anymore as PIDs can now be sparse. Instead, iterate
               over the array to find the process.

  - patch 3/4  removed Reviewed-by tags because of substantial changes.

  - patch 4/7  read_thread_id() hardening [Philippe]

  - patch 12   safer vAttach packet parsing [Phillipe]

  - patch 16   put APUs and RPUs in different clusters instead of GDB
               groups

changes since v1:
  - rename qemu_get_thread_id() to gdb_fmt_thread_id() [Philippe]
  - check qemu_strtoul() return value for 'D' packets [Philippe]


This series adds support for the multiprocess extension of the GDB
remote protocol in the QEMU GDB stub.

This extension is useful to split QEMU emulated CPUs in different
processes from the point of view of the GDB client. It adds the
possibility to debug different kind of processors (e.g. an AArch64 and
an ARMv7 CPU) at the same time (it is not possible otherwise since GDB
expects an SMP view at the thread granularity.

CPUs are grouped using specially named QOM containers. CPUs that are
children of such a container are grouped under the same GDB process.

The last patch groups the CPUs of different model in the zynqmp machines
into separate processes.

To test this patchset, you can use the following commands:

(Note that this requires a recent enough GDB, I think GDB 7.2 is OK.
Also, it must be compiled to support both ARM and AArch64 architectures)

Run QEMU: (-smp 6 in xlnx-zcu102 enables both cortex-a53 and cortex-r5
CPUs)

qemu-system-aarch64 -M xlnx-zcu102 -gdb tcp::1234 -S -smp 6

Run the following commands in GDB:

target extended :1234
add-inferior
inferior 2
attach 2
info threads

I want to thanks the Xilinx's QEMU team who sponsored this work for
their collaboration and their prototype implementation.


Luc Michel (16):
  hw/cpu: introduce CPU clusters
  gdbstub: introduce GDB processes
  gdbstub: add multiprocess support to '?' packets
  gdbstub: add multiprocess support to 'H' and 'T' packets
  gdbstub: add multiprocess support to vCont packets
  gdbstub: add multiprocess support to 'sC' packets
  gdbstub: add multiprocess support to (f|s)ThreadInfo and
    ThreadExtraInfo
  gdbstub: add multiprocess support to Xfer:features:read:
  gdbstub: add multiprocess support to gdb_vm_state_change()
  gdbstub: add multiprocess support to 'D' packets
  gdbstub: add support for extended mode packet
  gdbstub: add support for vAttach packets
  gdbstub: processes initialization on new peer connection
  gdbstub: gdb_set_stop_cpu: ignore request when process is not attached
  gdbstub: add multiprocess extension support
  arm/xlnx-zynqmp: put APUs and RPUs in separate CPU clusters

 include/hw/arm/xlnx-zynqmp.h |   3 +
 include/hw/cpu/cluster.h     |  58 +++
 gdbstub.c                    | 662 +++++++++++++++++++++++++++++++----
 hw/arm/xlnx-zynqmp.c         |  23 +-
 hw/cpu/cluster.c             |  50 +++
 MAINTAINERS                  |   2 +
 hw/cpu/Makefile.objs         |   2 +-
 7 files changed, 719 insertions(+), 81 deletions(-)
 create mode 100644 include/hw/cpu/cluster.h
 create mode 100644 hw/cpu/cluster.c

-- 
2.19.2


Re: [Qemu-devel] [PATCH v8 00/16] gdbstub: support for the multiprocess extension
Posted by Peter Maydell 5 years, 3 months ago
On Fri, 7 Dec 2018 at 09:01, Luc Michel <luc.michel@greensocs.com> wrote:
> This series adds support for the multiprocess extension of the GDB
> remote protocol in the QEMU GDB stub.
>
> This extension is useful to split QEMU emulated CPUs in different
> processes from the point of view of the GDB client. It adds the
> possibility to debug different kind of processors (e.g. an AArch64 and
> an ARMv7 CPU) at the same time (it is not possible otherwise since GDB
> expects an SMP view at the thread granularity.

I've applied this patchset to target-arm.next, thanks.
(I fixed up a few checkpatch nits about block comment style.)

> To test this patchset, you can use the following commands:
>
> (Note that this requires a recent enough GDB, I think GDB 7.2 is OK.
> Also, it must be compiled to support both ARM and AArch64 architectures)
>
> Run QEMU: (-smp 6 in xlnx-zcu102 enables both cortex-a53 and cortex-r5
> CPUs)
>
> qemu-system-aarch64 -M xlnx-zcu102 -gdb tcp::1234 -S -smp 6
>
> Run the following commands in GDB:
>
> target extended :1234
> add-inferior
> inferior 2
> attach 2
> info threads

This seems a bit clumsy -- is it just the limitations of
gdb here? I was expecting that just connecting to the
remote would cause gdb to interrogate it and attach to
all the inferior processes that it had.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8 00/16] gdbstub: support for the multiprocess extension
Posted by Luc Michel 5 years, 3 months ago
On 1/4/19 4:12 PM, Peter Maydell wrote:
> On Fri, 7 Dec 2018 at 09:01, Luc Michel <luc.michel@greensocs.com> wrote:
>> This series adds support for the multiprocess extension of the GDB
>> remote protocol in the QEMU GDB stub.
>>
>> This extension is useful to split QEMU emulated CPUs in different
>> processes from the point of view of the GDB client. It adds the
>> possibility to debug different kind of processors (e.g. an AArch64 and
>> an ARMv7 CPU) at the same time (it is not possible otherwise since GDB
>> expects an SMP view at the thread granularity.
> 
> I've applied this patchset to target-arm.next, thanks.
> (I fixed up a few checkpatch nits about block comment style.)
Thanks!

> 
>> To test this patchset, you can use the following commands:
>>
>> (Note that this requires a recent enough GDB, I think GDB 7.2 is OK.
>> Also, it must be compiled to support both ARM and AArch64 architectures)
>>
>> Run QEMU: (-smp 6 in xlnx-zcu102 enables both cortex-a53 and cortex-r5
>> CPUs)
>>
>> qemu-system-aarch64 -M xlnx-zcu102 -gdb tcp::1234 -S -smp 6
>>
>> Run the following commands in GDB:
>>
>> target extended :1234
>> add-inferior
>> inferior 2
>> attach 2
>> info threads
> 
> This seems a bit clumsy -- is it just the limitations of
> gdb here? I was expecting that just connecting to the
> remote would cause gdb to interrogate it and attach to
> all the inferior processes that it had.
AFAIK this is a "limitation" of GDB. In QEMU context it seems like a
limitation, but when you are connecting to a remote gdbserver that
exposes all the processes currently running on an OS, you probably don't
want to attach them all automatically.

Luc

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH v8 00/16] gdbstub: support for the multiprocess extension
Posted by Luc Michel 5 years, 4 months ago
Hi all,

What do you think of this re-roll? Is it in good shape for upstreaming?

Thanks!

-- 
Luc

On 12/7/18 10:01 AM, Luc Michel wrote:
> changes since v7:
>   - patch 1    Add documentation about cpu-cluster [Eduardo, Peter]
> 
>   - patch 1    Remove the cluster-id auto-assign mechanism [Eduardo]
> 
>   - patch 2    Replace create_unique_process() with
>                create_default_process() that is now called even if some
>                clusters are found. This default process is used for
>                CPUs that are not in a cluster [Eduardo, Peter]
> 
>   - patch 3    Refactor and harden gdb_get_cpu_pid() [Philippe]
> 
>   - patch 16   Go back to cluster-id manual assignment [Eduardo]
> 
> changes since v6:
>   - patch 4    Fix a refactoring issue in gdb_get_cpu [Edgar]
> 
>   - patch 5    Renamed gdb_first_cpu/gdb_next_cpu to
>                gdb_first_attached_cpu/gdb_next_attached_cpu [Edgar]
> 
>   - patch 7    Added the CPU name and removed the CPU index in the
>                ThreadInfo packet in multiprocess mode [Edgar]
> 
> changes since v5:
>   - patch 1    Rebased on top of master
> 
>   - patch 2    Cluster ID handling hardening to ensure uint32_t overflow
>                won't cause PID 0 to be used [Edgar]
> 
>   - patch 2    Fix the GDB process ordering function to avoid uint32_t
>                overflow when comparing two cluster_ids [Edgar]
> 
> changes since v4:
>   - patch 1    add cpu_cluster.[ch] files into MAINTAINERS Machine core
>                section (thanks Eduardo!) [Philippe, Eduardo]
> 
>   - patch 1    revert to uint32_t for cluster IDs [Philippe]
> 
>   - patch 1    fixed a coding style issue [patchew]
> 
> changes since v3:
>   - patch 1    cpu_cluster.h: remove QEMU_ from the multiple includes
>                guard #ifdef/#define [Alistair]
> 
>   - patch 1    cpu_cluster.c: include osdep.h first [Alistair]
> 
>   - patch 1    use uint64_t for cluster ID for prosperity :) [Philippe]
> 
>   - patch 1    auto-assign a cluster ID to newly created clusters [Philippe]
> 
>   - patch 2    fix mid-code variable declaration [Alistair]
> 
>   - patch 3    add a comment in gdb_get_cpu_pid() when retrieving CPU
>                parent canonical path [Alistair]
> 
>   - patch 14   fix a typo in the commit message [Alistair]
> 
> changes since v2:
>   - patch 1    introducing the cpu-cluster type. I didn't opt for an
>                Interface, but I can add one if you think it's necessary.
>                For now this class inherits from Device and has a
>                cluster-id property, used by the GDB stub to compute a
>                PID.
> 
>   - patch 2    removed GDB group related code as it has been replaced
>                with CPU clusters
> 
>   - patch 2/8  moved GDBProcess target_xml field introduction into patch
>                8 [Philippe]
> 
>   - patch 3    gdb_get_cpu_pid() now search for CPU being a child of a
>                cpu-cluster object. Use the cluster-id to compute the PID.
> 
>   - patch 4    gdb_get_process() does not rely on s->processes array
>                indices anymore as PIDs can now be sparse. Instead, iterate
>                over the array to find the process.
> 
>   - patch 3/4  removed Reviewed-by tags because of substantial changes.
> 
>   - patch 4/7  read_thread_id() hardening [Philippe]
> 
>   - patch 12   safer vAttach packet parsing [Phillipe]
> 
>   - patch 16   put APUs and RPUs in different clusters instead of GDB
>                groups
> 
> changes since v1:
>   - rename qemu_get_thread_id() to gdb_fmt_thread_id() [Philippe]
>   - check qemu_strtoul() return value for 'D' packets [Philippe]
> 
> 
> This series adds support for the multiprocess extension of the GDB
> remote protocol in the QEMU GDB stub.
> 
> This extension is useful to split QEMU emulated CPUs in different
> processes from the point of view of the GDB client. It adds the
> possibility to debug different kind of processors (e.g. an AArch64 and
> an ARMv7 CPU) at the same time (it is not possible otherwise since GDB
> expects an SMP view at the thread granularity.
> 
> CPUs are grouped using specially named QOM containers. CPUs that are
> children of such a container are grouped under the same GDB process.
> 
> The last patch groups the CPUs of different model in the zynqmp machines
> into separate processes.
> 
> To test this patchset, you can use the following commands:
> 
> (Note that this requires a recent enough GDB, I think GDB 7.2 is OK.
> Also, it must be compiled to support both ARM and AArch64 architectures)
> 
> Run QEMU: (-smp 6 in xlnx-zcu102 enables both cortex-a53 and cortex-r5
> CPUs)
> 
> qemu-system-aarch64 -M xlnx-zcu102 -gdb tcp::1234 -S -smp 6
> 
> Run the following commands in GDB:
> 
> target extended :1234
> add-inferior
> inferior 2
> attach 2
> info threads
> 
> I want to thanks the Xilinx's QEMU team who sponsored this work for
> their collaboration and their prototype implementation.
> 
> 
> Luc Michel (16):
>   hw/cpu: introduce CPU clusters
>   gdbstub: introduce GDB processes
>   gdbstub: add multiprocess support to '?' packets
>   gdbstub: add multiprocess support to 'H' and 'T' packets
>   gdbstub: add multiprocess support to vCont packets
>   gdbstub: add multiprocess support to 'sC' packets
>   gdbstub: add multiprocess support to (f|s)ThreadInfo and
>     ThreadExtraInfo
>   gdbstub: add multiprocess support to Xfer:features:read:
>   gdbstub: add multiprocess support to gdb_vm_state_change()
>   gdbstub: add multiprocess support to 'D' packets
>   gdbstub: add support for extended mode packet
>   gdbstub: add support for vAttach packets
>   gdbstub: processes initialization on new peer connection
>   gdbstub: gdb_set_stop_cpu: ignore request when process is not attached
>   gdbstub: add multiprocess extension support
>   arm/xlnx-zynqmp: put APUs and RPUs in separate CPU clusters
> 
>  include/hw/arm/xlnx-zynqmp.h |   3 +
>  include/hw/cpu/cluster.h     |  58 +++
>  gdbstub.c                    | 662 +++++++++++++++++++++++++++++++----
>  hw/arm/xlnx-zynqmp.c         |  23 +-
>  hw/cpu/cluster.c             |  50 +++
>  MAINTAINERS                  |   2 +
>  hw/cpu/Makefile.objs         |   2 +-
>  7 files changed, 719 insertions(+), 81 deletions(-)
>  create mode 100644 include/hw/cpu/cluster.h
>  create mode 100644 hw/cpu/cluster.c
> 

Re: [Qemu-devel] [PATCH v8 00/16] gdbstub: support for the multiprocess extension
Posted by Peter Maydell 5 years, 4 months ago
On Mon, 17 Dec 2018 at 08:23, Luc Michel <luc.michel@greensocs.com> wrote:
>
> Hi all,
>
> What do you think of this re-roll? Is it in good shape for upstreaming?

It's on my list to look at still, but I'm off work til
January now, I'm afraid.

thanks
-- PMM