* [RFC PATCH v2] accel/tcg: re-factor plugin_inject_cb so we can assert insn_idx is valid
@ 2021-09-14 12:04 Alex Bennée
2021-09-14 12:59 ` Richard Henderson
0 siblings, 1 reply; 2+ messages in thread
From: Alex Bennée @ 2021-09-14 12:04 UTC (permalink / raw)
To: qemu-devel
Cc: Richard Henderson, Paolo Bonzini, Alex Bennée, Peter Maydell
Coverity doesn't know enough about how we have arranged our plugin TCG
ops to know we will always have incremented insn_idx before injecting
the callback. Let us assert it for the benefit of Coverity and protect
ourselves from accidentally breaking the assumption and triggering
harder to grok errors deeper in the code if we attempt a negative
indexed array lookup.
However to get to this point we re-factor the code and remove the
second hand instruction boundary detection in favour of scanning the
full set of ops and using the existing INDEX_op_insn_start to cleanly
detect when the instruction has started. As we no longer need the
plugin specific list of ops we delete that.
My initial benchmarks shows no discernible impact of dropping the
plugin specific ops list.
Fixes: Coverity 1459509
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
include/tcg/tcg.h | 3 -
accel/tcg/plugin-gen.c | 157 ++++++++++++++++++++++-------------------
2 files changed, 85 insertions(+), 75 deletions(-)
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 44ccd86f3e..49a02db7e6 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -604,9 +604,6 @@ struct TCGContext {
/* descriptor of the instruction being translated */
struct qemu_plugin_insn *plugin_insn;
-
- /* list to quickly access the injected ops */
- QSIMPLEQ_HEAD(, TCGOp) plugin_ops;
#endif
GHashTable *const_table[TCG_TYPE_COUNT];
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 88e25c6df9..f145b815c0 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -163,11 +163,7 @@ static void gen_empty_mem_helper(void)
static void gen_plugin_cb_start(enum plugin_gen_from from,
enum plugin_gen_cb type, unsigned wr)
{
- TCGOp *op;
-
tcg_gen_plugin_cb_start(from, type, wr);
- op = tcg_last_op();
- QSIMPLEQ_INSERT_TAIL(&tcg_ctx->plugin_ops, op, plugin_link);
}
static void gen_wrapped(enum plugin_gen_from from,
@@ -707,62 +703,6 @@ static void plugin_gen_disable_mem_helper(const struct qemu_plugin_tb *ptb,
inject_mem_disable_helper(insn, begin_op);
}
-static void plugin_inject_cb(const struct qemu_plugin_tb *ptb, TCGOp *begin_op,
- int insn_idx)
-{
- enum plugin_gen_from from = begin_op->args[0];
- enum plugin_gen_cb type = begin_op->args[1];
-
- switch (from) {
- case PLUGIN_GEN_FROM_TB:
- switch (type) {
- case PLUGIN_GEN_CB_UDATA:
- plugin_gen_tb_udata(ptb, begin_op);
- return;
- case PLUGIN_GEN_CB_INLINE:
- plugin_gen_tb_inline(ptb, begin_op);
- return;
- default:
- g_assert_not_reached();
- }
- case PLUGIN_GEN_FROM_INSN:
- switch (type) {
- case PLUGIN_GEN_CB_UDATA:
- plugin_gen_insn_udata(ptb, begin_op, insn_idx);
- return;
- case PLUGIN_GEN_CB_INLINE:
- plugin_gen_insn_inline(ptb, begin_op, insn_idx);
- return;
- case PLUGIN_GEN_ENABLE_MEM_HELPER:
- plugin_gen_enable_mem_helper(ptb, begin_op, insn_idx);
- return;
- default:
- g_assert_not_reached();
- }
- case PLUGIN_GEN_FROM_MEM:
- switch (type) {
- case PLUGIN_GEN_CB_MEM:
- plugin_gen_mem_regular(ptb, begin_op, insn_idx);
- return;
- case PLUGIN_GEN_CB_INLINE:
- plugin_gen_mem_inline(ptb, begin_op, insn_idx);
- return;
- default:
- g_assert_not_reached();
- }
- case PLUGIN_GEN_AFTER_INSN:
- switch (type) {
- case PLUGIN_GEN_DISABLE_MEM_HELPER:
- plugin_gen_disable_mem_helper(ptb, begin_op, insn_idx);
- return;
- default:
- g_assert_not_reached();
- }
- default:
- g_assert_not_reached();
- }
-}
-
/* #define DEBUG_PLUGIN_GEN_OPS */
static void pr_ops(void)
{
@@ -820,21 +760,95 @@ static void pr_ops(void)
static void plugin_gen_inject(const struct qemu_plugin_tb *plugin_tb)
{
TCGOp *op;
- int insn_idx;
+ int insn_idx = -1;
pr_ops();
- insn_idx = -1;
- QSIMPLEQ_FOREACH(op, &tcg_ctx->plugin_ops, plugin_link) {
- enum plugin_gen_from from = op->args[0];
- enum plugin_gen_cb type = op->args[1];
-
- tcg_debug_assert(op->opc == INDEX_op_plugin_cb_start);
- /* ENABLE_MEM_HELPER is the first callback of an instruction */
- if (from == PLUGIN_GEN_FROM_INSN &&
- type == PLUGIN_GEN_ENABLE_MEM_HELPER) {
+
+ QTAILQ_FOREACH(op, &tcg_ctx->ops, link) {
+ switch (op->opc) {
+ case INDEX_op_insn_start:
insn_idx++;
+ break;
+ case INDEX_op_plugin_cb_start:
+ {
+ enum plugin_gen_from from = op->args[0];
+ enum plugin_gen_cb type = op->args[1];
+
+ switch (from) {
+ case PLUGIN_GEN_FROM_TB:
+ {
+ g_assert(insn_idx == -1);
+
+ switch (type) {
+ case PLUGIN_GEN_CB_UDATA:
+ plugin_gen_tb_udata(plugin_tb, op);
+ break;
+ case PLUGIN_GEN_CB_INLINE:
+ plugin_gen_tb_inline(plugin_tb, op);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+ break;
+ }
+ case PLUGIN_GEN_FROM_INSN:
+ {
+ g_assert(insn_idx >= 0);
+
+ switch (type) {
+ case PLUGIN_GEN_CB_UDATA:
+ plugin_gen_insn_udata(plugin_tb, op, insn_idx);
+ break;
+ case PLUGIN_GEN_CB_INLINE:
+ plugin_gen_insn_inline(plugin_tb, op, insn_idx);
+ break;
+ case PLUGIN_GEN_ENABLE_MEM_HELPER:
+ plugin_gen_enable_mem_helper(plugin_tb, op, insn_idx);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+ break;
+ }
+ case PLUGIN_GEN_FROM_MEM:
+ {
+ g_assert(insn_idx >= 0);
+
+ switch (type) {
+ case PLUGIN_GEN_CB_MEM:
+ plugin_gen_mem_regular(plugin_tb, op, insn_idx);
+ break;
+ case PLUGIN_GEN_CB_INLINE:
+ plugin_gen_mem_inline(plugin_tb, op, insn_idx);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+
+ 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();
+ }
+ break;
+ }
+ default:
+ /* plugins don't care about any other ops */
+ break;
}
- plugin_inject_cb(plugin_tb, op, insn_idx);
}
pr_ops();
}
@@ -847,7 +861,6 @@ bool plugin_gen_tb_start(CPUState *cpu, const TranslationBlock *tb, bool mem_onl
if (test_bit(QEMU_PLUGIN_EV_VCPU_TB_TRANS, cpu->plugin_mask)) {
ret = true;
- QSIMPLEQ_INIT(&tcg_ctx->plugin_ops);
ptb->vaddr = tb->pc;
ptb->vaddr2 = -1;
get_page_addr_code_hostp(cpu->env_ptr, tb->pc, &ptb->haddr1);
--
2.30.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [RFC PATCH v2] accel/tcg: re-factor plugin_inject_cb so we can assert insn_idx is valid
2021-09-14 12:04 [RFC PATCH v2] accel/tcg: re-factor plugin_inject_cb so we can assert insn_idx is valid Alex Bennée
@ 2021-09-14 12:59 ` Richard Henderson
0 siblings, 0 replies; 2+ messages in thread
From: Richard Henderson @ 2021-09-14 12:59 UTC (permalink / raw)
To: Alex Bennée, qemu-devel; +Cc: Peter Maydell, Paolo Bonzini
On 9/14/21 5:04 AM, Alex Bennée wrote:
> Coverity doesn't know enough about how we have arranged our plugin TCG
> ops to know we will always have incremented insn_idx before injecting
> the callback. Let us assert it for the benefit of Coverity and protect
> ourselves from accidentally breaking the assumption and triggering
> harder to grok errors deeper in the code if we attempt a negative
> indexed array lookup.
>
> However to get to this point we re-factor the code and remove the
> second hand instruction boundary detection in favour of scanning the
> full set of ops and using the existing INDEX_op_insn_start to cleanly
> detect when the instruction has started. As we no longer need the
> plugin specific list of ops we delete that.
>
> My initial benchmarks shows no discernible impact of dropping the
> plugin specific ops list.
>
> Fixes: Coverity 1459509
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> ---
> include/tcg/tcg.h | 3 -
> accel/tcg/plugin-gen.c | 157 ++++++++++++++++++++++-------------------
> 2 files changed, 85 insertions(+), 75 deletions(-)
>
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index 44ccd86f3e..49a02db7e6 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -604,9 +604,6 @@ struct TCGContext {
>
> /* descriptor of the instruction being translated */
> struct qemu_plugin_insn *plugin_insn;
> -
> - /* list to quickly access the injected ops */
> - QSIMPLEQ_HEAD(, TCGOp) plugin_ops;
> #endif
>
> GHashTable *const_table[TCG_TYPE_COUNT];
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 88e25c6df9..f145b815c0 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -163,11 +163,7 @@ static void gen_empty_mem_helper(void)
> static void gen_plugin_cb_start(enum plugin_gen_from from,
> enum plugin_gen_cb type, unsigned wr)
> {
> - TCGOp *op;
> -
> tcg_gen_plugin_cb_start(from, type, wr);
> - op = tcg_last_op();
> - QSIMPLEQ_INSERT_TAIL(&tcg_ctx->plugin_ops, op, plugin_link);
The plugin_link field of TCGOp is now dead too. Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-09-14 13:05 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-14 12:04 [RFC PATCH v2] accel/tcg: re-factor plugin_inject_cb so we can assert insn_idx is valid Alex Bennée
2021-09-14 12:59 ` Richard Henderson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.