>-----Original Message-----
>From: Eric Auger <eric.au...@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.au...@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.au...@redhat.com>
>>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>>> Reviewed-by: Matthew Rosato <mjros...@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-
>>> 2b6b31678...@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.
>
>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 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-2b6b31678...@linux.ibm.com/
Thanks
Zhenzhong
