Re: [PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit
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
Re: [PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit
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
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
0x1000: 0297 auipc t0,0
# 0x1000
0x1004: 02828613 addia2,t0,40
0x1008: f1402573 csrrs a0,mhartid,zero
OP:
ld_i32 tmp1,env,$0xfff0
brcond_i32 tmp1,$0x0,lt,$L0
1000
mov_i64 tmp2,$0x7ff9940152e0
ld_i32 tmp1,env,$0xef80
call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2
mov_i32 x5/t0,$0x1000
1004
mov_i64 tmp2,$0x7ff9940153e0
ld_i32 tmp1,env,$0xef80
call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2
add_i32 x12/a2,x5/t0,$0x28
1008 f1402573
mov_i64 tmp2,$0x7ff9940136c0
st_i64 tmp2,env,$0xef68
mov_i64 tmp2,$0x7ff994015530
ld_i32 tmp1,env,$0xef80
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,$0xef68 <-- after_insn clearing: dead due to
exit_tb above
set_label $L0
exit_tb $0x7ff9a443 <-- again, missing clearing (doesn't matter due to
$L0 label)
0, 0x1000, 0x297, "0297 auipc t0,0
# 0x1000"
0, 0x1004, 0x2828613, "02828613 addia2,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,$0xef68 <-- before_exit clearing of plugin_mem_cbs
exit_tb $0x0
mov_i64 tmp2,$0x0
st_i64 tmp2,env,$0xef68 <-- after_insn clearing (dead)
set_label $L0
mov_i64 tmp2,$0x0
st_i64 tmp2,env,$0xef68 <-- before_exit clearing (doesn't matter
due to $L0)
exit_tb $0x7f38c843
[...]
Fixes: #1381
Signed-off-by: Emilio Cota
---
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
[PATCH] plugin: fix clearing of plugin_mem_cbs before TB exit
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
0x1000: 0297 auipc t0,0
# 0x1000
0x1004: 02828613 addia2,t0,40
0x1008: f1402573 csrrs a0,mhartid,zero
OP:
ld_i32 tmp1,env,$0xfff0
brcond_i32 tmp1,$0x0,lt,$L0
1000
mov_i64 tmp2,$0x7ff9940152e0
ld_i32 tmp1,env,$0xef80
call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2
mov_i32 x5/t0,$0x1000
1004
mov_i64 tmp2,$0x7ff9940153e0
ld_i32 tmp1,env,$0xef80
call plugin(0x7ff9edbcb6f0),$0x11,$0,tmp1,tmp2
add_i32 x12/a2,x5/t0,$0x28
1008 f1402573
mov_i64 tmp2,$0x7ff9940136c0
st_i64 tmp2,env,$0xef68
mov_i64 tmp2,$0x7ff994015530
ld_i32 tmp1,env,$0xef80
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,$0xef68 <-- after_insn clearing: dead due to
exit_tb above
set_label $L0
exit_tb $0x7ff9a443 <-- again, missing clearing (doesn't matter due to $L0
label)
0, 0x1000, 0x297, "0297 auipc t0,0
# 0x1000"
0, 0x1004, 0x2828613, "02828613 addia2,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,$0xef68 <-- before_exit clearing of plugin_mem_cbs
exit_tb $0x0
mov_i64 tmp2,$0x0
st_i64 tmp2,env,$0xef68 <-- after_insn clearing (dead)
set_label $L0
mov_i64 tmp2,$0x0
st_i64 tmp2,env,$0xef68 <-- before_exit clearing (doesn't matter
due to $L0)
exit_tb $0x7f38c843
[...]
Fixes: #1381
Signed-off-by: Emilio Cota
---
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 =
