Re: [Qemu-devel] [PATCH v3 5/8] virtio-blk: use aliases instead of duplicate qdev properties

Re: [Qemu-devel] [PATCH v3 5/8] virtio-blk: use aliases instead of duplicate qdev properties

[Peter Crosthwaite]

On Fri, May 30, 2014 at 9:17 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 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 9c71afa..041d4e2 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;
> @@ -192,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);
> +    qdev_alias_all_properties(DEVICE(&dev->vdev), obj);
>  }
>
>  static int s390_virtio_serial_init(VirtIOS390Device *s390_dev)
> @@ -526,18 +526,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 f3f53dc..0cc8132 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)
> @@ -1309,7 +1309,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
>
>

Re: [Qemu-devel] [PATCH v3 3/8] virtio-blk: move x-data-plane qdev property to virtio-blk.h

[Cornelia Huck]

On Fri, 30 May 2014 13:17:44 +0200
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> 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 would add a x-data-plane property to s390-virtio devices, which
leads to insta-death if enabled since s390-virtio devices don't have
guest notifiers, no? Just an observation; in general, moving the
property makes sense to me.

> 
> 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(-)
> 

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

[Cornelia Huck]

On Fri, 30 May 2014 13:17:41 +0200
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> 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.

FWIW:

I gave your qom-alias-property branch a quick test on s390.

virtio-ccw: seems to work fine, hotunplug of virtio-blk-ccw is still
fine, and the virtio-blk memory leaks due to missing finalization that
valgind complained about are gone.

s390-virtio: still boots, but with x-data-plane=on we get the
predictable segfault in virtio_blk_data_plane_start() since s390-virtio
doesn't do notifiers. Maybe the dataplane code should do a quick check
for existence of the notifier callback when it allocates the dataplane
structure?

> 
> 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/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                   | 52 ++++++++++++++++++++++++++++++++++++++++++
>  12 files changed, 112 insertions(+), 47 deletions(-)
> 

[Qemu-devel] [PATCH 1/1] dataplane: bail out on unsupported transport

[Cornelia Huck]

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>
---
 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 e49c253..e0c653f 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -354,6 +354,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     VirtIOBlockDataPlane *s;
     int fd;
     Error *local_err = NULL;
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
     *dataplane = NULL;
 
@@ -361,6 +363,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 (blk->scsi) {
         error_setg(errp,
                    "device is incompatible with x-data-plane, use scsi=off");
-- 
1.8.5.5

Re: [Qemu-devel] [PATCH 1/1] dataplane: bail out on unsupported transport

[Stefan Hajnoczi]

On Tue, Jun 03, 2014 at 05:31:06PM +0200, Cornelia Huck wrote:
> 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>
> ---
>  hw/block/dataplane/virtio-blk.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Thanks!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Stefan Hajnoczi]

On Tue, Jun 03, 2014 at 05:11:23PM +0200, Cornelia Huck wrote:
> On Fri, 30 May 2014 13:17:41 +0200
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > 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.
> 
> FWIW:
> 
> I gave your qom-alias-property branch a quick test on s390.
> 
> virtio-ccw: seems to work fine, hotunplug of virtio-blk-ccw is still
> fine, and the virtio-blk memory leaks due to missing finalization that
> valgind complained about are gone.
> 
> s390-virtio: still boots, but with x-data-plane=on we get the
> predictable segfault in virtio_blk_data_plane_start() since s390-virtio
> doesn't do notifiers. Maybe the dataplane code should do a quick check
> for existence of the notifier callback when it allocates the dataplane
> structure?

Okay, good idea.  Thanks!

Stefan