RE: [PATCH v2 09/12] vfio/ccw: Use vfio_[attach/detach]_device

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Wednesday, September 27, 2023 6:00 PM
>Subject: Re: [PATCH v2 09/12] vfio/ccw: Use vfio_[attach/detach]_device
>
>
>
>On 9/26/23 13:32, Zhenzhong Duan wrote:
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> 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>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/hw/vfio/vfio-common.h |   5 --
>>  hw/vfio/ccw.c                 | 115 ++++++++--------------------------
>>  hw/vfio/common.c              |  10 +--
>>  3 files changed, 30 insertions(+), 100 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 12fbfbc37d..c486bdef2a 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -202,7 +202,6 @@ typedef struct {
>>      hwaddr pages;
>>  } VFIOBitmap;
>>
>> -void vfio_put_base_device(VFIODevice *vbasedev);
>>  void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>>  void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
>>  void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
>> @@ -220,11 +219,7 @@ 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);
>>  struct vfio_device_info *vfio_get_device_info(int fd);
>> -int vfio_get_device(VFIOGroup *group, const char *name,
>> -                    VFIODevice *vbasedev, Error **errp);
>>  int vfio_attach_device(char *name, 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 1e2fce83b0..6893a30ab1 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -572,88 +572,14 @@ 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)
>> -{
>> -    S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev);
>> -    char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
>> -                                 cdev->hostid.ssid,
>> -                                 cdev->hostid.devid);
>Digging into that few months later,
>
>new vfio_device_groupid uses
>
>+    tmp = g_strdup_printf("%s/iommu_group", vbasedev->sysfsdev);
>
>which is set as a prop here
>    DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev),
>we need to double check if this matches, this is not obvious at first sight. 
>At least
>if this is true this needs to be documented in the commit msg

Good finding. Indeed, there is a gap here if we use a symbol link as sysfsdev.
See s390_ccw_get_dev_info() for details. But if it's not a symbol link, I think
they are same.

>
>then the subchannel name is
>    char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
>                                 cdev->hostid.ssid,
>                                 cdev->hostid.devid);
>    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;
>        }
>    }
>
>while new code use
>+    QLIST_FOREACH(vbasedev_iter, &group->device_list, next) {
>+        if (strcmp(vbasedev_iter->name, vbasedev->name) == 0) {
>+            error_setg(errp, "device is already attached");
>+            vfio_put_group(group);
>+            return -EBUSY;
>+        }
>+    }
>
>We really need to double check the names, ie.
>name, vbasedev->name. That's a mess and that's my bad.

Thanks for catching, I think below change will make it same as original code:

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 6893a30ab1..a8ea0a6fa8 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -580,6 +580,9 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
     VFIODevice *vbasedev = &vcdev->vdev;
     Error *err = NULL;
     int ret;
+    char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
+                                 cdev->hostid.ssid,
+                                 cdev->hostid.devid);

     /* Call the class init function for subchannel. */
     if (cdc->realize) {
@@ -591,7 +594,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)

     vbasedev->ops = &vfio_ccw_ops;
     vbasedev->type = VFIO_DEVICE_TYPE_CCW;
-    vbasedev->name = g_strdup(cdev->mdevid);
+    vbasedev->name = name;
     vbasedev->dev = &vcdev->cdev.parent_obj.parent_obj;

     /*
@@ -604,7 +607,7 @@ static void vfio_ccw_realize(DeviceState *dev, Error **errp)
      */
     vbasedev->ram_block_discard_allowed = true;

-    ret = vfio_attach_device(vbasedev->name, vbasedev,
+    ret = vfio_attach_device(cdev->mdevid, vbasedev,
                              &address_space_memory, errp);
     if (ret) {
         goto out_attach_dev_err;

Thanks
Zhenzhong