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

[Richard Henderson <richard.henderson@linaro.org>]

On 7/19/21 11:22 AM, Richard Henderson wrote:
> 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    | 12 ++++++++++++
>   accel/tcg/translator.c  |  7 +------
>   cpu.c                   |  4 ----
>   4 files changed, 14 insertions(+), 10 deletions(-)

Bah, this should also contain

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/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 6710e15d8b..30a3be9ea7 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -272,6 +272,14 @@ static uint32_t cflags_for_breakpoints(CPUState *cpu, target_ulong pc,
>           }
>       }
>   
> +    if (unlikely(cpu->singlestep_enabled)) {
> +        /*
> +         * Record gdb single-step.  We should be exiting the TB by raising
> +         * EXCP_DEBUG, but to simplify other tests, disable chaining too.
> +         */
> +        bflags = CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP | 1;
> +    }
> +
>       if (unlikely(bflags)) {
>           cflags = (cflags & ~CF_COUNT_MASK) | bflags;
>       }
> @@ -409,7 +417,11 @@ void cpu_exec_step_atomic(CPUState *cpu)
>            * We only arrive in cpu_exec_step_atomic after beginning execution
>            * of an insn that includes an atomic operation we can't handle.
>            * Any breakpoint for this insn will have been recognized earlier.
> +         * But do record single-stepping.
>            */
> +        if (unlikely(cpu->singlestep_enabled)) {
> +            cflags |= CF_SINGLE_STEP;
> +        }
>   
>           tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
>           if (tb == NULL) {
> 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 660b56f431..addcb5db9c 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -316,10 +316,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);
>       }
>