On Wed, Nov 16, 2022 at 04:31:33PM -0700, Alex Williamson wrote:
> > 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,
Yes, I've carried this invariant forward through a few series now
> but I don't really see how this bit of paranoia is ever a
> possibility.
Right, it is an invariant assertion, it should never trigger and we've
never seen it trigger. I think at one point it was harder to see that
this is impossible so an assertion must have been added
> WARN_ON(group->container ^ group->container_users);
Ah, this needs a "logical xor" which is a bit obscure. In C I guess
this is the common way to do it:
/*
* There can only be users if there is a container, and if there is a
* container there must be users.
*/
WARN_ON(!group->container != !group->container_users);
I'm also happy to delete it, not sure it is a valuable invariant.
> > @@ -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.
Yes, it was missed, I fixed it
Thanks,
Jason
