| Message ID | 1460324732-30330-5-git-send-email-sergey.fedorov@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
On 19/04/16 13:55, Alex Bennée wrote: > 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? 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; >
Sergey Fedorov <serge.fdrv@gmail.com> writes: > On 19/04/16 13:55, Alex Bennée wrote: >> 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? > > 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
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;