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
> +{
> + 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
> + 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
> + &attach.pt_id,
> + sizeof(attach.pt_id)) ? -EFAULT : 0;
Also:
static_assert(__same_type(arg->flags), attach.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
> +
> + 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?
Jason
