[Qemu-devel] [PATCH RFC 0/2] virtio-ccw realization/plugged handling

[Qemu-devel] [PATCH RFC 0/2] virtio-ccw realization/plugged handling

[Cornelia Huck]

Turns out that reworking the realization/plugging sequence for virtio-ccw
to make it more similar to virtio-pci (as discussed in
https://marc.info/?l=qemu-devel&m=142954827813046&w=2) was easier than
expected. At least I have something that seems to look sane, produces
working virtio-ccw devices and does what is expected on device_add/
device_del.

I have not yet verified how this interacts with Shannon's latest patches
(https://marc.info/?l=qemu-devel&m=142961349029986&w=2), but I think
it should work.

Feedback welcome. I do hope being more like virtio-pci will benefit us
in the future :)

Cornelia Huck (2):
  virtio-ccw: change realization sequence
  virtio-ccw: implement ->device_plugged

 hw/s390x/virtio-ccw.c | 72 +++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 34 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH RFC 1/2] virtio-ccw: change realization sequence

[Cornelia Huck]

virtio-ccw has an odd sequence of realizing devices: first the
device-specific relization (net, block, ...), then the generic
realization. It feels less odd to have the generic realization
callback trigger the device-specific realization instead (and this
also matches what virtio-pci does).

One thing to note: We need to defer initializing the cu model in the
sense id data until after the device-specific realization has been
performed, as we need to refer to the virtio device's device_id.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/virtio-ccw.c | 41 +++++++++++++++++------------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index d32ecaf..d5952ee 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -615,8 +615,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
     return ret;
 }
 
-static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
-                                      VirtIODevice *vdev, Error **errp)
+static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
 {
     unsigned int cssid = 0;
     unsigned int ssid = 0;
@@ -627,6 +626,9 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
     SubchDev *sch;
     int num;
     DeviceState *parent = DEVICE(dev);
+    Error *err = NULL;
+    VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
+    VirtIODevice *vdev;
 
     sch = g_malloc0(sizeof(SubchDev));
 
@@ -739,6 +741,18 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
     memset(&sch->id, 0, sizeof(SenseId));
     sch->id.reserved = 0xff;
     sch->id.cu_type = VIRTIO_CCW_CU_TYPE;
+
+    if (k->realize) {
+        k->realize(dev, &err);
+    }
+    if (err) {
+        error_propagate(errp, err);
+        css_subch_assign(cssid, ssid, schid, devno, NULL);
+        goto out_err;
+    }
+
+    /* device_id is only set after vdev has been realized */
+    vdev = virtio_ccw_get_vdev(sch);
     sch->id.cu_model = vdev->device_id;
 
     /* Only the first 32 feature bits are used. */
@@ -786,10 +800,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
-        return;
     }
-
-    virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
 }
 
 static void virtio_ccw_net_instance_init(Object *obj)
@@ -812,10 +823,7 @@ static void virtio_ccw_blk_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
-        return;
     }
-
-    virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
 }
 
 static void virtio_ccw_blk_instance_init(Object *obj)
@@ -852,10 +860,7 @@ static void virtio_ccw_serial_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
-        return;
     }
-
-    virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
 }
 
 
@@ -877,10 +882,7 @@ static void virtio_ccw_balloon_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
-        return;
     }
-
-    virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
 }
 
 static void balloon_ccw_stats_get_all(Object *obj, struct Visitor *v,
@@ -945,10 +947,7 @@ static void virtio_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
-        return;
     }
-
-    virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
 }
 
 static void virtio_ccw_scsi_instance_init(Object *obj)
@@ -972,10 +971,7 @@ static void vhost_ccw_scsi_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     object_property_set_bool(OBJECT(vdev), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
-        return;
     }
-
-    virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
 }
 
 static void vhost_ccw_scsi_instance_init(Object *obj)
@@ -1003,8 +999,6 @@ static void virtio_ccw_rng_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     object_property_set_link(OBJECT(dev),
                              OBJECT(dev->vdev.conf.rng), "rng",
                              NULL);
-
-    virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
 }
 
 /* DeviceState to VirtioCcwDevice. Note: used on datapath,
@@ -1598,10 +1592,9 @@ static const TypeInfo virtio_ccw_rng = {
 static void virtio_ccw_busdev_realize(DeviceState *dev, Error **errp)
 {
     VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
-    VirtIOCCWDeviceClass *_info = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
 
     virtio_ccw_bus_new(&_dev->bus, sizeof(_dev->bus), _dev);
-    _info->realize(_dev, errp);
+    virtio_ccw_device_realize(_dev, errp);
 }
 
 static int virtio_ccw_busdev_exit(DeviceState *dev)
-- 
2.1.4

[Qemu-devel] [PATCH RFC 2/2] virtio-ccw: implement ->device_plugged

[Cornelia Huck]

Let's move operations that are only valid after the backend has been
realized to a ->device_plugged callback, just as virtio-pci does.
Also reorder setting up the host feature bits to the sequence used
by virtio-pci.

While we're at it, also add a ->device_unplugged callback to stop
ioeventfd, just to be on the safe side.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/virtio-ccw.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index d5952ee..0b5f5e3 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -625,10 +625,8 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
     bool found = false;
     SubchDev *sch;
     int num;
-    DeviceState *parent = DEVICE(dev);
     Error *err = NULL;
     VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
-    VirtIODevice *vdev;
 
     sch = g_malloc0(sizeof(SubchDev));
 
@@ -751,19 +749,6 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
         goto out_err;
     }
 
-    /* device_id is only set after vdev has been realized */
-    vdev = virtio_ccw_get_vdev(sch);
-    sch->id.cu_model = vdev->device_id;
-
-    /* Only the first 32 feature bits are used. */
-    dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus,
-                                                         dev->host_features[0]);
-
-    virtio_add_feature(&dev->host_features[0], VIRTIO_F_NOTIFY_ON_EMPTY);
-    virtio_add_feature(&dev->host_features[0], VIRTIO_F_BAD_FEATURE);
-
-    css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
-                          parent->hotplugged, 1);
     return;
 
 out_err:
@@ -1393,6 +1378,30 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
     return 0;
 }
 
+/* This is called by virtio-bus just after the device is plugged. */
+static void virtio_ccw_device_plugged(DeviceState *d)
+{
+    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+    SubchDev *sch = dev->sch;
+
+    sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
+
+    /* Only the first 32 feature bits are used. */
+    virtio_add_feature(&dev->host_features[0], VIRTIO_F_NOTIFY_ON_EMPTY);
+    virtio_add_feature(&dev->host_features[0], VIRTIO_F_BAD_FEATURE);
+    dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus,
+                                                         dev->host_features[0]);
+
+    css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
+                          d->hotplugged, 1);
+}
+
+static void virtio_ccw_device_unplugged(DeviceState *d)
+{
+    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+
+    virtio_ccw_stop_ioeventfd(dev);
+}
 /**************** Virtio-ccw Bus Device Descriptions *******************/
 
 static Property virtio_ccw_net_properties[] = {
@@ -1708,6 +1717,8 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->load_queue = virtio_ccw_load_queue;
     k->save_config = virtio_ccw_save_config;
     k->load_config = virtio_ccw_load_config;
+    k->device_plugged = virtio_ccw_device_plugged;
+    k->device_unplugged = virtio_ccw_device_unplugged;
 }
 
 static const TypeInfo virtio_ccw_bus_info = {
-- 
2.1.4