[Qemu-devel] [PATCH 0/2] Reduce QOM boilerplate with sysbus_init_child() helper

[Qemu-devel] [PATCH 0/2] Reduce QOM boilerplate with sysbus_init_child() helper

[Peter Maydell]

I noticed writing a new container device that the "container
inits child objects in-place in its device struct" coding
style results in a lot of boilerplate in device init:

  object_initialize() to init the child
  object_property_add_child() to make the child a child of the parent
  qdev_set_parent_bus() to put the child on the sysbus default bus

If you forget the second of these then things sort of
work but trying to add a child to the child will segfault;
if you forget the third then the device won't get reset.

Patch 1 provides a simple helper function sysbus_init_child()
which does all these things for you, reducing the boilerplate
and making it harder to get wrong.

Code that used to look like this:
    object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
    object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL);
    qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
can now look like this:
    sysbus_init_child(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC);

Patch 2 is a demonstration of it being used in bcm2835_peripherals.c.
I scripted it with a quick Coccinelle script, so there are a
couple of places where it missed opportunities to use the new
function. If people like the function we can apply it more widely,
but I didn't want to go in and hand-tweak code until we have
consensus that it's useful, has parameters in the right order, etc.

thanks
-- PMM

Peter Maydell (2):
  hw/sysbus.h: New sysbus_init_child() helper function
  hw/arm/bcm2835_peripherals: Use sysbus_init_child()

 include/hw/sysbus.h          | 12 ++++++++++++
 hw/arm/bcm2835_peripherals.c | 36 ++++++++++++------------------------
 hw/core/sysbus.c             | 14 ++++++++++++++
 3 files changed, 38 insertions(+), 24 deletions(-)

-- 
2.16.1

[Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Peter Maydell]

If you're using the increasingly-common QOM style of
having container devices create their child objects
in-place, you end up with a lot of boilerplate in the
container's init function:

  object_initialize() to init the child
  object_property_add_child() to make the child a
    child of the parent
  qdev_set_parent_bus() to put the child on the
    sysbus default bus

If you forget the second of these then things sort of
work but trying to add a child to the child will segfault;
if you forget the third then the device won't get reset.

Provide a helper function sysbus_init_child() which
does all these things for you, reducing the boilerplate
and making it harder to get wrong.

Code that used to look like this:
    object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
    object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL);
    qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
can now look like this:
    sysbus_init_child(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC);

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/sysbus.h | 12 ++++++++++++
 hw/core/sysbus.c    | 14 ++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index e88bb6dae0..6095e24e86 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -118,4 +118,16 @@ static inline DeviceState *sysbus_try_create_simple(const char *name,
     return sysbus_try_create_varargs(name, addr, irq, NULL);
 }
 
+/**
+ * sysbus_init_child: in-place initialize and parent a SysBus device
+ * @parent: object to parent the device to
+ * @childname: name to use as the child property name
+ * @child: child object
+ * @childsize: size of the storage for the object
+ * @childtype: type name of the child
+ */
+void sysbus_init_child(Object *parent, const char *childname,
+                       void *child, size_t childsize,
+                       const char *childtype);
+
 #endif /* HW_SYSBUS_H */
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 5d0887f499..acfa52dc68 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -21,6 +21,7 @@
 #include "hw/sysbus.h"
 #include "monitor/monitor.h"
 #include "exec/address-spaces.h"
+#include "qapi/error.h"
 
 static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *sysbus_get_fw_dev_path(DeviceState *dev);
@@ -372,6 +373,19 @@ BusState *sysbus_get_default(void)
     return main_system_bus;
 }
 
+void sysbus_init_child(Object *parent, const char *childname,
+                       void *child, size_t childsize,
+                       const char *childtype)
+{
+    object_initialize(child, childsize, childtype);
+    /* error_abort is fine here because this can only fail for
+     * programming-error reasons: child already parented, or
+     * parent already has a child with the given name.
+     */
+    object_property_add_child(parent, childname, OBJECT(child), &error_abort);
+    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
+}
+
 static void sysbus_register_types(void)
 {
     type_register_static(&system_bus_info);
-- 
2.16.1

[Qemu-devel] [PATCH 2/2] hw/arm/bcm2835_peripherals: Use sysbus_init_child()

[Peter Maydell]

Use the new sysbus_init_child() function rather than
opencoding the init-and-parent operations.

This commit was created with coccinelle:
spatch --in-place -sp_file sysbus-init-child.spatch --keep-comments hw/arm/bcm2835_peripherals.c

and the following spatch file:
@@
expression CHILD, CHILDTYPE, POBJ, CHILDNAME, ERREXPR;
@@
- object_initialize(&CHILD, sizeof(CHILD), CHILDTYPE);
- object_property_add_child(POBJ, CHILDNAME, OBJECT(&CHILD), ERREXPR);
- qdev_set_parent_bus(DEVICE(&CHILD), sysbus_get_default());
+ sysbus_init_child(POBJ, CHILDNAME, &CHILD, sizeof(CHILD), CHILDTYPE);

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/bcm2835_peripherals.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 13b63970d7..7968d611ef 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -41,9 +41,7 @@ static void bcm2835_peripherals_init(Object *obj)
                        MBOX_CHAN_COUNT << MBOX_AS_CHAN_SHIFT);
 
     /* Interrupt Controller */
-    object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
-    object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL);
-    qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
+    sysbus_init_child(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
 
     /* UART0 */
     s->uart0 = SYS_BUS_DEVICE(object_new("pl011"));
@@ -51,14 +49,11 @@ static void bcm2835_peripherals_init(Object *obj)
     qdev_set_parent_bus(DEVICE(s->uart0), sysbus_get_default());
 
     /* AUX / UART1 */
-    object_initialize(&s->aux, sizeof(s->aux), TYPE_BCM2835_AUX);
-    object_property_add_child(obj, "aux", OBJECT(&s->aux), NULL);
-    qdev_set_parent_bus(DEVICE(&s->aux), sysbus_get_default());
+    sysbus_init_child(obj, "aux", &s->aux, sizeof(s->aux), TYPE_BCM2835_AUX);
 
     /* Mailboxes */
-    object_initialize(&s->mboxes, sizeof(s->mboxes), TYPE_BCM2835_MBOX);
-    object_property_add_child(obj, "mbox", OBJECT(&s->mboxes), NULL);
-    qdev_set_parent_bus(DEVICE(&s->mboxes), sysbus_get_default());
+    sysbus_init_child(obj, "mbox", &s->mboxes, sizeof(s->mboxes),
+                      TYPE_BCM2835_MBOX);
 
     object_property_add_const_link(OBJECT(&s->mboxes), "mbox-mr",
                                    OBJECT(&s->mbox_mr), &error_abort);
@@ -86,32 +81,25 @@ static void bcm2835_peripherals_init(Object *obj)
                                    OBJECT(&s->gpu_bus_mr), &error_abort);
 
     /* Random Number Generator */
-    object_initialize(&s->rng, sizeof(s->rng), TYPE_BCM2835_RNG);
-    object_property_add_child(obj, "rng", OBJECT(&s->rng), NULL);
-    qdev_set_parent_bus(DEVICE(&s->rng), sysbus_get_default());
+    sysbus_init_child(obj, "rng", &s->rng, sizeof(s->rng), TYPE_BCM2835_RNG);
 
     /* Extended Mass Media Controller */
-    object_initialize(&s->sdhci, sizeof(s->sdhci), TYPE_SYSBUS_SDHCI);
-    object_property_add_child(obj, "sdhci", OBJECT(&s->sdhci), NULL);
-    qdev_set_parent_bus(DEVICE(&s->sdhci), sysbus_get_default());
+    sysbus_init_child(obj, "sdhci", &s->sdhci, sizeof(s->sdhci),
+                      TYPE_SYSBUS_SDHCI);
 
     /* SDHOST */
-    object_initialize(&s->sdhost, sizeof(s->sdhost), TYPE_BCM2835_SDHOST);
-    object_property_add_child(obj, "sdhost", OBJECT(&s->sdhost), NULL);
-    qdev_set_parent_bus(DEVICE(&s->sdhost), sysbus_get_default());
+    sysbus_init_child(obj, "sdhost", &s->sdhost, sizeof(s->sdhost),
+                      TYPE_BCM2835_SDHOST);
 
     /* DMA Channels */
-    object_initialize(&s->dma, sizeof(s->dma), TYPE_BCM2835_DMA);
-    object_property_add_child(obj, "dma", OBJECT(&s->dma), NULL);
-    qdev_set_parent_bus(DEVICE(&s->dma), sysbus_get_default());
+    sysbus_init_child(obj, "dma", &s->dma, sizeof(s->dma), TYPE_BCM2835_DMA);
 
     object_property_add_const_link(OBJECT(&s->dma), "dma-mr",
                                    OBJECT(&s->gpu_bus_mr), &error_abort);
 
     /* GPIO */
-    object_initialize(&s->gpio, sizeof(s->gpio), TYPE_BCM2835_GPIO);
-    object_property_add_child(obj, "gpio", OBJECT(&s->gpio), NULL);
-    qdev_set_parent_bus(DEVICE(&s->gpio), sysbus_get_default());
+    sysbus_init_child(obj, "gpio", &s->gpio, sizeof(s->gpio),
+                      TYPE_BCM2835_GPIO);
 
     object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhci",
                                    OBJECT(&s->sdhci.sdbus), &error_abort);
-- 
2.16.1

Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Igor Mammedov]

On Fri, 16 Feb 2018 13:45:15 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> If you're using the increasingly-common QOM style of
> having container devices create their child objects
> in-place, you end up with a lot of boilerplate in the
> container's init function:
> 
>   object_initialize() to init the child
>   object_property_add_child() to make the child a
>     child of the parent
>   qdev_set_parent_bus() to put the child on the
>     sysbus default bus
> 
> If you forget the second of these then things sort of
> work but trying to add a child to the child will segfault;
> if you forget the third then the device won't get reset.
> 
> Provide a helper function sysbus_init_child() which
> does all these things for you, reducing the boilerplate
> and making it harder to get wrong.
> 
> Code that used to look like this:
>     object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
>     object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL);
>     qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
> can now look like this:
>     sysbus_init_child(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/sysbus.h | 12 ++++++++++++
>  hw/core/sysbus.c    | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index e88bb6dae0..6095e24e86 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -118,4 +118,16 @@ static inline DeviceState *sysbus_try_create_simple(const char *name,
>      return sysbus_try_create_varargs(name, addr, irq, NULL);
>  }
>  
> +/**
> + * sysbus_init_child: in-place initialize and parent a SysBus device
> + * @parent: object to parent the device to
> + * @childname: name to use as the child property name
> + * @child: child object
> + * @childsize: size of the storage for the object
> + * @childtype: type name of the child
> + */
> +void sysbus_init_child(Object *parent, const char *childname,
> +                       void *child, size_t childsize,
> +                       const char *childtype);
> +
>  #endif /* HW_SYSBUS_H */
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 5d0887f499..acfa52dc68 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -21,6 +21,7 @@
>  #include "hw/sysbus.h"
>  #include "monitor/monitor.h"
>  #include "exec/address-spaces.h"
> +#include "qapi/error.h"
>  
>  static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>  static char *sysbus_get_fw_dev_path(DeviceState *dev);
> @@ -372,6 +373,19 @@ BusState *sysbus_get_default(void)
>      return main_system_bus;
>  }
>  
> +void sysbus_init_child(Object *parent, const char *childname,
> +                       void *child, size_t childsize,
> +                       const char *childtype)
> +{


> +    object_initialize(child, childsize, childtype);
> +    /* error_abort is fine here because this can only fail for
> +     * programming-error reasons: child already parented, or
> +     * parent already has a child with the given name.
> +     */
> +    object_property_add_child(parent, childname, OBJECT(child), &error_abort);
It would be useful not only for sysbus devices.
maybe we should extend object_initialize instead,
something like this:
   void object_initialize(void *data, size_t size, const char *typename,
                          Object *parent, const char *name)
and set parent in it.
git counts about 152 uses, so it would be tree wide change
but still not too much.


> +    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
and then assuming we don't create sysbus devices, nor should be able to,
which are not attached to sysbus_get_default() this one can go sysbus' instance_init()

then there won't be need for sysbus specific helper,
inheritance will do the rest of the job.

> +}
> +
>  static void sysbus_register_types(void)
>  {
>      type_register_static(&system_bus_info);

Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Philippe Mathieu-Daudé]

On 02/16/2018 01:28 PM, Igor Mammedov wrote:
> On Fri, 16 Feb 2018 13:45:15 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
...
>>  static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>>  static char *sysbus_get_fw_dev_path(DeviceState *dev);
>> @@ -372,6 +373,19 @@ BusState *sysbus_get_default(void)
>>      return main_system_bus;
>>  }
>>  
>> +void sysbus_init_child(Object *parent, const char *childname,
>> +                       void *child, size_t childsize,
>> +                       const char *childtype)
>> +{
> 
> 
>> +    object_initialize(child, childsize, childtype);
>> +    /* error_abort is fine here because this can only fail for
>> +     * programming-error reasons: child already parented, or
>> +     * parent already has a child with the given name.
>> +     */
>> +    object_property_add_child(parent, childname, OBJECT(child), &error_abort);
> It would be useful not only for sysbus devices.
> maybe we should extend object_initialize instead,
> something like this:
>    void object_initialize(void *data, size_t size, const char *typename,
>                           Object *parent, const char *name)
> and set parent in it.
> git counts about 152 uses, so it would be tree wide change
> but still not too much.

we can keep object_initialize() when no parent,
and add object_initialize_child(, const char *childname, Object *parent)
'parent' last because all previous args are child-related.

> 
> 
>> +    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
> and then assuming we don't create sysbus devices, nor should be able to,
> which are not attached to sysbus_get_default() this one can go sysbus' instance_init()

good idea.

> 
> then there won't be need for sysbus specific helper,
> inheritance will do the rest of the job.
> 
>> +}
>> +

Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Paolo Bonzini]

On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote:
> we can keep object_initialize() when no parent,
> and add object_initialize_child(, const char *childname, Object *parent)
> 'parent' last because all previous args are child-related.
> 
>>
>>> +    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
>> and then assuming we don't create sysbus devices, nor should be able to,
>> which are not attached to sysbus_get_default() this one can go sysbus' instance_init()
> good idea.
> 

Either last or first, like

	object_initialize_child(Object *parent, const char *childname,
				void *data, size_t size,
				const char *typename);

I'm not sure about moving qdev_set_parent_bus to instance_init().  It
would be a bit different from other buses and possibly confusing.

Potentially there could be a "hierarchy" of *_initialize_child
functions, adding or removing arguments as needed for the specific kind
of bus:

	/* adds qdev_set_parent_bus */
	device_initialize_child(Object *parent, const char *childname,
				BusState *bus, void *data, size_t size,
				const char *typename);
	/* uses sysbus_get_default() */
	sysbus_initialize_child(Object *parent, const char *childname,
				void *data, size_t size,
				const char *typename);
	/* initializes "addr" property too */
	pci_initialize_child(Object *parent, const char *childname,
			     BusState *bus, int addr,
			     void *data, size_t size,
			     const char *typename);

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Igor Mammedov]

On Tue, 20 Feb 2018 13:13:49 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote:
> > we can keep object_initialize() when no parent,
> > and add object_initialize_child(, const char *childname, Object *parent)
> > 'parent' last because all previous args are child-related.
> >   
> >>  
> >>> +    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());  
> >> and then assuming we don't create sysbus devices, nor should be able to,
> >> which are not attached to sysbus_get_default() this one can go sysbus' instance_init()  
> > good idea.
> >   
[...]

> I'm not sure about moving qdev_set_parent_bus to instance_init().  It
> would be a bit different from other buses and possibly confusing.
That's what this series sort of does, i.e. creating a type/class
specific helper(s). Which becomes confusing as number of helpers
increases (frankly it's just 2 different approaches to code i.e.
functional vs OOM).

It could be better to keep single QOM API and let SYSBUS base class
instance_init() to do all implicit initialization that must be done
for inherited classes.
Yes, it will be different from other devices with buses but
users won't really care (there is no other buss to assign to),
for them it will look like a typical bus-less device and it
would be less error-prone to code.

 
> Potentially there could be a "hierarchy" of *_initialize_child
> functions, adding or removing arguments as needed for the specific kind
> of bus:
> 
> 	/* adds qdev_set_parent_bus */
> 	device_initialize_child(Object *parent, const char *childname,
> 				BusState *bus, void *data, size_t size,
> 				const char *typename);
> 	/* uses sysbus_get_default() */
> 	sysbus_initialize_child(Object *parent, const char *childname,
> 				void *data, size_t size,
> 				const char *typename);


> 	/* initializes "addr" property too */
> 	pci_initialize_child(Object *parent, const char *childname,
> 			     BusState *bus, int addr,
> 			     void *data, size_t size,
> 			     const char *typename);
PCI could also incorporate PCI specific bus setting/
verification logic inside of base class.

It could allow us to drop bus assigning magic in qdev_device_add(),
bringing it closer to simple QOM object handling, with specifics
abstracted by respective TYPE implementations.
Maybe we would be able to unify -device with -object in the end.


> Thanks,
> 
> Paolo
> qdev_device_add

Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Philippe Mathieu-Daudé]

Hi,

On 02/20/2018 10:23 AM, Igor Mammedov wrote:
> On Tue, 20 Feb 2018 13:13:49 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote:
>>> we can keep object_initialize() when no parent,
>>> and add object_initialize_child(, const char *childname, Object *parent)
>>> 'parent' last because all previous args are child-related.
>>>   
>>>>  
>>>>> +    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());  
>>>> and then assuming we don't create sysbus devices, nor should be able to,
>>>> which are not attached to sysbus_get_default() this one can go sysbus' instance_init()  
>>> good idea.
>>>   
> [...]
> 
>> I'm not sure about moving qdev_set_parent_bus to instance_init().  It
>> would be a bit different from other buses and possibly confusing.
> That's what this series sort of does, i.e. creating a type/class
> specific helper(s). Which becomes confusing as number of helpers
> increases (frankly it's just 2 different approaches to code i.e.
> functional vs OOM).
> 
> It could be better to keep single QOM API and let SYSBUS base class
> instance_init() to do all implicit initialization that must be done
> for inherited classes.

What is the consensus on this series once 2.12 gets released?

> Yes, it will be different from other devices with buses but
> users won't really care (there is no other buss to assign to),
> for them it will look like a typical bus-less device and it
> would be less error-prone to code.
> 
>  
>> Potentially there could be a "hierarchy" of *_initialize_child
>> functions, adding or removing arguments as needed for the specific kind
>> of bus:
>>
>> 	/* adds qdev_set_parent_bus */
>> 	device_initialize_child(Object *parent, const char *childname,
>> 				BusState *bus, void *data, size_t size,
>> 				const char *typename);
>> 	/* uses sysbus_get_default() */
>> 	sysbus_initialize_child(Object *parent, const char *childname,
>> 				void *data, size_t size,
>> 				const char *typename);
> 
> 
>> 	/* initializes "addr" property too */
>> 	pci_initialize_child(Object *parent, const char *childname,
>> 			     BusState *bus, int addr,
>> 			     void *data, size_t size,
>> 			     const char *typename);
> PCI could also incorporate PCI specific bus setting/
> verification logic inside of base class.
> 
> It could allow us to drop bus assigning magic in qdev_device_add(),
> bringing it closer to simple QOM object handling, with specifics
> abstracted by respective TYPE implementations.
> Maybe we would be able to unify -device with -object in the end.

Thanks,

Phil.

Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Igor Mammedov]

On Sat, 24 Mar 2018 12:35:22 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Hi,
> 
> On 02/20/2018 10:23 AM, Igor Mammedov wrote:
> > On Tue, 20 Feb 2018 13:13:49 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >   
> >> On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote:  
> >>> we can keep object_initialize() when no parent,
> >>> and add object_initialize_child(, const char *childname, Object *parent)
> >>> 'parent' last because all previous args are child-related.
> >>>     
> >>>>    
> >>>>> +    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());    
> >>>> and then assuming we don't create sysbus devices, nor should be able to,
> >>>> which are not attached to sysbus_get_default() this one can go sysbus' instance_init()    
> >>> good idea.
> >>>     
> > [...]
> >   
> >> I'm not sure about moving qdev_set_parent_bus to instance_init().  It
> >> would be a bit different from other buses and possibly confusing.  
> > That's what this series sort of does, i.e. creating a type/class
> > specific helper(s). Which becomes confusing as number of helpers
> > increases (frankly it's just 2 different approaches to code i.e.
> > functional vs OOM).
> > 
> > It could be better to keep single QOM API and let SYSBUS base class
> > instance_init() to do all implicit initialization that must be done
> > for inherited classes.  
> 
> What is the consensus on this series once 2.12 gets released?
At least we should add and make current code use it
  object_initialize_child()
It should save quite a bit of code in ARM target

As for
  qdev_set_parent_bus(DEVICE, sysbus_get_default())
it is a separate issue, but I'd still go with
TYPE_SYS_BUS_DEVICE setting bus in its instance_init so that
inherited types nor their users would have to deal with it.

> > Yes, it will be different from other devices with buses but
> > users won't really care (there is no other buss to assign to),
> > for them it will look like a typical bus-less device and it
> > would be less error-prone to code.
> > 
> >    
> >> Potentially there could be a "hierarchy" of *_initialize_child
> >> functions, adding or removing arguments as needed for the specific kind
> >> of bus:
> >>
> >> 	/* adds qdev_set_parent_bus */
> >> 	device_initialize_child(Object *parent, const char *childname,
> >> 				BusState *bus, void *data, size_t size,
> >> 				const char *typename);
> >> 	/* uses sysbus_get_default() */
> >> 	sysbus_initialize_child(Object *parent, const char *childname,
> >> 				void *data, size_t size,
> >> 				const char *typename);  
> > 
> >   
> >> 	/* initializes "addr" property too */
> >> 	pci_initialize_child(Object *parent, const char *childname,
> >> 			     BusState *bus, int addr,
> >> 			     void *data, size_t size,
> >> 			     const char *typename);  
> > PCI could also incorporate PCI specific bus setting/
> > verification logic inside of base class.
> > 
> > It could allow us to drop bus assigning magic in qdev_device_add(),
> > bringing it closer to simple QOM object handling, with specifics
> > abstracted by respective TYPE implementations.
> > Maybe we would be able to unify -device with -object in the end.  
> 
> Thanks,
> 
> Phil.