[Qemu-devel] [PATCH v3 0/2] QOM realize, device-only

[Qemu-devel] [PATCH v3 0/2] QOM realize, device-only

[Andreas Färber]

Hello Anthony,

Next iteration, with documentation.

Available from:
https://github.com/afaerber/qemu-cpu/commits/realize-qdev.v3
git://github.com/afaerber/qemu-cpu.git realize-qdev.v3

Regards,
Andreas

v2 -> v3:
* Rebased onto qdev ObjectClass::unparent change.
* Documented DeviceState::realized field.
* Documented DeviceClass::realize vs. DeviceClass::init semantics,
  suggested by Eduardo. Utterly verbose, short of an example.

v1 -> v2:
* Dropped general-purpose qdev cleanups.
* Dropped ISA conversion, to be reposted as follow-up.

>From combined qom-next proposal:
* Implemented for DeviceClass rather than for ObjectClass.

>From my initial proposal:
* Merged Object::unrealize, proposed by Paolo.

Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>

Andreas Färber (2):
  qdev: Fold state enum into bool realized
  qdev: Prepare "realized" property

 hw/qdev-addr.c              |    2 +-
 hw/qdev-core.h              |   70 ++++++++++++++++++++++++----
 hw/qdev-properties-system.c |    4 +-
 hw/qdev-properties.c        |   24 +++++-----
 hw/qdev.c                   |  106 +++++++++++++++++++++++++++++++------------
 5 Dateien geändert, 153 Zeilen hinzugefügt(+), 53 Zeilen entfernt(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v3 1/2] qdev: Fold state enum into bool realized

[Andreas Färber]

Whether the device was initialized or not is QOM-level information and
currently unused. Drop it from device. This leaves the boolean state of
whether or not DeviceClass::init was called or not, a.k.a. "realized".

Suggested-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/qdev-addr.c              |    2 +-
 hw/qdev-core.h              |   18 ++++++++++--------
 hw/qdev-properties-system.c |    4 ++--
 hw/qdev-properties.c        |   24 ++++++++++++------------
 hw/qdev.c                   |   12 ++++++------
 5 Dateien geändert, 31 Zeilen hinzugefügt(+), 29 Zeilen entfernt(-)

diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 3bfe101..b4388f6 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -40,7 +40,7 @@ static void set_taddr(Object *obj, Visitor *v, void *opaque,
     Error *local_err = NULL;
     int64_t value;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index fdf14ec..494902d 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -8,11 +8,6 @@
 #include "hw/irq.h"
 #include "qapi/error.h"
 
-enum DevState {
-    DEV_STATE_CREATED = 1,
-    DEV_STATE_INITIALIZED,
-};
-
 enum {
     DEV_NVECTORS_UNSPECIFIED = -1,
 };
@@ -49,13 +44,20 @@ typedef struct DeviceClass {
     const char *bus_type;
 } DeviceClass;
 
-/* This structure should not be accessed directly.  We declare it here
-   so that it can be embedded in individual device state structures.  */
+/**
+ * DeviceState:
+ * @realized: Indicates whether the device has been fully constructed.
+ *
+ * This structure should not be accessed directly.  We declare it here
+ * so that it can be embedded in individual device state structures.
+ */
 struct DeviceState {
+    /*< private >*/
     Object parent_obj;
+    /*< public >*/
 
     const char *id;
-    enum DevState state;
+    bool realized;
     QemuOpts *opts;
     int hotplugged;
     BusState *parent_bus;
diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
index c73c713..ce0f793 100644
--- a/hw/qdev-properties-system.c
+++ b/hw/qdev-properties-system.c
@@ -42,7 +42,7 @@ static void set_pointer(Object *obj, Visitor *v, Property *prop,
     char *str;
     int ret;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -254,7 +254,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
     int32_t id;
     NetClientState *hubport;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f724357..a8a31f5 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -32,7 +32,7 @@ static void set_enum(Object *obj, Visitor *v, void *opaque,
     Property *prop = opaque;
     int *ptr = qdev_get_prop_ptr(dev, prop);
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -85,7 +85,7 @@ static void set_bit(Object *obj, Visitor *v, void *opaque,
     Error *local_err = NULL;
     bool value;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -125,7 +125,7 @@ static void set_uint8(Object *obj, Visitor *v, void *opaque,
     Property *prop = opaque;
     uint8_t *ptr = qdev_get_prop_ptr(dev, prop);
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -192,7 +192,7 @@ static void set_uint16(Object *obj, Visitor *v, void *opaque,
     Property *prop = opaque;
     uint16_t *ptr = qdev_get_prop_ptr(dev, prop);
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -225,7 +225,7 @@ static void set_uint32(Object *obj, Visitor *v, void *opaque,
     Property *prop = opaque;
     uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -250,7 +250,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque,
     Property *prop = opaque;
     int32_t *ptr = qdev_get_prop_ptr(dev, prop);
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -323,7 +323,7 @@ static void set_uint64(Object *obj, Visitor *v, void *opaque,
     Property *prop = opaque;
     uint64_t *ptr = qdev_get_prop_ptr(dev, prop);
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -413,7 +413,7 @@ static void set_string(Object *obj, Visitor *v, void *opaque,
     Error *local_err = NULL;
     char *str;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -477,7 +477,7 @@ static void set_mac(Object *obj, Visitor *v, void *opaque,
     int i, pos;
     char *str, *p;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -569,7 +569,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void *opaque,
     Error *local_err = NULL;
     char *str;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -640,7 +640,7 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque,
     const int64_t min = 512;
     const int64_t max = 32768;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -708,7 +708,7 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
     unsigned long dom = 0, bus = 0;
     unsigned int slot = 0, func = 0;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
diff --git a/hw/qdev.c b/hw/qdev.c
index e2a5c57..07eef5c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -151,7 +151,7 @@ int qdev_init(DeviceState *dev)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     int rc;
 
-    assert(dev->state == DEV_STATE_CREATED);
+    assert(!dev->realized);
 
     rc = dc->init(dev);
     if (rc < 0) {
@@ -174,7 +174,7 @@ int qdev_init(DeviceState *dev)
                                        dev->instance_id_alias,
                                        dev->alias_required_for_version);
     }
-    dev->state = DEV_STATE_INITIALIZED;
+    dev->realized = true;
     if (dev->hotplugged) {
         device_reset(dev);
     }
@@ -184,7 +184,7 @@ int qdev_init(DeviceState *dev)
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version)
 {
-    assert(dev->state == DEV_STATE_CREATED);
+    assert(!dev->realized);
     dev->instance_id_alias = alias_id;
     dev->alias_required_for_version = required_for_version;
 }
@@ -541,7 +541,7 @@ static void qdev_set_legacy_property(Object *obj, Visitor *v, void *opaque,
     char *ptr = NULL;
     int ret;
 
-    if (dev->state != DEV_STATE_CREATED) {
+    if (dev->realized) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
@@ -648,7 +648,7 @@ static void device_initfn(Object *obj)
     }
 
     dev->instance_id_alias = -1;
-    dev->state = DEV_STATE_CREATED;
+    dev->realized = false;
 
     class = object_get_class(OBJECT(dev));
     do {
@@ -671,7 +671,7 @@ static void device_finalize(Object *obj)
     BusState *bus;
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
-    if (dev->state == DEV_STATE_INITIALIZED) {
+    if (dev->realized) {
         while (dev->num_child_bus) {
             bus = QLIST_FIRST(&dev->child_bus);
             qbus_free(bus);
-- 
1.7.10.4

[Qemu-devel] [PATCH v3 2/2] qdev: Prepare "realized" property

[Andreas Färber]

Introduce the QOM realizefn suggested by Anthony.
Detailed documentation is supplied in the qdev header.

For now this implements a default DeviceClass::realize callback that
just wraps DeviceClass::init, which it deprecates.
Once all devices have been converted to DeviceClass::realize,
DeviceClass::init is to be removed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Anthony Liguori <anthony@codemonkey.ws>
---
 hw/qdev-core.h |   52 +++++++++++++++++++++++++++++-
 hw/qdev.c      |   96 ++++++++++++++++++++++++++++++++++++++++++--------------
 2 Dateien geändert, 123 Zeilen hinzugefügt(+), 25 Zeilen entfernt(-)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index 494902d..487a2bf 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -20,11 +20,59 @@ enum {
 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 (*DeviceUnrealize)(DeviceState *dev, Error **errp);
 
 struct VMStateDescription;
 
+/**
+ * DeviceClass:
+ * @props: Properties accessing state fields.
+ * @realize: Callback function invoked when the #DeviceState:realized
+ * property is changed to %true. The default invokes @init if not %NULL.
+ * @unrealize: Callback function invoked when the #DeviceState:realized
+ * property is changed to %false.
+ * @init: Callback function invoked when the #DeviceState::realized property
+ * is changed to %true. Deprecated, new types inheriting directly from
+ * TYPE_DEVICE should use @realize instead, new leaf types should consult
+ * their respective parent type.
+ *
+ * # Realization #
+ * Devices are constructed in two stages,
+ * 1) object instantiation via object_initialize() and
+ * 2) device realization via #DeviceState:realized property.
+ * The former may not fail (it might assert or exit), the latter may return
+ * error information to the caller and must be re-entrant.
+ * Trivial field initializations should go into #TypeInfo.instance_init.
+ * Operations depending on @props static properties should go into @realize.
+ * After successful realization, setting static properties will fail.
+ *
+ * As an interim step, the #DeviceState:realized property is set by deprecated
+ * functions qdev_init() and 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
+ * set in @realize will not be introspectable beforehand. Therefore devices
+ * must not create children during @realize; they should initialize them via
+ * object_initialize() in their own #TypeInfo.instance_init and forward the
+ * realization events appropriately.
+ *
+ * The @init callback is considered private to a particular bus implementation
+ * (immediate abstract child types of TYPE_DEVICE). Derived leaf types set an
+ * "init" callback on their parent class instead.
+ * Any type may override the @realize and/or @unrealize callbacks but needs
+ * to call (and thus save) the parent type's implementation if so desired.
+ * Usually this means storing the previous value of, e.g., @realized inside
+ * the type's class structure and overwriting it with a function that first
+ * invokes the stored callback, then performs any additional steps.
+ * If a type derived directly from TYPE_DEVICE implements @realize, it does
+ * not need to implement @init and therefore does not need to store and call
+ * #DeviceClass' default @realize callback.
+ */
 typedef struct DeviceClass {
+    /*< private >*/
     ObjectClass parent_class;
+    /*< public >*/
 
     const char *fw_name;
     const char *desc;
@@ -33,12 +81,14 @@ typedef struct DeviceClass {
 
     /* callbacks */
     void (*reset)(DeviceState *dev);
+    DeviceRealize realize;
+    DeviceUnrealize unrealize;
 
     /* device state */
     const struct VMStateDescription *vmsd;
 
     /* Private to qdev / bus.  */
-    qdev_initfn init;
+    qdev_initfn init; /* TODO remove, once users are converted to realize */
     qdev_event unplug;
     qdev_event exit;
     const char *bus_type;
diff --git a/hw/qdev.c b/hw/qdev.c
index 07eef5c..13697c8 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -148,37 +148,30 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
    Return 0 on success.  */
 int qdev_init(DeviceState *dev)
 {
-    DeviceClass *dc = DEVICE_GET_CLASS(dev);
-    int rc;
+    Error *local_err = NULL;
 
     assert(!dev->realized);
 
-    rc = dc->init(dev);
-    if (rc < 0) {
+    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+    if (local_err != NULL) {
+        error_free(local_err);
         qdev_free(dev);
-        return rc;
+        return -1;
     }
+    return 0;
+}
 
-    if (!OBJECT(dev)->parent) {
-        static int unattached_count = 0;
-        gchar *name = g_strdup_printf("device[%d]", unattached_count++);
-
-        object_property_add_child(container_get(qdev_get_machine(),
-                                                "/unattached"),
-                                  name, OBJECT(dev), NULL);
-        g_free(name);
-    }
+static void device_realize(DeviceState *dev, Error **err)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
-    if (qdev_get_vmsd(dev)) {
-        vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
-                                       dev->instance_id_alias,
-                                       dev->alias_required_for_version);
-    }
-    dev->realized = true;
-    if (dev->hotplugged) {
-        device_reset(dev);
+    if (dc->init) {
+        int rc = dc->init(dev);
+        if (rc < 0) {
+            error_setg(err, "Device initialization failed.");
+            return;
+        }
     }
-    return 0;
 }
 
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
@@ -636,6 +629,55 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
     assert_no_error(local_err);
 }
 
+static bool device_get_realized(Object *obj, Error **err)
+{
+    DeviceState *dev = DEVICE(obj);
+    return dev->realized;
+}
+
+static void device_set_realized(Object *obj, bool value, Error **err)
+{
+    DeviceState *dev = DEVICE(obj);
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
+    Error *local_err = NULL;
+
+    if (value && !dev->realized) {
+        if (dc->realize) {
+            dc->realize(dev, &local_err);
+        }
+
+        if (!obj->parent && local_err == NULL) {
+            static int unattached_count;
+            gchar *name = g_strdup_printf("device[%d]", unattached_count++);
+
+            object_property_add_child(container_get(qdev_get_machine(),
+                                                    "/unattached"),
+                                      name, obj, &local_err);
+            g_free(name);
+        }
+
+        if (qdev_get_vmsd(dev) && local_err == NULL) {
+            vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
+                                           dev->instance_id_alias,
+                                           dev->alias_required_for_version);
+        }
+        if (dev->hotplugged && local_err == NULL) {
+            device_reset(dev);
+        }
+    } else if (!value && dev->realized) {
+        if (dc->unrealize) {
+            dc->unrealize(dev, &local_err);
+        }
+    }
+
+    if (local_err != NULL) {
+        error_propagate(err, local_err);
+        return;
+    }
+
+    dev->realized = value;
+}
+
 static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -650,6 +692,9 @@ static void device_initfn(Object *obj)
     dev->instance_id_alias = -1;
     dev->realized = false;
 
+    object_property_add_bool(obj, "realized",
+                             device_get_realized, device_set_realized, NULL);
+
     class = object_get_class(OBJECT(dev));
     do {
         for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) {
@@ -709,7 +754,10 @@ static void device_unparent(Object *obj)
 
 static void device_class_init(ObjectClass *class, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(class);
+
     class->unparent = device_unparent;
+    dc->realize = device_realize;
 }
 
 void device_reset(DeviceState *dev)
@@ -732,7 +780,7 @@ Object *qdev_get_machine(void)
     return dev;
 }
 
-static TypeInfo device_type_info = {
+static const TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(DeviceState),
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v3 0/2] QOM realize, device-only

[Igor Mammedov]

ping

Re: [Qemu-devel] [PATCH v3 0/2] QOM realize, device-only

[Anthony Liguori]

Thanks, applied.

Regards,

Anthony Liguori