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