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

[Richard Henderson]

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...

> > +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?

> > +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~