On Wed, 16 Nov 2022 17:05:31 -0400
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> This makes VFIO_GROUP_SET_CONTAINER accept both a vfio container FD and an
> iommufd.
>
> In iommufd mode an IOAS will exist after the SET_CONTAINER, but it will
> not be attached to any groups.
>
> For VFIO this means that the VFIO_GROUP_GET_STATUS and
> VFIO_GROUP_FLAGS_VIABLE works subtly differently. With the container FD
> the iommu_group_claim_dma_owner() is done during SET_CONTAINER but for
> IOMMUFD this is done during VFIO_GROUP_GET_DEVICE_FD. Meaning that
> VFIO_GROUP_FLAGS_VIABLE could be set but GET_DEVICE_FD will fail due to
> viability.
>
> As GET_DEVICE_FD can fail for many reasons already this is not expected to
> be a meaningful difference.
>
> Reorganize the tests for if the group has an assigned container or iommu
> into a vfio_group_has_iommu() function and consolidate all the duplicated
> WARN_ON's etc related to this.
>
> Call container functions only if a container is actually present on the
> group.
>
> Tested-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> Tested-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> Tested-by: Lixiao Yang <lixiao.yang@xxxxxxxxx>
> Tested-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> Tested-by: Yu He <yu.he@xxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
> drivers/vfio/Kconfig | 1 +
> drivers/vfio/container.c | 7 +++-
> drivers/vfio/vfio.h | 2 +
> drivers/vfio/vfio_main.c | 86 +++++++++++++++++++++++++++++++++-------
> 4 files changed, 80 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index 86c381ceb9a1e9..1118d322eec97d 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -2,6 +2,7 @@
> menuconfig VFIO
> tristate "VFIO Non-Privileged userspace driver framework"
> select IOMMU_API
> + depends on IOMMUFD || !IOMMUFD
> select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64)
> select INTERVAL_TREE
> help
> diff --git a/drivers/vfio/container.c b/drivers/vfio/container.c
> index d97747dfb05d02..8772dad6808539 100644
> --- a/drivers/vfio/container.c
> +++ b/drivers/vfio/container.c
> @@ -516,8 +516,11 @@ int vfio_group_use_container(struct vfio_group *group)
> {
> lockdep_assert_held(&group->group_lock);
>
> - if (!group->container || !group->container->iommu_driver ||
> - WARN_ON(!group->container_users))
> + /*
> + * The container fd has been assigned with VFIO_GROUP_SET_CONTAINER but
> + * VFIO_SET_IOMMU hasn't been done yet.
> + */
> + if (!group->container->iommu_driver)
> return -EINVAL;
>
> if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 247590334e14b0..985e13d52989ca 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -10,6 +10,7 @@
> #include <linux/cdev.h>
> #include <linux/module.h>
>
> +struct iommufd_ctx;
> struct iommu_group;
> struct vfio_device;
> struct vfio_container;
> @@ -60,6 +61,7 @@ struct vfio_group {
> struct kvm *kvm;
> struct file *opened_file;
> struct blocking_notifier_head notifier;
> + struct iommufd_ctx *iommufd;
> };
>
> /* events for the backend driver notify callback */
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 5c0e810f8b4d08..8c124290ce9f0d 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -35,6 +35,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/interval_tree.h>
> #include <linux/iova_bitmap.h>
> +#include <linux/iommufd.h>
> #include "vfio.h"
>
> #define DRIVER_VERSION "0.3"
> @@ -665,6 +666,16 @@ EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
> /*
> * VFIO Group fd, /dev/vfio/$GROUP
> */
> +static bool vfio_group_has_iommu(struct vfio_group *group)
> +{
> + lockdep_assert_held(&group->group_lock);
> + if (!group->container)
> + WARN_ON(group->container_users);
> + else
> + WARN_ON(!group->container_users);
I think this is just carrying forward the WARN_ON that gets replaced in
set_container, but I don't really see how this bit of paranoia is ever
a possibility. If it is, a comment would be good, and perhaps simplify
to:
WARN_ON(group->container ^ group->container_users);
> + return group->container || group->iommufd;
> +}
> +
> /*
> * VFIO_GROUP_UNSET_CONTAINER should fail if there are other users or
> * if there was no container to unset. Since the ioctl is called on
[snip]
> @@ -900,7 +945,14 @@ static int vfio_group_ioctl_get_status(struct vfio_group *group,
> return -ENODEV;
> }
>
> - if (group->container)
> + /*
> + * 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.
> + */
> + if (group->container || group->iommufd)
Why didn't this use the vfio_group_has_iommu() helper? This is only
skipping the paranoia checks, which aren't currently obvious to me
anyway. Thanks,
Alex
> status.flags |= VFIO_GROUP_FLAGS_CONTAINER_SET |
> VFIO_GROUP_FLAGS_VIABLE;
> else if (!iommu_group_dma_owner_claimed(group->iommu_group))
