[v2] plugins: Set final instruction count in plugin_gen_tb_end

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

Posted by Matt Borgerson 9 months, 4 weeks ago

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..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;

+    /* 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)
-- 
2.34.1

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

Posted by Alex Bennée 8 months, 1 week ago

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.

re-queued to plugins/next now I have rth's gusa patches.


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

Posted by Matt Borgerson 8 months, 1 week ago

Thanks Alex

On Wed, Aug 30, 2023, 14:47 Alex Bennée <alex.bennee@linaro.org> wrote:

>
> 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.
>
> re-queued to plugins/next now I have rth's gusa patches.
>
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

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

Posted by Alex Bennée 9 months, 3 weeks ago

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

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

Posted by Alex Bennée 9 months, 3 weeks ago

Alex Bennée <alex.bennee@linaro.org> writes:

> 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

I think some newlines crept in, I was able to fix. Queued to
for-8.1/misc-fixes with the assert added.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

Posted by Matt Borgerson 9 months, 3 weeks ago

Thanks Alex!


On Mon, Jul 17, 2023 at 8:34 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Alex Bennée <alex.bennee@linaro.org> writes:
>
> > 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
>
> I think some newlines crept in, I was able to fix. Queued to
> for-8.1/misc-fixes with the assert added.
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro

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

Posted by Alex Bennée 9 months, 3 weeks ago

Matt Borgerson <contact@mborgerson.com> writes:

> Thanks Alex!
>
>
> On Mon, Jul 17, 2023 at 8:34 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Alex Bennée <alex.bennee@linaro.org> writes:
>>
>> > 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
>>
>> I think some newlines crept in, I was able to fix. Queued to
>> for-8.1/misc-fixes with the assert added.

Hmm so I ran into an issue:

  ./qemu-sh4 -plugin tests/plugin/libbb.so -d plugin ./tests/tcg/sh4-linux-user/testthread
  ERROR:../../accel/tcg/plugin-gen.c:874:plugin_gen_tb_end: assertion failed: (num_insns <= ptb->n)
  Bail out! ERROR:../../accel/tcg/plugin-gen.c:874:plugin_gen_tb_end: assertion failed: (num_insns <= ptb->n)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  bb's: 9202, insns: 42264
  fish: Job 1, './qemu-sh4 -plugin tests/plugin…' terminated by signal SIGSEGV (Address boundary error)

Further investigation shows that gUSA sequences can cause the number of
instructions to run ahead, which also makes the setting of the ptb->n =
num_insns unsafe, running ahead of the number of instructions signalled
by plugin_gen_insn_start/plugin_gen_insn_end.

  Thread 1 hit Hardware watchpoint 5: *(int *) 0x7ffd410a2904
  Old value = 4
  New value = 1
  0x000055f148c00ea8 in decode_gusa (ctx=0x7ffd410a28f0, env=0x55f14a4106e8) at ../../target/sh4/translate.c:2167
  2167        ctx->base.num_insns += max_insns - 1;
  (rr) p max_insns
  $6 = 4
  (rr) p max_insns -1
  $7 = 3
  (rr) p ctx->base.num_insns
  $8 = 1

So I think we have to drop this for now until we can either fix
decode_gusa or find something else.

Richard,

Any ideas?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

Posted by Philippe Mathieu-Daudé 8 months, 2 weeks ago

On 19/7/23 00:05, Alex Bennée wrote:
> 
> Matt Borgerson <contact@mborgerson.com> writes:
> 
>> Thanks Alex!
>>
>>
>> On Mon, Jul 17, 2023 at 8:34 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>>
>>> Alex Bennée <alex.bennee@linaro.org> writes:
>>>
>>>> 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
>>>
>>> I think some newlines crept in, I was able to fix. Queued to
>>> for-8.1/misc-fixes with the assert added.
> 
> Hmm so I ran into an issue:
> 
>    ./qemu-sh4 -plugin tests/plugin/libbb.so -d plugin ./tests/tcg/sh4-linux-user/testthread
>    ERROR:../../accel/tcg/plugin-gen.c:874:plugin_gen_tb_end: assertion failed: (num_insns <= ptb->n)
>    Bail out! ERROR:../../accel/tcg/plugin-gen.c:874:plugin_gen_tb_end: assertion failed: (num_insns <= ptb->n)
>    qemu: uncaught target signal 11 (Segmentation fault) - core dumped
>    bb's: 9202, insns: 42264
>    fish: Job 1, './qemu-sh4 -plugin tests/plugin…' terminated by signal SIGSEGV (Address boundary error)
> 
> Further investigation shows that gUSA sequences can cause the number of
> instructions to run ahead, which also makes the setting of the ptb->n =
> num_insns unsafe, running ahead of the number of instructions signalled
> by plugin_gen_insn_start/plugin_gen_insn_end.
> 
>    Thread 1 hit Hardware watchpoint 5: *(int *) 0x7ffd410a2904
>    Old value = 4
>    New value = 1
>    0x000055f148c00ea8 in decode_gusa (ctx=0x7ffd410a28f0, env=0x55f14a4106e8) at ../../target/sh4/translate.c:2167
>    2167        ctx->base.num_insns += max_insns - 1;
>    (rr) p max_insns
>    $6 = 4
>    (rr) p max_insns -1
>    $7 = 3

Is this the 'fail' label? (You can check by using '-d unimp'
which would dump "Unrecognized gUSA sequence").

'fail' is followed by 'done' which updates ctx->base.num_insns.

>    (rr) p ctx->base.num_insns
>    $8 = 1
> 
> So I think we have to drop this for now until we can either fix
> decode_gusa or find something else.
> 
> Richard,
> 
> Any ideas?
> 
> 
>