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

Tian, Kevin
2026-04-16T07:57:00Z

> From: Jacob Pan <jacob.pan@linux.microsoft.com>
> 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?