[PATCH v5 0/5] add non-strict mode support for arm-smmu-v3

[PATCH v5 0/5] add non-strict mode support for arm-smmu-v3

[Zhen Lei]

v4 -> v5:
1. change the type of global variable and struct member named "non_strict" from
   "int" to "bool".
2. cancel the unnecessary parameter "strict" of __arm_lpae_unmap which was added
   in v4.
3. change boot option "arm_iommu" to "iommu.non_strict".
4. convert __iommu_dma_unmap to use iommu_unmap_fast()/iommu_tlb_sync(), because
   non-leaf unmaps still need to be synchronous.

Thanks for Robin's review comments.

v3 -> v4:
1. Add a new member "non_strict" in struct iommu_domain to mark whether
   that domain use non-strict mode or not. This can help us to remove the
   capability which was added in prior version.
2. Add a new quirk IO_PGTABLE_QUIRK_NON_STRICT, so that we can get "strict
   mode" in io-pgtable-arm.c according to data->iop.cfg.quirks.
3. rename the new boot option to "arm_iommu".

v2 -> v3:
Add a bootup option "iommu_strict_mode" to make the manager can choose which
mode to be used. The first 5 patches have not changed.
+	iommu_strict_mode=	[arm-smmu-v3]
+		0 - strict mode (default)
+		1 - non-strict mode

v1 -> v2:
Use the lowest bit of the io_pgtable_ops.unmap's iova parameter to pass the strict mode:
0, IOMMU_STRICT;
1, IOMMU_NON_STRICT;
Treat 0 as IOMMU_STRICT, so that the unmap operation can compatible with
other IOMMUs which still use strict mode. In other words, this patch series
will not impact other IOMMU drivers. I tried add a new quirk IO_PGTABLE_QUIRK_NON_STRICT
in io_pgtable_cfg.quirks, but it can not pass the strict mode of the domain from SMMUv3
driver to io-pgtable module. 

Add a new member domain_non_strict in struct iommu_dma_cookie, this member will only be
initialized when the related domain and IOMMU driver support non-strict mode.

v1:
In common, a IOMMU unmap operation follow the below steps:
1. remove the mapping in page table of the specified iova range
2. execute tlbi command to invalid the mapping which is cached in TLB
3. wait for the above tlbi operation to be finished
4. free the IOVA resource
5. free the physical memory resource

This maybe a problem when unmap is very frequently, the combination of tlbi
and wait operation will consume a lot of time. A feasible method is put off
tlbi and iova-free operation, when accumulating to a certain number or
reaching a specified time, execute only one tlbi_all command to clean up
TLB, then free the backup IOVAs. Mark as non-strict mode.

But it must be noted that, although the mapping has already been removed in
the page table, it maybe still exist in TLB. And the freed physical memory
may also be reused for others. So a attacker can persistent access to memory
based on the just freed IOVA, to obtain sensible data or corrupt memory. So
the VFIO should always choose the strict mode.

Some may consider put off physical memory free also, that will still follow
strict mode. But for the map_sg cases, the memory allocation is not controlled
by IOMMU APIs, so it is not enforceable.

Fortunately, Intel and AMD have already applied the non-strict mode, and put
queue_iova() operation into the common file dma-iommu.c., and my work is based
on it. The difference is that arm-smmu-v3 driver will call IOMMU common APIs to
unmap, but Intel and AMD IOMMU drivers are not.

Below is the performance data of strict vs non-strict for NVMe device:
Randomly Read  IOPS: 146K(strict) vs 573K(non-strict)
Randomly Write IOPS: 143K(strict) vs 513K(non-strict)

Zhen Lei (5):
  iommu/arm-smmu-v3: fix the implementation of flush_iotlb_all hook
  iommu/dma: add support for non-strict mode
  iommu/io-pgtable-arm: add support for non-strict mode
  iommu/arm-smmu-v3: add support for non-strict mode
  iommu/arm-smmu-v3: add bootup option "iommu.non_strict"

 Documentation/admin-guide/kernel-parameters.txt | 13 +++++++++
 drivers/iommu/arm-smmu-v3.c                     | 35 ++++++++++++++++++++++++-
 drivers/iommu/dma-iommu.c                       | 29 +++++++++++++++++++-
 drivers/iommu/io-pgtable-arm.c                  | 20 +++++++++-----
 drivers/iommu/io-pgtable.h                      |  3 +++
 drivers/iommu/iommu.c                           |  1 +
 include/linux/iommu.h                           |  1 +
 7 files changed, 94 insertions(+), 8 deletions(-)

-- 
1.8.3

[PATCH v5 1/5] iommu/arm-smmu-v3: fix the implementation of flush_iotlb_all hook

[Zhen Lei]

.flush_iotlb_all can not just wait for previous tlbi operations to be
completed, but should also invalid all TLBs of the related domain.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Reviewed-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 1d64710..4402187 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1770,6 +1770,14 @@ static int arm_smmu_map(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->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;
@@ -1998,7 +2006,7 @@ static void arm_smmu_put_resv_regions(struct device *dev,
 	.map			= arm_smmu_map,
 	.unmap			= arm_smmu_unmap,
 	.map_sg			= default_iommu_map_sg,
-	.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,
--
1.8.3

[PATCH v5 2/5] iommu/dma: add support for non-strict mode

[Zhen Lei]

1. Save the related domain pointer in struct iommu_dma_cookie, make iovad
   capable call domain->ops->flush_iotlb_all to flush TLB.
2. During the iommu domain initialization phase, base on domain->non_strict
   field to check whether non-strict mode is supported or not. If so, call
   init_iova_flush_queue to register iovad->flush_cb callback.
3. All unmap(contains iova-free) APIs will finally invoke __iommu_dma_unmap
   -->iommu_dma_free_iova. If the domain is non-strict, call queue_iova to
   put off iova freeing, and omit iommu_tlb_sync operation.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/dma-iommu.c | 29 ++++++++++++++++++++++++++++-
 drivers/iommu/iommu.c     |  1 +
 include/linux/iommu.h     |  1 +
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index ddcbbdb..f0257e9 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;
+
+	/* Only be assigned in non-strict mode, otherwise it's NULL */
+	struct iommu_domain		*domain;
 };

 static inline size_t cookie_msi_granule(struct iommu_dma_cookie *cookie)
@@ -257,6 +260,17 @@ 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->domain;
+
+	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()
@@ -308,6 +322,14 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 	}

 	init_iova_domain(iovad, 1UL << order, base_pfn);
+
+	if (domain->non_strict) {
+		BUG_ON(!domain->ops->flush_iotlb_all);
+
+		cookie->domain = domain;
+		init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all, NULL);
+	}
+
 	if (!dev)
 		return 0;

@@ -390,6 +412,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->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));
@@ -405,7 +430,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 (!domain->non_strict)
+		iommu_tlb_sync(domain);
 	iommu_dma_free_iova(cookie, dma_addr, size);
 }

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 63b3756..6255a69 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1263,6 +1263,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,

 	domain->ops  = bus->iommu_ops;
 	domain->type = type;
+	domain->non_strict = false;
 	/* Assume all sizes by default; the driver may override this later */
 	domain->pgsize_bitmap  = bus->iommu_ops->pgsize_bitmap;

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 19938ee..4bbcf39 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -88,6 +88,7 @@ struct iommu_domain_geometry {

 struct iommu_domain {
 	unsigned type;
+	bool non_strict;
 	const struct iommu_ops *ops;
 	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
 	iommu_fault_handler_t handler;
--
1.8.3

[PATCH v5 3/5] iommu/io-pgtable-arm: add support for non-strict mode

[Zhen Lei]

To support the non-strict mode, now we only tlbi and sync for the strict
mode. But for the non-leaf case, always follow strict mode.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/io-pgtable-arm.c | 20 ++++++++++++++------
 drivers/iommu/io-pgtable.h     |  3 +++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 010a254..20d3e98 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -538,6 +538,7 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
 	phys_addr_t blk_paddr;
 	size_t tablesz = ARM_LPAE_GRANULE(data);
 	size_t split_sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
+	size_t unmapped = size;
 	int i, unmap_idx = -1;

 	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
@@ -575,11 +576,16 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
 		tablep = iopte_deref(pte, data);
 	}

-	if (unmap_idx < 0)
-		return __arm_lpae_unmap(data, iova, size, lvl, tablep);
+	if (unmap_idx < 0) {
+		unmapped = __arm_lpae_unmap(data, iova, size, lvl, tablep);
+		if (!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT))
+			return unmapped;
+	}

 	io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
-	return size;
+	io_pgtable_tlb_sync(&data->iop);
+
+	return unmapped;
 }

 static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
@@ -609,7 +615,7 @@ 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 {
+		} else if (!(iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT)) {
 			io_pgtable_tlb_add_flush(iop, iova, size, size, true);
 		}

@@ -771,7 +777,8 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
 	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 +870,8 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
 	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 2df7909..beb14a3 100644
--- a/drivers/iommu/io-pgtable.h
+++ b/drivers/iommu/io-pgtable.h
@@ -71,12 +71,15 @@ 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: Put off TLBs invalidation and release
+	 *	memory first.
 	 */
 	#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;
--
1.8.3

[PATCH v5 4/5] iommu/arm-smmu-v3: add support for non-strict mode

[Zhen Lei]

Dynamically choose strict or non-strict mode for page table config based
on the iommu domain type.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4402187..61eb7ec 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1622,6 +1622,11 @@ 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 (domain->type == IOMMU_DOMAIN_DMA) {
+		domain->non_strict = true;
+		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
+	}
+
 	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
 	if (!pgtbl_ops)
 		return -ENOMEM;
--
1.8.3

[PATCH v5 5/5] iommu/arm-smmu-v3: add bootup option "iommu.non_strict"

[Zhen Lei]

Add a bootup option to make the system manager can choose which mode to
be used. The default mode is strict.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 13 +++++++++++++
 drivers/iommu/arm-smmu-v3.c                     | 22 +++++++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 5cde1ff..cb9d043e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1720,6 +1720,19 @@
 		nobypass	[PPC/POWERNV]
 			Disable IOMMU bypass, using IOMMU for PCI devices.

+	iommu.non_strict=	[ARM64]
+			Format: { "0" | "1" }
+			0 - strict mode, default.
+			    Release IOVAs after the related TLBs are invalid
+			    completely.
+			1 - non-strict mode.
+			    Put off TLBs invalidation and release memory first.
+			    It's good for scatter-gather performance but lacks
+			    full isolation, an untrusted device can access the
+			    reused memory because the TLBs may still valid.
+			    Please take	full consideration before choosing this
+			    mode. Note that, VFIO will always use strict mode.
+
 	iommu.passthrough=
 			[ARM64] Configure DMA to bypass the IOMMU by default.
 			Format: { "0" | "1" }
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 61eb7ec..0eda90e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -631,6 +631,26 @@ struct arm_smmu_option_prop {
 	{ 0, NULL},
 };

+static bool smmu_non_strict __read_mostly;
+
+static int __init arm_smmu_setup(char *str)
+{
+	int ret;
+
+	ret = kstrtobool(str, &smmu_non_strict);
+	if (ret)
+		return ret;
+
+	if (smmu_non_strict) {
+		pr_warn("WARNING: iommu non-strict mode is chosen.\n"
+			"It's good for scatter-gather performance but lacks full isolation\n");
+		add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
+	}
+
+	return 0;
+}
+early_param("iommu.non_strict", arm_smmu_setup);
+
 static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
 						 struct arm_smmu_device *smmu)
 {
@@ -1622,7 +1642,7 @@ 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 (domain->type == IOMMU_DOMAIN_DMA) {
+	if ((domain->type == IOMMU_DOMAIN_DMA) && smmu_non_strict) {
 		domain->non_strict = true;
 		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
 	}
--
1.8.3

Re: [PATCH v5 5/5] iommu/arm-smmu-v3: add bootup option "iommu.non_strict"

[Robin Murphy]

On 15/08/18 02:28, Zhen Lei wrote:
> Add a bootup option to make the system manager can choose which mode to
> be used. The default mode is strict.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   Documentation/admin-guide/kernel-parameters.txt | 13 +++++++++++++
>   drivers/iommu/arm-smmu-v3.c                     | 22 +++++++++++++++++++++-
>   2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 5cde1ff..cb9d043e 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1720,6 +1720,19 @@
>   		nobypass	[PPC/POWERNV]
>   			Disable IOMMU bypass, using IOMMU for PCI devices.
> 
> +	iommu.non_strict=	[ARM64]
> +			Format: { "0" | "1" }
> +			0 - strict mode, default.
> +			    Release IOVAs after the related TLBs are invalid
> +			    completely.
> +			1 - non-strict mode.
> +			    Put off TLBs invalidation and release memory first.
> +			    It's good for scatter-gather performance but lacks
> +			    full isolation, an untrusted device can access the
> +			    reused memory because the TLBs may still valid.
> +			    Please take	full consideration before choosing this
> +			    mode. Note that, VFIO will always use strict mode.
> +
>   	iommu.passthrough=
>   			[ARM64] Configure DMA to bypass the IOMMU by default.
>   			Format: { "0" | "1" }
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 61eb7ec..0eda90e 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -631,6 +631,26 @@ struct arm_smmu_option_prop {
>   	{ 0, NULL},
>   };
> 
> +static bool smmu_non_strict __read_mostly;
> +
> +static int __init arm_smmu_setup(char *str)
> +{
> +	int ret;
> +
> +	ret = kstrtobool(str, &smmu_non_strict);
> +	if (ret)
> +		return ret;
> +
> +	if (smmu_non_strict) {
> +		pr_warn("WARNING: iommu non-strict mode is chosen.\n"
> +			"It's good for scatter-gather performance but lacks full isolation\n");
> +		add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
> +	}
> +
> +	return 0;
> +}
> +early_param("iommu.non_strict", arm_smmu_setup);

As I said on v3, the option should be parsed by iommu-dma, since that's 
where it takes effect, and I'm sure SMMUv2 users will be interested in 
trying it out too.

In other words, if iommu_dma_init_domain() does something like:

	if (iommu_dma_non_strict && domain->ops->flush_iotlb_all) {
		domain->non_strict = true;
		cookie->domain = domain;
		init_iova_flush_queue(...);
	}

> +
>   static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>   						 struct arm_smmu_device *smmu)
>   {
> @@ -1622,7 +1642,7 @@ 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 (domain->type == IOMMU_DOMAIN_DMA) {
> +	if ((domain->type == IOMMU_DOMAIN_DMA) && smmu_non_strict) {
>   		domain->non_strict = true;
>   		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
>   	}

...then all the driver should need to do is:

	if (domain->non_strict)
		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;


Now, that would make it possible to request non-strict mode even with 
drivers which *don't* understand it, but I think that's not actually 
harmful, just means that some TLBIs will still get issued synchronously 
and the flush queue might not do much. If you wanted to avoid even that, 
you could replace domain->non_strict with an iommu_domain_set_attr() 
call, so iommu_dma could tell up-front whether the driver understands 
non-strict mode and it's worth setting the queue up or not.

Robin.

Re: [PATCH v5 3/5] iommu/io-pgtable-arm: add support for non-strict mode

[Robin Murphy]

On 15/08/18 02:28, Zhen Lei wrote:
> To support the non-strict mode, now we only tlbi and sync for the strict
> mode. But for the non-leaf case, always follow strict mode.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/iommu/io-pgtable-arm.c | 20 ++++++++++++++------
>   drivers/iommu/io-pgtable.h     |  3 +++
>   2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 010a254..20d3e98 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -538,6 +538,7 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>   	phys_addr_t blk_paddr;
>   	size_t tablesz = ARM_LPAE_GRANULE(data);
>   	size_t split_sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
> +	size_t unmapped = size;
>   	int i, unmap_idx = -1;
> 
>   	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
> @@ -575,11 +576,16 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>   		tablep = iopte_deref(pte, data);
>   	}
> 
> -	if (unmap_idx < 0)

[ side note: the more I see this test the more it looks slightly wrong, 
but that's a separate issue. I'll have to sit down and really remember 
what's going on here... ]

> -		return __arm_lpae_unmap(data, iova, size, lvl, tablep);
> +	if (unmap_idx < 0) {
> +		unmapped = __arm_lpae_unmap(data, iova, size, lvl, tablep);
> +		if (!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT))
> +			return unmapped;
> +	}

I don't quite get this change - we should only be recursing back into 
__arm_lpae_unmap() here if nothing's actually been unmapped at this 
point - the block entry is simply replaced by a full next-level table 
and we're going to come back and split another block at that next level, 
or we raced against someone else splitting the same block and that's 
their table instead. Since that's reverting back to a "regular" unmap, I 
don't see where the need to introduce an additional flush to that path 
comes from (relative to the existing behaviour, at least).

>   	io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);

This is the flush which corresponds to whatever page split_blk_unmap() 
actually unmapped itself (and also covers any recursively-split 
intermediate-level entries)...

> -	return size;
> +	io_pgtable_tlb_sync(&data->iop);

...which does want this sync, but only technically for non-strict mode, 
since it's otherwise covered by the sync in iommu_unmap().

I'm not *against* tightening up the TLB maintenance here in general, but 
if so that should be a separately-reasoned patch, not snuck in with 
other changes.

Robin.

> +
> +	return unmapped;
>   }
> 
>   static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
> @@ -609,7 +615,7 @@ 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 {
> +		} else if (!(iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT)) {
>   			io_pgtable_tlb_add_flush(iop, iova, size, size, true);
>   		}
> 
> @@ -771,7 +777,8 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
>   	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 +870,8 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
>   	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 2df7909..beb14a3 100644
> --- a/drivers/iommu/io-pgtable.h
> +++ b/drivers/iommu/io-pgtable.h
> @@ -71,12 +71,15 @@ 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: Put off TLBs invalidation and release
> +	 *	memory first.
>   	 */
>   	#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;
> --
> 1.8.3
> 
> 

Re: [PATCH v5 5/5] iommu/arm-smmu-v3: add bootup option "iommu.non_strict"

[Leizhen (ThunderTown)]

On 2018/8/23 1:02, Robin Murphy wrote:
> On 15/08/18 02:28, Zhen Lei wrote:
>> Add a bootup option to make the system manager can choose which mode to
>> be used. The default mode is strict.
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt | 13 +++++++++++++
>>   drivers/iommu/arm-smmu-v3.c                     | 22 +++++++++++++++++++++-
>>   2 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 5cde1ff..cb9d043e 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1720,6 +1720,19 @@
>>           nobypass    [PPC/POWERNV]
>>               Disable IOMMU bypass, using IOMMU for PCI devices.
>>
>> +    iommu.non_strict=    [ARM64]
>> +            Format: { "0" | "1" }
>> +            0 - strict mode, default.
>> +                Release IOVAs after the related TLBs are invalid
>> +                completely.
>> +            1 - non-strict mode.
>> +                Put off TLBs invalidation and release memory first.
>> +                It's good for scatter-gather performance but lacks
>> +                full isolation, an untrusted device can access the
>> +                reused memory because the TLBs may still valid.
>> +                Please take    full consideration before choosing this
>> +                mode. Note that, VFIO will always use strict mode.
>> +
>>       iommu.passthrough=
>>               [ARM64] Configure DMA to bypass the IOMMU by default.
>>               Format: { "0" | "1" }
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 61eb7ec..0eda90e 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -631,6 +631,26 @@ struct arm_smmu_option_prop {
>>       { 0, NULL},
>>   };
>>
>> +static bool smmu_non_strict __read_mostly;
>> +
>> +static int __init arm_smmu_setup(char *str)
>> +{
>> +    int ret;
>> +
>> +    ret = kstrtobool(str, &smmu_non_strict);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (smmu_non_strict) {
>> +        pr_warn("WARNING: iommu non-strict mode is chosen.\n"
>> +            "It's good for scatter-gather performance but lacks full isolation\n");
>> +        add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
>> +    }
>> +
>> +    return 0;
>> +}
>> +early_param("iommu.non_strict", arm_smmu_setup);
> 
> As I said on v3, the option should be parsed by iommu-dma, since that's where it takes effect, and I'm sure SMMUv2 users will be interested in trying it out too.

OK, I am so sorry that I have not understood your opinion correctly.

> 
> In other words, if iommu_dma_init_domain() does something like:
> 
>     if (iommu_dma_non_strict && domain->ops->flush_iotlb_all) {
>         domain->non_strict = true;
>         cookie->domain = domain;
>         init_iova_flush_queue(...);
>     }
> 
>> +
>>   static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>>                            struct arm_smmu_device *smmu)
>>   {
>> @@ -1622,7 +1642,7 @@ 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 (domain->type == IOMMU_DOMAIN_DMA) {
>> +    if ((domain->type == IOMMU_DOMAIN_DMA) && smmu_non_strict) {
>>           domain->non_strict = true;
>>           pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
>>       }
> 
> ...then all the driver should need to do is:
> 
>     if (domain->non_strict)
>         pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
> 
> 
> Now, that would make it possible to request non-strict mode even with drivers which *don't* understand it, but I think that's not actually harmful, just means that some TLBIs will still get issued synchronously and the flush queue might not do much. If you wanted to avoid even that, you could replace domain->non_strict with an iommu_domain_set_attr() call, so iommu_dma could tell up-front whether the driver understands non-strict mode and it's worth setting the queue up or not.

OK, I will think seriously about it, thanks. I've been busy these days, I will reply to you as soon as possible.

> 
> Robin.
> 
> .
> 

-- 
Thanks!
BestRegards

Re: [PATCH v5 3/5] iommu/io-pgtable-arm: add support for non-strict mode

[Leizhen (ThunderTown)]

On 2018/8/23 1:52, Robin Murphy wrote:
> On 15/08/18 02:28, Zhen Lei wrote:
>> To support the non-strict mode, now we only tlbi and sync for the strict
>> mode. But for the non-leaf case, always follow strict mode.
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/iommu/io-pgtable-arm.c | 20 ++++++++++++++------
>>   drivers/iommu/io-pgtable.h     |  3 +++
>>   2 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
>> index 010a254..20d3e98 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -538,6 +538,7 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>>       phys_addr_t blk_paddr;
>>       size_t tablesz = ARM_LPAE_GRANULE(data);
>>       size_t split_sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
>> +    size_t unmapped = size;
>>       int i, unmap_idx = -1;
>>
>>       if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
>> @@ -575,11 +576,16 @@ static size_t arm_lpae_split_blk_unmap(struct arm_lpae_io_pgtable *data,
>>           tablep = iopte_deref(pte, data);
>>       }
>>
>> -    if (unmap_idx < 0)
> 
> [ side note: the more I see this test the more it looks slightly wrong, but that's a separate issue. I'll have to sit down and really remember what's going on here... ]
> 
>> -        return __arm_lpae_unmap(data, iova, size, lvl, tablep);
>> +    if (unmap_idx < 0) {
>> +        unmapped = __arm_lpae_unmap(data, iova, size, lvl, tablep);
>> +        if (!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT))
>> +            return unmapped;
>> +    }
> 
> I don't quite get this change - we should only be recursing back into __arm_lpae_unmap() here if nothing's actually been unmapped at this point - the block entry is simply replaced by a full next-level table and we're going to come back and split another block at that next level, or we raced against someone else splitting the same block and that's their table instead. Since that's reverting back to a "regular" unmap, I don't see where the need to introduce an additional flush to that path comes from (relative to the existing behaviour, at least).

The old block mapping maybe cached in TLBs, it should be invalidated completely before the new next-level mapping to be used. Just ensure that.

In fact, I think the code of arm_lpae_split_blk_unmap may has some mistakes. For example:
	if (size == split_sz)
		unmap_idx = ARM_LPAE_LVL_IDX(iova, lvl, data);

It means that "the size" can only be the block/page size of the next-level. Suppose current level is 2M block, but we may unmap 12K, and
the above "if" will limit us only be able to unmap 4K.

Furthermore, the situation "if (unmap_idx < 0)" should not appear.

Maybe my analysis is wrong, I will try to test it.


> 
>>       io_pgtable_tlb_add_flush(&data->iop, iova, size, size, true);
> 
> This is the flush which corresponds to whatever page split_blk_unmap() actually unmapped itself (and also covers any recursively-split intermediate-level entries)...
> 
>> -    return size;
>> +    io_pgtable_tlb_sync(&data->iop);
> 
> ...which does want this sync, but only technically for non-strict mode, since it's otherwise covered by the sync in iommu_unmap().

Because split_blk_unmap() is rarely to be called, it has little impact on the overall performance,
so I ommitted the if statement of non-strict, I will add it back.

> 
> I'm not *against* tightening up the TLB maintenance here in general, but if so that should be a separately-reasoned patch, not snuck in with other changes.

OK

> 
> Robin.
> 
>> +
>> +    return unmapped;
>>   }
>>
>>   static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>> @@ -609,7 +615,7 @@ 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 {
>> +        } else if (!(iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT)) {
>>               io_pgtable_tlb_add_flush(iop, iova, size, size, true);
>>           }
>>
>> @@ -771,7 +777,8 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
>>       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 +870,8 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
>>       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 2df7909..beb14a3 100644
>> --- a/drivers/iommu/io-pgtable.h
>> +++ b/drivers/iommu/io-pgtable.h
>> @@ -71,12 +71,15 @@ 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: Put off TLBs invalidation and release
>> +     *    memory first.
>>        */
>>       #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;
>> -- 
>> 1.8.3
>>
>>
> 
> .
> 

-- 
Thanks!
BestRegards

Re: [PATCH v5 0/5] add non-strict mode support for arm-smmu-v3

[Will Deacon]

Hi all,

On Wed, Aug 15, 2018 at 09:28:25AM +0800, Zhen Lei wrote:
> v4 -> v5:
> 1. change the type of global variable and struct member named "non_strict" from
>    "int" to "bool".
> 2. cancel the unnecessary parameter "strict" of __arm_lpae_unmap which was added
>    in v4.
> 3. change boot option "arm_iommu" to "iommu.non_strict".
> 4. convert __iommu_dma_unmap to use iommu_unmap_fast()/iommu_tlb_sync(), because
>    non-leaf unmaps still need to be synchronous.
> 
> Thanks for Robin's review comments.

Since this is 90% of the way there now, I suggest Robin picks up what's here
and incorporates his remaining review comments directly (especially since it
sounded like Zhen Lei hasn't got much free time lately). With that, I can
queue this lot via my smmu branch, which already has some stuff queued
for SMMUv3 and io-pgtable.

Please shout if you have any objections, but I'm keen for this not to
languish on the lists given how close it is!

Will

Re: [PATCH v5 0/5] add non-strict mode support for arm-smmu-v3

[Robin Murphy]

On 12/09/18 17:57, Will Deacon wrote:
> Hi all,
> 
> On Wed, Aug 15, 2018 at 09:28:25AM +0800, Zhen Lei wrote:
>> v4 -> v5:
>> 1. change the type of global variable and struct member named "non_strict" from
>>     "int" to "bool".
>> 2. cancel the unnecessary parameter "strict" of __arm_lpae_unmap which was added
>>     in v4.
>> 3. change boot option "arm_iommu" to "iommu.non_strict".
>> 4. convert __iommu_dma_unmap to use iommu_unmap_fast()/iommu_tlb_sync(), because
>>     non-leaf unmaps still need to be synchronous.
>>
>> Thanks for Robin's review comments.
> 
> Since this is 90% of the way there now, I suggest Robin picks up what's here
> and incorporates his remaining review comments directly (especially since it
> sounded like Zhen Lei hasn't got much free time lately). With that, I can
> queue this lot via my smmu branch, which already has some stuff queued
> for SMMUv3 and io-pgtable.
> 
> Please shout if you have any objections, but I'm keen for this not to
> languish on the lists given how close it is!

Sure, having got this far I'd like to see it get done too. I'll make 
some time and give it a go.

Robin.

Re: [PATCH v5 0/5] add non-strict mode support for arm-smmu-v3

[Leizhen (ThunderTown)]

On 2018/9/13 1:12, Robin Murphy wrote:
> On 12/09/18 17:57, Will Deacon wrote:
>> Hi all,
>>
>> On Wed, Aug 15, 2018 at 09:28:25AM +0800, Zhen Lei wrote:
>>> v4 -> v5:
>>> 1. change the type of global variable and struct member named "non_strict" from
>>>     "int" to "bool".
>>> 2. cancel the unnecessary parameter "strict" of __arm_lpae_unmap which was added
>>>     in v4.
>>> 3. change boot option "arm_iommu" to "iommu.non_strict".
>>> 4. convert __iommu_dma_unmap to use iommu_unmap_fast()/iommu_tlb_sync(), because
>>>     non-leaf unmaps still need to be synchronous.
>>>
>>> Thanks for Robin's review comments.
>>
>> Since this is 90% of the way there now, I suggest Robin picks up what's here
>> and incorporates his remaining review comments directly (especially since it
>> sounded like Zhen Lei hasn't got much free time lately). With that, I can
>> queue this lot via my smmu branch, which already has some stuff queued
>> for SMMUv3 and io-pgtable.
>>
>> Please shout if you have any objections, but I'm keen for this not to
>> languish on the lists given how close it is!
> 
> Sure, having got this far I'd like to see it get done too. I'll make some time and give it a go.

Thank you very much.

I've been too busy lately, at least I still have no time this week, I also think it's been too long.

So sorry and thanks again.

> 
> Robin.
> 
> .
> 

-- 
Thanks!
BestRegards