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

[Samiullah Khawaja <skhawaja@google.com>]

On Sat, Oct 31, 2026 at 01:27:05PM +0800, Baolu Lu wrote:
>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.

Sounds good. I will replace iommu_group_is_singleton() with
iommu_group_device_count() and add the comment.

>
>Thanks,
>baolu

Thanks,
Sami