Re: [PATCH] plugins: add two events for cpu_restore_state_from_tb() and cpu_io_recompile()

["Alex Bennée" <alex.bennee@linaro.org>]

Xingran Wang <wangxingran123456@outlook.com> writes:

> Currently, the instruction count obtained by plugins using the translation
> block execution callback is larger than the actual value. Adding callbacks
> in cpu_restore_state_from_tb() and cpu_io_recompile() allows plugins to
> correct the instruction count when exiting a translation block
> mid-execution, properly subtracting the excess unexecuted
> instructions.

This smells like exposing two much of the TCG internals to the plugin
mechanism. You can already detect when we don't reach the end of a block
of instructions by instrumentation as I did in:

  Message-Id: <20240718145958.1315270-1-alex.bennee@linaro.org>
  Date: Thu, 18 Jul 2024 15:59:58 +0100
  Subject: [RFC PATCH v3] contrib/plugins: control flow plugin
  From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>

So what exactly are we trying to achieve here? A more efficient
detection of short blocks?

>
> Signed-off-by: Xingran Wang <wangxingran123456@outlook.com>
> ---
>  accel/tcg/translate-all.c    |  27 ++++++++
>  include/qemu/plugin-event.h  |   2 +
>  include/qemu/plugin.h        |  24 +++++++
>  include/qemu/qemu-plugin.h   | 131 +++++++++++++++++++++++++++++++++++
>  plugins/api.c                |  78 +++++++++++++++++++++
>  plugins/core.c               |  42 +++++++++++
>  plugins/qemu-plugins.symbols |  10 +++
>  tests/tcg/plugins/bb.c       |  25 +++++++
>  8 files changed, 339 insertions(+)
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index fdf6d8ac19..642f684372 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -65,6 +65,7 @@
>  #include "internal-target.h"
>  #include "tcg/perf.h"
>  #include "tcg/insn-start-words.h"
> +#include "qemu/plugin.h"
>  
>  TBContext tb_ctx;
>  
> @@ -218,6 +219,19 @@ void cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>          cpu->neg.icount_decr.u16.low += insns_left;
>      }
>  
> +#ifdef CONFIG_PLUGIN
> +    /*
> +     * Notify the plugin with the relevant information
> +     * when restoring the execution state of a TB.
> +     */
> +    struct qemu_plugin_tb_restore ptb_restore;
> +    ptb_restore.cpu_index = cpu->cpu_index;
> +    ptb_restore.insns_left = insns_left;
> +    ptb_restore.tb_n = tb->icount;
> +    ptb_restore.tb_pc = tb->pc;
> +    qemu_plugin_tb_restore_cb(cpu, &ptb_restore);
> +#endif
> +

See also the unwind patches which is a more generic approach to ensuring
"special" registers are synced at midpoint when using the register API:

  Message-Id: <20240606032926.83599-1-richard.henderson@linaro.org>
  Date: Wed,  5 Jun 2024 20:29:17 -0700
  Subject: [PATCH v2 0/9] plugins: Use unwind info for special gdb registers
  From: Richard Henderson <richard.henderson@linaro.org>

<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro