| Message ID | 20230222043204.941336-1-cota@braap.org |
|---|---|
| State | New |
| Headers | show |
| Series | plugin: fix clearing of plugin_mem_cbs before TB exit | expand |
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>
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
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));
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(-)