Re: [PATCH 03/55] qdev: New qdev_new(), qdev_realize(), etc.

[Alistair Francis]

On Tue, May 19, 2020 at 9:26 PM Markus Armbruster <address@hidden> wrote:
>
> Alistair Francis <address@hidden> writes:
>
> > On Tue, May 19, 2020 at 8:11 AM Markus Armbruster <address@hidden> 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 <address@hidden>
> >> Cc: Marcel Apfelbaum <address@hidden>
> >> Cc: Alistair Francis <address@hidden>
> >> Cc: Gerd Hoffmann <address@hidden>
> >> Cc: Mark Cave-Ayland <address@hidden>
> >> Cc: David Gibson <address@hidden>
> >> Signed-off-by: Markus Armbruster <address@hidden>
> >> ---
> >>  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

>