Re: [PATCH 1/8] iommu: Lift and generalize the STE/CD update code from SMMUv3

[Samiullah Khawaja <skhawaja@google.com>]

On Mon, Mar 09, 2026 at 02:06:41PM +0800, Lu Baolu wrote:
>From: Jason Gunthorpe <jgg@nvidia.com>
>
>Many IOMMU implementations store data structures in host memory that can
>be quite big. The iommu is able to DMA read the host memory using an
>atomic quanta, usually 64 or 128 bits, and will read an entry using
>multiple quanta reads.
>
>Updating the host memory datastructure entry while the HW is concurrently
>DMA'ing it is a little bit involved, but if you want to do this hitlessly,
>while never making the entry non-valid, then it becomes quite complicated.
>
>entry_sync is a library to handle this task. It works on the notion of
>"used bits" which reflect which bits the HW is actually sensitive to and
>which bits are ignored by hardware. Many hardware specifications say
>things like 'if mode is X then bits ABC are ignored'.
>
>Using the ignored bits entry_sync can often compute a series of ordered
>writes and flushes that will allow the entry to be updated while keeping
>it valid. If such an update is not possible then entry will be made
>temporarily non-valid.
>
>A 64 and 128 bit quanta version is provided to support existing iommus.
>
>Co-developed-by: Lu Baolu <baolu.lu@linux.intel.com>
>Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>---
> drivers/iommu/Kconfig               |  14 +++
> drivers/iommu/Makefile              |   1 +
> drivers/iommu/entry_sync.h          |  66 +++++++++++++
> drivers/iommu/entry_sync_template.h | 143 ++++++++++++++++++++++++++++
> drivers/iommu/entry_sync.c          |  68 +++++++++++++
> 5 files changed, 292 insertions(+)
> create mode 100644 drivers/iommu/entry_sync.h
> create mode 100644 drivers/iommu/entry_sync_template.h
> create mode 100644 drivers/iommu/entry_sync.c
>
>diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>index f86262b11416..2650c9fa125b 100644
>--- a/drivers/iommu/Kconfig
>+++ b/drivers/iommu/Kconfig
>@@ -145,6 +145,20 @@ config IOMMU_DEFAULT_PASSTHROUGH
>
> endchoice
>
>+config IOMMU_ENTRY_SYNC
>+	bool
>+	default n
>+
>+config IOMMU_ENTRY_SYNC64
>+	bool
>+	select IOMMU_ENTRY_SYNC
>+	default n
>+
>+config IOMMU_ENTRY_SYNC128
>+	bool
>+	select IOMMU_ENTRY_SYNC
>+	default n
>+
> config OF_IOMMU
> 	def_bool y
> 	depends on OF && IOMMU_API
>diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>index 0275821f4ef9..bd923995497a 100644
>--- a/drivers/iommu/Makefile
>+++ b/drivers/iommu/Makefile
>@@ -10,6 +10,7 @@ obj-$(CONFIG_IOMMU_API) += iommu-traces.o
> obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
> obj-$(CONFIG_IOMMU_DEBUGFS) += iommu-debugfs.o
> obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>+obj-$(CONFIG_IOMMU_ENTRY_SYNC) += entry_sync.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
> obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
>diff --git a/drivers/iommu/entry_sync.h b/drivers/iommu/entry_sync.h
>new file mode 100644
>index 000000000000..004d421c71c0
>--- /dev/null
>+++ b/drivers/iommu/entry_sync.h
>@@ -0,0 +1,66 @@
>+/* SPDX-License-Identifier: GPL-2.0-only */
>+/*
>+ * Many IOMMU implementations store data structures in host memory that can be
>+ * quite big. The iommu is able to DMA read the host memory using an atomic
>+ * quanta, usually 64 or 128 bits, and will read an entry using multiple quanta
>+ * reads.
>+ *
>+ * Updating the host memory datastructure entry while the HW is concurrently
>+ * DMA'ing it is a little bit involved, but if you want to do this hitlessly,
>+ * while never making the entry non-valid, then it becomes quite complicated.
>+ *
>+ * entry_sync is a library to handle this task. It works on the notion of "used
>+ * bits" which reflect which bits the HW is actually sensitive to and which bits
>+ * are ignored by hardware. Many hardware specifications say things like 'if
>+ * mode is X then bits ABC are ignored'.
>+ *
>+ * Using the ignored bits entry_sync can often compute a series of ordered
>+ * writes and flushes that will allow the entry to be updated while keeping it
>+ * valid. If such an update is not possible then entry will be made temporarily
>+ * non-valid.
>+ *
>+ * A 64 and 128 bit quanta version is provided to support existing iommus.
>+ */
>+#ifndef IOMMU_ENTRY_SYNC_H
>+#define IOMMU_ENTRY_SYNC_H
>+
>+#include <linux/types.h>
>+#include <linux/compiler.h>
>+#include <linux/bug.h>
>+
>+/* Caller allocates a stack array of this length to call entry_sync_write() */
>+#define ENTRY_SYNC_MEMORY_LEN(writer) ((writer)->num_quantas * 3)
>+
>+struct entry_sync_writer_ops64;
>+struct entry_sync_writer64 {
>+	const struct entry_sync_writer_ops64 *ops;
>+	size_t num_quantas;
>+	size_t vbit_quanta;
>+};
>+
>+struct entry_sync_writer_ops64 {
>+	void (*get_used)(const __le64 *entry, __le64 *used);
>+	void (*sync)(struct entry_sync_writer64 *writer);
>+};
>+
>+void entry_sync_write64(struct entry_sync_writer64 *writer, __le64 *entry,
>+			const __le64 *target, __le64 *memory,
>+			size_t memory_len);
>+
>+struct entry_sync_writer_ops128;
>+struct entry_sync_writer128 {
>+	const struct entry_sync_writer_ops128 *ops;
>+	size_t num_quantas;
>+	size_t vbit_quanta;
>+};
>+
>+struct entry_sync_writer_ops128 {
>+	void (*get_used)(const u128 *entry, u128 *used);
>+	void (*sync)(struct entry_sync_writer128 *writer);
>+};
>+
>+void entry_sync_write128(struct entry_sync_writer128 *writer, u128 *entry,
>+			 const u128 *target, u128 *memory,
>+			 size_t memory_len);
>+
>+#endif
>diff --git a/drivers/iommu/entry_sync_template.h b/drivers/iommu/entry_sync_template.h
>new file mode 100644
>index 000000000000..646f518b098e
>--- /dev/null
>+++ b/drivers/iommu/entry_sync_template.h
>@@ -0,0 +1,143 @@
>+/* SPDX-License-Identifier: GPL-2.0-only */
>+#include "entry_sync.h"
>+#include <linux/args.h>
>+#include <linux/bitops.h>
>+
>+#ifndef entry_sync_writer
>+#define entry_sync_writer entry_sync_writer64
>+#define quanta_t __le64
>+#define NS(name) CONCATENATE(name, 64)
>+#endif
>+
>+/*
>+ * Figure out if we can do a hitless update of entry to become target. Returns a
>+ * bit mask where 1 indicates that a quanta word needs to be set disruptively.
>+ * unused_update is an intermediate value of entry that has unused bits set to
>+ * their new values.
>+ */
>+static u8 NS(entry_quanta_diff)(struct entry_sync_writer *writer,
>+				const quanta_t *entry, const quanta_t *target,
>+				quanta_t *unused_update, quanta_t *memory)
>+{
>+	quanta_t *target_used = memory + writer->num_quantas * 1;
>+	quanta_t *cur_used = memory + writer->num_quantas * 2;
>+	u8 used_qword_diff = 0;
>+	unsigned int i;
>+
>+	writer->ops->get_used(entry, cur_used);
>+	writer->ops->get_used(target, target_used);
>+
>+	for (i = 0; i != writer->num_quantas; i++) {
>+		/*
>+		 * Check that masks are up to date, the make functions are not

nit: "the make functions" looks like a typo.
>+		 * allowed to set a bit to 1 if the used function doesn't say it
>+		 * is used.
>+		 */
>+		WARN_ON_ONCE(target[i] & ~target_used[i]);
>+
>+		/* Bits can change because they are not currently being used */
>+		unused_update[i] = (entry[i] & cur_used[i]) |
>+				   (target[i] & ~cur_used[i]);
>+		/*
>+		 * Each bit indicates that a used bit in a qword needs to be
>+		 * changed after unused_update is applied.
>+		 */
>+		if ((unused_update[i] & target_used[i]) != target[i])
>+			used_qword_diff |= 1 << i;
>+	}
>+	return used_qword_diff;
>+}
>+
>+/*
>+ * Update the entry to the target configuration. The transition from the current
>+ * entry to the target entry takes place over multiple steps that attempts to
>+ * make the transition hitless if possible. This function takes care not to
>+ * create a situation where the HW can perceive a corrupted entry. HW is only
>+ * required to have a quanta-bit atomicity with stores from the CPU, while
>+ * entries are many quanta bit values big.
>+ *
>+ * The difference between the current value and the target value is analyzed to
>+ * determine which of three updates are required - disruptive, hitless or no
>+ * change.
>+ *
>+ * In the most general disruptive case we can make any update in three steps:
>+ *  - Disrupting the entry (V=0)
>+ *  - Fill now unused quanta words, except qword 0 which contains V
>+ *  - Make qword 0 have the final value and valid (V=1) with a single 64
>+ *    bit store
>+ *
>+ * However this disrupts the HW while it is happening. There are several
>+ * interesting cases where a STE/CD can be updated without disturbing the HW
>+ * because only a small number of bits are changing (S1DSS, CONFIG, etc) or
>+ * because the used bits don't intersect. We can detect this by calculating how
>+ * many 64 bit values need update after adjusting the unused bits and skip the
>+ * V=0 process. This relies on the IGNORED behavior described in the
>+ * specification.
>+ */
>+void NS(entry_sync_write)(struct entry_sync_writer *writer, quanta_t *entry,
>+			  const quanta_t *target, quanta_t *memory,
>+			  size_t memory_len)
>+{
>+	quanta_t *unused_update = memory + writer->num_quantas * 0;
>+	u8 used_qword_diff;
>+
>+	if (WARN_ON(memory_len !=
>+		    ENTRY_SYNC_MEMORY_LEN(writer) * sizeof(*memory)))
>+		return;
>+
>+	used_qword_diff = NS(entry_quanta_diff)(writer, entry, target,
>+						unused_update, memory);
>+	if (hweight8(used_qword_diff) == 1) {
>+		/*
>+		 * Only one quanta needs its used bits to be changed. This is a
>+		 * hitless update, update all bits the current entry is ignoring
>+		 * to their new values, then update a single "critical quanta"
>+		 * to change the entry and finally 0 out any bits that are now
>+		 * unused in the target configuration.
>+		 */
>+		unsigned int critical_qword_index = ffs(used_qword_diff) - 1;
>+
>+		/*
>+		 * Skip writing unused bits in the critical quanta since we'll
>+		 * be writing it in the next step anyways. This can save a sync
>+		 * when the only change is in that quanta.
>+		 */
>+		unused_update[critical_qword_index] =
>+			entry[critical_qword_index];
>+		NS(entry_set)(writer, entry, unused_update, 0,
>+			      writer->num_quantas);
>+		NS(entry_set)(writer, entry, target, critical_qword_index, 1);
>+		NS(entry_set)(writer, entry, target, 0, writer->num_quantas);
>+	} else if (used_qword_diff) {
>+		/*
>+		 * At least two quantas need their inuse bits to be changed.
>+		 * This requires a breaking update, zero the V bit, write all
>+		 * qwords but 0, then set qword 0
>+		 */
>+		unused_update[writer->vbit_quanta] = 0;
>+		NS(entry_set)(writer, entry, unused_update, writer->vbit_quanta, 1);
>+
>+		if (writer->vbit_quanta != 0)
>+			NS(entry_set)(writer, entry, target, 0,
>+				      writer->vbit_quanta - 1);

Looking at the definition of the entry_set below, the last argument is
length. So if vbit_quanta 1 then it would write zero len. Shouldn't it
be writing quantas before the vbit_quanta?
>+		if (writer->vbit_quanta != writer->num_quantas)
>+			NS(entry_set)(writer, entry, target,
>+				      writer->vbit_quanta,
>+				      writer->num_quantas - 1);

Sami here, the last argument should not have "- 1".
>+
>+		NS(entry_set)(writer, entry, target, writer->vbit_quanta, 1);
>+	} else {
>+		/*
>+		 * No inuse bit changed. Sanity check that all unused bits are 0
>+		 * in the entry. The target was already sanity checked by
>+		 * entry_quanta_diff().
>+		 */
>+		WARN_ON_ONCE(NS(entry_set)(writer, entry, target, 0,
>+					   writer->num_quantas));
>+	}
>+}
>+EXPORT_SYMBOL(NS(entry_sync_write));
>+
>+#undef entry_sync_writer
>+#undef quanta_t
>+#undef NS
>diff --git a/drivers/iommu/entry_sync.c b/drivers/iommu/entry_sync.c
>new file mode 100644
>index 000000000000..48d31270dbba
>--- /dev/null
>+++ b/drivers/iommu/entry_sync.c
>@@ -0,0 +1,68 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * Helpers for drivers to update multi-quanta entries shared with HW without
>+ * races to minimize breaking changes.
>+ */
>+#include "entry_sync.h"
>+#include <linux/kconfig.h>
>+#include <linux/atomic.h>
>+
>+#if IS_ENABLED(CONFIG_IOMMU_ENTRY_SYNC64)
>+static bool entry_set64(struct entry_sync_writer64 *writer, __le64 *entry,
>+			const __le64 *target, unsigned int start,
>+			unsigned int len)
>+{
>+	bool changed = false;
>+	unsigned int i;
>+
>+	for (i = start; len != 0; len--, i++) {
>+		if (entry[i] != target[i]) {
>+			WRITE_ONCE(entry[i], target[i]);
>+			changed = true;
>+		}
>+	}
>+
>+	if (changed)
>+		writer->ops->sync(writer);
>+	return changed;
>+}
>+
>+#define entry_sync_writer entry_sync_writer64
>+#define quanta_t __le64
>+#define NS(name) CONCATENATE(name, 64)
>+#include "entry_sync_template.h"
>+#endif
>+
>+#if IS_ENABLED(CONFIG_IOMMU_ENTRY_SYNC128)
>+static bool entry_set128(struct entry_sync_writer128 *writer, u128 *entry,
>+			 const u128 *target, unsigned int start,
>+			 unsigned int len)
>+{
>+	bool changed = false;
>+	unsigned int i;
>+
>+	for (i = start; len != 0; len--, i++) {
>+		if (entry[i] != target[i]) {
>+			/*
>+			 * Use cmpxchg128 to generate an indivisible write from
>+			 * the CPU to DMA'able memory. This must ensure that HW
>+			 * sees either the new or old 128 bit value and not
>+			 * something torn. As updates are serialized by a
>+			 * spinlock, we use the local (unlocked) variant to
>+			 * avoid unnecessary bus locking overhead.
>+			 */
>+			cmpxchg128_local(&entry[i], entry[i], target[i]);
>+			changed = true;
>+		}
>+	}
>+
>+	if (changed)
>+		writer->ops->sync(writer);
>+	return changed;
>+}
>+
>+#define entry_sync_writer entry_sync_writer128
>+#define quanta_t u128
>+#define NS(name) CONCATENATE(name, 128)
>+#include "entry_sync_template.h"
>+#endif
>-- 
>2.43.0
>