From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40598) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1asukQ-0005Bz-7l for qemu-devel@nongnu.org; Wed, 20 Apr 2016 12:13:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1asukP-00070V-1c for qemu-devel@nongnu.org; Wed, 20 Apr 2016 12:13:02 -0400 References: <1460044433-19282-1-git-send-email-sergey.fedorov@linaro.org> <1460044433-19282-8-git-send-email-sergey.fedorov@linaro.org> <87lh48v203.fsf@linaro.org> <57179253.9040505@gmail.com> <87fuuguywf.fsf@linaro.org> From: Sergey Fedorov Message-ID: <5717AA85.1010109@gmail.com> Date: Wed, 20 Apr 2016 19:12:53 +0300 MIME-Version: 1.0 In-Reply-To: <87fuuguywf.fsf@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 07/11] tcg/arm: Make direct jump patching thread-safe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Alex_Benn=c3=a9e?= Cc: Sergey Fedorov , qemu-devel@nongnu.org, Paolo Bonzini , Peter Crosthwaite , Richard Henderson , Andrzej Zaborowski , qemu-arm@nongnu.org On 20/04/16 17:40, Alex Bennée wrote: > Sergey Fedorov writes: >> On 20/04/16 16:33, Alex Bennée wrote: >>> Sergey Fedorov writes: >>>> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c >>>> index 3edf6a6f971c..5c69de20bc69 100644 >>>> --- a/tcg/arm/tcg-target.inc.c >>>> +++ b/tcg/arm/tcg-target.inc.c >>>> @@ -121,6 +121,13 @@ static inline void reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target) >>>> *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff); >>>> } >>>> >>>> +static inline void reloc_pc24_atomic(tcg_insn_unit *code_ptr, tcg_insn_unit *target) >>>> +{ >>>> + ptrdiff_t offset = (tcg_ptr_byte_diff(target, code_ptr) - 8) >> >>>> 2; >>> This seems like something a tcg_debug_assert should be ensuring we don't overflow. >> Then we should probably put the same assert into reloc_pc24() as well. >> >>>> + tcg_insn_unit insn = atomic_read(code_ptr); >>> Don't we already know what the instruction should be or could there be >>> multiple ones? >> Actually, it is always what tcg_out_b_noaddr() generates. I'm not sure >> it's worthwhile to introduce tcg_out_b_atomic() or something similar >> here. > No I don't think so. It depends if the branch instruction is going to > have multiple potential conditions (so requiring the read) or always be > the same. So I mean "regarding goto_tb, it's always branch immediate, unconditional". But concerning the function name, it should only patch the immediate offset of the instruction. > >>>> + atomic_set(code_ptr, (insn & ~0xffffff) | (offset & 0xffffff)); >>> Please use deposit32 to set the offset like the aarch64 code. >> Will do. >> >>>> +} >>>> + >>>> static void patch_reloc(tcg_insn_unit *code_ptr, int type, >>>> intptr_t value, intptr_t addend) >>>> { >>>> @@ -1038,6 +1045,16 @@ static void tcg_out_call(TCGContext *s, tcg_insn_unit *addr) >>>> } >>>> } >>>> >>>> +void arm_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr) >>>> +{ >>>> + tcg_insn_unit *code_ptr = (tcg_insn_unit *)jmp_addr; >>>> + tcg_insn_unit *target = (tcg_insn_unit *)addr; >>>> + >>>> + /* we could use a ldr pc, [pc, #-4] kind of branch and avoid the >>>> flush */ >>> So why don't we? >> I think because it's a bit more expensive to take this kind of branch. >> If we assume direct jumps are taken much more times than TB patching >> then it's preferable to optimize for direct jumps instead of for >> patching. > Looking again I see the comment came from code motion so I'm not overly > fussed its just comments like that always leave me hanging. "We could > ...." and I want to know why we don't then ;-) So let's leave it as is? Kind regards, Sergey >>>> + reloc_pc24_atomic(code_ptr, target); >>>> + flush_icache_range(jmp_addr, jmp_addr + 4); >>>> +} >>>> + >>>> static inline void tcg_out_goto_label(TCGContext *s, int cond, TCGLabel *l) >>>> { >>>> if (l->has_value) {