* [Qemu-devel] [PATCH 01/13] Return appropriate watch message to gdb
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 02/13] Refactor and enhance break/watchpoint API Jan Kiszka
` (11 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-fix-gdbstub-stop-reply.patch --]
[-- Type: text/plain, Size: 1444 bytes --]
Return the appropriate type prefix (r, a, none) when reporting
watchpoint hits to the gdb front-end.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1495,6 +1495,7 @@ static void gdb_vm_stopped(void *opaque,
{
GDBState *s = opaque;
char buf[256];
+ const char *type;
int ret;
if (s->state == RS_SYSCALL)
@@ -1505,8 +1506,20 @@ static void gdb_vm_stopped(void *opaque,
if (reason == EXCP_DEBUG) {
if (s->env->watchpoint_hit) {
- snprintf(buf, sizeof(buf), "T%02xwatch:" TARGET_FMT_lx ";",
- SIGTRAP,
+ switch (s->env->watchpoint[s->env->watchpoint_hit - 1].flags &
+ (PAGE_READ | PAGE_WRITE)) {
+ case PAGE_READ:
+ type = "r";
+ break;
+ case PAGE_READ | PAGE_WRITE:
+ type = "a";
+ break;
+ default:
+ type = "";
+ break;
+ }
+ snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
+ SIGTRAP, type,
s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
put_packet(s, buf);
s->env->watchpoint_hit = 0;
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 02/13] Refactor and enhance break/watchpoint API
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 01/13] Return appropriate watch message to gdb Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 17:24 ` Glauber Costa
2008-10-14 17:35 ` [Qemu-devel] " Glauber Costa
2008-10-14 9:12 ` [Qemu-devel] [PATCH 03/13] Set mem_io_vaddr on io_read Jan Kiszka
` (10 subsequent siblings)
12 siblings, 2 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-improve-breakpoint-watchpoint-interface.patch --]
[-- Type: text/plain, Size: 30691 bytes --]
This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow the
succeeding enhancements this series comes with.
First of all, it overcomes MAX_BREAKPOINTS/MAX_WATCHPOINTS by switching
to dynamically allocated data structures that are kept in linked lists.
This also allows to return a stable reference to the related objects,
required for later introduced x86 debug register support.
Breakpoints and watchpoints are stored with their full information set
and an additional flag field that makes them easily extensible for use
beyond pure guest debugging.
Finally, this restructuring lays the foundation for KVM to hook into
the debugging infrastructure, providing its own services where hardware
virtualization demands it. Once QEMUAccel is considered for merge,
those entry point should be included into its abstraction layer so that
accellerators can hook in even more cleanly.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-all.h | 23 +++--
cpu-defs.h | 26 +++---
exec.c | 201 ++++++++++++++++++++++++++++-------------------
gdbstub.c | 131 ++++++++++++++++--------------
target-alpha/translate.c | 7 -
target-arm/translate.c | 9 +-
target-cris/translate.c | 9 +-
target-i386/translate.c | 7 -
target-m68k/translate.c | 9 +-
target-mips/translate.c | 7 -
target-ppc/translate.c | 7 -
target-sh4/translate.c | 7 -
target-sparc/translate.c | 7 -
13 files changed, 264 insertions(+), 186 deletions(-)
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -761,12 +761,23 @@ extern int use_icount;
void cpu_interrupt(CPUState *s, int mask);
void cpu_reset_interrupt(CPUState *env, int mask);
-int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type);
-int cpu_watchpoint_remove(CPUState *env, target_ulong addr);
-void cpu_watchpoint_remove_all(CPUState *env);
-int cpu_breakpoint_insert(CPUState *env, target_ulong pc);
-int cpu_breakpoint_remove(CPUState *env, target_ulong pc);
-void cpu_breakpoint_remove_all(CPUState *env);
+/* Breakpoint/watchpoint flags */
+#define BP_MEM_READ 0x01
+#define BP_MEM_WRITE 0x02
+#define BP_MEM_ACCESS (BP_MEM_READ | BP_MEM_WRITE)
+#define BP_GDB 0x10
+
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
+ CPUBreakpoint **breakpoint);
+int cpu_breakpoint_remove(CPUState *env, target_ulong pc, int flags);
+void cpu_breakpoint_remove_by_ref(CPUState *env, CPUBreakpoint *breakpoint);
+void cpu_breakpoint_remove_all(CPUState *env, int mask);
+int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
+ int flags, CPUWatchpoint **watchpoint);
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr,
+ target_ulong len, int flags);
+void cpu_watchpoint_remove_by_ref(CPUState *env, CPUWatchpoint *watchpoint);
+void cpu_watchpoint_remove_all(CPUState *env, int mask);
#define SSTEP_ENABLE 0x1 /* Enable simulated HW single stepping */
#define SSTEP_NOIRQ 0x2 /* Do not use IRQ while single stepping */
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -537,7 +537,6 @@ void cpu_exec_init(CPUState *env)
cpu_index++;
}
env->cpu_index = cpu_index;
- env->nb_watchpoints = 0;
*penv = env;
#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
@@ -1311,107 +1310,150 @@ static void breakpoint_invalidate(CPUSta
#endif
/* Add a watchpoint. */
-int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
+int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
+ int flags, CPUWatchpoint **watchpoint)
{
- int i;
+ CPUWatchpoint *wp;
- for (i = 0; i < env->nb_watchpoints; i++) {
- if (addr == env->watchpoint[i].vaddr)
- return 0;
- }
- if (env->nb_watchpoints >= MAX_WATCHPOINTS)
- return -1;
+ wp = qemu_malloc(sizeof(*wp));
+ if (!wp)
+ return -ENOBUFS;
+
+ wp->vaddr = addr;
+ wp->len = len;
+ wp->flags = flags;
+
+ wp->next = env->watchpoints;
+ wp->prev = NULL;
+ if (wp->next)
+ wp->next->prev = wp;
+ env->watchpoints = wp;
- i = env->nb_watchpoints++;
- env->watchpoint[i].vaddr = addr;
- env->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
re-execute bits are in. */
tb_flush(env);
- return i;
-}
-/* Remove a watchpoint. */
-int cpu_watchpoint_remove(CPUState *env, target_ulong addr)
-{
- int i;
+ if (watchpoint)
+ *watchpoint = wp;
+ return 0;
+}
- for (i = 0; i < env->nb_watchpoints; i++) {
- if (addr == env->watchpoint[i].vaddr) {
- env->nb_watchpoints--;
- env->watchpoint[i] = env->watchpoint[env->nb_watchpoints];
- tlb_flush_page(env, addr);
+/* Remove a specific watchpoint. */
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
+ int flags)
+{
+ CPUWatchpoint *wp;
+
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+ if (addr == wp->vaddr && len == wp->len && flags == wp->flags) {
+ cpu_watchpoint_remove_by_ref(env, wp);
return 0;
}
}
- return -1;
+ return -ENOENT;
}
-/* Remove all watchpoints. */
-void cpu_watchpoint_remove_all(CPUState *env) {
- int i;
+/* Remove a specific watchpoint by reference. */
+void cpu_watchpoint_remove_by_ref(CPUState *env, CPUWatchpoint *watchpoint)
+{
+ if (watchpoint->next)
+ watchpoint->next->prev = watchpoint->prev;
+ if (watchpoint->prev)
+ watchpoint->prev->next = watchpoint->next;
+ else
+ env->watchpoints = watchpoint->next;
- for (i = 0; i < env->nb_watchpoints; i++) {
- tlb_flush_page(env, env->watchpoint[i].vaddr);
- }
- env->nb_watchpoints = 0;
+ tlb_flush_page(env, watchpoint->vaddr);
+
+ qemu_free(watchpoint);
}
-/* add a breakpoint. EXCP_DEBUG is returned by the CPU loop if a
- breakpoint is reached */
-int cpu_breakpoint_insert(CPUState *env, target_ulong pc)
+/* Remove all matching watchpoints. */
+void cpu_watchpoint_remove_all(CPUState *env, int mask)
{
-#if defined(TARGET_HAS_ICE)
- int i;
+ CPUWatchpoint *wp;
- for(i = 0; i < env->nb_breakpoints; i++) {
- if (env->breakpoints[i] == pc)
- return 0;
- }
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next)
+ if (wp->flags & mask)
+ cpu_watchpoint_remove_by_ref(env, wp);
+}
- if (env->nb_breakpoints >= MAX_BREAKPOINTS)
- return -1;
- env->breakpoints[env->nb_breakpoints++] = pc;
+/* Add a breakpoint. */
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
+ CPUBreakpoint **breakpoint)
+{
+#if defined(TARGET_HAS_ICE)
+ CPUBreakpoint *bp;
+
+ bp = qemu_malloc(sizeof(*bp));
+ if (!bp)
+ return -ENOBUFS;
+
+ bp->pc = pc;
+ bp->flags = flags;
+
+ bp->next = env->breakpoints;
+ bp->prev = NULL;
+ if (bp->next)
+ bp->next->prev = bp;
+ env->breakpoints = bp;
breakpoint_invalidate(env, pc);
+
+ if (breakpoint)
+ *breakpoint = bp;
return 0;
#else
- return -1;
+ return -ENOSYS;
#endif
}
-/* remove all breakpoints */
-void cpu_breakpoint_remove_all(CPUState *env) {
+/* Remove a specific breakpoint. */
+int cpu_breakpoint_remove(CPUState *env, target_ulong pc, int flags)
+{
#if defined(TARGET_HAS_ICE)
- int i;
- for(i = 0; i < env->nb_breakpoints; i++) {
- breakpoint_invalidate(env, env->breakpoints[i]);
+ CPUBreakpoint *bp;
+
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == pc && bp->flags == flags) {
+ cpu_breakpoint_remove_by_ref(env, bp);
+ return 0;
+ }
}
- env->nb_breakpoints = 0;
+ return -ENOENT;
+#else
+ return -ENOSYS;
#endif
}
-/* remove a breakpoint */
-int cpu_breakpoint_remove(CPUState *env, target_ulong pc)
+/* Remove a specific breakpoint by reference. */
+void cpu_breakpoint_remove_by_ref(CPUState *env, CPUBreakpoint *breakpoint)
{
#if defined(TARGET_HAS_ICE)
- int i;
- for(i = 0; i < env->nb_breakpoints; i++) {
- if (env->breakpoints[i] == pc)
- goto found;
- }
- return -1;
- found:
- env->nb_breakpoints--;
- if (i < env->nb_breakpoints)
- env->breakpoints[i] = env->breakpoints[env->nb_breakpoints];
+ if (breakpoint->next)
+ breakpoint->next->prev = breakpoint->prev;
+ if (breakpoint->prev)
+ breakpoint->prev->next = breakpoint->next;
+ else
+ env->breakpoints = breakpoint->next;
- breakpoint_invalidate(env, pc);
- return 0;
-#else
- return -1;
+ breakpoint_invalidate(env, breakpoint->pc);
+
+ qemu_free(breakpoint);
+#endif
+}
+
+/* Remove all matching breakpoints. */
+void cpu_breakpoint_remove_all(CPUState *env, int mask)
+{
+#if defined(TARGET_HAS_ICE)
+ CPUBreakpoint *bp;
+
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next)
+ if (bp->flags & mask)
+ cpu_breakpoint_remove_by_ref(env, bp);
#endif
}
@@ -1893,7 +1935,7 @@ int tlb_set_page_exec(CPUState *env, tar
target_phys_addr_t addend;
int ret;
CPUTLBEntry *te;
- int i;
+ CPUWatchpoint *wp;
target_phys_addr_t iotlb;
p = phys_page_find(paddr >> TARGET_PAGE_BITS);
@@ -1934,8 +1976,8 @@ int tlb_set_page_exec(CPUState *env, tar
code_address = address;
/* Make accesses to pages with watchpoints go via the
watchpoint trap routines. */
- for (i = 0; i < env->nb_watchpoints; i++) {
- if (vaddr == (env->watchpoint[i].vaddr & TARGET_PAGE_MASK)) {
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+ if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
iotlb = io_mem_watch + paddr;
/* TODO: The memory case can be optimized by not trapping
reads of pages with a write breakpoint. */
@@ -2464,13 +2506,12 @@ static void check_watchpoint(int offset,
{
CPUState *env = cpu_single_env;
target_ulong vaddr;
- int i;
+ CPUWatchpoint *wp;
vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
- for (i = 0; i < env->nb_watchpoints; i++) {
- if (vaddr == env->watchpoint[i].vaddr
- && (env->watchpoint[i].type & flags)) {
- env->watchpoint_hit = i + 1;
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
+ if (vaddr == wp->vaddr && (wp->flags & flags)) {
+ env->watchpoint_hit = wp;
cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
break;
}
@@ -2482,40 +2523,40 @@ static void check_watchpoint(int offset,
phys routines. */
static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
return ldub_phys(addr);
}
static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
return lduw_phys(addr);
}
static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
return ldl_phys(addr);
}
static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
stb_phys(addr, val);
}
static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
stw_phys(addr, val);
}
static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
stl_phys(addr, val);
}
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1147,10 +1147,64 @@ void gdb_register_coprocessor(CPUState *
}
}
+/* GDB breakpoint/watchpoint types */
+#define GDB_BREAKPOINT_SW 0
+#define GDB_BREAKPOINT_HW 1
+#define GDB_WATCHPOINT_WRITE 2
+#define GDB_WATCHPOINT_READ 3
+#define GDB_WATCHPOINT_ACCESS 4
+
+#ifndef CONFIG_USER_ONLY
+static const int xlat_gdb_type[] = {
+ [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,
+ [GDB_WATCHPOINT_READ] = BP_GDB | BP_MEM_READ,
+ [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
+};
+#endif
+
+static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
+ target_ulong len, int type)
+{
+ switch (type) {
+ case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
+ return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+#ifndef CONFIG_USER_ONLY
+ case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
+ return cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
+ NULL);
+#endif
+ default:
+ return -ENOSYS;
+ }
+}
+
+static int gdb_breakpoint_remove(CPUState *env, target_ulong addr,
+ target_ulong len, int type)
+{
+ switch (type) {
+ case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
+ return cpu_breakpoint_remove(env, addr, BP_GDB);
+#ifndef CONFIG_USER_ONLY
+ case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
+ return cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+#endif
+ default:
+ return -ENOSYS;
+ }
+}
+
+static void gdb_breakpoint_remove_all(CPUState *env)
+{
+ cpu_breakpoint_remove_all(env, BP_GDB);
+#ifndef CONFIG_USER_ONLY
+ cpu_watchpoint_remove_all(env, BP_GDB);
+#endif
+}
+
static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
{
const char *p;
- int ch, reg_size, type;
+ int ch, reg_size, type, res;
char buf[MAX_PACKET_LENGTH];
uint8_t mem_buf[MAX_PACKET_LENGTH];
uint8_t *registers;
@@ -1170,8 +1224,7 @@ static int gdb_handle_packet(GDBState *s
* because gdb is doing and initial connect and the state
* should be cleaned up.
*/
- cpu_breakpoint_remove_all(env);
- cpu_watchpoint_remove_all(env);
+ gdb_breakpoint_remove_all(env);
break;
case 'c':
if (*p != '\0') {
@@ -1205,8 +1258,7 @@ static int gdb_handle_packet(GDBState *s
exit(0);
case 'D':
/* Detach packet */
- cpu_breakpoint_remove_all(env);
- cpu_watchpoint_remove_all(env);
+ gdb_breakpoint_remove_all(env);
gdb_continue(s);
put_packet(s, "OK");
break;
@@ -1329,44 +1381,6 @@ static int gdb_handle_packet(GDBState *s
put_packet(s, "OK");
break;
case 'Z':
- type = strtoul(p, (char **)&p, 16);
- if (*p == ',')
- p++;
- addr = strtoull(p, (char **)&p, 16);
- if (*p == ',')
- p++;
- len = strtoull(p, (char **)&p, 16);
- switch (type) {
- case 0:
- case 1:
- if (cpu_breakpoint_insert(env, addr) < 0)
- goto breakpoint_error;
- put_packet(s, "OK");
- break;
-#ifndef CONFIG_USER_ONLY
- case 2:
- type = PAGE_WRITE;
- goto insert_watchpoint;
- case 3:
- type = PAGE_READ;
- goto insert_watchpoint;
- case 4:
- type = PAGE_READ | PAGE_WRITE;
- insert_watchpoint:
- if (cpu_watchpoint_insert(env, addr, type) < 0)
- goto breakpoint_error;
- put_packet(s, "OK");
- break;
-#endif
- default:
- put_packet(s, "");
- break;
- }
- break;
- breakpoint_error:
- put_packet(s, "E22");
- break;
-
case 'z':
type = strtoul(p, (char **)&p, 16);
if (*p == ',')
@@ -1375,17 +1389,16 @@ static int gdb_handle_packet(GDBState *s
if (*p == ',')
p++;
len = strtoull(p, (char **)&p, 16);
- if (type == 0 || type == 1) {
- cpu_breakpoint_remove(env, addr);
- put_packet(s, "OK");
-#ifndef CONFIG_USER_ONLY
- } else if (type >= 2 || type <= 4) {
- cpu_watchpoint_remove(env, addr);
- put_packet(s, "OK");
-#endif
- } else {
+ if (ch == 'Z')
+ res = gdb_breakpoint_insert(env, addr, len, type);
+ else
+ res = gdb_breakpoint_remove(env, addr, len, type);
+ if (res >= 0)
+ put_packet(s, "OK");
+ else if (res == -ENOSYS)
put_packet(s, "");
- }
+ else
+ put_packet(s, "E22");
break;
case 'q':
case 'Q':
@@ -1506,12 +1519,11 @@ static void gdb_vm_stopped(void *opaque,
if (reason == EXCP_DEBUG) {
if (s->env->watchpoint_hit) {
- switch (s->env->watchpoint[s->env->watchpoint_hit - 1].flags &
- (PAGE_READ | PAGE_WRITE)) {
- case PAGE_READ:
+ switch (s->env->watchpoint_hit->flags & BP_MEM_ACCESS) {
+ case BP_MEM_READ:
type = "r";
break;
- case PAGE_READ | PAGE_WRITE:
+ case BP_MEM_ACCESS:
type = "a";
break;
default:
@@ -1519,10 +1531,9 @@ static void gdb_vm_stopped(void *opaque,
break;
}
snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
- SIGTRAP, type,
- s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
+ SIGTRAP, type, s->env->watchpoint_hit->vaddr);
put_packet(s, buf);
- s->env->watchpoint_hit = 0;
+ s->env->watchpoint_hit = NULL;
return;
}
tb_flush(s->env);
Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -82,8 +82,6 @@ typedef uint64_t target_phys_addr_t;
#define EXCP_HLT 0x10001 /* hlt instruction reached */
#define EXCP_DEBUG 0x10002 /* cpu stopped after a breakpoint or singlestep */
#define EXCP_HALTED 0x10003 /* cpu is halted (waiting for external event) */
-#define MAX_BREAKPOINTS 32
-#define MAX_WATCHPOINTS 32
#define TB_JMP_CACHE_BITS 12
#define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
@@ -142,6 +140,19 @@ typedef struct icount_decr_u16 {
} icount_decr_u16;
#endif
+typedef struct CPUBreakpoint {
+ target_ulong pc;
+ int flags; /* BP_* */
+ struct CPUBreakpoint *prev, *next;
+} CPUBreakpoint;
+
+typedef struct CPUWatchpoint {
+ target_ulong vaddr;
+ target_ulong len;
+ int flags; /* BP_* */
+ struct CPUWatchpoint *prev, *next;
+} CPUWatchpoint;
+
#define CPU_TEMP_BUF_NLONGS 128
#define CPU_COMMON \
struct TranslationBlock *current_tb; /* currently executing TB */ \
@@ -174,16 +185,11 @@ typedef struct icount_decr_u16 {
\
/* from this point: preserved by CPU reset */ \
/* ice debug support */ \
- target_ulong breakpoints[MAX_BREAKPOINTS]; \
- int nb_breakpoints; \
+ CPUBreakpoint *breakpoints; \
int singlestep_enabled; \
\
- struct { \
- target_ulong vaddr; \
- int type; /* PAGE_READ/PAGE_WRITE */ \
- } watchpoint[MAX_WATCHPOINTS]; \
- int nb_watchpoints; \
- int watchpoint_hit; \
+ CPUWatchpoint *watchpoints; \
+ CPUWatchpoint *watchpoint_hit; \
\
struct GDBRegisterState *gdb_regs; \
\
Index: b/target-i386/translate.c
===================================================================
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7533,6 +7533,7 @@ static inline void gen_intermediate_code
DisasContext dc1, *dc = &dc1;
target_ulong pc_ptr;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj, cflags;
uint64_t flags;
target_ulong pc_start;
@@ -7616,9 +7617,9 @@ static inline void gen_intermediate_code
gen_icount_start();
for(;;) {
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == pc_ptr) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == pc_ptr) {
gen_debug(dc, pc_ptr - dc->cs_base);
break;
}
Index: b/target-alpha/translate.c
===================================================================
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2252,6 +2252,7 @@ static always_inline void gen_intermedia
target_ulong pc_start;
uint32_t insn;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj = -1;
int ret;
int num_insns;
@@ -2274,9 +2275,9 @@ static always_inline void gen_intermedia
gen_icount_start();
for (ret = 0; ret == 0;) {
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == ctx.pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == ctx.pc) {
gen_excp(&ctx, EXCP_DEBUG, 0);
break;
}
Index: b/target-arm/translate.c
===================================================================
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8544,6 +8544,7 @@ static inline void gen_intermediate_code
int search_pc)
{
DisasContext dc1, *dc = &dc1;
+ CPUBreakpoint *bp;
uint16_t *gen_opc_end;
int j, lj;
target_ulong pc_start;
@@ -8620,9 +8621,9 @@ static inline void gen_intermediate_code
}
#endif
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == dc->pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == dc->pc) {
gen_set_condexec(dc);
gen_set_pc_im(dc->pc);
gen_exception(EXCP_DEBUG);
@@ -8675,7 +8676,7 @@ static inline void gen_intermediate_code
/* Terminate the TB on memory ops if watchpoints are present. */
/* FIXME: This should be replacd by the deterministic execution
* IRQ raising bits. */
- if (dc->is_mem && env->nb_watchpoints)
+ if (dc->is_mem && env->watchpoints)
break;
/* Translation stops when a conditional branch is enoutered.
Index: b/target-cris/translate.c
===================================================================
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3006,10 +3006,11 @@ cris_decoder(DisasContext *dc)
static void check_breakpoint(CPUState *env, DisasContext *dc)
{
- int j;
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == dc->pc) {
+ CPUBreakpoint *bp;
+
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == dc->pc) {
cris_evaluate_flags (dc);
tcg_gen_movi_tl(env_pc, dc->pc);
t_gen_raise_exception(EXCP_DEBUG);
Index: b/target-m68k/translate.c
===================================================================
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2916,6 +2916,7 @@ gen_intermediate_code_internal(CPUState
{
DisasContext dc1, *dc = &dc1;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj;
target_ulong pc_start;
int pc_offset;
@@ -2949,9 +2950,9 @@ gen_intermediate_code_internal(CPUState
do {
pc_offset = dc->pc - pc_start;
gen_throws_exception = NULL;
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == dc->pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == dc->pc) {
gen_exception(dc, dc->pc, EXCP_DEBUG);
dc->is_jmp = DISAS_JUMP;
break;
@@ -2981,7 +2982,7 @@ gen_intermediate_code_internal(CPUState
/* Terminate the TB on memory ops if watchpoints are present. */
/* FIXME: This should be replaced by the deterministic execution
* IRQ raising bits. */
- if (dc->is_mem && env->nb_watchpoints)
+ if (dc->is_mem && env->watchpoints)
break;
} while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
!env->singlestep_enabled &&
Index: b/target-mips/translate.c
===================================================================
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -8442,6 +8442,7 @@ gen_intermediate_code_internal (CPUState
DisasContext ctx;
target_ulong pc_start;
uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int j, lj = -1;
int num_insns;
int max_insns;
@@ -8481,9 +8482,9 @@ gen_intermediate_code_internal (CPUState
#endif
gen_icount_start();
while (ctx.bstate == BS_NONE) {
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == ctx.pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == ctx.pc) {
save_cpu_state(&ctx, 1);
ctx.bstate = BS_BRANCH;
tcg_gen_helper_0_i(do_raise_exception, EXCP_DEBUG);
Index: b/target-ppc/translate.c
===================================================================
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -6198,6 +6198,7 @@ static always_inline void gen_intermedia
target_ulong pc_start;
uint16_t *gen_opc_end;
int supervisor, little_endian;
+ CPUBreakpoint *bp;
int j, lj = -1;
int num_insns;
int max_insns;
@@ -6252,9 +6253,9 @@ static always_inline void gen_intermedia
gen_icount_start();
/* Set env in case of segfault during code fetch */
while (ctx.exception == POWERPC_EXCP_NONE && gen_opc_ptr < gen_opc_end) {
- if (unlikely(env->nb_breakpoints > 0)) {
- for (j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == ctx.nip) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == ctx.nip) {
gen_update_nip(&ctx, ctx.nip);
gen_op_debug();
break;
Index: b/target-sh4/translate.c
===================================================================
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -1799,6 +1799,7 @@ gen_intermediate_code_internal(CPUState
DisasContext ctx;
target_ulong pc_start;
static uint16_t *gen_opc_end;
+ CPUBreakpoint *bp;
int i, ii;
int num_insns;
int max_insns;
@@ -1832,9 +1833,9 @@ gen_intermediate_code_internal(CPUState
max_insns = CF_COUNT_MASK;
gen_icount_start();
while (ctx.bstate == BS_NONE && gen_opc_ptr < gen_opc_end) {
- if (env->nb_breakpoints > 0) {
- for (i = 0; i < env->nb_breakpoints; i++) {
- if (ctx.pc == env->breakpoints[i]) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (ctx.pc == bp->pc) {
/* We have hit a breakpoint - make sure PC is up-to-date */
tcg_gen_movi_i32(cpu_pc, ctx.pc);
tcg_gen_helper_0_0(helper_debug);
Index: b/target-sparc/translate.c
===================================================================
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -4775,6 +4775,7 @@ static inline void gen_intermediate_code
target_ulong pc_start, last_pc;
uint16_t *gen_opc_end;
DisasContext dc1, *dc = &dc1;
+ CPUBreakpoint *bp;
int j, lj = -1;
int num_insns;
int max_insns;
@@ -4812,9 +4813,9 @@ static inline void gen_intermediate_code
max_insns = CF_COUNT_MASK;
gen_icount_start();
do {
- if (env->nb_breakpoints > 0) {
- for(j = 0; j < env->nb_breakpoints; j++) {
- if (env->breakpoints[j] == dc->pc) {
+ if (unlikely(env->breakpoints)) {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
+ if (bp->pc == dc->pc) {
if (dc->pc != pc_start)
save_state(dc, cpu_cond);
tcg_gen_helper_0_0(helper_debug);
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH 02/13] Refactor and enhance break/watchpoint API
2008-10-14 9:12 ` [Qemu-devel] [PATCH 02/13] Refactor and enhance break/watchpoint API Jan Kiszka
@ 2008-10-14 17:24 ` Glauber Costa
2008-10-14 17:45 ` [Qemu-devel] " Jan Kiszka
2008-10-14 17:35 ` [Qemu-devel] " Glauber Costa
1 sibling, 1 reply; 23+ messages in thread
From: Glauber Costa @ 2008-10-14 17:24 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
>
> Index: b/exec.c
> ===================================================================
> --- a/exec.c
> +++ b/exec.c
> @@ -537,7 +537,6 @@ void cpu_exec_init(CPUState *env)
> cpu_index++;
> }
> env->cpu_index = cpu_index;
> - env->nb_watchpoints = 0;
> *penv = env;
> #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
> register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
> @@ -1311,107 +1310,150 @@ static void breakpoint_invalidate(CPUSta
> #endif
>
> /* Add a watchpoint. */
> -int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
> +int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
> + int flags, CPUWatchpoint **watchpoint)
> {
> - int i;
> + CPUWatchpoint *wp;
>
> - for (i = 0; i < env->nb_watchpoints; i++) {
> - if (addr == env->watchpoint[i].vaddr)
> - return 0;
> - }
> - if (env->nb_watchpoints >= MAX_WATCHPOINTS)
> - return -1;
> + wp = qemu_malloc(sizeof(*wp));
> + if (!wp)
> + return -ENOBUFS;
> +
> + wp->vaddr = addr;
> + wp->len = len;
> + wp->flags = flags;
> +
> + wp->next = env->watchpoints;
> + wp->prev = NULL;
> + if (wp->next)
> + wp->next->prev = wp;
> + env->watchpoints = wp;
>
> - i = env->nb_watchpoints++;
> - env->watchpoint[i].vaddr = addr;
> - env->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
> re-execute bits are in. */
> tb_flush(env);
> Index: b/cpu-defs.h
> +typedef struct CPUBreakpoint {
> + target_ulong pc;
> + int flags; /* BP_* */
> + struct CPUBreakpoint *prev, *next;
> +} CPUBreakpoint;
> +
> +typedef struct CPUWatchpoint {
> + target_ulong vaddr;
> + target_ulong len;
> + int flags; /* BP_* */
> + struct CPUWatchpoint *prev, *next;
> +} CPUWatchpoint;
> +
Most of the time, you are transversing the list in a single direction.
So any particular reason to use a double linked list?
By the way, /me thinks it is about time for us to have a generic
linked list implementation
--
Glauber Costa.
"Free as in Freedom"
http://glommer.net
"The less confident you are, the more serious you have to act."
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] Re: [PATCH 02/13] Refactor and enhance break/watchpoint API
2008-10-14 17:24 ` Glauber Costa
@ 2008-10-14 17:45 ` Jan Kiszka
2008-10-14 17:51 ` Glauber Costa
0 siblings, 1 reply; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 17:45 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2694 bytes --]
Glauber Costa wrote:
>> Index: b/exec.c
>> ===================================================================
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -537,7 +537,6 @@ void cpu_exec_init(CPUState *env)
>> cpu_index++;
>> }
>> env->cpu_index = cpu_index;
>> - env->nb_watchpoints = 0;
>> *penv = env;
>> #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
>> register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
>> @@ -1311,107 +1310,150 @@ static void breakpoint_invalidate(CPUSta
>> #endif
>>
>> /* Add a watchpoint. */
>> -int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
>> +int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
>> + int flags, CPUWatchpoint **watchpoint)
>> {
>> - int i;
>> + CPUWatchpoint *wp;
>>
>> - for (i = 0; i < env->nb_watchpoints; i++) {
>> - if (addr == env->watchpoint[i].vaddr)
>> - return 0;
>> - }
>> - if (env->nb_watchpoints >= MAX_WATCHPOINTS)
>> - return -1;
>> + wp = qemu_malloc(sizeof(*wp));
>> + if (!wp)
>> + return -ENOBUFS;
>> +
>> + wp->vaddr = addr;
>> + wp->len = len;
>> + wp->flags = flags;
>> +
>> + wp->next = env->watchpoints;
>> + wp->prev = NULL;
>> + if (wp->next)
>> + wp->next->prev = wp;
>> + env->watchpoints = wp;
>>
>> - i = env->nb_watchpoints++;
>> - env->watchpoint[i].vaddr = addr;
>> - env->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
>> re-execute bits are in. */
>> tb_flush(env);
>
>> Index: b/cpu-defs.h
>> +typedef struct CPUBreakpoint {
>> + target_ulong pc;
>> + int flags; /* BP_* */
>> + struct CPUBreakpoint *prev, *next;
>> +} CPUBreakpoint;
>> +
>> +typedef struct CPUWatchpoint {
>> + target_ulong vaddr;
>> + target_ulong len;
>> + int flags; /* BP_* */
>> + struct CPUWatchpoint *prev, *next;
>> +} CPUWatchpoint;
>> +
>
> Most of the time, you are transversing the list in a single direction.
> So any particular reason to use a double linked list?
When looking as this patch only, one may get along with a singly-linked
list. But patch 13 adds a use case where the back-reference pays off.
> By the way, /me thinks it is about time for us to have a generic
> linked list implementation
Probably - but $SOMEONE will have to do the time-consuming conversion
work to make QEMU really benefit from this...
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] Re: [PATCH 02/13] Refactor and enhance break/watchpoint API
2008-10-14 17:45 ` [Qemu-devel] " Jan Kiszka
@ 2008-10-14 17:51 ` Glauber Costa
0 siblings, 0 replies; 23+ messages in thread
From: Glauber Costa @ 2008-10-14 17:51 UTC (permalink / raw)
To: qemu-devel
On Tue, Oct 14, 2008 at 3:45 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Glauber Costa wrote:
>>> Index: b/exec.c
>>> ===================================================================
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -537,7 +537,6 @@ void cpu_exec_init(CPUState *env)
>>> cpu_index++;
>>> }
>>> env->cpu_index = cpu_index;
>>> - env->nb_watchpoints = 0;
>>> *penv = env;
>>> #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
>>> register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
>>> @@ -1311,107 +1310,150 @@ static void breakpoint_invalidate(CPUSta
>>> #endif
>>>
>>> /* Add a watchpoint. */
>>> -int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
>>> +int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
>>> + int flags, CPUWatchpoint **watchpoint)
>>> {
>>> - int i;
>>> + CPUWatchpoint *wp;
>>>
>>> - for (i = 0; i < env->nb_watchpoints; i++) {
>>> - if (addr == env->watchpoint[i].vaddr)
>>> - return 0;
>>> - }
>>> - if (env->nb_watchpoints >= MAX_WATCHPOINTS)
>>> - return -1;
>>> + wp = qemu_malloc(sizeof(*wp));
>>> + if (!wp)
>>> + return -ENOBUFS;
>>> +
>>> + wp->vaddr = addr;
>>> + wp->len = len;
>>> + wp->flags = flags;
>>> +
>>> + wp->next = env->watchpoints;
>>> + wp->prev = NULL;
>>> + if (wp->next)
>>> + wp->next->prev = wp;
>>> + env->watchpoints = wp;
>>>
>>> - i = env->nb_watchpoints++;
>>> - env->watchpoint[i].vaddr = addr;
>>> - env->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
>>> re-execute bits are in. */
>>> tb_flush(env);
>>
>>> Index: b/cpu-defs.h
>>> +typedef struct CPUBreakpoint {
>>> + target_ulong pc;
>>> + int flags; /* BP_* */
>>> + struct CPUBreakpoint *prev, *next;
>>> +} CPUBreakpoint;
>>> +
>>> +typedef struct CPUWatchpoint {
>>> + target_ulong vaddr;
>>> + target_ulong len;
>>> + int flags; /* BP_* */
>>> + struct CPUWatchpoint *prev, *next;
>>> +} CPUWatchpoint;
>>> +
>>
>> Most of the time, you are transversing the list in a single direction.
>> So any particular reason to use a double linked list?
>
> When looking as this patch only, one may get along with a singly-linked
> list. But patch 13 adds a use case where the back-reference pays off.
fair.
>
>> By the way, /me thinks it is about time for us to have a generic
>> linked list implementation
>
> Probably - but $SOMEONE will have to do the time-consuming conversion
> work to make QEMU really benefit from this...
Actually we don't need a conversion. We just need an implementation,
and the conversion
happens through time, as old code gets replaced. But we still need the
$SOMEONE, and it's not
exactly my priority right now.
>
> Jan
>
>
--
Glauber Costa.
"Free as in Freedom"
http://glommer.net
"The less confident you are, the more serious you have to act."
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 02/13] Refactor and enhance break/watchpoint API
2008-10-14 9:12 ` [Qemu-devel] [PATCH 02/13] Refactor and enhance break/watchpoint API Jan Kiszka
2008-10-14 17:24 ` Glauber Costa
@ 2008-10-14 17:35 ` Glauber Costa
2008-10-14 17:53 ` [Qemu-devel] " Jan Kiszka
1 sibling, 1 reply; 23+ messages in thread
From: Glauber Costa @ 2008-10-14 17:35 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
> /* Add a watchpoint. */
> -int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
> +int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
> + int flags, CPUWatchpoint **watchpoint)
Any particular reason you're not simply returning the address of the watchpoint?
Although I must admit it's a matter of personal taste.
> {
> - int i;
> + CPUWatchpoint *wp;
>
> - for (i = 0; i < env->nb_watchpoints; i++) {
> - if (addr == env->watchpoint[i].vaddr)
> - return 0;
> - }
> - if (env->nb_watchpoints >= MAX_WATCHPOINTS)
> - return -1;
> + wp = qemu_malloc(sizeof(*wp));
> + if (!wp)
> + return -ENOBUFS;
> +
> + wp->vaddr = addr;
> + wp->len = len;
> + wp->flags = flags;
> +
> + wp->next = env->watchpoints;
> + wp->prev = NULL;
> + if (wp->next)
> + wp->next->prev = wp;
> + env->watchpoints = wp;
>
> - i = env->nb_watchpoints++;
> - env->watchpoint[i].vaddr = addr;
> - env->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
> re-execute bits are in. */
> tb_flush(env);
> - return i;
> -}
>
> -/* Remove a watchpoint. */
> -int cpu_watchpoint_remove(CPUState *env, target_ulong addr)
> -{
> - int i;
> + if (watchpoint)
> + *watchpoint = wp;
> + return 0;
> +}
>
> - for (i = 0; i < env->nb_watchpoints; i++) {
> - if (addr == env->watchpoint[i].vaddr) {
> - env->nb_watchpoints--;
> - env->watchpoint[i] = env->watchpoint[env->nb_watchpoints];
> - tlb_flush_page(env, addr);
> +/* Remove a specific watchpoint. */
> +int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
> + int flags)
> +{
> + CPUWatchpoint *wp;
> +
> + for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
> + if (addr == wp->vaddr && len == wp->len && flags == wp->flags) {
> + cpu_watchpoint_remove_by_ref(env, wp);
> return 0;
> }
> }
> - return -1;
> + return -ENOENT;
> }
>
> -/* Remove all watchpoints. */
> -void cpu_watchpoint_remove_all(CPUState *env) {
> - int i;
> +/* Remove a specific watchpoint by reference. */
> +void cpu_watchpoint_remove_by_ref(CPUState *env, CPUWatchpoint *watchpoint)
> +{
> + if (watchpoint->next)
> + watchpoint->next->prev = watchpoint->prev;
> + if (watchpoint->prev)
> + watchpoint->prev->next = watchpoint->next;
> + else
> + env->watchpoints = watchpoint->next;
>
> - for (i = 0; i < env->nb_watchpoints; i++) {
> - tlb_flush_page(env, env->watchpoint[i].vaddr);
> - }
> - env->nb_watchpoints = 0;
> + tlb_flush_page(env, watchpoint->vaddr);
> +
> + qemu_free(watchpoint);
> }
>
> -/* add a breakpoint. EXCP_DEBUG is returned by the CPU loop if a
> - breakpoint is reached */
> -int cpu_breakpoint_insert(CPUState *env, target_ulong pc)
> +/* Remove all matching watchpoints. */
> +void cpu_watchpoint_remove_all(CPUState *env, int mask)
> {
> -#if defined(TARGET_HAS_ICE)
> - int i;
> + CPUWatchpoint *wp;
>
> - for(i = 0; i < env->nb_breakpoints; i++) {
> - if (env->breakpoints[i] == pc)
> - return 0;
> - }
> + for (wp = env->watchpoints; wp != NULL; wp = wp->next)
> + if (wp->flags & mask)
> + cpu_watchpoint_remove_by_ref(env, wp);
> +}
>
> - if (env->nb_breakpoints >= MAX_BREAKPOINTS)
> - return -1;
> - env->breakpoints[env->nb_breakpoints++] = pc;
> +/* Add a breakpoint. */
> +int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
> + CPUBreakpoint **breakpoint)
> +{
> +#if defined(TARGET_HAS_ICE)
> + CPUBreakpoint *bp;
> +
> + bp = qemu_malloc(sizeof(*bp));
> + if (!bp)
> + return -ENOBUFS;
> +
> + bp->pc = pc;
> + bp->flags = flags;
> +
> + bp->next = env->breakpoints;
> + bp->prev = NULL;
> + if (bp->next)
> + bp->next->prev = bp;
> + env->breakpoints = bp;
>
> breakpoint_invalidate(env, pc);
> +
> + if (breakpoint)
> + *breakpoint = bp;
> return 0;
> #else
> - return -1;
> + return -ENOSYS;
> #endif
> }
>
> -/* remove all breakpoints */
> -void cpu_breakpoint_remove_all(CPUState *env) {
> +/* Remove a specific breakpoint. */
> +int cpu_breakpoint_remove(CPUState *env, target_ulong pc, int flags)
> +{
> #if defined(TARGET_HAS_ICE)
> - int i;
> - for(i = 0; i < env->nb_breakpoints; i++) {
> - breakpoint_invalidate(env, env->breakpoints[i]);
> + CPUBreakpoint *bp;
> +
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == pc && bp->flags == flags) {
> + cpu_breakpoint_remove_by_ref(env, bp);
> + return 0;
> + }
> }
> - env->nb_breakpoints = 0;
> + return -ENOENT;
> +#else
> + return -ENOSYS;
> #endif
> }
>
> -/* remove a breakpoint */
> -int cpu_breakpoint_remove(CPUState *env, target_ulong pc)
> +/* Remove a specific breakpoint by reference. */
> +void cpu_breakpoint_remove_by_ref(CPUState *env, CPUBreakpoint *breakpoint)
> {
> #if defined(TARGET_HAS_ICE)
> - int i;
> - for(i = 0; i < env->nb_breakpoints; i++) {
> - if (env->breakpoints[i] == pc)
> - goto found;
> - }
> - return -1;
> - found:
> - env->nb_breakpoints--;
> - if (i < env->nb_breakpoints)
> - env->breakpoints[i] = env->breakpoints[env->nb_breakpoints];
> + if (breakpoint->next)
> + breakpoint->next->prev = breakpoint->prev;
> + if (breakpoint->prev)
> + breakpoint->prev->next = breakpoint->next;
> + else
> + env->breakpoints = breakpoint->next;
>
> - breakpoint_invalidate(env, pc);
> - return 0;
> -#else
> - return -1;
> + breakpoint_invalidate(env, breakpoint->pc);
> +
> + qemu_free(breakpoint);
> +#endif
> +}
> +
> +/* Remove all matching breakpoints. */
> +void cpu_breakpoint_remove_all(CPUState *env, int mask)
> +{
> +#if defined(TARGET_HAS_ICE)
> + CPUBreakpoint *bp;
> +
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next)
> + if (bp->flags & mask)
> + cpu_breakpoint_remove_by_ref(env, bp);
> #endif
> }
>
> @@ -1893,7 +1935,7 @@ int tlb_set_page_exec(CPUState *env, tar
> target_phys_addr_t addend;
> int ret;
> CPUTLBEntry *te;
> - int i;
> + CPUWatchpoint *wp;
> target_phys_addr_t iotlb;
>
> p = phys_page_find(paddr >> TARGET_PAGE_BITS);
> @@ -1934,8 +1976,8 @@ int tlb_set_page_exec(CPUState *env, tar
> code_address = address;
> /* Make accesses to pages with watchpoints go via the
> watchpoint trap routines. */
> - for (i = 0; i < env->nb_watchpoints; i++) {
> - if (vaddr == (env->watchpoint[i].vaddr & TARGET_PAGE_MASK)) {
> + for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
> + if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
> iotlb = io_mem_watch + paddr;
> /* TODO: The memory case can be optimized by not trapping
> reads of pages with a write breakpoint. */
> @@ -2464,13 +2506,12 @@ static void check_watchpoint(int offset,
> {
> CPUState *env = cpu_single_env;
> target_ulong vaddr;
> - int i;
> + CPUWatchpoint *wp;
>
> vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
> - for (i = 0; i < env->nb_watchpoints; i++) {
> - if (vaddr == env->watchpoint[i].vaddr
> - && (env->watchpoint[i].type & flags)) {
> - env->watchpoint_hit = i + 1;
> + for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
> + if (vaddr == wp->vaddr && (wp->flags & flags)) {
> + env->watchpoint_hit = wp;
> cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
> break;
> }
> @@ -2482,40 +2523,40 @@ static void check_watchpoint(int offset,
> phys routines. */
> static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
> return ldub_phys(addr);
> }
>
> static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
> return lduw_phys(addr);
> }
>
> static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_READ);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
> return ldl_phys(addr);
> }
>
> static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
> stb_phys(addr, val);
> }
>
> static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
> stw_phys(addr, val);
> }
>
> static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, PAGE_WRITE);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
> stl_phys(addr, val);
> }
>
> Index: b/gdbstub.c
> ===================================================================
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1147,10 +1147,64 @@ void gdb_register_coprocessor(CPUState *
> }
> }
>
> +/* GDB breakpoint/watchpoint types */
> +#define GDB_BREAKPOINT_SW 0
> +#define GDB_BREAKPOINT_HW 1
> +#define GDB_WATCHPOINT_WRITE 2
> +#define GDB_WATCHPOINT_READ 3
> +#define GDB_WATCHPOINT_ACCESS 4
> +
> +#ifndef CONFIG_USER_ONLY
> +static const int xlat_gdb_type[] = {
> + [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,
> + [GDB_WATCHPOINT_READ] = BP_GDB | BP_MEM_READ,
> + [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
> +};
> +#endif
> +
> +static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
> + target_ulong len, int type)
> +{
> + switch (type) {
> + case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
> + return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
> +#ifndef CONFIG_USER_ONLY
> + case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
> + return cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
> + NULL);
> +#endif
> + default:
> + return -ENOSYS;
> + }
> +}
> +
> +static int gdb_breakpoint_remove(CPUState *env, target_ulong addr,
> + target_ulong len, int type)
> +{
> + switch (type) {
> + case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
> + return cpu_breakpoint_remove(env, addr, BP_GDB);
> +#ifndef CONFIG_USER_ONLY
> + case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
> + return cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
> +#endif
> + default:
> + return -ENOSYS;
> + }
> +}
> +
> +static void gdb_breakpoint_remove_all(CPUState *env)
> +{
> + cpu_breakpoint_remove_all(env, BP_GDB);
> +#ifndef CONFIG_USER_ONLY
> + cpu_watchpoint_remove_all(env, BP_GDB);
> +#endif
> +}
> +
> static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
> {
> const char *p;
> - int ch, reg_size, type;
> + int ch, reg_size, type, res;
> char buf[MAX_PACKET_LENGTH];
> uint8_t mem_buf[MAX_PACKET_LENGTH];
> uint8_t *registers;
> @@ -1170,8 +1224,7 @@ static int gdb_handle_packet(GDBState *s
> * because gdb is doing and initial connect and the state
> * should be cleaned up.
> */
> - cpu_breakpoint_remove_all(env);
> - cpu_watchpoint_remove_all(env);
> + gdb_breakpoint_remove_all(env);
> break;
> case 'c':
> if (*p != '\0') {
> @@ -1205,8 +1258,7 @@ static int gdb_handle_packet(GDBState *s
> exit(0);
> case 'D':
> /* Detach packet */
> - cpu_breakpoint_remove_all(env);
> - cpu_watchpoint_remove_all(env);
> + gdb_breakpoint_remove_all(env);
> gdb_continue(s);
> put_packet(s, "OK");
> break;
> @@ -1329,44 +1381,6 @@ static int gdb_handle_packet(GDBState *s
> put_packet(s, "OK");
> break;
> case 'Z':
> - type = strtoul(p, (char **)&p, 16);
> - if (*p == ',')
> - p++;
> - addr = strtoull(p, (char **)&p, 16);
> - if (*p == ',')
> - p++;
> - len = strtoull(p, (char **)&p, 16);
> - switch (type) {
> - case 0:
> - case 1:
> - if (cpu_breakpoint_insert(env, addr) < 0)
> - goto breakpoint_error;
> - put_packet(s, "OK");
> - break;
> -#ifndef CONFIG_USER_ONLY
> - case 2:
> - type = PAGE_WRITE;
> - goto insert_watchpoint;
> - case 3:
> - type = PAGE_READ;
> - goto insert_watchpoint;
> - case 4:
> - type = PAGE_READ | PAGE_WRITE;
> - insert_watchpoint:
> - if (cpu_watchpoint_insert(env, addr, type) < 0)
> - goto breakpoint_error;
> - put_packet(s, "OK");
> - break;
> -#endif
> - default:
> - put_packet(s, "");
> - break;
> - }
> - break;
> - breakpoint_error:
> - put_packet(s, "E22");
> - break;
> -
> case 'z':
> type = strtoul(p, (char **)&p, 16);
> if (*p == ',')
> @@ -1375,17 +1389,16 @@ static int gdb_handle_packet(GDBState *s
> if (*p == ',')
> p++;
> len = strtoull(p, (char **)&p, 16);
> - if (type == 0 || type == 1) {
> - cpu_breakpoint_remove(env, addr);
> - put_packet(s, "OK");
> -#ifndef CONFIG_USER_ONLY
> - } else if (type >= 2 || type <= 4) {
> - cpu_watchpoint_remove(env, addr);
> - put_packet(s, "OK");
> -#endif
> - } else {
> + if (ch == 'Z')
> + res = gdb_breakpoint_insert(env, addr, len, type);
> + else
> + res = gdb_breakpoint_remove(env, addr, len, type);
> + if (res >= 0)
> + put_packet(s, "OK");
> + else if (res == -ENOSYS)
> put_packet(s, "");
> - }
> + else
> + put_packet(s, "E22");
> break;
> case 'q':
> case 'Q':
> @@ -1506,12 +1519,11 @@ static void gdb_vm_stopped(void *opaque,
>
> if (reason == EXCP_DEBUG) {
> if (s->env->watchpoint_hit) {
> - switch (s->env->watchpoint[s->env->watchpoint_hit - 1].flags &
> - (PAGE_READ | PAGE_WRITE)) {
> - case PAGE_READ:
> + switch (s->env->watchpoint_hit->flags & BP_MEM_ACCESS) {
> + case BP_MEM_READ:
> type = "r";
> break;
> - case PAGE_READ | PAGE_WRITE:
> + case BP_MEM_ACCESS:
> type = "a";
> break;
> default:
> @@ -1519,10 +1531,9 @@ static void gdb_vm_stopped(void *opaque,
> break;
> }
> snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
> - SIGTRAP, type,
> - s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
> + SIGTRAP, type, s->env->watchpoint_hit->vaddr);
> put_packet(s, buf);
> - s->env->watchpoint_hit = 0;
> + s->env->watchpoint_hit = NULL;
> return;
> }
> tb_flush(s->env);
> Index: b/cpu-defs.h
> ===================================================================
> --- a/cpu-defs.h
> +++ b/cpu-defs.h
> @@ -82,8 +82,6 @@ typedef uint64_t target_phys_addr_t;
> #define EXCP_HLT 0x10001 /* hlt instruction reached */
> #define EXCP_DEBUG 0x10002 /* cpu stopped after a breakpoint or singlestep */
> #define EXCP_HALTED 0x10003 /* cpu is halted (waiting for external event) */
> -#define MAX_BREAKPOINTS 32
> -#define MAX_WATCHPOINTS 32
>
> #define TB_JMP_CACHE_BITS 12
> #define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
> @@ -142,6 +140,19 @@ typedef struct icount_decr_u16 {
> } icount_decr_u16;
> #endif
>
> +typedef struct CPUBreakpoint {
> + target_ulong pc;
> + int flags; /* BP_* */
> + struct CPUBreakpoint *prev, *next;
> +} CPUBreakpoint;
> +
> +typedef struct CPUWatchpoint {
> + target_ulong vaddr;
> + target_ulong len;
> + int flags; /* BP_* */
> + struct CPUWatchpoint *prev, *next;
> +} CPUWatchpoint;
> +
> #define CPU_TEMP_BUF_NLONGS 128
> #define CPU_COMMON \
> struct TranslationBlock *current_tb; /* currently executing TB */ \
> @@ -174,16 +185,11 @@ typedef struct icount_decr_u16 {
> \
> /* from this point: preserved by CPU reset */ \
> /* ice debug support */ \
> - target_ulong breakpoints[MAX_BREAKPOINTS]; \
> - int nb_breakpoints; \
> + CPUBreakpoint *breakpoints; \
> int singlestep_enabled; \
> \
> - struct { \
> - target_ulong vaddr; \
> - int type; /* PAGE_READ/PAGE_WRITE */ \
> - } watchpoint[MAX_WATCHPOINTS]; \
> - int nb_watchpoints; \
> - int watchpoint_hit; \
> + CPUWatchpoint *watchpoints; \
> + CPUWatchpoint *watchpoint_hit; \
> \
> struct GDBRegisterState *gdb_regs; \
> \
> Index: b/target-i386/translate.c
> ===================================================================
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7533,6 +7533,7 @@ static inline void gen_intermediate_code
> DisasContext dc1, *dc = &dc1;
> target_ulong pc_ptr;
> uint16_t *gen_opc_end;
> + CPUBreakpoint *bp;
> int j, lj, cflags;
> uint64_t flags;
> target_ulong pc_start;
> @@ -7616,9 +7617,9 @@ static inline void gen_intermediate_code
>
> gen_icount_start();
> for(;;) {
> - if (env->nb_breakpoints > 0) {
> - for(j = 0; j < env->nb_breakpoints; j++) {
> - if (env->breakpoints[j] == pc_ptr) {
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == pc_ptr) {
> gen_debug(dc, pc_ptr - dc->cs_base);
> break;
> }
> Index: b/target-alpha/translate.c
> ===================================================================
> --- a/target-alpha/translate.c
> +++ b/target-alpha/translate.c
> @@ -2252,6 +2252,7 @@ static always_inline void gen_intermedia
> target_ulong pc_start;
> uint32_t insn;
> uint16_t *gen_opc_end;
> + CPUBreakpoint *bp;
> int j, lj = -1;
> int ret;
> int num_insns;
> @@ -2274,9 +2275,9 @@ static always_inline void gen_intermedia
>
> gen_icount_start();
> for (ret = 0; ret == 0;) {
> - if (env->nb_breakpoints > 0) {
> - for(j = 0; j < env->nb_breakpoints; j++) {
> - if (env->breakpoints[j] == ctx.pc) {
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == ctx.pc) {
> gen_excp(&ctx, EXCP_DEBUG, 0);
> break;
> }
> Index: b/target-arm/translate.c
> ===================================================================
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -8544,6 +8544,7 @@ static inline void gen_intermediate_code
> int search_pc)
> {
> DisasContext dc1, *dc = &dc1;
> + CPUBreakpoint *bp;
> uint16_t *gen_opc_end;
> int j, lj;
> target_ulong pc_start;
> @@ -8620,9 +8621,9 @@ static inline void gen_intermediate_code
> }
> #endif
>
> - if (env->nb_breakpoints > 0) {
> - for(j = 0; j < env->nb_breakpoints; j++) {
> - if (env->breakpoints[j] == dc->pc) {
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == dc->pc) {
> gen_set_condexec(dc);
> gen_set_pc_im(dc->pc);
> gen_exception(EXCP_DEBUG);
> @@ -8675,7 +8676,7 @@ static inline void gen_intermediate_code
> /* Terminate the TB on memory ops if watchpoints are present. */
> /* FIXME: This should be replacd by the deterministic execution
> * IRQ raising bits. */
> - if (dc->is_mem && env->nb_watchpoints)
> + if (dc->is_mem && env->watchpoints)
> break;
>
> /* Translation stops when a conditional branch is enoutered.
> Index: b/target-cris/translate.c
> ===================================================================
> --- a/target-cris/translate.c
> +++ b/target-cris/translate.c
> @@ -3006,10 +3006,11 @@ cris_decoder(DisasContext *dc)
>
> static void check_breakpoint(CPUState *env, DisasContext *dc)
> {
> - int j;
> - if (env->nb_breakpoints > 0) {
> - for(j = 0; j < env->nb_breakpoints; j++) {
> - if (env->breakpoints[j] == dc->pc) {
> + CPUBreakpoint *bp;
> +
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == dc->pc) {
> cris_evaluate_flags (dc);
> tcg_gen_movi_tl(env_pc, dc->pc);
> t_gen_raise_exception(EXCP_DEBUG);
> Index: b/target-m68k/translate.c
> ===================================================================
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -2916,6 +2916,7 @@ gen_intermediate_code_internal(CPUState
> {
> DisasContext dc1, *dc = &dc1;
> uint16_t *gen_opc_end;
> + CPUBreakpoint *bp;
> int j, lj;
> target_ulong pc_start;
> int pc_offset;
> @@ -2949,9 +2950,9 @@ gen_intermediate_code_internal(CPUState
> do {
> pc_offset = dc->pc - pc_start;
> gen_throws_exception = NULL;
> - if (env->nb_breakpoints > 0) {
> - for(j = 0; j < env->nb_breakpoints; j++) {
> - if (env->breakpoints[j] == dc->pc) {
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == dc->pc) {
> gen_exception(dc, dc->pc, EXCP_DEBUG);
> dc->is_jmp = DISAS_JUMP;
> break;
> @@ -2981,7 +2982,7 @@ gen_intermediate_code_internal(CPUState
> /* Terminate the TB on memory ops if watchpoints are present. */
> /* FIXME: This should be replaced by the deterministic execution
> * IRQ raising bits. */
> - if (dc->is_mem && env->nb_watchpoints)
> + if (dc->is_mem && env->watchpoints)
> break;
> } while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
> !env->singlestep_enabled &&
> Index: b/target-mips/translate.c
> ===================================================================
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -8442,6 +8442,7 @@ gen_intermediate_code_internal (CPUState
> DisasContext ctx;
> target_ulong pc_start;
> uint16_t *gen_opc_end;
> + CPUBreakpoint *bp;
> int j, lj = -1;
> int num_insns;
> int max_insns;
> @@ -8481,9 +8482,9 @@ gen_intermediate_code_internal (CPUState
> #endif
> gen_icount_start();
> while (ctx.bstate == BS_NONE) {
> - if (env->nb_breakpoints > 0) {
> - for(j = 0; j < env->nb_breakpoints; j++) {
> - if (env->breakpoints[j] == ctx.pc) {
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == ctx.pc) {
> save_cpu_state(&ctx, 1);
> ctx.bstate = BS_BRANCH;
> tcg_gen_helper_0_i(do_raise_exception, EXCP_DEBUG);
> Index: b/target-ppc/translate.c
> ===================================================================
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -6198,6 +6198,7 @@ static always_inline void gen_intermedia
> target_ulong pc_start;
> uint16_t *gen_opc_end;
> int supervisor, little_endian;
> + CPUBreakpoint *bp;
> int j, lj = -1;
> int num_insns;
> int max_insns;
> @@ -6252,9 +6253,9 @@ static always_inline void gen_intermedia
> gen_icount_start();
> /* Set env in case of segfault during code fetch */
> while (ctx.exception == POWERPC_EXCP_NONE && gen_opc_ptr < gen_opc_end) {
> - if (unlikely(env->nb_breakpoints > 0)) {
> - for (j = 0; j < env->nb_breakpoints; j++) {
> - if (env->breakpoints[j] == ctx.nip) {
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == ctx.nip) {
> gen_update_nip(&ctx, ctx.nip);
> gen_op_debug();
> break;
> Index: b/target-sh4/translate.c
> ===================================================================
> --- a/target-sh4/translate.c
> +++ b/target-sh4/translate.c
> @@ -1799,6 +1799,7 @@ gen_intermediate_code_internal(CPUState
> DisasContext ctx;
> target_ulong pc_start;
> static uint16_t *gen_opc_end;
> + CPUBreakpoint *bp;
> int i, ii;
> int num_insns;
> int max_insns;
> @@ -1832,9 +1833,9 @@ gen_intermediate_code_internal(CPUState
> max_insns = CF_COUNT_MASK;
> gen_icount_start();
> while (ctx.bstate == BS_NONE && gen_opc_ptr < gen_opc_end) {
> - if (env->nb_breakpoints > 0) {
> - for (i = 0; i < env->nb_breakpoints; i++) {
> - if (ctx.pc == env->breakpoints[i]) {
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (ctx.pc == bp->pc) {
> /* We have hit a breakpoint - make sure PC is up-to-date */
> tcg_gen_movi_i32(cpu_pc, ctx.pc);
> tcg_gen_helper_0_0(helper_debug);
> Index: b/target-sparc/translate.c
> ===================================================================
> --- a/target-sparc/translate.c
> +++ b/target-sparc/translate.c
> @@ -4775,6 +4775,7 @@ static inline void gen_intermediate_code
> target_ulong pc_start, last_pc;
> uint16_t *gen_opc_end;
> DisasContext dc1, *dc = &dc1;
> + CPUBreakpoint *bp;
> int j, lj = -1;
> int num_insns;
> int max_insns;
> @@ -4812,9 +4813,9 @@ static inline void gen_intermediate_code
> max_insns = CF_COUNT_MASK;
> gen_icount_start();
> do {
> - if (env->nb_breakpoints > 0) {
> - for(j = 0; j < env->nb_breakpoints; j++) {
> - if (env->breakpoints[j] == dc->pc) {
> + if (unlikely(env->breakpoints)) {
> + for (bp = env->breakpoints; bp != NULL; bp = bp->next) {
> + if (bp->pc == dc->pc) {
> if (dc->pc != pc_start)
> save_state(dc, cpu_cond);
> tcg_gen_helper_0_0(helper_debug);
>
>
>
>
--
Glauber Costa.
"Free as in Freedom"
http://glommer.net
"The less confident you are, the more serious you have to act."
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] Re: [PATCH 02/13] Refactor and enhance break/watchpoint API
2008-10-14 17:35 ` [Qemu-devel] " Glauber Costa
@ 2008-10-14 17:53 ` Jan Kiszka
0 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 17:53 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 593 bytes --]
Glauber Costa wrote:
>> /* Add a watchpoint. */
>> -int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
>> +int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
>> + int flags, CPUWatchpoint **watchpoint)
>
> Any particular reason you're not simply returning the address of the watchpoint?
> Although I must admit it's a matter of personal taste.
Then I would have to express potential errors as pointer values. I think
I didn't find a precedence for this in QEMU, so I went for the above
interface.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 03/13] Set mem_io_vaddr on io_read
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 01/13] Return appropriate watch message to gdb Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 02/13] Refactor and enhance break/watchpoint API Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 17:39 ` Glauber Costa
2008-10-14 9:12 ` [Qemu-devel] [PATCH 04/13] Respect length of watchpoints Jan Kiszka
` (9 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-set-mem_io_vaddr-on-read.patch --]
[-- Type: text/plain, Size: 528 bytes --]
Required for read watchpoints.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
softmmu_template.h | 1 +
1 file changed, 1 insertion(+)
Index: b/softmmu_template.h
===================================================================
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -64,6 +64,7 @@ static inline DATA_TYPE glue(io_read, SU
cpu_io_recompile(env, retaddr);
}
+ env->mem_io_vaddr = addr;
#if SHIFT <= 2
res = io_mem_read[index][SHIFT](io_mem_opaque[index], physaddr);
#else
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH 03/13] Set mem_io_vaddr on io_read
2008-10-14 9:12 ` [Qemu-devel] [PATCH 03/13] Set mem_io_vaddr on io_read Jan Kiszka
@ 2008-10-14 17:39 ` Glauber Costa
2008-10-14 17:49 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 23+ messages in thread
From: Glauber Costa @ 2008-10-14 17:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
On Tue, Oct 14, 2008 at 7:12 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Required for read watchpoints.
can we have a slightly more informative description for the annotate commands ?
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> softmmu_template.h | 1 +
> 1 file changed, 1 insertion(+)
>
> Index: b/softmmu_template.h
> ===================================================================
> --- a/softmmu_template.h
> +++ b/softmmu_template.h
> @@ -64,6 +64,7 @@ static inline DATA_TYPE glue(io_read, SU
> cpu_io_recompile(env, retaddr);
> }
>
> + env->mem_io_vaddr = addr;
> #if SHIFT <= 2
> res = io_mem_read[index][SHIFT](io_mem_opaque[index], physaddr);
> #else
>
>
>
>
--
Glauber Costa.
"Free as in Freedom"
http://glommer.net
"The less confident you are, the more serious you have to act."
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] Re: [PATCH 03/13] Set mem_io_vaddr on io_read
2008-10-14 17:39 ` Glauber Costa
@ 2008-10-14 17:49 ` Jan Kiszka
0 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 17:49 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 964 bytes --]
Glauber Costa wrote:
> On Tue, Oct 14, 2008 at 7:12 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Required for read watchpoints.
> can we have a slightly more informative description for the annotate commands ?
"Analogously to write accesses, we need to save the memory address also
on read accesses in order to support read watchpoints."
Better?
Jan
>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> softmmu_template.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> Index: b/softmmu_template.h
>> ===================================================================
>> --- a/softmmu_template.h
>> +++ b/softmmu_template.h
>> @@ -64,6 +64,7 @@ static inline DATA_TYPE glue(io_read, SU
>> cpu_io_recompile(env, retaddr);
>> }
>>
>> + env->mem_io_vaddr = addr;
>> #if SHIFT <= 2
>> res = io_mem_read[index][SHIFT](io_mem_opaque[index], physaddr);
>> #else
>>
>>
>>
>>
>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 04/13] Respect length of watchpoints
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (2 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 03/13] Set mem_io_vaddr on io_read Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 17:50 ` Glauber Costa
2008-10-14 9:12 ` [Qemu-devel] [PATCH 05/13] Introduce next_cflags Jan Kiszka
` (8 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-watchpoint-len-support.patch --]
[-- Type: text/plain, Size: 4608 bytes --]
This adds length support for watchpoints. To keep things simple, only
aligned watchpoints are accepted.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-defs.h | 2 +-
exec.c | 30 ++++++++++++++++++++----------
2 files changed, 21 insertions(+), 11 deletions(-)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1313,14 +1313,21 @@ static void breakpoint_invalidate(CPUSta
int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
int flags, CPUWatchpoint **watchpoint)
{
+ target_ulong len_mask = ~(len - 1);
CPUWatchpoint *wp;
+ /* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */
+ if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr & ~len_mask)) {
+ fprintf(stderr, "qemu: tried to set invalid watchpoint at "
+ TARGET_FMT_lx ", len=" TARGET_FMT_lu "\n", addr, len);
+ return -EINVAL;
+ }
wp = qemu_malloc(sizeof(*wp));
if (!wp)
return -ENOBUFS;
wp->vaddr = addr;
- wp->len = len;
+ wp->len_mask = len_mask;
wp->flags = flags;
wp->next = env->watchpoints;
@@ -1344,10 +1351,12 @@ int cpu_watchpoint_insert(CPUState *env,
int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
int flags)
{
+ target_ulong len_mask = ~(len - 1);
CPUWatchpoint *wp;
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
- if (addr == wp->vaddr && len == wp->len && flags == wp->flags) {
+ if (addr == wp->vaddr && len_mask == wp->len_mask
+ && flags == wp->flags) {
cpu_watchpoint_remove_by_ref(env, wp);
return 0;
}
@@ -2502,7 +2511,7 @@ static CPUWriteMemoryFunc *notdirty_mem_
};
/* Generate a debug exception if a watchpoint has been hit. */
-static void check_watchpoint(int offset, int flags)
+static void check_watchpoint(int offset, int len_mask, int flags)
{
CPUState *env = cpu_single_env;
target_ulong vaddr;
@@ -2510,7 +2519,8 @@ static void check_watchpoint(int offset,
vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
- if (vaddr == wp->vaddr && (wp->flags & flags)) {
+ if ((vaddr == (wp->vaddr & len_mask) ||
+ (vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
env->watchpoint_hit = wp;
cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
break;
@@ -2523,40 +2533,40 @@ static void check_watchpoint(int offset,
phys routines. */
static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_READ);
return ldub_phys(addr);
}
static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_READ);
return lduw_phys(addr);
}
static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_READ);
return ldl_phys(addr);
}
static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_WRITE);
stb_phys(addr, val);
}
static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_WRITE);
stw_phys(addr, val);
}
static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
- check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
+ check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_WRITE);
stl_phys(addr, val);
}
Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -148,7 +148,7 @@ typedef struct CPUBreakpoint {
typedef struct CPUWatchpoint {
target_ulong vaddr;
- target_ulong len;
+ target_ulong len_mask;
int flags; /* BP_* */
struct CPUWatchpoint *prev, *next;
} CPUWatchpoint;
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH 04/13] Respect length of watchpoints
2008-10-14 9:12 ` [Qemu-devel] [PATCH 04/13] Respect length of watchpoints Jan Kiszka
@ 2008-10-14 17:50 ` Glauber Costa
2008-10-14 18:26 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 23+ messages in thread
From: Glauber Costa @ 2008-10-14 17:50 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
On Tue, Oct 14, 2008 at 7:12 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> This adds length support for watchpoints. To keep things simple, only
> aligned watchpoints are accepted.
why? It does not seem that much complicated to handle unaligned watchpoints.
Unless I'm totally wrong, we should just store the value as-is, and
then check for it.
As a matter of fact, because we're masking and testing for the mask,
it seems even more
complicated to require that. I agree a full aligned world would be a
happier world, but unfortunately,
unaligned accesses are quite common in x86.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> cpu-defs.h | 2 +-
> exec.c | 30 ++++++++++++++++++++----------
> 2 files changed, 21 insertions(+), 11 deletions(-)
>
> Index: b/exec.c
> ===================================================================
> --- a/exec.c
> +++ b/exec.c
> @@ -1313,14 +1313,21 @@ static void breakpoint_invalidate(CPUSta
> int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
> int flags, CPUWatchpoint **watchpoint)
> {
> + target_ulong len_mask = ~(len - 1);
> CPUWatchpoint *wp;
>
> + /* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */
> + if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr & ~len_mask)) {
> + fprintf(stderr, "qemu: tried to set invalid watchpoint at "
> + TARGET_FMT_lx ", len=" TARGET_FMT_lu "\n", addr, len);
> + return -EINVAL;
> + }
> wp = qemu_malloc(sizeof(*wp));
> if (!wp)
> return -ENOBUFS;
>
> wp->vaddr = addr;
> - wp->len = len;
> + wp->len_mask = len_mask;
> wp->flags = flags;
>
> wp->next = env->watchpoints;
> @@ -1344,10 +1351,12 @@ int cpu_watchpoint_insert(CPUState *env,
> int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
> int flags)
> {
> + target_ulong len_mask = ~(len - 1);
> CPUWatchpoint *wp;
>
> for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
> - if (addr == wp->vaddr && len == wp->len && flags == wp->flags) {
> + if (addr == wp->vaddr && len_mask == wp->len_mask
> + && flags == wp->flags) {
> cpu_watchpoint_remove_by_ref(env, wp);
> return 0;
> }
> @@ -2502,7 +2511,7 @@ static CPUWriteMemoryFunc *notdirty_mem_
> };
>
> /* Generate a debug exception if a watchpoint has been hit. */
> -static void check_watchpoint(int offset, int flags)
> +static void check_watchpoint(int offset, int len_mask, int flags)
> {
> CPUState *env = cpu_single_env;
> target_ulong vaddr;
> @@ -2510,7 +2519,8 @@ static void check_watchpoint(int offset,
>
> vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
> for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
> - if (vaddr == wp->vaddr && (wp->flags & flags)) {
> + if ((vaddr == (wp->vaddr & len_mask) ||
> + (vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
> env->watchpoint_hit = wp;
> cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
> break;
> @@ -2523,40 +2533,40 @@ static void check_watchpoint(int offset,
> phys routines. */
> static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_READ);
> return ldub_phys(addr);
> }
>
> static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_READ);
> return lduw_phys(addr);
> }
>
> static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_READ);
> return ldl_phys(addr);
> }
>
> static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_WRITE);
> stb_phys(addr, val);
> }
>
> static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_WRITE);
> stw_phys(addr, val);
> }
>
> static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_WRITE);
> stl_phys(addr, val);
> }
>
> Index: b/cpu-defs.h
> ===================================================================
> --- a/cpu-defs.h
> +++ b/cpu-defs.h
> @@ -148,7 +148,7 @@ typedef struct CPUBreakpoint {
>
> typedef struct CPUWatchpoint {
> target_ulong vaddr;
> - target_ulong len;
> + target_ulong len_mask;
> int flags; /* BP_* */
> struct CPUWatchpoint *prev, *next;
> } CPUWatchpoint;
It's less confusing if you call it len_mask from the beginning,
instead of changing your own patch for that purpose.
>
>
>
--
Glauber Costa.
"Free as in Freedom"
http://glommer.net
"The less confident you are, the more serious you have to act."
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] Re: [PATCH 04/13] Respect length of watchpoints
2008-10-14 17:50 ` Glauber Costa
@ 2008-10-14 18:26 ` Jan Kiszka
0 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 18:26 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6228 bytes --]
Glauber Costa wrote:
> On Tue, Oct 14, 2008 at 7:12 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> This adds length support for watchpoints. To keep things simple, only
>> aligned watchpoints are accepted.
>
> why? It does not seem that much complicated to handle unaligned watchpoints.
> Unless I'm totally wrong, we should just store the value as-is, and
> then check for it.
> As a matter of fact, because we're masking and testing for the mask,
> it seems even more
> complicated to require that. I agree a full aligned world would be a
> happier world, but unfortunately,
> unaligned accesses are quite common in x86.
Unaligned watchpoints also means multi-page watchpoints - and this
introduces some complexity. I think the fact that real x86 hw
watchpoints require alignment as well motivated the simplification. But
if there is a real need for it (e.g. some other arch using the
infrastructure for hw watchpoint emulation), I could rethink this.
However, I would prefer to apply such extension on top of the proposed
implementation.
>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> cpu-defs.h | 2 +-
>> exec.c | 30 ++++++++++++++++++++----------
>> 2 files changed, 21 insertions(+), 11 deletions(-)
>>
>> Index: b/exec.c
>> ===================================================================
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1313,14 +1313,21 @@ static void breakpoint_invalidate(CPUSta
>> int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
>> int flags, CPUWatchpoint **watchpoint)
>> {
>> + target_ulong len_mask = ~(len - 1);
>> CPUWatchpoint *wp;
>>
>> + /* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */
>> + if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr & ~len_mask)) {
>> + fprintf(stderr, "qemu: tried to set invalid watchpoint at "
>> + TARGET_FMT_lx ", len=" TARGET_FMT_lu "\n", addr, len);
>> + return -EINVAL;
>> + }
>> wp = qemu_malloc(sizeof(*wp));
>> if (!wp)
>> return -ENOBUFS;
>>
>> wp->vaddr = addr;
>> - wp->len = len;
>> + wp->len_mask = len_mask;
>> wp->flags = flags;
>>
>> wp->next = env->watchpoints;
>> @@ -1344,10 +1351,12 @@ int cpu_watchpoint_insert(CPUState *env,
>> int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
>> int flags)
>> {
>> + target_ulong len_mask = ~(len - 1);
>> CPUWatchpoint *wp;
>>
>> for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
>> - if (addr == wp->vaddr && len == wp->len && flags == wp->flags) {
>> + if (addr == wp->vaddr && len_mask == wp->len_mask
>> + && flags == wp->flags) {
>> cpu_watchpoint_remove_by_ref(env, wp);
>> return 0;
>> }
>> @@ -2502,7 +2511,7 @@ static CPUWriteMemoryFunc *notdirty_mem_
>> };
>>
>> /* Generate a debug exception if a watchpoint has been hit. */
>> -static void check_watchpoint(int offset, int flags)
>> +static void check_watchpoint(int offset, int len_mask, int flags)
>> {
>> CPUState *env = cpu_single_env;
>> target_ulong vaddr;
>> @@ -2510,7 +2519,8 @@ static void check_watchpoint(int offset,
>>
>> vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
>> for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
>> - if (vaddr == wp->vaddr && (wp->flags & flags)) {
>> + if ((vaddr == (wp->vaddr & len_mask) ||
>> + (vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
>> env->watchpoint_hit = wp;
>> cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
>> break;
>> @@ -2523,40 +2533,40 @@ static void check_watchpoint(int offset,
>> phys routines. */
>> static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_READ);
>> return ldub_phys(addr);
>> }
>>
>> static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_READ);
>> return lduw_phys(addr);
>> }
>>
>> static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_READ);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_READ);
>> return ldl_phys(addr);
>> }
>>
>> static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x0, BP_MEM_WRITE);
>> stb_phys(addr, val);
>> }
>>
>> static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x1, BP_MEM_WRITE);
>> stw_phys(addr, val);
>> }
>>
>> static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
>> uint32_t val)
>> {
>> - check_watchpoint(addr & ~TARGET_PAGE_MASK, BP_MEM_WRITE);
>> + check_watchpoint(addr & ~TARGET_PAGE_MASK, ~0x3, BP_MEM_WRITE);
>> stl_phys(addr, val);
>> }
>>
>> Index: b/cpu-defs.h
>> ===================================================================
>> --- a/cpu-defs.h
>> +++ b/cpu-defs.h
>> @@ -148,7 +148,7 @@ typedef struct CPUBreakpoint {
>>
>> typedef struct CPUWatchpoint {
>> target_ulong vaddr;
>> - target_ulong len;
>> + target_ulong len_mask;
>> int flags; /* BP_* */
>> struct CPUWatchpoint *prev, *next;
>> } CPUWatchpoint;
>
> It's less confusing if you call it len_mask from the beginning,
> instead of changing your own patch for that purpose.
OK. If I have to update the involved patches, I will merge this over.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 05/13] Introduce next_cflags
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (3 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 04/13] Respect length of watchpoints Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 06/13] Switch self-modified code recompilation to next_cflags Jan Kiszka
` (7 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-add-next_cflags.patch --]
[-- Type: text/plain, Size: 2823 bytes --]
Introduce next_cflags as part of CPUState. It controls the compile flags
of the next newly generated TB. After use, it will automatically be reset
to zero. This allows the caller to simply set and then forget about it,
e.g. to ensure that the next, and only the next TB will contain just a
single instruction. To avoid that next_cflags hits the wrong TB,
interrupt delivery is suppressed when this field is non-zero.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-defs.h | 4 ++++
cpu-exec.c | 9 +++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -183,6 +183,10 @@ typedef struct CPUWatchpoint {
} icount_decr; \
uint32_t can_do_io; /* nonzero if memory mapped IO is safe. */ \
\
+ /* Compile flags for generating next regular TB. \
+ Will be automatically zeroed after use. */ \
+ uint16_t next_cflags; \
+ \
/* from this point: preserved by CPU reset */ \
/* ice debug support */ \
CPUBreakpoint *breakpoints; \
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -150,7 +150,8 @@ static TranslationBlock *tb_find_slow(ta
}
not_found:
/* if no translated code available, then translate it now */
- tb = tb_gen_code(env, pc, cs_base, flags, 0);
+ tb = tb_gen_code(env, pc, cs_base, flags, env->next_cflags);
+ env->next_cflags = 0;
found:
/* we add the TB in the virtual pc hash table */
@@ -364,8 +365,12 @@ int cpu_exec(CPUState *env1)
next_tb = 0; /* force lookup of first TB */
for(;;) {
interrupt_request = env->interrupt_request;
+ /* Deliver interrupt, but only if we are not recompiling some
+ TB (non-zero next_cflags) and the current single-step mode
+ doesn't block IRQs. */
if (unlikely(interrupt_request) &&
- likely(!(env->singlestep_enabled & SSTEP_NOIRQ))) {
+ likely(env->next_cflags == 0 &&
+ !(env->singlestep_enabled & SSTEP_NOIRQ))) {
if (interrupt_request & CPU_INTERRUPT_DEBUG) {
env->interrupt_request &= ~CPU_INTERRUPT_DEBUG;
env->exception_index = EXCP_DEBUG;
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 06/13] Switch self-modified code recompilation to next_cflags
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (4 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 05/13] Introduce next_cflags Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 07/13] Restore pc on watchpoint hits Jan Kiszka
` (6 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-switch-selfmod-recompile-to-next_cflags.patch --]
[-- Type: text/plain, Size: 4147 bytes --]
Switching tb_invalidate_phys_page_range and tb_invalidate_phys_page over
to the new next_cflags scheme when self-modifying code was detected can
save a few lines of code and remove arch dependency.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
exec.c | 32 ++++----------------------------
1 file changed, 4 insertions(+), 28 deletions(-)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -885,12 +885,11 @@ TranslationBlock *tb_gen_code(CPUState *
void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t end,
int is_cpu_write_access)
{
- int n, current_tb_modified, current_tb_not_found, current_flags;
+ int n, current_tb_modified, current_tb_not_found;
CPUState *env = cpu_single_env;
PageDesc *p;
TranslationBlock *tb, *tb_next, *current_tb, *saved_tb;
target_ulong tb_start, tb_end;
- target_ulong current_pc, current_cs_base;
p = page_find(start >> TARGET_PAGE_BITS);
if (!p)
@@ -907,9 +906,6 @@ void tb_invalidate_phys_page_range(targe
current_tb_not_found = is_cpu_write_access;
current_tb_modified = 0;
current_tb = NULL; /* avoid warning */
- current_pc = 0; /* avoid warning */
- current_cs_base = 0; /* avoid warning */
- current_flags = 0; /* avoid warning */
tb = p->first_tb;
while (tb != NULL) {
n = (long)tb & 3;
@@ -946,14 +942,6 @@ void tb_invalidate_phys_page_range(targe
current_tb_modified = 1;
cpu_restore_state(current_tb, env,
env->mem_io_pc, NULL);
-#if defined(TARGET_I386)
- current_flags = env->hflags;
- current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
- current_cs_base = (target_ulong)env->segs[R_CS].base;
- current_pc = current_cs_base + env->eip;
-#else
-#error unsupported CPU
-#endif
}
#endif /* TARGET_HAS_PRECISE_SMC */
/* we need to do that to handle the case where a signal
@@ -987,7 +975,7 @@ void tb_invalidate_phys_page_range(targe
modifying the memory. It will ensure that it cannot modify
itself */
env->current_tb = NULL;
- tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
+ env->next_cflags = 1;
cpu_resume_from_signal(env, NULL);
}
#endif
@@ -1026,8 +1014,7 @@ static inline void tb_invalidate_phys_pa
static void tb_invalidate_phys_page(target_phys_addr_t addr,
unsigned long pc, void *puc)
{
- int n, current_flags, current_tb_modified;
- target_ulong current_pc, current_cs_base;
+ int n, current_tb_modified;
PageDesc *p;
TranslationBlock *tb, *current_tb;
#ifdef TARGET_HAS_PRECISE_SMC
@@ -1041,9 +1028,6 @@ static void tb_invalidate_phys_page(targ
tb = p->first_tb;
current_tb_modified = 0;
current_tb = NULL;
- current_pc = 0; /* avoid warning */
- current_cs_base = 0; /* avoid warning */
- current_flags = 0; /* avoid warning */
#ifdef TARGET_HAS_PRECISE_SMC
if (tb && pc != 0) {
current_tb = tb_find_pc(pc);
@@ -1063,14 +1047,6 @@ static void tb_invalidate_phys_page(targ
current_tb_modified = 1;
cpu_restore_state(current_tb, env, pc, puc);
-#if defined(TARGET_I386)
- current_flags = env->hflags;
- current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
- current_cs_base = (target_ulong)env->segs[R_CS].base;
- current_pc = current_cs_base + env->eip;
-#else
-#error unsupported CPU
-#endif
}
#endif /* TARGET_HAS_PRECISE_SMC */
tb_phys_invalidate(tb, addr);
@@ -1083,7 +1059,7 @@ static void tb_invalidate_phys_page(targ
modifying the memory. It will ensure that it cannot modify
itself */
env->current_tb = NULL;
- tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
+ env->next_cflags = 1;
cpu_resume_from_signal(env, puc);
}
#endif
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 07/13] Restore pc on watchpoint hits
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (5 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 06/13] Switch self-modified code recompilation to next_cflags Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 08/13] Remove premature memop TB terminations Jan Kiszka
` (5 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-restore-pc-on-watchpoint-hit.patch --]
[-- Type: text/plain, Size: 2623 bytes --]
In order to provide accurate information about the triggering
instruction, this patch adds the required bits to restore the pc if the
access happened inside a TB. With the BP_STOP_BEFORE_ACCESS flag, the
watchpoint user can control if the debug trap should be issued on or
after the accessing instruction.
In contrast to the earlier posted version, this one makes use of
next_cflags to ensure that the next TB contains just a single
instruction.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-all.h | 1 +
exec.c | 22 ++++++++++++++++++++--
2 files changed, 21 insertions(+), 2 deletions(-)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -2490,16 +2490,34 @@ static CPUWriteMemoryFunc *notdirty_mem_
static void check_watchpoint(int offset, int len_mask, int flags)
{
CPUState *env = cpu_single_env;
+ TranslationBlock *tb;
target_ulong vaddr;
CPUWatchpoint *wp;
+ if (env->watchpoint_hit) {
+ /* We re-entered the check after replacing the TB. Now raise
+ * the debug interrupt so that is will trigger after the
+ * current instruction. */
+ cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
+ return;
+ }
vaddr = (env->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
if ((vaddr == (wp->vaddr & len_mask) ||
(vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
env->watchpoint_hit = wp;
- cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
- break;
+ tb = tb_find_pc(env->mem_io_pc);
+ if (!tb) {
+ cpu_abort(env, "check_watchpoint: could not find TB for pc=%p",
+ (void *)env->mem_io_pc);
+ }
+ cpu_restore_state(tb, env, env->mem_io_pc, NULL);
+ tb_phys_invalidate(tb, -1);
+ if (wp->flags & BP_STOP_BEFORE_ACCESS)
+ env->exception_index = EXCP_DEBUG;
+ else
+ env->next_cflags = 1;
+ cpu_resume_from_signal(env, NULL);
}
}
}
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -765,6 +765,7 @@ void cpu_reset_interrupt(CPUState *env,
#define BP_MEM_READ 0x01
#define BP_MEM_WRITE 0x02
#define BP_MEM_ACCESS (BP_MEM_READ | BP_MEM_WRITE)
+#define BP_STOP_BEFORE_ACCESS 0x04
#define BP_GDB 0x10
int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 08/13] Remove premature memop TB terminations
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (6 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 07/13] Restore pc on watchpoint hits Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 09/13] qemu: gdbstub: manage CPUs as threads Jan Kiszka
` (4 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-remove-memop-tb-termination.patch --]
[-- Type: text/plain, Size: 2349 bytes --]
Now that we can properly restore the pc on watchpoint hits, there is no
more need for prematurely terminating TBs if watchpoints are present.
Remove all related bits.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
exec.c | 4 ----
target-arm/translate.c | 6 ------
target-m68k/translate.c | 6 ------
3 files changed, 16 deletions(-)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1313,10 +1313,6 @@ int cpu_watchpoint_insert(CPUState *env,
env->watchpoints = wp;
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
- re-execute bits are in. */
- tb_flush(env);
if (watchpoint)
*watchpoint = wp;
Index: b/target-arm/translate.c
===================================================================
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8673,12 +8673,6 @@ static inline void gen_intermediate_code
gen_set_label(dc->condlabel);
dc->condjmp = 0;
}
- /* Terminate the TB on memory ops if watchpoints are present. */
- /* FIXME: This should be replacd by the deterministic execution
- * IRQ raising bits. */
- if (dc->is_mem && env->watchpoints)
- break;
-
/* Translation stops when a conditional branch is enoutered.
* Otherwise the subsequent code could get translated several times.
* Also stop translation when a page boundary is reached. This
Index: b/target-m68k/translate.c
===================================================================
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2978,12 +2978,6 @@ gen_intermediate_code_internal(CPUState
dc->insn_pc = dc->pc;
disas_m68k_insn(env, dc);
num_insns++;
-
- /* Terminate the TB on memory ops if watchpoints are present. */
- /* FIXME: This should be replaced by the deterministic execution
- * IRQ raising bits. */
- if (dc->is_mem && env->watchpoints)
- break;
} while (!dc->is_jmp && gen_opc_ptr < gen_opc_end &&
!env->singlestep_enabled &&
(pc_offset) < (TARGET_PAGE_SIZE - 32) &&
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 09/13] qemu: gdbstub: manage CPUs as threads
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (7 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 08/13] Remove premature memop TB terminations Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 10/13] Introduce BP_WATCHPOINT_HIT flag Jan Kiszka
` (3 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-gdbstub-manage-cpus-as-threads.patch --]
[-- Type: text/plain, Size: 21551 bytes --]
This patch enhances QEMU's built-in debugger for SMP guest debugging.
Using the thread support of the gdb remote protocol, each VCPU is mapped
on a pseudo thread and exposed to the gdb frontend. This way you can
easy switch the focus of gdb between the VCPUs and observe their states.
On breakpoint hit, the focus is automatically adjusted just as for
normal multi-threaded application under gdb control.
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, SMP guest debugging was practically unfeasible.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
gdbstub.c | 273 +++++++++++++++++++++++++++++++++++++++++++-------------------
gdbstub.h | 1
vl.c | 1
3 files changed, 194 insertions(+), 81 deletions(-)
Index: b/gdbstub.h
===================================================================
--- a/gdbstub.h
+++ b/gdbstub.h
@@ -8,6 +8,7 @@ typedef void (*gdb_syscall_complete_cb)(
void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...);
int use_gdb_syscalls(void);
+void gdb_set_stop_cpu(CPUState *env);
#ifdef CONFIG_USER_ONLY
int gdb_handlesig (CPUState *, int);
void gdb_exit(CPUState *, int);
Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -8066,6 +8066,7 @@ static int main_loop(void)
ret = EXCP_INTERRUPT;
}
if (unlikely(ret == EXCP_DEBUG)) {
+ gdb_set_stop_cpu(cur_cpu);
vm_stop(EXCP_DEBUG);
}
/* If all cpus are halted then wait until the next IRQ */
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -69,7 +69,9 @@ enum RSState {
RS_SYSCALL,
};
typedef struct GDBState {
- CPUState *env; /* current CPU */
+ CPUState *c_cpu; /* current CPU for step/continue ops */
+ CPUState *g_cpu; /* current CPU for other ops */
+ CPUState *query_cpu; /* for q{f|s}ThreadInfo */
enum RSState state; /* parsing state */
char line_buf[MAX_PACKET_LENGTH];
int line_buf_index;
@@ -90,6 +92,8 @@ typedef struct GDBState {
*/
static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
+static GDBState *gdbserver_state;
+
/* This is an ugly hack to cope with both new and old gdb.
If gdb sends qXfer:features:read then assume we're talking to a newish
gdb that understands target descriptions. */
@@ -99,9 +103,6 @@ static int gdb_has_xml;
/* XXX: This is not thread safe. Do we care? */
static int gdbserver_fd = -1;
-/* XXX: remove this hack. */
-static GDBState gdbserver_state;
-
static int get_char(GDBState *s)
{
uint8_t ch;
@@ -126,8 +127,6 @@ static int get_char(GDBState *s)
}
#endif
-/* GDB stub state for use by semihosting syscalls. */
-static GDBState *gdb_syscall_state;
static gdb_syscall_complete_cb gdb_current_syscall_cb;
enum {
@@ -141,8 +140,8 @@ enum {
int use_gdb_syscalls(void)
{
if (gdb_syscall_mode == GDB_SYS_UNKNOWN) {
- gdb_syscall_mode = (gdb_syscall_state ? GDB_SYS_ENABLED
- : GDB_SYS_DISABLED);
+ gdb_syscall_mode = (gdbserver_state ? GDB_SYS_ENABLED
+ : GDB_SYS_DISABLED);
}
return gdb_syscall_mode == GDB_SYS_ENABLED;
}
@@ -1033,7 +1032,7 @@ static int memtox(char *buf, const char
return p - buf;
}
-const char *get_feature_xml(CPUState *env, const char *p, const char **newp)
+const char *get_feature_xml(const char *p, const char **newp)
{
extern const char *const xml_builtin[][2];
size_t len;
@@ -1059,7 +1058,7 @@ const char *get_feature_xml(CPUState *en
"<xi:include href=\"%s\"/>",
GDB_CORE_XML);
- for (r = env->gdb_regs; r; r = r->next) {
+ for (r = first_cpu->gdb_regs; r; r = r->next) {
strcat(target_xml, "<xi:include href=\"");
strcat(target_xml, r->xml);
strcat(target_xml, "\"/>");
@@ -1162,49 +1161,78 @@ static 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)
+static int gdb_handle_packet(GDBState *s, const char *line_buf)
{
+ CPUState *env;
const char *p;
- int ch, reg_size, type, res;
+ int ch, reg_size, type, res, thread;
char buf[MAX_PACKET_LENGTH];
uint8_t mem_buf[MAX_PACKET_LENGTH];
uint8_t *registers;
@@ -1224,26 +1252,26 @@ 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') {
addr = strtoull(p, (char **)&p, 16);
#if defined(TARGET_I386)
- env->eip = addr;
+ s->c_cpu->eip = addr;
#elif defined (TARGET_PPC)
- env->nip = addr;
+ s->c_cpu->nip = addr;
#elif defined (TARGET_SPARC)
- env->pc = addr;
- env->npc = addr + 4;
+ s->c_cpu->pc = addr;
+ s->c_cpu->npc = addr + 4;
#elif defined (TARGET_ARM)
- env->regs[15] = addr;
+ s->c_cpu->regs[15] = addr;
#elif defined (TARGET_SH4)
- env->pc = addr;
+ s->c_cpu->pc = addr;
#elif defined (TARGET_MIPS)
- env->active_tc.PC = addr;
+ s->c_cpu->active_tc.PC = addr;
#elif defined (TARGET_CRIS)
- env->pc = addr;
+ s->c_cpu->pc = addr;
#endif
}
gdb_continue(s);
@@ -1258,7 +1286,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;
@@ -1266,23 +1294,23 @@ static int gdb_handle_packet(GDBState *s
if (*p != '\0') {
addr = strtoull(p, (char **)&p, 16);
#if defined(TARGET_I386)
- env->eip = addr;
+ s->c_cpu->eip = addr;
#elif defined (TARGET_PPC)
- env->nip = addr;
+ s->c_cpu->nip = addr;
#elif defined (TARGET_SPARC)
- env->pc = addr;
- env->npc = addr + 4;
+ s->c_cpu->pc = addr;
+ s->c_cpu->npc = addr + 4;
#elif defined (TARGET_ARM)
- env->regs[15] = addr;
+ s->c_cpu->regs[15] = addr;
#elif defined (TARGET_SH4)
- env->pc = addr;
+ s->c_cpu->pc = addr;
#elif defined (TARGET_MIPS)
- env->active_tc.PC = addr;
+ s->c_cpu->active_tc.PC = addr;
#elif defined (TARGET_CRIS)
- env->pc = addr;
+ s->c_cpu->pc = addr;
#endif
}
- cpu_single_step(env, sstep_flags);
+ cpu_single_step(s->c_cpu, sstep_flags);
gdb_continue(s);
return RS_IDLE;
case 'F':
@@ -1301,7 +1329,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(s->c_cpu, ret, err);
if (type == 'C') {
put_packet(s, "T02");
} else {
@@ -1312,7 +1340,7 @@ static int gdb_handle_packet(GDBState *s
case 'g':
len = 0;
for (addr = 0; addr < num_g_regs; addr++) {
- reg_size = gdb_read_register(env, mem_buf + len, addr);
+ reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
len += reg_size;
}
memtohex(buf, mem_buf, len);
@@ -1323,7 +1351,7 @@ static int gdb_handle_packet(GDBState *s
len = strlen(p) / 2;
hextomem((uint8_t *)registers, p, len);
for (addr = 0; addr < num_g_regs && len > 0; addr++) {
- reg_size = gdb_write_register(env, registers, addr);
+ reg_size = gdb_write_register(s->g_cpu, registers, addr);
len -= reg_size;
registers += reg_size;
}
@@ -1334,7 +1362,7 @@ static int gdb_handle_packet(GDBState *s
if (*p == ',')
p++;
len = strtoull(p, NULL, 16);
- if (cpu_memory_rw_debug(env, addr, mem_buf, len, 0) != 0) {
+ if (cpu_memory_rw_debug(s->g_cpu, addr, mem_buf, len, 0) != 0) {
put_packet (s, "E14");
} else {
memtohex(buf, mem_buf, len);
@@ -1349,7 +1377,7 @@ static int gdb_handle_packet(GDBState *s
if (*p == ':')
p++;
hextomem(mem_buf, p, len);
- if (cpu_memory_rw_debug(env, addr, mem_buf, len, 1) != 0)
+ if (cpu_memory_rw_debug(s->g_cpu, addr, mem_buf, len, 1) != 0)
put_packet(s, "E14");
else
put_packet(s, "OK");
@@ -1361,7 +1389,7 @@ static int gdb_handle_packet(GDBState *s
if (!gdb_has_xml)
goto unknown_command;
addr = strtoull(p, (char **)&p, 16);
- reg_size = gdb_read_register(env, mem_buf, addr);
+ reg_size = gdb_read_register(s->g_cpu, mem_buf, addr);
if (reg_size) {
memtohex(buf, mem_buf, reg_size);
put_packet(s, buf);
@@ -1377,7 +1405,7 @@ static int gdb_handle_packet(GDBState *s
p++;
reg_size = strlen(p) / 2;
hextomem(mem_buf, p, reg_size);
- gdb_write_register(env, mem_buf, addr);
+ gdb_write_register(s->g_cpu, mem_buf, addr);
put_packet(s, "OK");
break;
case 'Z':
@@ -1390,9 +1418,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)
@@ -1400,6 +1428,45 @@ static int gdb_handle_packet(GDBState *s
else
put_packet(s, "E22");
break;
+ case 'H':
+ type = *p++;
+ thread = strtoull(p, (char **)&p, 16);
+ if (thread == -1 || thread == 0) {
+ put_packet(s, "OK");
+ break;
+ }
+ for (env = first_cpu; env != NULL; env = env->next_cpu)
+ if (env->cpu_index + 1 == thread)
+ break;
+ if (env == NULL) {
+ put_packet(s, "E22");
+ break;
+ }
+ switch (type) {
+ case 'c':
+ s->c_cpu = env;
+ put_packet(s, "OK");
+ break;
+ case 'g':
+ s->g_cpu = env;
+ put_packet(s, "OK");
+ break;
+ default:
+ put_packet(s, "E22");
+ break;
+ }
+ break;
+ case 'T':
+ thread = strtoull(p, (char **)&p, 16);
+#ifndef CONFIG_USER_ONLY
+ if (thread > 0 && thread < smp_cpus + 1)
+#else
+ if (thread == 1)
+#endif
+ put_packet(s, "OK");
+ else
+ put_packet(s, "E22");
+ break;
case 'q':
case 'Q':
/* parse any 'q' packets here */
@@ -1425,10 +1492,39 @@ static int gdb_handle_packet(GDBState *s
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");
+ break;
+ } else if (strcmp(p,"fThreadInfo") == 0) {
+ s->query_cpu = first_cpu;
+ goto report_cpuinfo;
+ } else if (strcmp(p,"sThreadInfo") == 0) {
+ report_cpuinfo:
+ if (s->query_cpu) {
+ snprintf(buf, sizeof(buf), "m%x", s->query_cpu->cpu_index+1);
+ put_packet(s, buf);
+ s->query_cpu = s->query_cpu->next_cpu;
+ } else
+ put_packet(s, "l");
+ break;
+ } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
+ thread = strtoull(p+16, (char **)&p, 16);
+ for (env = first_cpu; env != NULL; env = env->next_cpu)
+ if (env->cpu_index + 1 == thread) {
+ len = snprintf((char *)mem_buf, sizeof(mem_buf),
+ "CPU#%d [%s]", env->cpu_index,
+ env->halted ? "halted " : "running");
+ memtohex(buf, mem_buf, len);
+ put_packet(s, buf);
+ break;
+ }
+ break;
}
#ifdef CONFIG_LINUX_USER
else if (strncmp(p, "Offsets", 7) == 0) {
- TaskState *ts = env->opaque;
+ TaskState *ts = s->c_cpu->opaque;
snprintf(buf, sizeof(buf),
"Text=" TARGET_ABI_FMT_lx ";Data=" TARGET_ABI_FMT_lx
@@ -1455,7 +1551,7 @@ static int gdb_handle_packet(GDBState *s
gdb_has_xml = 1;
p += 19;
- xml = get_feature_xml(env, p, &p);
+ xml = get_feature_xml(p, &p);
if (!xml) {
sprintf(buf, "E00");
put_packet(s, buf);
@@ -1503,10 +1599,17 @@ static int gdb_handle_packet(GDBState *s
extern void tb_flush(CPUState *env);
+void gdb_set_stop_cpu(CPUState *env)
+{
+ gdbserver_state->c_cpu = env;
+ gdbserver_state->g_cpu = env;
+}
+
#ifndef CONFIG_USER_ONLY
static void gdb_vm_stopped(void *opaque, int reason)
{
- GDBState *s = opaque;
+ GDBState *s = gdbserver_state;
+ CPUState *env = s->c_cpu;
char buf[256];
const char *type;
int ret;
@@ -1515,11 +1618,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;
@@ -1530,20 +1633,22 @@ static void gdb_vm_stopped(void *opaque,
type = "";
break;
}
- snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
- SIGTRAP, type, s->env->watchpoint_hit->vaddr);
+ snprintf(buf, sizeof(buf),
+ "T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
+ SIGTRAP, env->cpu_index+1, 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;
} else {
ret = 0;
}
- snprintf(buf, sizeof(buf), "S%02x", ret);
+ snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, env->cpu_index+1);
put_packet(s, buf);
}
#endif
@@ -1562,7 +1667,7 @@ void gdb_do_syscall(gdb_syscall_complete
uint64_t i64;
GDBState *s;
- s = gdb_syscall_state;
+ s = gdbserver_state;
if (!s)
return;
gdb_current_syscall_cb = cb;
@@ -1607,15 +1712,14 @@ 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(s->c_cpu, 0);
#else
- cpu_interrupt(s->env, CPU_INTERRUPT_EXIT);
+ cpu_interrupt(s->c_cpu, CPU_INTERRUPT_EXIT);
#endif
}
static void gdb_read_byte(GDBState *s, int ch)
{
- CPUState *env = s->env;
int i, csum;
uint8_t reply;
@@ -1681,7 +1785,7 @@ static void gdb_read_byte(GDBState *s, i
} else {
reply = '+';
put_buffer(s, &reply, 1);
- s->state = gdb_handle_packet(s, env, s->line_buf);
+ s->state = gdb_handle_packet(s, s->line_buf);
}
break;
default:
@@ -1698,7 +1802,7 @@ gdb_handlesig (CPUState *env, int sig)
char buf[256];
int n;
- s = &gdbserver_state;
+ s = gdbserver_state;
if (gdbserver_fd < 0 || s->fd < 0)
return sig;
@@ -1746,7 +1850,7 @@ void gdb_exit(CPUState *env, int code)
GDBState *s;
char buf[4];
- s = &gdbserver_state;
+ s = gdbserver_state;
if (gdbserver_fd < 0 || s->fd < 0)
return;
@@ -1755,7 +1859,7 @@ void gdb_exit(CPUState *env, int code)
}
-static void gdb_accept(void *opaque)
+static void gdb_accept(void)
{
GDBState *s;
struct sockaddr_in sockaddr;
@@ -1777,13 +1881,20 @@ static void gdb_accept(void *opaque)
val = 1;
setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&val, sizeof(val));
- s = &gdbserver_state;
+ s = qemu_mallocz(sizeof(GDBState));
+ if (!s) {
+ errno = ENOMEM;
+ perror("accept");
+ return;
+ }
+
memset (s, 0, sizeof (GDBState));
- s->env = first_cpu; /* XXX: allow to change CPU */
+ s->c_cpu = first_cpu;
+ s->g_cpu = first_cpu;
s->fd = fd;
gdb_has_xml = 0;
- gdb_syscall_state = s;
+ gdbserver_state = s;
fcntl(fd, F_SETFL, O_NONBLOCK);
}
@@ -1825,7 +1936,7 @@ int gdbserver_start(int port)
if (gdbserver_fd < 0)
return -1;
/* accept connections */
- gdb_accept (NULL);
+ gdb_accept();
return 0;
}
#else
@@ -1838,11 +1949,10 @@ static int gdb_chr_can_receive(void *opa
static void gdb_chr_receive(void *opaque, const uint8_t *buf, int size)
{
- GDBState *s = opaque;
int i;
for (i = 0; i < size; i++) {
- gdb_read_byte(s, buf[i]);
+ gdb_read_byte(gdbserver_state, buf[i]);
}
}
@@ -1851,7 +1961,6 @@ static void gdb_chr_event(void *opaque,
switch (event) {
case CHR_EVENT_RESET:
vm_stop(EXCP_INTERRUPT);
- gdb_syscall_state = opaque;
gdb_has_xml = 0;
break;
default:
@@ -1886,11 +1995,13 @@ int gdbserver_start(const char *port)
if (!s) {
return -1;
}
- s->env = first_cpu; /* XXX: allow to change CPU */
+ s->c_cpu = first_cpu;
+ s->g_cpu = first_cpu;
s->chr = chr;
+ gdbserver_state = s;
qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
- gdb_chr_event, s);
- qemu_add_vm_stop_handler(gdb_vm_stopped, s);
+ gdb_chr_event, NULL);
+ qemu_add_vm_stop_handler(gdb_vm_stopped, NULL);
return 0;
}
#endif
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 10/13] Introduce BP_WATCHPOINT_HIT flag
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (8 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 09/13] qemu: gdbstub: manage CPUs as threads Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 11/13] Add debug exception hook Jan Kiszka
` (2 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-introduce-BP_WATCHPOINT_HIT-flag.patch --]
[-- Type: text/plain, Size: 3944 bytes --]
When one watchpoint is hit, others might have triggered as well. To
support users of the watchpoint API which need to detect such cases,
the BP_WATCHPOINT_HIT flag is introduced and maintained.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-all.h | 1 +
cpu-exec.c | 11 +++++++++++
exec.c | 31 ++++++++++++++++++-------------
3 files changed, 30 insertions(+), 13 deletions(-)
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -766,6 +766,7 @@ void cpu_reset_interrupt(CPUState *env,
#define BP_MEM_WRITE 0x02
#define BP_MEM_ACCESS (BP_MEM_READ | BP_MEM_WRITE)
#define BP_STOP_BEFORE_ACCESS 0x04
+#define BP_WATCHPOINT_HIT 0x08
#define BP_GDB 0x10
int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -236,6 +236,15 @@ static inline TranslationBlock *tb_find_
return tb;
}
+static void cpu_handle_debug_exception(CPUState *env)
+{
+ CPUWatchpoint *wp;
+
+ if (!env->watchpoint_hit)
+ for (wp = env->watchpoints; wp != NULL; wp = wp->next)
+ wp->flags &= ~BP_WATCHPOINT_HIT;
+}
+
/* main execution loop */
int cpu_exec(CPUState *env1)
@@ -290,6 +299,8 @@ int cpu_exec(CPUState *env1)
if (env->exception_index >= EXCP_INTERRUPT) {
/* exit request from the cpu execution loop */
ret = env->exception_index;
+ if (ret == EXCP_DEBUG)
+ cpu_handle_debug_exception(env);
break;
} else if (env->user_mode_only) {
/* if user mode only, we simulate a fake exception
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1328,7 +1328,7 @@ int cpu_watchpoint_remove(CPUState *env,
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
if (addr == wp->vaddr && len_mask == wp->len_mask
- && flags == wp->flags) {
+ && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
cpu_watchpoint_remove_by_ref(env, wp);
return 0;
}
@@ -2501,19 +2501,24 @@ static void check_watchpoint(int offset,
for (wp = env->watchpoints; wp != NULL; wp = wp->next) {
if ((vaddr == (wp->vaddr & len_mask) ||
(vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
- env->watchpoint_hit = wp;
- tb = tb_find_pc(env->mem_io_pc);
- if (!tb) {
- cpu_abort(env, "check_watchpoint: could not find TB for pc=%p",
- (void *)env->mem_io_pc);
+ wp->flags |= BP_WATCHPOINT_HIT;
+ if (!env->watchpoint_hit) {
+ env->watchpoint_hit = wp;
+ tb = tb_find_pc(env->mem_io_pc);
+ if (!tb) {
+ cpu_abort(env, "check_watchpoint: could not find TB for "
+ "pc=%p", (void *)env->mem_io_pc);
+ }
+ cpu_restore_state(tb, env, env->mem_io_pc, NULL);
+ tb_phys_invalidate(tb, -1);
+ if (wp->flags & BP_STOP_BEFORE_ACCESS)
+ env->exception_index = EXCP_DEBUG;
+ else
+ env->next_cflags = 1;
+ cpu_resume_from_signal(env, NULL);
}
- cpu_restore_state(tb, env, env->mem_io_pc, NULL);
- tb_phys_invalidate(tb, -1);
- if (wp->flags & BP_STOP_BEFORE_ACCESS)
- env->exception_index = EXCP_DEBUG;
- else
- env->next_cflags = 1;
- cpu_resume_from_signal(env, NULL);
+ } else {
+ wp->flags &= ~BP_WATCHPOINT_HIT;
}
}
}
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 11/13] Add debug exception hook
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (9 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 10/13] Introduce BP_WATCHPOINT_HIT flag Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 12/13] Introduce BP_CPU as a breakpoint type Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 13/13] x86: Debug register emulation Jan Kiszka
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-add-debug-exception-hook.patch --]
[-- Type: text/plain, Size: 1444 bytes --]
This patch allows to hook into the delivery of EXCP_DEBUG so that other
use beyond guest debugging becomes possible.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-exec.c | 13 +++++++++++++
exec-all.h | 4 ++++
2 files changed, 17 insertions(+)
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -236,6 +236,16 @@ static inline TranslationBlock *tb_find_
return tb;
}
+static CPUDebugExcpHandler *debug_excp_handler;
+
+CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler)
+{
+ CPUDebugExcpHandler *old_handler = debug_excp_handler;
+
+ debug_excp_handler = handler;
+ return old_handler;
+}
+
static void cpu_handle_debug_exception(CPUState *env)
{
CPUWatchpoint *wp;
@@ -243,6 +253,9 @@ static void cpu_handle_debug_exception(C
if (!env->watchpoint_hit)
for (wp = env->watchpoints; wp != NULL; wp = wp->next)
wp->flags &= ~BP_WATCHPOINT_HIT;
+
+ if (debug_excp_handler)
+ debug_excp_handler(env);
}
/* main execution loop */
Index: b/exec-all.h
===================================================================
--- a/exec-all.h
+++ b/exec-all.h
@@ -385,3 +385,7 @@ static inline int kqemu_is_ok(CPUState *
}
#endif
+
+typedef void (CPUDebugExcpHandler)(CPUState *env);
+
+CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler);
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 12/13] Introduce BP_CPU as a breakpoint type
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (10 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 11/13] Add debug exception hook Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
2008-10-14 9:12 ` [Qemu-devel] [PATCH 13/13] x86: Debug register emulation Jan Kiszka
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-introduce-BP_CPU.patch --]
[-- Type: text/plain, Size: 3408 bytes --]
Add another breakpoint/watchpoint type to BP_GDB: BP_CPU. This type is
intended for hardware-assisted break/watchpoint emulations like the x86
architecture requires.
To keep the highest priority for BP_GDB breakpoints, this type is
always inserted at the head of break/watchpoint lists, thus is found
first when looking up the origin of a debug interruption.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
cpu-all.h | 1 +
exec.c | 46 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 39 insertions(+), 8 deletions(-)
Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -768,6 +768,7 @@ void cpu_reset_interrupt(CPUState *env,
#define BP_STOP_BEFORE_ACCESS 0x04
#define BP_WATCHPOINT_HIT 0x08
#define BP_GDB 0x10
+#define BP_CPU 0x20
int cpu_breakpoint_insert(CPUState *env, target_ulong pc, int flags,
CPUBreakpoint **breakpoint);
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1290,7 +1290,7 @@ int cpu_watchpoint_insert(CPUState *env,
int flags, CPUWatchpoint **watchpoint)
{
target_ulong len_mask = ~(len - 1);
- CPUWatchpoint *wp;
+ CPUWatchpoint *wp, *prev_wp;
/* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */
if ((len != 1 && len != 2 && len != 4 && len != 8) || (addr & ~len_mask)) {
@@ -1306,11 +1306,26 @@ int cpu_watchpoint_insert(CPUState *env,
wp->len_mask = len_mask;
wp->flags = flags;
- wp->next = env->watchpoints;
- wp->prev = NULL;
+ /* keep all GDB-injected watchpoints in front */
+ if (!(flags & BP_GDB) && env->watchpoints) {
+ prev_wp = env->watchpoints;
+ while (prev_wp->next != NULL && (prev_wp->next->flags & BP_GDB))
+ prev_wp = prev_wp->next;
+ } else {
+ prev_wp = NULL;
+ }
+
+ /* Insert new watchpoint */
+ if (prev_wp) {
+ wp->next = prev_wp->next;
+ prev_wp->next = wp;
+ } else {
+ wp->next = env->watchpoints;
+ env->watchpoints = wp;
+ }
if (wp->next)
wp->next->prev = wp;
- env->watchpoints = wp;
+ wp->prev = prev_wp;
tlb_flush_page(env, addr);
@@ -1366,7 +1381,7 @@ int cpu_breakpoint_insert(CPUState *env,
CPUBreakpoint **breakpoint)
{
#if defined(TARGET_HAS_ICE)
- CPUBreakpoint *bp;
+ CPUBreakpoint *bp, *prev_bp;
bp = qemu_malloc(sizeof(*bp));
if (!bp)
@@ -1375,11 +1390,26 @@ int cpu_breakpoint_insert(CPUState *env,
bp->pc = pc;
bp->flags = flags;
- bp->next = env->breakpoints;
- bp->prev = NULL;
+ /* keep all GDB-injected breakpoints in front */
+ if (!(flags & BP_GDB) && env->breakpoints) {
+ prev_bp = env->breakpoints;
+ while (prev_bp->next != NULL && (prev_bp->next->flags & BP_GDB))
+ prev_bp = prev_bp->next;
+ } else {
+ prev_bp = NULL;
+ }
+
+ /* Insert new breakpoint */
+ if (prev_bp) {
+ bp->next = prev_bp->next;
+ prev_bp->next = bp;
+ } else {
+ bp->next = env->breakpoints;
+ env->breakpoints = bp;
+ }
if (bp->next)
bp->next->prev = bp;
- env->breakpoints = bp;
+ bp->prev = prev_bp;
breakpoint_invalidate(env, pc);
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH 13/13] x86: Debug register emulation
2008-10-14 9:12 [Qemu-devel] [PATCH 00/13] Enhance debugging support - 3rd take Jan Kiszka
` (11 preceding siblings ...)
2008-10-14 9:12 ` [Qemu-devel] [PATCH 12/13] Introduce BP_CPU as a breakpoint type Jan Kiszka
@ 2008-10-14 9:12 ` Jan Kiszka
12 siblings, 0 replies; 23+ messages in thread
From: Jan Kiszka @ 2008-10-14 9:12 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
[-- Attachment #1: qemu-x86-debug-register-support.patch --]
[-- Type: text/plain, Size: 10441 bytes --]
Built on top of previously enhanced breakpoint/watchpoint support, this
patch adds full debug register emulation for the x86 architecture.
Many corner cases were considered, and the result was successfully
tested inside a Linux guest with gdb, but I won't be surprised if one
or two scenarios still behave differently in reality.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
linux-user/main.c | 4 -
target-i386/cpu.h | 34 +++++++++++++
target-i386/helper.c | 118 ++++++++++++++++++++++++++++++++++++------------
target-i386/op_helper.c | 79 ++++++++++++++++++++++++++++++--
4 files changed, 199 insertions(+), 36 deletions(-)
Index: b/linux-user/main.c
===================================================================
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -405,7 +405,7 @@ void cpu_loop(CPUX86State *env)
queue_signal(env, info.si_signo, &info);
}
break;
- case EXCP01_SSTP:
+ case EXCP01_DB:
case EXCP03_INT3:
#ifndef TARGET_X86_64
if (env->eflags & VM_MASK) {
@@ -415,7 +415,7 @@ void cpu_loop(CPUX86State *env)
{
info.si_signo = SIGTRAP;
info.si_errno = 0;
- if (trapnr == EXCP01_SSTP) {
+ if (trapnr == EXCP01_DB) {
info.si_code = TARGET_TRAP_BRKPT;
info._sifields._sigfault._addr = env->eip;
} else {
Index: b/target-i386/cpu.h
===================================================================
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -199,6 +199,16 @@
#define CR4_OSFXSR_MASK (1 << 9)
#define CR4_OSXMMEXCPT_MASK (1 << 10)
+#define DR6_BD (1 << 13)
+#define DR6_BS (1 << 14)
+#define DR6_BT (1 << 15)
+#define DR6_FIXED_1 0xffff0ff0
+
+#define DR7_GD (1 << 13)
+#define DR7_TYPE_SHIFT 16
+#define DR7_LEN_SHIFT 18
+#define DR7_FIXED_1 0x00000400
+
#define PG_PRESENT_BIT 0
#define PG_RW_BIT 1
#define PG_USER_BIT 2
@@ -355,7 +365,7 @@
#define CPUID_MWAIT_EMX (1 << 0) /* enumeration supported */
#define EXCP00_DIVZ 0
-#define EXCP01_SSTP 1
+#define EXCP01_DB 1
#define EXCP02_NMI 2
#define EXCP03_INT3 3
#define EXCP04_INTO 4
@@ -587,6 +597,10 @@ typedef struct CPUX86State {
int exception_is_int;
target_ulong exception_next_eip;
target_ulong dr[8]; /* debug registers */
+ union {
+ CPUBreakpoint *cpu_breakpoint[4];
+ CPUWatchpoint *cpu_watchpoint[4];
+ }; /* break/watchpoints for dr[0..3] */
uint32_t smbase;
int old_exception; /* exception in flight */
@@ -776,6 +790,24 @@ static inline void cpu_clone_regs(CPUSta
#define CPU_PC_FROM_TB(env, tb) env->eip = tb->pc - tb->cs_base
+static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
+{
+ return (dr7 >> (index * 2)) & 3;
+}
+
+static inline int hw_breakpoint_type(unsigned long dr7, int index)
+{
+ return (dr7 >> (DR7_TYPE_SHIFT + (index * 2))) & 3;
+}
+
+static inline int hw_breakpoint_len(unsigned long dr7, int index)
+{
+ int len = ((dr7 >> (DR7_LEN_SHIFT + (index * 2))) & 3);
+ return (len == 2) ? 8 : len + 1;
+}
+
+int check_hw_breakpoints(CPUState *env, int force_dr6_update);
+
#include "cpu-all.h"
#include "svm.h"
Index: b/target-i386/helper.c
===================================================================
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -32,8 +32,6 @@
//#define DEBUG_MMU
-static int cpu_x86_register (CPUX86State *env, const char *cpu_model);
-
static void add_flagname_to_bitmaps(char *flagname, uint32_t *features,
uint32_t *ext_features,
uint32_t *ext2_features,
@@ -91,33 +89,6 @@ static void add_flagname_to_bitmaps(char
fprintf(stderr, "CPU feature %s not found\n", flagname);
}
-CPUX86State *cpu_x86_init(const char *cpu_model)
-{
- CPUX86State *env;
- static int inited;
-
- env = qemu_mallocz(sizeof(CPUX86State));
- if (!env)
- return NULL;
- cpu_exec_init(env);
- env->cpu_model_str = cpu_model;
-
- /* init various static tables */
- if (!inited) {
- inited = 1;
- optimize_flags_init();
- }
- if (cpu_x86_register(env, cpu_model) < 0) {
- cpu_x86_close(env);
- return NULL;
- }
- cpu_reset(env);
-#ifdef USE_KQEMU
- kqemu_init(env);
-#endif
- return env;
-}
-
typedef struct x86_def_t {
const char *name;
uint32_t level;
@@ -471,6 +442,12 @@ void cpu_reset(CPUX86State *env)
env->fpuc = 0x37f;
env->mxcsr = 0x1f80;
+
+ memset(env->dr, 0, sizeof(env->dr));
+ env->dr[6] = DR6_FIXED_1;
+ env->dr[7] = DR7_FIXED_1;
+ cpu_breakpoint_remove_all(env, BP_CPU);
+ cpu_watchpoint_remove_all(env, BP_CPU);
}
void cpu_x86_close(CPUX86State *env)
@@ -1267,4 +1244,87 @@ target_phys_addr_t cpu_get_phys_page_deb
paddr = (pte & TARGET_PAGE_MASK) + page_offset;
return paddr;
}
+
+int check_hw_breakpoints(CPUState *env, int force_dr6_update)
+{
+ target_ulong dr6;
+ int reg, type;
+ int hit_enabled = 0;
+
+ dr6 = env->dr[6] & ~0xf;
+ for (reg = 0; reg < 4; reg++) {
+ type = hw_breakpoint_type(env->dr[7], reg);
+ if ((type == 0 && env->dr[reg] == env->eip) ||
+ ((type & 1) && env->cpu_watchpoint[reg] &&
+ (env->cpu_watchpoint[reg]->flags & BP_WATCHPOINT_HIT))) {
+ dr6 |= 1 << reg;
+ if (hw_breakpoint_enabled(env->dr[7], reg))
+ hit_enabled = 1;
+ }
+ }
+ if (hit_enabled || force_dr6_update)
+ env->dr[6] = dr6;
+ return hit_enabled;
+}
+
+static CPUDebugExcpHandler *prev_debug_excp_handler;
+
+void raise_exception(int exception_index);
+
+static void breakpoint_handler(CPUState *env)
+{
+ CPUBreakpoint *bp;
+
+ if (env->watchpoint_hit) {
+ if (env->watchpoint_hit->flags & BP_CPU) {
+ env->watchpoint_hit = NULL;
+ if (check_hw_breakpoints(env, 0))
+ raise_exception(EXCP01_DB);
+ else
+ cpu_resume_from_signal(env, NULL);
+ }
+ } else {
+ for (bp = env->breakpoints; bp != NULL; bp = bp->next)
+ if (bp->pc == env->eip) {
+ if (bp->flags & BP_CPU) {
+ check_hw_breakpoints(env, 1);
+ raise_exception(EXCP01_DB);
+ }
+ break;
+ }
+ }
+ if (prev_debug_excp_handler)
+ prev_debug_excp_handler(env);
+}
#endif /* !CONFIG_USER_ONLY */
+
+CPUX86State *cpu_x86_init(const char *cpu_model)
+{
+ CPUX86State *env;
+ static int inited;
+
+ env = qemu_mallocz(sizeof(CPUX86State));
+ if (!env)
+ return NULL;
+ cpu_exec_init(env);
+ env->cpu_model_str = cpu_model;
+
+ /* init various static stuff */
+ if (!inited) {
+ inited = 1;
+ optimize_flags_init();
+#ifndef CONFIG_USER_ONLY
+ prev_debug_excp_handler =
+ cpu_set_debug_excp_handler(breakpoint_handler);
+#endif
+ }
+ if (cpu_x86_register(env, cpu_model) < 0) {
+ cpu_x86_close(env);
+ return NULL;
+ }
+ cpu_reset(env);
+#ifdef USE_KQEMU
+ kqemu_init(env);
+#endif
+ return env;
+}
Index: b/target-i386/op_helper.c
===================================================================
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -94,6 +94,53 @@ const CPU86_LDouble f15rk[7] =
3.32192809488736234781L, /*l2t*/
};
+static void hw_breakpoint_insert(int index)
+{
+ int type, err = 0;
+
+ switch (hw_breakpoint_type(env->dr[7], index)) {
+ case 0:
+ if (hw_breakpoint_enabled(env->dr[7], index))
+ err = cpu_breakpoint_insert(env, env->dr[index], BP_CPU,
+ &env->cpu_breakpoint[index]);
+ break;
+ case 1:
+ type = BP_CPU | BP_MEM_WRITE;
+ goto insert_wp;
+ case 2:
+ /* No support for I/O watchpoints yet */
+ break;
+ case 3:
+ type = BP_CPU | BP_MEM_ACCESS;
+ insert_wp:
+ err = cpu_watchpoint_insert(env, env->dr[index],
+ hw_breakpoint_len(env->dr[7], index),
+ type, &env->cpu_watchpoint[index]);
+ break;
+ }
+ if (err)
+ env->cpu_breakpoint[index] = NULL;
+}
+
+static void hw_breakpoint_remove(int index)
+{
+ if (!env->cpu_breakpoint[index])
+ return;
+ switch (hw_breakpoint_type(env->dr[7], index)) {
+ case 0:
+ if (hw_breakpoint_enabled(env->dr[7], index))
+ cpu_breakpoint_remove_by_ref(env, env->cpu_breakpoint[index]);
+ break;
+ case 1:
+ case 3:
+ cpu_watchpoint_remove_by_ref(env, env->cpu_watchpoint[index]);
+ break;
+ case 2:
+ /* No support for I/O watchpoints yet */
+ break;
+ }
+}
+
/* broken thread support */
spinlock_t global_cpu_lock = SPIN_LOCK_UNLOCKED;
@@ -496,6 +543,15 @@ static void switch_tss(int tss_selector,
/* XXX: different exception if CALL ? */
raise_exception_err(EXCP0D_GPF, 0);
}
+
+ /* reset local breakpoints */
+ if (env->dr[7] & 0x55) {
+ for (i = 0; i < 4; i++) {
+ if (hw_breakpoint_enabled(env->dr[7], i) == 0x1)
+ hw_breakpoint_remove(i);
+ }
+ env->dr[7] &= ~0x55;
+ }
}
/* check if Port I/O is allowed in TSS */
@@ -1879,8 +1935,11 @@ void helper_cmpxchg16b(target_ulong a0)
void helper_single_step(void)
{
- env->dr[6] |= 0x4000;
- raise_exception(EXCP01_SSTP);
+#ifndef CONFIG_USER_ONLY
+ check_hw_breakpoints(env, 1);
+#endif
+ env->dr[6] |= DR6_BS;
+ raise_exception(EXCP01_DB);
}
void helper_cpuid(void)
@@ -3082,10 +3141,22 @@ void helper_clts(void)
env->hflags &= ~HF_TS_MASK;
}
-/* XXX: do more */
void helper_movl_drN_T0(int reg, target_ulong t0)
{
- env->dr[reg] = t0;
+ int i;
+
+ if (reg < 4) {
+ hw_breakpoint_remove(reg);
+ env->dr[reg] = t0;
+ hw_breakpoint_insert(reg);
+ } else if (reg == 7) {
+ for (i = 0; i < 4; i++)
+ hw_breakpoint_remove(i);
+ env->dr[7] = t0;
+ for (i = 0; i < 4; i++)
+ hw_breakpoint_insert(i);
+ } else
+ env->dr[reg] = t0;
}
void helper_invlpg(target_ulong addr)
^ permalink raw reply [flat|nested] 23+ messages in thread