Re: [Qemu-arm] [PATCH 07/11] tcg/arm: Make direct jump patching thread-safe

[Sergey Fedorov]

On 20/04/16 17:40, Alex Bennée wrote:
> Sergey Fedorov <address@hidden> writes:
>> On 20/04/16 16:33, Alex Bennée wrote:
>>> Sergey Fedorov <address@hidden> 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) {