Re: [PATCH v2 10/16] iommu: Restore and reattach preserved domains to devices

[Samiullah Khawaja <skhawaja@google.com>]

On Thu, May 07, 2026 at 09:54:27PM +0800, Baolu Lu wrote:
>On 4/28/2026 1:56 AM, Samiullah Khawaja wrote:
>>Restore the preserved domains by restoring the page tables using restore
>>IOMMU domain op. Reattach the preserved domain to the device during
>>default domain setup. While attaching, reuse the domain ID that was used
>>in the previous kernel. The context entry setup is not needed as that is
>>preserved during liveupdate.
>>
>>Signed-off-by: Samiullah Khawaja<skhawaja@google.com>
>>---
>>  drivers/iommu/intel/iommu.c      | 49 ++++++++++++++------
>>  drivers/iommu/intel/iommu.h      |  3 +-
>>  drivers/iommu/intel/nested.c     |  2 +-
>>  drivers/iommu/iommu.c            | 61 ++++++++++++++++++++++++-
>>  drivers/iommu/liveupdate.c       | 78 ++++++++++++++++++++++++++++++++
>>  include/linux/iommu-liveupdate.h | 50 ++++++++++++++++++++
>>  6 files changed, 224 insertions(+), 19 deletions(-)
>
>Please split the changes in the Intel iommu driver and the iommu core
>into different patches.

Agreed. I will split these.
>
>>
>>diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>index 4118a0861f38..b90757164cd8 100644
>>--- a/drivers/iommu/intel/iommu.c
>>+++ b/drivers/iommu/intel/iommu.c
>>@@ -1031,7 +1031,8 @@ static bool first_level_by_default(struct intel_iommu *iommu)
>>  	return true;
>>  }
>>-int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
>>+int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu,
>>+			int restore_did)
>
>How about using a new helper for restored domain? For example,
>
>	int domain_reattach_iommu(...)
>
>or
>
>	int domain_restore_iommu(...)

Agreed. I didn't do this to avoid code duplication, but I think this
might be much clean. Also in the restore path, the did test can also be
done as you pointed out below.
>
>>  {
>>  	struct iommu_domain_info *info, *curr;
>>  	int num, ret = -ENOSPC;
>>@@ -1051,8 +1052,11 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
>>  		return 0;
>>  	}
>>-	num = ida_alloc_range(&iommu->domain_ida, IDA_START_DID,
>>-			      cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
>>+	if (restore_did >= IDA_START_DID)
>>+		num = restore_did;
>
>For a preserved domain ID, do we need to check whether it has been
>reserved from the ida pool?

Yes, I will add a sanity check once I move this logic into separate
function.
>
>>+	else
>>+		num = ida_alloc_range(&iommu->domain_ida, IDA_START_DID,
>>+				      cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
>>  	if (num < 0) {
>>  		pr_err("%s: No free domain ids\n", iommu->name);
>>  		goto err_unlock;
>>@@ -1320,10 +1324,14 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
>>  {
>>  	struct device_domain_info *info = dev_iommu_priv_get(dev);
>>  	struct intel_iommu *iommu = info->iommu;
>>+	struct device_ser *device_ser = NULL;
>>  	unsigned long flags;
>>  	int ret;
>>-	ret = domain_attach_iommu(domain, iommu);
>>+	device_ser = dev_iommu_restored_state(dev);
>>+
>>+	ret = domain_attach_iommu(domain, iommu,
>>+				  dev_iommu_restore_did(dev, &domain->domain));
>
>What is the expected behavior if there is a mismatch between the
>restored state and the availability of a domain ID? Specifically, if
>device_ser is found but dev_iommu_restore_did() fails (returns -1), how
>should the driver proceed in that case?

The restore of the domain should fail in this case, as this is
unexpected behaviour, as the DID should be preserved in the previous
kernel and in this (new) kernel the did should have been reclaimed
during iommu HW restore.

But I think we can sanity check these in the separate restore function.
I will rework this.
>
>>  	if (ret)
>>  		return ret;
>>@@ -1336,16 +1344,18 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
>>  	if (dev_is_real_dma_subdevice(dev))
>>  		return 0;
>>-	if (!sm_supported(iommu))
>>-		ret = domain_context_mapping(domain, dev);
>>-	else if (intel_domain_is_fs_paging(domain))
>>-		ret = domain_setup_first_level(iommu, domain, dev,
>>-					       IOMMU_NO_PASID, NULL);
>>-	else if (intel_domain_is_ss_paging(domain))
>>-		ret = domain_setup_second_level(iommu, domain, dev,
>>-						IOMMU_NO_PASID, NULL);
>>-	else if (WARN_ON(true))
>>-		ret = -EINVAL;
>>+	if (!device_ser) {
>>+		if (!sm_supported(iommu))
>>+			ret = domain_context_mapping(domain, dev);
>>+		else if (intel_domain_is_fs_paging(domain))
>>+			ret = domain_setup_first_level(iommu, domain, dev,
>>+						       IOMMU_NO_PASID, NULL);
>>+		else if (intel_domain_is_ss_paging(domain))
>>+			ret = domain_setup_second_level(iommu, domain, dev,
>>+							IOMMU_NO_PASID, NULL);
>>+		else if (WARN_ON(true))
>>+			ret = -EINVAL;
>>+	}
>>  	if (ret)
>>  		goto out_block_translation;
>>@@ -3170,6 +3180,15 @@ int paging_domain_compatible(struct iommu_domain *domain, struct device *dev)
>>  	struct intel_iommu *iommu = info->iommu;
>>  	int ret = -EINVAL;
>>+#ifdef CONFIG_IOMMU_LIVEUPDATE
>>+	/*
>>+	 * Restored IOMMU domains are already attached to the device and can
>>+	 * only be freed. So no need to check the compatibility.
>>+	 */
>>+	if (iommu_domain_restored_state(domain))
>>+		return 0;
>>+#endif
>>+
>>  	if (intel_domain_is_fs_paging(dmar_domain))
>>  		ret = paging_domain_compatible_first_stage(dmar_domain, iommu);
>>  	else if (intel_domain_is_ss_paging(dmar_domain))
>>@@ -3647,7 +3666,7 @@ domain_add_dev_pasid(struct iommu_domain *domain,
>>  	if (!dev_pasid)
>>  		return ERR_PTR(-ENOMEM);
>>-	ret = domain_attach_iommu(dmar_domain, iommu);
>>+	ret = domain_attach_iommu(dmar_domain, iommu, -1);
>>  	if (ret)
>>  		goto out_free;
>>diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>>index b0ec0b471a43..8e37acf7de12 100644
>>--- a/drivers/iommu/intel/iommu.h
>>+++ b/drivers/iommu/intel/iommu.h
>>@@ -1182,7 +1182,8 @@ void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
>>   */
>>  #define QI_OPT_WAIT_DRAIN		BIT(0)
>>-int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu);
>>+int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu,
>>+			int restore_did);
>>  void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu);
>>  void device_block_translation(struct device *dev);
>>  int paging_domain_compatible(struct iommu_domain *domain, struct device *dev);
>>diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
>>index 2b979bec56ce..6e13f697b463 100644
>>--- a/drivers/iommu/intel/nested.c
>>+++ b/drivers/iommu/intel/nested.c
>>@@ -40,7 +40,7 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
>>  		return ret;
>>  	}
>>-	ret = domain_attach_iommu(dmar_domain, iommu);
>>+	ret = domain_attach_iommu(dmar_domain, iommu, -1);
>>  	if (ret) {
>>  		dev_err_ratelimited(dev, "Failed to attach domain to iommu\n");
>>  		return ret;
>
>Thanks,
>baolu

Thanks,
Sami