From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40927) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TqPw3-0006Zw-2l for qemu-devel@nongnu.org; Wed, 02 Jan 2013 10:08:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TqPvz-0003Gv-HN for qemu-devel@nongnu.org; Wed, 02 Jan 2013 10:08:51 -0500 Received: from cantor2.suse.de ([195.135.220.15]:54652 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TqPvz-0003Gr-84 for qemu-devel@nongnu.org; Wed, 02 Jan 2013 10:08:47 -0500 Message-ID: <50E44D7A.8060305@suse.de> Date: Wed, 02 Jan 2013 16:08:42 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1354726153-30264-1-git-send-email-ehabkost@redhat.com> <1354726153-30264-9-git-send-email-ehabkost@redhat.com> In-Reply-To: <1354726153-30264-9-git-send-email-ehabkost@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 8/8] qom: Make CPU a child of DeviceState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Igor Mammedov , Don Slutz , qemu-devel@nongnu.org, Anthony Liguori , Paolo Bonzini Am 05.12.2012 17:49, schrieb Eduardo Habkost: > This finally makes the CPU class a child of DeviceState, allowing us to > start using DeviceState properties on CPU subclasses. To avoid confusion with child<> properties and DeviceState vs. DeviceClass I have reworded this to "subclass of Device" in my qom-cpu-dev queue. >=20 > It has no_user=3D1, as creating CPUs using -device doesn't work yet. >=20 > (based on a previous patch from Igor Mammedov) Can this comment be turned into or amended by the usual Signed-off-by? >=20 > Signed-off-by: Eduardo Habkost > --- > Changes v1 (imammedo) -> v2 (ehabkost): > - Change CPU type declaration to hae TYPE_DEVICE as parent >=20 > Changes v2 -> v3 (ehabkost): > - Set no_user=3D1 on the CPU class > --- > include/qemu/cpu.h | 6 +++--- > qom/cpu.c | 5 ++++- > 2 files changed, 7 insertions(+), 4 deletions(-) >=20 > diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h > index 61b7698..bc004fd 100644 > --- a/include/qemu/cpu.h > +++ b/include/qemu/cpu.h > @@ -20,7 +20,7 @@ > #ifndef QEMU_CPU_H > #define QEMU_CPU_H > =20 > -#include "qemu/object.h" > +#include "hw/qdev-core.h" > #include "qemu-thread.h" > =20 > /** [...] > diff --git a/qom/cpu.c b/qom/cpu.c > index 5b36046..d301f72 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -20,6 +20,7 @@ > =20 > #include "qemu/cpu.h" > #include "qemu-common.h" > +#include "hw/qdev-core.h" Already included via qom/cpu.h (formerly qemu/cpu.h) above, dropping. > =20 > void cpu_reset(CPUState *cpu) > { > @@ -36,14 +37,16 @@ static void cpu_common_reset(CPUState *cpu) > =20 > static void cpu_class_init(ObjectClass *klass, void *data) > { > + DeviceClass *dc =3D DEVICE_CLASS(klass); > CPUClass *k =3D CPU_CLASS(klass); > =20 > k->reset =3D cpu_common_reset; > + dc->no_user =3D 1; > } I wonder if we should add a comment that we are intentionally not hooking up dc->reset (yet)? > =20 > static TypeInfo cpu_type_info =3D { Would like to add the missing const while touching this. > .name =3D TYPE_CPU, > - .parent =3D TYPE_OBJECT, > + .parent =3D TYPE_DEVICE, > .instance_size =3D sizeof(CPUState), > .abstract =3D true, > .class_size =3D sizeof(CPUClass), My testing so far confirms that the combination of object_new() without qdev_init[_nofail]() is working fine. Using qdev_create() in the current state of stubs would lead to a silly if-bus-is-NULL-set-it-to-NULL sequence on top of object_new(). I do not expect qdev_create() to grow in functionality, so continuing to use object_new() should be okay - SoCs like my Tegra model may want to use object_initialize() so we cannot prescribe using qdev_create() anyway. qdev_init_nofail() would call the qdev initfn (to be replaced by realizefn, not used for CPU in this patch), then if no parent add it to /machine/unassigned, register VMSD if not NULL, update the internal state (blocking static property changes) and if hotplugged reset (unused due to dc->no_user and lack of dc->reset). The /machine/unassigned part may be interesting, e.g., for APIC modelling (so that we can model the former ptr property / now pointer-setting as a link<> property). With these considerations I am leaning towards accepting this patch if nobody objects, so that we can move on to the next refactorings... Regards, Andreas --=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