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

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

On 10/31/2026 11:39 AM, Samiullah Khawaja wrote:
> On Sat, Oct 31, 2026 at 11:04:22AM +0800, Baolu Lu wrote:
>> 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:
> 
> struct iommu_group is private to drivers/iommu/iommu.c, and
> iommu_preserve_device() is in liveupdate.c, so open coding it there is
> tricky.
> 
> You are correct that it drops the lock before returning. That is why I
> recheck the singleton state after setting device_ser, which handles the
> race.
> 
> I was thinking of adding something like iommu_group_lock() and
> iommu_group_unlock() for this, but the number of devices check also
> needs struct iommu_group definition.
> 
> To avoid misuse, I can add a comment on iommu_group_is_singleton() that
> the result may be stale and the caller must recheck. What do you think?

I would suggest something like this:

size_t iommu_group_device_count(struct iommu_group *group)
{
         size_t count;

         mutex_lock(&group->mutex);
         count = list_count_nodes(&group->devices);
         mutex_unlock(&group->mutex);

         return count;
}

and, add a comment stating that the device count may change after the
function returns, so the caller must recheck it.

Thanks,
baolu