Re: [Qemu-devel] [PATCH v3 06/34] tcg: Add EXCP_ATOMIC

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

Richard Henderson <rth@twiddle.net> writes:

> On 09/12/2016 07:16 AM, Alex Bennée wrote:
>>> +void cpu_exec_step(CPUState *cpu)
>>> +{
>>> +    CPUArchState *env = (CPUArchState *)cpu->env_ptr;
>>> +    TranslationBlock *tb;
>>> +    target_ulong cs_base, pc;
>>> +    uint32_t flags;
>>> +    bool old_tb_flushed;
>>> +
>>> +    old_tb_flushed = cpu->tb_flushed;
>>> +    cpu->tb_flushed = false;
>>
>> Advanced warning, these disappear soon when the async safe work (plus
>> safe tb flush) patches get merged.
>
> Fair enough.
>
> Having thought about this more, I think it may be better to handle this without
> flushing the tb.  To have parallel_cpus included in the TB flags or somesuch
> and keep that TB around.
>
> My thinking is that there are certain things, like alignment, that could result
> in repeated single-stepping.  So better to keep the TB around than keep having
> to regenerate it.
>
>>> +                /* ??? When we begin running cpus in parallel, we should
>>> +                   stop all cpus, clear parallel_cpus, and interpret a
>>> +                   single insn with cpu_exec_step.  In the meantime,
>>> +                   we should never get here.  */
>>> +                abort();
>>
>> Possibly more correct would be:
>>
>>          g_assert(parallel_cpus == false);
>>          abort();
>
> No, since it is here that we would *set* parallel_cpus to false.  Or did you
> mean assert parallel_cpus true?  Not that that helps for the moment...

For SoftMMU this particular loop should never hit because paralell_cpus
should be false, hence we never generate any code that might
EXCP_ATOMIC. It only fails at the moment because of the "hack" for
testing which makes parallel_cpus true.

The MTTCG adds a new thread function for MTTCG mode which
will have to handle EXCP_ATOMIC as discussed.

>
>>> +static void step_atomic(CPUState *cpu)
>>> +{
>>> +    start_exclusive();
>>> +
>>> +    /* Since we got here, we know that parallel_cpus must be true.  */
>>> +    parallel_cpus = false;
>>> +    cpu_exec_step(cpu);
>>> +    parallel_cpus = true;
>>> +
>>> +    end_exclusive();
>>> +}
>>> +
>>
>> Paolo's safe work patches bring the start/end_exclusive functions into
>> cpu-exec-common so I think after that has been merged this function
>> can also be moved and called directly from the MTTCG loop on an
>> EXCP_ATOMIC exit.
>
> Excellent.  Perhaps I should rebase this upon that right away.  Have you got a
> pointer to a tree handy?

Paolo posted a new version recently but I haven't built a tree with it
yet. I was hoping some of the hot-path and maybe barrier stuff would get
merged while I finish reviewing this ;-)

See:

    Subject: [PATCH v7 00/16] cpu-exec: Safe work in quiescent state
    Date: Mon, 12 Sep 2016 13:12:25 +0200
    Message-Id: <1473678761-8885-1-git-send-email-pbonzini@redhat.com>

>
>>> +bool parallel_cpus;
>>
>> So lets add some words to this to distinguish between this and the mttcg
>> enabled flags and its relation to -smp. Something like:
>>
>>   parallel_cpus indicates to the front-ends if code needs to be
>>   generated taking into account multiple threads of execution. It will
>>   be true for linux-user after the first thread clone and if system mode
>>   if MTTCG is enabled. On the transition from false->true any code
>>   generated while false needs to be invalidated. It may be temporally
>>   set to false when generating non-cached code in an exclusive context.
>
> Sure.
>
>
> r~


--
Alex Bennée