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

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

[Luc Michel]

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     |  38 +++
 gdbstub.c                    | 632 ++++++++++++++++++++++++++++++-----
 hw/arm/xlnx-zynqmp.c         |  21 +-
 hw/cpu/cluster.c             |  59 ++++
 MAINTAINERS                  |   2 +
 hw/cpu/Makefile.objs         |   2 +-
 7 files changed, 676 insertions(+), 81 deletions(-)
 create mode 100644 include/hw/cpu/cluster.h
 create mode 100644 hw/cpu/cluster.c

--
2.19.1

[Qemu-devel] [PATCH v5 01/16] hw/cpu: introduce CPU clusters

[Luc Michel]

This commit adds the cpu-cluster type. It aims at gathering CPUs from
the same cluster in a machine.

For now it only has a `cluster-id` property.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/cpu/cluster.h | 38 ++++++++++++++++++++++++++
 hw/cpu/cluster.c         | 59 ++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS              |  2 ++
 hw/cpu/Makefile.objs     |  2 +-
 4 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/cpu/cluster.h
 create mode 100644 hw/cpu/cluster.c

diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h
new file mode 100644
index 0000000000..11f50d5f6b
--- /dev/null
+++ b/include/hw/cpu/cluster.h
@@ -0,0 +1,38 @@
+/*
+ * QEMU CPU cluster
+ *
+ * Copyright (c) 2018 GreenSocs SAS
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ */
+#ifndef HW_CPU_CLUSTER_H
+#define HW_CPU_CLUSTER_H
+
+#include "qemu/osdep.h"
+#include "hw/qdev.h"
+
+#define TYPE_CPU_CLUSTER "cpu-cluster"
+#define CPU_CLUSTER(obj) \
+    OBJECT_CHECK(CPUClusterState, (obj), TYPE_CPU_CLUSTER)
+
+typedef struct CPUClusterState {
+    /*< private >*/
+    DeviceState parent_obj;
+
+    /*< public >*/
+    uint32_t cluster_id;
+} CPUClusterState;
+
+#endif
diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c
new file mode 100644
index 0000000000..e0ffd76152
--- /dev/null
+++ b/hw/cpu/cluster.c
@@ -0,0 +1,59 @@
+/*
+ * QEMU CPU cluster
+ *
+ * Copyright (c) 2018 GreenSocs SAS
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see
+ * <http://www.gnu.org/licenses/gpl-2.0.html>
+ */
+
+#include "qemu/osdep.h"
+#include "hw/cpu/cluster.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+
+static void cpu_cluster_init(Object *obj)
+{
+    static uint32_t cluster_id_auto_increment;
+    CPUClusterState *cluster = CPU_CLUSTER(obj);
+
+    cluster->cluster_id = cluster_id_auto_increment++;
+}
+
+static Property cpu_cluster_properties[] = {
+    DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void cpu_cluster_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = cpu_cluster_properties;
+}
+
+static const TypeInfo cpu_cluster_type_info = {
+    .name = TYPE_CPU_CLUSTER,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(CPUClusterState),
+    .instance_init = cpu_cluster_init,
+    .class_init = cpu_cluster_class_init,
+};
+
+static void cpu_cluster_register_types(void)
+{
+    type_register_static(&cpu_cluster_type_info);
+}
+
+type_init(cpu_cluster_register_types)
diff --git a/MAINTAINERS b/MAINTAINERS
index 126fe0be7e..8e20b0e672 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1022,11 +1022,13 @@ Machine core
 M: Eduardo Habkost <ehabkost@redhat.com>
 M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
 S: Supported
 F: hw/core/machine.c
 F: hw/core/null-machine.c
+F: hw/cpu/cluster.c
 F: include/hw/boards.h
+F: include/hw/cpu/cluster.h
 T: git git://github.com/ehabkost/qemu.git machine-next
 
 Xtensa Machines
 ---------------
 sim
diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
index cd52d20b65..8db9e8a7b3 100644
--- a/hw/cpu/Makefile.objs
+++ b/hw/cpu/Makefile.objs
@@ -1,5 +1,5 @@
 obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
 obj-$(CONFIG_REALVIEW) += realview_mpcore.o
 obj-$(CONFIG_A9MPCORE) += a9mpcore.o
 obj-$(CONFIG_A15MPCORE) += a15mpcore.o
-common-obj-y += core.o
+common-obj-y += core.o cluster.o
-- 
2.19.1

[Qemu-devel] [PATCH v5 02/16] gdbstub: introduce GDB processes

[Luc Michel]

Add a structure GDBProcess that represent processes from the GDB
semantic point of view.

CPUs can be split into different processes, by grouping them under
different cpu-cluster objects.  Each occurrence of a cpu-cluster object
implies the existence of the corresponding process in the GDB stub. The
GDB process ID is derived from the corresponding cluster ID as follows:

  GDB PID = cluster ID + 1

This is because PIDs -1 and 0 are reserved in GDB and cannot be used by
processes.

When no such container are found, all the CPUs are put in a unique GDB
process (create_unique_process()). This is also the case when compiled
in user mode, where multi-processes do not make much sense for now.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 gdbstub.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index c4e4f9f082..0d70b89598 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -27,10 +27,11 @@
 #include "monitor/monitor.h"
 #include "chardev/char.h"
 #include "chardev/char-fe.h"
 #include "sysemu/sysemu.h"
 #include "exec/gdbstub.h"
+#include "hw/cpu/cluster.h"
 #endif
 
 #define MAX_PACKET_LENGTH 4096
 
 #include "qemu/sockets.h"
@@ -294,10 +295,15 @@ typedef struct GDBRegisterState {
     gdb_reg_cb set_reg;
     const char *xml;
     struct GDBRegisterState *next;
 } GDBRegisterState;
 
+typedef struct GDBProcess {
+    uint32_t pid;
+    bool attached;
+} GDBProcess;
+
 enum RSState {
     RS_INACTIVE,
     RS_IDLE,
     RS_GETLINE,
     RS_GETLINE_ESC,
@@ -322,10 +328,13 @@ typedef struct GDBState {
     int running_state;
 #else
     CharBackend chr;
     Chardev *mon_chr;
 #endif
+    bool multiprocess;
+    GDBProcess *processes;
+    int process_num;
     char syscall_buf[256];
     gdb_syscall_complete_cb current_syscall_cb;
 } GDBState;
 
 /* By default use no IRQs and no timers while single stepping so as to
@@ -1749,10 +1758,24 @@ void gdb_exit(CPUArchState *env, int code)
 #ifndef CONFIG_USER_ONLY
   qemu_chr_fe_deinit(&s->chr, true);
 #endif
 }
 
+/*
+ * Create a unique process containing all the CPUs.
+ */
+static void create_unique_process(GDBState *s)
+{
+    GDBProcess *process;
+
+    s->processes = g_malloc0(sizeof(GDBProcess));
+    s->process_num = 1;
+    process = &s->processes[0];
+
+    process->pid = 1;
+}
+
 #ifdef CONFIG_USER_ONLY
 int
 gdb_handlesig(CPUState *cpu, int sig)
 {
     GDBState *s;
@@ -1846,10 +1869,11 @@ static bool gdb_accept(void)
     }
 
     s = g_malloc0(sizeof(GDBState));
     s->c_cpu = first_cpu;
     s->g_cpu = first_cpu;
+    create_unique_process(s);
     s->fd = fd;
     gdb_has_xml = false;
 
     gdbserver_state = s;
     return true;
@@ -2002,10 +2026,58 @@ static const TypeInfo char_gdb_type_info = {
     .name = TYPE_CHARDEV_GDB,
     .parent = TYPE_CHARDEV,
     .class_init = char_gdb_class_init,
 };
 
+static int find_cpu_clusters(Object *child, void *opaque)
+{
+    if (object_dynamic_cast(child, TYPE_CPU_CLUSTER)) {
+        GDBState *s = (GDBState *) opaque;
+        CPUClusterState *cluster = CPU_CLUSTER(child);
+        GDBProcess *process;
+
+        s->processes = g_renew(GDBProcess, s->processes, ++s->process_num);
+
+        process = &s->processes[s->process_num - 1];
+
+        /* GDB process IDs -1 and 0 are reserved */
+        process->pid = cluster->cluster_id + 1;
+        process->attached = false;
+        return 0;
+    }
+
+    return object_child_foreach(child, find_cpu_clusters, opaque);
+}
+
+static int pid_order(const void *a, const void *b)
+{
+    GDBProcess *pa = (GDBProcess *) a;
+    GDBProcess *pb = (GDBProcess *) b;
+
+    return pa->pid - pb->pid;
+}
+
+static void create_processes(GDBState *s)
+{
+    object_child_foreach(object_get_root(), find_cpu_clusters, s);
+
+    if (!s->processes) {
+        /* No CPU cluster specified by the machine */
+        create_unique_process(s);
+    } else {
+        /* Sort by PID */
+        qsort(s->processes, s->process_num, sizeof(s->processes[0]), pid_order);
+    }
+}
+
+static void cleanup_processes(GDBState *s)
+{
+    g_free(s->processes);
+    s->process_num = 0;
+    s->processes = NULL;
+}
+
 int gdbserver_start(const char *device)
 {
     trace_gdbstub_op_start(device);
 
     GDBState *s;
@@ -2058,15 +2130,19 @@ int gdbserver_start(const char *device)
                                    NULL, &error_abort);
         monitor_init(mon_chr, 0);
     } else {
         qemu_chr_fe_deinit(&s->chr, true);
         mon_chr = s->mon_chr;
+        cleanup_processes(s);
         memset(s, 0, sizeof(GDBState));
         s->mon_chr = mon_chr;
     }
     s->c_cpu = first_cpu;
     s->g_cpu = first_cpu;
+
+    create_processes(s);
+
     if (chr) {
         qemu_chr_fe_init(&s->chr, chr, &error_abort);
         qemu_chr_fe_set_handlers(&s->chr, gdb_chr_can_receive, gdb_chr_receive,
                                  gdb_chr_event, NULL, NULL, NULL, true);
     }
-- 
2.19.1

[Qemu-devel] [PATCH v5 03/16] gdbstub: add multiprocess support to '?' packets

[Luc Michel]

The gdb_get_cpu_pid() function does the PID lookup for the given CPU. It
checks if the CPU is a direct child of a CPU cluster. If it is, the
returned PID is the cluster ID plus one (cluster IDs start at 0, GDB
PIDs at 1). When the CPU is not a child of such a container, the PID of
the first process is returned.

The gdb_fmt_thread_id() function generates the string to be used to identify
a given thread, in a response packet for the peer. This function
supports generating thread IDs when multiprocess mode is enabled (in the
form `p<pid>.<tid>').

Use them in the reply to a '?' request.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 gdbstub.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 0d70b89598..d26bad4b67 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -638,10 +638,52 @@ static int memtox(char *buf, const char *mem, int len)
         }
     }
     return p - buf;
 }
 
+static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
+{
+#ifndef CONFIG_USER_ONLY
+    gchar *path, *name;
+    Object *obj;
+    CPUClusterState *cluster;
+    uint32_t ret;
+
+    path = object_get_canonical_path(OBJECT(cpu));
+    name = object_get_canonical_path_component(OBJECT(cpu));
+
+    if (path == NULL) {
+        ret = s->processes[0].pid;
+        goto out;
+    }
+
+    /*
+     * Retrieve the CPU parent path by removing the last '/' and the CPU name
+     * from the CPU canonical path. */
+    path[strlen(path) - strlen(name) - 1] = '\0';
+
+    obj = object_resolve_path_type(path, TYPE_CPU_CLUSTER, NULL);
+
+    if (obj == NULL) {
+        ret = s->processes[0].pid;
+        goto out;
+    }
+
+    cluster = CPU_CLUSTER(obj);
+    ret = cluster->cluster_id + 1;
+
+out:
+    g_free(name);
+    g_free(path);
+
+    return ret;
+
+#else
+    return s->processes[0].pid;
+#endif
+}
+
 static const char *get_feature_xml(const char *p, const char **newp,
                                    CPUClass *cc)
 {
     size_t len;
     int i;
@@ -907,10 +949,23 @@ static CPUState *find_cpu(uint32_t thread_id)
     }
 
     return NULL;
 }
 
+static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu,
+                           char *buf, size_t buf_size)
+{
+    if (s->multiprocess) {
+        snprintf(buf, buf_size, "p%02x.%02x",
+                 gdb_get_cpu_pid(s, cpu), cpu_gdb_index(cpu));
+    } else {
+        snprintf(buf, buf_size, "%02x", cpu_gdb_index(cpu));
+    }
+
+    return buf;
+}
+
 static int is_query_packet(const char *p, const char *query, char separator)
 {
     unsigned int query_len = strlen(query);
 
     return strncmp(p, query, query_len) == 0 &&
@@ -1018,22 +1073,23 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     const char *p;
     uint32_t thread;
     int ch, reg_size, type, res;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
+    char thread_id[16];
     uint8_t *registers;
     target_ulong addr, len;
 
     trace_gdbstub_io_command(line_buf);
 
     p = line_buf;
     ch = *p++;
     switch(ch) {
     case '?':
         /* TODO: Make this return the correct value for user-mode.  */
-        snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
-                 cpu_gdb_index(s->c_cpu));
+        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
+                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
         put_packet(s, buf);
         /* Remove all the breakpoints when this query is issued,
          * because gdb is doing and initial connect and the state
          * should be cleaned up.
          */
-- 
2.19.1

[Qemu-devel] [PATCH v5 04/16] gdbstub: add multiprocess support to 'H' and 'T' packets

[Luc Michel]

Add a couple of helper functions to cope with GDB threads and processes.

The gdb_get_process() function looks for a process given a pid.

The gdb_get_cpu() function returns the CPU corresponding to the (pid,
tid) pair given as parameters.

The read_thread_id() function parses the thread-id sent by the peer.
This function supports the multiprocess extension thread-id syntax.  The
return value specifies if the parsing failed, or if a special case was
encountered (all processes or all threads).

Use them in 'H' and 'T' packets handling to support the multiprocess
extension.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 134 insertions(+), 18 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index d26bad4b67..598a89b3ba 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -680,10 +680,73 @@ out:
 #else
     return s->processes[0].pid;
 #endif
 }
 
+static GDBProcess *gdb_get_process(const GDBState *s, uint32_t pid)
+{
+    int i;
+
+    if (!pid) {
+        /* 0 means any process, we take the first one */
+        return &s->processes[0];
+    }
+
+    for (i = 0; i < s->process_num; i++) {
+        if (s->processes[i].pid == pid) {
+            return &s->processes[i];
+        }
+    }
+
+    return NULL;
+}
+
+static GDBProcess *gdb_get_cpu_process(const GDBState *s, CPUState *cpu)
+{
+    return gdb_get_process(s, gdb_get_cpu_pid(s, cpu));
+}
+
+static CPUState *find_cpu(uint32_t thread_id)
+{
+    CPUState *cpu;
+
+    CPU_FOREACH(cpu) {
+        if (cpu_gdb_index(cpu) == thread_id) {
+            return cpu;
+        }
+    }
+
+    return NULL;
+}
+
+static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid)
+{
+    GDBProcess *process;
+    CPUState *cpu = find_cpu(tid);
+
+    if (!tid) {
+        /* 0 means any thread, we take the first one */
+        tid = 1;
+    }
+
+    if (cpu == NULL) {
+        return NULL;
+    }
+
+    process = gdb_get_cpu_process(s, cpu);
+
+    if (process->pid != pid) {
+        return NULL;
+    }
+
+    if (!process->attached) {
+        return NULL;
+    }
+
+    return cpu;
+}
+
 static const char *get_feature_xml(const char *p, const char **newp,
                                    CPUClass *cc)
 {
     size_t len;
     int i;
@@ -936,23 +999,10 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
 
     cpu_synchronize_state(cpu);
     cpu_set_pc(cpu, pc);
 }
 
-static CPUState *find_cpu(uint32_t thread_id)
-{
-    CPUState *cpu;
-
-    CPU_FOREACH(cpu) {
-        if (cpu_gdb_index(cpu) == thread_id) {
-            return cpu;
-        }
-    }
-
-    return NULL;
-}
-
 static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu,
                            char *buf, size_t buf_size)
 {
     if (s->multiprocess) {
         snprintf(buf, buf_size, "p%02x.%02x",
@@ -962,10 +1012,64 @@ static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu,
     }
 
     return buf;
 }
 
+typedef enum GDBThreadIdKind {
+    GDB_ONE_THREAD = 0,
+    GDB_ALL_THREADS,     /* One process, all threads */
+    GDB_ALL_PROCESSES,
+    GDB_READ_THREAD_ERR
+} GDBThreadIdKind;
+
+static GDBThreadIdKind read_thread_id(const char *buf, const char **end_buf,
+                                      uint32_t *pid, uint32_t *tid)
+{
+    unsigned long p, t;
+    int ret;
+
+    if (*buf == 'p') {
+        buf++;
+        ret = qemu_strtoul(buf, &buf, 16, &p);
+
+        if (ret) {
+            return GDB_READ_THREAD_ERR;
+        }
+
+        /* Skip '.' */
+        buf++;
+    } else {
+        p = 1;
+    }
+
+    ret = qemu_strtoul(buf, &buf, 16, &t);
+
+    if (ret) {
+        return GDB_READ_THREAD_ERR;
+    }
+
+    *end_buf = buf;
+
+    if (p == -1) {
+        return GDB_ALL_PROCESSES;
+    }
+
+    if (pid) {
+        *pid = p;
+    }
+
+    if (t == -1) {
+        return GDB_ALL_THREADS;
+    }
+
+    if (tid) {
+        *tid = t;
+    }
+
+    return GDB_ONE_THREAD;
+}
+
 static int is_query_packet(const char *p, const char *query, char separator)
 {
     unsigned int query_len = strlen(query);
 
     return strncmp(p, query, query_len) == 0 &&
@@ -1070,16 +1174,18 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
     CPUClass *cc;
     const char *p;
     uint32_t thread;
+    uint32_t pid, tid;
     int ch, reg_size, type, res;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     char thread_id[16];
     uint8_t *registers;
     target_ulong addr, len;
+    GDBThreadIdKind thread_kind;
 
     trace_gdbstub_io_command(line_buf);
 
     p = line_buf;
     ch = *p++;
@@ -1283,16 +1389,22 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         else
             put_packet(s, "E22");
         break;
     case 'H':
         type = *p++;
-        thread = strtoull(p, (char **)&p, 16);
-        if (thread == -1 || thread == 0) {
+
+        thread_kind = read_thread_id(p, &p, &pid, &tid);
+        if (thread_kind == GDB_READ_THREAD_ERR) {
+            put_packet(s, "E22");
+            break;
+        }
+
+        if (thread_kind != GDB_ONE_THREAD) {
             put_packet(s, "OK");
             break;
         }
-        cpu = find_cpu(thread);
+        cpu = gdb_get_cpu(s, pid, tid);
         if (cpu == NULL) {
             put_packet(s, "E22");
             break;
         }
         switch (type) {
@@ -1308,12 +1420,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
              put_packet(s, "E22");
              break;
         }
         break;
     case 'T':
-        thread = strtoull(p, (char **)&p, 16);
-        cpu = find_cpu(thread);
+        thread_kind = read_thread_id(p, &p, &pid, &tid);
+        if (thread_kind == GDB_READ_THREAD_ERR) {
+            put_packet(s, "E22");
+            break;
+        }
+        cpu = gdb_get_cpu(s, pid, tid);
 
         if (cpu != NULL) {
             put_packet(s, "OK");
         } else {
             put_packet(s, "E22");
-- 
2.19.1

[Qemu-devel] [PATCH v5 05/16] gdbstub: add multiprocess support to vCont packets

[Luc Michel]

Add the gdb_first_cpu() and gdb_next_cpu() to iterate over all
the CPUs in currently attached processes.

Add the gdb_first_cpu_in_process() and gdb_next_cpu_in_process() to
iterate over CPUs of a given process.

Use them to add multiprocess extension support to vCont packets.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 102 insertions(+), 15 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 598a89b3ba..ba8b1a3413 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -716,10 +716,40 @@ static CPUState *find_cpu(uint32_t thread_id)
     }
 
     return NULL;
 }
 
+static CPUState *get_first_cpu_in_process(const GDBState *s,
+                                          GDBProcess *process)
+{
+    CPUState *cpu;
+
+    CPU_FOREACH(cpu) {
+        if (gdb_get_cpu_pid(s, cpu) == process->pid) {
+            return cpu;
+        }
+    }
+
+    return NULL;
+}
+
+static CPUState *gdb_next_cpu_in_process(const GDBState *s, CPUState *cpu)
+{
+    uint32_t pid = gdb_get_cpu_pid(s, cpu);
+    cpu = CPU_NEXT(cpu);
+
+    while (cpu) {
+        if (gdb_get_cpu_pid(s, cpu) == pid) {
+            break;
+        }
+
+        cpu = CPU_NEXT(cpu);
+    }
+
+    return cpu;
+}
+
 static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid)
 {
     GDBProcess *process;
     CPUState *cpu = find_cpu(tid);
 
@@ -743,10 +773,41 @@ static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid)
     }
 
     return cpu;
 }
 
+/* Return the cpu following @cpu, while ignoring
+ * unattached processes.
+ */
+static CPUState *gdb_next_cpu(const GDBState *s, CPUState *cpu)
+{
+    cpu = CPU_NEXT(cpu);
+
+    while (cpu) {
+        if (gdb_get_cpu_process(s, cpu)->attached) {
+            break;
+        }
+
+        cpu = CPU_NEXT(cpu);
+    }
+
+    return cpu;
+}
+
+/* Return the first attached cpu */
+static CPUState *gdb_first_cpu(const GDBState *s)
+{
+    CPUState *cpu = first_cpu;
+    GDBProcess *process = gdb_get_cpu_process(s, cpu);
+
+    if (!process->attached) {
+        return gdb_next_cpu(s, cpu);
+    }
+
+    return cpu;
+}
+
 static const char *get_feature_xml(const char *p, const char **newp,
                                    CPUClass *cc)
 {
     size_t len;
     int i;
@@ -1081,14 +1142,16 @@ static int is_query_packet(const char *p, const char *query, char separator)
  * returns -ENOTSUP if a command is unsupported, -EINVAL or -ERANGE if there is
  *         a format error, 0 on success.
  */
 static int gdb_handle_vcont(GDBState *s, const char *p)
 {
-    int res, idx, signal = 0;
+    int res, signal = 0;
     char cur_action;
     char *newstates;
     unsigned long tmp;
+    uint32_t pid, tid;
+    GDBProcess *process;
     CPUState *cpu;
 #ifdef CONFIG_USER_ONLY
     int max_cpus = 1; /* global variable max_cpus exists only in system mode */
 
     CPU_FOREACH(cpu) {
@@ -1127,29 +1190,52 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
         } else if (cur_action != 'c' && cur_action != 's') {
             /* unknown/invalid/unsupported command */
             res = -ENOTSUP;
             goto out;
         }
-        /* thread specification. special values: (none), -1 = all; 0 = any */
-        if ((p[0] == ':' && p[1] == '-' && p[2] == '1') || (p[0] != ':')) {
-            if (*p == ':') {
-                p += 3;
-            }
-            for (idx = 0; idx < max_cpus; idx++) {
-                if (newstates[idx] == 1) {
-                    newstates[idx] = cur_action;
+
+        if (*p++ != ':') {
+            res = -ENOTSUP;
+            goto out;
+        }
+
+        switch (read_thread_id(p, &p, &pid, &tid)) {
+        case GDB_READ_THREAD_ERR:
+            res = -EINVAL;
+            goto out;
+
+        case GDB_ALL_PROCESSES:
+            cpu = gdb_first_cpu(s);
+            while (cpu) {
+                if (newstates[cpu->cpu_index] == 1) {
+                    newstates[cpu->cpu_index] = cur_action;
                 }
+
+                cpu = gdb_next_cpu(s, cpu);
             }
-        } else if (*p == ':') {
-            p++;
-            res = qemu_strtoul(p, &p, 16, &tmp);
-            if (res) {
+            break;
+
+        case GDB_ALL_THREADS:
+            process = gdb_get_process(s, pid);
+
+            if (!process->attached) {
+                res = -EINVAL;
                 goto out;
             }
 
-            /* 0 means any thread, so we pick the first valid CPU */
-            cpu = tmp ? find_cpu(tmp) : first_cpu;
+            cpu = get_first_cpu_in_process(s, process);
+            while (cpu) {
+                if (newstates[cpu->cpu_index] == 1) {
+                    newstates[cpu->cpu_index] = cur_action;
+                }
+
+                cpu = gdb_next_cpu_in_process(s, cpu);
+            }
+            break;
+
+        case GDB_ONE_THREAD:
+            cpu = gdb_get_cpu(s, pid, tid);
 
             /* invalid CPU/thread specified */
             if (!cpu) {
                 res = -EINVAL;
                 goto out;
@@ -1157,10 +1243,11 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
 
             /* only use if no previous match occourred */
             if (newstates[cpu->cpu_index] == 1) {
                 newstates[cpu->cpu_index] = cur_action;
             }
+            break;
         }
     }
     s->signal = signal;
     gdb_continue_partial(s, newstates);
 
-- 
2.19.1

[Qemu-devel] [PATCH v5 06/16] gdbstub: add multiprocess support to 'sC' packets

[Luc Michel]

Change the sC packet handling to support the multiprocess extension.
Instead of returning the first thread, we return the first thread of the
current process.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 gdbstub.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index ba8b1a3413..9be83486f5 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1546,13 +1546,18 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             type = strtoul(p, (char **)&p, 16);
             sstep_flags = type;
             put_packet(s, "OK");
             break;
         } else if (strcmp(p,"C") == 0) {
-            /* "Current thread" remains vague in the spec, so always return
-             *  the first CPU (gdb returns the first thread). */
-            put_packet(s, "QC1");
+            /* "Current thread" remains vague in the spec, so always return the
+             * first thread of the current process (gdb returns the first
+             * thread).
+             */
+            cpu = get_first_cpu_in_process(s, gdb_get_cpu_process(s, s->g_cpu));
+            snprintf(buf, sizeof(buf), "QC%s",
+                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
+            put_packet(s, buf);
             break;
         } else if (strcmp(p,"fThreadInfo") == 0) {
             s->query_cpu = first_cpu;
             goto report_cpuinfo;
         } else if (strcmp(p,"sThreadInfo") == 0) {
-- 
2.19.1

[Qemu-devel] [PATCH v5 07/16] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo

[Luc Michel]

Change the thread info related packets handling to support multiprocess
extension.

Add the CPUs class name in the extra info to help differentiate
them in multiprocess mode.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 9be83486f5..43c9211bfa 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1260,11 +1260,10 @@ out:
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
     CPUClass *cc;
     const char *p;
-    uint32_t thread;
     uint32_t pid, tid;
     int ch, reg_size, type, res;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
     char thread_id[16];
@@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             snprintf(buf, sizeof(buf), "QC%s",
                      gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
             put_packet(s, buf);
             break;
         } else if (strcmp(p,"fThreadInfo") == 0) {
-            s->query_cpu = first_cpu;
+            s->query_cpu = gdb_first_cpu(s);
             goto report_cpuinfo;
         } else if (strcmp(p,"sThreadInfo") == 0) {
         report_cpuinfo:
             if (s->query_cpu) {
-                snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu));
+                snprintf(buf, sizeof(buf), "m%s",
+                         gdb_fmt_thread_id(s, s->query_cpu,
+                                       thread_id, sizeof(thread_id)));
                 put_packet(s, buf);
-                s->query_cpu = CPU_NEXT(s->query_cpu);
+                s->query_cpu = gdb_next_cpu(s, s->query_cpu);
             } else
                 put_packet(s, "l");
             break;
         } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
-            thread = strtoull(p+16, (char **)&p, 16);
-            cpu = find_cpu(thread);
+            if (read_thread_id(p + 16, &p, &pid, &tid) == GDB_READ_THREAD_ERR) {
+                put_packet(s, "E22");
+                break;
+            }
+            cpu = gdb_get_cpu(s, pid, tid);
             if (cpu != NULL) {
                 cpu_synchronize_state(cpu);
-                /* memtohex() doubles the required space */
-                len = snprintf((char *)mem_buf, sizeof(buf) / 2,
-                               "CPU#%d [%s]", cpu->cpu_index,
-                               cpu->halted ? "halted " : "running");
+
+                if (s->multiprocess && (s->process_num > 1)) {
+                    /* Print the CPU model in multiprocess mode */
+                    ObjectClass *oc = object_get_class(OBJECT(cpu));
+                    const char *cpu_model = object_class_get_name(oc);
+                    len = snprintf((char *)mem_buf, sizeof(buf) / 2,
+                                   "CPU#%d %s [%s]", cpu->cpu_index,
+                                   cpu_model,
+                                   cpu->halted ? "halted " : "running");
+                } else {
+                    /* memtohex() doubles the required space */
+                    len = snprintf((char *)mem_buf, sizeof(buf) / 2,
+                                   "CPU#%d [%s]", cpu->cpu_index,
+                                   cpu->halted ? "halted " : "running");
+                }
                 trace_gdbstub_op_extra_info((char *)mem_buf);
                 memtohex(buf, mem_buf, len);
                 put_packet(s, buf);
             }
             break;
-- 
2.19.1

[Qemu-devel] [PATCH v5 08/16] gdbstub: add multiprocess support to Xfer:features:read:

[Luc Michel]

Change the Xfer:features:read: packet handling to support the
multiprocess extension. This packet is used to request the XML
description of the CPU. In multiprocess mode, different descriptions can
be sent for different processes.

This function now takes the process to send the description for as a
parameter, and use a buffer in the process structure to store the
generated description.

It takes the first CPU of the process to generate the description.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 43c9211bfa..aae3cce01a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -298,10 +298,12 @@ typedef struct GDBRegisterState {
 } GDBRegisterState;
 
 typedef struct GDBProcess {
     uint32_t pid;
     bool attached;
+
+    char target_xml[1024];
 } GDBProcess;
 
 enum RSState {
     RS_INACTIVE,
     RS_IDLE,
@@ -804,55 +806,57 @@ static CPUState *gdb_first_cpu(const GDBState *s)
     }
 
     return cpu;
 }
 
-static const char *get_feature_xml(const char *p, const char **newp,
-                                   CPUClass *cc)
+static const char *get_feature_xml(const GDBState *s, const char *p,
+                                   const char **newp, GDBProcess *process)
 {
     size_t len;
     int i;
     const char *name;
-    static char target_xml[1024];
+    CPUState *cpu = get_first_cpu_in_process(s, process);
+    CPUClass *cc = CPU_GET_CLASS(cpu);
 
     len = 0;
     while (p[len] && p[len] != ':')
         len++;
     *newp = p + len;
 
     name = NULL;
     if (strncmp(p, "target.xml", len) == 0) {
+        char *buf = process->target_xml;
+        const size_t buf_sz = sizeof(process->target_xml);
+
         /* Generate the XML description for this CPU.  */
-        if (!target_xml[0]) {
+        if (!buf[0]) {
             GDBRegisterState *r;
-            CPUState *cpu = first_cpu;
 
-            pstrcat(target_xml, sizeof(target_xml),
+            pstrcat(buf, buf_sz,
                     "<?xml version=\"1.0\"?>"
                     "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
                     "<target>");
             if (cc->gdb_arch_name) {
                 gchar *arch = cc->gdb_arch_name(cpu);
-                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
-                pstrcat(target_xml, sizeof(target_xml), arch);
-                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
+                pstrcat(buf, buf_sz, "<architecture>");
+                pstrcat(buf, buf_sz, arch);
+                pstrcat(buf, buf_sz, "</architecture>");
                 g_free(arch);
             }
-            pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
-            pstrcat(target_xml, sizeof(target_xml), cc->gdb_core_xml_file);
-            pstrcat(target_xml, sizeof(target_xml), "\"/>");
+            pstrcat(buf, buf_sz, "<xi:include href=\"");
+            pstrcat(buf, buf_sz, cc->gdb_core_xml_file);
+            pstrcat(buf, buf_sz, "\"/>");
             for (r = cpu->gdb_regs; r; r = r->next) {
-                pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
-                pstrcat(target_xml, sizeof(target_xml), r->xml);
-                pstrcat(target_xml, sizeof(target_xml), "\"/>");
+                pstrcat(buf, buf_sz, "<xi:include href=\"");
+                pstrcat(buf, buf_sz, r->xml);
+                pstrcat(buf, buf_sz, "\"/>");
             }
-            pstrcat(target_xml, sizeof(target_xml), "</target>");
+            pstrcat(buf, buf_sz, "</target>");
         }
-        return target_xml;
+        return buf;
     }
     if (cc->gdb_get_dynamic_xml) {
-        CPUState *cpu = first_cpu;
         char *xmlname = g_strndup(p, len);
         const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
 
         g_free(xmlname);
         if (xml) {
@@ -1258,10 +1262,11 @@ out:
 }
 
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
+    GDBProcess *process;
     CPUClass *cc;
     const char *p;
     uint32_t pid, tid;
     int ch, reg_size, type, res;
     uint8_t mem_buf[MAX_PACKET_LENGTH];
@@ -1639,18 +1644,19 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
         if (strncmp(p, "Xfer:features:read:", 19) == 0) {
             const char *xml;
             target_ulong total_len;
 
-            cc = CPU_GET_CLASS(first_cpu);
+            process = gdb_get_cpu_process(s, s->g_cpu);
+            cc = CPU_GET_CLASS(s->g_cpu);
             if (cc->gdb_core_xml_file == NULL) {
                 goto unknown_command;
             }
 
             gdb_has_xml = true;
             p += 19;
-            xml = get_feature_xml(p, &p, cc);
+            xml = get_feature_xml(s, p, &p, process);
             if (!xml) {
                 snprintf(buf, sizeof(buf), "E00");
                 put_packet(s, buf);
                 break;
             }
@@ -2319,10 +2325,11 @@ static int find_cpu_clusters(Object *child, void *opaque)
         process = &s->processes[s->process_num - 1];
 
         /* GDB process IDs -1 and 0 are reserved */
         process->pid = cluster->cluster_id + 1;
         process->attached = false;
+        process->target_xml[0] = '\0';
         return 0;
     }
 
     return object_child_foreach(child, find_cpu_clusters, opaque);
 }
-- 
2.19.1

[Qemu-devel] [PATCH v5 09/16] gdbstub: add multiprocess support to gdb_vm_state_change()

[Luc Michel]

Add support for multiprocess extension in gdb_vm_state_change()
function.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index aae3cce01a..decf56c610 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1713,10 +1713,11 @@ void gdb_set_stop_cpu(CPUState *cpu)
 static void gdb_vm_state_change(void *opaque, int running, RunState state)
 {
     GDBState *s = gdbserver_state;
     CPUState *cpu = s->c_cpu;
     char buf[256];
+    char thread_id[16];
     const char *type;
     int ret;
 
     if (running || s->state == RS_INACTIVE) {
         return;
@@ -1724,10 +1725,18 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
     /* Is there a GDB syscall waiting to be sent?  */
     if (s->current_syscall_cb) {
         put_packet(s, s->syscall_buf);
         return;
     }
+
+    if (cpu == NULL) {
+        /* No process attached */
+        return;
+    }
+
+    gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id));
+
     switch (state) {
     case RUN_STATE_DEBUG:
         if (cpu->watchpoint_hit) {
             switch (cpu->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
@@ -1741,12 +1750,12 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
                 break;
             }
             trace_gdbstub_hit_watchpoint(type, cpu_gdb_index(cpu),
                     (target_ulong)cpu->watchpoint_hit->vaddr);
             snprintf(buf, sizeof(buf),
-                     "T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
-                     GDB_SIGNAL_TRAP, cpu_gdb_index(cpu), type,
+                     "T%02xthread:%s;%swatch:" TARGET_FMT_lx ";",
+                     GDB_SIGNAL_TRAP, thread_id, type,
                      (target_ulong)cpu->watchpoint_hit->vaddr);
             cpu->watchpoint_hit = NULL;
             goto send_packet;
         } else {
             trace_gdbstub_hit_break();
@@ -1784,11 +1793,11 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state)
         trace_gdbstub_hit_unknown(state);
         ret = GDB_SIGNAL_UNKNOWN;
         break;
     }
     gdb_set_stop_cpu(cpu);
-    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, cpu_gdb_index(cpu));
+    snprintf(buf, sizeof(buf), "T%02xthread:%s;", ret, thread_id);
 
 send_packet:
     put_packet(s, buf);
 
     /* disable single step if it was enabled */
-- 
2.19.1

[Qemu-devel] [PATCH v5 10/16] gdbstub: add multiprocess support to 'D' packets

[Luc Michel]

'D' packets are used by GDB to detach from a process. In multiprocess
mode, the PID to detach from is sent in the request.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 gdbstub.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 7 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index decf56c610..bd4895ac0a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1039,24 +1039,39 @@ static int gdb_breakpoint_remove(target_ulong addr, target_ulong len, int type)
     default:
         return -ENOSYS;
     }
 }
 
+static inline void gdb_cpu_breakpoint_remove_all(CPUState *cpu)
+{
+    cpu_breakpoint_remove_all(cpu, BP_GDB);
+#ifndef CONFIG_USER_ONLY
+    cpu_watchpoint_remove_all(cpu, BP_GDB);
+#endif
+}
+
+static void gdb_process_breakpoint_remove_all(const GDBState *s, GDBProcess *p)
+{
+    CPUState *cpu = get_first_cpu_in_process(s, p);
+
+    while (cpu) {
+        gdb_cpu_breakpoint_remove_all(cpu);
+        cpu = gdb_next_cpu_in_process(s, cpu);
+    }
+}
+
 static void gdb_breakpoint_remove_all(void)
 {
     CPUState *cpu;
 
     if (kvm_enabled()) {
         kvm_remove_all_breakpoints(gdbserver_state->c_cpu);
         return;
     }
 
     CPU_FOREACH(cpu) {
-        cpu_breakpoint_remove_all(cpu, BP_GDB);
-#ifndef CONFIG_USER_ONLY
-        cpu_watchpoint_remove_all(cpu, BP_GDB);
-#endif
+        gdb_cpu_breakpoint_remove_all(cpu);
     }
 }
 
 static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
 {
@@ -1331,13 +1346,44 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         /* Kill the target */
         error_report("QEMU: Terminated via GDBstub");
         exit(0);
     case 'D':
         /* Detach packet */
-        gdb_breakpoint_remove_all();
-        gdb_syscall_mode = GDB_SYS_DISABLED;
-        gdb_continue(s);
+        pid = 1;
+
+        if (s->multiprocess) {
+            unsigned long lpid;
+            if (*p != ';') {
+                put_packet(s, "E22");
+                break;
+            }
+
+            if (qemu_strtoul(p + 1, &p, 16, &lpid)) {
+                put_packet(s, "E22");
+                break;
+            }
+
+            pid = lpid;
+        }
+
+        process = gdb_get_process(s, pid);
+        gdb_process_breakpoint_remove_all(s, process);
+        process->attached = false;
+
+        if (pid == gdb_get_cpu_pid(s, s->c_cpu)) {
+            s->c_cpu = gdb_first_cpu(s);
+        }
+
+        if (pid == gdb_get_cpu_pid(s, s->g_cpu)) {
+            s->g_cpu = gdb_first_cpu(s);
+        }
+
+        if (s->c_cpu == NULL) {
+            /* No more process attached */
+            gdb_syscall_mode = GDB_SYS_DISABLED;
+            gdb_continue(s);
+        }
         put_packet(s, "OK");
         break;
     case 's':
         if (*p != '\0') {
             addr = strtoull(p, (char **)&p, 16);
-- 
2.19.1

[Qemu-devel] [PATCH v5 11/16] gdbstub: add support for extended mode packet

[Luc Michel]

Add support for the '!' extended mode packet. This is required for the
multiprocess extension.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
 gdbstub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index bd4895ac0a..4132227092 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1294,10 +1294,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     trace_gdbstub_io_command(line_buf);
 
     p = line_buf;
     ch = *p++;
     switch(ch) {
+    case '!':
+        put_packet(s, "OK");
+        break;
     case '?':
         /* TODO: Make this return the correct value for user-mode.  */
         snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
                  gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
         put_packet(s, buf);
-- 
2.19.1

[Qemu-devel] [PATCH v5 12/16] gdbstub: add support for vAttach packets

[Luc Michel]

Add support for the vAttach packets. In multiprocess mode, GDB sends
them to attach to additional processes.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
 gdbstub.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index 4132227092..ba365808db 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1340,10 +1340,45 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
                     break;
                 }
                 goto unknown_command;
             }
             break;
+        } else if (strncmp(p, "Attach;", 7) == 0) {
+            unsigned long pid;
+
+            p += 7;
+
+            if (qemu_strtoul(p, &p, 16, &pid)) {
+                put_packet(s, "E22");
+                break;
+            }
+
+            process = gdb_get_process(s, pid);
+
+            if (process == NULL) {
+                put_packet(s, "E22");
+                break;
+            }
+
+            cpu = get_first_cpu_in_process(s, process);
+
+            if (cpu == NULL) {
+                /* Refuse to attach an empty process */
+                put_packet(s, "E22");
+                break;
+            }
+
+            process->attached = true;
+
+            s->g_cpu = cpu;
+            s->c_cpu = cpu;
+
+            snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
+                     gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id)));
+
+            put_packet(s, buf);
+            break;
         } else {
             goto unknown_command;
         }
     case 'k':
         /* Kill the target */
-- 
2.19.1

[Qemu-devel] [PATCH v5 13/16] gdbstub: processes initialization on new peer connection

[Luc Michel]

When a new connection is established, we set the first process to be
attached, and the others detached. The first CPU of the first process
is selected as the current CPU.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 gdbstub.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index ba365808db..1a3353ff3c 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2245,13 +2245,14 @@ static bool gdb_accept(void)
         close(fd);
         return false;
     }
 
     s = g_malloc0(sizeof(GDBState));
-    s->c_cpu = first_cpu;
-    s->g_cpu = first_cpu;
     create_unique_process(s);
+    s->processes[0].attached = true;
+    s->c_cpu = gdb_first_cpu(s);
+    s->g_cpu = s->c_cpu;
     s->fd = fd;
     gdb_has_xml = false;
 
     gdbserver_state = s;
     return true;
@@ -2333,12 +2334,23 @@ static void gdb_chr_receive(void *opaque, const uint8_t *buf, int size)
     }
 }
 
 static void gdb_chr_event(void *opaque, int event)
 {
+    int i;
+    GDBState *s = (GDBState *) opaque;
+
     switch (event) {
     case CHR_EVENT_OPENED:
+        /* Start with first process attached, others detached */
+        for (i = 0; i < s->process_num; i++) {
+            s->processes[i].attached = !i;
+        }
+
+        s->c_cpu = gdb_first_cpu(s);
+        s->g_cpu = s->c_cpu;
+
         vm_stop(RUN_STATE_PAUSED);
         gdb_has_xml = false;
         break;
     default:
         break;
@@ -2513,19 +2525,17 @@ int gdbserver_start(const char *device)
         mon_chr = s->mon_chr;
         cleanup_processes(s);
         memset(s, 0, sizeof(GDBState));
         s->mon_chr = mon_chr;
     }
-    s->c_cpu = first_cpu;
-    s->g_cpu = first_cpu;
 
     create_processes(s);
 
     if (chr) {
         qemu_chr_fe_init(&s->chr, chr, &error_abort);
         qemu_chr_fe_set_handlers(&s->chr, gdb_chr_can_receive, gdb_chr_receive,
-                                 gdb_chr_event, NULL, NULL, NULL, true);
+                                 gdb_chr_event, NULL, s, NULL, true);
     }
     s->state = chr ? RS_IDLE : RS_INACTIVE;
     s->mon_chr = mon_chr;
     s->current_syscall_cb = NULL;
 
-- 
2.19.1

[Qemu-devel] [PATCH v5 14/16] gdbstub: gdb_set_stop_cpu: ignore request when process is not attached

[Luc Michel]

When gdb_set_stop_cpu() is called with a CPU associated to a process
currently not attached by the GDB client, return without modifying the
stop CPU. Otherwise, GDB gets confused if it receives packets with a
thread-id it does not know about.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 gdbstub.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index 1a3353ff3c..c662ada6ff 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1787,10 +1787,19 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     return RS_IDLE;
 }
 
 void gdb_set_stop_cpu(CPUState *cpu)
 {
+    GDBProcess *p = gdb_get_cpu_process(gdbserver_state, cpu);
+
+    if (!p->attached) {
+        /* Having a stop CPU corresponding to a process that is not attached
+         * confuses GDB. So we ignore the request.
+         */
+        return;
+    }
+
     gdbserver_state->c_cpu = cpu;
     gdbserver_state->g_cpu = cpu;
 }
 
 #ifndef CONFIG_USER_ONLY
-- 
2.19.1

[Qemu-devel] [PATCH v5 15/16] gdbstub: add multiprocess extension support

[Luc Michel]

Add multiprocess extension support by enabling multiprocess mode when
the peer requests it, and by replying that we actually support it in the
qSupported reply packet.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 gdbstub.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index c662ada6ff..93a7c956c2 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1721,10 +1721,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             snprintf(buf, sizeof(buf), "PacketSize=%x", MAX_PACKET_LENGTH);
             cc = CPU_GET_CLASS(first_cpu);
             if (cc->gdb_core_xml_file != NULL) {
                 pstrcat(buf, sizeof(buf), ";qXfer:features:read+");
             }
+
+            if (strstr(p, "multiprocess+")) {
+                s->multiprocess = true;
+            }
+            pstrcat(buf, sizeof(buf), ";multiprocess+");
+
             put_packet(s, buf);
             break;
         }
         if (strncmp(p, "Xfer:features:read:", 19) == 0) {
             const char *xml;
-- 
2.19.1

[Qemu-devel] [PATCH v5 16/16] arm/xlnx-zynqmp: put APUs and RPUs in separate CPU clusters

[Luc Michel]

Create two separate CPU clusters for APUs and RPUs.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
 include/hw/arm/xlnx-zynqmp.h |  3 +++
 hw/arm/xlnx-zynqmp.c         | 21 +++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index 98f925ab84..591515c760 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -29,10 +29,11 @@
 #include "hw/dma/xlnx_dpdma.h"
 #include "hw/dma/xlnx-zdma.h"
 #include "hw/display/xlnx_dp.h"
 #include "hw/intc/xlnx-zynqmp-ipi.h"
 #include "hw/timer/xlnx-zynqmp-rtc.h"
+#include "hw/cpu/cluster.h"
 
 #define TYPE_XLNX_ZYNQMP "xlnx,zynqmp"
 #define XLNX_ZYNQMP(obj) OBJECT_CHECK(XlnxZynqMPState, (obj), \
                                        TYPE_XLNX_ZYNQMP)
 
@@ -75,10 +76,12 @@
 typedef struct XlnxZynqMPState {
     /*< private >*/
     DeviceState parent_obj;
 
     /*< public >*/
+    CPUClusterState apu_cluster;
+    CPUClusterState rpu_cluster;
     ARMCPU apu_cpu[XLNX_ZYNQMP_NUM_APU_CPUS];
     ARMCPU rpu_cpu[XLNX_ZYNQMP_NUM_RPU_CPUS];
     GICState gic;
     MemoryRegion gic_mr[XLNX_ZYNQMP_GIC_REGIONS][XLNX_ZYNQMP_GIC_ALIASES];
 
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index c195040350..42a138074c 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -176,16 +176,22 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
 {
     Error *err = NULL;
     int i;
     int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
 
+    object_initialize_child(OBJECT(s), "rpu-cluster", &s->rpu_cluster,
+                            sizeof(s->rpu_cluster), TYPE_CPU_CLUSTER,
+                            &error_abort, NULL);
+
+    qdev_init_nofail(DEVICE(&s->rpu_cluster));
+
     for (i = 0; i < num_rpus; i++) {
         char *name;
 
         object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
                           "cortex-r5f-" TYPE_ARM_CPU);
-        object_property_add_child(OBJECT(s), "rpu-cpu[*]",
+        object_property_add_child(OBJECT(&s->rpu_cluster), "rpu-cpu[*]",
                                   OBJECT(&s->rpu_cpu[i]), &error_abort);
 
         name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
         if (strcmp(name, boot_cpu)) {
             /* Secondary CPUs start in PSCI powered-down state */
@@ -211,14 +217,19 @@ static void xlnx_zynqmp_init(Object *obj)
 {
     XlnxZynqMPState *s = XLNX_ZYNQMP(obj);
     int i;
     int num_apus = MIN(smp_cpus, XLNX_ZYNQMP_NUM_APU_CPUS);
 
+    object_initialize_child(obj, "apu-cluster", &s->apu_cluster,
+                            sizeof(s->apu_cluster), TYPE_CPU_CLUSTER,
+                            &error_abort, NULL);
+
     for (i = 0; i < num_apus; i++) {
-        object_initialize_child(obj, "apu-cpu[*]", &s->apu_cpu[i],
-                                sizeof(s->apu_cpu[i]),
-                                "cortex-a53-" TYPE_ARM_CPU, &error_abort, NULL);
+        object_initialize_child(OBJECT(&s->apu_cluster), "apu-cpu[*]",
+                                &s->apu_cpu[i], sizeof(s->apu_cpu[i]),
+                                "cortex-a53-" TYPE_ARM_CPU, &error_abort,
+                                NULL);
     }
 
     sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic),
                           gic_class_name());
 
@@ -331,10 +342,12 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", num_apus);
     qdev_prop_set_bit(DEVICE(&s->gic), "has-security-extensions", s->secure);
     qdev_prop_set_bit(DEVICE(&s->gic),
                       "has-virtualization-extensions", s->virt);
 
+    qdev_init_nofail(DEVICE(&s->apu_cluster));
+
     /* Realize APUs before realizing the GIC. KVM requires this.  */
     for (i = 0; i < num_apus; i++) {
         char *name;
 
         object_property_set_int(OBJECT(&s->apu_cpu[i]), QEMU_PSCI_CONDUIT_SMC,
-- 
2.19.1

Re: [Qemu-devel] [Qemu-arm] [PATCH v5 01/16] hw/cpu: introduce CPU clusters

[Edgar E. Iglesias]

On Sat, Nov 10, 2018 at 09:11:32AM +0100, Luc Michel wrote:
> This commit adds the cpu-cluster type. It aims at gathering CPUs from
> the same cluster in a machine.
> 
> For now it only has a `cluster-id` property.
> 
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  include/hw/cpu/cluster.h | 38 ++++++++++++++++++++++++++
>  hw/cpu/cluster.c         | 59 ++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS              |  2 ++
>  hw/cpu/Makefile.objs     |  2 +-
>  4 files changed, 100 insertions(+), 1 deletion(-)
>  create mode 100644 include/hw/cpu/cluster.h
>  create mode 100644 hw/cpu/cluster.c
> 
> diff --git a/include/hw/cpu/cluster.h b/include/hw/cpu/cluster.h
> new file mode 100644
> index 0000000000..11f50d5f6b
> --- /dev/null
> +++ b/include/hw/cpu/cluster.h
> @@ -0,0 +1,38 @@
> +/*
> + * QEMU CPU cluster
> + *
> + * Copyright (c) 2018 GreenSocs SAS
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see
> + * <http://www.gnu.org/licenses/gpl-2.0.html>
> + */
> +#ifndef HW_CPU_CLUSTER_H
> +#define HW_CPU_CLUSTER_H
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev.h"
> +
> +#define TYPE_CPU_CLUSTER "cpu-cluster"
> +#define CPU_CLUSTER(obj) \
> +    OBJECT_CHECK(CPUClusterState, (obj), TYPE_CPU_CLUSTER)
> +
> +typedef struct CPUClusterState {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    uint32_t cluster_id;
> +} CPUClusterState;
> +
> +#endif
> diff --git a/hw/cpu/cluster.c b/hw/cpu/cluster.c
> new file mode 100644
> index 0000000000..e0ffd76152
> --- /dev/null
> +++ b/hw/cpu/cluster.c
> @@ -0,0 +1,59 @@
> +/*
> + * QEMU CPU cluster
> + *
> + * Copyright (c) 2018 GreenSocs SAS
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see
> + * <http://www.gnu.org/licenses/gpl-2.0.html>
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/cpu/cluster.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +
> +static void cpu_cluster_init(Object *obj)
> +{
> +    static uint32_t cluster_id_auto_increment;
> +    CPUClusterState *cluster = CPU_CLUSTER(obj);
> +
> +    cluster->cluster_id = cluster_id_auto_increment++;
> +}
> +
> +static Property cpu_cluster_properties[] = {
> +    DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void cpu_cluster_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = cpu_cluster_properties;
> +}
> +
> +static const TypeInfo cpu_cluster_type_info = {
> +    .name = TYPE_CPU_CLUSTER,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(CPUClusterState),
> +    .instance_init = cpu_cluster_init,
> +    .class_init = cpu_cluster_class_init,
> +};
> +
> +static void cpu_cluster_register_types(void)
> +{
> +    type_register_static(&cpu_cluster_type_info);
> +}
> +
> +type_init(cpu_cluster_register_types)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 126fe0be7e..8e20b0e672 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1022,11 +1022,13 @@ Machine core
>  M: Eduardo Habkost <ehabkost@redhat.com>
>  M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>  S: Supported
>  F: hw/core/machine.c
>  F: hw/core/null-machine.c
> +F: hw/cpu/cluster.c
>  F: include/hw/boards.h
> +F: include/hw/cpu/cluster.h
>  T: git git://github.com/ehabkost/qemu.git machine-next
>  
>  Xtensa Machines
>  ---------------
>  sim
> diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> index cd52d20b65..8db9e8a7b3 100644
> --- a/hw/cpu/Makefile.objs
> +++ b/hw/cpu/Makefile.objs
> @@ -1,5 +1,5 @@
>  obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
>  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
>  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
>  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> -common-obj-y += core.o
> +common-obj-y += core.o cluster.o
> -- 
> 2.19.1
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v5 02/16] gdbstub: introduce GDB processes

[Edgar E. Iglesias]

On Sat, Nov 10, 2018 at 09:11:33AM +0100, Luc Michel wrote:
> Add a structure GDBProcess that represent processes from the GDB
> semantic point of view.
> 
> CPUs can be split into different processes, by grouping them under
> different cpu-cluster objects.  Each occurrence of a cpu-cluster object
> implies the existence of the corresponding process in the GDB stub. The
> GDB process ID is derived from the corresponding cluster ID as follows:
> 
>   GDB PID = cluster ID + 1
> 
> This is because PIDs -1 and 0 are reserved in GDB and cannot be used by
> processes.
> 
> When no such container are found, all the CPUs are put in a unique GDB
> process (create_unique_process()). This is also the case when compiled
> in user mode, where multi-processes do not make much sense for now.
> 
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  gdbstub.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index c4e4f9f082..0d70b89598 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -27,10 +27,11 @@
>  #include "monitor/monitor.h"
>  #include "chardev/char.h"
>  #include "chardev/char-fe.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/gdbstub.h"
> +#include "hw/cpu/cluster.h"
>  #endif
>  
>  #define MAX_PACKET_LENGTH 4096
>  
>  #include "qemu/sockets.h"
> @@ -294,10 +295,15 @@ typedef struct GDBRegisterState {
>      gdb_reg_cb set_reg;
>      const char *xml;
>      struct GDBRegisterState *next;
>  } GDBRegisterState;
>  
> +typedef struct GDBProcess {
> +    uint32_t pid;
> +    bool attached;
> +} GDBProcess;
> +
>  enum RSState {
>      RS_INACTIVE,
>      RS_IDLE,
>      RS_GETLINE,
>      RS_GETLINE_ESC,
> @@ -322,10 +328,13 @@ typedef struct GDBState {
>      int running_state;
>  #else
>      CharBackend chr;
>      Chardev *mon_chr;
>  #endif
> +    bool multiprocess;
> +    GDBProcess *processes;
> +    int process_num;
>      char syscall_buf[256];
>      gdb_syscall_complete_cb current_syscall_cb;
>  } GDBState;
>  
>  /* By default use no IRQs and no timers while single stepping so as to
> @@ -1749,10 +1758,24 @@ void gdb_exit(CPUArchState *env, int code)
>  #ifndef CONFIG_USER_ONLY
>    qemu_chr_fe_deinit(&s->chr, true);
>  #endif
>  }
>  
> +/*
> + * Create a unique process containing all the CPUs.
> + */
> +static void create_unique_process(GDBState *s)
> +{
> +    GDBProcess *process;
> +
> +    s->processes = g_malloc0(sizeof(GDBProcess));
> +    s->process_num = 1;
> +    process = &s->processes[0];
> +
> +    process->pid = 1;
> +}
> +
>  #ifdef CONFIG_USER_ONLY
>  int
>  gdb_handlesig(CPUState *cpu, int sig)
>  {
>      GDBState *s;
> @@ -1846,10 +1869,11 @@ static bool gdb_accept(void)
>      }
>  
>      s = g_malloc0(sizeof(GDBState));
>      s->c_cpu = first_cpu;
>      s->g_cpu = first_cpu;
> +    create_unique_process(s);
>      s->fd = fd;
>      gdb_has_xml = false;
>  
>      gdbserver_state = s;
>      return true;
> @@ -2002,10 +2026,58 @@ static const TypeInfo char_gdb_type_info = {
>      .name = TYPE_CHARDEV_GDB,
>      .parent = TYPE_CHARDEV,
>      .class_init = char_gdb_class_init,
>  };
>  
> +static int find_cpu_clusters(Object *child, void *opaque)
> +{
> +    if (object_dynamic_cast(child, TYPE_CPU_CLUSTER)) {
> +        GDBState *s = (GDBState *) opaque;
> +        CPUClusterState *cluster = CPU_CLUSTER(child);
> +        GDBProcess *process;
> +
> +        s->processes = g_renew(GDBProcess, s->processes, ++s->process_num);
> +
> +        process = &s->processes[s->process_num - 1];
> +
> +        /* GDB process IDs -1 and 0 are reserved */
> +        process->pid = cluster->cluster_id + 1;

This may be theoretical but since both pid and cluster_id are uint32_t
you may end up with 0 here.

You may want to limit cluster_id's to something like the range 0 - INT32_MAX.



> +        process->attached = false;
> +        return 0;
> +    }
> +
> +    return object_child_foreach(child, find_cpu_clusters, opaque);
> +}
> +
> +static int pid_order(const void *a, const void *b)
> +{
> +    GDBProcess *pa = (GDBProcess *) a;
> +    GDBProcess *pb = (GDBProcess *) b;
> +
> +    return pa->pid - pb->pid;

Similarly here, is this safe?
e.g:
pa->pid = 1
pb->pid = 0x80000002


Otherwise:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> +}
> +
> +static void create_processes(GDBState *s)
> +{
> +    object_child_foreach(object_get_root(), find_cpu_clusters, s);
> +
> +    if (!s->processes) {
> +        /* No CPU cluster specified by the machine */
> +        create_unique_process(s);
> +    } else {
> +        /* Sort by PID */
> +        qsort(s->processes, s->process_num, sizeof(s->processes[0]), pid_order);
> +    }
> +}
> +
> +static void cleanup_processes(GDBState *s)
> +{
> +    g_free(s->processes);
> +    s->process_num = 0;
> +    s->processes = NULL;
> +}
> +
>  int gdbserver_start(const char *device)
>  {
>      trace_gdbstub_op_start(device);
>  
>      GDBState *s;
> @@ -2058,15 +2130,19 @@ int gdbserver_start(const char *device)
>                                     NULL, &error_abort);
>          monitor_init(mon_chr, 0);
>      } else {
>          qemu_chr_fe_deinit(&s->chr, true);
>          mon_chr = s->mon_chr;
> +        cleanup_processes(s);
>          memset(s, 0, sizeof(GDBState));
>          s->mon_chr = mon_chr;
>      }
>      s->c_cpu = first_cpu;
>      s->g_cpu = first_cpu;
> +
> +    create_processes(s);
> +
>      if (chr) {
>          qemu_chr_fe_init(&s->chr, chr, &error_abort);
>          qemu_chr_fe_set_handlers(&s->chr, gdb_chr_can_receive, gdb_chr_receive,
>                                   gdb_chr_event, NULL, NULL, NULL, true);
>      }
> -- 
> 2.19.1
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v5 03/16] gdbstub: add multiprocess support to '?' packets

[Edgar E. Iglesias]

On Sat, Nov 10, 2018 at 09:11:34AM +0100, Luc Michel wrote:
> The gdb_get_cpu_pid() function does the PID lookup for the given CPU. It
> checks if the CPU is a direct child of a CPU cluster. If it is, the
> returned PID is the cluster ID plus one (cluster IDs start at 0, GDB
> PIDs at 1). When the CPU is not a child of such a container, the PID of
> the first process is returned.
> 
> The gdb_fmt_thread_id() function generates the string to be used to identify
> a given thread, in a response packet for the peer. This function
> supports generating thread IDs when multiprocess mode is enabled (in the
> form `p<pid>.<tid>').
> 
> Use them in the reply to a '?' request.
> 
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>


This is also theoretical but:
When looking at this, it seems like you could have lazily created
the s->processes array entries (if you find a cluster but the
no valid entry in s->processes). Then we could perhaps eliminate the
scan of all objects at startup and also support CPU/Cluster hotplug.

Anyway, this looks good to me!
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Cheers,
Edgar


> ---
>  gdbstub.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 0d70b89598..d26bad4b67 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -638,10 +638,52 @@ static int memtox(char *buf, const char *mem, int len)
>          }
>      }
>      return p - buf;
>  }
>  
> +static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    gchar *path, *name;
> +    Object *obj;
> +    CPUClusterState *cluster;
> +    uint32_t ret;
> +
> +    path = object_get_canonical_path(OBJECT(cpu));
> +    name = object_get_canonical_path_component(OBJECT(cpu));
> +
> +    if (path == NULL) {
> +        ret = s->processes[0].pid;
> +        goto out;
> +    }
> +
> +    /*
> +     * Retrieve the CPU parent path by removing the last '/' and the CPU name
> +     * from the CPU canonical path. */
> +    path[strlen(path) - strlen(name) - 1] = '\0';
> +
> +    obj = object_resolve_path_type(path, TYPE_CPU_CLUSTER, NULL);
> +
> +    if (obj == NULL) {
> +        ret = s->processes[0].pid;
> +        goto out;
> +    }
> +
> +    cluster = CPU_CLUSTER(obj);
> +    ret = cluster->cluster_id + 1;
> +
> +out:
> +    g_free(name);
> +    g_free(path);
> +
> +    return ret;
> +
> +#else
> +    return s->processes[0].pid;
> +#endif
> +}
> +
>  static const char *get_feature_xml(const char *p, const char **newp,
>                                     CPUClass *cc)
>  {
>      size_t len;
>      int i;
> @@ -907,10 +949,23 @@ static CPUState *find_cpu(uint32_t thread_id)
>      }
>  
>      return NULL;
>  }
>  
> +static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu,
> +                           char *buf, size_t buf_size)
> +{
> +    if (s->multiprocess) {
> +        snprintf(buf, buf_size, "p%02x.%02x",
> +                 gdb_get_cpu_pid(s, cpu), cpu_gdb_index(cpu));
> +    } else {
> +        snprintf(buf, buf_size, "%02x", cpu_gdb_index(cpu));
> +    }
> +
> +    return buf;
> +}
> +
>  static int is_query_packet(const char *p, const char *query, char separator)
>  {
>      unsigned int query_len = strlen(query);
>  
>      return strncmp(p, query, query_len) == 0 &&
> @@ -1018,22 +1073,23 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      const char *p;
>      uint32_t thread;
>      int ch, reg_size, type, res;
>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> +    char thread_id[16];
>      uint8_t *registers;
>      target_ulong addr, len;
>  
>      trace_gdbstub_io_command(line_buf);
>  
>      p = line_buf;
>      ch = *p++;
>      switch(ch) {
>      case '?':
>          /* TODO: Make this return the correct value for user-mode.  */
> -        snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
> -                 cpu_gdb_index(s->c_cpu));
> +        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
> +                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
>          put_packet(s, buf);
>          /* Remove all the breakpoints when this query is issued,
>           * because gdb is doing and initial connect and the state
>           * should be cleaned up.
>           */
> -- 
> 2.19.1
> 
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v5 03/16] gdbstub: add multiprocess support to '?' packets

[Luc Michel]

Hi Edgar,

On 11/13/18 12:06 PM, Edgar E. Iglesias wrote:
> On Sat, Nov 10, 2018 at 09:11:34AM +0100, Luc Michel wrote:
>> The gdb_get_cpu_pid() function does the PID lookup for the given CPU. It
>> checks if the CPU is a direct child of a CPU cluster. If it is, the
>> returned PID is the cluster ID plus one (cluster IDs start at 0, GDB
>> PIDs at 1). When the CPU is not a child of such a container, the PID of
>> the first process is returned.
>>
>> The gdb_fmt_thread_id() function generates the string to be used to identify
>> a given thread, in a response packet for the peer. This function
>> supports generating thread IDs when multiprocess mode is enabled (in the
>> form `p<pid>.<tid>').
>>
>> Use them in the reply to a '?' request.
>>
>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> 
> 
> This is also theoretical but:
> When looking at this, it seems like you could have lazily created
> the s->processes array entries (if you find a cluster but the
> no valid entry in s->processes). Then we could perhaps eliminate the
> scan of all objects at startup and also support CPU/Cluster hotplug.
Yes you are right, this could be an improvement to this series to add
cluster hotplug support (CPU hotplug is actually supported). It's a
little bit tricky though since we would have to maintain the
s->processes array and properly signal to GDB when a process dies.

Cheers,
Luc

> 
> Anyway, this looks good to me!
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> Cheers,
> Edgar
> 
> 
>> ---
>>  gdbstub.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 58 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 0d70b89598..d26bad4b67 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -638,10 +638,52 @@ static int memtox(char *buf, const char *mem, int len)
>>          }
>>      }
>>      return p - buf;
>>  }
>>  
>> +static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> +    gchar *path, *name;
>> +    Object *obj;
>> +    CPUClusterState *cluster;
>> +    uint32_t ret;
>> +
>> +    path = object_get_canonical_path(OBJECT(cpu));
>> +    name = object_get_canonical_path_component(OBJECT(cpu));
>> +
>> +    if (path == NULL) {
>> +        ret = s->processes[0].pid;
>> +        goto out;
>> +    }
>> +
>> +    /*
>> +     * Retrieve the CPU parent path by removing the last '/' and the CPU name
>> +     * from the CPU canonical path. */
>> +    path[strlen(path) - strlen(name) - 1] = '\0';
>> +
>> +    obj = object_resolve_path_type(path, TYPE_CPU_CLUSTER, NULL);
>> +
>> +    if (obj == NULL) {
>> +        ret = s->processes[0].pid;
>> +        goto out;
>> +    }
>> +
>> +    cluster = CPU_CLUSTER(obj);
>> +    ret = cluster->cluster_id + 1;
>> +
>> +out:
>> +    g_free(name);
>> +    g_free(path);
>> +
>> +    return ret;
>> +
>> +#else
>> +    return s->processes[0].pid;
>> +#endif
>> +}
>> +
>>  static const char *get_feature_xml(const char *p, const char **newp,
>>                                     CPUClass *cc)
>>  {
>>      size_t len;
>>      int i;
>> @@ -907,10 +949,23 @@ static CPUState *find_cpu(uint32_t thread_id)
>>      }
>>  
>>      return NULL;
>>  }
>>  
>> +static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu,
>> +                           char *buf, size_t buf_size)
>> +{
>> +    if (s->multiprocess) {
>> +        snprintf(buf, buf_size, "p%02x.%02x",
>> +                 gdb_get_cpu_pid(s, cpu), cpu_gdb_index(cpu));
>> +    } else {
>> +        snprintf(buf, buf_size, "%02x", cpu_gdb_index(cpu));
>> +    }
>> +
>> +    return buf;
>> +}
>> +
>>  static int is_query_packet(const char *p, const char *query, char separator)
>>  {
>>      unsigned int query_len = strlen(query);
>>  
>>      return strncmp(p, query, query_len) == 0 &&
>> @@ -1018,22 +1073,23 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>>      const char *p;
>>      uint32_t thread;
>>      int ch, reg_size, type, res;
>>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>> +    char thread_id[16];
>>      uint8_t *registers;
>>      target_ulong addr, len;
>>  
>>      trace_gdbstub_io_command(line_buf);
>>  
>>      p = line_buf;
>>      ch = *p++;
>>      switch(ch) {
>>      case '?':
>>          /* TODO: Make this return the correct value for user-mode.  */
>> -        snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
>> -                 cpu_gdb_index(s->c_cpu));
>> +        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
>> +                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
>>          put_packet(s, buf);
>>          /* Remove all the breakpoints when this query is issued,
>>           * because gdb is doing and initial connect and the state
>>           * should be cleaned up.
>>           */
>> -- 
>> 2.19.1
>>
>>

Re: [Qemu-devel] [Qemu-arm] [PATCH v5 02/16] gdbstub: introduce GDB processes

[Luc Michel]

Hi Edgar,

On 11/13/18 11:52 AM, Edgar E. Iglesias wrote:
> On Sat, Nov 10, 2018 at 09:11:33AM +0100, Luc Michel wrote:
>> Add a structure GDBProcess that represent processes from the GDB
>> semantic point of view.
>>
>> CPUs can be split into different processes, by grouping them under
>> different cpu-cluster objects.  Each occurrence of a cpu-cluster object
>> implies the existence of the corresponding process in the GDB stub. The
>> GDB process ID is derived from the corresponding cluster ID as follows:
>>
>>   GDB PID = cluster ID + 1
>>
>> This is because PIDs -1 and 0 are reserved in GDB and cannot be used by
>> processes.
>>
>> When no such container are found, all the CPUs are put in a unique GDB
>> process (create_unique_process()). This is also the case when compiled
>> in user mode, where multi-processes do not make much sense for now.
>>
>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>> Acked-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>  gdbstub.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 76 insertions(+)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index c4e4f9f082..0d70b89598 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -27,10 +27,11 @@
>>  #include "monitor/monitor.h"
>>  #include "chardev/char.h"
>>  #include "chardev/char-fe.h"
>>  #include "sysemu/sysemu.h"
>>  #include "exec/gdbstub.h"
>> +#include "hw/cpu/cluster.h"
>>  #endif
>>  
>>  #define MAX_PACKET_LENGTH 4096
>>  
>>  #include "qemu/sockets.h"
>> @@ -294,10 +295,15 @@ typedef struct GDBRegisterState {
>>      gdb_reg_cb set_reg;
>>      const char *xml;
>>      struct GDBRegisterState *next;
>>  } GDBRegisterState;
>>  
>> +typedef struct GDBProcess {
>> +    uint32_t pid;
>> +    bool attached;
>> +} GDBProcess;
>> +
>>  enum RSState {
>>      RS_INACTIVE,
>>      RS_IDLE,
>>      RS_GETLINE,
>>      RS_GETLINE_ESC,
>> @@ -322,10 +328,13 @@ typedef struct GDBState {
>>      int running_state;
>>  #else
>>      CharBackend chr;
>>      Chardev *mon_chr;
>>  #endif
>> +    bool multiprocess;
>> +    GDBProcess *processes;
>> +    int process_num;
>>      char syscall_buf[256];
>>      gdb_syscall_complete_cb current_syscall_cb;
>>  } GDBState;
>>  
>>  /* By default use no IRQs and no timers while single stepping so as to
>> @@ -1749,10 +1758,24 @@ void gdb_exit(CPUArchState *env, int code)
>>  #ifndef CONFIG_USER_ONLY
>>    qemu_chr_fe_deinit(&s->chr, true);
>>  #endif
>>  }
>>  
>> +/*
>> + * Create a unique process containing all the CPUs.
>> + */
>> +static void create_unique_process(GDBState *s)
>> +{
>> +    GDBProcess *process;
>> +
>> +    s->processes = g_malloc0(sizeof(GDBProcess));
>> +    s->process_num = 1;
>> +    process = &s->processes[0];
>> +
>> +    process->pid = 1;
>> +}
>> +
>>  #ifdef CONFIG_USER_ONLY
>>  int
>>  gdb_handlesig(CPUState *cpu, int sig)
>>  {
>>      GDBState *s;
>> @@ -1846,10 +1869,11 @@ static bool gdb_accept(void)
>>      }
>>  
>>      s = g_malloc0(sizeof(GDBState));
>>      s->c_cpu = first_cpu;
>>      s->g_cpu = first_cpu;
>> +    create_unique_process(s);
>>      s->fd = fd;
>>      gdb_has_xml = false;
>>  
>>      gdbserver_state = s;
>>      return true;
>> @@ -2002,10 +2026,58 @@ static const TypeInfo char_gdb_type_info = {
>>      .name = TYPE_CHARDEV_GDB,
>>      .parent = TYPE_CHARDEV,
>>      .class_init = char_gdb_class_init,
>>  };
>>  
>> +static int find_cpu_clusters(Object *child, void *opaque)
>> +{
>> +    if (object_dynamic_cast(child, TYPE_CPU_CLUSTER)) {
>> +        GDBState *s = (GDBState *) opaque;
>> +        CPUClusterState *cluster = CPU_CLUSTER(child);
>> +        GDBProcess *process;
>> +
>> +        s->processes = g_renew(GDBProcess, s->processes, ++s->process_num);
>> +
>> +        process = &s->processes[s->process_num - 1];
>> +
>> +        /* GDB process IDs -1 and 0 are reserved */
>> +        process->pid = cluster->cluster_id + 1;
> 
> This may be theoretical but since both pid and cluster_id are uint32_t
> you may end up with 0 here.
> 
> You may want to limit cluster_id's to something like the range 0 - INT32_MAX.
That would be an efficient solution to workaround this problem. However
it seems a bit artificial to limit the cluster_id range in the "generic"
CPU_CLUSTER class, just for the GDB stub code to work correctly.

Another solution could be to add a `gdb_pid' property to the CPU_CLUSTER
type, that would automatically be set to `cpu_cluster + 1' during
realization (and customized by the platform if needed). That way, value
0 could be explicitly forbidden in the CPU_CLUSTER API. However, Peter
was not a big fan of having explicit GDB stuff in the platform.

If somebody comes with a good compromise, I can change this. Otherwise
if we want to be extra safe, maybe we can simply assert that cluster_id
UINT32_MAX is not used in GDB stub.

(snip)
>> +static int pid_order(const void *a, const void *b)
>> +{
>> +    GDBProcess *pa = (GDBProcess *) a;
>> +    GDBProcess *pb = (GDBProcess *) b;
>> +
>> +    return pa->pid - pb->pid;
> 
> Similarly here, is this safe?
> e.g:
> pa->pid = 1
> pb->pid = 0x80000002
To make it safe, I think we can do explicit comparisons:

if (pa->pid < pb->pid) {
    return -1;
} else if (pa->pid > pb->pid) {
    return 1;
} else {
    return 0;
}


Thanks,
Luc

> 
> 
> Otherwise:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> 
> 
>> +}
>> +
>> +static void create_processes(GDBState *s)
>> +{
>> +    object_child_foreach(object_get_root(), find_cpu_clusters, s);
>> +
>> +    if (!s->processes) {
>> +        /* No CPU cluster specified by the machine */
>> +        create_unique_process(s);
>> +    } else {
>> +        /* Sort by PID */
>> +        qsort(s->processes, s->process_num, sizeof(s->processes[0]), pid_order);
>> +    }
>> +}
>> +
>> +static void cleanup_processes(GDBState *s)
>> +{
>> +    g_free(s->processes);
>> +    s->process_num = 0;
>> +    s->processes = NULL;
>> +}
>> +
>>  int gdbserver_start(const char *device)
>>  {
>>      trace_gdbstub_op_start(device);
>>  
>>      GDBState *s;
>> @@ -2058,15 +2130,19 @@ int gdbserver_start(const char *device)
>>                                     NULL, &error_abort);
>>          monitor_init(mon_chr, 0);
>>      } else {
>>          qemu_chr_fe_deinit(&s->chr, true);
>>          mon_chr = s->mon_chr;
>> +        cleanup_processes(s);
>>          memset(s, 0, sizeof(GDBState));
>>          s->mon_chr = mon_chr;
>>      }
>>      s->c_cpu = first_cpu;
>>      s->g_cpu = first_cpu;
>> +
>> +    create_processes(s);
>> +
>>      if (chr) {
>>          qemu_chr_fe_init(&s->chr, chr, &error_abort);
>>          qemu_chr_fe_set_handlers(&s->chr, gdb_chr_can_receive, gdb_chr_receive,
>>                                   gdb_chr_event, NULL, NULL, NULL, true);
>>      }
>> -- 
>> 2.19.1
>>
>>

Re: [Qemu-devel] [Qemu-arm] [PATCH v5 03/16] gdbstub: add multiprocess support to '?' packets

[Edgar E. Iglesias]

On Wed, Nov 14, 2018 at 09:43:27AM +0100, Luc Michel wrote:
> Hi Edgar,
> 
> On 11/13/18 12:06 PM, Edgar E. Iglesias wrote:
> > On Sat, Nov 10, 2018 at 09:11:34AM +0100, Luc Michel wrote:
> >> The gdb_get_cpu_pid() function does the PID lookup for the given CPU. It
> >> checks if the CPU is a direct child of a CPU cluster. If it is, the
> >> returned PID is the cluster ID plus one (cluster IDs start at 0, GDB
> >> PIDs at 1). When the CPU is not a child of such a container, the PID of
> >> the first process is returned.
> >>
> >> The gdb_fmt_thread_id() function generates the string to be used to identify
> >> a given thread, in a response packet for the peer. This function
> >> supports generating thread IDs when multiprocess mode is enabled (in the
> >> form `p<pid>.<tid>').
> >>
> >> Use them in the reply to a '?' request.
> >>
> >> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> >> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> > 
> > 
> > This is also theoretical but:
> > When looking at this, it seems like you could have lazily created
> > the s->processes array entries (if you find a cluster but the
> > no valid entry in s->processes). Then we could perhaps eliminate the
> > scan of all objects at startup and also support CPU/Cluster hotplug.
> Yes you are right, this could be an improvement to this series to add
> cluster hotplug support (CPU hotplug is actually supported). It's a
> little bit tricky though since we would have to maintain the
> s->processes array and properly signal to GDB when a process dies.

Hi Luc,

IMO, support for hotplugging could be added incrementally with follow-up work.
I wonder if the GDB stub would handle it today, without your patches?

Cheers,
Edgar


> 
> Cheers,
> Luc
> 
> > 
> > Anyway, this looks good to me!
> > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > 
> > Cheers,
> > Edgar
> > 
> > 
> >> ---
> >>  gdbstub.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 58 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/gdbstub.c b/gdbstub.c
> >> index 0d70b89598..d26bad4b67 100644
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -638,10 +638,52 @@ static int memtox(char *buf, const char *mem, int len)
> >>          }
> >>      }
> >>      return p - buf;
> >>  }
> >>  
> >> +static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
> >> +{
> >> +#ifndef CONFIG_USER_ONLY
> >> +    gchar *path, *name;
> >> +    Object *obj;
> >> +    CPUClusterState *cluster;
> >> +    uint32_t ret;
> >> +
> >> +    path = object_get_canonical_path(OBJECT(cpu));
> >> +    name = object_get_canonical_path_component(OBJECT(cpu));
> >> +
> >> +    if (path == NULL) {
> >> +        ret = s->processes[0].pid;
> >> +        goto out;
> >> +    }
> >> +
> >> +    /*
> >> +     * Retrieve the CPU parent path by removing the last '/' and the CPU name
> >> +     * from the CPU canonical path. */
> >> +    path[strlen(path) - strlen(name) - 1] = '\0';
> >> +
> >> +    obj = object_resolve_path_type(path, TYPE_CPU_CLUSTER, NULL);
> >> +
> >> +    if (obj == NULL) {
> >> +        ret = s->processes[0].pid;
> >> +        goto out;
> >> +    }
> >> +
> >> +    cluster = CPU_CLUSTER(obj);
> >> +    ret = cluster->cluster_id + 1;
> >> +
> >> +out:
> >> +    g_free(name);
> >> +    g_free(path);
> >> +
> >> +    return ret;
> >> +
> >> +#else
> >> +    return s->processes[0].pid;
> >> +#endif
> >> +}
> >> +
> >>  static const char *get_feature_xml(const char *p, const char **newp,
> >>                                     CPUClass *cc)
> >>  {
> >>      size_t len;
> >>      int i;
> >> @@ -907,10 +949,23 @@ static CPUState *find_cpu(uint32_t thread_id)
> >>      }
> >>  
> >>      return NULL;
> >>  }
> >>  
> >> +static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu,
> >> +                           char *buf, size_t buf_size)
> >> +{
> >> +    if (s->multiprocess) {
> >> +        snprintf(buf, buf_size, "p%02x.%02x",
> >> +                 gdb_get_cpu_pid(s, cpu), cpu_gdb_index(cpu));
> >> +    } else {
> >> +        snprintf(buf, buf_size, "%02x", cpu_gdb_index(cpu));
> >> +    }
> >> +
> >> +    return buf;
> >> +}
> >> +
> >>  static int is_query_packet(const char *p, const char *query, char separator)
> >>  {
> >>      unsigned int query_len = strlen(query);
> >>  
> >>      return strncmp(p, query, query_len) == 0 &&
> >> @@ -1018,22 +1073,23 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >>      const char *p;
> >>      uint32_t thread;
> >>      int ch, reg_size, type, res;
> >>      uint8_t mem_buf[MAX_PACKET_LENGTH];
> >>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
> >> +    char thread_id[16];
> >>      uint8_t *registers;
> >>      target_ulong addr, len;
> >>  
> >>      trace_gdbstub_io_command(line_buf);
> >>  
> >>      p = line_buf;
> >>      ch = *p++;
> >>      switch(ch) {
> >>      case '?':
> >>          /* TODO: Make this return the correct value for user-mode.  */
> >> -        snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
> >> -                 cpu_gdb_index(s->c_cpu));
> >> +        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
> >> +                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
> >>          put_packet(s, buf);
> >>          /* Remove all the breakpoints when this query is issued,
> >>           * because gdb is doing and initial connect and the state
> >>           * should be cleaned up.
> >>           */
> >> -- 
> >> 2.19.1
> >>
> >>

Re: [Qemu-devel] [Qemu-arm] [PATCH v5 02/16] gdbstub: introduce GDB processes

[Edgar E. Iglesias]

On Wed, Nov 14, 2018 at 11:14:31AM +0100, Luc Michel wrote:
> Hi Edgar,
> 
> On 11/13/18 11:52 AM, Edgar E. Iglesias wrote:
> > On Sat, Nov 10, 2018 at 09:11:33AM +0100, Luc Michel wrote:
> >> Add a structure GDBProcess that represent processes from the GDB
> >> semantic point of view.
> >>
> >> CPUs can be split into different processes, by grouping them under
> >> different cpu-cluster objects.  Each occurrence of a cpu-cluster object
> >> implies the existence of the corresponding process in the GDB stub. The
> >> GDB process ID is derived from the corresponding cluster ID as follows:
> >>
> >>   GDB PID = cluster ID + 1
> >>
> >> This is because PIDs -1 and 0 are reserved in GDB and cannot be used by
> >> processes.
> >>
> >> When no such container are found, all the CPUs are put in a unique GDB
> >> process (create_unique_process()). This is also the case when compiled
> >> in user mode, where multi-processes do not make much sense for now.
> >>
> >> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> >> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> >> ---
> >>  gdbstub.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 76 insertions(+)
> >>
> >> diff --git a/gdbstub.c b/gdbstub.c
> >> index c4e4f9f082..0d70b89598 100644
> >> --- a/gdbstub.c
> >> +++ b/gdbstub.c
> >> @@ -27,10 +27,11 @@
> >>  #include "monitor/monitor.h"
> >>  #include "chardev/char.h"
> >>  #include "chardev/char-fe.h"
> >>  #include "sysemu/sysemu.h"
> >>  #include "exec/gdbstub.h"
> >> +#include "hw/cpu/cluster.h"
> >>  #endif
> >>  
> >>  #define MAX_PACKET_LENGTH 4096
> >>  
> >>  #include "qemu/sockets.h"
> >> @@ -294,10 +295,15 @@ typedef struct GDBRegisterState {
> >>      gdb_reg_cb set_reg;
> >>      const char *xml;
> >>      struct GDBRegisterState *next;
> >>  } GDBRegisterState;
> >>  
> >> +typedef struct GDBProcess {
> >> +    uint32_t pid;
> >> +    bool attached;
> >> +} GDBProcess;
> >> +
> >>  enum RSState {
> >>      RS_INACTIVE,
> >>      RS_IDLE,
> >>      RS_GETLINE,
> >>      RS_GETLINE_ESC,
> >> @@ -322,10 +328,13 @@ typedef struct GDBState {
> >>      int running_state;
> >>  #else
> >>      CharBackend chr;
> >>      Chardev *mon_chr;
> >>  #endif
> >> +    bool multiprocess;
> >> +    GDBProcess *processes;
> >> +    int process_num;
> >>      char syscall_buf[256];
> >>      gdb_syscall_complete_cb current_syscall_cb;
> >>  } GDBState;
> >>  
> >>  /* By default use no IRQs and no timers while single stepping so as to
> >> @@ -1749,10 +1758,24 @@ void gdb_exit(CPUArchState *env, int code)
> >>  #ifndef CONFIG_USER_ONLY
> >>    qemu_chr_fe_deinit(&s->chr, true);
> >>  #endif
> >>  }
> >>  
> >> +/*
> >> + * Create a unique process containing all the CPUs.
> >> + */
> >> +static void create_unique_process(GDBState *s)
> >> +{
> >> +    GDBProcess *process;
> >> +
> >> +    s->processes = g_malloc0(sizeof(GDBProcess));
> >> +    s->process_num = 1;
> >> +    process = &s->processes[0];
> >> +
> >> +    process->pid = 1;
> >> +}
> >> +
> >>  #ifdef CONFIG_USER_ONLY
> >>  int
> >>  gdb_handlesig(CPUState *cpu, int sig)
> >>  {
> >>      GDBState *s;
> >> @@ -1846,10 +1869,11 @@ static bool gdb_accept(void)
> >>      }
> >>  
> >>      s = g_malloc0(sizeof(GDBState));
> >>      s->c_cpu = first_cpu;
> >>      s->g_cpu = first_cpu;
> >> +    create_unique_process(s);
> >>      s->fd = fd;
> >>      gdb_has_xml = false;
> >>  
> >>      gdbserver_state = s;
> >>      return true;
> >> @@ -2002,10 +2026,58 @@ static const TypeInfo char_gdb_type_info = {
> >>      .name = TYPE_CHARDEV_GDB,
> >>      .parent = TYPE_CHARDEV,
> >>      .class_init = char_gdb_class_init,
> >>  };
> >>  
> >> +static int find_cpu_clusters(Object *child, void *opaque)
> >> +{
> >> +    if (object_dynamic_cast(child, TYPE_CPU_CLUSTER)) {
> >> +        GDBState *s = (GDBState *) opaque;
> >> +        CPUClusterState *cluster = CPU_CLUSTER(child);
> >> +        GDBProcess *process;
> >> +
> >> +        s->processes = g_renew(GDBProcess, s->processes, ++s->process_num);
> >> +
> >> +        process = &s->processes[s->process_num - 1];
> >> +
> >> +        /* GDB process IDs -1 and 0 are reserved */
> >> +        process->pid = cluster->cluster_id + 1;
> > 
> > This may be theoretical but since both pid and cluster_id are uint32_t
> > you may end up with 0 here.
> > 
> > You may want to limit cluster_id's to something like the range 0 - INT32_MAX.
> That would be an efficient solution to workaround this problem. However
> it seems a bit artificial to limit the cluster_id range in the "generic"
> CPU_CLUSTER class, just for the GDB stub code to work correctly.
> 
> Another solution could be to add a `gdb_pid' property to the CPU_CLUSTER
> type, that would automatically be set to `cpu_cluster + 1' during
> realization (and customized by the platform if needed). That way, value
> 0 could be explicitly forbidden in the CPU_CLUSTER API. However, Peter
> was not a big fan of having explicit GDB stuff in the platform.
> 
> If somebody comes with a good compromise, I can change this. Otherwise
> if we want to be extra safe, maybe we can simply assert that cluster_id
> UINT32_MAX is not used in GDB stub.

Yeah, the latter will at least avoid subtle errors at runtime. Sounds
good to me...


> 
> (snip)
> >> +static int pid_order(const void *a, const void *b)
> >> +{
> >> +    GDBProcess *pa = (GDBProcess *) a;
> >> +    GDBProcess *pb = (GDBProcess *) b;
> >> +
> >> +    return pa->pid - pb->pid;
> > 
> > Similarly here, is this safe?
> > e.g:
> > pa->pid = 1
> > pb->pid = 0x80000002
> To make it safe, I think we can do explicit comparisons:
> 
> if (pa->pid < pb->pid) {
>     return -1;
> } else if (pa->pid > pb->pid) {
>     return 1;
> } else {
>     return 0;
> }


Looks good.

Thanks,
Edgar


> 
> 
> Thanks,
> Luc
> 
> > 
> > 
> > Otherwise:
> > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > 
> > 
> > 
> >> +}
> >> +
> >> +static void create_processes(GDBState *s)
> >> +{
> >> +    object_child_foreach(object_get_root(), find_cpu_clusters, s);
> >> +
> >> +    if (!s->processes) {
> >> +        /* No CPU cluster specified by the machine */
> >> +        create_unique_process(s);
> >> +    } else {
> >> +        /* Sort by PID */
> >> +        qsort(s->processes, s->process_num, sizeof(s->processes[0]), pid_order);
> >> +    }
> >> +}
> >> +
> >> +static void cleanup_processes(GDBState *s)
> >> +{
> >> +    g_free(s->processes);
> >> +    s->process_num = 0;
> >> +    s->processes = NULL;
> >> +}
> >> +
> >>  int gdbserver_start(const char *device)
> >>  {
> >>      trace_gdbstub_op_start(device);
> >>  
> >>      GDBState *s;
> >> @@ -2058,15 +2130,19 @@ int gdbserver_start(const char *device)
> >>                                     NULL, &error_abort);
> >>          monitor_init(mon_chr, 0);
> >>      } else {
> >>          qemu_chr_fe_deinit(&s->chr, true);
> >>          mon_chr = s->mon_chr;
> >> +        cleanup_processes(s);
> >>          memset(s, 0, sizeof(GDBState));
> >>          s->mon_chr = mon_chr;
> >>      }
> >>      s->c_cpu = first_cpu;
> >>      s->g_cpu = first_cpu;
> >> +
> >> +    create_processes(s);
> >> +
> >>      if (chr) {
> >>          qemu_chr_fe_init(&s->chr, chr, &error_abort);
> >>          qemu_chr_fe_set_handlers(&s->chr, gdb_chr_can_receive, gdb_chr_receive,
> >>                                   gdb_chr_event, NULL, NULL, NULL, true);
> >>      }
> >> -- 
> >> 2.19.1
> >>
> >>

Re: [Qemu-devel] [Qemu-arm] [PATCH v5 03/16] gdbstub: add multiprocess support to '?' packets

[Luc Michel]

On 11/14/18 11:27 AM, Edgar E. Iglesias wrote:
> On Wed, Nov 14, 2018 at 09:43:27AM +0100, Luc Michel wrote:
>> Hi Edgar,
>>
>> On 11/13/18 12:06 PM, Edgar E. Iglesias wrote:
>>> On Sat, Nov 10, 2018 at 09:11:34AM +0100, Luc Michel wrote:
>>>> The gdb_get_cpu_pid() function does the PID lookup for the given CPU. It
>>>> checks if the CPU is a direct child of a CPU cluster. If it is, the
>>>> returned PID is the cluster ID plus one (cluster IDs start at 0, GDB
>>>> PIDs at 1). When the CPU is not a child of such a container, the PID of
>>>> the first process is returned.
>>>>
>>>> The gdb_fmt_thread_id() function generates the string to be used to identify
>>>> a given thread, in a response packet for the peer. This function
>>>> supports generating thread IDs when multiprocess mode is enabled (in the
>>>> form `p<pid>.<tid>').
>>>>
>>>> Use them in the reply to a '?' request.
>>>>
>>>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>>>> Acked-by: Alistair Francis <alistair.francis@wdc.com>
>>>
>>>
>>> This is also theoretical but:
>>> When looking at this, it seems like you could have lazily created
>>> the s->processes array entries (if you find a cluster but the
>>> no valid entry in s->processes). Then we could perhaps eliminate the
>>> scan of all objects at startup and also support CPU/Cluster hotplug.
>> Yes you are right, this could be an improvement to this series to add
>> cluster hotplug support (CPU hotplug is actually supported). It's a
>> little bit tricky though since we would have to maintain the
>> s->processes array and properly signal to GDB when a process dies.
> 
> Hi Luc,
> 
> IMO, support for hotplugging could be added incrementally with follow-up work.
> I wonder if the GDB stub would handle it today, without your patches?
Yes as of today, CPU hotplugging works fine with the GDB stub / GDB
client. I took extra care in this patch series so that it continues to
work correctly (both for system mode, and user mode where 1 QEMU CPU ==
1 guest thread). The only thing that would need extra care is cluster
hotplugging, which can be added incrementally with follow-up work.

Thanks,
Luc


> 
> Cheers,
> Edgar
> 
> 
>>
>> Cheers,
>> Luc
>>
>>>
>>> Anyway, this looks good to me!
>>> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>
>>> Cheers,
>>> Edgar
>>>
>>>
>>>> ---
>>>>  gdbstub.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 58 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gdbstub.c b/gdbstub.c
>>>> index 0d70b89598..d26bad4b67 100644
>>>> --- a/gdbstub.c
>>>> +++ b/gdbstub.c
>>>> @@ -638,10 +638,52 @@ static int memtox(char *buf, const char *mem, int len)
>>>>          }
>>>>      }
>>>>      return p - buf;
>>>>  }
>>>>  
>>>> +static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
>>>> +{
>>>> +#ifndef CONFIG_USER_ONLY
>>>> +    gchar *path, *name;
>>>> +    Object *obj;
>>>> +    CPUClusterState *cluster;
>>>> +    uint32_t ret;
>>>> +
>>>> +    path = object_get_canonical_path(OBJECT(cpu));
>>>> +    name = object_get_canonical_path_component(OBJECT(cpu));
>>>> +
>>>> +    if (path == NULL) {
>>>> +        ret = s->processes[0].pid;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Retrieve the CPU parent path by removing the last '/' and the CPU name
>>>> +     * from the CPU canonical path. */
>>>> +    path[strlen(path) - strlen(name) - 1] = '\0';
>>>> +
>>>> +    obj = object_resolve_path_type(path, TYPE_CPU_CLUSTER, NULL);
>>>> +
>>>> +    if (obj == NULL) {
>>>> +        ret = s->processes[0].pid;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    cluster = CPU_CLUSTER(obj);
>>>> +    ret = cluster->cluster_id + 1;
>>>> +
>>>> +out:
>>>> +    g_free(name);
>>>> +    g_free(path);
>>>> +
>>>> +    return ret;
>>>> +
>>>> +#else
>>>> +    return s->processes[0].pid;
>>>> +#endif
>>>> +}
>>>> +
>>>>  static const char *get_feature_xml(const char *p, const char **newp,
>>>>                                     CPUClass *cc)
>>>>  {
>>>>      size_t len;
>>>>      int i;
>>>> @@ -907,10 +949,23 @@ static CPUState *find_cpu(uint32_t thread_id)
>>>>      }
>>>>  
>>>>      return NULL;
>>>>  }
>>>>  
>>>> +static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu,
>>>> +                           char *buf, size_t buf_size)
>>>> +{
>>>> +    if (s->multiprocess) {
>>>> +        snprintf(buf, buf_size, "p%02x.%02x",
>>>> +                 gdb_get_cpu_pid(s, cpu), cpu_gdb_index(cpu));
>>>> +    } else {
>>>> +        snprintf(buf, buf_size, "%02x", cpu_gdb_index(cpu));
>>>> +    }
>>>> +
>>>> +    return buf;
>>>> +}
>>>> +
>>>>  static int is_query_packet(const char *p, const char *query, char separator)
>>>>  {
>>>>      unsigned int query_len = strlen(query);
>>>>  
>>>>      return strncmp(p, query, query_len) == 0 &&
>>>> @@ -1018,22 +1073,23 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>>>>      const char *p;
>>>>      uint32_t thread;
>>>>      int ch, reg_size, type, res;
>>>>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>>>>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>>>> +    char thread_id[16];
>>>>      uint8_t *registers;
>>>>      target_ulong addr, len;
>>>>  
>>>>      trace_gdbstub_io_command(line_buf);
>>>>  
>>>>      p = line_buf;
>>>>      ch = *p++;
>>>>      switch(ch) {
>>>>      case '?':
>>>>          /* TODO: Make this return the correct value for user-mode.  */
>>>> -        snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
>>>> -                 cpu_gdb_index(s->c_cpu));
>>>> +        snprintf(buf, sizeof(buf), "T%02xthread:%s;", GDB_SIGNAL_TRAP,
>>>> +                 gdb_fmt_thread_id(s, s->c_cpu, thread_id, sizeof(thread_id)));
>>>>          put_packet(s, buf);
>>>>          /* Remove all the breakpoints when this query is issued,
>>>>           * because gdb is doing and initial connect and the state
>>>>           * should be cleaned up.
>>>>           */
>>>> -- 
>>>> 2.19.1
>>>>
>>>>