RE: [PATCH v5 10/19] vfio: Add infrastructure for bind_iommufd from userspace

Liu, Yi L
2023-02-28T02:35:42Z

> From: Jason Gunthorpe <jgg@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@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.
>

Got it. not only pt_id, also dev_id. 😊

Regards,
Yi Liu