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

[Jason Gunthorpe <jgg@nvidia.com>]

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