From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48851) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1as9rx-0005YF-0p for qemu-devel@nongnu.org; Mon, 18 Apr 2016 10:09:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1as9rs-0001HX-Hx for qemu-devel@nongnu.org; Mon, 18 Apr 2016 10:09:40 -0400 Received: from mail-wm0-x229.google.com ([2a00:1450:400c:c09::229]:33034) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1as9rr-0001HH-Qp for qemu-devel@nongnu.org; Mon, 18 Apr 2016 10:09:36 -0400 Received: by mail-wm0-x229.google.com with SMTP id f198so22027161wme.0 for ; Mon, 18 Apr 2016 07:09:35 -0700 (PDT) References: <1460666749-24452-1-git-send-email-sergey.fedorov@linaro.org> <1460666749-24452-5-git-send-email-sergey.fedorov@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1460666749-24452-5-git-send-email-sergey.fedorov@linaro.org> Date: Mon, 18 Apr 2016 15:09:33 +0100 Message-ID: <87inzfvwiq.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Fedorov Cc: qemu-devel@nongnu.org, Sergey Fedorov , Paolo Bonzini , Peter Crosthwaite , Richard Henderson , Andreas =?utf-8?Q?F=C3=A4rber?= Sergey Fedorov writes: > From: Sergey Fedorov > > 'tb_invalidated_flag' was meant to catch two events: > * some TB has been invalidated by tb_phys_invalidate(); > * the whole translation buffer has been flushed by tb_flush(). > > Then it was checked: > * in cpu_exec() to ensure that the last executed TB can be safely > linked to directly call the next one; > * in cpu_exec_nocache() to decide if the original TB should be provided > for further possible invalidation along with the temporarily > generated TB. > > It is always safe to patch an invalidated TB since it is not going to be > used anyway. Wouldn't that have implications for code searching through the linked list of jump patched TBs? > It is also safe to call tb_phys_invalidate() for an already > invalidated TB. Thus, setting this flag in tb_phys_invalidate() is > simply unnecessary. Moreover, it can prevent from pretty proper linking > of TBs, if any arbitrary TB has been invalidated. So just don't touch it > in tb_phys_invalidate(). > > If this flag is only used to catch whether tb_flush() has been called > then rename it to 'tb_flushed'. Declare it as 'bool' and stick to using > only 'true' and 'false' to set its value. Also, instead of setting it in > tb_gen_code(), just after tb_flush() has been called, do it right inside > of tb_flush(). > > In cpu_exec(), this flag is used to track if tb_flush() has been called > and have made 'next_tb' (a reference to the last executed TB) invalid > for linking it to directly call the next TB. tb_flush() can be called > during the CPU execution loop from tb_gen_code(), during TB execution or > by another thread while 'tb_lock' is released. Catch for translation > buffer flush reliably by resetting this flag once before first TB lookup > and each time we find it set before trying to add a direct jump. Don't > touch in in tb_find_physical(). > > Each vCPU has its own execution loop in multithreaded mode and thus > should have its own copy of the flag to be able to reset it with its own > 'next_tb' and don't affect any other vCPU execution thread. So make this > flag per-vCPU and move it to CPUState. > > In cpu_exec_nocache(), we only need to check if tb_flush() has been > called from tb_gen_code() called by cpu_exec_nocache() itself. To do > this reliably, preserve the old value of the flag, reset it before > calling tb_gen_code(), check afterwards, and combine the saved value > back to the flag. > > This patch is based on the patch "tcg: move tb_invalidated_flag to > CPUState" from Paolo Bonzini . > > Signed-off-by: Sergey Fedorov > Signed-off-by: Sergey Fedorov > --- > cpu-exec.c | 21 +++++++++++---------- > include/exec/exec-all.h | 2 -- > include/qom/cpu.h | 2 ++ > translate-all.c | 5 +---- > 4 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/cpu-exec.c b/cpu-exec.c > index 36942340d7e3..966e016b7d75 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -199,16 +199,20 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles, > TranslationBlock *orig_tb, bool ignore_icount) > { > TranslationBlock *tb; > + bool old_tb_flushed; > > /* Should never happen. > We only end up here when an existing TB is too long. */ > if (max_cycles > CF_COUNT_MASK) > max_cycles = CF_COUNT_MASK; > > + old_tb_flushed = cpu->tb_flushed; > + cpu->tb_flushed = false; > tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base, orig_tb->flags, > max_cycles | CF_NOCACHE > | (ignore_icount ? CF_IGNORE_ICOUNT : 0)); > - tb->orig_tb = tcg_ctx.tb_ctx.tb_invalidated_flag ? NULL : orig_tb; > + tb->orig_tb = cpu->tb_flushed ? NULL : orig_tb; > + cpu->tb_flushed |= old_tb_flushed; > cpu->current_tb = tb; > /* execute the generated code */ > trace_exec_tb_nocache(tb, tb->pc); > @@ -229,8 +233,6 @@ static TranslationBlock *tb_find_physical(CPUState *cpu, > unsigned int h; > tb_page_addr_t phys_pc, phys_page1; > > - tcg_ctx.tb_ctx.tb_invalidated_flag = 0; > - > /* find translated block using physical mappings */ > phys_pc = get_page_addr_code(env, pc); > phys_page1 = phys_pc & TARGET_PAGE_MASK; > @@ -443,6 +445,7 @@ int cpu_exec(CPUState *cpu) > } > > next_tb = 0; /* force lookup of first TB */ > + cpu->tb_flushed = false; > for(;;) { > interrupt_request = cpu->interrupt_request; > if (unlikely(interrupt_request)) { > @@ -507,14 +510,12 @@ int cpu_exec(CPUState *cpu) > } > tb_lock(); > tb = tb_find_fast(cpu); > - /* Note: we do it here to avoid a gcc bug on Mac OS X when > - doing it in tb_find_slow */ Is this still true? Would it make more sense to push the patching down to the gen_code? I got slightly confused as to what next_tb ends up meaning at what point in the run loop. > - if (tcg_ctx.tb_ctx.tb_invalidated_flag) { > - /* as some TB could have been invalidated because > - of memory exceptions while generating the code, we > - must recompute the hash index here */ > + if (cpu->tb_flushed) { > + /* Ensure that no TB jump will be modified as the > + * translation buffer has been flushed. > + */ > next_tb = 0; > - tcg_ctx.tb_ctx.tb_invalidated_flag = 0; > + cpu->tb_flushed = false; > } > /* see if we can patch the calling TB. When the TB > spans two pages, we cannot safely do a direct > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 736209505a68..0ba845e12b12 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -288,8 +288,6 @@ struct TBContext { > /* statistics */ > int tb_flush_count; > int tb_phys_invalidate_count; > - > - int tb_invalidated_flag; > }; > > void tb_free(TranslationBlock *tb); > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index b7a10f791acc..c1ae24d1fcbb 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -238,6 +238,7 @@ struct kvm_run; > * @crash_occurred: Indicates the OS reported a crash (panic) for this CPU > * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this > * CPU and return to its top level loop. > + * @tb_flushed: Indicates the translation buffer has been flushed. > * @singlestep_enabled: Flags for single-stepping. > * @icount_extra: Instructions until next timer event. > * @icount_decr: Number of cycles left, with interrupt flag in high bit. > @@ -289,6 +290,7 @@ struct CPUState { > bool stopped; > bool crash_occurred; > bool exit_request; > + bool tb_flushed; > uint32_t interrupt_request; > int singlestep_enabled; > int64_t icount_extra; > diff --git a/translate-all.c b/translate-all.c > index 0d5d9449dc6b..acce9396581e 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -844,6 +844,7 @@ void tb_flush(CPUState *cpu) > > CPU_FOREACH(cpu) { > memset(cpu->tb_jmp_cache, 0, sizeof(cpu->tb_jmp_cache)); > + cpu->tb_flushed = true; > } > > memset(tcg_ctx.tb_ctx.tb_phys_hash, 0, sizeof(tcg_ctx.tb_ctx.tb_phys_hash)); > @@ -990,8 +991,6 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr) > invalidate_page_bitmap(p); > } > > - tcg_ctx.tb_ctx.tb_invalidated_flag = 1; > - > /* remove the TB from the hash list */ > h = tb_jmp_cache_hash_func(tb->pc); > CPU_FOREACH(cpu) { > @@ -1081,8 +1080,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu, > /* cannot fail at this point */ > tb = tb_alloc(pc); > assert(tb != NULL); > - /* Don't forget to invalidate previous TB info. */ > - tcg_ctx.tb_ctx.tb_invalidated_flag = 1; > } > > gen_code_buf = tcg_ctx.code_gen_ptr; -- Alex Bennée