From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:53178)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <serge.fdrv@gmail.com>) id 1ZcybU-0000mo-SN
	for qemu-devel@nongnu.org; Fri, 18 Sep 2015 12:33:41 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <serge.fdrv@gmail.com>) id 1ZcybR-0000dp-MH
	for qemu-devel@nongnu.org; Fri, 18 Sep 2015 12:33:40 -0400
Received: from mail-lb0-x22c.google.com ([2a00:1450:4010:c04::22c]:35429)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <serge.fdrv@gmail.com>) id 1ZcybR-0000dY-EP
	for qemu-devel@nongnu.org; Fri, 18 Sep 2015 12:33:37 -0400
Received: by lbpo4 with SMTP id o4so27928472lbp.2
	for <qemu-devel@nongnu.org>; Fri, 18 Sep 2015 09:33:36 -0700 (PDT)
References: <1442227888-11467-1-git-send-email-serge.fdrv@gmail.com>
	<CAFEAcA9qy5=BNE3LAUtrLxcjHh+Yk1Jo2s=S00CP890epaZ2FA@mail.gmail.com>
	<55FC1A8E.1070401@gmail.com>
	<CAFEAcA-sULXAfJTm=ok8iengNxVg9=OcTaxbJezPrns1NXXLdw@mail.gmail.com>
From: Sergey Fedorov <serge.fdrv@gmail.com>
Message-ID: <55FC3CDE.5000007@gmail.com>
Date: Fri, 18 Sep 2015 19:33:34 +0300
MIME-Version: 1.0
In-Reply-To: <CAFEAcA-sULXAfJTm=ok8iengNxVg9=OcTaxbJezPrns1NXXLdw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: QEMU Developers <qemu-devel@nongnu.org>

On 18.09.2015 17:14, Peter Maydell wrote:
> On 18 September 2015 at 15:07, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 18.09.2015 16:50, Peter Maydell wrote:
>>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.fdrv@gmail.com> 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...

Best,
Sergey