From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K2RU5-0006RU-Kh for qemu-devel@nongnu.org; Sat, 31 May 2008 09:51:01 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K2RU4-0006R3-Jz for qemu-devel@nongnu.org; Sat, 31 May 2008 09:51:01 -0400 Received: from [199.232.76.173] (port=55177 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K2RU4-0006Qx-DL for qemu-devel@nongnu.org; Sat, 31 May 2008 09:51:00 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]:34495) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1K2RU3-0006A7-Lt for qemu-devel@nongnu.org; Sat, 31 May 2008 09:51:00 -0400 Received: from smtp08.web.de (fmsmtp08.dlan.cinetic.de [172.20.5.216]) by fmmailgate03.web.de (Postfix) with ESMTP id D85F0DD8F3AF for ; Sat, 31 May 2008 15:50:58 +0200 (CEST) Received: from [88.65.47.172] (helo=[192.168.1.198]) by smtp08.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.109 #226) id 1K2RU2-0008Du-00 for qemu-devel@nongnu.org; Sat, 31 May 2008 15:50:58 +0200 Resent-To: qemu-devel@nongnu.org Resent-Message-Id: <484157C2.30803@web.de> Message-ID: <48415646.5070404@web.de> Date: Sat, 31 May 2008 15:44:38 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <48414AC8.7080206@web.de> In-Reply-To: <48414AC8.7080206@web.de> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Sender: jan.kiszka@web.de Subject: [Qemu-devel] [PATCH 5/5] Enhance SMP guest debugging Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org 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 --- 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