Re: [PATCH 09/13] vfio: Add infrastructure for bind_iommufd and attach

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 17 Jan 2023 05:49:38 -0800
Yi Liu <yi.l.liu@xxxxxxxxx> wrote:

> This prepares to add ioctls for device cdev fd. This infrastructure includes:
>     - add vfio_iommufd_attach() to support iommufd pgtable attach after
>       bind_iommufd. A NULL pt_id indicates detach.
>     - let vfio_iommufd_bind() to accept pt_id, e.g. the comapt_ioas_id in the
>       legacy group path, and also return back dev_id if caller requires it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
>  drivers/vfio/group.c     | 12 +++++-
>  drivers/vfio/iommufd.c   | 79 ++++++++++++++++++++++++++++++----------
>  drivers/vfio/vfio.h      | 15 ++++++--
>  drivers/vfio/vfio_main.c | 10 +++--
>  4 files changed, 88 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index 7200304663e5..9484bb1c54a9 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -157,6 +157,8 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
>  static int vfio_device_group_open(struct vfio_device_file *df)
>  {
>  	struct vfio_device *device = df->device;
> +	u32 ioas_id;
> +	u32 *pt_id = NULL;
>  	int ret;
>  
>  	mutex_lock(&device->group->group_lock);
> @@ -165,6 +167,14 @@ static int vfio_device_group_open(struct vfio_device_file *df)
>  		goto err_unlock_group;
>  	}
>  
> +	if (device->group->iommufd) {
> +		ret = iommufd_vfio_compat_ioas_id(device->group->iommufd,
> +						  &ioas_id);
> +		if (ret)
> +			goto err_unlock_group;
> +		pt_id = &ioas_id;
> +	}
> +
>  	mutex_lock(&device->dev_set->lock);
>  	/*
>  	 * Here we pass the KVM pointer with the group under the lock.  If the
> @@ -174,7 +184,7 @@ static int vfio_device_group_open(struct vfio_device_file *df)
>  	df->kvm = device->group->kvm;
>  	df->iommufd = device->group->iommufd;
>  
> -	ret = vfio_device_open(df);
> +	ret = vfio_device_open(df, NULL, pt_id);
>  	if (ret)
>  		goto err_unlock_device;
>  	mutex_unlock(&device->dev_set->lock);
> diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> index 4f82a6fa7c6c..412644fdbf16 100644
> --- a/drivers/vfio/iommufd.c
> +++ b/drivers/vfio/iommufd.c
> @@ -10,9 +10,17 @@
>  MODULE_IMPORT_NS(IOMMUFD);
>  MODULE_IMPORT_NS(IOMMUFD_VFIO);
>  
> -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
> +/* @pt_id == NULL implies detach */
> +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> +{
> +	lockdep_assert_held(&vdev->dev_set->lock);
> +
> +	return vdev->ops->attach_ioas(vdev, pt_id);
> +}


I find this patch pretty confusing, I think it's rooted in all these
multiplexed interfaces, which extend all the way out to userspace with
a magic, reserved page table ID to detach a device from an IOAS.  It
seems like it would be simpler to make a 'detach' API, a detach_ioas
callback on the vfio_device_ops, and certainly not an
vfio_iommufd_attach() function that does a detach provided the correct
args while also introducing a __vfio_iommufd_detach() function.

This series is also missing an update to
Documentation/driver-api/vfio.rst, which is already behind relative to
the iommufd interfaces.  Thanks,

Alex




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux