> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Tuesday, May 23, 2023 6:16 AM
>
> On Sat, 13 May 2023 06:28:24 -0700
> Yi Liu <yi.l.liu@xxxxxxxxx> 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 <yanting.jiang@xxxxxxxxx>
> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx>
> > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> > ---
> > 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.
[1] https://lore.kernel.org/kvm/c203f11f-4d9f-cf43-03ab-e41a858bdd92@xxxxxxxxx/
[2] https://lore.kernel.org/kvm/ZFFUyhqID+LtUB%2FD@xxxxxxxxxx/
> > +
> > + 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)
