From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46281) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkYGh-0005x8-MQ for qemu-devel@nongnu.org; Fri, 09 Oct 2015 10:03:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkYGd-0000VS-KI for qemu-devel@nongnu.org; Fri, 09 Oct 2015 10:03:31 -0400 Received: from mail-lb0-x235.google.com ([2a00:1450:4010:c04::235]:32842) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkYGd-0000VN-DS for qemu-devel@nongnu.org; Fri, 09 Oct 2015 10:03:27 -0400 Received: by lbos8 with SMTP id s8so81199925lbo.0 for ; Fri, 09 Oct 2015 07:03:26 -0700 (PDT) References: <1443434870-5702-1-git-send-email-serge.fdrv@gmail.com> <1443434870-5702-3-git-send-email-serge.fdrv@gmail.com> <5617C6D1.5080608@gmail.com> From: Sergey Fedorov Message-ID: <5617C92C.4050304@gmail.com> Date: Fri, 9 Oct 2015 17:03:24 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Fix CPU breakpoint handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On 09.10.2015 17:00, Peter Maydell wrote: > On 9 October 2015 at 14:53, Sergey Fedorov wrote: >> On 08.10.2015 21:40, Peter Maydell wrote: >>> On 28 September 2015 at 11:07, Sergey Fedorov 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 >>>> 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.