Re: [PATCH for-6.1 v6 17/17] accel/tcg: Record singlestep_enabled in tb->cflags

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> Set CF_SINGLE_STEP when single-stepping is enabled.
> This avoids the need to flush all tb's when turning
> single-stepping on or off.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/exec-all.h   | 1 +
>  accel/tcg/cpu-exec.c      | 7 ++++++-
>  accel/tcg/translate-all.c | 4 ----
>  accel/tcg/translator.c    | 7 +------
>  cpu.c                     | 4 ----
>  5 files changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 6873cce8df..5d1b6d80fb 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -497,6 +497,7 @@ struct TranslationBlock {
>  #define CF_COUNT_MASK    0x000001ff
>  #define CF_NO_GOTO_TB    0x00000200 /* Do not chain with goto_tb */
>  #define CF_NO_GOTO_PTR   0x00000400 /* Do not chain with goto_ptr */
> +#define CF_SINGLE_STEP   0x00000800 /* gdbstub single-step in effect */
>  #define CF_LAST_IO       0x00008000 /* Last insn may be an IO access.  */
>  #define CF_MEMI_ONLY     0x00010000 /* Only instrument memory ops */
>  #define CF_USE_ICOUNT    0x00020000
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 5cc6363f4c..fc895cf51e 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu)
>      uint32_t cflags = cpu->tcg_cflags;
>  
>      /*
> +     * Record gdb single-step.  We should be exiting the TB by raising
> +     * EXCP_DEBUG, but to simplify other tests, disable chaining too.
> +     *
>       * For singlestep and -d nochain, suppress goto_tb so that
>       * we can log -d cpu,exec after every TB.
>       */
> -    if (singlestep) {
> +    if (unlikely(cpu->singlestep_enabled)) {
> +        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP |
>      1;

What does CF_SINGLE_STEP achieve that isn't already handled by having:

  cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1;

(btw did we mask CF_COUNT_MASK somewhere else?). Because surely the
CF_COUNT is part of cflags so limits the TB's that could be returned
anyway?


> +    } else if (singlestep) {
>          cflags |= CF_NO_GOTO_TB | 1;
>      } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>          cflags |= CF_NO_GOTO_TB;
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index bf82c15aab..bbfcfb698c 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1432,10 +1432,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>      }
>      QEMU_BUILD_BUG_ON(CF_COUNT_MASK + 1 != TCG_MAX_INSNS);
>  
> -    if (cpu->singlestep_enabled) {
> -        max_insns = 1;
> -    }
> -
>   buffer_overflow:
>      tb = tcg_tb_alloc(tcg_ctx);
>      if (unlikely(!tb)) {
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index b45337f3ba..c53a7f8e44 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -38,11 +38,6 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
>          return false;
>      }
>  
> -    /* Suppress goto_tb in the case of single-steping.  */
> -    if (db->singlestep_enabled) {
> -        return false;
> -    }
> -
>      /* Check for the dest on the same page as the start of the TB.  */
>      return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0;
>  }
> @@ -60,7 +55,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>      db->is_jmp = DISAS_NEXT;
>      db->num_insns = 0;
>      db->max_insns = max_insns;
> -    db->singlestep_enabled = cpu->singlestep_enabled;
> +    db->singlestep_enabled = cflags & CF_SINGLE_STEP;
>  
>      ops->init_disas_context(db, cpu);
>      tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
> diff --git a/cpu.c b/cpu.c
> index d6ae5ae581..e1799a15bc 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -326,10 +326,6 @@ void cpu_single_step(CPUState *cpu, int enabled)
>          cpu->singlestep_enabled = enabled;
>          if (kvm_enabled()) {
>              kvm_update_guest_debug(cpu, 0);
> -        } else {
> -            /* must flush all the translated code to avoid inconsistencies */
> -            /* XXX: only flush what is necessary */
> -            tb_flush(cpu);
>          }
>          trace_breakpoint_singlestep(cpu->cpu_index, enabled);
>      }


-- 
Alex Bennée