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

["Alex Bennée" <alex.bennee@linaro.org>]

Sergey Fedorov <serge.fdrv@gmail.com> writes:

> On 19/04/16 14:37, Alex Bennée wrote:
>> Sergey Fedorov <sergey.fedorov@linaro.org> writes:
> (snip)
>>> 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)) {
>>>                      tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
>>>                                  next_tb & TB_EXIT_MASK, tb);
>>>                  }
>> We've already discussed on IRC the confusion of next_tb ;-)
>
> Yes, I'd rather add a patch to clean up 'next_tb' naming to "tcg: Misc
> clean-up patches" series. This series is to deal with direct block
> chaining only.

Yeah that makes sense.

>
>>> diff --git a/target-alpha/translate.c b/target-alpha/translate.c
>>> index 5b86992dd367..5fa66309ce2e 100644
>>> --- a/target-alpha/translate.c
>>> +++ b/target-alpha/translate.c
>>> @@ -464,7 +464,10 @@ static bool use_goto_tb(DisasContext *ctx, uint64_t dest)
>>>      if (in_superpage(ctx, dest)) {
>>>          return true;
>>>      }
>>> -    /* Check for the dest on the same page as the start of the TB.  */
>>> +    /* Direct jumps with goto_tb are only safe within the page this TB resides
>>> +     * in because we don't take care of direct jumps when address mapping
>>> +     * changes.
>>> +     */
>> I'm all for harmonising the comments but I think for the common case we
>> could refer to a central location for the page linking rules and only
>> expand the comment for subtle differences between the front ends.
>
> You mean, it'd be better to put a detailed explanation in a comment for
> e.g. tlb_flush() and refer to it in every place like this?

Yes.

>
>
>>>      return ((ctx->tb->pc ^ dest) & TARGET_PAGE_MASK) == 0;
>>>  }
> (snip)
>>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>>> index 940ec8d981d1..aeb3e84e8d40 100644
>>> --- a/target-arm/translate.c
>>> +++ b/target-arm/translate.c
>>> @@ -4054,7 +4054,12 @@ static inline void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
>>>      TranslationBlock *tb;
>>>
>>>      tb = s->tb;
>>> -    if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {
>>> +    /* 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.
>>> +     */
>>> +    if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) ||
>>> +        ((s->pc - 1) & TARGET_PAGE_MASK) == (dest &
>>> TARGET_PAGE_MASK)) {
>> Isn't this check avoided by the fact the translate loop bails on end_of_page?
>
> A TB can straddle a page boundary in Thumb mode. A TB can begin with an
> T32 instruction straddling a page boundary, thus this check should be
> useful :)

OK.

>
> Kind regards,
> Sergey


--
Alex Bennée