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, Alex > [1] > https://lore.kernel.org/kvm/[email protected]/ > [2] https://lore.kernel.org/kvm/zffuyhqid+ltub%[email protected]/ > > > > + > > > + return vdev->ops->attach_ioas(vdev, pt_id); > > > +} > > > + > > > +void vfio_iommufd_detach(struct vfio_device *vdev) > > > +{ > > > + lockdep_assert_held(&vdev->dev_set->lock); > > > + > > > + if (!vfio_device_is_noiommu(vdev)) > > > + vdev->ops->detach_ioas(vdev); > > > +} > > > + > > > struct iommufd_ctx *vfio_iommufd_physical_ictx(struct vfio_device *vdev) > > > { > > > if (vdev->iommufd_device) > > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h > > > index 8b359a7794be..50553f67600f 100644 > > > --- a/drivers/vfio/vfio.h > > > +++ b/drivers/vfio/vfio.h > > > @@ -241,6 +241,8 @@ int vfio_iommufd_bind(struct vfio_device_file *df); > > > void vfio_iommufd_unbind(struct vfio_device_file *df); > > > int vfio_iommufd_compat_attach_ioas(struct vfio_device *device, > > > struct iommufd_ctx *ictx); > > > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id); > > > +void vfio_iommufd_detach(struct vfio_device *vdev); > > > #else > > > static inline int > > > vfio_iommufd_compat_probe_noiommu(struct vfio_device *device, > > > @@ -282,6 +284,10 @@ int vfio_device_fops_cdev_open(struct inode *inode, > > > struct > > file *filep); > > > void vfio_device_cdev_close(struct vfio_device_file *df); > > > long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df, > > > struct vfio_device_bind_iommufd __user > > > *arg); > > > +int vfio_ioctl_device_attach(struct vfio_device_file *df, > > > + struct vfio_device_attach_iommufd_pt __user *arg); > > > +int vfio_ioctl_device_detach(struct vfio_device_file *df, > > > + struct vfio_device_detach_iommufd_pt __user *arg); > > > int vfio_cdev_init(struct class *device_class); > > > void vfio_cdev_cleanup(void); > > > #else > > > @@ -315,6 +321,18 @@ static inline long > > > vfio_device_ioctl_bind_iommufd(struct > > vfio_device_file *df, > > > return -EOPNOTSUPP; > > > } > > > > > > +static inline int vfio_ioctl_device_attach(struct vfio_device_file *df, > > > + struct vfio_device_attach_iommufd_pt > > > __user > > *arg) > > > +{ > > > + return -EOPNOTSUPP; > > > +} > > > + > > > +static inline int vfio_ioctl_device_detach(struct vfio_device_file *df, > > > + struct vfio_device_detach_iommufd_pt > > > > > __user *arg) > > > +{ > > > + return -EOPNOTSUPP; > > > +} > > > + > > > static inline int vfio_cdev_init(struct class *device_class) > > > { > > > return 0; > > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > > > index c9fa39ac4b02..8c3f26b4929b 100644 > > > --- a/drivers/vfio/vfio_main.c > > > +++ b/drivers/vfio/vfio_main.c > > > @@ -1165,6 +1165,14 @@ static long vfio_device_fops_unl_ioctl(struct file > > > *filep, > > > ret = vfio_ioctl_device_feature(device, (void __user *)arg); > > > break; > > > > > > + case VFIO_DEVICE_ATTACH_IOMMUFD_PT: > > > + ret = vfio_ioctl_device_attach(df, (void __user *)arg); > > > + break; > > > + > > > + case VFIO_DEVICE_DETACH_IOMMUFD_PT: > > > + ret = vfio_ioctl_device_detach(df, (void __user *)arg); > > > + break; > > > + > > > default: > > > if (unlikely(!device->ops->ioctl)) > > > ret = -EINVAL; > > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > > > index 07c917de31e9..770f5f949929 100644 > > > --- a/include/uapi/linux/vfio.h > > > +++ b/include/uapi/linux/vfio.h > > > @@ -222,6 +222,58 @@ struct vfio_device_bind_iommufd { > > > > > > #define VFIO_DEVICE_BIND_IOMMUFD _IO(VFIO_TYPE, VFIO_BASE + 19) > > > > > > +/* > > > + * VFIO_DEVICE_ATTACH_IOMMUFD_PT - _IOW(VFIO_TYPE, VFIO_BASE + 20, > > > + * struct > > > vfio_device_attach_iommufd_pt) > > > + * > > > + * Attach a vfio device to an iommufd address space specified by IOAS > > > + * id or hw_pagetable (hwpt) id. > > > + * > > > + * Available only after a device has been bound to iommufd via > > > + * VFIO_DEVICE_BIND_IOMMUFD > > > + * > > > + * Undo by VFIO_DEVICE_DETACH_IOMMUFD_PT or device fd close. > > > + * > > > + * @argsz: User filled size of this data. > > > + * @flags: Must be 0. > > > + * @pt_id: Input the target id which can represent an ioas or a > > > hwpt > > > + * allocated via iommufd subsystem. > > > + * Output the input ioas id or the attached hwpt id which > > > could > > > + * be the specified hwpt itself or a hwpt automatically > > > created > > > + * for the specified ioas by kernel during the attachment. > > > + * > > > + * Return: 0 on success, -errno on failure. > > > + */ > > > +struct vfio_device_attach_iommufd_pt { > > > + __u32 argsz; > > > + __u32 flags; > > > + __u32 pt_id; > > > +}; > > > + > > > +#define VFIO_DEVICE_ATTACH_IOMMUFD_PT _IO(VFIO_TYPE, > > VFIO_BASE + 20) > > > + > > > +/* > > > + * VFIO_DEVICE_DETACH_IOMMUFD_PT - _IOW(VFIO_TYPE, VFIO_BASE + 21, > > > + * struct > > > vfio_device_detach_iommufd_pt) > > > + * > > > + * Detach a vfio device from the iommufd address space it has been > > > + * attached to. After it, device should be in a blocking DMA state. > > > + * > > > + * Available only after a device has been bound to iommufd via > > > + * VFIO_DEVICE_BIND_IOMMUFD. > > > > These "[a]vailable only after" comments are meaningless, if the user > > has the file descriptor the ioctl is available. We can say that ATTACH > > should be used after BIND to associate the device with an address space > > within the bound iommufd and DETACH removes that association, but the > > user is welcome to call everything in the wrong order and we need to be > > prepared for that anyway. Thanks, > > Oh, yes. it's available as long as FD is got. But it is expected to fail if > the order is not met. This should be what the comment really wants > to deliver. Will have a look at other ioctls as well. > > Regards, > Yi Liu > > > > > Alex > > > > > + * > > > + * @argsz: User filled size of this data. > > > + * @flags: Must be 0. > > > + * > > > + * Return: 0 on success, -errno on failure. > > > + */ > > > +struct vfio_device_detach_iommufd_pt { > > > + __u32 argsz; > > > + __u32 flags; > > > +}; > > > + > > > +#define VFIO_DEVICE_DETACH_IOMMUFD_PT _IO(VFIO_TYPE, > > VFIO_BASE + 21) > > > + > > > /** > > > * VFIO_DEVICE_GET_INFO - _IOR(VFIO_TYPE, VFIO_BASE + 7, > > > * struct > > > vfio_device_info) >
