Re: [Intel-gfx] [PATCH 06/10] vfio-iommufd: Allow iommufd to be used in place of a container fd

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Nov 01, 2022 at 08:09:52AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 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