Re: [PATCH V4 03/10] iommufd: Allow binding to a noiommu device

[Jacob Pan]

Hi Kevin,

On Thu, 16 Apr 2026 07:56:55 +0000
"Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:

> > From: Jacob Pan <jacob.pan@xxxxxxxxxxxxxxxxxxx>
> > Sent: Wednesday, April 15, 2026 5:14 AM
> > 
> > +static bool is_vfio_noiommu(struct iommufd_device *idev)
> > +{
> > +	return !device_iommu_mapped(idev->dev) ||
> > !idev->dev->iommu; +}
> > +  
> 
> could you add some words why both conditions are checked? It's not
> obvious to me.
> 
> btw there is no need to add "_vfio_" in the name. 
Actually, I think the first check is not needed. group can be NULL if
no containers selected nor VFIO_GROUP is set.

I will change it to
+/*
+ * A noiommu device has no IOMMU driver attached regardless of whether it
+ * enters via the cdev path (no iommu_group) or the group path (fake
+ * noiommu iommu_group). In both cases dev->iommu is NULL.
+ */
+static bool iommufd_device_is_noiommu(struct iommufd_device *idev)
+{
+       return !idev->dev->iommu;
+}

> 
> > @@ -238,11 +232,11 @@ struct iommufd_device
> > *iommufd_device_bind(struct iommufd_ctx *ictx,
> >  	 * to restore cache coherency.
> >  	 */
> >  	if (!device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
> > -		return ERR_PTR(-EINVAL);
> > +		return -EINVAL;
> > 
> > -	igroup = iommufd_get_group(ictx, dev);
> > +	igroup = iommufd_get_group(idev->ictx, dev);  
> 
> pointless change.
will fix.

> > +struct iommufd_device *iommufd_device_bind(struct iommufd_ctx
> > *ictx,
> > +					   struct device *dev, u32
> > *id) +{
> > +	struct iommufd_device *idev;
> > +	int rc;
> > +
> >  	idev = iommufd_object_alloc(ictx, idev,
> > IOMMUFD_OBJ_DEVICE);
> > -	if (IS_ERR(idev)) {
> > -		rc = PTR_ERR(idev);
> > -		goto out_release_owner;
> > -	}
> > +	if (IS_ERR(idev))
> > +		return idev;
> > +
> >  	idev->ictx = ictx;
> > -	if (!iommufd_selftest_is_mock_dev(dev))
> > -		iommufd_ctx_get(ictx);
> >  	idev->dev = dev;
> >  	idev->enforce_cache_coherency =
> >  		device_iommu_capable(dev,
> > IOMMU_CAP_ENFORCE_CACHE_COHERENCY);
> > +
> > +	if (!is_vfio_noiommu(idev)) {
> > +		rc = iommufd_bind_iommu(idev);
> > +		if (rc)
> > +			goto err_out;
> > +	} else {
> > +		struct iommufd_group *igroup;
> > +
> > +		/*
> > +		 * Create a dummy igroup, lots of stuff expects
> > ths igroup to be
> > +		 * present, but a NULL igroup->group is OK
> > +		 */
> > +		igroup = iommufd_alloc_group(ictx, NULL);
> > +		if (IS_ERR(igroup)) {
> > +			rc = PTR_ERR(igroup);
> > +			goto err_out;
> > +		}
> > +		idev->igroup = igroup;  
> 
> though simple it's slightly clearer to move them into a wrapper e.g.
> iommufd_bind_dummy_iommu().
will move to a helper:
static int iommufd_bind_noiommu(struct iommufd_device *idev)
{
	struct iommufd_group *igroup;

	igroup = iommufd_alloc_group(idev->ictx, NULL);
	if (IS_ERR(igroup))
		return PTR_ERR(igroup);
	idev->igroup = igroup;
	return 0;
}
> > +	}
> > +
> > +	if (!iommufd_selftest_is_mock_dev(dev))
> > +		iommufd_ctx_get(ictx);  
> 
> better add a comment for the ordering here - this must be done
> here otherwise iommufd_device_destroy() will have a problem
> to handle partial initialization.
> 
will do.

> >  	/* The calling driver is a user until
> > iommufd_device_unbind() */ refcount_inc(&idev->obj.users);
> > -	/* igroup refcount moves into iommufd_device */
> > -	idev->igroup = igroup;  
> 
> this is common, why not leaving it here?
I think it is just easier to assign in each helper function where
igroup is allocated/get.

As you suggested above, iommufd_bind_noiommu(struct iommufd_device
*idev)