io-pgtable fixes + ARM short-descriptor format

IOMMU hardware with range-based TLB maintenance commands can work happily with the iova and size arguments passed via the tlb_add_flush callback, but for IOMMUs which require separate commands per entry in the range, it is not straightforward to infer the necessary granularity when it comes to issuing the actual commands.

Add an additional argument indicating the granularity for the benefit of drivers needing to know, and update the ARM LPAE code appropriately (for non-leaf invalidations we currently just assume the worst-case page granularity rather than walking the table to check).

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm-smmu-v3.c    |  2 +-
 drivers/iommu/arm-smmu.c       |  2 +-
 drivers/iommu/io-pgtable-arm.c | 27 +++++++++++++++------------
 drivers/iommu/io-pgtable.h     |  4 ++--
 drivers/iommu/ipmmu-vmsa.c     |  4 ++--
 5 files changed, 21 insertions(+), 18 deletions(-)

drivers/iommu/arm-smmu-v3.c +1 -1

index 4e5118a..c302b65 100644
@@ -1335,7 +1335,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 }
 
 static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
-					  bool leaf, void *cookie)
+					  size_t granule, bool leaf, void *cookie)
 {
 	struct arm_smmu_domain *smmu_domain = cookie;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;

drivers/iommu/arm-smmu.c +1 -1

index 47dc7a7..601e3dd 100644
@@ -582,7 +582,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 }
 
 static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
-					  bool leaf, void *cookie)
+					  size_t granule, bool leaf, void *cookie)
 {
 	struct arm_smmu_domain *smmu_domain = cookie;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;

drivers/iommu/io-pgtable-arm.c +3 -1

index 366a354..9088d27 100644
@@ -58,8 +58,10 @@
 	((((d)->levels - ((l) - ARM_LPAE_START_LVL(d) + 1))		\
 	  * (d)->bits_per_level) + (d)->pg_shift)
 
+#define ARM_LPAE_GRANULE(d)		(1UL << (d)->pg_shift)
+
 #define ARM_LPAE_PAGES_PER_PGD(d)					\
-	DIV_ROUND_UP((d)->pgd_size, 1UL << (d)->pg_shift)
+	DIV_ROUND_UP((d)->pgd_size, ARM_LPAE_GRANULE(d))
 
 /*
  * Calculate the index at level l used to map virtual address a using the

When invalidating an IOVA range potentially spanning multiple pages, such as when removing an entire intermediate-level table, we currently only issue an invalidation for the first IOVA of that range. Since the architecture specifies that address-based TLB maintenance operations target a single entry, an SMMU could feasibly retain live entries for subsequent pages within that unmapped range, which is not good.

Make sure we hit every possible entry by iterating over the whole range at the granularity provided by the pagetable implementation.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm-smmu.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Quoted 6 lines

drivers/iommu/io-pgtable-arm.c +3 -1

+#define ARM_LPAE_GRANULE(d)		(1UL << (d)->pg_shift)
+
  #define ARM_LPAE_PAGES_PER_PGD(d)					\
-	DIV_ROUND_UP((d)->pgd_size, 1UL << (d)->pg_shift)
+	DIV_ROUND_UP((d)->pgd_size, ARM_LPAE_GRANULE(d))

drivers/iommu/io-pgtable-arm.c line 174

Quoted 8 lines

/*
* Calculate the index at level l used to map virtual address a using the

drivers/iommu/io-pgtable-arm.c +1 -1

@@ -169,7 +171,7 @@
  /* IOPTE accessors */
  #define iopte_deref(pte,d)					\
  	(__va((pte) & ((1ULL << ARM_LPAE_MAX_ADDR_BITS) - 1)	\
-	& ~((1ULL << (d)->pg_shift) - 1)))
+	& ~(ARM_LPAE_GRANULE(d) - 1)))

Do we run the risk of truncating the VA on 32-bit ARM here?

Will

Quoted 10 lines · Robin Murphy

When invalidating an IOVA range potentially spanning multiple pages, such as when removing an entire intermediate-level table, we currently only issue an invalidation for the first IOVA of that range. Since the architecture specifies that address-based TLB maintenance operations target a single entry, an SMMU could feasibly retain live entries for subsequent pages within that unmapped range, which is not good.

Make sure we hit every possible entry by iterating over the whole range at the granularity provided by the pagetable implementation.

Quoted 4 lines

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/arm-smmu.c | 19 ++++++++++++++++---
  1 file changed, 16 insertions(+), 3 deletions(-)

Can you do something similar for arm-smmu-v3.c as well, please?

Quoted 11 lines · Will Deacon

Quoted 10 lines · Robin Murphy

When invalidating an IOVA range potentially spanning multiple pages, such as when removing an entire intermediate-level table, we currently only issue an invalidation for the first IOVA of that range. Since the architecture specifies that address-based TLB maintenance operations target a single entry, an SMMU could feasibly retain live entries for subsequent pages within that unmapped range, which is not good.

Make sure we hit every possible entry by iterating over the whole range at the granularity provided by the pagetable implementation.

Quoted 6 lines

Quoted 4 lines

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm-smmu.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Can you do something similar for arm-smmu-v3.c as well, please?

Something like this? (untested as I don't have a v3 model set up):

------>8------
From: Robin Murphy <[email protected]>
Date: Mon, 7 Dec 2015 12:52:56 +0000
Subject: [PATCH] iommu/arm-smmu: Fix TLB invalidation

SMMUv3 operates under the same rules as SMMUv2 and the CPU architectures, so when invalidating an IOVA range we have to hit every address for which a TLB entry might exist.

To fix this, issue commands for the whole range rather than just the initial address; as a minor optimisation, try to avoid flooding the queue by falling back to 'invalidate all' if the range is large.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm-smmu-v3.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/iommu/arm-smmu-v3.c +0 -0

index c302b65..afa0b41 100644

On 07/12/15 11:08, Will Deacon wrote:
[...]

Quoted 6 lines

Quoted 6 lines

drivers/iommu/io-pgtable-arm.c +3 -1

+#define ARM_LPAE_GRANULE(d)		(1UL << (d)->pg_shift)
+
 #define ARM_LPAE_PAGES_PER_PGD(d)					\
-	DIV_ROUND_UP((d)->pgd_size, 1UL << (d)->pg_shift)
+	DIV_ROUND_UP((d)->pgd_size, ARM_LPAE_GRANULE(d))

Quoted 10 lines

drivers/iommu/io-pgtable-arm.c line 174

Quoted 8 lines

/*
* Calculate the index at level l used to map virtual address a using the

drivers/iommu/io-pgtable-arm.c +1 -1

@@ -169,7 +171,7 @@
 /* IOPTE accessors */
 #define iopte_deref(pte,d)					\
 	(__va((pte) & ((1ULL << ARM_LPAE_MAX_ADDR_BITS) - 1)	\
-	& ~((1ULL << (d)->pg_shift) - 1)))
+	& ~(ARM_LPAE_GRANULE(d) - 1)))

Do we run the risk of truncating the VA on 32-bit ARM here?

Indeed, in all honesty I'd missed that, but since __va is going to truncate it to a 32-bit void * anyway, doing it earlier in the expression actually seems to result in better code - with iopte_deref wrapped like so to make it easier to pick out:

When invalidating an IOVA range potentially spanning multiple pages, such as when removing an entire intermediate-level table, we currently only issue an invalidation for the first IOVA of that range. Since the architecture specifies that address-based TLB maintenance operations target a single entry, an SMMU could feasibly retain live entries for subsequent pages within that unmapped range, which is not good.

Make sure we hit every possible entry by iterating over the whole range at the granularity provided by the pagetable implementation.

Signed-off-by Robin Murphy <[email protected]>

---

v2: include SMMUv3 fix, use the same shorter loop construct everywhere.

 drivers/iommu/arm-smmu-v3.c |  5 ++++-
 drivers/iommu/arm-smmu.c    | 16 +++++++++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

drivers/iommu/arm-smmu-v3.c +4 -1

index c302b65..8bb5abf 100644
@@ -1354,7 +1354,10 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
 	}
 
-	arm_smmu_cmdq_issue_cmd(smmu, &cmd);
+	do {
+		arm_smmu_cmdq_issue_cmd(smmu, &cmd);
+		cmd.tlbi.addr += granule;
+	} while (size -= granule);
 }
 
 static struct iommu_gather_ops arm_smmu_gather_ops = {

drivers/iommu/arm-smmu.c +3 -1

index 601e3dd..eb28c3e 100644
@@ -597,12 +597,18 @@ static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
 		if (!IS_ENABLED(CONFIG_64BIT) || smmu->version == ARM_SMMU_V1) {
 			iova &= ~12UL;
 			iova |= ARM_SMMU_CB_ASID(cfg);
-			writel_relaxed(iova, reg);
+			do {
+				writel_relaxed(iova, reg);
+				iova += granule;

Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.

Signed-off-by Yong Wu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

drivers/iommu/io-pgtable-arm-v7s.c new file +4 -0

+/* PTE type bits: these are all mixed up with XN/PXN bits in most cases */
+#define ARM_V7S_PTE_TYPE_TABLE		0x1
+#define ARM_V7S_PTE_TYPE_PAGE		0x2
+#define ARM_V7S_PTE_TYPE_CONT_PAGE	0x1

From the spec, This is Large page, Do we need add a comment for

drivers/iommu/io-pgtable-arm-v7s.c new file +14 -0

+
+#define ARM_V7S_PTE_IS_VALID(pte)	(((pte) & 0x3) != 0)
+#define ARM_V7S_PTE_IS_TABLE(pte, lvl)	(lvl < 2 && ((pte) & ARM_V7S_PTE_TYPE_TABLE))
+
+/* Page table bits */
+#define ARM_V7S_ATTR_XN(lvl)		BIT(4 * (2 - (lvl)))
+#define ARM_V7S_ATTR_B			BIT(2)
+#define ARM_V7S_ATTR_C			BIT(3)
+#define ARM_V7S_ATTR_NS_TABLE		BIT(3)
+#define ARM_V7S_ATTR_NS_SECTION		BIT(19)
+
+#define ARM_V7S_CONT_SECTION		BIT(18)
+#define ARM_V7S_CONT_PAGE_XN_SHIFT	15
+

drivers/iommu/io-pgtable-arm-v7s.c new file +14 -0

+static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp,
+				   struct arm_v7s_io_pgtable *data)
+{
+	struct device *dev = data->iop.cfg.iommu_dev;
+	dma_addr_t dma;
+	size_t size = ARM_V7S_TABLE_SIZE(lvl);
+	void *table = NULL;
+
+	if (lvl == 1)
+		table = (void *)__get_dma_pages(__GFP_ZERO, get_order(size));
+	else if (lvl == 2)
+		table = kmem_cache_zalloc(data->l2_tables, gfp);
+	if (table && !selftest_running) {
+		dma = dma_map_single(dev, table, size, DMA_TO_DEVICE);

drivers/iommu/io-pgtable-arm-v7s.c new file +13 -0

+	return table;
+
+out_unmap:
+	dev_err(dev, "Cannot accommodate DMA translation for IOMMU page tables\n");
+	dma_unmap_single(dev, dma, size, DMA_TO_DEVICE);
+out_free:
+	if (lvl == 1)
+		free_pages((unsigned long)table, get_order(size));
+	else
+		kmem_cache_free(data->l2_tables, table);
+	return NULL;
+}
+

drivers/iommu/io-pgtable-arm-v7s.c new file +11 -0

+static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
+			    unsigned long iova, phys_addr_t paddr, int prot,
+			    int lvl, int num_entries, arm_v7s_iopte *ptep)
+{
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	arm_v7s_iopte pte = arm_v7s_prot_to_pte(prot, lvl, cfg);
+	int i;
+
+	for (i = 0; i < num_entries; i++)
+		if (ARM_V7S_PTE_IS_TABLE(ptep[i], lvl)) {
+			/*

drivers/iommu/io-pgtable-arm-v7s.c new file +6 -0

+		} else if (ptep[i]) {
+			/* We require an unmap first */
+			WARN_ON(!selftest_running);
+			return -EEXIST;
+		}
+

drivers/iommu/io-pgtable-arm-v7s.c new file +32 -0

+static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
+			    unsigned long iova, size_t size, int lvl,
+			    arm_v7s_iopte *ptep)
+{
+	arm_v7s_iopte pte[ARM_V7S_CONT_PAGES];
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	const struct iommu_gather_ops *tlb = cfg->tlb;
+	void *cookie = data->iop.cookie;
+	int idx, i = 0, num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
+
+	/* Something went horribly wrong and we ran out of page table */
+	if (WARN_ON(lvl > 2))
+		return 0;
+
+	idx = ARM_V7S_LVL_IDX(iova, lvl);
+	ptep += idx;
+	do {
+		if (WARN_ON(!ARM_V7S_PTE_IS_VALID(ptep[i])))
+			return 0;
+		pte[i] = ptep[i];
+	} while (++i < num_entries);
+
+	/*
+	 * If we've hit a contiguous 'large page' entry at this level, it
+	 * needs splitting first, unless we're unmapping the whole lot.
+	 */
+	if (num_entries <= 1 && arm_v7s_pte_is_cont(pte[0], lvl))
+		arm_v7s_split_cont(data, iova, idx, lvl, ptep);
+
+	/* If the size matches this level, we're in the right place */
+	if (num_entries) {
+		size_t blk_size = ARM_V7S_BLOCK_SIZE(lvl);

drivers/iommu/io-pgtable-arm-v7s.c new file +38 -0

+				ptep = iopte_deref(pte[i], lvl);
+				__arm_v7s_free_table(ptep, lvl + 1, data);
+			} else {
+				tlb->tlb_add_flush(iova, blk_size, blk_size,
+						   true, cookie);
+			}
+			iova += blk_size;
+		}
+		return size;
+	} else if (lvl == 1 && !ARM_V7S_PTE_IS_TABLE(pte[0], lvl)) {
+		/*
+		 * Insert a table at the next level to map the old region,
+		 * minus the part we want to unmap
+		 */
+		return arm_v7s_split_blk_unmap(data, iova, size, ptep);
+	}
+
+	/* Keep on walkin' */
+	ptep = iopte_deref(pte[0], lvl);
+	return __arm_v7s_unmap(data, iova, size, lvl + 1, ptep);
+}
+
+static int arm_v7s_unmap(struct io_pgtable_ops *ops, unsigned long iova,
+			 size_t size)
+{
+	size_t unmapped;
+	struct arm_v7s_io_pgtable *data = io_pgtable_ops_to_data(ops);
+	struct io_pgtable *iop = &data->iop;
+
+	unmapped = __arm_v7s_unmap(data, iova, size, 1, data->pgd);
+	if (unmapped)
+		iop->cfg.tlb->tlb_sync(iop->cookie);
+
+	return unmapped;
+}
+
+static phys_addr_t arm_v7s_iova_to_phys(struct io_pgtable_ops *ops,
+					unsigned long iova)

drivers/iommu/io-pgtable-arm-v7s.c new file +9 -0

+	if (!ARM_V7S_PTE_IS_VALID(pte))
+		return 0;
+
+	mask = ARM_V7S_LVL_MASK(lvl);
+	if (arm_v7s_pte_is_cont(pte, lvl))
+		mask *= ARM_V7S_CONT_PAGES;
+	return (pte & mask) | (iova & ~mask);
+}
+

In the case of corrupted page tables, or when an invalid size is given, __arm_lpae_unmap() may recurse beyond the maximum number of levels. Unfortunately the detection of this error condition only happens *after* calculating a nonsense offset from something which might not be a valid table pointer and dereferencing that to see if it is a valid PTE.

Make things a little more robust by checking the level is valid before doing anything which depends on it being so.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/iommu/io-pgtable-arm.c +5 -3

index 7df9777..366a354 100644
@@ -486,11 +486,13 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable
 *data, void *cookie = data->iop.cookie;
 	size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
 
+	/* Something went horribly wrong and we ran out of page table */
+	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
+		return 0;
+
 	ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
 	pte = *ptep;
-
-	/* Something went horribly wrong and we ran out of page table */
-	if (WARN_ON(!pte || (lvl == ARM_LPAE_MAX_LEVELS)))
+	if (WARN_ON(!pte))
 		return 0;
 
 	/* If the size matches this level, we're in the right place */

There is no need to keep a useful accessor for a public structure hidden away in a private implementation. Move it out alongside the structure definition so that other implementations may reuse it.

Signed-off-by Robin Murphy <[email protected]>

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

drivers/iommu/io-pgtable-arm.c +0 -3

index 9088d27..1619681 100644
@@ -38,9 +38,6 @@
 #define io_pgtable_to_data(x)						\
 	container_of((x), struct arm_lpae_io_pgtable, iop)
 
-#define io_pgtable_ops_to_pgtable(x)					\
-	container_of((x), struct io_pgtable, ops)
-
 #define io_pgtable_ops_to_data(x)					\
 	io_pgtable_to_data(io_pgtable_ops_to_pgtable(x))
 

drivers/iommu/io-pgtable.h +1 -0

index 2e18469..36673c8 100644
@@ -131,6 +131,8 @@ struct io_pgtable {
 	struct io_pgtable_ops	ops;
 };
 
+#define io_pgtable_ops_to_pgtable(x) container_of((x), struct io_pgtable,

ops) +
/**
* struct io_pgtable_init_fns - Alloc/free a set of page tables for a
* particular format.

Hi Robin,

Thank you for the patch.

Quoted 6 lines · On Friday 04 December 2015 17:52:58 Robin Murphy wrote:

In the case of corrupted page tables, or when an invalid size is given, __arm_lpae_unmap() may recurse beyond the maximum number of levels. Unfortunately the detection of this error condition only happens *after* calculating a nonsense offset from something which might not be a valid table pointer and dereferencing that to see if it is a valid PTE.

Quoted 4 lines

Make things a little more robust by checking the level is valid before doing anything which depends on it being so.

Signed-off-by Robin Murphy <[email protected]>

This looks good to me.

Reviewed-by Laurent Pinchart <[email protected]>

I'm curious though, have you seen this error in practice ?

Quoted 25 lines

---
  drivers/iommu/io-pgtable-arm.c | 8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

drivers/iommu/io-pgtable-arm.c +5 -3

index 7df9777..366a354 100644
@@ -486,11 +486,13 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable
 *data, void *cookie = data->iop.cookie;
  	size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
 
+	/* Something went horribly wrong and we ran out of page table */
+	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
+		return 0;
+
  	ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
  	pte = *ptep;
-
-	/* Something went horribly wrong and we ran out of page table */
-	if (WARN_ON(!pte || (lvl == ARM_LPAE_MAX_LEVELS)))
+	if (WARN_ON(!pte))
  		return 0;
 
  	/* If the size matches this level, we're in the right place */

Hi Robin,

Thanks very much for your rewriting. It looks more pretty.

This works well in my selftest. and I will push to our chrome branch
and request our video/display to test more.

Only a little comment below.

Quoted 7 lines · Robin Murphy

Add a nearly-complete ARMv7 short descriptor implementation, omitting only a few legacy and CPU-centric aspects which shouldn't be necessary for IOMMU API use anyway.

Signed-off-by Yong Wu <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

[...]

drivers/iommu/io-pgtable-arm-v7s.c line 81

Quoted 4 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +4 -0

+/* PTE type bits: these are all mixed up with XN/PXN bits in most cases */
+#define ARM_V7S_PTE_TYPE_TABLE		0x1
+#define ARM_V7S_PTE_TYPE_PAGE		0x2
+#define ARM_V7S_PTE_TYPE_CONT_PAGE	0x1

From the spec, This is Large page, Do we need add a comment for
readable?
/* Large page */

and add /* Supersection */ for CONT_SECTION too.

Quoted 14 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +14 -0

+
+#define ARM_V7S_PTE_IS_VALID(pte)	(((pte) & 0x3) != 0)
+#define ARM_V7S_PTE_IS_TABLE(pte, lvl)	(lvl < 2 && ((pte) & ARM_V7S_PTE_TYPE_TABLE))
+
+/* Page table bits */
+#define ARM_V7S_ATTR_XN(lvl)		BIT(4 * (2 - (lvl)))
+#define ARM_V7S_ATTR_B			BIT(2)
+#define ARM_V7S_ATTR_C			BIT(3)
+#define ARM_V7S_ATTR_NS_TABLE		BIT(3)
+#define ARM_V7S_ATTR_NS_SECTION		BIT(19)
+
+#define ARM_V7S_CONT_SECTION		BIT(18)
+#define ARM_V7S_CONT_PAGE_XN_SHIFT	15
+

[...]

Quoted 22 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +22 -0

+static void *__arm_v7s_alloc_table(int lvl, gfp_t gfp,
+				   struct arm_v7s_io_pgtable *data)
+{
+	struct device *dev = data->iop.cfg.iommu_dev;
+	dma_addr_t dma;
+	size_t size = ARM_V7S_TABLE_SIZE(lvl);
+	void *table = NULL;
+
+	if (lvl == 1)
+		table = (void *)__get_dma_pages(__GFP_ZERO, get_order(size));
+	else if (lvl == 2)
+		table = kmem_cache_zalloc(data->l2_tables, gfp);
+	if (table && !selftest_running) {
+		dma = dma_map_single(dev, table, size, DMA_TO_DEVICE);
+		if (dma_mapping_error(dev, dma))
+			goto out_free;
+		/*
+		 * We depend on the IOMMU being able to work with any physical
+		 * address directly, so if the DMA layer suggests otherwise by
+		 * translating or truncating them, that bodes very badly...
+		 */
+		if (dma != virt_to_phys(table))

Review

Quoted 2 lines

(quoted diff) +2 -0

+			goto out_unmap;
+	}

There is some special while we use kmem_cache, we save the physical address into the pagetable, then get the virtual address via phys_to_virt, then free it.
It isn't same with the normal case that saving the va and free the va. Do we need add kmemleak_ignore here?

Quoted 13 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +13 -0

+	return table;
+
+out_unmap:
+	dev_err(dev, "Cannot accommodate DMA translation for IOMMU page tables\n");
+	dma_unmap_single(dev, dma, size, DMA_TO_DEVICE);
+out_free:
+	if (lvl == 1)
+		free_pages((unsigned long)table, get_order(size));
+	else
+		kmem_cache_free(data->l2_tables, table);
+	return NULL;
+}
+

[...]

Quoted 11 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +11 -0

+static int arm_v7s_init_pte(struct arm_v7s_io_pgtable *data,
+			    unsigned long iova, phys_addr_t paddr, int prot,
+			    int lvl, int num_entries, arm_v7s_iopte *ptep)
+{
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	arm_v7s_iopte pte = arm_v7s_prot_to_pte(prot, lvl, cfg);
+	int i;
+
+	for (i = 0; i < num_entries; i++)
+		if (ARM_V7S_PTE_IS_TABLE(ptep[i], lvl)) {
+			/*

drivers/iommu/io-pgtable-arm-v7s.c line 344

Quoted 10 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +10 -0

+			 * We need to unmap and free the old table before
+			 * overwriting it with a block entry.
+			 */
+			arm_v7s_iopte *tblp;
+			size_t sz = ARM_V7S_BLOCK_SIZE(lvl);
+
+			tblp = ptep + i - ARM_V7S_LVL_IDX(iova, lvl);
+			if (WARN_ON(__arm_v7s_unmap(data, iova, sz, lvl, tblp)
+					!= sz))
+				return -EINVAL;

Here it may come from Will's "iommu/io-pgtable-arm: Unmap and free table when overwriting with block".

But if we have IO_PGTABLE_QUIRK_TLBI_ON_MAP, the condition(1) in that comment don't exist. So we don't need take care whether the exist one is a pgtable or not, we could always return -EEXIST here?

Quoted 6 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +6 -0

+		} else if (ptep[i]) {
+			/* We require an unmap first */
+			WARN_ON(!selftest_running);
+			return -EEXIST;
+		}
+

[...]

Quoted 32 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +32 -0

+static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
+			    unsigned long iova, size_t size, int lvl,
+			    arm_v7s_iopte *ptep)
+{
+	arm_v7s_iopte pte[ARM_V7S_CONT_PAGES];
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	const struct iommu_gather_ops *tlb = cfg->tlb;
+	void *cookie = data->iop.cookie;
+	int idx, i = 0, num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
+
+	/* Something went horribly wrong and we ran out of page table */
+	if (WARN_ON(lvl > 2))
+		return 0;
+
+	idx = ARM_V7S_LVL_IDX(iova, lvl);
+	ptep += idx;
+	do {
+		if (WARN_ON(!ARM_V7S_PTE_IS_VALID(ptep[i])))
+			return 0;
+		pte[i] = ptep[i];
+	} while (++i < num_entries);
+
+	/*
+	 * If we've hit a contiguous 'large page' entry at this level, it
+	 * needs splitting first, unless we're unmapping the whole lot.
+	 */
+	if (num_entries <= 1 && arm_v7s_pte_is_cont(pte[0], lvl))
+		arm_v7s_split_cont(data, iova, idx, lvl, ptep);
+
+	/* If the size matches this level, we're in the right place */
+	if (num_entries) {
+		size_t blk_size = ARM_V7S_BLOCK_SIZE(lvl);

drivers/iommu/io-pgtable-arm-v7s.c line 555

Quoted 10 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +10 -0

+
+		__arm_v7s_set_pte(ptep, 0, num_entries, cfg);
+
+		for (i = 0; i < num_entries; i++) {
+			if (ARM_V7S_PTE_IS_TABLE(pte[i], lvl)) {
+				/* Also flush any partial walks */
+				tlb->tlb_add_flush(iova, blk_size,
+						   ARM_V7S_BLOCK_SIZE(2),
+						   false, cookie);
+				tlb->tlb_sync(cookie);

MTK iommu driver will get a warning here in my test.

There is a tlb_sync here, and in arm_v7s_unmap, there is another one. then the flow is:

tlb->tlb_add_flush(xxx)
tlb->tlb_sync()
tlb->tlb_sync()

In MTK tlb_sync, The code is:

static void mtk_iommu_tlb_sync(void *cookie)

{
	struct mtk_iommu_data *data = cookie;
	int ret;
	u32 tmp;

	ret = readl_poll_timeout_atomic(data->base + REG_MMU_CPE_DONE, tmp,
					tmp != 0, 10, 100000);
	if (ret) {
		dev_warn(data->dev,
			 "Partial TLB flush timed out, falling back to full flush\n");
		mtk_iommu_tlb_flush_all(cookie);
	}
	/* Clear the CPE status */
	writel_relaxed(0, data->base + REG_MMU_CPE_DONE);
}

In the end of this function, We have to write 0 to clear the CPE status, then the HW could check the status in the next time.

So if we call tlb_sync twice continually. It will time out in the second time, then we can see this log:

Partial TLB flush timed out, falling back to full flush

I don't know whether it is our HW special behavior, is this case ok in the arm-smmu.c?
Is there some suggestion about this?

Quoted 43 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +43 -0

+				ptep = iopte_deref(pte[i], lvl);
+				__arm_v7s_free_table(ptep, lvl + 1, data);
+			} else {
+				tlb->tlb_add_flush(iova, blk_size, blk_size,
+						   true, cookie);
+			}
+			iova += blk_size;
+		}
+		return size;
+	} else if (lvl == 1 && !ARM_V7S_PTE_IS_TABLE(pte[0], lvl)) {
+		/*
+		 * Insert a table at the next level to map the old region,
+		 * minus the part we want to unmap
+		 */
+		return arm_v7s_split_blk_unmap(data, iova, size, ptep);
+	}
+
+	/* Keep on walkin' */
+	ptep = iopte_deref(pte[0], lvl);
+	return __arm_v7s_unmap(data, iova, size, lvl + 1, ptep);
+}
+
+static int arm_v7s_unmap(struct io_pgtable_ops *ops, unsigned long iova,
+			 size_t size)
+{
+	size_t unmapped;
+	struct arm_v7s_io_pgtable *data = io_pgtable_ops_to_data(ops);
+	struct io_pgtable *iop = &data->iop;
+
+	unmapped = __arm_v7s_unmap(data, iova, size, 1, data->pgd);
+	if (unmapped)
+		iop->cfg.tlb->tlb_sync(iop->cookie);
+
+	return unmapped;
+}
+
+static phys_addr_t arm_v7s_iova_to_phys(struct io_pgtable_ops *ops,
+					unsigned long iova)
+{
+	struct arm_v7s_io_pgtable *data = io_pgtable_ops_to_data(ops);
+	arm_v7s_iopte *ptep = data->pgd, pte = ARM_V7S_PTE_TYPE_TABLE;
+	int lvl = 0;
+	u32 mask;

drivers/iommu/io-pgtable-arm-v7s.c line 603

Quoted 5 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +5 -0

+
+	while (ARM_V7S_PTE_IS_TABLE(pte, lvl)) {
+		pte = ptep[ARM_V7S_LVL_IDX(iova, ++lvl)];
+		ptep = iopte_deref(pte, lvl);
+	}

If we would like it always enter this while.
Could we use do{}while? then we don't need initialize the pte.

And in this file, the valid lvl should be 1 or 2. but here the "lvl" is initialized to 0. Do we need add a enum for the lvl for more safe and readable?

Quoted 9 lines

drivers/iommu/io-pgtable-arm-v7s.c new file +9 -0

+	if (!ARM_V7S_PTE_IS_VALID(pte))
+		return 0;
+
+	mask = ARM_V7S_LVL_MASK(lvl);
+	if (arm_v7s_pte_is_cont(pte, lvl))
+		mask *= ARM_V7S_CONT_PAGES;
+	return (pte & mask) | (iova & ~mask);
+}
+

[...]