Re: [Qemu-devel] [PATCH v3 5/8] virtio-ccw: cleanup.

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Mon, 15 Apr 2013 16:31:24 +0200
KONRAD Frédéric <fred.konrad@greensocs.com> wrote:

> On 15/04/2013 15:38, Cornelia Huck wrote:
> > On Sun, 14 Apr 2013 23:26:34 +0200
> > fred.konrad@greensocs.com wrote:
> >
> >> From: KONRAD Frederic <fred.konrad@greensocs.com>
> >>
> >> This is a cleanup for virtio-ccw.
> >> The init function is replaced by the device_plugged callback from
> >> virtio-bus.
> > Hm, so you replaced a callback that might return an error by another
> > one that can't...
> Yes, I think this is not the right thing to do.
> 
> Originally, virtio-device was able to be hot-plugged in virtio-bus, 
> that's why I
> used this callback.
> 
> So two solutions,
>      * Leave the init function as this.
>      * Modify the callback prototype (returning the error).
> 
> And probably do the same with PCI and S390.
> 
> What do you think is the best?

For virtio-ccw, I want to be able to fail initialization if the user
specified an invalid devno or if the channel subsystem is full.
Moreover, the exit function removes the subchannel from the
configuration, and I'd like that to be symmetrical to the init function
adding the subchannel.

So I think I'd prefer keeping the init function. It might be a good
idea to move generating the crws to the plugging function, though.

> 
> Thanks,
> Fred
> 
> >
> >> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> >> ---
> >>   hw/s390x/virtio-ccw.c | 34 ++++++++++++++--------------------
> >>   1 file changed, 14 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >> index 5d62606..4857f97 100644
> >> --- a/hw/s390x/virtio-ccw.c
> >> +++ b/hw/s390x/virtio-ccw.c
> >> @@ -392,8 +392,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >>       return ret;
> >>   }
> >>
> >> -static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >> +/* 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);
> >>       unsigned int cssid = 0;
> >>       unsigned int ssid = 0;
> >>       unsigned int schid;
> >> @@ -401,7 +403,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >>       bool have_devno = false;
> >>       bool found = false;
> >>       SubchDev *sch;
> >> -    int ret;
> >>       int num;
> >>       DeviceState *parent = DEVICE(dev);
> >>
> >> @@ -410,7 +411,7 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >>       sch->driver_data = dev;
> >>       dev->sch = sch;
> >>
> >> -    dev->vdev = vdev;
> >> +    dev->vdev = dev->bus.vdev;
> >>       dev->indicators = 0;
> >>
> >>       /* Initialize subchannel structure. */
> >> @@ -425,19 +426,16 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >>           num = sscanf(dev->bus_id, "%x.%x.%04x", &cssid, &ssid, &devno);
> >>           if (num == 3) {
> >>               if ((cssid > MAX_CSSID) || (ssid > MAX_SSID)) {
> >> -                ret = -EINVAL;
> >>                   error_report("Invalid cssid or ssid: cssid %x, ssid %x",
> >>                                cssid, ssid);
> >>                   goto out_err;
> >>               }
> >>               /* Enforce use of virtual cssid. */
> >>               if (cssid != VIRTUAL_CSSID) {
> >> -                ret = -EINVAL;
> >>                   error_report("cssid %x not valid for virtio devices", cssid);
> >>                   goto out_err;
> >>               }
> >>               if (css_devno_used(cssid, ssid, devno)) {
> >> -                ret = -EEXIST;
> >>                   error_report("Device %x.%x.%04x already exists", cssid, ssid,
> >>                                devno);
> >>                   goto out_err;
> >> @@ -447,7 +445,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >>               sch->devno = devno;
> >>               have_devno = true;
> >>           } else {
> >> -            ret = -EINVAL;
> >>               error_report("Malformed devno parameter '%s'", dev->bus_id);
> >>               goto out_err;
> >>           }
> >> @@ -464,7 +461,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >>               }
> >>           }
> >>           if (!found) {
> >> -            ret = -ENODEV;
> >>               error_report("No free subchannel found for %x.%x.%04x", cssid, ssid,
> >>                            devno);
> >>               goto out_err;
> >> @@ -488,7 +484,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >>                           if (devno == MAX_SCHID) {
> >>                               devno = 0;
> >>                           } else if (devno == schid - 1) {
> >> -                            ret = -ENODEV;
> >>                               error_report("No free devno found");
> >>                               goto out_err;
> >>                           } else {
> >> @@ -506,7 +501,6 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >>               }
> >>           }
> >>           if (!found) {
> >> -            ret = -ENODEV;
> >>               error_report("Virtual channel subsystem is full!");
> >>               goto out_err;
> >>           }
> >> @@ -525,20 +519,19 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> >>       sch->id.cu_type = VIRTIO_CCW_CU_TYPE;
> >>       sch->id.cu_model = dev->vdev->device_id;
> >>
> >> -    virtio_bind_device(vdev, &virtio_ccw_bindings, DEVICE(dev));
> >>       /* Only the first 32 feature bits are used. */
> >> -    dev->host_features[0] = vdev->get_features(vdev, dev->host_features[0]);
> >> +    dev->host_features[0] = dev->vdev->get_features(dev->vdev,
> >> +                                                    dev->host_features[0]);
> >>       dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> >>       dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> >>
> >>       css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
> >>                             parent->hotplugged, 1);
> >> -    return 0;
> >> +    return;
> >>
> >>   out_err:
> >>       dev->sch = NULL;
> >>       g_free(sch);
> >> -    return ret;
> > What happens here? We now have a VirtioCcwDevice without an associated
> > subchannel device (i. e. no way to do any I/O). With the old code, we'd
> > just have failed initializing the virtio device, but now we have a
> > useless device laying around.
> >
> >>   }
> >>
> >>   static int virtio_ccw_exit(VirtioCcwDevice *dev)
>