Debugger enhancements

PATCH In review Jan Kiszka 4 participants 20 messages May 31, 2008 – Jun 1, 2008

lore

Share & tools

lore

Jan Kiszka

author

2008-05-31 13:50 UTC #0 lore raw

cover Debugger enhancements

This patch series strives at improving QEMU /wrt guest debugging - for me a very important use case of this software.

It consists of patches that refactor and enhance the breakpoint/watchpoint support, originally driven by the need to extend QEMU's internal API in order to enhance (real) hardware-assisted debugging in KVM. But as the KVM maintainer refused to merge the core of my patches until the QEMU bits went mainline, I was forced to rebase them immediately :->. In fact, these patches now come with even more added value also for QEMU, ie. full watchpoint support with exact PC addresses.

Moreover, the series contains an old, unmerged patch of mine that improves the debugability of SMP guests. For this submission, I basically rebased and polished it further. I'm using it for months now as it is de facto mandatory for SMP debugging.

Please review!

Jan

Jan Kiszka

author

2008-05-31 13:50 UTC #1 lore raw

PATCH 1/5 Refactor breakpoint API and gdbstub integration

This patch prepares the QEMU cpu_watchpoint/breakpoint API to allow us hooking in with KVM and customizing guest debugging (maybe QEMUAccel should provide appropriate callbacks for this on day).

But - probably more interesting here - it also prepares QEMU to extend its own debugging support, namely allowing to differentiate between watchpoint types and taking their length in account. Moreover, the gdbstub is fixed to not report back unsupported breakpoint types as error.

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

---
 cpu-all.h |   14 ++++++++++----
 exec.c    |   32 ++++++++++++++++++++++----------
 gdbstub.c |   50 +++++++++++++++++++++++++++++---------------------
 3 files changed, 61 insertions(+), 35 deletions(-)

cpu-all.h +10 -4

Index: b/cpu-all.h
===================================================================
@@ -797,11 +797,17 @@ extern CPUState *cpu_single_env;
 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 cpu_watchpoint_remove(CPUState *env, target_ulong addr);
+#define GDB_BREAKPOINT_SW       0
+#define GDB_BREAKPOINT_HW       1
+#define GDB_WATCHPOINT_WRITE    2
+#define GDB_WATCHPOINT_READ     3
+#define GDB_WATCHPOINT_ACCESS   4
+
+int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len, int type);
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len, int type);
 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);
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, target_ulong len, int type);
+int cpu_breakpoint_remove(CPUState *env, target_ulong pc, target_ulong len, int type);
 void cpu_breakpoint_remove_all(CPUState *env);
 
 #define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */

exec.c +22 ~~-10~~

Index: b/exec.c
===================================================================
@@ -1175,16 +1175,22 @@ static void breakpoint_invalidate(CPUSta
 #endif
 
 /* Add a watchpoint.  */
-int  cpu_watchpoint_insert(CPUState *env, target_ulong addr)
+int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
+                          int type)
 {
     int i;
 
+    if (type != GDB_WATCHPOINT_WRITE)
+        return -ENOSYS;
+    if (len != 1 && len != 2 && len != 4)
+        return -EINVAL;
+
     for (i = 0; i < env->nb_watchpoints; i++) {
         if (addr == env->watchpoint[i].vaddr)
             return 0;
     }
     if (env->nb_watchpoints >= MAX_WATCHPOINTS)
-        return -1;
+        return -ENOBUFS;
 
     i = env->nb_watchpoints++;
     env->watchpoint[i].vaddr = addr;

@@ -1197,10 +1203,14 @@ int  cpu_watchpoint_insert(CPUState *env
 }
 
 /* Remove a watchpoint.  */
-int cpu_watchpoint_remove(CPUState *env, target_ulong addr)
+int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
+                          int type)
 {
     int i;
 
+    if (type != GDB_WATCHPOINT_WRITE)
+        return -ENOSYS;
+
     for (i = 0; i < env->nb_watchpoints; i++) {
         if (addr == env->watchpoint[i].vaddr) {
             env->nb_watchpoints--;

@@ -1209,7 +1219,7 @@ int cpu_watchpoint_remove(CPUState *env,
             return 0;
         }
     }
-    return -1;
+    return -ENOENT;
 }
 
 /* Remove all watchpoints. */

@@ -1224,7 +1234,8 @@ void cpu_watchpoint_remove_all(CPUState 
 
 /* add a breakpoint. EXCP_DEBUG is returned by the CPU loop if a
    breakpoint is reached */
-int cpu_breakpoint_insert(CPUState *env, target_ulong pc)
+int cpu_breakpoint_insert(CPUState *env, target_ulong pc, target_ulong len,
+                          int type)
 {
 #if defined(TARGET_HAS_ICE)
     int i;

@@ -1235,13 +1246,13 @@ int cpu_breakpoint_insert(CPUState *env,
     }
 
     if (env->nb_breakpoints >= MAX_BREAKPOINTS)
-        return -1;
+        return -ENOBUFS;
     env->breakpoints[env->nb_breakpoints++] = pc;
 
     breakpoint_invalidate(env, pc);
     return 0;
 #else
-    return -1;
+    return -ENOSYS;
 #endif
 }

@@ -1257,7 +1268,8 @@ void cpu_breakpoint_remove_all(CPUState 
 }
 
 /* remove a breakpoint */
-int cpu_breakpoint_remove(CPUState *env, target_ulong pc)
+int cpu_breakpoint_remove(CPUState *env, target_ulong pc, target_ulong len,
+                          int type)
 {
 #if defined(TARGET_HAS_ICE)
     int i;

@@ -1265,7 +1277,7 @@ int cpu_breakpoint_remove(CPUState *env,
         if (env->breakpoints[i] == pc)
             goto found;
     }
-    return -1;
+    return -ENOENT;
  found:
     env->nb_breakpoints--;
     if (i < env->nb_breakpoints)

@@ -1274,7 +1286,7 @@ int cpu_breakpoint_remove(CPUState *env,
     breakpoint_invalidate(env, pc);
     return 0;
 #else
-    return -1;
+    return -ENOSYS;
 #endif
 }

gdbstub.c +29 ~~-21~~

74 lines not on this page · raw message

Jan Kiszka

author

2008-05-31 13:50 UTC #2 lore raw

PATCH 2/5 Watchpoint length and type awareness

In reply to Jan Kiszka

Extend QEMU's memory watchpoint with length and type awareness. The length parameter is no correctly accounted for. gdbstub is enhanced to report the correct watchpoint type back on hits. To keep things simple, I decided to reject unaligned watchpoints instead of mapping them on multiple instances.

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

---
 cpu-defs.h |    2 ++
 exec.c     |   33 ++++++++++++++++++---------------
 gdbstub.c  |   16 ++++++++++++++--
 3 files changed, 34 insertions(+), 17 deletions(-)

exec.c +18 ~~-15~~

Index: b/exec.c
===================================================================
@@ -1180,13 +1180,13 @@ int cpu_watchpoint_insert(CPUState *env,
 {
     int i;
 
-    if (type != GDB_WATCHPOINT_WRITE)
-        return -ENOSYS;
-    if (len != 1 && len != 2 && len != 4)
+    /* sanity checks: allow power-of-2 lengths, deny unaligned breakpoints */
+    if ((len != 1 && len != 2 && len != 4) || (addr & (len-1)))
         return -EINVAL;
 
     for (i = 0; i < env->nb_watchpoints; i++) {
-        if (addr == env->watchpoint[i].vaddr)
+        if (addr == env->watchpoint[i].vaddr &&
+            len == env->watchpoint[i].len && type == env->watchpoint[i].type)
             return 0;
     }
     if (env->nb_watchpoints >= MAX_WATCHPOINTS)

@@ -1194,6 +1194,8 @@ int cpu_watchpoint_insert(CPUState *env,
 
     i = env->nb_watchpoints++;
     env->watchpoint[i].vaddr = addr;
+    env->watchpoint[i].len = len;
+    env->watchpoint[i].type = type;
     tlb_flush_page(env, addr);
     /* FIXME: This flush is needed because of the hack to make memory ops
        terminate the TB.  It can be removed once the proper IO trap and

@@ -1208,11 +1210,9 @@ int cpu_watchpoint_remove(CPUState *env,
 {
     int i;
 
-    if (type != GDB_WATCHPOINT_WRITE)
-        return -ENOSYS;
-
     for (i = 0; i < env->nb_watchpoints; i++) {
-        if (addr == env->watchpoint[i].vaddr) {
+        if (addr == env->watchpoint[i].vaddr &&
+            len == env->watchpoint[i].len && type == env->watchpoint[i].type) {
             env->nb_watchpoints--;
             env->watchpoint[i] = env->watchpoint[env->nb_watchpoints];
             tlb_flush_page(env, addr);

@@ -2393,7 +2393,8 @@ static uint32_t watch_mem_readl(void *op
 /* Generate a debug exception if a watchpoint has been hit.
    Returns the real physical address of the access.  addr will be a host
    address in case of a RAM location.  */
-static target_ulong check_watchpoint(target_phys_addr_t addr)
+static target_ulong check_watchpoint(target_phys_addr_t addr,
+                                     int len, int type)
 {
     CPUState *env = cpu_single_env;
     target_ulong watch;

@@ -2405,9 +2406,11 @@ static target_ulong check_watchpoint(tar
         watch = env->watchpoint[i].vaddr;
         if (((env->mem_write_vaddr ^ watch) & TARGET_PAGE_MASK) == 0) {
             retaddr = addr - env->watchpoint[i].addend;
-            if (((addr ^ watch) & ~TARGET_PAGE_MASK) == 0) {
-                cpu_single_env->watchpoint_hit = i + 1;
-                cpu_interrupt(cpu_single_env, CPU_INTERRUPT_DEBUG);
+            if (((addr ^ watch) & (~TARGET_PAGE_MASK - (len - 1))) == 0 &&
+                (env->watchpoint[i].type == type ||
+                 env->watchpoint[i].type == GDB_WATCHPOINT_ACCESS)) {
+                env->watchpoint_hit = i + 1;
+                cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
                 break;
             }
         }

@@ -2418,21 +2421,21 @@ static target_ulong check_watchpoint(tar
 static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
                              uint32_t val)
 {
-    addr = check_watchpoint(addr);
+    addr = check_watchpoint(addr, 1, GDB_WATCHPOINT_WRITE);
     stb_phys(addr, val);
 }
 
 static void watch_mem_writew(void *opaque, target_phys_addr_t addr,
                              uint32_t val)
 {
-    addr = check_watchpoint(addr);
+    addr = check_watchpoint(addr, 2, GDB_WATCHPOINT_WRITE);
     stw_phys(addr, val);
 }
 
 static void watch_mem_writel(void *opaque, target_phys_addr_t addr,
                              uint32_t val)
 {
-    addr = check_watchpoint(addr);
+    addr = check_watchpoint(addr, 4, GDB_WATCHPOINT_WRITE);
     stl_phys(addr, val);
 }

cpu-defs.h +2 -0

Index: b/cpu-defs.h
===================================================================
@@ -156,6 +156,8 @@ typedef struct CPUTLBEntry {
     struct {                                                            \
         target_ulong vaddr;                                             \
         target_phys_addr_t addend;                                      \
+        target_ulong len;                                               \
+        int type;                                                       \
     } watchpoint[MAX_WATCHPOINTS];                                      \
     int nb_watchpoints;                                                 \
     int watchpoint_hit;                                                 \

gdbstub.c +14 -2

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

@@ -1213,8 +1214,19 @@ 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].type) {
+            case GDB_WATCHPOINT_READ:
+                type = "r";
+                break;
+            case GDB_WATCHPOINT_ACCESS:
+                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-05-31 13:50 UTC #3 lore raw

PATCH 3/5 Add read watchpoint support

In reply to Jan Kiszka

This patch adds read access detection to the watchpoint subsystem. It adds a few additional instructions to the io_read path even when watchpoints are disabled. Me feeling is that this does not cause a relevant slowdown and can be justified by the enhanced feature set.

But I must say that the softmmu subsystem of QEMU is not yet my home. So parts of this patch were not really designed, but engineered ;). Nevertheless, things work smoothly for me, no regressions found so far.

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

---
 cpu-defs.h         |    8 +++---
 exec.c             |   70 ++++++++++++++++++++++++++++-------------------------
 softmmu_template.h |   15 +++++++----
 3 files changed, 52 insertions(+), 41 deletions(-)

exec.c +38 ~~-32~~

Index: b/exec.c
===================================================================
@@ -808,9 +808,9 @@ void tb_invalidate_phys_page_range(targe
             if (current_tb_not_found) {
                 current_tb_not_found = 0;
                 current_tb = NULL;
-                if (env->mem_write_pc) {
+                if (env->iomem_access_pc) {
                     /* now we have a real cpu fault */
-                    current_tb = tb_find_pc(env->mem_write_pc);
+                    current_tb = tb_find_pc(env->iomem_access_pc);
                 }
             }
             if (current_tb == tb &&

@@ -823,7 +823,7 @@ void tb_invalidate_phys_page_range(targe
 
                 current_tb_modified = 1;
                 cpu_restore_state(current_tb, env,
-                                  env->mem_write_pc, NULL);
+                                  env->iomem_access_pc, NULL);
 #if defined(TARGET_I386)
                 current_flags = env->hflags;
                 current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));

@@ -855,7 +855,7 @@ void tb_invalidate_phys_page_range(targe
     if (!p->first_tb) {
         invalidate_page_bitmap(p);
         if (is_cpu_write_access) {
-            tlb_unprotect_code_phys(env, start, env->mem_write_vaddr);
+            tlb_unprotect_code_phys(env, start, env->iomem_access_vaddr);
         }
     }
 #endif

@@ -881,7 +881,7 @@ static inline void tb_invalidate_phys_pa
     if (1) {
         if (loglevel) {
             fprintf(logfile, "modifying code at 0x%x size=%d EIP=%x PC=%08x\n",
-                   cpu_single_env->mem_write_vaddr, len,
+                   cpu_single_env->iomem_access_vaddr, len,
                    cpu_single_env->eip,
                    cpu_single_env->eip + (long)cpu_single_env->segs[R_CS].base);
         }

@@ -1769,7 +1769,7 @@ int tlb_set_page_exec(CPUState *env, tar
     PhysPageDesc *p;
     unsigned long pd;
     unsigned int index;
-    target_ulong address;
+    target_ulong address, address_read;
     target_phys_addr_t addend;
     int ret;
     CPUTLBEntry *te;

@@ -1801,18 +1801,18 @@ int tlb_set_page_exec(CPUState *env, tar
             addend = (unsigned long)phys_ram_base + (pd & TARGET_PAGE_MASK);
         }
 
+        address_read = address;
         /* Make accesses to pages with watchpoints go via the
            watchpoint trap routines.  */
         for (i = 0; i < env->nb_watchpoints; i++) {
             if (vaddr == (env->watchpoint[i].vaddr & TARGET_PAGE_MASK)) {
                 if (address & ~TARGET_PAGE_MASK) {
                     env->watchpoint[i].addend = 0;
-                    address = vaddr | io_mem_watch;
+                    address_read = address = vaddr | io_mem_watch;
                 } else {
                     env->watchpoint[i].addend = pd - paddr +
                         (unsigned long) phys_ram_base;
-                    /* TODO: Figure out how to make read watchpoints coexist
-                       with code.  */
+                    address_read = vaddr | io_mem_watch;
                     pd = (pd & TARGET_PAGE_MASK) | io_mem_watch | IO_MEM_ROMD;
                 }
             }

@@ -1823,7 +1823,7 @@ int tlb_set_page_exec(CPUState *env, tar
         te = &env->tlb_table[mmu_idx][index];
         te->addend = addend;
         if (prot & PAGE_READ) {
-            te->addr_read = address;
+            te->addr_read = address_read;
         } else {
             te->addr_read = -1;
         }

@@ -2304,7 +2304,8 @@ static void notdirty_mem_writeb(void *op
     /* we remove the notdirty callback only if the code has been
        flushed */
     if (dirty_flags == 0xff)
-        tlb_set_dirty(cpu_single_env, addr, cpu_single_env->mem_write_vaddr);
+        tlb_set_dirty(cpu_single_env, addr,
+                      cpu_single_env->iomem_access_vaddr);
 }
 
 static void notdirty_mem_writew(void *opaque, target_phys_addr_t addr, uint32_t val)

@@ -2330,7 +2331,8 @@ static void notdirty_mem_writew(void *op
     /* we remove the notdirty callback only if the code has been
        flushed */
     if (dirty_flags == 0xff)
-        tlb_set_dirty(cpu_single_env, addr, cpu_single_env->mem_write_vaddr);
+        tlb_set_dirty(cpu_single_env, addr,
+                      cpu_single_env->iomem_access_vaddr);
 }
 
 static void notdirty_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)

@@ -2356,7 +2358,8 @@ static void notdirty_mem_writel(void *op
     /* we remove the notdirty callback only if the code has been
        flushed */
     if (dirty_flags == 0xff)
-        tlb_set_dirty(cpu_single_env, addr, cpu_single_env->mem_write_vaddr);
+        tlb_set_dirty(cpu_single_env, addr,
+                      cpu_single_env->iomem_access_vaddr);
 }
 
 static CPUReadMemoryFunc *error_mem_read[3] = {

@@ -2372,24 +2375,6 @@ static CPUWriteMemoryFunc *notdirty_mem_
 };
 
 #if defined(CONFIG_SOFTMMU)
-/* Watchpoint access routines.  Watchpoints are inserted using TLB tricks,
-   so these check for a hit then pass through to the normal out-of-line
-   phys routines.  */
-static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
-{
-    return ldub_phys(addr);
-}
-
-static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
-{
-    return lduw_phys(addr);
-}
-
-static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
-{
-    return ldl_phys(addr);
-}
-
 /* Generate a debug exception if a watchpoint has been hit.
    Returns the real physical address of the access.  addr will be a host
    address in case of a RAM location.  */

@@ -2404,7 +2389,7 @@ static target_ulong check_watchpoint(tar
     retaddr = addr;
     for (i = 0; i < env->nb_watchpoints; i++) {
         watch = env->watchpoint[i].vaddr;
-        if (((env->mem_write_vaddr ^ watch) & TARGET_PAGE_MASK) == 0) {
+        if (((env->iomem_access_vaddr ^ watch) & TARGET_PAGE_MASK) == 0) {
             retaddr = addr - env->watchpoint[i].addend;
             if (((addr ^ watch) & (~TARGET_PAGE_MASK - (len - 1))) == 0 &&
                 (env->watchpoint[i].type == type ||

@@ -2418,6 +2403,27 @@ static target_ulong check_watchpoint(tar
     return retaddr;
 }
 
+
+/* Watchpoint access routines.  Watchpoints are inserted using TLB tricks,
+   so these check for a hit then pass through to the normal out-of-line
+   phys routines.  */
+static uint32_t watch_mem_readb(void *opaque, target_phys_addr_t addr)
+{
+    addr = check_watchpoint(addr, 1, GDB_WATCHPOINT_READ);
+    return ldub_phys(addr);
+}
+
+static uint32_t watch_mem_readw(void *opaque, target_phys_addr_t addr)
+{
+    addr = check_watchpoint(addr, 2, GDB_WATCHPOINT_READ);
+    return lduw_phys(addr);
+}
+
+static uint32_t watch_mem_readl(void *opaque, target_phys_addr_t addr)
+{
+    addr = check_watchpoint(addr, 4, GDB_WATCHPOINT_READ);
+    return ldl_phys(addr);
+}
 static void watch_mem_writeb(void *opaque, target_phys_addr_t addr,
                              uint32_t val)
 {

cpu-defs.h +4 -4

Index: b/cpu-defs.h
===================================================================
@@ -136,10 +136,10 @@ typedef struct CPUTLBEntry {
     /* in order to avoid passing too many arguments to the memory       \
        write helpers, we store some rarely used information in the CPU  \
        context) */                                                      \
-    unsigned long mem_write_pc; /* host pc at which the memory was      \
-                                   written */                           \
-    target_ulong mem_write_vaddr; /* target virtual addr at which the   \
-                                     memory was written */              \
+    unsigned long iomem_access_pc; /* host pc at which the io-memory    \
+                                      was accessed */                   \
+    target_ulong iomem_access_vaddr; /* target virtual addr at which    \
+                                        the io-memory was accessed */   \
     int halted; /* TRUE if the CPU is in suspend state */               \
     /* The meaning of the MMU modes is defined in the target code. */   \
     CPUTLBEntry tlb_table[NB_MMU_MODES][CPU_TLB_SIZE];                  \

softmmu_template.h +10 -5

Index: b/softmmu_template.h
===================================================================
@@ -51,12 +51,15 @@ static DATA_TYPE glue(glue(slow_ld, SUFF
                                                         int mmu_idx,
                                                         void *retaddr);
 static inline DATA_TYPE glue(io_read, SUFFIX)(target_phys_addr_t physaddr,
-                                              target_ulong tlb_addr)
+                                              target_ulong tlb_addr,
+                                              void *retaddr)
 {
     DATA_TYPE res;
     int index;
 
     index = (tlb_addr >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
+    env->iomem_access_vaddr = tlb_addr;
+    env->iomem_access_pc = (unsigned long)retaddr;
 #if SHIFT <= 2
     res = io_mem_read[index][SHIFT](io_mem_opaque[index], physaddr);
 #else

@@ -95,7 +98,8 @@ DATA_TYPE REGPARM glue(glue(__ld, SUFFIX
             /* IO access */
             if ((addr & (DATA_SIZE - 1)) != 0)
                 goto do_unaligned_access;
-            res = glue(io_read, SUFFIX)(physaddr, tlb_addr);
+            retaddr = GETPC();
+            res = glue(io_read, SUFFIX)(physaddr, tlb_addr, retaddr);
         } else if (((addr & ~TARGET_PAGE_MASK) + DATA_SIZE - 1) >= TARGET_PAGE_SIZE) {
             /* slow unaligned access (it spans two pages or IO) */
         do_unaligned_access:

@@ -147,7 +151,8 @@ static DATA_TYPE glue(glue(slow_ld, SUFF
             /* IO access */
             if ((addr & (DATA_SIZE - 1)) != 0)
                 goto do_unaligned_access;
-            res = glue(io_read, SUFFIX)(physaddr, tlb_addr);
+            retaddr = GETPC();
+            res = glue(io_read, SUFFIX)(physaddr, tlb_addr, retaddr);
         } else if (((addr & ~TARGET_PAGE_MASK) + DATA_SIZE - 1) >= TARGET_PAGE_SIZE) {
         do_unaligned_access:
             /* slow unaligned access (it spans two pages) */

@@ -191,8 +196,8 @@ static inline void glue(io_write, SUFFIX
     int index;
 
     index = (tlb_addr >> IO_MEM_SHIFT) & (IO_MEM_NB_ENTRIES - 1);
-    env->mem_write_vaddr = tlb_addr;
-    env->mem_write_pc = (unsigned long)retaddr;
+    env->iomem_access_vaddr = tlb_addr;
+    env->iomem_access_pc = (unsigned long)retaddr;
 #if SHIFT <= 2
     io_mem_write[index][SHIFT](io_mem_opaque[index], physaddr, val);
 #else

Jan Kiszka

author

2008-05-31 13:50 UTC #4 lore raw

PATCH 4/5 Report exact PC on watchpoint hit

In reply to Jan Kiszka

Watchpoints may hit us right in the middle of a TB, but so far we complete the TB before servicing the breakpoint trap. This patch picks up the preexisting feature of QEMU to leave a TB immediately if the underlying memory page changes. The result is that the exact instruction pointer is reported back to gdb. This improves the usefulness of watchpoints significantly!

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

---
 exec.c |    6 ++++++
 1 file changed, 6 insertions(+)

exec.c +6 -0

Index: b/exec.c
===================================================================
@@ -2382,6 +2382,7 @@ static target_ulong check_watchpoint(tar
                                      int len, int type)
 {
     CPUState *env = cpu_single_env;
+    target_phys_addr_t tb_addr;
     target_ulong watch;
     target_ulong retaddr;
     int i;

@@ -2395,6 +2396,11 @@ static target_ulong check_watchpoint(tar
                 (env->watchpoint[i].type == type ||
                  env->watchpoint[i].type == GDB_WATCHPOINT_ACCESS)) {
                 env->watchpoint_hit = i + 1;
+                if (env->current_tb) {
+                    tb_addr = env->current_tb->page_addr[0];
+                    tb_invalidate_phys_page_range(tb_addr,
+                                            tb_addr+TARGET_PAGE_SIZE-1, 1);
+                }
                 cpu_interrupt(env, CPU_INTERRUPT_DEBUG);
                 break;
             }

Paul Brook

2008-05-31 14:11 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 4/5] Report exact PC on watchpoint hit

exec.c static target_ulong check_watchpoint(tar · line 2399

Quoted 5 lines

exec.c +4 -0

@@ -2395,6 +2396,11 @@ static target_ulong check_watchpoint(tar
+                if (env->current_tb) {
+                    tb_addr = env->current_tb->page_addr[0];
+                    tb_invalidate_phys_page_range(tb_addr,
+                                            tb_addr+TARGET_PAGE_SIZE-1,

This is wrong. env->current_tb is not what you think it it is. TB chaining means we may be an any TB reachable from there.

Paul

Jan Kiszka

author

2008-05-31 14:42 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 4/5] Report exact PC on watchpoint hit

Quoted 8 lines · Paul Brook

exec.c static target_ulong check_watchpoint(tar · line 2399
Quoted 5 lines
exec.c +4 -0
@@ -2395,6 +2396,11 @@ static target_ulong check_watchpoint(tar
+                if (env->current_tb) {
+                    tb_addr = env->current_tb->page_addr[0];
+                    tb_invalidate_phys_page_range(tb_addr,
+                                            tb_addr+TARGET_PAGE_SIZE-1,
This is wrong. env->current_tb is not what you think it it is. TB chaining means we may be an any TB reachable from there.

OK, I see. But the general approach to obtain the exact PC is fine? So should I flush the whole memory range instead?

Jan

Paul Brook

2008-05-31 15:17 UTC #7 lore raw

Re: [Qemu-devel] [PATCH 4/5] Report exact PC on watchpoint hit

Quoted 10 lines · Jan Kiszka

Quoted 8 lines · Paul Brook
exec.c static target_ulong check_watchpoint(tar · line 2399
Quoted 5 lines
exec.c +4 -0
@@ -2395,6 +2396,11 @@ static target_ulong check_watchpoint(tar
+                if (env->current_tb) {
+                    tb_addr = env->current_tb->page_addr[0];
+                    tb_invalidate_phys_page_range(tb_addr,
+                                            tb_addr+TARGET_PAGE_SIZE-1,
This is wrong. env->current_tb is not what you think it it is. TB chaining means we may be an any TB reachable from there.

Quoted 2 lines

OK, I see. But the general approach to obtain the exact PC is fine? So should I flush the whole memory range instead?

By whole memory range you mean all of everything? That sounds a fairly poor solution. The way arm and m68k handle this is to never put a memory op in the middle of a TB.

Your solution also only works for targets that define TARGET_HAS_PRECISE_SMC.

Paul

Jan Kiszka

author

2008-05-31 13:51 UTC #8 lore raw

PATCH 5/5 Enhance SMP guest debugging

In reply to Jan Kiszka

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

This patch adds SMP awareness to the debugger. It allows to set the debugger focus explicitly via the monitor command "cpu", but it also automatically switches the focus on breakpoint hit to the reporting CPU.

Furthermore, the patch propagates breakpoint and watchpoint insertions or removals to all CPUs, not just the current one as it was the case so far.

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

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

---
 cpu-defs.h |    3 +++
 exec.c     |   54 ++++++++++++++++++++++++++++++++++++++----------------
 gdbstub.c  |   25 ++++++++++++-------------
 monitor.c  |   19 ++++++++++++-------
 monitor.h  |   15 +++++++++++++++
 vl.c       |    2 ++
 6 files changed, 82 insertions(+), 36 deletions(-)

gdbstub.c +12 ~~-13~~

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;

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

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

@@ -1210,11 +1211,11 @@ static void gdb_vm_stopped(void *opaque,
         return;
 
     /* disable single step if it was enable */
-    cpu_single_step(s->env, 0);
+    cpu_single_step(env, 0);
 
     if (reason == EXCP_DEBUG) {
-        if (s->env->watchpoint_hit) {
-            switch (s->env->watchpoint[s->env->watchpoint_hit - 1].type) {
+        if (env->watchpoint_hit) {
+            switch (env->watchpoint[env->watchpoint_hit - 1].type) {
             case GDB_WATCHPOINT_READ:
                 type = "r";
                 break;

@@ -1227,12 +1228,12 @@ static void gdb_vm_stopped(void *opaque,
             }
             snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
                      SIGTRAP, type,
-                     s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
+                     env->watchpoint[env->watchpoint_hit - 1].vaddr);
             put_packet(s, buf);
-            s->env->watchpoint_hit = 0;
+            env->watchpoint_hit = 0;
             return;
         }
-	tb_flush(s->env);
+	tb_flush(env);
         ret = SIGTRAP;
     } else if (reason == EXCP_INTERRUPT) {
         ret = SIGINT;

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

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

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

monitor.c +12 -7

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

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

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

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

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"

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

cpu-defs.h +3 -0

Index: b/cpu-defs.h
===================================================================
@@ -174,3 +174,6 @@ typedef struct CPUTLBEntry {
     const char *cpu_model_str;
 
 #endif
+
+#define foreach_cpu(env) \
+    for(env = first_cpu; env != NULL; env = env->next_cpu)

exec.c +38 ~~-16~~

124 lines not on this page · raw message

Fabrice Bellard

2008-05-31 16:51 UTC #9 lore raw

In reply to Jan Kiszka

Hi,

I cannot accept the patches for several reasons:

1) You mix cosmetic and functional patches.

2) The current watchpoint code is not implemented correctly so it is not safe to improve it using the same system (IMHO it should not have been commited in its current state). A correct implementation should not delay the DEBUG exception. It should be implemented like the "normal" MMU exceptions.

Regards,

Fabrice.

Quoted 24 lines · Jan Kiszka

This patch series strives at improving QEMU /wrt guest debugging - for me a very important use case of this software.
It consists of patches that refactor and enhance the breakpoint/watchpoint support, originally driven by the need to extend QEMU's internal API in order to enhance (real) hardware-assisted debugging in KVM. But as the KVM maintainer refused to merge the core of my patches until the QEMU bits went mainline, I was forced to rebase them immediately :->. In fact, these patches now come with even more added value also for QEMU, ie. full watchpoint support with exact PC addresses.
Moreover, the series contains an old, unmerged patch of mine that improves the debugability of SMP guests. For this submission, I basically rebased and polished it further. I'm using it for months now as it is de facto mandatory for SMP debugging.
Please review!
Jan

Paul Brook

2008-05-31 17:05 UTC #10 lore raw

Quoted 5 lines

2) The current watchpoint code is not implemented correctly so it is not safe to improve it using the same system (IMHO it should not have been commited in its current state). A correct implementation should not delay the DEBUG exception. It should be implemented like the "normal" MMU exceptions.

On most targets watchpoint traps occur after the instruction completes, so you have to defer the DEBUG exception.
Normal MMU faults occur before the instruction completes.

Paul

Jan Kiszka

author

2008-05-31 17:29 UTC #11 lore raw

Quoted 9 lines · Paul Brook

Quoted 5 lines
2) The current watchpoint code is not implemented correctly so it is not safe to improve it using the same system (IMHO it should not have been commited in its current state). A correct implementation should not delay the DEBUG exception. It should be implemented like the "normal" MMU exceptions.
On most targets watchpoint traps occur after the instruction completes, so you have to defer the DEBUG exception.
Normal MMU faults occur before the instruction completes.

OK, that explains why it is not an immediately raised exception.

Still, instead of changing the TB stop condition as you suggested, I would prefer to pay the price only on watchpoint hit. Any show stopper to reuse the pattern of tb_invalidate_phys_page_range (find pc, [invalidate TB?,] regenerate TB @PC with CF_SINGLE_INSN, resume execution)? Or is you suggestion simpler and not as costly as I assume?

Jan

Fabrice Bellard

2008-05-31 18:34 UTC #12 lore raw

In reply to Paul Brook

Quoted 9 lines · Paul Brook

Quoted 5 lines
2) The current watchpoint code is not implemented correctly so it is not safe to improve it using the same system (IMHO it should not have been commited in its current state). A correct implementation should not delay the DEBUG exception. It should be implemented like the "normal" MMU exceptions.
On most targets watchpoint traps occur after the instruction completes, so you have to defer the DEBUG exception.
Normal MMU faults occur before the instruction completes.

If this is the wanted behavior then the same system as the Self Modifying Code on x86 should be used. Basically it consists in doing as a MMU fault and single stepping one instruction after. Unfortunately I fear the implementation will be complicated.

Fabrice.

Jan Kiszka

author

2008-06-01 13:54 UTC #13 lore raw

Quoted 6 lines · Fabrice Bellard

Quoted 5 lines · Paul Brook
Quoted 5 lines
2) The current watchpoint code is not implemented correctly so it is not safe to improve it using the same system (IMHO it should not have been commited in its current state). A correct implementation should not delay the DEBUG exception. It should be implemented like the "normal" MMU exceptions.

Quoted 8 lines

Quoted 3 lines
On most targets watchpoint traps occur after the instruction completes, so you have to defer the DEBUG exception.
Normal MMU faults occur before the instruction completes.
If this is the wanted behavior then the same system as the Self Modifying Code on x86 should be used. Basically it consists in doing as a MMU fault and single stepping one instruction after. Unfortunately I fear the implementation will be complicated.

It isn't, in fact, and it allows for a few nice cleanups. I've a working version here now, based on a new singlestep_enable flag. Will re-post my whole series once I've integrated your other remarks.

Jan

Jamie Lokier

2008-06-01 12:39 UTC #14 lore raw

In reply to Paul Brook

Quoted 3 lines · Paul Brook

On most targets watchpoint traps occur after the instruction completes, so you have to defer the DEBUG exception. Normal MMU faults occur before the instruction completes.

Ok. It might be a useful architecture-independent debugging feature to _also_ have the ability to trap a watchpoint before the memory access though - especially writes. Are there any tracing tools which use the gdbstub support and could make use of that? Can GDB make use of that?

-- Jamie

Jan Kiszka

author

2008-06-01 13:56 UTC #15 lore raw

Quoted 10 lines · Jamie Lokier

Quoted 3 lines · Paul Brook
On most targets watchpoint traps occur after the instruction completes, so you have to defer the DEBUG exception. Normal MMU faults occur before the instruction completes.
Ok. It might be a useful architecture-independent debugging feature to _also_ have the ability to trap a watchpoint before the memory access though - especially writes. Are there any tracing tools which use the gdbstub support and could make use of that? Can GDB make use of that?

The spec of gdb's remote protocol does not mention where the instruction pointer is supposed to point at when a watchpoint is reported. But making this configurable is a good idea, will take it into account. I'm currently reworking my queue, and I just added a watchpoint flag that will decide about this behavior. Just needs to be implemented, but that shouldn't be hard. Will likely take yet another monitor command, just like for allow/block IRQs on singlestep.

Jan

Jan Kiszka

author

2008-05-31 17:20 UTC #16 lore raw

In reply to Fabrice Bellard

Quoted 5 lines · Fabrice Bellard

Hi,
I cannot accept the patches for several reasons:
1) You mix cosmetic and functional patches.

Do you have specific hunks in mind? I'm a bit blind ATM, not seeing where I changed coding style or naming for cosmetic reasons.

Quoted 6 lines

2) The current watchpoint code is not implemented correctly so it is not safe to improve it using the same system (IMHO it should not have been commited in its current state). A correct implementation should not delay the DEBUG exception. It should be implemented like the "normal" MMU exceptions.

Yeah, I'm sitting over this point right now, asking myself why a watchpoint exception was modeled as CPU_INTERRUPT_DEBUG

Do you have a pattern at hand how to signal this correctly? As I don't want to raise an exception on the guest CPU, I'm a bit clueless how to achieve this.

Thanks,
Jan

Fabrice Bellard

2008-05-31 18:43 UTC #17 lore raw

Quoted 9 lines · Jan Kiszka

Quoted 5 lines · Fabrice Bellard
Hi,
I cannot accept the patches for several reasons:
1) You mix cosmetic and functional patches.
Do you have specific hunks in mind? I'm a bit blind ATM, not seeing where I changed coding style or naming for cosmetic reasons.

You renammed mem_write_pc and mem_write_vaddr.

BTW, why did you add 'len' and 'type' parameters to breakpoints ?

I don't think it is a good idea to say that breakpoints/watchpoints apply to all processors. Such behavior should be handled at a higher level.

It would also be interesting if the watchpoint/breakpoint implementation could be used to implement CPU watchpoints and breakpoints (I am thinking about the x86 DRx registers here).

Fabrice.

Paul Brook

2008-06-01 00:06 UTC #18 lore raw

Quoted 6 lines

I don't think it is a good idea to say that breakpoints/watchpoints apply to all processors. Such behavior should be handled at a higher level.
It would also be interesting if the watchpoint/breakpoint implementation could be used to implement CPU watchpoints and breakpoints (I am thinking about the x86 DRx registers here).

Watchpoints and breakpoints are architecture independent debugger features. The gdb break/watchpoints should be independent of any user visible debug hardware.

I agree it probably makes sense for both to share an implementation though.

Quoted 4 lines

If this is the wanted behavior then the same system as the Self Modifying Code on x86 should be used. Basically it consists in doing as a MMU fault and single stepping one instruction after. Unfortunately I fear the implementation will be complicated.

FWIW I'm planning on reviving the instruction counting patches and making some modifications to the mmio TLB handling, which should provide infrastructure for doing this in a better way.

Paul

Jan Kiszka

author

2008-06-01 13:53 UTC #19 lore raw

In reply to Fabrice Bellard

Quoted 11 lines · Fabrice Bellard

Quoted 8 lines · Jan Kiszka
Quoted 5 lines · Fabrice Bellard
Hi,
I cannot accept the patches for several reasons:
1) You mix cosmetic and functional patches.
Do you have specific hunks in mind? I'm a bit blind ATM, not seeing where I changed coding style or naming for cosmetic reasons.
You renammed mem_write_pc and mem_write_vaddr.

mem -> iomem dated back when I was confused about plain RAM access going through handlers that are called io_read/write. Reverted this. But write -> access is required as I changed the semantics of these variables.

Quoted 5 lines

BTW, why did you add 'len' and 'type' parameters to breakpoints ?
I don't think it is a good idea to say that breakpoints/watchpoints apply to all processors. Such behavior should be handled at a higher level.

Yes, I agree meanwhile. Now I introduced a host-injection layer to gdbstub that handles this. All cpu_* services are per-CPU again.

Quoted 4 lines

It would also be interesting if the watchpoint/breakpoint implementation could be used to implement CPU watchpoints and breakpoints (I am thinking about the x86 DRx registers here).

Good point! And a hard argument to keep per-CPU breakpoint/watchpoint lists. I've already prepared my patches to tell host-injected breakpoints apart from guest-injected ones. Will now look into at least basic debug register support of x86 (which was on my list anyway).

Jan