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

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

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

> On 18/04/16 20:51, Sergey Fedorov wrote:
>> On 18/04/16 20:17, Alex Bennée wrote:
>>> Sergey Fedorov <serge.fdrv@gmail.com> writes:
>>>> On 18/04/16 17:09, Alex Bennée wrote:
>>>>> Sergey Fedorov <sergey.fedorov@linaro.org> writes:
>>>>>> 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'm mainly eyeing the tb_lock/unlock which would be nice to push further
>>> down the call chain if we can, especially if the need to lock
>>> tb_find_fast can be removed later on.
>> Yes, it would be nice to possibly have all tb_lock/unlock() calls (or at
>> least their pairs) in the same block. There is a lot to be thought over :)
>
> It's not so simple because tb_find_fast() is also called in replay mode
> to find a TB for cpu_exec_nocache()... I'm not sure it's worth touching
> it now.

If the locking is pushed into tb_find_fast or further down is this an
issue?

> Although it may be possible to improve the code structure of
> cpu_exec() in some other way. (It's really scary, indeed.) Actually,
> I've been keeping that in mind for some time. Do you think if MTTCG
> would benefit from some cpu_exec() refactoring to make it more clear and
> easy to understand?

The cpu-exec stuff certainly suffers from a slight "there be dragons"
quality thanks to the longjmp stuff and quite detailed handling of each
IRQ case in the main loop. It doesn't help there is still some special
x86 #ifdef sauce in there.

It would be nice if we could get to the main out-loop being a page where
the flow is easy to follow.

We handle exceptions
We handle IRQs (be clear about the difference, I think exceptions are internal?)
We loop from TB to TB generating as we go
 - when we exit the loop
  - a longjmp
  - an exit_request
  - an expiration of icount

Having said that apart from tb_lock() considerations there isn't much
messing about MTTCG does around this code. All the important stuff has
been done dealing with the exit conditions from cpu_exec.

I think this puts re-factoring on the nice-to-have list rather than
essential for MTTCG.

>
> Kind regards,
> Sergey


--
Alex Bennée