Re: [PATCH v6 05/18] iommu: Implement device and IOMMU HW preservation

[Baolu Lu]

On 10/31/2026 9:45 AM, Samiullah Khawaja wrote:
> > > +
> > >  #if IS_ENABLED(CONFIG_IOMMUFD_TEST)
> > >  void iommu_device_unregister_bus(struct iommu_device *iommu,
> > >                   const struct bus_type *bus,
> > > @@ -626,8 +668,8 @@ DEFINE_MUTEX(iommu_probe_device_lock);
> > >  static int __iommu_probe_device(struct device *dev, struct list_head 
> > > *group_list)
> > >  {
> > > +    struct group_device *gdev, *gdev2;
> > >      struct iommu_group *group;
> > > -    struct group_device *gdev;
> > >      int ret;
> > >      /*
> > > @@ -661,6 +703,13 @@ static int __iommu_probe_device(struct device 
> > > *dev, struct list_head *group_list
> > >          goto err_put_group;
> > >      }
> > > +    for_each_group_device(group, gdev2) {
> > > +        if (dev_iommu_has_preserved_state(gdev2->dev)) {
> > > +            ret = -EBUSY;
> > > +            goto err_free_gdev;
> > > +        }
> > > +    }
> > > +
> > >      /*
> > >       * The gdev must be in the list before calling
> > >       * iommu_setup_default_domain()
> > > @@ -697,6 +746,7 @@ static int __iommu_probe_device(struct device 
> > > *dev, struct list_head *group_list
> > >  err_remove_gdev:
> > >      list_del(&gdev->list);
> > > +err_free_gdev:
> > >      __iommu_group_free_device(group, gdev);
> > >  err_put_group:
> > >      iommu_deinit_device(dev);
> > > @@ -3543,6 +3593,26 @@ bool iommu_group_dma_owner_claimed(struct 
> > > iommu_group *group)
> > >  }
> > >  EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
> > > +/**
> > > + * iommu_group_is_singleton() - Query if group contains exactly one 
> > > device
> > > + * @group: The group.
> > > + *
> > > + * Return: true if group contains exactly 1 device, false otherwise.
> > > + */
> >
> > We discussed this interface before, and the consensus was that such an
> > interface is not feasible, because the membership of an iommu_group is
> > not fixed, given the hot-plug support of buses like PCI.
> >
> > I am fine with the usage model of ensuring the iommu group of a
> > preserved device to be a singleton, but I don't think we even need such
> > a kAPI. It should be handled case by case.
>
> The iommu_group membership can change, this is why we check it in two
> places, both under group->mutex:
>
> - During preservation we check that the group is a singleton, and check
>    again after setting device_ser.
> - During probe we refuse to add a device to a group that has a preserved
>    device.
>
> So if a device is added to the group while we are preserving, either the
> probe fails because the group already has a preserved device, or the
> preservation fails because the group is no longer a singleton. Could you
> please elaborate what you mean by handling it case by case?

I am fine with the singleton-group checking logic you described above,
but I don't think we need a helper like iommu_group_is_singleton(). It
drops group->mutex before returning, so its result may already be stale
when the caller uses it. That makes it easy to misuse in the future.

I'd prefer to open-code the check where group->mutex is already held, as
iommu_attach_device() does today:

int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
{
        /* Caller must be a probed driver on dev */
        struct iommu_group *group = dev->iommu_group;
        int ret;

        if (!group)
                return -ENODEV;

        /*
         * Lock the group to make sure the device-count doesn't
         * change while we are attaching
         */
        mutex_lock(&group->mutex);
        ret = -EINVAL;
        if (list_count_nodes(&group->devices) != 1)
                goto out_unlock;

        ret = __iommu_attach_group(domain, group);

out_unlock:
        mutex_unlock(&group->mutex);
        return ret;

}
EXPORT_SYMBOL_GPL(iommu_attach_device);

Thanks,
baolu