> From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> Sent: Friday, March 10, 2023 4:08 PM
>
> > 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.
Ok. I'm fine with either way.
> > -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.
Right.
> > +
> > +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().
Yes. and also no vfio_iommufd_unbind().
>
> > +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.
Ok. maybe still have vfio_iommufd_attach_compat_ioas() but
only call it if it's not noiommu mode. vfio_device_group_open()
can call probe_noiommu() first and has a bool to mark noiommu.
Jason had a remark that it's better to keep the
iommufd_vfio_compat_ioas_get_id() in iommufd.c
>
> > +
> > + 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);
> > }
> >
