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.

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..809529990a 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, int num_insns)
 {
     struct qemu_plugin_tb *ptb = tcg_ctx->plugin_tb;

+    /* 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..4feaa47b08 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, int 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, int num_insns)
 { }

 static inline void plugin_gen_disable_mem_helpers(void)
-- 
2.34.1

Hi Matt,

On 14/7/23 06:18, Matt Borgerson wrote:
> 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.
> 
> 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/include/exec/plugin-gen.h b/include/exec/plugin-gen.h
> index 52828781bc..4feaa47b08 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, int num_insns);

num_insns is a 'size_t'.

Hi Philippe,

> num_insns is a 'size_t'.

You are right. I copied the `int` type from `DisasContextBase`, but it
should really be `size_t`. I'll send an updated patch.

Thanks,
Matt

On Fri, Jul 14, 2023 at 11:09 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Matt,
>
> On 14/7/23 06:18, Matt Borgerson wrote:
> > 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.
> >
> > 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/include/exec/plugin-gen.h b/include/exec/plugin-gen.h
> > index 52828781bc..4feaa47b08 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, int num_insns);
>
> num_insns is a 'size_t'.