[Qemu-devel] [PATCH v2 0/2] target-arm: Fix breakpoint handling

[Qemu-devel] [PATCH v2 0/2] 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 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 (2):
  target-arm: Fix GDB breakpoint handling
  target-arm: Fix CPU breakpoint handling

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

-- 
1.9.1

[Qemu-devel] [PATCH v2 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 1425a1d..1d4d8cb 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -897,6 +897,15 @@ void arm_debug_excp_handler(CPUState *cs)
             }
         }
     } else {
+        CPUBreakpoint *bp;
+        uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
+
+        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
+            if (bp->pc == pc && !(bp->flags & BP_CPU)) {
+                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

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

[Peter Maydell]

On 28 September 2015 at 11:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 1425a1d..1d4d8cb 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -897,6 +897,15 @@ void arm_debug_excp_handler(CPUState *cs)
>              }
>          }
>      } else {
> +        CPUBreakpoint *bp;
> +        uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
> +
> +        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> +            if (bp->pc == pc && !(bp->flags & BP_CPU)) {
> +                return;
> +            }
> +        }
> +

In current master you can write this more simply as

    if (cpu_breakpoint_test(cs, pc, BP_GDB)) {
        return;
    }

since rth's patchset introduced that utility function.

thanks
-- PMM

[Qemu-devel] [PATCH v2 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 | 14 ++++++++------
 target-arm/translate.c     | 13 ++++++++-----
 4 files changed, 36 insertions(+), 22 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 1d4d8cb..8ec8590 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;
@@ -899,6 +908,7 @@ void arm_debug_excp_handler(CPUState *cs)
     } else {
         CPUBreakpoint *bp;
         uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
+        bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
 
         QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
             if (bp->pc == pc && !(bp->flags & BP_CPU)) {
@@ -906,18 +916,15 @@ void arm_debug_excp_handler(CPUState *cs)
             }
         }
 
-        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 ec0936c..426229f 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -11082,11 +11082,14 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
         if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
             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);
+                    } else {
+                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+                    }
+                    /* End the TB early; it's likely not going to be executed */
+                    dc->is_jmp = DISAS_UPDATE;
+                    break;
                 }
             }
         }
@@ -11209,7 +11212,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
         }
     }
 
-done_generating:
     gen_tb_end(tb, num_insns);
 
 #ifdef DEBUG_DISAS
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 84a21ac..405d6d0 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11328,11 +11328,14 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
         if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
             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);
+                    } else {
+                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
+                    }
+                    /* End the TB early; it's likely not going to be executed */
+                    dc->is_jmp = DISAS_UPDATE;
+                    break;
                 }
             }
         }
-- 
1.9.1

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

[Peter Maydell]

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

> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index ec0936c..426229f 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -11082,11 +11082,14 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>              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);
> +                    } else {
> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);

We shouldn't just continue here, because now we'll try to generate the
code for the instruction even in the "we know this will always be a bp"
case. Also, you've dropped the "advance PC" part which we need so this
TB is not zero length.

> +                    }
> +                    /* End the TB early; it's likely not going to be executed */
> +                    dc->is_jmp = DISAS_UPDATE;

gen_exception_internal_insn sets is_jmp to DISAS_EXC, and then this
overrides it; so this line should go inside the "is this a BP_CPU bp?"
if clause. (I think the effect is just that we pointlessly generate some
unreachable code after the exception generating call.)

> +                    break;
>                  }
>              }
>          }
> @@ -11209,7 +11212,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>          }
>      }
>
> -done_generating:
>      gen_tb_end(tb, num_insns);
>
>  #ifdef DEBUG_DISAS
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 84a21ac..405d6d0 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11328,11 +11328,14 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>              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);
> +                    } else {
> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +                    }
> +                    /* End the TB early; it's likely not going to be executed */
> +                    dc->is_jmp = DISAS_UPDATE;

Similar comments here.

Annoying corner case which I don't think we need to handle necessarily:
if you set a breakpoint on a 32-bit Thumb instruction which spans a page
boundary, and the second page is not present, we will end up taking the
page fault when I think we should take the breakpoint. I can't think
of a way to get that right, so just commenting that it isn't handled
right would do.

> +                    break;
>                  }
>              }
>          }
> --
> 1.9.1
>

Otherwise I think this is the right approach.

thanks
-- PMM

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

[Sergey Fedorov]

On 08.10.2015 21:40, Peter Maydell wrote:
> On 28 September 2015 at 11:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> 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>
>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> index ec0936c..426229f 100644
>> --- a/target-arm/translate-a64.c
>> +++ b/target-arm/translate-a64.c
>> @@ -11082,11 +11082,14 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>              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);
>> +                    } else {
>> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> We shouldn't just continue here, because now we'll try to generate the
> code for the instruction even in the "we know this will always be a bp"
> case. Also, you've dropped the "advance PC" part which we need so this
> TB is not zero length.

Actually, I was going to do the same way as some architectures (e.g.
alpha) did: always translate one instruction so that TB size is
determined by actual instruction decoding. At least, it makes sense for
AArch32 where we can have 16/32-bit insns. If we advance PC incorrectly,
we will get "Disassembler disagrees with translator over instruction
decoding" warning messages when in_asm log enabled. I can rewrite it
with PC advancement, but at least, I would like to change the
advancement to 4 bytes for A64 translation.

Best regards,
Sergey

>
>> +                    }
>> +                    /* End the TB early; it's likely not going to be executed */
>> +                    dc->is_jmp = DISAS_UPDATE;
> gen_exception_internal_insn sets is_jmp to DISAS_EXC, and then this
> overrides it; so this line should go inside the "is this a BP_CPU bp?"
> if clause. (I think the effect is just that we pointlessly generate some
> unreachable code after the exception generating call.)
>
>> +                    break;
>>                  }
>>              }
>>          }
>> @@ -11209,7 +11212,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>          }
>>      }
>>
>> -done_generating:
>>      gen_tb_end(tb, num_insns);
>>
>>  #ifdef DEBUG_DISAS
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 84a21ac..405d6d0 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -11328,11 +11328,14 @@ static inline void gen_intermediate_code_internal(ARMCPU *cpu,
>>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>              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);
>> +                    } else {
>> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>> +                    }
>> +                    /* End the TB early; it's likely not going to be executed */
>> +                    dc->is_jmp = DISAS_UPDATE;
> Similar comments here.
>
> Annoying corner case which I don't think we need to handle necessarily:
> if you set a breakpoint on a 32-bit Thumb instruction which spans a page
> boundary, and the second page is not present, we will end up taking the
> page fault when I think we should take the breakpoint. I can't think
> of a way to get that right, so just commenting that it isn't handled
> right would do.
>
>> +                    break;
>>                  }
>>              }
>>          }
>> --
>> 1.9.1
>>
> Otherwise I think this is the right approach.
>
> thanks
> -- PMM

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

[Peter Maydell]

On 9 October 2015 at 14:53, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 08.10.2015 21:40, Peter Maydell wrote:
>> On 28 September 2015 at 11:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>> 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>
>>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>>> index ec0936c..426229f 100644
>>> --- a/target-arm/translate-a64.c
>>> +++ b/target-arm/translate-a64.c
>>> @@ -11082,11 +11082,14 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>>              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);
>>> +                    } else {
>>> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>> We shouldn't just continue here, because now we'll try to generate the
>> code for the instruction even in the "we know this will always be a bp"
>> case. Also, you've dropped the "advance PC" part which we need so this
>> TB is not zero length.
>
> Actually, I was going to do the same way as some architectures (e.g.
> alpha) did: always translate one instruction so that TB size is
> determined by actual instruction decoding. At least, it makes sense for
> AArch32 where we can have 16/32-bit insns. If we advance PC incorrectly,
> we will get "Disassembler disagrees with translator over instruction
> decoding" warning messages when in_asm log enabled. I can rewrite it
> with PC advancement, but at least, I would like to change the
> advancement to 4 bytes for A64 translation.

Hmm, I see. I'm not sure it makes sense to do a bunch of extra
work at codegen just to avoid a debug message. It would be nicer
to suppress that warning some other way.

thanks
-- PMM

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

[Sergey Fedorov]

On 09.10.2015 17:00, Peter Maydell wrote:
> On 9 October 2015 at 14:53, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 08.10.2015 21:40, Peter Maydell wrote:
>>> On 28 September 2015 at 11:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>>> 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>
>>>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>>>> index ec0936c..426229f 100644
>>>> --- a/target-arm/translate-a64.c
>>>> +++ b/target-arm/translate-a64.c
>>>> @@ -11082,11 +11082,14 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>>>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>>>              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);
>>>> +                    } else {
>>>> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>> We shouldn't just continue here, because now we'll try to generate the
>>> code for the instruction even in the "we know this will always be a bp"
>>> case. Also, you've dropped the "advance PC" part which we need so this
>>> TB is not zero length.
>> Actually, I was going to do the same way as some architectures (e.g.
>> alpha) did: always translate one instruction so that TB size is
>> determined by actual instruction decoding. At least, it makes sense for
>> AArch32 where we can have 16/32-bit insns. If we advance PC incorrectly,
>> we will get "Disassembler disagrees with translator over instruction
>> decoding" warning messages when in_asm log enabled. I can rewrite it
>> with PC advancement, but at least, I would like to change the
>> advancement to 4 bytes for A64 translation.
> Hmm, I see. I'm not sure it makes sense to do a bunch of extra
> work at codegen just to avoid a debug message. It would be nicer
> to suppress that warning some other way.
>

I think we could try figuring out the actual instruction length in case
of Thumb instruction, i.e. to do partial instruction decoding.

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

[Sergey Fedorov]

On 08.10.2015 21:40, Peter Maydell wrote:
> Annoying corner case which I don't think we need to handle necessarily:
> if you set a breakpoint on a 32-bit Thumb instruction which spans a page
> boundary, and the second page is not present, we will end up taking the
> page fault when I think we should take the breakpoint. I can't think
> of a way to get that right, so just commenting that it isn't handled
> right would do.

Could you please point out the piece of code which will generate the
page fault? Maybe I will give it a thought :)

Best regards,
Sergey

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

[Peter Maydell]

On 9 October 2015 at 14:59, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 08.10.2015 21:40, Peter Maydell wrote:
>> Annoying corner case which I don't think we need to handle necessarily:
>> if you set a breakpoint on a 32-bit Thumb instruction which spans a page
>> boundary, and the second page is not present, we will end up taking the
>> page fault when I think we should take the breakpoint. I can't think
>> of a way to get that right, so just commenting that it isn't handled
>> right would do.
>
> Could you please point out the piece of code which will generate the
> page fault? Maybe I will give it a thought :)

When you call arm_ldl_code() and friends, they will end up longjmp()ing
out of the codegen phase if the load faults. This then turns into a
guest-visible fault in the usual way.

To avoid this you'd need to instead call functions which return
a transaction status, but then:
 (a) you need to restructure the translate.c code so it can
 deal with the idea of backing out if the instruction isn't
 actually present
 (b) this still wouldn't work for linux-user mode, where we
 don't have any way to say "do a memory access, but let me know
 if it would fail rather than longjmping"

thanks
-- PMM

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

[Sergey Fedorov]

On 09.10.2015 17:04, Peter Maydell wrote:
> On 9 October 2015 at 14:59, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 08.10.2015 21:40, Peter Maydell wrote:
>>> Annoying corner case which I don't think we need to handle necessarily:
>>> if you set a breakpoint on a 32-bit Thumb instruction which spans a page
>>> boundary, and the second page is not present, we will end up taking the
>>> page fault when I think we should take the breakpoint. I can't think
>>> of a way to get that right, so just commenting that it isn't handled
>>> right would do.
>> Could you please point out the piece of code which will generate the
>> page fault? Maybe I will give it a thought :)
> When you call arm_ldl_code() and friends, they will end up longjmp()ing
> out of the codegen phase if the load faults. This then turns into a
> guest-visible fault in the usual way.
>
> To avoid this you'd need to instead call functions which return
> a transaction status, but then:
>  (a) you need to restructure the translate.c code so it can
>  deal with the idea of backing out if the instruction isn't
>  actually present
>  (b) this still wouldn't work for linux-user mode, where we
>  don't have any way to say "do a memory access, but let me know
>  if it would fail rather than longjmping"

Thank you for the explanation, Peter. I see, if we do insn translation
then we take the page fault instead of the CPU breakpoint. As of user
mode, can we actually set any CPU breakpoint? If not, as I guess, then
(b) is not applicable to the possible solution that you have described.

Best regards,
Sergey

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

[Peter Maydell]

On 9 October 2015 at 16:55, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> Thank you for the explanation, Peter. I see, if we do insn translation
> then we take the page fault instead of the CPU breakpoint. As of user
> mode, can we actually set any CPU breakpoint? If not, as I guess, then
> (b) is not applicable to the possible solution that you have described.

You're right, we can only have GDB breakpoints in user-mode.

-- PMM

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

[Sergey Fedorov]

On 09.10.2015 17:04, Peter Maydell wrote:
> On 9 October 2015 at 14:59, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 08.10.2015 21:40, Peter Maydell wrote:
>>> Annoying corner case which I don't think we need to handle necessarily:
>>> if you set a breakpoint on a 32-bit Thumb instruction which spans a page
>>> boundary, and the second page is not present, we will end up taking the
>>> page fault when I think we should take the breakpoint. I can't think
>>> of a way to get that right, so just commenting that it isn't handled
>>> right would do.
>> Could you please point out the piece of code which will generate the
>> page fault? Maybe I will give it a thought :)
> When you call arm_ldl_code() and friends, they will end up longjmp()ing
> out of the codegen phase if the load faults. This then turns into a
> guest-visible fault in the usual way.
>
> To avoid this you'd need to instead call functions which return
> a transaction status, but then:
>  (a) you need to restructure the translate.c code so it can
>  deal with the idea of backing out if the instruction isn't
>  actually present
>  (b) this still wouldn't work for linux-user mode, where we
>  don't have any way to say "do a memory access, but let me know
>  if it would fail rather than longjmping"

Hm... Seems we will get the page fault instead of the CPU breakpoint not
only on a 32-bit Thumb instruction which spans a page boundary but on
every instruction translation fetch when the page is not present. Do I
understand it correctly?

Best regards,
Sergey

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

[Sergey Fedorov]

On 08.10.2015 21:40, Peter Maydell wrote:
> Annoying corner case which I don't think we need to handle necessarily:
> if you set a breakpoint on a 32-bit Thumb instruction which spans a page
> boundary, and the second page is not present, we will end up taking the
> page fault when I think we should take the breakpoint. I can't think
> of a way to get that right, so just commenting that it isn't handled
> right would do.

What I found in ARM ARMv8 (DDI0487A_g) section G2.9.1 About Breakpoint
exceptions:

For each instruction in the program flow, all of the breakpoints are
tested. When a breakpoint is tested, it generates
a Breakpoint debug event if all of the following are true:
• The breakpoint is enabled. That is, the breakpoint enable control for
it, DBGBCR<n>.E, is 1.
• The conditions specified in the DBGBCR<n> are met.
• The comparisons with the values held in one or both of the DBGBVR<n>
and DBGBXVR<n>, as applicable,
are successful.
• If the breakpoint is linked to another breakpoint, the comparisons
made by that other breakpoint are also
successful.
• The instruction is committed for execution.

If I understand correctly, the last item in the list specifies that any
page fault exception which would occur in the normal instruction
execution has more priority than the breakpoint exception. If so,
Everything should be okay.

Best regards,
Sergey

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

[Peter Maydell]

On 12 October 2015 at 13:41, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> If I understand correctly, the last item in the list specifies that any
> page fault exception which would occur in the normal instruction
> execution has more priority than the breakpoint exception. If so,
> Everything should be okay.

Oh, good catch. This is more clearly noted in the sections
about synchronous exception prioritization in D1.13.2 and
G1.11.2, which clearly list prefetch abort as higher priority
than breakpoints.

thanks
-- PMM