diff mbox series

[v1,11/12] accel/tcg: allow plugin instrumentation to be disable via cflags

Message ID 20210209182749.31323-12-alex.bennee@linaro.org
State Superseded
Headers show
Series fix plugins double counting with mmio, cleanup CF_ flags | expand

Commit Message

Alex Bennée Feb. 9, 2021, 6:27 p.m. UTC 
When icount is enabled and we recompile an MMIO access we end up
double counting the instruction execution. To avoid this we introduce
the CF_NOINSTR cflag which disables instrumentation for the next TB.
As this is part of the hashed compile flags we will only execute the
generated TB while coming out of a cpu_io_recompile.

While we are at it delete the old TODO. We might as well keep the
translation handy as it's likely you will repeatedly hit it on each
MMIO access.

Reported-by: Aaron Lindsay <aaron@os.amperecomputing.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 include/exec/exec-all.h   |  3 ++-
 accel/tcg/translate-all.c | 17 ++++++++---------
 accel/tcg/translator.c    |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.20.1

Comments

Richard Henderson Feb. 9, 2021, 9:05 p.m. UTC | #1 
On 2/9/21 10:27 AM, Alex Bennée wrote:
> When icount is enabled and we recompile an MMIO access we end up

> double counting the instruction execution. To avoid this we introduce

> the CF_NOINSTR cflag which disables instrumentation for the next TB.

> As this is part of the hashed compile flags we will only execute the

> generated TB while coming out of a cpu_io_recompile.

> 

> While we are at it delete the old TODO. We might as well keep the

> translation handy as it's likely you will repeatedly hit it on each

> MMIO access.

> 

> Reported-by: Aaron Lindsay <aaron@os.amperecomputing.com>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  include/exec/exec-all.h   |  3 ++-

>  accel/tcg/translate-all.c | 17 ++++++++---------

>  accel/tcg/translator.c    |  2 +-

>  3 files changed, 11 insertions(+), 11 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
diff mbox series

Patch

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index e08179de34..ebf015e22d 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -454,6 +454,7 @@  struct TranslationBlock {
     uint32_t cflags;    /* compile flags */
 #define CF_COUNT_MASK  0x00007fff
 #define CF_LAST_IO     0x00008000 /* Last insn may be an IO access.  */
+#define CF_NOINSTR     0x00010000 /* Disable instrumentation of TB */
 #define CF_USE_ICOUNT  0x00020000
 #define CF_INVALID     0x00040000 /* TB is stale. Set with @jmp_lock held */
 #define CF_PARALLEL    0x00080000 /* Generate code for a parallel context */
@@ -461,7 +462,7 @@  struct TranslationBlock {
 #define CF_CLUSTER_SHIFT 24
 /* cflags' mask for hashing/comparison */
 #define CF_HASH_MASK   \
-    (CF_COUNT_MASK | CF_LAST_IO | CF_USE_ICOUNT | CF_PARALLEL | CF_CLUSTER_MASK)
+    (CF_COUNT_MASK | CF_LAST_IO | CF_NOINSTR | CF_USE_ICOUNT | CF_PARALLEL | CF_CLUSTER_MASK)
 
     /* Per-vCPU dynamic tracing state used to generate this TB */
     uint32_t trace_vcpu_dstate;
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 27b3042f1d..3dee698457 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2398,7 +2398,8 @@  void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr)
 }
 
 #ifndef CONFIG_USER_ONLY
-/* in deterministic execution mode, instructions doing device I/Os
+/*
+ * In deterministic execution mode, instructions doing device I/Os
  * must be at the end of the TB.
  *
  * Called by softmmu_template.h, with iothread mutex not held.
@@ -2429,19 +2430,17 @@  void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
         n = 2;
     }
 
-    /* Generate a new TB executing the I/O insn.  */
-    cpu->cflags_next_tb = curr_cflags() | CF_LAST_IO | n;
+    /*
+     * Exit the loop and potentially generate a new TB executing the
+     * just the I/O insns. We also disable instrumentation so we don't
+     * double count the instruction.
+     */
+    cpu->cflags_next_tb = curr_cflags() | CF_NOINSTR | CF_LAST_IO | n;
 
     qemu_log_mask_and_addr(CPU_LOG_EXEC, tb->pc,
                            "cpu_io_recompile: rewound execution of TB to "
                            TARGET_FMT_lx "\n", tb->pc);
 
-    /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
-     * the first in the TB) then we end up generating a whole new TB and
-     *  repeating the fault, which is horribly inefficient.
-     *  Better would be to execute just this insn uncached, or generate a
-     *  second new TB.
-     */
     cpu_loop_exit_noexc(cpu);
 }
 
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index a49a794065..14d1ea795d 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -58,7 +58,7 @@  void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
     ops->tb_start(db, cpu);
     tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
 
-    plugin_enabled = plugin_gen_tb_start(cpu, tb);
+    plugin_enabled = !(tb_cflags(db->tb) & CF_NOINSTR) && plugin_gen_tb_start(cpu, tb);
 
     while (true) {
         db->num_insns++;