[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 2/7] accel/tcg: suppress IRQ check for special TBs

From: Richard Henderson
Subject: Re: [PATCH v1 2/7] accel/tcg: suppress IRQ check for special TBs
Date: Wed, 24 Nov 2021 15:42:21 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0

On 11/24/21 11:24 AM, Alex Bennée wrote:

Richard Henderson <richard.henderson@linaro.org> writes:

On 11/23/21 9:57 PM, Alex Bennée wrote:
Generally when we set cpu->cflags_next_tb it is because we want to
carefully control the execution of the next TB. Currently there is a
race that causes cflags_next_tb to get ignored if an IRQ is processed
before we execute any actual instructions.
To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
this check in the generated code so we know we will definitely execute
the next block.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245
   include/exec/exec-all.h   |  1 +
   include/exec/gen-icount.h | 21 +++++++++++++++++----
   accel/tcg/cpu-exec.c      | 14 ++++++++++++++
   3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 6bb2a0f7ec..35d8e93976 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -503,6 +503,7 @@ struct TranslationBlock {
   #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 
+#define CF_NOIRQ         0x00100000 /* Generate an uninterruptible TB */
   #define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
   #define CF_CLUSTER_SHIFT 24
   diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 610cba58fe..c57204ddad 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
       TCGv_i32 count;
   -    tcg_ctx->exitreq_label = gen_new_label();
       if (tb_cflags(tb) & CF_USE_ICOUNT) {
           count = tcg_temp_local_new_i32();
       } else {
@@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
           icount_start_insn = tcg_last_op();
   -    tcg_gen_brcondi_i32(TCG_COND_LT, count, 0,
+    /*
+     * Emit the check against icount_decr.u32 to see if we should exit
+     * unless we suppress the check with CF_NOIRQ. If we are using
+     * icount and have suppressed interruption the higher level code
+     * should have ensured we don't run more instructions than the
+     * budget.
+     */
+    if (tb_cflags(tb) & CF_NOIRQ) {
+        tcg_ctx->exitreq_label = NULL;
+    } else {
+        tcg_ctx->exitreq_label = gen_new_label();
+        tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
+    }
         if (tb_cflags(tb) & CF_USE_ICOUNT) {
           tcg_gen_st16_i32(count, cpu_env,
@@ -74,8 +85,10 @@ static inline void gen_tb_end(const TranslationBlock *tb, 
int num_insns)
   -    gen_set_label(tcg_ctx->exitreq_label);
-    tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
+    if (tcg_ctx->exitreq_label) {
+        gen_set_label(tcg_ctx->exitreq_label);
+        tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
+    }

Split patch here, I think.

Not including the header to cpu_handle_interrupt?

Correct.  Introduce CF_NOIRQ without using it yet.

With icount, in cpu_loop_exec_tb, we have no idea what's coming; we
only know that we want no more than N insns to execute.

I think technically we do because all asynchronous interrupt are tied to
the icount (which is part of the budget calculation - icount_get_limit).

Are you sure that's plain icount and not replay?
In icount_get_limit we talk about timers, not any other asynchronous interrupt, like a keyboard press.

In theory we could drop the interrupt check altogether in icount mode
because we should always end and exit to the outer loop when a timer is
going to expire.

But we know nothing about synchronous exceptions or anything else.

I wonder what would happen if we checked u16.high in icount mode? No
timer should ever set it - although I guess it could get set during an
IO operation.

Uh, we do, via u32?  I'm not sure what you're saying here.

Perhaps really all icount exit checks should be done at the end of
blocks? I suspect that breaks too many assumptions in the rest of the

There are multiple exits at the end of the block, which is why we do the check at the beginning of the next block. Besides, we have to check that the block we're about to execute is within budget.

Are there cases of setting cpu->cflags_next_tb which we are happy to get
preempted by asynchronous events?

Well, icount.

I guess in the SMC case it wouldn't
matter because when we get back from the IRQ things get reset?

SMC probably would work with an interrupt, but we'd wind up having to repeat the process of flushing all TBs on the page, so we might as well perform the one store and get it over with.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]