> From: Tian, Kevin <kevin.t...@intel.com> > Sent: Friday, March 10, 2023 4:08 PM > > > From: Liu, Yi L <yi.l....@intel.com> > > 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); > > } > >