Enhance debugging support - 2nd take

PATCH Superseded Jan Kiszka 6 participants 41 messages Jul 3, 2008 – Sep 15, 2008

lore

Share & tools

lore

Jan Kiszka

author

2008-07-03 16:04 UTC #0 lore raw

cover Enhance debugging support - 2nd take

Hi,

here comes a rebased and slightly changed version of my debug patch series.

The only deeper change comes with patch 5: Instead of introducing a new single step mode, this patch enables pushing the TB's cflags from an arbitrary context into the next regular TB recompilation - without the need to start that compilation manually. That's a less invasive approach. Originally, I tried to use the cpu_io_recompile pattern for this, but then I realized that the scenarios are actually too different.

To summarize the major contributions of this series:
- Fixes and enhances host-injected watchpoint support
- Enhances internal debugging API
- Fixes SMP guest debugging
- Adds full support for x86 debug registers

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2008-07-03 16:04 UTC #15 lore raw

PATCH 12/13 Introduce BP_CPU as a breakpoint type

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 <[email protected]>

---
 cpu-all.h |    1 +
 exec.c    |   46 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 39 insertions(+), 8 deletions(-)

cpu-all.h +1 -0

Index: b/cpu-all.h
===================================================================
@@ -806,6 +806,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);

exec.c +38 -8

74 lines not on this page · raw message

Jan Kiszka

author

2008-07-03 16:05 UTC #2 lore raw

PATCH 2/13 Refactor and enhance break/watchpoint API

In reply to Jan Kiszka

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 <[email protected]>

---
 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   |   23 ++---
 target-sparc/translate.c |    7 -
 13 files changed, 272 insertions(+), 194 deletions(-)

cpu-all.h +17 -6

Index: b/cpu-all.h
===================================================================
@@ -799,12 +799,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 */

exec.c +121 ~~-80~~

303 lines not on this page · raw message

gdbstub.c +71 ~~-60~~

180 lines not on this page · raw message

cpu-defs.h +16 ~~-10~~

Index: 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;                                      \
                                                                         \
     /* Core interrupt code */                                           \
     jmp_buf jmp_env;                                                    \

target-i386/translate.c +4 -3

Index: b/target-i386/translate.c
===================================================================
@@ -7151,6 +7151,7 @@ static inline int 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;

@@ -7234,9 +7235,9 @@ static inline int 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;
                 }

target-alpha/translate.c +4 -3

Index: b/target-alpha/translate.c
===================================================================
@@ -1981,6 +1981,7 @@ static always_inline int gen_intermediat
     target_ulong pc_start;
     uint32_t insn;
     uint16_t *gen_opc_end;
+    CPUBreakpoint *bp;
     int j, lj = -1;
     int ret;
     int num_insns;

@@ -2003,9 +2004,9 @@ static always_inline int gen_intermediat
 
     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;
                 }

target-arm/translate.c +5 -4

Index: b/target-arm/translate.c
===================================================================
@@ -8549,6 +8549,7 @@ static inline int gen_intermediate_code_
                                                  int search_pc)
 {
     DisasContext dc1, *dc = &dc1;
+    CPUBreakpoint *bp;
     uint16_t *gen_opc_end;
     int j, lj;
     target_ulong pc_start;

@@ -8623,9 +8624,9 @@ static inline int 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);

@@ -8678,7 +8679,7 @@ static inline int 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.

target-cris/translate.c +5 -4

Index: b/target-cris/translate.c
===================================================================
@@ -2973,10 +2973,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);

target-m68k/translate.c +5 -4

Index: b/target-m68k/translate.c
===================================================================
@@ -2917,6 +2917,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;

@@ -2950,9 +2951,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;

@@ -2982,7 +2983,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 &&

target-mips/translate.c +4 -3

Index: b/target-mips/translate.c
===================================================================
@@ -7815,6 +7815,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;

@@ -7855,9 +7856,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);

target-ppc/translate.c +4 -3

Index: b/target-ppc/translate.c
===================================================================
@@ -6180,6 +6180,7 @@ static always_inline int gen_intermediat
     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;

@@ -6234,9 +6235,9 @@ static always_inline int gen_intermediat
     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;

target-sh4/translate.c +12 ~~-11~~

Index: b/target-sh4/translate.c
===================================================================
@@ -1202,6 +1202,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;

@@ -1235,17 +1236,17 @@ 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]) {
-		    /* We have hit a breakpoint - make sure PC is up-to-date */
-		    gen_op_movl_imm_PC(ctx.pc);
-		    gen_op_debug();
-		    ctx.bstate = BS_EXCP;
-		    break;
-		}
-	    }
-	}
+        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 */
+                    gen_op_movl_imm_PC(ctx.pc);
+                    gen_op_debug();
+                    ctx.bstate = BS_EXCP;
+                    break;
+                }
+            }
+        }
         if (search_pc) {
             i = gen_opc_ptr - gen_opc_buf;
             if (ii < i) {

target-sparc/translate.c +4 -3

Index: b/target-sparc/translate.c
===================================================================
@@ -4720,6 +4720,7 @@ static inline int 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;

@@ -4757,9 +4758,9 @@ static inline int 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);

Jan Kiszka

author

2008-07-03 16:05 UTC #1 lore raw

PATCH 1/13 Return appropriate watch message to gdb

In reply to Jan Kiszka

Return the appropriate type prefix (r, a, none) when reporting watchpoint hits to the gdb front-end.

Signed-off-by Jan Kiszka <[email protected]>

---
 gdbstub.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

gdbstub.c +15 -2

Index: b/gdbstub.c
===================================================================
@@ -1225,6 +1225,7 @@ static void gdb_vm_stopped(void *opaque,
 {
     GDBState *s = opaque;
     char buf[256];
+    const char *type;
     int ret;
 
     if (s->state == RS_SYSCALL)

@@ -1235,8 +1236,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;

Jan Kiszka

author

2008-07-03 16:05 UTC #11 lore raw

PATCH 8/13 Remove premature memop TB terminations

In reply to Jan Kiszka

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 <[email protected]>

---
 exec.c                  |    4 ----
 target-arm/translate.c  |    6 ------
 target-m68k/translate.c |    6 ------
 3 files changed, 16 deletions(-)

exec.c +0 -4

Index: b/exec.c
===================================================================
@@ -1257,10 +1257,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;

target-arm/translate.c +0 -6

Index: b/target-arm/translate.c
===================================================================
@@ -8676,12 +8676,6 @@ static inline int 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

target-m68k/translate.c +0 -6

Index: b/target-m68k/translate.c
===================================================================
@@ -2979,12 +2979,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) &&

Jan Kiszka

author

2008-07-03 16:05 UTC #13 lore raw

PATCH 10/13 Introduce BP_WATCHPOINT_HIT flag

In reply to Jan Kiszka

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 <[email protected]>

---
 cpu-all.h  |    1 +
 cpu-exec.c |   11 +++++++++++
 exec.c     |   31 ++++++++++++++++++-------------
 3 files changed, 30 insertions(+), 13 deletions(-)

cpu-all.h +1 -0

Index: b/cpu-all.h
===================================================================
@@ -804,6 +804,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,

cpu-exec.c +11 -0

Index: b/cpu-exec.c
===================================================================
@@ -232,6 +232,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)

@@ -286,6 +295,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

exec.c +18 ~~-13~~

Index: b/exec.c
===================================================================
@@ -1272,7 +1272,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;
         }

@@ -2396,19 +2396,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;
         }
     }
 }

Jan Kiszka

author

2008-07-03 16:05 UTC #14 lore raw

PATCH 11/13 Add debug exception hook

In reply to Jan Kiszka

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 <[email protected]>

---
 cpu-exec.c |   13 +++++++++++++
 exec-all.h |    4 ++++
 2 files changed, 17 insertions(+)

cpu-exec.c +13 -0

Index: b/cpu-exec.c
===================================================================
@@ -232,6 +232,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;

@@ -239,6 +249,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 */

exec-all.h +4 -0

Index: b/exec-all.h
===================================================================
@@ -418,3 +418,7 @@ static inline int kqemu_is_ok(CPUState *
 }
 
 #endif
+
+typedef void (CPUDebugExcpHandler)(CPUState *env);
+
+CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler);

Jan Kiszka

author

2008-07-03 16:05 UTC #7 lore raw

PATCH 6/13 Switch self-modified code recompilation to next_cflags

In reply to Jan Kiszka

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 <[email protected]>

---
 exec.c |   32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

exec.c +4 ~~-28~~

Index: b/exec.c
===================================================================
@@ -831,12 +831,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)

@@ -853,9 +852,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;

@@ -892,14 +888,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

@@ -933,7 +921,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

@@ -972,8 +960,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

@@ -987,9 +974,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);

@@ -1009,14 +993,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);

@@ -1029,7 +1005,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

Jan Kiszka

author

2008-07-03 16:05 UTC #12 lore raw

PATCH 9/13 Improve debugging of SMP guests

In reply to Jan Kiszka

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

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

Signed-off-by Jan Kiszka <[email protected]>

---
 gdbstub.c |   85 ++++++++++++++++++++++++++++++++++++++++----------------------
 monitor.c |   19 ++++++++-----
 monitor.h |   15 ++++++++++
 vl.c      |    2 +
 4 files changed, 85 insertions(+), 36 deletions(-)

gdbstub.c +56 ~~-29~~

Index: b/gdbstub.c
===================================================================
@@ -34,6 +34,7 @@
 #include "sysemu.h"
 #include "gdbstub.h"
 #endif
+#include "monitor.h"
 
 #include "qemu_socket.h"
 #ifdef _WIN32

@@ -58,7 +59,6 @@ enum RSState {
     RS_SYSCALL,
 };
 typedef struct GDBState {
-    CPUState *env; /* current CPU */
     enum RSState state; /* parsing state */
     char line_buf[4096];
     int line_buf_index;

@@ -977,43 +977,71 @@ const int xlat_gdb_type[] = {
 };
 #endif
 
-static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
-                                 target_ulong len, int type)
+static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type)
 {
+    CPUState *env;
+    int err = 0;
+
     switch (type) {
     case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
-        return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+            if (err)
+                break;
+        }
+        return err;
 #ifndef CONFIG_USER_ONLY
     case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
-        return cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
-                                     NULL);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
+                                        NULL);
+            if (err)
+                break;
+        }
+        return err;
 #endif
     default:
         return -ENOSYS;
     }
 }
 
-static int gdb_breakpoint_remove(CPUState *env, target_ulong addr,
-                                 target_ulong len, int type)
+static int gdb_breakpoint_remove(target_ulong addr, target_ulong len, int type)
 {
+    CPUState *env;
+    int err = 0;
+
     switch (type) {
     case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
-        return cpu_breakpoint_remove(env, addr, BP_GDB);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_breakpoint_remove(env, addr, BP_GDB);
+            if (err)
+                break;
+        }
+        return err;
 #ifndef CONFIG_USER_ONLY
     case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
-        return cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+            if (err)
+                break;
+        }
+        return err;
 #endif
     default:
         return -ENOSYS;
     }
 }
 
-static void gdb_breakpoint_remove_all(CPUState *env)
+static void gdb_breakpoint_remove_all(void)
 {
-    cpu_breakpoint_remove_all(env, BP_GDB);
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        cpu_breakpoint_remove_all(env, BP_GDB);
 #ifndef CONFIG_USER_ONLY
-    cpu_watchpoint_remove_all(env, BP_GDB);
+        cpu_watchpoint_remove_all(env, BP_GDB);
 #endif
+    }
 }
 
 static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)

@@ -1039,7 +1067,7 @@ static int gdb_handle_packet(GDBState *s
          * because gdb is doing and initial connect and the state
          * should be cleaned up.
          */
-        gdb_breakpoint_remove_all(env);
+        gdb_breakpoint_remove_all();
         break;
     case 'c':
         if (*p != '\0') {

@@ -1073,7 +1101,7 @@ static int gdb_handle_packet(GDBState *s
         exit(0);
     case 'D':
         /* Detach packet */
-        gdb_breakpoint_remove_all(env);
+        gdb_breakpoint_remove_all();
         gdb_continue(s);
         put_packet(s, "OK");
         break;

@@ -1116,7 +1144,7 @@ static int gdb_handle_packet(GDBState *s
                 p++;
             type = *p;
             if (gdb_current_syscall_cb)
-                gdb_current_syscall_cb(s->env, ret, err);
+                gdb_current_syscall_cb(mon_get_cpu(), ret, err);
             if (type == 'C') {
                 put_packet(s, "T02");
             } else {

@@ -1171,9 +1199,9 @@ static int gdb_handle_packet(GDBState *s
             p++;
         len = strtoull(p, (char **)&p, 16);
         if (ch == 'Z')
-            res = gdb_breakpoint_insert(env, addr, len, type);
+            res = gdb_breakpoint_insert(addr, len, type);
         else
-            res = gdb_breakpoint_remove(env, addr, len, type);
+            res = gdb_breakpoint_remove(addr, len, type);
         if (res >= 0)
              put_packet(s, "OK");
         else if (res == -ENOSYS)

@@ -1237,6 +1265,7 @@ extern void tb_flush(CPUState *env);
 static void gdb_vm_stopped(void *opaque, int reason)
 {
     GDBState *s = opaque;
+    CPUState *env = mon_get_cpu();
     char buf[256];
     const char *type;
     int ret;

@@ -1245,11 +1274,11 @@ static void gdb_vm_stopped(void *opaque,
         return;
 
     /* disable single step if it was enable */
-    cpu_single_step(s->env, 0);
+    cpu_single_step(env, 0);
 
     if (reason == EXCP_DEBUG) {
-        if (s->env->watchpoint_hit) {
-            switch (s->env->watchpoint_hit->flags & BP_MEM_ACCESS) {
+        if (env->watchpoint_hit) {
+            switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
                 type = "r";
                 break;

@@ -1261,12 +1290,12 @@ static void gdb_vm_stopped(void *opaque,
                 break;
             }
             snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
-                     SIGTRAP, type, s->env->watchpoint_hit->vaddr);
+                     SIGTRAP, type, env->watchpoint_hit->vaddr);
             put_packet(s, buf);
-            s->env->watchpoint_hit = NULL;
+            env->watchpoint_hit = NULL;
             return;
         }
-	tb_flush(s->env);
+	tb_flush(env);
         ret = SIGTRAP;
     } else if (reason == EXCP_INTERRUPT) {
         ret = SIGINT;

@@ -1336,15 +1365,15 @@ void gdb_do_syscall(gdb_syscall_complete
     va_end(va);
     put_packet(s, buf);
 #ifdef CONFIG_USER_ONLY
-    gdb_handlesig(s->env, 0);
+    gdb_handlesig(mon_get_cpu(), 0);
 #else
-    cpu_interrupt(s->env, CPU_INTERRUPT_EXIT);
+    cpu_interrupt(mon_get_cpu(), CPU_INTERRUPT_EXIT);
 #endif
 }
 
 static void gdb_read_byte(GDBState *s, int ch)
 {
-    CPUState *env = s->env;
+    CPUState *env = mon_get_cpu();
     int i, csum;
     uint8_t reply;

@@ -1508,7 +1537,6 @@ static void gdb_accept(void *opaque)
 
     s = &gdbserver_state;
     memset (s, 0, sizeof (GDBState));
-    s->env = first_cpu; /* XXX: allow to change CPU */
     s->fd = fd;
 
     gdb_syscall_state = s;

@@ -1611,7 +1639,6 @@ int gdbserver_start(const char *port)
     if (!s) {
         return -1;
     }
-    s->env = first_cpu; /* XXX: allow to change CPU */
     s->chr = chr;
     qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
                           gdb_chr_event, s);

monitor.c +12 -7

Index: b/monitor.c
===================================================================
@@ -261,8 +261,7 @@ static void do_info_blockstats(void)
     bdrv_info_stats();
 }
 
-/* get the current CPU defined by the user */
-static int mon_set_cpu(int cpu_index)
+static int mon_set_cpu_index(int cpu_index)
 {
     CPUState *env;

@@ -275,10 +274,16 @@ static int mon_set_cpu(int cpu_index)
     return -1;
 }
 
-static CPUState *mon_get_cpu(void)
+void mon_set_cpu(CPUState *env)
+{
+    mon_cpu = env;
+}
+
+/* get the current CPU defined by the user or by a breakpoint hit */
+CPUState *mon_get_cpu(void)
 {
     if (!mon_cpu) {
-        mon_set_cpu(0);
+        mon_set_cpu(first_cpu);
     }
     return mon_cpu;
 }

@@ -302,8 +307,8 @@ static void do_info_cpus(void)
 {
     CPUState *env;
 
-    /* just to set the default cpu if not already done */
-    mon_get_cpu();
+    if (!mon_cpu)
+        mon_set_cpu(first_cpu);
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
         term_printf("%c CPU #%d:",

@@ -326,7 +331,7 @@ static void do_info_cpus(void)
 
 static void do_cpu_set(int index)
 {
-    if (mon_set_cpu(index) < 0)
+    if (mon_set_cpu_index(index) < 0)
         term_printf("Invalid CPU index\n");
 }

monitor.h new file +15 -0

Index: b/monitor.h
===================================================================
@@ -0,0 +1,15 @@
+#ifndef QEMU_MONITOR_H
+#define QEMU_MONITOR_H
+
+void mon_set_cpu(CPUState *env);
+
+#ifdef CONFIG_USER_ONLY
+static inline CPUState *mon_get_cpu(void)
+{
+    return first_cpu;
+}
+#else
+CPUState *mon_get_cpu(void);
+#endif
+
+#endif /* QEMU_MONITOR_H */

vl.c +2 -0

Index: b/vl.c
===================================================================
@@ -33,6 +33,7 @@
 #include "console.h"
 #include "sysemu.h"
 #include "gdbstub.h"
+#include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "block.h"

@@ -7257,6 +7258,7 @@ static int main_loop(void)
                 ret = EXCP_INTERRUPT;
             }
             if (unlikely(ret == EXCP_DEBUG)) {
+                mon_set_cpu(cur_cpu);
                 vm_stop(EXCP_DEBUG);
             }
             /* If all cpus are halted then wait until the next IRQ */

Jan Kiszka

author

2008-07-03 16:05 UTC #16 lore raw

PATCH 13/13 x86: Debug register emulation

In reply to Jan Kiszka

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 <[email protected]>

---
 linux-user/main.c       |    4 -
 target-i386/cpu.h       |   33 +++++++++++++
 target-i386/helper.c    |  118 ++++++++++++++++++++++++++++++++++++------------
 target-i386/op_helper.c |   79 ++++++++++++++++++++++++++++++--
 4 files changed, 198 insertions(+), 36 deletions(-)

linux-user/main.c +2 -2

Index: b/linux-user/main.c
===================================================================
@@ -403,7 +403,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) {

@@ -413,7 +413,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 {

target-i386/cpu.h +32 -1

Index: 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

@@ -334,7 +344,7 @@
 #define CPUID_EXT3_SKINIT  (1 << 12)
 
 #define EXCP00_DIVZ	0
-#define EXCP01_SSTP	1
+#define EXCP01_DB	1
 #define EXCP02_NMI	2
 #define EXCP03_INT3	3
 #define EXCP04_INTO	4

@@ -566,6 +576,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 */

@@ -755,6 +769,23 @@ 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)
+{
+    return ((dr7 >> (DR7_LEN_SHIFT + (index * 2))) & 3) + 1;
+}
+
+int check_hw_breakpoints(CPUState *env, int force_dr6_update);
+
 #include "cpu-all.h"
 
 #include "svm.h"

target-i386/helper.c +89 ~~-29~~

140 lines not on this page · raw message

target-i386/op_helper.c +75 -4

105 lines not on this page · raw message

Jan Kiszka

author

2008-07-03 16:05 UTC #4 lore raw

PATCH 4/13 Respect length of watchpoints

In reply to Jan Kiszka

This adds length support for watchpoints. To keep things simple, only aligned watchpoints are accepted.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpu-defs.h |    2 +-
 exec.c     |   28 ++++++++++++++++++----------
 2 files changed, 19 insertions(+), 11 deletions(-)

exec.c +18 ~~-10~~

Index: b/exec.c
===================================================================
@@ -1259,14 +1259,19 @@ 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) || (addr & ~len_mask))
+        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;

@@ -1290,10 +1295,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;
         }

@@ -2399,7 +2406,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;

@@ -2407,7 +2414,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;

@@ -2420,40 +2428,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);
 }

cpu-defs.h +1 -1

Index: 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;

Jan Kiszka

author

2008-07-03 16:05 UTC #8 lore raw

PATCH 7/13 Restore pc on watchpoint hits

In reply to Jan Kiszka

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 <[email protected]>

---
 cpu-all.h |    1 +
 exec.c    |   22 ++++++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

exec.c +20 -2

Index: b/exec.c
===================================================================
@@ -2385,16 +2385,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);
         }
     }
 }

cpu-all.h +1 -0

Index: b/cpu-all.h
===================================================================
@@ -803,6 +803,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,

Jan Kiszka

author

2008-07-03 16:05 UTC #3 lore raw

PATCH 3/13 Set mem_io_vaddr on io_read

In reply to Jan Kiszka

Required for read watchpoints.

Signed-off-by Jan Kiszka <[email protected]>

---
 softmmu_template.h |    1 +
 1 file changed, 1 insertion(+)

softmmu_template.h +1 -0

Index: 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

Jan Kiszka

author

2008-07-03 16:05 UTC #5 lore raw

PATCH 5/13 Introduce next_cflags

In reply to Jan Kiszka

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.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpu-defs.h |    4 ++++
 cpu-exec.c |    3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

cpu-defs.h +4 -0

Index: 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;                                         \

cpu-exec.c +2 -1

Index: 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 */

Paul Brook

2008-07-04 20:49 UTC #9 lore raw

Re: [Qemu-devel] [PATCH 7/13] Restore pc on watchpoint hits

In reply to Jan Kiszka

Quoted 5 lines · Jan Kiszka

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.

I'm fairly sure this won't work. What happens when an interrupt occurs?

Paul

Jan Kiszka

2008-07-04 21:15 UTC #10 lore raw

[Qemu-devel] Re: [PATCH 7/13] Restore pc on watchpoint hits

Quoted 8 lines · Paul Brook

Quoted 5 lines · Jan Kiszka
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.
I'm fairly sure this won't work. What happens when an interrupt occurs?

When in interrupt is raised before the watchpoint is re-triggered (after restore), it might be handled first - hmm... Maybe this has to be prevented to avoid inconsistencies (ie. no guest IRQ delivery when watchpoints are pending). Will check again and update the patch.

Jan

Jan Kiszka

2008-07-04 22:03 UTC #6 lore raw

PATCH 5/13 Introduce next_cflags - v2

In reply to Jan Kiszka

[ Paul, this should close the interrupt window you were concerned about. ]

To avoid that next_cflags hits the wrong TB, interrupt delivery is suppressed while this field is non-zero.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpu-defs.h |    4 ++++
 cpu-exec.c |    9 +++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

cpu-defs.h +4 -0

Index: 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;                                         \

cpu-exec.c +7 -2

Index: 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 */

@@ -360,8 +361,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;

Jan Kiszka

author

2008-07-11 10:40 UTC #17 lore raw

In reply to Jan Kiszka

Quoted 11 lines · Jan Kiszka

Hi,
here comes a rebased and slightly changed version of my debug patch series.
The only deeper change comes with patch 5: Instead of introducing a new single step mode, this patch enables pushing the TB's cflags from an arbitrary context into the next regular TB recompilation - without the need to start that compilation manually. That's a less invasive approach. Originally, I tried to use the cpu_io_recompile pattern for this, but then I realized that the scenarios are actually too different.

Quoted 6 lines

To summarize the major contributions of this series:
- Fixes and enhances host-injected watchpoint support
- Enhances internal debugging API
- Fixes SMP guest debugging
- Adds full support for x86 debug registers

May I repeat my request for comments, suggestions, wishes regarding this series? If there are concerns, even vague ones, please let us discuss them. I think the last one raised by Paul should be solved by the update I sent. If you don't think so, share your concerns.

If there are no more concern, then it would be nice to merge the series and test things further this way. You can be sure I won't run away once all bits are in, I'm most interested in a stable and clean solution.

See, we (CT as well as our customer) would be more happy to spend the currently available resources on improving the patches than on maintaining them out of tree (specifically as we need them in KVM as well).

Thanks,
Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2008-08-20 15:18 UTC #18 lore raw

cover Enhance debugging support - 2nd take

In reply to Jan Kiszka

Quoted 11 lines · Jan Kiszka

Hi,
here comes a rebased and slightly changed version of my debug patch series.
The only deeper change comes with patch 5: Instead of introducing a new single step mode, this patch enables pushing the TB's cflags from an arbitrary context into the next regular TB recompilation - without the need to start that compilation manually. That's a less invasive approach. Originally, I tried to use the cpu_io_recompile pattern for this, but then I realized that the scenarios are actually too different.

Quoted 5 lines

To summarize the major contributions of this series:
- Fixes and enhances host-injected watchpoint support
- Enhances internal debugging API
- Fixes SMP guest debugging
- Adds full support for x86 debug registers

No changes since the last round, just a refresh against latest SVN - and a reminder for the maintainers to have a look at this series.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2008-08-20 15:21 UTC #28 lore raw

PATCH RESEND 3/13 Set mem_io_vaddr on io_read

Required for read watchpoints.

Signed-off-by Jan Kiszka <[email protected]>

---
 softmmu_template.h |    1 +
 1 file changed, 1 insertion(+)

softmmu_template.h +1 -0

Index: 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

Jan Kiszka

author

2008-08-20 15:21 UTC #29 lore raw

PATCH RESEND 4/13 Respect length of watchpoints

In reply to Jan Kiszka

This adds length support for watchpoints. To keep things simple, only aligned watchpoints are accepted.

Signed-off-by Jan Kiszka <[email protected]>

---
 cpu-defs.h |    2 +-
 exec.c     |   28 ++++++++++++++++++----------
 2 files changed, 19 insertions(+), 11 deletions(-)

exec.c +18 ~~-10~~

Index: b/exec.c
===================================================================
@@ -1279,14 +1279,19 @@ 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) || (addr & ~len_mask))
+        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;

@@ -1310,10 +1315,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;
         }

@@ -2419,7 +2426,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;

@@ -2427,7 +2434,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;

@@ -2440,40 +2448,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);
 }

cpu-defs.h +1 -1

Index: 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;

Jan Kiszka

author

2008-08-20 15:21 UTC #19 lore raw

PATCH RESEND 1/13 Return appropriate watch message to gdb

In reply to Jan Kiszka

Return the appropriate type prefix (r, a, none) when reporting watchpoint hits to the gdb front-end.

Signed-off-by Jan Kiszka <[email protected]>

---
 gdbstub.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

gdbstub.c +15 -2

Index: b/gdbstub.c
===================================================================
@@ -1225,6 +1225,7 @@ static void gdb_vm_stopped(void *opaque,
 {
     GDBState *s = opaque;
     char buf[256];
+    const char *type;
     int ret;
 
     if (s->state == RS_SYSCALL)

@@ -1235,8 +1236,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;

Jan Kiszka

author

2008-08-20 15:21 UTC #20 lore raw

PATCH RESEND 2/13 Refactor and enhance break/watchpoint API

In reply to Jan Kiszka

This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow the succeeding enhancements this series comes with.

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.

Signed-off-by Jan Kiszka <[email protected]>

---
 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   |   23 ++---
 target-sparc/translate.c |    7 -
 13 files changed, 272 insertions(+), 194 deletions(-)

cpu-all.h +17 -6

Index: b/cpu-all.h
===================================================================
@@ -799,12 +799,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 */

exec.c +121 ~~-80~~

303 lines not on this page · raw message

gdbstub.c +71 ~~-60~~

180 lines not on this page · raw message

cpu-defs.h +16 ~~-10~~

Index: 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;                                      \
                                                                         \
     /* Core interrupt code */                                           \
     jmp_buf jmp_env;                                                    \

target-i386/translate.c +4 -3

Index: b/target-i386/translate.c
===================================================================
@@ -7155,6 +7155,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;

@@ -7238,9 +7239,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;
                 }

target-alpha/translate.c +4 -3

Index: b/target-alpha/translate.c
===================================================================
@@ -1981,6 +1981,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;

@@ -2003,9 +2004,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;
                 }

target-arm/translate.c +5 -4

Index: b/target-arm/translate.c
===================================================================
@@ -8554,6 +8554,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;

@@ -8630,9 +8631,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);

@@ -8685,7 +8686,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.

target-cris/translate.c +5 -4

Index: b/target-cris/translate.c
===================================================================
@@ -2973,10 +2973,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);

target-m68k/translate.c +5 -4

Index: b/target-m68k/translate.c
===================================================================
@@ -2917,6 +2917,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;

@@ -2950,9 +2951,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;

@@ -2982,7 +2983,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 &&

target-mips/translate.c +4 -3

Index: b/target-mips/translate.c
===================================================================
@@ -8459,6 +8459,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;

@@ -8498,9 +8499,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);

target-ppc/translate.c +4 -3

Index: b/target-ppc/translate.c
===================================================================
@@ -6181,6 +6181,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;

@@ -6235,9 +6236,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;

target-sh4/translate.c +12 ~~-11~~

Index: b/target-sh4/translate.c
===================================================================
@@ -1202,6 +1202,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;

@@ -1235,17 +1236,17 @@ 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]) {
-		    /* We have hit a breakpoint - make sure PC is up-to-date */
-		    gen_op_movl_imm_PC(ctx.pc);
-		    gen_op_debug();
-		    ctx.bstate = BS_EXCP;
-		    break;
-		}
-	    }
-	}
+        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 */
+                    gen_op_movl_imm_PC(ctx.pc);
+                    gen_op_debug();
+                    ctx.bstate = BS_EXCP;
+                    break;
+                }
+            }
+        }
         if (search_pc) {
             i = gen_opc_ptr - gen_opc_buf;
             if (ii < i) {

target-sparc/translate.c +4 -3

Index: b/target-sparc/translate.c
===================================================================
@@ -4731,6 +4731,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;

@@ -4771,9 +4772,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);

Jan Kiszka

author

2008-08-20 15:21 UTC #30 lore raw

PATCH RESEND 5/13 Introduce next_cflags

In reply to Jan Kiszka

Signed-off-by Jan Kiszka <[email protected]>

---
 cpu-defs.h |    4 ++++
 cpu-exec.c |    9 +++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

cpu-defs.h +4 -0

Index: 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;                                         \

cpu-exec.c +7 -2

Index: 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 */

@@ -361,8 +362,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;

Jan Kiszka

author

2008-08-20 15:21 UTC #33 lore raw

PATCH RESEND 8/13 Remove premature memop TB terminations

In reply to Jan Kiszka

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 <[email protected]>

---
 exec.c                  |    4 ----
 target-arm/translate.c  |    6 ------
 target-m68k/translate.c |    6 ------
 3 files changed, 16 deletions(-)

exec.c +0 -4

Index: b/exec.c
===================================================================
@@ -1277,10 +1277,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;

target-arm/translate.c +0 -6

Index: b/target-arm/translate.c
===================================================================
@@ -8683,12 +8683,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

target-m68k/translate.c +0 -6

Index: b/target-m68k/translate.c
===================================================================
@@ -2979,12 +2979,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) &&

Jan Kiszka

author

2008-08-20 15:21 UTC #35 lore raw

PATCH RESEND 10/13 Introduce BP_WATCHPOINT_HIT flag

In reply to Jan Kiszka

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 <[email protected]>

---
 cpu-all.h  |    1 +
 cpu-exec.c |   11 +++++++++++
 exec.c     |   31 ++++++++++++++++++-------------
 3 files changed, 30 insertions(+), 13 deletions(-)

cpu-all.h +1 -0

Index: b/cpu-all.h
===================================================================
@@ -804,6 +804,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,

cpu-exec.c +11 -0

Index: b/cpu-exec.c
===================================================================
@@ -233,6 +233,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)

@@ -287,6 +296,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

exec.c +18 ~~-13~~

Index: b/exec.c
===================================================================
@@ -1292,7 +1292,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;
         }

@@ -2416,19 +2416,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;
         }
     }
 }

Jan Kiszka

author

2008-08-20 15:21 UTC #32 lore raw

PATCH RESEND 7/13 Restore pc on watchpoint hits

In reply to Jan Kiszka

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 <[email protected]>

---
 cpu-all.h |    1 +
 exec.c    |   22 ++++++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

exec.c +20 -2

Index: b/exec.c
===================================================================
@@ -2405,16 +2405,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);
         }
     }
 }

cpu-all.h +1 -0

Index: b/cpu-all.h
===================================================================
@@ -803,6 +803,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,

Jan Kiszka

author

2008-08-20 15:21 UTC #36 lore raw

PATCH RESEND 11/13 Add debug exception hook

In reply to Jan Kiszka

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 <[email protected]>

---
 cpu-exec.c |   13 +++++++++++++
 exec-all.h |    4 ++++
 2 files changed, 17 insertions(+)

cpu-exec.c +13 -0

Index: b/cpu-exec.c
===================================================================
@@ -233,6 +233,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;

@@ -240,6 +250,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 */

exec-all.h +4 -0

Index: b/exec-all.h
===================================================================
@@ -388,3 +388,7 @@ static inline int kqemu_is_ok(CPUState *
 }
 
 #endif
+
+typedef void (CPUDebugExcpHandler)(CPUState *env);
+
+CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler);

Jan Kiszka

author

2008-08-20 15:21 UTC #38 lore raw

PATCH RESEND 13/13 x86: Debug register emulation

In reply to Jan Kiszka

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 <[email protected]>

---
 linux-user/main.c       |    4 -
 target-i386/cpu.h       |   33 +++++++++++++
 target-i386/helper.c    |  118 ++++++++++++++++++++++++++++++++++++------------
 target-i386/op_helper.c |   79 ++++++++++++++++++++++++++++++--
 4 files changed, 198 insertions(+), 36 deletions(-)

linux-user/main.c +2 -2

Index: 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 {

target-i386/cpu.h +32 -1

Index: 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

@@ -334,7 +344,7 @@
 #define CPUID_EXT3_SKINIT  (1 << 12)
 
 #define EXCP00_DIVZ	0
-#define EXCP01_SSTP	1
+#define EXCP01_DB	1
 #define EXCP02_NMI	2
 #define EXCP03_INT3	3
 #define EXCP04_INTO	4

@@ -566,6 +576,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 */

@@ -755,6 +769,23 @@ 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)
+{
+    return ((dr7 >> (DR7_LEN_SHIFT + (index * 2))) & 3) + 1;
+}
+
+int check_hw_breakpoints(CPUState *env, int force_dr6_update);
+
 #include "cpu-all.h"
 
 #include "svm.h"

target-i386/helper.c +89 ~~-29~~

140 lines not on this page · raw message

target-i386/op_helper.c +75 -4

105 lines not on this page · raw message

Jan Kiszka

author

2008-08-20 16:18 UTC #31 lore raw

PATCH RESEND 6/13 Switch self-modified code recompilation to next_cflags

In reply to Jan Kiszka

Signed-off-by Jan Kiszka <[email protected]>

---
 exec.c |   32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

exec.c +4 ~~-28~~

Index: b/exec.c
===================================================================
@@ -851,12 +851,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)

@@ -873,9 +872,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;

@@ -912,14 +908,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

@@ -953,7 +941,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

@@ -992,8 +980,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

@@ -1007,9 +994,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);

@@ -1029,14 +1013,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);

@@ -1049,7 +1025,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

Jan Kiszka

author

2008-08-20 16:21 UTC #34 lore raw

PATCH RESEND 9/13 Improve debugging of SMP guests

In reply to Jan Kiszka

Signed-off-by Jan Kiszka <[email protected]>

---
 gdbstub.c |   85 ++++++++++++++++++++++++++++++++++++++++----------------------
 monitor.c |   19 ++++++++-----
 monitor.h |   15 ++++++++++
 vl.c      |    2 +
 4 files changed, 85 insertions(+), 36 deletions(-)

gdbstub.c +56 ~~-29~~

Index: b/gdbstub.c
===================================================================
@@ -34,6 +34,7 @@
 #include "sysemu.h"
 #include "gdbstub.h"
 #endif
+#include "monitor.h"
 
 #include "qemu_socket.h"
 #ifdef _WIN32

@@ -58,7 +59,6 @@ enum RSState {
     RS_SYSCALL,
 };
 typedef struct GDBState {
-    CPUState *env; /* current CPU */
     enum RSState state; /* parsing state */
     char line_buf[4096];
     int line_buf_index;

@@ -977,43 +977,71 @@ const int xlat_gdb_type[] = {
 };
 #endif
 
-static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
-                                 target_ulong len, int type)
+static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type)
 {
+    CPUState *env;
+    int err = 0;
+
     switch (type) {
     case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
-        return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+            if (err)
+                break;
+        }
+        return err;
 #ifndef CONFIG_USER_ONLY
     case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
-        return cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
-                                     NULL);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
+                                        NULL);
+            if (err)
+                break;
+        }
+        return err;
 #endif
     default:
         return -ENOSYS;
     }
 }
 
-static int gdb_breakpoint_remove(CPUState *env, target_ulong addr,
-                                 target_ulong len, int type)
+static int gdb_breakpoint_remove(target_ulong addr, target_ulong len, int type)
 {
+    CPUState *env;
+    int err = 0;
+
     switch (type) {
     case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
-        return cpu_breakpoint_remove(env, addr, BP_GDB);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_breakpoint_remove(env, addr, BP_GDB);
+            if (err)
+                break;
+        }
+        return err;
 #ifndef CONFIG_USER_ONLY
     case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
-        return cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+            if (err)
+                break;
+        }
+        return err;
 #endif
     default:
         return -ENOSYS;
     }
 }
 
-static void gdb_breakpoint_remove_all(CPUState *env)
+static void gdb_breakpoint_remove_all(void)
 {
-    cpu_breakpoint_remove_all(env, BP_GDB);
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        cpu_breakpoint_remove_all(env, BP_GDB);
 #ifndef CONFIG_USER_ONLY
-    cpu_watchpoint_remove_all(env, BP_GDB);
+        cpu_watchpoint_remove_all(env, BP_GDB);
 #endif
+    }
 }
 
 static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)

@@ -1039,7 +1067,7 @@ static int gdb_handle_packet(GDBState *s
          * because gdb is doing and initial connect and the state
          * should be cleaned up.
          */
-        gdb_breakpoint_remove_all(env);
+        gdb_breakpoint_remove_all();
         break;
     case 'c':
         if (*p != '\0') {

@@ -1073,7 +1101,7 @@ static int gdb_handle_packet(GDBState *s
         exit(0);
     case 'D':
         /* Detach packet */
-        gdb_breakpoint_remove_all(env);
+        gdb_breakpoint_remove_all();
         gdb_continue(s);
         put_packet(s, "OK");
         break;

@@ -1116,7 +1144,7 @@ static int gdb_handle_packet(GDBState *s
                 p++;
             type = *p;
             if (gdb_current_syscall_cb)
-                gdb_current_syscall_cb(s->env, ret, err);
+                gdb_current_syscall_cb(mon_get_cpu(), ret, err);
             if (type == 'C') {
                 put_packet(s, "T02");
             } else {

@@ -1171,9 +1199,9 @@ static int gdb_handle_packet(GDBState *s
             p++;
         len = strtoull(p, (char **)&p, 16);
         if (ch == 'Z')
-            res = gdb_breakpoint_insert(env, addr, len, type);
+            res = gdb_breakpoint_insert(addr, len, type);
         else
-            res = gdb_breakpoint_remove(env, addr, len, type);
+            res = gdb_breakpoint_remove(addr, len, type);
         if (res >= 0)
              put_packet(s, "OK");
         else if (res == -ENOSYS)

@@ -1237,6 +1265,7 @@ extern void tb_flush(CPUState *env);
 static void gdb_vm_stopped(void *opaque, int reason)
 {
     GDBState *s = opaque;
+    CPUState *env = mon_get_cpu();
     char buf[256];
     const char *type;
     int ret;

@@ -1245,11 +1274,11 @@ static void gdb_vm_stopped(void *opaque,
         return;
 
     /* disable single step if it was enable */
-    cpu_single_step(s->env, 0);
+    cpu_single_step(env, 0);
 
     if (reason == EXCP_DEBUG) {
-        if (s->env->watchpoint_hit) {
-            switch (s->env->watchpoint_hit->flags & BP_MEM_ACCESS) {
+        if (env->watchpoint_hit) {
+            switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
                 type = "r";
                 break;

@@ -1261,12 +1290,12 @@ static void gdb_vm_stopped(void *opaque,
                 break;
             }
             snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
-                     SIGTRAP, type, s->env->watchpoint_hit->vaddr);
+                     SIGTRAP, type, env->watchpoint_hit->vaddr);
             put_packet(s, buf);
-            s->env->watchpoint_hit = NULL;
+            env->watchpoint_hit = NULL;
             return;
         }
-	tb_flush(s->env);
+	tb_flush(env);
         ret = SIGTRAP;
     } else if (reason == EXCP_INTERRUPT) {
         ret = SIGINT;

@@ -1336,15 +1365,15 @@ void gdb_do_syscall(gdb_syscall_complete
     va_end(va);
     put_packet(s, buf);
 #ifdef CONFIG_USER_ONLY
-    gdb_handlesig(s->env, 0);
+    gdb_handlesig(mon_get_cpu(), 0);
 #else
-    cpu_interrupt(s->env, CPU_INTERRUPT_EXIT);
+    cpu_interrupt(mon_get_cpu(), CPU_INTERRUPT_EXIT);
 #endif
 }
 
 static void gdb_read_byte(GDBState *s, int ch)
 {
-    CPUState *env = s->env;
+    CPUState *env = mon_get_cpu();
     int i, csum;
     uint8_t reply;

@@ -1508,7 +1537,6 @@ static void gdb_accept(void *opaque)
 
     s = &gdbserver_state;
     memset (s, 0, sizeof (GDBState));
-    s->env = first_cpu; /* XXX: allow to change CPU */
     s->fd = fd;
 
     gdb_syscall_state = s;

@@ -1611,7 +1639,6 @@ int gdbserver_start(const char *port)
     if (!s) {
         return -1;
     }
-    s->env = first_cpu; /* XXX: allow to change CPU */
     s->chr = chr;
     qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
                           gdb_chr_event, s);

monitor.c +12 -7

Index: b/monitor.c
===================================================================
@@ -263,8 +263,7 @@ static void do_info_blockstats(void)
     bdrv_info_stats();
 }
 
-/* get the current CPU defined by the user */
-static int mon_set_cpu(int cpu_index)
+static int mon_set_cpu_index(int cpu_index)
 {
     CPUState *env;

@@ -277,10 +276,16 @@ static int mon_set_cpu(int cpu_index)
     return -1;
 }
 
-static CPUState *mon_get_cpu(void)
+void mon_set_cpu(CPUState *env)
+{
+    mon_cpu = env;
+}
+
+/* get the current CPU defined by the user or by a breakpoint hit */
+CPUState *mon_get_cpu(void)
 {
     if (!mon_cpu) {
-        mon_set_cpu(0);
+        mon_set_cpu(first_cpu);
     }
     return mon_cpu;
 }

@@ -304,8 +309,8 @@ static void do_info_cpus(void)
 {
     CPUState *env;
 
-    /* just to set the default cpu if not already done */
-    mon_get_cpu();
+    if (!mon_cpu)
+        mon_set_cpu(first_cpu);
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
         term_printf("%c CPU #%d:",

@@ -328,7 +333,7 @@ static void do_info_cpus(void)
 
 static void do_cpu_set(int index)
 {
-    if (mon_set_cpu(index) < 0)
+    if (mon_set_cpu_index(index) < 0)
         term_printf("Invalid CPU index\n");
 }

monitor.h new file +15 -0

Index: b/monitor.h
===================================================================
@@ -0,0 +1,15 @@
+#ifndef QEMU_MONITOR_H
+#define QEMU_MONITOR_H
+
+void mon_set_cpu(CPUState *env);
+
+#ifdef CONFIG_USER_ONLY
+static inline CPUState *mon_get_cpu(void)
+{
+    return first_cpu;
+}
+#else
+CPUState *mon_get_cpu(void);
+#endif
+
+#endif /* QEMU_MONITOR_H */

vl.c +2 -0

Index: b/vl.c
===================================================================
@@ -33,6 +33,7 @@
 #include "console.h"
 #include "sysemu.h"
 #include "gdbstub.h"
+#include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "block.h"

@@ -7564,6 +7565,7 @@ static int main_loop(void)
                 ret = EXCP_INTERRUPT;
             }
             if (unlikely(ret == EXCP_DEBUG)) {
+                mon_set_cpu(cur_cpu);
                 vm_stop(EXCP_DEBUG);
             }
             /* If all cpus are halted then wait until the next IRQ */

Jan Kiszka

author

2008-08-20 16:21 UTC #37 lore raw

PATCH RESEND 12/13 Introduce BP_CPU as a breakpoint type

In reply to Jan Kiszka

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 <[email protected]>

---
 cpu-all.h |    1 +
 exec.c    |   46 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 39 insertions(+), 8 deletions(-)

cpu-all.h +1 -0

Index: b/cpu-all.h
===================================================================
@@ -806,6 +806,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);

exec.c +38 -8

74 lines not on this page · raw message

Anthony Liguori

2008-08-21 20:20 UTC #39 lore raw

In reply to Jan Kiszka

Quoted 5 lines · Jan Kiszka

Jan Kiszka wrote:
No changes since the last round, just a refresh against latest SVN - and a reminder for the maintainers to have a look at this series.

A lot of long series in the past couple days :-)

Have you addressed all of Fabrice's comments?

Regards,

Anthony Liguori

Quoted 3 lines

Jan

Jan Kiszka

2008-08-21 22:53 UTC #40 lore raw

Quoted 8 lines · Anthony Liguori

Quoted 5 lines · Jan Kiszka
Jan Kiszka wrote:
No changes since the last round, just a refresh against latest SVN -
and
a reminder for the maintainers to have a look at this series.
A lot of long series in the past couple days :-)

Thus substantial progress... :)

Quoted 2 lines

Have you addressed all of Fabrice's comments?

According to my understanding, yes.

The last indications I received from him were some concerns about potential regressions, but nothing concrete I could work with. The one regression an earlier version caused, broken support for self-modifying code, was fixed and successfully tested long ago (concerns patches 5 and 6).

All the debugging features that were changed and improved (guest and host) were tested as well. E.g.: kgdb's self-test nicely starts to work under qemu after patch #13 is applied, host-injected watchpoints were already successfully used for Linux kernel debugging "in the field" (Siemens internal).

Jan

Anthony Liguori

2008-09-07 02:15 UTC #21 lore raw

Re: [Qemu-devel] [RESEND][PATCH 2/13] Refactor and enhance break/watchpoint API

In reply to Jan Kiszka

Does anyone have any objections to this series? It literally touches every architectures so I don't want to commit it without some Ack'ing for the appropriate architecture maintainers.

I've heard a lot of reports recently that gdb support in QEMU for x86 is pretty broken so I think the refactoring is needed.

Regards,

Anthony Liguori

Aurelien Jarno

2008-09-07 19:02 UTC #22 lore raw

Re: [Qemu-devel] [RESEND][PATCH 2/13] Refactor and enhance break/watchpoint API

Quoted 7 lines · Anthony Liguori

Does anyone have any objections to this series? It literally touches every architectures so I don't want to commit it without some Ack'ing for the appropriate architecture maintainers.
I've heard a lot of reports recently that gdb support in QEMU for x86 is pretty broken so I think the refactoring is needed.

After a quick look, it is ok from my side. Note however that the patch changes code that has been changed recently on the SH4 target and probably on the Alpha and PPC ones (TCG conversion). It should not be really difficult to merge the patch by hand though.

Cheers,
Aurelien

Signature · 4 lines

  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net

Blue Swirl

2008-09-07 19:11 UTC #24 lore raw

Re: [Qemu-devel] [RESEND][PATCH 2/13] Refactor and enhance break/watchpoint API

In reply to Anthony Liguori

Quoted 3 lines · Anthony Liguori

Does anyone have any objections to this series? It literally touches every architectures so I don't want to commit it without some Ack'ing for the appropriate architecture maintainers.

Sparc part is OK.

+
+#ifndef CONFIG_USER_ONLY
+const int xlat_gdb_type[] = {
+ [GDB_WATCHPOINT_WRITE] = BP_GDB | BP_MEM_WRITE,

"static" missing.

Jan Kiszka

author

2008-09-08 08:42 UTC #23 lore raw

[Qemu-devel] Re: [RESEND][PATCH 2/13] Refactor and enhance break/watchpoint API

In reply to Aurelien Jarno

Quoted 5 lines · Aurelien Jarno

Quoted 4 lines · Anthony Liguori
Does anyone have any objections to this series? It literally touches every architectures so I don't want to commit it without some Ack'ing for the appropriate architecture maintainers.

Quoted 8 lines

Quoted 3 lines
I've heard a lot of reports recently that gdb support in QEMU for x86 is pretty broken so I think the refactoring is needed.
After a quick look, it is ok from my side. Note however that the patch changes code that has been changed recently on the SH4 target and probably on the Alpha and PPC ones (TCG conversion). It should not be really difficult to merge the patch by hand though.

Will send out a rebased version later (maybe I already have it 'cause I rebased end of last week - I just need to sort out the new compiler warnings I get with today's SVN head).

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jan Kiszka

author

2008-09-08 08:44 UTC #25 lore raw

[Qemu-devel] Re: [RESEND][PATCH 2/13] Refactor and enhance break/watchpoint API

In reply to Blue Swirl

Quoted 12 lines · Blue Swirl

Quoted 3 lines · Anthony Liguori
Does anyone have any objections to this series? It literally touches every architectures so I don't want to commit it without some Ack'ing for the appropriate architecture maintainers.
Sparc part is OK.
gdbstub.c +4 -0
+
+#ifndef CONFIG_USER_ONLY
+const int xlat_gdb_type[] = {
+    [GDB_WATCHPOINT_WRITE]  = BP_GDB | BP_MEM_WRITE,

Quoted 2 lines

"static" missing.

Yep, thanks, will fix.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Jan Kiszka

2008-09-08 20:07 UTC #27 lore raw

[Qemu-devel] Re: [RESEND][PATCH 2/13] Refactor and enhance break/watchpoint API

In reply to Jan Kiszka

Quoted 12 lines · Jan Kiszka

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.

Quoted 6 lines

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.

Rebased over #5181, made xlat_gdb_type static.

Signed-off-by Jan Kiszka <[email protected]>

---
 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(-)

cpu-all.h +17 -6

Index: b/cpu-all.h
===================================================================
@@ -799,12 +799,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 */

exec.c +121 ~~-80~~

303 lines not on this page · raw message

gdbstub.c +71 ~~-60~~

180 lines not on this page · raw message

cpu-defs.h +16 ~~-10~~

Index: 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;                                      \
                                                                         \
     /* Core interrupt code */                                           \
     jmp_buf jmp_env;                                                    \

target-i386/translate.c +4 -3

Index: b/target-i386/translate.c
===================================================================
@@ -7155,6 +7155,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;

@@ -7238,9 +7239,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;
                 }

target-alpha/translate.c +4 -3

Index: b/target-alpha/translate.c
===================================================================
@@ -1884,6 +1884,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;

@@ -1906,9 +1907,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;
                 }

target-arm/translate.c +5 -4

Index: b/target-arm/translate.c
===================================================================
@@ -8551,6 +8551,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;

@@ -8627,9 +8628,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);

@@ -8682,7 +8683,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.

target-cris/translate.c +5 -4

Index: b/target-cris/translate.c
===================================================================
@@ -2973,10 +2973,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);

target-m68k/translate.c +5 -4

Index: b/target-m68k/translate.c
===================================================================
@@ -2915,6 +2915,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;

@@ -2948,9 +2949,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;

@@ -2980,7 +2981,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 &&

target-mips/translate.c +4 -3

Index: b/target-mips/translate.c
===================================================================
@@ -8458,6 +8458,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;

@@ -8497,9 +8498,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);

target-ppc/translate.c +4 -3

Index: b/target-ppc/translate.c
===================================================================
@@ -6215,6 +6215,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;

@@ -6269,9 +6270,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;

target-sh4/translate.c +4 -3

Index: b/target-sh4/translate.c
===================================================================
@@ -1768,6 +1768,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;

@@ -1801,9 +1802,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);

target-sparc/translate.c +4 -3

Index: b/target-sparc/translate.c
===================================================================
@@ -4681,6 +4681,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;

@@ -4718,9 +4719,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);

Jan Kiszka

author

2008-09-15 13:22 UTC #26 lore raw

[Qemu-devel] Re: [RESEND][PATCH 2/13] Refactor and enhance break/watchpoint API

In reply to Anthony Liguori

Quoted 3 lines · Anthony Liguori

Does anyone have any objections to this series? It literally touches every architectures so I don't want to commit it without some Ack'ing for the appropriate architecture maintainers.

What's the latest status of this? Waiting for Paul's ack? Or is the merge already scheduled for next free time slot?

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux