Re: [Intel-gfx] [PATCH v5 17/19] vfio: Add VFIO_DEVICE_AT[DE]TACH_IOMMUFD_PT

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Feb 28, 2023 at 02:01:36PM +0000, Liu, Yi L wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, February 28, 2023 9:44 PM
> > 
> > On Tue, Feb 28, 2023 at 01:36:24PM +0000, Liu, Yi L wrote:
> > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > Sent: Tuesday, February 28, 2023 9:26 PM
> > > >
> > > > On Tue, Feb 28, 2023 at 01:22:50PM +0000, Liu, Yi L wrote:
> > > >
> > > > > > A null iommufd pointer and a bound df flag is sufficient to see that
> > > > > > it is compat mode.
> > > > >
> > > > > Hope df->is_cdev_device suits your expectation.:-) The code will look
> > > > > like below:
> > > >
> > > > Yes, this is better.. However I'd suggest 'uses_container' as it is
> > > > clearer what the special case is
> > >
> > > Surely doable. Need to add a helper like below:
> > >
> > > bool vfio_device_group_uses_container()
> > > {
> > > 	lockdep_assert_held(&device->group->group_lock);
> > > 	return device->group->container;
> > > }
> > 
> > It should come from the df.
> > 
> > If you have a df then by definition:
> >   smp_load_acquire(..) == false     - Not bound
> >   df->device->iommufd_ctx != NULL   - Using iommufd
> >   df->group->containter != NULL     - Using legacy container
> >   all other cases                   - NO_IOMMU
> > 
> > No locking required since all these cases after the smp_load_acquire
> > must be fixed for the lifetime of the df.
> 
> Do you mean the df->access_granted (introduced in patch 07) or a new
> flag?

yes

> Following your suggestion, it seems a mandatory requirement to do the
> smp_load_acquire(..) == false check first, and then call into the vfio_device_open()
> which further calls vfio_device_first_open() to check the iommufd/
> legacy container/noiommu stuffs. Is it?

Figuring out if an open should happen or not is a different operation,
you already build exclusion between cdev/group so we don't need to
care about the open path. 

> df->group->containter this may need a helper to avoid decoding group
> field. May be just store container in df?

At worst a flag, but a helper seems like a good idea anyhow, then it
can be compiled out

Jason