Re: [Qemu-devel] [PATCH] target-arm: fix CPU breakpoint handling

[Peter Maydell]

On 14 September 2015 at 11:51, Sergey Fedorov <address@hidden> 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 exceptoin hanlder.

"exception".

>
> Generate a call to helper to check CPU breakpoints and raise an
> exception only if any breakpoint architecturally matches.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> ---
>  target-arm/helper.h        |  2 ++
>  target-arm/op_helper.c     | 20 +++++++++++++++++++-
>  target-arm/translate-a64.c | 12 +++++++-----
>  target-arm/translate.c     | 12 +++++++-----
>  4 files changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 827b33d..c2a85c7 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -54,6 +54,8 @@ DEF_HELPER_1(yield, void, env)
>  DEF_HELPER_1(pre_hvc, void, env)
>  DEF_HELPER_2(pre_smc, void, env, i32)
>
> +DEF_HELPER_1(check_breakpoints, void, env)
> +
>  DEF_HELPER_3(cpsr_write, void, env, i32, i32)
>  DEF_HELPER_1(cpsr_read, i32, env)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index b298e57..24dcefd 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -877,6 +877,15 @@ bool arm_debug_check_watchpoint(CPUState *cs)
>      return check_watchpoints(cpu);
>  }
>
> +void HELPER(check_breakpoints)(CPUARMState *env)
> +{
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +
> +    if (check_breakpoints(cpu)) {
> +        HELPER(exception_internal(env, EXCP_DEBUG));
> +    }
> +}
> +
>  void arm_debug_excp_handler(CPUState *cs)
>  {
>      /* Called by core code when a watchpoint or breakpoint fires;
> @@ -904,7 +913,16 @@ void arm_debug_excp_handler(CPUState *cs)
>                      arm_debug_target_el(env));
>          }
>      } else {
> -        if (check_breakpoints(cpu)) {
> +        CPUBreakpoint *bp;
> +        uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
> +
> +        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> +            if (bp->pc == pc && !(bp->flags & BP_CPU)) {
> +                return;
> +            }
> +        }

This extra code looks right, but isn't it fixing a different bug?

> +
> +        {

Rather than adding the extra block I would just de-indent the code
that used to live inside the if() and move the variable declaration
to the top of the new block.

>              bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
>              if (extended_addresses_enabled(env)) {
>                  env->exception.fsr = (1 << 9) | 0x22;
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 5c13e15..a5927fd 100644
> --- 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?

thanks
-- PMM