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

[Liu, Yi L]

> From: Liu, Yi L <yi.l.liu at intel.com>
> Sent: Tuesday, February 28, 2023 10:35 AM
> 
> > From: Jason Gunthorpe <jgg at nvidia.com>
> > Sent: Tuesday, February 28, 2023 2:30 AM
> >
> > 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 at intel.com>
> > > Reviewed-by: Kevin Tian <kevin.tian at 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.
> >

Seems like pt_id is no more needed in the vfio_iommufd_bind()
since it can get compat_ioas_id in the function itself. Cdev path
never passes a pt_id to vfio_iommufd_bind() as its attach is done
by separate ATTACH ioctl. Can we use the dev_id pointer to indicate
if it needs to get the compat ioas and attach it?

vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx,
		      u32 *dev_id)
{
...
        if (!dev_id) {
             u32 ioas_id;

             ret = iommufd_vfio_compat_ioas_id(ictx, &ioas_id);
             if (ret)
		goto err_unbind;

             ret = vdev->ops->attach_ioas(vdev, &ioas_id);
             if (ret)
		goto err_unbind;
       }
...
}

Regards,
Yi Liu