[Qemu-devel] [PATCH] improve gdbstub for SMP debugging

[Jan Kiszka]

Hi,

while trying to debug SMP kernel issues, I always wondered why the
built-in debugger was so annoying unreliable in SMP mode. Finally I
actually looked into the effect, and I found the gdbstub only being
prepared for UP mode.

So here comes an attempt to improve the situation. This patch gets rid
of the static binding of the gdbstub to a single virtual CPU. As we
don't have something like currently_or_last_executed_cpu outside
cpu_exec(), I interconnected the debugger with the monitor, defining
that the currently monitored CPU is also the currently debugged one. If
some interruption requests is received from the debugger front-end, the
monitored CPU is used as environment. On the other hand, if some virtual
CPU hits a breakpoint, the monitored CPU is updated before entering the
debugger.

Of course, this patch also implements that breakpoints and watchpoints
are applied to every virtual CPU, not just the current one (that was the
reason for the unreliable behavior I used to observe :->).

Jan

---
 cpu-defs.h |    3 +++
 gdbstub.c  |   40 ++++++++++++++++++++++------------------
 monitor.c  |   15 ++++++++++-----
 monitor.h  |   18 ++++++++++++++++++
 vl.c       |    2 ++
 5 files changed, 55 insertions(+), 23 deletions(-)

Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -151,3 +151,6 @@ typedef struct CPUTLBEntry {
     const char *cpu_model_str;
 
 #endif
+
+#define foreach_cpu(env) \
+    for(env = first_cpu; env != NULL; env = env->next_cpu)
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -34,6 +34,7 @@
 #include "sysemu.h"
 #include "gdbstub.h"
 #endif
+#include "monitor.h"
 
 #include "qemu_socket.h"
 #ifdef _WIN32
@@ -58,7 +59,6 @@ enum RSState {
     RS_SYSCALL,
 };
 typedef struct GDBState {
-    CPUState *env; /* current CPU */
     enum RSState state; /* parsing state */
     char line_buf[4096];
     int line_buf_index;
@@ -876,6 +876,7 @@ static int gdb_handle_packet(GDBState *s
     uint8_t mem_buf[4096];
     uint32_t *registers;
     target_ulong addr, len;
+    CPUState *env_iter;
 
 #ifdef DEBUG_GDB
     printf("command='%s'\n", line_buf);
@@ -957,7 +958,7 @@ static int gdb_handle_packet(GDBState *s
                 p++;
             type = *p;
             if (gdb_current_syscall_cb)
-                gdb_current_syscall_cb(s->env, ret, err);
+                gdb_current_syscall_cb(mon_get_cpu(), ret, err);
             if (type == 'C') {
                 put_packet(s, "T02");
             } else {
@@ -1015,13 +1016,15 @@ static int gdb_handle_packet(GDBState *s
             p++;
         len = strtoull(p, (char **)&p, 16);
         if (type == 0 || type == 1) {
-            if (cpu_breakpoint_insert(env, addr) < 0)
-                goto breakpoint_error;
+            foreach_cpu(env_iter)
+                if (cpu_breakpoint_insert(env_iter, addr) < 0)
+                    goto breakpoint_error;
             put_packet(s, "OK");
 #ifndef CONFIG_USER_ONLY
         } else if (type == 2) {
-            if (cpu_watchpoint_insert(env, addr) < 0)
-                goto breakpoint_error;
+            foreach_cpu(env_iter)
+                if (cpu_watchpoint_insert(env_iter, addr) < 0)
+                    goto breakpoint_error;
             put_packet(s, "OK");
 #endif
         } else {
@@ -1038,11 +1041,13 @@ static int gdb_handle_packet(GDBState *s
             p++;
         len = strtoull(p, (char **)&p, 16);
         if (type == 0 || type == 1) {
-            cpu_breakpoint_remove(env, addr);
+            foreach_cpu(env_iter)
+                cpu_breakpoint_remove(env_iter, addr);
             put_packet(s, "OK");
 #ifndef CONFIG_USER_ONLY
         } else if (type == 2) {
-            cpu_watchpoint_remove(env, addr);
+            foreach_cpu(env_iter)
+                cpu_watchpoint_remove(env_iter, addr);
             put_packet(s, "OK");
 #endif
         } else {
@@ -1081,6 +1086,7 @@ extern void tb_flush(CPUState *env);
 static void gdb_vm_stopped(void *opaque, int reason)
 {
     GDBState *s = opaque;
+    CPUState *env = mon_get_cpu();
     char buf[256];
     int ret;
 
@@ -1088,18 +1094,18 @@ static void gdb_vm_stopped(void *opaque,
         return;
 
     /* disable single step if it was enable */
-    cpu_single_step(s->env, 0);
+    cpu_single_step(env, 0);
 
     if (reason == EXCP_DEBUG) {
-        if (s->env->watchpoint_hit) {
+        if (env->watchpoint_hit) {
             snprintf(buf, sizeof(buf), "T%02xwatch:" TARGET_FMT_lx ";",
                      SIGTRAP,
-                     s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
+                     env->watchpoint[env->watchpoint_hit - 1].vaddr);
             put_packet(s, buf);
-            s->env->watchpoint_hit = 0;
+            env->watchpoint_hit = 0;
             return;
         }
-       tb_flush(s->env);
+       tb_flush(env);
         ret = SIGTRAP;
     } else if (reason == EXCP_INTERRUPT) {
         ret = SIGINT;
@@ -1169,15 +1175,15 @@ void gdb_do_syscall(gdb_syscall_complete
     va_end(va);
     put_packet(s, buf);
 #ifdef CONFIG_USER_ONLY
-    gdb_handlesig(s->env, 0);
+    gdb_handlesig(mon_get_cpu(), 0);
 #else
-    cpu_interrupt(s->env, CPU_INTERRUPT_EXIT);
+    cpu_interrupt(mon_get_cpu(), CPU_INTERRUPT_EXIT);
 #endif
 }
 
 static void gdb_read_byte(GDBState *s, int ch)
 {
-    CPUState *env = s->env;
+    CPUState *env = mon_get_cpu();
     int i, csum;
     uint8_t reply;
 
@@ -1337,7 +1343,6 @@ static void gdb_accept(void *opaque)
 
     s = &gdbserver_state;
     memset (s, 0, sizeof (GDBState));
-    s->env = first_cpu; /* XXX: allow to change CPU */
     s->fd = fd;
 
     gdb_syscall_state = s;
@@ -1440,7 +1445,6 @@ int gdbserver_start(const char *port)
     if (!s) {
         return -1;
     }
-    s->env = first_cpu; /* XXX: allow to change CPU */
     s->chr = chr;
     qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
                           gdb_chr_event, s);
Index: b/monitor.c
===================================================================
--- a/monitor.c
+++ b/monitor.c
@@ -264,8 +264,7 @@ static void do_info_blockstats(void)
     bdrv_info_stats();
 }
 
-/* get the current CPU defined by the user */
-static int mon_set_cpu(int cpu_index)
+static int mon_set_cpu_index(int cpu_index)
 {
     CPUState *env;
 
@@ -278,10 +277,16 @@ static int mon_set_cpu(int cpu_index)
     return -1;
 }
 
-static CPUState *mon_get_cpu(void)
+void mon_set_cpu(CPUState *env)
+{
+    mon_cpu = env;
+}
+
+/* get the current CPU defined by the user or by a breakpoint hit */
+CPUState *mon_get_cpu(void)
 {
     if (!mon_cpu) {
-        mon_set_cpu(0);
+        mon_set_cpu(first_cpu);
     }
     return mon_cpu;
 }
@@ -335,7 +340,7 @@ static void do_info_cpus(void)
 
 static void do_cpu_set(int index)
 {
-    if (mon_set_cpu(index) < 0)
+    if (mon_set_cpu_index(index) < 0)
         term_printf("Invalid CPU index\n");
 }
 
Index: b/monitor.h
===================================================================
--- /dev/null
+++ b/monitor.h
@@ -0,0 +1,18 @@
+#ifndef QEMU_MONITOR_H
+#define QEMU_MONITOR_H
+
+#ifdef CONFIG_USER_ONLY
+static inline void mon_set_cpu(CPUState *env)
+{
+}
+
+static inline CPUState *mon_get_cpu(void)
+{
+       return first_cpu;
+}
+#else
+void mon_set_cpu(CPUState *env);
+CPUState *mon_get_cpu(void);
+#endif
+
+#endif /* QEMU_MONITOR_H */
Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -33,6 +33,7 @@
 #include "console.h"
 #include "sysemu.h"
 #include "gdbstub.h"
+#include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "block.h"
@@ -7509,6 +7510,7 @@ static int main_loop(void)
                 ret = EXCP_INTERRUPT;
             }
             if (ret == EXCP_DEBUG) {
+                mon_set_cpu(cur_cpu);
                 vm_stop(EXCP_DEBUG);
             }
             /* If all cpus are halted then wait until the next IRQ */