[Qemu-devel] [PATCH 11/15] Improve debugging of SMP guests - v2

[Jan Kiszka <jan.kiszka@siemens.com>]

This patch enhances QEMU's built-in debugger for SMP guest debugging.
It allows to set the debugger focus explicitly via the monitor command
"cpu", and it automatically switches the focus on breakpoint hit to
the reporting CPU.

Furthermore, the patch propagates breakpoint and watchpoint insertions
or removals to all CPUs, not just the current one as it was the case so
far. Without this property, SMP guest debugging is practically
unfeasible.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 gdbstub.c |   85 ++++++++++++++++++++++++++++++++++++++++----------------------
 monitor.c |   19 ++++++++-----
 monitor.h |   15 ++++++++++
 vl.c      |    2 +
 4 files changed, 85 insertions(+), 36 deletions(-)

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;
@@ -977,43 +977,71 @@ const int xlat_gdb_type[] = {
 };
 #endif
 
-static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
-                                 target_ulong len, int type)
+static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type)
 {
+    CPUState *env;
+    int err = 0;
+
     switch (type) {
     case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
-        return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+            if (err)
+                break;
+        }
+        return err;
 #ifndef CONFIG_USER_ONLY
     case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
-        return cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
-                                     NULL);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
+                                        NULL);
+            if (err)
+                break;
+        }
+        return err;
 #endif
     default:
         return -ENOSYS;
     }
 }
 
-static int gdb_breakpoint_remove(CPUState *env, target_ulong addr,
-                                 target_ulong len, int type)
+static int gdb_breakpoint_remove(target_ulong addr, target_ulong len, int type)
 {
+    CPUState *env;
+    int err = 0;
+
     switch (type) {
     case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
-        return cpu_breakpoint_remove(env, addr, BP_GDB);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_breakpoint_remove(env, addr, BP_GDB);
+            if (err)
+                break;
+        }
+        return err;
 #ifndef CONFIG_USER_ONLY
     case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
-        return cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+            if (err)
+                break;
+        }
+        return err;
 #endif
     default:
         return -ENOSYS;
     }
 }
 
-static void gdb_breakpoint_remove_all(CPUState *env)
+static void gdb_breakpoint_remove_all(void)
 {
-    cpu_breakpoint_remove_all(env, BP_GDB);
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        cpu_breakpoint_remove_all(env, BP_GDB);
 #ifndef CONFIG_USER_ONLY
-    cpu_watchpoint_remove_all(env, BP_GDB);
+        cpu_watchpoint_remove_all(env, BP_GDB);
 #endif
+    }
 }
 
 static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
@@ -1039,7 +1067,7 @@ static int gdb_handle_packet(GDBState *s
          * because gdb is doing and initial connect and the state
          * should be cleaned up.
          */
-        gdb_breakpoint_remove_all(env);
+        gdb_breakpoint_remove_all();
         break;
     case 'c':
         if (*p != '\0') {
@@ -1073,7 +1101,7 @@ static int gdb_handle_packet(GDBState *s
         exit(0);
     case 'D':
         /* Detach packet */
-        gdb_breakpoint_remove_all(env);
+        gdb_breakpoint_remove_all();
         gdb_continue(s);
         put_packet(s, "OK");
         break;
@@ -1116,7 +1144,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 {
@@ -1171,9 +1199,9 @@ static int gdb_handle_packet(GDBState *s
             p++;
         len = strtoull(p, (char **)&p, 16);
         if (ch == 'Z')
-            res = gdb_breakpoint_insert(env, addr, len, type);
+            res = gdb_breakpoint_insert(addr, len, type);
         else
-            res = gdb_breakpoint_remove(env, addr, len, type);
+            res = gdb_breakpoint_remove(addr, len, type);
         if (res >= 0)
              put_packet(s, "OK");
         else if (res == -ENOSYS)
@@ -1237,6 +1265,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];
     const char *type;
     int ret;
@@ -1245,11 +1274,11 @@ 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) {
-            switch (s->env->watchpoint_hit->flags & BP_MEM_ACCESS) {
+        if (env->watchpoint_hit) {
+            switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
                 type = "r";
                 break;
@@ -1261,12 +1290,12 @@ static void gdb_vm_stopped(void *opaque,
                 break;
             }
             snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
-                     SIGTRAP, type, s->env->watchpoint_hit->vaddr);
+                     SIGTRAP, type, env->watchpoint_hit->vaddr);
             put_packet(s, buf);
-            s->env->watchpoint_hit = NULL;
+            env->watchpoint_hit = NULL;
             return;
         }
-	tb_flush(s->env);
+	tb_flush(env);
         ret = SIGTRAP;
     } else if (reason == EXCP_INTERRUPT) {
         ret = SIGINT;
@@ -1336,15 +1365,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;
 
@@ -1508,7 +1537,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;
@@ -1611,7 +1639,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
@@ -261,8 +261,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;
 
@@ -275,10 +274,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;
 }
@@ -302,8 +307,8 @@ static void do_info_cpus(void)
 {
     CPUState *env;
 
-    /* just to set the default cpu if not already done */
-    mon_get_cpu();
+    if (!mon_cpu)
+        mon_set_cpu(first_cpu);
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
         term_printf("%c CPU #%d:",
@@ -326,7 +331,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,15 @@
+#ifndef QEMU_MONITOR_H
+#define QEMU_MONITOR_H
+
+void mon_set_cpu(CPUState *env);
+
+#ifdef CONFIG_USER_ONLY
+static inline CPUState *mon_get_cpu(void)
+{
+    return first_cpu;
+}
+#else
+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"
@@ -7112,6 +7113,7 @@ static int main_loop(void)
                 ret = EXCP_INTERRUPT;
             }
             if (unlikely(ret == EXCP_DEBUG)) {
+                mon_set_cpu(cur_cpu);
                 vm_stop(EXCP_DEBUG);
             }
             /* If all cpus are halted then wait until the next IRQ */