On Fri, Jun 02, 2023 at 05:16:48AM -0700, 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.
>
> noiommu devices do not support [AT|DE]TACH, if user invokes the two ioctls
> on such devices, shall fail.
Stale comment
> Tested-by: Yanting Jiang <yanting.ji...@intel.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.th...@huawei.com>
> Tested-by: Terrence Xu <terrence...@intel.com>
> Signed-off-by: Yi Liu <yi.l....@intel.com>
> ---
> drivers/vfio/device_cdev.c | 66 ++++++++++++++++++++++++++++++++++++++
> drivers/vfio/vfio.h | 16 +++++++++
> drivers/vfio/vfio_main.c | 8 +++++
> include/uapi/linux/vfio.h | 42 ++++++++++++++++++++++++
> 4 files changed, 132 insertions(+)
>
> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> index a4498ddbe774..6e1d499ee160 100644
> --- a/drivers/vfio/device_cdev.c
> +++ b/drivers/vfio/device_cdev.c
> @@ -167,6 +167,72 @@ long vfio_df_ioctl_bind_iommufd(struct vfio_device_file
> *df,
> return ret;
> }
>
> +int vfio_df_ioctl_attach_pt(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;
I feel like vfio_device_fops_unl_ioctl() should do these group tests
for the whole lot
@@ -1187,19 +1187,24 @@ static long vfio_device_fops_unl_ioctl(struct file
*filep,
if (ret)
return ret;
+ /* cdev only ioctls */
+ if (IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV) && !df->group) {
+ switch (cmd) {
+ case VFIO_DEVICE_ATTACH_IOMMUFD_PT:
+ ret = vfio_df_ioctl_attach_pt(df, (void __user *)arg);
+ goto out;
+
+ case VFIO_DEVICE_DETACH_IOMMUFD_PT:
+ ret = vfio_df_ioctl_detach_pt(df, (void __user *)arg);
+ goto out;
+ }
+ }
+
switch (cmd) {
case VFIO_DEVICE_FEATURE:
ret = vfio_ioctl_device_feature(device, (void __user *)arg);
break;
- case VFIO_DEVICE_ATTACH_IOMMUFD_PT:
- ret = vfio_df_ioctl_attach_pt(df, (void __user *)arg);
- break;
-
- case VFIO_DEVICE_DETACH_IOMMUFD_PT:
- ret = vfio_df_ioctl_detach_pt(df, (void __user *)arg);
- break;
-
default:
if (unlikely(!device->ops->ioctl))
ret = -EINVAL;
And also make a local var for void __user * to avoid the repeated
casts.
Also this construction avoids the stub static inlines since the
IS_ENABLED will compile out the call. Just use a normal function
prototype outside any ifdef.
> +
> + mutex_lock(&device->dev_set->lock);
> + ret = device->ops->attach_ioas(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;
Don't use the ?:
if (copy_to_user()..) {
ret = -EFAULT;
goto out_detach;
}
Jason
