diff mbox

[RFC,v3,05/19] exec: add assert_debug_safe and notes on debug structures

Message ID 1464986428-6739-6-git-send-email-alex.bennee@linaro.org
State New
Headers show

Commit Message

Alex Bennée June 3, 2016, 8:40 p.m. UTC 
The debug structures are different from many of the other shared ones as
they are modified from two places:

  - architectural debug registers
  - the gdb stub

The first is usually in the context of vCPU currently running so
modifications are safe. The second is generally only changed while the
vCPUs are halted and therefore not going to clash.

I fixed the commenting on DEBUG_SUBPAGE when I added the new DEBUG_DEBUG
flag.

KNOWN ISSUES: arm powerctl resets will trigger the asserts in
multi-thread mode with smp > 1 as the reset operation is cross-cpu and
clears all watchpoints.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 exec.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

Comments

Sergey Fedorov June 24, 2016, 3:28 p.m. UTC | #1 
On 03/06/16 23:40, Alex Bennée wrote:
> diff --git a/exec.c b/exec.c
> index a3a93ae..b225282 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -25,6 +25,7 @@
>  #include "qemu/cutils.h"
>  #include "cpu.h"
>  #include "exec/exec-all.h"
> +#include "qom/cpu.h"
>  #include "tcg.h"
>  #include "hw/qdev-core.h"
>  #if !defined(CONFIG_USER_ONLY)
> @@ -62,7 +63,46 @@
>  #include "qemu/mmap-alloc.h"
>  #endif
>  
> -//#define DEBUG_SUBPAGE
> +/* #define DEBUG_SUBPAGE */
> +/* #define DEBUG_DEBUG */
> +
> +#ifdef DEBUG_DEBUG
> +#define CHECK_DEBUG_SAFE 1
> +#else
> +#define CHECK_DEBUG_SAFE 0
> +#endif
> +
> +/*
> + * Safe access to debugging structures.
> + *
> + * Breakpoints and Watchpoints are kept in the vCPU structures. There
> + * are two ways they are manipulated:
> + *
> + *   - Outside of the context of the vCPU thread (e.g. gdbstub)
> + *   - Inside the context of the vCPU (architectural debug registers)
> + *
> + * In system emulation mode the chance of corruption is usually
> + * mitigated by the fact the vCPUs is usually suspended whenever these
> + * changes are made.
> + *
> + * In user emulation mode it is less clear (XXX: work this out)
> + */
> +
> +#ifdef CONFIG_SOFTMMU
> +#define assert_debug_safe(cpu) do {                                     \
> +        if (CHECK_DEBUG_SAFE) {                                         \
> +            g_assert(!cpu->created ||                                   \
> +                     (cpu_is_stopped(cpu) || cpu == current_cpu));      \

There's no need in parentheses around "cpu_is_stopped(cpu) || cpu ==
current_cpu".

> +        }                                                               \
> +    } while (0)
> +#else
> +#define assert_debug_safe(cpu) do {                                     \
> +        if (CHECK_DEBUG_SAFE) {                                         \
> +            g_assert(false);                                            \
> +        }                                                               \

Can be simply:

#define assert_debug_safe(cpu)

Kind regards,
Sergey

> +    } while (0)
> +#endif
> +
>  
>  #if !defined(CONFIG_USER_ONLY)
>  /* ram_list is read under rcu_read_lock()/rcu_read_unlock().  Writes
>
diff mbox

Patch

diff --git a/exec.c b/exec.c
index a3a93ae..b225282 100644
--- a/exec.c
+++ b/exec.c
@@ -25,6 +25,7 @@ 
 #include "qemu/cutils.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
+#include "qom/cpu.h"
 #include "tcg.h"
 #include "hw/qdev-core.h"
 #if !defined(CONFIG_USER_ONLY)
@@ -62,7 +63,46 @@ 
 #include "qemu/mmap-alloc.h"
 #endif
 
-//#define DEBUG_SUBPAGE
+/* #define DEBUG_SUBPAGE */
+/* #define DEBUG_DEBUG */
+
+#ifdef DEBUG_DEBUG
+#define CHECK_DEBUG_SAFE 1
+#else
+#define CHECK_DEBUG_SAFE 0
+#endif
+
+/*
+ * Safe access to debugging structures.
+ *
+ * Breakpoints and Watchpoints are kept in the vCPU structures. There
+ * are two ways they are manipulated:
+ *
+ *   - Outside of the context of the vCPU thread (e.g. gdbstub)
+ *   - Inside the context of the vCPU (architectural debug registers)
+ *
+ * In system emulation mode the chance of corruption is usually
+ * mitigated by the fact the vCPUs is usually suspended whenever these
+ * changes are made.
+ *
+ * In user emulation mode it is less clear (XXX: work this out)
+ */
+
+#ifdef CONFIG_SOFTMMU
+#define assert_debug_safe(cpu) do {                                     \
+        if (CHECK_DEBUG_SAFE) {                                         \
+            g_assert(!cpu->created ||                                   \
+                     (cpu_is_stopped(cpu) || cpu == current_cpu));      \
+        }                                                               \
+    } while (0)
+#else
+#define assert_debug_safe(cpu) do {                                     \
+        if (CHECK_DEBUG_SAFE) {                                         \
+            g_assert(false);                                            \
+        }                                                               \
+    } while (0)
+#endif
+
 
 #if !defined(CONFIG_USER_ONLY)
 /* ram_list is read under rcu_read_lock()/rcu_read_unlock().  Writes
@@ -737,6 +777,8 @@  int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
 {
     CPUWatchpoint *wp;
 
+    assert_debug_safe(cpu);
+
     /* forbid ranges which are empty or run off the end of the address space */
     if (len == 0 || (addr + len - 1) < addr) {
         error_report("tried to set invalid watchpoint at %"
@@ -769,6 +811,8 @@  int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
 {
     CPUWatchpoint *wp;
 
+    assert_debug_safe(cpu);
+
     QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
         if (addr == wp->vaddr && len == wp->len
                 && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
@@ -782,6 +826,8 @@  int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
 /* Remove a specific watchpoint by reference.  */
 void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint)
 {
+    assert_debug_safe(cpu);
+
     QTAILQ_REMOVE(&cpu->watchpoints, watchpoint, entry);
 
     tlb_flush_page(cpu, watchpoint->vaddr);
@@ -794,6 +840,8 @@  void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
 {
     CPUWatchpoint *wp, *next;
 
+    assert_debug_safe(cpu);
+
     QTAILQ_FOREACH_SAFE(wp, &cpu->watchpoints, entry, next) {
         if (wp->flags & mask) {
             cpu_watchpoint_remove_by_ref(cpu, wp);
@@ -829,6 +877,8 @@  int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
 {
     CPUBreakpoint *bp;
 
+    assert_debug_safe(cpu);
+
     bp = g_malloc(sizeof(*bp));
 
     bp->pc = pc;
@@ -849,11 +899,16 @@  int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
     return 0;
 }
 
-/* Remove a specific breakpoint.  */
+/* Remove a specific breakpoint.
+ *
+ * See cpu_breakpoint_insert notes for information about locking.
+ */
 int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
 {
     CPUBreakpoint *bp;
 
+    assert_debug_safe(cpu);
+
     QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
         if (bp->pc == pc && bp->flags == flags) {
             cpu_breakpoint_remove_by_ref(cpu, bp);