Re: [PATCH v2 04/58] qdev: New qdev_new(), qdev_realize(), etc.

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/29/20 3:44 PM, Markus Armbruster wrote:
> We commonly plug devices into their bus right when we create them,
> like this:
> 
>     dev = qdev_create(bus, type_name);
> 
> Note that @dev is a weak reference.  The reference from @bus to @dev
> is the only strong one.
> 
> We realize at some later time, either with
> 
>     object_property_set_bool(OBJECT(dev), true, "realized", errp);
> 
> or its convenience wrapper
> 
>     qdev_init_nofail(dev);
> 
> If @dev still has no QOM parent then, realizing makes the
> /machine/unattached/ orphanage its QOM parent.
> 
> Note that the device returned by qdev_create() is plugged into a bus,
> but doesn't have a QOM parent, yet.  Until it acquires one,
> unrealizing the bus will hang in bus_unparent():
> 
>     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>         DeviceState *dev = kid->child;
>         object_unparent(OBJECT(dev));
>     }
> 
> object_unparent() does nothing when its argument has no QOM parent,
> and the loop spins forever.
> 
> Device state "no QOM parent, but plugged into bus" is dangerous.
> 
> Paolo suggested to delay plugging into the bus until realize.  We need
> to plug into the parent bus before we call the device's realize
> method, in case it uses the parent bus.  So the dangerous state still
> exists, but only within realization, where we can manage it safely.
> 
> This commit creates infrastructure to do this:
> 
>     dev = qdev_new(type_name);
>     ...
>     qdev_realize_and_unref(dev, bus, errp)
> 
> Note that @dev becomes a strong reference here.
> qdev_realize_and_unref() drops it.  There is also plain
> qdev_realize(), which doesn't drop it.
> 
> The remainder of this series will convert all users to this new
> interface.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/hw/qdev-core.h | 11 +++++-
>  hw/core/bus.c          | 14 +++++++
>  hw/core/qdev.c         | 90 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 114 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index b870b27966..fba29308f7 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -57,7 +57,7 @@ typedef void (*BusUnrealize)(BusState *bus);
>   * After successful realization, setting static properties will fail.
>   *
>   * As an interim step, the #DeviceState:realized property can also be
> - * set with qdev_init_nofail().
> + * set with qdev_realize() or qdev_init_nofail().
>   * In the future, devices will propagate this state change to their children
>   * and along busses they expose.
>   * The point in time will be deferred to machine creation, so that values
> @@ -322,7 +322,13 @@ compat_props_add(GPtrArray *arr,
>  
>  DeviceState *qdev_create(BusState *bus, const char *name);
>  DeviceState *qdev_try_create(BusState *bus, const char *name);
> +DeviceState *qdev_new(const char *name);
> +DeviceState *qdev_try_new(const char *name);
>  void qdev_init_nofail(DeviceState *dev);
> +bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
> +bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
> +void qdev_unrealize(DeviceState *dev);
> +
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
>  HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
> @@ -411,6 +417,9 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
>  void qbus_create_inplace(void *bus, size_t size, const char *typename,
>                           DeviceState *parent, const char *name);
>  BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
> +bool qbus_realize(BusState *bus, Error **errp);
> +void qbus_unrealize(BusState *bus);
> +
>  /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
>   *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
>   *           0 otherwise. */
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index 33a4443217..6f6071f5fa 100644
> --- a/hw/core/bus.c
> +++ b/hw/core/bus.c
> @@ -164,6 +164,20 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>      return bus;
>  }
>  
> +bool qbus_realize(BusState *bus, Error **errp)
> +{
> +    Error *err = NULL;
> +
> +    object_property_set_bool(OBJECT(bus), true, "realized", &err);
> +    error_propagate(errp, err);
> +    return !err;
> +}
> +
> +void qbus_unrealize(BusState *bus)
> +{
> +    object_property_set_bool(OBJECT(bus), false, "realized", &error_abort);
> +}
> +
>  static bool bus_get_realized(Object *obj, Error **errp)
>  {
>      BusState *bus = BUS(obj);
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index a68ba674db..f2c5cee278 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -176,6 +176,32 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>      return dev;
>  }
>  
> +/*
> + * Create a device on the heap.
> + * A type @name must exist.
> + * This only initializes the device state structure and allows
> + * properties to be set.  The device still needs to be realized.  See
> + * qdev-core.h.
> + */
> +DeviceState *qdev_new(const char *name)
> +{
> +    return DEVICE(object_new(name));
> +}
> +
> +/*
> + * Try to create a device on the heap.
> + * This is like qdev_new(), except it returns %NULL when type @name
> + * does not exist.
> + */
> +DeviceState *qdev_try_new(const char *name)
> +{
> +    if (!object_class_by_name(name)) {
> +        return NULL;
> +    }
> +
> +    return DEVICE(object_new(name));

Or:

       return qdev_new(name);
> +}
> +
>  static QTAILQ_HEAD(, DeviceListener) device_listeners
>      = QTAILQ_HEAD_INITIALIZER(device_listeners);
>  
> @@ -427,6 +453,66 @@ void qdev_init_nofail(DeviceState *dev)
>      object_unref(OBJECT(dev));
>  }
>  
> +/*
> + * Realize @dev.
> + * @dev must not be plugged into a bus.
> + * Plug @dev into @bus if non-null, else into the main system bus.

So this is the historical part...

> + * This takes a reference to @dev.
> + * If @dev has no QOM parent, make one up, taking another reference.
> + * On success, return true.
> + * On failure, store an error through @errp and return false.
> + */
> +bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
> +{
> +    Error *err = NULL;
> +
> +    assert(!dev->realized && !dev->parent_bus);
> +
> +    if (!bus) {
> +        /*
> +         * Assert that the device really is a SysBusDevice before we
> +         * put it onto the sysbus.  Non-sysbus devices which aren't
> +         * being put onto a bus should be realized with
> +         * object_property_set_bool(OBJECT(dev), true, "realized",
> +         * errp);
> +         */
> +        g_assert(object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE));
> +        bus = sysbus_get_default();

OK, so we take a QDEV argument.

If we have a BUS argument, we use it as parent bus.

If we don't have a BUS argument, we expect the device is a SYSBUSDEV and
use sysbus_get_default as parent bus.

So we can not use this function realize a QDEV which is not a SYSBUSDEV.
For that, we have to use 'object_property_set_bool(OBJECT(QDEV), true,
"realized")'.

IOW:
- to realize a QDEV, we have to use the OBJECT API,
- to realize a SYSBUSDEV, we have to use the QDEV API.

I'm not suggesting you to clean that now, I'm simply taking notes about
future improvements, because this is really not clear for new contributors.

> +    }
> +
> +    qdev_set_parent_bus(dev, bus);
> +
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +    }
> +    return !err;
> +}
> +
> +/*
> + * Realize @dev and drop a reference.
> + * This is like qdev_realize(), except the caller must hold a
> + * (private) reference, which is dropped on return regardless of
> + * success or failure.  Intended use:
> + *     dev = qdev_new();
> + *     [...]
> + *     qdev_realize_and_unref(dev, bus, errp);
> + * Now @dev can go away without further ado.
> + */
> +bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp)
> +{
> +    bool ret;
> +
> +    ret = qdev_realize(dev, bus, errp);
> +    object_unref(OBJECT(dev));
> +    return ret;
> +}
> +
> +void qdev_unrealize(DeviceState *dev)
> +{
> +    object_property_set_bool(OBJECT(dev), false, "realized", &error_abort);
> +}
> +
>  static int qdev_assert_realized_properly(Object *obj, void *opaque)
>  {
>      DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
> @@ -1002,6 +1088,10 @@ post_realize_fail:
>  fail:
>      error_propagate(errp, local_err);
>      if (unattached_parent) {
> +        /*
> +         * Beware, this doesn't just revert
> +         * object_property_add_child(), it also runs bus_remove()!
> +         */
>          object_unparent(OBJECT(dev));
>          unattached_count--;
>      }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>