Re: [PATCH v5 11/18] iommu: Restore and reattach preserved domains to devices

[Samiullah Khawaja <skhawaja@google.com>]

On Wed, Oct 07, 2026 at 12:44:31PM -0700, Nicolin Chen wrote:
>On Mon, Sep 21, 2026 at 12:48:27AM +0000, Samiullah Khawaja wrote:
>> @@ -694,7 +700,8 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>>  	}
>>
>>  	for_each_group_device(group, gdev2) {
>> -		if (dev_iommu_preserved_state(gdev2->dev)) {
>> +		if (dev_iommu_preserved_state(gdev2->dev) ||
>> +		    dev_iommu_restored_state(gdev2->dev)) {
>>  			ret = -EBUSY;
>>  			goto err_free_gdev;
>
>Maybe it should -EBUSY on a group that already has a device so it
>wouldn't end up with a multi-device group.

Agreed. This should make sure that a preserved device does not get added
into a group that already has a device. Will update.
>
>> @@ -2211,6 +2242,7 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>>  	ret = domain->ops->attach_dev(domain, dev, old);
>>  	if (ret)
>>  		return ret;
>> +
>>  	dev->iommu->attach_deferred = 0;
>>  	trace_attach_device_to_domain(dev);
>>  	return 0;
>
>Unnecessary change.

Will remove.
>
>> @@ -3175,6 +3207,62 @@ int iommu_fwspec_add_ids(struct device *dev, const u32 *ids, int num_ids)
>>  }
>>  EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
>>
>> +static struct device *__iommu_group_restored_device(struct iommu_group *group)
>> +{
>> +	struct group_device *gdev;
>> +
>> +	lockdep_assert_held(&group->mutex);
>> +	for_each_group_device(group, gdev) {
>> +		if (!dev_is_pci(gdev->dev))
>> +			continue;
>> +
>> +		if (dev_iommu_restored_state(gdev->dev))
>> +			return gdev->dev;
>
>list_first_entry instead of for_each_group_device since there's a
>singleton enforcement.

Agreed. Will Update.
>
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static int __iommu_group_restore_domain(struct iommu_group *group)
>> +{
>> +	struct iommu_device_ser *device_ser;
>> +	struct iommu_domain *domain;
>> +	struct device *dev;
>> +	void *owner;
>> +	int ret;
>> +
>> +	lockdep_assert_held(&group->mutex);
>> +	if (group->domain)
>> +		return -EBUSY;
>> +
>> +	dev = __iommu_group_restored_device(group);
>> +	device_ser = dev_iommu_restored_state(dev);
>> +	if (!device_ser)
>> +		return -ENOENT;
>> +
>> +	ret = __iommu_group_alloc_blocking_domain(group);
>> +	if (ret)
>> +		return ret;
>> +
>> +	domain = iommu_restore_domain(dev, device_ser, &owner);
>> +	if (WARN_ON(IS_ERR(domain)))
>> +		return PTR_ERR(domain);
>> +
>> +	/* The restored domain is attached with the restored device. */
>> +	ret = __iommu_group_set_domain(group, domain);
>> +	if (ret)
>> +		return ret;
>
>If (ret), how about the restored domain by iommu_restore_domain()?

The restored domain is not leaked and it remains restored and associated
with the preserved device in FLB and will be reused later if there is
another rescan.
>
>> +	/*
>> +	 * Ownership of groups with preserved devices is set during boot. These
>> +	 * will be reclaimed later by the entity (iommufd) that preserved them.
>> +	 */
>> +	WARN_ON(group->owner);
>> +	group->owner = owner;
>> +	group->owner_cnt = 1;
>> +	return ret;
>> +}
>> +
>>  /**
>>   * iommu_setup_default_domain - Set the default_domain for the group
>>   * @group: Group to change
>> @@ -3233,6 +3321,16 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>>
>>  	/* We must set default_domain early for __iommu_device_set_domain */
>>  	group->default_domain = dom;
>> +
>> +	/* Preserved devices need to be attached to the restore domain */
>> +	if (__iommu_group_restored_device(group)) {
>> +		ret = __iommu_group_restore_domain(group);
>
>__iommu_group_restored_device is called twice: here (outside) and
>inside __iommu_group_restore_domain.
>
>Perhaps change to:
>	dev = __iommu_group_restored_device(group);
>	if (dev)  {
>		ret = __iommu_device_restore_domain(dev);
>	...
>?

I will have to get the group again inside the
__iommu_device_restore_domain(), but I think that is fine. Will update
this.
>
>> +void iommu_init_device_preserved_data(struct device *dev)
>> +{
>> +	struct iommu_device_ser *device_ser = NULL;
>
>"= NULL" doesn't seem necessary.

Will remove.
>
>> +	struct iommu_device_array_ser *array;
>> +	struct iommu_flb_obj *flb_obj;
>> +	int ret, idx;
>> +
>> +	if (!dev_is_pci(dev))
>> +		return;
>> +
>> +	ret = iommu_liveupdate_flb_get_incoming(&flb_obj);
>> +	if (ret)
>> +		return;
>> +
>> +	mutex_lock(&flb_obj->lock);
>> +	array = phys_to_virt(flb_obj->ser->device_array_phys);
>> +	iommu_liveupdate_for_each_arr(array) {
>> +		iommu_liveupdate_for_each_obj(array, device_ser, idx) {
>> +			if (match_device_ser(device_ser, to_pci_dev(dev))) {
>> +				device_ser->hdr.flags |= IOMMU_SER_FLAG_INCOMING;
>> +				goto out;
>> +			}
>> +		}
>> +	}
>> +
>> +	device_ser = NULL;
>> +out:
>> +	WRITE_ONCE(dev->iommu->device_ser, device_ser);
>
>dev->iommu->device_ser is NULL after kzalloc.
>
>So, maybe drop "device_ser = NULL" and move WRITE_ONCE() into the
>loop (under match_device_ser)?

Will update.
>
>> +	mutex_unlock(&flb_obj->lock);
>> +	liveupdate_flb_put_incoming(&iommu_flb);
>
>Hmm, you might want to check the lifecycle of this flb thing.
>
>dev->iommu->device_ser points to something inside the flb, which
>might be freed somewhere?

The lifecycle is bound to the FD that is preserved into LUO. The
incoming FLB is only freed when that FD is finished, and the device_ser
will be cleared before finish. That logic is not part of phase 1, as it
doesn't do the iommufd restore. In phase 1, iommufd's can_finish()
always returns false, so the FLB is never freed.
>
>> +struct iommu_domain *iommu_restore_domain(struct device *dev,
>[...]
>> +	domain_ser = phys_to_virt(ser->domain_iommu_ser.domain_phys);
>> +	if (domain_ser->restored_domain) {
>> +		*owner = ser;
>> +		domain = domain_ser->restored_domain;
>> +		goto out;
>> +	}
>> +
>> +	domain_ser->hdr.flags |=  IOMMU_SER_FLAG_INCOMING;
>
>Drop the extra space before "IOMMU".

Will update.
>
>> +/**
>> + * dev_iommu_restore_did() - Get restored domain ID for a device
>> + * @dev: Target device
>> + * @domain: Target domain
>> + *
>> + * Fetches the domain ID preserved for @dev and @domain across Live Update.
>> + *
>> + * Return: Domain ID or -1 on error.
>> + */
>> +static inline int dev_iommu_restore_did(struct device *dev, struct iommu_domain *domain)
>
>"did" is an intel thing..
>
>> +{
>> +	struct iommu_device_ser *ser = dev_iommu_restored_state(dev);
>> +
>> +	if (ser && iommu_domain_restored_state(domain))
>> +		return ser->domain_iommu_ser.attachment_id;
>
>... so, it could be just dev_iommu_restored_attachment_id()?

Yes, this looks good. I will update it.
>
>Nicolin

Thanks,
Sami