> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Tuesday, February 28, 2023 2:39 AM
>
> On Mon, Feb 27, 2023 at 03:11:33AM -0800, Yi Liu wrote:
> > This adds ioctl for userspace to attach device cdev fd to and detach
> > from IOAS/hw_pagetable managed by iommufd.
> >
> > VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach vfio device to IOAS,
> hw_pagetable
> > managed by iommufd. Attach can be
> > undo by
> VFIO_DEVICE_DETACH_IOMMUFD_PT
> > or device fd close.
> > VFIO_DEVICE_DETACH_IOMMUFD_PT: detach vfio device from the
> current attached
> > IOAS or hw_pagetable managed by
> iommufd.
> >
> > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > ---
> > drivers/vfio/device_cdev.c | 76
> ++++++++++++++++++++++++++++++++++++++
> > drivers/vfio/vfio.h | 16 ++++++++
> > drivers/vfio/vfio_main.c | 8 ++++
> > include/uapi/linux/vfio.h | 52 ++++++++++++++++++++++++++
> > 4 files changed, 152 insertions(+)
> >
> > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > index 37f80e368551..5b5a249a6612 100644
> > --- a/drivers/vfio/device_cdev.c
> > +++ b/drivers/vfio/device_cdev.c
> > @@ -191,6 +191,82 @@ long vfio_device_ioctl_bind_iommufd(struct
> vfio_device_file *df,
> > return ret;
> > }
> >
> > +int vfio_ioctl_device_attach(struct vfio_device_file *df,
> > + void __user *arg)
>
> This should be
>
> struct vfio_device_attach_iommufd_pt __user *arg
Got it.
> > +{
> > + struct vfio_device *device = df->device;
> > + struct vfio_device_attach_iommufd_pt attach;
> > + unsigned long minsz;
> > + int ret;
> > +
> > + minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
> > +
> > + if (copy_from_user(&attach, (void __user *)arg, minsz))
>
> No cast
Yes.
> > + return -EFAULT;
> > +
> > + if (attach.argsz < minsz || attach.flags ||
> > + attach.pt_id == IOMMUFD_INVALID_ID)
> > + return -EINVAL;
> > +
> > + if (!device->ops->bind_iommufd)
> > + return -ENODEV;
> > +
> > + mutex_lock(&device->dev_set->lock);
> > + if (df->noiommu) {
> > + ret = -EINVAL;
> > + goto out_unlock;
> > + }
> > +
> > + ret = device->ops->attach_ioas(device, &attach.pt_id);
> > + if (ret)
> > + goto out_unlock;
> > +
> > + ret = copy_to_user((void __user *)arg +
> > + offsetofend(struct
> vfio_device_attach_iommufd_pt, flags),
>
> This should just be &arg->flags
Yes, can use arg->xxx here. I guess you mean &arg->pt_id.
>
> > + &attach.pt_id,
> > + sizeof(attach.pt_id)) ? -EFAULT : 0;
>
> Also:
>
> static_assert(__same_type(arg->flags), attach.pt_id);
Got it. but s/arg->flags/arg->pt_id/
> > + if (ret)
> > + goto out_detach;
> > + mutex_unlock(&device->dev_set->lock);
> > +
> > + return 0;
> > +
> > +out_detach:
> > + device->ops->detach_ioas(device);
>
>
> > +out_unlock:
> > + mutex_unlock(&device->dev_set->lock);
> > + return ret;
> > +}
> > +
> > +int vfio_ioctl_device_detach(struct vfio_device_file *df,
> > + void __user *arg)
> > +{
> > + struct vfio_device *device = df->device;
> > + struct vfio_device_detach_iommufd_pt detach;
> > + unsigned long minsz;
> > +
> > + minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
> > +
> > + if (copy_from_user(&detach, (void __user *)arg, minsz))
> > + return -EFAULT;
>
> Same comments here
Sure.
> > +
> > + if (detach.argsz < minsz || detach.flags)
> > + return -EINVAL;
> > +
> > + if (!device->ops->bind_iommufd)
> > + return -ENODEV;
> > +
> > + mutex_lock(&device->dev_set->lock);
> > + if (df->noiommu) {
> > + mutex_unlock(&device->dev_set->lock);
> > + return -EINVAL;
> > + }
>
> This seems strange. no iommu mode should have a NULL dev->iommufctx.
> Why do we have a df->noiommu at all?
This is due to the vfio_device_first_open(). Detail as below comment (part of
patch 0016).
+ /*
+ * For group/container path, iommufd pointer is NULL when comes
+ * into this helper. Its noiommu support is handled by
+ * vfio_device_group_use_iommu()
+ *
+ * For iommufd compat mode, iommufd pointer here is a valid value.
+ * Its noiommu support is in vfio_iommufd_bind().
+ *
+ * For device cdev path, iommufd pointer here is a valid value for
+ * normal cases, but it is NULL if it's noiommu. Check df->noiommu
+ * to differentiate cdev noiommu from the group/container path which
+ * also passes NULL iommufd pointer in. If set then do nothing.
+ */
if (iommufd)
ret = vfio_iommufd_bind(device, iommufd, dev_id, pt_id);
- else
+ else if (!df->noiommu)
ret = vfio_device_group_use_iommu(device);
if (ret)
goto err_module_put;
Regards,
Yi Liu
