From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50055) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWr1q-0003qu-DI for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:34:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWr1o-0003ad-Mb for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:34:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40415) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWr1o-0003ZS-Ek for qemu-devel@nongnu.org; Mon, 29 Apr 2013 12:34:12 -0400 Date: Mon, 29 Apr 2013 18:33:15 +0200 From: Igor Mammedov Message-ID: <20130429183315.24127163@thinkpad> In-Reply-To: <517E9EB7.6010400@suse.de> References: <1367247776-7695-1-git-send-email-imammedo@redhat.com> <1367247776-7695-3-git-send-email-imammedo@redhat.com> <517E9EB7.6010400@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/7] target-i386: Attach ICC bus to CPU on its creation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?ISO-8859-1?B?RuRyYmVy?= Cc: aliguori@us.ibm.com, ehabkost@redhat.com, mst@redhat.com, jan.kiszka@siemens.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, aderumier@odiso.com, lcapitulino@redhat.com, jfrei@linux.vnet.ibm.com, yang.z.zhang@intel.com, pbonzini@redhat.com, lig.fnst@cn.fujitsu.com, rth@twiddle.net On Mon, 29 Apr 2013 18:24:23 +0200 Andreas F=E4rber wrote: > Am 29.04.2013 17:02, schrieb Igor Mammedov: > > X86CPU should have parent bus so it could provide bus for child APIC. > >=20 > > Signed-off-by: Igor Mammedov > > Signed-off-by: Andreas F=E4rber > > --- > > TODO: fix unplug of bus-less devices in device-del > >=20 > > v2: > > - pass icc_bridge to cpu_x86_create instead of resolving it each time > > --- > > hw/i386/pc.c | 10 ++++++---- > > hw/i386/pc_piix.c | 2 +- > > hw/i386/pc_q35.c | 2 +- > > include/hw/i386/pc.h | 2 +- > > target-i386/cpu.c | 11 +++++++++-- > > target-i386/cpu.h | 3 ++- > > 6 files changed, 20 insertions(+), 10 deletions(-) > >=20 > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 7c7dd86..658ff6c 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -890,12 +890,13 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,= int level) > > } > > } > > =20 > > -static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Erro= r **errp) > > +static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > > + DeviceState *icc_bridge, Error **errp) > > { > > X86CPU *cpu; > > Error *local_err =3D NULL; > > =20 > > - cpu =3D cpu_x86_create(cpu_model, errp); > > + cpu =3D cpu_x86_create(cpu_model, icc_bridge, errp); > > if (!cpu) { > > return cpu; > > } > > @@ -913,7 +914,7 @@ static X86CPU *pc_new_cpu(const char *cpu_model, in= t64_t apic_id, Error **errp) > > return cpu; > > } > > =20 > > -void pc_cpus_init(const char *cpu_model) > > +void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge) > > { > > int i; > > Error *error =3D NULL; > > @@ -928,7 +929,8 @@ void pc_cpus_init(const char *cpu_model) > > } > > =20 > > for (i =3D 0; i < smp_cpus; i++) { > > - pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error); > > + pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), > > + icc_bridge, &error); > > if (error) { > > fprintf(stderr, "%s\n", error_get_pretty(error)); > > error_free(error); > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > index 51b738a..2190f0a 100644 > > --- a/hw/i386/pc_piix.c > > +++ b/hw/i386/pc_piix.c > > @@ -93,7 +93,7 @@ static void pc_init1(MemoryRegion *system_memory, > > object_property_add_child(qdev_get_machine(), "icc-bridge", > > OBJECT(icc_bridge), NULL); > > =20 > > - pc_cpus_init(cpu_model); > > + pc_cpus_init(cpu_model, icc_bridge); > > pc_acpi_init("acpi-dsdt.aml"); > > =20 > > if (kvmclock_enabled) { > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > index 317dd0c..a926e38 100644 > > --- a/hw/i386/pc_q35.c > > +++ b/hw/i386/pc_q35.c > > @@ -80,7 +80,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args) > > object_property_add_child(qdev_get_machine(), "icc-bridge", > > OBJECT(icc_bridge), NULL); > > =20 > > - pc_cpus_init(cpu_model); > > + pc_cpus_init(cpu_model, icc_bridge); > > pc_acpi_init("q35-acpi-dsdt.aml"); > > =20 > > kvmclock_create(); > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > index 14b504c..8a6e76c 100644 > > --- a/include/hw/i386/pc.h > > +++ b/include/hw/i386/pc.h > > @@ -78,7 +78,7 @@ extern int fd_bootchk; > > void pc_register_ferr_irq(qemu_irq irq); > > void pc_acpi_smi_interrupt(void *opaque, int irq, int level); > > =20 > > -void pc_cpus_init(const char *cpu_model); > > +void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge); > > void pc_acpi_init(const char *default_dsdt); > > void *pc_memory_init(MemoryRegion *system_memory, > > const char *kernel_filename, > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 0d9493d..94856ec 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -41,6 +41,7 @@ > > #endif > > =20 > > #include "sysemu/sysemu.h" > > +#include "hw/cpu/icc_bus.h" > > #ifndef CONFIG_USER_ONLY > > #include "hw/xen/xen.h" > > #include "hw/sysbus.h" > > @@ -1618,7 +1619,8 @@ static void cpu_x86_register(X86CPU *cpu, const c= har *name, Error **errp) > > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", er= rp); > > } > > =20 > > -X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) > > +X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, > > + Error **errp) > > { > > X86CPU *cpu =3D NULL; > > CPUX86State *env; > > @@ -1635,6 +1637,10 @@ X86CPU *cpu_x86_create(const char *cpu_model, Er= ror **errp) > > features =3D model_pieces[1]; > > =20 > > cpu =3D X86_CPU(object_new(TYPE_X86_CPU)); > > +#ifndef CONFIG_USER_ONLY > > + qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "i= cc")); > > + object_unref(OBJECT(cpu)); > > +#endif > > env =3D &cpu->env; > > env->cpu_model_str =3D cpu_model; > > =20 > > @@ -1659,7 +1665,7 @@ X86CPU *cpu_x86_init(const char *cpu_model) > > Error *error =3D NULL; > > X86CPU *cpu; > > =20 > > - cpu =3D cpu_x86_create(cpu_model, &error); > > + cpu =3D cpu_x86_create(cpu_model, NULL, &error); > > if (error) { > > goto out; > > } >=20 > This strikes me as unsafe: It relies on cpu_x86_init() / cpu_init() not > being used for softmmu. Could you please send a follow-up to squash > adding icc_bridge =3D=3D NULL error handling in the #ifdef? Intention was to make in unsafe, on target0i386 softmmu there is only one user pc_cpus_init(). But printing nice error instead of NULL deference would be nice, I'll resend it. >=20 > Andreas >=20 > > @@ -2346,6 +2352,7 @@ 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; > > =20 > > xcc->parent_reset =3D cc->reset; > > cc->reset =3D x86_cpu_reset; > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index ab151d5..f193752 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -897,7 +897,8 @@ typedef struct CPUX86State { > > #include "cpu-qom.h" > > =20 > > X86CPU *cpu_x86_init(const char *cpu_model); > > -X86CPU *cpu_x86_create(const char *cpu_model, Error **errp); > > +X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, > > + Error **errp); > > int cpu_x86_exec(CPUX86State *s); > > void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf); > > void x86_cpudef_setup(void); > >=20 >=20 >=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 --=20 Regards, Igor