> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Wednesday, March 8, 2023 9:29 PM
>
> @@ -177,7 +177,7 @@ static int vfio_device_group_open(struct
> vfio_device_file *df)
> mutex_lock(&device->group->group_lock);
> if (!vfio_group_has_iommu(device->group)) {
> ret = -EINVAL;
> - goto out_unlock;
> + goto err_unlock;
> }
My impression - out_xxx means go to do xxx while err_xxx means
go to do something for error xxx, though in many places the two
are mixed to both meaning 'do xxx'.
either way I don't see a need of changing it.
> -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
> +static int vfio_iommufd_device_probe_comapt_noiommu(struct vfio_device
> *vdev,
> + struct iommufd_ctx *ictx)
s/comapt/compat/
btw it's clearer to move this check into vfio_device_group_open().
if noiommu then pass NULL to vfio_device_open(), same as the cdev path.
> +
> +int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
> +{
> u32 device_id;
> int ret;
>
> lockdep_assert_held(&vdev->dev_set->lock);
>
> if (vfio_device_is_noiommu(vdev)) {
> - if (!capable(CAP_SYS_RAWIO))
> - return -EPERM;
> -
> - /*
> - * Require no compat ioas to be assigned to proceed. The
> basic
> - * statement is that the user cannot have done something
> that
> - * implies they expected translation to exist
> - */
> - if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
> - return -EPERM;
> - return 0;
> + ret = vfio_iommufd_device_probe_comapt_noiommu(vdev,
> ictx);
> + if (ret)
> + return ret;
> }
>
> if (WARN_ON(!vdev->ops->bind_iommufd))
> return -ENODEV;
>
> - ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id);
> - if (ret)
> - return ret;
> + /* The legacy path has no way to return the device id */
> + return vdev->ops->bind_iommufd(vdev, ictx, &device_id);
> +}
>
> - ret = iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id);
> - if (ret)
> - goto err_unbind;
> - ret = vdev->ops->attach_ioas(vdev, &ioas_id);
> - if (ret)
> - goto err_unbind;
after noiommu check and attach_ioas are moved out then this
entire function can be removed now. Just call the ops in
vfio_device_first_open().
> +int vfio_iommufd_attach_compat_ioas(struct vfio_device *vdev,
> + struct iommufd_ctx *ictx)
> +{
> + u32 ioas_id;
> + int ret;
> +
> + lockdep_assert_held(&vdev->dev_set->lock);
>
> /*
> - * The legacy path has no way to return the device id or the selected
> - * pt_id
> + * If the driver doesn't provide this op then it means the device does
> + * not do DMA at all. So nothing to do.
> */
> - return 0;
> + if (WARN_ON(!vdev->ops->bind_iommufd))
> + return -ENODEV;
>
> -err_unbind:
> - if (vdev->ops->unbind_iommufd)
> - vdev->ops->unbind_iommufd(vdev);
> - return ret;
> + if (vfio_device_is_noiommu(vdev)) {
> + if
> (WARN_ON(vfio_iommufd_device_probe_comapt_noiommu(vdev, ictx)))
> + return -EINVAL;
> + return 0;
> + }
no need. let's directly call following from vfio_device_group_open().
In that case no need to do noiommu check twice in one function.
> +
> + ret = iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id);
> + if (ret)
> + return ret;
> +
> + /* The legacy path has no way to return the selected pt_id */
> + return vdev->ops->attach_ioas(vdev, &ioas_id);
> }
>
