io-pgtable ARM short descriptor format

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.

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

+	}
+	return pte;
+}
+
+static bool arm_v7s_pte_is_cont(arm_v7s_iopte pte, int lvl)
+{
+	if (lvl == 1 && !ARM_V7S_PTE_IS_TABLE(pte, lvl))
+		return pte & ARM_V7S_CONT_SECTION;
+	else if (lvl == 2)
+		return !(pte & ARM_V7S_PTE_TYPE_PAGE);
+	return false;
+}
+
+static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *, unsigned long,
+			   size_t, int, arm_v7s_iopte *);
+

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

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

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

+static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
+				   unsigned long iova, size_t size,
+				   arm_v7s_iopte *ptep)
+{
+	unsigned long blk_start, blk_end, blk_size;
+	phys_addr_t blk_paddr;
+	arm_v7s_iopte table = 0;
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	int prot = arm_v7s_pte_to_prot(*ptep, 1);
+
+	blk_size = ARM_V7S_BLOCK_SIZE(1);
+	blk_start = iova & ARM_V7S_LVL_MASK(1);
+	blk_end = blk_start + ARM_V7S_BLOCK_SIZE(1);
+	blk_paddr = *ptep & ARM_V7S_LVL_MASK(1);

Quoted 3 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.

[...]

Review

Quoted 9 lines

(quoted diff) +9 -0

+
+#define ARM_V7S_BLOCK_SIZE(lvl)		(1UL << ARM_V7S_LVL_SHIFT(lvl))
+#define ARM_V7S_LVL_MASK(lvl)		((u32)(~0U << ARM_V7S_LVL_SHIFT(lvl)))
+#define ARM_V7S_TABLE_MASK		((u32)(~0U << ARM_V7S_TABLE_SHIFT))
+#define _ARM_V7S_IDX_MASK(lvl)		(ARM_V7S_PTES_PER_LVL(lvl) - 1)
+#define ARM_V7S_LVL_IDX(addr, lvl)	({				\
+	int _l = lvl;							\
+	((u32)(addr) >> ARM_V7S_LVL_SHIFT(_l)) & _ARM_V7S_IDX_MASK(_l); \
+})

lvl here is not a function, it is 1 or 2, Could we use for simple?

#define ARM_V7S_LVL_IDX(addr, lvl)		\
	((u32)(addr) >> ARM_V7S_LVL_SHIFT(lvl)) & _ARM_V7S_IDX_MASK(lvl)

Review

Quoted 13 lines

(quoted diff) +13 -0

+static arm_v7s_iopte arm_v7s_cont_to_pte(arm_v7s_iopte pte, int lvl)
+{
+	if (lvl == 1) {
+		pte &= ~ARM_V7S_CONT_SECTION;
+	} else if (lvl == 2) {
+		arm_v7s_iopte xn = pte & BIT(ARM_V7S_CONT_PAGE_XN_SHIFT);
+		arm_v7s_iopte tex = pte & (ARM_V7S_CONT_PAGE_TEX_MASK <<
+					   ARM_V7S_CONT_PAGE_TEX_SHIFT);
+
+		pte ^= xn | tex;
+		pte |= (xn >> ARM_V7S_CONT_PAGE_XN_SHIFT) |
+		       (tex >> ARM_V7S_CONT_PAGE_TEX_SHIFT);
+		pte ^= ARM_V7S_PTE_TYPE_PAGE | ARM_V7S_PTE_TYPE_CONT_PAGE;

If XN bit is 1, We may lost XN bit for small page here.

We could runs ok as we have IO_PGTABLE_QUIRK_NO_PERMS.

Maybe this is enough: pte ^= ARM_V7S_PTE_TYPE_PAGE;

Quoted 24 lines

(quoted diff) +24 -0

+	}
+	return pte;
+}
+
+static bool arm_v7s_pte_is_cont(arm_v7s_iopte pte, int lvl)
+{
+	if (lvl == 1 && !ARM_V7S_PTE_IS_TABLE(pte, lvl))
+		return pte & ARM_V7S_CONT_SECTION;
+	else if (lvl == 2)
+		return !(pte & ARM_V7S_PTE_TYPE_PAGE);
+	return false;
+}
+
+static int __arm_v7s_unmap(struct arm_v7s_io_pgtable *, unsigned long,
+			   size_t, int, arm_v7s_iopte *);
+
+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;
+

Review

Quoted 2 lines

(quoted diff) +2 -0

+	for (i = 0; i < num_entries; i++)
+		if (ARM_V7S_PTE_IS_TABLE(ptep[i], lvl)) {

If somebody abuse the mapping, He map 4K firstly, then map 1M in the
same iova address(don't unmap the 4K before), then it will also enter here to free the existed lvl2 pagetable silently. Should we add a checking whether the existed pagetable is empty? if the pagetable is not empty, return a warning.

Review

Quoted 10 lines

(quoted diff) +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))

if "i" isn't 0 here(There is a pgtable item in a supersection), it
seems not right. Maybe:

     tblp = ptep + i - ARM_V7S_LVL_IDX(iova + i * sz, lvl);
     if (WARN_ON(__arm_v7s_unmap(data, iova + i * sz, sz, lvl, tblp)
		!= sz))

Quoted 7 lines

(quoted diff) +7 -0

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

[...]

Quoted 19 lines

(quoted diff) +19 -0

+static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
+				   unsigned long iova, size_t size,
+				   arm_v7s_iopte *ptep)
+{
+	unsigned long blk_start, blk_end, blk_size;
+	phys_addr_t blk_paddr;
+	arm_v7s_iopte table = 0;
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	int prot = arm_v7s_pte_to_prot(*ptep, 1);
+
+	blk_size = ARM_V7S_BLOCK_SIZE(1);
+	blk_start = iova & ARM_V7S_LVL_MASK(1);
+	blk_end = blk_start + ARM_V7S_BLOCK_SIZE(1);
+	blk_paddr = *ptep & ARM_V7S_LVL_MASK(1);
+
+	for (; blk_start < blk_end; blk_start += size, blk_paddr += size) {
+		arm_v7s_iopte *tablep;
+
+		/* Unmap! */

Review

Quoted 5 lines

(quoted diff) +5 -0

+		if (blk_start == iova)
+			continue;
+
+		/* __arm_v7s_map expects a pointer to the start of the table */
+		tablep = &table - ARM_V7S_LVL_IDX(blk_start, 1);

The "table" seems not be initialized here. (LPAE too.) maybe I don't
get it here.

If we would like to get the start of the table, maybe :

tablep = ptep - ARM_V7S_LVL_IDX(blk_start, 1);

Even though we change "tablep" here, it will also fail in the
__arm_v7s_map since there is a value in current ptep(the pa of the section), then it will call iopte_deref instread of creating a new pgtable in the __arm_v7s_map below, then it will abort.

so maybe we need unmap the ptep before this "for" loop. __arm_v7s_set_pte(ptep, 0, 1, cfg);

Review

Quoted 6 lines

(quoted diff) +6 -0

+		if (__arm_v7s_map(data, blk_start, blk_paddr, size, prot, 1,
+				  tablep) < 0) {
+			if (table) {
+				/* Free the table we allocated */
+				tablep = iopte_deref(table, 1);
+				__arm_v7s_free_table(tablep, 2, data);

Following Will's quesion before, do we need flush tlb here?

Review

Quoted 6 lines

(quoted diff) +6 -0

+			}
+			return 0; /* Bytes unmapped */
+		}
+	}
+
+	__arm_v7s_set_pte(ptep, table, 1, cfg);

Is this in order to update the lvl2 pgtable base address? why it's
not updated in __arm_v7s_map which create a lvl2 pgtable?

Review

Quoted 5 lines

(quoted diff) +5 -0

+	iova &= ~(blk_size - 1);
+	cfg->tlb->tlb_add_flush(iova, blk_size, blk_size, true, data->iop.cookie);
+	return size;
+}
+

All the others looks good for me. Thanks.

Quoted 4 lines · Yong Wu

Quoted 3 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.

Quoted 19 lines

Quoted 19 lines

(quoted diff) +19 -0

+static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
+				   unsigned long iova, size_t size,
+				   arm_v7s_iopte *ptep)
+{
+	unsigned long blk_start, blk_end, blk_size;
+	phys_addr_t blk_paddr;
+	arm_v7s_iopte table = 0;
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	int prot = arm_v7s_pte_to_prot(*ptep, 1);
+
+	blk_size = ARM_V7S_BLOCK_SIZE(1);
+	blk_start = iova & ARM_V7S_LVL_MASK(1);
+	blk_end = blk_start + ARM_V7S_BLOCK_SIZE(1);
+	blk_paddr = *ptep & ARM_V7S_LVL_MASK(1);
+
+	for (; blk_start < blk_end; blk_start += size, blk_paddr += size) {
+		arm_v7s_iopte *tablep;
+
+		/* Unmap! */

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		if (blk_start == iova)
+			continue;
+
+		/* __arm_v7s_map expects a pointer to the start of the table */
+		tablep = &table - ARM_V7S_LVL_IDX(blk_start, 1);

The "table" seems not be initialized here. (LPAE too.) maybe I don't
get it here.

It took me about 2 hours of staring at the original code to fully get my head round it, too ;) The comment would perhaps be better worded as "__arm_v7s_map expects a pointer to the start of *a* table". What we're doing is building up the whole level 2 table (with the relevant pages mapped) in advance _before_ touching the live level 1 table. Since __arm_v7s_map() is going to take the table pointer we pass and write an entry for the new level 2 table at the relevant index, we construct _just that entry_ ("arm_v7s_iopte table = 0;") and fake up a corresponding 'level 1 table pointer' - chances are that tablep itself is dangling way off the bottom of the stack somewhere, but the only entry in that 'table' that's going to be dereferenced is the one backed by the local variable.

Quoted 13 lines

If we would like to get the start of the table, maybe :

tablep = ptep - ARM_V7S_LVL_IDX(blk_start, 1);

Even though we change "tablep" here, it will also fail in the
__arm_v7s_map since there is a value in current ptep(the pa of the section), then it will call iopte_deref instread of creating a new pgtable in the __arm_v7s_map below, then it will abort.

so maybe we need unmap the ptep before this "for" loop. __arm_v7s_set_pte(ptep, 0, 1, cfg);

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+		if (__arm_v7s_map(data, blk_start, blk_paddr, size, prot, 1,
+				  tablep) < 0) {
+			if (table) {
+				/* Free the table we allocated */
+				tablep = iopte_deref(table, 1);
+				__arm_v7s_free_table(tablep, 2, data);

Following Will's quesion before, do we need flush tlb here?

No; this is just cleaning up the uncommitted preparatory work if map failure left a partially-created next-level table - in fact, now that I look at it, with only two levels the only way that can happen is if we allocate a new empty table but find nonzero entries when installing the PTEs, and that suggests a level of system corruption I'm not sure it's even worth trying to handle.

Anyway, from the IOMMU's perspective, at this point it knows nothing about the new table and the section mapping is still live...

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+			}
+			return 0; /* Bytes unmapped */
+		}
+	}
+
+	__arm_v7s_set_pte(ptep, table, 1, cfg);

Is this in order to update the lvl2 pgtable base address? why it's
not updated in __arm_v7s_map which create a lvl2 pgtable?

...until here, when we drop the table entry over the top of the section entry and TLBI the section, for an atomic valid->valid transition.

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	iova &= ~(blk_size - 1);
+	cfg->tlb->tlb_add_flush(iova, blk_size, blk_size, true, data->iop.cookie);
+	return size;
+}
+

All the others looks good for me. Thanks.

Cool, thanks. I'll send out a proper v3 once everyone's finished with merge window stuff, but below is the local diff I now have.

Robin.

Quoted 4 lines · Yong Wu

Quoted 3 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.

Quoted 19 lines

Quoted 19 lines

(quoted diff) +19 -0

+static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,
+				   unsigned long iova, size_t size,
+				   arm_v7s_iopte *ptep)
+{
+	unsigned long blk_start, blk_end, blk_size;
+	phys_addr_t blk_paddr;
+	arm_v7s_iopte table = 0;
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	int prot = arm_v7s_pte_to_prot(*ptep, 1);
+
+	blk_size = ARM_V7S_BLOCK_SIZE(1);
+	blk_start = iova & ARM_V7S_LVL_MASK(1);
+	blk_end = blk_start + ARM_V7S_BLOCK_SIZE(1);
+	blk_paddr = *ptep & ARM_V7S_LVL_MASK(1);
+
+	for (; blk_start < blk_end; blk_start += size, blk_paddr += size) {
+		arm_v7s_iopte *tablep;
+
+		/* Unmap! */

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		if (blk_start == iova)
+			continue;
+
+		/* __arm_v7s_map expects a pointer to the start of the table */
+		tablep = &table - ARM_V7S_LVL_IDX(blk_start, 1);

The "table" seems not be initialized here. (LPAE too.) maybe I don't
get it here.

It took me about 2 hours of staring at the original code to fully get my head round it, too ;) The comment would perhaps be better worded as "__arm_v7s_map expects a pointer to the start of *a* table". What we're doing is building up the whole level 2 table (with the relevant pages mapped) in advance _before_ touching the live level 1 table. Since __arm_v7s_map() is going to take the table pointer we pass and write an entry for the new level 2 table at the relevant index, we construct _just that entry_ ("arm_v7s_iopte table = 0;") and fake up a corresponding 'level 1 table pointer' - chances are that tablep itself is dangling way off the bottom of the stack somewhere, but the only entry in that 'table' that's going to be dereferenced is the one backed by the local variable.

Quoted 13 lines

If we would like to get the start of the table, maybe :

tablep = ptep - ARM_V7S_LVL_IDX(blk_start, 1);

Even though we change "tablep" here, it will also fail in the
__arm_v7s_map since there is a value in current ptep(the pa of the section), then it will call iopte_deref instread of creating a new pgtable in the __arm_v7s_map below, then it will abort.

so maybe we need unmap the ptep before this "for" loop. __arm_v7s_set_pte(ptep, 0, 1, cfg);

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+		if (__arm_v7s_map(data, blk_start, blk_paddr, size, prot, 1,
+				  tablep) < 0) {
+			if (table) {
+				/* Free the table we allocated */
+				tablep = iopte_deref(table, 1);
+				__arm_v7s_free_table(tablep, 2, data);

Following Will's quesion before, do we need flush tlb here?

No; this is just cleaning up the uncommitted preparatory work if map failure left a partially-created next-level table - in fact, now that I look at it, with only two levels the only way that can happen is if we allocate a new empty table but find nonzero entries when installing the PTEs, and that suggests a level of system corruption I'm not sure it's even worth trying to handle.

Anyway, from the IOMMU's perspective, at this point it knows nothing about the new table and the section mapping is still live...

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+			}
+			return 0; /* Bytes unmapped */
+		}
+	}
+
+	__arm_v7s_set_pte(ptep, table, 1, cfg);

Is this in order to update the lvl2 pgtable base address? why it's
not updated in __arm_v7s_map which create a lvl2 pgtable?

...until here, when we drop the table entry over the top of the section entry and TLBI the section, for an atomic valid->valid transition.

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	iova &= ~(blk_size - 1);
+	cfg->tlb->tlb_add_flush(iova, blk_size, blk_size, true, data->iop.cookie);
+	return size;
+}
+

All the others looks good for me. Thanks.

Cool, thanks. I'll send out a proper v3 once everyone's finished with merge window stuff, but below is the local diff I now have.

Robin.

Add some simple wrappers to avoid having the guts of the TLB operations spilled all over the page table implementations, and to provide a point to implement extra common functionality.

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

---
 drivers/iommu/io-pgtable-arm-v7s.c | 48 ++++++++++++++++----------------------
 drivers/iommu/io-pgtable-arm.c     | 21 +++++++----------
 drivers/iommu/io-pgtable.c         |  2 +-
 drivers/iommu/io-pgtable.h         | 16 +++++++++++++
 4 files changed, 46 insertions(+), 41 deletions(-)

Add some simple wrappers to avoid having the guts of the TLB operations spilled all over the page table implementations, and to provide a point to implement extra common functionality.

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

---
 drivers/iommu/io-pgtable-arm-v7s.c | 48 ++++++++++++++++-------------------
 drivers/iommu/io-pgtable-arm.c     | 21 +++++++----------
 drivers/iommu/io-pgtable.c         |  2 +-
 drivers/iommu/io-pgtable.h         | 16 +++++++++++++
 4 files changed, 46 insertions(+), 41 deletions(-)

(quoted diff) +0 -0

b/drivers/iommu/io-pgtable-arm-v7s.c index 17800db..3164b6b 100644

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

@@ -416,9 +416,7 @@ static int arm_v7s_map(struct io_pgtable_ops *ops,
 unsigned long iova, phys_addr_t paddr, size_t size, int prot)
 {
 	struct arm_v7s_io_pgtable *data = io_pgtable_ops_to_data(ops);
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
-	const struct iommu_gather_ops *tlb = cfg->tlb;
-	void *cookie = data->iop.cookie;
+	struct io_pgtable *iop = &data->iop;
 	int ret;
 
 	/* If no access, then nothing to do */

@@ -430,10 +428,10 @@ static int arm_v7s_map(struct io_pgtable_ops *ops,

unsigned long iova, * Synchronise all PTE updates for the new mapping before there's * a chance for anything to kick off a table walk for the new iova. */

drivers/iommu/io-pgtable-arm-v7s.c +19 -25

-	if (cfg->quirks & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
-		tlb->tlb_add_flush(iova, size, ARM_V7S_BLOCK_SIZE(2), false,
-				   cookie);
-		tlb->tlb_sync(cookie);
+	if (iop->cfg.quirks & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
+		io_pgtable_tlb_add_flush(iop, iova, size,
+					 ARM_V7S_BLOCK_SIZE(2), false);
+		io_pgtable_tlb_sync(iop);
 	} else {
 		wmb();
 	}

@@ -461,8 +459,7 @@ static void arm_v7s_split_cont(struct arm_v7s_io_pgtable
 *data, unsigned long iova, int idx, int lvl,
 			       arm_v7s_iopte *ptep)
 {
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
-	void *cookie = data->iop.cookie;
+	struct io_pgtable *iop = &data->iop;
 	arm_v7s_iopte pte;
 	size_t size = ARM_V7S_BLOCK_SIZE(lvl);
 	int i;

@@ -474,11 +471,11 @@ static void arm_v7s_split_cont(struct
 arm_v7s_io_pgtable *data, pte += size;
 	}
 
-	__arm_v7s_pte_sync(ptep, ARM_V7S_CONT_PAGES, cfg);
+	__arm_v7s_pte_sync(ptep, ARM_V7S_CONT_PAGES, &iop->cfg);
 
 	size *= ARM_V7S_CONT_PAGES;
-	cfg->tlb->tlb_add_flush(iova, size, size, true, cookie);
-	cfg->tlb->tlb_sync(cookie);
+	io_pgtable_tlb_add_flush(iop, iova, size, size, true);
+	io_pgtable_tlb_sync(iop);
 }
 
 static int arm_v7s_split_blk_unmap(struct arm_v7s_io_pgtable *data,

@@ -488,7 +485,6 @@ static int arm_v7s_split_blk_unmap(struct
 arm_v7s_io_pgtable *data, unsigned long blk_start, blk_end, blk_size;
 	phys_addr_t blk_paddr;
 	arm_v7s_iopte table = 0;
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
 	int prot = arm_v7s_pte_to_prot(*ptep, 1);
 
 	blk_size = ARM_V7S_BLOCK_SIZE(1);

@@ -516,9 +512,9 @@ static int arm_v7s_split_blk_unmap(struct
 arm_v7s_io_pgtable *data, }
 	}
 
-	__arm_v7s_set_pte(ptep, table, 1, cfg);
+	__arm_v7s_set_pte(ptep, table, 1, &data->iop.cfg);
 	iova &= ~(blk_size - 1);
-	cfg->tlb->tlb_add_flush(iova, blk_size, blk_size, true, data-
 iop.cookie);
+	io_pgtable_tlb_add_flush(&data->iop, iova, blk_size, blk_size, true);
 return size;
 }

@@ -527,9 +523,7 @@ static int __arm_v7s_unmap(struct arm_v7s_io_pgtable
 *data, 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;
+	struct io_pgtable *iop = &data->iop;
 	int idx, i = 0, num_entries = size >> ARM_V7S_LVL_SHIFT(lvl);
 
 	/* Something went horribly wrong and we ran out of page table */

@@ -555,20 +549,19 @@ static int __arm_v7s_unmap(struct arm_v7s_io_pgtable
 *data, if (num_entries) {
 		size_t blk_size = ARM_V7S_BLOCK_SIZE(lvl);
 
-		__arm_v7s_set_pte(ptep, 0, num_entries, cfg);
+		__arm_v7s_set_pte(ptep, 0, num_entries, &iop->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(lvl + 1),
-						   false, cookie);
-				tlb->tlb_sync(cookie);
+				io_pgtable_tlb_add_flush(iop, iova, blk_size,
+					ARM_V7S_BLOCK_SIZE(lvl + 1), false);
+				io_pgtable_tlb_sync(iop);
 				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);
+				io_pgtable_tlb_add_flush(iop, iova, blk_size,
+							 blk_size, true);
 			}
 			iova += blk_size;
 		}

@@ -589,13 +582,12 @@ static int __arm_v7s_unmap(struct arm_v7s_io_pgtable
 *data, 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;
+	size_t unmapped;
 
 	unmapped = __arm_v7s_unmap(data, iova, size, 1, data->pgd);
 	if (unmapped)
-		iop->cfg.tlb->tlb_sync(iop->cookie);
+		io_pgtable_tlb_sync(&data->iop);
 
 	return unmapped;
 }

drivers/iommu/io-pgtable-arm.c +8 -10

index 8bbcbfe..4095af2 100644
@@ -445,7 +445,6 @@ static int arm_lpae_split_blk_unmap(struct
 arm_lpae_io_pgtable *data, unsigned long blk_start, blk_end;
 	phys_addr_t blk_paddr;
 	arm_lpae_iopte table = 0;
-	struct io_pgtable_cfg *cfg = &data->iop.cfg;
 
 	blk_start = iova & ~(blk_size - 1);
 	blk_end = blk_start + blk_size;

@@ -471,9 +470,9 @@ static int arm_lpae_split_blk_unmap(struct
 arm_lpae_io_pgtable *data, }
 	}
 
-	__arm_lpae_set_pte(ptep, table, cfg);
+	__arm_lpae_set_pte(ptep, table, &data->iop.cfg);
 	iova &= ~(blk_size - 1);
-	cfg->tlb->tlb_add_flush(iova, blk_size, blk_size, true, data-
 iop.cookie);
+	io_pgtable_tlb_add_flush(&data->iop, iova, blk_size, blk_size, true);
 return size;
 }

@@ -482,8 +481,7 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable
 *data, arm_lpae_iopte *ptep)
 {
 	arm_lpae_iopte pte;
-	const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
-	void *cookie = data->iop.cookie;
+	struct io_pgtable *iop = &data->iop;
 	size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
 
 	/* Something went horribly wrong and we ran out of page table */

@@ -497,17 +495,17 @@ static int __arm_lpae_unmap(struct arm_lpae_io_pgtable
 *data,
 
 	/* If the size matches this level, we're in the right place */
 	if (size == blk_size) {
-		__arm_lpae_set_pte(ptep, 0, &data->iop.cfg);
+		__arm_lpae_set_pte(ptep, 0, &iop->cfg);
 
 		if (!iopte_leaf(pte, lvl)) {
 			/* Also flush any partial walks */
-			tlb->tlb_add_flush(iova, size, ARM_LPAE_GRANULE(data),
-					   false, cookie);
-			tlb->tlb_sync(cookie);
+			io_pgtable_tlb_add_flush(iop, iova, size,
+						ARM_LPAE_GRANULE(data), false);
+			io_pgtable_tlb_sync(iop);
 			ptep = iopte_deref(pte, data);
 			__arm_lpae_free_pgtable(data, lvl + 1, ptep);
 		} else {
-			tlb->tlb_add_flush(iova, size, size, true, cookie);
+			io_pgtable_tlb_add_flush(iop, iova, size, size, true);
 		}
 

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

 		return size;

@@ -531,13 +529,12 @@ static int arm_lpae_unmap(struct io_pgtable_ops *ops,
 unsigned long iova, {
 	size_t unmapped;
 	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
-	struct io_pgtable *iop = &data->iop;
 	arm_lpae_iopte *ptep = data->pgd;
 	int lvl = ARM_LPAE_START_LVL(data);
 
 	unmapped = __arm_lpae_unmap(data, iova, size, lvl, ptep);
 	if (unmapped)
-		iop->cfg.tlb->tlb_sync(iop->cookie);
+		io_pgtable_tlb_sync(&data->iop);
 
 	return unmapped;
 }

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

index 8c615b7..876f6a7 100644
@@ -75,6 +75,6 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops)
 		return;
 
 	iop = container_of(ops, struct io_pgtable, ops);
-	iop->cfg.tlb->tlb_flush_all(iop->cookie);
+	io_pgtable_tlb_flush_all(iop);
 	io_pgtable_init_table[iop->fmt]->free(iop);
 }

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

index aa57073..95c5565 100644
@@ -144,6 +144,22 @@ struct io_pgtable {
 
 #define io_pgtable_ops_to_pgtable(x) container_of((x), struct io_pgtable,
 ops)
 
+static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop)
+{
+	iop->cfg.tlb->tlb_flush_all(iop->cookie);
+}
+
+static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop,
+		unsigned long iova, size_t size, size_t granule, bool leaf)
+{
+	iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie);
+}
+
+static inline void io_pgtable_tlb_sync(struct io_pgtable *iop)
+{
+	iop->cfg.tlb->tlb_sync(iop->cookie);
+}
+
 /**
  * struct io_pgtable_init_fns - Alloc/free a set of page tables for a

* particular format.

In certain unmapping situations it is quite possible to end up issuing back-to-back TLB synchronisations, which at best is a waste of time and effort, and at worst causes some hardware to get rather confused. Whilst the pagetable implementations, or the IOMMU drivers, or both, could keep track of things to avoid this happening, it seems to make the most sense to prevent code duplication and add some simple state tracking in the common interface between the two.

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

---
 drivers/iommu/io-pgtable.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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

index 95c5565..d06219b 100644

Quoted 13 lines · Robin Murphy

In certain unmapping situations it is quite possible to end up issuing back-to-back TLB synchronisations, which at best is a waste of time and effort, and at worst causes some hardware to get rather confused. Whilst the pagetable implementations, or the IOMMU drivers, or both, could keep track of things to avoid this happening, it seems to make the most sense to prevent code duplication and add some simple state tracking in the common interface between the two.

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

---
  drivers/iommu/io-pgtable.h | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/iommu/io-pgtable.h void free_io_pgtable_ops(struct io_pgtable_ops *ops); · line 135

Quoted 9 lines

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

index 95c5565..d06219b 100644
@@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops);
   * @fmt:    The page table format.
   * @cookie: An opaque token provided by the IOMMU driver and passed back to
   *          any callback routines.
+ * @sync_flag: Private flag for optimising out redundant syncs.

It makes sense to factor this out like you're proposing, but maybe we can think of a better name? How about "tlb_sync_pending", to follow "tlb_flush_pending" in the core code?

Quoted 14 lines · Will Deacon

Quoted 13 lines · Robin Murphy

In certain unmapping situations it is quite possible to end up issuing back-to-back TLB synchronisations, which at best is a waste of time and effort, and at worst causes some hardware to get rather confused. Whilst the pagetable implementations, or the IOMMU drivers, or both, could keep track of things to avoid this happening, it seems to make the most sense to prevent code duplication and add some simple state tracking in the common interface between the two.

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

---
  drivers/iommu/io-pgtable.h | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

Quoted 15 lines

Quoted 11 lines

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

index 95c5565..d06219b 100644
@@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops
 *ops);
   * @fmt:    The page table format.
   * @cookie: An opaque token provided by the IOMMU driver and passed

back to
* any callback routines.
+ * @sync_flag: Private flag for optimising out redundant syncs.

It makes sense to factor this out like you're proposing, but maybe we can think of a better name? How about "tlb_sync_pending", to follow "tlb_flush_pending" in the core code?

Ooh, tlb_flush_pending is a much nicer name indeed. It's almost as if I

threw this together in a pre-holiday rush and put very little thought into it...

I've fixed it up locally, but I'll save the repost until after -rc1, especially in case Yong has any further comments in the meantime.

Thanks,
Robin.

_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Quoted 26 lines · Robin Murphy

In certain unmapping situations it is quite possible to end up issuing back-to-back TLB synchronisations, which at best is a waste of time and effort, and at worst causes some hardware to get rather confused. Whilst the pagetable implementations, or the IOMMU drivers, or both, could keep track of things to avoid this happening, it seems to make the most sense to prevent code duplication and add some simple state tracking in the common interface between the two.

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

---

 drivers/iommu/io-pgtable.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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

index 95c5565..d06219b 100644
@@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops
 *ops);
 
  * @fmt:    The page table format.
  * @cookie: An opaque token provided by the IOMMU driver and passed back
  to
  *          any callback routines.
 

drivers/iommu/io-pgtable.h void free_io_pgtable_ops(struct io_pgtable_ops *ops); · line 135

Quoted 1 line

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

+ * @sync_flag: Private flag for optimising out redundant syncs.

It makes sense to factor this out like you're proposing, but maybe we can think of a better name? How about "tlb_sync_pending", to follow "tlb_flush_pending" in the core code?