[PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit

[PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit

[Emilio Cota]

Currently we are wrongly accessing plugin_tb->mem_helper at
translation time from plugin_gen_disable_mem_helpers, which is
called before generating a TB exit, e.g. with exit_tb.

Recall that it is only during TB finalisation, i.e. when we go over
the TB post-translation to inject or remove plugin instrumentation,
when plugin_tb->mem_helper is set. This means that we never clear
plugin_mem_cbs when calling plugin_gen_disable_mem_helpers since
mem_helper is always false. Therefore a guest instruction that uses
helpers and emits an explicit TB exit results in plugin_mem_cbs being
set upon exiting, which is caught by an assertion as reported in
the reopening of issue #1381 and replicated below.

Fix this by (1) adding an insertion point before exiting a TB
("before_exit"), and (2) deciding whether or not to emit the
clearing of plugin_mem_cbs at this newly-added insertion point
during TB finalisation.

While at it, suffix plugin_gen_disable_mem_helpers with _before_exit
to make its intent more clear.

- Before:
$ ./qemu-system-riscv32 -M spike -nographic -plugin contrib/plugins/libexeclog.so -d plugin,in_asm,op
IN:
Priv: 3; Virt: 0
0x00001000:  00000297          auipc                   t0,0                    # 0x1000
0x00001004:  02828613          addi                    a2,t0,40
0x00001008:  f1402573          csrrs                   a0,mhartid,zero

OP:
 ld_i32 tmp1,env,$0xfffffffffffffff0
 brcond_i32 tmp1,$0x0,lt,$L0

 ---- 00001000 00000000
 mov_i64 tmp2,$0x7ff9940152e0
 ld_i32 tmp1,env,$0xffffffffffffef80
 call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2
 mov_i32 x5/t0,$0x1000

 ---- 00001004 00000000
 mov_i64 tmp2,$0x7ff9940153e0
 ld_i32 tmp1,env,$0xffffffffffffef80
 call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2
 add_i32 x12/a2,x5/t0,$0x28

 ---- 00001008 f1402573
 mov_i64 tmp2,$0x7ff9940136c0
 st_i64 tmp2,env,$0xffffffffffffef68
 mov_i64 tmp2,$0x7ff994015530
 ld_i32 tmp1,env,$0xffffffffffffef80
 call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2 <-- sets plugin_mem_cbs
 call csrr,$0x0,$1,x10/a0,env,$0xf14  <-- helper that might access memory
 mov_i32 pc,$0x100c
 exit_tb $0x0  <-- exit_tb right after the helper; missing clearing of plugin_mem_cbs
 mov_i64 tmp2,$0x0
 st_i64 tmp2,env,$0xffffffffffffef68 <-- after_insn clearing: dead due to exit_tb above
 set_label $L0
 exit_tb $0x7ff9a4000043 <-- again, missing clearing (doesn't matter due to $L0 label)

0, 0x1000, 0x297, "00000297          auipc                   t0,0                    # 0x1000"
0, 0x1004, 0x2828613, "02828613          addi                    a2,t0,40"
**
ERROR:../accel/tcg/cpu-exec.c:983:cpu_exec_loop: assertion failed: (cpu->plugin_mem_cbs == ((void *)0))
Bail out! ERROR:../accel/tcg/cpu-exec.c:983:cpu_exec_loop: assertion failed: (cpu->plugin_mem_cbs == ((void *)0))
Aborted (core dumped)

- After:
$ ./qemu-system-riscv32 -M spike -nographic -plugin contrib/plugins/libexeclog.so -d plugin,in_asm,op
(snip)
 call plugin(0x7f19bc9e36f0),$0x11,$0,tmp1,tmp2 <-- sets plugin_mem_cbs
 call csrr,$0x0,$1,x10/a0,env,$0xf14
 mov_i32 pc,$0x100c
 mov_i64 tmp2,$0x0
 st_i64 tmp2,env,$0xffffffffffffef68 <-- before_exit clearing of plugin_mem_cbs
 exit_tb $0x0
 mov_i64 tmp2,$0x0
 st_i64 tmp2,env,$0xffffffffffffef68 <-- after_insn clearing (dead)
 set_label $L0
 mov_i64 tmp2,$0x0
 st_i64 tmp2,env,$0xffffffffffffef68 <-- before_exit clearing (doesn't matter due to $L0)
 exit_tb $0x7f38c8000043
[...]

Fixes: #1381
Signed-off-by: Emilio Cota <cota@braap.org>
---
 accel/tcg/plugin-gen.c    | 44 ++++++++++++++++++++-------------------
 include/exec/plugin-gen.h |  4 ++--
 include/qemu/plugin.h     |  3 ---
 tcg/tcg-op.c              |  6 +++---
 4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 17a686bd9e..b4fc171b8e 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -67,6 +67,7 @@ enum plugin_gen_from {
     PLUGIN_GEN_FROM_INSN,
     PLUGIN_GEN_FROM_MEM,
     PLUGIN_GEN_AFTER_INSN,
+    PLUGIN_GEN_BEFORE_EXIT,
     PLUGIN_GEN_N_FROMS,
 };
 
@@ -177,6 +178,7 @@ static void plugin_gen_empty_callback(enum plugin_gen_from from)
 {
     switch (from) {
     case PLUGIN_GEN_AFTER_INSN:
+    case PLUGIN_GEN_BEFORE_EXIT:
         gen_wrapped(from, PLUGIN_GEN_DISABLE_MEM_HELPER,
                     gen_empty_mem_helper);
         break;
@@ -575,7 +577,7 @@ static void inject_mem_helper(TCGOp *begin_op, GArray *arr)
  * that we can read them at run-time (i.e. when the helper executes).
  * This run-time access is performed from qemu_plugin_vcpu_mem_cb.
  *
- * Note that plugin_gen_disable_mem_helpers undoes (2). Since it
+ * Note that inject_mem_disable_helper undoes (2). Since it
  * is possible that the code we generate after the instruction is
  * dead, we also add checks before generating tb_exit etc.
  */
@@ -600,7 +602,6 @@ static void inject_mem_enable_helper(struct qemu_plugin_tb *ptb,
         rm_ops(begin_op);
         return;
     }
-    ptb->mem_helper = true;
 
     arr = g_array_sized_new(false, false,
                             sizeof(struct qemu_plugin_dyn_cb), n_cbs);
@@ -623,27 +624,25 @@ static void inject_mem_disable_helper(struct qemu_plugin_insn *plugin_insn,
     inject_mem_helper(begin_op, NULL);
 }
 
-/* called before finishing a TB with exit_tb, goto_tb or goto_ptr */
-void plugin_gen_disable_mem_helpers(void)
+/*
+ * Called before finishing a TB with exit_tb, goto_tb or goto_ptr.
+ *
+ * Most helpers that access memory are wrapped by before/after_insn
+ * instrumentation, which enables/disables mem callbacks. Some of these
+ * helpers, however, finish a TB early (e.g. call exit_tb), which means
+ * the after_insn instrumentation never gets called.
+ *
+ * To ensure that mem callbacks are disabled, here we add an
+ * instrumentation point ("before_exit") so that when finalising the
+ * translation we can disable mem callbacks before exiting, if needed.
+ */
+void plugin_gen_disable_mem_helpers_before_exit(void)
 {
-    TCGv_ptr ptr;
-
-    /*
-     * 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) {
+    /* If no plugins are enabled, do not generate anything */
+    if (tcg_ctx->plugin_insn == NULL) {
         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);
+    plugin_gen_empty_callback(PLUGIN_GEN_BEFORE_EXIT);
 }
 
 static void plugin_gen_tb_udata(const struct qemu_plugin_tb *ptb,
@@ -730,6 +729,9 @@ static void pr_ops(void)
             case PLUGIN_GEN_AFTER_INSN:
                 name = "after insn";
                 break;
+            case PLUGIN_GEN_BEFORE_EXIT:
+                name = "before exit";
+                break;
             default:
                 break;
             }
@@ -830,6 +832,7 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
                 break;
             }
             case PLUGIN_GEN_AFTER_INSN:
+            case PLUGIN_GEN_BEFORE_EXIT:
             {
                 g_assert(insn_idx >= 0);
 
@@ -879,7 +882,6 @@ 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/exec/plugin-gen.h b/include/exec/plugin-gen.h
index 5f5506f1cc..f9f10c5fac 100644
--- a/include/exec/plugin-gen.h
+++ b/include/exec/plugin-gen.h
@@ -26,7 +26,7 @@ void plugin_gen_tb_end(CPUState *cpu);
 void plugin_gen_insn_start(CPUState *cpu, const struct DisasContextBase *db);
 void plugin_gen_insn_end(void);
 
-void plugin_gen_disable_mem_helpers(void);
+void plugin_gen_disable_mem_helpers_before_exit(void);
 void plugin_gen_empty_mem_callback(TCGv addr, uint32_t info);
 
 static inline void plugin_insn_append(abi_ptr pc, const void *from, size_t size)
@@ -66,7 +66,7 @@ static inline void plugin_gen_insn_end(void)
 static inline void plugin_gen_tb_end(CPUState *cpu)
 { }
 
-static inline void plugin_gen_disable_mem_helpers(void)
+static inline void plugin_gen_disable_mem_helpers_before_exit(void)
 { }
 
 static inline void plugin_gen_empty_mem_callback(TCGv addr, uint32_t info)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index fb338ba576..f6d10aae50 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -164,9 +164,6 @@ struct qemu_plugin_tb {
     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];
 };
 
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index c581ae77c4..f7c0d90862 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2797,7 +2797,7 @@ void tcg_gen_exit_tb(const TranslationBlock *tb, unsigned idx)
         tcg_debug_assert(idx == TB_EXIT_REQUESTED);
     }
 
-    plugin_gen_disable_mem_helpers();
+    plugin_gen_disable_mem_helpers_before_exit();
     tcg_gen_op1i(INDEX_op_exit_tb, val);
 }
 
@@ -2812,7 +2812,7 @@ void tcg_gen_goto_tb(unsigned idx)
     tcg_debug_assert((tcg_ctx->goto_tb_issue_mask & (1 << idx)) == 0);
     tcg_ctx->goto_tb_issue_mask |= 1 << idx;
 #endif
-    plugin_gen_disable_mem_helpers();
+    plugin_gen_disable_mem_helpers_before_exit();
     tcg_gen_op1i(INDEX_op_goto_tb, idx);
 }
 
@@ -2825,7 +2825,7 @@ void tcg_gen_lookup_and_goto_ptr(void)
         return;
     }
 
-    plugin_gen_disable_mem_helpers();
+    plugin_gen_disable_mem_helpers_before_exit();
     ptr = tcg_temp_new_ptr();
     gen_helper_lookup_tb_ptr(ptr, cpu_env);
     tcg_gen_op1i(INDEX_op_goto_ptr, tcgv_ptr_arg(ptr));
-- 
2.34.1

Re: [PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit

[Frédéric Pétrot]

Le 22/02/2023 à 05:32, Emilio Cota a écrit :
> Currently we are wrongly accessing plugin_tb->mem_helper at
> translation time from plugin_gen_disable_mem_helpers, which is
> called before generating a TB exit, e.g. with exit_tb.
> 
> Recall that it is only during TB finalisation, i.e. when we go over
> the TB post-translation to inject or remove plugin instrumentation,
> when plugin_tb->mem_helper is set. This means that we never clear
> plugin_mem_cbs when calling plugin_gen_disable_mem_helpers since
> mem_helper is always false. Therefore a guest instruction that uses
> helpers and emits an explicit TB exit results in plugin_mem_cbs being
> set upon exiting, which is caught by an assertion as reported in
> the reopening of issue #1381 and replicated below.
> 
> Fix this by (1) adding an insertion point before exiting a TB
> ("before_exit"), and (2) deciding whether or not to emit the
> clearing of plugin_mem_cbs at this newly-added insertion point
> during TB finalisation.
> 
> While at it, suffix plugin_gen_disable_mem_helpers with _before_exit
> to make its intent more clear.
> 
> - Before:
> $ ./qemu-system-riscv32 -M spike -nographic -plugin contrib/plugins/libexeclog.so -d plugin,in_asm,op
> IN:
> Priv: 3; Virt: 0
> 0x00001000:  00000297          auipc                   t0,0                    # 0x1000
> 0x00001004:  02828613          addi                    a2,t0,40
> 0x00001008:  f1402573          csrrs                   a0,mhartid,zero
> 
> OP:
>   ld_i32 tmp1,env,$0xfffffffffffffff0
>   brcond_i32 tmp1,$0x0,lt,$L0
> 
>   ---- 00001000 00000000
>   mov_i64 tmp2,$0x7ff9940152e0
>   ld_i32 tmp1,env,$0xffffffffffffef80
>   call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2
>   mov_i32 x5/t0,$0x1000
> 
>   ---- 00001004 00000000
>   mov_i64 tmp2,$0x7ff9940153e0
>   ld_i32 tmp1,env,$0xffffffffffffef80
>   call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2
>   add_i32 x12/a2,x5/t0,$0x28
> 
>   ---- 00001008 f1402573
>   mov_i64 tmp2,$0x7ff9940136c0
>   st_i64 tmp2,env,$0xffffffffffffef68
>   mov_i64 tmp2,$0x7ff994015530
>   ld_i32 tmp1,env,$0xffffffffffffef80
>   call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2 <-- sets plugin_mem_cbs
>   call csrr,$0x0,$1,x10/a0,env,$0xf14  <-- helper that might access memory
>   mov_i32 pc,$0x100c
>   exit_tb $0x0  <-- exit_tb right after the helper; missing clearing of plugin_mem_cbs
>   mov_i64 tmp2,$0x0
>   st_i64 tmp2,env,$0xffffffffffffef68 <-- after_insn clearing: dead due to exit_tb above
>   set_label $L0
>   exit_tb $0x7ff9a4000043 <-- again, missing clearing (doesn't matter due to $L0 label)
> 
> 0, 0x1000, 0x297, "00000297          auipc                   t0,0                    # 0x1000"
> 0, 0x1004, 0x2828613, "02828613          addi                    a2,t0,40"
> **
> ERROR:../accel/tcg/cpu-exec.c:983:cpu_exec_loop: assertion failed: (cpu->plugin_mem_cbs == ((void *)0))
> Bail out! ERROR:../accel/tcg/cpu-exec.c:983:cpu_exec_loop: assertion failed: (cpu->plugin_mem_cbs == ((void *)0))
> Aborted (core dumped)
> 
> - After:
> $ ./qemu-system-riscv32 -M spike -nographic -plugin contrib/plugins/libexeclog.so -d plugin,in_asm,op
> (snip)
>   call plugin(0x7f19bc9e36f0),$0x11,$0,tmp1,tmp2 <-- sets plugin_mem_cbs
>   call csrr,$0x0,$1,x10/a0,env,$0xf14
>   mov_i32 pc,$0x100c
>   mov_i64 tmp2,$0x0
>   st_i64 tmp2,env,$0xffffffffffffef68 <-- before_exit clearing of plugin_mem_cbs
>   exit_tb $0x0
>   mov_i64 tmp2,$0x0
>   st_i64 tmp2,env,$0xffffffffffffef68 <-- after_insn clearing (dead)
>   set_label $L0
>   mov_i64 tmp2,$0x0
>   st_i64 tmp2,env,$0xffffffffffffef68 <-- before_exit clearing (doesn't matter due to $L0)
>   exit_tb $0x7f38c8000043
> [...]
> 
> Fixes: #1381
> Signed-off-by: Emilio Cota <cota@braap.org>
> ---
>   accel/tcg/plugin-gen.c    | 44 ++++++++++++++++++++-------------------
>   include/exec/plugin-gen.h |  4 ++--
>   include/qemu/plugin.h     |  3 ---
>   tcg/tcg-op.c              |  6 +++---
>   4 files changed, 28 insertions(+), 29 deletions(-)

    Thanks Emilio for the fix, and Aaron for pointing it out to me.

    Tested-by: Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr>

Re: [PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit

[Richard Henderson]

On 2/21/23 18:32, Emilio Cota wrote:
> Currently we are wrongly accessing plugin_tb->mem_helper at
> translation time from plugin_gen_disable_mem_helpers, which is
> called before generating a TB exit, e.g. with exit_tb.
> 
> Recall that it is only during TB finalisation, i.e. when we go over
> the TB post-translation to inject or remove plugin instrumentation,
> when plugin_tb->mem_helper is set. This means that we never clear
> plugin_mem_cbs when calling plugin_gen_disable_mem_helpers since
> mem_helper is always false. Therefore a guest instruction that uses
> helpers and emits an explicit TB exit results in plugin_mem_cbs being
> set upon exiting, which is caught by an assertion as reported in
> the reopening of issue #1381 and replicated below.
> 
> Fix this by (1) adding an insertion point before exiting a TB
> ("before_exit"), and (2) deciding whether or not to emit the
> clearing of plugin_mem_cbs at this newly-added insertion point
> during TB finalisation.

This is an improvement, but incomplete, because it does not handle the exception exit 
case, via cpu_loop_exit.


r~

Re: [PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit

[Emilio Cota]

On Tue, Feb 28, 2023 at 10:50:26 -1000, Richard Henderson wrote:
> On 2/21/23 18:32, Emilio Cota wrote:
> > Currently we are wrongly accessing plugin_tb->mem_helper at
> > translation time from plugin_gen_disable_mem_helpers, which is
> > called before generating a TB exit, e.g. with exit_tb.
> > 
> > Recall that it is only during TB finalisation, i.e. when we go over
> > the TB post-translation to inject or remove plugin instrumentation,
> > when plugin_tb->mem_helper is set. This means that we never clear
> > plugin_mem_cbs when calling plugin_gen_disable_mem_helpers since
> > mem_helper is always false. Therefore a guest instruction that uses
> > helpers and emits an explicit TB exit results in plugin_mem_cbs being
> > set upon exiting, which is caught by an assertion as reported in
> > the reopening of issue #1381 and replicated below.
> > 
> > Fix this by (1) adding an insertion point before exiting a TB
> > ("before_exit"), and (2) deciding whether or not to emit the
> > clearing of plugin_mem_cbs at this newly-added insertion point
> > during TB finalisation.
> 
> This is an improvement, but incomplete, because it does not handle the
> exception exit case, via cpu_loop_exit.

AFAICT that is already handled -- see the call to
qemu_plugin_disable_mem_helpers upon returning from the longjmp
in cpu-exec.c.

I do think that doing the clearing in C as done in your series
is a better solution. It is simpler and what I most like about
it is that it generates less code. In fact I wanted to mention
that approach as an alternative in the commit log, but forgot
to do so.

Thanks,
		Emilio