| Message ID | 20210601150106.12761-5-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | TCI fixes and cleanups | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > As noted by qemu-plugins.h, enum qemu_plugin_cb_flags is > currently unused -- plugins can neither read nor write > guest registers. No objection to this - although we hopefully will introduce the ability to read registers at some point. I saw no indication that the ability to mark helpers for that is going away, just the mechanism is changing? > > Cc: Alex Bennée <alex.bennee@linaro.org> > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/plugin-helpers.h | 1 - > include/qemu/plugin.h | 1 - > accel/tcg/plugin-gen.c | 8 ++++---- > plugins/core.c | 30 ++++++------------------------ > 4 files changed, 10 insertions(+), 30 deletions(-) > > diff --git a/accel/tcg/plugin-helpers.h b/accel/tcg/plugin-helpers.h > index 1916ee7920..853bd21677 100644 > --- a/accel/tcg/plugin-helpers.h > +++ b/accel/tcg/plugin-helpers.h > @@ -1,5 +1,4 @@ > #ifdef CONFIG_PLUGIN > -/* Note: no TCG flags because those are overwritten later */ > DEF_HELPER_2(plugin_vcpu_udata_cb, void, i32, ptr) > DEF_HELPER_4(plugin_vcpu_mem_cb, void, i32, i32, i64, ptr) > #endif > diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h > index c5a79a89f0..0fefbc6084 100644 > --- a/include/qemu/plugin.h > +++ b/include/qemu/plugin.h > @@ -79,7 +79,6 @@ enum plugin_dyn_cb_subtype { > struct qemu_plugin_dyn_cb { > union qemu_plugin_cb_sig f; > void *userp; > - unsigned tcg_flags; > enum plugin_dyn_cb_subtype type; > /* @rw applies to mem callbacks only (both regular and inline) */ > enum qemu_plugin_mem_rw rw; > diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c > index 48bd2f36f0..88e25c6df9 100644 > --- a/accel/tcg/plugin-gen.c > +++ b/accel/tcg/plugin-gen.c > @@ -384,7 +384,7 @@ static TCGOp *copy_st_ptr(TCGOp **begin_op, TCGOp *op) > } > > static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void *empty_func, > - void *func, unsigned tcg_flags, int *cb_idx) > + void *func, int *cb_idx) > { > /* copy all ops until the call */ > do { > @@ -411,7 +411,7 @@ static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void *empty_func, > tcg_debug_assert(i < MAX_OPC_PARAM_ARGS); > } > op->args[*cb_idx] = (uintptr_t)func; > - op->args[*cb_idx + 1] = tcg_flags; > + op->args[*cb_idx + 1] = (*begin_op)->args[*cb_idx + 1]; This confuses me. We are dropping tcg_flags because we aren't using them but why are we copying the next args from begin_op? Should we have been doing that before? > > return op; > } > @@ -438,7 +438,7 @@ static TCGOp *append_udata_cb(const struct qemu_plugin_dyn_cb *cb, > > /* call */ > op = copy_call(&begin_op, op, HELPER(plugin_vcpu_udata_cb), > - cb->f.vcpu_udata, cb->tcg_flags, cb_idx); > + cb->f.vcpu_udata, cb_idx); > > return op; > } > @@ -489,7 +489,7 @@ static TCGOp *append_mem_cb(const struct qemu_plugin_dyn_cb *cb, > if (type == PLUGIN_GEN_CB_MEM) { > /* call */ > op = copy_call(&begin_op, op, HELPER(plugin_vcpu_mem_cb), > - cb->f.vcpu_udata, cb->tcg_flags, cb_idx); > + cb->f.vcpu_udata, cb_idx); > } > > return op; > diff --git a/plugins/core.c b/plugins/core.c > index 55d188af51..e1bcdb570d 100644 > --- a/plugins/core.c > +++ b/plugins/core.c > @@ -295,33 +295,15 @@ void plugin_register_inline_op(GArray **arr, > dyn_cb->inline_insn.imm = imm; > } > > -static inline uint32_t cb_to_tcg_flags(enum qemu_plugin_cb_flags flags) > -{ > - uint32_t ret; > - > - switch (flags) { > - case QEMU_PLUGIN_CB_RW_REGS: > - ret = 0; > - break; > - case QEMU_PLUGIN_CB_R_REGS: > - ret = TCG_CALL_NO_WG; > - break; > - case QEMU_PLUGIN_CB_NO_REGS: > - default: > - ret = TCG_CALL_NO_RWG; > - } > - return ret; > -} > - > -inline void > -plugin_register_dyn_cb__udata(GArray **arr, > - qemu_plugin_vcpu_udata_cb_t cb, > - enum qemu_plugin_cb_flags flags, void *udata) > +void plugin_register_dyn_cb__udata(GArray **arr, > + qemu_plugin_vcpu_udata_cb_t cb, > + enum qemu_plugin_cb_flags flags, > + void *udata) > { > struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr); > > dyn_cb->userp = udata; > - dyn_cb->tcg_flags = cb_to_tcg_flags(flags); > + /* Note flags are discarded as unused. */ "currently unused" would be a slightly more hopeful statement ;-) > dyn_cb->f.vcpu_udata = cb; > dyn_cb->type = PLUGIN_CB_REGULAR; > } > @@ -336,7 +318,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr, > > dyn_cb = plugin_get_dyn_cb(arr); > dyn_cb->userp = udata; > - dyn_cb->tcg_flags = cb_to_tcg_flags(flags); > + /* Note flags are discarded as unused. */ > dyn_cb->type = PLUGIN_CB_REGULAR; > dyn_cb->rw = rw; > dyn_cb->f.generic = cb; -- Alex Bennée
On 6/2/21 2:22 AM, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> As noted by qemu-plugins.h, enum qemu_plugin_cb_flags is >> currently unused -- plugins can neither read nor write >> guest registers. > > No objection to this - although we hopefully will introduce the ability > to read registers at some point. I saw no indication that the ability to > mark helpers for that is going away, just the mechanism is changing? The mechanism is going away. I'll figure out how to replace it when there's some call to do so. >> @@ -411,7 +411,7 @@ static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void *empty_func, >> tcg_debug_assert(i < MAX_OPC_PARAM_ARGS); >> } >> op->args[*cb_idx] = (uintptr_t)func; >> - op->args[*cb_idx + 1] = tcg_flags; >> + op->args[*cb_idx + 1] = (*begin_op)->args[*cb_idx + 1]; > > This confuses me. We are dropping tcg_flags because we aren't using them > but why are we copying the next args from begin_op? Should we have been > doing that before? You were overwriting the field before, now we're copying it. r~
diff --git a/accel/tcg/plugin-helpers.h b/accel/tcg/plugin-helpers.h index 1916ee7920..853bd21677 100644 --- a/accel/tcg/plugin-helpers.h +++ b/accel/tcg/plugin-helpers.h @@ -1,5 +1,4 @@ #ifdef CONFIG_PLUGIN -/* Note: no TCG flags because those are overwritten later */ DEF_HELPER_2(plugin_vcpu_udata_cb, void, i32, ptr) DEF_HELPER_4(plugin_vcpu_mem_cb, void, i32, i32, i64, ptr) #endif diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h index c5a79a89f0..0fefbc6084 100644 --- a/include/qemu/plugin.h +++ b/include/qemu/plugin.h @@ -79,7 +79,6 @@ enum plugin_dyn_cb_subtype { struct qemu_plugin_dyn_cb { union qemu_plugin_cb_sig f; void *userp; - unsigned tcg_flags; enum plugin_dyn_cb_subtype type; /* @rw applies to mem callbacks only (both regular and inline) */ enum qemu_plugin_mem_rw rw; diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c index 48bd2f36f0..88e25c6df9 100644 --- a/accel/tcg/plugin-gen.c +++ b/accel/tcg/plugin-gen.c @@ -384,7 +384,7 @@ static TCGOp *copy_st_ptr(TCGOp **begin_op, TCGOp *op) } static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void *empty_func, - void *func, unsigned tcg_flags, int *cb_idx) + void *func, int *cb_idx) { /* copy all ops until the call */ do { @@ -411,7 +411,7 @@ static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void *empty_func, tcg_debug_assert(i < MAX_OPC_PARAM_ARGS); } op->args[*cb_idx] = (uintptr_t)func; - op->args[*cb_idx + 1] = tcg_flags; + op->args[*cb_idx + 1] = (*begin_op)->args[*cb_idx + 1]; return op; } @@ -438,7 +438,7 @@ static TCGOp *append_udata_cb(const struct qemu_plugin_dyn_cb *cb, /* call */ op = copy_call(&begin_op, op, HELPER(plugin_vcpu_udata_cb), - cb->f.vcpu_udata, cb->tcg_flags, cb_idx); + cb->f.vcpu_udata, cb_idx); return op; } @@ -489,7 +489,7 @@ static TCGOp *append_mem_cb(const struct qemu_plugin_dyn_cb *cb, if (type == PLUGIN_GEN_CB_MEM) { /* call */ op = copy_call(&begin_op, op, HELPER(plugin_vcpu_mem_cb), - cb->f.vcpu_udata, cb->tcg_flags, cb_idx); + cb->f.vcpu_udata, cb_idx); } return op; diff --git a/plugins/core.c b/plugins/core.c index 55d188af51..e1bcdb570d 100644 --- a/plugins/core.c +++ b/plugins/core.c @@ -295,33 +295,15 @@ void plugin_register_inline_op(GArray **arr, dyn_cb->inline_insn.imm = imm; } -static inline uint32_t cb_to_tcg_flags(enum qemu_plugin_cb_flags flags) -{ - uint32_t ret; - - switch (flags) { - case QEMU_PLUGIN_CB_RW_REGS: - ret = 0; - break; - case QEMU_PLUGIN_CB_R_REGS: - ret = TCG_CALL_NO_WG; - break; - case QEMU_PLUGIN_CB_NO_REGS: - default: - ret = TCG_CALL_NO_RWG; - } - return ret; -} - -inline void -plugin_register_dyn_cb__udata(GArray **arr, - qemu_plugin_vcpu_udata_cb_t cb, - enum qemu_plugin_cb_flags flags, void *udata) +void plugin_register_dyn_cb__udata(GArray **arr, + qemu_plugin_vcpu_udata_cb_t cb, + enum qemu_plugin_cb_flags flags, + void *udata) { struct qemu_plugin_dyn_cb *dyn_cb = plugin_get_dyn_cb(arr); dyn_cb->userp = udata; - dyn_cb->tcg_flags = cb_to_tcg_flags(flags); + /* Note flags are discarded as unused. */ dyn_cb->f.vcpu_udata = cb; dyn_cb->type = PLUGIN_CB_REGULAR; } @@ -336,7 +318,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr, dyn_cb = plugin_get_dyn_cb(arr); dyn_cb->userp = udata; - dyn_cb->tcg_flags = cb_to_tcg_flags(flags); + /* Note flags are discarded as unused. */ dyn_cb->type = PLUGIN_CB_REGULAR; dyn_cb->rw = rw; dyn_cb->f.generic = cb;