On Tue, Nov 01, 2022 at 08:09:52AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Sent: Wednesday, October 26, 2022 2:51 AM
> >
> > menuconfig VFIO
> > tristate "VFIO Non-Privileged userspace driver framework"
> > select IOMMU_API
> > + depends on IOMMUFD || !IOMMUFD
>
> Out of curiosity. What is the meaning of this dependency claim?
>
> > @@ -717,12 +735,23 @@ static int vfio_group_ioctl_set_container(struct
> > vfio_group *group,
> > }
> >
> > container = vfio_container_from_file(f.file);
> > - ret = -EINVAL;
>
> this changes the errno from -EINVAL to -EBADF for the original container
> path. Is it desired?
Yes, EBADFD is the right error code (it is a typo it was EBADF)
> > if (container) {
> > ret = vfio_container_attach_group(container, group);
> > goto out_unlock;
> > }
> >
> > + iommufd = iommufd_ctx_from_file(f.file);
> > + if (!IS_ERR(iommufd)) {
>
> The only errno which iommufd_ctx_from_file() may return is -EBADFD
> which duplicates with -EBADF assignment in following line.
The concept is that other places using iommufd_ctx_from_file() should
forward the return code directly. vfio is probably the only thing that
is going to be multiplexing like this.
> > + u32 ioas_id;
> > +
> > + group->iommufd = iommufd;
> > + ret = iommufd_vfio_compat_ioas_id(iommufd, &ioas_id);
>
> exchange the order of above two lines and only assign group->iommufd
> when the compat call succeeds.
Yeah, that is probably a small bug:
- group->iommufd = iommufd;
ret = iommufd_vfio_compat_ioas_id(iommufd, &ioas_id);
+ if (ret) {
+ iommufd_ctx_put(group->iommufd);
+ goto out_unlock;
+ }
+
+ group->iommufd = iommufd;
goto out_unlock;
> > @@ -900,7 +940,7 @@ static int vfio_group_ioctl_get_status(struct
> > vfio_group *group,
> > return -ENODEV;
> > }
> >
> > - if (group->container)
> > + if (group->container || group->iommufd)
> > status.flags |= VFIO_GROUP_FLAGS_CONTAINER_SET |
> > VFIO_GROUP_FLAGS_VIABLE;
>
> Copy some explanation from commit msg to explain the subtle difference
> between container and iommufd.
/*
* With the container FD the iommu_group_claim_dma_owner() is done
* during SET_CONTAINER but for IOMMFD this is done during
* VFIO_GROUP_GET_DEVICE_FD. Meaning that with iommufd
* VFIO_GROUP_FLAGS_VIABLE could be set but GET_DEVICE_FD will fail due
* to viability.
*/
Thanks,
Jason
