[Qemu-devel] [PATCH v2 0/2] Architectural watchpoint check

[Qemu-devel] [PATCH v2 0/2] Architectural watchpoint check

[Sergey Fedorov]

This series is intended to fix ARM watchpoint emulation misbehavior.
QEMU hangs when QEMU watchpoint fires but it does not pass additional
architectural checks in ARM CPU debug exception handler. For details,
please see individual patches. The most relevant parts of the original
discussion about ARM breakpoint and watchpoint emulation misbehavior can be
found at:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00527.html

Changes in v2:
 * Check moved before setting cpu->watchpoint_hit
 * Pointer to watchpoint being checked passed to debug_check_watchpoint()
   callback
 * Comment for debug_check_watchpoint() callback improved

Sergey Fedorov (2):
  cpu: Add callback to check architectural watchpoint match
  target-arm: Implement checking of fired watchpoint

 exec.c                 |  5 +++++
 include/qom/cpu.h      |  3 +++
 qom/cpu.c              |  9 +++++++++
 target-arm/cpu.c       |  1 +
 target-arm/internals.h |  3 +++
 target-arm/op_helper.c | 35 +++++++++++++++++++++--------------
 6 files changed, 42 insertions(+), 14 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/2] cpu: Add callback to check architectural watchpoint match

[Sergey Fedorov]

When QEMU watchpoint matches, that is not definitely an architectural
watchpoint match yet. If it is a stop-before-access watchpoint then that
is hardly possible to ignore it after throwing a TCG exception.

A special callback is introduced to check for architectural watchpoint
match before raising a TCG exception.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---

Notes:
    Changes in v2:
     * Check moved before setting cpu->watchpoint_hit
     * Pointer to watchpoint being checked passed to debug_check_watchpoint()
       callback
     * Comment for debug_check_watchpoint() callback improved

 exec.c            | 5 +++++
 include/qom/cpu.h | 3 +++
 qom/cpu.c         | 9 +++++++++
 3 files changed, 17 insertions(+)

diff --git a/exec.c b/exec.c
index 9e076bc..a20f8ea 100644
--- a/exec.c
+++ b/exec.c
@@ -2024,6 +2024,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
 static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
 {
     CPUState *cpu = current_cpu;
+    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUArchState *env = cpu->env_ptr;
     target_ulong pc, cs_base;
     target_ulong vaddr;
@@ -2050,6 +2051,10 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
             wp->hitattrs = attrs;
             if (!cpu->watchpoint_hit) {
                 cpu->watchpoint_hit = wp;
+                if (wp->flags & BP_CPU && !cc->debug_check_watchpoint(cpu)) {
+                    cpu->watchpoint_hit = NULL;
+                    continue;
+                }
                 tb_check_watchpoint(cpu);
                 if (wp->flags & BP_STOP_BEFORE_ACCESS) {
                     cpu->exception_index = EXCP_DEBUG;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 035179c..095ba08 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -106,6 +106,8 @@ struct TranslationBlock;
  *       a memory access with the specified memory transaction attributes.
  * @gdb_read_register: Callback for letting GDB read a register.
  * @gdb_write_register: Callback for letting GDB write a register.
+ * @debug_check_watchpoint: Callback for checking an architectural watchpoint
+ * match.
  * @debug_excp_handler: Callback for handling debug exceptions.
  * @write_elf64_note: Callback for writing a CPU-specific ELF note to a
  * 64-bit VM coredump.
@@ -165,6 +167,7 @@ typedef struct CPUClass {
     int (*asidx_from_attrs)(CPUState *cpu, MemTxAttrs attrs);
     int (*gdb_read_register)(CPUState *cpu, uint8_t *buf, int reg);
     int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
+    bool (*debug_check_watchpoint)(CPUState *cpu);
     void (*debug_excp_handler)(CPUState *cpu);
 
     int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
diff --git a/qom/cpu.c b/qom/cpu.c
index 8f537a4..5caa0ee 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -188,6 +188,14 @@ static int cpu_common_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg)
     return 0;
 }
 
+static bool cpu_common_debug_check_watchpoint(CPUState *cpu)
+{
+    /* If no extra check is required, QEMU watchpoint match can be considered
+     * as an architectural match.
+     */
+    return true;
+}
+
 bool target_words_bigendian(void);
 static bool cpu_common_virtio_is_big_endian(CPUState *cpu)
 {
@@ -352,6 +360,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->gdb_write_register = cpu_common_gdb_write_register;
     k->virtio_is_big_endian = cpu_common_virtio_is_big_endian;
     k->debug_excp_handler = cpu_common_noop;
+    k->debug_check_watchpoint = cpu_common_debug_check_watchpoint;
     k->cpu_exec_enter = cpu_common_noop;
     k->cpu_exec_exit = cpu_common_noop;
     k->cpu_exec_interrupt = cpu_common_exec_interrupt;
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/2] target-arm: Implement checking of fired watchpoint

[Sergey Fedorov]

ARM stops before access to a location covered by watchpoint. Also, QEMU
watchpoint fire is not necessarily an architectural watchpoint match.
Unfortunately, that is hardly possible to ignore a fired watchpoint in
debug exception handler. So move watchpoint check from debug exception
handler to the dedicated watchpoint checking callback.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.c       |  1 +
 target-arm/internals.h |  3 +++
 target-arm/op_helper.c | 35 +++++++++++++++++++++--------------
 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 0e582c4..21ec18e 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1474,6 +1474,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_arch_name = arm_gdb_arch_name;
     cc->gdb_stop_before_watchpoint = true;
     cc->debug_excp_handler = arm_debug_excp_handler;
+    cc->debug_check_watchpoint = arm_debug_check_watchpoint;
 
     cc->disas_set_info = arm_disas_set_info;
 
diff --git a/target-arm/internals.h b/target-arm/internals.h
index d226bbe..7e67eaa 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -409,6 +409,9 @@ void hw_breakpoint_update(ARMCPU *cpu, int n);
  */
 void hw_breakpoint_update_all(ARMCPU *cpu);
 
+/* Callback function for checking if a watchpoint should trigger. */
+bool arm_debug_check_watchpoint(CPUState *cs);
+
 /* Callback function for when a watchpoint or breakpoint triggers. */
 void arm_debug_excp_handler(CPUState *cs);
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index a5ee65f..859691a 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -975,6 +975,16 @@ void HELPER(check_breakpoints)(CPUARMState *env)
     }
 }
 
+bool arm_debug_check_watchpoint(CPUState *cs)
+{
+    /* Called by core code when a CPU watchpoint fires; need to check if this
+     * is also an architectural watchpoint match.
+     */
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    return check_watchpoints(cpu);
+}
+
 void arm_debug_excp_handler(CPUState *cs)
 {
     /* Called by core code when a watchpoint or breakpoint fires;
@@ -986,23 +996,20 @@ void arm_debug_excp_handler(CPUState *cs)
 
     if (wp_hit) {
         if (wp_hit->flags & BP_CPU) {
+            bool wnr = (wp_hit->flags & BP_WATCHPOINT_HIT_WRITE) != 0;
+            bool same_el = arm_debug_target_el(env) == arm_current_el(env);
+
             cs->watchpoint_hit = NULL;
-            if (check_watchpoints(cpu)) {
-                bool wnr = (wp_hit->flags & BP_WATCHPOINT_HIT_WRITE) != 0;
-                bool same_el = arm_debug_target_el(env) == arm_current_el(env);
-
-                if (extended_addresses_enabled(env)) {
-                    env->exception.fsr = (1 << 9) | 0x22;
-                } else {
-                    env->exception.fsr = 0x2;
-                }
-                env->exception.vaddress = wp_hit->hitaddr;
-                raise_exception(env, EXCP_DATA_ABORT,
-                                syn_watchpoint(same_el, 0, wnr),
-                                arm_debug_target_el(env));
+
+            if (extended_addresses_enabled(env)) {
+                env->exception.fsr = (1 << 9) | 0x22;
             } else {
-                cpu_resume_from_signal(cs, NULL);
+                env->exception.fsr = 0x2;
             }
+            env->exception.vaddress = wp_hit->hitaddr;
+            raise_exception(env, EXCP_DATA_ABORT,
+                    syn_watchpoint(same_el, 0, wnr),
+                    arm_debug_target_el(env));
         }
     } else {
         uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 0/2] Architectural watchpoint check

[Sergey Fedorov]

On 29.01.2016 22:17, Sergey Fedorov wrote:
> This series is intended to fix ARM watchpoint emulation misbehavior.
> QEMU hangs when QEMU watchpoint fires but it does not pass additional
> architectural checks in ARM CPU debug exception handler. For details,
> please see individual patches. The most relevant parts of the original
> discussion about ARM breakpoint and watchpoint emulation misbehavior can be
> found at:
> https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00527.html
>
> Changes in v2:
>  * Check moved before setting cpu->watchpoint_hit
>  * Pointer to watchpoint being checked passed to debug_check_watchpoint()
>    callback
>  * Comment for debug_check_watchpoint() callback improved
>
> Sergey Fedorov (2):
>   cpu: Add callback to check architectural watchpoint match
>   target-arm: Implement checking of fired watchpoint
>
>  exec.c                 |  5 +++++
>  include/qom/cpu.h      |  3 +++
>  qom/cpu.c              |  9 +++++++++
>  target-arm/cpu.c       |  1 +
>  target-arm/internals.h |  3 +++
>  target-arm/op_helper.c | 35 +++++++++++++++++++++--------------
>  6 files changed, 42 insertions(+), 14 deletions(-)
>

Please ignore this series. Somehow I send the old patches again. I'm
sending v3 with the correct patches.

Best regards,
Sergey