From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50700) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoxvI-0006Wa-Ux for qemu-devel@nongnu.org; Wed, 21 Oct 2015 14:15:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zoxv8-0004hi-K8 for qemu-devel@nongnu.org; Wed, 21 Oct 2015 14:15:40 -0400 Received: from mail-lb0-x231.google.com ([2a00:1450:4010:c04::231]:33399) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zoxv8-0004he-2m for qemu-devel@nongnu.org; Wed, 21 Oct 2015 14:15:30 -0400 Received: by lbbec13 with SMTP id ec13so42553006lbb.0 for ; Wed, 21 Oct 2015 11:15:29 -0700 (PDT) References: <1445003887-14475-1-git-send-email-peter.maydell@linaro.org> <1445003887-14475-14-git-send-email-peter.maydell@linaro.org> From: Sergey Fedorov Message-ID: <5627D63E.5080404@gmail.com> Date: Wed, 21 Oct 2015 21:15:26 +0300 MIME-Version: 1.0 In-Reply-To: <1445003887-14475-14-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 13/13] target-arm: Fix CPU breakpoint handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , qemu-devel@nongnu.org On 16.10.2015 16:58, Peter Maydell wrote: > From: 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 > Reviewed-by: Peter Maydell > Signed-off-by: Peter Maydell > --- > 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(-) > (snip) > diff --git a/target-arm/translate.c b/target-arm/translate.c > index 1273000..9f1d740 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -11342,11 +11342,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; > } > } > } It turns out that this change introduced an issue which can be illustrated by the following test: cat >test.s <is_jmp = DISAS_UPDATE'. With the following patch everything is okay: diff --git a/target-arm/translate.c b/target-arm/translate.c index 9f1d740..b55c5c2 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -11345,7 +11345,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb) 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 As far as I understand, we can't do this in target-arm/translate.c before dc->pc is advanced properly because CPU state's PC doesn't get updated as in target-arm/translate-a64.c. Compare: target-arm/translate.c: case DISAS_JUMP: case DISAS_UPDATE: /* indicate that the hash table must be used to find the next TB */ tcg_gen_exit_tb(0); break; target-arm/translate-a64.c: case DISAS_UPDATE: gen_a64_set_pc_im(dc->pc); /* fall through */ case DISAS_JUMP: /* indicate that the hash table must be used to find the next TB */ tcg_gen_exit_tb(0); break; I think we could fix this problem by cleaning up DISAS_UPDATE usage in target-arm/translate.c and implementing PC update as in target-arm/translate-a64.c. I could prepare a patch for that. Another problem, I think, is that we should somehow restore the CPU state before raising an exception from check_breakpoints() helper. But so far I have no idea how to fix this... Any suggestions are highly appreciated :) Best regards, Sergey