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

["Frédéric Pétrot" <frederic.petrot@univ-grenoble-alpes.fr>]

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>