Re: [PATCH v2 10/10] accel/tcg: Encode breakpoint info into tb->cflags

[Peter Maydell]

On Mon, 12 Jul 2021 at 16:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Having this data in cflags means that hashing takes care
> of selecting a TB with or without exceptions built in.
> Which means that we no longer need to flush all TBs.
>
> This does require that we single-step while we're within a page
> that contains a breakpoint, so it's not yet ideal, but should be
> an improvement over some corner-case slowdowns.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/404
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/exec-all.h   |  7 ++++
>  accel/tcg/cpu-exec.c      | 68 ++++++++++++++++++++++++++++++-
>  accel/tcg/translate-all.c |  4 --
>  accel/tcg/translator.c    | 85 +++++++++++++++++++++------------------
>  cpu.c                     | 24 -----------
>  5 files changed, 119 insertions(+), 69 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 6873cce8df..7ab2578f71 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -502,9 +502,16 @@ struct TranslationBlock {
>  #define CF_USE_ICOUNT    0x00020000
>  #define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held 
> */
>  #define CF_PARALLEL      0x00080000 /* Generate code for a parallel context 
> */
> +#define CF_BP_MASK       0x00300000 /* See below */
> +#define CF_BP_SHIFT      20
>  #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
>  #define CF_CLUSTER_SHIFT 24
>
> +#define CF_BP_NONE       (0 << CF_BP_SHIFT) /* TB does not interact with BPs 
> */
> +#define CF_BP_SSTEP      (1 << CF_BP_SHIFT) /* gdbstub single-step in effect 
> */
> +#define CF_BP_GDB        (2 << CF_BP_SHIFT) /* gdbstub breakpoint at tb->pc 
> */
> +#define CF_BP_CPU        (3 << CF_BP_SHIFT) /* arch breakpoint at tb->pc */
> +
>      /* Per-vCPU dynamic tracing state used to generate this TB */
>      uint32_t trace_vcpu_dstate;
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 4d043a11aa..179a425ece 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -222,6 +222,65 @@ static inline void log_cpu_exec(target_ulong pc, 
> CPUState *cpu,
>      }
>  }
>
> +static uint32_t cflags_for_breakpoints(CPUState *cpu, target_ulong pc,
> +                                       uint32_t cflags)
> +{
> +    uint32_t bflags = 0;
> +
> +    if (unlikely(cpu->singlestep_enabled)) {
> +        bflags = CF_BP_SSTEP;
> +    } else {

Won't this ignore breakpoints when singlestepping ?

-- PMM