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

Jacob Pan
2026-05-05T23:04:26Z

Hi Kevin,

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

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