RE: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device

[Duan, Zhenzhong]

Hi Eric,

>-----Original Message-----
>From: qemu-devel-bounces+zhenzhong.duan=intel.com@nongnu.org <qemu-
>devel-bounces+zhenzhong.duan=intel.com@nongnu.org> On Behalf Of Eric
>Auger
>Sent: Monday, October 9, 2023 4:15 PM
>Subject: Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>
>Hi Zhenzhong,
>
>On 10/9/23 03:25, Duan, Zhenzhong wrote:
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.auger@redhat.com>
>>> Sent: Monday, October 9, 2023 1:46 AM
>>> Subject: Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>>>
>>> Hi Zhenzhong,
>>> On 10/8/23 12:21, Duan, Zhenzhong wrote:
>>>> Hi Eric,
>>>>
>>>>> -----Original Message-----
>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>> Sent: Wednesday, October 4, 2023 11:44 PM
>>>>> Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>>>>>
>>>>> Let the vfio-ccw device use vfio_attach_device() and
>>>>> vfio_detach_device(), hence hiding the details of the used
>>>>> IOMMU backend.
>>>>>
>>>>> Note that the migration reduces the following trace
>>>>> "vfio: subchannel %s has already been attached" (featuring
>>>>> cssid.ssid.devid) into "device is already attached"
>>>>>
>>>>> 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>
>>>>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>>>>
>>>>> ---
>>>>>
>>>>> v3:
>>>>> - simplified vbasedev->dev setting
>>>>>
>>>>> v2 -> v3:
>>>>> - Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev
>>>>>  while keeping into account Matthew's comment
>>>>>  https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-
>>>>> 2b6b31678b53@linux.ibm.com/
>>>>> ---
>>>>> include/hw/vfio/vfio-common.h |   5 --
>>>>> hw/vfio/ccw.c                 | 122 +++++++++-------------------------
>>>>> hw/vfio/common.c              |  10 +--
>>>>> 3 files changed, 37 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..6ec35fedc9 100644
>>>>> --- a/hw/vfio/ccw.c
>>>>> +++ b/hw/vfio/ccw.c
>>>>> @@ -572,88 +572,15 @@ 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);
>>>>> -    VFIODevice *vbasedev;
>>>>> -
>>>>> -    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;
>>>>> -        }
>>>>> -    }
>>>>> -
>>>>> -    /*
>>>>> -     * All vfio-ccw devices are believed to operate in a way compatible 
>>>>> with
>>>>> -     * discarding of memory in RAM blocks, ie. pages pinned in the host 
>>>>> are
>>>>> -     * in the current working set of the guest driver and therefore never
>>>>> -     * overlap e.g., with pages available to the guest balloon driver.  
>>>>> This
>>>>> -     * needs to be set before vfio_get_device() for vfio common to handle
>>>>> -     * ram_block_discard_disable().
>>>>> -     */
>>>>> -    vcdev->vdev.ram_block_discard_allowed = true;
>>>>> -
>>>>> -    if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, errp)) {
>>>>> -        goto out_err;
>>>>> -    }
>>>>> -
>>>>> -    vcdev->vdev.ops = &vfio_ccw_ops;
>>>>> -    vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
>>>>> -    vcdev->vdev.name = name;
>>>>> -    vcdev->vdev.dev = DEVICE(vcdev);
>>>>> -
>>>>> -    return;
>>>>> -
>>>>> -out_err:
>>>>> -    g_free(name);
>>>>> -}
>>>>> -
>>>>> -static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error
>**errp)
>>>>> -{
>>>>> -    char *tmp, group_path[PATH_MAX];
>>>>> -    ssize_t len;
>>>>> -    int groupid;
>>>>> -
>>>>> -    tmp =
>>> g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s/iommu_group",
>>>>> -                          cdev->hostid.cssid, cdev->hostid.ssid,
>>>>> -                          cdev->hostid.devid, cdev->mdevid);
>>>>> -    len = readlink(tmp, group_path, sizeof(group_path));
>>>>> -    g_free(tmp);
>>>>> -
>>>>> -    if (len <= 0 || len >= sizeof(group_path)) {
>>>>> -        error_setg(errp, "vfio: no iommu_group found");
>>>>> -        return NULL;
>>>>> -    }
>>>>> -
>>>>> -    group_path[len] = 0;
>>>>> -
>>>>> -    if (sscanf(basename(group_path), "%d", &groupid) != 1) {
>>>>> -        error_setg(errp, "vfio: failed to read %s", group_path);
>>>>> -        return NULL;
>>>>> -    }
>>>>> -
>>>>> -    return vfio_get_group(groupid, &address_space_memory, errp);
>>>>> -}
>>>>> -
>>>>> static void vfio_ccw_realize(DeviceState *dev, Error **errp)
>>>>> {
>>>>> -    VFIOGroup *group;
>>>>>     S390CCWDevice *cdev = S390_CCW_DEVICE(dev);
>>>>>     VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
>>>>>     S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
>>>>> +    VFIODevice *vbasedev = &vcdev->vdev;
>>>>>     Error *err = NULL;
>>>>> +    char *name;
>>>>> +    int ret;
>>>>>
>>>>>     /* Call the class init function for subchannel. */
>>>>>     if (cdc->realize) {
>>>>> @@ -663,14 +590,31 @@ static void vfio_ccw_realize(DeviceState *dev,
>Error
>>>>> **errp)
>>>>>         }
>>>>>     }
>>>>>
>>>>> -    group = vfio_ccw_get_group(cdev, &err);
>>>>> -    if (!group) {
>>>>> -        goto out_group_err;
>>>>> -    }
>>>>> +    name = g_strdup_printf("%x.%x.%04x", vcdev->cdev.hostid.cssid,
>>>>> +                           vcdev->cdev.hostid.ssid,
>>>>> +                           vcdev->cdev.hostid.devid);
>>>>> +    vbasedev->sysfsdev = g_strdup_printf("/sys/bus/css/devices/%s/%s",
>>>>> +                                         name,
>>>>> +                                         cdev->mdevid);
>>>> Hoping not late for you to include this in v5.
>>>> I think no need to re-assign sysfsdev as it's a user property, we'd better 
>>>> to
>>>> keep the original user value. Also looks a memory leak here.
>>> OK I removed it.
>>>>> +    vbasedev->ops = &vfio_ccw_ops;
>>>>> +    vbasedev->type = VFIO_DEVICE_TYPE_CCW;
>>>>> +    vbasedev->name = name;
>>>> There will be a potential failure when a second mdev device under
>>>> same cssid.ssid.devid attached. We can use cdev->mdevid as name.
>>> But this mathes vfio_ccw_get_device() existing code where
>>> vcdev->vdev.name = name; and
>>> name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
>>>                                  cdev->hostid.ssid,
>>>                                  cdev->hostid.devid);
>> I suspect this is a bug of the existing code.
>Then I would prefer we fix it separately. This patch migrates the
>existing code without functional change intended.

OK, make sense. I'll do that after your v5 get in.

>
>>
>>> cdev->mdevid is passed as first arg of vfio_attach_device() instead .
>> vfio_attach_device() uses cdev->mdevid to get device FD, nothing more.
>>
>> If we use cssid.ssid.devid as name, then different mdev under same
>cssid.ssid.devid will have same name, and the second mdev attachment will fail 
>to
>attach in vfio_attach_device():
>>
>>     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;
>>         }
>>     }
>I get your point but this conversion matches the existing
>vfio_ccw_get_device() code:
>    char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
>                                 cdev->hostid.ssid,
>                                 cdev->hostid.devid);
>    VFIODevice *vbasedev;
>
>    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;
>        }
>    }
>
>>
>>> i think this also matches
>>>
>https://lore.kernel.org/all/PH7PR11MB67222DD282F98E03095FBA8A92C1A@PH
>>> 7PR11MB6722.namprd11.prod.outlook.com/
>>> no?
>> It doesn't match what Mattew suggested: https://lore.kernel.org/qemu-
>devel/6e04ab8f-dc84-e9c2-deea-2b6b31678b53@linux.ibm.com/
>this was RFC v3. At that time we did not pass any "name" arg to
>
>vfio_attach_device(VFIODevice *vbasedev, AddressSpace *as, Error **errp)
>and vbasedev->name was used when calling vfio_get_device() while we now
>use cdev->mdevid. Besides Mattew ran some basic tests on PATCH v3:
>https://lore.kernel.org/all/33b7803c-f231-d4fb-d9d9-
>26a097a89e93@redhat.com/
>So I would be tempted to leave it as is (without the sysfsdev overwrite
>which came from Mattew's suggestion in
>https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-
>2b6b31678b53@linux.ibm.com/
>). Thanks Eric

OK, then go ahead.

Thanks
Zhenzhong