From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45033) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZczEn-0004UQ-4N for qemu-devel@nongnu.org; Fri, 18 Sep 2015 13:14:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZczEj-0002za-TG for qemu-devel@nongnu.org; Fri, 18 Sep 2015 13:14:17 -0400 Received: from mail-lb0-x234.google.com ([2a00:1450:4010:c04::234]:34320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZczEj-0002zW-Ki for qemu-devel@nongnu.org; Fri, 18 Sep 2015 13:14:13 -0400 Received: by lbbmp1 with SMTP id mp1so28412417lbb.1 for ; Fri, 18 Sep 2015 10:14:12 -0700 (PDT) References: <1442227888-11467-1-git-send-email-serge.fdrv@gmail.com> <55FC1A8E.1070401@gmail.com> <55FC3CDE.5000007@gmail.com> From: Sergey Fedorov Message-ID: <55FC4662.60901@gmail.com> Date: Fri, 18 Sep 2015 20:14:10 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] 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 18.09.2015 19:36, Peter Maydell wrote: > On 18 September 2015 at 17:33, Sergey Fedorov wrote: >> On 18.09.2015 17:14, Peter Maydell wrote: >>> On 18 September 2015 at 15:07, Sergey Fedorov wrote: >>>> On 18.09.2015 16:50, Peter Maydell wrote: >>>>> On 14 September 2015 at 11:51, Sergey Fedorov wrote: >>>>>> --- a/target-arm/translate-a64.c >>>>>> +++ b/target-arm/translate-a64.c >>>>>> @@ -11000,11 +11000,13 @@ 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); >>>>>> + break; >>>>>> + } else { >>>>>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG); >>>>>> + goto done_generating; >>>>>> + } >>>>> You seem to have dropped the "advance the PC" code -- why is that ok? >>>>> >>>> I also dropped the immediately following goto statement. With these >>>> changes PC is advanced in the same way as it happens during normal >>>> translation. That is because we actually have to do the instruction >>>> translation process here to support the case when a breakpoint with >>>> matching PC is architecturally mismatched. As I understand, that >>>> "advance the PC" code was necessary to produce a TB with non-zero size >>>> so that it can be invalidated later when we clear the breakpoint. >>> OK, that makes sense for the BP_CPU case but you still have the >>> "goto done_generating;" in the else clause... >>> >>> Also, should we maybe make this TB be only one insn long even for >>> the BP_CPU case? It seems like in the common case we will only >>> execute one insn. >>> >> Right, I have to fix this PC advancement. But I can't think of why we >> will only execute one insn... > Well, typically we'll take the BP exception in the helper function. > There's nothing inherently wrong with translating further code > after this insn, but it's usually not going to be executed, so > we might as well end the TB early. > Thank, it is clear now. What about getting rid of "goto done generating" and always translate one insn to update PC accordingly? Sometimes in_asm log complains about that when disassembler and translator disagree about the instruction size. Best, Sergey