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

["Duan, Zhenzhong" <zhenzhong.duan@intel.com>]

Hi Eric, Matthew,

>-----Original Message-----
>From: Matthew Rosato <mjrosato@linux.ibm.com>
>Sent: Wednesday, September 27, 2023 9:26 PM
>Subject: Re: [PATCH v2 09/12] vfio/ccw: Use vfio_[attach/detach]_device
>
>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.

Digged this further. I think it's ok even if a smbol link is provided to vbasedev->sysfsdev.
Because we just want to get iommu group number.

vfio_device_groupid() can survive even with a symbol link such as:

/sys/bus/mdev/devices/7e270a25-e163-4922-af60-757fc8ed48c6/iommu_group

>>
>>>
>>> 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/

You are right, my mistake. I just remembered I have included your suggested change
in above link to this patch. So no need to add more change here.

It looks like your change also fixed a vbasedev->name issue, from "cssid.ssid.devid"
to "mdevid".

Look forward to your test result	with this series, thanks!

BRs.
Zhenzhong