[Qemu-devel] [PATCH 5/5] Enhance SMP guest debugging

[Jan Kiszka]

I bet a few people already stumbled over the effect that QEMU only
maintains breakpoints and watchpoints for the first CPU, even in SMP
mode. The results are missed breakpoints and confused operators.

This patch adds SMP awareness to the debugger. It allows to set the
debugger focus explicitly via the monitor command "cpu", but it also
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.

I considered moving the breakpoint and watchpoint lists out of CPUState
as they are effectively only useful when being in sync across all CPUs.
But I stepped back from this for now until I get an ACK on such a
change. At that chance, I would also suggest to turn the lists into
dynamic ones, overcoming their hard limits.

Signed-off-by: Jan Kiszka <address@hidden>
---
 cpu-defs.h |    3 +++
 exec.c     |   54 ++++++++++++++++++++++++++++++++++++++----------------
 gdbstub.c  |   25 ++++++++++++-------------
 monitor.c  |   19 ++++++++++++-------
 monitor.h  |   15 +++++++++++++++
 vl.c       |    2 ++
 6 files changed, 82 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;
@@ -1050,7 +1050,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 {
@@ -1202,6 +1202,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];
     char *type;
     int ret;
@@ -1210,11 +1211,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[s->env->watchpoint_hit - 1].type) {
+        if (env->watchpoint_hit) {
+            switch (env->watchpoint[env->watchpoint_hit - 1].type) {
             case GDB_WATCHPOINT_READ:
                 type = "r";
                 break;
@@ -1227,12 +1228,12 @@ static void gdb_vm_stopped(void *opaque,
             }
             snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
                      SIGTRAP, type,
-                     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;
@@ -1302,15 +1303,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;
 
@@ -1474,7 +1475,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;
@@ -1577,7 +1577,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;
 }
@@ -305,8 +310,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:",
@@ -329,7 +334,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"
@@ -7116,6 +7117,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 */
Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -174,3 +174,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/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1178,6 +1178,7 @@ static void breakpoint_invalidate(CPUSta
 int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
                           int type)
 {
+    CPUState *env_iter;
     int i;
 
     /* sanity checks: allow power-of-2 lengths, deny unaligned breakpoints */
@@ -1192,10 +1193,12 @@ int cpu_watchpoint_insert(CPUState *env,
     if (env->nb_watchpoints >= MAX_WATCHPOINTS)
         return -ENOBUFS;
 
-    i = env->nb_watchpoints++;
-    env->watchpoint[i].vaddr = addr;
-    env->watchpoint[i].len = len;
-    env->watchpoint[i].type = type;
+    foreach_cpu(env_iter) {
+        i = env_iter->nb_watchpoints++;
+        env_iter->watchpoint[i].vaddr = addr;
+        env_iter->watchpoint[i].len = len;
+        env_iter->watchpoint[i].type = type;
+    }
     tlb_flush_page(env, addr);
     /* FIXME: This flush is needed because of the hack to make memory ops
        terminate the TB.  It can be removed once the proper IO trap and
@@ -1208,13 +1211,17 @@ int cpu_watchpoint_insert(CPUState *env,
 int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
                           int type)
 {
+    CPUState *env_iter;
     int i;
 
     for (i = 0; i < env->nb_watchpoints; i++) {
         if (addr == env->watchpoint[i].vaddr &&
             len == env->watchpoint[i].len && type == env->watchpoint[i].type) {
-            env->nb_watchpoints--;
-            env->watchpoint[i] = env->watchpoint[env->nb_watchpoints];
+            foreach_cpu(env_iter) {
+                env_iter->nb_watchpoints--;
+                env_iter->watchpoint[i] =
+                    env_iter->watchpoint[env->nb_watchpoints];
+            }
             tlb_flush_page(env, addr);
             return 0;
         }
@@ -1223,13 +1230,17 @@ int cpu_watchpoint_remove(CPUState *env,
 }
 
 /* Remove all watchpoints. */
-void cpu_watchpoint_remove_all(CPUState *env) {
+void cpu_watchpoint_remove_all(CPUState *env)
+{
+    CPUState *env_iter;
     int i;
 
     for (i = 0; i < env->nb_watchpoints; i++) {
         tlb_flush_page(env, env->watchpoint[i].vaddr);
     }
-    env->nb_watchpoints = 0;
+    foreach_cpu(env_iter) {
+        env_iter->nb_watchpoints = 0;
+    }
 }
 
 /* add a breakpoint. EXCP_DEBUG is returned by the CPU loop if a
@@ -1238,6 +1249,7 @@ int cpu_breakpoint_insert(CPUState *env,
                           int type)
 {
 #if defined(TARGET_HAS_ICE)
+    CPUState *env_iter;
     int i;
 
     for(i = 0; i < env->nb_breakpoints; i++) {
@@ -1247,8 +1259,9 @@ int cpu_breakpoint_insert(CPUState *env,
 
     if (env->nb_breakpoints >= MAX_BREAKPOINTS)
         return -ENOBUFS;
-    env->breakpoints[env->nb_breakpoints++] = pc;
-
+    foreach_cpu(env_iter) {
+        env_iter->breakpoints[env_iter->nb_breakpoints++] = pc;
+    }
     breakpoint_invalidate(env, pc);
     return 0;
 #else
@@ -1257,13 +1270,18 @@ int cpu_breakpoint_insert(CPUState *env,
 }
 
 /* remove all breakpoints */
-void cpu_breakpoint_remove_all(CPUState *env) {
+void cpu_breakpoint_remove_all(CPUState *env)
+{
 #if defined(TARGET_HAS_ICE)
+    CPUState *env_iter;
     int i;
+
     for(i = 0; i < env->nb_breakpoints; i++) {
         breakpoint_invalidate(env, env->breakpoints[i]);
     }
-    env->nb_breakpoints = 0;
+    foreach_cpu(env_iter) {
+        env->nb_breakpoints = 0;
+    }
 #endif
 }
 
@@ -1272,17 +1290,21 @@ int cpu_breakpoint_remove(CPUState *env,
                           int type)
 {
 #if defined(TARGET_HAS_ICE)
+    CPUState *env_iter;
     int i;
+
     for(i = 0; i < env->nb_breakpoints; i++) {
         if (env->breakpoints[i] == pc)
             goto found;
     }
     return -ENOENT;
- found:
-    env->nb_breakpoints--;
-    if (i < env->nb_breakpoints)
-      env->breakpoints[i] = env->breakpoints[env->nb_breakpoints];
 
+ found:
+    foreach_cpu(env_iter) {
+        env_iter->nb_breakpoints--;
+        env_iter->breakpoints[i] =
+            env_iter->breakpoints[env_iter->nb_breakpoints];
+    }
     breakpoint_invalidate(env, pc);
     return 0;
 #else