iommu/arm-smmu: Updates for 4.3

From: Robin Murphy <[email protected]>

Currently, users of the LPAE page table code are (ab)using dma_map_page() as a means to flush page table updates for non-coherent IOMMUs. Since from the CPU's point of view, creating IOMMU page tables *is* passing DMA buffers to a device (the IOMMU's page table walker), there's little reason not to use the DMA API correctly.

Allow IOMMU drivers to opt into DMA API operations for page table allocation and updates by providing their appropriate device pointer. The expectation is that an LPAE IOMMU should have a full view of system memory, so use streaming mappings to avoid unnecessary pressure on ZONE_DMA, and treat any DMA translation as a warning sign.

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

Signed-off-by Will Deacon <[email protected]>

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

drivers/iommu/Kconfig +2 -1

index f1fb1d3ccc56..d77a848d50de 100644
@@ -23,7 +23,8 @@ config IOMMU_IO_PGTABLE
 config IOMMU_IO_PGTABLE_LPAE
 	bool "ARMv7/v8 Long Descriptor Format"
 	select IOMMU_IO_PGTABLE
-	depends on ARM || ARM64 || COMPILE_TEST
+	# SWIOTLB guarantees a dma_to_phys() implementation
+	depends on ARM || ARM64 || (COMPILE_TEST && SWIOTLB)
 	help
 	  Enable support for the ARM long descriptor pagetable format.
 	  This allocator supports 4K/2M/1G, 16K/32M and 64K/512M page

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

index 4e460216bd16..28cca8a652f9 100644
@@ -200,12 +200,76 @@ typedef u64 arm_lpae_iopte;
 
 static bool selftest_running = false;
 
+static dma_addr_t __arm_lpae_dma_addr(struct device *dev, void *pages)
+{
+	return phys_to_dma(dev, virt_to_phys(pages));
+}
+
+static void *__arm_lpae_alloc_pages(size_t size, gfp_t gfp,
+				    struct io_pgtable_cfg *cfg)
+{
+	struct device *dev = cfg->iommu_dev;
+	dma_addr_t dma;
+	void *pages = alloc_pages_exact(size, gfp | __GFP_ZERO);
+
+	if (!pages)
+		return NULL;
+
+	if (dev) {

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

+	}
+
+	return pages;
+
+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:
+	free_pages_exact(pages, size);
+	return NULL;
+}
+
+static void __arm_lpae_free_pages(void *pages, size_t size,
+				  struct io_pgtable_cfg *cfg)
+{
+	struct device *dev = cfg->iommu_dev;
+
+	if (dev)
+		dma_unmap_single(dev, __arm_lpae_dma_addr(dev, pages),
+				 size, DMA_TO_DEVICE);
+	free_pages_exact(pages, size);
+}
+

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

+	else if (cfg->tlb->flush_pgtable)
+		cfg->tlb->flush_pgtable(ptep, sizeof(pte), cookie);
+}
+
 static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
 			     unsigned long iova, phys_addr_t paddr,
 			     arm_lpae_iopte prot, int lvl,
 			     arm_lpae_iopte *ptep)
 {
 	arm_lpae_iopte pte = prot;
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
 
 	/* We require an unmap first */

drivers/iommu/io-pgtable-arm.c +14 -19

 	if (iopte_leaf(*ptep, lvl)) {

@@ -213,7 +277,7 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable
 *data, return -EEXIST;
 	}
 
-	if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
+	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
 		pte |= ARM_LPAE_PTE_NS;
 
 	if (lvl == ARM_LPAE_MAX_LEVELS - 1)

@@ -224,8 +288,7 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable
 *data, pte |= ARM_LPAE_PTE_AF | ARM_LPAE_PTE_SH_IS;
 	pte |= pfn_to_iopte(paddr >> data->pg_shift, data);
 
-	*ptep = pte;
-	data->iop.cfg.tlb->flush_pgtable(ptep, sizeof(*ptep), data->iop.cookie);
+	__arm_lpae_set_pte(ptep, pte, cfg, data->iop.cookie);
 	return 0;
 }
 

@@ -236,12 +299,13 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable
 *data, unsigned long iova, arm_lpae_iopte *cptep, pte;
 	void *cookie = data->iop.cookie;
 	size_t block_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
 
 	/* Find our entry at the current level */
 	ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
 
 	/* If we can install a leaf entry at this level, then do so */
-	if (size == block_size && (size & data->iop.cfg.pgsize_bitmap))
+	if (size == block_size && (size & cfg->pgsize_bitmap))
 		return arm_lpae_init_pte(data, iova, paddr, prot, lvl, ptep);
 
 	/* We can't allocate tables at the final level */

@@ -251,18 +315,15 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable
 *data, unsigned long iova, /* Grab a pointer to the next level */
 	pte = *ptep;
 	if (!pte) {
-		cptep = alloc_pages_exact(1UL << data->pg_shift,
-					 GFP_ATOMIC | __GFP_ZERO);
+		cptep = __arm_lpae_alloc_pages(1UL << data->pg_shift,
+					       GFP_ATOMIC, cfg);
 		if (!cptep)
 			return -ENOMEM;
 
-		data->iop.cfg.tlb->flush_pgtable(cptep, 1UL << data->pg_shift,
-						 cookie);
 		pte = __pa(cptep) | ARM_LPAE_PTE_TYPE_TABLE;
-		if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_NS)
+		if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
 			pte |= ARM_LPAE_PTE_NSTABLE;
-		*ptep = pte;
-		data->iop.cfg.tlb->flush_pgtable(ptep, sizeof(*ptep), cookie);
+		__arm_lpae_set_pte(ptep, pte, cfg, cookie);
 	} else {
 		cptep = iopte_deref(pte, data);
 	}

@@ -347,7 +408,7 @@ static void __arm_lpae_free_pgtable(struct
 arm_lpae_io_pgtable *data, int lvl, __arm_lpae_free_pgtable(data, lvl + 1,
 iopte_deref(pte, data)); }
 
-	free_pages_exact(start, table_size);
+	__arm_lpae_free_pages(start, table_size, &data->iop.cfg);
 }
 
 static void arm_lpae_free_pgtable(struct io_pgtable *iop)

@@ -366,8 +427,8 @@ 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;
 	void *cookie = data->iop.cookie;
-	const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
 
 	blk_start = iova & ~(blk_size - 1);
 	blk_end = blk_start + blk_size;

@@ -393,10 +454,9 @@ static int arm_lpae_split_blk_unmap(struct
 arm_lpae_io_pgtable *data, }
 	}
 
-	*ptep = table;
-	tlb->flush_pgtable(ptep, sizeof(*ptep), cookie);
+	__arm_lpae_set_pte(ptep, table, cfg, cookie);
 	iova &= ~(blk_size - 1);
-	tlb->tlb_add_flush(iova, blk_size, true, cookie);
+	cfg->tlb->tlb_add_flush(iova, blk_size, true, cookie);
 	return size;
 }
 

@@ -418,13 +478,12 @@ 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) {
-		*ptep = 0;
-		tlb->flush_pgtable(ptep, sizeof(*ptep), cookie);
+		__arm_lpae_set_pte(ptep, 0, &data->iop.cfg, cookie);
 
 		if (!iopte_leaf(pte, lvl)) {
 			/* Also flush any partial walks */
 			tlb->tlb_add_flush(iova, size, false, cookie);
-			tlb->tlb_sync(data->iop.cookie);
+			tlb->tlb_sync(cookie);
 			ptep = iopte_deref(pte, data);
 			__arm_lpae_free_pgtable(data, lvl + 1, ptep);

drivers/iommu/io-pgtable-arm.c +6 -4

 		} else {

@@ -640,11 +699,12 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg
 *cfg, void *cookie) cfg->arm_lpae_s1_cfg.mair[1] = 0;
 
 	/* Looking good; allocate a pgd */
-	data->pgd = alloc_pages_exact(data->pgd_size, GFP_KERNEL | __GFP_ZERO);
+	data->pgd = __arm_lpae_alloc_pages(data->pgd_size, GFP_KERNEL, cfg);
 	if (!data->pgd)
 		goto out_free_data;
 
-	cfg->tlb->flush_pgtable(data->pgd, data->pgd_size, cookie);
+	if (cfg->tlb->flush_pgtable)
+		cfg->tlb->flush_pgtable(data->pgd, data->pgd_size, cookie);
 
 	/* TTBRs */
 	cfg->arm_lpae_s1_cfg.ttbr[0] = virt_to_phys(data->pgd);

@@ -728,11 +788,12 @@ arm_64_lpae_alloc_pgtable_s2(struct io_pgtable_cfg
 *cfg, void *cookie) cfg->arm_lpae_s2_cfg.vtcr = reg;
 
 	/* Allocate pgd pages */
-	data->pgd = alloc_pages_exact(data->pgd_size, GFP_KERNEL | __GFP_ZERO);
+	data->pgd = __arm_lpae_alloc_pages(data->pgd_size, GFP_KERNEL, cfg);
 	if (!data->pgd)
 		goto out_free_data;
 
-	cfg->tlb->flush_pgtable(data->pgd, data->pgd_size, cookie);
+	if (cfg->tlb->flush_pgtable)
+		cfg->tlb->flush_pgtable(data->pgd, data->pgd_size, cookie);
 
 	/* VTTBR */
 	cfg->arm_lpae_s2_cfg.vttbr = virt_to_phys(data->pgd);

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

index 10e32f69c668..c69529c78914 100644
@@ -41,6 +41,8 @@ struct iommu_gather_ops {
  * @ias:           Input address (iova) size, in bits.
  * @oas:           Output address (paddr) size, in bits.
  * @tlb:           TLB management callbacks for this set of tables.
+ * @iommu_dev:     The device representing the DMA configuration for the
+ *                 page table walker.
  */
 struct io_pgtable_cfg {
 	#define IO_PGTABLE_QUIRK_ARM_NS	(1 << 0)	/* Set NS bit in PTEs */

@@ -49,6 +51,7 @@ struct io_pgtable_cfg {
 	unsigned int			ias;
 	unsigned int			oas;
 	const struct iommu_gather_ops	*tlb;
+	struct device			*iommu_dev;
 
 	/* Low-level data specific to the table format */
 	union {

Hi Will and Robin,

Thank you for the patch.

Quoted 8 lines · On Monday 03 August 2015 14:25:47 Will Deacon wrote:

From: Robin Murphy <[email protected]>

Currently, users of the LPAE page table code are (ab)using dma_map_page() as a means to flush page table updates for non-coherent IOMMUs. Since from the CPU's point of view, creating IOMMU page tables *is* passing DMA buffers to a device (the IOMMU's page table walker), there's little reason not to use the DMA API correctly.

Quoted 5 lines

Allow IOMMU drivers to opt into DMA API operations for page table allocation and updates by providing their appropriate device pointer. The expectation is that an LPAE IOMMU should have a full view of system memory, so use streaming mappings to avoid unnecessary pressure on ZONE_DMA, and treat any DMA translation as a warning sign.

I like the idea of doing this in core code rather than in individual drivers, but I believe we're not using the right API. Please see below.

Quoted 51 lines

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

Signed-off-by Will Deacon <[email protected]>

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

drivers/iommu/Kconfig +2 -1

index f1fb1d3ccc56..d77a848d50de 100644
@@ -23,7 +23,8 @@ config IOMMU_IO_PGTABLE
  config IOMMU_IO_PGTABLE_LPAE
  	bool "ARMv7/v8 Long Descriptor Format"
  	select IOMMU_IO_PGTABLE
-	depends on ARM || ARM64 || COMPILE_TEST
+	# SWIOTLB guarantees a dma_to_phys() implementation
+	depends on ARM || ARM64 || (COMPILE_TEST && SWIOTLB)
  	help
  	  Enable support for the ARM long descriptor pagetable format.
  	  This allocator supports 4K/2M/1G, 16K/32M and 64K/512M page

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

index 4e460216bd16..28cca8a652f9 100644
@@ -200,12 +200,76 @@ typedef u64 arm_lpae_iopte;
 
  static bool selftest_running = false;
 
+static dma_addr_t __arm_lpae_dma_addr(struct device *dev, void *pages)
+{
+	return phys_to_dma(dev, virt_to_phys(pages));
+}
+
+static void *__arm_lpae_alloc_pages(size_t size, gfp_t gfp,
+				    struct io_pgtable_cfg *cfg)
+{
+	struct device *dev = cfg->iommu_dev;
+	dma_addr_t dma;
+	void *pages = alloc_pages_exact(size, gfp | __GFP_ZERO);
+
+	if (!pages)
+		return NULL;
+
+	if (dev) {
+		dma = dma_map_single(dev, pages, 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

drivers/iommu/io-pgtable-arm.c typedef u64 arm_lpae_iopte; · line 228

Quoted 5 lines

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

+		 * address directly, so if the DMA layer suggests it can't by
+		 * giving us back some translation, that bodes very badly...
+		 */
+		if (dma != __arm_lpae_dma_addr(dev, pages))
+			goto out_unmap;

Why do we need to create a mapping at all then ? Because dma_sync_single_for_device() requires it ?

Quoted 24 lines

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

+	}
+
+	return pages;
+
+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:
+	free_pages_exact(pages, size);
+	return NULL;
+}
+
+static void __arm_lpae_free_pages(void *pages, size_t size,
+				  struct io_pgtable_cfg *cfg)
+{
+	struct device *dev = cfg->iommu_dev;
+
+	if (dev)
+		dma_unmap_single(dev, __arm_lpae_dma_addr(dev, pages),
+				 size, DMA_TO_DEVICE);
+	free_pages_exact(pages, size);
+}
+

drivers/iommu/io-pgtable-arm.c typedef u64 arm_lpae_iopte; · line 261

Quoted 10 lines

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

+static void __arm_lpae_set_pte(arm_lpae_iopte *ptep, arm_lpae_iopte pte,
+			       struct io_pgtable_cfg *cfg, void *cookie)
+{
+	struct device *dev = cfg->iommu_dev;
+
+	*ptep = pte;
+
+	if (dev)
+		dma_sync_single_for_device(dev, __arm_lpae_dma_addr(dev, ptep),
+					   sizeof(pte), DMA_TO_DEVICE);

This is what I believe to be an API abuse. The dma_sync_single_for_device() API is meant to pass ownership of a buffer to the device. Unless I'm mistaken, once that's done the CPU isn't allowed to touch the buffer anymore until dma_sync_single_for_cpu() is called to get ownership of the buffer back. Sure, it might work on many ARM systems, but we really should be careful not to use APIs as delicate as DMA mapping and cache handling for purposes different than what they explicitly allow.

It might be that I'm wrong and that the streaming DMA API allows this exact kind of usage, but I haven't found a clear indication of that in the documentation. It could also be that all implementations would support it today, and that we would then consider it should be explicitly allowed by the API. In both cases a documentation patch would be welcome.

Hi Laurent,

[ +RMK, as his patch is indirectly involved here ]

Quoted 4 lines · Laurent Pinchart

This is what I believe to be an API abuse. The dma_sync_single_for_device() API is meant to pass ownership of a buffer to the device. Unless I'm mistaken, once that's done the CPU isn't allowed to touch the buffer anymore until dma_sync_single_for_cpu() is called to get ownership of the buffer back.

[3]:Yes, there may generally be exceptions to that, but not in the context of this code. Unless the Renesas IPMMU does something I don't know about?

Quoted 11 lines · Robin Murphy

Hi Laurent,

[ +RMK, as his patch is indirectly involved here ]

Quoted 6 lines · Laurent Pinchart

This is what I believe to be an API abuse. The dma_sync_single_for_device()
API is meant to pass ownership of a buffer to the device. Unless I'm mistaken, once that's done the CPU isn't allowed to touch the buffer anymore until dma_sync_single_for_cpu() is called to get ownership of the buffer back.

That's what I thought up until recently, but it's not strictly true - see Documentation/DMA-API.txt which Robin quoted.

Quoted 2 lines

[3]:Yes, there may generally be exceptions to that, but not in the context of this code. Unless the Renesas IPMMU does something I don't know about?

If an IOMMU does write to its page tables, then the only way to handle those is using DMA-coherent memory, either via a coherent mapping or allocated via dma_alloc_coherent(). The streaming DMA API is wholely unsuitable for any mapping where both the CPU and DMA device both want to simultaneously alter the contained data.

Quoted 11 lines · On Tuesday 04 August 2015 15:56:42 Russell King - ARM Linux wrote:

Quoted 7 lines · Robin Murphy

Quoted 6 lines · Laurent Pinchart

This is what I believe to be an API abuse. The dma_sync_single_for_device()
API is meant to pass ownership of a buffer to the device. Unless I'm mistaken, once that's done the CPU isn't allowed to touch the buffer anymore until dma_sync_single_for_cpu() is called to get ownership of the buffer back.

That's what I thought up until recently, but it's not strictly true - see Documentation/DMA-API.txt which Robin quoted.

I find the documentation slightly unclear on this topic. I have nothing against updating it to state that the sync functions ensure visibility of the data to the device or CPU instead of transferring ownership if that's the general understanding of how the API work (or should work). This would of course need to be carefully reviewed to ensure that the current implementation really works that way across all architectures, or at least that it can be made to work that way.

Quoted 13 lines · Laurent Pinchart

Quoted 11 lines · On Tuesday 04 August 2015 15:56:42 Russell King - ARM Linux wrote:

Quoted 7 lines · Robin Murphy

Quoted 6 lines · Laurent Pinchart

This is what I believe to be an API abuse. The dma_sync_single_for_device()
API is meant to pass ownership of a buffer to the device. Unless I'm mistaken, once that's done the CPU isn't allowed to touch the buffer anymore until dma_sync_single_for_cpu() is called to get ownership of the buffer back.

That's what I thought up until recently, but it's not strictly true - see Documentation/DMA-API.txt which Robin quoted.

Quoted 7 lines

I find the documentation slightly unclear on this topic. I have nothing against updating it to state that the sync functions ensure visibility of the data to the device or CPU instead of transferring ownership if that's the general understanding of how the API work (or should work). This would of course need to be carefully reviewed to ensure that the current implementation really works that way across all architectures, or at least that it can be made to work that way.

I was hoping to send this lot off to Joerg tomorrow morning, since it's removing a hack from the io-pgtable users before it has chance to spread further. This use of the API is certainly ok on arm and arm64 (where these drivers currently run) and I don't see why it wouldn't work for any IOMMU that treats the page tables as read-only. Once we start getting into hardware access/dirty bits, then the streaming-DMA API is a lost cause but I would expect those systems to be using SVM with the CPU page tables anyway and therefore be cache-coherent.

I agree that the Documentation isn't brilliant, but that could be addressed in a separate patch (especially since I don't think it would be merged via the iommu tree).

Is that alright?

A late change to the SMMUv3 architecture ensures that the OAS field will be monotonically increasing, so we can assume that an unknown OAS is at least 48-bit and use that, rather than fail the device probe.

Signed-off-by Will Deacon <[email protected]>

---
  drivers/iommu/arm-smmu-v3.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

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

index 15fb669282b4..a435e706226a 100644