diff mbox series

Add support of callbacks after instructions to plugin api

Message ID 20230808134435.2719-1-m.tyutin@yadro.com
State New
Headers show
Series Add support of callbacks after instructions to plugin api | expand

Commit Message

Mikhail Tyutin Aug. 8, 2023, 1:44 p.m. UTC 
Initially, we can only call the callback BEFORE instructions. This commit adds the ability to insert the callback AFTER instructions.

No callback call for control-flow instructions.

Signed-off-by: Aleksandr Anenkov <a.anenkov@yadro.com>
Signed-off-by: Mikhail Tyutin <m.tyutin@yadro.com>
---
 accel/tcg/plugin-gen.c       | 25 ++++++++++++++++++++-----
 accel/tcg/translator.c       | 18 +++++++++++++-----
 include/qemu/plugin.h        |  1 +
 include/qemu/qemu-plugin.h   | 33 ++++++++++++++++++++++++++++++++-
 plugins/api.c                | 26 ++++++++++++++++++++++++--
 plugins/qemu-plugins.symbols |  2 ++
 tcg/tcg-op.c                 | 16 ++++++++++++++++
 7 files changed, 108 insertions(+), 13 deletions(-)

Comments

Richard Henderson Aug. 8, 2023, 2:47 p.m. UTC | #1 
On 8/8/23 06:44, Mikhail Tyutin wrote:
> Initially, we can only call the callback BEFORE instructions. This commit adds the ability to insert the callback AFTER instructions.
> 
> No callback call for control-flow instructions.

You're going to miss whole categories of instructions, not just control-flow.  You're 
going to miss anything that raises an exception.  The list goes on and on.  This is why we 
didn't implement this "after" hook in the first place.


r~
Alex Bennée Aug. 8, 2023, 3:15 p.m. UTC | #2 
Mikhail Tyutin <m.tyutin@yadro.com> writes:

> Initially, we can only call the callback BEFORE instructions. This
> commit adds the ability to insert the callback AFTER instructions.

What is the use case for this? Because:

<snip>
>  
> +
> +        /* Stop translation if translate_insn so indicated.  */
> +        if (db->is_jmp != DISAS_NEXT) {
> +            break;
> +        }
> +
>          /*
>           * We can't instrument after instructions that change control
>           * flow although this only really affects post-load operations.
> @@ -193,11 +199,6 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
>              plugin_gen_insn_end();
>          }
>  
> -        /* Stop translation if translate_insn so indicated.  */
> -        if (db->is_jmp != DISAS_NEXT) {
> -            break;
> -        }
> -
>          /* Stop translation if the output buffer is full,
>             or we have executed all of the allowed instructions.  */
>          if (tcg_op_buf_full() || db->num_insns >= db->max_insns) {
> @@ -211,6 +212,13 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
>      gen_tb_end(tb, cflags, icount_start_insn, db->num_insns);
>  
>      if (plugin_enabled) {
> +        /*
> +         * Last chance to call plugin_gen_insn_end() if is skipped in translation
> +         * loop above.
> +         */
> +        if (db->is_jmp != DISAS_NEXT && tcg_ctx->exitreq_label == NULL) {
> +            plugin_gen_insn_end();
> +        }
>          plugin_gen_tb_end(cpu);
>      }
>  
<snip>
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -2819,6 +2819,22 @@ void tcg_gen_exit_tb(const TranslationBlock *tb, unsigned idx)
>          tcg_debug_assert(idx == TB_EXIT_REQUESTED);
>      }
>  
> +#ifdef CONFIG_PLUGIN
> +    /*
> +     * Some of instruction generators insert exit_tb explicitelly to
> +     * trigger early exit from translation block. On the other hand
> +     * translation loop (translator_loop()) inserts plugin callbacks
> +     * after instruction is generated, but it appears as dead code
> +     * because of the explicit exit_tb insert.
> +     *
> +     * Calling plugin_gen_insn_end() here before the exit allows
> +     * plugins to receive control before translation block exits.
> +     */
> +    if (tcg_ctx->plugin_insn) {
> +        plugin_gen_insn_end();
> +    }
> +#endif
> +

This isn't enough as we can exit the run loop in helpers. This is why
the execlog plugin jumps the hoops it does to complete handling of
execution on the next instruction.
Mikhail Tyutin Aug. 8, 2023, 3:48 p.m. UTC | #3 
> On 8/8/23 06:44, Mikhail Tyutin wrote:
> > Initially, we can only call the callback BEFORE instructions. This commit adds the ability to insert the callback AFTER instructions.
> >
> > No callback call for control-flow instructions.
> 
> You're going to miss whole categories of instructions, not just control-flow.  You're
> going to miss anything that raises an exception.  The list goes on and on.  This is why we
> didn't implement this "after" hook in the first place.
> 

To be fair it works quite well for code translations in user-mode and baremetal applications. At least we can intercept a set of instructions that have registers as operands and even syscall-like instructions. Logically it had to work identically to memory 'store' callbacks, but we had to add a shortcut to fix problem when some of code translators inserts exit_tb operation explicitly. Maybe there is better way to do it.

We use such AFTER callback in plugins to capture CPU state changes in generic way (using registers API patch I posted earlier). Without it, BEFORE callback has to be added to 'current' and 'following' instructions to achieve the same effect. Having callbacks on different instructions adds complexity to the callbacks itself to performs state dumps at appropriate conditions (e.g. was 'previous' instruction the one we instrumented or it was some jump).
diff mbox series

Patch

diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 5c13615112..88dcbda651 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -180,6 +180,8 @@  static void plugin_gen_empty_callback(enum plugin_gen_from from)
 {
     switch (from) {
     case PLUGIN_GEN_AFTER_INSN:
+        gen_wrapped(from, PLUGIN_GEN_CB_UDATA, gen_empty_udata_cb);
+        gen_wrapped(from, PLUGIN_GEN_CB_INLINE, gen_empty_inline_cb);
         gen_wrapped(from, PLUGIN_GEN_DISABLE_MEM_HELPER,
                     gen_empty_mem_helper);
         break;
@@ -598,18 +600,21 @@  static void plugin_gen_tb_inline(const struct qemu_plugin_tb *ptb,
 }
 
 static void plugin_gen_insn_udata(const struct qemu_plugin_tb *ptb,
+                                  enum plugin_dyn_cb_type cb_type,
                                   TCGOp *begin_op, int insn_idx)
 {
+    g_assert(cb_type == PLUGIN_CB_INSN || cb_type == PLUGIN_CB_AFTER_INSN);
     struct qemu_plugin_insn *insn = g_ptr_array_index(ptb->insns, insn_idx);
-
-    inject_udata_cb(insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_REGULAR], begin_op);
+    inject_udata_cb(insn->cbs[cb_type][PLUGIN_CB_REGULAR], begin_op);
 }
 
 static void plugin_gen_insn_inline(const struct qemu_plugin_tb *ptb,
+                                   enum plugin_dyn_cb_type cb_type,
                                    TCGOp *begin_op, int insn_idx)
 {
+    g_assert(cb_type == PLUGIN_CB_INSN || cb_type == PLUGIN_CB_AFTER_INSN);
     struct qemu_plugin_insn *insn = g_ptr_array_index(ptb->insns, insn_idx);
-    inject_inline_cb(insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_INLINE],
+    inject_inline_cb(insn->cbs[cb_type][PLUGIN_CB_INLINE],
                      begin_op, op_ok);
 }
 
@@ -738,10 +743,12 @@  static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
 
                 switch (type) {
                 case PLUGIN_GEN_CB_UDATA:
-                    plugin_gen_insn_udata(plugin_tb, op, insn_idx);
+                    plugin_gen_insn_udata(plugin_tb, PLUGIN_CB_INSN,
+                                          op, insn_idx);
                     break;
                 case PLUGIN_GEN_CB_INLINE:
-                    plugin_gen_insn_inline(plugin_tb, op, insn_idx);
+                    plugin_gen_insn_inline(plugin_tb, PLUGIN_CB_INSN,
+                                           op, insn_idx);
                     break;
                 case PLUGIN_GEN_ENABLE_MEM_HELPER:
                     plugin_gen_enable_mem_helper(plugin_tb, op, insn_idx);
@@ -773,6 +780,14 @@  static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb)
                 g_assert(insn_idx >= 0);
 
                 switch (type) {
+                case PLUGIN_GEN_CB_UDATA:
+                    plugin_gen_insn_udata(plugin_tb, PLUGIN_CB_AFTER_INSN,
+                                          op, insn_idx);
+                    break;
+                case PLUGIN_GEN_CB_INLINE:
+                    plugin_gen_insn_inline(plugin_tb, PLUGIN_CB_AFTER_INSN,
+                                           op, insn_idx);
+                    break;
                 case PLUGIN_GEN_DISABLE_MEM_HELPER:
                     plugin_gen_disable_mem_helper(plugin_tb, op, insn_idx);
                     break;
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 1a6a5448c8..5e57dc754e 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -180,6 +180,12 @@  void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
             ops->translate_insn(db, cpu);
         }
 
+
+        /* Stop translation if translate_insn so indicated.  */
+        if (db->is_jmp != DISAS_NEXT) {
+            break;
+        }
+
         /*
          * We can't instrument after instructions that change control
          * flow although this only really affects post-load operations.
@@ -193,11 +199,6 @@  void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
             plugin_gen_insn_end();
         }
 
-        /* Stop translation if translate_insn so indicated.  */
-        if (db->is_jmp != DISAS_NEXT) {
-            break;
-        }
-
         /* Stop translation if the output buffer is full,
            or we have executed all of the allowed instructions.  */
         if (tcg_op_buf_full() || db->num_insns >= db->max_insns) {
@@ -211,6 +212,13 @@  void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
     gen_tb_end(tb, cflags, icount_start_insn, db->num_insns);
 
     if (plugin_enabled) {
+        /*
+         * Last chance to call plugin_gen_insn_end() if is skipped in translation
+         * loop above.
+         */
+        if (db->is_jmp != DISAS_NEXT && tcg_ctx->exitreq_label == NULL) {
+            plugin_gen_insn_end();
+        }
         plugin_gen_tb_end(cpu);
     }
 
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index bc0781cab8..b221650281 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -68,6 +68,7 @@  union qemu_plugin_cb_sig {
 enum plugin_dyn_cb_type {
     PLUGIN_CB_INSN,
     PLUGIN_CB_MEM,
+    PLUGIN_CB_AFTER_INSN,
     PLUGIN_N_CB_TYPES,
 };
 
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 50a9957279..21e25e895d 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -51,7 +51,7 @@  typedef uint64_t qemu_plugin_id_t;
 
 extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
 
-#define QEMU_PLUGIN_VERSION 1
+#define QEMU_PLUGIN_VERSION 2
 
 /**
  * struct qemu_info_t - system information for plugins
@@ -314,6 +314,21 @@  void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
                                             enum qemu_plugin_cb_flags flags,
                                             void *userdata);
 
+/**
+ * qemu_plugin_register_vcpu_after_insn_exec_cb() - register cb
+ * after insn execution
+ * @insn: the opaque qemu_plugin_insn handle for an instruction
+ * @cb: callback function
+ * @flags: does the plugin read or write the CPU's registers?
+ * @userdata: any plugin data to pass to the @cb?
+ *
+ * The @cb function is called every time after a non-control-flow
+ * instruction is executed
+ */
+void qemu_plugin_register_vcpu_after_insn_exec_cb(
+    struct qemu_plugin_insn *insn, qemu_plugin_vcpu_udata_cb_t cb,
+    enum qemu_plugin_cb_flags flags, void *userdata);
+
 /**
  * qemu_plugin_register_vcpu_insn_exec_inline() - insn execution inline op
  * @insn: the opaque qemu_plugin_insn handle for an instruction
@@ -328,6 +343,22 @@  void qemu_plugin_register_vcpu_insn_exec_inline(struct qemu_plugin_insn *insn,
                                                 enum qemu_plugin_op op,
                                                 void *ptr, uint64_t imm);
 
+/**
+ * qemu_plugin_register_vcpu_after_insn_exec_inline() - after insn execution
+ * inline op
+ * @insn: the opaque qemu_plugin_insn handle for an instruction
+ * @op: the type of qemu_plugin_op (e.g. ADD_U64)
+ * @ptr: the target memory location for the op
+ * @imm: the op data (e.g. 1)
+ *
+ * Insert an inline op to every time after a non-control-flow
+ * instruction executes.
+ * Useful if you just want to increment a single counter somewhere in memory.
+ */
+void qemu_plugin_register_vcpu_after_insn_exec_inline(
+    struct qemu_plugin_insn *insn, enum qemu_plugin_op op,
+    void *ptr, uint64_t imm);
+
 /**
  * qemu_plugin_tb_n_insns() - query helper for number of insns in TB
  * @tb: opaque handle to TB passed to callback
diff --git a/plugins/api.c b/plugins/api.c
index 2078b16edb..5d4aedc0b5 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -114,16 +114,38 @@  void qemu_plugin_register_vcpu_insn_exec_cb(struct qemu_plugin_insn *insn,
     }
 }
 
+void qemu_plugin_register_vcpu_after_insn_exec_cb(
+    struct qemu_plugin_insn *insn, qemu_plugin_vcpu_udata_cb_t cb,
+    enum qemu_plugin_cb_flags flags, void *udata)
+{
+    if (!insn->mem_only) {
+        plugin_register_dyn_cb__udata(
+            &insn->cbs[PLUGIN_CB_AFTER_INSN][PLUGIN_CB_REGULAR],
+            cb, flags, udata);
+    }
+}
+
 void qemu_plugin_register_vcpu_insn_exec_inline(struct qemu_plugin_insn *insn,
                                                 enum qemu_plugin_op op,
                                                 void *ptr, uint64_t imm)
 {
     if (!insn->mem_only) {
-        plugin_register_inline_op(&insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_INLINE],
-                                  0, op, ptr, imm);
+        plugin_register_inline_op(
+            &insn->cbs[PLUGIN_CB_INSN][PLUGIN_CB_INLINE],
+            0, op, ptr, imm);
     }
 }
 
+void qemu_plugin_register_vcpu_after_insn_exec_inline(
+    struct qemu_plugin_insn *insn, enum qemu_plugin_op op,
+    void *ptr, uint64_t imm)
+{
+    if (!insn->mem_only) {
+        plugin_register_inline_op(
+            &insn->cbs[PLUGIN_CB_AFTER_INSN][PLUGIN_CB_INLINE],
+            0, op, ptr, imm);
+    }
+}
 
 /*
  * We always plant memory instrumentation because they don't finalise until
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index 71f6c90549..d6c25521d1 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -26,7 +26,9 @@ 
   qemu_plugin_register_vcpu_idle_cb;
   qemu_plugin_register_vcpu_init_cb;
   qemu_plugin_register_vcpu_insn_exec_cb;
+  qemu_plugin_register_vcpu_after_insn_exec_cb;
   qemu_plugin_register_vcpu_insn_exec_inline;
+  qemu_plugin_register_vcpu_after_insn_exec_inline;
   qemu_plugin_register_vcpu_mem_cb;
   qemu_plugin_register_vcpu_mem_inline;
   qemu_plugin_register_vcpu_resume_cb;
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 7aadb37756..566da1cb04 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2819,6 +2819,22 @@  void tcg_gen_exit_tb(const TranslationBlock *tb, unsigned idx)
         tcg_debug_assert(idx == TB_EXIT_REQUESTED);
     }
 
+#ifdef CONFIG_PLUGIN
+    /*
+     * Some of instruction generators insert exit_tb explicitelly to
+     * trigger early exit from translation block. On the other hand
+     * translation loop (translator_loop()) inserts plugin callbacks
+     * after instruction is generated, but it appears as dead code
+     * because of the explicit exit_tb insert.
+     *
+     * Calling plugin_gen_insn_end() here before the exit allows
+     * plugins to receive control before translation block exits.
+     */
+    if (tcg_ctx->plugin_insn) {
+        plugin_gen_insn_end();
+    }
+#endif
+
     tcg_gen_op1i(INDEX_op_exit_tb, val);
 }