Hi Xingran,

On 9/2/24 03:42, Alex Bennée wrote:
Xingran Wang <wangxingran123...@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:


I agree that this is definitely a QEMU implementation "detail", and should not be a concern for end users.

The documentation already warns that all instructions may not execute, and that in this case, it's better to instrument them directly, instead of TB: https://www.qemu.org/docs/master/devel/tcg-plugins.html#translation-blocks.

Finally, even if we integrated an API like what you propose in this patch, I think it would be very easy for plugins writers to make a mistake using it, as you need to keep track of everything yourself.

If we want to stay on the topic of this patch, one direction that would be better in my opinion is a "after_tb_exec" API, where the TB passed in parameter is guaranteed to have all its instructions that were executed (not interrupted).

   Message-Id: <20240718145958.1315270-1-alex.ben...@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.ben...@linaro.org>

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


Signed-off-by: Xingran Wang <wangxingran123...@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.hender...@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.hender...@linaro.org>

<snip>


Thanks,
Pierrick

Reply via email to