From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56896) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2pxO-0007Ad-HU for qemu-devel@nongnu.org; Fri, 26 Jul 2013 17:53:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V2pxN-0004a7-69 for qemu-devel@nongnu.org; Fri, 26 Jul 2013 17:53:50 -0400 Received: from cantor2.suse.de ([195.135.220.15]:47050 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2pxM-0004a0-Sv for qemu-devel@nongnu.org; Fri, 26 Jul 2013 17:53:49 -0400 Message-ID: <51F2EFE7.4000706@suse.de> Date: Fri, 26 Jul 2013 23:53:43 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1374808132-30729-1-git-send-email-chen.fan.fnst@cn.fujitsu.com> In-Reply-To: <1374808132-30729-1-git-send-email-chen.fan.fnst@cn.fujitsu.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RESEND][PATCH v2] cpu: Correct cpu-hotplug failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chen Fan , Igor Mammedov Cc: qemu-devel@nongnu.org Am 26.07.2013 05:08, schrieb Chen Fan: > This regression is caused by commit c643bed99. >=20 > When using x86_64-softmmu --enable-kvm boot qemu, cpu-add command fails= to add a vcpu, > there show (KVM: setting VAPIC address failed), but at boot, there is n= o problem. >=20 > The reason for this is: > in hotplug case it uses an uninitialized vcpu to set apic into kvm, the= n calls qemu_init_vcpu below. > ->x86_cpu_realizefn > ->x86_cpu_apic_realize > ->qdev_init > ->device_set_realized > ->device_reset (hotplugged =3D=3D 1) > ->apic_reset_common > ->vapic_base_update > ->kvm_apic_vapic_base_update (at here, vcpu is uninitialized, kv= mfd is zero, fail to ioctl). >=20 > But in startup case system wide reset after vcpu was initialized. > ->machine->init -> qdev_machine_creation_done -> qemu_system_reset -> > cpu_synchronize_all_post_reset -> cpu_synchronize_post_reset. >=20 > v1-v2: Change cpu_apic_realize to post_vcpu_init. >=20 > Signed-off-by: Chen Fan I have started to overhaul this patch (e.g., error handling and naming) but am not feeling comfortable to include that in tonight's pull - will prepare a v3 for Monday's pull. Reset looks broken? Would need to move that after APIC realization - needs testing. Andreas > --- > include/qom/cpu.h | 2 ++ > qom/cpu.c | 13 +++++++++++++ > target-i386/cpu.c | 10 ++++------ > 3 files changed, 19 insertions(+), 6 deletions(-) >=20 > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index daf1835..4b16385 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -80,6 +80,7 @@ struct TranslationBlock; > * @synchronize_from_tb: Callback for synchronizing state from a TCG > * #TranslationBlock. > * @get_phys_page_debug: Callback for obtaining a physical address. > + * @post_vcpu_init: Callback for doing some extra initialization. > * @vmsd: State description for migration. > * > * Represents a CPU family or model. > @@ -108,6 +109,7 @@ typedef struct CPUClass { > void (*set_pc)(CPUState *cpu, vaddr value); > void (*synchronize_from_tb)(CPUState *cpu, struct TranslationBlock= *tb); > hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr); > + void (*post_vcpu_init)(CPUState *cpu, Error **errp); > =20 > const struct VMStateDescription *vmsd; > int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu, > diff --git a/qom/cpu.c b/qom/cpu.c > index 5c45ab5..28f63b7 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -213,12 +213,25 @@ static ObjectClass *cpu_common_class_by_name(cons= t char *cpu_model) > return NULL; > } > =20 > +static void post_vcpu_init(CPUState *cpu, Error **errp) > +{ > + CPUClass *cc =3D CPU_GET_CLASS(cpu); > + if (cc->post_vcpu_init !=3D NULL) { > + (*cc->post_vcpu_init)(cpu, errp); > + } > +} > + > static void cpu_common_realizefn(DeviceState *dev, Error **errp) > { > CPUState *cpu =3D CPU(dev); > =20 > qemu_init_vcpu(cpu); > =20 > + post_vcpu_init(cpu, errp); > + if (error_is_set(errp)) { > + return; > + } > + > if (dev->hotplugged) { > cpu_synchronize_post_init(cpu); > notifier_list_notify(&cpu_added_notifiers, dev); > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index cd350cb..d51ab8b 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2311,8 +2311,9 @@ static void x86_cpu_apic_create(X86CPU *cpu, Erro= r **errp) > apic->cpu =3D cpu; > } > =20 > -static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp) > +static void x86_cpu_apic_realize(CPUState *s, Error **errp) > { > + X86CPU *cpu =3D X86_CPU(s); > CPUX86State *env =3D &cpu->env; > =20 > if (env->apic_state =3D=3D NULL) { > @@ -2326,7 +2327,7 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Err= or **errp) > } > } > #else > -static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp) > +static void x86_cpu_apic_realize(CPUState *s, Error **errp) > { > } > #endif > @@ -2388,10 +2389,6 @@ static void x86_cpu_realizefn(DeviceState *dev, = Error **errp) > =20 > mce_init(cpu); > =20 > - x86_cpu_apic_realize(cpu, &local_err); > - if (local_err !=3D NULL) { > - goto out; > - } > cpu_reset(CPU(cpu)); > =20 > xcc->parent_realize(dev, &local_err); > @@ -2540,6 +2537,7 @@ static void x86_cpu_common_class_init(ObjectClass= *oc, void *data) > cc->synchronize_from_tb =3D x86_cpu_synchronize_from_tb; > cc->get_arch_id =3D x86_cpu_get_arch_id; > cc->get_paging_enabled =3D x86_cpu_get_paging_enabled; > + cc->post_vcpu_init =3D x86_cpu_apic_realize; > #ifndef CONFIG_USER_ONLY > cc->get_memory_mapping =3D x86_cpu_get_memory_mapping; > cc->get_phys_page_debug =3D x86_cpu_get_phys_page_debug; >=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