* [Qemu-devel] [PATCH v2 01/15] gdbstub: introduce GDB processes
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 16:15 ` Philippe Mathieu-Daudé
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 02/15] gdbstub: add multiprocess support to '?' packets Luc Michel
` (13 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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 a QOM
container named after the GDB_CPU_GROUP_NAME macro (`gdb-group[*]').
Each occurrence of such a container implies the existence of the
corresponding process in the GDB stub. The gdb_cpu_group_container_get()
function can be used to create a new container.
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>
---
include/exec/gdbstub.h | 8 +++++
gdbstub.c | 67 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 08363969c1..a3e4159bf4 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -1,8 +1,10 @@
#ifndef GDBSTUB_H
#define GDBSTUB_H
+#include "qom/object.h"
+
#define DEFAULT_GDBSTUB_PORT "1234"
/* GDB breakpoint/watchpoint types */
#define GDB_BREAKPOINT_SW 0
#define GDB_BREAKPOINT_HW 1
@@ -129,6 +131,12 @@ void gdbserver_cleanup(void);
extern bool gdb_has_xml;
/* in gdbstub-xml.c, generated by scripts/feature_to_c.sh */
extern const char *const xml_builtin[][2];
+#define GDB_CPU_GROUP_NAME "gdb-group"
+
+static inline Object *gdb_cpu_group_container_get(Object *parent)
+{
+ return container_get(parent, "/" GDB_CPU_GROUP_NAME "[*]");
+}
#endif
diff --git a/gdbstub.c b/gdbstub.c
index d6ab95006c..5c86218f49 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -295,10 +295,17 @@ typedef struct GDBRegisterState {
gdb_reg_cb set_reg;
const char *xml;
struct GDBRegisterState *next;
} GDBRegisterState;
+typedef struct GDBProcess {
+ uint32_t pid;
+ bool attached;
+
+ char target_xml[1024];
+} GDBProcess;
+
enum RSState {
RS_INACTIVE,
RS_IDLE,
RS_GETLINE,
RS_GETLINE_ESC,
@@ -323,10 +330,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
@@ -1750,10 +1760,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;
@@ -1847,10 +1871,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;
@@ -2003,10 +2028,48 @@ static const TypeInfo char_gdb_type_info = {
.name = TYPE_CHARDEV_GDB,
.parent = TYPE_CHARDEV,
.class_init = char_gdb_class_init,
};
+static void create_processes(GDBState *s)
+{
+ Object *container;
+ int i = 0;
+ char process_str[16];
+
+ container = object_resolve_path(GDB_CPU_GROUP_NAME "[0]", NULL);
+
+ while (container) {
+ s->processes = g_renew(GDBProcess, s->processes, i + 1);
+
+ GDBProcess *process = &s->processes[i];
+
+ /* GDB process IDs -1 and 0 are reserved */
+ process->pid = i + 1;
+ process->attached = false;
+ process->target_xml[0] = '\0';
+
+ i++;
+ snprintf(process_str, sizeof(process_str), GDB_CPU_GROUP_NAME "[%d]", i);
+ container = object_resolve_path(process_str, NULL);
+ }
+
+ if (!s->processes) {
+ /* No CPU group specified by the machine */
+ create_unique_process(s);
+ } else {
+ s->process_num = i;
+ }
+}
+
+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;
@@ -2055,15 +2118,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/15] gdbstub: introduce GDB processes
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 01/15] gdbstub: introduce GDB processes Luc Michel
@ 2018-10-01 16:15 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 16:15 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
Hi Luc,
On 01/10/2018 13:56, 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 a QOM
> container named after the GDB_CPU_GROUP_NAME macro (`gdb-group[*]').
> Each occurrence of such a container implies the existence of the
> corresponding process in the GDB stub. The gdb_cpu_group_container_get()
> function can be used to create a new container.
>
> 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>
> ---
> include/exec/gdbstub.h | 8 +++++
> gdbstub.c | 67 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 75 insertions(+)
>
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 08363969c1..a3e4159bf4 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -1,8 +1,10 @@
> #ifndef GDBSTUB_H
> #define GDBSTUB_H
>
> +#include "qom/object.h"
> +
> #define DEFAULT_GDBSTUB_PORT "1234"
>
> /* GDB breakpoint/watchpoint types */
> #define GDB_BREAKPOINT_SW 0
> #define GDB_BREAKPOINT_HW 1
> @@ -129,6 +131,12 @@ void gdbserver_cleanup(void);
> extern bool gdb_has_xml;
>
> /* in gdbstub-xml.c, generated by scripts/feature_to_c.sh */
> extern const char *const xml_builtin[][2];
>
> +#define GDB_CPU_GROUP_NAME "gdb-group"
> +
> +static inline Object *gdb_cpu_group_container_get(Object *parent)
> +{
> + return container_get(parent, "/" GDB_CPU_GROUP_NAME "[*]");
> +}
> #endif
> diff --git a/gdbstub.c b/gdbstub.c
> index d6ab95006c..5c86218f49 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -295,10 +295,17 @@ typedef struct GDBRegisterState {
> gdb_reg_cb set_reg;
> const char *xml;
> struct GDBRegisterState *next;
> } GDBRegisterState;
>
> +typedef struct GDBProcess {
> + uint32_t pid;
> + bool attached;
> +
> + char target_xml[1024];
I'd add this field in the patch #7 "support to Xfer:features:read:"
where you start using it.
> +} GDBProcess;
> +
> enum RSState {
> RS_INACTIVE,
> RS_IDLE,
> RS_GETLINE,
> RS_GETLINE_ESC,
> @@ -323,10 +330,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
> @@ -1750,10 +1760,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;
> @@ -1847,10 +1871,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;
> @@ -2003,10 +2028,48 @@ static const TypeInfo char_gdb_type_info = {
> .name = TYPE_CHARDEV_GDB,
> .parent = TYPE_CHARDEV,
> .class_init = char_gdb_class_init,
> };
>
> +static void create_processes(GDBState *s)
> +{
> + Object *container;
> + int i = 0;
> + char process_str[16];
> +
> + container = object_resolve_path(GDB_CPU_GROUP_NAME "[0]", NULL);
> +
> + while (container) {
> + s->processes = g_renew(GDBProcess, s->processes, i + 1);
> +
> + GDBProcess *process = &s->processes[i];
> +
> + /* GDB process IDs -1 and 0 are reserved */
> + process->pid = i + 1;
> + process->attached = false;
> + process->target_xml[0] = '\0';
> +
> + i++;
> + snprintf(process_str, sizeof(process_str), GDB_CPU_GROUP_NAME "[%d]", i);
> + container = object_resolve_path(process_str, NULL);
> + }
> +
> + if (!s->processes) {
> + /* No CPU group specified by the machine */
> + create_unique_process(s);
> + } else {
> + s->process_num = i;
> + }
> +}
> +
> +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;
> @@ -2055,15 +2118,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);
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 02/15] gdbstub: add multiprocess support to '?' packets
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 01/15] gdbstub: introduce GDB processes Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 15:20 ` Philippe Mathieu-Daudé
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 03/15] gdbstub: add multiprocess support to 'H' and 'T' packets Luc Michel
` (12 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
The gdb_get_cpu_pid() function does the PID lookup for the given CPU. It
checks if the CPU is in a QOM container named after the
GDB_CPU_GROUP_NAME macro. If found, it returns the correponding PID,
which is the group ID plus one (group IDs start at 0, GDB PIDs at 1).
When the CPU is not a child of such a container, PID 1 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 it in the reply to a '?' request.
Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
gdbstub.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 43 insertions(+), 2 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 5c86218f49..ac9d540fda 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -640,10 +640,37 @@ 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)
+{
+ gchar *path;
+ gchar *cont;
+ const char *left;
+ unsigned long pid;
+
+ if (!s->multiprocess || (s->process_num == 1)) {
+ return 1;
+ }
+
+ path = object_get_canonical_path(OBJECT(cpu));
+ cont = g_strrstr(path, "/" GDB_CPU_GROUP_NAME "[");
+
+ if (cont == NULL) {
+ return 1;
+ }
+
+ cont += strlen("/" GDB_CPU_GROUP_NAME "[");
+
+ if (qemu_strtoul(cont, &left, 10, &pid)) {
+ return 1;
+ }
+
+ return pid + 1;
+}
+
static const char *get_feature_xml(const char *p, const char **newp,
CPUClass *cc)
{
size_t len;
int i;
@@ -909,10 +936,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 &&
@@ -1020,22 +1060,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/15] gdbstub: add multiprocess support to '?' packets
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 02/15] gdbstub: add multiprocess support to '?' packets Luc Michel
@ 2018-10-01 15:20 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 15:20 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
On 01/10/2018 13:56, Luc Michel wrote:
> The gdb_get_cpu_pid() function does the PID lookup for the given CPU. It
> checks if the CPU is in a QOM container named after the
> GDB_CPU_GROUP_NAME macro. If found, it returns the correponding PID,
> which is the group ID plus one (group IDs start at 0, GDB PIDs at 1).
> When the CPU is not a child of such a container, PID 1 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 it in the reply to a '?' request.
>
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>
No need to separate tags with empty line.
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> gdbstub.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 5c86218f49..ac9d540fda 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -640,10 +640,37 @@ 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)
> +{
> + gchar *path;
> + gchar *cont;
> + const char *left;
> + unsigned long pid;
> +
> + if (!s->multiprocess || (s->process_num == 1)) {
> + return 1;
> + }
> +
> + path = object_get_canonical_path(OBJECT(cpu));
> + cont = g_strrstr(path, "/" GDB_CPU_GROUP_NAME "[");
> +
> + if (cont == NULL) {
> + return 1;
> + }
> +
> + cont += strlen("/" GDB_CPU_GROUP_NAME "[");
> +
> + if (qemu_strtoul(cont, &left, 10, &pid)) {
> + return 1;
> + }
> +
> + return pid + 1;
> +}
> +
> static const char *get_feature_xml(const char *p, const char **newp,
> CPUClass *cc)
> {
> size_t len;
> int i;
> @@ -909,10 +936,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 &&
> @@ -1020,22 +1060,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.
> */
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 03/15] gdbstub: add multiprocess support to 'H' and 'T' packets
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 01/15] gdbstub: introduce GDB processes Luc Michel
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 02/15] gdbstub: add multiprocess support to '?' packets Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 17:07 ` Philippe Mathieu-Daudé
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 04/15] gdbstub: add multiprocess support to vCont packets Luc Michel
` (11 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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>
---
gdbstub.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 131 insertions(+), 18 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index ac9d540fda..a21ce3ca18 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -667,10 +667,74 @@ static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
}
return pid + 1;
}
+static GDBProcess *gdb_get_process(const GDBState *s, uint32_t pid)
+{
+ uint32_t process_idx;
+
+ if (!pid) {
+ /* 0 means any process, we take the first one */
+ pid = 1;
+ }
+
+ /* GDB PIDs are in the range [1;n] */
+ process_idx = pid - 1;
+
+ if (process_idx >= s->process_num) {
+ return NULL;
+ }
+
+ return &s->processes[process_idx];
+}
+
+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;
@@ -923,23 +987,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",
@@ -949,10 +1000,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 &&
@@ -1057,16 +1162,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++;
@@ -1270,16 +1377,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) {
@@ -1295,12 +1408,12 @@ 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);
+ read_thread_id(p, &p, &pid, &tid);
+ cpu = gdb_get_cpu(s, pid, tid);
if (cpu != NULL) {
put_packet(s, "OK");
} else {
put_packet(s, "E22");
--
2.19.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 03/15] gdbstub: add multiprocess support to 'H' and 'T' packets
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 03/15] gdbstub: add multiprocess support to 'H' and 'T' packets Luc Michel
@ 2018-10-01 17:07 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 17:07 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
On 01/10/2018 13:56, Luc Michel wrote:
> 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>
> ---
> gdbstub.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 131 insertions(+), 18 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index ac9d540fda..a21ce3ca18 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -667,10 +667,74 @@ static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
> }
>
> return pid + 1;
> }
>
> +static GDBProcess *gdb_get_process(const GDBState *s, uint32_t pid)
> +{
> + uint32_t process_idx;
> +
> + if (!pid) {
> + /* 0 means any process, we take the first one */
> + pid = 1;
> + }
> +
> + /* GDB PIDs are in the range [1;n] */
> + process_idx = pid - 1;
> +
> + if (process_idx >= s->process_num) {
> + return NULL;
> + }
> +
> + return &s->processes[process_idx];
> +}
> +
> +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;
> @@ -923,23 +987,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",
> @@ -949,10 +1000,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 &&
> @@ -1057,16 +1162,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++;
> @@ -1270,16 +1377,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) {
> @@ -1295,12 +1408,12 @@ 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);
> + read_thread_id(p, &p, &pid, &tid);
This part can now be safer too:
thread_kind = read_thread_id(p, &p, &pid, &tid);
if (thread_kind == GDB_READ_THREAD_ERR) {
put_packet(s, "E22");
break;
}
With this change:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> + cpu = gdb_get_cpu(s, pid, tid);
>
> if (cpu != NULL) {
> put_packet(s, "OK");
> } else {
> put_packet(s, "E22");
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 04/15] gdbstub: add multiprocess support to vCont packets
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (2 preceding siblings ...)
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 03/15] gdbstub: add multiprocess support to 'H' and 'T' packets Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 17:00 ` Philippe Mathieu-Daudé
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 05/15] gdbstub: add multiprocess support to 'sC' packets Luc Michel
` (10 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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>
---
gdbstub.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 102 insertions(+), 15 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index a21ce3ca18..779cc8b241 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -704,10 +704,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);
@@ -731,10 +761,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;
@@ -1069,14 +1130,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) {
@@ -1115,29 +1178,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;
@@ -1145,10 +1231,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 04/15] gdbstub: add multiprocess support to vCont packets
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 04/15] gdbstub: add multiprocess support to vCont packets Luc Michel
@ 2018-10-01 17:00 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 17:00 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
On 01/10/2018 13:56, Luc Michel wrote:
> 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>
> ---
> gdbstub.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 102 insertions(+), 15 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index a21ce3ca18..779cc8b241 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -704,10 +704,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);
>
> @@ -731,10 +761,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;
> @@ -1069,14 +1130,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) {
> @@ -1115,29 +1178,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;
> @@ -1145,10 +1231,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);
>
>
Nice.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 05/15] gdbstub: add multiprocess support to 'sC' packets
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (3 preceding siblings ...)
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 04/15] gdbstub: add multiprocess support to vCont packets Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-04 17:33 ` Alistair Francis
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 06/15] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo Luc Michel
` (9 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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>
---
gdbstub.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 779cc8b241..3242f0d261 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1530,13 +1530,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/15] gdbstub: add multiprocess support to 'sC' packets
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 05/15] gdbstub: add multiprocess support to 'sC' packets Luc Michel
@ 2018-10-04 17:33 ` Alistair Francis
0 siblings, 0 replies; 39+ messages in thread
From: Alistair Francis @ 2018-10-04 17:33 UTC (permalink / raw)
To: Luc Michel
Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
Alistair Francis, Mark Burton, Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On Mon, Oct 1, 2018 at 4:57 AM Luc Michel <luc.michel@greensocs.com> wrote:
>
> 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>
Alistair
> ---
> gdbstub.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 779cc8b241..3242f0d261 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1530,13 +1530,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.0
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 06/15] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (4 preceding siblings ...)
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 05/15] gdbstub: add multiprocess support to 'sC' packets Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 17:15 ` Philippe Mathieu-Daudé
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 07/15] gdbstub: add multiprocess support to Xfer:features:read: Luc Michel
` (8 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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>
---
gdbstub.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 3242f0d261..63d7913269 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1248,11 +1248,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];
@@ -1540,30 +1539,43 @@ 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);
+ read_thread_id(p + 16, &p, &pid, &tid);
+ 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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/15] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 06/15] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo Luc Michel
@ 2018-10-01 17:15 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 17:15 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
On 01/10/2018 13:56, Luc Michel wrote:
> 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>
> ---
> gdbstub.c | 32 ++++++++++++++++++++++----------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 3242f0d261..63d7913269 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1248,11 +1248,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];
> @@ -1540,30 +1539,43 @@ 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);
> + read_thread_id(p + 16, &p, &pid, &tid);
Check similar to patch #3:
if (read_thread_id(p, &p, &pid, &tid) == GDB_READ_THREAD_ERR) {
put_packet(s, "E22");
break;
}
With equivalent change:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> + 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;
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 07/15] gdbstub: add multiprocess support to Xfer:features:read:
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (5 preceding siblings ...)
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 06/15] gdbstub: add multiprocess support to (f|s)ThreadInfo and ThreadExtraInfo Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 16:28 ` Philippe Mathieu-Daudé
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 08/15] gdbstub: add multiprocess support to gdb_vm_state_change() Luc Michel
` (7 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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>
---
gdbstub.c | 44 ++++++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 63d7913269..9065e8e140 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -792,55 +792,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) {
@@ -1246,10 +1248,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];
@@ -1620,18 +1623,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;
}
--
2.19.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/15] gdbstub: add multiprocess support to Xfer:features:read:
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 07/15] gdbstub: add multiprocess support to Xfer:features:read: Luc Michel
@ 2018-10-01 16:28 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 16:28 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
Hi Luc,
On 01/10/2018 13:56, Luc Michel wrote:
> 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>
> ---
> gdbstub.c | 44 ++++++++++++++++++++++++--------------------
> 1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 63d7913269..9065e8e140 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -792,55 +792,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];
As noted in your patch #2, I'd add GDBProcess::target_xml in this same
patch.
With this change:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> + 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) {
> @@ -1246,10 +1248,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];
> @@ -1620,18 +1623,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;
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 08/15] gdbstub: add multiprocess support to gdb_vm_state_change()
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (6 preceding siblings ...)
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 07/15] gdbstub: add multiprocess support to Xfer:features:read: Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 16:30 ` Philippe Mathieu-Daudé
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 09/15] gdbstub: add multiprocess support to 'D' packets Luc Michel
` (6 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
Add support for multiprocess extension in gdb_vm_state_change()
function.
Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
gdbstub.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 9065e8e140..c1a02c34cd 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1692,10 +1692,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;
@@ -1703,10 +1704,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:
@@ -1720,12 +1729,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();
@@ -1763,11 +1772,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/15] gdbstub: add multiprocess support to gdb_vm_state_change()
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 08/15] gdbstub: add multiprocess support to gdb_vm_state_change() Luc Michel
@ 2018-10-01 16:30 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 16:30 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
On 01/10/2018 13:56, Luc Michel wrote:
> 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 9065e8e140..c1a02c34cd 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1692,10 +1692,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;
> @@ -1703,10 +1704,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:
> @@ -1720,12 +1729,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();
> @@ -1763,11 +1772,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 */
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 09/15] gdbstub: add multiprocess support to 'D' packets
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (7 preceding siblings ...)
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 08/15] gdbstub: add multiprocess support to gdb_vm_state_change() Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 10/15] gdbstub: add support for extended mode packet Luc Michel
` (5 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
'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 c1a02c34cd..299783b3b8 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1025,24 +1025,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)
{
@@ -1317,13 +1332,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v2 10/15] gdbstub: add support for extended mode packet
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (8 preceding siblings ...)
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 09/15] gdbstub: add multiprocess support to 'D' packets Luc Michel
@ 2018-10-01 11:56 ` Luc Michel
2018-10-01 16:39 ` Philippe Mathieu-Daudé
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 11/15] gdbstub: add support for vAttach packets Luc Michel
` (4 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:56 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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 299783b3b8..d372972dd3 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1280,10 +1280,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 10/15] gdbstub: add support for extended mode packet
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 10/15] gdbstub: add support for extended mode packet Luc Michel
@ 2018-10-01 16:39 ` Philippe Mathieu-Daudé
2018-10-02 9:26 ` Luc Michel
0 siblings, 1 reply; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 16:39 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
Hi Luc,
On 01/10/2018 13:56, Luc Michel wrote:
> 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 299783b3b8..d372972dd3 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1280,10 +1280,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");
Don't we want to also support the 'R' packet?
> + 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);
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 10/15] gdbstub: add support for extended mode packet
2018-10-01 16:39 ` Philippe Mathieu-Daudé
@ 2018-10-02 9:26 ` Luc Michel
0 siblings, 0 replies; 39+ messages in thread
From: Luc Michel @ 2018-10-02 9:26 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
On 10/1/18 6:39 PM, Philippe Mathieu-Daudé wrote:
> Hi Luc,
>
> On 01/10/2018 13:56, Luc Michel wrote:
>> 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 299783b3b8..d372972dd3 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1280,10 +1280,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");
>
> Don't we want to also support the 'R' packet?
Hi Philippe,
Thank you for all you reviews!
I'm not sure about this one:
- do you think calling qemu_system_reset() is the right thing to do?
- what should we do in user mode? Is there a way to restart the
emulated binary?
- Looking at the GDB sources, GDB seems to send an 'R' packet on the
"run" command. It starts by a "vKill;pid" packet, then a "vRun". If
"vRun" is not supported by the remote, it falls back to "R". So it seems
that if we want to support "run", we must also implement "vKill;pid",
which probably doesn't make much sense for QEMU. One possible
implementation that would probably work for system mode would be to:
- do nothing on 'vKill;pid' packet
- do not implement the 'vRun' packet
- call qemu_system_reset() on 'R' packet
But it does not align well with the current 'k' packet behaviour, which
simply do an exit(0). Do you have an opinion on this?
Thanks.
Luc.
>
>> + 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);
>>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 11/15] gdbstub: add support for vAttach packets
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (9 preceding siblings ...)
2018-10-01 11:56 ` [Qemu-devel] [PATCH v2 10/15] gdbstub: add support for extended mode packet Luc Michel
@ 2018-10-01 11:57 ` Luc Michel
2018-10-01 16:45 ` Philippe Mathieu-Daudé
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 12/15] gdbstub: processes initialization on new peer connection Luc Michel
` (3 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:57 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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 | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/gdbstub.c b/gdbstub.c
index d372972dd3..b6de821025 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1326,10 +1326,42 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
break;
}
goto unknown_command;
}
break;
+ } else if (strncmp(p, "Attach", 6) == 0) {
+ unsigned long pid;
+
+ p += 7;
+
+ qemu_strtoul(p, &p, 16, &pid);
+
+ 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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 11/15] gdbstub: add support for vAttach packets
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 11/15] gdbstub: add support for vAttach packets Luc Michel
@ 2018-10-01 16:45 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 16:45 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
Hi Luc,
On 01/10/2018 13:57, Luc Michel wrote:
> 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 | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d372972dd3..b6de821025 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1326,10 +1326,42 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> break;
> }
> goto unknown_command;
> }
> break;
> + } else if (strncmp(p, "Attach", 6) == 0) {
Shouldn't this be strncmp("Attach;", 7)?
> + unsigned long pid;
> +
> + p += 7;
> +
> + qemu_strtoul(p, &p, 16, &pid);
You should check for EINVAL/ERANGE here.
Rest of the patch is OK.
> +
> + 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 */
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 12/15] gdbstub: processes initialization on new peer connection
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (10 preceding siblings ...)
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 11/15] gdbstub: add support for vAttach packets Luc Michel
@ 2018-10-01 11:57 ` Luc Michel
2018-10-04 17:42 ` Alistair Francis
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 13/15] gdbstub: gdb_set_stop_cpu: ignore request when process is not attached Luc Michel
` (2 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:57 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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>
---
gdbstub.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index b6de821025..c27a3edf1d 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2221,13 +2221,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;
@@ -2309,12 +2310,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;
@@ -2474,19 +2486,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 12/15] gdbstub: processes initialization on new peer connection
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 12/15] gdbstub: processes initialization on new peer connection Luc Michel
@ 2018-10-04 17:42 ` Alistair Francis
0 siblings, 0 replies; 39+ messages in thread
From: Alistair Francis @ 2018-10-04 17:42 UTC (permalink / raw)
To: Luc Michel
Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
Alistair Francis, Mark Burton, Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On Mon, Oct 1, 2018 at 5:07 AM Luc Michel <luc.michel@greensocs.com> wrote:
>
> 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>
Alistair
> ---
> gdbstub.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index b6de821025..c27a3edf1d 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2221,13 +2221,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;
> @@ -2309,12 +2310,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;
> @@ -2474,19 +2486,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.0
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 13/15] gdbstub: gdb_set_stop_cpu: ignore request when process is not attached
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (11 preceding siblings ...)
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 12/15] gdbstub: processes initialization on new peer connection Luc Michel
@ 2018-10-01 11:57 ` Luc Michel
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 14/15] gdbstub: add multiprocess extension support Luc Michel
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups Luc Michel
14 siblings, 0 replies; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:57 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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 get confused if it receives packets with a
thread-id it does not know about.
Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
gdbstub.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/gdbstub.c b/gdbstub.c
index c27a3edf1d..51cc11981e 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1763,10 +1763,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.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v2 14/15] gdbstub: add multiprocess extension support
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (12 preceding siblings ...)
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 13/15] gdbstub: gdb_set_stop_cpu: ignore request when process is not attached Luc Michel
@ 2018-10-01 11:57 ` Luc Michel
2018-10-01 16:35 ` Philippe Mathieu-Daudé
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups Luc Michel
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:57 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
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>
---
gdbstub.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/gdbstub.c b/gdbstub.c
index 51cc11981e..89f6803533 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1692,15 +1692,19 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, "OK");
break;
}
#endif /* !CONFIG_USER_ONLY */
if (is_query_packet(p, "Supported", ':')) {
+ if (strstr(p, "multiprocess+")) {
+ s->multiprocess = true;
+ }
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+");
}
+ pstrcat(buf, sizeof(buf), ";multiprocess+");
put_packet(s, buf);
break;
}
if (strncmp(p, "Xfer:features:read:", 19) == 0) {
const char *xml;
--
2.19.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 14/15] gdbstub: add multiprocess extension support
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 14/15] gdbstub: add multiprocess extension support Luc Michel
@ 2018-10-01 16:35 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-01 16:35 UTC (permalink / raw)
To: Luc Michel, qemu-devel
Cc: Peter Maydell, alistair, mark.burton, Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
On 01/10/2018 13:57, Luc Michel wrote:
> 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>
> ---
> gdbstub.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 51cc11981e..89f6803533 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1692,15 +1692,19 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> put_packet(s, "OK");
> break;
> }
> #endif /* !CONFIG_USER_ONLY */
> if (is_query_packet(p, "Supported", ':')) {
> + if (strstr(p, "multiprocess+")) {
> + s->multiprocess = true;
> + }
> 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+");
> }
Maybe move altogether:
if (strstr(p, "multiprocess+")) {
s->multiprocess = true;
}
> + pstrcat(buf, sizeof(buf), ";multiprocess+");
Regardless:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> put_packet(s, buf);
> break;
> }
> if (strncmp(p, "Xfer:features:read:", 19) == 0) {
> const char *xml;
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-01 11:56 [Qemu-devel] [PATCH v2 00/15] gdbstub: support for the multiprocess extension Luc Michel
` (13 preceding siblings ...)
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 14/15] gdbstub: add multiprocess extension support Luc Michel
@ 2018-10-01 11:57 ` Luc Michel
2018-10-02 11:33 ` Philippe Mathieu-Daudé
14 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-01 11:57 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, qemu-arm, Peter Maydell, saipava, edgari, alistair,
Philippe Mathieu-Daudé,
mark.burton
Create two separate QOM containers for APUs and RPUs to indicate to the
GDB stub that those CPUs should be put in different processes.
Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
hw/arm/xlnx-zynqmp.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index c195040350..5e92adbc71 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -22,10 +22,11 @@
#include "hw/arm/xlnx-zynqmp.h"
#include "hw/intc/arm_gic_common.h"
#include "exec/address-spaces.h"
#include "sysemu/kvm.h"
#include "kvm_arm.h"
+#include "exec/gdbstub.h"
#define GIC_NUM_SPI_INTR 160
#define ARM_PHYS_TIMER_PPI 30
#define ARM_VIRT_TIMER_PPI 27
@@ -175,17 +176,18 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
Error **errp)
{
Error *err = NULL;
int i;
int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
+ Object *rpu_group = gdb_cpu_group_container_get(OBJECT(s));
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(rpu_group, "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 */
@@ -210,13 +212,14 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
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 *apu_group = gdb_cpu_group_container_get(obj);
for (i = 0; i < num_apus; i++) {
- object_initialize_child(obj, "apu-cpu[*]", &s->apu_cpu[i],
+ object_initialize_child(apu_group, "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),
--
2.19.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-01 11:57 ` [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups Luc Michel
@ 2018-10-02 11:33 ` Philippe Mathieu-Daudé
2018-10-02 11:58 ` Peter Maydell
0 siblings, 1 reply; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-02 11:33 UTC (permalink / raw)
To: Luc Michel, Eduardo Habkost, Andreas Färber, Thomas Huth,
Paolo Bonzini
Cc: qemu-devel, Peter Maydell, alistair, mark.burton,
Philippe Mathieu-Daudé,
saipava, edgari, qemu-arm
Cc'ing more QOM involved people.
On 01/10/2018 13:57, Luc Michel wrote:
> Create two separate QOM containers for APUs and RPUs to indicate to the
> GDB stub that those CPUs should be put in different processes.
>
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> ---
> hw/arm/xlnx-zynqmp.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index c195040350..5e92adbc71 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -22,10 +22,11 @@
> #include "hw/arm/xlnx-zynqmp.h"
> #include "hw/intc/arm_gic_common.h"
> #include "exec/address-spaces.h"
> #include "sysemu/kvm.h"
> #include "kvm_arm.h"
> +#include "exec/gdbstub.h"
>
> #define GIC_NUM_SPI_INTR 160
>
> #define ARM_PHYS_TIMER_PPI 30
> #define ARM_VIRT_TIMER_PPI 27
> @@ -175,17 +176,18 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
> Error **errp)
> {
> Error *err = NULL;
> int i;
> int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
> + Object *rpu_group = gdb_cpu_group_container_get(OBJECT(s));
I'd rather keep this generic: not involve 'gdb' container name.
(qemu) info qom-tree
/machine (xlnx-zcu102-machine)
/soc (xlnx,zynqmp)
/gdb-group[0] (container)
/apu-cpu[3] (cortex-a53-arm-cpu)
/unnamed-gpio-in[0] (irq)
/unnamed-gpio-in[2] (irq)
/unnamed-gpio-in[1] (irq)
/unnamed-gpio-in[3] (irq)
...
/gdb-group[1] (container)
Maybe your create_processes() from patch [1] should enumerate all CPUs
with object_resolve_path_type("", TYPE_CPU, NULL) then sort by cpu?
[1] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00089.html
>
> 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(rpu_group, "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 */
> @@ -210,13 +212,14 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
> 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 *apu_group = gdb_cpu_group_container_get(obj);
>
> for (i = 0; i < num_apus; i++) {
> - object_initialize_child(obj, "apu-cpu[*]", &s->apu_cpu[i],
> + object_initialize_child(apu_group, "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),
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-02 11:33 ` Philippe Mathieu-Daudé
@ 2018-10-02 11:58 ` Peter Maydell
2018-10-03 11:44 ` Luc Michel
0 siblings, 1 reply; 39+ messages in thread
From: Peter Maydell @ 2018-10-02 11:58 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Luc Michel, Eduardo Habkost, Andreas Färber, Thomas Huth,
Paolo Bonzini, QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On 2 October 2018 at 12:33, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Cc'ing more QOM involved people.
>
> On 01/10/2018 13:57, Luc Michel wrote:
>> Create two separate QOM containers for APUs and RPUs to indicate to the
>> GDB stub that those CPUs should be put in different processes.
>>
>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>> ---
>> hw/arm/xlnx-zynqmp.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>> index c195040350..5e92adbc71 100644
>> --- a/hw/arm/xlnx-zynqmp.c
>> +++ b/hw/arm/xlnx-zynqmp.c
>> @@ -22,10 +22,11 @@
>> #include "hw/arm/xlnx-zynqmp.h"
>> #include "hw/intc/arm_gic_common.h"
>> #include "exec/address-spaces.h"
>> #include "sysemu/kvm.h"
>> #include "kvm_arm.h"
>> +#include "exec/gdbstub.h"
>>
>> #define GIC_NUM_SPI_INTR 160
>>
>> #define ARM_PHYS_TIMER_PPI 30
>> #define ARM_VIRT_TIMER_PPI 27
>> @@ -175,17 +176,18 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
>> Error **errp)
>> {
>> Error *err = NULL;
>> int i;
>> int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
>> + Object *rpu_group = gdb_cpu_group_container_get(OBJECT(s));
>
> I'd rather keep this generic: not involve 'gdb' container name.
Yes, I agree. We should structure how we construct our
model to follow what the hardware has (two CPU clusters
with 4 cores each), and then the gdb code should introspect
the system later to decide how it exposes things to the gdb
user. GDB specifics should (as far as possible) be kept out
of the board code.
The fact that there are two clusters here also
affects other things, like whether they have the
same view of memory, whether they can share translated
code (they shouldn't but do at the moment), and so on --
it's not just a GDB-relevant distinction. So we should
be modelling it somehow, definitely. I don't have a clear
view how just yet.
This probably ties into the stuff I have somewhere on
my todo list about supporting multiple heterogenous
systems. The problem with this xilinx board is that it
is trying to model that kind of system but we don't actually
properly support that in QEMU yet.
thanks
-- PMM
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-02 11:58 ` Peter Maydell
@ 2018-10-03 11:44 ` Luc Michel
2018-10-04 16:07 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 39+ messages in thread
From: Luc Michel @ 2018-10-03 11:44 UTC (permalink / raw)
To: Peter Maydell, Philippe Mathieu-Daudé
Cc: Eduardo Habkost, Andreas Färber, Thomas Huth, Paolo Bonzini,
QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On 10/2/18 1:58 PM, Peter Maydell wrote:
> On 2 October 2018 at 12:33, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> Cc'ing more QOM involved people.
>>
>> On 01/10/2018 13:57, Luc Michel wrote:
>>> Create two separate QOM containers for APUs and RPUs to indicate to the
>>> GDB stub that those CPUs should be put in different processes.
>>>
>>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>>> ---
>>> hw/arm/xlnx-zynqmp.c | 7 +++++--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>>> index c195040350..5e92adbc71 100644
>>> --- a/hw/arm/xlnx-zynqmp.c
>>> +++ b/hw/arm/xlnx-zynqmp.c
>>> @@ -22,10 +22,11 @@
>>> #include "hw/arm/xlnx-zynqmp.h"
>>> #include "hw/intc/arm_gic_common.h"
>>> #include "exec/address-spaces.h"
>>> #include "sysemu/kvm.h"
>>> #include "kvm_arm.h"
>>> +#include "exec/gdbstub.h"
>>>
>>> #define GIC_NUM_SPI_INTR 160
>>>
>>> #define ARM_PHYS_TIMER_PPI 30
>>> #define ARM_VIRT_TIMER_PPI 27
>>> @@ -175,17 +176,18 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
>>> Error **errp)
>>> {
>>> Error *err = NULL;
>>> int i;
>>> int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
>>> + Object *rpu_group = gdb_cpu_group_container_get(OBJECT(s));
>>
>> I'd rather keep this generic: not involve 'gdb' container name.
>
> Yes, I agree. We should structure how we construct our
> model to follow what the hardware has (two CPU clusters
> with 4 cores each), and then the gdb code should introspect
> the system later to decide how it exposes things to the gdb
> user. GDB specifics should (as far as possible) be kept out
> of the board code.
>
> The fact that there are two clusters here also
> affects other things, like whether they have the
> same view of memory, whether they can share translated
> code (they shouldn't but do at the moment), and so on --
> it's not just a GDB-relevant distinction. So we should
> be modelling it somehow, definitely. I don't have a clear
> view how just yet.
So for now, maybe it's better to rely on an heuristic such as the one
suggested by Philippe in the gdb code to group the CPUs. Once QEMU gains
more supports for such heterogeneous architectures, we can remove the
heuristic and put the proper QOM introspection code instead.
Luc
>
> This probably ties into the stuff I have somewhere on
> my todo list about supporting multiple heterogenous
> systems. The problem with this xilinx board is that it
> is trying to model that kind of system but we don't actually
> properly support that in QEMU yet.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-03 11:44 ` Luc Michel
@ 2018-10-04 16:07 ` Philippe Mathieu-Daudé
2018-10-04 19:52 ` Eduardo Habkost
0 siblings, 1 reply; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-04 16:07 UTC (permalink / raw)
To: Luc Michel, Eduardo Habkost
Cc: Peter Maydell, Andreas Färber, Thomas Huth, Paolo Bonzini,
QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On 03/10/2018 13:44, Luc Michel wrote:
> On 10/2/18 1:58 PM, Peter Maydell wrote:
>> On 2 October 2018 at 12:33, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>> Cc'ing more QOM involved people.
>>>
>>> On 01/10/2018 13:57, Luc Michel wrote:
>>>> Create two separate QOM containers for APUs and RPUs to indicate to the
>>>> GDB stub that those CPUs should be put in different processes.
>>>>
>>>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>>>> ---
>>>> hw/arm/xlnx-zynqmp.c | 7 +++++--
>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
>>>> index c195040350..5e92adbc71 100644
>>>> --- a/hw/arm/xlnx-zynqmp.c
>>>> +++ b/hw/arm/xlnx-zynqmp.c
>>>> @@ -22,10 +22,11 @@
>>>> #include "hw/arm/xlnx-zynqmp.h"
>>>> #include "hw/intc/arm_gic_common.h"
>>>> #include "exec/address-spaces.h"
>>>> #include "sysemu/kvm.h"
>>>> #include "kvm_arm.h"
>>>> +#include "exec/gdbstub.h"
>>>>
>>>> #define GIC_NUM_SPI_INTR 160
>>>>
>>>> #define ARM_PHYS_TIMER_PPI 30
>>>> #define ARM_VIRT_TIMER_PPI 27
>>>> @@ -175,17 +176,18 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
>>>> Error **errp)
>>>> {
>>>> Error *err = NULL;
>>>> int i;
>>>> int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
>>>> + Object *rpu_group = gdb_cpu_group_container_get(OBJECT(s));
>>>
>>> I'd rather keep this generic: not involve 'gdb' container name.
>>
>> Yes, I agree. We should structure how we construct our
>> model to follow what the hardware has (two CPU clusters
>> with 4 cores each), and then the gdb code should introspect
>> the system later to decide how it exposes things to the gdb
>> user. GDB specifics should (as far as possible) be kept out
>> of the board code.
>>
>> The fact that there are two clusters here also
>> affects other things, like whether they have the
>> same view of memory, whether they can share translated
>> code (they shouldn't but do at the moment), and so on --
>> it's not just a GDB-relevant distinction. So we should
>> be modelling it somehow, definitely. I don't have a clear
>> view how just yet.
>
> So for now, maybe it's better to rely on an heuristic such as the one
> suggested by Philippe in the gdb code to group the CPUs. Once QEMU gains
> more supports for such heterogeneous architectures, we can remove the
> heuristic and put the proper QOM introspection code instead.
I'm not sure this is the best approach, just suggested because using
object_resolve_path_type("", TYPE_CPU, NULL) seemed to me the
quicker/easiest approach.
Eduardo: Do you have other thoughts on how to resolve those generic
containers, without involving any gdb-specific tag?
>> This probably ties into the stuff I have somewhere on
>> my todo list about supporting multiple heterogenous
>> systems. The problem with this xilinx board is that it
>> is trying to model that kind of system but we don't actually
>> properly support that in QEMU yet.
>>
>> thanks
>> -- PMM
>>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-04 16:07 ` Philippe Mathieu-Daudé
@ 2018-10-04 19:52 ` Eduardo Habkost
2018-10-04 20:01 ` Peter Maydell
0 siblings, 1 reply; 39+ messages in thread
From: Eduardo Habkost @ 2018-10-04 19:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Luc Michel, Peter Maydell, Andreas Färber, Thomas Huth,
Paolo Bonzini, QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On Thu, Oct 04, 2018 at 06:07:45PM +0200, Philippe Mathieu-Daudé wrote:
> On 03/10/2018 13:44, Luc Michel wrote:
> > On 10/2/18 1:58 PM, Peter Maydell wrote:
> >> On 2 October 2018 at 12:33, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>> Cc'ing more QOM involved people.
> >>>
> >>> On 01/10/2018 13:57, Luc Michel wrote:
> >>>> Create two separate QOM containers for APUs and RPUs to indicate to the
> >>>> GDB stub that those CPUs should be put in different processes.
> >>>>
> >>>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> >>>> ---
> >>>> hw/arm/xlnx-zynqmp.c | 7 +++++--
> >>>> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> >>>> index c195040350..5e92adbc71 100644
> >>>> --- a/hw/arm/xlnx-zynqmp.c
> >>>> +++ b/hw/arm/xlnx-zynqmp.c
> >>>> @@ -22,10 +22,11 @@
> >>>> #include "hw/arm/xlnx-zynqmp.h"
> >>>> #include "hw/intc/arm_gic_common.h"
> >>>> #include "exec/address-spaces.h"
> >>>> #include "sysemu/kvm.h"
> >>>> #include "kvm_arm.h"
> >>>> +#include "exec/gdbstub.h"
> >>>>
> >>>> #define GIC_NUM_SPI_INTR 160
> >>>>
> >>>> #define ARM_PHYS_TIMER_PPI 30
> >>>> #define ARM_VIRT_TIMER_PPI 27
> >>>> @@ -175,17 +176,18 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
> >>>> Error **errp)
> >>>> {
> >>>> Error *err = NULL;
> >>>> int i;
> >>>> int num_rpus = MIN(smp_cpus - XLNX_ZYNQMP_NUM_APU_CPUS, XLNX_ZYNQMP_NUM_RPU_CPUS);
> >>>> + Object *rpu_group = gdb_cpu_group_container_get(OBJECT(s));
> >>>
> >>> I'd rather keep this generic: not involve 'gdb' container name.
> >>
> >> Yes, I agree. We should structure how we construct our
> >> model to follow what the hardware has (two CPU clusters
> >> with 4 cores each), and then the gdb code should introspect
> >> the system later to decide how it exposes things to the gdb
> >> user. GDB specifics should (as far as possible) be kept out
> >> of the board code.
Agreed.
> >>
> >> The fact that there are two clusters here also
> >> affects other things, like whether they have the
> >> same view of memory, whether they can share translated
> >> code (they shouldn't but do at the moment), and so on --
> >> it's not just a GDB-relevant distinction. So we should
> >> be modelling it somehow, definitely. I don't have a clear
> >> view how just yet.
> >
> > So for now, maybe it's better to rely on an heuristic such as the one
> > suggested by Philippe in the gdb code to group the CPUs. Once QEMU gains
> > more supports for such heterogeneous architectures, we can remove the
> > heuristic and put the proper QOM introspection code instead.
>
> I'm not sure this is the best approach, just suggested because using
> object_resolve_path_type("", TYPE_CPU, NULL) seemed to me the
> quicker/easiest approach.
>
> Eduardo: Do you have other thoughts on how to resolve those generic
> containers, without involving any gdb-specific tag?
Changing the object hierarchy based on GDB groups doesn't seem
right, but I don't think it would be a big deal if we have the
board code explicitly telling the GDB code how to group the CPUs.
If you really want to do it implicitly, would it work if you
simply group the CPUs based on object_get_canonical_path()?
If a more explicit GDB grouping API is acceptable, what about
just adding a INTERFACE_GDB_GROUP interface name to (existing)
container objects that we expect to become GDB groups?
I'm not sure which way is better. I'm a bit worried that making
things too implicit could easily break (e.g. if somebody changes
the CPU QOM hierarchy in the future for unrelated reasons).
>
> >> This probably ties into the stuff I have somewhere on
> >> my todo list about supporting multiple heterogenous
> >> systems. The problem with this xilinx board is that it
> >> is trying to model that kind of system but we don't actually
> >> properly support that in QEMU yet.
> >>
> >> thanks
> >> -- PMM
> >>
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-04 19:52 ` Eduardo Habkost
@ 2018-10-04 20:01 ` Peter Maydell
2018-10-04 21:53 ` Eduardo Habkost
0 siblings, 1 reply; 39+ messages in thread
From: Peter Maydell @ 2018-10-04 20:01 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Philippe Mathieu-Daudé,
Luc Michel, Andreas Färber, Thomas Huth, Paolo Bonzini,
QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On 4 October 2018 at 20:52, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Changing the object hierarchy based on GDB groups doesn't seem
> right, but I don't think it would be a big deal if we have the
> board code explicitly telling the GDB code how to group the CPUs.
>
> If you really want to do it implicitly, would it work if you
> simply group the CPUs based on object_get_canonical_path()?
>
> If a more explicit GDB grouping API is acceptable, what about
> just adding a INTERFACE_GDB_GROUP interface name to (existing)
> container objects that we expect to become GDB groups?
>
> I'm not sure which way is better. I'm a bit worried that making
> things too implicit could easily break (e.g. if somebody changes
> the CPU QOM hierarchy in the future for unrelated reasons).
I don't want things implicit. I just don't want the explicitness
to be "this is all about GDB", because it isn't. I want us
to explicitly say "these 4 CPUs are in one cluster" (or
whatever term we use), because that affects more than merely GDB.
thanks
-- PMM
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-04 20:01 ` Peter Maydell
@ 2018-10-04 21:53 ` Eduardo Habkost
2018-10-05 13:50 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 39+ messages in thread
From: Eduardo Habkost @ 2018-10-04 21:53 UTC (permalink / raw)
To: Peter Maydell
Cc: Philippe Mathieu-Daudé,
Luc Michel, Andreas Färber, Thomas Huth, Paolo Bonzini,
QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On Thu, Oct 04, 2018 at 09:01:09PM +0100, Peter Maydell wrote:
> On 4 October 2018 at 20:52, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > Changing the object hierarchy based on GDB groups doesn't seem
> > right, but I don't think it would be a big deal if we have the
> > board code explicitly telling the GDB code how to group the CPUs.
> >
> > If you really want to do it implicitly, would it work if you
> > simply group the CPUs based on object_get_canonical_path()?
> >
> > If a more explicit GDB grouping API is acceptable, what about
> > just adding a INTERFACE_GDB_GROUP interface name to (existing)
> > container objects that we expect to become GDB groups?
> >
> > I'm not sure which way is better. I'm a bit worried that making
> > things too implicit could easily break (e.g. if somebody changes
> > the CPU QOM hierarchy in the future for unrelated reasons).
>
> I don't want things implicit. I just don't want the explicitness
> to be "this is all about GDB", because it isn't. I want us
> to explicitly say "these 4 CPUs are in one cluster" (or
> whatever term we use), because that affects more than merely GDB.
We already have a way to say "these 4 CPUs are in one cluster",
don't we? That's the QOM hierarchy.
My question is if "the CPUs are in one cluster" should implicitly
mean "the CPUs are in one GDB group".
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-04 21:53 ` Eduardo Habkost
@ 2018-10-05 13:50 ` Philippe Mathieu-Daudé
2018-10-05 18:49 ` Eduardo Habkost
0 siblings, 1 reply; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-10-05 13:50 UTC (permalink / raw)
To: Eduardo Habkost, Peter Maydell
Cc: Luc Michel, Andreas Färber, Thomas Huth, Paolo Bonzini,
QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On 04/10/2018 23:53, Eduardo Habkost wrote:
> On Thu, Oct 04, 2018 at 09:01:09PM +0100, Peter Maydell wrote:
>> On 4 October 2018 at 20:52, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> Changing the object hierarchy based on GDB groups doesn't seem
>>> right, but I don't think it would be a big deal if we have the
>>> board code explicitly telling the GDB code how to group the CPUs.
>>>
>>> If you really want to do it implicitly, would it work if you
>>> simply group the CPUs based on object_get_canonical_path()?
>>>
>>> If a more explicit GDB grouping API is acceptable, what about
>>> just adding a INTERFACE_GDB_GROUP interface name to (existing)
>>> container objects that we expect to become GDB groups?
>>>
>>> I'm not sure which way is better. I'm a bit worried that making
>>> things too implicit could easily break (e.g. if somebody changes
>>> the CPU QOM hierarchy in the future for unrelated reasons).
>>
>> I don't want things implicit. I just don't want the explicitness
>> to be "this is all about GDB", because it isn't. I want us
>> to explicitly say "these 4 CPUs are in one cluster" (or
>> whatever term we use), because that affects more than merely GDB.
>
> We already have a way to say "these 4 CPUs are in one cluster",
> don't we? That's the QOM hierarchy.
>
> My question is if "the CPUs are in one cluster" should implicitly
> mean "the CPUs are in one GDB group".
>
What about having the container implement INTERFACE_CPU_CLUSTER?
Or even cleaner, add a TYPE_CPU_CLUSTER which is just a container for
TYPE_CPU[*]?
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-05 13:50 ` Philippe Mathieu-Daudé
@ 2018-10-05 18:49 ` Eduardo Habkost
2018-10-17 17:02 ` Luc Michel
0 siblings, 1 reply; 39+ messages in thread
From: Eduardo Habkost @ 2018-10-05 18:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Luc Michel, Andreas Färber, Thomas Huth,
Paolo Bonzini, QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On Fri, Oct 05, 2018 at 03:50:01PM +0200, Philippe Mathieu-Daudé wrote:
> On 04/10/2018 23:53, Eduardo Habkost wrote:
> > On Thu, Oct 04, 2018 at 09:01:09PM +0100, Peter Maydell wrote:
> >> On 4 October 2018 at 20:52, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>> Changing the object hierarchy based on GDB groups doesn't seem
> >>> right, but I don't think it would be a big deal if we have the
> >>> board code explicitly telling the GDB code how to group the CPUs.
> >>>
> >>> If you really want to do it implicitly, would it work if you
> >>> simply group the CPUs based on object_get_canonical_path()?
> >>>
> >>> If a more explicit GDB grouping API is acceptable, what about
> >>> just adding a INTERFACE_GDB_GROUP interface name to (existing)
> >>> container objects that we expect to become GDB groups?
> >>>
> >>> I'm not sure which way is better. I'm a bit worried that making
> >>> things too implicit could easily break (e.g. if somebody changes
> >>> the CPU QOM hierarchy in the future for unrelated reasons).
> >>
> >> I don't want things implicit. I just don't want the explicitness
> >> to be "this is all about GDB", because it isn't. I want us
> >> to explicitly say "these 4 CPUs are in one cluster" (or
> >> whatever term we use), because that affects more than merely GDB.
> >
> > We already have a way to say "these 4 CPUs are in one cluster",
> > don't we? That's the QOM hierarchy.
> >
> > My question is if "the CPUs are in one cluster" should implicitly
> > mean "the CPUs are in one GDB group".
> >
>
> What about having the container implement INTERFACE_CPU_CLUSTER?
>
> Or even cleaner, add a TYPE_CPU_CLUSTER which is just a container for
> TYPE_CPU[*]?
Sounds good to me. An interface sounds more flexible, to avoid
clashing with existing type hierarchies for
nodes/sockets/cores/etc.
But first of all, I think we need a good definition of what
exactly is a cluster, and what is the purpose of this
abstraction.
If we end up with a new abstraction that is only going to used by
GDB code and nothing else, I don't see the point of pretending it
is not a GDB-specific abstraction.
--
Eduardo
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 15/15] arm/xlnx-zynqmp: put APUs and RPUs in separate GDB groups
2018-10-05 18:49 ` Eduardo Habkost
@ 2018-10-17 17:02 ` Luc Michel
0 siblings, 0 replies; 39+ messages in thread
From: Luc Michel @ 2018-10-17 17:02 UTC (permalink / raw)
To: Eduardo Habkost, Philippe Mathieu-Daudé
Cc: Peter Maydell, Andreas Färber, Thomas Huth, Paolo Bonzini,
QEMU Developers, Alistair Francis, Mark Burton,
Philippe Mathieu-Daudé,
Sai Pavan Boddu, Edgar Iglesias, qemu-arm
On 10/5/18 8:49 PM, Eduardo Habkost wrote:
> On Fri, Oct 05, 2018 at 03:50:01PM +0200, Philippe Mathieu-Daudé wrote:
>> On 04/10/2018 23:53, Eduardo Habkost wrote:
>>> On Thu, Oct 04, 2018 at 09:01:09PM +0100, Peter Maydell wrote:
>>>> On 4 October 2018 at 20:52, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>> Changing the object hierarchy based on GDB groups doesn't seem
>>>>> right, but I don't think it would be a big deal if we have the
>>>>> board code explicitly telling the GDB code how to group the CPUs.
>>>>>
>>>>> If you really want to do it implicitly, would it work if you
>>>>> simply group the CPUs based on object_get_canonical_path()?
>>>>>
>>>>> If a more explicit GDB grouping API is acceptable, what about
>>>>> just adding a INTERFACE_GDB_GROUP interface name to (existing)
>>>>> container objects that we expect to become GDB groups?
>>>>>
>>>>> I'm not sure which way is better. I'm a bit worried that making
>>>>> things too implicit could easily break (e.g. if somebody changes
>>>>> the CPU QOM hierarchy in the future for unrelated reasons).
>>>>
>>>> I don't want things implicit. I just don't want the explicitness
>>>> to be "this is all about GDB", because it isn't. I want us
>>>> to explicitly say "these 4 CPUs are in one cluster" (or
>>>> whatever term we use), because that affects more than merely GDB.
>>>
>>> We already have a way to say "these 4 CPUs are in one cluster",
>>> don't we? That's the QOM hierarchy.
>>>
>>> My question is if "the CPUs are in one cluster" should implicitly
>>> mean "the CPUs are in one GDB group".
>>>
>>
>> What about having the container implement INTERFACE_CPU_CLUSTER?
>>
>> Or even cleaner, add a TYPE_CPU_CLUSTER which is just a container for
>> TYPE_CPU[*]?
>
> Sounds good to me. An interface sounds more flexible, to avoid
> clashing with existing type hierarchies for
> nodes/sockets/cores/etc.
But we still need a container sub-class specialized for that matter
right? Or are we going to have the generic container class implements
this not-so-generic interface?
>
> But first of all, I think we need a good definition of what
> exactly is a cluster, and what is the purpose of this
> abstraction.
I think it has implications that go way beyond this patch set.
Here we want to put the APUs (cortex-a53) and the RPUs (cortex-r5) in
different groups mainly because they have different architectures (I
think the address space is more or less the same for all the CPUs in
this SoC).
The current configuration is wrong since the A53 and the R5 probably
don't have the same features, hence for the same piece of code,
translations can differ from one another (e.g. one could have VFPv4 and
not the other). So the translation cache should not be shared.
We could imagine modelling more complex heterogeneous architectures. One
that come to my mind is a many-core chip from Kalray, which is organised
in 16 clusters of 16 cores each. In a cluster, the cores are SMP,
accessing the same SRAM. But inter-cluster communication is done through
an explicit NoC, using DMAs.
In that case, a "cluster" QEMU abstraction would make sense since cores
between clusters must not share the same address space, nor translation
cache.
Regarding GDB, two CPUs should be put in different groups if:
- Their architectures are different
- or if the extra XML descriptions we send to GDB for those CPUs are
different (extra registers).
So for now I think we can introduce this new "cpu cluster" abstraction
as it makes sense for the kind of system we (could) want to model in
QEMU. For now it will only be used by the GDB stub but it definitely has
a deeper implication.
>
> If we end up with a new abstraction that is only going to used by
> GDB code and nothing else, I don't see the point of pretending it
> is not a GDB-specific abstraction.
>
^ permalink raw reply [flat|nested] 39+ messages in thread