Re: [PATCH v5 10/18] iommu/vt-d: Restore IOMMU state and reclaimed domain ids

[Baolu Lu <baolu.lu@linux.intel.com>]

On 9/21/26 08:48, Samiullah Khawaja wrote:
> During boot fetch the preserved state of IOMMU unit and if found then
> restore the state.
> 
> - Reuse the root_table that was preserved in the previous kernel.
> - Reclaim the domain ids of the preserved domains for each preserved
>    devices so these are not acquired by another domain.
> 
> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
> ---
>   drivers/iommu/intel/iommu.c      | 111 +++++++++++++++++++------------
>   drivers/iommu/intel/iommu.h      |   7 ++
>   drivers/iommu/intel/liveupdate.c |  79 ++++++++++++++++++++++
>   3 files changed, 154 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 474c926172c5..c5044e834337 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -987,28 +987,30 @@ static void iommu_disable_translation(struct intel_iommu *iommu)
>   	raw_spin_unlock_irqrestore(&iommu->register_lock, flag);
>   }
>   
> -static void disable_dmar_iommu(struct intel_iommu *iommu)
> +static void release_dmar_iommu(struct intel_iommu *iommu)

Can we put the code refactor in a separate patch? Mixing it with a
feature patch makes it hard for review.

>   {
> -	/*
> -	 * All iommu domains must have been detached from the devices,
> -	 * hence there should be no domain IDs in use.
> -	 */
> -	if (WARN_ON(!ida_is_empty(&iommu->domain_ida)))
> -		return;
> +	struct iommu_hw_ser *iommu_ser;
>   
> -	if (iommu->gcmd & DMA_GCMD_TE)
> -		iommu_disable_translation(iommu);
> -}
> +	iommu_ser = iommu_get_preserved_data(iommu->reg_phys, IOMMU_INTEL);
> +	if (!iommu_ser) {
> +		/*
> +		 * All iommu domains must have been detached from the devices,
> +		 * hence there should be no domain IDs in use.
> +		 */
> +		WARN_ON(!ida_is_empty(&iommu->domain_ida));
> +
> +		if ((iommu->gcmd & DMA_GCMD_TE))
> +			iommu_disable_translation(iommu);
> +	}
>   
> -static void free_dmar_iommu(struct intel_iommu *iommu)
> -{
>   	if (iommu->copied_tables) {
>   		bitmap_free(iommu->copied_tables);
>   		iommu->copied_tables = NULL;
>   	}
>   
> -	/* free context mapping */
> -	free_context_table(iommu);
> +	/* free context mapping if there is no serialized state. */
> +	if (!iommu_ser)
> +		free_context_table(iommu);
>   
>   	if (ecap_prs(iommu->ecap))
>   		intel_iommu_finish_prq(iommu);
> @@ -1632,12 +1634,19 @@ static int copy_translation_tables(struct intel_iommu *iommu)
>   
>   static int __init init_dmars(void)
>   {
> +	struct iommu_hw_ser *iommu_ser;
>   	struct dmar_drhd_unit *drhd;
>   	struct intel_iommu *iommu;
>   	int ret;
>   
>   	for_each_iommu(iommu, drhd) {
> +		iommu_ser = iommu_get_preserved_data(iommu->reg_phys, IOMMU_INTEL);
>   		if (drhd->ignored) {
> +			if (WARN_ON(iommu_ser)) {
> +				ret = -EINVAL;
> +				goto free_iommu;
> +			}
> +
>   			iommu_disable_translation(iommu);
>   			continue;
>   		}
> @@ -1655,7 +1664,9 @@ static int __init init_dmars(void)
>   		}
>   
>   		intel_iommu_init_qi(iommu);
> -		init_translation_status(iommu);
> +
> +		if (!iommu_ser)
> +			init_translation_status(iommu);
>   
>   		if (translation_pre_enabled(iommu) && !is_kdump_kernel()) {
>   			iommu_disable_translation(iommu);
> @@ -1664,14 +1675,18 @@ static int __init init_dmars(void)
>   				iommu->name);
>   		}
>   
> -		/*
> -		 * TBD:
> -		 * we could share the same root & context tables
> -		 * among all IOMMU's. Need to Split it later.
> -		 */
> -		ret = iommu_alloc_root_entry(iommu);
> -		if (ret)
> -			goto free_iommu;
> +		if (iommu_ser) {
> +			intel_iommu_liveupdate_restore_root_table(iommu, iommu_ser);
> +		} else {
> +			/*
> +			 * TBD:
> +			 * we could share the same root & context tables
> +			 * among all IOMMU's. Need to Split it later.
> +			 */
> +			ret = iommu_alloc_root_entry(iommu);
> +			if (ret)
> +				goto free_iommu;
> +		}
>   
>   		if (translation_pre_enabled(iommu)) {
>   			pr_info("Translation already enabled - trying to copy translation structures\n");
> @@ -1707,7 +1722,10 @@ static int __init init_dmars(void)
>   	 */
>   	for_each_active_iommu(iommu, drhd) {
>   		iommu_flush_write_buffer(iommu);
> -		iommu_set_root_entry(iommu);
> +
> +		iommu_ser = iommu_get_preserved_data(iommu->reg_phys, IOMMU_INTEL);
> +		if (!iommu_ser)
> +			iommu_set_root_entry(iommu);
>   	}
>   
>   	check_tylersburg_isoch();
> @@ -1752,10 +1770,8 @@ static int __init init_dmars(void)
>   	return 0;
>   
>   free_iommu:
> -	for_each_active_iommu(iommu, drhd) {
> -		disable_dmar_iommu(iommu);
> -		free_dmar_iommu(iommu);
> -	}
> +	for_each_active_iommu(iommu, drhd)
> +		release_dmar_iommu(iommu);
>   
>   	return ret;
>   }
> @@ -2136,17 +2152,28 @@ int dmar_parse_one_satc(struct acpi_dmar_header *hdr, void *arg)
>   static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
>   {
>   	struct intel_iommu *iommu = dmaru->iommu;
> +	struct iommu_hw_ser *iommu_ser;
>   	int ret;
>   
> +	/* Use IOMMU HW unit MMIO base to identify the preserved state. */
> +	iommu_ser = iommu_get_preserved_data(iommu->reg_phys, IOMMU_INTEL);
> +
>   	/*
>   	 * Disable translation if already enabled prior to OS handover.
>   	 */
> -	if (iommu->gcmd & DMA_GCMD_TE)
> +	if (!iommu_ser && iommu->gcmd & DMA_GCMD_TE)
>   		iommu_disable_translation(iommu);
>   
> -	ret = iommu_alloc_root_entry(iommu);
> -	if (ret)
> -		goto out;
> +	if (iommu_ser) {
> +		if (WARN_ON(dmaru->ignored))
> +			return -EINVAL;
> +
> +		intel_iommu_liveupdate_restore_root_table(iommu, iommu_ser);
> +	} else {
> +		ret = iommu_alloc_root_entry(iommu);
> +		if (ret)
> +			goto out;
> +	}
>   
>   	intel_svm_check(iommu);
>   
> @@ -2165,23 +2192,23 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
>   	if (ecap_prs(iommu->ecap)) {
>   		ret = intel_iommu_enable_prq(iommu);
>   		if (ret)
> -			goto disable_iommu;
> +			goto out;
>   	}
>   
>   	ret = dmar_set_interrupt(iommu);
>   	if (ret)
> -		goto disable_iommu;
> +		goto out;
> +
> +	if (!iommu_ser)
> +		iommu_set_root_entry(iommu);
>   
> -	iommu_set_root_entry(iommu);
>   	iommu_enable_translation(iommu);
>   
>   	iommu_disable_protect_mem_regions(iommu);
>   	return 0;
>   
> -disable_iommu:
> -	disable_dmar_iommu(iommu);
>   out:
> -	free_dmar_iommu(iommu);
> +	release_dmar_iommu(iommu);
>   	return ret;
>   }
>   
> @@ -2195,12 +2222,10 @@ int dmar_iommu_hotplug(struct dmar_drhd_unit *dmaru, bool insert)
>   	if (iommu == NULL)
>   		return -EINVAL;
>   
> -	if (insert) {
> +	if (insert)
>   		ret = intel_iommu_add(dmaru);
> -	} else {
> -		disable_dmar_iommu(iommu);
> -		free_dmar_iommu(iommu);
> -	}
> +	else
> +		release_dmar_iommu(iommu);
>   
>   	return ret;
>   }
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 4feb5bd76b18..3a2cb08c0ac1 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -1308,10 +1308,17 @@ int intel_iommu_preserve(struct iommu_device *iommu,
>   void intel_iommu_unpreserve(struct iommu_device *iommu,
>   			    struct iommu_hw_ser *iommu_ser);
>   void clear_unpreserved_context_entries(struct intel_iommu *iommu);
> +void intel_iommu_liveupdate_restore_root_table(struct intel_iommu *iommu,
> +					       struct iommu_hw_ser *iommu_ser);
>   #else
>   static inline void clear_unpreserved_context_entries(struct intel_iommu *iommu)
>   {
>   }
> +
> +static inline void intel_iommu_liveupdate_restore_root_table(struct intel_iommu *iommu,
> +							     struct iommu_hw_ser *iommu_ser)
> +{
> +}
>   #endif
>   
>   #ifdef CONFIG_INTEL_IOMMU_SVM
> diff --git a/drivers/iommu/intel/liveupdate.c b/drivers/iommu/intel/liveupdate.c
> index 501dc0e9cc0a..c9e553379683 100644
> --- a/drivers/iommu/intel/liveupdate.c
> +++ b/drivers/iommu/intel/liveupdate.c
> @@ -272,6 +272,85 @@ static int preserve_iommu_context_tables(struct device_domain_info *info)
>   	return 0;
>   }
>   
> +static void restore_iommu_context(struct intel_iommu *iommu)
> +{
> +	struct context_entry *context;
> +	int i;
> +
> +	for (i = 0; i < ROOT_ENTRY_NR; i++) {
> +		context = iommu_context_addr(iommu, i, 0, 0);
> +		if (context)
> +			iommu_restore_pages(virt_to_phys(context));

Why not use the serialized context_tables_bitmap as the source of truth,
and check it against the present bits?

> +
> +		if (!sm_supported(iommu))
> +			continue;
> +
> +		context = iommu_context_addr(iommu, i, 0x80, 0);
> +		if (context)
> +			iommu_restore_pages(virt_to_phys(context));
> +	}
> +}
> +
> +static int _restore_used_domain_ids(struct iommu_device_ser *ser, void *arg)
> +{
> +	int id = ser->domain_iommu_ser.attachment_id;
> +	struct iommu_hw_ser *iommu_hw_ser;
> +	struct intel_iommu *iommu = arg;
> +	int ret;
> +
> +	if (WARN_ON(!ser->domain_iommu_ser.iommu_phys))
> +		return 0;
> +
> +	iommu_hw_ser = phys_to_virt(ser->domain_iommu_ser.iommu_phys);
> +	if (iommu_hw_ser->type != IOMMU_INTEL)
> +		return 0;
> +
> +	/* Only allocate domain ID from associated IOMMU HW unit */
> +	if (iommu_hw_ser->intel.phys_addr != iommu->reg_phys)
> +		return 0;
> +
> +	guard(mutex)(&iommu->did_lock);
> +
> +	/*
> +	 * The domain IDs are reclaimed while the IOMMU HW unit is being
> +	 * restored and not registered with the IOMMU core. So if the ID already
> +	 * exists, it is safe to assume that a preserved device sharing the same
> +	 * domain ID reclaimed it.
> +	 */
> +	if (ida_exists(&iommu->domain_ida, id))
> +		return 0;
> +
> +	ret = ida_alloc_range(&iommu->domain_ida, id, id, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;

Commit 12a2ea2d91be ("iommu/vt-d: Handle DID reservation errors when
copying context tables") on the iommu/next branch introduced a new
helper named reserve_domain_id() for reserving a domain ID inherited
from the previous kernel. Could it be reused here?

> +
> +	return 0;
> +}
> +
> +/**
> + * intel_iommu_liveupdate_restore_root_table() - Restore root table and reclaim domain IDs
> + * @iommu: Target IOMMU
> + * @iommu_ser: Serialized IOMMU hardware state from previous kernel
> + *
> + * Restores the preserved root table and context tables for the IOMMU hardware
> + * instance across Live Update, and reclaims all domain IDs previously allocated
> + * to preserved devices so they are not reused.
> + */
> +void intel_iommu_liveupdate_restore_root_table(struct intel_iommu *iommu,
> +					       struct iommu_hw_ser *iommu_ser)
> +{

This restores the preserved root table without checking the new
environment. Is it possible that kexec has a different kernel command
line (say intel_iommu=sm_off) for the new kernel than the previous
kernel? sm_supported(iommu) will then return a value different from the
real hardware configuration.

> +	if (!iommu_ser->intel.restored)
> +		iommu_restore_pages(iommu_ser->intel.root_table);
> +
> +	iommu->root_entry = __va(iommu_ser->intel.root_table);

Why does the above line execute no matter the value of
iommu_ser->intel.restored? It would be more readable if the code looked
like...

	if (iommu_ser->intel.restored)
		return;

	iommu_restore_pages(iommu_ser->intel.root_table);
	... ...

Or I overlooked anything?

> +
> +	if (!iommu_ser->intel.restored)
> +		restore_iommu_context(iommu);
> +
> +	iommu_ser->intel.restored = 1;
> +	BUG_ON(iommu_for_each_preserved_device(_restore_used_domain_ids, iommu));
> +}
> +
>   /**
>    * intel_iommu_preserve_device() - Intel IOMMU callback to preserve device state
>    * @dev: Target device

Thanks,
baolu