Sergey Fedorov <[email protected]> writes: > On 19/04/16 13:55, Alex Bennée wrote: >> Sergey Fedorov <[email protected]> writes: >> >>> From: Sergey Fedorov <[email protected]> >>> >>> 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 <[email protected]> >>> Signed-off-by: Sergey Fedorov <[email protected]> >>> --- >>> >>> 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? > > Currently, it is synchronized by 'tb_lock', so no need in a memory > barrier here. So this is a simple rearrangement of code to a more > suitable place and maybe just a preparation for relaxing locking scheme > in future. It would be ahead of time and unnecessary overhead to put a > barrier in this patch. Do you think it's worth to mention that in the > commit message?
Good point. Maybe it would be better to clean-up the comment in tb_link_page() with the assumption that memory consistency for linking the new TB is either explicit (user mode, tb_lock) or implicit in single threaded softmmu emulation. We can then update the comment when we add the MTTCG patches. > > Kind regards, > Sergey > >> >>> /* check next page if needed */ >>> virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK; >>> phys_page2 = -1; >> -- Alex Bennée
