Re: [PATCH v3 7/8] plugins: distinct types for callbacks

2024-03-26 Thread Pierrick Bouvier

On 3/25/24 23:23, Richard Henderson wrote:

On 3/25/24 02:41, Pierrick Bouvier wrote:

To prevent errors when writing new types of callbacks or inline
operations, we split callbacks data to distinct types.

Signed-off-by: Pierrick Bouvier 
---
   include/qemu/plugin.h  | 46 ++---
   plugins/plugin.h   |  2 +-
   accel/tcg/plugin-gen.c | 58 +---
   plugins/core.c | 76 ++
   4 files changed, 98 insertions(+), 84 deletions(-)

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index bb224b8e4c7..a078229942f 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -73,34 +73,40 @@ enum plugin_dyn_cb_type {
   PLUGIN_CB_INLINE_STORE_U64,
   };
   
+struct qemu_plugin_regular_cb {

+union qemu_plugin_cb_sig f;
+TCGHelperInfo *info;
+void *userp;
+enum qemu_plugin_mem_rw rw;
+};
+
+struct qemu_plugin_inline_cb {
+qemu_plugin_u64 entry;
+enum qemu_plugin_op op;
+uint64_t imm;
+enum qemu_plugin_mem_rw rw;
+};


Do you still need 'op' anymore here?
It seems redundant with 'type'.



You're right, removed it in a new commit, will post new series.


Otherwise,
Reviewed-by: Richard Henderson 


r~




Re: [PATCH v3 7/8] plugins: distinct types for callbacks

2024-03-25 Thread Richard Henderson

On 3/25/24 02:41, Pierrick Bouvier wrote:

To prevent errors when writing new types of callbacks or inline
operations, we split callbacks data to distinct types.

Signed-off-by: Pierrick Bouvier 
---
  include/qemu/plugin.h  | 46 ++---
  plugins/plugin.h   |  2 +-
  accel/tcg/plugin-gen.c | 58 +---
  plugins/core.c | 76 ++
  4 files changed, 98 insertions(+), 84 deletions(-)

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index bb224b8e4c7..a078229942f 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -73,34 +73,40 @@ enum plugin_dyn_cb_type {
  PLUGIN_CB_INLINE_STORE_U64,
  };
  
+struct qemu_plugin_regular_cb {

+union qemu_plugin_cb_sig f;
+TCGHelperInfo *info;
+void *userp;
+enum qemu_plugin_mem_rw rw;
+};
+
+struct qemu_plugin_inline_cb {
+qemu_plugin_u64 entry;
+enum qemu_plugin_op op;
+uint64_t imm;
+enum qemu_plugin_mem_rw rw;
+};


Do you still need 'op' anymore here?
It seems redundant with 'type'.

Otherwise,
Reviewed-by: Richard Henderson 


r~



[PATCH v3 7/8] plugins: distinct types for callbacks

2024-03-25 Thread Pierrick Bouvier
To prevent errors when writing new types of callbacks or inline
operations, we split callbacks data to distinct types.

Signed-off-by: Pierrick Bouvier 
---
 include/qemu/plugin.h  | 46 ++---
 plugins/plugin.h   |  2 +-
 accel/tcg/plugin-gen.c | 58 +---
 plugins/core.c | 76 ++
 4 files changed, 98 insertions(+), 84 deletions(-)

diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index bb224b8e4c7..a078229942f 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -73,34 +73,40 @@ enum plugin_dyn_cb_type {
 PLUGIN_CB_INLINE_STORE_U64,
 };
 
+struct qemu_plugin_regular_cb {
+union qemu_plugin_cb_sig f;
+TCGHelperInfo *info;
+void *userp;
+enum qemu_plugin_mem_rw rw;
+};
+
+struct qemu_plugin_inline_cb {
+qemu_plugin_u64 entry;
+enum qemu_plugin_op op;
+uint64_t imm;
+enum qemu_plugin_mem_rw rw;
+};
+
+struct qemu_plugin_conditional_cb {
+union qemu_plugin_cb_sig f;
+TCGHelperInfo *info;
+void *userp;
+qemu_plugin_u64 entry;
+enum qemu_plugin_cond cond;
+uint64_t imm;
+};
+
 /*
  * A dynamic callback has an insertion point that is determined at run-time.
  * Usually the insertion point is somewhere in the code cache; think for
  * instance of a callback to be called upon the execution of a particular TB.
  */
 struct qemu_plugin_dyn_cb {
-void *userp;
 enum plugin_dyn_cb_type type;
-/* @rw applies to mem callbacks only (both regular and inline) */
-enum qemu_plugin_mem_rw rw;
-/* fields specific to each dyn_cb type go here */
 union {
-struct {
-union qemu_plugin_cb_sig f;
-TCGHelperInfo *info;
-} regular;
-struct {
-union qemu_plugin_cb_sig f;
-TCGHelperInfo *info;
-qemu_plugin_u64 entry;
-enum qemu_plugin_cond cond;
-uint64_t imm;
-} cond;
-struct {
-qemu_plugin_u64 entry;
-enum qemu_plugin_op op;
-uint64_t imm;
-} inline_insn;
+struct qemu_plugin_regular_cb regular;
+struct qemu_plugin_conditional_cb cond;
+struct qemu_plugin_inline_cb inline_insn;
 };
 };
 
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 7d4b4e21f7c..80d5daa9171 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -108,7 +108,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
  enum qemu_plugin_mem_rw rw,
  void *udata);
 
-void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index);
+void exec_inline_op(struct qemu_plugin_inline_cb *cb, int cpu_index);
 
 int plugin_num_vcpus(void);
 
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 7ecaf670d93..16618adf1bc 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -108,13 +108,13 @@ static void gen_disable_mem_helper(void)
offsetof(ArchCPU, env));
 }
 
-static void gen_udata_cb(struct qemu_plugin_dyn_cb *cb)
+static void gen_udata_cb(struct qemu_plugin_regular_cb *cb)
 {
 TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
 
 tcg_gen_ld_i32(cpu_index, tcg_env,
-offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
-tcg_gen_call2(cb->regular.f.vcpu_udata, cb->regular.info, NULL,
+tcg_gen_call2(cb->f.vcpu_udata, cb->info, NULL,
   tcgv_i32_temp(cpu_index),
   tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
 tcg_temp_free_i32(cpu_index);
@@ -160,21 +160,21 @@ static TCGCond plugin_cond_to_tcgcond(enum 
qemu_plugin_cond cond)
 }
 }
 
-static void gen_udata_cond_cb(struct qemu_plugin_dyn_cb *cb)
+static void gen_udata_cond_cb(struct qemu_plugin_conditional_cb *cb)
 {
-TCGv_ptr ptr = gen_plugin_u64_ptr(cb->cond.entry);
+TCGv_ptr ptr = gen_plugin_u64_ptr(cb->entry);
 TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
 TCGv_i64 val = tcg_temp_ebb_new_i64();
 TCGLabel *after_cb = gen_new_label();
 
 /* Condition should be negated, as calling the cb is the "else" path */
-TCGCond cond = tcg_invert_cond(plugin_cond_to_tcgcond(cb->cond.cond));
+TCGCond cond = tcg_invert_cond(plugin_cond_to_tcgcond(cb->cond));
 
 tcg_gen_ld_i64(val, ptr, 0);
-tcg_gen_brcondi_i64(cond, val, cb->cond.imm, after_cb);
+tcg_gen_brcondi_i64(cond, val, cb->imm, after_cb);
 tcg_gen_ld_i32(cpu_index, tcg_env,
-offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
-tcg_gen_call2(cb->cond.f.vcpu_udata, cb->cond.info, NULL,
+tcg_gen_call2(cb->f.vcpu_udata, cb->info, NULL,
   tcgv_i32_temp(cpu_index),
   tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
 gen_set_label(after_cb);
@@ -184,39 +184,39 @@ static void gen_udata_cond_cb(struct qemu_plugin_dyn_cb 
*cb)
 tcg_temp_free_ptr(ptr);
 }
 
-static void gen_inline_add_u64_cb(struct