[Qemu-devel] [PATCH 0/5] Debugger enhancements

[Qemu-devel] [PATCH 0/5] Debugger enhancements

[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

[Qemu-devel] [PATCH 3/5] Add read watchpoint support

[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 <jan.kiszka@web.de>

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

Index: b/exec.c
===================================================================
--- a/exec.c
+++ 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)
 {
Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ 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];                  \
Index: b/softmmu_template.h
===================================================================
--- a/softmmu_template.h
+++ 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

[Qemu-devel] [PATCH 2/5] Watchpoint length and type awareness

[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 <jan.kiszka@web.de>
---
 cpu-defs.h |    2 ++
 exec.c     |   33 ++++++++++++++++++---------------
 gdbstub.c  |   16 ++++++++++++++--
 3 files changed, 34 insertions(+), 17 deletions(-)

Index: b/exec.c
===================================================================
--- a/exec.c
+++ 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);
 }
 
Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ 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;                                                 \
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ 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;

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

[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 <jan.kiszka@web.de>
---
 exec.c |    6 ++++++
 1 file changed, 6 insertions(+)

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

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

[Paul Brook]

> @@ -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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

Paul Brook wrote:
>> @@ -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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

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

[Paul Brook]

On Saturday 31 May 2008, Jan Kiszka wrote:
> Paul Brook wrote:
> >> @@ -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?

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

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

[Jan Kiszka]

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

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

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

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

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 cpu-defs.h |    3 +++
 exec.c     |   54 ++++++++++++++++++++++++++++++++++++++----------------
 gdbstub.c  |   25 ++++++++++++-------------
 monitor.c  |   19 ++++++++++++-------
 monitor.h  |   15 +++++++++++++++
 vl.c       |    2 ++
 6 files changed, 82 insertions(+), 36 deletions(-)

Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -34,6 +34,7 @@
 #include "sysemu.h"
 #include "gdbstub.h"
 #endif
+#include "monitor.h"
 
 #include "qemu_socket.h"
 #ifdef _WIN32
@@ -58,7 +59,6 @@ enum RSState {
     RS_SYSCALL,
 };
 typedef struct GDBState {
-    CPUState *env; /* current CPU */
     enum RSState state; /* parsing state */
     char line_buf[4096];
     int line_buf_index;
@@ -1050,7 +1050,7 @@ static int gdb_handle_packet(GDBState *s
                 p++;
             type = *p;
             if (gdb_current_syscall_cb)
-                gdb_current_syscall_cb(s->env, ret, err);
+                gdb_current_syscall_cb(mon_get_cpu(), ret, err);
             if (type == 'C') {
                 put_packet(s, "T02");
             } else {
@@ -1202,6 +1202,7 @@ extern void tb_flush(CPUState *env);
 static void gdb_vm_stopped(void *opaque, int reason)
 {
     GDBState *s = opaque;
+    CPUState *env = mon_get_cpu();
     char buf[256];
     char *type;
     int ret;
@@ -1210,11 +1211,11 @@ static void gdb_vm_stopped(void *opaque,
         return;
 
     /* disable single step if it was enable */
-    cpu_single_step(s->env, 0);
+    cpu_single_step(env, 0);
 
     if (reason == EXCP_DEBUG) {
-        if (s->env->watchpoint_hit) {
-            switch (s->env->watchpoint[s->env->watchpoint_hit - 1].type) {
+        if (env->watchpoint_hit) {
+            switch (env->watchpoint[env->watchpoint_hit - 1].type) {
             case GDB_WATCHPOINT_READ:
                 type = "r";
                 break;
@@ -1227,12 +1228,12 @@ static void gdb_vm_stopped(void *opaque,
             }
             snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
                      SIGTRAP, type,
-                     s->env->watchpoint[s->env->watchpoint_hit - 1].vaddr);
+                     env->watchpoint[env->watchpoint_hit - 1].vaddr);
             put_packet(s, buf);
-            s->env->watchpoint_hit = 0;
+            env->watchpoint_hit = 0;
             return;
         }
-	tb_flush(s->env);
+	tb_flush(env);
         ret = SIGTRAP;
     } else if (reason == EXCP_INTERRUPT) {
         ret = SIGINT;
@@ -1302,15 +1303,15 @@ void gdb_do_syscall(gdb_syscall_complete
     va_end(va);
     put_packet(s, buf);
 #ifdef CONFIG_USER_ONLY
-    gdb_handlesig(s->env, 0);
+    gdb_handlesig(mon_get_cpu(), 0);
 #else
-    cpu_interrupt(s->env, CPU_INTERRUPT_EXIT);
+    cpu_interrupt(mon_get_cpu(), CPU_INTERRUPT_EXIT);
 #endif
 }
 
 static void gdb_read_byte(GDBState *s, int ch)
 {
-    CPUState *env = s->env;
+    CPUState *env = mon_get_cpu();
     int i, csum;
     uint8_t reply;
 
@@ -1474,7 +1475,6 @@ static void gdb_accept(void *opaque)
 
     s = &gdbserver_state;
     memset (s, 0, sizeof (GDBState));
-    s->env = first_cpu; /* XXX: allow to change CPU */
     s->fd = fd;
 
     gdb_syscall_state = s;
@@ -1577,7 +1577,6 @@ int gdbserver_start(const char *port)
     if (!s) {
         return -1;
     }
-    s->env = first_cpu; /* XXX: allow to change CPU */
     s->chr = chr;
     qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
                           gdb_chr_event, s);
Index: b/monitor.c
===================================================================
--- a/monitor.c
+++ b/monitor.c
@@ -264,8 +264,7 @@ static void do_info_blockstats(void)
     bdrv_info_stats();
 }
 
-/* get the current CPU defined by the user */
-static int mon_set_cpu(int cpu_index)
+static int mon_set_cpu_index(int cpu_index)
 {
     CPUState *env;
 
@@ -278,10 +277,16 @@ static int mon_set_cpu(int cpu_index)
     return -1;
 }
 
-static CPUState *mon_get_cpu(void)
+void mon_set_cpu(CPUState *env)
+{
+    mon_cpu = env;
+}
+
+/* get the current CPU defined by the user or by a breakpoint hit */
+CPUState *mon_get_cpu(void)
 {
     if (!mon_cpu) {
-        mon_set_cpu(0);
+        mon_set_cpu(first_cpu);
     }
     return mon_cpu;
 }
@@ -305,8 +310,8 @@ static void do_info_cpus(void)
 {
     CPUState *env;
 
-    /* just to set the default cpu if not already done */
-    mon_get_cpu();
+    if (!mon_cpu)
+        mon_set_cpu(first_cpu);
 
     for(env = first_cpu; env != NULL; env = env->next_cpu) {
         term_printf("%c CPU #%d:",
@@ -329,7 +334,7 @@ static void do_info_cpus(void)
 
 static void do_cpu_set(int index)
 {
-    if (mon_set_cpu(index) < 0)
+    if (mon_set_cpu_index(index) < 0)
         term_printf("Invalid CPU index\n");
 }
 
Index: b/monitor.h
===================================================================
--- /dev/null
+++ b/monitor.h
@@ -0,0 +1,15 @@
+#ifndef QEMU_MONITOR_H
+#define QEMU_MONITOR_H
+
+void mon_set_cpu(CPUState *env);
+
+#ifdef CONFIG_USER_ONLY
+static inline CPUState *mon_get_cpu(void)
+{
+    return first_cpu;
+}
+#else
+CPUState *mon_get_cpu(void);
+#endif
+
+#endif /* QEMU_MONITOR_H */
Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -33,6 +33,7 @@
 #include "console.h"
 #include "sysemu.h"
 #include "gdbstub.h"
+#include "monitor.h"
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "block.h"
@@ -7116,6 +7117,7 @@ static int main_loop(void)
                 ret = EXCP_INTERRUPT;
             }
             if (unlikely(ret == EXCP_DEBUG)) {
+                mon_set_cpu(cur_cpu);
                 vm_stop(EXCP_DEBUG);
             }
             /* If all cpus are halted then wait until the next IRQ */
Index: b/cpu-defs.h
===================================================================
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -174,3 +174,6 @@ typedef struct CPUTLBEntry {
     const char *cpu_model_str;
 
 #endif
+
+#define foreach_cpu(env) \
+    for(env = first_cpu; env != NULL; env = env->next_cpu)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1178,6 +1178,7 @@ static void breakpoint_invalidate(CPUSta
 int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
                           int type)
 {
+    CPUState *env_iter;
     int i;
 
     /* sanity checks: allow power-of-2 lengths, deny unaligned breakpoints */
@@ -1192,10 +1193,12 @@ int cpu_watchpoint_insert(CPUState *env,
     if (env->nb_watchpoints >= MAX_WATCHPOINTS)
         return -ENOBUFS;
 
-    i = env->nb_watchpoints++;
-    env->watchpoint[i].vaddr = addr;
-    env->watchpoint[i].len = len;
-    env->watchpoint[i].type = type;
+    foreach_cpu(env_iter) {
+        i = env_iter->nb_watchpoints++;
+        env_iter->watchpoint[i].vaddr = addr;
+        env_iter->watchpoint[i].len = len;
+        env_iter->watchpoint[i].type = type;
+    }
     tlb_flush_page(env, addr);
     /* FIXME: This flush is needed because of the hack to make memory ops
        terminate the TB.  It can be removed once the proper IO trap and
@@ -1208,13 +1211,17 @@ int cpu_watchpoint_insert(CPUState *env,
 int cpu_watchpoint_remove(CPUState *env, target_ulong addr, target_ulong len,
                           int type)
 {
+    CPUState *env_iter;
     int i;
 
     for (i = 0; i < env->nb_watchpoints; i++) {
         if (addr == env->watchpoint[i].vaddr &&
             len == env->watchpoint[i].len && type == env->watchpoint[i].type) {
-            env->nb_watchpoints--;
-            env->watchpoint[i] = env->watchpoint[env->nb_watchpoints];
+            foreach_cpu(env_iter) {
+                env_iter->nb_watchpoints--;
+                env_iter->watchpoint[i] =
+                    env_iter->watchpoint[env->nb_watchpoints];
+            }
             tlb_flush_page(env, addr);
             return 0;
         }
@@ -1223,13 +1230,17 @@ int cpu_watchpoint_remove(CPUState *env,
 }
 
 /* Remove all watchpoints. */
-void cpu_watchpoint_remove_all(CPUState *env) {
+void cpu_watchpoint_remove_all(CPUState *env)
+{
+    CPUState *env_iter;
     int i;
 
     for (i = 0; i < env->nb_watchpoints; i++) {
         tlb_flush_page(env, env->watchpoint[i].vaddr);
     }
-    env->nb_watchpoints = 0;
+    foreach_cpu(env_iter) {
+        env_iter->nb_watchpoints = 0;
+    }
 }
 
 /* add a breakpoint. EXCP_DEBUG is returned by the CPU loop if a
@@ -1238,6 +1249,7 @@ int cpu_breakpoint_insert(CPUState *env,
                           int type)
 {
 #if defined(TARGET_HAS_ICE)
+    CPUState *env_iter;
     int i;
 
     for(i = 0; i < env->nb_breakpoints; i++) {
@@ -1247,8 +1259,9 @@ int cpu_breakpoint_insert(CPUState *env,
 
     if (env->nb_breakpoints >= MAX_BREAKPOINTS)
         return -ENOBUFS;
-    env->breakpoints[env->nb_breakpoints++] = pc;
-
+    foreach_cpu(env_iter) {
+        env_iter->breakpoints[env_iter->nb_breakpoints++] = pc;
+    }
     breakpoint_invalidate(env, pc);
     return 0;
 #else
@@ -1257,13 +1270,18 @@ int cpu_breakpoint_insert(CPUState *env,
 }
 
 /* remove all breakpoints */
-void cpu_breakpoint_remove_all(CPUState *env) {
+void cpu_breakpoint_remove_all(CPUState *env)
+{
 #if defined(TARGET_HAS_ICE)
+    CPUState *env_iter;
     int i;
+
     for(i = 0; i < env->nb_breakpoints; i++) {
         breakpoint_invalidate(env, env->breakpoints[i]);
     }
-    env->nb_breakpoints = 0;
+    foreach_cpu(env_iter) {
+        env->nb_breakpoints = 0;
+    }
 #endif
 }
 
@@ -1272,17 +1290,21 @@ int cpu_breakpoint_remove(CPUState *env,
                           int type)
 {
 #if defined(TARGET_HAS_ICE)
+    CPUState *env_iter;
     int i;
+
     for(i = 0; i < env->nb_breakpoints; i++) {
         if (env->breakpoints[i] == pc)
             goto found;
     }
     return -ENOENT;
- found:
-    env->nb_breakpoints--;
-    if (i < env->nb_breakpoints)
-      env->breakpoints[i] = env->breakpoints[env->nb_breakpoints];
 
+ found:
+    foreach_cpu(env_iter) {
+        env_iter->nb_breakpoints--;
+        env_iter->breakpoints[i] =
+            env_iter->breakpoints[env_iter->nb_breakpoints];
+    }
     breakpoint_invalidate(env, pc);
     return 0;
 #else

[Qemu-devel] [PATCH 1/5] Refactor breakpoint API and gdbstub integration

[Jan Kiszka]

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 <jan.kiszka@web.de>
---
 cpu-all.h |   14 ++++++++++----
 exec.c    |   32 ++++++++++++++++++++++----------
 gdbstub.c |   50 +++++++++++++++++++++++++++++---------------------
 3 files changed, 61 insertions(+), 35 deletions(-)

Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ 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 */
Index: b/exec.c
===================================================================
--- a/exec.c
+++ 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
 }
 
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -951,7 +951,7 @@ static void cpu_gdb_write_registers(CPUS
 static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
 {
     const char *p;
-    int ch, reg_size, type;
+    int ch, reg_size, type, res;
     char buf[4096];
     uint8_t mem_buf[4096];
     uint32_t *registers;
@@ -1103,21 +1103,20 @@ static int gdb_handle_packet(GDBState *s
         if (*p == ',')
             p++;
         len = strtoull(p, (char **)&p, 16);
-        if (type == 0 || type == 1) {
-            if (cpu_breakpoint_insert(env, addr) < 0)
-                goto breakpoint_error;
-            put_packet(s, "OK");
+        switch (type) {
+        case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
+            res = cpu_breakpoint_insert(env, addr, len, type);
+            break;
 #ifndef CONFIG_USER_ONLY
-        } else if (type == 2) {
-            if (cpu_watchpoint_insert(env, addr) < 0)
-                goto breakpoint_error;
-            put_packet(s, "OK");
+        case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
+            res = cpu_watchpoint_insert(env, addr, len, type);
+            break;
 #endif
-        } else {
-        breakpoint_error:
-            put_packet(s, "E22");
+        default:
+            res = -ENOSYS;
+            break;
         }
-        break;
+        goto answer_bp_packet;
     case 'z':
         type = strtoul(p, (char **)&p, 16);
         if (*p == ',')
@@ -1126,17 +1125,26 @@ static int gdb_handle_packet(GDBState *s
         if (*p == ',')
             p++;
         len = strtoull(p, (char **)&p, 16);
-        if (type == 0 || type == 1) {
-            cpu_breakpoint_remove(env, addr);
-            put_packet(s, "OK");
+        switch (type) {
+        case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
+            res = cpu_breakpoint_remove(env, addr, len, type);
+            break;
 #ifndef CONFIG_USER_ONLY
-        } else if (type == 2) {
-            cpu_watchpoint_remove(env, addr);
-            put_packet(s, "OK");
+        case GDB_WATCHPOINT_WRITE ... GDB_WATCHPOINT_ACCESS:
+            res = cpu_watchpoint_remove(env, addr, len, type);
+            break;
 #endif
-        } else {
-            goto breakpoint_error;
+        default:
+            res = -ENOSYS;
+            break;
         }
+    answer_bp_packet:
+        if (res >= 0)
+            put_packet(s, "OK");
+        else if (res == -ENOSYS)
+            put_packet(s, "");
+        else
+            put_packet(s, "E22");
         break;
     case 'q':
     case 'Q':

Re: [Qemu-devel] [PATCH 0/5] Debugger enhancements

[Fabrice Bellard]

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.

Jan Kiszka wrote:
> 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
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 0/5] Debugger enhancements

[Paul Brook]

> 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

[Qemu-devel] Re: [PATCH 0/5] Debugger enhancements

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

Paul Brook wrote:
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Debugger enhancements

[Fabrice Bellard]

Paul Brook wrote:
>> 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.

[Qemu-devel] Re: [PATCH 0/5] Debugger enhancements

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1035 bytes --]

Fabrice Bellard wrote:
> Paul Brook wrote:
>>> 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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Debugger enhancements

[Jamie Lokier]

Paul Brook wrote:
> 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

[Qemu-devel] Re: [PATCH 0/5] Debugger enhancements

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Jamie Lokier wrote:
> Paul Brook wrote:
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

[Qemu-devel] Re: [PATCH 0/5] Debugger enhancements

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

Fabrice Bellard wrote:
> 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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [Qemu-devel] Re: [PATCH 0/5] Debugger enhancements

[Fabrice Bellard]

Jan Kiszka wrote:
> Fabrice Bellard wrote:
>> 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.

Re: [Qemu-devel] Re: [PATCH 0/5] Debugger enhancements

[Paul Brook]

> 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.

> 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

[Qemu-devel] Re: [PATCH 0/5] Debugger enhancements

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

Fabrice Bellard wrote:
> Jan Kiszka wrote:
>> Fabrice Bellard wrote:
>>> 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.

> 
> 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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]