| Message ID | 1458815961-31979-5-git-send-email-sergey.fedorov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > translate-all.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/translate-all.c b/translate-all.c > index ca01dd325b8d..f68716e1819f 100644 > --- a/translate-all.c > +++ b/translate-all.c > @@ -1131,19 +1131,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 > @@ -1251,6 +1238,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; maybe these should be further up the function with the other jmp setting code? > + > + /* 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); > + } Why would tb->jmp_reset_offset[0] == TB_JMP_RESET_OFFSET_INVALID not be the case as it is set a few lines further up. > + > /* check next page if needed */ > virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK; > phys_page2 = -1; -- Alex Bennée
On 24/03/16 18:11, Alex Bennée wrote: > sergey.fedorov@linaro.org writes: >> From: Sergey Fedorov <serge.fdrv@gmail.com> >> >> diff --git a/translate-all.c b/translate-all.c >> index ca01dd325b8d..f68716e1819f 100644 >> --- a/translate-all.c >> +++ b/translate-all.c >> @@ -1131,19 +1131,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 >> @@ -1251,6 +1238,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; > maybe these should be further up the function with the other jmp setting code? I meant to keep them together with the following lines. > >> + >> + /* 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); >> + } > Why would tb->jmp_reset_offset[0] == TB_JMP_RESET_OFFSET_INVALID not be > the case as it is set a few lines further up. Because tcg_gen_code() gets called in between and it is passed '&tcg_ctx' which holds a pointer to 'tb->jmp_reset_offset'. tcg_out_op() is called from tcg_gen_code() and sets 'tb->jmp_reset_offset[n]' indirectly, as well as 'tb->jmp_insn_offset[n]'. Kind regards, Sergey
Sergey Fedorov <serge.fdrv@gmail.com> writes: > On 24/03/16 18:11, Alex Bennée wrote: >> sergey.fedorov@linaro.org writes: >>> From: Sergey Fedorov <serge.fdrv@gmail.com> >>> >>> diff --git a/translate-all.c b/translate-all.c >>> index ca01dd325b8d..f68716e1819f 100644 >>> --- a/translate-all.c >>> +++ b/translate-all.c >>> @@ -1131,19 +1131,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 >>> @@ -1251,6 +1238,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; >> maybe these should be further up the function with the other jmp setting code? > > I meant to keep them together with the following lines. > >> >>> + >>> + /* 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); >>> + } >> Why would tb->jmp_reset_offset[0] == TB_JMP_RESET_OFFSET_INVALID not be >> the case as it is set a few lines further up. > > Because tcg_gen_code() gets called in between and it is passed > '&tcg_ctx' which holds a pointer to 'tb->jmp_reset_offset'. tcg_out_op() > is called from tcg_gen_code() and sets 'tb->jmp_reset_offset[n]' > indirectly, as well as 'tb->jmp_insn_offset[n]'. OK a quick addition to the comment: "these may have been reset in tcg_gen_code" would be helpful here. > > Kind regards, > Sergey -- Alex Bennée
On 24/03/16 18:40, Alex Bennée wrote: > Sergey Fedorov <serge.fdrv@gmail.com> writes: > >> On 24/03/16 18:11, Alex Bennée wrote: >>> sergey.fedorov@linaro.org writes: >>>> From: Sergey Fedorov <serge.fdrv@gmail.com> >>>> >>>> diff --git a/translate-all.c b/translate-all.c >>>> index ca01dd325b8d..f68716e1819f 100644 >>>> --- a/translate-all.c >>>> +++ b/translate-all.c >>>> @@ -1131,19 +1131,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 >>>> @@ -1251,6 +1238,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; >>> maybe these should be further up the function with the other jmp setting code? >> I meant to keep them together with the following lines. >> >>>> + >>>> + /* 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); >>>> + } >>> Why would tb->jmp_reset_offset[0] == TB_JMP_RESET_OFFSET_INVALID not be >>> the case as it is set a few lines further up. >> Because tcg_gen_code() gets called in between and it is passed >> '&tcg_ctx' which holds a pointer to 'tb->jmp_reset_offset'. tcg_out_op() >> is called from tcg_gen_code() and sets 'tb->jmp_reset_offset[n]' >> indirectly, as well as 'tb->jmp_insn_offset[n]'. > OK a quick addition to the comment: "these may have been reset in > tcg_gen_code" would be helpful here. The other way around: 'tb->jmp_reset_offset[n]' are reset before calling tcg_gen_code() and are set during tcg_gen_code() execution. Kind regards, Sergey
diff --git a/translate-all.c b/translate-all.c index ca01dd325b8d..f68716e1819f 100644 --- a/translate-all.c +++ b/translate-all.c @@ -1131,19 +1131,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 @@ -1251,6 +1238,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 */ + 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;