From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49340) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8AGc-0001yw-Vx for qemu-devel@nongnu.org; Thu, 25 Jun 2015 12:44:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z8AGZ-0002NH-Mh for qemu-devel@nongnu.org; Thu, 25 Jun 2015 12:44:46 -0400 Received: from cantor2.suse.de ([195.135.220.15]:57821 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8AGZ-0002Mu-DD for qemu-devel@nongnu.org; Thu, 25 Jun 2015 12:44:43 -0400 Message-ID: <558C2FFA.4040607@suse.de> Date: Thu, 25 Jun 2015 18:44:42 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <136b4b0f782ea2ccd7feb69a355bae482f423313.1435195913.git.zhugh.fnst@cn.fujitsu.com> In-Reply-To: <136b4b0f782ea2ccd7feb69a355bae482f423313.1435195913.git.zhugh.fnst@cn.fujitsu.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RESEND PATCH v8 3/4] cpu/apic: drop icc bus/bridge List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhu Guihua , qemu-devel@nongnu.org Cc: chen.fan.fnst@cn.fujitsu.com, imammedo@redhat.com, izumi.taku@jp.fujitsu.com, ehabkost@redhat.com, pbonzini@redhat.com Am 25.06.2015 um 04:17 schrieb Zhu Guihua: > From: Chen Fan >=20 > After CPU hotplug has been converted to BUS-less hot-plug infrastructur= e, > the only function ICC bus performs is to propagate reset to LAPICs. How= ever > LAPIC could be reset by registering its reset handler after all device = are > initialized. > Do so and drop ~200LOC of not needed anymore ICCBus related code. >=20 > Signed-off-by: Chen Fan > Signed-off-by: Zhu Guihua > --- > hw/i386/pc.c | 24 +++++++++--------------- > hw/i386/pc_piix.c | 9 +-------- > hw/i386/pc_q35.c | 9 +-------- > hw/intc/apic_common.c | 5 ++--- > include/hw/i386/apic_internal.h | 7 ++++--- > include/hw/i386/pc.h | 2 +- > target-i386/cpu.c | 23 +++++++++++++++-------- > target-i386/cpu.h | 1 + > 8 files changed, 34 insertions(+), 46 deletions(-) >=20 > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 9f16128..c547d74 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -59,7 +59,6 @@ > #include "qemu/error-report.h" > #include "hw/acpi/acpi.h" > #include "hw/acpi/cpu_hotplug.h" > -#include "hw/cpu/icc_bus.h" > #include "hw/boards.h" > #include "hw/pci/pci_host.h" > #include "acpi-build.h" > @@ -969,27 +968,25 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,= int level) > } > } > =20 > +#define x86_cpu_apic_reset_order 0x1 > + > static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > - DeviceState *icc_bridge, Error **errp) > + Error **errp) > { > X86CPU *cpu =3D NULL; > Error *local_err =3D NULL; > =20 > - if (icc_bridge =3D=3D NULL) { > - error_setg(&local_err, "Invalid icc-bridge value"); > - goto out; > - } > - > cpu =3D cpu_x86_create(cpu_model, &local_err); > if (local_err !=3D NULL) { > goto out; > } > =20 > - qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "i= cc")); > - > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_er= r); > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err= ); > =20 > + qemu_register_reset_common(x86_cpu_apic_reset, cpu, > + x86_cpu_apic_reset_order); This usage of an out-of-file reset handler with opaque argument is a little ugly. > + > out: > if (local_err) { > error_propagate(errp, local_err); > @@ -1003,7 +1000,6 @@ static const char *current_cpu_model; > =20 > void pc_hot_add_cpu(const int64_t id, Error **errp) > { > - DeviceState *icc_bridge; > X86CPU *cpu; > int64_t apic_id =3D x86_cpu_apic_id_from_index(id); > Error *local_err =3D NULL; > @@ -1032,9 +1028,7 @@ void pc_hot_add_cpu(const int64_t id, Error **err= p) > return; > } > =20 > - icc_bridge =3D DEVICE(object_resolve_path_type("icc-bridge", > - TYPE_ICC_BRIDGE, NULL= )); > - cpu =3D pc_new_cpu(current_cpu_model, apic_id, icc_bridge, &local_= err); > + cpu =3D pc_new_cpu(current_cpu_model, apic_id, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > @@ -1042,7 +1036,7 @@ void pc_hot_add_cpu(const int64_t id, Error **err= p) > object_unref(OBJECT(cpu)); > } > =20 > -void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge) > +void pc_cpus_init(const char *cpu_model) > { > int i; > X86CPU *cpu =3D NULL; > @@ -1068,7 +1062,7 @@ void pc_cpus_init(const char *cpu_model, DeviceSt= ate *icc_bridge) > =20 > for (i =3D 0; i < smp_cpus; i++) { > cpu =3D pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), > - icc_bridge, &error); > + &error); > if (error) { > error_report_err(error); > exit(1); [...] > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 1fb88f6..e169ce3 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -43,7 +43,6 @@ > =20 > #include "sysemu/sysemu.h" > #include "hw/qdev-properties.h" > -#include "hw/cpu/icc_bus.h" > #ifndef CONFIG_USER_ONLY > #include "exec/address-spaces.h" > #include "hw/xen/xen.h" > @@ -2719,7 +2718,6 @@ static void mce_init(X86CPU *cpu) > #ifndef CONFIG_USER_ONLY > static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > { > - DeviceState *dev =3D DEVICE(cpu); > APICCommonState *apic; > const char *apic_type =3D "apic"; > =20 > @@ -2729,11 +2727,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Err= or **errp) > apic_type =3D "xen-apic"; > } > =20 > - cpu->apic_state =3D qdev_try_create(qdev_get_parent_bus(dev), apic= _type); > - if (cpu->apic_state =3D=3D NULL) { > - error_setg(errp, "APIC device '%s' could not be created", apic= _type); > - return; > - } > + cpu->apic_state =3D DEVICE(object_new(apic_type)); > =20 > object_property_add_child(OBJECT(cpu), "apic", > OBJECT(cpu->apic_state), NULL); > @@ -2754,6 +2748,20 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Er= ror **errp) > errp); > } > =20 > +void x86_cpu_apic_reset(void *opaque) > +{ > + X86CPU *cpu =3D opaque; > + DeviceClass *dc; > + > + if (cpu->apic_state) { > + dc =3D DEVICE_GET_CLASS(cpu->apic_state); > + > + if (dc->reset !=3D NULL) { > + (*dc->reset)(cpu->apic_state); This is the same as dc->reset(cpu->apic_state); ... > + } ... which makes this the same as device_reset(cpu->apic_state);. Why does this need to be a reset handler of its own? > + } > +} > + > static void x86_cpu_machine_done(Notifier *n, void *unused) > { > X86CPU *cpu =3D container_of(n, X86CPU, machine_done); > @@ -3136,7 +3144,6 @@ static void x86_cpu_common_class_init(ObjectClass= *oc, void *data) > =20 > xcc->parent_realize =3D dc->realize; > dc->realize =3D x86_cpu_realizefn; > - dc->bus_type =3D TYPE_ICC_BUS; > dc->props =3D x86_cpu_properties; > =20 > xcc->parent_reset =3D cc->reset; [snip] More comments on the preceding patch. Regards, Andreas --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Felix Imend=F6rffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=FCrnberg)