>-----Original Message-----
>From: Eric Auger <eric.au...@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.au...@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.au...@redhat.com>
>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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
