From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37888) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNg1d-0005Uu-FN for qemu-devel@nongnu.org; Thu, 04 Apr 2013 05:00:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UNg1Y-0005nS-7H for qemu-devel@nongnu.org; Thu, 04 Apr 2013 05:00:05 -0400 Received: from cantor2.suse.de ([195.135.220.15]:56505 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNg1X-0005nA-U2 for qemu-devel@nongnu.org; Thu, 04 Apr 2013 05:00:00 -0400 Message-ID: <515D410B.2000404@suse.de> Date: Thu, 04 Apr 2013 10:59:55 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1363876125-8264-1-git-send-email-imammedo@redhat.com> <1363876125-8264-3-git-send-email-imammedo@redhat.com> In-Reply-To: <1363876125-8264-3-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 02/12] target-i386: split APIC creation from initialization in x86_cpu_realizefn() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, ehabkost@redhat.com, mst@redhat.com, jan.kiszka@siemens.com, stefano.stabellini@eu.citrix.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, aderumier@odiso.com, armbru@redhat.com, blauwirbel@gmail.com, quintela@redhat.com, alex.williamson@redhat.com, kraxel@redhat.com, anthony.perard@citrix.com, yang.z.zhang@intel.com, pbonzini@redhat.com, lcapitulino@redhat.com, rth@twiddle.net Am 21.03.2013 15:28, schrieb Igor Mammedov: > When APIC is hotplugged during CPU hotplug, device_set_realized() > calls device_reset() on it. And if QEMU runs in KVM mode, following > call chain will fail: > apic_reset_common() > -> kvm_apic_vapic_base_update() > -> kvm_vcpu_ioctl(cpu->kvm_fd,...) > due to cpu->kvm_fd not being initialized yet. >=20 > cpu->kvm_fd is initialized during qemu_init_vcpu() call but x86_cpu_api= c_init() > can't be moved after it because kvm_init_vcpu() -> kvm_arch_reset_vcpu(= ) > relies on APIC to determine if CPU is BSP for setting initial env->mp_s= tate. >=20 > So split APIC device creation from its initialization and realize APIC > after CPU is created, when it's safe to call APIC's reset method. >=20 > Signed-off-by: Igor Mammedov > --- > target-i386/cpu.c | 24 +++++++++++++++++++++--- > 1 files changed, 21 insertions(+), 3 deletions(-) >=20 > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index e905bcf..affbb76 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2051,9 +2051,8 @@ static void mce_init(X86CPU *cpu) > #define MSI_ADDR_BASE 0xfee00000 > =20 > #ifndef CONFIG_USER_ONLY > -static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) > +static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > { > - static int apic_mapped; > CPUX86State *env =3D &cpu->env; > APICCommonState *apic; > const char *apic_type =3D "apic"; > @@ -2076,6 +2075,16 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error= **errp) > /* TODO: convert to link<> */ > apic =3D APIC_COMMON(env->apic_state); > apic->cpu =3D cpu; > +} > + > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) > +{ > + CPUX86State *env =3D &cpu->env; > + static int apic_mapped; > + > + if (env->apic_state =3D=3D NULL) { > + return; > + } > =20 > if (qdev_init(env->apic_state)) { > error_setg(errp, "APIC device '%s' could not be initialized", > @@ -2093,6 +2102,10 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error= **errp) > apic_mapped =3D 1; > } > } > +#else > +static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) > +{ > +} > #endif > =20 > static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > @@ -2143,7 +2156,7 @@ static void x86_cpu_realizefn(DeviceState *dev, E= rror **errp) > qemu_register_reset(x86_cpu_machine_reset_cb, cpu); > =20 > if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) { > - x86_cpu_apic_init(cpu, &local_err); > + x86_cpu_apic_create(cpu, &local_err); > if (local_err !=3D NULL) { > goto out; > } > @@ -2152,6 +2165,11 @@ static void x86_cpu_realizefn(DeviceState *dev, = Error **errp) > =20 > mce_init(cpu); > qemu_init_vcpu(&cpu->env); > + > + x86_cpu_apic_init(cpu, &local_err); If we use the function like this, my request on IRC was to rename it to _realize please instead of _init. What's the plan for APIC link<>s above? I'm not opposed to the function split, but I wondered if - since we know there are only three possible APIC types - a union would allow us to make this an _initialize function, more closely following the QOM workflow? Could be done as follow-up. Andreas > + if (local_err !=3D NULL) { > + goto out; > + } > cpu_reset(CPU(cpu)); > =20 > xcc->parent_realize(dev, &local_err); >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg