Re: [Qemu-devel] [PATCH v3 4/4] tcg: rework tb_invalidated_flag

[Sergey Fedorov <serge.fdrv@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On 18/04/16 17:09, Alex Bennée wrote:
> Sergey Fedorov <sergey.fedorov@linaro.org> writes:
>> From: Sergey Fedorov <serge.fdrv@gmail.com>
>>
>> 'tb_invalidated_flag' was meant to catch two events:
>>  * some TB has been invalidated by tb_phys_invalidate();
>>  * the whole translation buffer has been flushed by tb_flush().
>>
>> Then it was checked:
>>  * in cpu_exec() to ensure that the last executed TB can be safely
>>    linked to directly call the next one;
>>  * in cpu_exec_nocache() to decide if the original TB should be provided
>>    for further possible invalidation along with the temporarily
>>    generated TB.
>>
>> It is always safe to patch an invalidated TB since it is not going to be
>> used anyway.
> Wouldn't that have implications for code searching through the linked
> list of jump patched TBs?

The only implication I can see is that the jump in that already
invalidated TB could just get reset back later on in
tb_phys_invalidate(). We could keep track of invalidated TB's and skip
patching those but it's also some overhead in the main CPU execution
loop wich I'm not sure is worth to be introduced.

(snip)
>> diff --git a/cpu-exec.c b/cpu-exec.c
(snip)
>> @@ -507,14 +510,12 @@ int cpu_exec(CPUState *cpu)
>>                  }
>>                  tb_lock();
>>                  tb = tb_find_fast(cpu);
>> -                /* Note: we do it here to avoid a gcc bug on Mac OS X when
>> -                   doing it in tb_find_slow */
> Is this still true? Would it make more sense to push the patching down
> to the gen_code?

This comment comes up to the commit:

    commit 1538800276aa7228d74f9d00bf275f54dc9e9b43              
    Author: bellard <bellard@c046a42c-6fe2-441c-8c8c-71466251a162>
    Date:   Mon Dec 19 01:42:32 2005 +0000                       

        workaround for gcc bug on PowerPC


It was added more than ten years ago. Anyway, now this code is here not
because of the bug: we need to reset 'next_tb' which is a local variable
in cpu_exec(). Personally, I don't think it would be neater to hide it
into gen_code(). Do you have some thoughts on how we could benefit from
doing so? BTW, I had a feeling that it may be useful to reorganize
cpu_exec() a bit, although I don't have a solid idea of how to do this
so far.

>
> I got slightly confused as to what next_tb ends up meaning at what point
> in the run loop.

Yes, it seems to be a misleading name for this variable. As it was
discussed on IRC, I'd like to break it into two variables, say 'last_tb'
and 'tb_exit_idx', as soon as cpu_tb_exec() returns. Probably this
series could also include such a patch.

Kind regards,
Sergey

[-- Attachment #2: Type: text/html, Size: 3914 bytes --]