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

[Nicolin Chen <nicolinc@nvidia.com>]

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.

> @@ -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.

> @@ -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.

> +	}
> +
> +	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()?

> +	/*
> +	 * 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);
	...
?

> +void iommu_init_device_preserved_data(struct device *dev)
> +{
> +	struct iommu_device_ser *device_ser = NULL;

"= NULL" doesn't seem necessary.

> +	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)?

> +	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?

> +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".

> +/**
> + * 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()?

Nicolin