| Message ID | 20240316015720.3661236-6-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | plugins: Rewrite plugin code generation | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > The out-of-line function pointer is mutually exclusive > with inline expansion, so move it into the union. > Wrap the pointer in a structure named 'regular' to match > PLUGIN_CB_REGULAR. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
On 3/16/24 05:57, Richard Henderson wrote: > The out-of-line function pointer is mutually exclusive > with inline expansion, so move it into the union. > Wrap the pointer in a structure named 'regular' to match > PLUGIN_CB_REGULAR. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/qemu/plugin.h | 4 +++- > accel/tcg/plugin-gen.c | 4 ++-- > plugins/core.c | 8 ++++---- > 3 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h > index 12a96cea2a..143262dca8 100644 > --- a/include/qemu/plugin.h > +++ b/include/qemu/plugin.h > @@ -84,13 +84,15 @@ enum plugin_dyn_cb_subtype { > * instance of a callback to be called upon the execution of a particular TB. > */ > struct qemu_plugin_dyn_cb { > - union qemu_plugin_cb_sig f; > void *userp; > enum plugin_dyn_cb_subtype 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; > + } regular; > struct { > qemu_plugin_u64 entry; > enum qemu_plugin_op op; While we are cleaning this, maybe this could be only a union (moving rw and userp to fields), and only type + union would be used. Even if we duplicate userp in regular, and mem_cb, it would be much more readable. For instance, userp is never used by inline operations. > diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c > index 8028786c7b..c56f104aee 100644 > --- a/accel/tcg/plugin-gen.c > +++ b/accel/tcg/plugin-gen.c > @@ -431,7 +431,7 @@ static TCGOp *append_udata_cb(const struct qemu_plugin_dyn_cb *cb, > } > > /* call */ > - op = copy_call(&begin_op, op, cb->f.vcpu_udata, cb_idx); > + op = copy_call(&begin_op, op, cb->regular.f.vcpu_udata, cb_idx); > > return op; > } > @@ -479,7 +479,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, cb->f.vcpu_udata, cb_idx); > + op = copy_call(&begin_op, op, cb->regular.f.vcpu_udata, cb_idx); > } > > return op; > diff --git a/plugins/core.c b/plugins/core.c > index 4487cb7c48..837c373690 100644 > --- a/plugins/core.c > +++ b/plugins/core.c > @@ -342,7 +342,7 @@ void plugin_register_dyn_cb__udata(GArray **arr, > > dyn_cb->userp = udata; > /* Note flags are discarded as unused. */ > - dyn_cb->f.vcpu_udata = cb; > + dyn_cb->regular.f.vcpu_udata = cb; > dyn_cb->type = PLUGIN_CB_REGULAR; > } > > @@ -359,7 +359,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr, > /* Note flags are discarded as unused. */ > dyn_cb->type = PLUGIN_CB_REGULAR; > dyn_cb->rw = rw; > - dyn_cb->f.generic = cb; > + dyn_cb->regular.f.vcpu_mem = cb; > } > > /* > @@ -511,8 +511,8 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr, > } > switch (cb->type) { > case PLUGIN_CB_REGULAR: > - cb->f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw), > - vaddr, cb->userp); > + cb->regular.f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw), > + vaddr, cb->userp); > break; > case PLUGIN_CB_INLINE: > exec_inline_op(cb, cpu->cpu_index);
On 3/19/24 03:18, Pierrick Bouvier wrote: > On 3/16/24 05:57, Richard Henderson wrote: >> The out-of-line function pointer is mutually exclusive >> with inline expansion, so move it into the union. >> Wrap the pointer in a structure named 'regular' to match >> PLUGIN_CB_REGULAR. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> include/qemu/plugin.h | 4 +++- >> accel/tcg/plugin-gen.c | 4 ++-- >> plugins/core.c | 8 ++++---- >> 3 files changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h >> index 12a96cea2a..143262dca8 100644 >> --- a/include/qemu/plugin.h >> +++ b/include/qemu/plugin.h >> @@ -84,13 +84,15 @@ enum plugin_dyn_cb_subtype { >> * instance of a callback to be called upon the execution of a particular TB. >> */ >> struct qemu_plugin_dyn_cb { >> - union qemu_plugin_cb_sig f; >> void *userp; >> enum plugin_dyn_cb_subtype 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; >> + } regular; >> struct { >> qemu_plugin_u64 entry; >> enum qemu_plugin_op op; > > While we are cleaning this, maybe this could be only a union (moving rw and userp to > fields), and only type + union would be used. > Even if we duplicate userp in regular, and mem_cb, it would be much more readable. > For instance, userp is never used by inline operations. I was wondering about this. But I was also thinking about your follow-on patch set to add conditional calls: do we want a multiplicity of union members, or will we want separate bits and pieces that can be strung together? E.g. TCGCond cond; /* ALWAYS, or compare entry vs imm. */ /* PLUGIN_CB_REGULAR_COND or PLUGIN_CB_INLINE_* */ qemu_plugin_u64 entry; uint64_t imm; /* PLUGIN_CB_REGULAR_* */ union qemu_plugin_cb_sig f; void *userp; r~
On 3/20/24 01:30, Richard Henderson wrote: > On 3/19/24 03:18, Pierrick Bouvier wrote: >> On 3/16/24 05:57, Richard Henderson wrote: >>> The out-of-line function pointer is mutually exclusive >>> with inline expansion, so move it into the union. >>> Wrap the pointer in a structure named 'regular' to match >>> PLUGIN_CB_REGULAR. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> include/qemu/plugin.h | 4 +++- >>> accel/tcg/plugin-gen.c | 4 ++-- >>> plugins/core.c | 8 ++++---- >>> 3 files changed, 9 insertions(+), 7 deletions(-) >>> >>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h >>> index 12a96cea2a..143262dca8 100644 >>> --- a/include/qemu/plugin.h >>> +++ b/include/qemu/plugin.h >>> @@ -84,13 +84,15 @@ enum plugin_dyn_cb_subtype { >>> * instance of a callback to be called upon the execution of a particular TB. >>> */ >>> struct qemu_plugin_dyn_cb { >>> - union qemu_plugin_cb_sig f; >>> void *userp; >>> enum plugin_dyn_cb_subtype 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; >>> + } regular; >>> struct { >>> qemu_plugin_u64 entry; >>> enum qemu_plugin_op op; >> >> While we are cleaning this, maybe this could be only a union (moving rw and userp to >> fields), and only type + union would be used. >> Even if we duplicate userp in regular, and mem_cb, it would be much more readable. >> For instance, userp is never used by inline operations. > > I was wondering about this. But I was also thinking about your follow-on patch set to add > conditional calls: do we want a multiplicity of union members, or will we want separate > bits and pieces that can be strung together? > > E.g. > > TCGCond cond; /* ALWAYS, or compare entry vs imm. */ > > /* PLUGIN_CB_REGULAR_COND or PLUGIN_CB_INLINE_* */ > qemu_plugin_u64 entry; > uint64_t imm; > > /* PLUGIN_CB_REGULAR_* */ > union qemu_plugin_cb_sig f; > void *userp; > > In an ideal world: struct regular_cb { // full data for regular cb }; struct conditional_cb { // full data for cond cb }; struct inline_op { // full data for inline op }; ... and struct qemu_plugin_dyn_cb { enum plugin_dyn_cb_subtype type; union { struct regular_cb regular; struct conditional_cb conditional; struct inline_op inline; }; }; It's the same as describing the structs directly inside union, but at least you can duplicate fields without thinking too much. In terms of memory layout it does not change anything, the upper bound is still the largest struct, whether you share fields or not. In short, an algebraic data type. Hope it's not a bad word around here :) > r~
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h index 12a96cea2a..143262dca8 100644 --- a/include/qemu/plugin.h +++ b/include/qemu/plugin.h @@ -84,13 +84,15 @@ enum plugin_dyn_cb_subtype { * instance of a callback to be called upon the execution of a particular TB. */ struct qemu_plugin_dyn_cb { - union qemu_plugin_cb_sig f; void *userp; enum plugin_dyn_cb_subtype 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; + } regular; struct { qemu_plugin_u64 entry; enum qemu_plugin_op op; diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c index 8028786c7b..c56f104aee 100644 --- a/accel/tcg/plugin-gen.c +++ b/accel/tcg/plugin-gen.c @@ -431,7 +431,7 @@ static TCGOp *append_udata_cb(const struct qemu_plugin_dyn_cb *cb, } /* call */ - op = copy_call(&begin_op, op, cb->f.vcpu_udata, cb_idx); + op = copy_call(&begin_op, op, cb->regular.f.vcpu_udata, cb_idx); return op; } @@ -479,7 +479,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, cb->f.vcpu_udata, cb_idx); + op = copy_call(&begin_op, op, cb->regular.f.vcpu_udata, cb_idx); } return op; diff --git a/plugins/core.c b/plugins/core.c index 4487cb7c48..837c373690 100644 --- a/plugins/core.c +++ b/plugins/core.c @@ -342,7 +342,7 @@ void plugin_register_dyn_cb__udata(GArray **arr, dyn_cb->userp = udata; /* Note flags are discarded as unused. */ - dyn_cb->f.vcpu_udata = cb; + dyn_cb->regular.f.vcpu_udata = cb; dyn_cb->type = PLUGIN_CB_REGULAR; } @@ -359,7 +359,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr, /* Note flags are discarded as unused. */ dyn_cb->type = PLUGIN_CB_REGULAR; dyn_cb->rw = rw; - dyn_cb->f.generic = cb; + dyn_cb->regular.f.vcpu_mem = cb; } /* @@ -511,8 +511,8 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr, } switch (cb->type) { case PLUGIN_CB_REGULAR: - cb->f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw), - vaddr, cb->userp); + cb->regular.f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw), + vaddr, cb->userp); break; case PLUGIN_CB_INLINE: exec_inline_op(cb, cpu->cpu_index);
The out-of-line function pointer is mutually exclusive with inline expansion, so move it into the union. Wrap the pointer in a structure named 'regular' to match PLUGIN_CB_REGULAR. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/qemu/plugin.h | 4 +++- accel/tcg/plugin-gen.c | 4 ++-- plugins/core.c | 8 ++++---- 3 files changed, 9 insertions(+), 7 deletions(-)