Re: [RFC v3 11/18] vfio/ccw: Use vfio_[attach/detach]_device

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 1/31/23 3:52 PM, Eric Auger wrote:
> Let the vfio-ccw device use vfio_attach_device() and
> vfio_detach_device(), hence hiding the details of the used
> IOMMU backend.
> 
> Also now all the devices have been migrated to use the new
> vfio_attach_device/vfio_detach_device API, let's turn the
> legacy functions into static functions, local to container.c.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Hi Eric,

While testing the cdev series on s390 I ran into a couple of issues with this patch, see below.

> ---
>  include/hw/vfio/vfio-common.h |   4 --
>  hw/vfio/ccw.c                 | 118 ++++++++--------------------------
>  hw/vfio/container.c           |   8 +--
>  3 files changed, 32 insertions(+), 98 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 9465c4b021..1580f9617c 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -176,10 +176,6 @@ void vfio_region_unmap(VFIORegion *region);
>  void vfio_region_exit(VFIORegion *region);
>  void vfio_region_finalize(VFIORegion *region);
>  void vfio_reset_handler(void *opaque);
> -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp);
> -void vfio_put_group(VFIOGroup *group);
> -int vfio_get_device(VFIOGroup *group, const char *name,
> -                    VFIODevice *vbasedev, Error **errp);
>  int vfio_attach_device(VFIODevice *vbasedev, AddressSpace *as, Error **errp);
>  void vfio_detach_device(VFIODevice *vbasedev);
>  
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 0354737666..6fde7849cc 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -579,27 +579,32 @@ static void vfio_ccw_put_region(VFIOCCWDevice *vcdev)
>      g_free(vcdev->io_region);
>  }
>  
> -static void vfio_ccw_put_device(VFIOCCWDevice *vcdev)
> -{
> -    g_free(vcdev->vdev.name);
> -    vfio_put_base_device(&vcdev->vdev);
> -}
> -
> -static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
> -                                Error **errp)
> +static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>  {
> +    CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev);
> +    S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj, ccw_dev);
> +    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> +    S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
> +    VFIODevice *vbasedev = &vcdev->vdev;
> +    Error *err = NULL;
>      char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid,
>                                   vcdev->cdev.hostid.ssid,
>                                   vcdev->cdev.hostid.devid);


We can't get these cssid, ssid, devid values quite yet, they are currently 0s.  That has to happen after cdc->realize()


> -    VFIODevice *vbasedev;
> +    int ret;
>  
> -    QLIST_FOREACH(vbasedev, &group->device_list, next) {
> -        if (strcmp(vbasedev->name, name) == 0) {
> -            error_setg(errp, "vfio: subchannel %s has already been attached",
> -                       name);
> -            goto out_err;
> +    /* Call the class init function for subchannel. */
> +    if (cdc->realize) {
> +        cdc->realize(cdev, vcdev->vdev.sysfsdev, &err);
> +        if (err) {
> +            goto out_err_propagate;
>          }
>      }
> +    vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s",
> +                                         name, cdev->mdevid);
> +    vbasedev->ops = &vfio_ccw_ops;
> +    vbasedev->type = VFIO_DEVICE_TYPE_CCW;
> +    vbasedev->name = name;

vbasedev->name is being set to the wrong value here, it needs to be the uuid.

See below for a suggested diff on top of this patch that solves the issue for me.

Thanks,
Matt

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 6fde7849cc..394b73358f 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -587,9 +587,6 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
     S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
     VFIODevice *vbasedev = &vcdev->vdev;
     Error *err = NULL;
-    char *name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid,
-                                 vcdev->cdev.hostid.ssid,
-                                 vcdev->cdev.hostid.devid);
     int ret;
 
     /* Call the class init function for subchannel. */
@@ -599,11 +596,14 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
             goto out_err_propagate;
         }
     }
-    vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s",
-                                         name, cdev->mdevid);
+    vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s",
+                                         vcdev->cdev.hostid.cssid,
+                                         vcdev->cdev.hostid.ssid,
+                                         vcdev->cdev.hostid.devid,
+                                         cdev->mdevid);
     vbasedev->ops = &vfio_ccw_ops;
     vbasedev->type = VFIO_DEVICE_TYPE_CCW;
-    vbasedev->name = name;
+    vbasedev->name = g_strdup(cdev->mdevid);
     vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj;
 
     /*