| Message ID | 20240316015720.3661236-8-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | plugins: Rewrite plugin code generation | expand |
On 3/16/24 05:57, Richard Henderson wrote: > Introduce a new plugin_cb op and migrate one operation. > By using emit_before_op, we do not need to emit opcodes > early and modify them later -- we can simply emit the > final set of opcodes once. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/tcg/tcg-op-common.h | 1 + > include/tcg/tcg-opc.h | 1 + > accel/tcg/plugin-gen.c | 74 +++++++++++++++++++++---------------- > tcg/tcg-op.c | 5 +++ > 4 files changed, 50 insertions(+), 31 deletions(-) > > diff --git a/include/tcg/tcg-op-common.h b/include/tcg/tcg-op-common.h > index 2d932a515e..9de5a7f280 100644 > --- a/include/tcg/tcg-op-common.h > +++ b/include/tcg/tcg-op-common.h > @@ -74,6 +74,7 @@ void tcg_gen_goto_tb(unsigned idx); > */ > void tcg_gen_lookup_and_goto_ptr(void); > > +void tcg_gen_plugin_cb(unsigned from); > void tcg_gen_plugin_cb_start(unsigned from, unsigned type, unsigned wr); > void tcg_gen_plugin_cb_end(void); > > diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h > index b80227fa1c..3b7cb2bce1 100644 > --- a/include/tcg/tcg-opc.h > +++ b/include/tcg/tcg-opc.h > @@ -197,6 +197,7 @@ DEF(exit_tb, 0, 0, 1, TCG_OPF_BB_EXIT | TCG_OPF_BB_END) > DEF(goto_tb, 0, 0, 1, TCG_OPF_BB_EXIT | TCG_OPF_BB_END) > DEF(goto_ptr, 0, 1, 0, TCG_OPF_BB_EXIT | TCG_OPF_BB_END) > > +DEF(plugin_cb, 0, 0, 1, TCG_OPF_NOT_PRESENT) > DEF(plugin_cb_start, 0, 0, 3, TCG_OPF_NOT_PRESENT) > DEF(plugin_cb_end, 0, 0, 0, TCG_OPF_NOT_PRESENT) > > diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c > index c56f104aee..8fa342b425 100644 > --- a/accel/tcg/plugin-gen.c > +++ b/accel/tcg/plugin-gen.c > @@ -207,8 +207,7 @@ static void plugin_gen_empty_callback(enum plugin_gen_from from) > { > switch (from) { > case PLUGIN_GEN_AFTER_INSN: > - gen_wrapped(from, PLUGIN_GEN_DISABLE_MEM_HELPER, > - gen_empty_mem_helper); > + tcg_gen_plugin_cb(from); > break; > case PLUGIN_GEN_FROM_INSN: > /* > @@ -614,16 +613,6 @@ static void inject_mem_enable_helper(struct qemu_plugin_tb *ptb, > inject_mem_helper(begin_op, arr); > } > > -static void inject_mem_disable_helper(struct qemu_plugin_insn *plugin_insn, > - TCGOp *begin_op) > -{ > - if (likely(!plugin_insn->mem_helper)) { > - rm_ops(begin_op); > - return; > - } > - inject_mem_helper(begin_op, NULL); > -} > - > /* called before finishing a TB with exit_tb, goto_tb or goto_ptr */ > void plugin_gen_disable_mem_helpers(void) > { > @@ -709,11 +698,14 @@ static void plugin_gen_enable_mem_helper(struct qemu_plugin_tb *ptb, > inject_mem_enable_helper(ptb, insn, begin_op); > } > > -static void plugin_gen_disable_mem_helper(struct qemu_plugin_tb *ptb, > - TCGOp *begin_op, int insn_idx) > +static void gen_disable_mem_helper(struct qemu_plugin_tb *ptb, > + struct qemu_plugin_insn *insn) > { > - struct qemu_plugin_insn *insn = g_ptr_array_index(ptb->insns, insn_idx); > - inject_mem_disable_helper(insn, begin_op); > + if (insn->mem_helper) { > + tcg_gen_st_ptr(tcg_constant_ptr(0), tcg_env, > + offsetof(CPUState, plugin_mem_cbs) - > + offsetof(ArchCPU, env)); > + } > } > > /* #define DEBUG_PLUGIN_GEN_OPS */ > @@ -772,16 +764,49 @@ static void pr_ops(void) > > static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb) > { > - TCGOp *op; > + TCGOp *op, *next; > int insn_idx = -1; > > pr_ops(); > > - QTAILQ_FOREACH(op, &tcg_ctx->ops, link) { > + /* > + * While injecting code, we cannot afford to reuse any ebb temps > + * that might be live within the existing opcode stream. > + * The simplest solution is to release them all and create new. > + */ > + memset(tcg_ctx->free_temps, 0, sizeof(tcg_ctx->free_temps)); > + Not an expert at this, but wouldn't that break an existing TB that already has some ops on those temps? > + QTAILQ_FOREACH_SAFE(op, &tcg_ctx->ops, link, next) { > switch (op->opc) { > case INDEX_op_insn_start: > insn_idx++; > break; > + > + case INDEX_op_plugin_cb: > + { > + enum plugin_gen_from from = op->args[0]; > + struct qemu_plugin_insn *insn = NULL; > + > + if (insn_idx >= 0) { > + insn = g_ptr_array_index(plugin_tb->insns, insn_idx); > + } > + > + tcg_ctx->emit_before_op = op; > + > + switch (from) { > + case PLUGIN_GEN_AFTER_INSN: > + assert(insn != NULL); > + gen_disable_mem_helper(plugin_tb, insn); > + break; > + default: > + g_assert_not_reached(); > + } > + > + tcg_ctx->emit_before_op = NULL; > + tcg_op_remove(tcg_ctx, op); > + break; > + } > + > case INDEX_op_plugin_cb_start: > { > enum plugin_gen_from from = op->args[0]; > @@ -846,19 +871,6 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb) > > break; > } > - case PLUGIN_GEN_AFTER_INSN: > - { > - g_assert(insn_idx >= 0); > - > - switch (type) { > - case PLUGIN_GEN_DISABLE_MEM_HELPER: > - plugin_gen_disable_mem_helper(plugin_tb, op, insn_idx); > - break; > - default: > - g_assert_not_reached(); > - } > - break; > - } > default: > g_assert_not_reached(); > } > diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c > index aa6bc6f57d..0f2026c91c 100644 > --- a/tcg/tcg-op.c > +++ b/tcg/tcg-op.c > @@ -312,6 +312,11 @@ void tcg_gen_mb(TCGBar mb_type) > } > } > > +void tcg_gen_plugin_cb(unsigned from) > +{ > + tcg_gen_op1(INDEX_op_plugin_cb, from); > +} > + > void tcg_gen_plugin_cb_start(unsigned from, unsigned type, unsigned wr) > { > tcg_gen_op3(INDEX_op_plugin_cb_start, from, type, wr);
On 3/19/24 03:32, Pierrick Bouvier wrote: >> static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb) >> { >> - TCGOp *op; >> + TCGOp *op, *next; >> int insn_idx = -1; >> pr_ops(); >> - QTAILQ_FOREACH(op, &tcg_ctx->ops, link) { >> + /* >> + * While injecting code, we cannot afford to reuse any ebb temps >> + * that might be live within the existing opcode stream. >> + * The simplest solution is to release them all and create new. >> + */ >> + memset(tcg_ctx->free_temps, 0, sizeof(tcg_ctx->free_temps)); >> + > > Not an expert at this, but wouldn't that break an existing TB that already has some ops on > those temps? No, this only affects allocation of new temps -- if free_temps is empty, a new temp will be allocated from tcg_ctx->nb_temps++. Zeroing free_temps here ensures that we *do not* reuse a temp that might already be live across any plugin insertion point. Between insertion points, we will free plugin temps and only reuse those. r~
On 3/19/24 23:56, Richard Henderson wrote: > On 3/19/24 03:32, Pierrick Bouvier wrote: >>> static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb) >>> { >>> - TCGOp *op; >>> + TCGOp *op, *next; >>> int insn_idx = -1; >>> pr_ops(); >>> - QTAILQ_FOREACH(op, &tcg_ctx->ops, link) { >>> + /* >>> + * While injecting code, we cannot afford to reuse any ebb temps >>> + * that might be live within the existing opcode stream. >>> + * The simplest solution is to release them all and create new. >>> + */ >>> + memset(tcg_ctx->free_temps, 0, sizeof(tcg_ctx->free_temps)); >>> + >> >> Not an expert at this, but wouldn't that break an existing TB that already has some ops on >> those temps? > > No, this only affects allocation of new temps -- if free_temps is empty, a new temp will > be allocated from tcg_ctx->nb_temps++. > > Zeroing free_temps here ensures that we *do not* reuse a temp that might already be live > across any plugin insertion point. Between insertion points, we will free plugin temps > and only reuse those. > Thanks, by looking at tcg_temp_new_internal fn, and with your explaination, it makes more sense. > > r~
diff --git a/include/tcg/tcg-op-common.h b/include/tcg/tcg-op-common.h index 2d932a515e..9de5a7f280 100644 --- a/include/tcg/tcg-op-common.h +++ b/include/tcg/tcg-op-common.h @@ -74,6 +74,7 @@ void tcg_gen_goto_tb(unsigned idx); */ void tcg_gen_lookup_and_goto_ptr(void); +void tcg_gen_plugin_cb(unsigned from); void tcg_gen_plugin_cb_start(unsigned from, unsigned type, unsigned wr); void tcg_gen_plugin_cb_end(void); diff --git a/include/tcg/tcg-opc.h b/include/tcg/tcg-opc.h index b80227fa1c..3b7cb2bce1 100644 --- a/include/tcg/tcg-opc.h +++ b/include/tcg/tcg-opc.h @@ -197,6 +197,7 @@ DEF(exit_tb, 0, 0, 1, TCG_OPF_BB_EXIT | TCG_OPF_BB_END) DEF(goto_tb, 0, 0, 1, TCG_OPF_BB_EXIT | TCG_OPF_BB_END) DEF(goto_ptr, 0, 1, 0, TCG_OPF_BB_EXIT | TCG_OPF_BB_END) +DEF(plugin_cb, 0, 0, 1, TCG_OPF_NOT_PRESENT) DEF(plugin_cb_start, 0, 0, 3, TCG_OPF_NOT_PRESENT) DEF(plugin_cb_end, 0, 0, 0, TCG_OPF_NOT_PRESENT) diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c index c56f104aee..8fa342b425 100644 --- a/accel/tcg/plugin-gen.c +++ b/accel/tcg/plugin-gen.c @@ -207,8 +207,7 @@ static void plugin_gen_empty_callback(enum plugin_gen_from from) { switch (from) { case PLUGIN_GEN_AFTER_INSN: - gen_wrapped(from, PLUGIN_GEN_DISABLE_MEM_HELPER, - gen_empty_mem_helper); + tcg_gen_plugin_cb(from); break; case PLUGIN_GEN_FROM_INSN: /* @@ -614,16 +613,6 @@ static void inject_mem_enable_helper(struct qemu_plugin_tb *ptb, inject_mem_helper(begin_op, arr); } -static void inject_mem_disable_helper(struct qemu_plugin_insn *plugin_insn, - TCGOp *begin_op) -{ - if (likely(!plugin_insn->mem_helper)) { - rm_ops(begin_op); - return; - } - inject_mem_helper(begin_op, NULL); -} - /* called before finishing a TB with exit_tb, goto_tb or goto_ptr */ void plugin_gen_disable_mem_helpers(void) { @@ -709,11 +698,14 @@ static void plugin_gen_enable_mem_helper(struct qemu_plugin_tb *ptb, inject_mem_enable_helper(ptb, insn, begin_op); } -static void plugin_gen_disable_mem_helper(struct qemu_plugin_tb *ptb, - TCGOp *begin_op, int insn_idx) +static void gen_disable_mem_helper(struct qemu_plugin_tb *ptb, + struct qemu_plugin_insn *insn) { - struct qemu_plugin_insn *insn = g_ptr_array_index(ptb->insns, insn_idx); - inject_mem_disable_helper(insn, begin_op); + if (insn->mem_helper) { + tcg_gen_st_ptr(tcg_constant_ptr(0), tcg_env, + offsetof(CPUState, plugin_mem_cbs) - + offsetof(ArchCPU, env)); + } } /* #define DEBUG_PLUGIN_GEN_OPS */ @@ -772,16 +764,49 @@ static void pr_ops(void) static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb) { - TCGOp *op; + TCGOp *op, *next; int insn_idx = -1; pr_ops(); - QTAILQ_FOREACH(op, &tcg_ctx->ops, link) { + /* + * While injecting code, we cannot afford to reuse any ebb temps + * that might be live within the existing opcode stream. + * The simplest solution is to release them all and create new. + */ + memset(tcg_ctx->free_temps, 0, sizeof(tcg_ctx->free_temps)); + + QTAILQ_FOREACH_SAFE(op, &tcg_ctx->ops, link, next) { switch (op->opc) { case INDEX_op_insn_start: insn_idx++; break; + + case INDEX_op_plugin_cb: + { + enum plugin_gen_from from = op->args[0]; + struct qemu_plugin_insn *insn = NULL; + + if (insn_idx >= 0) { + insn = g_ptr_array_index(plugin_tb->insns, insn_idx); + } + + tcg_ctx->emit_before_op = op; + + switch (from) { + case PLUGIN_GEN_AFTER_INSN: + assert(insn != NULL); + gen_disable_mem_helper(plugin_tb, insn); + break; + default: + g_assert_not_reached(); + } + + tcg_ctx->emit_before_op = NULL; + tcg_op_remove(tcg_ctx, op); + break; + } + case INDEX_op_plugin_cb_start: { enum plugin_gen_from from = op->args[0]; @@ -846,19 +871,6 @@ static void plugin_gen_inject(struct qemu_plugin_tb *plugin_tb) break; } - case PLUGIN_GEN_AFTER_INSN: - { - g_assert(insn_idx >= 0); - - switch (type) { - case PLUGIN_GEN_DISABLE_MEM_HELPER: - plugin_gen_disable_mem_helper(plugin_tb, op, insn_idx); - break; - default: - g_assert_not_reached(); - } - break; - } default: g_assert_not_reached(); } diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c index aa6bc6f57d..0f2026c91c 100644 --- a/tcg/tcg-op.c +++ b/tcg/tcg-op.c @@ -312,6 +312,11 @@ void tcg_gen_mb(TCGBar mb_type) } } +void tcg_gen_plugin_cb(unsigned from) +{ + tcg_gen_op1(INDEX_op_plugin_cb, from); +} + void tcg_gen_plugin_cb_start(unsigned from, unsigned type, unsigned wr) { tcg_gen_op3(INDEX_op_plugin_cb_start, from, type, wr);
Introduce a new plugin_cb op and migrate one operation. By using emit_before_op, we do not need to emit opcodes early and modify them later -- we can simply emit the final set of opcodes once. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/tcg/tcg-op-common.h | 1 + include/tcg/tcg-opc.h | 1 + accel/tcg/plugin-gen.c | 74 +++++++++++++++++++++---------------- tcg/tcg-op.c | 5 +++ 4 files changed, 50 insertions(+), 31 deletions(-)