From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49655) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asVNa-0004TT-6C for qemu-devel@nongnu.org; Tue, 19 Apr 2016 09:07:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asVNW-0007aA-5b for qemu-devel@nongnu.org; Tue, 19 Apr 2016 09:07:46 -0400 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:38182) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asVNU-0007a1-Vk for qemu-devel@nongnu.org; Tue, 19 Apr 2016 09:07:42 -0400 Received: by mail-wm0-x231.google.com with SMTP id u206so28628294wme.1 for ; Tue, 19 Apr 2016 06:07:40 -0700 (PDT) References: <1460324732-30330-1-git-send-email-sergey.fedorov@linaro.org> <1460324732-30330-5-git-send-email-sergey.fedorov@linaro.org> <87bn55x3yl.fsf@linaro.org> <5716279A.1000403@gmail.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <5716279A.1000403@gmail.com> Date: Tue, 19 Apr 2016 14:07:37 +0100 Message-ID: <877fftwxuu.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 04/10] tcg: Init TB's direct jumps before making it visible List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Fedorov Cc: Sergey Fedorov , qemu-devel@nongnu.org, Paolo Bonzini , Peter Crosthwaite , Richard Henderson Sergey Fedorov writes: > 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