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

[Matthew Rosato]

On 9/27/23 8:09 AM, Duan, Zhenzhong wrote:
> 
> 
>> -----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

I haven't tried this particular version of the patches yet (either Eric F. or I 
will), but it looks like this change would re-introduce at least part of the 
breakage I reported during the RFC?

https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-2b6b31678b53@linux.ibm.com/

Thanks,
Matt