On Tue, May 19, 2020 at 9:26 PM Markus Armbruster <arm...@redhat.com> wrote: > > Alistair Francis <alistai...@gmail.com> writes: > > > On Tue, May 19, 2020 at 8:11 AM Markus Armbruster <arm...@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 <m...@redhat.com> > >> Cc: Marcel Apfelbaum <marcel.apfelb...@gmail.com> > >> Cc: Alistair Francis <alist...@alistair23.me> > >> Cc: Gerd Hoffmann <kra...@redhat.com> > >> Cc: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> > >> Cc: David Gibson <da...@gibson.dropbear.id.au> > >> Signed-off-by: Markus Armbruster <arm...@redhat.com> > >> --- > >> include/hw/qdev-core.h | 11 ++++- > >> hw/core/bus.c | 14 +++++++ > >> hw/core/qdev.c | 94 ++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 118 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 08c5eab24a..bf622604a3 100644 > >> --- a/hw/core/bus.c > >> +++ b/hw/core/bus.c > >> @@ -169,6 +169,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), true, "realized", &error_abort); > > > > Not false? > > > > Alistair > > Reasons it's &error_abort:
I meant why is this not setting the bool to false instead of true? > > 1. PATCH 06 and 07 transform variations of > > object_property_set_bool(..., false, "realized", &error_abort); > > to > > qdev_unrealize(...); > > No untransformed unrealization remain. Thus, we always abort on > unrealization error before this series. > > 2. If unrealize could fail, we'd be in deep trouble. Recent commit > b69c3c21a5 "qdev: Unrealize must not fail" explains: > > Devices may have component devices and buses. > > Device realization may fail. Realization is recursive: a device's > realize() method realizes its components, and device_set_realized() > realizes its buses (which should in turn realize the devices on that > bus, except bus_set_realized() doesn't implement that, yet). > > When realization of a component or bus fails, we need to roll back: > unrealize everything we realized so far. If any of these unrealizes > failed, the device would be left in an inconsistent state. Must not > happen. Makes sense. Maybe worth putting this in a comment here? > > device_set_realized() lets it happen: it ignores errors in the roll > back code starting at label child_realize_fail. > > Since realization is recursive, unrealization must be recursive, too. > But how could a partly failed unrealize be rolled back? We'd have to > re-realize, which can fail. This design is fundamentally broken. > > device_set_realized() does not roll back at all. Instead, it keeps > unrealizing, ignoring further errors. > > It can screw up even for a device with no buses: if the lone > dc->unrealize() fails, it still unregisters vmstate, and calls > listeners' unrealize() callback. > > bus_set_realized() does not roll back either. Instead, it stops > unrealizing. > > Fortunately, no unrealize method can fail, as we'll see below. > > Clearer now? Clear on the error_abort. > > With any luck, people will use the simpler qdev_unrealize() and > qbus_unrealize(), which is the form that doesn't let them get the error > handling wrong. I like it when interfaces make misuse hard :) Sounds good :) Alistair >