Re: [Qemu-devel] [PATCH v3 04/10] tcg: Init TB's direct jumps before making it visible
Sergey Fedorovwrites: > On 19/04/16 13:55, Alex Bennée wrote: >> Sergey Fedorov writes: >> >>> From: Sergey Fedorov >>> >>> 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 >>> Signed-off-by: Sergey Fedorov >>> --- >>> >>> 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
Re: [Qemu-devel] [PATCH v3 04/10] tcg: Init TB's direct jumps before making it visible
On 19/04/16 13:55, Alex Bennée wrote: > Sergey Fedorovwrites: > >> From: Sergey Fedorov >> >> 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 >> Signed-off-by: Sergey Fedorov >> --- >> >> 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? Kind regards, Sergey > >> /* check next page if needed */ >> virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK; >> phys_page2 = -1; >
Re: [Qemu-devel] [PATCH v3 04/10] tcg: Init TB's direct jumps before making it visible
Sergey Fedorovwrites: > From: Sergey Fedorov > > 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 > Signed-off-by: Sergey Fedorov > --- > > 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
[Qemu-devel] [PATCH v3 04/10] tcg: Init TB's direct jumps before making it visible
From: Sergey FedorovInitialize 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 Signed-off-by: Sergey Fedorov --- 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); +} + /* check next page if needed */ virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK; phys_page2 = -1; -- 2.8.1