From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60501) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVSsQ-00039h-LM for qemu-devel@nongnu.org; Tue, 10 Mar 2015 18:43:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVSsK-0007uk-Dl for qemu-devel@nongnu.org; Tue, 10 Mar 2015 18:43:50 -0400 Received: from cantor2.suse.de ([195.135.220.15]:37378 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVSsK-0007ud-7o for qemu-devel@nongnu.org; Tue, 10 Mar 2015 18:43:44 -0400 Message-ID: <54FF739D.9030102@suse.de> Date: Tue, 10 Mar 2015 23:43:41 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1426024655-17561-1-git-send-email-ehabkost@redhat.com> <1426024655-17561-2-git-send-email-ehabkost@redhat.com> In-Reply-To: <1426024655-17561-2-git-send-email-ehabkost@redhat.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost , qemu-devel@nongnu.org Cc: zhugh.fnst@cn.fujitsu.com, "Michael S. Tsirkin" , tangchen@cn.fujitsu.com, chen.fan.fnst@cn.fujitsu.com, isimatu.yasuaki@jp.fujitsu.com, Paolo Bonzini , Gu Zheng , Igor Mammedov , anshul.makkar@profitbricks.com Am 10.03.2015 um 22:57 schrieb Eduardo Habkost: > Instead of passing icc_bridge from the PC initialization code to > cpu_x86_create(), make the PC initialization code attach the CPU to > icc_bridge. >=20 > The only difference here is that icc_bridge attachment will now be done > after x86_cpu_parse_featurestr() is called. But this shouldn't make any > difference, as property setters shouldn't depend on icc_bridge. >=20 > Signed-off-by: Eduardo Habkost > --- > Changes v1 -> v2: > * Keep existing check for NULL icc_bridge and error reporting, instead > of assing assert(icc_bridge) > --- > hw/i386/pc.c | 13 +++++++++++-- > target-i386/cpu.c | 14 ++------------ > target-i386/cpu.h | 3 +-- > 3 files changed, 14 insertions(+), 16 deletions(-) >=20 > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index b5b2aad..a26e0ec 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -992,18 +992,27 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,= int level) > static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > DeviceState *icc_bridge, Error **errp) > { > - X86CPU *cpu; > + X86CPU *cpu =3D NULL; > Error *local_err =3D NULL; > =20 > - cpu =3D cpu_x86_create(cpu_model, icc_bridge, &local_err); > + 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); We had previously discussed reference counting. Here I would expect: OBJECT(cpu)->ref =3D=3D 1 > if (local_err !=3D NULL) { > error_propagate(errp, local_err); > return NULL; > } > =20 > + qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "i= cc")); OBJECT(cpu)->ref =3D=3D 2 > + object_unref(OBJECT(cpu)); OBJECT(cpu)->ref =3D=3D 1 > + > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_er= r); > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err= ); OBJECT(cpu)->ref =3D=3D 1 or 2 depending on DeviceClass::realize :) > =20 > +out: > if (local_err) { > error_propagate(errp, local_err); > object_unref(OBJECT(cpu)); object_unref(NULL) looks unusual but is valid. Should we change the return NULL to jump here, too, then? OBJECT(cpu)->ref =3D=3D 0 or 1 I wonder whether we need another object_unref(OBJECT(cpu)) for the non-error case, either here or in the callers? Out of scope for this patch, of course. Regards, Andreas [snip] --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Felix Imend=F6rffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton; HRB 21284 (AG N=FCrnberg)