Re: [Qemu-devel] ARM softmmu breakpoint misbehavior

[Peter Maydell]

On 24 August 2015 at 18:36, Sergey Fedorov <address@hidden> wrote:
> Hi all,
>
> Seems there is a bug in ARM breakpoint emulation. I am not sure how to
> fix it and I would appreciate any suggestion. It is best illustrated by
> a simple test which sets up and enables an unlinked address match
> breakpoint but does not enable debug exceptions globally by
> MDSCR_EL1.MDE bit.

Thanks for the test case, that's very useful.

> I managed to avoid this segmentation fault with this patch:
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 663c05d..223b939 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -889,6 +889,15 @@ void arm_debug_excp_handler(CPUState *cs)
>              }
>          }
>      } else {
> +        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;
> +            }
> +        }
> +
>          if (check_breakpoints(cpu)) {
>              bool same_el = (arm_debug_target_el(env) ==
> arm_current_el(env));
>              if (extended_addresses_enabled(env)) {
> @@ -900,6 +909,8 @@ void arm_debug_excp_handler(CPUState *cs)
>              raise_exception(env, EXCP_PREFETCH_ABORT,
>                              syn_breakpoint(same_el),
>                              arm_debug_target_el(env));
> +        } else {
> +            cpu_resume_from_signal(cs, NULL);
>          }
>      }
>  }
>
> The patch adds a check for non-CPU breakpoints first, then calls
> cpu_resume_from_signal() if no CPU breakpoint matches.

This approach won't work (as you've noticed :-)), because
it's trying to say "ignore the bp hit and continue execution",
but when we translated code for this TB all we generated was
a TB that says "generate debug exception". We never generated
a TB with the real code for the instruction in it, so trying
to resume will just cause us to raise the debug exception
again.

I think what we need to do is have the translate-a64.c
code be smarter, and actually generate the real code
if we're not going to really hit the bp. Except that we
don't really have all the info in the flags to know for
sure about that. So we probably need to do something like
generating a call to a helper which checks whether this
bp should hit and doesn't throw the exception unless it
has to, with the actual code for the insn following.
I need to think about how this ought to work...

The watchpoint code has a chance of cpu_resume_from_signal
doing the right thing, because we really did have the
code to do the load/store. However I have a feeling this
won't interact properly with the fact that ARM needs
BP_STOP_BEFORE_ACCESS on its watchpoints (unlike x86, which
is where I was looking at when I wrote the ARM wp handling
code.) So we may well be broken there as well in the
case where check_watchpoints() returns false.

thanks
-- PMM