Sergey Fedorov <sergey.fedo...@linaro.org> writes:
> From: Sergey Fedorov <serge.f...@gmail.com>
>
> The check is to make sure that another thread hasn't already done the
> same while we were outside of tb_lock. Mention this in a comment.
>
> Signed-off-by: Sergey Fedorov <serge.f...@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedo...@linaro.org>
> ---
>
> Changes in v2:
> * Typo fixed in the commit title
> * Complete rewrite of the commit body and the patch based on Paolo's comments
>
> include/exec/exec-all.h | 31 +++++++++++++++++--------------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index b055716ed690..8e81ef5fb2c2 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -391,21 +391,24 @@ static inline void tb_set_jmp_target(TranslationBlock
> *tb,
> static inline void tb_add_jump(TranslationBlock *tb, int n,
> TranslationBlock *tb_next)
> {
> - /* NOTE: this test is only needed for thread safety */
> - if (!tb->jmp_list_next[n]) {
> - qemu_log_mask_and_addr(CPU_LOG_EXEC, tb->pc,
> - "Linking TBs %p [" TARGET_FMT_lx
> - "] index %d -> %p [" TARGET_FMT_lx "]\n",
> - tb->tc_ptr, tb->pc, n,
> - tb_next->tc_ptr, tb_next->pc);
> - /* patch the native jump address */
> - tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
> -
> - /* add in TB jmp circular list */
> - tb->jmp_list_next[n] = tb_next->jmp_list_first;
> - assert(((uintptr_t)tb & 3) == 0);
> - tb_next->jmp_list_first = (uintptr_t)tb | n;
> + if (tb->jmp_list_next[n]) {
> + /* Another thread has already done this while we were
> + * outside of the lock; nothing to do in this case */
> + return;
> }
> + qemu_log_mask_and_addr(CPU_LOG_EXEC, tb->pc,
> + "Linking TBs %p [" TARGET_FMT_lx
> + "] index %d -> %p [" TARGET_FMT_lx "]\n",
> + tb->tc_ptr, tb->pc, n,
> + tb_next->tc_ptr, tb_next->pc);
> +
> + /* patch the native jump address */
> + tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
> +
> + /* add in TB jmp circular list */
> + tb->jmp_list_next[n] = tb_next->jmp_list_first;
> + assert(((uintptr_t)tb & 3) == 0);
I think this assert can be dropped. The only call explicitly masks with
TB_EXIT_MASK (which would be a better choice than the number 3 anyway)
so something really strange would have had to happen in the intervening
few lines.
Otherwise:
Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
> + tb_next->jmp_list_first = (uintptr_t)tb | n;
> }
>
> /* GETRA is the true target of the return instruction that we'll execute,
--
Alex Bennée
