[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 <address@hidden>
---
 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':