Re: [Qemu-devel] [PATCH 8/8] qom: Make CPU a child of DeviceState

["Andreas Färber" <afaerber@suse.de>]

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: Eduardo Habkost <ehabkost@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)?

>  
>  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.

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg