Sergey Fedorov <serge.f...@gmail.com> writes:
> On 19/04/16 14:01, Alex Bennée wrote:
>> Sergey Fedorov <sergey.fedo...@linaro.org> writes:
>>
>>> From: Sergey Fedorov <serge.f...@gmail.com>
>>>
>>> The check is to make sure that another thread hasn't already done the
>>> same while we were outside of tb_lock. Mention this in a comment.
>>>
>>> Signed-off-by: Sergey Fedorov <serge.f...@gmail.com>
>>> Signed-off-by: Sergey Fedorov <sergey.fedo...@linaro.org>
>>> ---
>>>
>>> Changes in v2:
>>> * Typo fixed in the commit title
>>> * Complete rewrite of the commit body and the patch based on Paolo's
>>> comments
>>>
>>> include/exec/exec-all.h | 31 +++++++++++++++++--------------
>>> 1 file changed, 17 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> index b055716ed690..8e81ef5fb2c2 100644
>>> --- a/include/exec/exec-all.h
>>> +++ b/include/exec/exec-all.h
>>> @@ -391,21 +391,24 @@ static inline void tb_set_jmp_target(TranslationBlock
>>> *tb,
>>> static inline void tb_add_jump(TranslationBlock *tb, int n,
>>> TranslationBlock *tb_next)
>>> {
>>> - /* NOTE: this test is only needed for thread safety */
>>> - if (!tb->jmp_list_next[n]) {
>>> - qemu_log_mask_and_addr(CPU_LOG_EXEC, tb->pc,
>>> - "Linking TBs %p [" TARGET_FMT_lx
>>> - "] index %d -> %p [" TARGET_FMT_lx "]\n",
>>> - tb->tc_ptr, tb->pc, n,
>>> - tb_next->tc_ptr, tb_next->pc);
>>> - /* patch the native jump address */
>>> - tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
>>> -
>>> - /* add in TB jmp circular list */
>>> - tb->jmp_list_next[n] = tb_next->jmp_list_first;
>>> - assert(((uintptr_t)tb & 3) == 0);
>>> - tb_next->jmp_list_first = (uintptr_t)tb | n;
>>> + if (tb->jmp_list_next[n]) {
>>> + /* Another thread has already done this while we were
>>> + * outside of the lock; nothing to do in this case */
>>> + return;
>>> }
>>> + qemu_log_mask_and_addr(CPU_LOG_EXEC, tb->pc,
>>> + "Linking TBs %p [" TARGET_FMT_lx
>>> + "] index %d -> %p [" TARGET_FMT_lx "]\n",
>>> + tb->tc_ptr, tb->pc, n,
>>> + tb_next->tc_ptr, tb_next->pc);
>>> +
>>> + /* patch the native jump address */
>>> + tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
>>> +
>>> + /* add in TB jmp circular list */
>>> + tb->jmp_list_next[n] = tb_next->jmp_list_first;
>>> + assert(((uintptr_t)tb & 3) == 0);
>> I think this assert can be dropped. The only call explicitly masks with
>> TB_EXIT_MASK (which would be a better choice than the number 3 anyway)
>> so something really strange would have had to happen in the intervening
>> few lines.
>
> What about the same assert in tb_gen_code()?
I think in tb_link_page it is reasonable although fairly unlikely that
tb_alloc is going to start spitting out unaligned TranslationBlocks.
>
> Kind regards,
> Sergey
>
>>
>> Otherwise:
>>
>> Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
>>
>>> + tb_next->jmp_list_first = (uintptr_t)tb | n;
>>> }
>>>
>>> /* GETRA is the true target of the return instruction that we'll execute,
>>
--
Alex Bennée
