Re: [PATCH 9/9] accel/tcg: Improve can_do_io management

[Gregory Price]

On Sat, Apr 06, 2024 at 12:32:48PM -1000, Richard Henderson wrote:
> We already attempted to set and clear can_do_io before the first
> and last insns, but only used the initial value of max_insns and
> the call to translator_io_start to find those insns.
> 
> Now that we track insn_start in DisasContextBase, and now that
> we have emit_before_op, we can wait until we have finished
> translation to identify the true first and last insns and emit
> the sets of can_do_io at that time.
> 
> This fixes case of a translation block which crossed a page boundary,
> and for which the second page turned out to be mmio.

I love when I get to say this: I knew it! :D

https://lore.kernel.org/qemu-devel/ZbvVB4J+AHkLNuE2@memverge.com/

Great fix, much appreciate the effort!

Reviewed-by: Gregory Price <gregory.price@memverge.com>

> In this case we
> truncate the block, and the previous logic for can_do_io could leave
> a block with a single insn with can_do_io set to false, which would
> fail an assertion in cpu_io_recompile.
> 
> Reported-by: Jørgen Hansen <Jorgen.Hansen@wdc.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/translator.h |  1 -
>  accel/tcg/translator.c    | 45 ++++++++++++++++++++-------------------
>  2 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index ceaeca8c91..2c4fb818e7 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -87,7 +87,6 @@ typedef struct DisasContextBase {
>      int num_insns;
>      int max_insns;
>      bool singlestep_enabled;
> -    int8_t saved_can_do_io;
>      bool plugin_enabled;
>      struct TCGOp *insn_start;
>      void *host_addr[2];
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index ae61c154c2..9de0bc34c8 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -18,20 +18,14 @@
>  
>  static void set_can_do_io(DisasContextBase *db, bool val)
>  {
> -    if (db->saved_can_do_io != val) {
> -        db->saved_can_do_io = val;
> -
> -        QEMU_BUILD_BUG_ON(sizeof_field(CPUState, neg.can_do_io) != 1);
> -        tcg_gen_st8_i32(tcg_constant_i32(val), tcg_env,
> -                        offsetof(ArchCPU, parent_obj.neg.can_do_io) -
> -                        offsetof(ArchCPU, env));
> -    }
> +    QEMU_BUILD_BUG_ON(sizeof_field(CPUState, neg.can_do_io) != 1);
> +    tcg_gen_st8_i32(tcg_constant_i32(val), tcg_env,
> +                    offsetof(ArchCPU, parent_obj.neg.can_do_io) -
> +                    offsetof(ArchCPU, env));
>  }
>  
>  bool translator_io_start(DisasContextBase *db)
>  {
> -    set_can_do_io(db, true);
> -
>      /*
>       * Ensure that this instruction will be the last in the TB.
>       * The target may override this to something more forceful.
> @@ -84,13 +78,6 @@ static TCGOp *gen_tb_start(DisasContextBase *db, uint32_t 
> cflags)
>                           - offsetof(ArchCPU, env));
>      }
>  
> -    /*
> -     * cpu->neg.can_do_io is set automatically here at the beginning of
> -     * each translation block.  The cost is minimal, plus it would be
> -     * very easy to forget doing it in the translator.
> -     */
> -    set_can_do_io(db, db->max_insns == 1);
> -
>      return icount_start_insn;
>  }
>  
> @@ -129,6 +116,7 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, 
> int *max_insns,
>  {
>      uint32_t cflags = tb_cflags(tb);
>      TCGOp *icount_start_insn;
> +    TCGOp *first_insn_start = NULL;
>      bool plugin_enabled;
>  
>      /* Initialize DisasContext */
> @@ -139,7 +127,6 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, 
> int *max_insns,
>      db->num_insns = 0;
>      db->max_insns = *max_insns;
>      db->singlestep_enabled = cflags & CF_SINGLE_STEP;
> -    db->saved_can_do_io = -1;
>      db->insn_start = NULL;
>      db->host_addr[0] = host_pc;
>      db->host_addr[1] = NULL;
> @@ -159,6 +146,9 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, 
> int *max_insns,
>          *max_insns = ++db->num_insns;
>          ops->insn_start(db, cpu);
>          db->insn_start = tcg_last_op();
> +        if (first_insn_start == NULL) {
> +            first_insn_start = db->insn_start;
> +        }
>          tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
>  
>          if (plugin_enabled) {
> @@ -171,10 +161,6 @@ void translator_loop(CPUState *cpu, TranslationBlock 
> *tb, int *max_insns,
>           * done next -- either exiting this loop or locate the start of
>           * the next instruction.
>           */
> -        if (db->num_insns == db->max_insns) {
> -            /* Accept I/O on the last instruction.  */
> -            set_can_do_io(db, true);
> -        }
>          ops->translate_insn(db, cpu);
>  
>          /*
> @@ -207,6 +193,21 @@ void translator_loop(CPUState *cpu, TranslationBlock 
> *tb, int *max_insns,
>      ops->tb_stop(db, cpu);
>      gen_tb_end(tb, cflags, icount_start_insn, db->num_insns);
>  
> +    /*
> +     * Manage can_do_io for the translation block: set to false before
> +     * the first insn and set to true before the last insn.
> +     */
> +    if (db->num_insns == 1) {
> +        tcg_debug_assert(first_insn_start == db->insn_start);
> +    } else {
> +        tcg_debug_assert(first_insn_start != db->insn_start);
> +        tcg_ctx->emit_before_op = first_insn_start;
> +        set_can_do_io(db, false);
> +    }
> +    tcg_ctx->emit_before_op = db->insn_start;
> +    set_can_do_io(db, true);
> +    tcg_ctx->emit_before_op = NULL;
> +
>      if (plugin_enabled) {
>          plugin_gen_tb_end(cpu, db->num_insns);
>      }
> -- 
> 2.34.1
> 
>