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

[Samiullah Khawaja <skhawaja@google.com>]

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