Re: [PATCH v5 4/9] iommufd: Allow binding to a noiommu device

[Yi Liu <yi.l.liu@intel.com>]

On 5/12/26 02:41, Jacob Pan wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Allow iommufd to bind devices without an IOMMU (noiommu mode) by creating
> a dummy IOMMU group for such devices and skipping hwpt operations.
> 
> This enables noiommu devices to operate through the same iommufd API as IOMMU-
> capable devices.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
> ---
> v5:
>     - simplify logic and rename iommufd_device_is_noiommu (Kevin, Yi)
>     - use a helper iommufd_bind_noiommu instead of open coding (Kevin)
>     - move IOMMU cap check under iommufd_bind_iommu() (Yi)
>     - reword comments for partial init (Yi)
>     - misc minor clean up
> v4:
>     - Update the description of the module parameter (Alex)
> v3:
>     - Consolidate into fewer patches
> ---
>   drivers/iommu/iommufd/device.c | 148 ++++++++++++++++++++++++---------
>   1 file changed, 109 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index d03076fcf3c2..4d75720432cc 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -23,6 +23,16 @@ struct iommufd_attach {
>   	struct xarray device_array;
>   };
>   
> +/*
> + * 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.

Are you trying to identify the case where both group and cdev interfaces
coexist? In such a scenario, the group path creates a fake iommu group
while the cdev path creates a dummy igroup. Both operations need to be
performed, but checking iommu_group might interfere with the cdev
noiommu path logic.

Is that why you're checking idev->dev->iommu here instead? If so, could
you please make this rationale more explicit in the comment?

At first glance, the comment is quite confusing. It's unclear why a
cdev-path-only helper function needs to reference the group path at all.
The last sentence in particular seems to suggest this helper covers both
paths, which adds to the confusion.

> + */
> +static bool iommufd_device_is_noiommu(struct iommufd_device *idev)
> +{
> +	return IS_ENABLED(CONFIG_IOMMUFD_NOIOMMU) && !idev->dev->iommu;
> +}
> +
>   static void iommufd_group_release(struct kref *kref)
>   {
>   	struct iommufd_group *igroup =
> @@ -30,9 +40,11 @@ static void iommufd_group_release(struct kref *kref)
>   
>   	WARN_ON(!xa_empty(&igroup->pasid_attach));
>   
> -	xa_cmpxchg(&igroup->ictx->groups, iommu_group_id(igroup->group), igroup,
> -		   NULL, GFP_KERNEL);
> -	iommu_group_put(igroup->group);
> +	if (igroup->group) {
> +		xa_cmpxchg(&igroup->ictx->groups, iommu_group_id(igroup->group),
> +			   igroup, NULL, GFP_KERNEL);
> +		iommu_group_put(igroup->group);
> +	}
>   	mutex_destroy(&igroup->lock);
>   	kfree(igroup);
>   }
> @@ -204,32 +216,19 @@ void iommufd_device_destroy(struct iommufd_object *obj)
>   	struct iommufd_device *idev =
>   		container_of(obj, struct iommufd_device, obj);
>   
> -	iommu_device_release_dma_owner(idev->dev);
> +	if (!idev->igroup)
> +		return;
> +	if (!iommufd_device_is_noiommu(idev))
> +		iommu_device_release_dma_owner(idev->dev);
>   	iommufd_put_group(idev->igroup);
>   	if (!iommufd_selftest_is_mock_dev(idev->dev))
>   		iommufd_ctx_put(idev->ictx);
>   }
>   
> -/**
> - * iommufd_device_bind - Bind a physical device to an iommu fd
> - * @ictx: iommufd file descriptor
> - * @dev: Pointer to a physical device struct
> - * @id: Output ID number to return to userspace for this device
> - *
> - * A successful bind establishes an ownership over the device and returns
> - * struct iommufd_device pointer, otherwise returns error pointer.
> - *
> - * A driver using this API must set driver_managed_dma and must not touch
> - * the device until this routine succeeds and establishes ownership.
> - *
> - * Binding a PCI device places the entire RID under iommufd control.
> - *
> - * The caller must undo this with iommufd_device_unbind()
> - */
> -struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
> -					   struct device *dev, u32 *id)
> +static int iommufd_bind_iommu(struct iommufd_device *idev)
>   {
> -	struct iommufd_device *idev;
> +	struct iommufd_ctx *ictx = idev->ictx;
> +	struct device *dev = idev->dev;
>   	struct iommufd_group *igroup;
>   	int rc;
>   
> @@ -238,11 +237,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);
>   	if (IS_ERR(igroup))
> -		return ERR_CAST(igroup);
> +		return PTR_ERR(igroup);
>   
>   	/*
>   	 * For historical compat with VFIO the insecure interrupt path is
> @@ -268,21 +267,80 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
>   	if (rc)
>   		goto out_group_put;
>   
> +	/* igroup refcount moves into iommufd_device */
> +	idev->igroup = igroup;
> +	idev->enforce_cache_coherency =
> +		device_iommu_capable(dev, IOMMU_CAP_ENFORCE_CACHE_COHERENCY);
> +	return 0;
> +
> +out_group_put:
> +	iommufd_put_group(igroup);
> +	return rc;
> +}
> +
> +/*
> + * Noiommu devices have no real IOMMU group. Create a dummy igroup so that
> + * internal code paths that expect idev->igroup to be present still work.
> + * A NULL igroup->group distinguishes this from a real IOMMU-backed group.
> + */
> +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;
> +}
> +
> +/**
> + * iommufd_device_bind - Bind a physical device to an iommu fd
> + * @ictx: iommufd file descriptor
> + * @dev: Pointer to a physical device struct
> + * @id: Output ID number to return to userspace for this device
> + *
> + * A successful bind establishes an ownership over the device and returns
> + * struct iommufd_device pointer, otherwise returns error pointer.
> + *
> + * A driver using this API must set driver_managed_dma and must not touch
> + * the device until this routine succeeds and establishes ownership.
> + *
> + * Binding a PCI device places the entire RID under iommufd control.
> + *
> + * The caller must undo this with iommufd_device_unbind()
> + */
> +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;
> +	idev->dev = dev;
> +
> +	if (!iommufd_device_is_noiommu(idev)) {
> +		rc = iommufd_bind_iommu(idev);
> +		if (rc)
> +			goto err_out;
> +	} else {
> +		rc = iommufd_bind_noiommu(idev);
> +		if (rc)
> +			goto err_out;
> +	}

how about moving the 'if (rc)'out of the two branches?

> +
> +	/*
> +	 * Take a ctx reference after bind succeeds. This must happen here
> +	 * so that iommufd_device_destroy() can handle partial initialization
> +	 */

s/after bind succeeds/after bind_iommu/noiommu() succeeds. or more
explicit, "idev->igroup is set." :)

Other parts LGTM, feel free to add my r-b tag after solving the above
nits.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

>   	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);
>   	/* The calling driver is a user until iommufd_device_unbind() */
>   	refcount_inc(&idev->obj.users);
> -	/* igroup refcount moves into iommufd_device */
> -	idev->igroup = igroup;
>   
>   	/*
>   	 * If the caller fails after this success it must call
> @@ -294,11 +352,14 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
>   	*id = idev->obj.id;
>   	return idev;
>   
> -out_release_owner:
> -	iommu_device_release_dma_owner(dev);
> -out_group_put:
> -	iommufd_put_group(igroup);
> +err_out:
> +	/*
> +	 * iommufd_device_destroy() handles partially initialized idev,
> +	 * so iommufd_object_abort_and_destroy() is safe to call here.
> +	 */
> +	iommufd_object_abort_and_destroy(ictx, &idev->obj);
>   	return ERR_PTR(rc);
> +
>   }
>   EXPORT_SYMBOL_NS_GPL(iommufd_device_bind, "IOMMUFD");
>   
> @@ -512,6 +573,9 @@ static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
>   	struct iommufd_attach_handle *handle;
>   	int rc;
>   
> +	if (iommufd_device_is_noiommu(idev))
> +		return 0;
> +
>   	if (!iommufd_hwpt_compatible_device(hwpt, idev))
>   		return -EINVAL;
>   
> @@ -559,6 +623,9 @@ static void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
>   {
>   	struct iommufd_attach_handle *handle;
>   
> +	if (iommufd_device_is_noiommu(idev))
> +		return;
> +
>   	handle = iommufd_device_get_attach_handle(idev, pasid);
>   	if (pasid == IOMMU_NO_PASID)
>   		iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
> @@ -577,6 +644,9 @@ static int iommufd_hwpt_replace_device(struct iommufd_device *idev,
>   	struct iommufd_attach_handle *handle, *old_handle;
>   	int rc;
>   
> +	if (iommufd_device_is_noiommu(idev))
> +		return 0;
> +
>   	if (!iommufd_hwpt_compatible_device(hwpt, idev))
>   		return -EINVAL;
>   
> @@ -652,7 +722,7 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
>   		goto err_release_devid;
>   	}
>   
> -	if (attach_resv) {
> +	if (attach_resv && !iommufd_device_is_noiommu(idev)) {
>   		rc = iommufd_device_attach_reserved_iova(idev, hwpt_paging);
>   		if (rc)
>   			goto err_release_devid;