Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Nov 06, 2017 at 04:43:34PM -0800, Alistair Francis wrote:
> On Mon, Nov 6, 2017 at 12:13 PM, Emilio G. Cota <cota@braap.org> wrote:
> > On Mon, Nov 06, 2017 at 12:10:22 -0200, Eduardo Habkost wrote:
> >> IMO, initialization state doesn't belong to CPUClass.  We already
> >> have a single accelerator object in MachineState::accelerator,
> >> and tcg_initialized could be moved to a AccelState::initialized
> >> field.
> >
> > I don't know how to cleanly get AccelState from a CPUClass pointer
> > (as I said I'm not familiar with object code / qom) -- suggestions
> > welcome! The best I could come up in the limited time I have for
> > this is to use a static bool, as shown below.

I don't believe a TYPE_ACCEL object is created in *-user.  We can
consider changing that, but not during 2.11 freeze.

> >
> >
> > ---8<---
> >
> > Subject: [PATCH] qom: move CPUClass.tcg_initialize to a global
> >
> > 55c3cee ("qom: Introduce CPUClass.tcg_initialize", 2017-10-24)
> > introduces a per-CPUClass bool that we check so that the target CPU
> > is initialized for TCG only once. This works well except when
> > we end up creating more than one CPUClass, in which case we end
> > up incorrectly initializing TCG more than once, i.e. once for
> > each CPUClass.
> >
> > This can be replicated with:
> >   $ aarch64-softmmu/qemu-system-aarch64 -machine xlnx-zcu102 -smp 6 \
> >       -global driver=xlnx,,zynqmp,property=has_rpu,value=on
> > In this case the class name of the "RPUs" is prefixed by "cortex-r5-",
> > whereas the "regular" CPUs are prefixed by "cortex-a53-". This
> > results in two CPUClass instances being created.
> >
> > Fix it by introducing a static variable, so that only the first
> > target CPU being initialized will initialize the target-dependent
> > part of TCG, regardless of CPUClass instances.
> >
> > Fixes: 55c3ceef61fcf06fc98ddc752b7cce788ce7680b
> > Signed-off-by: Emilio G. Cota <cota@braap.org>
> 
> This works great for me, is this ok to be applied?

It depends if we are already relying on initialization of CPUs of
different types to be calling two different ->tcg_initialize()
functions.  I think that's the case today, so this looks
reasonable as a quick fix for 2.11.

I would wait for an Acked-by from Richard Henderson before
applying it, though.  Richard, what do you think?

> 
> Thanks,
> Alistair
> 
> > ---
> >  exec.c            | 5 +++--
> >  include/qom/cpu.h | 1 -
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 97a24a8..8b579c0 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -792,11 +792,12 @@ void cpu_exec_initfn(CPUState *cpu)
> >  void cpu_exec_realizefn(CPUState *cpu, Error **errp)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    static bool tcg_target_initialized;
> >
> >      cpu_list_add(cpu);
> >
> > -    if (tcg_enabled() && !cc->tcg_initialized) {
> > -        cc->tcg_initialized = true;
> > +    if (tcg_enabled() && !tcg_target_initialized) {
> > +        tcg_target_initialized = true;
> >          cc->tcg_initialize();
> >      }
> >
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index fa4b0c9..c2fa151 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -209,7 +209,6 @@ typedef struct CPUClass {
> >      /* Keep non-pointer data at the end to minimize holes.  */
> >      int gdb_num_core_regs;
> >      bool gdb_stop_before_watchpoint;
> > -    bool tcg_initialized;
> >  } CPUClass;
> >
> >  #ifdef HOST_WORDS_BIGENDIAN
> > --
> > 2.7.4
> >
> >

-- 
Eduardo