Re: [Intel-gfx] [PATCH v5 10/19] vfio: Add infrastructure for bind_iommufd from userspace

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Feb 27, 2023 at 03:11:26AM -0800, Yi Liu wrote:
> For the device fd opened from cdev, userspace needs to bind it to an
> iommufd and attach it to IOAS managed by iommufd. With such operations,
> userspace can set up a secure DMA context and hence access device.
> 
> This changes the existing vfio_iommufd_bind() to accept a pt_id pointer
> as an optional input, and also an dev_id pointer to selectively return
> the dev_id to prepare for adding bind_iommufd ioctl, which does the bind
> first and then attach IOAS.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
>  drivers/vfio/group.c     | 17 ++++++++++++++---
>  drivers/vfio/iommufd.c   | 21 +++++++++------------
>  drivers/vfio/vfio.h      |  9 ++++++---
>  drivers/vfio/vfio_main.c | 10 ++++++----
>  4 files changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index d8771d585cb1..e44232551448 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -169,6 +169,7 @@ static void vfio_device_group_get_kvm_safe(struct vfio_device *device)
>  static int vfio_device_group_open(struct vfio_device_file *df)
>  {
>  	struct vfio_device *device = df->device;
> +	u32 ioas_id;
>  	int ret;
>  
>  	mutex_lock(&device->group->group_lock);
> @@ -177,6 +178,13 @@ static int vfio_device_group_open(struct vfio_device_file *df)
>  		goto out_unlock;
>  	}
>  
> +	if (device->group->iommufd) {
> +		ret = iommufd_vfio_compat_ioas_id(device->group->iommufd,
> +						  &ioas_id);
> +		if (ret)
> +			goto out_unlock;
> +	}

I don't really like this being moved out of iommufd.c

Pass in a NULL pt_id and the do some

> -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
> +int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx,
> +		      u32 *dev_id, u32 *pt_id)
>  {
> -	u32 ioas_id;
>  	u32 device_id;
>  	int ret;
>  
> @@ -29,17 +29,14 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
>  	if (ret)
>  		return ret;
>  
> -	ret = iommufd_vfio_compat_ioas_id(ictx, &ioas_id);
> -	if (ret)
> -		goto err_unbind;

  io_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx,
		      u32 *dev_id, u32 *pt_id)
{
   u32 tmp_pt_id;
   if (!pt_id) {
       pt_id = &tmp_pt_id;
       ret = iommufd_vfio_compat_ioas_id(ictx, pt_id);
       if (ret)
		goto err_unbind;
  
   }

To handle it

And the commit message is sort of out of sync with the patch, more like:

vfio: Pass the pt_id as an argument to vfio_iommufd_bind()

To support binding the cdev the pt_id must come from userspace instead
of being forced to the compat_ioas_id.


Jason