[Qemu-devel] [PATCH v2 00/16] Fix crashes with introspection of ARM devices

[Qemu-devel] [PATCH v2 00/16] Fix crashes with introspection of ARM devices

[Thomas Huth]

As discovered recently, you can crash QEMU with a lot of devices
that do not get the reference counting of child objects right.
You just have to run 'device-list-properties' and call 'info qtree'
afterwards.
This patch series fixes a bunch of these problems in the ARM code.
I did not fix all problems yet, since it is quite time consuming,
and this series is big enough already. The remaining problems can
be fixed in an independent patch series later.

v2:
 - Updated the first patch according to the review feedback from v1
 - Added more patches with additional fixes

Thomas Huth (16):
  qom/object: Add a new function object_initialize_child()
  hw/core/sysbus: Add a function for creating and attaching an object
  hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported
    machines
  hw/arm/armv7: Fix crash when introspecting the "iotkit" device
  hw/cpu/a15mpcore: Fix introspection problem with the a15mpcore_priv
    device
  hw/display/xlnx_dp: Move problematic code from instance_init to
    realize
  hw/arm/xlnx-zynqmp: Fix crash when introspecting the "xlnx,zynqmp"
    device
  hw/arm/msf2-soc: Fix introspection problem with the "msf2-soc" device
  hw/cpu/a9mpcore: Fix introspection problems with the "a9mpcore_priv"
    device
  hw/arm/fsl-imx6: Fix introspection problems with the "fsl,imx6" device
  hw/arm/fsl-imx7: Fix introspection problems with the "fsl,imx7" device
  hw/arm/fsl-imx25: Fix introspection problem with the "fsl,imx25"
    device
  hw/arm/fsl-imx31: Fix introspection problem with the "fsl,imx31"
    device
  hw/cpu/arm11mpcore: Fix introspection problem with 'arm11mpcore_priv'
  hw/*/realview: Fix introspection problem with 'realview_mpcore' &
    'realview_gic'
  hw/arm/allwinner-a10: Fix introspection problem with 'allwinner-a10'

 hw/arm/allwinner-a10.c   | 19 +++++-----
 hw/arm/armv7m.c          |  7 ++--
 hw/arm/bcm2836.c         | 18 +++------
 hw/arm/fsl-imx25.c       | 30 +++++++--------
 hw/arm/fsl-imx31.c       | 26 ++++++-------
 hw/arm/fsl-imx6.c        | 56 ++++++++++------------------
 hw/arm/fsl-imx7.c        | 96 ++++++++++++++++--------------------------------
 hw/arm/iotkit.c          | 74 ++++++++++++++++---------------------
 hw/arm/msf2-soc.c        | 15 ++++----
 hw/arm/xlnx-zynqmp.c     | 61 ++++++++++++++----------------
 hw/core/sysbus.c         |  8 ++++
 hw/cpu/a15mpcore.c       |  8 ++--
 hw/cpu/a9mpcore.c        | 18 ++++-----
 hw/cpu/arm11mpcore.c     | 14 +++----
 hw/cpu/realview_mpcore.c |  8 ++--
 hw/display/xlnx_dp.c     | 23 +++++-------
 hw/intc/armv7m_nvic.c    |  5 +--
 hw/intc/realview_gic.c   |  7 +---
 include/hw/sysbus.h      |  3 ++
 include/qom/object.h     | 23 +++++++++++-
 qom/object.c             | 15 ++++++++
 21 files changed, 244 insertions(+), 290 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Thomas Huth]

A lot of code is using the object_initialize() function followed by a call
to object_property_add_child() to add the newly initialized object as a child
of the current object. Both functions increase the reference counter of the
new object, but many spots that call these two functions then forget to drop
one of the superfluous references. So the newly created object is often not
cleaned up correctly when the parent is destroyed. In the worst case, this
can cause crashes, e.g. because device objects are not correctly removed from
their parent_bus.

Since this is a common pattern between many code spots, let's introdcue a
new function that takes care of calling all three required initialization
functions, first object_initialize(), then object_property_add_child() and
finally object_unref().

And while we're at object.h, also fix some copy-n-paste errors in the
comments there ("to store the area" --> "to store the error").

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/qom/object.h | 23 +++++++++++++++++++++--
 qom/object.c         | 15 +++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index f3d2308..3362db0 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
 void object_initialize(void *obj, size_t size, const char *typename);
 
 /**
+ * object_initialize_child:
+ * @parentobj: The parent object to add a property to
+ * @propname: The name of the property
+ * @childobj: A pointer to the memory to be used for the object.
+ * @size: The maximum size available at @obj for the object.
+ * @type: The name of the type of the object to instantiate.
+ * @errp: If an error occurs, a pointer to an area to store the error
+ *
+ * This function will initialize an object. The memory for the object should
+ * have already been allocated. The object will then be added as child property
+ * to a parent with object_property_add_child() function. The returned object
+ * has a reference count of 1 (for the "child<...>" property from the parent),
+ * so the object will get finalized automatically when the parent gets removed.
+ */
+void object_initialize_child(Object *parentobj, const char *propname,
+                             void *childobj, size_t size, const char *type,
+                             Error **errp);
+
+/**
  * object_dynamic_cast:
  * @obj: The object to cast.
  * @typename: The @typename to cast to.
@@ -1382,7 +1401,7 @@ Object *object_resolve_path_component(Object *parent, const gchar *part);
  * @obj: the object to add a property to
  * @name: the name of the property
  * @child: the child object
- * @errp: if an error occurs, a pointer to an area to store the area
+ * @errp: if an error occurs, a pointer to an area to store the error
  *
  * Child properties form the composition tree.  All objects need to be a child
  * of another object.  Objects can only be a child of one object.
@@ -1420,7 +1439,7 @@ void object_property_allow_set_link(const Object *, const char *,
  * @child: a pointer to where the link object reference is stored
  * @check: callback to veto setting or NULL if the property is read-only
  * @flags: additional options for the link
- * @errp: if an error occurs, a pointer to an area to store the area
+ * @errp: if an error occurs, a pointer to an area to store the error
  *
  * Links establish relationships between objects.  Links are unidirectional
  * although two links can be combined to form a bidirectional relationship
diff --git a/qom/object.c b/qom/object.c
index 4609e34..7be7638 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -392,6 +392,21 @@ void object_initialize(void *data, size_t size, const char *typename)
     object_initialize_with_type(data, size, type);
 }
 
+void object_initialize_child(Object *parentobj, const char *propname,
+                             void *childobj, size_t size, const char *type,
+                             Error **errp)
+{
+    object_initialize(childobj, size, type);
+    object_property_add_child(parentobj, propname, OBJECT(childobj), errp);
+    /*
+     * Since object_property_add_child added a reference to the child object,
+     * we can drop the reference added by object_initialize(), so the child
+     * property will own the only reference to the object.
+     */
+    object_unref(OBJECT(childobj));
+}
+
+
 static inline bool object_property_is_child(ObjectProperty *prop)
 {
     return strstart(prop->type, "child<", NULL);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Paolo Bonzini]

On 13/07/2018 10:27, Thomas Huth wrote:
> A lot of code is using the object_initialize() function followed by a call
> to object_property_add_child() to add the newly initialized object as a child
> of the current object. Both functions increase the reference counter of the
> new object, but many spots that call these two functions then forget to drop
> one of the superfluous references. So the newly created object is often not
> cleaned up correctly when the parent is destroyed. In the worst case, this
> can cause crashes, e.g. because device objects are not correctly removed from
> their parent_bus.
> 
> Since this is a common pattern between many code spots, let's introdcue a
> new function that takes care of calling all three required initialization
> functions, first object_initialize(), then object_property_add_child() and
> finally object_unref().
> 
> And while we're at object.h, also fix some copy-n-paste errors in the
> comments there ("to store the area" --> "to store the error").

Even though I'd prefer the full cleanup, I can live with this. :)

Series

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

except for patch 6 for which I've sent a replacement.

Paolo

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/qom/object.h | 23 +++++++++++++++++++++--
>  qom/object.c         | 15 +++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3d2308..3362db0 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>  void object_initialize(void *obj, size_t size, const char *typename);
>  
>  /**
> + * object_initialize_child:
> + * @parentobj: The parent object to add a property to
> + * @propname: The name of the property
> + * @childobj: A pointer to the memory to be used for the object.
> + * @size: The maximum size available at @obj for the object.
> + * @type: The name of the type of the object to instantiate.
> + * @errp: If an error occurs, a pointer to an area to store the error
> + *
> + * This function will initialize an object. The memory for the object should
> + * have already been allocated. The object will then be added as child property
> + * to a parent with object_property_add_child() function. The returned object
> + * has a reference count of 1 (for the "child<...>" property from the parent),
> + * so the object will get finalized automatically when the parent gets removed.
> + */
> +void object_initialize_child(Object *parentobj, const char *propname,
> +                             void *childobj, size_t size, const char *type,
> +                             Error **errp);
> +
> +/**
>   * object_dynamic_cast:
>   * @obj: The object to cast.
>   * @typename: The @typename to cast to.
> @@ -1382,7 +1401,7 @@ Object *object_resolve_path_component(Object *parent, const gchar *part);
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @child: the child object
> - * @errp: if an error occurs, a pointer to an area to store the area
> + * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Child properties form the composition tree.  All objects need to be a child
>   * of another object.  Objects can only be a child of one object.
> @@ -1420,7 +1439,7 @@ void object_property_allow_set_link(const Object *, const char *,
>   * @child: a pointer to where the link object reference is stored
>   * @check: callback to veto setting or NULL if the property is read-only
>   * @flags: additional options for the link
> - * @errp: if an error occurs, a pointer to an area to store the area
> + * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Links establish relationships between objects.  Links are unidirectional
>   * although two links can be combined to form a bidirectional relationship
> diff --git a/qom/object.c b/qom/object.c
> index 4609e34..7be7638 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -392,6 +392,21 @@ void object_initialize(void *data, size_t size, const char *typename)
>      object_initialize_with_type(data, size, type);
>  }
>  
> +void object_initialize_child(Object *parentobj, const char *propname,
> +                             void *childobj, size_t size, const char *type,
> +                             Error **errp)
> +{
> +    object_initialize(childobj, size, type);
> +    object_property_add_child(parentobj, propname, OBJECT(childobj), errp);
> +    /*
> +     * Since object_property_add_child added a reference to the child object,
> +     * we can drop the reference added by object_initialize(), so the child
> +     * property will own the only reference to the object.
> +     */
> +    object_unref(OBJECT(childobj));
> +}
> +
> +
>  static inline bool object_property_is_child(ObjectProperty *prop)
>  {
>      return strstart(prop->type, "child<", NULL);
> 

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:29AM +0200, Thomas Huth wrote:
> A lot of code is using the object_initialize() function followed by a call
> to object_property_add_child() to add the newly initialized object as a child
> of the current object. Both functions increase the reference counter of the
> new object, but many spots that call these two functions then forget to drop
> one of the superfluous references. So the newly created object is often not
> cleaned up correctly when the parent is destroyed. In the worst case, this
> can cause crashes, e.g. because device objects are not correctly removed from
> their parent_bus.
> 
> Since this is a common pattern between many code spots, let's introdcue a
> new function that takes care of calling all three required initialization
> functions, first object_initialize(), then object_property_add_child() and
> finally object_unref().
> 
> And while we're at object.h, also fix some copy-n-paste errors in the
> comments there ("to store the area" --> "to store the error").
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/qom/object.h | 23 +++++++++++++++++++++--
>  qom/object.c         | 15 +++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3d2308..3362db0 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>  void object_initialize(void *obj, size_t size, const char *typename);
>  
>  /**
> + * object_initialize_child:
> + * @parentobj: The parent object to add a property to
> + * @propname: The name of the property
> + * @childobj: A pointer to the memory to be used for the object.
> + * @size: The maximum size available at @obj for the object.
> + * @type: The name of the type of the object to instantiate.
> + * @errp: If an error occurs, a pointer to an area to store the error
> + *
> + * This function will initialize an object. The memory for the object should
> + * have already been allocated. The object will then be added as child property
> + * to a parent with object_property_add_child() function. The returned object
> + * has a reference count of 1 (for the "child<...>" property from the parent),
> + * so the object will get finalized automatically when the parent gets removed.
> + */
> +void object_initialize_child(Object *parentobj, const char *propname,
> +                             void *childobj, size_t size, const char *type,
> +                             Error **errp);

I wonder if we should deprecate object_initialize() and support
only object_initialize_child() later.  Initializing an object
contained inside another one without making it a child of the
parent object is a recipe for trouble.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Andreas Färber]

Am 13.07.2018 um 23:16 schrieb Eduardo Habkost:
> I wonder if we should deprecate object_initialize() and support
> only object_initialize_child() later.  Initializing an object
> contained inside another one without making it a child of the
> parent object is a recipe for trouble.

The root container object needs to be initialized, too.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 11:29:17PM +0200, Andreas Färber wrote:
> Am 13.07.2018 um 23:16 schrieb Eduardo Habkost:
> > I wonder if we should deprecate object_initialize() and support
> > only object_initialize_child() later.  Initializing an object
> > contained inside another one without making it a child of the
> > parent object is a recipe for trouble.
> 
> The root container object needs to be initialized, too.

If the object is not embedded in another struct, I would expect
it to be created using object_new() instead of
object_initialize().

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Thomas Huth]

On 13.07.2018 23:46, Eduardo Habkost wrote:
> On Fri, Jul 13, 2018 at 11:29:17PM +0200, Andreas Färber wrote:
>> Am 13.07.2018 um 23:16 schrieb Eduardo Habkost:
>>> I wonder if we should deprecate object_initialize() and support
>>> only object_initialize_child() later.  Initializing an object
>>> contained inside another one without making it a child of the
>>> parent object is a recipe for trouble.
>>
>> The root container object needs to be initialized, too.
> 
> If the object is not embedded in another struct, I would expect
> it to be created using object_new() instead of
> object_initialize().

True. So I guess having a closer look at code that calls
object_initialize() only is something for our TODO lists once 3.0 has
been released...

 Thomas

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:29AM +0200, Thomas Huth wrote:
> A lot of code is using the object_initialize() function followed by a call
> to object_property_add_child() to add the newly initialized object as a child
> of the current object. Both functions increase the reference counter of the
> new object, but many spots that call these two functions then forget to drop
> one of the superfluous references. So the newly created object is often not
> cleaned up correctly when the parent is destroyed. In the worst case, this
> can cause crashes, e.g. because device objects are not correctly removed from
> their parent_bus.
> 
> Since this is a common pattern between many code spots, let's introdcue a
> new function that takes care of calling all three required initialization
> functions, first object_initialize(), then object_property_add_child() and
> finally object_unref().
> 
> And while we're at object.h, also fix some copy-n-paste errors in the
> comments there ("to store the area" --> "to store the error").
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Potential candidates for using the new function, found using the
following Coccinelle patch:

@@
expression child, size, type, parent, errp, propname;
@@
-object_initialize(child, size, type);
-object_property_add_child(
+object_initialize_child(
                           parent, propname, 
-                          OBJECT(child),
+                          child, size, type,
                           errp);

Some of them (very few) already call object_unref() and need to
be fixed manually.

Most of the remaining ~50 object_initialize() callers are also
candidates, even if they don't call object_property_add_child()
today.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index bb9d33848d..e5923f85af 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -189,8 +189,8 @@ static void aspeed_board_init(MachineState *machine,
     DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
 
     bmc = g_new0(AspeedBoardState, 1);
-    object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&bmc->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &bmc->soc, (sizeof(bmc->soc)), cfg->soc_name,
                               &error_abort);
 
     sc = ASPEED_SOC_GET_CLASS(&bmc->soc);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index e68911af0f..994262756f 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -106,11 +106,11 @@ static void aspeed_soc_init(Object *obj)
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
     int i;
 
-    object_initialize(&s->cpu, sizeof(s->cpu), sc->info->cpu_type);
-    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
+    object_initialize_child(obj, "cpu",  &s->cpu, sizeof(s->cpu),
+                            sc->info->cpu_type, NULL);
 
-    object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
-    object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
+    object_initialize_child(obj, "scu",  &s->scu, sizeof(s->scu),
+                            TYPE_ASPEED_SCU, NULL);
     qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
     qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",
                          sc->info->silicon_rev);
@@ -121,35 +121,34 @@ static void aspeed_soc_init(Object *obj)
     object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu),
                               "hw-prot-key", &error_abort);
 
-    object_initialize(&s->vic, sizeof(s->vic), TYPE_ASPEED_VIC);
-    object_property_add_child(obj, "vic", OBJECT(&s->vic), NULL);
+    object_initialize_child(obj, "vic",  &s->vic, sizeof(s->vic),
+                            TYPE_ASPEED_VIC, NULL);
     qdev_set_parent_bus(DEVICE(&s->vic), sysbus_get_default());
 
-    object_initialize(&s->timerctrl, sizeof(s->timerctrl), TYPE_ASPEED_TIMER);
-    object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), NULL);
+    object_initialize_child(obj, "timerctrl",  &s->timerctrl,
+                            sizeof(s->timerctrl), TYPE_ASPEED_TIMER, NULL);
     object_property_add_const_link(OBJECT(&s->timerctrl), "scu",
                                    OBJECT(&s->scu), &error_abort);
     qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
 
-    object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
-    object_property_add_child(obj, "i2c", OBJECT(&s->i2c), NULL);
+    object_initialize_child(obj, "i2c",  &s->i2c, sizeof(s->i2c),
+                            TYPE_ASPEED_I2C, NULL);
     qdev_set_parent_bus(DEVICE(&s->i2c), sysbus_get_default());
 
-    object_initialize(&s->fmc, sizeof(s->fmc), sc->info->fmc_typename);
-    object_property_add_child(obj, "fmc", OBJECT(&s->fmc), NULL);
+    object_initialize_child(obj, "fmc",  &s->fmc, sizeof(s->fmc),
+                            sc->info->fmc_typename, NULL);
     qdev_set_parent_bus(DEVICE(&s->fmc), sysbus_get_default());
     object_property_add_alias(obj, "num-cs", OBJECT(&s->fmc), "num-cs",
                               &error_abort);
 
     for (i = 0; i < sc->info->spis_num; i++) {
-        object_initialize(&s->spi[i], sizeof(s->spi[i]),
-                          sc->info->spi_typename[i]);
-        object_property_add_child(obj, "spi[*]", OBJECT(&s->spi[i]), NULL);
+        object_initialize_child(obj, "spi[*]",  &s->spi[i], sizeof(s->spi[i]),
+                                sc->info->spi_typename[i], NULL);
         qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
     }
 
-    object_initialize(&s->sdmc, sizeof(s->sdmc), TYPE_ASPEED_SDMC);
-    object_property_add_child(obj, "sdmc", OBJECT(&s->sdmc), NULL);
+    object_initialize_child(obj, "sdmc",  &s->sdmc, sizeof(s->sdmc),
+                            TYPE_ASPEED_SDMC, NULL);
     qdev_set_parent_bus(DEVICE(&s->sdmc), sysbus_get_default());
     qdev_prop_set_uint32(DEVICE(&s->sdmc), "silicon-rev",
                          sc->info->silicon_rev);
@@ -157,15 +156,15 @@ static void aspeed_soc_init(Object *obj)
                               "ram-size", &error_abort);
 
     for (i = 0; i < sc->info->wdts_num; i++) {
-        object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_ASPEED_WDT);
-        object_property_add_child(obj, "wdt[*]", OBJECT(&s->wdt[i]), NULL);
+        object_initialize_child(obj, "wdt[*]",  &s->wdt[i], sizeof(s->wdt[i]),
+                                TYPE_ASPEED_WDT, NULL);
         qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus_get_default());
         qdev_prop_set_uint32(DEVICE(&s->wdt[i]), "silicon-rev",
                                     sc->info->silicon_rev);
     }
 
-    object_initialize(&s->ftgmac100, sizeof(s->ftgmac100), TYPE_FTGMAC100);
-    object_property_add_child(obj, "ftgmac100", OBJECT(&s->ftgmac100), NULL);
+    object_initialize_child(obj, "ftgmac100",  &s->ftgmac100,
+                            sizeof(s->ftgmac100), TYPE_FTGMAC100, NULL);
     qdev_set_parent_bus(DEVICE(&s->ftgmac100), sysbus_get_default());
 }
 
diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 6be7660e8c..0fbfd4e4fb 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -41,8 +41,8 @@ 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);
+    object_initialize_child(obj, "ic",  &s->ic, sizeof(s->ic),
+                            TYPE_BCM2835_IC, NULL);
     qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
 
     /* UART0 */
@@ -51,21 +51,21 @@ 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);
+    object_initialize_child(obj, "aux",  &s->aux, sizeof(s->aux),
+                            TYPE_BCM2835_AUX, NULL);
     qdev_set_parent_bus(DEVICE(&s->aux), sysbus_get_default());
 
     /* Mailboxes */
-    object_initialize(&s->mboxes, sizeof(s->mboxes), TYPE_BCM2835_MBOX);
-    object_property_add_child(obj, "mbox", OBJECT(&s->mboxes), NULL);
+    object_initialize_child(obj, "mbox",  &s->mboxes, sizeof(s->mboxes),
+                            TYPE_BCM2835_MBOX, NULL);
     qdev_set_parent_bus(DEVICE(&s->mboxes), sysbus_get_default());
 
     object_property_add_const_link(OBJECT(&s->mboxes), "mbox-mr",
                                    OBJECT(&s->mbox_mr), &error_abort);
 
     /* Framebuffer */
-    object_initialize(&s->fb, sizeof(s->fb), TYPE_BCM2835_FB);
-    object_property_add_child(obj, "fb", OBJECT(&s->fb), NULL);
+    object_initialize_child(obj, "fb",  &s->fb, sizeof(s->fb),
+                            TYPE_BCM2835_FB, NULL);
     object_property_add_alias(obj, "vcram-size", OBJECT(&s->fb), "vcram-size",
                               &error_abort);
     qdev_set_parent_bus(DEVICE(&s->fb), sysbus_get_default());
@@ -74,8 +74,8 @@ static void bcm2835_peripherals_init(Object *obj)
                                    OBJECT(&s->gpu_bus_mr), &error_abort);
 
     /* Property channel */
-    object_initialize(&s->property, sizeof(s->property), TYPE_BCM2835_PROPERTY);
-    object_property_add_child(obj, "property", OBJECT(&s->property), NULL);
+    object_initialize_child(obj, "property",  &s->property,
+                            sizeof(s->property), TYPE_BCM2835_PROPERTY, NULL);
     object_property_add_alias(obj, "board-rev", OBJECT(&s->property),
                               "board-rev", &error_abort);
     qdev_set_parent_bus(DEVICE(&s->property), sysbus_get_default());
@@ -86,31 +86,31 @@ 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);
+    object_initialize_child(obj, "rng",  &s->rng, sizeof(s->rng),
+                            TYPE_BCM2835_RNG, NULL);
     qdev_set_parent_bus(DEVICE(&s->rng), sysbus_get_default());
 
     /* Extended Mass Media Controller */
-    object_initialize(&s->sdhci, sizeof(s->sdhci), TYPE_SYSBUS_SDHCI);
-    object_property_add_child(obj, "sdhci", OBJECT(&s->sdhci), NULL);
+    object_initialize_child(obj, "sdhci",  &s->sdhci, sizeof(s->sdhci),
+                            TYPE_SYSBUS_SDHCI, NULL);
     qdev_set_parent_bus(DEVICE(&s->sdhci), sysbus_get_default());
 
     /* SDHOST */
-    object_initialize(&s->sdhost, sizeof(s->sdhost), TYPE_BCM2835_SDHOST);
-    object_property_add_child(obj, "sdhost", OBJECT(&s->sdhost), NULL);
+    object_initialize_child(obj, "sdhost",  &s->sdhost, sizeof(s->sdhost),
+                            TYPE_BCM2835_SDHOST, NULL);
     qdev_set_parent_bus(DEVICE(&s->sdhost), sysbus_get_default());
 
     /* DMA Channels */
-    object_initialize(&s->dma, sizeof(s->dma), TYPE_BCM2835_DMA);
-    object_property_add_child(obj, "dma", OBJECT(&s->dma), NULL);
+    object_initialize_child(obj, "dma",  &s->dma, sizeof(s->dma),
+                            TYPE_BCM2835_DMA, NULL);
     qdev_set_parent_bus(DEVICE(&s->dma), sysbus_get_default());
 
     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);
+    object_initialize_child(obj, "gpio",  &s->gpio, sizeof(s->gpio),
+                            TYPE_BCM2835_GPIO, NULL);
     qdev_set_parent_bus(DEVICE(&s->gpio), sysbus_get_default());
 
     object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhci",
diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index 726abb9b48..90faca0710 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -35,8 +35,8 @@ static void digic_init(Object *obj)
     DeviceState *dev;
     int i;
 
-    object_initialize(&s->cpu, sizeof(s->cpu), "arm946-" TYPE_ARM_CPU);
-    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
+    object_initialize_child(obj, "cpu",  &s->cpu, sizeof(s->cpu),
+                            "arm946-" TYPE_ARM_CPU, NULL);
 
     for (i = 0; i < DIGIC4_NB_TIMERS; i++) {
 #define DIGIC_TIMER_NAME_MLEN    11
diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index 9f3ee14739..76769531b2 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -72,8 +72,8 @@ static void imx25_pdk_init(MachineState *machine)
     unsigned int alias_offset;
     int i;
 
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX25);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_FSL_IMX25,
                               &error_abort);
 
     object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_fatal);
diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index 864c7bd411..42f540fcdd 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -71,8 +71,8 @@ static void kzm_init(MachineState *machine)
     unsigned int alias_offset;
     unsigned int i;
 
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX31);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_FSL_IMX31,
                               &error_abort);
 
     object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_fatal);
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 22180c56fb..4849f5a6d1 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -111,8 +111,8 @@ static void init_sysbus_child(Object *parent, const char *childname,
                               void *child, size_t childsize,
                               const char *childtype)
 {
-    object_initialize(child, childsize, childtype);
-    object_property_add_child(parent, childname, OBJECT(child), &error_abort);
+    object_initialize_child(parent, childname,  child, childsize, childtype,
+                            &error_abort);
     qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
 
 }
@@ -301,10 +301,10 @@ static void mps2tz_common_init(MachineState *machine)
     /* The sec_resp_cfg output from the IoTKit must be split into multiple
      * lines, one for each of the PPCs we create here.
      */
-    object_initialize(&mms->sec_resp_splitter, sizeof(mms->sec_resp_splitter),
-                      TYPE_SPLIT_IRQ);
-    object_property_add_child(OBJECT(machine), "sec-resp-splitter",
-                              OBJECT(&mms->sec_resp_splitter), &error_abort);
+    object_initialize_child(OBJECT(machine), "sec-resp-splitter",
+                               &mms->sec_resp_splitter,
+                              sizeof(mms->sec_resp_splitter), TYPE_SPLIT_IRQ,
+                              &error_abort);
     object_property_set_int(OBJECT(&mms->sec_resp_splitter), 5,
                             "num-lines", &error_fatal);
     object_property_set_bool(OBJECT(&mms->sec_resp_splitter), true,
@@ -338,10 +338,10 @@ static void mps2tz_common_init(MachineState *machine)
      * Tx, Rx and "combined" IRQs are sent to the NVIC separately.
      * Create the OR gate for this.
      */
-    object_initialize(&mms->uart_irq_orgate, sizeof(mms->uart_irq_orgate),
-                      TYPE_OR_IRQ);
-    object_property_add_child(OBJECT(mms), "uart-irq-orgate",
-                              OBJECT(&mms->uart_irq_orgate), &error_abort);
+    object_initialize_child(OBJECT(mms), "uart-irq-orgate",
+                               &mms->uart_irq_orgate,
+                              sizeof(mms->uart_irq_orgate), TYPE_OR_IRQ,
+                              &error_abort);
     object_property_set_int(OBJECT(&mms->uart_irq_orgate), 10, "num-lines",
                             &error_fatal);
     object_property_set_bool(OBJECT(&mms->uart_irq_orgate), true,
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 66899c28dc..6c9f89cbfd 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -175,9 +175,9 @@ static void raspi_init(MachineState *machine, int version)
     BusState *bus;
     DeviceState *carddev;
 
-    object_initialize(&s->soc, sizeof(s->soc),
-                      version == 3 ? TYPE_BCM2837 : TYPE_BCM2836);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc),
+                              version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
                               &error_abort);
 
     /* Allocate and map RAM */
diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index ee140e5d9e..fe094c95d4 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -55,8 +55,8 @@ static void sabrelite_init(MachineState *machine)
         exit(1);
     }
 
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX6);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_FSL_IMX6,
                               &error_abort);
 
     object_property_set_bool(OBJECT(&s->soc), true, "realized", &err);
diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index b6bc6a93b8..1d5a4c6be8 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -91,8 +91,8 @@ static void xlnx_zcu102_init(MachineState *machine)
     memory_region_allocate_system_memory(&s->ddr_ram, NULL, "ddr-ram",
                                          ram_size);
 
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_XLNX_ZYNQMP,
                               &error_abort);
 
     object_property_set_link(OBJECT(&s->soc), OBJECT(&s->ddr_ram),
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index a1365b44a0..f0420df8c4 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -133,10 +133,9 @@ static void xlnx_zynqmp_create_rpu(XlnxZynqMPState *s, const char *boot_cpu,
     for (i = 0; i < num_rpus; i++) {
         char *name;
 
-        object_initialize(&s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
-                          "cortex-r5f-" TYPE_ARM_CPU);
-        object_property_add_child(OBJECT(s), "rpu-cpu[*]",
-                                  OBJECT(&s->rpu_cpu[i]), &error_abort);
+        object_initialize_child(OBJECT(s), "rpu-cpu[*]",
+                                   &s->rpu_cpu[i], sizeof(s->rpu_cpu[i]),
+                                  "cortex-r5f-" TYPE_ARM_CPU, &error_abort);
 
         name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
         if (strcmp(name, boot_cpu)) {
diff --git a/hw/microblaze/xlnx-zynqmp-pmu.c b/hw/microblaze/xlnx-zynqmp-pmu.c
index 999a5657cf..62f2e43707 100644
--- a/hw/microblaze/xlnx-zynqmp-pmu.c
+++ b/hw/microblaze/xlnx-zynqmp-pmu.c
@@ -62,9 +62,8 @@ static void xlnx_zynqmp_pmu_soc_init(Object *obj)
 {
     XlnxZynqMPPMUSoCState *s = XLNX_ZYNQMP_PMU_SOC(obj);
 
-    object_initialize(&s->cpu, sizeof(s->cpu),
-                      TYPE_MICROBLAZE_CPU);
-    object_property_add_child(obj, "pmu-cpu", OBJECT(&s->cpu),
+    object_initialize_child(obj, "pmu-cpu",
+                              &s->cpu, sizeof(s->cpu), TYPE_MICROBLAZE_CPU,
                               &error_abort);
 
     object_initialize(&s->intc, sizeof(s->intc), TYPE_XLNX_PMU_IO_INTC);
@@ -163,9 +162,9 @@ static void xlnx_zynqmp_pmu_init(MachineState *machine)
                                 pmu_ram);
 
     /* Create the PMU device */
-    object_initialize(pmu, sizeof(XlnxZynqMPPMUSoCState), TYPE_XLNX_ZYNQMP_PMU_SOC);
-    object_property_add_child(OBJECT(machine), "pmu", OBJECT(pmu),
-                              &error_abort);
+    object_initialize_child(OBJECT(machine), "pmu",
+                              pmu, sizeof(XlnxZynqMPPMUSoCState),
+                              TYPE_XLNX_ZYNQMP_PMU_SOC, &error_abort);
     object_property_set_bool(OBJECT(pmu), true, "realized", &error_fatal);
 
     for (i = 0; i < 32; i++) {
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 29ea313798..8590ec657b 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -721,8 +721,8 @@ static void designware_pcie_host_init(Object *obj)
     DesignwarePCIEHost *s = DESIGNWARE_PCIE_HOST(obj);
     DesignwarePCIERoot *root = &s->root;
 
-    object_initialize(root, sizeof(*root), TYPE_DESIGNWARE_PCIE_ROOT);
-    object_property_add_child(obj, "root", OBJECT(root), NULL);
+    object_initialize_child(obj, "root",  root, sizeof(*root),
+                            TYPE_DESIGNWARE_PCIE_ROOT, NULL);
     qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(root), "multifunction", false);
 }
diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
index 2583b151a4..060e80930b 100644
--- a/hw/pci-host/gpex.c
+++ b/hw/pci-host/gpex.c
@@ -120,8 +120,8 @@ static void gpex_host_initfn(Object *obj)
     GPEXHost *s = GPEX_HOST(obj);
     GPEXRootState *root = &s->gpex_root;
 
-    object_initialize(root, sizeof(*root), TYPE_GPEX_ROOT_DEVICE);
-    object_property_add_child(obj, "gpex_root", OBJECT(root), NULL);
+    object_initialize_child(obj, "gpex_root",  root, sizeof(*root),
+                            TYPE_GPEX_ROOT_DEVICE, NULL);
     qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(root), "multifunction", false);
 }
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 02f9576588..d590035659 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -205,8 +205,8 @@ static void q35_host_initfn(Object *obj)
     memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
                           "pci-conf-data", 4);
 
-    object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
-    object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
+    object_initialize_child(OBJECT(s), "mch",  &s->mch, sizeof(s->mch),
+                            TYPE_MCH_PCI_DEVICE, NULL);
     qdev_prop_set_int32(DEVICE(&s->mch), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(&s->mch), "multifunction", false);
     /* mch's object_initialize resets the default value, set it again */
diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
index 60309afe9e..f82d3f3e33 100644
--- a/hw/pci-host/xilinx-pcie.c
+++ b/hw/pci-host/xilinx-pcie.c
@@ -149,8 +149,8 @@ static void xilinx_pcie_host_init(Object *obj)
     XilinxPCIEHost *s = XILINX_PCIE_HOST(obj);
     XilinxPCIERoot *root = &s->root;
 
-    object_initialize(root, sizeof(*root), TYPE_XILINX_PCIE_ROOT);
-    object_property_add_child(obj, "root", OBJECT(root), NULL);
+    object_initialize_child(obj, "root",  root, sizeof(*root),
+                            TYPE_XILINX_PCIE_ROOT, NULL);
     qdev_prop_set_int32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
     qdev_prop_set_bit(DEVICE(root), "multifunction", false);
 }
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0989f6a05f..93eb2948cc 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -726,18 +726,18 @@ static void pnv_chip_power8_instance_init(Object *obj)
 {
     Pnv8Chip *chip8 = PNV8_CHIP(obj);
 
-    object_initialize(&chip8->psi, sizeof(chip8->psi), TYPE_PNV_PSI);
-    object_property_add_child(obj, "psi", OBJECT(&chip8->psi), NULL);
+    object_initialize_child(obj, "psi",  &chip8->psi, sizeof(chip8->psi),
+                            TYPE_PNV_PSI, NULL);
     object_property_add_const_link(OBJECT(&chip8->psi), "xics",
                                    OBJECT(qdev_get_machine()), &error_abort);
 
-    object_initialize(&chip8->lpc, sizeof(chip8->lpc), TYPE_PNV_LPC);
-    object_property_add_child(obj, "lpc", OBJECT(&chip8->lpc), NULL);
+    object_initialize_child(obj, "lpc",  &chip8->lpc, sizeof(chip8->lpc),
+                            TYPE_PNV_LPC, NULL);
     object_property_add_const_link(OBJECT(&chip8->lpc), "psi",
                                    OBJECT(&chip8->psi), &error_abort);
 
-    object_initialize(&chip8->occ, sizeof(chip8->occ), TYPE_PNV_OCC);
-    object_property_add_child(obj, "occ", OBJECT(&chip8->occ), NULL);
+    object_initialize_child(obj, "occ",  &chip8->occ, sizeof(chip8->occ),
+                            TYPE_PNV_OCC, NULL);
     object_property_add_const_link(OBJECT(&chip8->occ), "psi",
                                    OBJECT(&chip8->psi), &error_abort);
 }
@@ -975,9 +975,8 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
         }
 
         snprintf(core_name, sizeof(core_name), "core[%d]", core_hwid);
-        object_initialize(pnv_core, typesize, typename);
-        object_property_add_child(OBJECT(chip), core_name, OBJECT(pnv_core),
-                                  &error_fatal);
+        object_initialize_child(OBJECT(chip), core_name,
+                                  pnv_core, typesize, typename, &error_fatal);
         object_property_set_int(OBJECT(pnv_core), smp_threads, "nr-threads",
                                 &error_fatal);
         object_property_set_int(OBJECT(pnv_core), core_hwid,
diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 5b969127c3..2e29089e66 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -445,8 +445,8 @@ static void pnv_psi_init(Object *obj)
 {
     PnvPsi *psi = PNV_PSI(obj);
 
-    object_initialize(&psi->ics, sizeof(psi->ics), TYPE_ICS_SIMPLE);
-    object_property_add_child(obj, "ics-psi", OBJECT(&psi->ics), NULL);
+    object_initialize_child(obj, "ics-psi",  &psi->ics, sizeof(psi->ics),
+                            TYPE_ICS_SIMPLE, NULL);
 }
 
 static const uint8_t irq_to_xivr[] = {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3f5e1d3ec2..8c4bdb147a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1700,8 +1700,8 @@ static void spapr_create_nvram(sPAPRMachineState *spapr)
 
 static void spapr_rtc_create(sPAPRMachineState *spapr)
 {
-    object_initialize(&spapr->rtc, sizeof(spapr->rtc), TYPE_SPAPR_RTC);
-    object_property_add_child(OBJECT(spapr), "rtc", OBJECT(&spapr->rtc),
+    object_initialize_child(OBJECT(spapr), "rtc",
+                              &spapr->rtc, sizeof(spapr->rtc), TYPE_SPAPR_RTC,
                               &error_fatal);
     object_property_set_bool(OBJECT(&spapr->rtc), true, "realized",
                               &error_fatal);
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index 8a8dbe1c00..b323eac391 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -105,8 +105,8 @@ static void riscv_sifive_e_init(MachineState *machine)
     int i;
 
     /* Initialize SoC */
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_E_SOC);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_RISCV_E_SOC,
                               &error_abort);
     object_property_set_bool(OBJECT(&s->soc), true, "realized",
                             &error_abort);
@@ -139,9 +139,9 @@ static void riscv_sifive_e_soc_init(Object *obj)
 {
     SiFiveESoCState *s = RISCV_E_SOC(obj);
 
-    object_initialize(&s->cpus, sizeof(s->cpus), TYPE_RISCV_HART_ARRAY);
-    object_property_add_child(obj, "cpus", OBJECT(&s->cpus),
-                              &error_abort);
+    object_initialize_child(obj, "cpus",
+                              &s->cpus, sizeof(s->cpus),
+                              TYPE_RISCV_HART_ARRAY, &error_abort);
     object_property_set_str(OBJECT(&s->cpus), SIFIVE_E_CPU, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 3a6ffeb437..c94900f345 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -244,8 +244,8 @@ static void riscv_sifive_u_init(MachineState *machine)
     int i;
 
     /* Initialize SoC */
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_U_SOC);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_RISCV_U_SOC,
                               &error_abort);
     object_property_set_bool(OBJECT(&s->soc), true, "realized",
                             &error_abort);
@@ -303,9 +303,9 @@ static void riscv_sifive_u_soc_init(Object *obj)
 {
     SiFiveUSoCState *s = RISCV_U_SOC(obj);
 
-    object_initialize(&s->cpus, sizeof(s->cpus), TYPE_RISCV_HART_ARRAY);
-    object_property_add_child(obj, "cpus", OBJECT(&s->cpus),
-                              &error_abort);
+    object_initialize_child(obj, "cpus",
+                              &s->cpus, sizeof(s->cpus),
+                              TYPE_RISCV_HART_ARRAY, &error_abort);
     object_property_set_str(OBJECT(&s->cpus), SIFIVE_U_CPU, "cpu-type",
                             &error_abort);
     object_property_set_int(OBJECT(&s->cpus), smp_cpus, "num-harts",
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index f94e2b6707..44639a8c99 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -171,8 +171,8 @@ static void spike_v1_10_0_board_init(MachineState *machine)
     int i;
 
     /* Initialize SOC */
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY,
                               &error_abort);
     object_property_set_str(OBJECT(&s->soc), SPIKE_V1_10_0_CPU, "cpu-type",
                             &error_abort);
@@ -254,8 +254,8 @@ static void spike_v1_09_1_board_init(MachineState *machine)
     int i;
 
     /* Initialize SOC */
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY,
                               &error_abort);
     object_property_set_str(OBJECT(&s->soc), SPIKE_V1_09_1_CPU, "cpu-type",
                             &error_abort);
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index aeada2498d..d597916885 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -274,8 +274,8 @@ static void riscv_virt_board_init(MachineState *machine)
     void *fdt;
 
     /* Initialize SOC */
-    object_initialize(&s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY);
-    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+    object_initialize_child(OBJECT(machine), "soc",
+                              &s->soc, sizeof(s->soc), TYPE_RISCV_HART_ARRAY,
                               &error_abort);
     object_property_set_str(OBJECT(&s->soc), VIRT_CPU, "cpu-type",
                             &error_abort);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d4e4d98b59..acf7b4e40e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2232,8 +2232,8 @@ void virtio_instance_init_common(Object *proxy_obj, void *data,
 {
     DeviceState *vdev = data;
 
-    object_initialize(vdev, vdev_size, vdev_name);
-    object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev), NULL);
+    object_initialize_child(proxy_obj, "virtio-backend",  vdev, vdev_size,
+                            vdev_name, NULL);
     object_unref(OBJECT(vdev));
     qdev_alias_all_properties(vdev, proxy_obj);
 }
diff --git a/qom/object.c b/qom/object.c
index 7be7638db1..91ff795b38 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -396,8 +396,7 @@ void object_initialize_child(Object *parentobj, const char *propname,
                              void *childobj, size_t size, const char *type,
                              Error **errp)
 {
-    object_initialize(childobj, size, type);
-    object_property_add_child(parentobj, propname, OBJECT(childobj), errp);
+    object_initialize_child(parentobj, propname,  childobj, size, type, errp);
     /*
      * Since object_property_add_child added a reference to the child object,
      * we can drop the reference added by object_initialize(), so the child
 
-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Thomas Huth]

On 14.07.2018 00:57, Eduardo Habkost wrote:
> On Fri, Jul 13, 2018 at 10:27:29AM +0200, Thomas Huth wrote:
>> A lot of code is using the object_initialize() function followed by a call
>> to object_property_add_child() to add the newly initialized object as a child
>> of the current object. Both functions increase the reference counter of the
>> new object, but many spots that call these two functions then forget to drop
>> one of the superfluous references. So the newly created object is often not
>> cleaned up correctly when the parent is destroyed. In the worst case, this
>> can cause crashes, e.g. because device objects are not correctly removed from
>> their parent_bus.
>>
>> Since this is a common pattern between many code spots, let's introdcue a
>> new function that takes care of calling all three required initialization
>> functions, first object_initialize(), then object_property_add_child() and
>> finally object_unref().
>>
>> And while we're at object.h, also fix some copy-n-paste errors in the
>> comments there ("to store the area" --> "to store the error").
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Potential candidates for using the new function, found using the
> following Coccinelle patch:
> 
> @@
> expression child, size, type, parent, errp, propname;
> @@
> -object_initialize(child, size, type);
> -object_property_add_child(
> +object_initialize_child(
>                            parent, propname, 
> -                          OBJECT(child),
> +                          child, size, type,
>                            errp);
> 
> Some of them (very few) already call object_unref() and need to
> be fixed manually.
> 
> Most of the remaining ~50 object_initialize() callers are also
> candidates, even if they don't call object_property_add_child()
> today.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index bb9d33848d..e5923f85af 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -189,8 +189,8 @@ static void aspeed_board_init(MachineState *machine,
>      DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
>  
>      bmc = g_new0(AspeedBoardState, 1);
> -    object_initialize(&bmc->soc, (sizeof(bmc->soc)), cfg->soc_name);
> -    object_property_add_child(OBJECT(machine), "soc", OBJECT(&bmc->soc),
> +    object_initialize_child(OBJECT(machine), "soc",
> +                              &bmc->soc, (sizeof(bmc->soc)), cfg->soc_name,
>                                &error_abort);
>  
>      sc = ASPEED_SOC_GET_CLASS(&bmc->soc);
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index e68911af0f..994262756f 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -106,11 +106,11 @@ static void aspeed_soc_init(Object *obj)
>      AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>      int i;
>  
> -    object_initialize(&s->cpu, sizeof(s->cpu), sc->info->cpu_type);
> -    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> +    object_initialize_child(obj, "cpu",  &s->cpu, sizeof(s->cpu),
> +                            sc->info->cpu_type, NULL);
>  
> -    object_initialize(&s->scu, sizeof(s->scu), TYPE_ASPEED_SCU);
> -    object_property_add_child(obj, "scu", OBJECT(&s->scu), NULL);
> +    object_initialize_child(obj, "scu",  &s->scu, sizeof(s->scu),
> +                            TYPE_ASPEED_SCU, NULL);
>      qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
>      qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev",

Thanks, that's definitely something we should do for 3.1! But for 3.0, I
think we better only focus on the ones that cause reproducible problem.

And the spots that also use qdev_set_parent_bus(...,
sysbus_get_default()) should use sysbus_init_child_obj() instead.

 Thomas

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Thomas Huth]

On 07/14/2018 12:57 AM, Eduardo Habkost wrote:
> On Fri, Jul 13, 2018 at 10:27:29AM +0200, Thomas Huth wrote:
>> A lot of code is using the object_initialize() function followed by a call
>> to object_property_add_child() to add the newly initialized object as a child
>> of the current object. Both functions increase the reference counter of the
>> new object, but many spots that call these two functions then forget to drop
>> one of the superfluous references. So the newly created object is often not
>> cleaned up correctly when the parent is destroyed. In the worst case, this
>> can cause crashes, e.g. because device objects are not correctly removed from
>> their parent_bus.
>>
>> Since this is a common pattern between many code spots, let's introdcue a
>> new function that takes care of calling all three required initialization
>> functions, first object_initialize(), then object_property_add_child() and
>> finally object_unref().
>>
>> And while we're at object.h, also fix some copy-n-paste errors in the
>> comments there ("to store the area" --> "to store the error").
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Potential candidates for using the new function, found using the
> following Coccinelle patch:
> 
> @@
> expression child, size, type, parent, errp, propname;
> @@
> -object_initialize(child, size, type);
> -object_property_add_child(
> +object_initialize_child(
>                            parent, propname, 
> -                          OBJECT(child),
> +                          child, size, type,
>                            errp);
> 
> Some of them (very few) already call object_unref() and need to
> be fixed manually.
> 
> Most of the remaining ~50 object_initialize() callers are also
> candidates, even if they don't call object_property_add_child()
> today.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Care to turn this into a proper patch, now that we left the freeze period?

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d4e4d98b59..acf7b4e40e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2232,8 +2232,8 @@ void virtio_instance_init_common(Object *proxy_obj, void *data,
>  {
>      DeviceState *vdev = data;
>  
> -    object_initialize(vdev, vdev_size, vdev_name);
> -    object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev), NULL);
> +    object_initialize_child(proxy_obj, "virtio-backend",  vdev, vdev_size,
> +                            vdev_name, NULL);
>      object_unref(OBJECT(vdev));
>      qdev_alias_all_properties(vdev, proxy_obj);
>  }
> diff --git a/qom/object.c b/qom/object.c
> index 7be7638db1..91ff795b38 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -396,8 +396,7 @@ void object_initialize_child(Object *parentobj, const char *propname,
>                               void *childobj, size_t size, const char *type,
>                               Error **errp)
>  {
> -    object_initialize(childobj, size, type);
> -    object_property_add_child(parentobj, propname, OBJECT(childobj), errp);
> +    object_initialize_child(parentobj, propname,  childobj, size, type, errp);
>      /*
>       * Since object_property_add_child added a reference to the child object,
>       * we can drop the reference added by object_initialize(), so the child

At least these two hunks need manual adjustment.

 Thomas

Re: [Qemu-devel] [PATCH v2 01/16] qom/object: Add a new function object_initialize_child()

[Eduardo Habkost]

On Thu, Aug 16, 2018 at 01:59:49PM +0200, Thomas Huth wrote:
> On 07/14/2018 12:57 AM, Eduardo Habkost wrote:
> > On Fri, Jul 13, 2018 at 10:27:29AM +0200, Thomas Huth wrote:
> >> A lot of code is using the object_initialize() function followed by a call
> >> to object_property_add_child() to add the newly initialized object as a child
> >> of the current object. Both functions increase the reference counter of the
> >> new object, but many spots that call these two functions then forget to drop
> >> one of the superfluous references. So the newly created object is often not
> >> cleaned up correctly when the parent is destroyed. In the worst case, this
> >> can cause crashes, e.g. because device objects are not correctly removed from
> >> their parent_bus.
> >>
> >> Since this is a common pattern between many code spots, let's introdcue a
> >> new function that takes care of calling all three required initialization
> >> functions, first object_initialize(), then object_property_add_child() and
> >> finally object_unref().
> >>
> >> And while we're at object.h, also fix some copy-n-paste errors in the
> >> comments there ("to store the area" --> "to store the error").
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > Potential candidates for using the new function, found using the
> > following Coccinelle patch:
> > 
> > @@
> > expression child, size, type, parent, errp, propname;
> > @@
> > -object_initialize(child, size, type);
> > -object_property_add_child(
> > +object_initialize_child(
> >                            parent, propname, 
> > -                          OBJECT(child),
> > +                          child, size, type,
> >                            errp);
> > 
> > Some of them (very few) already call object_unref() and need to
> > be fixed manually.
> > 
> > Most of the remaining ~50 object_initialize() callers are also
> > candidates, even if they don't call object_property_add_child()
> > today.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Care to turn this into a proper patch, now that we left the freeze period?

It's possible, but we need a volunteer to review each hunk
because the existing code might be (correctly) calling
object_unref() (either immediately or when parent is finalized).

I will keep this in my TODO list, but it's not my top priority
right now.

-- 
Eduardo

[Qemu-devel] [PATCH v2 02/16] hw/core/sysbus: Add a function for creating and attaching an object

[Thomas Huth]

A lot of functions are initializing an object and attach it immediately
afterwards to the system bus. Provide a common function for this, which
also uses object_initialize_child() to make sure that the reference
counter is correctly initialized to 1 afterwards.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/core/sysbus.c    | 8 ++++++++
 include/hw/sysbus.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index ecfb0cf..e2436ce 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -376,6 +376,14 @@ BusState *sysbus_get_default(void)
     return main_system_bus;
 }
 
+void sysbus_init_child_obj(Object *parent, const char *childname, void *child,
+                           size_t childsize, const char *childtype)
+{
+    object_initialize_child(parent, childname, child, childsize, childtype,
+                            &error_abort);
+    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
+}
+
 static void sysbus_register_types(void)
 {
     type_register_static(&system_bus_info);
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index e88bb6d..e405232 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -96,6 +96,9 @@ void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
                    MemoryRegion *mem);
 MemoryRegion *sysbus_address_space(SysBusDevice *dev);
 
+void sysbus_init_child_obj(Object *parent, const char *childname, void *child,
+                           size_t childsize, const char *childtype);
+
 /* Call func for every dynamically created sysbus device in the system */
 void foreach_dynamic_sysbus_device(FindSysbusDeviceFunc *func, void *opaque);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 02/16] hw/core/sysbus: Add a function for creating and attaching an object

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:30AM +0200, Thomas Huth wrote:
> A lot of functions are initializing an object and attach it immediately
> afterwards to the system bus. Provide a common function for this, which
> also uses object_initialize_child() to make sure that the reference
> counter is correctly initialized to 1 afterwards.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/core/sysbus.c    | 8 ++++++++
>  include/hw/sysbus.h | 3 +++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index ecfb0cf..e2436ce 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -376,6 +376,14 @@ BusState *sysbus_get_default(void)
>      return main_system_bus;
>  }
>  
> +void sysbus_init_child_obj(Object *parent, const char *childname, void *child,
> +                           size_t childsize, const char *childtype)
> +{
> +    object_initialize_child(parent, childname, child, childsize, childtype,
> +                            &error_abort);
> +    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
> +}
> +
>  static void sysbus_register_types(void)
>  {
>      type_register_static(&system_bus_info);
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index e88bb6d..e405232 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -96,6 +96,9 @@ void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
>                     MemoryRegion *mem);
>  MemoryRegion *sysbus_address_space(SysBusDevice *dev);
>  
> +void sysbus_init_child_obj(Object *parent, const char *childname, void *child,
> +                           size_t childsize, const char *childtype);
> +

Documentation about the reference ownership rules would be nice,
but I don't think this should block the bug fixes.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v2 03/16] hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported machines

[Thomas Huth]

When trying to "device_add bcm2837" on a machine that is not suitable for
this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":

echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
 "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp stdio

{"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{"return": {}}
{"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
 hotplugged on this machine"}}
Segmentation fault (core dumped)

The qdev_set_parent_bus() from instance_init adds a link to the child devices
which is not valid anymore after the bcm2837 instance has been destroyed.
Unfortunately, the child devices do not get destroyed / unlinked correctly
because both object_initialize() and object_property_add_child() increase
the reference count of the child objects by one, but only one reference
is dropped when the parent gets removed. So let's use the new functions
object_initialize_child() and sysbus_init_child_obj() instead to create
the objects, which will take care of creating the child objects with the
correct reference count of one.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/bcm2836.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 6805a7d..af97b2f 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -51,25 +51,19 @@ static void bcm2836_init(Object *obj)
     int n;
 
     for (n = 0; n < BCM283X_NCPUS; n++) {
-        object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
-                          info->cpu_type);
-        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
-                                  &error_abort);
+        object_initialize_child(obj, "cpu[*]", &s->cpus[n], sizeof(s->cpus[n]),
+                                info->cpu_type, &error_abort);
     }
 
-    object_initialize(&s->control, sizeof(s->control), TYPE_BCM2836_CONTROL);
-    object_property_add_child(obj, "control", OBJECT(&s->control), NULL);
-    qdev_set_parent_bus(DEVICE(&s->control), sysbus_get_default());
+    sysbus_init_child_obj(obj, "control", &s->control, sizeof(s->control),
+                          TYPE_BCM2836_CONTROL);
 
-    object_initialize(&s->peripherals, sizeof(s->peripherals),
-                      TYPE_BCM2835_PERIPHERALS);
-    object_property_add_child(obj, "peripherals", OBJECT(&s->peripherals),
-                              &error_abort);
+    sysbus_init_child_obj(obj, "peripherals", &s->peripherals,
+                          sizeof(s->peripherals), TYPE_BCM2835_PERIPHERALS);
     object_property_add_alias(obj, "board-rev", OBJECT(&s->peripherals),
                               "board-rev", &error_abort);
     object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals),
                               "vcram-size", &error_abort);
-    qdev_set_parent_bus(DEVICE(&s->peripherals), sysbus_get_default());
 }
 
 static void bcm2836_realize(DeviceState *dev, Error **errp)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 03/16] hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported machines

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:31AM +0200, Thomas Huth wrote:
> When trying to "device_add bcm2837" on a machine that is not suitable for
> this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":
> 
> echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
>  "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " \
>  "'arguments': {'command-line': 'info qtree'}}" | \
>  aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp stdio
> 
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
>  "package": "build-all"}, "capabilities": []}}
> {"return": {}}
> {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
>  hotplugged on this machine"}}
> Segmentation fault (core dumped)
> 
> The qdev_set_parent_bus() from instance_init adds a link to the child devices
> which is not valid anymore after the bcm2837 instance has been destroyed.
> Unfortunately, the child devices do not get destroyed / unlinked correctly
> because both object_initialize() and object_property_add_child() increase
> the reference count of the child objects by one, but only one reference
> is dropped when the parent gets removed. So let's use the new functions
> object_initialize_child() and sysbus_init_child_obj() instead to create
> the objects, which will take care of creating the child objects with the
> correct reference count of one.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

The usage of &error_abort in code that can be triggered from
device-list-properties still makes me nervous, but that's a
separate issue.


> ---
>  hw/arm/bcm2836.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 6805a7d..af97b2f 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -51,25 +51,19 @@ static void bcm2836_init(Object *obj)
>      int n;
>  
>      for (n = 0; n < BCM283X_NCPUS; n++) {
> -        object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
> -                          info->cpu_type);
> -        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
> -                                  &error_abort);
> +        object_initialize_child(obj, "cpu[*]", &s->cpus[n], sizeof(s->cpus[n]),
> +                                info->cpu_type, &error_abort);
>      }
>  
> -    object_initialize(&s->control, sizeof(s->control), TYPE_BCM2836_CONTROL);
> -    object_property_add_child(obj, "control", OBJECT(&s->control), NULL);
> -    qdev_set_parent_bus(DEVICE(&s->control), sysbus_get_default());
> +    sysbus_init_child_obj(obj, "control", &s->control, sizeof(s->control),
> +                          TYPE_BCM2836_CONTROL);
>  
> -    object_initialize(&s->peripherals, sizeof(s->peripherals),
> -                      TYPE_BCM2835_PERIPHERALS);
> -    object_property_add_child(obj, "peripherals", OBJECT(&s->peripherals),
> -                              &error_abort);
> +    sysbus_init_child_obj(obj, "peripherals", &s->peripherals,
> +                          sizeof(s->peripherals), TYPE_BCM2835_PERIPHERALS);
>      object_property_add_alias(obj, "board-rev", OBJECT(&s->peripherals),
>                                "board-rev", &error_abort);
>      object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals),
>                                "vcram-size", &error_abort);
> -    qdev_set_parent_bus(DEVICE(&s->peripherals), sysbus_get_default());
>  }
>  
>  static void bcm2836_realize(DeviceState *dev, Error **errp)
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 03/16] hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported machines

[Thomas Huth]

On 13.07.2018 23:26, Eduardo Habkost wrote:
> On Fri, Jul 13, 2018 at 10:27:31AM +0200, Thomas Huth wrote:
>> When trying to "device_add bcm2837" on a machine that is not suitable for
>> this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":
>>
>> echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
>>  "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " \
>>  "'arguments': {'command-line': 'info qtree'}}" | \
>>  aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp stdio
>>
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
>>  "package": "build-all"}, "capabilities": []}}
>> {"return": {}}
>> {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
>>  hotplugged on this machine"}}
>> Segmentation fault (core dumped)
>>
>> The qdev_set_parent_bus() from instance_init adds a link to the child devices
>> which is not valid anymore after the bcm2837 instance has been destroyed.
>> Unfortunately, the child devices do not get destroyed / unlinked correctly
>> because both object_initialize() and object_property_add_child() increase
>> the reference count of the child objects by one, but only one reference
>> is dropped when the parent gets removed. So let's use the new functions
>> object_initialize_child() and sysbus_init_child_obj() instead to create
>> the objects, which will take care of creating the child objects with the
>> correct reference count of one.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> The usage of &error_abort in code that can be triggered from
> device-list-properties still makes me nervous, but that's a
> separate issue.

I first had similar thoughts, but I think it's a clear coding issue if
the abort triggers here (and not something that the user should normally
be able to trigger somehow), so error_abort should be ok in this case.

 Thomas

Re: [Qemu-devel] [PATCH v2 03/16] hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported machines

[Eduardo Habkost]

On Mon, Jul 16, 2018 at 09:09:13AM +0200, Thomas Huth wrote:
> On 13.07.2018 23:26, Eduardo Habkost wrote:
> > On Fri, Jul 13, 2018 at 10:27:31AM +0200, Thomas Huth wrote:
> >> When trying to "device_add bcm2837" on a machine that is not suitable for
> >> this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":
> >>
> >> echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
> >>  "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " \
> >>  "'arguments': {'command-line': 'info qtree'}}" | \
> >>  aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp stdio
> >>
> >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
> >>  "package": "build-all"}, "capabilities": []}}
> >> {"return": {}}
> >> {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
> >>  hotplugged on this machine"}}
> >> Segmentation fault (core dumped)
> >>
> >> The qdev_set_parent_bus() from instance_init adds a link to the child devices
> >> which is not valid anymore after the bcm2837 instance has been destroyed.
> >> Unfortunately, the child devices do not get destroyed / unlinked correctly
> >> because both object_initialize() and object_property_add_child() increase
> >> the reference count of the child objects by one, but only one reference
> >> is dropped when the parent gets removed. So let's use the new functions
> >> object_initialize_child() and sysbus_init_child_obj() instead to create
> >> the objects, which will take care of creating the child objects with the
> >> correct reference count of one.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > The usage of &error_abort in code that can be triggered from
> > device-list-properties still makes me nervous, but that's a
> > separate issue.
> 
> I first had similar thoughts, but I think it's a clear coding issue if
> the abort triggers here (and not something that the user should normally
> be able to trigger somehow), so error_abort should be ok in this case.

Agreed.  We don't really have an option, as instance_init can't
report errors.

-- 
Eduardo

[Qemu-devel] [PATCH v2 04/16] hw/arm/armv7: Fix crash when introspecting the "iotkit" device

[Thomas Huth]

QEMU currently crashes when introspecting the "iotkit" device and
runnint "info qtree" afterwards, e.g. when running QEMU like this:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'iotkit'}}" "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio

Use the new functions object_initialize_child() and sysbus_init_child_obj()
to make sure that all objects get cleaned up correctly when the instances
are destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/armv7m.c       |  7 +++--
 hw/arm/iotkit.c       | 74 ++++++++++++++++++++++-----------------------------
 hw/intc/armv7m_nvic.c |  5 ++--
 3 files changed, 37 insertions(+), 49 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 9e00d40..6b07666 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -134,14 +134,13 @@ static void armv7m_instance_init(Object *obj)
 
     memory_region_init(&s->container, obj, "armv7m-container", UINT64_MAX);
 
-    object_initialize(&s->nvic, sizeof(s->nvic), TYPE_NVIC);
-    qdev_set_parent_bus(DEVICE(&s->nvic), sysbus_get_default());
+    sysbus_init_child_obj(obj, "nvnic", &s->nvic, sizeof(s->nvic), TYPE_NVIC);
     object_property_add_alias(obj, "num-irq",
                               OBJECT(&s->nvic), "num-irq", &error_abort);
 
     for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
-        object_initialize(&s->bitband[i], sizeof(s->bitband[i]), TYPE_BITBAND);
-        qdev_set_parent_bus(DEVICE(&s->bitband[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "bitband[*]", &s->bitband[i],
+                              sizeof(s->bitband[i]), TYPE_BITBAND);
     }
 }
 
diff --git a/hw/arm/iotkit.c b/hw/arm/iotkit.c
index 133d5bb..d758385 100644
--- a/hw/arm/iotkit.c
+++ b/hw/arm/iotkit.c
@@ -30,15 +30,6 @@ static void make_alias(IoTKit *s, MemoryRegion *mr, const char *name,
     memory_region_add_subregion_overlap(&s->container, base, mr, -1500);
 }
 
-static void init_sysbus_child(Object *parent, const char *childname,
-                              void *child, size_t childsize,
-                              const char *childtype)
-{
-    object_initialize(child, childsize, childtype);
-    object_property_add_child(parent, childname, OBJECT(child), &error_abort);
-    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
-}
-
 static void irq_status_forwarder(void *opaque, int n, int level)
 {
     qemu_irq destirq = opaque;
@@ -119,53 +110,52 @@ static void iotkit_init(Object *obj)
 
     memory_region_init(&s->container, obj, "iotkit-container", UINT64_MAX);
 
-    init_sysbus_child(obj, "armv7m", &s->armv7m, sizeof(s->armv7m),
-                      TYPE_ARMV7M);
+    sysbus_init_child_obj(obj, "armv7m", &s->armv7m, sizeof(s->armv7m),
+                          TYPE_ARMV7M);
     qdev_prop_set_string(DEVICE(&s->armv7m), "cpu-type",
                          ARM_CPU_TYPE_NAME("cortex-m33"));
 
-    init_sysbus_child(obj, "secctl", &s->secctl, sizeof(s->secctl),
-                      TYPE_IOTKIT_SECCTL);
-    init_sysbus_child(obj, "apb-ppc0", &s->apb_ppc0, sizeof(s->apb_ppc0),
-                      TYPE_TZ_PPC);
-    init_sysbus_child(obj, "apb-ppc1", &s->apb_ppc1, sizeof(s->apb_ppc1),
-                      TYPE_TZ_PPC);
-    init_sysbus_child(obj, "mpc", &s->mpc, sizeof(s->mpc), TYPE_TZ_MPC);
-    object_initialize(&s->mpc_irq_orgate, sizeof(s->mpc_irq_orgate),
-                      TYPE_OR_IRQ);
-    object_property_add_child(obj, "mpc-irq-orgate",
-                              OBJECT(&s->mpc_irq_orgate), &error_abort);
+    sysbus_init_child_obj(obj, "secctl", &s->secctl, sizeof(s->secctl),
+                          TYPE_IOTKIT_SECCTL);
+    sysbus_init_child_obj(obj, "apb-ppc0", &s->apb_ppc0, sizeof(s->apb_ppc0),
+                          TYPE_TZ_PPC);
+    sysbus_init_child_obj(obj, "apb-ppc1", &s->apb_ppc1, sizeof(s->apb_ppc1),
+                          TYPE_TZ_PPC);
+    sysbus_init_child_obj(obj, "mpc", &s->mpc, sizeof(s->mpc), TYPE_TZ_MPC);
+    object_initialize_child(obj, "mpc-irq-orgate", &s->mpc_irq_orgate,
+                            sizeof(s->mpc_irq_orgate), TYPE_OR_IRQ,
+                            &error_abort);
+
     for (i = 0; i < ARRAY_SIZE(s->mpc_irq_splitter); i++) {
         char *name = g_strdup_printf("mpc-irq-splitter-%d", i);
         SplitIRQ *splitter = &s->mpc_irq_splitter[i];
 
-        object_initialize(splitter, sizeof(*splitter), TYPE_SPLIT_IRQ);
-        object_property_add_child(obj, name, OBJECT(splitter), &error_abort);
+        object_initialize_child(obj, name, splitter, sizeof(*splitter),
+                                TYPE_SPLIT_IRQ, &error_abort);
         g_free(name);
     }
-    init_sysbus_child(obj, "timer0", &s->timer0, sizeof(s->timer0),
-                      TYPE_CMSDK_APB_TIMER);
-    init_sysbus_child(obj, "timer1", &s->timer1, sizeof(s->timer1),
-                      TYPE_CMSDK_APB_TIMER);
-    init_sysbus_child(obj, "dualtimer", &s->dualtimer, sizeof(s->dualtimer),
-                      TYPE_UNIMPLEMENTED_DEVICE);
-    object_initialize(&s->ppc_irq_orgate, sizeof(s->ppc_irq_orgate),
-                      TYPE_OR_IRQ);
-    object_property_add_child(obj, "ppc-irq-orgate",
-                              OBJECT(&s->ppc_irq_orgate), &error_abort);
-    object_initialize(&s->sec_resp_splitter, sizeof(s->sec_resp_splitter),
-                      TYPE_SPLIT_IRQ);
-    object_property_add_child(obj, "sec-resp-splitter",
-                              OBJECT(&s->sec_resp_splitter), &error_abort);
+    sysbus_init_child_obj(obj, "timer0", &s->timer0, sizeof(s->timer0),
+                          TYPE_CMSDK_APB_TIMER);
+    sysbus_init_child_obj(obj, "timer1", &s->timer1, sizeof(s->timer1),
+                          TYPE_CMSDK_APB_TIMER);
+    sysbus_init_child_obj(obj, "dualtimer", &s->dualtimer, sizeof(s->dualtimer),
+                          TYPE_UNIMPLEMENTED_DEVICE);
+    object_initialize_child(obj, "ppc-irq-orgate", &s->ppc_irq_orgate,
+                            sizeof(s->ppc_irq_orgate), TYPE_OR_IRQ,
+                            &error_abort);
+    object_initialize_child(obj, "sec-resp-splitter", &s->sec_resp_splitter,
+                            sizeof(s->sec_resp_splitter), TYPE_SPLIT_IRQ,
+                            &error_abort);
     for (i = 0; i < ARRAY_SIZE(s->ppc_irq_splitter); i++) {
         char *name = g_strdup_printf("ppc-irq-splitter-%d", i);
         SplitIRQ *splitter = &s->ppc_irq_splitter[i];
 
-        object_initialize(splitter, sizeof(*splitter), TYPE_SPLIT_IRQ);
-        object_property_add_child(obj, name, OBJECT(splitter), &error_abort);
+        object_initialize_child(obj, name, splitter, sizeof(*splitter),
+                                TYPE_SPLIT_IRQ, &error_abort);
+        g_free(name);
     }
-    init_sysbus_child(obj, "s32ktimer", &s->s32ktimer, sizeof(s->s32ktimer),
-                      TYPE_UNIMPLEMENTED_DEVICE);
+    sysbus_init_child_obj(obj, "s32ktimer", &s->s32ktimer, sizeof(s->s32ktimer),
+                          TYPE_UNIMPLEMENTED_DEVICE);
 }
 
 static void iotkit_exp_irq(void *opaque, int n, int level)
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 661be88..7a5330f 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -2296,9 +2296,8 @@ static void armv7m_nvic_instance_init(Object *obj)
     NVICState *nvic = NVIC(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    object_initialize(&nvic->systick[M_REG_NS],
-                      sizeof(nvic->systick[M_REG_NS]), TYPE_SYSTICK);
-    qdev_set_parent_bus(DEVICE(&nvic->systick[M_REG_NS]), sysbus_get_default());
+    sysbus_init_child_obj(obj, "systick-reg-ns", &nvic->systick[M_REG_NS],
+                          sizeof(nvic->systick[M_REG_NS]), TYPE_SYSTICK);
     /* We can't initialize the secure systick here, as we don't know
      * yet if we need it.
      */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 04/16] hw/arm/armv7: Fix crash when introspecting the "iotkit" device

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:32AM +0200, Thomas Huth wrote:
> QEMU currently crashes when introspecting the "iotkit" device and
> runnint "info qtree" afterwards, e.g. when running QEMU like this:
> 
> echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
>  "'arguments':{'typename':'iotkit'}}" "{'execute': 'human-monitor-command', " \
>  "'arguments': {'command-line': 'info qtree'}}" | \
>  aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
> 
> Use the new functions object_initialize_child() and sysbus_init_child_obj()
> to make sure that all objects get cleaned up correctly when the instances
> are destroyed.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v2 05/16] hw/cpu/a15mpcore: Fix introspection problem with the a15mpcore_priv device

[Thomas Huth]

There is a memory management problem when introspecting the a15mpcore_priv
device. It can be seen with valgrind when running QEMU like this:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'a15mpcore_priv'}}"\
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}"  | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{"return": {}}
{"return": [{"name": "num-cpu", "type": "uint32"}, {"name": "num-irq",
 "type": "uint32"}, {"name": "a15mp-priv-container[0]", "type":
  "child<qemu:memory-region>"}]}
==24978== Invalid read of size 8
==24978==    at 0x618EBA: qdev_print (qdev-monitor.c:686)
==24978==    by 0x618EBA: qbus_print (qdev-monitor.c:719)
[...]

Use the new sysbus_init_child_obj() function to make sure that we get
the reference counting of the child objects right.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/cpu/a15mpcore.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index bc05152..43c1079 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -35,15 +35,13 @@ static void a15mp_priv_initfn(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     A15MPPrivState *s = A15MPCORE_PRIV(obj);
-    DeviceState *gicdev;
 
     memory_region_init(&s->container, obj, "a15mp-priv-container", 0x8000);
     sysbus_init_mmio(sbd, &s->container);
 
-    object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
-    gicdev = DEVICE(&s->gic);
-    qdev_set_parent_bus(gicdev, sysbus_get_default());
-    qdev_prop_set_uint32(gicdev, "revision", 2);
+    sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic),
+                          gic_class_name());
+    qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
 }
 
 static void a15mp_priv_realize(DeviceState *dev, Error **errp)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 05/16] hw/cpu/a15mpcore: Fix introspection problem with the a15mpcore_priv device

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:33AM +0200, Thomas Huth wrote:
> There is a memory management problem when introspecting the a15mpcore_priv
> device. It can be seen with valgrind when running QEMU like this:
> 
> echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
>  "'arguments':{'typename':'a15mpcore_priv'}}"\
>  "{'execute': 'human-monitor-command', " \
>  "'arguments': {'command-line': 'info qtree'}}"  | \
>  valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
>  "package": "build-all"}, "capabilities": []}}
> {"return": {}}
> {"return": [{"name": "num-cpu", "type": "uint32"}, {"name": "num-irq",
>  "type": "uint32"}, {"name": "a15mp-priv-container[0]", "type":
>   "child<qemu:memory-region>"}]}
> ==24978== Invalid read of size 8
> ==24978==    at 0x618EBA: qdev_print (qdev-monitor.c:686)
> ==24978==    by 0x618EBA: qbus_print (qdev-monitor.c:719)
> [...]
> 
> Use the new sysbus_init_child_obj() function to make sure that we get
> the reference counting of the child objects right.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/cpu/a15mpcore.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
> index bc05152..43c1079 100644
> --- a/hw/cpu/a15mpcore.c
> +++ b/hw/cpu/a15mpcore.c
> @@ -35,15 +35,13 @@ static void a15mp_priv_initfn(Object *obj)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      A15MPPrivState *s = A15MPCORE_PRIV(obj);
> -    DeviceState *gicdev;
>  
>      memory_region_init(&s->container, obj, "a15mp-priv-container", 0x8000);
>      sysbus_init_mmio(sbd, &s->container);
>  
> -    object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
> -    gicdev = DEVICE(&s->gic);
> -    qdev_set_parent_bus(gicdev, sysbus_get_default());
> -    qdev_prop_set_uint32(gicdev, "revision", 2);
> +    sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic),
> +                          gic_class_name());
> +    qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);

I assume qdev_set_parent_bus() won't trigger any code that looks
at "revision", so the prop_set/set_parent_bus ordering change
won't matter.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v2 06/16] hw/display/xlnx_dp: Move problematic code from instance_init to realize

[Thomas Huth]

aux_create_slave() calls qdev_init_nofail() which in turn "realizes"
the corresponding object. Thus this most not be called from an
instance_init function. Move the code to the realize function instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/display/xlnx_dp.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 5130122..c7542d8 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1225,28 +1225,23 @@ static void xlnx_dp_init(Object *obj)
                              xlnx_dp_set_dpdma,
                              OBJ_PROP_LINK_STRONG,
                              &error_abort);
+}
 
-    /*
-     * Initialize AUX Bus.
-     */
-    s->aux_bus = aux_init_bus(DEVICE(obj), "aux");
+static void xlnx_dp_realize(DeviceState *dev, Error **errp)
+{
+    XlnxDPState *s = XLNX_DP(dev);
+    DisplaySurface *surface;
+    struct audsettings as;
 
-    /*
-     * Initialize DPCD and EDID..
-     */
+    s->aux_bus = aux_init_bus(dev, "aux");
+
+    /* Initialize DPCD and EDID */
     s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd", 0x00000));
     s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc"));
     i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50);
 
     fifo8_create(&s->rx_fifo, 16);
     fifo8_create(&s->tx_fifo, 16);
-}
-
-static void xlnx_dp_realize(DeviceState *dev, Error **errp)
-{
-    XlnxDPState *s = XLNX_DP(dev);
-    DisplaySurface *surface;
-    struct audsettings as;
 
     s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
     surface = qemu_console_surface(s->console);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 06/16] hw/display/xlnx_dp: Move problematic code from instance_init to realize

[Paolo Bonzini]

On 13/07/2018 10:27, Thomas Huth wrote:
> aux_create_slave() calls qdev_init_nofail() which in turn "realizes"
> the corresponding object. Thus this most not be called from an
> instance_init function. Move the code to the realize function instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/display/xlnx_dp.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)

> +    s->aux_bus = aux_init_bus(dev, "aux");

aux_init_bus can remain in the same place, and likewise the qdev_create
that assigns to s->edid.

The only thing that has to move is the qdev_init_nofail and
aux_bus_map_device, like this:

----------------- 8< ------------------
From: Paolo Bonzini <pbonzinI@redhat.com>
Subject: [PATCH] hw/display/xlnx_dp: Move problematic code from instance_init to realize

aux_create_slave() calls qdev_init_nofail() which in turn "realizes"
the corresponding object. This is unlike qdev_create(), and it is wrong
because qdev_init_nofail() must not be called from an instance_init
function.  Move qdev_init_nofail() and the subsequent aux_map_slave into
the caller's realize function.
 
Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 51301220e8..589ef59dfd 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1234,7 +1234,7 @@ static void xlnx_dp_init(Object *obj)
     /*
      * Initialize DPCD and EDID..
      */
-    s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd", 0x00000));
+    s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd"));
     s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc"));
     i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50);
 
@@ -1248,6 +1248,9 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
     DisplaySurface *surface;
     struct audsettings as;
 
+    qdev_init_nofail(DEVICE(s->dpcd));
+    aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000);
+
     s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
     surface = qemu_console_surface(s->console);
     xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL,
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index b8a8721201..2fe807d42f 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -74,9 +74,11 @@ AUXBus *aux_init_bus(DeviceState *parent, const char *name)
     return bus;
 }
 
-static void aux_bus_map_device(AUXBus *bus, AUXSlave *dev, hwaddr addr)
+void aux_map_slave(AUXSlave *aux_dev, hwaddr addr)
 {
-    memory_region_add_subregion(bus->aux_io, addr, dev->mmio);
+    DeviceState *dev = DEVICE(aux_dev);
+    AUXBus *bus = AUX_BUS(qdev_get_parent_bus(dev));
+    memory_region_add_subregion(bus->aux_io, addr, aux_dev->mmio);
 }
 
 static bool aux_bus_is_bridge(AUXBus *bus, DeviceState *dev)
@@ -260,15 +262,13 @@ static void aux_slave_dev_print(Monitor *mon, DeviceState *dev, int indent)
                    memory_region_size(s->mmio));
 }
 
-DeviceState *aux_create_slave(AUXBus *bus, const char *type, uint32_t addr)
+DeviceState *aux_create_slave(AUXBus *bus, const char *type)
 {
     DeviceState *dev;
 
     dev = DEVICE(object_new(type));
     assert(dev);
     qdev_set_parent_bus(dev, &bus->qbus);
-    qdev_init_nofail(dev);
-    aux_bus_map_device(AUX_BUS(qdev_get_parent_bus(dev)), AUX_SLAVE(dev), addr);
     return dev;
 }
 
diff --git a/include/hw/misc/auxbus.h b/include/hw/misc/auxbus.h
index 68ade8a90f..c15b444748 100644
--- a/include/hw/misc/auxbus.h
+++ b/include/hw/misc/auxbus.h
@@ -123,6 +123,18 @@ I2CBus *aux_get_i2c_bus(AUXBus *bus);
  */
 void aux_init_mmio(AUXSlave *aux_slave, MemoryRegion *mmio);
 
-DeviceState *aux_create_slave(AUXBus *bus, const char *name, uint32_t addr);
+/* aux_create_slave: Create a new device on an AUX bus
+ *
+ * @bus The AUX bus for the new device.
+ * @name The type of the device to be created.
+ */
+DeviceState *aux_create_slave(AUXBus *bus, const char *name);
+
+/* aux_map_slave: Map the mmio for an AUX slave on the bus.
+ *
+ * @dev The AUX slave.
+ * @addr The address for the slave's mmio.
+ */
+void aux_map_slave(AUXSlave *dev, hwaddr addr);
 
 #endif /* HW_MISC_AUXBUS_H */


Paolo

> +    /* Initialize DPCD and EDID */
>      s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd", 0x00000));
>      s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc"));
>      i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50);
>  
>      fifo8_create(&s->rx_fifo, 16);
>      fifo8_create(&s->tx_fifo, 16);
> -}
> -
> -static void xlnx_dp_realize(DeviceState *dev, Error **errp)
> -{
> -    XlnxDPState *s = XLNX_DP(dev);
> -    DisplaySurface *surface;
> -    struct audsettings as;
>  
>      s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s);
>      surface = qemu_console_surface(s->console);
> 

Re: [Qemu-devel] [PATCH v2 06/16] hw/display/xlnx_dp: Move problematic code from instance_init to realize

[Thomas Huth]

On 13.07.2018 13:13, Paolo Bonzini wrote:
> On 13/07/2018 10:27, Thomas Huth wrote:
>> aux_create_slave() calls qdev_init_nofail() which in turn "realizes"
>> the corresponding object. Thus this most not be called from an
>> instance_init function. Move the code to the realize function instead.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/display/xlnx_dp.c | 23 +++++++++--------------
>>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
>> +    s->aux_bus = aux_init_bus(dev, "aux");
> 
> aux_init_bus can remain in the same place, and likewise the qdev_create
> that assigns to s->edid.
> 
> The only thing that has to move is the qdev_init_nofail and
> aux_bus_map_device, like this:
> 
> ----------------- 8< ------------------
> From: Paolo Bonzini <pbonzinI@redhat.com>
> Subject: [PATCH] hw/display/xlnx_dp: Move problematic code from instance_init to realize

Your patch looks good at a first quick glance, but it seems not to work as expected: When I now run QEMU like this:

echo "{'execute':'qmp_capabilities'}" \
 "{'execute':'device-list-properties'," \
 "'arguments':{'typename':'xlnx,zynqmp'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio

then QEMU ends up in an endless loop and I've got to kill it.

 Thomas

Re: [Qemu-devel] [PATCH v2 06/16] hw/display/xlnx_dp: Move problematic code from instance_init to realize

[Paolo Bonzini]

On 13/07/2018 17:59, Thomas Huth wrote:
> Your patch looks good at a first quick glance, but it seems not to work as expected: When I now run QEMU like this:
> 
> echo "{'execute':'qmp_capabilities'}" \
>  "{'execute':'device-list-properties'," \
>  "'arguments':{'typename':'xlnx,zynqmp'}}" \
>  "{'execute': 'human-monitor-command', " \
>  "'arguments': {'command-line': 'info qtree'}}" | \
>  aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
> 
> then QEMU ends up in an endless loop and I've got to kill it.

There are two more bugs that my patch makes un-latent, where the
objects are created but not added as children.  Therefore when
you call object_unparent on them, nothing happens.

In particular dpcd and edid give you an infinite loop in bus_unparent,
because device_unparent is not called and does not remove them from
the list of devices on the bus.

The following incremental changes fix everything for me.  Note that
aux_create_slave/qdev_create already do the unref for you.

Thanks,

Paolo

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 589ef59dfd..6439bd05ef 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1235,8 +1235,11 @@ static void xlnx_dp_init(Object *obj)
      * Initialize DPCD and EDID..
      */
     s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd"));
+    object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd), NULL);
+
     s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc"));
     i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50);
+    object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid), NULL);
 
     fifo8_create(&s->rx_fifo, 16);
     fifo8_create(&s->tx_fifo, 16);
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 2fe807d42f..0e56d9a8a4 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -32,6 +32,7 @@
 #include "hw/misc/auxbus.h"
 #include "hw/i2c/i2c.h"
 #include "monitor/monitor.h"
+#include "qapi/error.h"
 
 #ifndef DEBUG_AUX
 #define DEBUG_AUX 0
@@ -63,9 +64,14 @@ static void aux_bus_class_init(ObjectClass *klass, void *data)
 AUXBus *aux_init_bus(DeviceState *parent, const char *name)
 {
     AUXBus *bus;
+    Object *auxtoi2c;
 
     bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name));
-    bus->bridge = AUXTOI2C(qdev_create(BUS(bus), TYPE_AUXTOI2C));
+    auxtoi2c = object_new_with_props(TYPE_AUXTOI2C, OBJECT(bus), "i2c",
+                                     &error_abort, NULL);
+    qdev_set_parent_bus(DEVICE(auxtoi2c), BUS(bus));
+
+    bus->bridge = AUXTOI2C(auxtoi2c);
 
     /* Memory related. */
     bus->aux_io = g_malloc(sizeof(*bus->aux_io));

Re: [Qemu-devel] [PATCH v2 06/16] hw/display/xlnx_dp: Move problematic code from instance_init to realize

[Thomas Huth]

On 13.07.2018 19:13, Paolo Bonzini wrote:
> On 13/07/2018 17:59, Thomas Huth wrote:
>> Your patch looks good at a first quick glance, but it seems not to work as expected: When I now run QEMU like this:
>>
>> echo "{'execute':'qmp_capabilities'}" \
>>  "{'execute':'device-list-properties'," \
>>  "'arguments':{'typename':'xlnx,zynqmp'}}" \
>>  "{'execute': 'human-monitor-command', " \
>>  "'arguments': {'command-line': 'info qtree'}}" | \
>>  aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
>>
>> then QEMU ends up in an endless loop and I've got to kill it.
> 
> There are two more bugs that my patch makes un-latent, where the
> objects are created but not added as children.  Therefore when
> you call object_unparent on them, nothing happens.
> 
> In particular dpcd and edid give you an infinite loop in bus_unparent,
> because device_unparent is not called and does not remove them from
> the list of devices on the bus.
> 
> The following incremental changes fix everything for me.  Note that
> aux_create_slave/qdev_create already do the unref for you.

Thanks, that fixes the problem, indeed. I'll squash this into your patch
and send out a v3 series.

 Thomas

[Qemu-devel] [PATCH v2 07/16] hw/arm/xlnx-zynqmp: Fix crash when introspecting the "xlnx, zynqmp" device

[Thomas Huth]

QEMU currently crashes when e.g. doing something like this:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'xlnx,zynqmp'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" \
 |  aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio

Use the new object_initialize_child() and sysbus_init_child_obj()
functions to get the refernce counting of the child objects right, so
that they are properly cleaned up when the parent gets destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/xlnx-zynqmp.c | 61 ++++++++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 33 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 29df35f..a1365b4 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -166,64 +166,59 @@ static void xlnx_zynqmp_init(Object *obj)
     int num_apus = MIN(smp_cpus, XLNX_ZYNQMP_NUM_APU_CPUS);
 
     for (i = 0; i < num_apus; i++) {
-        object_initialize(&s->apu_cpu[i], sizeof(s->apu_cpu[i]),
-                          "cortex-a53-" TYPE_ARM_CPU);
-        object_property_add_child(obj, "apu-cpu[*]", OBJECT(&s->apu_cpu[i]),
-                                  &error_abort);
+        object_initialize_child(obj, "apu-cpu[*]", &s->apu_cpu[i],
+                                sizeof(s->apu_cpu[i]),
+                                "cortex-a53-" TYPE_ARM_CPU, &error_abort);
     }
 
-    object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
-    qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
+    sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic),
+                          gic_class_name());
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) {
-        object_initialize(&s->gem[i], sizeof(s->gem[i]), TYPE_CADENCE_GEM);
-        qdev_set_parent_bus(DEVICE(&s->gem[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "gem[*]", &s->gem[i], sizeof(s->gem[i]),
+                              TYPE_CADENCE_GEM);
     }
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_UARTS; i++) {
-        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_CADENCE_UART);
-        qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "uart[*]", &s->uart[i], sizeof(s->uart[i]),
+                              TYPE_CADENCE_UART);
     }
 
-    object_initialize(&s->sata, sizeof(s->sata), TYPE_SYSBUS_AHCI);
-    qdev_set_parent_bus(DEVICE(&s->sata), sysbus_get_default());
+    sysbus_init_child_obj(obj, "sata", &s->sata, sizeof(s->sata),
+                          TYPE_SYSBUS_AHCI);
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
-        object_initialize(&s->sdhci[i], sizeof(s->sdhci[i]),
-                          TYPE_SYSBUS_SDHCI);
-        qdev_set_parent_bus(DEVICE(&s->sdhci[i]),
-                            sysbus_get_default());
+        sysbus_init_child_obj(obj, "sdhci[*]", &s->sdhci[i],
+                              sizeof(s->sdhci[i]), TYPE_SYSBUS_SDHCI);
     }
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_SPIS; i++) {
-        object_initialize(&s->spi[i], sizeof(s->spi[i]),
-                          TYPE_XILINX_SPIPS);
-        qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "spi[*]", &s->spi[i], sizeof(s->spi[i]),
+                              TYPE_XILINX_SPIPS);
     }
 
-    object_initialize(&s->qspi, sizeof(s->qspi), TYPE_XLNX_ZYNQMP_QSPIPS);
-    qdev_set_parent_bus(DEVICE(&s->qspi), sysbus_get_default());
+    sysbus_init_child_obj(obj, "qspi", &s->qspi, sizeof(s->qspi),
+                          TYPE_XLNX_ZYNQMP_QSPIPS);
 
-    object_initialize(&s->dp, sizeof(s->dp), TYPE_XLNX_DP);
-    qdev_set_parent_bus(DEVICE(&s->dp), sysbus_get_default());
+    sysbus_init_child_obj(obj, "xxxdp", &s->dp, sizeof(s->dp), TYPE_XLNX_DP);
 
-    object_initialize(&s->dpdma, sizeof(s->dpdma), TYPE_XLNX_DPDMA);
-    qdev_set_parent_bus(DEVICE(&s->dpdma), sysbus_get_default());
+    sysbus_init_child_obj(obj, "dp-dma", &s->dpdma, sizeof(s->dpdma),
+                          TYPE_XLNX_DPDMA);
 
-    object_initialize(&s->ipi, sizeof(s->ipi), TYPE_XLNX_ZYNQMP_IPI);
-    qdev_set_parent_bus(DEVICE(&s->ipi), sysbus_get_default());
+    sysbus_init_child_obj(obj, "ipi", &s->ipi, sizeof(s->ipi),
+                          TYPE_XLNX_ZYNQMP_IPI);
 
-    object_initialize(&s->rtc, sizeof(s->rtc), TYPE_XLNX_ZYNQMP_RTC);
-    qdev_set_parent_bus(DEVICE(&s->rtc), sysbus_get_default());
+    sysbus_init_child_obj(obj, "rtc", &s->rtc, sizeof(s->rtc),
+                          TYPE_XLNX_ZYNQMP_RTC);
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_GDMA_CH; i++) {
-        object_initialize(&s->gdma[i], sizeof(s->gdma[i]), TYPE_XLNX_ZDMA);
-        qdev_set_parent_bus(DEVICE(&s->gdma[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "gdma[*]", &s->gdma[i], sizeof(s->gdma[i]),
+                              TYPE_XLNX_ZDMA);
     }
 
     for (i = 0; i < XLNX_ZYNQMP_NUM_ADMA_CH; i++) {
-        object_initialize(&s->adma[i], sizeof(s->adma[i]), TYPE_XLNX_ZDMA);
-        qdev_set_parent_bus(DEVICE(&s->adma[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "adma[*]", &s->adma[i], sizeof(s->adma[i]),
+                              TYPE_XLNX_ZDMA);
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 07/16] hw/arm/xlnx-zynqmp: Fix crash when introspecting the "xlnx, zynqmp" device

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:35AM +0200, Thomas Huth wrote:
> QEMU currently crashes when e.g. doing something like this:
> 
> echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
>  "'arguments':{'typename':'xlnx,zynqmp'}}" \
>  "{'execute': 'human-monitor-command', " \
>  "'arguments': {'command-line': 'info qtree'}}" \
>  |  aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
> 
> Use the new object_initialize_child() and sysbus_init_child_obj()
> functions to get the refernce counting of the child objects right, so
> that they are properly cleaned up when the parent gets destroyed.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v2 08/16] hw/arm/msf2-soc: Fix introspection problem with the "msf2-soc" device

[Thomas Huth]

Valgrind currently reports a problem when running QEMU like this:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'msf2-soc'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==23097== Invalid read of size 8
==23097==    at 0x6192AA: qdev_print (qdev-monitor.c:686)
==23097==    by 0x6192AA: qbus_print (qdev-monitor.c:719)
[...]

Use the new sysbus_init_child_obj() function to make sure that the child
objects are cleaned up correctly when the parent gets destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/msf2-soc.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index edb3ba8..dbefade 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -68,19 +68,18 @@ static void m2sxxx_soc_initfn(Object *obj)
     MSF2State *s = MSF2_SOC(obj);
     int i;
 
-    object_initialize(&s->armv7m, sizeof(s->armv7m), TYPE_ARMV7M);
-    qdev_set_parent_bus(DEVICE(&s->armv7m), sysbus_get_default());
+    sysbus_init_child_obj(obj, "armv7m", &s->armv7m, sizeof(s->armv7m),
+                          TYPE_ARMV7M);
 
-    object_initialize(&s->sysreg, sizeof(s->sysreg), TYPE_MSF2_SYSREG);
-    qdev_set_parent_bus(DEVICE(&s->sysreg), sysbus_get_default());
+    sysbus_init_child_obj(obj, "sysreg", &s->sysreg, sizeof(s->sysreg),
+                          TYPE_MSF2_SYSREG);
 
-    object_initialize(&s->timer, sizeof(s->timer), TYPE_MSS_TIMER);
-    qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
+    sysbus_init_child_obj(obj, "timer", &s->timer, sizeof(s->timer),
+                          TYPE_MSS_TIMER);
 
     for (i = 0; i < MSF2_NUM_SPIS; i++) {
-        object_initialize(&s->spi[i], sizeof(s->spi[i]),
+        sysbus_init_child_obj(obj, "spi[*]", &s->spi[i], sizeof(s->spi[i]),
                           TYPE_MSS_SPI);
-        qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 08/16] hw/arm/msf2-soc: Fix introspection problem with the "msf2-soc" device

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:36AM +0200, Thomas Huth wrote:
> Valgrind currently reports a problem when running QEMU like this:
> 
> echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
>  "'arguments':{'typename':'msf2-soc'}}" \
>  "{'execute': 'human-monitor-command', " \
>  "'arguments': {'command-line': 'info qtree'}}" | \
>  valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
> [...]
> ==23097== Invalid read of size 8
> ==23097==    at 0x6192AA: qdev_print (qdev-monitor.c:686)
> ==23097==    by 0x6192AA: qbus_print (qdev-monitor.c:719)
> [...]
> 
> Use the new sysbus_init_child_obj() function to make sure that the child
> objects are cleaned up correctly when the parent gets destroyed.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v2 09/16] hw/cpu/a9mpcore: Fix introspection problems with the "a9mpcore_priv" device

[Thomas Huth]

Running QEMU with valgrind indicates a problem here:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'a9mpcore_priv'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==30996== Invalid read of size 8
==30996==    at 0x6185DA: qdev_print (qdev-monitor.c:686)
==30996==    by 0x6185DA: qbus_print (qdev-monitor.c:719)
==30996==    by 0x452B38: handle_hmp_command (monitor.c:3446)
[...]

Use the new sysbus_init_child_obj() function to make sure that the objects
are cleaned up correctly when the parent gets destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/cpu/a9mpcore.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index f17f292..a5b8678 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -27,20 +27,18 @@ static void a9mp_priv_initfn(Object *obj)
     memory_region_init(&s->container, obj, "a9mp-priv-container", 0x2000);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
 
-    object_initialize(&s->scu, sizeof(s->scu), TYPE_A9_SCU);
-    qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
+    sysbus_init_child_obj(obj, "scu", &s->scu, sizeof(s->scu), TYPE_A9_SCU);
 
-    object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
-    qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
+    sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic), TYPE_ARM_GIC);
 
-    object_initialize(&s->gtimer, sizeof(s->gtimer), TYPE_A9_GTIMER);
-    qdev_set_parent_bus(DEVICE(&s->gtimer), sysbus_get_default());
+    sysbus_init_child_obj(obj, "gtimer", &s->gtimer, sizeof(s->gtimer),
+                          TYPE_A9_GTIMER);
 
-    object_initialize(&s->mptimer, sizeof(s->mptimer), TYPE_ARM_MPTIMER);
-    qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
+    sysbus_init_child_obj(obj, "mptimer", &s->mptimer, sizeof(s->mptimer),
+                          TYPE_ARM_MPTIMER);
 
-    object_initialize(&s->wdt, sizeof(s->wdt), TYPE_ARM_MPTIMER);
-    qdev_set_parent_bus(DEVICE(&s->wdt), sysbus_get_default());
+    sysbus_init_child_obj(obj, "wdt", &s->wdt, sizeof(s->wdt),
+                          TYPE_ARM_MPTIMER);
 }
 
 static void a9mp_priv_realize(DeviceState *dev, Error **errp)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 09/16] hw/cpu/a9mpcore: Fix introspection problems with the "a9mpcore_priv" device

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:37AM +0200, Thomas Huth wrote:
> Running QEMU with valgrind indicates a problem here:
> 
> echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
>  "'arguments':{'typename':'a9mpcore_priv'}}" \
>  "{'execute': 'human-monitor-command', " \
>  "'arguments': {'command-line': 'info qtree'}}" | \
>  valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
> [...]
> ==30996== Invalid read of size 8
> ==30996==    at 0x6185DA: qdev_print (qdev-monitor.c:686)
> ==30996==    by 0x6185DA: qbus_print (qdev-monitor.c:719)
> ==30996==    by 0x452B38: handle_hmp_command (monitor.c:3446)
> [...]
> 
> Use the new sysbus_init_child_obj() function to make sure that the objects
> are cleaned up correctly when the parent gets destroyed.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

[Qemu-devel] [PATCH v2 10/16] hw/arm/fsl-imx6: Fix introspection problems with the "fsl, imx6" device

[Thomas Huth]

Running QEMU with valgrind indicates a problem here:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'fsl,imx6'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==32417== Invalid read of size 8
==32417==    at 0x618A7A: qdev_print (qdev-monitor.c:686)
==32417==    by 0x618A7A: qbus_print (qdev-monitor.c:719)
==32417==    by 0x452B38: handle_hmp_command (monitor.c:3446)
[...]

Use the new sysbus_init_child_obj() and object_initialize_child() to make
sure that the objects are removed correctly when the parent gets destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/fsl-imx6.c | 56 ++++++++++++++++++++-----------------------------------
 1 file changed, 20 insertions(+), 36 deletions(-)

diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 4f51bd9..26ca61f 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -38,73 +38,57 @@ static void fsl_imx6_init(Object *obj)
     int i;
 
     for (i = 0; i < MIN(smp_cpus, FSL_IMX6_NUM_CPUS); i++) {
-        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
-                          "cortex-a9-" TYPE_ARM_CPU);
         snprintf(name, NAME_SIZE, "cpu%d", i);
-        object_property_add_child(obj, name, OBJECT(&s->cpu[i]), NULL);
+        object_initialize_child(obj, name, &s->cpu[i], sizeof(s->cpu[i]),
+                                "cortex-a9-" TYPE_ARM_CPU, NULL);
     }
 
-    object_initialize(&s->a9mpcore, sizeof(s->a9mpcore), TYPE_A9MPCORE_PRIV);
-    qdev_set_parent_bus(DEVICE(&s->a9mpcore), sysbus_get_default());
-    object_property_add_child(obj, "a9mpcore", OBJECT(&s->a9mpcore), NULL);
+    sysbus_init_child_obj(obj, "a9mpcore", &s->a9mpcore, sizeof(s->a9mpcore),
+                          TYPE_A9MPCORE_PRIV);
 
-    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX6_CCM);
-    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
-    object_property_add_child(obj, "ccm", OBJECT(&s->ccm), NULL);
+    sysbus_init_child_obj(obj, "ccm", &s->ccm, sizeof(s->ccm), TYPE_IMX6_CCM);
 
-    object_initialize(&s->src, sizeof(s->src), TYPE_IMX6_SRC);
-    qdev_set_parent_bus(DEVICE(&s->src), sysbus_get_default());
-    object_property_add_child(obj, "src", OBJECT(&s->src), NULL);
+    sysbus_init_child_obj(obj, "src", &s->src, sizeof(s->src), TYPE_IMX6_SRC);
 
     for (i = 0; i < FSL_IMX6_NUM_UARTS; i++) {
-        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
-        qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
         snprintf(name, NAME_SIZE, "uart%d", i + 1);
-        object_property_add_child(obj, name, OBJECT(&s->uart[i]), NULL);
+        sysbus_init_child_obj(obj, name, &s->uart[i], sizeof(s->uart[i]),
+                              TYPE_IMX_SERIAL);
     }
 
-    object_initialize(&s->gpt, sizeof(s->gpt), TYPE_IMX6_GPT);
-    qdev_set_parent_bus(DEVICE(&s->gpt), sysbus_get_default());
-    object_property_add_child(obj, "gpt", OBJECT(&s->gpt), NULL);
+    sysbus_init_child_obj(obj, "gpt", &s->gpt, sizeof(s->gpt), TYPE_IMX6_GPT);
 
     for (i = 0; i < FSL_IMX6_NUM_EPITS; i++) {
-        object_initialize(&s->epit[i], sizeof(s->epit[i]), TYPE_IMX_EPIT);
-        qdev_set_parent_bus(DEVICE(&s->epit[i]), sysbus_get_default());
         snprintf(name, NAME_SIZE, "epit%d", i + 1);
-        object_property_add_child(obj, name, OBJECT(&s->epit[i]), NULL);
+        sysbus_init_child_obj(obj, name, &s->epit[i], sizeof(s->epit[i]),
+                              TYPE_IMX_EPIT);
     }
 
     for (i = 0; i < FSL_IMX6_NUM_I2CS; i++) {
-        object_initialize(&s->i2c[i], sizeof(s->i2c[i]), TYPE_IMX_I2C);
-        qdev_set_parent_bus(DEVICE(&s->i2c[i]), sysbus_get_default());
         snprintf(name, NAME_SIZE, "i2c%d", i + 1);
-        object_property_add_child(obj, name, OBJECT(&s->i2c[i]), NULL);
+        sysbus_init_child_obj(obj, name, &s->i2c[i], sizeof(s->i2c[i]),
+                              TYPE_IMX_I2C);
     }
 
     for (i = 0; i < FSL_IMX6_NUM_GPIOS; i++) {
-        object_initialize(&s->gpio[i], sizeof(s->gpio[i]), TYPE_IMX_GPIO);
-        qdev_set_parent_bus(DEVICE(&s->gpio[i]), sysbus_get_default());
         snprintf(name, NAME_SIZE, "gpio%d", i + 1);
-        object_property_add_child(obj, name, OBJECT(&s->gpio[i]), NULL);
+        sysbus_init_child_obj(obj, name, &s->gpio[i], sizeof(s->gpio[i]),
+                              TYPE_IMX_GPIO);
     }
 
     for (i = 0; i < FSL_IMX6_NUM_ESDHCS; i++) {
-        object_initialize(&s->esdhc[i], sizeof(s->esdhc[i]), TYPE_IMX_USDHC);
-        qdev_set_parent_bus(DEVICE(&s->esdhc[i]), sysbus_get_default());
         snprintf(name, NAME_SIZE, "sdhc%d", i + 1);
-        object_property_add_child(obj, name, OBJECT(&s->esdhc[i]), NULL);
+        sysbus_init_child_obj(obj, name, &s->esdhc[i], sizeof(s->esdhc[i]),
+                              TYPE_IMX_USDHC);
     }
 
     for (i = 0; i < FSL_IMX6_NUM_ECSPIS; i++) {
-        object_initialize(&s->spi[i], sizeof(s->spi[i]), TYPE_IMX_SPI);
-        qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
         snprintf(name, NAME_SIZE, "spi%d", i + 1);
-        object_property_add_child(obj, name, OBJECT(&s->spi[i]), NULL);
+        sysbus_init_child_obj(obj, name, &s->spi[i], sizeof(s->spi[i]),
+                              TYPE_IMX_SPI);
     }
 
-    object_initialize(&s->eth, sizeof(s->eth), TYPE_IMX_ENET);
-    qdev_set_parent_bus(DEVICE(&s->eth), sysbus_get_default());
-    object_property_add_child(obj, "eth", OBJECT(&s->eth), NULL);
+    sysbus_init_child_obj(obj, "eth", &s->eth, sizeof(s->eth), TYPE_IMX_ENET);
 }
 
 static void fsl_imx6_realize(DeviceState *dev, Error **errp)
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 11/16] hw/arm/fsl-imx7: Fix introspection problems with the "fsl, imx7" device

[Thomas Huth]

Running QEMU with valgrind indicates a problem here:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'fsl,imx7'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==27284== Invalid read of size 8
==27284==    at 0x618F7A: qdev_print (qdev-monitor.c:686)
==27284==    by 0x618F7A: qbus_print (qdev-monitor.c:719)
==27284==    by 0x452B38: handle_hmp_command (monitor.c:3446)
[...]

Use the new sysbus_init_child_obj() and object_initialize_child() to make
sure that the objects are removed correctly when the parent gets destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/fsl-imx7.c | 96 +++++++++++++++++++------------------------------------
 1 file changed, 32 insertions(+), 64 deletions(-)

diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index 44fde03..4aad720 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -30,157 +30,125 @@
 
 static void fsl_imx7_init(Object *obj)
 {
-    BusState *sysbus = sysbus_get_default();
     FslIMX7State *s = FSL_IMX7(obj);
     char name[NAME_SIZE];
     int i;
 
 
     for (i = 0; i < MIN(smp_cpus, FSL_IMX7_NUM_CPUS); i++) {
-        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
-                          ARM_CPU_TYPE_NAME("cortex-a7"));
         snprintf(name, NAME_SIZE, "cpu%d", i);
-        object_property_add_child(obj, name, OBJECT(&s->cpu[i]),
-                                  &error_fatal);
+        object_initialize_child(obj, name, &s->cpu[i], sizeof(s->cpu[i]),
+                                ARM_CPU_TYPE_NAME("cortex-a7"), &error_fatal);
     }
 
     /*
      * A7MPCORE
      */
-    object_initialize(&s->a7mpcore, sizeof(s->a7mpcore), TYPE_A15MPCORE_PRIV);
-    qdev_set_parent_bus(DEVICE(&s->a7mpcore), sysbus);
-    object_property_add_child(obj, "a7mpcore",
-                              OBJECT(&s->a7mpcore), &error_fatal);
+    sysbus_init_child_obj(obj, "a7mpcore", &s->a7mpcore, sizeof(s->a7mpcore),
+                          TYPE_A15MPCORE_PRIV);
 
     /*
      * GPIOs 1 to 7
      */
     for (i = 0; i < FSL_IMX7_NUM_GPIOS; i++) {
-        object_initialize(&s->gpio[i], sizeof(s->gpio[i]),
-                          TYPE_IMX_GPIO);
-        qdev_set_parent_bus(DEVICE(&s->gpio[i]), sysbus);
         snprintf(name, NAME_SIZE, "gpio%d", i);
-        object_property_add_child(obj, name,
-                                  OBJECT(&s->gpio[i]), &error_fatal);
+        sysbus_init_child_obj(obj, name, &s->gpio[i], sizeof(s->gpio[i]),
+                          TYPE_IMX_GPIO);
     }
 
     /*
      * GPT1, 2, 3, 4
      */
     for (i = 0; i < FSL_IMX7_NUM_GPTS; i++) {
-        object_initialize(&s->gpt[i], sizeof(s->gpt[i]), TYPE_IMX7_GPT);
-        qdev_set_parent_bus(DEVICE(&s->gpt[i]), sysbus);
         snprintf(name, NAME_SIZE, "gpt%d", i);
-        object_property_add_child(obj, name, OBJECT(&s->gpt[i]),
-                                  &error_fatal);
+        sysbus_init_child_obj(obj, name, &s->gpt[i], sizeof(s->gpt[i]),
+                              TYPE_IMX7_GPT);
     }
 
     /*
      * CCM
      */
-    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
-    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus);
-    object_property_add_child(obj, "ccm", OBJECT(&s->ccm), &error_fatal);
+    sysbus_init_child_obj(obj, "ccm", &s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
 
     /*
      * Analog
      */
-    object_initialize(&s->analog, sizeof(s->analog), TYPE_IMX7_ANALOG);
-    qdev_set_parent_bus(DEVICE(&s->analog), sysbus);
-    object_property_add_child(obj, "analog", OBJECT(&s->analog), &error_fatal);
+    sysbus_init_child_obj(obj, "analog", &s->analog, sizeof(s->analog),
+                          TYPE_IMX7_ANALOG);
 
     /*
      * GPCv2
      */
-    object_initialize(&s->gpcv2, sizeof(s->gpcv2), TYPE_IMX_GPCV2);
-    qdev_set_parent_bus(DEVICE(&s->gpcv2), sysbus);
-    object_property_add_child(obj, "gpcv2", OBJECT(&s->gpcv2), &error_fatal);
+    sysbus_init_child_obj(obj, "gpcv2", &s->gpcv2, sizeof(s->gpcv2),
+                          TYPE_IMX_GPCV2);
 
     for (i = 0; i < FSL_IMX7_NUM_ECSPIS; i++) {
-        object_initialize(&s->spi[i], sizeof(s->spi[i]), TYPE_IMX_SPI);
-        qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
         snprintf(name, NAME_SIZE, "spi%d", i + 1);
-        object_property_add_child(obj, name, OBJECT(&s->spi[i]), NULL);
+        sysbus_init_child_obj(obj, name, &s->spi[i], sizeof(s->spi[i]),
+                              TYPE_IMX_SPI);
     }
 
 
     for (i = 0; i < FSL_IMX7_NUM_I2CS; i++) {
-        object_initialize(&s->i2c[i], sizeof(s->i2c[i]), TYPE_IMX_I2C);
-        qdev_set_parent_bus(DEVICE(&s->i2c[i]), sysbus_get_default());
         snprintf(name, NAME_SIZE, "i2c%d", i + 1);
-        object_property_add_child(obj, name, OBJECT(&s->i2c[i]), NULL);
+        sysbus_init_child_obj(obj, name, &s->i2c[i], sizeof(s->i2c[i]),
+                              TYPE_IMX_I2C);
     }
 
     /*
      * UART
      */
     for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
-            object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
-            qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus);
             snprintf(name, NAME_SIZE, "uart%d", i);
-            object_property_add_child(obj, name, OBJECT(&s->uart[i]),
-                                      &error_fatal);
+            sysbus_init_child_obj(obj, name, &s->uart[i], sizeof(s->uart[i]),
+                                  TYPE_IMX_SERIAL);
     }
 
     /*
      * Ethernet
      */
     for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
-            object_initialize(&s->eth[i], sizeof(s->eth[i]), TYPE_IMX_ENET);
-            qdev_set_parent_bus(DEVICE(&s->eth[i]), sysbus);
             snprintf(name, NAME_SIZE, "eth%d", i);
-            object_property_add_child(obj, name, OBJECT(&s->eth[i]),
-                                      &error_fatal);
+            sysbus_init_child_obj(obj, name, &s->eth[i], sizeof(s->eth[i]),
+                                  TYPE_IMX_ENET);
     }
 
     /*
      * SDHCI
      */
     for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
-            object_initialize(&s->usdhc[i], sizeof(s->usdhc[i]),
-                              TYPE_IMX_USDHC);
-            qdev_set_parent_bus(DEVICE(&s->usdhc[i]), sysbus);
             snprintf(name, NAME_SIZE, "usdhc%d", i);
-            object_property_add_child(obj, name, OBJECT(&s->usdhc[i]),
-                                      &error_fatal);
+            sysbus_init_child_obj(obj, name, &s->usdhc[i], sizeof(s->usdhc[i]),
+                              TYPE_IMX_USDHC);
     }
 
     /*
      * SNVS
      */
-    object_initialize(&s->snvs, sizeof(s->snvs), TYPE_IMX7_SNVS);
-    qdev_set_parent_bus(DEVICE(&s->snvs), sysbus);
-    object_property_add_child(obj, "snvs", OBJECT(&s->snvs), &error_fatal);
+    sysbus_init_child_obj(obj, "snvs", &s->snvs, sizeof(s->snvs),
+                          TYPE_IMX7_SNVS);
 
     /*
      * Watchdog
      */
     for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
-            object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_IMX2_WDT);
-            qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus);
             snprintf(name, NAME_SIZE, "wdt%d", i);
-            object_property_add_child(obj, name, OBJECT(&s->wdt[i]),
-                                      &error_fatal);
+            sysbus_init_child_obj(obj, name, &s->wdt[i], sizeof(s->wdt[i]),
+                                  TYPE_IMX2_WDT);
     }
 
     /*
      * GPR
      */
-    object_initialize(&s->gpr, sizeof(s->gpr), TYPE_IMX7_GPR);
-    qdev_set_parent_bus(DEVICE(&s->gpr), sysbus);
-    object_property_add_child(obj, "gpr", OBJECT(&s->gpr), &error_fatal);
+    sysbus_init_child_obj(obj, "gpr", &s->gpr, sizeof(s->gpr), TYPE_IMX7_GPR);
 
-    object_initialize(&s->pcie, sizeof(s->pcie), TYPE_DESIGNWARE_PCIE_HOST);
-    qdev_set_parent_bus(DEVICE(&s->pcie), sysbus);
-    object_property_add_child(obj, "pcie", OBJECT(&s->pcie), &error_fatal);
+    sysbus_init_child_obj(obj, "pcie", &s->pcie, sizeof(s->pcie),
+                          TYPE_DESIGNWARE_PCIE_HOST);
 
     for (i = 0; i < FSL_IMX7_NUM_USBS; i++) {
-        object_initialize(&s->usb[i],
-                          sizeof(s->usb[i]), TYPE_CHIPIDEA);
-        qdev_set_parent_bus(DEVICE(&s->usb[i]), sysbus);
         snprintf(name, NAME_SIZE, "usb%d", i);
-        object_property_add_child(obj, name,
-                                  OBJECT(&s->usb[i]), &error_fatal);
+        sysbus_init_child_obj(obj, name, &s->usb[i], sizeof(s->usb[i]),
+                              TYPE_CHIPIDEA);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 12/16] hw/arm/fsl-imx25: Fix introspection problem with the "fsl, imx25" device

[Thomas Huth]

Running QEMU with valgrind indicates a problem here:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'fsl,imx25'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==26724== Invalid read of size 8
==26724==    at 0x6190DA: qdev_print (qdev-monitor.c:686)
==26724==    by 0x6190DA: qbus_print (qdev-monitor.c:719)
[...]

Use the new sysbus_init_child_obj() to make sure that the objects are
cleaned up correctly when the parent gets destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/fsl-imx25.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 37056f9..bd07040 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -39,38 +39,36 @@ static void fsl_imx25_init(Object *obj)
 
     object_initialize(&s->cpu, sizeof(s->cpu), "arm926-" TYPE_ARM_CPU);
 
-    object_initialize(&s->avic, sizeof(s->avic), TYPE_IMX_AVIC);
-    qdev_set_parent_bus(DEVICE(&s->avic), sysbus_get_default());
+    sysbus_init_child_obj(obj, "avic", &s->avic, sizeof(s->avic),
+                          TYPE_IMX_AVIC);
 
-    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX25_CCM);
-    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
+    sysbus_init_child_obj(obj, "ccm", &s->ccm, sizeof(s->ccm), TYPE_IMX25_CCM);
 
     for (i = 0; i < FSL_IMX25_NUM_UARTS; i++) {
-        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
-        qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "uart[*]", &s->uart[i], sizeof(s->uart[i]),
+                              TYPE_IMX_SERIAL);
     }
 
     for (i = 0; i < FSL_IMX25_NUM_GPTS; i++) {
-        object_initialize(&s->gpt[i], sizeof(s->gpt[i]), TYPE_IMX25_GPT);
-        qdev_set_parent_bus(DEVICE(&s->gpt[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "gpt[*]", &s->gpt[i], sizeof(s->gpt[i]),
+                              TYPE_IMX25_GPT);
     }
 
     for (i = 0; i < FSL_IMX25_NUM_EPITS; i++) {
-        object_initialize(&s->epit[i], sizeof(s->epit[i]), TYPE_IMX_EPIT);
-        qdev_set_parent_bus(DEVICE(&s->epit[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "epit[*]", &s->epit[i], sizeof(s->epit[i]),
+                              TYPE_IMX_EPIT);
     }
 
-    object_initialize(&s->fec, sizeof(s->fec), TYPE_IMX_FEC);
-    qdev_set_parent_bus(DEVICE(&s->fec), sysbus_get_default());
+    sysbus_init_child_obj(obj, "fec", &s->fec, sizeof(s->fec), TYPE_IMX_FEC);
 
     for (i = 0; i < FSL_IMX25_NUM_I2CS; i++) {
-        object_initialize(&s->i2c[i], sizeof(s->i2c[i]), TYPE_IMX_I2C);
-        qdev_set_parent_bus(DEVICE(&s->i2c[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "i2c[*]", &s->i2c[i], sizeof(s->i2c[i]),
+                              TYPE_IMX_I2C);
     }
 
     for (i = 0; i < FSL_IMX25_NUM_GPIOS; i++) {
-        object_initialize(&s->gpio[i], sizeof(s->gpio[i]), TYPE_IMX_GPIO);
-        qdev_set_parent_bus(DEVICE(&s->gpio[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "gpio[*]", &s->gpio[i], sizeof(s->gpio[i]),
+                              TYPE_IMX_GPIO);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 13/16] hw/arm/fsl-imx31: Fix introspection problem with the "fsl, imx31" device

[Thomas Huth]

Running QEMU with valgrind indicates a problem here:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'fsl,imx31'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==26172== Invalid read of size 8
==26172==    at 0x6191FA: qdev_print (qdev-monitor.c:686)
==26172==    by 0x6191FA: qbus_print (qdev-monitor.c:719)
[...]

Use the new sysbus_init_child_obj() to make sure that the objects are
cleaned up correctly when the parent gets destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/fsl-imx31.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 891850c..ec8239a 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -36,33 +36,31 @@ static void fsl_imx31_init(Object *obj)
 
     object_initialize(&s->cpu, sizeof(s->cpu), "arm1136-" TYPE_ARM_CPU);
 
-    object_initialize(&s->avic, sizeof(s->avic), TYPE_IMX_AVIC);
-    qdev_set_parent_bus(DEVICE(&s->avic), sysbus_get_default());
+    sysbus_init_child_obj(obj, "avic", &s->avic, sizeof(s->avic),
+                          TYPE_IMX_AVIC);
 
-    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX31_CCM);
-    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
+    sysbus_init_child_obj(obj, "ccm", &s->ccm, sizeof(s->ccm), TYPE_IMX31_CCM);
 
     for (i = 0; i < FSL_IMX31_NUM_UARTS; i++) {
-        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
-        qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "uart[*]", &s->uart[i], sizeof(s->uart[i]),
+                              TYPE_IMX_SERIAL);
     }
 
-    object_initialize(&s->gpt, sizeof(s->gpt), TYPE_IMX31_GPT);
-    qdev_set_parent_bus(DEVICE(&s->gpt), sysbus_get_default());
+    sysbus_init_child_obj(obj, "gpt", &s->gpt, sizeof(s->gpt), TYPE_IMX31_GPT);
 
     for (i = 0; i < FSL_IMX31_NUM_EPITS; i++) {
-        object_initialize(&s->epit[i], sizeof(s->epit[i]), TYPE_IMX_EPIT);
-        qdev_set_parent_bus(DEVICE(&s->epit[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "epit[*]", &s->epit[i], sizeof(s->epit[i]),
+                              TYPE_IMX_EPIT);
     }
 
     for (i = 0; i < FSL_IMX31_NUM_I2CS; i++) {
-        object_initialize(&s->i2c[i], sizeof(s->i2c[i]), TYPE_IMX_I2C);
-        qdev_set_parent_bus(DEVICE(&s->i2c[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "i2c[*]", &s->i2c[i], sizeof(s->i2c[i]),
+                              TYPE_IMX_I2C);
     }
 
     for (i = 0; i < FSL_IMX31_NUM_GPIOS; i++) {
-        object_initialize(&s->gpio[i], sizeof(s->gpio[i]), TYPE_IMX_GPIO);
-        qdev_set_parent_bus(DEVICE(&s->gpio[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "gpio[*]", &s->gpio[i], sizeof(s->gpio[i]),
+                              TYPE_IMX_GPIO);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 14/16] hw/cpu/arm11mpcore: Fix introspection problem with 'arm11mpcore_priv'

[Thomas Huth]

Valgrind reports an error here:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'arm11mpcore_priv'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==3145== Invalid read of size 8
==3145==    at 0x61873A: qdev_print (qdev-monitor.c:686)
==3145==    by 0x61873A: qbus_print (qdev-monitor.c:719)
[...]

Use sysbus_init_child_obj() to fix it.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/cpu/arm11mpcore.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/cpu/arm11mpcore.c b/hw/cpu/arm11mpcore.c
index eb24465..8aead37 100644
--- a/hw/cpu/arm11mpcore.c
+++ b/hw/cpu/arm11mpcore.c
@@ -121,19 +121,17 @@ static void mpcore_priv_initfn(Object *obj)
                        "mpcore-priv-container", 0x2000);
     sysbus_init_mmio(sbd, &s->container);
 
-    object_initialize(&s->scu, sizeof(s->scu), TYPE_ARM11_SCU);
-    qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
+    sysbus_init_child_obj(obj, "scu", &s->scu, sizeof(s->scu), TYPE_ARM11_SCU);
 
-    object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
-    qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
+    sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic), TYPE_ARM_GIC);
     /* Request the legacy 11MPCore GIC behaviour: */
     qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 0);
 
-    object_initialize(&s->mptimer, sizeof(s->mptimer), TYPE_ARM_MPTIMER);
-    qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
+    sysbus_init_child_obj(obj, "mptimer", &s->mptimer, sizeof(s->mptimer),
+                          TYPE_ARM_MPTIMER);
 
-    object_initialize(&s->wdtimer, sizeof(s->wdtimer), TYPE_ARM_MPTIMER);
-    qdev_set_parent_bus(DEVICE(&s->wdtimer), sysbus_get_default());
+    sysbus_init_child_obj(obj, "wdtimer", &s->wdtimer, sizeof(s->wdtimer),
+                          TYPE_ARM_MPTIMER);
 }
 
 static Property mpcore_priv_properties[] = {
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 15/16] hw/*/realview: Fix introspection problem with 'realview_mpcore' & 'realview_gic'

[Thomas Huth]

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'realview_mpcore'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==2654== Invalid read of size 8
==2654==    at 0x61878A: qdev_print (qdev-monitor.c:686)
==2654==    by 0x61878A: qbus_print (qdev-monitor.c:719)
==2654==    by 0x452B38: handle_hmp_command (monitor.c:3446)
==2654==    by 0x452D70: qmp_human_monitor_command (monitor.c:821)
[...]

Use sysbus_init_child_obj() to fix it.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/cpu/realview_mpcore.c | 8 ++++----
 hw/intc/realview_gic.c   | 7 ++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/cpu/realview_mpcore.c b/hw/cpu/realview_mpcore.c
index 39d4ebe..9d3f837 100644
--- a/hw/cpu/realview_mpcore.c
+++ b/hw/cpu/realview_mpcore.c
@@ -101,14 +101,14 @@ static void mpcore_rirq_init(Object *obj)
     SysBusDevice *privbusdev;
     int i;
 
-    object_initialize(&s->priv, sizeof(s->priv), TYPE_ARM11MPCORE_PRIV);
-    qdev_set_parent_bus(DEVICE(&s->priv), sysbus_get_default());
+    sysbus_init_child_obj(obj, "a11priv", &s->priv, sizeof(s->priv),
+                          TYPE_ARM11MPCORE_PRIV);
     privbusdev = SYS_BUS_DEVICE(&s->priv);
     sysbus_init_mmio(sbd, sysbus_mmio_get_region(privbusdev, 0));
 
     for (i = 0; i < 4; i++) {
-        object_initialize(&s->gic[i], sizeof(s->gic[i]), TYPE_REALVIEW_GIC);
-        qdev_set_parent_bus(DEVICE(&s->gic[i]), sysbus_get_default());
+        sysbus_init_child_obj(obj, "gic[*]", &s->gic[i], sizeof(s->gic[i]),
+                              TYPE_REALVIEW_GIC);
     }
 }
 
diff --git a/hw/intc/realview_gic.c b/hw/intc/realview_gic.c
index 50bbab6..7f2ff85 100644
--- a/hw/intc/realview_gic.c
+++ b/hw/intc/realview_gic.c
@@ -54,16 +54,13 @@ static void realview_gic_init(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     RealViewGICState *s = REALVIEW_GIC(obj);
-    DeviceState *gicdev;
 
     memory_region_init(&s->container, OBJECT(s),
                        "realview-gic-container", 0x2000);
     sysbus_init_mmio(sbd, &s->container);
 
-    object_initialize(&s->gic, sizeof(s->gic), TYPE_ARM_GIC);
-    gicdev = DEVICE(&s->gic);
-    qdev_set_parent_bus(gicdev, sysbus_get_default());
-    qdev_prop_set_uint32(gicdev, "num-cpu", 1);
+    sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic), TYPE_ARM_GIC);
+    qdev_prop_set_uint32(DEVICE(&s->gic), "num-cpu", 1);
 }
 
 static void realview_gic_class_init(ObjectClass *oc, void *data)
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 16/16] hw/arm/allwinner-a10: Fix introspection problem with 'allwinner-a10'

[Thomas Huth]

Valgrind complains:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'allwinner-a10'}}" \
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
[...]
==32519== Invalid read of size 8
==32519==    at 0x61869A: qdev_print (qdev-monitor.c:686)
==32519==    by 0x61869A: qbus_print (qdev-monitor.c:719)
==32519==    by 0x452B38: handle_hmp_command (monitor.c:3446)
[...]

Use sysbus_init_child_obj() to fix the issue.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/allwinner-a10.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index c5fbc65..9da00f9 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -27,20 +27,19 @@ static void aw_a10_init(Object *obj)
 {
     AwA10State *s = AW_A10(obj);
 
-    object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
-    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
+    object_initialize_child(obj, "cpu", &s->cpu, sizeof(s->cpu),
+                            "cortex-a8-" TYPE_ARM_CPU, NULL);
 
-    object_initialize(&s->intc, sizeof(s->intc), TYPE_AW_A10_PIC);
-    qdev_set_parent_bus(DEVICE(&s->intc), sysbus_get_default());
+    sysbus_init_child_obj(obj, "intc", &s->intc, sizeof(s->intc),
+                          TYPE_AW_A10_PIC);
 
-    object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
-    qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
+    sysbus_init_child_obj(obj, "timer", &s->timer, sizeof(s->timer),
+                          TYPE_AW_A10_PIT);
 
-    object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
-    qdev_set_parent_bus(DEVICE(&s->emac), sysbus_get_default());
+    sysbus_init_child_obj(obj, "emac", &s->emac, sizeof(s->emac), TYPE_AW_EMAC);
 
-    object_initialize(&s->sata, sizeof(s->sata), TYPE_ALLWINNER_AHCI);
-    qdev_set_parent_bus(DEVICE(&s->sata), sysbus_get_default());
+    sysbus_init_child_obj(obj, "sata", &s->sata, sizeof(s->sata),
+                          TYPE_ALLWINNER_AHCI);
 }
 
 static void aw_a10_realize(DeviceState *dev, Error **errp)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 00/16] Fix crashes with introspection of ARM devices

[Richard Henderson]

On 07/13/2018 03:27 AM, Thomas Huth wrote:
> v2:
>  - Updated the first patch according to the review feedback from v1
>  - Added more patches with additional fixes
> 
> Thomas Huth (16):
>   qom/object: Add a new function object_initialize_child()
>   hw/core/sysbus: Add a function for creating and attaching an object
>   hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported
>     machines
>   hw/arm/armv7: Fix crash when introspecting the "iotkit" device
>   hw/cpu/a15mpcore: Fix introspection problem with the a15mpcore_priv
>     device
>   hw/display/xlnx_dp: Move problematic code from instance_init to
>     realize
>   hw/arm/xlnx-zynqmp: Fix crash when introspecting the "xlnx,zynqmp"
>     device
>   hw/arm/msf2-soc: Fix introspection problem with the "msf2-soc" device
>   hw/cpu/a9mpcore: Fix introspection problems with the "a9mpcore_priv"
>     device
>   hw/arm/fsl-imx6: Fix introspection problems with the "fsl,imx6" device
>   hw/arm/fsl-imx7: Fix introspection problems with the "fsl,imx7" device
>   hw/arm/fsl-imx25: Fix introspection problem with the "fsl,imx25"
>     device
>   hw/arm/fsl-imx31: Fix introspection problem with the "fsl,imx31"
>     device
>   hw/cpu/arm11mpcore: Fix introspection problem with 'arm11mpcore_priv'
>   hw/*/realview: Fix introspection problem with 'realview_mpcore' &
>     'realview_gic'
>   hw/arm/allwinner-a10: Fix introspection problem with 'allwinner-a10'

That is a great cleanup.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v2 00/16] Fix crashes with introspection of ARM devices

[Eduardo Habkost]

On Fri, Jul 13, 2018 at 10:27:28AM +0200, Thomas Huth wrote:
> As discovered recently, you can crash QEMU with a lot of devices
> that do not get the reference counting of child objects right.
> You just have to run 'device-list-properties' and call 'info qtree'
> afterwards.
> This patch series fixes a bunch of these problems in the ARM code.
> I did not fix all problems yet, since it is quite time consuming,
> and this series is big enough already. The remaining problems can
> be fixed in an independent patch series later.
> 
> v2:
>  - Updated the first patch according to the review feedback from v1
>  - Added more patches with additional fixes

OK, I will stop replying to each individual patch and just send
a single one for the series:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo