Re: [PATCH v2 07/16] iommu/vt-d: Implement device and iommu preserve/unpreserve ops

[Samiullah Khawaja <skhawaja@google.com>]

On Thu, May 07, 2026 at 02:25:14PM +0800, Baolu Lu wrote:
>On 4/28/26 01:56, Samiullah Khawaja wrote:
>>Add implementation of the device and iommu presevation in a separate
>>file. Also set the device and iommu preserve/unpreserve ops in the
>>struct iommu_ops.
>>
>>During normal shutdown the iommu translation is disabled. Since the root
>>table is preserved during live update, it needs to be cleaned up and the
>>context entries of the unpreserved devices need to be cleared.
>
>This is not related to preserve/unpreserve ops and could be made in a
>separated patch?

Agreed. I will move this stuff to a separate patch.
>
>>
>>Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>>---
>>  MAINTAINERS                      |   1 +
>>  drivers/iommu/intel/Makefile     |   1 +
>>  drivers/iommu/intel/iommu.c      |  52 +++++++++++-
>>  drivers/iommu/intel/iommu.h      |  28 +++++++
>>  drivers/iommu/intel/liveupdate.c | 139 +++++++++++++++++++++++++++++++
>>  drivers/iommu/iommu.c            |  18 ++++
>>  include/linux/iommu-liveupdate.h |  10 +++
>>  include/linux/iommu.h            |  14 ++++
>>  include/linux/kho/abi/iommu.h    |  18 ++++
>>  9 files changed, 277 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/iommu/intel/liveupdate.c
>>
>>diff --git a/MAINTAINERS b/MAINTAINERS
>>index 980041955abc..9f5c02c6c8c1 100644
>>--- a/MAINTAINERS
>>+++ b/MAINTAINERS
>>@@ -13495,6 +13495,7 @@ M:	Samiullah Khawaja <skhawaja@google.com>
>>  R:	Pranjal Shrivastava <praan@google.com>
>>  L:	iommu@lists.linux.dev
>>  S:	Maintained
>>+F:	drivers/iommu/intel/liveupdate.c
>
>This file is deeply integrated into the Intel IOMMU driver. Maintaining
>it separately from the rest of the driver would add unnecessary
>complexity. I suggest merging this entry into the existing Intel IOMMU
>block so they can be managed together.

I understand your concern. This file is driver specific and should
definitely be routed through your tree.

My goal with listing it here wasn't to change the merge path, but rather
to ensure I am responsible for the liveupdate-specific logic in this
file. Because this file acts as the backend to the generic Live Update
framework, keeping it tied to the Liveupdate IOMMU entry helps guarantee
that future changes stay architecturally aligned and tested across the
whole liveupdate feature.

However, I think I will add a dedicated 'IOMMU VT-d LIVEUPDATE' block
where you are also listed as maintainer, and the patches for this file
should absolutely continue to flow through the Intel IOMMU tree.
>
>>  F:	drivers/iommu/liveupdate.c
>>  F:	include/linux/iommu-liveupdate.h
>>  F:	include/linux/kho/abi/iommu.h
>>diff --git a/drivers/iommu/intel/Makefile b/drivers/iommu/intel/Makefile
>>index ada651c4a01b..d38fc101bc35 100644
>>+

[snip]
>>+static void unpreserve_iommu_context_table(struct intel_iommu *iommu, int end)
>>+{
>>+	struct context_entry *context;
>>+	int i;
>>+
>>+	for (i = 0; i < end; i++) {
>>+		context = iommu_context_addr(iommu, i, 0, 0);
>>+		if (context)
>>+			iommu_unpreserve_page(context);
>>+
>>+		if (!sm_supported(iommu))
>>+			continue;
>>+
>>+		context = iommu_context_addr(iommu, i, 0x80, 0);
>>+		if (context)
>>+			iommu_unpreserve_page(context);
>>+	}
>>+}
>>+
>>+static int preserve_iommu_context_table(struct intel_iommu *iommu)
>
>Since this function preserves all context tables, should it be renamed
>to preserve_iommu_context_tables()?

Agreed. Will change this.
>
>>+{
>>+	struct context_entry *context;
>>+	int ret;
>>+	int i;
>>+
>>+	for (i = 0; i < ROOT_ENTRY_NR; i++) {
>>+		/*
>>+		 * Alloc the context tables now to make sure the iommu unit is
>>+		 * properly preserved. These might stay unused and wastes around
>>+		 * 32MB max in scalable mode.
>>+		 */
>
>Instead of allocating and preserving context tables for all root entries
>(as noted, can waste up to 32MB), could we restrict this only to the
>entries possibly in use by active PCI devices?

I think the hotplug devices or VFs created through SR-IOV will be missed
that way. Lets say device A is preserved and the associated iommu is
also preserved. And then a new device B is hotplugged and preserved,
then the context table for that will be missed.

Since we don't track the context_tables that are preserved, there is no
way to incremently preserve the new-ones. Let me look into the behaviour
of KHO, maybe we can make the preserve call idempotent and do these
incrementally.
>
>>+		spin_lock(&iommu->lock);
>>+		context = iommu_context_addr(iommu, i, 0, 1);
>>+		spin_unlock(&iommu->lock);
>>+		if (!context) {
>>+			ret = -ENOMEM;
>>+			goto error;
>>+		}

[snip]
>>diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>index 1c424b32c5fc..999be5127c65 100644
>>--- a/include/linux/iommu.h
>>+++ b/include/linux/iommu.h
>>@@ -1207,6 +1207,20 @@ static inline void *dev_iommu_priv_get(struct device *dev)
>>  void dev_iommu_priv_set(struct device *dev, void *priv);
>>+typedef int (*iommu_dev_iter_fn)(struct device *dev,
>>+				 struct iommu_device *iommu, void *arg);
>>+
>>+/**
>>+ * struct iommu_dev_iter - Iterator for devices attached to an IOMMU
>>+ */
>>+struct iommu_dev_iter {
>>+	struct iommu_device *iommu;
>>+	iommu_dev_iter_fn fn;
>>+	void *arg;
>>+};
>>+
>>+void iommu_for_each_dev(struct iommu_dev_iter *iter);
>
>Is a generic helper necessary here? I am concerned about potential races
>with concurrent operations especially probe_device or release_device.
>And also the hot-plug/unplug scenarios?
>
>Since the current preservation logic is limited to PCI devices, it might
>be safer and simpler to use the existing for_each_pci_dev() for this
>specific case instead of introducing a new generic helper?

The helper is used late during shutdown to clean up context entries for
devices that are not preserved. During this phase, the system is
quiescent and hotplug is disabled, so there are no races with concurrent
probe or release operations.

I was using for_each_pci_dev() in previous version, but the iterator is
needed to clear context entries of both PCI and non-PCI devices that are
unpreserved.

Please also see related comments on previous version:
https://lore.kernel.org/all/ab3R54-kyHGDEW9L@google.com/
>
>>+
>>  extern struct mutex iommu_probe_device_lock;
>>  int iommu_probe_device(struct device *dev);

[snip]
>>+	};
>>  } __packed;
>>  /**
>
>Thanks,
>baolu
>

Thanks,
Sami