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:
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.
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?
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 :)
