On Wed, Jan 02, 2013 at 05:40:58PM +0100, Igor Mammedov wrote: > On Wed, 02 Jan 2013 16:08:42 +0100 > Andreas Färber <afaer...@suse.de> wrote: > > > 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. > > > > > > > > It has no_user=1, as creating CPUs using -device doesn't work yet. > > > > > > > > (based on a previous patch from Igor Mammedov) > > > > Can this comment be turned into or amended by the usual Signed-off-by? > Signed-off-by should be ok.
OK to me, as well. Should I resubmit, or can Andreas edit it when committing the patch? > > > > > > > > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> > > > --- > > > Changes v1 (imammedo) -> v2 (ehabkost): > > > - Change CPU type declaration to hae TYPE_DEVICE as parent > > > > > > Changes v2 -> v3 (ehabkost): > > > - Set no_user=1 on the CPU class > > > --- > > > include/qemu/cpu.h | 6 +++--- > > > qom/cpu.c | 5 ++++- > > > 2 files changed, 7 insertions(+), 4 deletions(-) > > > > > > 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 > > > > > > -#include "qemu/object.h" > > > +#include "hw/qdev-core.h" > > > #include "qemu-thread.h" > > > > > > /** > > [...] > > > 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 @@ > > > > > > #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. > > > > > > > > void cpu_reset(CPUState *cpu) > > > { > > > @@ -36,14 +37,16 @@ static void cpu_common_reset(CPUState *cpu) > > > > > > static void cpu_class_init(ObjectClass *klass, void *data) > > > { > > > + DeviceClass *dc = DEVICE_CLASS(klass); > > > CPUClass *k = CPU_CLASS(klass); > > > > > > k->reset = cpu_common_reset; > > > + dc->no_user = 1; > > > } > > > > I wonder if we should add a comment that we are intentionally not > > hooking up dc->reset (yet)? > not relevant to this patch, could be separate patch though. > > > > > > > > > static TypeInfo cpu_type_info = { > > > > Would like to add the missing const while touching this. > > > > > .name = TYPE_CPU, > > > - .parent = TYPE_OBJECT, > > > + .parent = TYPE_DEVICE, > > > .instance_size = sizeof(CPUState), > > > .abstract = true, > > > .class_size = sizeof(CPUClass), > > > > My testing so far confirms that the combination of object_new() without > > qdev_init[_nofail]() is working fine. > +1, I tested this combo for (x86)-(softmmu|linux-user) targets, no issues were > found so far. > > > > > 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... > +1 > > > > > Regards, > > Andreas > > > -- Eduardo