[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag |
Date: |
Mon, 18 Apr 2016 15:09:33 +0100 |
User-agent: |
mu4e 0.9.17; emacs 25.0.92.6 |
Sergey Fedorov <address@hidden> writes:
> From: Sergey Fedorov <address@hidden>
>
> '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 <address@hidden>.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> Signed-off-by: Sergey Fedorov <address@hidden>
> ---
> 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
- [Qemu-devel] [PATCH v3 0/4] tcg: Misc clean-up patches, Sergey Fedorov, 2016/04/14
- [Qemu-devel] [PATCH v3 1/4] tcg: code_bitmap is not used by user-mode emulation, Sergey Fedorov, 2016/04/14
- [Qemu-devel] [PATCH v3 2/4] tcg: reorganize tb_find_physical loop, Sergey Fedorov, 2016/04/14
- [Qemu-devel] [PATCH v3 3/4] cpu-exec: elide more icount code if CONFIG_USER_ONLY, Sergey Fedorov, 2016/04/14
- [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Sergey Fedorov, 2016/04/14
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag,
Alex Bennée <=
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Sergey Fedorov, 2016/04/18
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Peter Maydell, 2016/04/18
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Alex Bennée, 2016/04/18
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Sergey Fedorov, 2016/04/18
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Sergey Fedorov, 2016/04/21
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Alex Bennée, 2016/04/21
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Sergey Fedorov, 2016/04/21
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Sergey Fedorov, 2016/04/21
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Alex Bennée, 2016/04/21
- Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag, Sergey Fedorov, 2016/04/22