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(-)
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
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~
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
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>
© 2016 - 2024 Red Hat, Inc.