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

Tian, Kevin
2022-11-01T08:10:05Z

> 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?

> 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.

What about having it return NULL pointer similar as the container
helper does?

> + 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.

> @@ -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.