Re: [PATCH v4 12/18] iommu/vt-d: Handle reattach of the restored domain

[Samiullah Khawaja]

On Thu, Aug 27, 2026 at 04:12:01PM +0800, Baolu Lu wrote:
> On 8/8/26 10:27, Samiullah Khawaja wrote:
> > Reattach the restored domain to the preserved device using restored
> > domain ID. While reattaching do not setup the context and PASID entries
> > as those are preserved during liveupdate.
> >
> > Signed-off-by: Samiullah Khawaja <skhawaja@xxxxxxxxxx>
> > ---
> >  drivers/iommu/intel/iommu.c      |   9 ++-
> >  drivers/iommu/intel/iommu.h      |  10 +++
> >  drivers/iommu/intel/liveupdate.c | 120 +++++++++++++++++++++++++++++++
> >  3 files changed, 136 insertions(+), 3 deletions(-)

[snip]

> > +
> > +static int domain_reattach_iommu(struct dmar_domain *domain,
> > +				 struct intel_iommu *iommu,
> > +				 struct iommu_device_ser *device_ser)
> > +{
> > +	struct iommu_domain_info *info, *curr;
> > +	int restored_did;
> > +	int ret;
> > +
> > +	if (!iommu_domain_restored_state(&domain->domain))
> > +		return -EINVAL;
> > +
> > +	restored_did = device_ser->domain_iommu_ser.attachment_id;
> > +	if (!ida_exists(&iommu->domain_ida, restored_did))
> > +		return -EINVAL;
>
> It seems that checking only whether the domain ID is reserved on this
> IOMMU may not be sufficient. It would be safer to also verify that:
>
> - device_ser->domain_iommu_ser.iommu_phys matches iommu->reg_phys, and
> - device_ser->domain_iommu_ser.domain_phys matches @domain.
>
> ?

Interesting.. The caller of this attach from core fetches the correct
domain based on the checks you mentioned. But you are right, adding a
check here makes sense to prevent anyone else calling attach with a
mismatch.
> > +
> > +	info = kzalloc_obj(*info);
> > +	if (!info)
> > +		return -ENOMEM;
> > +
> > +	guard(mutex)(&iommu->did_lock);
> > +	curr = xa_load(&domain->iommu_array, iommu->seq_id);
> > +	if (curr) {
> > +		curr->refcnt++;
> > +		kfree(info);
> > +		return 0;
> > +	}
> > +
> > +	info->refcnt	= 1;
> > +	info->did	= restored_did;
> > +	info->iommu	= iommu;
> > +	curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
> > +			  NULL, info, GFP_KERNEL);
> > +	if (curr) {
> > +		ret = xa_err(curr) ? : -EBUSY;
> > +		goto err_unlock;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_unlock:
> > +	kfree(info);
> > +	return ret;
> > +}
> > +
> > +/**
> > + * intel_iommu_restore_device() - Restore device domain attachment after live update
> > + * @domain: Restored domain
> > + * @dev: Restored device
> > + *
> > + * Return: 0 on success, or negative error code.
> > + */
> > +int intel_iommu_restore_device(struct iommu_domain *domain,
> > +			       struct device *dev)
> > +{
> > +	struct iommu_device_ser *device_ser = dev_iommu_restored_state(dev);
> > +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> > +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > +	struct intel_iommu *iommu = info->iommu;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	if (!device_ser)
> > +		return -EINVAL;
> > +
> > +	if (dev_is_real_dma_subdevice(dev))
> > +		return -EOPNOTSUPP;
>
> ... or, move above check here to ensure the attachment relationship
> between the @device and @domain?

I think it is good to keep it in the domain_reattach_iommu() as that
does the iommu <-> domain association.
> > +
> > +	ret = domain_reattach_iommu(dmar_domain, iommu, device_ser);
> > +	if (ret)
> > +		return ret;
> > +
> > +	info->domain = dmar_domain;
> > +	info->domain_attached = true;
> > +	spin_lock_irqsave(&dmar_domain->lock, flags);
> > +	list_add(&info->link, &dmar_domain->devices);
> > +	spin_unlock_irqrestore(&dmar_domain->lock, flags);
> > +
> > +	if (!sm_supported(iommu))
> > +		intel_iommu_enable_pci_ats(info);
>
> Could you please clarify the PCI ATS behavior across live update?
>
> My understanding is that PCI devices may bypass reset during kexec and
> are then re-initialized by the new kernel (is that correct?). If so, ATS
> state in hardware would depend on its state before kexec in the old
> kernel.
>
> In a normal reboot, ATS is expected to be disabled and the device ATC is
> empty. But that assumption may not hold for live update. If that is
> true, can we still use the same approach to keep hardware ATS state and
> info->ats_enabled in sync?

Regarding ATS, it remains enabled in the preserved device during kexec
and we should keep it in sync with the info->ats_enabled here. We might
have to disable it in the next kernel if it is unsupported (or disabled
globally) in the next kernel. I will put those changes here in next
revision.
> > +
> > +	ret = cache_tag_assign_domain(dmar_domain, dev, IOMMU_NO_PASID);
> > +	if (ret)
> > +		goto err;
> > +
> > +	ret = iopf_for_domain_set(domain, dev);
> > +	if (ret)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +err:
> > +	/*
> > +	 * Detach the restored domain from device and iommu on failure, but keep
> > +	 * the hardware state intact.
> > +	 */
> > +	info->domain_attached = false;
> > +	cache_tag_unassign_domain(info->domain, dev, IOMMU_NO_PASID);
> > +	spin_lock_irqsave(&info->domain->lock, flags);
> > +	list_del(&info->link);
> > +	spin_unlock_irqrestore(&info->domain->lock, flags);
> > +
> > +	domain_detach_reattached_iommu(info->domain, iommu);
> > +	info->domain = NULL;
> > +	return ret;
> > +}
> > +
> >  /**
> >   * intel_iommu_preserve_device() - Intel IOMMU callback to preserve device state
> >   * @dev: Target device
>
> Thanks,
> baolu

Thanks,
Sami