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

[Liu, Yi L]

> From: Jason Gunthorpe <jgg at nvidia.com>
> Sent: Tuesday, February 28, 2023 8:54 PM
> 
> On Tue, Feb 28, 2023 at 12:42:31PM +0000, Liu, Yi L wrote:
> > > From: Jason Gunthorpe <jgg at nvidia.com>
> > > Sent: Tuesday, February 28, 2023 8:32 PM
> > >
> > > On Tue, Feb 28, 2023 at 02:51:28AM +0000, Liu, Yi L wrote:
> > > > > This seems strange. no iommu mode should have a NULL dev-
> > > >iommufctx.
> > > > > Why do we have a df->noiommu at all?
> > > >
> > > > This is due to the vfio_device_first_open(). Detail as below comment
> (part
> > > of
> > > > patch 0016).
> > > >
> > > > +	/*
> > > > +	 * For group/container path, iommufd pointer is NULL when comes
> > > > +	 * into this helper. Its noiommu support is handled by
> > > > +	 * vfio_device_group_use_iommu()
> > > > +	 *
> > > > +	 * For iommufd compat mode, iommufd pointer here is a valid value.
> > > > +	 * Its noiommu support is in vfio_iommufd_bind().
> > > > +	 *
> > > > +	 * For device cdev path, iommufd pointer here is a valid value for
> > > > +	 * normal cases, but it is NULL if it's noiommu. Check df->noiommu
> > > > +	 * to differentiate cdev noiommu from the group/container path
> > > which
> > > > +	 * also passes NULL iommufd pointer in. If set then do nothing.
> > > > +	 */
> > >
> > > If the group is in iommufd mode then it should set this pointer too.
> >
> > Yes, but the key point is that both the group in legacy mode and the
> > cdev path sets iommufd==NULL. And the handling for the two should
> > be different. So needs this extra info to differentiate them in
> > vfio_device_first_open().
> 
> Don't encode that in the iommufd pointer, it is confusing.

Maybe I failed to make it clear. As the below code, When
iommufd==!NULL, no need to differentiate whether it is
the group compat mode or the cdev path. But if iommufd==NULL,
it may be the legacy group code or the cdev noiommu mode. So
df->noiommu is added. But I agree this noiommu flag is confusing.
May use the df->is_cdev_device flag as the purpose here is to
differentiate cdev path and group path.

	if (iommufd)
		ret = vfio_iommufd_bind(device, iommufd, dev_id);
	else if (!df->noiommu)
		ret = vfio_device_group_use_iommu(device);
	if (ret)
		goto err_module_put;


> 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:

	if (iommufd) {
		ret = vfio_iommufd_bind(device, iommufd, dev_id);

		if (!ret && !df->is_cdev_device) {
			ret = vfio_iommufd_attach_compat(device); // new helper as in patch 10 discussed
			if (ret)
				vfio_iommufd_unbind(device);
		}
	} else if (!df->is_cdev_device) {
		ret = vfio_device_group_use_iommu(device);
	}
	if (ret)
		goto err_module_put;

Regards,
Yi Liu