[Qemu-devel] [RFC 0/5] virtio: use alias properties in transport devices

[Qemu-devel] [RFC 0/5] virtio: use alias properties in transport devices

[Stefan Hajnoczi]

This RFC is just the beginning.  The same problem exists for virtio-net and
other devices.  I am looking for feedback before I convert all of virtio.

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 qdev child alias property that lets the transport device forward
   qdev property accesses into the virtio device (the child).

2. Use qdev child 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.

Note about the new qdev child alias property type: I haven't made the alias
fully transparent yet.  Perhaps we should hide the alias completely?
$ qemu-system-x86_64 -device virtio-blk-pci,\?
...
virtio-blk-pci.logical_block_size=ChildAlias  <--- should be uint64 or similar

Stefan Hajnoczi (5):
  qdev: add child alias properties
  virtio: add child alias properties for virtio-blk
  virtio: use child aliases for virtio-blk transport properties
  virtio-blk: drop virtio_blk_set_conf()
  virtio: fix virtio-blk child refcount in transports

 hw/block/virtio-blk.c          |  6 ------
 hw/core/qdev-properties.c      | 28 ++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-bus.c     |  4 ++--
 hw/s390x/s390-virtio-bus.h     |  1 -
 hw/s390x/virtio-ccw.c          |  6 +++---
 hw/s390x/virtio-ccw.h          |  1 -
 hw/virtio/virtio-pci.c         |  6 +++---
 hw/virtio/virtio-pci.h         |  1 -
 include/hw/block/block.h       | 14 ++++++++++++++
 include/hw/qdev-properties.h   | 28 ++++++++++++++++++++++++++++
 include/hw/virtio/virtio-blk.h | 17 ++++++++++++++++-
 11 files changed, 94 insertions(+), 18 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [RFC 1/5] qdev: add child alias properties

[Stefan Hajnoczi]

Child alias properties allow a parent object to expose child object
properties.  This makes it possible for a parent object to forward all
or a subset of a child's properties.

Currently we achieve similar behavior by duplicating property
definitions and copying the fields around.  It turns out virtio has
several double-frees since we didn't get the memory management right.

Using child alias properties it will be much easier for a parent object
to set a child's properties without fragile memory management issues
since the actual field only exists once in the child object.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/core/qdev-properties.c    | 28 ++++++++++++++++++++++++++++
 include/hw/qdev-properties.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 77d0c66..e62a4fd 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1002,3 +1002,31 @@ PropertyInfo qdev_prop_size = {
     .get = get_size,
     .set = set_size,
 };
+
+/* --- alias that forwards to a child object's property --- */
+
+static void get_child_alias(Object *obj, Visitor *v, void *opaque,
+                            const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
+
+    object_property_get(child, v, name, errp);
+}
+
+static void set_child_alias(Object *obj, Visitor *v, void *opaque,
+                            const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
+
+    object_property_set(child, v, name, errp);
+}
+
+PropertyInfo qdev_prop_child_alias = {
+    .name  = "ChildAlias",
+    .get = get_child_alias,
+    .set = set_child_alias,
+};
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 3c000ee..5ab1cac 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -13,6 +13,7 @@ extern PropertyInfo qdev_prop_uint32;
 extern PropertyInfo qdev_prop_int32;
 extern PropertyInfo qdev_prop_uint64;
 extern PropertyInfo qdev_prop_size;
+extern PropertyInfo qdev_prop_child_alias;
 extern PropertyInfo qdev_prop_string;
 extern PropertyInfo qdev_prop_chr;
 extern PropertyInfo qdev_prop_ptr;
@@ -61,6 +62,33 @@ extern PropertyInfo qdev_prop_arraylen;
         .defval    = (bool)_defval,                              \
         }
 
+/**
+ * DEFINE_PROP_CHILD_ALIAS:
+ * @_name: property name
+ * @_state: name of the device state structure type
+ * @_field: name of field in @_state, must be Object subclass
+ *
+ * Define device properties that alias a child object's property.  For example,
+ * use the following to forward the 'baz' property where Foo embeds a Bar
+ * object:
+ *
+ *   typedef struct {
+ *       Object parent_obj;
+ *
+ *       Bar bar_obj;
+ *   } Foo;
+ *
+ *   DEFINE_PROP_CHILD_ALIAS("baz", Foo, bar_obj)
+ *
+ * Any access to Foo's 'baz' property actually accesses bar_obj's 'baz'
+ * property.
+ */
+#define DEFINE_PROP_CHILD_ALIAS(_name, _state, _field) {         \
+        .name      = (_name),                                    \
+        .info      = &(qdev_prop_child_alias),                   \
+        .offset    = offsetof(_state, _field),                   \
+        }
+
 #define PROP_ARRAY_LEN_PREFIX "len-"
 
 /**
-- 
1.8.5.3

[Qemu-devel] [RFC 2/5] virtio: add child alias properties for virtio-blk

[Stefan Hajnoczi]

Now that qdev child alias properties exist, define aliases for
virtio-blk properties.  The aliases will replace the duplicated
properties that current live in virtio-blk-pci, virtio-blk-ccw, and
friends.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/block/block.h       | 14 ++++++++++++++
 include/hw/virtio/virtio-blk.h | 17 +++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 7c3d6c8..702f1eb 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -52,11 +52,25 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
     DEFINE_PROP_UINT32("discard_granularity", _state, \
                        _conf.discard_granularity, -1)
 
+#define DEFINE_BLOCK_CHILD_ALIASES(_state, _field)                      \
+    DEFINE_PROP_CHILD_ALIAS("drive", _state, _field),                   \
+    DEFINE_PROP_CHILD_ALIAS("logical_block_size", _state, _field),      \
+    DEFINE_PROP_CHILD_ALIAS("physical_block_size", _state, _field),     \
+    DEFINE_PROP_CHILD_ALIAS("min_io_size", _state, _field),             \
+    DEFINE_PROP_CHILD_ALIAS("opt_io_size", _state, _field),             \
+    DEFINE_PROP_CHILD_ALIAS("bootindex", _state, _field),               \
+    DEFINE_PROP_CHILD_ALIAS("discard_granularity", _state, _field)
+
 #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
     DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
     DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
     DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
 
+#define DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field)      \
+    DEFINE_PROP_CHILD_ALIAS("cyls", _state, _field),        \
+    DEFINE_PROP_CHILD_ALIAS("heads", _state, _field),       \
+    DEFINE_PROP_CHILD_ALIAS("secs", _state, _field)
+
 /* Configuration helpers */
 
 void blkconf_serial(BlockConf *conf, char **serial);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index e4c41ff..5095ff4 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -153,6 +153,23 @@ typedef struct VirtIOBlock {
         DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
 #endif /* __linux__ */
 
+#ifdef __linux__
+#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                       \
+        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                           \
+        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                       \
+        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                    \
+        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),                \
+        DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),                      \
+        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
+#else
+#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                       \
+        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                           \
+        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                       \
+        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                    \
+        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),                \
+        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
+#endif /* __linux__ */
+
 void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
 
 #endif
-- 
1.8.5.3

[Qemu-devel] [RFC 3/5] virtio: use child aliases for virtio-blk transport properties

[Stefan Hajnoczi]

PCI, CCW, and legacy s390 virtio kept their own copy of VirtIOBlkConf
and duplicated qdev properties from the virtio-blk device.  The conf
struct was then memcpy into the VirtIOBlock object's VirtIOBlkConf.

This risks double-free since the same 'serial' property char* would be
duplicated and freed by both release functions.

Instead of making the conf struct memcpy smarter, avoid duplication
altogether and simply alias properties into the child object.  Now there
is only one copy of VirtIOBlkConf and it lives inside the virtio device.

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

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index e4fc353..064947b 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -179,7 +179,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;
@@ -525,7 +524,7 @@ static const TypeInfo s390_virtio_net = {
 };
 
 static Property s390_virtio_blk_properties[] = {
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
+    DEFINE_VIRTIO_BLK_CHILD_ALIASES(VirtIOBlkS390, vdev),
     DEFINE_PROP_END_OF_LIST(),
 };
 
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 a01801e..ba62dad 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -726,7 +726,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;
@@ -1127,11 +1126,11 @@ 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_VIRTIO_BLK_CHILD_ALIASES(VirtIOBlkCcw, vdev),
     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),
+    DEFINE_PROP_CHILD_ALIAS("x-data-plane", VirtIOBlkCcw, vdev),
 #endif
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 4393e44..7eea6b9 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -134,7 +134,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 7b91841..ba86e1c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1068,10 +1068,10 @@ static Property virtio_blk_pci_properties[] = {
                     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),
+    DEFINE_PROP_CHILD_ALIAS("x-data-plane", VirtIOBlkPCI, vdev),
 #endif
     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk),
+    DEFINE_VIRTIO_BLK_CHILD_ALIASES(VirtIOBlkPCI, vdev),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1079,7 +1079,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;
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.8.5.3

[Qemu-devel] [RFC 4/5] virtio-blk: drop virtio_blk_set_conf()

[Stefan Hajnoczi]

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

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.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 8a568e5..4781351 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -642,12 +642,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 5095ff4..a7d9910 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -170,6 +170,4 @@ typedef struct VirtIOBlock {
         DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
 #endif /* __linux__ */
 
-void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
-
 #endif
-- 
1.8.5.3

[Qemu-devel] [RFC 5/5] 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 064947b..1996c53 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -191,6 +191,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));
 }
 
 static int s390_virtio_serial_init(VirtIOS390Device *s390_dev)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index ba62dad..b80deb6 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -739,6 +739,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));
 }
 
 static int virtio_ccw_serial_init(VirtioCcwDevice *ccw_dev)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ba86e1c..44421c8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1106,6 +1106,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));
 }
 
 static const TypeInfo virtio_blk_pci_info = {
-- 
1.8.5.3

Re: [Qemu-devel] [RFC 0/5] virtio: use alias properties in transport devices

[Stefan Hajnoczi]

On Wed, Mar 19, 2014 at 4:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> This RFC is just the beginning.  The same problem exists for virtio-net and
> other devices.  I am looking for feedback before I convert all of virtio.
>
> 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 qdev child alias property that lets the transport device forward
>    qdev property accesses into the virtio device (the child).
>
> 2. Use qdev child 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.
>
> Note about the new qdev child alias property type: I haven't made the alias
> fully transparent yet.  Perhaps we should hide the alias completely?
> $ qemu-system-x86_64 -device virtio-blk-pci,\?
> ...
> virtio-blk-pci.logical_block_size=ChildAlias  <--- should be uint64 or similar
>
> Stefan Hajnoczi (5):
>   qdev: add child alias properties
>   virtio: add child alias properties for virtio-blk
>   virtio: use child aliases for virtio-blk transport properties
>   virtio-blk: drop virtio_blk_set_conf()
>   virtio: fix virtio-blk child refcount in transports
>
>  hw/block/virtio-blk.c          |  6 ------
>  hw/core/qdev-properties.c      | 28 ++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-bus.c     |  4 ++--
>  hw/s390x/s390-virtio-bus.h     |  1 -
>  hw/s390x/virtio-ccw.c          |  6 +++---
>  hw/s390x/virtio-ccw.h          |  1 -
>  hw/virtio/virtio-pci.c         |  6 +++---
>  hw/virtio/virtio-pci.h         |  1 -
>  include/hw/block/block.h       | 14 ++++++++++++++
>  include/hw/qdev-properties.h   | 28 ++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-blk.h | 17 ++++++++++++++++-
>  11 files changed, 94 insertions(+), 18 deletions(-)

Ping?

If you agree with this approach I'd like to get this code merged.  We
still need to fix the other virtio devices.

Stefan

Re: [Qemu-devel] [RFC 1/5] qdev: add child alias properties

[Andreas Färber]

Am 19.03.2014 16:48, schrieb Stefan Hajnoczi:
> Child alias properties allow a parent object to expose child object
> properties.  This makes it possible for a parent object to forward all
> or a subset of a child's properties.
> 
> Currently we achieve similar behavior by duplicating property
> definitions and copying the fields around.  It turns out virtio has
> several double-frees since we didn't get the memory management right.
> 
> Using child alias properties it will be much easier for a parent object
> to set a child's properties without fragile memory management issues
> since the actual field only exists once in the child object.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/core/qdev-properties.c    | 28 ++++++++++++++++++++++++++++
>  include/hw/qdev-properties.h | 28 ++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 77d0c66..e62a4fd 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1002,3 +1002,31 @@ PropertyInfo qdev_prop_size = {
>      .get = get_size,
>      .set = set_size,
>  };
> +
> +/* --- alias that forwards to a child object's property --- */
> +
> +static void get_child_alias(Object *obj, Visitor *v, void *opaque,
> +                            const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
> +
> +    object_property_get(child, v, name, errp);
> +}
> +
> +static void set_child_alias(Object *obj, Visitor *v, void *opaque,
> +                            const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
> +
> +    object_property_set(child, v, name, errp);
> +}
> +
> +PropertyInfo qdev_prop_child_alias = {
> +    .name  = "ChildAlias",

Won't this turn into the QMP-exposed type of the property? That would be
wrong then, requiring per-type alias property types. They could be
macro-automated of course, if we need them.

Paolo had cleaned up type names to match QAPI schema for 2.0, CC'ing.

Regards,
Andreas

> +    .get = get_child_alias,
> +    .set = set_child_alias,
> +};
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 3c000ee..5ab1cac 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -13,6 +13,7 @@ extern PropertyInfo qdev_prop_uint32;
>  extern PropertyInfo qdev_prop_int32;
>  extern PropertyInfo qdev_prop_uint64;
>  extern PropertyInfo qdev_prop_size;
> +extern PropertyInfo qdev_prop_child_alias;
>  extern PropertyInfo qdev_prop_string;
>  extern PropertyInfo qdev_prop_chr;
>  extern PropertyInfo qdev_prop_ptr;
> @@ -61,6 +62,33 @@ extern PropertyInfo qdev_prop_arraylen;
>          .defval    = (bool)_defval,                              \
>          }
>  
> +/**
> + * DEFINE_PROP_CHILD_ALIAS:
> + * @_name: property name
> + * @_state: name of the device state structure type
> + * @_field: name of field in @_state, must be Object subclass
> + *
> + * Define device properties that alias a child object's property.  For example,
> + * use the following to forward the 'baz' property where Foo embeds a Bar
> + * object:
> + *
> + *   typedef struct {
> + *       Object parent_obj;
> + *
> + *       Bar bar_obj;
> + *   } Foo;
> + *
> + *   DEFINE_PROP_CHILD_ALIAS("baz", Foo, bar_obj)
> + *
> + * Any access to Foo's 'baz' property actually accesses bar_obj's 'baz'
> + * property.
> + */
> +#define DEFINE_PROP_CHILD_ALIAS(_name, _state, _field) {         \
> +        .name      = (_name),                                    \
> +        .info      = &(qdev_prop_child_alias),                   \
> +        .offset    = offsetof(_state, _field),                   \
> +        }
> +
>  #define PROP_ARRAY_LEN_PREFIX "len-"
>  
>  /**

-- 
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] [RFC 1/5] qdev: add child alias properties

[Paolo Bonzini]

Il 14/05/2014 15:27, Andreas Färber ha scritto:
>> > +PropertyInfo qdev_prop_child_alias = {
>> > +    .name  = "ChildAlias",
> Won't this turn into the QMP-exposed type of the property? That would be
> wrong then, requiring per-type alias property types. They could be
> macro-automated of course, if we need them.
>
> Paolo had cleaned up type names to match QAPI schema for 2.0, CC'ing.

Replying to this patch series has been on my todo list for a long time...

I think we should avoid making the ChildAlias property a qdev property. 
  This means redoing how "-device foo,help" works, so that it includes 
QOM properties as well.

Paolo

Re: [Qemu-devel] [RFC 2/5] virtio: add child alias properties for virtio-blk

[Peter Crosthwaite]

On Wed, Mar 19, 2014 at 3:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Now that qdev child alias properties exist, define aliases for
> virtio-blk properties.  The aliases will replace the duplicated
> properties that current live in virtio-blk-pci, virtio-blk-ccw, and
> friends.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/block/block.h       | 14 ++++++++++++++
>  include/hw/virtio/virtio-blk.h | 17 +++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index 7c3d6c8..702f1eb 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -52,11 +52,25 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>      DEFINE_PROP_UINT32("discard_granularity", _state, \
>                         _conf.discard_granularity, -1)
>
> +#define DEFINE_BLOCK_CHILD_ALIASES(_state, _field)                      \
> +    DEFINE_PROP_CHILD_ALIAS("drive", _state, _field),                   \
> +    DEFINE_PROP_CHILD_ALIAS("logical_block_size", _state, _field),      \
> +    DEFINE_PROP_CHILD_ALIAS("physical_block_size", _state, _field),     \
> +    DEFINE_PROP_CHILD_ALIAS("min_io_size", _state, _field),             \
> +    DEFINE_PROP_CHILD_ALIAS("opt_io_size", _state, _field),             \
> +    DEFINE_PROP_CHILD_ALIAS("bootindex", _state, _field),               \
> +    DEFINE_PROP_CHILD_ALIAS("discard_granularity", _state, _field)
> +
>  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
>      DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
>      DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
>      DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
>
> +#define DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field)      \
> +    DEFINE_PROP_CHILD_ALIAS("cyls", _state, _field),        \
> +    DEFINE_PROP_CHILD_ALIAS("heads", _state, _field),       \
> +    DEFINE_PROP_CHILD_ALIAS("secs", _state, _field)
> +
>  /* Configuration helpers */
>
>  void blkconf_serial(BlockConf *conf, char **serial);
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index e4c41ff..5095ff4 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -153,6 +153,23 @@ typedef struct VirtIOBlock {
>          DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
>  #endif /* __linux__ */
>
> +#ifdef __linux__
> +#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                       \
> +        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                           \
> +        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                       \
> +        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                    \
> +        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),                \
> +        DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),                      \
> +        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
> +#else
> +#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                       \
> +        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                           \
> +        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                       \
> +        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                    \
> +        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),                \
> +        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
> +#endif /* __linux__ */

Can the duplication be avoided with:

#ifdef __linux__
#define DEFINE_VIRTIO_BLK_CHILD_ALIASES_LINUX \
        DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),
#else
#define DEFINE_VIRTIO_BLK_CHILD_ALIASES_LINUX
#endif

?

Regards,
Peter


> +
>  void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
>
>  #endif
> --
> 1.8.5.3
>
>

Re: [Qemu-devel] [RFC 1/5] qdev: add child alias properties

[Peter Crosthwaite]

On Wed, Mar 19, 2014 at 3:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Child alias properties allow a parent object to expose child object
> properties.  This makes it possible for a parent object to forward all
> or a subset of a child's properties.
>

So this sounds useful to my MemoryRegion and GPIO QOMification plan.
One of the missing pieces is how do you set a child property when you
only have a ref to the parent which I guess is a big motivator here.
The idea is, memory region properties can be set by the parent device
that created them (Seems similar to what you are doing with blk stuffs
here really).

This implementation however seems to assume a 1:1 mapping between the
parent and child property namespaces limiting its use in cases where
multiple child objects of the same type want to expose on the parent
level. Is it somehow possible to change the name of the property as
exposed by the parent ...

> Currently we achieve similar behavior by duplicating property
> definitions and copying the fields around.  It turns out virtio has
> several double-frees since we didn't get the memory management right.
>
> Using child alias properties it will be much easier for a parent object
> to set a child's properties without fragile memory management issues
> since the actual field only exists once in the child object.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/core/qdev-properties.c    | 28 ++++++++++++++++++++++++++++
>  include/hw/qdev-properties.h | 28 ++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 77d0c66..e62a4fd 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1002,3 +1002,31 @@ PropertyInfo qdev_prop_size = {
>      .get = get_size,
>      .set = set_size,
>  };
> +
> +/* --- alias that forwards to a child object's property --- */
> +
> +static void get_child_alias(Object *obj, Visitor *v, void *opaque,
> +                            const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
> +
> +    object_property_get(child, v, name, errp);
> +}
> +
> +static void set_child_alias(Object *obj, Visitor *v, void *opaque,
> +                            const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
> +
> +    object_property_set(child, v, name, errp);
> +}
> +
> +PropertyInfo qdev_prop_child_alias = {
> +    .name  = "ChildAlias",
> +    .get = get_child_alias,
> +    .set = set_child_alias,
> +};
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 3c000ee..5ab1cac 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -13,6 +13,7 @@ extern PropertyInfo qdev_prop_uint32;
>  extern PropertyInfo qdev_prop_int32;
>  extern PropertyInfo qdev_prop_uint64;
>  extern PropertyInfo qdev_prop_size;
> +extern PropertyInfo qdev_prop_child_alias;
>  extern PropertyInfo qdev_prop_string;
>  extern PropertyInfo qdev_prop_chr;
>  extern PropertyInfo qdev_prop_ptr;
> @@ -61,6 +62,33 @@ extern PropertyInfo qdev_prop_arraylen;
>          .defval    = (bool)_defval,                              \
>          }
>
> +/**
> + * DEFINE_PROP_CHILD_ALIAS:
> + * @_name: property name
> + * @_state: name of the device state structure type
> + * @_field: name of field in @_state, must be Object subclass
> + *
> + * Define device properties that alias a child object's property.  For example,
> + * use the following to forward the 'baz' property where Foo embeds a Bar

... such that the parent renames the property to "bar-baz" to clarify
whose "baz" is being forwarded?

Regards,
Peter

> + * object:
> + *
> + *   typedef struct {
> + *       Object parent_obj;
> + *
> + *       Bar bar_obj;
> + *   } Foo;
> + *
> + *   DEFINE_PROP_CHILD_ALIAS("baz", Foo, bar_obj)
> + *
> + * Any access to Foo's 'baz' property actually accesses bar_obj's 'baz'
> + * property.
> + */
> +#define DEFINE_PROP_CHILD_ALIAS(_name, _state, _field) {         \
> +        .name      = (_name),                                    \
> +        .info      = &(qdev_prop_child_alias),                   \
> +        .offset    = offsetof(_state, _field),                   \
> +        }
> +
>  #define PROP_ARRAY_LEN_PREFIX "len-"
>
>  /**
> --
> 1.8.5.3
>
>

Re: [Qemu-devel] [RFC 1/5] qdev: add child alias properties

[Andreas Färber]

Am 14.05.2014 16:13, schrieb Peter Crosthwaite:
> On Wed, Mar 19, 2014 at 3:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> Child alias properties allow a parent object to expose child object
>> properties.  This makes it possible for a parent object to forward all
>> or a subset of a child's properties.
>>
> 
> So this sounds useful to my MemoryRegion and GPIO QOMification plan.
> One of the missing pieces is how do you set a child property when you
> only have a ref to the parent which I guess is a big motivator here.
> The idea is, memory region properties can be set by the parent device
> that created them (Seems similar to what you are doing with blk stuffs
> here really).
> 
> This implementation however seems to assume a 1:1 mapping between the
> parent and child property namespaces limiting its use in cases where
> multiple child objects of the same type want to expose on the parent
> level. Is it somehow possible to change the name of the property as
> exposed by the parent ...
> 
>> Currently we achieve similar behavior by duplicating property
>> definitions and copying the fields around.  It turns out virtio has
>> several double-frees since we didn't get the memory management right.
>>
>> Using child alias properties it will be much easier for a parent object
>> to set a child's properties without fragile memory management issues
>> since the actual field only exists once in the child object.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  hw/core/qdev-properties.c    | 28 ++++++++++++++++++++++++++++
>>  include/hw/qdev-properties.h | 28 ++++++++++++++++++++++++++++
>>  2 files changed, 56 insertions(+)
>>
>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 77d0c66..e62a4fd 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -1002,3 +1002,31 @@ PropertyInfo qdev_prop_size = {
>>      .get = get_size,
>>      .set = set_size,
>>  };
>> +
>> +/* --- alias that forwards to a child object's property --- */
>> +
>> +static void get_child_alias(Object *obj, Visitor *v, void *opaque,
>> +                            const char *name, Error **errp)
>> +{
>> +    DeviceState *dev = DEVICE(obj);
>> +    Property *prop = opaque;
>> +    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
>> +
>> +    object_property_get(child, v, name, errp);
>> +}
>> +
>> +static void set_child_alias(Object *obj, Visitor *v, void *opaque,
>> +                            const char *name, Error **errp)
>> +{
>> +    DeviceState *dev = DEVICE(obj);
>> +    Property *prop = opaque;
>> +    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
>> +
>> +    object_property_set(child, v, name, errp);
>> +}
>> +
>> +PropertyInfo qdev_prop_child_alias = {
>> +    .name  = "ChildAlias",
>> +    .get = get_child_alias,
>> +    .set = set_child_alias,
>> +};
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 3c000ee..5ab1cac 100644
>> --- a/include/hw/qdev-properties.h
>> +++ b/include/hw/qdev-properties.h
>> @@ -13,6 +13,7 @@ extern PropertyInfo qdev_prop_uint32;
>>  extern PropertyInfo qdev_prop_int32;
>>  extern PropertyInfo qdev_prop_uint64;
>>  extern PropertyInfo qdev_prop_size;
>> +extern PropertyInfo qdev_prop_child_alias;
>>  extern PropertyInfo qdev_prop_string;
>>  extern PropertyInfo qdev_prop_chr;
>>  extern PropertyInfo qdev_prop_ptr;
>> @@ -61,6 +62,33 @@ extern PropertyInfo qdev_prop_arraylen;
>>          .defval    = (bool)_defval,                              \
>>          }
>>
>> +/**
>> + * DEFINE_PROP_CHILD_ALIAS:
>> + * @_name: property name
>> + * @_state: name of the device state structure type
>> + * @_field: name of field in @_state, must be Object subclass
>> + *
>> + * Define device properties that alias a child object's property.  For example,
>> + * use the following to forward the 'baz' property where Foo embeds a Bar
> 
> ... such that the parent renames the property to "bar-baz" to clarify
> whose "baz" is being forwarded?

Didn't you claim just yesterday that bar.baz=foo or similar was already
working today?

I think that in your case adding properties to the parent is wrong.

virtio on the other hand needs this because devices live both on their
own on a virtio-mmio bus and get created as PCI/CCW devices by the user.

Regards,
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] [RFC 1/5] qdev: add child alias properties

[Peter Crosthwaite]

On Wed, May 14, 2014 at 2:17 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 14.05.2014 16:13, schrieb Peter Crosthwaite:
>> On Wed, Mar 19, 2014 at 3:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> Child alias properties allow a parent object to expose child object
>>> properties.  This makes it possible for a parent object to forward all
>>> or a subset of a child's properties.
>>>
>>
>> So this sounds useful to my MemoryRegion and GPIO QOMification plan.
>> One of the missing pieces is how do you set a child property when you
>> only have a ref to the parent which I guess is a big motivator here.
>> The idea is, memory region properties can be set by the parent device
>> that created them (Seems similar to what you are doing with blk stuffs
>> here really).
>>
>> This implementation however seems to assume a 1:1 mapping between the
>> parent and child property namespaces limiting its use in cases where
>> multiple child objects of the same type want to expose on the parent
>> level. Is it somehow possible to change the name of the property as
>> exposed by the parent ...
>>
>>> Currently we achieve similar behavior by duplicating property
>>> definitions and copying the fields around.  It turns out virtio has
>>> several double-frees since we didn't get the memory management right.
>>>
>>> Using child alias properties it will be much easier for a parent object
>>> to set a child's properties without fragile memory management issues
>>> since the actual field only exists once in the child object.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  hw/core/qdev-properties.c    | 28 ++++++++++++++++++++++++++++
>>>  include/hw/qdev-properties.h | 28 ++++++++++++++++++++++++++++
>>>  2 files changed, 56 insertions(+)
>>>
>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index 77d0c66..e62a4fd 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -1002,3 +1002,31 @@ PropertyInfo qdev_prop_size = {
>>>      .get = get_size,
>>>      .set = set_size,
>>>  };
>>> +
>>> +/* --- alias that forwards to a child object's property --- */
>>> +
>>> +static void get_child_alias(Object *obj, Visitor *v, void *opaque,
>>> +                            const char *name, Error **errp)
>>> +{
>>> +    DeviceState *dev = DEVICE(obj);
>>> +    Property *prop = opaque;
>>> +    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
>>> +
>>> +    object_property_get(child, v, name, errp);
>>> +}
>>> +
>>> +static void set_child_alias(Object *obj, Visitor *v, void *opaque,
>>> +                            const char *name, Error **errp)
>>> +{
>>> +    DeviceState *dev = DEVICE(obj);
>>> +    Property *prop = opaque;
>>> +    Object *child = OBJECT(qdev_get_prop_ptr(dev, prop));
>>> +
>>> +    object_property_set(child, v, name, errp);
>>> +}
>>> +
>>> +PropertyInfo qdev_prop_child_alias = {
>>> +    .name  = "ChildAlias",
>>> +    .get = get_child_alias,
>>> +    .set = set_child_alias,
>>> +};
>>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>>> index 3c000ee..5ab1cac 100644
>>> --- a/include/hw/qdev-properties.h
>>> +++ b/include/hw/qdev-properties.h
>>> @@ -13,6 +13,7 @@ extern PropertyInfo qdev_prop_uint32;
>>>  extern PropertyInfo qdev_prop_int32;
>>>  extern PropertyInfo qdev_prop_uint64;
>>>  extern PropertyInfo qdev_prop_size;
>>> +extern PropertyInfo qdev_prop_child_alias;
>>>  extern PropertyInfo qdev_prop_string;
>>>  extern PropertyInfo qdev_prop_chr;
>>>  extern PropertyInfo qdev_prop_ptr;
>>> @@ -61,6 +62,33 @@ extern PropertyInfo qdev_prop_arraylen;
>>>          .defval    = (bool)_defval,                              \
>>>          }
>>>
>>> +/**
>>> + * DEFINE_PROP_CHILD_ALIAS:
>>> + * @_name: property name
>>> + * @_state: name of the device state structure type
>>> + * @_field: name of field in @_state, must be Object subclass
>>> + *
>>> + * Define device properties that alias a child object's property.  For example,
>>> + * use the following to forward the 'baz' property where Foo embeds a Bar
>>
>> ... such that the parent renames the property to "bar-baz" to clarify
>> whose "baz" is being forwarded?
>
> Didn't you claim just yesterday that bar.baz=foo or similar was already
> working today?
>

Yeh but I had to patch QOM to make that work. Thats newly invented
syntax. This would be a possible alternative. Just shopping for
options here :)

> I think that in your case adding properties to the parent is wrong.
>

Ok. I'll post that QOM patch along with the rest.

Regards,
Peter

> virtio on the other hand needs this because devices live both on their
> own on a virtio-mmio bus and get created as PCI/CCW devices by the user.
>
> Regards,
> 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] [RFC 2/5] virtio: add child alias properties for virtio-blk

[Stefan Hajnoczi]

On Wed, May 14, 2014 at 02:04:25PM +0000, Peter Crosthwaite wrote:
> On Wed, Mar 19, 2014 at 3:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > Now that qdev child alias properties exist, define aliases for
> > virtio-blk properties.  The aliases will replace the duplicated
> > properties that current live in virtio-blk-pci, virtio-blk-ccw, and
> > friends.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  include/hw/block/block.h       | 14 ++++++++++++++
> >  include/hw/virtio/virtio-blk.h | 17 +++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index 7c3d6c8..702f1eb 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -52,11 +52,25 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
> >      DEFINE_PROP_UINT32("discard_granularity", _state, \
> >                         _conf.discard_granularity, -1)
> >
> > +#define DEFINE_BLOCK_CHILD_ALIASES(_state, _field)                      \
> > +    DEFINE_PROP_CHILD_ALIAS("drive", _state, _field),                   \
> > +    DEFINE_PROP_CHILD_ALIAS("logical_block_size", _state, _field),      \
> > +    DEFINE_PROP_CHILD_ALIAS("physical_block_size", _state, _field),     \
> > +    DEFINE_PROP_CHILD_ALIAS("min_io_size", _state, _field),             \
> > +    DEFINE_PROP_CHILD_ALIAS("opt_io_size", _state, _field),             \
> > +    DEFINE_PROP_CHILD_ALIAS("bootindex", _state, _field),               \
> > +    DEFINE_PROP_CHILD_ALIAS("discard_granularity", _state, _field)
> > +
> >  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
> >      DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
> >      DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
> >      DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
> >
> > +#define DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field)      \
> > +    DEFINE_PROP_CHILD_ALIAS("cyls", _state, _field),        \
> > +    DEFINE_PROP_CHILD_ALIAS("heads", _state, _field),       \
> > +    DEFINE_PROP_CHILD_ALIAS("secs", _state, _field)
> > +
> >  /* Configuration helpers */
> >
> >  void blkconf_serial(BlockConf *conf, char **serial);
> > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> > index e4c41ff..5095ff4 100644
> > --- a/include/hw/virtio/virtio-blk.h
> > +++ b/include/hw/virtio/virtio-blk.h
> > @@ -153,6 +153,23 @@ typedef struct VirtIOBlock {
> >          DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
> >  #endif /* __linux__ */
> >
> > +#ifdef __linux__
> > +#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                       \
> > +        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                           \
> > +        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                       \
> > +        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                    \
> > +        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),                \
> > +        DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),                      \
> > +        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
> > +#else
> > +#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                       \
> > +        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                           \
> > +        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                       \
> > +        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                    \
> > +        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),                \
> > +        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
> > +#endif /* __linux__ */
> 
> Can the duplication be avoided with:
> 
> #ifdef __linux__
> #define DEFINE_VIRTIO_BLK_CHILD_ALIASES_LINUX \
>         DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),
> #else
> #define DEFINE_VIRTIO_BLK_CHILD_ALIASES_LINUX
> #endif
> 
> ?

Absolutely, but I want each patch to do only one thing.

The "duplication" I referred to in the commit description is something
else: for virtio we create two sets of qdev properties with the same
name.  The first set is in the parent object and the second set is in
the child.  The actual implementation of this in hw/virtio/virtio-pci.c
varies between device types: in some cases we really have the property
values duplicated, in other cases we share the memory between parent and
child.  It's nuts and I want to eliminate that.

The __linux__ ifdef duplication is a minor thing that doesn't worry me.
It could be fixed in a trivial patch.

Stefan

Re: [Qemu-devel] [RFC 2/5] virtio: add child alias properties for virtio-blk

[Peter Crosthwaite]

On Wed, May 14, 2014 at 2:33 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, May 14, 2014 at 02:04:25PM +0000, Peter Crosthwaite wrote:
>> On Wed, Mar 19, 2014 at 3:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > Now that qdev child alias properties exist, define aliases for
>> > virtio-blk properties.  The aliases will replace the duplicated
>> > properties that current live in virtio-blk-pci, virtio-blk-ccw, and
>> > friends.
>> >
>> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> >  include/hw/block/block.h       | 14 ++++++++++++++
>> >  include/hw/virtio/virtio-blk.h | 17 +++++++++++++++++
>> >  2 files changed, 31 insertions(+)
>> >
>> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
>> > index 7c3d6c8..702f1eb 100644
>> > --- a/include/hw/block/block.h
>> > +++ b/include/hw/block/block.h
>> > @@ -52,11 +52,25 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>> >      DEFINE_PROP_UINT32("discard_granularity", _state, \
>> >                         _conf.discard_granularity, -1)
>> >
>> > +#define DEFINE_BLOCK_CHILD_ALIASES(_state, _field)                      \
>> > +    DEFINE_PROP_CHILD_ALIAS("drive", _state, _field),                   \
>> > +    DEFINE_PROP_CHILD_ALIAS("logical_block_size", _state, _field),      \
>> > +    DEFINE_PROP_CHILD_ALIAS("physical_block_size", _state, _field),     \
>> > +    DEFINE_PROP_CHILD_ALIAS("min_io_size", _state, _field),             \
>> > +    DEFINE_PROP_CHILD_ALIAS("opt_io_size", _state, _field),             \
>> > +    DEFINE_PROP_CHILD_ALIAS("bootindex", _state, _field),               \
>> > +    DEFINE_PROP_CHILD_ALIAS("discard_granularity", _state, _field)
>> > +
>> >  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
>> >      DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
>> >      DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
>> >      DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
>> >
>> > +#define DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field)      \
>> > +    DEFINE_PROP_CHILD_ALIAS("cyls", _state, _field),        \
>> > +    DEFINE_PROP_CHILD_ALIAS("heads", _state, _field),       \
>> > +    DEFINE_PROP_CHILD_ALIAS("secs", _state, _field)
>> > +
>> >  /* Configuration helpers */
>> >
>> >  void blkconf_serial(BlockConf *conf, char **serial);
>> > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
>> > index e4c41ff..5095ff4 100644
>> > --- a/include/hw/virtio/virtio-blk.h
>> > +++ b/include/hw/virtio/virtio-blk.h
>> > @@ -153,6 +153,23 @@ typedef struct VirtIOBlock {
>> >          DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
>> >  #endif /* __linux__ */
>> >
>> > +#ifdef __linux__
>> > +#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                       \
>> > +        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                           \
>> > +        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                       \
>> > +        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                    \
>> > +        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),                \
>> > +        DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),                      \
>> > +        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
>> > +#else
>> > +#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                       \
>> > +        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                           \
>> > +        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                       \
>> > +        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                    \
>> > +        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),                \
>> > +        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
>> > +#endif /* __linux__ */
>>
>> Can the duplication be avoided with:
>>
>> #ifdef __linux__
>> #define DEFINE_VIRTIO_BLK_CHILD_ALIASES_LINUX \
>>         DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),
>> #else
>> #define DEFINE_VIRTIO_BLK_CHILD_ALIASES_LINUX
>> #endif
>>
>> ?
>
> Absolutely, but I want each patch to do only one thing.
>
> The "duplication" I referred to in the commit description is something
> else: for virtio we create two sets of qdev properties with the same
> name.  The first set is in the parent object and the second set is in
> the child.  The actual implementation of this in hw/virtio/virtio-pci.c
> varies between device types: in some cases we really have the property
> values duplicated, in other cases we share the memory between parent and
> child.  It's nuts and I want to eliminate that.
>

Right, just overload of the term "duplication". I was only reffering
to the duplicated text in your added code, not the function of the
patch. Sorry about confusion.

Regards,
Peter

> The __linux__ ifdef duplication is a minor thing that doesn't worry me.
> It could be fixed in a trivial patch.
>
> Stefan
>

Re: [Qemu-devel] [RFC 0/5] virtio: use alias properties in transport devices

[Frederic Konrad]

On 19/03/2014 16:48, Stefan Hajnoczi wrote:
> This RFC is just the beginning.  The same problem exists for virtio-net and
> other devices.  I am looking for feedback before I convert all of virtio.
>
> 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 qdev child alias property that lets the transport device forward
>     qdev property accesses into the virtio device (the child).
>
> 2. Use qdev child 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.
>
> Note about the new qdev child alias property type: I haven't made the alias
> fully transparent yet.  Perhaps we should hide the alias completely?
> $ qemu-system-x86_64 -device virtio-blk-pci,\?
> ...
> virtio-blk-pci.logical_block_size=ChildAlias  <--- should be uint64 or similar
>
> Stefan Hajnoczi (5):
>    qdev: add child alias properties
>    virtio: add child alias properties for virtio-blk
>    virtio: use child aliases for virtio-blk transport properties
>    virtio-blk: drop virtio_blk_set_conf()
>    virtio: fix virtio-blk child refcount in transports
>
>   hw/block/virtio-blk.c          |  6 ------
>   hw/core/qdev-properties.c      | 28 ++++++++++++++++++++++++++++
>   hw/s390x/s390-virtio-bus.c     |  4 ++--
>   hw/s390x/s390-virtio-bus.h     |  1 -
>   hw/s390x/virtio-ccw.c          |  6 +++---
>   hw/s390x/virtio-ccw.h          |  1 -
>   hw/virtio/virtio-pci.c         |  6 +++---
>   hw/virtio/virtio-pci.h         |  1 -
>   include/hw/block/block.h       | 14 ++++++++++++++
>   include/hw/qdev-properties.h   | 28 ++++++++++++++++++++++++++++
>   include/hw/virtio/virtio-blk.h | 17 ++++++++++++++++-
>   11 files changed, 94 insertions(+), 18 deletions(-)
>
Hi,

Sorry I missed this.

Is there any repo with those patches I can clone?

Thanks,
Fred

Re: [Qemu-devel] [RFC 0/5] virtio: use alias properties in transport devices

[Stefan Hajnoczi]

On Fri, May 16, 2014 at 05:08:56PM +0200, Frederic Konrad wrote:
> Is there any repo with those patches I can clone?

https://github.com/stefanha/qemu/tree/qdev-qom-alias-property
git://github.com/stefanha/qemu.git qdev-qom-alias-property

Given the feedback I have received, the alias needs to be reimplemented
at the QOM Object property level instead of the qdev property level.  I
have started working on this.

Stefan