Re: [Qemu-devel] [PATCH v2 04/22] target-*: Introduce and use cpu_breakpoint_test

[Peter Maydell]

On 18 September 2015 at 05:55, Richard Henderson <address@hidden> wrote:
> Reduce the boilerplate required for each target.  At the same time,
> move the test for breakpoint after calling tcg_gen_insn_start.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  include/qom/cpu.h             | 16 +++++++++++++++
>  target-alpha/translate.c      | 13 ++++--------
>  target-arm/translate-a64.c    | 20 +++++++------------
>  target-arm/translate.c        | 46 
> ++++++++++++++++++++-----------------------
>  target-cris/translate.c       | 27 ++++++++-----------------
>  target-i386/translate.c       | 17 +++++++---------
>  target-lm32/translate.c       | 25 +++++++----------------
>  target-m68k/translate.c       | 18 ++++++-----------
>  target-microblaze/translate.c | 36 ++++++++++++---------------------
>  target-mips/translate.c       | 25 ++++++++++-------------
>  target-moxie/translate.c      | 19 +++++++-----------
>  target-openrisc/translate.c   | 24 +++++++---------------
>  target-ppc/translate.c        | 14 +++++--------
>  target-s390x/translate.c      | 16 ++++++---------
>  target-sh4/translate.c        | 20 ++++++++-----------
>  target-sparc/translate.c      | 23 ++++++++++------------
>  target-unicore32/translate.c  | 24 ++++++++++------------
>  target-xtensa/translate.c     | 25 +++++++----------------
>  18 files changed, 159 insertions(+), 249 deletions(-)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 302673d..e11dca3 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -679,6 +679,7 @@ void cpu_single_step(CPUState *cpu, int enabled);
>  /* 0x08 currently unused */
>  #define BP_GDB                0x10
>  #define BP_CPU                0x20
> +#define BP_ANY                (BP_GDB | BP_CPU)
>  #define BP_WATCHPOINT_HIT_READ 0x40
>  #define BP_WATCHPOINT_HIT_WRITE 0x80
>  #define BP_WATCHPOINT_HIT (BP_WATCHPOINT_HIT_READ | BP_WATCHPOINT_HIT_WRITE)
> @@ -689,6 +690,21 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int 
> flags);
>  void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *breakpoint);
>  void cpu_breakpoint_remove_all(CPUState *cpu, int mask);
>
> +/* Return true if PC matches an installed breakpoint.  */
> +static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
> +{
> +    CPUBreakpoint *bp;
> +
> +    if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
> +        QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
> +            if (bp->pc == pc && (bp->flags & mask)) {
> +                return true;
> +            }
> +        }
> +    }
> +    return false;
> +}

This won't work with the fix for ARM breakpoints Sergey currently has
on list: http://patchwork.ozlabs.org/patch/517359/
where we need to behave differently for "there's a GDB breakpoint
here" and "there's a CPU breakpoint here" (because the complex
conditions on the latter require us to call a helper function to
see if we need to actually generate an EXCP_DEBUG exception).

> @@ -2913,14 +2912,6 @@ static inline void 
> gen_intermediate_code_internal(AlphaCPU *cpu,
>
>      gen_tb_start(tb);
>      do {
> -        if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
> -            QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -                if (bp->pc == ctx.pc) {
> -                    gen_excp(&ctx, EXCP_DEBUG, 0);
> -                    break;
> -                }
> -            }
> -        }
>          if (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -2936,6 +2927,10 @@ static inline void 
> gen_intermediate_code_internal(AlphaCPU *cpu,
>          tcg_gen_insn_start(ctx.pc);
>          num_insns++;
>
> +        if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
> +            gen_excp(&ctx, EXCP_DEBUG, 0);
> +            break;
> +        }
>          if (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 4670941..c1efd30 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -11007,7 +11007,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
>      DisasContext dc1, *dc = &dc1;
> -    CPUBreakpoint *bp;
>      int j, lj;
>      target_ulong pc_start;
>      target_ulong next_page_start;
> @@ -11079,18 +11078,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>      tcg_clear_temp_count();
>
>      do {
> -        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 (search_pc) {
>              j = tcg_op_buf_count();
>              if (lj < j) {
> @@ -11106,6 +11093,13 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>          tcg_gen_insn_start(dc->pc);
>          num_insns++;
>
> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> +            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 (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>              gen_io_start();
>          }

Do you know why some but not all targets do this "advance PC"
thing if there's a breakpoint?

thanks
-- PMM