[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Fix CPU breakpoint handling
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Fix CPU breakpoint handling |
Date: |
Thu, 8 Oct 2015 19:40:36 +0100 |
On 28 September 2015 at 11:07, Sergey Fedorov <address@hidden> 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 <address@hidden>
> 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