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

[Richard Henderson]

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,
> > > tcg_ctx->exitreq_label);
> > > +    /*
> > > +     * 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)
> > >                               tcgv_i32_arg(tcg_constant_i32(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);
> > > +    }
> > >    }
> > >      #endif
> >
> > 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
> code.

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.


r~