[Qemu-devel] [PATCH v4 0/9] virtio-blk: use alias properties in transport devices

[Qemu-devel] [PATCH v4 0/9] virtio-blk: use alias properties in transport devices

[Stefan Hajnoczi]

v4:
 * Coding style: typedef struct { on a single line [Andreas]
 * Add "dataplane: bail out on unsupported transport" for s390-virtio [Cornelia]

v3:
 * Split qdev_alias_all_properties() into its own patch [Peter Crosthwaite]
 * Do not dereference DEVICE_CLASS(class) inline [Peter Crosthwaite]

v2:
 * Add qdev_alias_all_properties() instead of virtio-blk-specific function [Paolo]
 * Explain refcount handling in doc comment [Paolo]
 * Fix "property" duplicate typo [Peter Crosthwaite]
 * Add "the same object or" to clarify commit description [Igor]

Thanks for the feedback on the RFC.  This time around the alias property is
implemented at the QOM property level instead of at the qdev property level.

Note that this series only addresses virtio-blk.  In later series we can
convert virtio net, scsi, rng, and serial.

The virtio transport/device split is broken as follows:

1. The virtio-blk device is never finalized because the transport devices
   (virtio-blk-pci and friends) leak the refcount.

2. If we fix the refcount leak then we double-free the 'serial' string property
   upon hot unplug since its char* is copied into the virtio-blk device which
   has an identical 'serial' qdev property.

This series solves both of these problems as follows:

1. Introduce a QOM alias property that lets the transport device forward
   property accesses into the virtio device (the child).

2. Use alias properties in transport devices, instead of keeping a duplicate
   copy of the VirtIOBlkConf struct.

3. Fix the virtio-blk device refcount leak.  It's now safe to do this since the
   double-free has been resolved.

Tested that hotplug/hotunplug of virtio-blk-pci still works.

Cornelia Huck (1):
  dataplane: bail out on unsupported transport

Stefan Hajnoczi (8):
  qom: add object_property_add_alias()
  virtio-blk: avoid qdev property definition duplication
  virtio-blk: move x-data-plane qdev property to virtio-blk.h
  qdev: add qdev_alias_all_properties()
  virtio-blk: use aliases instead of duplicate qdev properties
  virtio-blk: drop virtio_blk_set_conf()
  virtio: fix virtio-blk child refcount in transports
  virtio-blk: move qdev properties into virtio-blk.c

 hw/block/dataplane/virtio-blk.c | 10 ++++++++
 hw/block/virtio-blk.c           | 18 +++++++++------
 hw/core/qdev.c                  | 21 +++++++++++++++++
 hw/s390x/s390-virtio-bus.c      | 10 ++------
 hw/s390x/s390-virtio-bus.h      |  1 -
 hw/s390x/virtio-ccw.c           |  7 ++----
 hw/s390x/virtio-ccw.h           |  1 -
 hw/virtio/virtio-pci.c          |  7 ++----
 hw/virtio/virtio-pci.h          |  1 -
 include/hw/qdev-properties.h    |  2 ++
 include/hw/virtio/virtio-blk.h  | 19 ---------------
 include/qom/object.h            | 20 ++++++++++++++++
 qom/object.c                    | 51 +++++++++++++++++++++++++++++++++++++++++
 13 files changed, 121 insertions(+), 47 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v4 1/9] qom: add object_property_add_alias()

[Stefan Hajnoczi]

Sometimes an object needs to present a property which is actually on
another object, or it needs to provide an alias name for an existing
property.

Examples:
  a.foo -> b.foo
  a.old_name -> a.new_name

The new object_property_add_alias() API allows objects to alias a
property on the same object or another object.  The source and target
names can be different.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
v4:
 * Coding style: typedef struct { on a single line [Andreas]

v2:
 * Explain refcount handling in doc comment [Paolo]
 * Fix "property" duplicate typo [Peter Crosthwaite]
 * Add "the same object or" to clarify commit description [Igor]
---
 include/qom/object.h | 20 ++++++++++++++++++++
 qom/object.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index a641dcd..854a0d5 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1203,6 +1203,26 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
                                     const uint64_t *v, Error **Errp);
 
 /**
+ * object_property_add_alias:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @target_obj: the object to forward property access to
+ * @target_name: the name of the property on the forwarded object
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Add an alias for a property on an object.  This function will add a property
+ * of the same type as the forwarded property.
+ *
+ * The caller must ensure that <code>@target_obj</code> stays alive as long as
+ * this property exists.  In the case of a child object or an alias on the same
+ * object this will be the case.  For aliases to other objects the caller is
+ * responsible for taking a reference.
+ */
+void object_property_add_alias(Object *obj, const char *name,
+                               Object *target_obj, const char *target_name,
+                               Error **errp);
+
+/**
  * object_child_foreach:
  * @obj: the object whose children will be navigated
  * @fn: the iterator function to be called
diff --git a/qom/object.c b/qom/object.c
index e42b254..b83c3a7 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1515,6 +1515,57 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
                         NULL, NULL, (void *)v, errp);
 }
 
+typedef struct {
+    Object *target_obj;
+    const char *target_name;
+} AliasProperty;
+
+static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    AliasProperty *prop = opaque;
+
+    object_property_get(prop->target_obj, v, prop->target_name, errp);
+}
+
+static void property_set_alias(Object *obj, struct Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    AliasProperty *prop = opaque;
+
+    object_property_set(prop->target_obj, v, prop->target_name, errp);
+}
+
+static void property_release_alias(Object *obj, const char *name, void *opaque)
+{
+    AliasProperty *prop = opaque;
+
+    g_free(prop);
+}
+
+void object_property_add_alias(Object *obj, const char *name,
+                               Object *target_obj, const char *target_name,
+                               Error **errp)
+{
+    AliasProperty *prop;
+    ObjectProperty *target_prop;
+
+    target_prop = object_property_find(target_obj, target_name, errp);
+    if (!target_prop) {
+        return;
+    }
+
+    prop = g_malloc(sizeof(*prop));
+    prop->target_obj = target_obj;
+    prop->target_name = target_name;
+
+    object_property_add(obj, name, target_prop->type,
+                        property_get_alias,
+                        property_set_alias,
+                        property_release_alias,
+                        prop, errp);
+}
+
 static void object_instance_init(Object *obj)
 {
     object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
-- 
1.9.3

[Qemu-devel] [PATCH v4 2/9] virtio-blk: avoid qdev property definition duplication

[Stefan Hajnoczi]

It becomes unwiedly to duplicate all virtio-blk qdev property
definitions due to an #ifdef.  The C preprocessor syntax makes it a
little hard to resolve this cleanly but we can extract the #ifdef and
call a macro it defines later.

Avoiding duplication is important since it will only get worse when we
move the x-data-plane qdev property here too.  We'd have a combinatorial
explosion since x-data-plane has its own #ifdef.

Suggested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 include/hw/virtio/virtio-blk.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 4bc9b54..7bafad5 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -137,21 +137,19 @@ typedef struct VirtIOBlock {
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
 
 #ifdef __linux__
-#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
-        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
-        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
-        DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
-        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
-        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),                \
-        DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
+#define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field) \
+        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
 #else
+#define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)
+#endif
+
 #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
+        DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)                    \
         DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
         DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
         DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
         DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
         DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
-#endif /* __linux__ */
 
 void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 3/9] dataplane: bail out on unsupported transport

[Stefan Hajnoczi]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

If the virtio transport does not support notifiers (like s390-virtio),
we can't use dataplane. Bail out early and let the user know what is
wrong.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index c10b7b7..92ebb44 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -331,6 +331,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
 {
     VirtIOBlockDataPlane *s;
     Error *local_err = NULL;
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
     *dataplane = NULL;
 
@@ -338,6 +340,14 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
         return;
     }
 
+    /* Don't try if transport does not support notifiers. */
+    if (!k->set_guest_notifiers || !k->set_host_notifier) {
+        error_setg(errp,
+                   "device is incompatible with x-data-plane "
+                   "(transport does not support notifiers)");
+        return;
+    }
+
     /* If dataplane is (re-)enabled while the guest is running there could be
      * block jobs that can conflict.
      */
-- 
1.9.3

[Qemu-devel] [PATCH v4 4/9] virtio-blk: move x-data-plane qdev property to virtio-blk.h

[Stefan Hajnoczi]

Move the x-data-plane property.  Originally it was outside since not
every transport may wish to support dataplane.  But that makes little
sense when we have a dedicated CONFIG_VIRTIO_BLK_DATA_PLANE ifdef
already.

This move makes it easier to switch to property aliases in the next
patch.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/s390x/virtio-ccw.c          | 3 ---
 hw/virtio/virtio-pci.c         | 3 ---
 include/hw/virtio/virtio-blk.h | 8 ++++++++
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 05656a2..d7ff0a0 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1403,9 +1403,6 @@ static Property virtio_ccw_blk_properties[] = {
     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    DEFINE_PROP_BIT("x-data-plane", VirtIOBlkCcw, blk.data_plane, 0, false),
-#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..0751a1e 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1067,9 +1067,6 @@ static Property virtio_blk_pci_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, blk.data_plane, 0, false),
-#endif
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 7bafad5..56c98b1 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -143,8 +143,16 @@ typedef struct VirtIOBlock {
 #define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)
 #endif
 
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+#define DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field) \
+    DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0, false),
+#else
+#define DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field)
+#endif
+
 #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
         DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)                    \
+        DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field)               \
         DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
         DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
         DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
-- 
1.9.3

[Qemu-devel] [PATCH v4 5/9] qdev: add qdev_alias_all_properties()

[Stefan Hajnoczi]

The qdev_alias_all_properties() function creates QOM alias properties
for each qdev property on a DeviceState.  This is useful for parent
objects that wish to forward property accesses to their children.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
v3:
 * Split qdev_alias_all_properties() into its own patch [Peter Crosthwaite]
 * Do not dereference DEVICE_CLASS(class) inline [Peter Crosthwaite]
---
 hw/core/qdev.c               | 21 +++++++++++++++++++++
 include/hw/qdev-properties.h |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e65a5aa..850cd9e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -776,6 +776,27 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
     }
 }
 
+/* @qdev_alias_all_properties - Add alias properties to the source object for
+ * all qdev properties on the target DeviceState.
+ */
+void qdev_alias_all_properties(DeviceState *target, Object *source)
+{
+    ObjectClass *class;
+    Property *prop;
+
+    class = object_get_class(OBJECT(target));
+    do {
+        DeviceClass *dc = DEVICE_CLASS(class);
+
+        for (prop = dc->props; prop && prop->name; prop++) {
+            object_property_add_alias(source, prop->name,
+                                      OBJECT(target), prop->name,
+                                      &error_abort);
+        }
+        class = object_class_get_parent(class);
+    } while (class != object_class_by_name(TYPE_DEVICE));
+}
+
 static bool device_get_realized(Object *obj, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index c962b6b..3726bf3 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -193,6 +193,8 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
  */
 void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
 
+void qdev_alias_all_properties(DeviceState *target, Object *source);
+
 /**
  * @qdev_prop_set_after_realize:
  * @dev: device
-- 
1.9.3

[Qemu-devel] [PATCH v4 6/9] virtio-blk: use aliases instead of duplicate qdev properties

[Stefan Hajnoczi]

virtio-blk-pci, virtio-blk-s390, and virtio-blk-ccw all duplicate the
qdev properties of their VirtIOBlock child.  This approach does not work
well with string or pointer properties since we must be careful about
leaking or double-freeing them.

Use the QOM alias property to forward property accesses to the
VirtIOBlock child.  This way no duplication is necessary.

Remember to stop calling virtio_blk_set_conf() so that we don't clobber
the values already set on the VirtIOBlock instance.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
v3:
 * Split qdev_alias_all_properties() into its own patch [Peter Crosthwaite]

v2:
 * Add qdev_alias_all_properties() instead of virtio-blk-specific
   function [Paolo]
---
 hw/s390x/s390-virtio-bus.c | 9 +--------
 hw/s390x/s390-virtio-bus.h | 1 -
 hw/s390x/virtio-ccw.c      | 3 +--
 hw/s390x/virtio-ccw.h      | 1 -
 hw/virtio/virtio-pci.c     | 3 +--
 hw/virtio/virtio-pci.h     | 1 -
 6 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 7c8c81b..38984ab 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -167,7 +167,6 @@ static int s390_virtio_blk_init(VirtIOS390Device *s390_dev)
 {
     VirtIOBlkS390 *dev = VIRTIO_BLK_S390(s390_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    virtio_blk_set_conf(vdev, &(dev->blk));
     qdev_set_parent_bus(vdev, BUS(&s390_dev->bus));
     if (qdev_init(vdev) < 0) {
         return -1;
@@ -180,6 +179,7 @@ static void s390_virtio_blk_instance_init(Object *obj)
     VirtIOBlkS390 *dev = VIRTIO_BLK_S390(obj);
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
+    qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
 }
 
 static int s390_virtio_serial_init(VirtIOS390Device *s390_dev)
@@ -513,18 +513,11 @@ static const TypeInfo s390_virtio_net = {
     .class_init    = s390_virtio_net_class_init,
 };
 
-static Property s390_virtio_blk_properties[] = {
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
     VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
 
     k->init = s390_virtio_blk_init;
-    dc->props = s390_virtio_blk_properties;
 }
 
 static const TypeInfo s390_virtio_blk = {
diff --git a/hw/s390x/s390-virtio-bus.h b/hw/s390x/s390-virtio-bus.h
index ac81bd8..ffd0df7 100644
--- a/hw/s390x/s390-virtio-bus.h
+++ b/hw/s390x/s390-virtio-bus.h
@@ -124,7 +124,6 @@ void s390_virtio_reset_idx(VirtIOS390Device *dev);
 typedef struct VirtIOBlkS390 {
     VirtIOS390Device parent_obj;
     VirtIOBlock vdev;
-    VirtIOBlkConf blk;
 } VirtIOBlkS390;
 
 /* virtio-scsi-s390 */
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index d7ff0a0..9fa6f32 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -800,7 +800,6 @@ static int virtio_ccw_blk_init(VirtioCcwDevice *ccw_dev)
 {
     VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(ccw_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    virtio_blk_set_conf(vdev, &(dev->blk));
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
     if (qdev_init(vdev) < 0) {
         return -1;
@@ -814,6 +813,7 @@ static void virtio_ccw_blk_instance_init(Object *obj)
     VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(obj);
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
+    qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
 }
 
 static int virtio_ccw_serial_init(VirtioCcwDevice *ccw_dev)
@@ -1400,7 +1400,6 @@ static const TypeInfo virtio_ccw_net = {
 static Property virtio_ccw_blk_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index b8b8a8a..5a1f16e 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -144,7 +144,6 @@ typedef struct VHostSCSICcw {
 typedef struct VirtIOBlkCcw {
     VirtioCcwDevice parent_obj;
     VirtIOBlock vdev;
-    VirtIOBlkConf blk;
 } VirtIOBlkCcw;
 
 /* virtio-balloon-ccw */
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 0751a1e..3bb782f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1068,7 +1068,6 @@ static Property virtio_blk_pci_properties[] = {
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1076,7 +1075,6 @@ static int virtio_blk_pci_init(VirtIOPCIProxy *vpci_dev)
 {
     VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
-    virtio_blk_set_conf(vdev, &(dev->blk));
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
     if (qdev_init(vdev) < 0) {
         return -1;
@@ -1104,6 +1102,7 @@ static void virtio_blk_pci_instance_init(Object *obj)
     VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(obj);
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
+    qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
 }
 
 static const TypeInfo virtio_blk_pci_info = {
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index dc332ae..1cea157 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -131,7 +131,6 @@ struct VHostSCSIPCI {
 struct VirtIOBlkPCI {
     VirtIOPCIProxy parent_obj;
     VirtIOBlock vdev;
-    VirtIOBlkConf blk;
 };
 
 /*
-- 
1.9.3

[Qemu-devel] [PATCH v4 7/9] virtio-blk: drop virtio_blk_set_conf()

[Stefan Hajnoczi]

This function is no longer used since parent objects now use child
aliases to set the VirtIOBlkConf directly.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/block/virtio-blk.c          | 6 ------
 include/hw/virtio/virtio-blk.h | 2 --
 2 files changed, 8 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 85aa871..cfb8fa3 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -655,12 +655,6 @@ static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
-void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
-{
-    VirtIOBlock *s = VIRTIO_BLK(dev);
-    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
-}
-
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
 /* Disable dataplane thread during live migration since it does not
  * update the dirty memory bitmap yet.
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 56c98b1..59cf1f2 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -159,8 +159,6 @@ typedef struct VirtIOBlock {
         DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
         DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
 
-void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
-
 int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
                                VirtQueueElement *elem);
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 8/9] virtio: fix virtio-blk child refcount in transports

[Stefan Hajnoczi]

object_initialize() leaves the object with a refcount of 1.
object_property_add_child() adds its own reference which is dropped
again when the property is deleted.

The upshot of this is that we always have a refcount >= 1.  Upon hot
unplug the virtio-blk child is not finalized!

Drop our reference after the child property has been added to the
parent.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/s390x/s390-virtio-bus.c | 1 +
 hw/s390x/virtio-ccw.c      | 1 +
 hw/virtio/virtio-pci.c     | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 38984ab..3438a88 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -179,6 +179,7 @@ static void s390_virtio_blk_instance_init(Object *obj)
     VirtIOBlkS390 *dev = VIRTIO_BLK_S390(obj);
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
+    object_unref(OBJECT(&dev->vdev));
     qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
 }
 
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 9fa6f32..0553fea 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -813,6 +813,7 @@ static void virtio_ccw_blk_instance_init(Object *obj)
     VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(obj);
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
+    object_unref(OBJECT(&dev->vdev));
     qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 3bb782f..abf05a9 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1102,6 +1102,7 @@ static void virtio_blk_pci_instance_init(Object *obj)
     VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(obj);
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
+    object_unref(OBJECT(&dev->vdev));
     qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 8/9] virtio: fix virtio-blk child refcount in transports

[Peter Crosthwaite]

On Wed, Jun 18, 2014 at 7:58 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> object_initialize() leaves the object with a refcount of 1.
> object_property_add_child() adds its own reference which is dropped
> again when the property is deleted.
>
> The upshot of this is that we always have a refcount >= 1.  Upon hot
> unplug the virtio-blk child is not finalized!
>
> Drop our reference after the child property has been added to the
> parent.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Still not a fan, but I can't see a better way without more core QOM or
hotplug plumbing and that's not going to happen inside 2.1. So

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Regards,
Peter

> ---
>  hw/s390x/s390-virtio-bus.c | 1 +
>  hw/s390x/virtio-ccw.c      | 1 +
>  hw/virtio/virtio-pci.c     | 1 +
>  3 files changed, 3 insertions(+)
>
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 38984ab..3438a88 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -179,6 +179,7 @@ static void s390_virtio_blk_instance_init(Object *obj)
>      VirtIOBlkS390 *dev = VIRTIO_BLK_S390(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> +    object_unref(OBJECT(&dev->vdev));
>      qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
>  }
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 9fa6f32..0553fea 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -813,6 +813,7 @@ static void virtio_ccw_blk_instance_init(Object *obj)
>      VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> +    object_unref(OBJECT(&dev->vdev));
>      qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
>  }
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 3bb782f..abf05a9 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1102,6 +1102,7 @@ static void virtio_blk_pci_instance_init(Object *obj)
>      VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BLK);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> +    object_unref(OBJECT(&dev->vdev));
>      qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
>  }
>
> --
> 1.9.3
>
>

[Qemu-devel] [PATCH v4 9/9] virtio-blk: move qdev properties into virtio-blk.c

[Stefan Hajnoczi]

There is no need to make DEFINE_VIRTIO_BLK_PROPERTIES() public.  Inline
it into virtio-blk.c so it cannot be used by mistake from other source
files.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
 hw/block/virtio-blk.c          | 12 +++++++++++-
 include/hw/virtio/virtio-blk.h | 23 -----------------------
 2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cfb8fa3..d36a3d4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -762,7 +762,17 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
 }
 
 static Property virtio_blk_properties[] = {
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlock, blk),
+    DEFINE_BLOCK_PROPERTIES(VirtIOBlock, blk.conf),
+    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtIOBlock, blk.serial),
+    DEFINE_PROP_BIT("config-wce", VirtIOBlock, blk.config_wce, 0, true),
+    DEFINE_PROP_IOTHREAD("x-iothread", VirtIOBlock, blk.iothread),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtIOBlock, blk.scsi, 0, true),
+#endif
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+    DEFINE_PROP_BIT("x-data-plane", VirtIOBlock, blk.data_plane, 0, false),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 59cf1f2..b3ed57a 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -136,29 +136,6 @@ typedef struct VirtIOBlock {
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
 
-#ifdef __linux__
-#define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field) \
-        DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true),
-#else
-#define DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)
-#endif
-
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-#define DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field) \
-    DEFINE_PROP_BIT("x-data-plane", _state, _field.data_plane, 0, false),
-#else
-#define DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field)
-#endif
-
-#define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
-        DEFINE_VIRTIO_BLK_PROPERTIES_LINUX(_state, _field)                    \
-        DEFINE_VIRTIO_BLK_PROPERTIES_DATA_PLANE(_state, _field)               \
-        DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
-        DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf),                     \
-        DEFINE_PROP_STRING("serial", _state, _field.serial),                  \
-        DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true),    \
-        DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
-
 int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
                                VirtQueueElement *elem);
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 0/9] virtio-blk: use alias properties in transport devices

[Paolo Bonzini]

Il 18/06/2014 11:58, Stefan Hajnoczi ha scritto:
> v4:
>  * Coding style: typedef struct { on a single line [Andreas]
>  * Add "dataplane: bail out on unsupported transport" for s390-virtio [Cornelia]
>
> v3:
>  * Split qdev_alias_all_properties() into its own patch [Peter Crosthwaite]
>  * Do not dereference DEVICE_CLASS(class) inline [Peter Crosthwaite]
>
> v2:
>  * Add qdev_alias_all_properties() instead of virtio-blk-specific function [Paolo]
>  * Explain refcount handling in doc comment [Paolo]
>  * Fix "property" duplicate typo [Peter Crosthwaite]
>  * Add "the same object or" to clarify commit description [Igor]
>
> Thanks for the feedback on the RFC.  This time around the alias property is
> implemented at the QOM property level instead of at the qdev property level.
>
> Note that this series only addresses virtio-blk.  In later series we can
> convert virtio net, scsi, rng, and serial.
>
> The virtio transport/device split is broken as follows:
>
> 1. The virtio-blk device is never finalized because the transport devices
>    (virtio-blk-pci and friends) leak the refcount.
>
> 2. If we fix the refcount leak then we double-free the 'serial' string property
>    upon hot unplug since its char* is copied into the virtio-blk device which
>    has an identical 'serial' qdev property.
>
> This series solves both of these problems as follows:
>
> 1. Introduce a QOM alias property that lets the transport device forward
>    property accesses into the virtio device (the child).
>
> 2. Use alias properties in transport devices, instead of keeping a duplicate
>    copy of the VirtIOBlkConf struct.
>
> 3. Fix the virtio-blk device refcount leak.  It's now safe to do this since the
>    double-free has been resolved.
>
> Tested that hotplug/hotunplug of virtio-blk-pci still works.
>
> Cornelia Huck (1):
>   dataplane: bail out on unsupported transport
>
> Stefan Hajnoczi (8):
>   qom: add object_property_add_alias()
>   virtio-blk: avoid qdev property definition duplication
>   virtio-blk: move x-data-plane qdev property to virtio-blk.h
>   qdev: add qdev_alias_all_properties()
>   virtio-blk: use aliases instead of duplicate qdev properties
>   virtio-blk: drop virtio_blk_set_conf()
>   virtio: fix virtio-blk child refcount in transports
>   virtio-blk: move qdev properties into virtio-blk.c
>
>  hw/block/dataplane/virtio-blk.c | 10 ++++++++
>  hw/block/virtio-blk.c           | 18 +++++++++------
>  hw/core/qdev.c                  | 21 +++++++++++++++++
>  hw/s390x/s390-virtio-bus.c      | 10 ++------
>  hw/s390x/s390-virtio-bus.h      |  1 -
>  hw/s390x/virtio-ccw.c           |  7 ++----
>  hw/s390x/virtio-ccw.h           |  1 -
>  hw/virtio/virtio-pci.c          |  7 ++----
>  hw/virtio/virtio-pci.h          |  1 -
>  include/hw/qdev-properties.h    |  2 ++
>  include/hw/virtio/virtio-blk.h  | 19 ---------------
>  include/qom/object.h            | 20 ++++++++++++++++
>  qom/object.c                    | 51 +++++++++++++++++++++++++++++++++++++++++
>  13 files changed, 121 insertions(+), 47 deletions(-)
>

Andreas, can you review this series?

Paolo

Re: [Qemu-devel] [PATCH v4 0/9] virtio-blk: use alias properties in transport devices

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3246 bytes --]

On Wed, Jun 18, 2014 at 05:58:27PM +0800, Stefan Hajnoczi wrote:
> v4:
>  * Coding style: typedef struct { on a single line [Andreas]
>  * Add "dataplane: bail out on unsupported transport" for s390-virtio [Cornelia]
> 
> v3:
>  * Split qdev_alias_all_properties() into its own patch [Peter Crosthwaite]
>  * Do not dereference DEVICE_CLASS(class) inline [Peter Crosthwaite]
> 
> v2:
>  * Add qdev_alias_all_properties() instead of virtio-blk-specific function [Paolo]
>  * Explain refcount handling in doc comment [Paolo]
>  * Fix "property" duplicate typo [Peter Crosthwaite]
>  * Add "the same object or" to clarify commit description [Igor]
> 
> Thanks for the feedback on the RFC.  This time around the alias property is
> implemented at the QOM property level instead of at the qdev property level.
> 
> Note that this series only addresses virtio-blk.  In later series we can
> convert virtio net, scsi, rng, and serial.
> 
> The virtio transport/device split is broken as follows:
> 
> 1. The virtio-blk device is never finalized because the transport devices
>    (virtio-blk-pci and friends) leak the refcount.
> 
> 2. If we fix the refcount leak then we double-free the 'serial' string property
>    upon hot unplug since its char* is copied into the virtio-blk device which
>    has an identical 'serial' qdev property.
> 
> This series solves both of these problems as follows:
> 
> 1. Introduce a QOM alias property that lets the transport device forward
>    property accesses into the virtio device (the child).
> 
> 2. Use alias properties in transport devices, instead of keeping a duplicate
>    copy of the VirtIOBlkConf struct.
> 
> 3. Fix the virtio-blk device refcount leak.  It's now safe to do this since the
>    double-free has been resolved.
> 
> Tested that hotplug/hotunplug of virtio-blk-pci still works.
> 
> Cornelia Huck (1):
>   dataplane: bail out on unsupported transport
> 
> Stefan Hajnoczi (8):
>   qom: add object_property_add_alias()
>   virtio-blk: avoid qdev property definition duplication
>   virtio-blk: move x-data-plane qdev property to virtio-blk.h
>   qdev: add qdev_alias_all_properties()
>   virtio-blk: use aliases instead of duplicate qdev properties
>   virtio-blk: drop virtio_blk_set_conf()
>   virtio: fix virtio-blk child refcount in transports
>   virtio-blk: move qdev properties into virtio-blk.c
> 
>  hw/block/dataplane/virtio-blk.c | 10 ++++++++
>  hw/block/virtio-blk.c           | 18 +++++++++------
>  hw/core/qdev.c                  | 21 +++++++++++++++++
>  hw/s390x/s390-virtio-bus.c      | 10 ++------
>  hw/s390x/s390-virtio-bus.h      |  1 -
>  hw/s390x/virtio-ccw.c           |  7 ++----
>  hw/s390x/virtio-ccw.h           |  1 -
>  hw/virtio/virtio-pci.c          |  7 ++----
>  hw/virtio/virtio-pci.h          |  1 -
>  include/hw/qdev-properties.h    |  2 ++
>  include/hw/virtio/virtio-blk.h  | 19 ---------------
>  include/qom/object.h            | 20 ++++++++++++++++
>  qom/object.c                    | 51 +++++++++++++++++++++++++++++++++++++++++
>  13 files changed, 121 insertions(+), 47 deletions(-)

Applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]