> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Tuesday, January 17, 2023 9:50 PM
>
> This adds two vfio device ioctls for userspace using iommufd on vfio
> devices.
>
> VFIO_DEVICE_BIND_IOMMUFD: bind device to an iommufd, hence gain
> DMA
> control provided by the iommufd. VFIO no
> iommu is indicated by passing a minus
> fd value.
Can't this be a flag bit for better readability than using a special value?
> VFIO_DEVICE_ATTACH_IOMMUFD_PT: attach device to ioas, page tables
> managed by iommufd. Attach can be
> undo by passing IOMMUFD_INVALID_ID
> to kernel.
With Alex' remark we need a separate DETACH cmd now.
>
> + /*
> + * For group path, iommufd pointer is NULL when comes into this
> + * helper. Its noiommu support is in container.c.
> + *
> + * For iommufd compat mode, iommufd pointer here is a valid value.
> + * Its noiommu support is supposed to be in vfio_iommufd_bind().
> + *
> + * For device cdev path, iommufd pointer here is a valid value for
> + * normal cases, but it is NULL if it's noiommu. The reason is
> + * that userspace uses iommufd==-1 to indicate noiommu mode in
> this
> + * path. So caller of this helper will pass in a NULL iommufd
> + * pointer. To differentiate it from the group path which also
> + * passes NULL iommufd pointer in, df->noiommu is used. For cdev
> + * noiommu, df->noiommu would be set to mark noiommu case for
> cdev
> + * path.
> + *
> + * So if df->noiommu is set then this helper just goes ahead to
> + * open device. If not, it depends on if iommufd pointer is NULL
> + * to handle the group path, iommufd compat mode, normal cases in
> + * the cdev path.
> + */
> if (iommufd)
> ret = vfio_iommufd_bind(device, iommufd, dev_id, pt_id);
> - else
> + else if (!df->noiommu)
> ret = vfio_device_group_use_iommu(device);
> if (ret)
> goto err_module_put;
Isn't 'ret' uninitialized when df->noiommu is true?
> +static int vfio_ioctl_device_attach(struct vfio_device *device,
> + struct vfio_device_feature __user *arg)
> +{
> + struct vfio_device_attach_iommufd_pt attach;
> + int ret;
> + bool is_attach;
> +
> + if (copy_from_user(&attach, (void __user *)arg, sizeof(attach)))
> + return -EFAULT;
> +
> + if (attach.flags)
> + return -EINVAL;
> +
> + if (!device->ops->bind_iommufd)
> + return -ENODEV;
> +
this should fail if noiommu is true.
