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

[Baolu Lu <baolu.lu@linux.intel.com>]

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