From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39443) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asorU-0003BI-Jz for qemu-devel@nongnu.org; Wed, 20 Apr 2016 05:56:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asorR-0006qg-Cc for qemu-devel@nongnu.org; Wed, 20 Apr 2016 05:55:56 -0400 Received: from mail-wm0-x22f.google.com ([2a00:1450:400c:c09::22f]:35448) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asorR-0006qa-36 for qemu-devel@nongnu.org; Wed, 20 Apr 2016 05:55:53 -0400 Received: by mail-wm0-x22f.google.com with SMTP id e201so43944519wme.0 for ; Wed, 20 Apr 2016 02:55:52 -0700 (PDT) References: <1460044433-19282-1-git-send-email-sergey.fedorov@linaro.org> <1460044433-19282-6-git-send-email-sergey.fedorov@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <1460044433-19282-6-git-send-email-sergey.fedorov@linaro.org> Date: Wed, 20 Apr 2016 10:55:50 +0100 Message-ID: <87potkvc2h.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 05/11] tcg/i386: Make direct jump patching thread-safe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Fedorov Cc: qemu-devel@nongnu.org, Sergey Fedorov , Paolo Bonzini , Peter Crosthwaite , Richard Henderson Sergey Fedorov writes: > From: Sergey Fedorov > > Ensure direct jump patching in i386 is atomic by: > * naturally aligning a location of direct jump address; > * using atomic_read()/atomic_set() for code patching. > > Signed-off-by: Sergey Fedorov > Signed-off-by: Sergey Fedorov > --- > include/exec/exec-all.h | 2 +- > tcg/i386/tcg-target.inc.c | 17 +++++++++++++++++ > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 59709c9dd5c9..82399175fe80 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -312,7 +312,7 @@ void ppc_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr); > static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr) > { > /* patch the branch destination */ > - stl_le_p((void*)jmp_addr, addr - (jmp_addr + 4)); > + atomic_set((int32_t *)jmp_addr, addr - (jmp_addr + 4)); > /* no need to flush icache explicitly */ > } > #elif defined(__s390x__) > diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c > index 9187d34caf6d..3ffb7b3124d8 100644 > --- a/tcg/i386/tcg-target.inc.c > +++ b/tcg/i386/tcg-target.inc.c > @@ -1123,6 +1123,19 @@ static void tcg_out_jmp(TCGContext *s, tcg_insn_unit *dest) > tcg_out_branch(s, 0, dest); > } > > +static void tcg_out_nopn(TCGContext *s, int n) > +{ > + static const uint8_t nop1[] = { 0x90 }; > + static const uint8_t nop2[] = { 0x66, 0x90 }; > + static const uint8_t nop3[] = { 0x8d, 0x76, 0x00 }; > + static const uint8_t *const nopn[] = { nop1, nop2, nop3 }; > + int i; > + assert(n <= ARRAY_SIZE(nopn)); > + for (i = 0; i < n; ++i) { > + tcg_out8(s, nopn[n - 1][i]); > + } > +} *shudder* I recall x86 instruction encoding is weird. Maybe a comment for the function to describe the 3 forms of NOP we have here? > + > #if defined(CONFIG_SOFTMMU) > /* helper signature: helper_ret_ld_mmu(CPUState *env, target_ulong addr, > * int mmu_idx, uintptr_t ra) > @@ -1777,6 +1790,10 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, > case INDEX_op_goto_tb: > if (s->tb_jmp_offset) { > /* direct jump method */ > + /* align jump displacement for atomic pathing */ s/pathing/patching/ > + if (((uintptr_t)s->code_ptr & 3) != 3) { > + tcg_out_nopn(s, 3 - ((uintptr_t)s->code_ptr & 3)); > + } apropos my previous comments. I think the intention could be made clearer the use of well named helper functions to check alignment and calculate number elements until next alignment. > tcg_out8(s, OPC_JMP_long); /* jmp im */ > s->tb_jmp_offset[args[0]] = tcg_current_code_size(s); > tcg_out32(s, 0); -- Alex Bennée