Re: [Qemu-devel] [PULL 13/13] target-arm: Fix CPU breakpoint handling

[Sergey Fedorov <serge.fdrv@gmail.com>]

On 16.10.2015 16:58, Peter Maydell wrote:
> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> 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 <serge.fdrv@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  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 <<EOF
.text
.global _start
_start:
adr r0, bp
mcr p14, 0, r0, c0, c0, 4 // DBGBVR0
mov r0, #1
orr r0, r0, #(0xf << 5)
mcr p14, 0, r0, c0, c0, 5 // DBGBCR0
bp:
nop
wfi
b .
EOF

arm-linux-gnueabi-as -o test.o test.s
arm-linux-gnueabi-ld -Ttext=0x40000000 -o test.elf test.o
./qemu-system-arm -nographic -machine virt -cpu cortex-a15 -kernel \
test.elf -D qemu.log -d in_asm,exec -singlestep

Actually, that is the same test as in
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02715.html but
for AArch32.

Running this test QEMU hangs executing code at the address where
breakpoint is set:

----------------
IN:
0x40000000:  e28f000c      add  r0, pc, #12     ; 0xc

Trace 0x7f7c8bdc0028 [40000000]
----------------
IN:
0x40000004:  ee000e90      mcr  14, 0, r0, cr0, cr0, {4}

Trace 0x7f7c8bdc0070 [40000004]
----------------
IN:
0x40000008:  e3a00001      mov  r0, #1  ; 0x1

Trace 0x7f7c8bdc00b0 [40000008]
----------------
IN:
0x4000000c:  e3800e1e      orr  r0, r0, #480    ; 0x1e0

Trace 0x7f7c8bdc00f0 [4000000c]
----------------
IN:
0x40000010:  ee000eb0      mcr  14, 0, r0, cr0, cr0, {5}

Trace 0x7f7c8bdc0140 [40000010]
----------------
IN:
0x40000014:  e1a00000      nop                  (mov r0,r0)

Trace 0x7f7c8bdc0180 [40000014]
Trace 0x7f7c8bdc0180 [40000014]
Trace 0x7f7c8bdc0180 [40000014]
Trace 0x7f7c8bdc0180 [40000014]
...

I can conclude that it is due to 'dc->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