[Qemu-devel] [PATCH v3] target-arm: Fix breakpoint handling

[Qemu-devel] [PATCH v3] target-arm: Fix breakpoint handling

[Sergey Fedorov]

This series is intended to fix ARM breakpoint emulation misbehavior.
QEMU hangs when QEMU breakpoint 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 emulation misbehavior can be found at:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html

Changes in v3:
 * GDB breakpoint check in arm_debug_excp_handler() simplified
 * Debug exception generating fixed in translation code

Changes in v2:
 * The initial commit split into two parts
 * Extra block eliminated in arm_debug_excp_handler()
 * Just one instruction always translated in case of breakpoint PC match

Sergey Fedorov (1):
  target-arm: Fix CPU breakpoint handling

 target-arm/helper.h        |  2 ++
 target-arm/op_helper.c     | 29 ++++++++++++++++++-----------
 target-arm/translate-a64.c | 17 ++++++++++++-----
 target-arm/translate.c     | 17 ++++++++++++-----
 4 files changed, 44 insertions(+), 21 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v3 1/2] target-arm: Fix GDB breakpoint handling

[Sergey Fedorov]

GDB breakpoints have higher priority so they have to be checked first.
Should GDB breakpoint match, just return from the debug exception
handler.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---
 target-arm/op_helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 1425a1d..67b18c0 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -897,6 +897,12 @@ void arm_debug_excp_handler(CPUState *cs)
             }
         }
     } else {
+        uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
+
+        if (cpu_breakpoint_test(cs, pc, BP_GDB)) {
+            return;
+        }
+
         if (check_breakpoints(cpu)) {
             bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
             if (extended_addresses_enabled(env)) {
-- 
1.9.1

[Qemu-devel] [PATCH v3 2/2] target-arm: Fix CPU breakpoint handling

[Sergey Fedorov]

A QEMU breakpoint match is not definitely an architectural breakpoint
match. If an exception is generated unconditionally during translation,
it is hardly possible to ignore it in the debug exception handler.

Generate a call to a helper to check CPU breakpoints and raise an
exception only if any breakpoint matches architecturally.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---
 target-arm/helper.h        |  2 ++
 target-arm/op_helper.c     | 29 ++++++++++++++++++-----------
 target-arm/translate-a64.c | 17 ++++++++++++-----
 target-arm/translate.c     | 19 ++++++++++++++-----
 4 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/target-arm/helper.h b/target-arm/helper.h
index 827b33d..c2a85c7 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -54,6 +54,8 @@ DEF_HELPER_1(yield, void, env)
 DEF_HELPER_1(pre_hvc, void, env)
 DEF_HELPER_2(pre_smc, void, env, i32)
 
+DEF_HELPER_1(check_breakpoints, void, env)
+
 DEF_HELPER_3(cpsr_write, void, env, i32, i32)
 DEF_HELPER_1(cpsr_read, i32, env)
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 67b18c0..7929c71 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -867,6 +867,15 @@ static bool check_breakpoints(ARMCPU *cpu)
     return false;
 }
 
+void HELPER(check_breakpoints)(CPUARMState *env)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+
+    if (check_breakpoints(cpu)) {
+        HELPER(exception_internal(env, EXCP_DEBUG));
+    }
+}
+
 void arm_debug_excp_handler(CPUState *cs)
 {
     /* Called by core code when a watchpoint or breakpoint fires;
@@ -898,23 +907,21 @@ void arm_debug_excp_handler(CPUState *cs)
         }
     } else {
         uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
+        bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
 
         if (cpu_breakpoint_test(cs, pc, BP_GDB)) {
             return;
         }
 
-        if (check_breakpoints(cpu)) {
-            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;
-            }
-            /* FAR is UNKNOWN, so doesn't need setting */
-            raise_exception(env, EXCP_PREFETCH_ABORT,
-                            syn_breakpoint(same_el),
-                            arm_debug_target_el(env));
+        if (extended_addresses_enabled(env)) {
+            env->exception.fsr = (1 << 9) | 0x22;
+        } else {
+            env->exception.fsr = 0x2;
         }
+        /* FAR is UNKNOWN, so doesn't need setting */
+        raise_exception(env, EXCP_PREFETCH_ABORT,
+                        syn_breakpoint(same_el),
+                        arm_debug_target_el(env));
     }
 }
 
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index e65e309..09a5dde 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -11084,11 +11084,18 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
             CPUBreakpoint *bp;
             QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
                 if (bp->pc == dc->pc) {
-                    gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
-                    /* Advance PC so that clearing the breakpoint will
-                       invalidate this TB.  */
-                    dc->pc += 2;
-                    goto done_generating;
+                    if (bp->flags & BP_CPU) {
+                        gen_helper_check_breakpoints(cpu_env);
+                        /* End the TB early; it likely won't be executed */
+                        dc->is_jmp = DISAS_UPDATE;
+                    } else {
+                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+                        /* Advance PC so that clearing the breakpoint will
+                           invalidate this TB.  */
+                        dc->pc += 4;
+                        goto done_generating;
+                    }
+                    break;
                 }
             }
         }
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 22c3587..0652721 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11329,11 +11329,20 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             CPUBreakpoint *bp;
             QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
                 if (bp->pc == dc->pc) {
-                    gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
-                    /* Advance PC so that clearing the breakpoint will
-                       invalidate this TB.  */
-                    dc->pc += 2;
-                    goto done_generating;
+                    if (bp->flags & BP_CPU) {
+                        gen_helper_check_breakpoints(cpu_env);
+                        /* End the TB early; it's likely not going to be executed */
+                        dc->is_jmp = DISAS_UPDATE;
+                    } else {
+                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+                        /* Advance PC so that clearing the breakpoint will
+                           invalidate this TB.  */
+                        /* TODO: Advance PC by correct instruction length to
+                         * avoid disassembler error messages */
+                        dc->pc += 2;
+                        goto done_generating;
+                    }
+                    break;
                 }
             }
         }
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3] target-arm: Fix breakpoint handling

[Peter Maydell]

On 13 October 2015 at 10:56, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> This series is intended to fix ARM breakpoint emulation misbehavior.
> QEMU hangs when QEMU breakpoint 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 emulation misbehavior can be found at:
> https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html
>
> Changes in v3:
>  * GDB breakpoint check in arm_debug_excp_handler() simplified
>  * Debug exception generating fixed in translation code
>
> Changes in v2:
>  * The initial commit split into two parts
>  * Extra block eliminated in arm_debug_excp_handler()
>  * Just one instruction always translated in case of breakpoint PC match
>
> Sergey Fedorov (1):
>   target-arm: Fix CPU breakpoint handling

Something weird happened to this series -- you can see the
cover letter only mentions one of the two patches.

I've applied them both to target-arm.next now.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3] target-arm: Fix breakpoint handling

[Sergey Fedorov]

On 16.10.2015 16:01, Peter Maydell wrote:
> On 13 October 2015 at 10:56, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> This series is intended to fix ARM breakpoint emulation misbehavior.
>> QEMU hangs when QEMU breakpoint 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 emulation misbehavior can be found at:
>> https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html
>>
>> Changes in v3:
>>  * GDB breakpoint check in arm_debug_excp_handler() simplified
>>  * Debug exception generating fixed in translation code
>>
>> Changes in v2:
>>  * The initial commit split into two parts
>>  * Extra block eliminated in arm_debug_excp_handler()
>>  * Just one instruction always translated in case of breakpoint PC match
>>
>> Sergey Fedorov (1):
>>   target-arm: Fix CPU breakpoint handling
> Something weird happened to this series -- you can see the
> cover letter only mentions one of the two patches.
>
> I've applied them both to target-arm.next now.

Yes, I figured out it a bit late that I had messed the cover letter...
But it seemed to be not so important, and I decided not to resend the
series.

Best regards,
Sergey