From: Alistair Francis <alistair23@gmail.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: "Daniel P. Berrange" <berrange@redhat.com>,
Eduardo Habkost <ehabkost@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
Alistair Francis <alistair@alistair23.me>,
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
Gerd Hoffmann <kraxel@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>,
David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH 03/55] qdev: New qdev_new(), qdev_realize(), etc.
Date: Tue, 19 May 2020 21:51:27 -0700 [thread overview]
Message-ID: <CAKmqyKPMRdD=NptG0UrboaJnArR3F0kScdcZ-inARuQJW_X5xw@mail.gmail.com> (raw)
In-Reply-To: <87mu63ut96.fsf@dusky.pond.sub.org>
On Tue, May 19, 2020 at 9:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Alistair Francis <alistair23@gmail.com> writes:
>
> > On Tue, May 19, 2020 at 8:11 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>
> >> ---
> >> 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
>
next prev parent reply other threads:[~2020-05-20 5:01 UTC|newest]
Thread overview: 125+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-19 14:54 [PATCH 00/55] qdev: Rework how we plug into the parent bus Markus Armbruster
2020-05-19 14:54 ` [PATCH 01/55] qdev: Rename qbus_realize() to qbus_init() Markus Armbruster
2020-05-19 14:54 ` [PATCH 02/55] qdev: Drop redundant bus realization Markus Armbruster
2020-05-20 12:00 ` Philippe Mathieu-Daudé
2020-05-20 14:25 ` Markus Armbruster
2020-05-19 14:54 ` [PATCH 03/55] qdev: New qdev_new(), qdev_realize(), etc Markus Armbruster
2020-05-19 21:02 ` Alistair Francis
2020-05-20 4:26 ` Markus Armbruster
2020-05-20 4:51 ` Alistair Francis [this message]
2020-05-20 7:29 ` Markus Armbruster
2020-05-20 6:22 ` Paolo Bonzini
2020-05-20 8:11 ` Markus Armbruster
2020-05-20 8:17 ` Paolo Bonzini
2020-05-20 14:42 ` Markus Armbruster
2020-05-20 16:28 ` Paolo Bonzini
2020-05-25 6:30 ` Markus Armbruster
2020-05-25 6:40 ` Paolo Bonzini
2020-05-29 12:22 ` Markus Armbruster
2020-05-20 8:49 ` Gerd Hoffmann
2020-05-19 14:55 ` [PATCH 04/55] qdev: Put qdev_new() to use with Coccinelle Markus Armbruster
2020-05-19 14:55 ` [PATCH 05/55] qdev: Convert to qbus_realize(), qbus_unrealize() Markus Armbruster
2020-05-19 14:55 ` [PATCH 06/55] qdev: Convert to qdev_unrealize() with Coccinelle Markus Armbruster
2020-05-19 14:55 ` [PATCH 07/55] qdev: Convert to qdev_unrealize() manually Markus Armbruster
2020-05-20 6:25 ` Paolo Bonzini
2020-05-20 8:12 ` Markus Armbruster
2020-05-19 14:55 ` [PATCH 08/55] qdev: Convert uses of qdev_create() with Coccinelle Markus Armbruster
2020-05-20 6:30 ` Paolo Bonzini
2020-05-20 8:16 ` Markus Armbruster
2020-05-19 14:55 ` [PATCH 09/55] qdev: Convert uses of qdev_create() manually Markus Armbruster
2020-05-19 14:55 ` [PATCH 10/55] qdev: Convert uses of qdev_set_parent_bus() with Coccinelle Markus Armbruster
2020-05-19 14:55 ` [PATCH 11/55] qdev: Convert uses of qdev_set_parent_bus() manually Markus Armbruster
2020-05-19 14:55 ` [PATCH 12/55] pci: New pci_new(), pci_realize_and_unref() etc Markus Armbruster
2020-05-19 14:55 ` [PATCH 13/55] hw/ppc: Eliminate two superfluous QOM casts Markus Armbruster
2020-05-26 11:56 ` Philippe Mathieu-Daudé
2020-05-19 14:55 ` [PATCH 14/55] pci: Convert uses of pci_create() etc. with Coccinelle Markus Armbruster
2020-05-19 14:55 ` [PATCH 15/55] pci: Convert uses of pci_create() etc. manually Markus Armbruster
2020-05-19 14:55 ` [PATCH 16/55] pci: pci_create(), pci_create_multifunction() are now unused, drop Markus Armbruster
2020-05-19 14:55 ` [PATCH 17/55] isa: New isa_new(), isa_realize_and_unref() etc Markus Armbruster
2020-05-19 14:55 ` [PATCH 18/55] isa: Convert uses of isa_create() with Coccinelle Markus Armbruster
2020-05-19 14:55 ` [PATCH 19/55] isa: Convert uses of isa_create(), isa_try_create() manually Markus Armbruster
2020-05-19 14:55 ` [PATCH 20/55] isa: isa_create(), isa_try_create() are now unused, drop Markus Armbruster
2020-05-19 14:55 ` [PATCH 21/55] ssi: ssi_auto_connect_slaves() never does anything, drop Markus Armbruster
2020-05-19 21:08 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 22/55] ssi: Convert uses of ssi_create_slave_no_init() with Coccinelle Markus Armbruster
2020-05-19 21:07 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 23/55] ssi: Convert last use of ssi_create_slave_no_init() manually Markus Armbruster
2020-05-19 20:58 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 24/55] ssi: ssi_create_slave_no_init() is now unused, drop Markus Armbruster
2020-05-19 21:11 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 25/55] usb: New usb_new(), usb_realize_and_unref() Markus Armbruster
2020-05-20 8:44 ` Gerd Hoffmann
2020-05-19 14:55 ` [PATCH 26/55] usb: Convert uses of usb_create() Markus Armbruster
2020-05-20 8:45 ` Gerd Hoffmann
2020-05-19 14:55 ` [PATCH 27/55] usb: usb_create() is now unused, drop Markus Armbruster
2020-05-20 8:46 ` Gerd Hoffmann
2020-05-19 14:55 ` [PATCH 28/55] usb: Eliminate usb_try_create_simple() Markus Armbruster
2020-05-20 8:46 ` Gerd Hoffmann
2020-05-19 14:55 ` [PATCH 29/55] qdev: qdev_create(), qdev_try_create() are now unused, drop Markus Armbruster
2020-05-19 14:55 ` [PATCH 30/55] auxbus: New aux_realize_bus(), pairing with aux_init_bus() Markus Armbruster
2020-05-26 11:54 ` Philippe Mathieu-Daudé
2020-05-27 4:39 ` Markus Armbruster
2020-05-19 14:55 ` [PATCH 31/55] auxbus: Convert a use of qdev_set_parent_bus() Markus Armbruster
2020-05-19 14:55 ` [PATCH 32/55] auxbus: Eliminate aux_create_slave() Markus Armbruster
2020-05-20 11:52 ` Philippe Mathieu-Daudé
2020-05-19 14:55 ` [PATCH 33/55] qom: Tidy up a few object_initialize_child() calls Markus Armbruster
2020-05-19 21:14 ` Alistair Francis
2020-05-26 11:51 ` Philippe Mathieu-Daudé
2020-05-19 14:55 ` [PATCH 34/55] qom: Less verbose object_initialize_child() Markus Armbruster
2020-05-19 21:16 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 35/55] macio: Convert use of qdev_set_parent_bus() Markus Armbruster
2020-05-19 14:55 ` [PATCH 36/55] macio: Eliminate macio_init_child_obj() Markus Armbruster
2020-05-19 14:55 ` [PATCH 37/55] sysbus: Drop useless OBJECT() in sysbus_init_child_obj() calls Markus Armbruster
2020-05-20 12:02 ` Philippe Mathieu-Daudé
2020-05-19 14:55 ` [PATCH 38/55] microbit: Tidy up sysbus_init_child_obj() @child argument Markus Armbruster
2020-05-20 12:06 ` Philippe Mathieu-Daudé
2020-05-20 14:49 ` Markus Armbruster
2020-05-20 14:54 ` Philippe Mathieu-Daudé
2020-05-19 14:55 ` [PATCH 39/55] sysbus: Tidy up sysbus_init_child_obj()'s @childsize arg, part 1 Markus Armbruster
2020-05-19 14:55 ` [PATCH 40/55] hw/arm/armsse: Pass correct child size to sysbus_init_child_obj() Markus Armbruster
2020-05-20 11:51 ` Philippe Mathieu-Daudé
2020-05-20 14:54 ` Markus Armbruster
2020-05-19 14:55 ` [PATCH 41/55] sysbus: Tidy up sysbus_init_child_obj()'s @childsize arg, part 2 Markus Armbruster
2020-05-19 14:55 ` [PATCH 42/55] sysbus: New sysbus_realize(), sysbus_realize_and_unref() Markus Armbruster
2020-05-19 14:55 ` [PATCH 43/55] sysbus: Convert to sysbus_realize() etc. with Coccinelle Markus Armbruster
2020-05-19 21:18 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 44/55] qdev: Drop qdev_realize() support for null bus Markus Armbruster
2020-05-19 14:55 ` [PATCH 45/55] sysbus: Convert qdev_set_parent_bus() use with Coccinelle, part 1 Markus Armbruster
2020-05-19 21:25 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 46/55] sysbus: Convert qdev_set_parent_bus() use with Coccinelle, part 2 Markus Armbruster
2020-05-19 21:26 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 47/55] sysbus: Convert qdev_set_parent_bus() use with Coccinelle, part 3 Markus Armbruster
2020-05-19 14:55 ` [PATCH 48/55] sysbus: Convert qdev_set_parent_bus() use with Coccinelle, part 4 Markus Armbruster
2020-05-19 14:55 ` [PATCH 49/55] sysbus: sysbus_init_child_obj() is now unused, drop Markus Armbruster
2020-05-19 14:55 ` [PATCH 50/55] s390x/event-facility: Simplify creation of SCLP event devices Markus Armbruster
2020-05-20 8:09 ` David Hildenbrand
2020-05-21 8:44 ` David Hildenbrand
2020-05-25 7:01 ` Markus Armbruster
2020-05-25 8:26 ` Paolo Bonzini
2020-05-26 6:27 ` Markus Armbruster
2020-05-26 7:51 ` Paolo Bonzini
2020-05-26 8:59 ` Markus Armbruster
2020-05-29 13:45 ` Markus Armbruster
2020-05-26 9:45 ` Cornelia Huck
2020-05-26 11:23 ` Paolo Bonzini
2020-05-26 11:38 ` Cornelia Huck
2020-05-26 9:59 ` David Hildenbrand
2020-05-19 14:55 ` [PATCH 51/55] qdev: Make qdev_realize() support bus-less devices Markus Armbruster
2020-05-20 6:43 ` Paolo Bonzini
2020-05-20 15:02 ` Markus Armbruster
2020-05-20 16:24 ` Paolo Bonzini
2020-05-25 6:38 ` Markus Armbruster
2020-05-25 10:11 ` Paolo Bonzini
2020-05-26 5:14 ` Markus Armbruster
2020-05-26 7:54 ` Paolo Bonzini
2020-05-19 14:55 ` [PATCH 52/55] qdev: Use qdev_realize() in qdev_device_add() Markus Armbruster
2020-05-19 14:55 ` [PATCH 53/55] qdev: Convert bus-less devices to qdev_realize() with Coccinelle Markus Armbruster
2020-05-19 21:28 ` Alistair Francis
2020-05-19 14:55 ` [PATCH 54/55] qdev: qdev_init_nofail() is now unused, drop Markus Armbruster
2020-05-19 14:55 ` [PATCH 55/55] MAINTAINERS: Make section QOM cover hw/core/*bus.c as well Markus Armbruster
2020-05-20 6:46 ` [PATCH 00/55] qdev: Rework how we plug into the parent bus Paolo Bonzini
2020-06-08 10:56 ` Markus Armbruster
2020-06-08 10:59 ` Paolo Bonzini
2020-06-09 6:41 ` Markus Armbruster
2020-06-09 6:55 ` Paolo Bonzini
2020-06-09 9:34 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKmqyKPMRdD=NptG0UrboaJnArR3F0kScdcZ-inARuQJW_X5xw@mail.gmail.com' \
--to=alistair23@gmail.com \
--cc=alistair@alistair23.me \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=ehabkost@redhat.com \
--cc=kraxel@redhat.com \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).