Re: [PATCH 07/14] iommu/vt-d: Restore IOMMU state and reclaimed domain ids

[Samiullah Khawaja <skhawaja@google.com>]

Hi Vipin,

On Thu, Mar 19, 2026 at 01:54:27PM -0700, Vipin Sharma wrote:
>On Tue, Feb 03, 2026 at 10:09:41PM +0000, 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.
>
>Can this be two patches? One just restoring root table and second one to
>reclaim device ids.

This patch is restoring the state of the preserved IOMMU. The domain ids
are part of the iommu state and used in the context entries that are
being restored in this patch, that is why I kept these in one patch. I
will reword the subject of this patch and add a note on this in commit
message.
>
>> -static void init_translation_status(struct intel_iommu *iommu)
>> +static void init_translation_status(struct intel_iommu *iommu, bool restoring)
>>  {
>>  	u32 gsts;
>>
>>  	gsts = readl(iommu->reg + DMAR_GSTS_REG);
>> -	if (gsts & DMA_GSTS_TES)
>> +	if (!restoring && (gsts & DMA_GSTS_TES))
>>  		iommu->flags |= VTD_FLAG_TRANS_PRE_ENABLED;
>>  }
>
>I think we can just check in the caller, init_dmars(), and not call this
>function. We are already modifying that function, so no need to make
>changes here as well. WDYT?

Yes, I think we can move the if outside.
>
>>
>> @@ -670,10 +670,16 @@ void dmar_fault_dump_ptes(struct intel_iommu *iommu, u16 source_id,
>>  #endif
>>
>>  /* iommu handling */
>> -static int iommu_alloc_root_entry(struct intel_iommu *iommu)
>> +static int iommu_alloc_root_entry(struct intel_iommu *iommu, struct iommu_ser *restored_state)
>
>Nit: Since we are using iommu_ser at other places, I will recommend to
>keep it same variable name here as well.

Hmm... that is a good point. I think I will update the other places and
change it to restored_state/preserved_state to better reflect the
context.
>
>> @@ -1636,8 +1643,10 @@ static int __init init_dmars(void)
>>  						   intel_pasid_max_id);
>>  		}
>>
>> +		iommu_ser = iommu_get_preserved_data(iommu->reg_phys, IOMMU_INTEL);
>> +
>>  		intel_iommu_init_qi(iommu);
>> -		init_translation_status(iommu);
>> +		init_translation_status(iommu, !!iommu_ser);
>
>Just put 'if' here to avoid changes in init_translation_status().

Agreed.
>
>> +static int __restore_used_domain_ids(struct device_ser *ser, void *arg)
>
>Nit: Just curious, why two __?

I will remove these.
>
>> +{
>> +	int id = ser->domain_iommu_ser.did;
>> +	struct intel_iommu *iommu = arg;
>> +
>> +	ida_alloc_range(&iommu->domain_ida, id, id, GFP_ATOMIC);
>
>Should we check if allocation succeeded or not?

Good point. This should not fail as we are reserving these ids during
init. But I think I will add a WARN here.
>
>> +	return 0;
>> +}
>> +
>> +void intel_iommu_liveupdate_restore_root_table(struct intel_iommu *iommu,
>> +					       struct iommu_ser *iommu_ser)
>> +{
>> +	BUG_ON(!kho_restore_folio(iommu_ser->intel.root_table));
>> +	iommu->root_entry = __va(iommu_ser->intel.root_table);
>> +
>> +	restore_iommu_context(iommu);
>> +	iommu_for_each_preserved_device(__restore_used_domain_ids, iommu);
>> +	pr_info("Restored IOMMU[0x%llx] Root Table at: 0x%llx\n",
>> +		iommu->reg_phys, iommu_ser->intel.root_table);
>
>Should raw pointer values be printed like this?

I will remove this.
>