Re: [PATCH v2] plugins: Set final instruction count in plugin_gen_tb_end

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

Matt Borgerson <contact@mborgerson.com> writes:

> Translation logic may partially decode an instruction, then abort and
> remove the instruction from the TB. This can happen for example when an
> instruction spans two pages. In this case, plugins may get an incorrect
> result when calling qemu_plugin_tb_n_insns to query for the number of
> instructions in the TB. This patch updates plugin_gen_tb_end to set the
> final instruction count.

For some reason this fails to apply cleanly:

  git am ./v2_20230714_contact_plugins_set_final_instruction_count_in_plugin_gen_tb_end.mbx
  Applying: plugins: Set final instruction count in plugin_gen_tb_end
  error: corrupt patch at line 31           
  Patch failed at 0001 plugins: Set final instruction count in plugin_gen_tb_end

>
> Signed-off-by: Matt Borgerson <contact@mborgerson.com>
> ---
>  accel/tcg/plugin-gen.c    | 5 ++++-
>  accel/tcg/translator.c    | 2 +-
>  include/exec/plugin-gen.h | 4 ++--
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 5c13615112..f18ecd6902 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -866,10 +866,13 @@ void plugin_gen_insn_end(void)
>   * do any clean-up here and make sure things are reset in
>   * plugin_gen_tb_start.
>   */
> -void plugin_gen_tb_end(CPUState *cpu)
> +void plugin_gen_tb_end(CPUState *cpu, size_t num_insns)
>  {
>      struct qemu_plugin_tb *ptb = tcg_ctx->plugin_tb;
>

I think it might be worth a:

  g_assert(num_insns <= ptb->n);

here to prevent misuse of the API.


> +    /* translator may have removed instructions, update final count */
> +    ptb->n = num_insns;
> +
>      /* collect instrumentation requests */
>      qemu_plugin_tb_trans_cb(cpu, ptb);
>
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 0fd9efceba..141f514886 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -215,7 +215,7 @@ void translator_loop(CPUState *cpu,
> TranslationBlock *tb, int *max_insns,
>      gen_tb_end(tb, cflags, icount_start_insn, db->num_insns);
>
>      if (plugin_enabled) {
> -        plugin_gen_tb_end(cpu);
> +        plugin_gen_tb_end(cpu, db->num_insns);
>      }
>
>      /* The disas_log hook may use these values rather than recompute.  */
> diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h
> index 52828781bc..c4552b5061 100644
> --- a/include/exec/plugin-gen.h
> +++ b/include/exec/plugin-gen.h
> @@ -20,7 +20,7 @@ struct DisasContextBase;
>
>  bool plugin_gen_tb_start(CPUState *cpu, const struct DisasContextBase *db,
>                           bool supress);
> -void plugin_gen_tb_end(CPUState *cpu);
> +void plugin_gen_tb_end(CPUState *cpu, size_t num_insns);
>  void plugin_gen_insn_start(CPUState *cpu, const struct DisasContextBase *db);
>  void plugin_gen_insn_end(void);
>
> @@ -42,7 +42,7 @@ void plugin_gen_insn_start(CPUState *cpu, const
> struct DisasContextBase *db)
>  static inline void plugin_gen_insn_end(void)
>  { }
>
> -static inline void plugin_gen_tb_end(CPUState *cpu)
> +static inline void plugin_gen_tb_end(CPUState *cpu, size_t num_insns)
>  { }
>
>  static inline void plugin_gen_disable_mem_helpers(void)


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro