> From: Alex Williamson <[email protected]> > Sent: Tuesday, May 23, 2023 11:50 PM > > On Tue, 23 May 2023 01:20:17 +0000 > "Liu, Yi L" <[email protected]> wrote: > > > > From: Alex Williamson <[email protected]> > > > Sent: Tuesday, May 23, 2023 6:16 AM > > > > > > On Sat, 13 May 2023 06:28:24 -0700 > > > Yi Liu <[email protected]> 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. > > > > > > > > Tested-by: Yanting Jiang <[email protected]> > > > > Tested-by: Shameer Kolothum <[email protected]> > > > > Signed-off-by: Yi Liu <[email protected]> > > > > --- > > > > drivers/vfio/device_cdev.c | 66 ++++++++++++++++++++++++++++++++++++++ > > > > drivers/vfio/iommufd.c | 18 +++++++++++ > > > > drivers/vfio/vfio.h | 18 +++++++++++ > > > > drivers/vfio/vfio_main.c | 8 +++++ > > > > include/uapi/linux/vfio.h | 52 ++++++++++++++++++++++++++++++ > > > > 5 files changed, 162 insertions(+) > > > > > > > > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c > > > > index 291cc678a18b..3f14edb80a93 100644 > > > > --- a/drivers/vfio/device_cdev.c > > > > +++ b/drivers/vfio/device_cdev.c > > > > @@ -174,6 +174,72 @@ long vfio_device_ioctl_bind_iommufd(struct > vfio_device_file > > > *df, > > > > return ret; > > > > } > > > > > > > > +int vfio_ioctl_device_attach(struct vfio_device_file *df, > > > > + struct vfio_device_attach_iommufd_pt > > > > __user *arg) > > > > +{ > > > > + 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, arg, minsz)) > > > > + return -EFAULT; > > > > + > > > > + if (attach.argsz < minsz || attach.flags) > > > > + return -EINVAL; > > > > + > > > > + /* ATTACH only allowed for cdev fds */ > > > > + if (df->group) > > > > + return -EINVAL; > > > > + > > > > + mutex_lock(&device->dev_set->lock); > > > > + ret = vfio_iommufd_attach(device, &attach.pt_id); > > > > + if (ret) > > > > + goto out_unlock; > > > > + > > > > + ret = copy_to_user(&arg->pt_id, &attach.pt_id, > > > > + sizeof(attach.pt_id)) ? -EFAULT : 0; > > > > + if (ret) > > > > + goto out_detach; > > > > + mutex_unlock(&device->dev_set->lock); > > > > + > > > > + return 0; > > > > + > > > > +out_detach: > > > > + vfio_iommufd_detach(device); > > > > +out_unlock: > > > > + mutex_unlock(&device->dev_set->lock); > > > > + return ret; > > > > +} > > > > + > > > > +int vfio_ioctl_device_detach(struct vfio_device_file *df, > > > > + struct vfio_device_detach_iommufd_pt > > > > __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, arg, minsz)) > > > > + return -EFAULT; > > > > + > > > > + if (detach.argsz < minsz || detach.flags) > > > > + return -EINVAL; > > > > + > > > > + /* DETACH only allowed for cdev fds */ > > > > + if (df->group) > > > > + return -EINVAL; > > > > + > > > > + mutex_lock(&device->dev_set->lock); > > > > + vfio_iommufd_detach(device); > > > > + mutex_unlock(&device->dev_set->lock); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static char *vfio_device_devnode(const struct device *dev, umode_t > > > > *mode) > > > > { > > > > return kasprintf(GFP_KERNEL, "vfio/devices/%s", dev_name(dev)); > > > > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c > > > > index 83575b65ea01..799ea322a7d4 100644 > > > > --- a/drivers/vfio/iommufd.c > > > > +++ b/drivers/vfio/iommufd.c > > > > @@ -112,6 +112,24 @@ void vfio_iommufd_unbind(struct vfio_device_file > > > > *df) > > > > vdev->ops->unbind_iommufd(vdev); > > > > } > > > > > > > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id) > > > > +{ > > > > + lockdep_assert_held(&vdev->dev_set->lock); > > > > + > > > > + if (vfio_device_is_noiommu(vdev)) > > > > + return 0; > > > > > > Isn't this an invalid operation for a noiommu cdev, ie. -EINVAL? We > > > return success and copy back the provided pt_id, why would a user not > > > consider it a bug that they can't use whatever value was there with > > > iommufd? > > > > Yes, this is the question I asked in [1]. At that time, it appears to me > > that better to allow it [2]. Maybe it's more suitable to ask it here. > > From an API perspective it seems wrong. We return success without > doing anything. A user would be right to consider it a bug that the > attach operation works but there's not actually any association to the > IOAS. Thanks,
The current version is kind of tradeoff based on prior remarks when I asked the question. As prior comment[2], it appears to me the attach shall success for noiommu devices as well, but per your remark it seems not in plan. So anyway, we may just fail the attach/detach for noiommu devices. Is it? btw. Should we document it somewhere as well? E.g. noiommu userspace does not support attach/detach? Userspace should know it is opening noiommu devices. Regards, Yi Liu
