Re: [PATCH 07/10] vfio: Refactor vfio_device open and close

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Dec 02, 2022 at 02:37:18PM +0800, Yi Liu wrote:
> On 2022/12/2 14:15, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Thursday, December 1, 2022 10:56 PM
> > > 
> > > This refactor makes the vfio_device_open() to accept device, iommufd_ctx
> > > pointer and kvm pointer. These parameters are generic items in today's
> > > group path and furute device cdev path. Caller of vfio_device_open() should
> > > take care the necessary protections. e.g. the current group path need to
> > > hold the group_lock to ensure the iommufd_ctx and kvm pointer are valid.
> > > 
> > > This refactor also wraps the group spefcific codes in the device open and
> > > close paths to be paired helpers like:
> > > 
> > > - vfio_device_group_open/close(): call vfio_device_open/close()
> > > - vfio_device_group_use/unuse_iommu(): call iommufd or container
> > > use/unuse
> > 
> > this pair is container specific. iommufd vs. container is selected
> > in vfio_device_first_open().
> 
> got it.
> 
> > > @@ -765,77 +796,56 @@ static int vfio_device_first_open(struct vfio_device
> > > *device)
> > >   	if (!try_module_get(device->dev->driver->owner))
> > >   		return -ENODEV;
> > > 
> > > -	/*
> > > -	 * Here we pass the KVM pointer with the group under the lock.  If
> > > the
> > > -	 * device driver will use it, it must obtain a reference and release it
> > > -	 * during close_device.
> > > -	 */
> > > -	mutex_lock(&device->group->group_lock);
> > > -	if (!vfio_group_has_iommu(device->group)) {
> > > -		ret = -EINVAL;
> > > +	if (iommufd)
> > > +		ret = vfio_iommufd_bind(device, iommufd);
> > 
> > This probably should be renamed to vfio_device_iommufd_bind().
> 
> I'd like to see if Jason wants to modify it or not as it is
> introduced in vfio compat series.
> 
> https://lore.kernel.org/kvm/6-v4-42cd2eb0e3eb+335a-vfio_iommufd_jgg@nvidia.com/

Ah, it is a bit late, if we want to change it please make a patch

> > 
> > > +	else
> > > +		ret = vfio_device_group_use_iommu(device);
> > 
> > what about vfio_device_container_bind()?
> 
> maybe use_iommu seems suit more. bind is more likely a kind of
> associating something with something. but this helper is more kind
> of using the container. so I chose use_iommu. But I see the value
> of using bind, it would make the two branches aligned.

I prefer use iommu, "bind" is so vauge

Jason