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

[Qemu-devel] [PATCH 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

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 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>
---
 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 54cd70a..64ed543 100644
--- a/exec.c
+++ b/exec.c
@@ -1921,6 +1921,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;
@@ -1947,6 +1948,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 39712ab..4e0a1b9 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -101,6 +101,8 @@ struct TranslationBlock;
  * @get_phys_page_debug: Callback for obtaining a physical address.
  * @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.
@@ -155,6 +157,7 @@ typedef struct CPUClass {
     hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
     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 62f4b5d..def1298 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -186,6 +186,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)
 {
@@ -348,6 +356,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

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

[Peter Maydell]

On 14 September 2015 at 11:50, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> 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>
> ---
>  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 54cd70a..64ed543 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1921,6 +1921,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;
> @@ -1947,6 +1948,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;
> +                }

Rather than setting cpu->watchpoint_hit, and then undoing it if the
"is this a non-matching CPU wp" check failed, it would be better
to just do the test before we assign to cpu->watchpoint_hit.
We can just pass the wp pointer through to the debug_check_watchpoint
hook in case the target needs it. (I think the ARM one at the
moment doesn't, though it would be slightly more efficient to check
only the wp in question, rather than all of them.)

>                  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 39712ab..4e0a1b9 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -101,6 +101,8 @@ struct TranslationBlock;
>   * @get_phys_page_debug: Callback for obtaining a physical address.
>   * @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.

I think we could make this slightly more informative:
 * @debug_check_watchpoint: Callback: return true if the architectural
    watchpoint whose address has matched should really fire.

>   * @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.
> @@ -155,6 +157,7 @@ typedef struct CPUClass {
>      hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
>      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);

Otherwise looks good.

thanks
-- PMM

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

[Peter Maydell]

On 14 September 2015 at 11:50, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> 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>
> ---
>  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 54cd70a..64ed543 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1921,6 +1921,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;
> @@ -1947,6 +1948,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;

Missed this on first readthrough, but this code doesn't clear the
BP_WATCHPOINT_HIT flags from wp->flags if we decide that the
architectural watchpoint shouldn't fire. That means that next time
around when we call check_watchpoint() it might decide spruriously
that it should fire.

thanks
-- PMM

[Qemu-devel] [PATCH 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>
---
 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 cc6c6f3..69dd158 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1428,6 +1428,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->gdb_core_xml_file = "arm-core.xml";
     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 924aff9..251d5f6 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -372,6 +372,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 1425a1d..b298e57 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -867,6 +867,16 @@ static bool check_breakpoints(ARMCPU *cpu)
     return false;
 }
 
+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;
@@ -878,23 +888,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 {
         if (check_breakpoints(cpu)) {
-- 
1.9.1

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

[Peter Maydell]

On 14 September 2015 at 11:50, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> 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>

thanks
-- PMM

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

[Peter Maydell]

On 14 September 2015 at 11:50, Sergey Fedorov <serge.fdrv@gmail.com> 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
>
> Sergey Fedorov (2):
>   cpu: Add callback to check architectural watchpoint match
>   target-arm: Implement checking of fired watchpoint

Thanks for this -- I've put this and your other two debug related
patches on my to-review queue; ping me in a week or so if I haven't
got to them by then.

-- PMM