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

[Tian, Kevin]

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

> @@ -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.

> +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().

> +	}
> +
> +	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.

>  	/* 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?