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

[Jacob Pan]

Hi Baolu,

On Wed, 13 May 2026 15:37:20 +0800
Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx> wrote:

> On 5/12/26 02:41, Jacob Pan wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > Signed-off-by: Jacob Pan <jacob.pan@xxxxxxxxxxxxxxxxxxx>
> > ---
> > 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.
> > + */
> > +static bool iommufd_device_is_noiommu(struct iommufd_device *idev)
> > +{
> > +	return IS_ENABLED(CONFIG_IOMMUFD_NOIOMMU) &&
> > !idev->dev->iommu;  
> 
> How about using device_iommu_mapped()? As I understand it, this is the
> standard way to determine whether a device is protected by an IOMMU.
> 
device_iommu_mapped() — checks dev->iommu_group != NULL

For noiommu via the cdev path, there's no iommu_group at all, so both
would return the same thing. But for noiommu via the group path (fake
noiommu group), the device does have an iommu_group (the fake one),
but dev->iommu is still NULL.

I know today we only call this check in the cdev path but seems more
robust not to rely on group.

> > +}
> > +
> >   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;  
> 
> I don't quite follow this logic. Is this check being added
> specifically for the new noiommu mode? Since the noiommu mode
> introduces the convention that "igroup->group == NULL" implies
> noiommu, there should be no cases where idev->igroup itself is NULL.
> 
idev->igroup can be null on error path only, not limited to noiommu. It
is the partially initialized idev case.
/*
 * iommufd_device_destroy() handles partially initialized idev,
 * so iommufd_object_abort_and_destroy() is safe to call here.
 */

How about add this comment?
 /* igroup is NULL when destroy called during bind error cleanup */

> > +	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;
> >    
> 
> Thanks,
> baolu