Re: [PATCH 06/14] iommu/vt-d: Implement device and iommu preserve/unpreserve ops

[Samiullah Khawaja <skhawaja@google.com>]

On Thu, Mar 19, 2026 at 09:04:31AM -0700, Vipin Sharma wrote:
>On Tue, Feb 03, 2026 at 10:09:40PM +0000, Samiullah Khawaja wrote:
>>  /*
>>   * Take a root_entry and return the Lower Context Table Pointer (LCTP)
>>   * if marked present.
>> @@ -2378,8 +2379,10 @@ void intel_iommu_shutdown(void)
>>  		/* Disable PMRs explicitly here. */
>>  		iommu_disable_protect_mem_regions(iommu);
>>
>> -		/* Make sure the IOMMUs are switched off */
>> -		iommu_disable_translation(iommu);
>> +		if (!__maybe_clean_unpreserved_context_entries(iommu)) {
>> +			/* Make sure the IOMMUs are switched off */
>> +			iommu_disable_translation(iommu);
>> +		}
>
>I think instead of having a "maybe" function where it might or might not
>clear the context entry, it will be simpler to just make if-else
>explicit here.
>
>@@ -2375,8 +2375,11 @@ void intel_iommu_shutdown(void)
>                /* Disable PMRs explicitly here. */
>                iommu_disable_protect_mem_regions(iommu);
>
>-               /* Make sure the IOMMUs are switched off */
>-               iommu_disable_translation(iommu);
>+               if (iommu->iommu.outgoing_preserved_state)

This is only defined when CONFIG_LIVEUPDATE is enabled and would need
the #ifdefs :(.

But I can add a function, iommu_outgoing_preserved_state() or
iommu_is_outgoing_preserved() that would return NULL or false
respectively when liveupdate is disabled, in the skeleton patch to check
this.

I will update this in the next revision.
>+                       clear_unpreserved_context_entries();
>+               else
>+                       /* Make sure the IOMMUs are switched off */
>+                       iommu_disable_translation(iommu);
>        }
> }
>
>clear_unpreserved_context_entries() will explicitly do what
>__maybe_clean_unpreserved_context_entries() is doing.
>
>>  	}
>>  }
>>
>> @@ -2902,6 +2905,38 @@ static const struct iommu_dirty_ops intel_second_stage_dirty_ops = {
>>  	.set_dirty_tracking = intel_iommu_set_dirty_tracking,
>>  };
>>
>> +#ifdef CONFIG_IOMMU_LIVEUPDATE
>> +static bool __maybe_clean_unpreserved_context_entries(struct intel_iommu *iommu)
>
>Nit: s/clean/clear, this matches domain_context_clear().

Agreed.
>
>> +{
>> +	struct device_domain_info *info;
>> +	struct pci_dev *pdev = NULL;
>> +
>> +	if (!iommu->iommu.outgoing_preserved_state)
>> +		return false;
>> +
>> +	for_each_pci_dev(pdev) {
>> +		info = dev_iommu_priv_get(&pdev->dev);
>> +		if (!info)
>> +			continue;
>> +
>> +		if (info->iommu != iommu)
>> +			continue;
>> +
>> +		if (dev_iommu_preserved_state(&pdev->dev))
>> +			continue;
>> +
>> +		domain_context_clear(info);
>> +	}
>> +
>> +	return true;
>> +}
>> +++ b/drivers/iommu/intel/liveupdate.c
>> @@ -0,0 +1,134 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/*
>> + * Copyright (C) 2025, Google LLC
>> + * Author: Samiullah Khawaja <skhawaja@google.com>
>> + */
>> +
>> +#define pr_fmt(fmt)    "iommu: liveupdate: " fmt

>Should this be 'DMAR: liveupdate' to be consistent with other files in
>intel iommu?

Agreed. Will change.
>
>> +
>> +#include <linux/kexec_handover.h>
>> +#include <linux/liveupdate.h>
>> +#include <linux/iommu-lu.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +
>> +#include "iommu.h"
>> +#include "../iommu-pages.h"
>> +
>> +static void unpreserve_iommu_context(struct intel_iommu *iommu, int end)
>
>Nit: Can this be unpreserve_iommu_context_table(), similar to existing
>free_context_table()? Same for corresponding preserve function

Agreed. I will change this.
>
>> +{
>> +	struct context_entry *context;
>> +	int i;
>> +
>> +	if (end < 0)
>> +		end = ROOT_ENTRY_NR;
>
>Can't we just pass ROOT_ENTRY_NR as end value instead of -1?

Yes, we can do that. Will update.
>

Thanks,
Sami