[PATCH 0/4] plugin patches to fix #1381

[PATCH 0/4] plugin patches to fix #1381

[Emilio Cota]

Hi,

These are the plugin fixes that I mentioned here:
  https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg02865.html

They should fix https://gitlab.com/qemu-project/qemu/-/issues/1381

Thanks,
		Emilio

[PATCH 1/4] plugins: fix optimization in plugin_gen_disable_mem_helpers

[Emilio Cota]

We were mistakenly checking tcg_ctx->plugin_insn as a canary to know
whether the TB had emitted helpers that might have accessed memory.

The problem is that tcg_ctx->plugin_insn gets updated on every
instruction in the TB, which results in us wrongly performing the
optimization (i.e. not clearing cpu->plugin_mem_cbs) way too often,
since it's not rare that the last instruction in the TB doesn't
use helpers.

Fix it by tracking a per-TB canary.

While at it, expand documentation.

Related: #1381

Signed-off-by: Emilio Cota <cota@braap.org>
---
 accel/tcg/plugin-gen.c | 26 ++++++++++++++++++--------
 include/qemu/plugin.h  |  7 +++++++
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index c7d6514840..17a686bd9e 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -579,7 +579,8 @@ static void inject_mem_helper(TCGOp *begin_op, GArray *arr)
  * is possible that the code we generate after the instruction is
  * dead, we also add checks before generating tb_exit etc.
  */
-static void inject_mem_enable_helper(struct qemu_plugin_insn *plugin_insn,
+static void inject_mem_enable_helper(struct qemu_plugin_tb *ptb,
+                                     struct qemu_plugin_insn *plugin_insn,
                                      TCGOp *begin_op)
 {
     GArray *cbs[2];
@@ -599,6 +600,7 @@ static void inject_mem_enable_helper(struct qemu_plugin_insn *plugin_insn,
         rm_ops(begin_op);
         return;
     }
+    ptb->mem_helper = true;
 
     arr = g_array_sized_new(false, false,
                             sizeof(struct qemu_plugin_dyn_cb), n_cbs);
@@ -626,15 +628,22 @@ void plugin_gen_disable_mem_helpers(void)
 {
     TCGv_ptr ptr;
 
-    if (likely(tcg_ctx->plugin_insn == NULL ||
-               !tcg_ctx->plugin_insn->mem_helper)) {
+    /*
+     * We could emit the clearing unconditionally and be done. However, this can
+     * be wasteful if for instance plugins don't track memory accesses, or if
+     * most TBs don't use helpers. Instead, emit the clearing iff the TB calls
+     * helpers that might access guest memory.
+     *
+     * Note: we do not reset plugin_tb->mem_helper here; a TB might have several
+     * exit points, and we want to emit the clearing from all of them.
+     */
+    if (!tcg_ctx->plugin_tb->mem_helper) {
         return;
     }
     ptr = tcg_const_ptr(NULL);
     tcg_gen_st_ptr(ptr, cpu_env, offsetof(CPUState, plugin_mem_cbs) -
                                  offsetof(ArchCPU, env));
     tcg_temp_free_ptr(ptr);
-    tcg_ctx->plugin_insn->mem_helper = false;
 }
 
 static void plugin_gen_tb_udata(const struct qemu_plugin_tb *ptb,
@@ -682,14 +691,14 @@ static void plugin_gen_mem_inline(const struct qemu_plugin_tb *ptb,
     inject_inline_cb(cbs, begin_op, op_rw);
 }
 
-static void plugin_gen_enable_mem_helper(const struct qemu_plugin_tb *ptb,
+static void plugin_gen_enable_mem_helper(struct qemu_plugin_tb *ptb,
                                          TCGOp *begin_op, int insn_idx)
 {
     struct qemu_plugin_insn *insn = g_ptr_array_index(ptb->insns, insn_idx);
-    inject_mem_enable_helper(insn, begin_op);
+    inject_mem_enable_helper(ptb, insn, begin_op);
 }
 
-static void plugin_gen_disable_mem_helper(const struct qemu_plugin_tb *ptb,
+static void plugin_gen_disable_mem_helper(struct qemu_plugin_tb *ptb,
                                           TCGOp *begin_op, int insn_idx)
 {
     struct qemu_plugin_insn *insn = g_ptr_array_index(ptb->insns, insn_idx);
@@ -750,7 +759,7 @@ static void pr_ops(void)
 #endif
 }
 
-static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
+static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
 {
     TCGOp *op;
     int insn_idx = -1;
@@ -870,6 +879,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
         ptb->haddr1 = db->host_addr[0];
         ptb->haddr2 = NULL;
         ptb->mem_only = mem_only;
+        ptb->mem_helper = false;
 
         plugin_gen_empty_callback(PLUGIN_GEN_FROM_TB);
     }
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index a772e14193..e0ebedef84 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -118,7 +118,10 @@ struct qemu_plugin_insn {
     void *haddr;
     GArray *cbs[PLUGIN_N_CB_TYPES][PLUGIN_N_CB_SUBTYPES];
     bool calls_helpers;
+
+    /* if set, the instruction calls helpers that might access guest memory */
     bool mem_helper;
+
     bool mem_only;
 };
 
@@ -158,6 +161,10 @@ struct qemu_plugin_tb {
     void *haddr1;
     void *haddr2;
     bool mem_only;
+
+    /* if set, the TB calls helpers that might access guest memory */
+    bool mem_helper;
+
     GArray *cbs[PLUGIN_N_CB_SUBTYPES];
 };
 
-- 
2.34.1

Re: [PATCH 1/4] plugins: fix optimization in plugin_gen_disable_mem_helpers

[Aaron Lindsay]

On Jan 08 11:47, Emilio Cota wrote:
> We were mistakenly checking tcg_ctx->plugin_insn as a canary to know
> whether the TB had emitted helpers that might have accessed memory.
> 
> The problem is that tcg_ctx->plugin_insn gets updated on every
> instruction in the TB, which results in us wrongly performing the
> optimization (i.e. not clearing cpu->plugin_mem_cbs) way too often,
> since it's not rare that the last instruction in the TB doesn't
> use helpers.
> 
> Fix it by tracking a per-TB canary.
> 
> While at it, expand documentation.
> 
> Related: #1381
> 
> Signed-off-by: Emilio Cota <cota@braap.org>
> ---
>  accel/tcg/plugin-gen.c | 26 ++++++++++++++++++--------
>  include/qemu/plugin.h  |  7 +++++++
>  2 files changed, 25 insertions(+), 8 deletions(-)

Tested-by: Aaron Lindsay <aaron@os.amperecomputing.com>

[PATCH 2/4] translator: always pair plugin_gen_insn_{start, end} calls

[Emilio Cota]

Related: #1381

Signed-off-by: Emilio Cota <cota@braap.org>
---
 accel/tcg/translator.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 061519691f..ef5193c67e 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -100,19 +100,24 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int max_insns,
             ops->translate_insn(db, cpu);
         }
 
-        /* Stop translation if translate_insn so indicated.  */
-        if (db->is_jmp != DISAS_NEXT) {
-            break;
-        }
-
         /*
          * We can't instrument after instructions that change control
          * flow although this only really affects post-load operations.
+         *
+         * Calling plugin_gen_insn_end() before we possibly stop translation
+         * is important. Even if this ends up as dead code, plugin generation
+         * needs to see a matching plugin_gen_insn_{start,end}() pair in order
+         * to accurately track instrumented helpers that might access memory.
          */
         if (plugin_enabled) {
             plugin_gen_insn_end();
         }
 
+        /* Stop translation if translate_insn so indicated.  */
+        if (db->is_jmp != DISAS_NEXT) {
+            break;
+        }
+
         /* Stop translation if the output buffer is full,
            or we have executed all of the allowed instructions.  */
         if (tcg_op_buf_full() || db->num_insns >= db->max_insns) {
-- 
2.34.1

Re: [PATCH 2/4] translator: always pair plugin_gen_insn_{start, end} calls

[Philippe Mathieu-Daudé]

On 8/1/23 17:47, Emilio Cota wrote:
> Related: #1381
> 
> Signed-off-by: Emilio Cota <cota@braap.org>
> ---
>   accel/tcg/translator.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/4] translator: always pair plugin_gen_insn_{start,end} calls

[Aaron Lindsay]

On Jan 08 11:47, Emilio Cota wrote:
> Related: #1381
> 
> Signed-off-by: Emilio Cota <cota@braap.org>
> ---
>  accel/tcg/translator.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Tested-by: Aaron Lindsay <aaron@os.amperecomputing.com>

[PATCH 3/4] tcg: exclude lookup_tb_ptr from helper instrumentation

[Emilio Cota]

It is internal to TCG and therefore we know it does not
access guest memory.

Related: #1381

Signed-off-by: Emilio Cota <cota@braap.org>
---
 tcg/tcg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index da91779890..ee67eefc0c 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1652,8 +1652,10 @@ void tcg_gen_callN(void *func, TCGTemp *ret, int nargs, TCGTemp **args)
     op = tcg_op_alloc(INDEX_op_call, total_args);
 
 #ifdef CONFIG_PLUGIN
-    /* detect non-plugin helpers */
-    if (tcg_ctx->plugin_insn && unlikely(strncmp(info->name, "plugin_", 7))) {
+    /* flag helpers that are not internal to TCG */
+    if (tcg_ctx->plugin_insn &&
+        strncmp(info->name, "plugin_", 7) &&
+        strcmp(info->name, "lookup_tb_ptr")) {
         tcg_ctx->plugin_insn->calls_helpers = true;
     }
 #endif
-- 
2.34.1

[PATCH 4/4] cpu-exec: assert that plugin_mem_cbs is NULL after execution

[Emilio Cota]

Fixes: #1381

Signed-off-by: Emilio Cota <cota@braap.org>
---
 accel/tcg/cpu-exec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 356fe348de..de4ba6e23c 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -504,6 +504,7 @@ static void cpu_exec_exit(CPUState *cpu)
     if (cc->tcg_ops->cpu_exec_exit) {
         cc->tcg_ops->cpu_exec_exit(cpu);
     }
+    g_assert(cpu->plugin_mem_cbs == NULL);
 }
 
 void cpu_exec_step_atomic(CPUState *cpu)
@@ -1031,6 +1032,7 @@ int cpu_exec(CPUState *cpu)
 
             cpu_loop_exec_tb(cpu, tb, pc, &last_tb, &tb_exit);
 
+            g_assert(cpu->plugin_mem_cbs == NULL);
             /* Try to align the host and virtual clocks
                if the guest is in advance */
             align_clocks(&sc, cpu);
-- 
2.34.1

Re: [PATCH 4/4] cpu-exec: assert that plugin_mem_cbs is NULL after execution

[Alex Bennée]

Emilio Cota <cota@braap.org> writes:

> Fixes: #1381
>
> Signed-off-by: Emilio Cota <cota@braap.org>
> ---
>  accel/tcg/cpu-exec.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 356fe348de..de4ba6e23c 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -504,6 +504,7 @@ static void cpu_exec_exit(CPUState *cpu)
>      if (cc->tcg_ops->cpu_exec_exit) {
>          cc->tcg_ops->cpu_exec_exit(cpu);
>      }
> +    g_assert(cpu->plugin_mem_cbs == NULL);
>  }
>  
>  void cpu_exec_step_atomic(CPUState *cpu)
> @@ -1031,6 +1032,7 @@ int cpu_exec(CPUState *cpu)
>  
>              cpu_loop_exec_tb(cpu, tb, pc, &last_tb, &tb_exit);
>  
> +            g_assert(cpu->plugin_mem_cbs == NULL);
>              /* Try to align the host and virtual clocks
>                 if the guest is in advance */
>              align_clocks(&sc, cpu);

Can we assert this anywhere else? This fails with non-plugin enabled
builds:

  https://gitlab.com/stsquad/qemu/-/pipelines/741478109/failures

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 4/4] cpu-exec: assert that plugin_mem_cbs is NULL after execution

[Emilio Cota]

On Mon, Jan 09, 2023 at 13:52:36 +0000, Alex Bennée wrote:
> Emilio Cota <cota@braap.org> writes:
> > --- a/accel/tcg/cpu-exec.c
> > +++ b/accel/tcg/cpu-exec.c
> > @@ -504,6 +504,7 @@ static void cpu_exec_exit(CPUState *cpu)
> >      if (cc->tcg_ops->cpu_exec_exit) {
> >          cc->tcg_ops->cpu_exec_exit(cpu);
> >      }
> > +    g_assert(cpu->plugin_mem_cbs == NULL);
(snip)
> > +            g_assert(cpu->plugin_mem_cbs == NULL);
> 
> Can we assert this anywhere else? This fails with non-plugin enabled
> builds:
> 
>   https://gitlab.com/stsquad/qemu/-/pipelines/741478109/failures

Ouch. Fixed in v2 with:

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index de4ba6e23c..0010b2d31e 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -504,7 +504,7 @@ static void cpu_exec_exit(CPUState *cpu)
     if (cc->tcg_ops->cpu_exec_exit) {
         cc->tcg_ops->cpu_exec_exit(cpu);
     }
-    g_assert(cpu->plugin_mem_cbs == NULL);
+    QEMU_PLUGIN_ASSERT(cpu->plugin_mem_cbs == NULL);
 }
 
 void cpu_exec_step_atomic(CPUState *cpu)
@@ -1032,7 +1032,7 @@ int cpu_exec(CPUState *cpu)
 
             cpu_loop_exec_tb(cpu, tb, pc, &last_tb, &tb_exit);
 
-            g_assert(cpu->plugin_mem_cbs == NULL);
+            QEMU_PLUGIN_ASSERT(cpu->plugin_mem_cbs == NULL);
             /* Try to align the host and virtual clocks
                if the guest is in advance */
             align_clocks(&sc, cpu);
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index e0ebedef84..fb338ba576 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -59,6 +59,8 @@ get_plugin_meminfo_rw(qemu_plugin_meminfo_t i)
 #ifdef CONFIG_PLUGIN
 extern QemuOptsList qemu_plugin_opts;
 
+#define QEMU_PLUGIN_ASSERT(cond) g_assert(cond)
+
 static inline void qemu_plugin_add_opts(void)
 {
     qemu_add_opts(&qemu_plugin_opts);
@@ -250,6 +252,8 @@ void qemu_plugin_user_postfork(bool is_child);
 
 #else /* !CONFIG_PLUGIN */
 
+#define QEMU_PLUGIN_ASSERT(cond)
+
 static inline void qemu_plugin_add_opts(void)
 { }

Thanks,
		Emilio

Re: [PATCH 4/4] cpu-exec: assert that plugin_mem_cbs is NULL after execution

[Alex Bennée]

Emilio Cota <cota@braap.org> writes:

> On Mon, Jan 09, 2023 at 13:52:36 +0000, Alex Bennée wrote:
>> Emilio Cota <cota@braap.org> writes:
>> > --- a/accel/tcg/cpu-exec.c
>> > +++ b/accel/tcg/cpu-exec.c
>> > @@ -504,6 +504,7 @@ static void cpu_exec_exit(CPUState *cpu)
>> >      if (cc->tcg_ops->cpu_exec_exit) {
>> >          cc->tcg_ops->cpu_exec_exit(cpu);
>> >      }
>> > +    g_assert(cpu->plugin_mem_cbs == NULL);
> (snip)
>> > +            g_assert(cpu->plugin_mem_cbs == NULL);
>> 
>> Can we assert this anywhere else? This fails with non-plugin enabled
>> builds:
>> 
>>   https://gitlab.com/stsquad/qemu/-/pipelines/741478109/failures
>
> Ouch. Fixed in v2 with:

Thanks, Queued to plugins/next with me manually merging the fix.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro