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

[Samiullah Khawaja <skhawaja@google.com>]

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?

Thanks,
Sami