Richard Henderson <richard.hender...@linaro.org> writes:
> On 11/24/21 11:24 AM, Alex Bennée wrote: >> Richard Henderson <richard.hender...@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.ben...@linaro.org> >>>> Cc: Pavel Dovgalyuk <pavel.dovgal...@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. Hmm right - and I guess other I/O during record of icount. I guess it's only fully deterministic (outside of replay) if it's a totally "isolated" from external system events. >> 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. Hmm I didn't think we needed to care about synchronous events but now you have me wandering what happens in icount mode when an exception happens mid-block? >> 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. I mean should we detect if something has called cpu_exit() mid block rather than just icount expiring. <snip> >> 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. I guess. Makes the patch a bit noisier though... -- Alex Bennée
