[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 8/8] qom: Make CPU a child of DeviceState
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 8/8] qom: Make CPU a child of DeviceState |
Date: |
Wed, 2 Jan 2013 14:49:29 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden> 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 <address@hidden>
> > > ---
> > > 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