From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44691) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eC334-0000l9-Fj for qemu-devel@nongnu.org; Tue, 07 Nov 2017 07:32:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eC32y-0000Xy-D4 for qemu-devel@nongnu.org; Tue, 07 Nov 2017 07:32:10 -0500 Date: Tue, 7 Nov 2017 10:31:49 -0200 From: Eduardo Habkost Message-ID: <20171107123149.GZ3111@localhost.localdomain> References: <1509734853-3014-1-git-send-email-cota@braap.org> <20171103185610.GA3907@flamenco> <20171103200233.GI3111@localhost.localdomain> <20171103222407.GA22411@flamenco> <20171106141022.GO3111@localhost.localdomain> <20171106201332.GA2152@flamenco> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] hw: add .min_cpus and .default_cpus fields to machine_class List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alistair Francis Cc: "Emilio G. Cota" , Peter Maydell , Thomas Huth , Igor Mitsyanko , Richard Henderson , "qemu-devel@nongnu.org Developers" , qemu-arm , Igor Mammedov , Marcel Apfelbaum , "Edgar E . Iglesias" 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 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 > > 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