Hi Zhenzhong,
On 10/9/23 03:25, Duan, Zhenzhong wrote:
>
>> -----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.
Then I would prefer we fix it separately. This patch migrates the
existing code without functional change intended.
>
>> 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-2b6b31678...@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-26a097a89...@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-2b6b31678...@linux.ibm.com/
). Thanks Eric
>
> Thanks
> Zhenzhong
>
