Re: [Qemu-arm] [PATCH v4 09/10] tcg: Clean up direct block chaining safety checks

[Alex Bennée]

Sergey Fedorov <address@hidden> writes:

> On 21/04/16 16:18, Alex Bennée wrote:
>> Sergey Fedorov <address@hidden> writes:
>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>> index bbfcbfb54385..065cc9159477 100644
>>> --- a/cpu-exec.c
>>> +++ b/cpu-exec.c
>>> @@ -508,11 +508,8 @@ int cpu_exec(CPUState *cpu)
>>>                      next_tb = 0;
>>>                      tcg_ctx.tb_ctx.tb_invalidated_flag = 0;
>>>                  }
>>> -                /* see if we can patch the calling TB. When the TB
>>> -                   spans two pages, we cannot safely do a direct
>>> -                   jump. */
>>> -                if (next_tb != 0 && tb->page_addr[1] == -1
>>> -                    && !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>>> +                /* See if we can patch the calling TB. */
>>> +                if (next_tb != 0 &&
>>> !qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>> A pointer to the definitive comment helps ;-)
>>
>>                 /* See if we can patch the calling TB, see tcg_gen_goto_tb */
>
> I'm not so sure that the comment for tcg_gen_goto_tb() would be of much
> use here. Actually, what we check here is if we know the calling TB
> (what is called 'next_tb' here so far) and if logging settings don't
> forbid us to chain TBs. The note in the comment for tcg_gen_goto_tb() is
> all about when goto_tb TCG ops can be emitted by the target translation
> code, not so relevant here, I suppose.

True, it makes more sense on the following patches. It's not a major
thing.

>
> Kind regards,
> Sergey
>
>>
>>>                      tb_add_jump((TranslationBlock *)(next_tb & 
>>> ~TB_EXIT_MASK),
>>>                                  next_tb & TB_EXIT_MASK, tb);
>>>                  }
> (snip)
>>> diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
>>> index c446d3dc7293..ace39619ef89 100644
>>> --- a/tcg/tcg-op.h
>>> +++ b/tcg/tcg-op.h
>>> @@ -753,6 +753,16 @@ static inline void tcg_gen_exit_tb(uintptr_t val)
>>>      tcg_gen_op1i(INDEX_op_exit_tb, val);
>>>  }
>>>
>>> +/**
>>> + * tcg_gen_goto_tb() - output goto_tb TCG operation
>>> + * @idx: Direct jump slot index (0 or 1)
>>> + *
>>> + * See tcg/README for more info about this TCG operation.
>>> + *
>>> + * NOTE: Direct jumps with goto_tb are only safe within the pages this TB
>>> + * resides in because we don't take care of direct jumps when address 
>>> mapping
>>> + * changes, e.g. in tlb_flush().
>>> + */
>>>  void tcg_gen_goto_tb(unsigned idx);
>>>
>>>  #if TARGET_LONG_BITS == 32


--
Alex Bennée