[Qemu-devel] [PATCH RFC 0/3] Recursive QOM realize

[Qemu-devel] [PATCH RFC 0/3] Recursive QOM realize

[Andreas Färber]

Hello,

To honor the Soft Freeze, here is an early throw at recursive realization
based on earlier patches from Paolo.

Originally Paolo and me had implemented QOM realize at Object level.
Paolo's goal was to set realized = true on /machine and it propagating from
there on. This series now implements {realize,unrealize}_children at
DeviceState level instead and propagates realized changes along busses rather
than child<> properties. On machine creation done, a depth-first search is done
for devices from /machine, which are then expected to further propagate the
property change.

The idea is to get this framework in place so that we can slowly drop
qdev_init[_nofail]() / object_property_set_bool() calls in realizefns and
machine inits and avoid coding manual realize propagation as interim solution.

Improvements that I would like to do as follow-ups is implementing realized
property for busses so that we can set realized = true on the bus and have it
propagate from there rather than enumerating the bus from the parent device.

Regards,
Andreas

>From Paolo's qom-next patches:
* Implemented in DeviceClass rather than ObjectClass
* Implemented [un]realize_children by depth-first search for devices and
  propagating along busses rather than child<> properties.
* Implemented central realization in qdev_machine_creation_done().
* Added patch asserting that late-realizing devices no longer create children.

Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Hu Tao <hutao@cn.fujitsu.com>
Cc: Peter C. Crosthwaite <peter.crosthwaite@xilinx.com>

Andreas Färber (3):
  qdev: Add support for recursive realization
  qdev: Realize on machine creation done
  qdev: Assert no new devices get created during realization

 hw/core/qdev.c         | 92 +++++++++++++++++++++++++++++++++++++++++++++++---
 include/hw/qdev-core.h |  4 +++
 2 files changed, 92 insertions(+), 4 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 1/3] qdev: Add support for recursive realization

[Andreas Färber]

Add realize_children and unrealize_children QOM methods to DeviceClass.
Call them after realized = true and before realized = false respectively.

The default implementation walks busses and realizes their devices.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[AF: Transferred from Object to DeviceState]
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev.c         | 58 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/hw/qdev-core.h |  4 ++++
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9190a7e..33e874a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -183,6 +183,29 @@ static void device_realize(DeviceState *dev, Error **err)
     }
 }
 
+static int device_realize_one(DeviceState *dev, void *opaque)
+{
+    Error **errp = opaque;
+    Error *err = NULL;
+
+    object_property_set_bool(OBJECT(dev), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return -1;
+    }
+    return 0;
+}
+
+static int bus_realize_children(BusState *bus, void *opaque)
+{
+    return qbus_walk_children(bus, device_realize_one, NULL, opaque);
+}
+
+static void device_realize_children(DeviceState *dev, Error **errp)
+{
+    qdev_walk_children(dev, NULL, bus_realize_children, errp);
+}
+
 static void device_unrealize(DeviceState *dev, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
@@ -196,6 +219,29 @@ static void device_unrealize(DeviceState *dev, Error **errp)
     }
 }
 
+static int device_unrealize_one(DeviceState *dev, void *opaque)
+{
+    Error **errp = opaque;
+    Error *err = NULL;
+
+    object_property_set_bool(OBJECT(dev), false, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return -1;
+    }
+    return 0;
+}
+
+static int bus_unrealize_children(BusState *bus, void *opaque)
+{
+    return qbus_walk_children(bus, device_unrealize_one, NULL, opaque);
+}
+
+static void device_unrealize_children(DeviceState *dev, Error **errp)
+{
+    qdev_walk_children(dev, NULL, bus_unrealize_children, errp);
+}
+
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version)
 {
@@ -678,7 +724,7 @@ static bool device_get_realized(Object *obj, Error **err)
     return dev->realized;
 }
 
-static void device_set_realized(Object *obj, bool value, Error **err)
+static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
@@ -704,24 +750,26 @@ static void device_set_realized(Object *obj, bool value, Error **err)
                                            dev->instance_id_alias,
                                            dev->alias_required_for_version);
         }
+        dev->realized = true;
+        dc->realize_children(dev, errp);
         if (dev->hotplugged && local_err == NULL) {
             device_reset(dev);
         }
     } else if (!value && dev->realized) {
+        dc->unrealize_children(dev, errp);
         if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
         if (dc->unrealize) {
             dc->unrealize(dev, &local_err);
         }
+        dev->realized = false;
     }
 
     if (local_err != NULL) {
-        error_propagate(err, local_err);
+        error_propagate(errp, local_err);
         return;
     }
-
-    dev->realized = value;
 }
 
 static void device_initfn(Object *obj)
@@ -825,7 +873,9 @@ static void device_class_init(ObjectClass *class, void *data)
 
     class->unparent = device_unparent;
     dc->realize = device_realize;
+    dc->realize_children = device_realize_children;
     dc->unrealize = device_unrealize;
+    dc->unrealize_children = device_unrealize_children;
 }
 
 void device_reset(DeviceState *dev)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 7fbffcb..81dc2f3 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -21,7 +21,9 @@ typedef int (*qdev_initfn)(DeviceState *dev);
 typedef int (*qdev_event)(DeviceState *dev);
 typedef void (*qdev_resetfn)(DeviceState *dev);
 typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
+typedef void (*DeviceRealizeChildren)(DeviceState *dev, Error **errp);
 typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp);
+typedef void (*DeviceUnrealizeChildren)(DeviceState *dev, Error **errp);
 
 struct VMStateDescription;
 
@@ -88,7 +90,9 @@ typedef struct DeviceClass {
     /* callbacks */
     void (*reset)(DeviceState *dev);
     DeviceRealize realize;
+    DeviceRealizeChildren realize_children;
     DeviceUnrealize unrealize;
+    DeviceUnrealizeChildren unrealize_children;
 
     /* device state */
     const struct VMStateDescription *vmsd;
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 2/3] qdev: Realize on machine creation done

[Andreas Färber]

As a preparation for Anthony's targetted late realization of devices,
iterate /machine children in qdev_machine_creation_done() and set
realized = true for devices.

Suggested-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 33e874a..ec621cd 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -332,8 +332,37 @@ void qdev_free(DeviceState *dev)
     object_unparent(OBJECT(dev));
 }
 
+static int object_realize_one(Object *obj, void *opaque)
+{
+    Error **errp = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_DEVICE)) {
+        Error *err = NULL;
+
+        object_property_set_bool(obj, true, "realized", &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return -1;
+        }
+        return 0;
+    } else {
+        return object_child_foreach(obj, object_realize_one, opaque);
+    }
+}
+
 void qdev_machine_creation_done(void)
 {
+    Error *err = NULL;
+
+    object_child_foreach(qdev_get_machine(), object_realize_one, &err);
+    if (err != NULL) {
+        qerror_report_err(err);
+        error_free(err);
+        /* In the future we may want to let the user address any errors
+         * via QMP, but for now there's not much we can do here. */
+        exit(1);
+    }
+
     /*
      * ok, initial machine setup is done, starting from now we can
      * only create hotpluggable devices
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 3/3] qdev: Assert no new devices get created during realization

[Andreas Färber]

This is safe because current devices are already realized at this point
and busses are not devices.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/core/qdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index ec621cd..e86e72d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -33,6 +33,7 @@
 #include "qapi/qmp/qjson.h"
 #include "monitor/monitor.h"
 
+static bool devices_realizing;
 int qdev_hotplug = 0;
 static bool qdev_hot_added = false;
 static bool qdev_hot_removed = false;
@@ -354,7 +355,9 @@ void qdev_machine_creation_done(void)
 {
     Error *err = NULL;
 
+    devices_realizing = true;
     object_child_foreach(qdev_get_machine(), object_realize_one, &err);
+    devices_realizing = false;
     if (err != NULL) {
         qerror_report_err(err);
         error_free(err);
@@ -808,6 +811,8 @@ static void device_initfn(Object *obj)
     Property *prop;
     Error *err = NULL;
 
+    assert(!devices_realizing);
+
     if (qdev_hotplug) {
         dev->hotplugged = 1;
         qdev_hot_added = true;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH RFC 0/3] Recursive QOM realize

[Paolo Bonzini]

Il 15/07/2013 15:40, Andreas Färber ha scritto:
> Originally Paolo and me had implemented QOM realize at Object level.
> Paolo's goal was to set realized = true on /machine and it propagating from
> there on. This series now implements {realize,unrealize}_children at
> DeviceState level instead and propagates realized changes along busses rather
> than child<> properties.

You are right that realize must be done after the bus is realized (and
unrealize must be done before the bus).  But I'm afraid this opens a can
of worms.

> On machine creation done, a depth-first search is done
> for devices from /machine, which are then expected to further propagate the
> property change.

How do you ensure that devices are realized before their bus's parent
_and_ before their parent?  With two constraints for each device, we
have a graph, not anymore a tree.  Example:


(1) this is the composition tree

           /machine
     ,------'  |   '------.
/pci-host    /isa      /superio
                   ,----'  '----.
                 /i8254        /i8259


(2) this is the bus tree

                PCI (/pci-host)
                 |
                ISA (/isa)
    ,-----------' '------.
/superio/i8254        /superio/i8259


The constraints are:

- pci-host before isa
- superio before superio/i8254
- superio before superio/i8259
- isa before superio/i8254
- isa before superio/i8259

So the two valid orders are

- /machine, pci-host, superio, isa, superio/i8254, superio/8259
- /machine, pci-host, isa, superio, superio/i8254, superio/8259

You cannot say whether superio or isa are encountered first, so you
cannot say whether it is superio or isa that should "hold off" the visit
of their children (in either the QOM tree or the bus tree).  What avoids
us having to do a full topological ordering of the graph?

Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] Recursive QOM realize

[Andreas Färber]

Am 15.07.2013 16:43, schrieb Paolo Bonzini:
> Il 15/07/2013 15:40, Andreas Färber ha scritto:
>> Originally Paolo and me had implemented QOM realize at Object level.
>> Paolo's goal was to set realized = true on /machine and it propagating from
>> there on. This series now implements {realize,unrealize}_children at
>> DeviceState level instead and propagates realized changes along busses rather
>> than child<> properties.
> 
> You are right that realize must be done after the bus is realized (and
> unrealize must be done before the bus).  But I'm afraid this opens a can
> of worms.
> 
>> On machine creation done, a depth-first search is done
>> for devices from /machine, which are then expected to further propagate the
>> property change.
> 
> How do you ensure that devices are realized before their bus's parent
> _and_ before their parent?  With two constraints for each device, we
> have a graph, not anymore a tree.  Example:
> 
> 
> (1) this is the composition tree
> 
>            /machine
>      ,------'  |   '------.
> /pci-host    /isa      /superio
>                    ,----'  '----.
>                  /i8254        /i8259
> 
> 
> (2) this is the bus tree
> 
>                 PCI (/pci-host)
>                  |
>                 ISA (/isa)
>     ,-----------' '------.
> /superio/i8254        /superio/i8259
> 
> 
> The constraints are:
> 
> - pci-host before isa
> - superio before superio/i8254
> - superio before superio/i8259
> - isa before superio/i8254
> - isa before superio/i8259
> 
> So the two valid orders are
> 
> - /machine, pci-host, superio, isa, superio/i8254, superio/8259
> - /machine, pci-host, isa, superio, superio/i8254, superio/8259
> 
> You cannot say whether superio or isa are encountered first, so you
> cannot say whether it is superio or isa that should "hold off" the visit
> of their children (in either the QOM tree or the bus tree).  What avoids
> us having to do a full topological ordering of the graph?

I would say your example is wrong. :) There should be no /machine/isa
node. Is this hypothetical or do we need to fix qemu.git?

There will be a /machine/sysbus node though, which may lead to similar
ordering uncertainties. However SysBusDevices don't usually have a
hosting device today, so I don't think it's a problem at the moment. And
not for busses either since they are no devices. If we have a
/machine/superio that would be a SysBusDevice (in PReP it would be a
PCIDevice and thus not directly on /machine), we would need to walk its
children to their bus and its parent device and assure it is realized
before - I think there's still sufficient time until 1.6 to get
something minimal sorted out.

Do you have a concrete example where we need such strict constraints?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH RFC 0/3] Recursive QOM realize

[Paolo Bonzini]

Il 15/07/2013 17:06, Andreas Färber ha scritto:
> Am 15.07.2013 16:43, schrieb Paolo Bonzini:
>> Il 15/07/2013 15:40, Andreas Färber ha scritto:
>>> Originally Paolo and me had implemented QOM realize at Object level.
>>> Paolo's goal was to set realized = true on /machine and it propagating from
>>> there on. This series now implements {realize,unrealize}_children at
>>> DeviceState level instead and propagates realized changes along busses rather
>>> than child<> properties.
>>
>> You are right that realize must be done after the bus is realized (and
>> unrealize must be done before the bus).  But I'm afraid this opens a can
>> of worms.
>>
>>> On machine creation done, a depth-first search is done
>>> for devices from /machine, which are then expected to further propagate the
>>> property change.
>>
>> How do you ensure that devices are realized before their bus's parent
>> _and_ before their parent?  With two constraints for each device, we
>> have a graph, not anymore a tree.  Example:
>>
>>
>> (1) this is the composition tree
>>
>>            /machine
>>      ,------'  |   '------.
>> /pci-host    /isa      /superio
>>                    ,----'  '----.
>>                  /i8254        /i8259
>>
>>
>> (2) this is the bus tree
>>
>>                 PCI (/pci-host)
>>                  |
>>                 ISA (/isa)
>>     ,-----------' '------.
>> /superio/i8254        /superio/i8259
>>
>>
>> The constraints are:
>>
>> - pci-host before isa
>> - superio before superio/i8254
>> - superio before superio/i8259
>> - isa before superio/i8254
>> - isa before superio/i8259
>>
>> So the two valid orders are
>>
>> - /machine, pci-host, superio, isa, superio/i8254, superio/8259
>> - /machine, pci-host, isa, superio, superio/i8254, superio/8259
>>
>> You cannot say whether superio or isa are encountered first, so you
>> cannot say whether it is superio or isa that should "hold off" the visit
>> of their children (in either the QOM tree or the bus tree).  What avoids
>> us having to do a full topological ordering of the graph?
> 
> I would say your example is wrong. :) There should be no /machine/isa
> node.

Why not?  And anyway, just replace /superio with /pcnet-isa and
/superio/i8254 with /pcnet-isa/pcnet, and you get the same scenario.

Perhaps you could say my example is wrong, because one of the two
constraints should not be there.  If you have a good argument for that,
I can buy it. :)

> Is this hypothetical or do we need to fix qemu.git?

It is hypothetical, PC is not QOMified yet.

> There will be a /machine/sysbus node though, which may lead to similar
> ordering uncertainties. However SysBusDevices don't usually have a
> hosting device today, so I don't think it's a problem at the moment. And
> not for busses either since they are no devices. If we have a
> /machine/superio that would be a SysBusDevice (in PReP it would be a
> PCIDevice and thus not directly on /machine), we would need to walk its
> children to their bus and its parent device and assure it is realized
> before - I think there's still sufficient time until 1.6 to get
> something minimal sorted out.

I don't think this is 1.6 material, and there is no need to start with
something minimal.  Let's focus on getting things right.

Perhaps "right" means that only one of the two trees need to be visited.
 That's what I did in my old prototype, but I'm fairly convinced it was
wrong.

> Do you have a concrete example where we need such strict constraints?

Does there need to be a concrete example?

Paolo

Re: [Qemu-devel] [PATCH RFC 0/3] Recursive QOM realize

[Andreas Färber]

Am 15.07.2013 17:37, schrieb Paolo Bonzini:
> Il 15/07/2013 17:06, Andreas Färber ha scritto:
>> Am 15.07.2013 16:43, schrieb Paolo Bonzini:
>>> Il 15/07/2013 15:40, Andreas Färber ha scritto:
>>>> Originally Paolo and me had implemented QOM realize at Object level.
>>>> Paolo's goal was to set realized = true on /machine and it propagating from
>>>> there on. This series now implements {realize,unrealize}_children at
>>>> DeviceState level instead and propagates realized changes along busses rather
>>>> than child<> properties.
>>>
>>> You are right that realize must be done after the bus is realized (and
>>> unrealize must be done before the bus).  But I'm afraid this opens a can
>>> of worms.
>>>
>>>> On machine creation done, a depth-first search is done
>>>> for devices from /machine, which are then expected to further propagate the
>>>> property change.
>>>
>>> How do you ensure that devices are realized before their bus's parent
>>> _and_ before their parent?  With two constraints for each device, we
>>> have a graph, not anymore a tree.  Example:
>>>
>>>
>>> (1) this is the composition tree
>>>
>>>            /machine
>>>      ,------'  |   '------.
>>> /pci-host    /isa      /superio
>>>                    ,----'  '----.
>>>                  /i8254        /i8259
>>>
>>>
>>> (2) this is the bus tree
>>>
>>>                 PCI (/pci-host)
>>>                  |
>>>                 ISA (/isa)
>>>     ,-----------' '------.
>>> /superio/i8254        /superio/i8259
>>>
>>>
>>> The constraints are:
>>>
>>> - pci-host before isa
>>> - superio before superio/i8254
>>> - superio before superio/i8259
>>> - isa before superio/i8254
>>> - isa before superio/i8259
>>>
>>> So the two valid orders are
>>>
>>> - /machine, pci-host, superio, isa, superio/i8254, superio/8259
>>> - /machine, pci-host, isa, superio, superio/i8254, superio/8259
>>>
>>> You cannot say whether superio or isa are encountered first, so you
>>> cannot say whether it is superio or isa that should "hold off" the visit
>>> of their children (in either the QOM tree or the bus tree).  What avoids
>>> us having to do a full topological ordering of the graph?
>>
>> I would say your example is wrong. :) There should be no /machine/isa
>> node.
> 
> Why not?  And anyway, just replace /superio with /pcnet-isa and
> /superio/i8254 with /pcnet-isa/pcnet, and you get the same scenario.
> 
> Perhaps you could say my example is wrong, because one of the two
> constraints should not be there.  If you have a good argument for that,
> I can buy it. :)
> 
>> Is this hypothetical or do we need to fix qemu.git?
> 
> It is hypothetical, PC is not QOMified yet.
> 
>> There will be a /machine/sysbus node though, which may lead to similar
>> ordering uncertainties. However SysBusDevices don't usually have a
>> hosting device today, so I don't think it's a problem at the moment. And
>> not for busses either since they are no devices. If we have a
>> /machine/superio that would be a SysBusDevice (in PReP it would be a
>> PCIDevice and thus not directly on /machine), we would need to walk its
>> children to their bus and its parent device and assure it is realized
>> before - I think there's still sufficient time until 1.6 to get
>> something minimal sorted out.
> 
> I don't think this is 1.6 material, and there is no need to start with
> something minimal.  Let's focus on getting things right.
> 
> Perhaps "right" means that only one of the two trees need to be visited.
>  That's what I did in my old prototype, but I'm fairly convinced it was
> wrong.
> 
>> Do you have a concrete example where we need such strict constraints?
> 
> Does there need to be a concrete example?

My interest is this: Take a look at my tegra branch or PReP PCI or other
composited SysBusDevices. Today I need to realize child devices in
DeviceClass::realize, when I know that they should be realized through
realize_children instead, outside of realize. So I don't want to convert
all SysBusDevices to hand-code recursive realization (or at least the
current version of it) and then go through and remove all that again in
favor of realize_children. And since we are converting some devices for
1.6 I would like to have the work-saving solution for 1.6 as well. So it
is not about /machine (2/3 is unused) but about the realize_children
infrastructure (1/3).

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg