| Message ID | 20200529134523.8477-5-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qdev: Rework how we plug into the parent bus | expand |
On Fri, May 29, 2020 at 7:03 AM Markus Armbruster <armbru@redhat.com> 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> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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)); > +} > + > 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. > + * 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(); > + } > + > + 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--; > } > -- > 2.21.3 > >
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>
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > 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); Yes, that's better. >> +} >> + >> 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... Yes, same behavior as qdev_create(), to ease conversion. >> + * 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")'. Correct. > IOW: > - to realize a QDEV, we have to use the OBJECT API, To realize a bus-less device, actually. > - to realize a SYSBUSDEV, we have to use the QDEV API. To realize a device that plugs into a bus, including the sysbus pseudo-bus. > 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. You're right, it's less than clear at this point of the series. PATCH 46 gets rid of the unintuitive "null means main system bus" in qdev_realize(). PATCH 54 makes qdev_realize() support bus-less devices the obvious way: pass a null bus argument. PATCH 55+56 put that to use. No realization with object_property_set_bool() remains. I figure I could've made review easier by better explaining where the series is headed. >> + } >> + >> + 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> Thanks!
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)); +} + 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. + * 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(); + } + + 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--; }
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(-)