[PATCH v5 17/19] vfio: Add VFIO_DEVICE_AT[DE]TACH_IOMMUFD_PT

[Liu, Yi L]

> From: Jason Gunthorpe <jgg at nvidia.com>
> Sent: Thursday, March 2, 2023 1:49 AM
> 
> On Wed, Mar 01, 2023 at 02:04:00PM +0000, Liu, Yi L wrote:
> > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > index 2a13442add43..ed3ffe7ceb3f 100644
> > --- a/drivers/vfio/group.c
> > +++ b/drivers/vfio/group.c
> > @@ -777,6 +777,11 @@ void vfio_device_group_unregister(struct
> vfio_device *device)
> >  	mutex_unlock(&device->group->device_lock);
> >  }
> >
> > +bool vfio_device_group_uses_container(struct vfio_device *device)
> > +{
> > +	return READ_ONCE(device->group->container);
> > +}
> 
> As I said this should take in the vfio_device_file because as long as
> a vfio_device_file exists then group->contianer is required to be stable.

Ok, let me store vfio_group in vfio_devcie_file instead of reach
it by df->device->group.

btw. With vfio_group stored in vfio_device_file, it looks like
the is_cdev_device flag (introduced in patch 14) is not necessary
now, we can always define the group pointer in vfio_device_file
even group code is compiled out, then we can use this group
pointer to check if the vfio_device_file is used in the group path
or the cdev path. Is it?

> > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > index 121a75fadceb..4b5b17e8aaa1 100644
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -422,9 +422,22 @@ static int vfio_device_first_open(struct
> vfio_device_file *df)
> >  	if (!try_module_get(device->dev->driver->owner))
> >  		return -ENODEV;
> >
> > +	/*
> > +	 * The handling here depends on what the user is using.
> > +	 *
> > +	 * If user uses iommufd in the group compat mode or the
> > +	 * cdev path, call vfio_iommufd_bind().
> > +	 *
> > +	 * If user uses container in the group legacy mode, call
> > +	 * vfio_device_group_use_iommu().
> > +	 *
> > +	 * If user doesn't use iommufd nor container, this is
> > +	 * the noiommufd mode in the cdev path, nothing needs
> > +	 * to be done here just go ahead to open device.
> > +	 */
> >  	if (iommufd)
> >  		ret = vfio_iommufd_bind(device, iommufd);
> > -	else
> > +	else if (vfio_device_group_uses_container(device))
> >  		ret = vfio_device_group_use_iommu(device);
> 
> But yes, this makes alot more sense..
> 
> Jason

Regards,
Yi Liu