[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit
From: |
Emilio Cota |
Subject: |
[PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit |
Date: |
Tue, 21 Feb 2023 23:32:04 -0500 |
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
- [PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit,
Emilio Cota <=