[PATCH v8 0/7] Add non-strict mode support for iommu-dma

[PATCH v8 0/7] Add non-strict mode support for iommu-dma

[Robin Murphy]

Hi all,

Hopefully this is the last spin of the series - I've now dropped my light
touch and fixed up all the various prose text, plus implemented the proper
quirk support for short-descriptor because it's actually just a trivial
cut-and-paste job.

Robin.


Robin Murphy (2):
  iommu/io-pgtable-arm-v7s: Add support for non-strict mode
  iommu/arm-smmu: Support non-strict mode

Zhen Lei (5):
  iommu/arm-smmu-v3: Implement flush_iotlb_all hook
  iommu/dma: Add support for non-strict mode
  iommu: Add "iommu.strict" command line option
  iommu/io-pgtable-arm: Add support for non-strict mode
  iommu/arm-smmu-v3: Add support for non-strict mode

 .../admin-guide/kernel-parameters.txt         | 12 +++
 drivers/iommu/arm-smmu-v3.c                   | 89 +++++++++++++-----
 drivers/iommu/arm-smmu.c                      | 93 +++++++++++++------
 drivers/iommu/dma-iommu.c                     | 32 ++++++-
 drivers/iommu/io-pgtable-arm-v7s.c            | 11 ++-
 drivers/iommu/io-pgtable-arm.c                | 14 ++-
 drivers/iommu/io-pgtable.h                    |  5 +
 drivers/iommu/iommu.c                         | 23 +++++
 include/linux/iommu.h                         |  1 +
 9 files changed, 225 insertions(+), 55 deletions(-)

-- 
2.19.0.dirty

[PATCH v8 1/7] iommu/arm-smmu-v3: Implement flush_iotlb_all hook

[Robin Murphy]

From: Zhen Lei <thunder.leizhen@huawei.com>

.flush_iotlb_all is currently stubbed to arm_smmu_iotlb_sync() since the
only time it would ever need to actually do anything is for callers
doing their own explicit batching, e.g.:

	iommu_unmap_fast(domain, ...);
	iommu_unmap_fast(domain, ...);
	iommu_iotlb_flush_all(domain, ...);

where since io-pgtable still issues the TLBI commands implicitly in the
unmap instead of implementing .iotlb_range_add, the "flush" only needs
to ensure completion of those already-in-flight invalidations.

However, we're about to start using it in anger with flush queues, so
let's get a proper implementation wired up.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
[rm: document why it wasn't a bug]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu-v3.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index e395f1ff3f81..f10c852479fc 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1781,6 +1781,14 @@ arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova, size_t size)
 	return ops->unmap(ops, iova, size);
 }
 
+static void arm_smmu_flush_iotlb_all(struct iommu_domain *domain)
+{
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+
+	if (smmu_domain->smmu)
+		arm_smmu_tlb_inv_context(smmu_domain);
+}
+
 static void arm_smmu_iotlb_sync(struct iommu_domain *domain)
 {
 	struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
@@ -2008,7 +2016,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.attach_dev		= arm_smmu_attach_dev,
 	.map			= arm_smmu_map,
 	.unmap			= arm_smmu_unmap,
-	.flush_iotlb_all	= arm_smmu_iotlb_sync,
+	.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
 	.iotlb_sync		= arm_smmu_iotlb_sync,
 	.iova_to_phys		= arm_smmu_iova_to_phys,
 	.add_device		= arm_smmu_add_device,
-- 
2.19.0.dirty

[PATCH v8 2/7] iommu/dma: Add support for non-strict mode

[Robin Murphy]

From: Zhen Lei <thunder.leizhen@huawei.com>

With the flush queue infrastructure already abstracted into IOVA
domains, hooking it up in iommu-dma is pretty simple. Since there is a
degree of dependency on the IOMMU driver knowing what to do to play
along, we key the whole thing off a domain attribute which will be set
on default DMA ops domains to request non-strict invalidation. That way,
drivers can indicate the appropriate support by acknowledging the
attribute, and we can easily fall back to strict invalidation otherwise.

The flush queue callback needs a handle on the iommu_domain which owns
our cookie, so we have to add a pointer back to that, but neatly, that's
also sufficient to indicate whether we're using a flush queue or not,
and thus which way to release IOVAs. The only slight subtlety is
switching __iommu_dma_unmap() from calling iommu_unmap() to explicit
iommu_unmap_fast()/iommu_tlb_sync() so that we can elide the sync
entirely in non-strict mode.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
[rm: convert to domain attribute, tweak comments and commit message]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v8:
 - Rewrite commit message/comments
 - Don't initialise "attr" unnecessarily
 - Rename "domain" to "fq_domain" for clarity
 - Don't let init_iova_flush_queue() be called more than once

 drivers/iommu/dma-iommu.c | 32 +++++++++++++++++++++++++++++++-
 include/linux/iommu.h     |  1 +
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 511ff9a1d6d9..cc1bf786cfac 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -55,6 +55,9 @@ struct iommu_dma_cookie {
 	};
 	struct list_head		msi_page_list;
 	spinlock_t			msi_lock;
+
+	/* Domain for flush queue callback; NULL if flush queue not in use */
+	struct iommu_domain		*fq_domain;
 };
 
 static inline size_t cookie_msi_granule(struct iommu_dma_cookie *cookie)
@@ -257,6 +260,20 @@ static int iova_reserve_iommu_regions(struct device *dev,
 	return ret;
 }
 
+static void iommu_dma_flush_iotlb_all(struct iova_domain *iovad)
+{
+	struct iommu_dma_cookie *cookie;
+	struct iommu_domain *domain;
+
+	cookie = container_of(iovad, struct iommu_dma_cookie, iovad);
+	domain = cookie->fq_domain;
+	/*
+	 * The IOMMU driver supporting DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
+	 * implies that ops->flush_iotlb_all must be non-NULL.
+	 */
+	domain->ops->flush_iotlb_all(domain);
+}
+
 /**
  * iommu_dma_init_domain - Initialise a DMA mapping domain
  * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
@@ -275,6 +292,7 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
 	unsigned long order, base_pfn, end_pfn;
+	int attr;
 
 	if (!cookie || cookie->type != IOMMU_DMA_IOVA_COOKIE)
 		return -EINVAL;
@@ -308,6 +326,13 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 	}
 
 	init_iova_domain(iovad, 1UL << order, base_pfn);
+
+	if (!cookie->fq_domain && !iommu_domain_get_attr(domain,
+			DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, &attr) && attr) {
+		cookie->fq_domain = domain;
+		init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all, NULL);
+	}
+
 	if (!dev)
 		return 0;
 
@@ -393,6 +418,9 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie *cookie,
 	/* The MSI case is only ever cleaning up its most recent allocation */
 	if (cookie->type == IOMMU_DMA_MSI_COOKIE)
 		cookie->msi_iova -= size;
+	else if (cookie->fq_domain)	/* non-strict mode */
+		queue_iova(iovad, iova_pfn(iovad, iova),
+				size >> iova_shift(iovad), 0);
 	else
 		free_iova_fast(iovad, iova_pfn(iovad, iova),
 				size >> iova_shift(iovad));
@@ -408,7 +436,9 @@ static void __iommu_dma_unmap(struct iommu_domain *domain, dma_addr_t dma_addr,
 	dma_addr -= iova_off;
 	size = iova_align(iovad, size + iova_off);
 
-	WARN_ON(iommu_unmap(domain, dma_addr, size) != size);
+	WARN_ON(iommu_unmap_fast(domain, dma_addr, size) != size);
+	if (!cookie->fq_domain)
+		iommu_tlb_sync(domain);
 	iommu_dma_free_iova(cookie, dma_addr, size);
 }
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 87994c265bf5..decabe8e8dbe 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -124,6 +124,7 @@ enum iommu_attr {
 	DOMAIN_ATTR_FSL_PAMU_ENABLE,
 	DOMAIN_ATTR_FSL_PAMUV1,
 	DOMAIN_ATTR_NESTING,	/* two stages of translation */
+	DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
 	DOMAIN_ATTR_MAX,
 };
 
-- 
2.19.0.dirty

[PATCH v8 3/7] iommu: Add "iommu.strict" command line option

[Robin Murphy]

From: Zhen Lei <thunder.leizhen@huawei.com>

Add a generic command line option to enable lazy unmapping via IOVA
flush queues, which will initally be suuported by iommu-dma. This echoes
the semantics of "intel_iommu=strict" (albeit with the opposite default
value), but in the driver-agnostic fashion of "iommu.passthrough".

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
[rm: move handling out of SMMUv3 driver, clean up documentation]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v8:
 - Rename "non-strict" to "strict" to better match existing options
 - Rewrite doc text/commit message
 - Downgrade boot-time message from warn/taint to info

 .../admin-guide/kernel-parameters.txt         | 12 ++++++++++
 drivers/iommu/iommu.c                         | 23 +++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 9871e649ffef..92ae12aeabf4 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1749,6 +1749,18 @@
 		nobypass	[PPC/POWERNV]
 			Disable IOMMU bypass, using IOMMU for PCI devices.
 
+	iommu.strict=	[ARM64] Configure TLB invalidation behaviour
+			Format: { "0" | "1" }
+			0 - Lazy mode.
+			  Request that DMA unmap operations use deferred
+			  invalidation of hardware TLBs, for increased
+			  throughput at the cost of reduced device isolation.
+			  Will fall back to strict mode if not supported by
+			  the relevant IOMMU driver.
+			1 - Strict mode (default).
+			  DMA unmap operations invalidate IOMMU hardware TLBs
+			  synchronously.
+
 	iommu.passthrough=
 			[ARM64] Configure DMA to bypass the IOMMU by default.
 			Format: { "0" | "1" }
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8c15c5980299..02b6603f0616 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -41,6 +41,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
 #else
 static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
 #endif
+static bool iommu_dma_strict __read_mostly = true;
 
 struct iommu_callback_data {
 	const struct iommu_ops *ops;
@@ -131,6 +132,21 @@ static int __init iommu_set_def_domain_type(char *str)
 }
 early_param("iommu.passthrough", iommu_set_def_domain_type);
 
+static int __init iommu_dma_setup(char *str)
+{
+	int ret;
+
+	ret = kstrtobool(str, &iommu_dma_strict);
+	if (ret)
+		return ret;
+
+	if (!iommu_dma_strict)
+		pr_info("Enabling deferred TLB invalidation for DMA; protection against malicious/malfunctioning devices may be reduced.\n");
+
+	return 0;
+}
+early_param("iommu.strict", iommu_dma_setup);
+
 static ssize_t iommu_group_attr_show(struct kobject *kobj,
 				     struct attribute *__attr, char *buf)
 {
@@ -1072,6 +1088,13 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 		group->default_domain = dom;
 		if (!group->domain)
 			group->domain = dom;
+
+		if (dom && !iommu_dma_strict) {
+			int attr = 1;
+			iommu_domain_set_attr(dom,
+					      DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
+					      &attr);
+		}
 	}
 
 	ret = iommu_group_add_device(group, dev);
-- 
2.19.0.dirty

[PATCH v8 4/7] iommu/io-pgtable-arm: Add support for non-strict mode

[Robin Murphy]

From: Zhen Lei <thunder.leizhen@huawei.com>

Non-strict mode is simply a case of skipping 'regular' leaf TLBIs, since
the sync is already factored out into ops->iotlb_sync at the core API
level. Non-leaf invalidations where we change the page table structure
itself still have to be issued synchronously in order to maintain walk
caches correctly.

To save having to reason about it too much, make sure the invalidation
in arm_lpae_split_blk_unmap() just performs its own unconditional sync
to minimise the window in which we're technically violating the break-
before-make requirement on a live mapping. This might work out redundant
with an outer-level sync for strict unmaps, but we'll never be splitting
blocks on a DMA fastpath anyway.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
[rm: tweak comment, commit message, split_blk_unmap logic and barriers]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v8: Add barrier for the fiddly cross-cpu flush case

 drivers/iommu/io-pgtable-arm.c | 14 ++++++++++++--
 drivers/iommu/io-pgtable.h     |  5 +++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 2f79efd16a05..237cacd4a62b 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -576,6 +576,7 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
 		tablep = iopte_deref(pte, data);
 	} else if (unmap_idx >= 0) {
 		io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
+		io_pgtable_tlb_sync(&data->iop);
 		return size;
 	}
 
@@ -609,6 +610,13 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
 			io_pgtable_tlb_sync(iop);
 			ptep = iopte_deref(pte, data);
 			__arm_lpae_free_pgtable(data, lvl + 1, ptep);
+		} else if (iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT) {
+			/*
+			 * Order the PTE update against queueing the IOVA, to
+			 * guarantee that a flush callback from a different CPU
+			 * has observed it before the TLBIALL can be issued.
+			 */
+			smp_wmb();
 		} else {
 			io_pgtable_tlb_add_flush(iop, iova, size, size, true);
 		}
@@ -771,7 +779,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 	u64 reg;
 	struct arm_lpae_io_pgtable *data;
 
-	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA))
+	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA |
+			    IO_PGTABLE_QUIRK_NON_STRICT))
 		return NULL;
 
 	data = arm_lpae_alloc_pgtable(cfg);
@@ -863,7 +872,8 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg *cfg, void *cookie)
 	struct arm_lpae_io_pgtable *data;
 
 	/* The NS quirk doesn't apply at stage 2 */
-	if (cfg->quirks & ~IO_PGTABLE_QUIRK_NO_DMA)
+	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_NO_DMA |
+			    IO_PGTABLE_QUIRK_NON_STRICT))
 		return NULL;
 
 	data = arm_lpae_alloc_pgtable(cfg);
diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
index 2df79093cad9..47d5ae559329 100644
--- a/drivers/iommu/io-pgtable.h
+++ b/drivers/iommu/io-pgtable.h
@@ -71,12 +71,17 @@ struct io_pgtable_cfg {
 	 *	be accessed by a fully cache-coherent IOMMU or CPU (e.g. for a
 	 *	software-emulated IOMMU), such that pagetable updates need not
 	 *	be treated as explicit DMA data.
+	 *
+	 * IO_PGTABLE_QUIRK_NON_STRICT: Skip issuing synchronous leaf TLBIs
+	 *	on unmap, for DMA domains using the flush queue mechanism for
+	 *	delayed invalidation.
 	 */
 	#define IO_PGTABLE_QUIRK_ARM_NS		BIT(0)
 	#define IO_PGTABLE_QUIRK_NO_PERMS	BIT(1)
 	#define IO_PGTABLE_QUIRK_TLBI_ON_MAP	BIT(2)
 	#define IO_PGTABLE_QUIRK_ARM_MTK_4GB	BIT(3)
 	#define IO_PGTABLE_QUIRK_NO_DMA		BIT(4)
+	#define IO_PGTABLE_QUIRK_NON_STRICT	BIT(5)
 	unsigned long			quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
-- 
2.19.0.dirty

[PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode

[Robin Murphy]

From: Zhen Lei <thunder.leizhen@huawei.com>

Now that io-pgtable knows how to dodge strict TLB maintenance, all
that's left to do is bridge the gap between the IOMMU core requesting
DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE for default domains, and showing the
appropriate IO_PGTABLE_QUIRK_NON_STRICT flag to alloc_io_pgtable_ops().

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
[rm: convert to domain attribute, tweak commit message]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v8:
 - Use nested switches for attrs
 - Document barrier semantics

 drivers/iommu/arm-smmu-v3.c | 79 ++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f10c852479fc..db402e8b068b 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -612,6 +612,7 @@ struct arm_smmu_domain {
 	struct mutex			init_mutex; /* Protects smmu pointer */
 
 	struct io_pgtable_ops		*pgtbl_ops;
+	bool				non_strict;
 
 	enum arm_smmu_domain_stage	stage;
 	union {
@@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
 	}
 
+	/*
+	 * NOTE: when io-pgtable is in non-strict mode, we may get here with
+	 * PTEs previously cleared by unmaps on the current CPU not yet visible
+	 * to the SMMU. We are relying on the DSB implicit in queue_inc_prod()
+	 * to guarantee those are observed before the TLBI. Do be careful, 007.
+	 */
 	arm_smmu_cmdq_issue_cmd(smmu, &cmd);
 	__arm_smmu_tlb_sync(smmu);
 }
@@ -1633,6 +1640,9 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 	if (smmu->features & ARM_SMMU_FEAT_COHERENCY)
 		pgtbl_cfg.quirks = IO_PGTABLE_QUIRK_NO_DMA;
 
+	if (smmu_domain->non_strict)
+		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
+
 	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
 	if (!pgtbl_ops)
 		return -ENOMEM;
@@ -1934,15 +1944,27 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
-	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
-		return -EINVAL;
-
-	switch (attr) {
-	case DOMAIN_ATTR_NESTING:
-		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
-		return 0;
+	switch (domain->type) {
+	case IOMMU_DOMAIN_UNMANAGED:
+		switch (attr) {
+		case DOMAIN_ATTR_NESTING:
+			*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
+			return 0;
+		default:
+			return -ENODEV;
+		}
+		break;
+	case IOMMU_DOMAIN_DMA:
+		switch (attr) {
+		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
+			*(int *)data = smmu_domain->non_strict;
+			return 0;
+		default:
+			return -ENODEV;
+		}
+		break;
 	default:
-		return -ENODEV;
+		return -EINVAL;
 	}
 }
 
@@ -1952,26 +1974,37 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	int ret = 0;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
-	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
-		return -EINVAL;
-
 	mutex_lock(&smmu_domain->init_mutex);
 
-	switch (attr) {
-	case DOMAIN_ATTR_NESTING:
-		if (smmu_domain->smmu) {
-			ret = -EPERM;
-			goto out_unlock;
+	switch (domain->type) {
+	case IOMMU_DOMAIN_UNMANAGED:
+		switch (attr) {
+		case DOMAIN_ATTR_NESTING:
+			if (smmu_domain->smmu) {
+				ret = -EPERM;
+				goto out_unlock;
+			}
+
+			if (*(int *)data)
+				smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
+			else
+				smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+			break;
+		default:
+			ret = -ENODEV;
+		}
+		break;
+	case IOMMU_DOMAIN_DMA:
+		switch(attr) {
+		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
+			smmu_domain->non_strict = *(int *)data;
+			break;
+		default:
+			ret = -ENODEV;
 		}
-
-		if (*(int *)data)
-			smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
-		else
-			smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
-
 		break;
 	default:
-		ret = -ENODEV;
+		ret = -EINVAL;
 	}
 
 out_unlock:
-- 
2.19.0.dirty

[PATCH v8 6/7] iommu/io-pgtable-arm-v7s: Add support for non-strict mode

[Robin Murphy]

As for LPAE, it's simply a case of skipping the leaf invalidation for a
regular unmap, and ensuring that the one in split_blk_unmap() is paired
with an explicit sync ASAP rather than relying on one which might only
eventually happen way down the line.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v8: Do this properly instead of just hacking around not doing it
    (look, it turns out barely any more complicated!)

 drivers/iommu/io-pgtable-arm-v7s.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index b5948ba6b3b3..445c3bde0480 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -587,6 +587,7 @@ static size_t arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
 	}
 
 	io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
+	io_pgtable_tlb_sync(&data->iop);
 	return size;
 }
 
@@ -642,6 +643,13 @@ static size_t __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
 				io_pgtable_tlb_sync(iop);
 				ptep = iopte_deref(pte[i], lvl);
 				__arm_v7s_free_table(ptep, lvl + 1, data);
+			} else if (iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT) {
+				/*
+				 * Order the PTE update against queueing the IOVA, to
+				 * guarantee that a flush callback from a different CPU
+				 * has observed it before the TLBIALL can be issued.
+				 */
+				smp_wmb();
 			} else {
 				io_pgtable_tlb_add_flush(iop, iova, blk_size,
 							 blk_size, true);
@@ -712,7 +720,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
 			    IO_PGTABLE_QUIRK_NO_PERMS |
 			    IO_PGTABLE_QUIRK_TLBI_ON_MAP |
 			    IO_PGTABLE_QUIRK_ARM_MTK_4GB |
-			    IO_PGTABLE_QUIRK_NO_DMA))
+			    IO_PGTABLE_QUIRK_NO_DMA |
+			    IO_PGTABLE_QUIRK_NON_STRICT))
 		return NULL;
 
 	/* If ARM_MTK_4GB is enabled, the NO_PERMS is also expected. */
-- 
2.19.0.dirty

[PATCH v8 7/7] iommu/arm-smmu: Support non-strict mode

[Robin Murphy]

All we need is to wire up .flush_iotlb_all properly and implement the
domain attribute, and iommu-dma and io-pgtable will do the rest for us.
The only real subtlety is documenting the barrier semantics we're
introducing between io-pgtable and the drivers for non-strict flushes.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v8:
 - Use nested switches for attrs
 - Fix barriers

 drivers/iommu/arm-smmu.c | 93 ++++++++++++++++++++++++++++------------
 1 file changed, 66 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index fd1b80ef9490..79ece565d96d 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -246,6 +246,7 @@ struct arm_smmu_domain {
 	const struct iommu_gather_ops	*tlb_ops;
 	struct arm_smmu_cfg		cfg;
 	enum arm_smmu_domain_stage	stage;
+	bool				non_strict;
 	struct mutex			init_mutex; /* Protects smmu pointer */
 	spinlock_t			cb_lock; /* Serialises ATS1* ops and TLB syncs */
 	struct iommu_domain		domain;
@@ -447,7 +448,11 @@ static void arm_smmu_tlb_inv_context_s1(void *cookie)
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	void __iomem *base = ARM_SMMU_CB(smmu_domain->smmu, cfg->cbndx);
 
-	writel_relaxed(cfg->asid, base + ARM_SMMU_CB_S1_TLBIASID);
+	/*
+	 * NOTE: this is not a relaxed write; it needs to guarantee that PTEs
+	 * cleared by the current CPU are visible to the SMMU before the TLBI.
+	 */
+	writel(cfg->asid, base + ARM_SMMU_CB_S1_TLBIASID);
 	arm_smmu_tlb_sync_context(cookie);
 }
 
@@ -457,7 +462,8 @@ static void arm_smmu_tlb_inv_context_s2(void *cookie)
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	void __iomem *base = ARM_SMMU_GR0(smmu);
 
-	writel_relaxed(smmu_domain->cfg.vmid, base + ARM_SMMU_GR0_TLBIVMID);
+	/* NOTE: see above */
+	writel(smmu_domain->cfg.vmid, base + ARM_SMMU_GR0_TLBIVMID);
 	arm_smmu_tlb_sync_global(smmu);
 }
 
@@ -863,6 +869,9 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	if (smmu->features & ARM_SMMU_FEAT_COHERENT_WALK)
 		pgtbl_cfg.quirks = IO_PGTABLE_QUIRK_NO_DMA;
 
+	if (smmu_domain->non_strict)
+		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
+
 	smmu_domain->smmu = smmu;
 	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
 	if (!pgtbl_ops) {
@@ -1252,6 +1261,14 @@ static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
 	return ops->unmap(ops, iova, size);
 }
 
+static void arm_smmu_flush_iotlb_all(struct iommu_domain *domain)
+{
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+
+	if (smmu_domain->tlb_ops)
+		smmu_domain->tlb_ops->tlb_flush_all(smmu_domain);
+}
+
 static void arm_smmu_iotlb_sync(struct iommu_domain *domain)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
@@ -1470,15 +1487,27 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
-	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
-		return -EINVAL;
-
-	switch (attr) {
-	case DOMAIN_ATTR_NESTING:
-		*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
-		return 0;
+	switch(domain->type) {
+	case IOMMU_DOMAIN_UNMANAGED:
+		switch (attr) {
+		case DOMAIN_ATTR_NESTING:
+			*(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
+			return 0;
+		default:
+			return -ENODEV;
+		}
+		break;
+	case IOMMU_DOMAIN_DMA:
+		switch (attr) {
+		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
+			*(int *)data = smmu_domain->non_strict;
+			return 0;
+		default:
+			return -ENODEV;
+		}
+		break;
 	default:
-		return -ENODEV;
+		return -EINVAL;
 	}
 }
 
@@ -1488,28 +1517,38 @@ static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
 	int ret = 0;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
-	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
-		return -EINVAL;
-
 	mutex_lock(&smmu_domain->init_mutex);
 
-	switch (attr) {
-	case DOMAIN_ATTR_NESTING:
-		if (smmu_domain->smmu) {
-			ret = -EPERM;
-			goto out_unlock;
+	switch(domain->type) {
+	case IOMMU_DOMAIN_UNMANAGED:
+		switch (attr) {
+		case DOMAIN_ATTR_NESTING:
+			if (smmu_domain->smmu) {
+				ret = -EPERM;
+				goto out_unlock;
+			}
+
+			if (*(int *)data)
+				smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
+			else
+				smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+			break;
+		default:
+			ret = -ENODEV;
+		}
+		break;
+	case IOMMU_DOMAIN_DMA:
+		switch (attr) {
+		case DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE:
+			smmu_domain->non_strict = *(int *)data;
+			break;
+		default:
+			ret = -ENODEV;
 		}
-
-		if (*(int *)data)
-			smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
-		else
-			smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
-
 		break;
 	default:
-		ret = -ENODEV;
+		ret = -EINVAL;
 	}
-
 out_unlock:
 	mutex_unlock(&smmu_domain->init_mutex);
 	return ret;
@@ -1562,7 +1601,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.attach_dev		= arm_smmu_attach_dev,
 	.map			= arm_smmu_map,
 	.unmap			= arm_smmu_unmap,
-	.flush_iotlb_all	= arm_smmu_iotlb_sync,
+	.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
 	.iotlb_sync		= arm_smmu_iotlb_sync,
 	.iova_to_phys		= arm_smmu_iova_to_phys,
 	.add_device		= arm_smmu_add_device,
-- 
2.19.0.dirty

Re: [PATCH v8 0/7] Add non-strict mode support for iommu-dma

[John Garry]

On 20/09/2018 17:10, Robin Murphy wrote:
> Hi all,
>
> Hopefully this is the last spin of the series - I've now dropped my light
> touch and fixed up all the various prose text, plus implemented the proper
> quirk support for short-descriptor because it's actually just a trivial
> cut-and-paste job.
>
> Robin.
>

Hi Robin,

JFYI, I'm trying to test this patchset to get some figures and provide a 
tested-by tag, but 4/8 seems to rely on "iommu/io-pgtable-arm: Fix race 
handling in split_blk_unmap()" - more specifically, it seems to rely on 
the version which Will rewrote in your patch review, and I am not sure 
on what branch it exists on, if any.

Thanks,
John

>
> Robin Murphy (2):
>   iommu/io-pgtable-arm-v7s: Add support for non-strict mode
>   iommu/arm-smmu: Support non-strict mode
>
> Zhen Lei (5):
>   iommu/arm-smmu-v3: Implement flush_iotlb_all hook
>   iommu/dma: Add support for non-strict mode
>   iommu: Add "iommu.strict" command line option
>   iommu/io-pgtable-arm: Add support for non-strict mode
>   iommu/arm-smmu-v3: Add support for non-strict mode
>
>  .../admin-guide/kernel-parameters.txt         | 12 +++
>  drivers/iommu/arm-smmu-v3.c                   | 89 +++++++++++++-----
>  drivers/iommu/arm-smmu.c                      | 93 +++++++++++++------
>  drivers/iommu/dma-iommu.c                     | 32 ++++++-
>  drivers/iommu/io-pgtable-arm-v7s.c            | 11 ++-
>  drivers/iommu/io-pgtable-arm.c                | 14 ++-
>  drivers/iommu/io-pgtable.h                    |  5 +
>  drivers/iommu/iommu.c                         | 23 +++++
>  include/linux/iommu.h                         |  1 +
>  9 files changed, 225 insertions(+), 55 deletions(-)
>

Re: [PATCH v8 0/7] Add non-strict mode support for iommu-dma

[Robin Murphy]

Hi John,

On 2018-09-21 10:20 AM, John Garry wrote:
> On 20/09/2018 17:10, Robin Murphy wrote:
>> Hi all,
>>
>> Hopefully this is the last spin of the series - I've now dropped my light
>> touch and fixed up all the various prose text, plus implemented the 
>> proper
>> quirk support for short-descriptor because it's actually just a trivial
>> cut-and-paste job.
>>
>> Robin.
>>
> 
> Hi Robin,
> 
> JFYI, I'm trying to test this patchset to get some figures and provide a 
> tested-by tag, but 4/8 seems to rely on "iommu/io-pgtable-arm: Fix race 
> handling in split_blk_unmap()" - more specifically, it seems to rely on 
> the version which Will rewrote in your patch review, and I am not sure 
> on what branch it exists on, if any.

Sorry, I should have said this is based on Will's iommu/devel branch:

https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=iommu/devel

Robin.

> 
> Thanks,
> John
> 
>>
>> Robin Murphy (2):
>>   iommu/io-pgtable-arm-v7s: Add support for non-strict mode
>>   iommu/arm-smmu: Support non-strict mode
>>
>> Zhen Lei (5):
>>   iommu/arm-smmu-v3: Implement flush_iotlb_all hook
>>   iommu/dma: Add support for non-strict mode
>>   iommu: Add "iommu.strict" command line option
>>   iommu/io-pgtable-arm: Add support for non-strict mode
>>   iommu/arm-smmu-v3: Add support for non-strict mode
>>
>>  .../admin-guide/kernel-parameters.txt         | 12 +++
>>  drivers/iommu/arm-smmu-v3.c                   | 89 +++++++++++++-----
>>  drivers/iommu/arm-smmu.c                      | 93 +++++++++++++------
>>  drivers/iommu/dma-iommu.c                     | 32 ++++++-
>>  drivers/iommu/io-pgtable-arm-v7s.c            | 11 ++-
>>  drivers/iommu/io-pgtable-arm.c                | 14 ++-
>>  drivers/iommu/io-pgtable.h                    |  5 +
>>  drivers/iommu/iommu.c                         | 23 +++++
>>  include/linux/iommu.h                         |  1 +
>>  9 files changed, 225 insertions(+), 55 deletions(-)
>>
> 
> 

Re: [PATCH v8 0/7] Add non-strict mode support for iommu-dma

[Robin Murphy]

On 21/09/18 10:29, Robin Murphy wrote:
> Hi John,
> 
> On 2018-09-21 10:20 AM, John Garry wrote:
>> On 20/09/2018 17:10, Robin Murphy wrote:
>>> Hi all,
>>>
>>> Hopefully this is the last spin of the series - I've now dropped my 
>>> light
>>> touch and fixed up all the various prose text, plus implemented the 
>>> proper
>>> quirk support for short-descriptor because it's actually just a trivial
>>> cut-and-paste job.
>>>
>>> Robin.
>>>
>>
>> Hi Robin,
>>
>> JFYI, I'm trying to test this patchset to get some figures and provide 
>> a tested-by tag, but 4/8 seems to rely on "iommu/io-pgtable-arm: Fix 
>> race handling in split_blk_unmap()" - more specifically, it seems to 
>> rely on the version which Will rewrote in your patch review, and I am 
>> not sure on what branch it exists on, if any.
> 
> Sorry, I should have said this is based on Will's iommu/devel branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=iommu/devel 

FWIW I've now pushed out a complete branch here:

   git://linux-arm.org/linux-rm iommu/non-strict


Robin.

Re: [PATCH v8 0/7] Add non-strict mode support for iommu-dma

[John Garry]

On 21/09/2018 12:03, Robin Murphy wrote:
> On 21/09/18 10:29, Robin Murphy wrote:
>> Hi John,
>>
>> On 2018-09-21 10:20 AM, John Garry wrote:
>>> On 20/09/2018 17:10, Robin Murphy wrote:
>>>> Hi all,
>>>>
>>>> Hopefully this is the last spin of the series - I've now dropped my
>>>> light
>>>> touch and fixed up all the various prose text, plus implemented the
>>>> proper
>>>> quirk support for short-descriptor because it's actually just a trivial
>>>> cut-and-paste job.
>>>>
>>>> Robin.
>>>>
>>>
>>> Hi Robin,
>>>
>>> JFYI, I'm trying to test this patchset to get some figures and
>>> provide a tested-by tag, but 4/8 seems to rely on
>>> "iommu/io-pgtable-arm: Fix race handling in split_blk_unmap()" - more
>>> specifically, it seems to rely on the version which Will rewrote in
>>> your patch review, and I am not sure on what branch it exists on, if
>>> any.
>>
>> Sorry, I should have said this is based on Will's iommu/devel branch:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/log/?h=iommu/devel
>
>
> FWIW I've now pushed out a complete branch here:
>
>   git://linux-arm.org/linux-rm iommu/non-strict
>

Cheers

So for my network test scenario I was getting a boost @ 250K vs 160K 
packet(s)/second with strict off/on

For NVMe single disk performance, I was getting a boost @ 582K vs 370K 
IOPS with strict off/on.

I wasn't seeing such a boost for other storage controller scenario 
(that's with 6 SSDs), with 776K vs 740K IOPS for strict off/on, but SMMU 
off was ~800K IOPS.

FWIW:
Tested-by: John Garry <john.garry@huawei.com>

Thanks,
John

>
> Robin.
>
> .
>

Re: [PATCH v8 4/7] iommu/io-pgtable-arm: Add support for non-strict mode

[Will Deacon]

On Thu, Sep 20, 2018 at 05:10:24PM +0100, Robin Murphy wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Non-strict mode is simply a case of skipping 'regular' leaf TLBIs, since
> the sync is already factored out into ops->iotlb_sync at the core API
> level. Non-leaf invalidations where we change the page table structure
> itself still have to be issued synchronously in order to maintain walk
> caches correctly.
> 
> To save having to reason about it too much, make sure the invalidation
> in arm_lpae_split_blk_unmap() just performs its own unconditional sync
> to minimise the window in which we're technically violating the break-
> before-make requirement on a live mapping. This might work out redundant
> with an outer-level sync for strict unmaps, but we'll never be splitting
> blocks on a DMA fastpath anyway.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> [rm: tweak comment, commit message, split_blk_unmap logic and barriers]
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v8: Add barrier for the fiddly cross-cpu flush case
> 
>  drivers/iommu/io-pgtable-arm.c | 14 ++++++++++++--
>  drivers/iommu/io-pgtable.h     |  5 +++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 2f79efd16a05..237cacd4a62b 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -576,6 +576,7 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>  		tablep = iopte_deref(pte, data);
>  	} else if (unmap_idx >= 0) {
>  		io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
> +		io_pgtable_tlb_sync(&data->iop);
>  		return size;
>  	}
>  
> @@ -609,6 +610,13 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>  			io_pgtable_tlb_sync(iop);
>  			ptep = iopte_deref(pte, data);
>  			__arm_lpae_free_pgtable(data, lvl + 1, ptep);
> +		} else if (iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT) {
> +			/*
> +			 * Order the PTE update against queueing the IOVA, to
> +			 * guarantee that a flush callback from a different CPU
> +			 * has observed it before the TLBIALL can be issued.
> +			 */
> +			smp_wmb();

Looks good to me. In the case that everything happens on the same CPU, are
we relying on the TLB invalidation code in the SMMU driver(s) to provide the
DSB for pushing the new entry out to the walker?

Will

Re: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode

[Will Deacon]

On Thu, Sep 20, 2018 at 05:10:25PM +0100, Robin Murphy wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Now that io-pgtable knows how to dodge strict TLB maintenance, all
> that's left to do is bridge the gap between the IOMMU core requesting
> DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE for default domains, and showing the
> appropriate IO_PGTABLE_QUIRK_NON_STRICT flag to alloc_io_pgtable_ops().
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> [rm: convert to domain attribute, tweak commit message]
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v8:
>  - Use nested switches for attrs
>  - Document barrier semantics
> 
>  drivers/iommu/arm-smmu-v3.c | 79 ++++++++++++++++++++++++++-----------
>  1 file changed, 56 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index f10c852479fc..db402e8b068b 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -612,6 +612,7 @@ struct arm_smmu_domain {
>  	struct mutex			init_mutex; /* Protects smmu pointer */
>  
>  	struct io_pgtable_ops		*pgtbl_ops;
> +	bool				non_strict;
>  
>  	enum arm_smmu_domain_stage	stage;
>  	union {
> @@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie)
>  		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
>  	}
>  
> +	/*
> +	 * NOTE: when io-pgtable is in non-strict mode, we may get here with
> +	 * PTEs previously cleared by unmaps on the current CPU not yet visible
> +	 * to the SMMU. We are relying on the DSB implicit in queue_inc_prod()
> +	 * to guarantee those are observed before the TLBI. Do be careful, 007.
> +	 */

Good, so you can ignore my comment on the previous patch :)

Will

Re: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode

[Robin Murphy]

On 28/09/18 13:19, Will Deacon wrote:
> On Thu, Sep 20, 2018 at 05:10:25PM +0100, Robin Murphy wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> Now that io-pgtable knows how to dodge strict TLB maintenance, all
>> that's left to do is bridge the gap between the IOMMU core requesting
>> DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE for default domains, and showing the
>> appropriate IO_PGTABLE_QUIRK_NON_STRICT flag to alloc_io_pgtable_ops().
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> [rm: convert to domain attribute, tweak commit message]
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v8:
>>   - Use nested switches for attrs
>>   - Document barrier semantics
>>
>>   drivers/iommu/arm-smmu-v3.c | 79 ++++++++++++++++++++++++++-----------
>>   1 file changed, 56 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index f10c852479fc..db402e8b068b 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -612,6 +612,7 @@ struct arm_smmu_domain {
>>   	struct mutex			init_mutex; /* Protects smmu pointer */
>>   
>>   	struct io_pgtable_ops		*pgtbl_ops;
>> +	bool				non_strict;
>>   
>>   	enum arm_smmu_domain_stage	stage;
>>   	union {
>> @@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie)
>>   		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
>>   	}
>>   
>> +	/*
>> +	 * NOTE: when io-pgtable is in non-strict mode, we may get here with
>> +	 * PTEs previously cleared by unmaps on the current CPU not yet visible
>> +	 * to the SMMU. We are relying on the DSB implicit in queue_inc_prod()
>> +	 * to guarantee those are observed before the TLBI. Do be careful, 007.
>> +	 */
> 
> Good, so you can ignore my comment on the previous patch :)

Well, I suppose that comment in io-pgtable *could* have explicitly noted 
that same-CPU order is dealt with elsewhere - feel free to fix it up if 
you think it would be a helpful reminder for the future.

Cheers,
Robin.

Re: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode

[Will Deacon]

On Fri, Sep 28, 2018 at 01:26:00PM +0100, Robin Murphy wrote:
> On 28/09/18 13:19, Will Deacon wrote:
> > On Thu, Sep 20, 2018 at 05:10:25PM +0100, Robin Murphy wrote:
> > > From: Zhen Lei <thunder.leizhen@huawei.com>
> > > 
> > > Now that io-pgtable knows how to dodge strict TLB maintenance, all
> > > that's left to do is bridge the gap between the IOMMU core requesting
> > > DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE for default domains, and showing the
> > > appropriate IO_PGTABLE_QUIRK_NON_STRICT flag to alloc_io_pgtable_ops().
> > > 
> > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> > > [rm: convert to domain attribute, tweak commit message]
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > > 
> > > v8:
> > >   - Use nested switches for attrs
> > >   - Document barrier semantics
> > > 
> > >   drivers/iommu/arm-smmu-v3.c | 79 ++++++++++++++++++++++++++-----------
> > >   1 file changed, 56 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > index f10c852479fc..db402e8b068b 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -612,6 +612,7 @@ struct arm_smmu_domain {
> > >   	struct mutex			init_mutex; /* Protects smmu pointer */
> > >   	struct io_pgtable_ops		*pgtbl_ops;
> > > +	bool				non_strict;
> > >   	enum arm_smmu_domain_stage	stage;
> > >   	union {
> > > @@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> > >   		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
> > >   	}
> > > +	/*
> > > +	 * NOTE: when io-pgtable is in non-strict mode, we may get here with
> > > +	 * PTEs previously cleared by unmaps on the current CPU not yet visible
> > > +	 * to the SMMU. We are relying on the DSB implicit in queue_inc_prod()
> > > +	 * to guarantee those are observed before the TLBI. Do be careful, 007.
> > > +	 */
> > 
> > Good, so you can ignore my comment on the previous patch :)
> 
> Well, I suppose that comment in io-pgtable *could* have explicitly noted
> that same-CPU order is dealt with elsewhere - feel free to fix it up if you
> think it would be a helpful reminder for the future.

I think I'll move it into the documentation for the new attribute, so that
any driver authors wanting to enable lazy invalidation know that they need
to provide this guarantee in their full TLB invalidation callback.

Will

Re: [PATCH v8 3/7] iommu: Add "iommu.strict" command line option

[Will Deacon]

On Thu, Sep 20, 2018 at 05:10:23PM +0100, Robin Murphy wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> Add a generic command line option to enable lazy unmapping via IOVA
> flush queues, which will initally be suuported by iommu-dma. This echoes
> the semantics of "intel_iommu=strict" (albeit with the opposite default
> value), but in the driver-agnostic fashion of "iommu.passthrough".
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> [rm: move handling out of SMMUv3 driver, clean up documentation]
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v8:
>  - Rename "non-strict" to "strict" to better match existing options
>  - Rewrite doc text/commit message
>  - Downgrade boot-time message from warn/taint to info
> 
>  .../admin-guide/kernel-parameters.txt         | 12 ++++++++++
>  drivers/iommu/iommu.c                         | 23 +++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 9871e649ffef..92ae12aeabf4 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1749,6 +1749,18 @@
>  		nobypass	[PPC/POWERNV]
>  			Disable IOMMU bypass, using IOMMU for PCI devices.
>  
> +	iommu.strict=	[ARM64] Configure TLB invalidation behaviour
> +			Format: { "0" | "1" }
> +			0 - Lazy mode.
> +			  Request that DMA unmap operations use deferred
> +			  invalidation of hardware TLBs, for increased
> +			  throughput at the cost of reduced device isolation.
> +			  Will fall back to strict mode if not supported by
> +			  the relevant IOMMU driver.
> +			1 - Strict mode (default).
> +			  DMA unmap operations invalidate IOMMU hardware TLBs
> +			  synchronously.
> +
>  	iommu.passthrough=
>  			[ARM64] Configure DMA to bypass the IOMMU by default.
>  			Format: { "0" | "1" }
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8c15c5980299..02b6603f0616 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -41,6 +41,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
>  #else
>  static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
>  #endif
> +static bool iommu_dma_strict __read_mostly = true;
>  
>  struct iommu_callback_data {
>  	const struct iommu_ops *ops;
> @@ -131,6 +132,21 @@ static int __init iommu_set_def_domain_type(char *str)
>  }
>  early_param("iommu.passthrough", iommu_set_def_domain_type);
>  
> +static int __init iommu_dma_setup(char *str)
> +{
> +	int ret;
> +
> +	ret = kstrtobool(str, &iommu_dma_strict);
> +	if (ret)
> +		return ret;
> +
> +	if (!iommu_dma_strict)
> +		pr_info("Enabling deferred TLB invalidation for DMA; protection against malicious/malfunctioning devices may be reduced.\n");

Printing here isn't quite right, because if somebody boots with something
like:

	"iommu.strict=1 iommu.strict=0 iommu.strict=0 iommu.strict=1"

then we'll print the wrong thing twice :)

I think we either need to drop the print, or move it to a the DMA domain
initialisation.

Will

Re: [PATCH v8 0/7] Add non-strict mode support for iommu-dma

[Will Deacon]

On Thu, Sep 20, 2018 at 05:10:20PM +0100, Robin Murphy wrote:
> Hopefully this is the last spin of the series - I've now dropped my light
> touch and fixed up all the various prose text, plus implemented the proper
> quirk support for short-descriptor because it's actually just a trivial
> cut-and-paste job.

Did you manage to clarify how the lifetime of the domain/flush queue interacts
with the periodic timer after the cookie has been freed?

Will

Re: [PATCH v8 0/7] Add non-strict mode support for iommu-dma

[Robin Murphy]

On 28/09/18 14:36, Will Deacon wrote:
> On Thu, Sep 20, 2018 at 05:10:20PM +0100, Robin Murphy wrote:
>> Hopefully this is the last spin of the series - I've now dropped my light
>> touch and fixed up all the various prose text, plus implemented the proper
>> quirk support for short-descriptor because it's actually just a trivial
>> cut-and-paste job.
> 
> Did you manage to clarify how the lifetime of the domain/flush queue interacts
> with the periodic timer after the cookie has been freed?

Oh, yes - it turned out to be wrapped up in put_iova_domain() already 
(see free_iova_flush_queue()), so all looks good in that regard.

Robin.

Re: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode

[Will Deacon]

On Fri, Sep 28, 2018 at 01:47:04PM +0100, Will Deacon wrote:
> On Fri, Sep 28, 2018 at 01:26:00PM +0100, Robin Murphy wrote:
> > On 28/09/18 13:19, Will Deacon wrote:
> > > On Thu, Sep 20, 2018 at 05:10:25PM +0100, Robin Murphy wrote:
> > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > > index f10c852479fc..db402e8b068b 100644
> > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > @@ -612,6 +612,7 @@ struct arm_smmu_domain {
> > > >   	struct mutex			init_mutex; /* Protects smmu pointer */
> > > >   	struct io_pgtable_ops		*pgtbl_ops;
> > > > +	bool				non_strict;
> > > >   	enum arm_smmu_domain_stage	stage;
> > > >   	union {
> > > > @@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> > > >   		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
> > > >   	}
> > > > +	/*
> > > > +	 * NOTE: when io-pgtable is in non-strict mode, we may get here with
> > > > +	 * PTEs previously cleared by unmaps on the current CPU not yet visible
> > > > +	 * to the SMMU. We are relying on the DSB implicit in queue_inc_prod()
> > > > +	 * to guarantee those are observed before the TLBI. Do be careful, 007.
> > > > +	 */
> > > 
> > > Good, so you can ignore my comment on the previous patch :)
> > 
> > Well, I suppose that comment in io-pgtable *could* have explicitly noted
> > that same-CPU order is dealt with elsewhere - feel free to fix it up if you
> > think it would be a helpful reminder for the future.
> 
> I think I'll move it into the documentation for the new attribute, so that
> any driver authors wanting to enable lazy invalidation know that they need
> to provide this guarantee in their full TLB invalidation callback.

Hmm, so I started doing this but then realised we already required this
behaviour for tlb_add_flush() afaict. That would mean that mainline
currently has a bug for arm-smmu.c, because we use the _relaxed I/O
accessors in there and there's no DSB after clearing the PTE on unmap().

Am I missing something?

Will

Re: [PATCH v8 5/7] iommu/arm-smmu-v3: Add support for non-strict mode

[Robin Murphy]

On 28/09/18 14:55, Will Deacon wrote:
> On Fri, Sep 28, 2018 at 01:47:04PM +0100, Will Deacon wrote:
>> On Fri, Sep 28, 2018 at 01:26:00PM +0100, Robin Murphy wrote:
>>> On 28/09/18 13:19, Will Deacon wrote:
>>>> On Thu, Sep 20, 2018 at 05:10:25PM +0100, Robin Murphy wrote:
>>>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>>>> index f10c852479fc..db402e8b068b 100644
>>>>> --- a/drivers/iommu/arm-smmu-v3.c
>>>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>>>> @@ -612,6 +612,7 @@ struct arm_smmu_domain {
>>>>>    	struct mutex			init_mutex; /* Protects smmu pointer */
>>>>>    	struct io_pgtable_ops		*pgtbl_ops;
>>>>> +	bool				non_strict;
>>>>>    	enum arm_smmu_domain_stage	stage;
>>>>>    	union {
>>>>> @@ -1407,6 +1408,12 @@ static void arm_smmu_tlb_inv_context(void *cookie)
>>>>>    		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
>>>>>    	}
>>>>> +	/*
>>>>> +	 * NOTE: when io-pgtable is in non-strict mode, we may get here with
>>>>> +	 * PTEs previously cleared by unmaps on the current CPU not yet visible
>>>>> +	 * to the SMMU. We are relying on the DSB implicit in queue_inc_prod()
>>>>> +	 * to guarantee those are observed before the TLBI. Do be careful, 007.
>>>>> +	 */
>>>>
>>>> Good, so you can ignore my comment on the previous patch :)
>>>
>>> Well, I suppose that comment in io-pgtable *could* have explicitly noted
>>> that same-CPU order is dealt with elsewhere - feel free to fix it up if you
>>> think it would be a helpful reminder for the future.
>>
>> I think I'll move it into the documentation for the new attribute, so that
>> any driver authors wanting to enable lazy invalidation know that they need
>> to provide this guarantee in their full TLB invalidation callback.
> 
> Hmm, so I started doing this but then realised we already required this
> behaviour for tlb_add_flush() afaict. That would mean that mainline
> currently has a bug for arm-smmu.c, because we use the _relaxed I/O
> accessors in there and there's no DSB after clearing the PTE on unmap().
> 
> Am I missing something?

Argh, no - I started having the same suspicion when changing those 
writel()s in patch #7, resolved to either mention it to you or 
investigate it myself as a separate fix, then promptly forgot entirely. 
Thanks for the reminder...

Robin.

Re: [PATCH v8 3/7] iommu: Add "iommu.strict" command line option

[Robin Murphy]

On 28/09/18 13:51, Will Deacon wrote:
> On Thu, Sep 20, 2018 at 05:10:23PM +0100, Robin Murphy wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> Add a generic command line option to enable lazy unmapping via IOVA
>> flush queues, which will initally be suuported by iommu-dma. This echoes
>> the semantics of "intel_iommu=strict" (albeit with the opposite default
>> value), but in the driver-agnostic fashion of "iommu.passthrough".
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> [rm: move handling out of SMMUv3 driver, clean up documentation]
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v8:
>>   - Rename "non-strict" to "strict" to better match existing options
>>   - Rewrite doc text/commit message
>>   - Downgrade boot-time message from warn/taint to info
>>
>>   .../admin-guide/kernel-parameters.txt         | 12 ++++++++++
>>   drivers/iommu/iommu.c                         | 23 +++++++++++++++++++
>>   2 files changed, 35 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 9871e649ffef..92ae12aeabf4 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1749,6 +1749,18 @@
>>   		nobypass	[PPC/POWERNV]
>>   			Disable IOMMU bypass, using IOMMU for PCI devices.
>>   
>> +	iommu.strict=	[ARM64] Configure TLB invalidation behaviour
>> +			Format: { "0" | "1" }
>> +			0 - Lazy mode.
>> +			  Request that DMA unmap operations use deferred
>> +			  invalidation of hardware TLBs, for increased
>> +			  throughput at the cost of reduced device isolation.
>> +			  Will fall back to strict mode if not supported by
>> +			  the relevant IOMMU driver.
>> +			1 - Strict mode (default).
>> +			  DMA unmap operations invalidate IOMMU hardware TLBs
>> +			  synchronously.
>> +
>>   	iommu.passthrough=
>>   			[ARM64] Configure DMA to bypass the IOMMU by default.
>>   			Format: { "0" | "1" }
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 8c15c5980299..02b6603f0616 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -41,6 +41,7 @@ static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_IDENTITY;
>>   #else
>>   static unsigned int iommu_def_domain_type = IOMMU_DOMAIN_DMA;
>>   #endif
>> +static bool iommu_dma_strict __read_mostly = true;
>>   
>>   struct iommu_callback_data {
>>   	const struct iommu_ops *ops;
>> @@ -131,6 +132,21 @@ static int __init iommu_set_def_domain_type(char *str)
>>   }
>>   early_param("iommu.passthrough", iommu_set_def_domain_type);
>>   
>> +static int __init iommu_dma_setup(char *str)
>> +{
>> +	int ret;
>> +
>> +	ret = kstrtobool(str, &iommu_dma_strict);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!iommu_dma_strict)
>> +		pr_info("Enabling deferred TLB invalidation for DMA; protection against malicious/malfunctioning devices may be reduced.\n");
> 
> Printing here isn't quite right, because if somebody boots with something
> like:
> 
> 	"iommu.strict=1 iommu.strict=0 iommu.strict=0 iommu.strict=1"
> 
> then we'll print the wrong thing twice :)

But it's not wrong! For those two brief moments it *is* enabled :P

For reasons of conciseness, the subtlety that "enabled" doesn't 
necessarily imply "in use" is only conveyed by the "may".

> I think we either need to drop the print, or move it to a the DMA domain
> initialisation.

TBH I did toy with moving it around, but in the end it seemed neatest to 
have it right there next to the parameter handling, with the added 
advantage that it appears early in the log where system-wide things 
might be expected to appear, rather than mixed in with all the driver 
noise later.

AFAICS it's no worse than various other parameters - try booting with, 
say, "mem=640k mem=1G mem=cheese" and one finds that memory is in fact 
not limited (nor indeed cheese) regardless of what the log might say.

Robin.