Re: [Qemu-devel] [PATCH v3 04/10] tcg: Init TB's direct jumps before making it visible

["Alex Bennée" <alex.bennee@linaro.org>]

Sergey Fedorov <sergey.fedorov@linaro.org> writes:

> From: Sergey Fedorov <serge.fdrv@gmail.com>
>
> Initialize TB's direct jump list data fields and reset the jumps before
> tb_link_page() puts it into the physical hash table and the physical
> page list. So TB is completely initialized before it becomes visible.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
>
> Changes in v2:
>  * Tweaked a comment
>
>  translate-all.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index 7ac7916f2792..dfa7f0d64e76 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1133,19 +1133,6 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>          tb->page_addr[1] = -1;
>      }
>
> -    assert(((uintptr_t)tb & 3) == 0);
> -    tb->jmp_list_first = (uintptr_t)tb | 2;
> -    tb->jmp_list_next[0] = (uintptr_t)NULL;
> -    tb->jmp_list_next[1] = (uintptr_t)NULL;
> -
> -    /* init original jump addresses */
> -    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
> -        tb_reset_jump(tb, 0);
> -    }
> -    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
> -        tb_reset_jump(tb, 1);
> -    }
> -
>  #ifdef DEBUG_TB_CHECK
>      tb_page_check();
>  #endif
> @@ -1254,6 +1241,20 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>          ROUND_UP((uintptr_t)gen_code_buf + gen_code_size + search_size,
>                   CODE_GEN_ALIGN);
>
> +    /* init jump list */
> +    assert(((uintptr_t)tb & 3) == 0);
> +    tb->jmp_list_first = (uintptr_t)tb | 2;
> +    tb->jmp_list_next[0] = (uintptr_t)NULL;
> +    tb->jmp_list_next[1] = (uintptr_t)NULL;
> +
> +    /* init original jump addresses wich has been set during tcg_gen_code() */
> +    if (tb->jmp_reset_offset[0] != TB_JMP_RESET_OFFSET_INVALID) {
> +        tb_reset_jump(tb, 0);
> +    }
> +    if (tb->jmp_reset_offset[1] != TB_JMP_RESET_OFFSET_INVALID) {
> +        tb_reset_jump(tb, 1);
> +    }
> +

If we are really concerned about ensuring everything is set before we
insert the TB into the list should we not have an explicit write barrier
before we call to link the page?

>      /* check next page if needed */
>      virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK;
>      phys_page2 = -1;


--
Alex Bennée