Re: [Qemu-devel] [PATCH 20/30] s390x: convert virtio-ccw to hotplug handler API

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 25 Sep 2014 13:08:38 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 24 Sep 2014 11:48:09 +0000
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/s390x/virtio-ccw.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> Well, I think I now see what's going on here. More below...
> 
> 
> > @@ -1620,13 +1620,13 @@ static Property virtio_ccw_properties[] = {
> >  static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> > 
> >      dc->props = virtio_ccw_properties;
> >      dc->init = virtio_ccw_busdev_init;
> >      dc->exit = virtio_ccw_busdev_exit;
> > -    dc->unplug = virtio_ccw_busdev_unplug;
> 
> Before, this callback was invoked when a device of the virtio-ccw class
> was unplugged.
> 
> >      dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
> > -
> > +    hc->unplug = virtio_ccw_busdev_unplug;
> 
> Now, this callback is supposed to be invoked instead. However, the
> unplugging code invokes the callback for the _parent bus_, which
> means...
> 
> >  }
> > 
> >  static const TypeInfo virtio_ccw_device_info = {
> 
> >  static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
> >  {
> >      SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> > +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> > 
> >      k->init = virtual_css_bridge_init;
> > +    hc->unplug = qdev_simple_device_unplug_cb;
> 
> ...we're invoking this one, as the parent bus for virtio-ccw devices is
> the virtual-css bus.
> 
> If I change this callback to the virtio-ccw one, everything works as
> expected.
> 
> >  }
> 
> So, to summarize, what happened before was
> 
> bridge device              <--- (simple unplug invoked for dev)
simple unplug should not exits for above device

>  -> virtual bus
>   -> virtio proxy device   <--- virtio unplug invoked for dev

>    -> virtio bus
>     -> virtio device
> 
> which your patch changed to
> 
> bridge device
>  -> virtual bus            <--- simple unplug invoked for virtio proxy dev
>   -> virtio proxy device
>    -> virtio bus           <--- (virtio unplug invoked for virtio dev)
>     -> virtio device
> 
> Am I understanding this correctly?
Let's try other way around:

bridge device (virtual_css_bridge) - non hotpluggable sysbus device
  -> virtual bus (VIRTUAL_CSS_BUS)
   -> virtio proxy device |
    -> virtio bus         |- virtio_ccw_device_foo composite device managed with
     -> virtio device     |  device_add/del command

internal "virtio device" is the only child of "virtio bus" and it's not supposed
to be managed via device_add/del.

So to unplug "virtio proxy device" we should call virtio_ccw_busdev_unplug
stored in "bridge device" with "virtual bus" using bridge as hotplug-handler.

does following patch work for you?
---

From 8f249aa4686f0a7dfa5d9636d1eee68f1d264316 Mon Sep 17 00:00:00 2001
From: Igor Mammedov <imammedo@redhat.com>
Date: Fri, 19 Sep 2014 09:07:10 +0000
Subject: [PATCH] s390x: convert virtio-ccw to hotplug handler API

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/s390x/virtio-ccw.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 33a1d86..036bd20 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -230,7 +230,7 @@ VirtualCssBus *virtual_css_bus_init(void)
     cbus = VIRTUAL_CSS_BUS(bus);
 
     /* Enable hotplugging */
-    bus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(bus, dev, &error_abort);
 
     return cbus;
 }
@@ -1590,7 +1590,8 @@ static int virtio_ccw_busdev_exit(DeviceState *dev)
     return _info->exit(_dev);
 }
 
-static int virtio_ccw_busdev_unplug(DeviceState *dev)
+static void virtio_ccw_busdev_unplug(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
 {
     VirtioCcwDevice *_dev = (VirtioCcwDevice *)dev;
     SubchDev *sch = _dev->sch;
@@ -1609,7 +1610,6 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev)
     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid, 1, 0);
 
     object_unparent(OBJECT(dev));
-    return 0;
 }
 
 static Property virtio_ccw_properties[] = {
@@ -1624,9 +1624,7 @@ static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
     dc->props = virtio_ccw_properties;
     dc->init = virtio_ccw_busdev_init;
     dc->exit = virtio_ccw_busdev_exit;
-    dc->unplug = virtio_ccw_busdev_unplug;
     dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
-
 }
 
 static const TypeInfo virtio_ccw_device_info = {
@@ -1636,6 +1634,10 @@ static const TypeInfo virtio_ccw_device_info = {
     .class_init = virtio_ccw_device_class_init,
     .class_size = sizeof(VirtIOCCWDeviceClass),
     .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 /***************** Virtual-css Bus Bridge Device ********************/
@@ -1650,8 +1652,10 @@ static int virtual_css_bridge_init(SysBusDevice *dev)
 static void virtual_css_bridge_class_init(ObjectClass *klass, void *data)
 {
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = virtual_css_bridge_init;
+    hc->unplug = virtio_ccw_busdev_unplug;
 }
 
 static const TypeInfo virtual_css_bridge_info = {
@@ -1659,6 +1663,10 @@ static const TypeInfo virtual_css_bridge_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SysBusDevice),
     .class_init    = virtual_css_bridge_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 /* virtio-ccw-bus */
@@ -1667,13 +1675,11 @@ static void virtio_ccw_bus_new(VirtioBusState *bus, size_t bus_size,
                                VirtioCcwDevice *dev)
 {
     DeviceState *qdev = DEVICE(dev);
-    BusState *qbus;
     char virtio_bus_name[] = "virtio-bus";
 
     qbus_create_inplace(bus, bus_size, TYPE_VIRTIO_CCW_BUS,
                         qdev, virtio_bus_name);
-    qbus = BUS(bus);
-    qbus->allow_hotplug = 1;
+    qbus_set_hotplug_handler(BUS(bus), qdev, &error_abort);
 }
 
 static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
-- 
1.8.3.1