[RFC PATCH v2 0/3] arm64: IOMMU-backed DMA mapping

[RFC PATCH v2 0/3] arm64: IOMMU-backed DMA mapping

[Robin Murphy]

Hi all,

This is an updated RFC to address some of the initial comments[1].
The first two patches of the original posting, along with the IOVA
series, are now in -next so aren't included here.

If this is starting to look tidy enough, then I'll get to work on
porting arch/arm as well so I can repost as a formal patch set.

Thanks,
Robin.


A branch with all the necessary bits is available at:

   git://linux-arm.org/linux-rm iommu/dma

Main changes since v1:
* rebased to -rc7
* moved common code to drivers/iommu, and tidied up the config
  dependencies
* renamed iommu_dma_mapping to better reflect that it's a managed domain
* abstracted away the explicit use of archdata
* finished the map_sg implementation
* fixed up the mmap implementation with the arch/arm one for now
* removed the iova_to_phys accessor in favour of (more flexibly)
  exposing the iommu domain for where it's needed

More complicated things still to do:
* understand and sort out the compound pages issue
* get domain and group allocation right - I'd like to try integrating
  Joerg's proposals for that area[2][3], which should help.

[1]:http://article.gmane.org/gmane.linux.kernel.iommu/8213
[2]:http://thread.gmane.org/gmane.linux.ports.tegra/20907
[3]:http://thread.gmane.org/gmane.linux.kernel.iommu/8492

Robin Murphy (3):
  iommu: implement common IOMMU ops for DMA mapping
  arm64: add IOMMU dma_ops
  arm64: hook up IOMMU dma_ops

 arch/arm64/Kconfig                   |   1 +
 arch/arm64/include/asm/device.h      |   3 +
 arch/arm64/include/asm/dma-mapping.h |  28 +-
 arch/arm64/mm/dma-mapping.c          | 335 +++++++++++++++++++++
 drivers/iommu/Kconfig                |   7 +
 drivers/iommu/Makefile               |   1 +
 drivers/iommu/dma-iommu.c            | 552 +++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h            |  94 ++++++
 8 files changed, 1016 insertions(+), 5 deletions(-)
 create mode 100644 drivers/iommu/dma-iommu.c
 create mode 100644 include/linux/dma-iommu.h

-- 
1.9.1

[RFC PATCH v2 1/3] iommu: implement common IOMMU ops for DMA mapping

[Robin Murphy]

Taking inspiration from the existing arch/arm code, break out some
generic functions to interface the DMA-API to the IOMMU-API. This will
do the bulk of the heavy lifting for IOMMU-backed dma-mapping.

Whilst this RFC series is aimed at enabling arm64, once any remaining
obvious issues in the common code are addressed we can complete the
refactoring by porting arch/arm over for a merge-worthy series.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/Kconfig     |   7 +
 drivers/iommu/Makefile    |   1 +
 drivers/iommu/dma-iommu.c | 552 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |  94 ++++++++
 4 files changed, 654 insertions(+)
 create mode 100644 drivers/iommu/dma-iommu.c
 create mode 100644 include/linux/dma-iommu.h

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index a839ca9..19027bb 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -20,6 +20,13 @@ config OF_IOMMU
        def_bool y
        depends on OF && IOMMU_API
 
+# IOMMU-agnostic DMA-mapping layer
+config IOMMU_DMA
+	def_bool n
+	depends on NEED_SG_DMA_LENGTH
+	select IOMMU_API
+	select IOMMU_IOVA
+
 config FSL_PAMU
 	bool "Freescale IOMMU support"
 	depends on PPC_E500MC
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 0b1b94e..37bfc4e 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_IOMMU_API) += iommu.o
 obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
+obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
 obj-$(CONFIG_IOMMU_IOVA) += iova.o
 obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o
 obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
new file mode 100644
index 0000000..b97cc0b9
--- /dev/null
+++ b/drivers/iommu/dma-iommu.c
@@ -0,0 +1,552 @@
+/*
+ * A fairly generic DMA-API to IOMMU-API glue layer.
+ *
+ * Copyright (C) 2014 ARM Ltd.
+ *
+ * based in part on arch/arm/mm/dma-mapping.c:
+ * Copyright (C) 2000-2004 Russell King
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define pr_fmt(fmt)	"%s: " fmt, __func__
+
+#include <linux/dma-contiguous.h>
+#include <linux/dma-iommu.h>
+#include <linux/iova.h>
+
+int iommu_dma_init(void)
+{
+	return iommu_iova_cache_init();
+}
+
+struct iommu_dma_domain {
+	struct iommu_domain *domain;
+	struct iova_domain *iovad;
+	struct kref kref;
+};
+
+static inline dma_addr_t dev_dma_addr(struct device *dev, dma_addr_t addr)
+{
+	BUG_ON(addr < dev->dma_pfn_offset);
+	return addr - ((dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
+}
+
+static int __dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
+{
+	int prot = coherent ? IOMMU_CACHE : 0;
+
+	switch (dir) {
+	case DMA_BIDIRECTIONAL:
+		return prot | IOMMU_READ | IOMMU_WRITE;
+	case DMA_TO_DEVICE:
+		return prot | IOMMU_READ;
+	case DMA_FROM_DEVICE:
+		return prot | IOMMU_WRITE;
+	default:
+		return 0;
+	}
+}
+
+static struct iova *__alloc_iova(struct device *dev, size_t size, bool coherent)
+{
+	struct iommu_dma_domain *dom = get_dma_domain(dev);
+	struct iova_domain *iovad = dom->iovad;
+	unsigned long shift = iova_shift(iovad);
+	unsigned long length = iova_align(iovad, size) >> shift;
+	unsigned long limit_pfn = iovad->dma_32bit_pfn;
+	u64 dma_limit = coherent ? dev->coherent_dma_mask : *dev->dma_mask;
+
+	limit_pfn = min(limit_pfn, (unsigned long)(dma_limit >> shift));
+	/* Alignment should probably come from a domain/device attribute... */
+	return alloc_iova(iovad, length, limit_pfn, false);
+}
+
+/*
+ * Create a mapping in device IO address space for specified pages
+ */
+dma_addr_t iommu_dma_create_iova_mapping(struct device *dev,
+		struct page **pages, size_t size, bool coherent)
+{
+	struct iommu_dma_domain *dom = get_dma_domain(dev);
+	struct iova_domain *iovad = dom->iovad;
+	struct iommu_domain *domain = dom->domain;
+	struct iova *iova;
+	unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	dma_addr_t addr_lo, addr_hi;
+	int i, prot = __dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+
+	iova = __alloc_iova(dev, size, coherent);
+	if (!iova)
+		return DMA_ERROR_CODE;
+
+	addr_hi = addr_lo = iova_dma_addr(iovad, iova);
+	for (i = 0; i < count; ) {
+		unsigned int next_pfn = page_to_pfn(pages[i]) + 1;
+		phys_addr_t phys = page_to_phys(pages[i]);
+		unsigned int len, j;
+
+		for (j = i+1; j < count; j++, next_pfn++)
+			if (page_to_pfn(pages[j]) != next_pfn)
+				break;
+
+		len = (j - i) << PAGE_SHIFT;
+		if (iommu_map(domain, addr_hi, phys, len, prot))
+			goto fail;
+		addr_hi += len;
+		i = j;
+	}
+	return dev_dma_addr(dev, addr_lo);
+fail:
+	iommu_unmap(domain, addr_lo, addr_hi - addr_lo);
+	__free_iova(iovad, iova);
+	return DMA_ERROR_CODE;
+}
+
+int iommu_dma_release_iova_mapping(struct device *dev, dma_addr_t iova,
+		size_t size)
+{
+	struct iommu_dma_domain *dom = get_dma_domain(dev);
+	struct iova_domain *iovad = dom->iovad;
+	size_t offset = iova_offset(iovad, iova);
+	size_t len = iova_align(iovad, size + offset);
+
+	iommu_unmap(dom->domain, iova - offset, len);
+	free_iova(iovad, iova_pfn(iovad, iova));
+	return 0;
+}
+
+struct page **iommu_dma_alloc_buffer(struct device *dev, size_t size,
+		gfp_t gfp, struct dma_attrs *attrs,
+		void (clear_buffer)(struct page *page, size_t size))
+{
+	struct page **pages;
+	int count = size >> PAGE_SHIFT;
+	int array_size = count * sizeof(struct page *);
+	int i = 0;
+
+	if (array_size <= PAGE_SIZE)
+		pages = kzalloc(array_size, GFP_KERNEL);
+	else
+		pages = vzalloc(array_size);
+	if (!pages)
+		return NULL;
+
+	if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
+		unsigned long order = get_order(size);
+		struct page *page;
+
+		page = dma_alloc_from_contiguous(dev, count, order);
+		if (!page)
+			goto error;
+
+		if (clear_buffer)
+			clear_buffer(page, size);
+
+		for (i = 0; i < count; i++)
+			pages[i] = page + i;
+
+		return pages;
+	}
+
+	/*
+	 * IOMMU can map any pages, so himem can also be used here
+	 */
+	gfp |= __GFP_NOWARN | __GFP_HIGHMEM;
+
+	while (count) {
+		int j, order = __fls(count);
+
+		pages[i] = alloc_pages(gfp, order);
+		while (!pages[i] && order)
+			pages[i] = alloc_pages(gfp, --order);
+		if (!pages[i])
+			goto error;
+
+		if (order) {
+			split_page(pages[i], order);
+			j = 1 << order;
+			while (--j)
+				pages[i + j] = pages[i] + j;
+		}
+
+		if (clear_buffer)
+			clear_buffer(pages[i], PAGE_SIZE << order);
+		i += 1 << order;
+		count -= 1 << order;
+	}
+
+	return pages;
+error:
+	while (i--)
+		if (pages[i])
+			__free_pages(pages[i], 0);
+	if (array_size <= PAGE_SIZE)
+		kfree(pages);
+	else
+		vfree(pages);
+	return NULL;
+}
+
+int iommu_dma_free_buffer(struct device *dev, struct page **pages, size_t size,
+		struct dma_attrs *attrs)
+{
+	int count = size >> PAGE_SHIFT;
+	int array_size = count * sizeof(struct page *);
+	int i;
+
+	if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
+		dma_release_from_contiguous(dev, pages[0], count);
+	} else {
+		for (i = 0; i < count; i++)
+			if (pages[i])
+				__free_pages(pages[i], 0);
+	}
+
+	if (array_size <= PAGE_SIZE)
+		kfree(pages);
+	else
+		vfree(pages);
+	return 0;
+}
+
+static dma_addr_t __iommu_dma_map_page(struct device *dev, struct page *page,
+		unsigned long offset, size_t size, enum dma_data_direction dir,
+		bool coherent)
+{
+	dma_addr_t dma_addr;
+	struct iommu_dma_domain *dom = get_dma_domain(dev);
+	struct iova_domain *iovad = dom->iovad;
+	phys_addr_t phys = page_to_phys(page) + offset;
+	size_t iova_off = iova_offset(iovad, phys);
+	size_t len = iova_align(iovad, size + iova_off);
+	int prot = __dma_direction_to_prot(dir, coherent);
+	struct iova *iova = __alloc_iova(dev, len, coherent);
+
+	if (!iova)
+		return DMA_ERROR_CODE;
+
+	dma_addr = iova_dma_addr(iovad, iova);
+	if (iommu_map(dom->domain, dma_addr, phys - iova_off, len, prot)) {
+		__free_iova(iovad, iova);
+		return DMA_ERROR_CODE;
+	}
+
+	return dev_dma_addr(dev, dma_addr + iova_off);
+}
+
+dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
+		unsigned long offset, size_t size, enum dma_data_direction dir,
+		struct dma_attrs *attrs)
+{
+	return __iommu_dma_map_page(dev, page, offset, size, dir, false);
+}
+
+dma_addr_t iommu_dma_coherent_map_page(struct device *dev, struct page *page,
+		unsigned long offset, size_t size, enum dma_data_direction dir,
+		struct dma_attrs *attrs)
+{
+	return __iommu_dma_map_page(dev, page, offset, size, dir, true);
+}
+
+void iommu_dma_unmap_page(struct device *dev, dma_addr_t handle, size_t size,
+		enum dma_data_direction dir, struct dma_attrs *attrs)
+{
+	struct iommu_dma_domain *dom = get_dma_domain(dev);
+	struct iova_domain *iovad = dom->iovad;
+	size_t offset = iova_offset(iovad, handle);
+	size_t len = iova_align(iovad, size + offset);
+	dma_addr_t iova = handle - offset;
+
+	if (!iova)
+		return;
+
+	iommu_unmap(dom->domain, iova, len);
+	free_iova(iovad, iova_pfn(iovad, iova));
+}
+
+static int finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
+		dma_addr_t dma_addr)
+{
+	struct scatterlist *s, *seg = sg;
+	unsigned long seg_mask = dma_get_seg_boundary(dev);
+	unsigned int max_len = dma_get_max_seg_size(dev);
+	unsigned int seg_len = 0, seg_dma = 0;
+	int i, count = 1;
+
+	for_each_sg(sg, s, nents, i) {
+		/* Un-swizzling the fields here, hence the naming mismatch */
+		unsigned int s_offset = sg_dma_address(s);
+		unsigned int s_length = sg_dma_len(s);
+		unsigned int s_dma_len = s->length;
+
+		s->offset = s_offset;
+		s->length = s_length;
+		sg_dma_address(s) = DMA_ERROR_CODE;
+		sg_dma_len(s) = 0;
+
+		if (seg_len && (seg_dma + seg_len == dma_addr + s_offset) &&
+		    (seg_len + s_dma_len <= max_len) &&
+		    ((seg_dma & seg_mask) <= seg_mask - (seg_len + s_length))
+		   ) {
+			sg_dma_len(seg) += s_dma_len;
+		} else {
+			if (seg_len) {
+				seg = sg_next(seg);
+				count++;
+			}
+			sg_dma_len(seg) = s_dma_len;
+			sg_dma_address(seg) = dma_addr + s_offset;
+
+			seg_len = s_offset;
+			seg_dma = dma_addr + s_offset;
+		}
+		seg_len += s_length;
+		dma_addr += s_dma_len;
+	}
+	return count;
+}
+
+static void invalidate_sg(struct scatterlist *sg, int nents)
+{
+	struct scatterlist *s;
+	int i;
+
+	for_each_sg(sg, s, nents, i) {
+		if (sg_dma_address(s) != DMA_ERROR_CODE)
+			s->offset = sg_dma_address(s);
+		if (sg_dma_len(s))
+			s->length = sg_dma_len(s);
+		sg_dma_address(s) = DMA_ERROR_CODE;
+		sg_dma_len(s) = 0;
+	}
+}
+
+static int __iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+		int nents, enum dma_data_direction dir, struct dma_attrs *attrs,
+		bool coherent)
+{
+	struct iommu_dma_domain *dom = get_dma_domain(dev);
+	struct iova_domain *iovad = dom->iovad;
+	struct iova *iova;
+	struct scatterlist *s;
+	dma_addr_t dma_addr;
+	size_t iova_len = 0;
+	int i, prot = __dma_direction_to_prot(dir, coherent);
+
+	/*
+	 * Work out how much IOVA space we need, and align the segments to
+	 * IOVA granules for the IOMMU driver to handle. With some clever
+	 * trickery we can modify the list in a reversible manner.
+	 */
+	for_each_sg(sg, s, nents, i) {
+		size_t s_offset = iova_offset(iovad, s->offset);
+		size_t s_length = s->length;
+
+		sg_dma_address(s) = s->offset;
+		sg_dma_len(s) = s_length;
+		s->offset -= s_offset;
+		s_length = iova_align(iovad, s_length + s_offset);
+		s->length = s_length;
+
+		iova_len += s_length;
+	}
+
+	iova = __alloc_iova(dev, iova_len, coherent);
+	if (!iova)
+		goto out_restore_sg;
+
+	/*
+	 * We'll leave any physical concatenation to the IOMMU driver's
+	 * implementation - it knows better than we do.
+	 */
+	dma_addr = iova_dma_addr(iovad, iova);
+	if (iommu_map_sg(dom->domain, dma_addr, sg, nents, prot) < iova_len)
+		goto out_free_iova;
+
+	return finalise_sg(dev, sg, nents, dev_dma_addr(dev, dma_addr));
+
+out_free_iova:
+	__free_iova(iovad, iova);
+out_restore_sg:
+	invalidate_sg(sg, nents);
+	return 0;
+}
+
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+		enum dma_data_direction dir, struct dma_attrs *attrs)
+{
+	return __iommu_dma_map_sg(dev, sg, nents, dir, attrs, false);
+}
+
+int iommu_dma_coherent_map_sg(struct device *dev, struct scatterlist *sg,
+		int nents, enum dma_data_direction dir, struct dma_attrs *attrs)
+{
+	return __iommu_dma_map_sg(dev, sg, nents, dir, attrs, true);
+}
+
+void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
+		enum dma_data_direction dir, struct dma_attrs *attrs)
+{
+	struct iommu_dma_domain *dom = get_dma_domain(dev);
+	struct iova_domain *iovad = dom->iovad;
+	struct scatterlist *s;
+	int i;
+	dma_addr_t iova = sg_dma_address(sg) & ~iova_mask(iovad);
+	size_t len = 0;
+
+	/*
+	 * The scatterlist segments are mapped into contiguous IOVA space,
+	 * so just add up the total length and unmap it in one go.
+	 */
+	for_each_sg(sg, s, nents, i)
+		len += sg_dma_len(s);
+
+	iommu_unmap(dom->domain, iova, len);
+	free_iova(iovad, iova_pfn(iovad, iova));
+}
+
+struct iommu_dma_domain *iommu_dma_create_domain(const struct iommu_ops *ops,
+		dma_addr_t base, size_t size)
+{
+	struct iommu_dma_domain *dom;
+	struct iommu_domain *domain;
+	struct iova_domain *iovad;
+	struct iommu_domain_geometry *dg;
+	unsigned long order, base_pfn, end_pfn;
+
+	pr_debug("base=%pad\tsize=0x%zx\n", &base, size);
+	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
+	if (!dom)
+		return NULL;
+
+	/*
+	 * HACK: We'd like to ask the relevant IOMMU in ops for a suitable
+	 * domain, but until that happens, bypass the bus nonsense and create
+	 * one directly for this specific device/IOMMU combination...
+	 */
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+
+	if (!domain)
+		goto out_free_dma_domain;
+	domain->ops = ops;
+
+	if (ops->domain_init(domain))
+		goto out_free_iommu_domain;
+	/*
+	 * ...and do the bare minimum to sanity-check that the domain allows
+	 * at least some access to the device...
+	 */
+	dg = &domain->geometry;
+	if (!(base <= dg->aperture_end && base + size > dg->aperture_start)) {
+		pr_warn("DMA range outside IOMMU capability; is DT correct?\n");
+		goto out_free_iommu_domain;
+	}
+	/* ...then finally give it a kicking to make sure it fits */
+	dg->aperture_start = max(base, dg->aperture_start);
+	dg->aperture_end = min(base + size - 1, dg->aperture_end);
+	/*
+	 * Note that this almost certainly breaks the case where multiple
+	 * devices with different DMA capabilities need to share a domain,
+	 * but we don't have the necessary information to handle that here
+	 * anyway - "proper" group and domain allocation needs to involve
+	 * the IOMMU driver and a complete view of the bus.
+	 */
+
+	iovad = kzalloc(sizeof(*iovad), GFP_KERNEL);
+	if (!iovad)
+		goto out_free_iommu_domain;
+
+	/* Use the smallest supported page size for IOVA granularity */
+	order = __ffs(ops->pgsize_bitmap);
+	base_pfn = max(dg->aperture_start >> order, (dma_addr_t)1);
+	end_pfn = dg->aperture_end >> order;
+	init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
+
+	dom->domain = domain;
+	dom->iovad = iovad;
+	kref_init(&dom->kref);
+	pr_debug("domain %p created\n", dom);
+	return dom;
+
+out_free_iommu_domain:
+	kfree(domain);
+out_free_dma_domain:
+	kfree(dom);
+	return NULL;
+}
+
+static void iommu_dma_free_domain(struct kref *kref)
+{
+	struct iommu_dma_domain *dom;
+
+	dom = container_of(kref, struct iommu_dma_domain, kref);
+	put_iova_domain(dom->iovad);
+	iommu_domain_free(dom->domain);
+	kfree(dom);
+	pr_debug("domain %p freed\n", dom);
+}
+
+void iommu_dma_release_domain(struct iommu_dma_domain *dom)
+{
+	kref_put(&dom->kref, iommu_dma_free_domain);
+}
+
+struct iommu_domain *iommu_dma_raw_domain(struct iommu_dma_domain *dom)
+{
+	return dom ? dom->domain : NULL;
+}
+
+int iommu_dma_attach_device(struct device *dev, struct iommu_dma_domain *dom)
+{
+	int ret;
+
+	kref_get(&dom->kref);
+	ret = iommu_attach_device(dom->domain, dev);
+	if (ret)
+		iommu_dma_release_domain(dom);
+	else
+		set_dma_domain(dev, dom);
+	pr_debug("%s%s attached to domain %p\n", dev_name(dev),
+			ret?" *not*":"", dom);
+	return ret;
+}
+
+void iommu_dma_detach_device(struct device *dev)
+{
+	struct iommu_dma_domain *dom = get_dma_domain(dev);
+
+	if (!dom) {
+		dev_warn(dev, "Not attached\n");
+		return;
+	}
+	set_dma_domain(dev, NULL);
+	iommu_detach_device(dom->domain, dev);
+	iommu_dma_release_domain(dom);
+	pr_debug("%s detached from domain %p\n", dev_name(dev), dom);
+}
+
+int iommu_dma_supported(struct device *dev, u64 mask)
+{
+	/*
+	 * This looks awful, but really it's reasonable to assume that if an
+	 * IOMMU can't address everything that the CPU can, it probably isn't
+	 * generic enough to be using this implementation in the first place.
+	 */
+	return 1;
+}
+
+int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
+{
+	return dma_addr == DMA_ERROR_CODE;
+}
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
new file mode 100644
index 0000000..4bba85a
--- /dev/null
+++ b/include/linux/dma-iommu.h
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2014 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __DMA_IOMMU_H
+#define __DMA_IOMMU_H
+
+#ifdef __KERNEL__
+
+#include <linux/types.h>
+#include <linux/iommu.h>
+
+#ifdef CONFIG_IOMMU_DMA
+
+int iommu_dma_init(void);
+
+
+struct iommu_dma_domain *iommu_dma_create_domain(const struct iommu_ops *ops,
+		dma_addr_t base, size_t size);
+void iommu_dma_release_domain(struct iommu_dma_domain *dma_domain);
+
+struct iommu_domain *iommu_dma_raw_domain(struct iommu_dma_domain *dma_domain);
+
+int iommu_dma_attach_device(struct device *dev, struct iommu_dma_domain *domain);
+void iommu_dma_detach_device(struct device *dev);
+
+/*
+ * Implementation of these is left to arch code - it can associate domains
+ * with devices however it likes, provided the lookup is efficient.
+ */
+struct iommu_dma_domain *get_dma_domain(struct device *dev);
+void set_dma_domain(struct device *dev, struct iommu_dma_domain *dma_domain);
+
+
+dma_addr_t iommu_dma_create_iova_mapping(struct device *dev,
+		struct page **pages, size_t size, bool coherent);
+int iommu_dma_release_iova_mapping(struct device *dev, dma_addr_t iova,
+		size_t size);
+
+struct page **iommu_dma_alloc_buffer(struct device *dev, size_t size,
+		gfp_t gfp, struct dma_attrs *attrs,
+		void (clear_buffer)(struct page *page, size_t size));
+int iommu_dma_free_buffer(struct device *dev, struct page **pages, size_t size,
+		struct dma_attrs *attrs);
+
+dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
+		unsigned long offset, size_t size, enum dma_data_direction dir,
+		struct dma_attrs *attrs);
+dma_addr_t iommu_dma_coherent_map_page(struct device *dev, struct page *page,
+		unsigned long offset, size_t size, enum dma_data_direction dir,
+		struct dma_attrs *attrs);
+void iommu_dma_unmap_page(struct device *dev, dma_addr_t handle, size_t size,
+		enum dma_data_direction dir, struct dma_attrs *attrs);
+
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+		enum dma_data_direction dir, struct dma_attrs *attrs);
+int iommu_dma_coherent_map_sg(struct device *dev, struct scatterlist *sg,
+		int nents, enum dma_data_direction dir,
+		struct dma_attrs *attrs);
+void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sgl, int nents,
+		enum dma_data_direction dir, struct dma_attrs *attrs);
+
+int iommu_dma_supported(struct device *dev, u64 mask);
+int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
+
+#else
+
+static inline int iommu_dma_init(void)
+{
+	return 0;
+}
+
+static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
+{
+	return NULL;
+}
+
+void set_dma_domain(struct device *dev, struct iommu_dma_domain *dma_domain) { }
+
+#endif  /* CONFIG_IOMMU_DMA */
+
+#endif	/* __KERNEL__ */
+#endif	/* __DMA_IOMMU_H */
-- 
1.9.1

[RFC PATCH v2 1/3] iommu: implement common IOMMU ops for DMA mapping

[Will Deacon]

Hi Robin,

On Fri, Feb 06, 2015 at 02:55:13PM +0000, Robin Murphy wrote:
> Taking inspiration from the existing arch/arm code, break out some
> generic functions to interface the DMA-API to the IOMMU-API. This will
> do the bulk of the heavy lifting for IOMMU-backed dma-mapping.
> 
> Whilst this RFC series is aimed at enabling arm64, once any remaining
> obvious issues in the common code are addressed we can complete the
> refactoring by porting arch/arm over for a merge-worthy series.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/Kconfig     |   7 +
>  drivers/iommu/Makefile    |   1 +
>  drivers/iommu/dma-iommu.c | 552 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-iommu.h |  94 ++++++++
>  4 files changed, 654 insertions(+)
>  create mode 100644 drivers/iommu/dma-iommu.c
>  create mode 100644 include/linux/dma-iommu.h
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index a839ca9..19027bb 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -20,6 +20,13 @@ config OF_IOMMU
>         def_bool y
>         depends on OF && IOMMU_API
> 
> +# IOMMU-agnostic DMA-mapping layer
> +config IOMMU_DMA
> +       def_bool n

This can just be `bool' (n is the default).

> +       depends on NEED_SG_DMA_LENGTH
> +       select IOMMU_API
> +       select IOMMU_IOVA
> +
>  config FSL_PAMU
>         bool "Freescale IOMMU support"
>         depends on PPC_E500MC
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 0b1b94e..37bfc4e 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_IOMMU_API) += iommu.o
>  obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>  obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
> +obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>  obj-$(CONFIG_IOMMU_IOVA) += iova.o
>  obj-$(CONFIG_OF_IOMMU) += of_iommu.o
>  obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> new file mode 100644
> index 0000000..b97cc0b9
> --- /dev/null
> +++ b/drivers/iommu/dma-iommu.c
> @@ -0,0 +1,552 @@
> +/*
> + * A fairly generic DMA-API to IOMMU-API glue layer.
> + *
> + * Copyright (C) 2014 ARM Ltd.
> + *
> + * based in part on arch/arm/mm/dma-mapping.c:
> + * Copyright (C) 2000-2004 Russell King
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#define pr_fmt(fmt)    "%s: " fmt, __func__
> +
> +#include <linux/dma-contiguous.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/iova.h>
> +
> +int iommu_dma_init(void)
> +{
> +       return iommu_iova_cache_init();
> +}
> +
> +struct iommu_dma_domain {
> +       struct iommu_domain *domain;
> +       struct iova_domain *iovad;
> +       struct kref kref;
> +};
> +
> +static inline dma_addr_t dev_dma_addr(struct device *dev, dma_addr_t addr)
> +{
> +       BUG_ON(addr < dev->dma_pfn_offset);

Shouldn't we convert the pfn_offset to an address before this check?

> +       return addr - ((dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
> +}
> +
> +static int __dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
> +{
> +       int prot = coherent ? IOMMU_CACHE : 0;
> +
> +       switch (dir) {
> +       case DMA_BIDIRECTIONAL:
> +               return prot | IOMMU_READ | IOMMU_WRITE;
> +       case DMA_TO_DEVICE:
> +               return prot | IOMMU_READ;
> +       case DMA_FROM_DEVICE:
> +               return prot | IOMMU_WRITE;
> +       default:
> +               return 0;
> +       }
> +}

We could actually stick this into a header file as a generic helper.
arch/arm/ already uses its own flavour, for example.

> +static struct iova *__alloc_iova(struct device *dev, size_t size, bool coherent)
> +{
> +       struct iommu_dma_domain *dom = get_dma_domain(dev);
> +       struct iova_domain *iovad = dom->iovad;
> +       unsigned long shift = iova_shift(iovad);
> +       unsigned long length = iova_align(iovad, size) >> shift;
> +       unsigned long limit_pfn = iovad->dma_32bit_pfn;
> +       u64 dma_limit = coherent ? dev->coherent_dma_mask : *dev->dma_mask;

Should we use dma_get_mask in the non-coherent case in case the mask pointer
is NULL?

> +       limit_pfn = min(limit_pfn, (unsigned long)(dma_limit >> shift));

min_t avoids the explicit cast.

> +       /* Alignment should probably come from a domain/device attribute... */
> +       return alloc_iova(iovad, length, limit_pfn, false);
> +}
> +
> +/*
> + * Create a mapping in device IO address space for specified pages
> + */
> +dma_addr_t iommu_dma_create_iova_mapping(struct device *dev,
> +               struct page **pages, size_t size, bool coherent)
> +{
> +       struct iommu_dma_domain *dom = get_dma_domain(dev);
> +       struct iova_domain *iovad = dom->iovad;
> +       struct iommu_domain *domain = dom->domain;
> +       struct iova *iova;
> +       unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;

Why not just make the size parameter npages instead?

> +       dma_addr_t addr_lo, addr_hi;
> +       int i, prot = __dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
> +
> +       iova = __alloc_iova(dev, size, coherent);
> +       if (!iova)
> +               return DMA_ERROR_CODE;
> +
> +       addr_hi = addr_lo = iova_dma_addr(iovad, iova);
> +       for (i = 0; i < count; ) {
> +               unsigned int next_pfn = page_to_pfn(pages[i]) + 1;
> +               phys_addr_t phys = page_to_phys(pages[i]);
> +               unsigned int len, j;
> +
> +               for (j = i+1; j < count; j++, next_pfn++)
> +                       if (page_to_pfn(pages[j]) != next_pfn)
> +                               break;
> +
> +               len = (j - i) << PAGE_SHIFT;
> +               if (iommu_map(domain, addr_hi, phys, len, prot))
> +                       goto fail;
> +               addr_hi += len;
> +               i = j;
> +       }

This loop is pretty miserable to read. Could we pass this off to ->map_sg
and avoid the need to check that the pfns are contiguous?

> +       return dev_dma_addr(dev, addr_lo);
> +fail:
> +       iommu_unmap(domain, addr_lo, addr_hi - addr_lo);
> +       __free_iova(iovad, iova);
> +       return DMA_ERROR_CODE;
> +}
> +
> +int iommu_dma_release_iova_mapping(struct device *dev, dma_addr_t iova,
> +               size_t size)
> +{
> +       struct iommu_dma_domain *dom = get_dma_domain(dev);
> +       struct iova_domain *iovad = dom->iovad;
> +       size_t offset = iova_offset(iovad, iova);
> +       size_t len = iova_align(iovad, size + offset);
> +
> +       iommu_unmap(dom->domain, iova - offset, len);
> +       free_iova(iovad, iova_pfn(iovad, iova));
> +       return 0;
> +}
> +
> +struct page **iommu_dma_alloc_buffer(struct device *dev, size_t size,
> +               gfp_t gfp, struct dma_attrs *attrs,
> +               void (clear_buffer)(struct page *page, size_t size))
> +{
> +       struct page **pages;
> +       int count = size >> PAGE_SHIFT;
> +       int array_size = count * sizeof(struct page *);
> +       int i = 0;
> +
> +       if (array_size <= PAGE_SIZE)
> +               pages = kzalloc(array_size, GFP_KERNEL);
> +       else
> +               pages = vzalloc(array_size);
> +       if (!pages)
> +               return NULL;
> +
> +       if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> +               unsigned long order = get_order(size);
> +               struct page *page;
> +
> +               page = dma_alloc_from_contiguous(dev, count, order);
> +               if (!page)
> +                       goto error;
> +
> +               if (clear_buffer)
> +                       clear_buffer(page, size);

For cache-coherent systems, this is just a memset so perhaps we could
default to that if no function pointer is passed? It would also be cool
to do the highmem vs lowmem stuff in core code, but that's more fiddly
than it looks and probably not worth the hassle.

> +struct iommu_dma_domain *iommu_dma_create_domain(const struct iommu_ops *ops,
> +               dma_addr_t base, size_t size)
> +{
> +       struct iommu_dma_domain *dom;
> +       struct iommu_domain *domain;
> +       struct iova_domain *iovad;
> +       struct iommu_domain_geometry *dg;
> +       unsigned long order, base_pfn, end_pfn;
> +
> +       pr_debug("base=%pad\tsize=0x%zx\n", &base, size);

We can lose the debug prints for upstream.

> +       dom = kzalloc(sizeof(*dom), GFP_KERNEL);
> +       if (!dom)
> +               return NULL;
> +
> +       /*
> +        * HACK: We'd like to ask the relevant IOMMU in ops for a suitable
> +        * domain, but until that happens, bypass the bus nonsense and create
> +        * one directly for this specific device/IOMMU combination...
> +        */
> +       domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> +
> +       if (!domain)
> +               goto out_free_dma_domain;
> +       domain->ops = ops;
> +
> +       if (ops->domain_init(domain))
> +               goto out_free_iommu_domain;
> +       /*
> +        * ...and do the bare minimum to sanity-check that the domain allows
> +        * at least some access to the device...
> +        */
> +       dg = &domain->geometry;
> +       if (!(base <= dg->aperture_end && base + size > dg->aperture_start)) {
> +               pr_warn("DMA range outside IOMMU capability; is DT correct?\n");

We probably shouldn't print things about DT in core (non-OF) code.

> +               goto out_free_iommu_domain;
> +       }
> +       /* ...then finally give it a kicking to make sure it fits */
> +       dg->aperture_start = max(base, dg->aperture_start);
> +       dg->aperture_end = min(base + size - 1, dg->aperture_end);
> +       /*
> +        * Note that this almost certainly breaks the case where multiple
> +        * devices with different DMA capabilities need to share a domain,
> +        * but we don't have the necessary information to handle that here
> +        * anyway - "proper" group and domain allocation needs to involve
> +        * the IOMMU driver and a complete view of the bus.
> +        */
> +
> +       iovad = kzalloc(sizeof(*iovad), GFP_KERNEL);
> +       if (!iovad)
> +               goto out_free_iommu_domain;
> +
> +       /* Use the smallest supported page size for IOVA granularity */
> +       order = __ffs(ops->pgsize_bitmap);
> +       base_pfn = max(dg->aperture_start >> order, (dma_addr_t)1);
> +       end_pfn = dg->aperture_end >> order;
> +       init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
> +
> +       dom->domain = domain;
> +       dom->iovad = iovad;
> +       kref_init(&dom->kref);
> +       pr_debug("domain %p created\n", dom);
> +       return dom;
> +
> +out_free_iommu_domain:
> +       kfree(domain);
> +out_free_dma_domain:
> +       kfree(dom);
> +       return NULL;
> +}
> +
> +static void iommu_dma_free_domain(struct kref *kref)
> +{
> +       struct iommu_dma_domain *dom;
> +
> +       dom = container_of(kref, struct iommu_dma_domain, kref);
> +       put_iova_domain(dom->iovad);
> +       iommu_domain_free(dom->domain);
> +       kfree(dom);
> +       pr_debug("domain %p freed\n", dom);
> +}
> +
> +void iommu_dma_release_domain(struct iommu_dma_domain *dom)
> +{
> +       kref_put(&dom->kref, iommu_dma_free_domain);
> +}
> +
> +struct iommu_domain *iommu_dma_raw_domain(struct iommu_dma_domain *dom)
> +{
> +       return dom ? dom->domain : NULL;
> +}
> +
> +int iommu_dma_attach_device(struct device *dev, struct iommu_dma_domain *dom)
> +{
> +       int ret;
> +
> +       kref_get(&dom->kref);
> +       ret = iommu_attach_device(dom->domain, dev);
> +       if (ret)
> +               iommu_dma_release_domain(dom);

Shouldn't this be the responsibility of the caller?

> +       else
> +               set_dma_domain(dev, dom);
> +       pr_debug("%s%s attached to domain %p\n", dev_name(dev),
> +                       ret?" *not*":"", dom);
> +       return ret;
> +}
> +
> +void iommu_dma_detach_device(struct device *dev)
> +{
> +       struct iommu_dma_domain *dom = get_dma_domain(dev);
> +
> +       if (!dom) {
> +               dev_warn(dev, "Not attached\n");
> +               return;
> +       }
> +       set_dma_domain(dev, NULL);
> +       iommu_detach_device(dom->domain, dev);

I think this means that we should be checking for NULL DMA domains in all of
the IOVA calls, or can we assume our callers behave themselves?

> +       iommu_dma_release_domain(dom);
> +       pr_debug("%s detached from domain %p\n", dev_name(dev), dom);
> +}
> +
> +int iommu_dma_supported(struct device *dev, u64 mask)
> +{
> +       /*
> +        * This looks awful, but really it's reasonable to assume that if an
> +        * IOMMU can't address everything that the CPU can, it probably isn't
> +        * generic enough to be using this implementation in the first place.
> +        */
> +       return 1;

Hmm, perhaps, but I don't think it's *completely* unrealistic to have an
IOMMU with more limited physical addressing caps than the CPU. Maybe we
could have an IOMMU callback for querying the addressing capabilities?

It shouldn't block this series, I'm just interested in your thoughts on
solving this in the future.

Will

[RFC PATCH v2 1/3] iommu: implement common IOMMU ops for DMA mapping

[Robin Murphy]

Hi Will,

Thanks for the comments,

On 09/02/15 04:05, Will Deacon wrote:
> Hi Robin,
>
> On Fri, Feb 06, 2015 at 02:55:13PM +0000, Robin Murphy wrote:
>> Taking inspiration from the existing arch/arm code, break out some
>> generic functions to interface the DMA-API to the IOMMU-API. This will
>> do the bulk of the heavy lifting for IOMMU-backed dma-mapping.
>>
>> Whilst this RFC series is aimed at enabling arm64, once any remaining
>> obvious issues in the common code are addressed we can complete the
>> refactoring by porting arch/arm over for a merge-worthy series.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/Kconfig     |   7 +
>>   drivers/iommu/Makefile    |   1 +
>>   drivers/iommu/dma-iommu.c | 552 ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/dma-iommu.h |  94 ++++++++
>>   4 files changed, 654 insertions(+)
>>   create mode 100644 drivers/iommu/dma-iommu.c
>>   create mode 100644 include/linux/dma-iommu.h
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index a839ca9..19027bb 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -20,6 +20,13 @@ config OF_IOMMU
>>          def_bool y
>>          depends on OF && IOMMU_API
>>
>> +# IOMMU-agnostic DMA-mapping layer
>> +config IOMMU_DMA
>> +       def_bool n
>
> This can just be `bool' (n is the default).
>

Good to know, fixed.

>> +       depends on NEED_SG_DMA_LENGTH
>> +       select IOMMU_API
>> +       select IOMMU_IOVA
>> +
>>   config FSL_PAMU
>>          bool "Freescale IOMMU support"
>>          depends on PPC_E500MC
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 0b1b94e..37bfc4e 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -1,6 +1,7 @@
>>   obj-$(CONFIG_IOMMU_API) += iommu.o
>>   obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>>   obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
>> +obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>>   obj-$(CONFIG_IOMMU_IOVA) += iova.o
>>   obj-$(CONFIG_OF_IOMMU) += of_iommu.o
>>   obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> new file mode 100644
>> index 0000000..b97cc0b9
>> --- /dev/null
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -0,0 +1,552 @@
>> +/*
>> + * A fairly generic DMA-API to IOMMU-API glue layer.
>> + *
>> + * Copyright (C) 2014 ARM Ltd.
>> + *
>> + * based in part on arch/arm/mm/dma-mapping.c:
>> + * Copyright (C) 2000-2004 Russell King
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#define pr_fmt(fmt)    "%s: " fmt, __func__
>> +
>> +#include <linux/dma-contiguous.h>
>> +#include <linux/dma-iommu.h>
>> +#include <linux/iova.h>
>> +
>> +int iommu_dma_init(void)
>> +{
>> +       return iommu_iova_cache_init();
>> +}
>> +
>> +struct iommu_dma_domain {
>> +       struct iommu_domain *domain;
>> +       struct iova_domain *iovad;
>> +       struct kref kref;
>> +};
>> +
>> +static inline dma_addr_t dev_dma_addr(struct device *dev, dma_addr_t addr)
>> +{
>> +       BUG_ON(addr < dev->dma_pfn_offset);
>
> Shouldn't we convert the pfn_offset to an address before this check?
>

Oops, yes indeed we should...

>> +       return addr - ((dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
>> +}
>> +
>> +static int __dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
>> +{
>> +       int prot = coherent ? IOMMU_CACHE : 0;
>> +
>> +       switch (dir) {
>> +       case DMA_BIDIRECTIONAL:
>> +               return prot | IOMMU_READ | IOMMU_WRITE;
>> +       case DMA_TO_DEVICE:
>> +               return prot | IOMMU_READ;
>> +       case DMA_FROM_DEVICE:
>> +               return prot | IOMMU_WRITE;
>> +       default:
>> +               return 0;
>> +       }
>> +}
>
> We could actually stick this into a header file as a generic helper.
> arch/arm/ already uses its own flavour, for example.

Well, it's already been said that the arch/arm implementation should go 
away as part of merging this ;) Having had a good look around there 
don't appear to be any other potential users to share it with.

>> +static struct iova *__alloc_iova(struct device *dev, size_t size, bool coherent)
>> +{
>> +       struct iommu_dma_domain *dom = get_dma_domain(dev);
>> +       struct iova_domain *iovad = dom->iovad;
>> +       unsigned long shift = iova_shift(iovad);
>> +       unsigned long length = iova_align(iovad, size) >> shift;
>> +       unsigned long limit_pfn = iovad->dma_32bit_pfn;
>> +       u64 dma_limit = coherent ? dev->coherent_dma_mask : *dev->dma_mask;
>
> Should we use dma_get_mask in the non-coherent case in case the mask pointer
> is NULL?

Good point, fixed.

>> +       limit_pfn = min(limit_pfn, (unsigned long)(dma_limit >> shift));
>
> min_t avoids the explicit cast.

Agreed, it does look a little nicer that way (although this is more of a 
nasty workaround for the mask not reflecting dma-ranges limits - 
hopefully that'll get sorted soon[1] so this can go back to just using 
the mask)

[1]:http://www.spinics.net/lists/arm-kernel/msg397604.html

>
>> +       /* Alignment should probably come from a domain/device attribute... */
>> +       return alloc_iova(iovad, length, limit_pfn, false);
>> +}
>> +
>> +/*
>> + * Create a mapping in device IO address space for specified pages
>> + */
>> +dma_addr_t iommu_dma_create_iova_mapping(struct device *dev,
>> +               struct page **pages, size_t size, bool coherent)
>> +{
>> +       struct iommu_dma_domain *dom = get_dma_domain(dev);
>> +       struct iova_domain *iovad = dom->iovad;
>> +       struct iommu_domain *domain = dom->domain;
>> +       struct iova *iova;
>> +       unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>
> Why not just make the size parameter npages instead?
>
>> +       dma_addr_t addr_lo, addr_hi;
>> +       int i, prot = __dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
>> +
>> +       iova = __alloc_iova(dev, size, coherent);
>> +       if (!iova)
>> +               return DMA_ERROR_CODE;
>> +
>> +       addr_hi = addr_lo = iova_dma_addr(iovad, iova);
>> +       for (i = 0; i < count; ) {
>> +               unsigned int next_pfn = page_to_pfn(pages[i]) + 1;
>> +               phys_addr_t phys = page_to_phys(pages[i]);
>> +               unsigned int len, j;
>> +
>> +               for (j = i+1; j < count; j++, next_pfn++)
>> +                       if (page_to_pfn(pages[j]) != next_pfn)
>> +                               break;
>> +
>> +               len = (j - i) << PAGE_SHIFT;
>> +               if (iommu_map(domain, addr_hi, phys, len, prot))
>> +                       goto fail;
>> +               addr_hi += len;
>> +               i = j;
>> +       }
>
> This loop is pretty miserable to read. Could we pass this off to ->map_sg
> and avoid the need to check that the pfns are contiguous?

All part of the copy-paste magic ;) Y'know, other than the bit of extra 
work to set up the sg_table (and the possible hard size limit to that on 
some architectures), using scatterlists as the general primitive makes a 
lot of sense. With a full iommu_dma_alloc_attrs() implementation using 
sg_alloc_table_from_pages(), this function can end up mostly going away, 
and we get benefits...

>
>> +       return dev_dma_addr(dev, addr_lo);
>> +fail:
>> +       iommu_unmap(domain, addr_lo, addr_hi - addr_lo);
>> +       __free_iova(iovad, iova);
>> +       return DMA_ERROR_CODE;
>> +}
>> +
>> +int iommu_dma_release_iova_mapping(struct device *dev, dma_addr_t iova,
>> +               size_t size)
>> +{
>> +       struct iommu_dma_domain *dom = get_dma_domain(dev);
>> +       struct iova_domain *iovad = dom->iovad;
>> +       size_t offset = iova_offset(iovad, iova);
>> +       size_t len = iova_align(iovad, size + offset);
>> +
>> +       iommu_unmap(dom->domain, iova - offset, len);
>> +       free_iova(iovad, iova_pfn(iovad, iova));
>> +       return 0;
>> +}
>> +
>> +struct page **iommu_dma_alloc_buffer(struct device *dev, size_t size,
>> +               gfp_t gfp, struct dma_attrs *attrs,
>> +               void (clear_buffer)(struct page *page, size_t size))
>> +{
>> +       struct page **pages;
>> +       int count = size >> PAGE_SHIFT;
>> +       int array_size = count * sizeof(struct page *);
>> +       int i = 0;
>> +
>> +       if (array_size <= PAGE_SIZE)
>> +               pages = kzalloc(array_size, GFP_KERNEL);
>> +       else
>> +               pages = vzalloc(array_size);
>> +       if (!pages)
>> +               return NULL;
>> +
>> +       if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
>> +               unsigned long order = get_order(size);
>> +               struct page *page;
>> +
>> +               page = dma_alloc_from_contiguous(dev, count, order);
>> +               if (!page)
>> +                       goto error;
>> +
>> +               if (clear_buffer)
>> +                       clear_buffer(page, size);
>
> For cache-coherent systems, this is just a memset so perhaps we could
> default to that if no function pointer is passed? It would also be cool
> to do the highmem vs lowmem stuff in core code, but that's more fiddly
> than it looks and probably not worth the hassle.

...like being able to use sg_miter_* to magically memset everything here 
and have the architecture provide just the cache flush callback (if it 
wants to). I'll definitely investigate this as it might help solve some 
other issues too.

>> +struct iommu_dma_domain *iommu_dma_create_domain(const struct iommu_ops *ops,
>> +               dma_addr_t base, size_t size)
>> +{
>> +       struct iommu_dma_domain *dom;
>> +       struct iommu_domain *domain;
>> +       struct iova_domain *iovad;
>> +       struct iommu_domain_geometry *dg;
>> +       unsigned long order, base_pfn, end_pfn;
>> +
>> +       pr_debug("base=%pad\tsize=0x%zx\n", &base, size);
>
> We can lose the debug prints for upstream.
>
>> +       dom = kzalloc(sizeof(*dom), GFP_KERNEL);
>> +       if (!dom)
>> +               return NULL;
>> +
>> +       /*
>> +        * HACK: We'd like to ask the relevant IOMMU in ops for a suitable
>> +        * domain, but until that happens, bypass the bus nonsense and create
>> +        * one directly for this specific device/IOMMU combination...
>> +        */
>> +       domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>> +
>> +       if (!domain)
>> +               goto out_free_dma_domain;
>> +       domain->ops = ops;
>> +
>> +       if (ops->domain_init(domain))
>> +               goto out_free_iommu_domain;
>> +       /*
>> +        * ...and do the bare minimum to sanity-check that the domain allows
>> +        * at least some access to the device...
>> +        */
>> +       dg = &domain->geometry;
>> +       if (!(base <= dg->aperture_end && base + size > dg->aperture_start)) {
>> +               pr_warn("DMA range outside IOMMU capability; is DT correct?\n");
>
> We probably shouldn't print things about DT in core (non-OF) code.

True. I'll change it for now, although I hope the default domain stuff 
will make most of this entirely redundant soon.

>> +               goto out_free_iommu_domain;
>> +       }
>> +       /* ...then finally give it a kicking to make sure it fits */
>> +       dg->aperture_start = max(base, dg->aperture_start);
>> +       dg->aperture_end = min(base + size - 1, dg->aperture_end);
>> +       /*
>> +        * Note that this almost certainly breaks the case where multiple
>> +        * devices with different DMA capabilities need to share a domain,
>> +        * but we don't have the necessary information to handle that here
>> +        * anyway - "proper" group and domain allocation needs to involve
>> +        * the IOMMU driver and a complete view of the bus.
>> +        */
>> +
>> +       iovad = kzalloc(sizeof(*iovad), GFP_KERNEL);
>> +       if (!iovad)
>> +               goto out_free_iommu_domain;
>> +
>> +       /* Use the smallest supported page size for IOVA granularity */
>> +       order = __ffs(ops->pgsize_bitmap);
>> +       base_pfn = max(dg->aperture_start >> order, (dma_addr_t)1);
>> +       end_pfn = dg->aperture_end >> order;
>> +       init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
>> +
>> +       dom->domain = domain;
>> +       dom->iovad = iovad;
>> +       kref_init(&dom->kref);
>> +       pr_debug("domain %p created\n", dom);
>> +       return dom;
>> +
>> +out_free_iommu_domain:
>> +       kfree(domain);
>> +out_free_dma_domain:
>> +       kfree(dom);
>> +       return NULL;
>> +}
>> +
>> +static void iommu_dma_free_domain(struct kref *kref)
>> +{
>> +       struct iommu_dma_domain *dom;
>> +
>> +       dom = container_of(kref, struct iommu_dma_domain, kref);
>> +       put_iova_domain(dom->iovad);
>> +       iommu_domain_free(dom->domain);
>> +       kfree(dom);
>> +       pr_debug("domain %p freed\n", dom);
>> +}
>> +
>> +void iommu_dma_release_domain(struct iommu_dma_domain *dom)
>> +{
>> +       kref_put(&dom->kref, iommu_dma_free_domain);
>> +}
>> +
>> +struct iommu_domain *iommu_dma_raw_domain(struct iommu_dma_domain *dom)
>> +{
>> +       return dom ? dom->domain : NULL;
>> +}
>> +
>> +int iommu_dma_attach_device(struct device *dev, struct iommu_dma_domain *dom)
>> +{
>> +       int ret;
>> +
>> +       kref_get(&dom->kref);
>> +       ret = iommu_attach_device(dom->domain, dev);
>> +       if (ret)
>> +               iommu_dma_release_domain(dom);
>
> Shouldn't this be the responsibility of the caller?

It's the responsibility of the caller to release its reference by making 
a matching detach call on a successfully attached device. The original 
code only *takes* the reference on success - I just shuffled things 
around to avoid the race with someone else detaching the last device and 
taking down the domain before this iommu_attach_device() returns.

>> +       else
>> +               set_dma_domain(dev, dom);
>> +       pr_debug("%s%s attached to domain %p\n", dev_name(dev),
>> +                       ret?" *not*":"", dom);
>> +       return ret;
>> +}
>> +
>> +void iommu_dma_detach_device(struct device *dev)
>> +{
>> +       struct iommu_dma_domain *dom = get_dma_domain(dev);
>> +
>> +       if (!dom) {
>> +               dev_warn(dev, "Not attached\n");
>> +               return;
>> +       }
>> +       set_dma_domain(dev, NULL);
>> +       iommu_detach_device(dom->domain, dev);
>
> I think this means that we should be checking for NULL DMA domains in all of
> the IOVA calls, or can we assume our callers behave themselves?

This check is a hangover from the original code, which similarly never 
checks the validity of archdata.mapping elsewhere. I'm inclined to 
simply remove it and let broken callers blow up violently in 
iommu_detach_device() because I really can't think of a justifiable 
reason to let them off with a warning. IMO this is intended more for 
arch code than for any old driver to use willy-nilly, so they really 
should be getting it right.

>
>> +       iommu_dma_release_domain(dom);
>> +       pr_debug("%s detached from domain %p\n", dev_name(dev), dom);
>> +}
>> +
>> +int iommu_dma_supported(struct device *dev, u64 mask)
>> +{
>> +       /*
>> +        * This looks awful, but really it's reasonable to assume that if an
>> +        * IOMMU can't address everything that the CPU can, it probably isn't
>> +        * generic enough to be using this implementation in the first place.
>> +        */
>> +       return 1;
>
> Hmm, perhaps, but I don't think it's *completely* unrealistic to have an
> IOMMU with more limited physical addressing caps than the CPU. Maybe we
> could have an IOMMU callback for querying the addressing capabilities?
>
> It shouldn't block this series, I'm just interested in your thoughts on
> solving this in the future.

I see potentially two places the relevant data could already exist:

* the DMA mask of the IOMMU device itself - accessible, but possibly not 
reliable if the IOMMU driver doesn't bother to set it correctly (e.g. 
ours). Also, really this should only represent the IOMMU's own DMA (i.e. 
PTW interface) capability which, sanity notwithstanding, could 
conceivably differ from that of the master interface.

* the output address size of the io_pgtable_config - not widely used 
yet, and not really something to be poking around in outside of the 
IOMMU driver itself.

Given that it's a fixed property of the hardware it seems like something 
we could reasonably express with e.g. a mask in iommu_ops, but strictly 
we'd still need to go digging around for the IOMMU device's 
dma_pfn_offset as well in case it's behind some wacky interconnect. 
Unless of course the PTW and master interfaces are hooked up to 
*different* wacky interconnects, in which case, urgh...

Robin.

[RFC PATCH v2 1/3] iommu: implement common IOMMU ops for DMA mapping

[Marek Szyprowski]

Hello Robin,

I'm sorry for a delay, but I've just finished testing this patchser on 
Exynos5433
with Exynos DRM and other drivers.

On 2015-02-06 15:55, Robin Murphy wrote:
> Taking inspiration from the existing arch/arm code, break out some
> generic functions to interface the DMA-API to the IOMMU-API. This will
> do the bulk of the heavy lifting for IOMMU-backed dma-mapping.
>
> Whilst this RFC series is aimed at enabling arm64, once any remaining
> obvious issues in the common code are addressed we can complete the
> refactoring by porting arch/arm over for a merge-worthy series.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   drivers/iommu/Kconfig     |   7 +
>   drivers/iommu/Makefile    |   1 +
>   drivers/iommu/dma-iommu.c | 552 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/dma-iommu.h |  94 ++++++++
>   4 files changed, 654 insertions(+)
>   create mode 100644 drivers/iommu/dma-iommu.c
>   create mode 100644 include/linux/dma-iommu.h
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index a839ca9..19027bb 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -20,6 +20,13 @@ config OF_IOMMU
>          def_bool y
>          depends on OF && IOMMU_API
>   
> +# IOMMU-agnostic DMA-mapping layer
> +config IOMMU_DMA
> +	def_bool n
> +	depends on NEED_SG_DMA_LENGTH
> +	select IOMMU_API
> +	select IOMMU_IOVA
> +
>   config FSL_PAMU
>   	bool "Freescale IOMMU support"
>   	depends on PPC_E500MC
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 0b1b94e..37bfc4e 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -1,6 +1,7 @@
>   obj-$(CONFIG_IOMMU_API) += iommu.o
>   obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>   obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
> +obj-$(CONFIG_IOMMU_DMA) += dma-iommu.o
>   obj-$(CONFIG_IOMMU_IOVA) += iova.o
>   obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o
>   obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> new file mode 100644
> index 0000000..b97cc0b9
> --- /dev/null
> +++ b/drivers/iommu/dma-iommu.c
> @@ -0,0 +1,552 @@
> +/*
> + * A fairly generic DMA-API to IOMMU-API glue layer.
> + *
> + * Copyright (C) 2014 ARM Ltd.
> + *
> + * based in part on arch/arm/mm/dma-mapping.c:
> + * Copyright (C) 2000-2004 Russell King
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#define pr_fmt(fmt)	"%s: " fmt, __func__
> +
> +#include <linux/dma-contiguous.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/iova.h>
> +
> +int iommu_dma_init(void)
> +{
> +	return iommu_iova_cache_init();
> +}
> +
> +struct iommu_dma_domain {
> +	struct iommu_domain *domain;
> +	struct iova_domain *iovad;
> +	struct kref kref;
> +};
> +
> +static inline dma_addr_t dev_dma_addr(struct device *dev, dma_addr_t addr)
> +{
> +	BUG_ON(addr < dev->dma_pfn_offset);
> +	return addr - ((dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
> +}
> +
> +static int __dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
> +{
> +	int prot = coherent ? IOMMU_CACHE : 0;
> +
> +	switch (dir) {
> +	case DMA_BIDIRECTIONAL:
> +		return prot | IOMMU_READ | IOMMU_WRITE;
> +	case DMA_TO_DEVICE:
> +		return prot | IOMMU_READ;
> +	case DMA_FROM_DEVICE:
> +		return prot | IOMMU_WRITE;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static struct iova *__alloc_iova(struct device *dev, size_t size, bool coherent)
> +{
> +	struct iommu_dma_domain *dom = get_dma_domain(dev);
> +	struct iova_domain *iovad = dom->iovad;
> +	unsigned long shift = iova_shift(iovad);
> +	unsigned long length = iova_align(iovad, size) >> shift;
> +	unsigned long limit_pfn = iovad->dma_32bit_pfn;
> +	u64 dma_limit = coherent ? dev->coherent_dma_mask : *dev->dma_mask;
> +
> +	limit_pfn = min(limit_pfn, (unsigned long)(dma_limit >> shift));
> +	/* Alignment should probably come from a domain/device attribute... */
> +	return alloc_iova(iovad, length, limit_pfn, false);

DMA-mapping subsystem must provide IOVAs, which are at least size 
aligned, some drivers
depends on this feature (mainly because they are not able to specify 
alignment in other
way...). I've discussed this issue long time ago when CMA has been 
developed and the
conclusion was that size alignment is guaranteed by dma-mapping and 
drivers can rely on
it.

> +}
> +
> +/*
> + * Create a mapping in device IO address space for specified pages
> + */
> +dma_addr_t iommu_dma_create_iova_mapping(struct device *dev,
> +		struct page **pages, size_t size, bool coherent)
> +{
> +	struct iommu_dma_domain *dom = get_dma_domain(dev);
> +	struct iova_domain *iovad = dom->iovad;
> +	struct iommu_domain *domain = dom->domain;
> +	struct iova *iova;
> +	unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +	dma_addr_t addr_lo, addr_hi;
> +	int i, prot = __dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
> +
> +	iova = __alloc_iova(dev, size, coherent);
> +	if (!iova)
> +		return DMA_ERROR_CODE;
> +
> +	addr_hi = addr_lo = iova_dma_addr(iovad, iova);
> +	for (i = 0; i < count; ) {
> +		unsigned int next_pfn = page_to_pfn(pages[i]) + 1;
> +		phys_addr_t phys = page_to_phys(pages[i]);
> +		unsigned int len, j;
> +
> +		for (j = i+1; j < count; j++, next_pfn++)
> +			if (page_to_pfn(pages[j]) != next_pfn)
> +				break;
> +
> +		len = (j - i) << PAGE_SHIFT;
> +		if (iommu_map(domain, addr_hi, phys, len, prot))
> +			goto fail;
> +		addr_hi += len;
> +		i = j;
> +	}
> +	return dev_dma_addr(dev, addr_lo);
> +fail:
> +	iommu_unmap(domain, addr_lo, addr_hi - addr_lo);
> +	__free_iova(iovad, iova);
> +	return DMA_ERROR_CODE;
> +}
> +
> +int iommu_dma_release_iova_mapping(struct device *dev, dma_addr_t iova,
> +		size_t size)
> +{
> +	struct iommu_dma_domain *dom = get_dma_domain(dev);
> +	struct iova_domain *iovad = dom->iovad;
> +	size_t offset = iova_offset(iovad, iova);
> +	size_t len = iova_align(iovad, size + offset);
> +
> +	iommu_unmap(dom->domain, iova - offset, len);
> +	free_iova(iovad, iova_pfn(iovad, iova));
> +	return 0;
> +}
> +
> +struct page **iommu_dma_alloc_buffer(struct device *dev, size_t size,
> +		gfp_t gfp, struct dma_attrs *attrs,
> +		void (clear_buffer)(struct page *page, size_t size))
> +{
> +	struct page **pages;
> +	int count = size >> PAGE_SHIFT;
> +	int array_size = count * sizeof(struct page *);
> +	int i = 0;
> +
> +	if (array_size <= PAGE_SIZE)
> +		pages = kzalloc(array_size, GFP_KERNEL);
> +	else
> +		pages = vzalloc(array_size);
> +	if (!pages)
> +		return NULL;
> +
> +	if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> +		unsigned long order = get_order(size);
> +		struct page *page;
> +
> +		page = dma_alloc_from_contiguous(dev, count, order);
> +		if (!page)
> +			goto error;
> +
> +		if (clear_buffer)
> +			clear_buffer(page, size);
> +
> +		for (i = 0; i < count; i++)
> +			pages[i] = page + i;
> +
> +		return pages;
> +	}
> +
> +	/*
> +	 * IOMMU can map any pages, so himem can also be used here
> +	 */
> +	gfp |= __GFP_NOWARN | __GFP_HIGHMEM;
> +
> +	while (count) {
> +		int j, order = __fls(count);
> +
> +		pages[i] = alloc_pages(gfp, order);
> +		while (!pages[i] && order)
> +			pages[i] = alloc_pages(gfp, --order);
> +		if (!pages[i])
> +			goto error;
> +
> +		if (order) {
> +			split_page(pages[i], order);
> +			j = 1 << order;
> +			while (--j)
> +				pages[i + j] = pages[i] + j;
> +		}
> +
> +		if (clear_buffer)
> +			clear_buffer(pages[i], PAGE_SIZE << order);
> +		i += 1 << order;
> +		count -= 1 << order;
> +	}
> +
> +	return pages;
> +error:
> +	while (i--)
> +		if (pages[i])
> +			__free_pages(pages[i], 0);
> +	if (array_size <= PAGE_SIZE)
> +		kfree(pages);
> +	else
> +		vfree(pages);
> +	return NULL;
> +}
> +
> +int iommu_dma_free_buffer(struct device *dev, struct page **pages, size_t size,
> +		struct dma_attrs *attrs)
> +{
> +	int count = size >> PAGE_SHIFT;
> +	int array_size = count * sizeof(struct page *);
> +	int i;
> +
> +	if (dma_get_attr(DMA_ATTR_FORCE_CONTIGUOUS, attrs)) {
> +		dma_release_from_contiguous(dev, pages[0], count);
> +	} else {
> +		for (i = 0; i < count; i++)
> +			if (pages[i])
> +				__free_pages(pages[i], 0);
> +	}
> +
> +	if (array_size <= PAGE_SIZE)
> +		kfree(pages);
> +	else
> +		vfree(pages);
> +	return 0;
> +}
> +
> +static dma_addr_t __iommu_dma_map_page(struct device *dev, struct page *page,
> +		unsigned long offset, size_t size, enum dma_data_direction dir,
> +		bool coherent)
> +{
> +	dma_addr_t dma_addr;
> +	struct iommu_dma_domain *dom = get_dma_domain(dev);
> +	struct iova_domain *iovad = dom->iovad;
> +	phys_addr_t phys = page_to_phys(page) + offset;
> +	size_t iova_off = iova_offset(iovad, phys);
> +	size_t len = iova_align(iovad, size + iova_off);
> +	int prot = __dma_direction_to_prot(dir, coherent);
> +	struct iova *iova = __alloc_iova(dev, len, coherent);
> +
> +	if (!iova)
> +		return DMA_ERROR_CODE;
> +
> +	dma_addr = iova_dma_addr(iovad, iova);
> +	if (iommu_map(dom->domain, dma_addr, phys - iova_off, len, prot)) {
> +		__free_iova(iovad, iova);
> +		return DMA_ERROR_CODE;
> +	}
> +
> +	return dev_dma_addr(dev, dma_addr + iova_off);
> +}
> +
> +dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> +		unsigned long offset, size_t size, enum dma_data_direction dir,
> +		struct dma_attrs *attrs)
> +{
> +	return __iommu_dma_map_page(dev, page, offset, size, dir, false);
> +}
> +
> +dma_addr_t iommu_dma_coherent_map_page(struct device *dev, struct page *page,
> +		unsigned long offset, size_t size, enum dma_data_direction dir,
> +		struct dma_attrs *attrs)
> +{
> +	return __iommu_dma_map_page(dev, page, offset, size, dir, true);
> +}
> +
> +void iommu_dma_unmap_page(struct device *dev, dma_addr_t handle, size_t size,
> +		enum dma_data_direction dir, struct dma_attrs *attrs)
> +{
> +	struct iommu_dma_domain *dom = get_dma_domain(dev);
> +	struct iova_domain *iovad = dom->iovad;
> +	size_t offset = iova_offset(iovad, handle);
> +	size_t len = iova_align(iovad, size + offset);
> +	dma_addr_t iova = handle - offset;
> +
> +	if (!iova)
> +		return;
> +
> +	iommu_unmap(dom->domain, iova, len);
> +	free_iova(iovad, iova_pfn(iovad, iova));
> +}
> +
> +static int finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
> +		dma_addr_t dma_addr)
> +{
> +	struct scatterlist *s, *seg = sg;
> +	unsigned long seg_mask = dma_get_seg_boundary(dev);
> +	unsigned int max_len = dma_get_max_seg_size(dev);
> +	unsigned int seg_len = 0, seg_dma = 0;
> +	int i, count = 1;
> +
> +	for_each_sg(sg, s, nents, i) {
> +		/* Un-swizzling the fields here, hence the naming mismatch */
> +		unsigned int s_offset = sg_dma_address(s);
> +		unsigned int s_length = sg_dma_len(s);
> +		unsigned int s_dma_len = s->length;
> +
> +		s->offset = s_offset;
> +		s->length = s_length;
> +		sg_dma_address(s) = DMA_ERROR_CODE;
> +		sg_dma_len(s) = 0;
> +
> +		if (seg_len && (seg_dma + seg_len == dma_addr + s_offset) &&
> +		    (seg_len + s_dma_len <= max_len) &&
> +		    ((seg_dma & seg_mask) <= seg_mask - (seg_len + s_length))
> +		   ) {
> +			sg_dma_len(seg) += s_dma_len;
> +		} else {
> +			if (seg_len) {
> +				seg = sg_next(seg);
> +				count++;
> +			}
> +			sg_dma_len(seg) = s_dma_len;
> +			sg_dma_address(seg) = dma_addr + s_offset;
> +
> +			seg_len = s_offset;
> +			seg_dma = dma_addr + s_offset;
> +		}
> +		seg_len += s_length;
> +		dma_addr += s_dma_len;
> +	}
> +	return count;
> +}
> +
> +static void invalidate_sg(struct scatterlist *sg, int nents)
> +{
> +	struct scatterlist *s;
> +	int i;
> +
> +	for_each_sg(sg, s, nents, i) {
> +		if (sg_dma_address(s) != DMA_ERROR_CODE)
> +			s->offset = sg_dma_address(s);
> +		if (sg_dma_len(s))
> +			s->length = sg_dma_len(s);
> +		sg_dma_address(s) = DMA_ERROR_CODE;
> +		sg_dma_len(s) = 0;
> +	}
> +}
> +
> +static int __iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
> +		int nents, enum dma_data_direction dir, struct dma_attrs *attrs,
> +		bool coherent)
> +{
> +	struct iommu_dma_domain *dom = get_dma_domain(dev);
> +	struct iova_domain *iovad = dom->iovad;
> +	struct iova *iova;
> +	struct scatterlist *s;
> +	dma_addr_t dma_addr;
> +	size_t iova_len = 0;
> +	int i, prot = __dma_direction_to_prot(dir, coherent);
> +
> +	/*
> +	 * Work out how much IOVA space we need, and align the segments to
> +	 * IOVA granules for the IOMMU driver to handle. With some clever
> +	 * trickery we can modify the list in a reversible manner.
> +	 */
> +	for_each_sg(sg, s, nents, i) {
> +		size_t s_offset = iova_offset(iovad, s->offset);
> +		size_t s_length = s->length;
> +
> +		sg_dma_address(s) = s->offset;
> +		sg_dma_len(s) = s_length;
> +		s->offset -= s_offset;
> +		s_length = iova_align(iovad, s_length + s_offset);
> +		s->length = s_length;
> +
> +		iova_len += s_length;
> +	}
> +
> +	iova = __alloc_iova(dev, iova_len, coherent);
> +	if (!iova)
> +		goto out_restore_sg;
> +
> +	/*
> +	 * We'll leave any physical concatenation to the IOMMU driver's
> +	 * implementation - it knows better than we do.
> +	 */
> +	dma_addr = iova_dma_addr(iovad, iova);
> +	if (iommu_map_sg(dom->domain, dma_addr, sg, nents, prot) < iova_len)
> +		goto out_free_iova;
> +
> +	return finalise_sg(dev, sg, nents, dev_dma_addr(dev, dma_addr));
> +
> +out_free_iova:
> +	__free_iova(iovad, iova);
> +out_restore_sg:
> +	invalidate_sg(sg, nents);
> +	return 0;
> +}
> +
> +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> +		enum dma_data_direction dir, struct dma_attrs *attrs)
> +{
> +	return __iommu_dma_map_sg(dev, sg, nents, dir, attrs, false);
> +}
> +
> +int iommu_dma_coherent_map_sg(struct device *dev, struct scatterlist *sg,
> +		int nents, enum dma_data_direction dir, struct dma_attrs *attrs)
> +{
> +	return __iommu_dma_map_sg(dev, sg, nents, dir, attrs, true);
> +}
> +
> +void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> +		enum dma_data_direction dir, struct dma_attrs *attrs)
> +{
> +	struct iommu_dma_domain *dom = get_dma_domain(dev);
> +	struct iova_domain *iovad = dom->iovad;
> +	struct scatterlist *s;
> +	int i;
> +	dma_addr_t iova = sg_dma_address(sg) & ~iova_mask(iovad);
> +	size_t len = 0;
> +
> +	/*
> +	 * The scatterlist segments are mapped into contiguous IOVA space,
> +	 * so just add up the total length and unmap it in one go.
> +	 */
> +	for_each_sg(sg, s, nents, i)
> +		len += sg_dma_len(s);
> +
> +	iommu_unmap(dom->domain, iova, len);
> +	free_iova(iovad, iova_pfn(iovad, iova));
> +}
> +
> +struct iommu_dma_domain *iommu_dma_create_domain(const struct iommu_ops *ops,
> +		dma_addr_t base, size_t size)
> +{
> +	struct iommu_dma_domain *dom;
> +	struct iommu_domain *domain;
> +	struct iova_domain *iovad;
> +	struct iommu_domain_geometry *dg;
> +	unsigned long order, base_pfn, end_pfn;
> +
> +	pr_debug("base=%pad\tsize=0x%zx\n", &base, size);
> +	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
> +	if (!dom)
> +		return NULL;
> +
> +	/*
> +	 * HACK: We'd like to ask the relevant IOMMU in ops for a suitable
> +	 * domain, but until that happens, bypass the bus nonsense and create
> +	 * one directly for this specific device/IOMMU combination...
> +	 */
> +	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> +
> +	if (!domain)
> +		goto out_free_dma_domain;
> +	domain->ops = ops;
> +
> +	if (ops->domain_init(domain))
> +		goto out_free_iommu_domain;
> +	/*
> +	 * ...and do the bare minimum to sanity-check that the domain allows
> +	 * at least some access to the device...
> +	 */
> +	dg = &domain->geometry;
> +	if (!(base <= dg->aperture_end && base + size > dg->aperture_start)) {
> +		pr_warn("DMA range outside IOMMU capability; is DT correct?\n");
> +		goto out_free_iommu_domain;
> +	}
> +	/* ...then finally give it a kicking to make sure it fits */
> +	dg->aperture_start = max(base, dg->aperture_start);
> +	dg->aperture_end = min(base + size - 1, dg->aperture_end);
> +	/*
> +	 * Note that this almost certainly breaks the case where multiple
> +	 * devices with different DMA capabilities need to share a domain,
> +	 * but we don't have the necessary information to handle that here
> +	 * anyway - "proper" group and domain allocation needs to involve
> +	 * the IOMMU driver and a complete view of the bus.
> +	 */
> +
> +	iovad = kzalloc(sizeof(*iovad), GFP_KERNEL);
> +	if (!iovad)
> +		goto out_free_iommu_domain;
> +
> +	/* Use the smallest supported page size for IOVA granularity */
> +	order = __ffs(ops->pgsize_bitmap);
> +	base_pfn = max(dg->aperture_start >> order, (dma_addr_t)1);
> +	end_pfn = dg->aperture_end >> order;
> +	init_iova_domain(iovad, 1UL << order, base_pfn, end_pfn);
> +
> +	dom->domain = domain;
> +	dom->iovad = iovad;
> +	kref_init(&dom->kref);
> +	pr_debug("domain %p created\n", dom);
> +	return dom;
> +
> +out_free_iommu_domain:
> +	kfree(domain);
> +out_free_dma_domain:
> +	kfree(dom);
> +	return NULL;
> +}
> +
> +static void iommu_dma_free_domain(struct kref *kref)
> +{
> +	struct iommu_dma_domain *dom;
> +
> +	dom = container_of(kref, struct iommu_dma_domain, kref);
> +	put_iova_domain(dom->iovad);
> +	iommu_domain_free(dom->domain);
> +	kfree(dom);
> +	pr_debug("domain %p freed\n", dom);
> +}
> +
> +void iommu_dma_release_domain(struct iommu_dma_domain *dom)
> +{
> +	kref_put(&dom->kref, iommu_dma_free_domain);
> +}
> +
> +struct iommu_domain *iommu_dma_raw_domain(struct iommu_dma_domain *dom)
> +{
> +	return dom ? dom->domain : NULL;
> +}
> +
> +int iommu_dma_attach_device(struct device *dev, struct iommu_dma_domain *dom)
> +{
> +	int ret;
> +
> +	kref_get(&dom->kref);
> +	ret = iommu_attach_device(dom->domain, dev);
> +	if (ret)
> +		iommu_dma_release_domain(dom);
> +	else
> +		set_dma_domain(dev, dom);
> +	pr_debug("%s%s attached to domain %p\n", dev_name(dev),
> +			ret?" *not*":"", dom);
> +	return ret;
> +}
> +
> +void iommu_dma_detach_device(struct device *dev)
> +{
> +	struct iommu_dma_domain *dom = get_dma_domain(dev);
> +
> +	if (!dom) {
> +		dev_warn(dev, "Not attached\n");
> +		return;
> +	}
> +	set_dma_domain(dev, NULL);
> +	iommu_detach_device(dom->domain, dev);
> +	iommu_dma_release_domain(dom);
> +	pr_debug("%s detached from domain %p\n", dev_name(dev), dom);
> +}
> +
> +int iommu_dma_supported(struct device *dev, u64 mask)
> +{
> +	/*
> +	 * This looks awful, but really it's reasonable to assume that if an
> +	 * IOMMU can't address everything that the CPU can, it probably isn't
> +	 * generic enough to be using this implementation in the first place.
> +	 */
> +	return 1;
> +}
> +
> +int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
> +{
> +	return dma_addr == DMA_ERROR_CODE;
> +}
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> new file mode 100644
> index 0000000..4bba85a
> --- /dev/null
> +++ b/include/linux/dma-iommu.h
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (C) 2014 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef __DMA_IOMMU_H
> +#define __DMA_IOMMU_H
> +
> +#ifdef __KERNEL__
> +
> +#include <linux/types.h>
> +#include <linux/iommu.h>
> +
> +#ifdef CONFIG_IOMMU_DMA
> +
> +int iommu_dma_init(void);
> +
> +
> +struct iommu_dma_domain *iommu_dma_create_domain(const struct iommu_ops *ops,
> +		dma_addr_t base, size_t size);
> +void iommu_dma_release_domain(struct iommu_dma_domain *dma_domain);
> +
> +struct iommu_domain *iommu_dma_raw_domain(struct iommu_dma_domain *dma_domain);
> +
> +int iommu_dma_attach_device(struct device *dev, struct iommu_dma_domain *domain);
> +void iommu_dma_detach_device(struct device *dev);
> +
> +/*
> + * Implementation of these is left to arch code - it can associate domains
> + * with devices however it likes, provided the lookup is efficient.
> + */
> +struct iommu_dma_domain *get_dma_domain(struct device *dev);
> +void set_dma_domain(struct device *dev, struct iommu_dma_domain *dma_domain);
> +
> +
> +dma_addr_t iommu_dma_create_iova_mapping(struct device *dev,
> +		struct page **pages, size_t size, bool coherent);
> +int iommu_dma_release_iova_mapping(struct device *dev, dma_addr_t iova,
> +		size_t size);
> +
> +struct page **iommu_dma_alloc_buffer(struct device *dev, size_t size,
> +		gfp_t gfp, struct dma_attrs *attrs,
> +		void (clear_buffer)(struct page *page, size_t size));
> +int iommu_dma_free_buffer(struct device *dev, struct page **pages, size_t size,
> +		struct dma_attrs *attrs);
> +
> +dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> +		unsigned long offset, size_t size, enum dma_data_direction dir,
> +		struct dma_attrs *attrs);
> +dma_addr_t iommu_dma_coherent_map_page(struct device *dev, struct page *page,
> +		unsigned long offset, size_t size, enum dma_data_direction dir,
> +		struct dma_attrs *attrs);
> +void iommu_dma_unmap_page(struct device *dev, dma_addr_t handle, size_t size,
> +		enum dma_data_direction dir, struct dma_attrs *attrs);
> +
> +int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
> +		enum dma_data_direction dir, struct dma_attrs *attrs);
> +int iommu_dma_coherent_map_sg(struct device *dev, struct scatterlist *sg,
> +		int nents, enum dma_data_direction dir,
> +		struct dma_attrs *attrs);
> +void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sgl, int nents,
> +		enum dma_data_direction dir, struct dma_attrs *attrs);
> +
> +int iommu_dma_supported(struct device *dev, u64 mask);
> +int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
> +
> +#else
> +
> +static inline int iommu_dma_init(void)
> +{
> +	return 0;
> +}
> +
> +static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
> +{
> +	return NULL;
> +}
> +
> +void set_dma_domain(struct device *dev, struct iommu_dma_domain *dma_domain) { }
> +
> +#endif  /* CONFIG_IOMMU_DMA */
> +
> +#endif	/* __KERNEL__ */
> +#endif	/* __DMA_IOMMU_H */

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Robin Murphy]

Taking some inspiration from the arch/arm code, implement the
arch-specific side of the DMA mapping ops using the new IOMMU-DMA layer.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm64/include/asm/device.h      |   3 +
 arch/arm64/include/asm/dma-mapping.h |  17 ++
 arch/arm64/mm/dma-mapping.c          | 320 +++++++++++++++++++++++++++++++++++
 3 files changed, 340 insertions(+)

diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..510cee1 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -20,6 +20,9 @@ struct dev_archdata {
 	struct dma_map_ops *dma_ops;
 #ifdef CONFIG_IOMMU_API
 	void *iommu;			/* private IOMMU data */
+#ifdef CONFIG_IOMMU_DMA
+	struct iommu_dma_domain *dma_domain;
+#endif
 #endif
 	bool dma_coherent;
 };
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index 6932bb5..c1b271f 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -62,13 +62,30 @@ static inline bool is_device_dma_coherent(struct device *dev)
 
 #include <asm-generic/dma-mapping-common.h>
 
+#ifdef CONFIG_IOMMU_DMA
+static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
+{
+	return dev->archdata.dma_domain;
+}
+
+static inline void set_dma_domain(struct device *dev,
+				  struct iommu_dma_domain *dma_domain)
+{
+	dev->archdata.dma_domain = dma_domain;
+}
+#endif
+
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
+	if (WARN_ON(dev && get_dma_domain(dev)))
+		return DMA_ERROR_CODE;
 	return (dma_addr_t)paddr;
 }
 
 static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t dev_addr)
 {
+	if (WARN_ON(dev && get_dma_domain(dev)))
+		return 0;
 	return (phys_addr_t)dev_addr;
 }
 
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 0a24b9b..28e771c 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -23,6 +23,7 @@
 #include <linux/genalloc.h>
 #include <linux/dma-mapping.h>
 #include <linux/dma-contiguous.h>
+#include <linux/dma-iommu.h>
 #include <linux/vmalloc.h>
 #include <linux/swiotlb.h>
 
@@ -426,6 +427,7 @@ static int __init arm64_dma_init(void)
 
 	ret |= swiotlb_late_init();
 	ret |= atomic_pool_init();
+	ret |= iommu_dma_init();
 
 	return ret;
 }
@@ -439,3 +441,321 @@ static int __init dma_debug_do_init(void)
 	return 0;
 }
 fs_initcall(dma_debug_do_init);
+
+
+#ifdef CONFIG_IOMMU_DMA
+
+static struct page **__atomic_get_pages(void *addr)
+{
+	struct page *page;
+	phys_addr_t phys;
+
+	phys = gen_pool_virt_to_phys(atomic_pool, (unsigned long)addr);
+	page = phys_to_page(phys);
+
+	return (struct page **)page;
+}
+
+static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
+{
+	struct vm_struct *area;
+
+	if (__in_atomic_pool(cpu_addr, PAGE_SIZE))
+		return __atomic_get_pages(cpu_addr);
+
+	area = find_vm_area(cpu_addr);
+	if (!area)
+		return NULL;
+
+	return area->pages;
+}
+
+static void *__iommu_alloc_atomic(struct device *dev, size_t size,
+				  dma_addr_t *handle, bool coherent)
+{
+	struct page *page;
+	void *addr;
+
+	addr = __alloc_from_pool(size, &page);
+	if (!addr)
+		return NULL;
+
+	*handle = iommu_dma_create_iova_mapping(dev, &page, size, coherent);
+	if (*handle == DMA_ERROR_CODE) {
+		__free_from_pool(addr, size);
+		return NULL;
+	}
+	return addr;
+}
+
+static void __iommu_free_atomic(struct device *dev, void *cpu_addr,
+				dma_addr_t handle, size_t size)
+{
+	iommu_dma_release_iova_mapping(dev, handle, size);
+	__free_from_pool(cpu_addr, size);
+}
+
+static void __dma_clear_buffer(struct page *page, size_t size)
+{
+	void *ptr = page_address(page);
+
+	memset(ptr, 0, size);
+	__dma_flush_range(ptr, ptr + size);
+}
+
+static void *__iommu_alloc_attrs(struct device *dev, size_t size,
+				 dma_addr_t *handle, gfp_t gfp,
+				 struct dma_attrs *attrs)
+{
+	bool coherent = is_device_dma_coherent(dev);
+	pgprot_t prot = coherent ? __pgprot(PROT_NORMAL) :
+				   __pgprot(PROT_NORMAL_NC);
+	struct page **pages;
+	void *addr = NULL;
+
+	*handle = DMA_ERROR_CODE;
+	size = PAGE_ALIGN(size);
+
+	if (!(gfp & __GFP_WAIT))
+		return __iommu_alloc_atomic(dev, size, handle, coherent);
+	/*
+	 * FIXME: This isn't even true any more!
+	 *
+	 * Following is a work-around (a.k.a. hack) to prevent pages
+	 * with __GFP_COMP being passed to split_page() which cannot
+	 * handle them.  The real problem is that this flag probably
+	 * should be 0 on ARM as it is not supported on this
+	 * platform; see CONFIG_HUGETLBFS.
+	 */
+	gfp &= ~(__GFP_COMP);
+
+	pages = iommu_dma_alloc_buffer(dev, size, gfp, attrs,
+			__dma_clear_buffer);
+	if (!pages)
+		return NULL;
+
+	*handle = iommu_dma_create_iova_mapping(dev, pages, size, coherent);
+	if (*handle == DMA_ERROR_CODE)
+		goto err_mapping;
+
+	addr = dma_common_pages_remap(pages, size, VM_USERMAP,
+				      __get_dma_pgprot(attrs, prot, coherent),
+				      __builtin_return_address(0));
+	if (addr)
+		return addr;
+
+	iommu_dma_release_iova_mapping(dev, *handle, size);
+err_mapping:
+	iommu_dma_free_buffer(dev, pages, size, attrs);
+	return NULL;
+}
+
+static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
+			       dma_addr_t handle, struct dma_attrs *attrs)
+{
+	struct page **pages;
+
+	size = PAGE_ALIGN(size);
+	if (__in_atomic_pool(cpu_addr, size)) {
+		__iommu_free_atomic(dev, cpu_addr, handle, size);
+		return;
+	}
+
+	pages = __iommu_get_pages(cpu_addr, attrs);
+	if (!pages) {
+		WARN(1, "trying to free invalid coherent area: %p\n", cpu_addr);
+		return;
+	}
+
+	dma_common_free_remap(cpu_addr, size, VM_USERMAP);
+
+	iommu_dma_release_iova_mapping(dev, handle, size);
+	iommu_dma_free_buffer(dev, pages, size, attrs);
+}
+
+static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
+			      void *cpu_addr, dma_addr_t dma_addr, size_t size,
+			      struct dma_attrs *attrs)
+{
+	unsigned long uaddr = vma->vm_start;
+	unsigned long usize = vma->vm_end - vma->vm_start;
+	struct page **pages = __iommu_get_pages(cpu_addr, attrs);
+	int ret;
+
+	if (!pages)
+		return -ENXIO;
+
+	vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot,
+					     is_device_dma_coherent(dev));
+
+	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
+		return ret;
+
+	do {
+		ret = vm_insert_page(vma, uaddr, *pages++);
+		if (ret) {
+			pr_err("Remapping memory failed: %d\n", ret);
+			return ret;
+		}
+		uaddr += PAGE_SIZE;
+		usize -= PAGE_SIZE;
+	} while (usize > 0);
+
+	return 0;
+}
+
+static void __iommu_sync_single_for_cpu(struct device *dev,
+					dma_addr_t dev_addr, size_t size,
+					enum dma_data_direction dir)
+{
+	if (!is_device_dma_coherent(dev) && (dev_addr != DMA_ERROR_CODE)) {
+		struct iommu_dma_domain *dma_domain = get_dma_domain(dev);
+		struct iommu_domain *domain = iommu_dma_raw_domain(dma_domain);
+		phys_addr_t phys = iommu_iova_to_phys(domain, dev_addr);
+
+		__dma_unmap_area(phys_to_virt(phys), size, dir);
+	}
+}
+
+static void __iommu_sync_single_for_device(struct device *dev,
+					   dma_addr_t dev_addr, size_t size,
+					   enum dma_data_direction dir)
+{
+	if (!is_device_dma_coherent(dev) && (dev_addr != DMA_ERROR_CODE)) {
+		struct iommu_dma_domain *dma_domain = get_dma_domain(dev);
+		struct iommu_domain *domain = iommu_dma_raw_domain(dma_domain);
+		phys_addr_t phys = iommu_iova_to_phys(domain, dev_addr);
+
+		__dma_map_area(phys_to_virt(phys), size, dir);
+	}
+}
+
+static dma_addr_t __iommu_map_page(struct device *dev, struct page *page,
+				   unsigned long offset, size_t size,
+				   enum dma_data_direction dir,
+				   struct dma_attrs *attrs)
+{
+	dma_addr_t dev_addr;
+
+	if (is_device_dma_coherent(dev))
+		return iommu_dma_coherent_map_page(dev, page, offset, size,
+				dir, attrs);
+
+	dev_addr = iommu_dma_map_page(dev, page, offset, size, dir, attrs);
+
+	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
+		__iommu_sync_single_for_device(dev, dev_addr, size, dir);
+
+	return dev_addr;
+}
+
+static void __iommu_unmap_page(struct device *dev, dma_addr_t dev_addr,
+			       size_t size, enum dma_data_direction dir,
+			       struct dma_attrs *attrs)
+{
+	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
+		__iommu_sync_single_for_cpu(dev, dev_addr, size, dir);
+
+	iommu_dma_unmap_page(dev, dev_addr, size, dir, attrs);
+}
+
+static void __iommu_sync_sg_for_cpu(struct device *dev,
+				    struct scatterlist *sgl, int nelems,
+				    enum dma_data_direction dir)
+{
+	struct scatterlist *sg;
+	int i;
+
+	if (is_device_dma_coherent(dev))
+		return;
+
+	for_each_sg(sgl, sg, nelems, i)
+		__dma_unmap_area(sg_virt(sg), sg->length, dir);
+}
+
+static void __iommu_sync_sg_for_device(struct device *dev,
+				       struct scatterlist *sgl, int nelems,
+				       enum dma_data_direction dir)
+{
+	struct scatterlist *sg;
+	int i;
+
+	if (is_device_dma_coherent(dev))
+		return;
+
+	for_each_sg(sgl, sg, nelems, i)
+		__dma_map_area(sg_virt(sg), sg->length, dir);
+}
+
+static int __iommu_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
+				int nelems, enum dma_data_direction dir,
+				struct dma_attrs *attrs)
+{
+	if (is_device_dma_coherent(dev))
+		return iommu_dma_coherent_map_sg(dev, sgl, nelems, dir, attrs);
+
+	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
+		__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
+
+	return iommu_dma_map_sg(dev, sgl, nelems, dir, attrs);
+}
+
+static void __iommu_unmap_sg_attrs(struct device *dev,
+				   struct scatterlist *sgl, int nelems,
+				   enum dma_data_direction dir,
+				   struct dma_attrs *attrs)
+{
+	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
+		__iommu_sync_sg_for_cpu(dev, sgl, nelems, dir);
+
+	iommu_dma_unmap_sg(dev, sgl, nelems, dir, attrs);
+}
+
+static struct dma_map_ops iommu_dma_ops = {
+	.alloc = __iommu_alloc_attrs,
+	.free = __iommu_free_attrs,
+	.mmap = __iommu_mmap_attrs,
+	.map_page = __iommu_map_page,
+	.unmap_page = __iommu_unmap_page,
+	.map_sg = __iommu_map_sg_attrs,
+	.unmap_sg = __iommu_unmap_sg_attrs,
+	.sync_single_for_cpu = __iommu_sync_single_for_cpu,
+	.sync_single_for_device = __iommu_sync_single_for_device,
+	.sync_sg_for_cpu = __iommu_sync_sg_for_cpu,
+	.sync_sg_for_device = __iommu_sync_sg_for_device,
+	.dma_supported = iommu_dma_supported,
+	.mapping_error = iommu_dma_mapping_error,
+};
+
+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				  const struct iommu_ops *ops)
+{
+	struct iommu_dma_domain *dma_domain;
+
+	if (!ops)
+		return;
+
+	dma_domain = iommu_dma_create_domain(ops, dma_base, size);
+	if (!dma_domain) {
+		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
+				size, dev_name(dev));
+		return;
+	}
+
+	if (iommu_dma_attach_device(dev, dma_domain))
+		pr_warn("Failed to attach device %s to IOMMU mapping\n",
+				dev_name(dev));
+	else
+		dev->archdata.dma_ops = &iommu_dma_ops;
+
+	/* drop the initial mapping refcount */
+	iommu_dma_release_domain(dma_domain);
+}
+
+#else
+
+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				  struct iommu_ops *iommu)
+{ }
+
+#endif  /* CONFIG_IOMMU_DMA */
-- 
1.9.1

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Will Deacon]

On Fri, Feb 06, 2015 at 02:55:14PM +0000, Robin Murphy wrote:
> Taking some inspiration from the arch/arm code, implement the
> arch-specific side of the DMA mapping ops using the new IOMMU-DMA layer.

Is anybody looking at porting arch/arm/ over to this?

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  arch/arm64/include/asm/device.h      |   3 +
>  arch/arm64/include/asm/dma-mapping.h |  17 ++
>  arch/arm64/mm/dma-mapping.c          | 320 +++++++++++++++++++++++++++++++++++
>  3 files changed, 340 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
> index 243ef25..510cee1 100644
> --- a/arch/arm64/include/asm/device.h
> +++ b/arch/arm64/include/asm/device.h
> @@ -20,6 +20,9 @@ struct dev_archdata {
>  	struct dma_map_ops *dma_ops;
>  #ifdef CONFIG_IOMMU_API
>  	void *iommu;			/* private IOMMU data */
> +#ifdef CONFIG_IOMMU_DMA
> +	struct iommu_dma_domain *dma_domain;
> +#endif
>  #endif
>  	bool dma_coherent;
>  };
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index 6932bb5..c1b271f 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -62,13 +62,30 @@ static inline bool is_device_dma_coherent(struct device *dev)
>  
>  #include <asm-generic/dma-mapping-common.h>
>  
> +#ifdef CONFIG_IOMMU_DMA
> +static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
> +{
> +	return dev->archdata.dma_domain;
> +}

You use this function in patch 1, so you probably need to reorder the series
slightly.

> +static inline void set_dma_domain(struct device *dev,
> +				  struct iommu_dma_domain *dma_domain)
> +{
> +	dev->archdata.dma_domain = dma_domain;
> +}
> +#endif
> +
>  static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
>  {
> +	if (WARN_ON(dev && get_dma_domain(dev)))
> +		return DMA_ERROR_CODE;
>  	return (dma_addr_t)paddr;
>  }
>  
>  static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t dev_addr)
>  {
> +	if (WARN_ON(dev && get_dma_domain(dev)))
> +		return 0;
>  	return (phys_addr_t)dev_addr;
>  }
>  
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 0a24b9b..28e771c 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -23,6 +23,7 @@
>  #include <linux/genalloc.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/dma-contiguous.h>
> +#include <linux/dma-iommu.h>
>  #include <linux/vmalloc.h>
>  #include <linux/swiotlb.h>
>  
> @@ -426,6 +427,7 @@ static int __init arm64_dma_init(void)
>  
>  	ret |= swiotlb_late_init();
>  	ret |= atomic_pool_init();
> +	ret |= iommu_dma_init();
>  
>  	return ret;
>  }
> @@ -439,3 +441,321 @@ static int __init dma_debug_do_init(void)
>  	return 0;
>  }
>  fs_initcall(dma_debug_do_init);
> +
> +
> +#ifdef CONFIG_IOMMU_DMA
> +
> +static struct page **__atomic_get_pages(void *addr)
> +{
> +	struct page *page;
> +	phys_addr_t phys;
> +
> +	phys = gen_pool_virt_to_phys(atomic_pool, (unsigned long)addr);
> +	page = phys_to_page(phys);
> +
> +	return (struct page **)page;
> +}
> +
> +static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
> +{
> +	struct vm_struct *area;
> +
> +	if (__in_atomic_pool(cpu_addr, PAGE_SIZE))
> +		return __atomic_get_pages(cpu_addr);
> +
> +	area = find_vm_area(cpu_addr);
> +	if (!area)
> +		return NULL;
> +
> +	return area->pages;
> +}
> +
> +static void *__iommu_alloc_atomic(struct device *dev, size_t size,
> +				  dma_addr_t *handle, bool coherent)
> +{
> +	struct page *page;
> +	void *addr;
> +
> +	addr = __alloc_from_pool(size, &page);
> +	if (!addr)
> +		return NULL;
> +
> +	*handle = iommu_dma_create_iova_mapping(dev, &page, size, coherent);
> +	if (*handle == DMA_ERROR_CODE) {
> +		__free_from_pool(addr, size);
> +		return NULL;
> +	}
> +	return addr;
> +}
> +
> +static void __iommu_free_atomic(struct device *dev, void *cpu_addr,
> +				dma_addr_t handle, size_t size)
> +{
> +	iommu_dma_release_iova_mapping(dev, handle, size);
> +	__free_from_pool(cpu_addr, size);
> +}

Up until this point, I don't get why things need to be arch-specific. Surely
we should only care about cache and MMU management in the arch code?

> +static void __dma_clear_buffer(struct page *page, size_t size)
> +{
> +	void *ptr = page_address(page);
> +
> +	memset(ptr, 0, size);
> +	__dma_flush_range(ptr, ptr + size);
> +}
> +
> +static void *__iommu_alloc_attrs(struct device *dev, size_t size,
> +				 dma_addr_t *handle, gfp_t gfp,
> +				 struct dma_attrs *attrs)
> +{
> +	bool coherent = is_device_dma_coherent(dev);
> +	pgprot_t prot = coherent ? __pgprot(PROT_NORMAL) :
> +				   __pgprot(PROT_NORMAL_NC);
> +	struct page **pages;
> +	void *addr = NULL;
> +
> +	*handle = DMA_ERROR_CODE;
> +	size = PAGE_ALIGN(size);
> +
> +	if (!(gfp & __GFP_WAIT))
> +		return __iommu_alloc_atomic(dev, size, handle, coherent);
> +	/*
> +	 * FIXME: This isn't even true any more!
> +	 *
> +	 * Following is a work-around (a.k.a. hack) to prevent pages
> +	 * with __GFP_COMP being passed to split_page() which cannot
> +	 * handle them.  The real problem is that this flag probably
> +	 * should be 0 on ARM as it is not supported on this
> +	 * platform; see CONFIG_HUGETLBFS.
> +	 */
> +	gfp &= ~(__GFP_COMP);

We should fix this... is it not just a case of calling split_huge_page
for compond pages?

Will

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Robin Murphy]

On 09/02/15 06:02, Will Deacon wrote:
> On Fri, Feb 06, 2015 at 02:55:14PM +0000, Robin Murphy wrote:
>> Taking some inspiration from the arch/arm code, implement the
>> arch-specific side of the DMA mapping ops using the new IOMMU-DMA layer.
>
> Is anybody looking at porting arch/arm/ over to this?

If not, future me from once-there-are-no-more-major-changes-to-patch-1 
will be.

>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   arch/arm64/include/asm/device.h      |   3 +
>>   arch/arm64/include/asm/dma-mapping.h |  17 ++
>>   arch/arm64/mm/dma-mapping.c          | 320 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 340 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
>> index 243ef25..510cee1 100644
>> --- a/arch/arm64/include/asm/device.h
>> +++ b/arch/arm64/include/asm/device.h
>> @@ -20,6 +20,9 @@ struct dev_archdata {
>>   	struct dma_map_ops *dma_ops;
>>   #ifdef CONFIG_IOMMU_API
>>   	void *iommu;			/* private IOMMU data */
>> +#ifdef CONFIG_IOMMU_DMA
>> +	struct iommu_dma_domain *dma_domain;
>> +#endif
>>   #endif
>>   	bool dma_coherent;
>>   };
>> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
>> index 6932bb5..c1b271f 100644
>> --- a/arch/arm64/include/asm/dma-mapping.h
>> +++ b/arch/arm64/include/asm/dma-mapping.h
>> @@ -62,13 +62,30 @@ static inline bool is_device_dma_coherent(struct device *dev)
>>
>>   #include <asm-generic/dma-mapping-common.h>
>>
>> +#ifdef CONFIG_IOMMU_DMA
>> +static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
>> +{
>> +	return dev->archdata.dma_domain;
>> +}
>
> You use this function in patch 1, so you probably need to reorder the series
> slightly.

Good point. I need to introduce the dummy stubs unconditionally first, 
then configure them out here.

>> +static inline void set_dma_domain(struct device *dev,
>> +				  struct iommu_dma_domain *dma_domain)
>> +{
>> +	dev->archdata.dma_domain = dma_domain;
>> +}
>> +#endif
>> +
>>   static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
>>   {
>> +	if (WARN_ON(dev && get_dma_domain(dev)))
>> +		return DMA_ERROR_CODE;
>>   	return (dma_addr_t)paddr;
>>   }
>>
>>   static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t dev_addr)
>>   {
>> +	if (WARN_ON(dev && get_dma_domain(dev)))
>> +		return 0;
>>   	return (phys_addr_t)dev_addr;
>>   }
>>
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index 0a24b9b..28e771c 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/genalloc.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/dma-contiguous.h>
>> +#include <linux/dma-iommu.h>
>>   #include <linux/vmalloc.h>
>>   #include <linux/swiotlb.h>
>>
>> @@ -426,6 +427,7 @@ static int __init arm64_dma_init(void)
>>
>>   	ret |= swiotlb_late_init();
>>   	ret |= atomic_pool_init();
>> +	ret |= iommu_dma_init();
>>
>>   	return ret;
>>   }
>> @@ -439,3 +441,321 @@ static int __init dma_debug_do_init(void)
>>   	return 0;
>>   }
>>   fs_initcall(dma_debug_do_init);
>> +
>> +
>> +#ifdef CONFIG_IOMMU_DMA
>> +
>> +static struct page **__atomic_get_pages(void *addr)
>> +{
>> +	struct page *page;
>> +	phys_addr_t phys;
>> +
>> +	phys = gen_pool_virt_to_phys(atomic_pool, (unsigned long)addr);
>> +	page = phys_to_page(phys);
>> +
>> +	return (struct page **)page;
>> +}
>> +
>> +static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
>> +{
>> +	struct vm_struct *area;
>> +
>> +	if (__in_atomic_pool(cpu_addr, PAGE_SIZE))
>> +		return __atomic_get_pages(cpu_addr);
>> +
>> +	area = find_vm_area(cpu_addr);
>> +	if (!area)
>> +		return NULL;
>> +
>> +	return area->pages;
>> +}
>> +
>> +static void *__iommu_alloc_atomic(struct device *dev, size_t size,
>> +				  dma_addr_t *handle, bool coherent)
>> +{
>> +	struct page *page;
>> +	void *addr;
>> +
>> +	addr = __alloc_from_pool(size, &page);
>> +	if (!addr)
>> +		return NULL;
>> +
>> +	*handle = iommu_dma_create_iova_mapping(dev, &page, size, coherent);
>> +	if (*handle == DMA_ERROR_CODE) {
>> +		__free_from_pool(addr, size);
>> +		return NULL;
>> +	}
>> +	return addr;
>> +}
>> +
>> +static void __iommu_free_atomic(struct device *dev, void *cpu_addr,
>> +				dma_addr_t handle, size_t size)
>> +{
>> +	iommu_dma_release_iova_mapping(dev, handle, size);
>> +	__free_from_pool(cpu_addr, size);
>> +}
>
> Up until this point, I don't get why things need to be arch-specific. Surely
> we should only care about cache and MMU management in the arch code?

I agree, but the atomic pool implementation being private got in the 
way. However, the idea of replacing the _get_pages() stuff with 
scatterlists makes me think I can clean this up too.

>> +static void __dma_clear_buffer(struct page *page, size_t size)
>> +{
>> +	void *ptr = page_address(page);
>> +
>> +	memset(ptr, 0, size);
>> +	__dma_flush_range(ptr, ptr + size);
>> +}
>> +
>> +static void *__iommu_alloc_attrs(struct device *dev, size_t size,
>> +				 dma_addr_t *handle, gfp_t gfp,
>> +				 struct dma_attrs *attrs)
>> +{
>> +	bool coherent = is_device_dma_coherent(dev);
>> +	pgprot_t prot = coherent ? __pgprot(PROT_NORMAL) :
>> +				   __pgprot(PROT_NORMAL_NC);
>> +	struct page **pages;
>> +	void *addr = NULL;
>> +
>> +	*handle = DMA_ERROR_CODE;
>> +	size = PAGE_ALIGN(size);
>> +
>> +	if (!(gfp & __GFP_WAIT))
>> +		return __iommu_alloc_atomic(dev, size, handle, coherent);
>> +	/*
>> +	 * FIXME: This isn't even true any more!
>> +	 *
>> +	 * Following is a work-around (a.k.a. hack) to prevent pages
>> +	 * with __GFP_COMP being passed to split_page() which cannot
>> +	 * handle them.  The real problem is that this flag probably
>> +	 * should be 0 on ARM as it is not supported on this
>> +	 * platform; see CONFIG_HUGETLBFS.
>> +	 */
>> +	gfp &= ~(__GFP_COMP);
>
> We should fix this... is it not just a case of calling split_huge_page
> for compond pages?

I've been reading up on the matter and so far I'd concur, but I'm 
starting to wonder if we really need to do half of this at all. AFAICT 
the only real reason for splitting pages is so the mmap() implementation 
can use vm_insert_page() - if that was changed to use remap_pfn_range() 
instead would the need go away entirely? (SWIOTLB doesn't seem to worry 
about any of this stuff)

Robin.

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Yingjoe Chen]

On Fri, 2015-02-06 at 14:55 +0000, Robin Murphy wrote
<...>
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index 6932bb5..c1b271f 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -62,13 +62,30 @@ static inline bool is_device_dma_coherent(struct device *dev)
>  
>  #include <asm-generic/dma-mapping-common.h>
>  
> +#ifdef CONFIG_IOMMU_DMA
> +static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
> +{
> +	return dev->archdata.dma_domain;
> +}
> +
> +static inline void set_dma_domain(struct device *dev,
> +				  struct iommu_dma_domain *dma_domain)
> +{
> +	dev->archdata.dma_domain = dma_domain;
> +}
> +#endif
> +
>  static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
>  {
> +	if (WARN_ON(dev && get_dma_domain(dev)))
> +		return DMA_ERROR_CODE;
>  	return (dma_addr_t)paddr;
>  }


Hi Robin, 

Build fail if CONFIG_IOMMU_DMA is not enabled.

In file included from ../include/linux/dma-mapping.h:82:0,
                 from ../arch/arm64/kernel/asm-offsets.c:23:
../arch/arm64/include/asm/dma-mapping.h: In function 'phys_to_dma':
../arch/arm64/include/asm/dma-mapping.h:81:2: error: implicit declaration of function 'get_dma_domain' [-Werror=implicit-function-declaration]
  if (WARN_ON(dev && get_dma_domain(dev)))
  ^

Joe.C

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Robin Murphy]

On 10/02/15 04:39, Yingjoe Chen wrote:
> On Fri, 2015-02-06 at 14:55 +0000, Robin Murphy wrote
> <...>
>> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
>> index 6932bb5..c1b271f 100644
>> --- a/arch/arm64/include/asm/dma-mapping.h
>> +++ b/arch/arm64/include/asm/dma-mapping.h
>> @@ -62,13 +62,30 @@ static inline bool is_device_dma_coherent(struct device *dev)
>>
>>   #include <asm-generic/dma-mapping-common.h>
>>
>> +#ifdef CONFIG_IOMMU_DMA
>> +static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
>> +{
>> +	return dev->archdata.dma_domain;
>> +}
>> +
>> +static inline void set_dma_domain(struct device *dev,
>> +				  struct iommu_dma_domain *dma_domain)
>> +{
>> +	dev->archdata.dma_domain = dma_domain;
>> +}
>> +#endif
>> +
>>   static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
>>   {
>> +	if (WARN_ON(dev && get_dma_domain(dev)))
>> +		return DMA_ERROR_CODE;
>>   	return (dma_addr_t)paddr;
>>   }
>
>
> Hi Robin,
>
> Build fail if CONFIG_IOMMU_DMA is not enabled.
>
> In file included from ../include/linux/dma-mapping.h:82:0,
>                   from ../arch/arm64/kernel/asm-offsets.c:23:
> ../arch/arm64/include/asm/dma-mapping.h: In function 'phys_to_dma':
> ../arch/arm64/include/asm/dma-mapping.h:81:2: error: implicit declaration of function 'get_dma_domain' [-Werror=implicit-function-declaration]
>    if (WARN_ON(dev && get_dma_domain(dev)))
>    ^
>
> Joe.C

Bah, how did I manage to make such a half-finished mess of the includes? 
Current fixup diff below.

Thanks,
Robin.

--->8---
diff --git a/arch/arm64/include/asm/dma-mapping.h 
b/arch/arm64/include/asm/dma-mapping.h
index 5246d1a..208a268 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -58,6 +58,7 @@ static inline bool is_device_dma_coherent(struct 
device *dev)
  }

  #include <asm-generic/dma-mapping-common.h>
+#include <linux/dma-iommu.h>

  #ifdef CONFIG_IOMMU_DMA

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 9267b20..412d61a 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -23,7 +23,6 @@
  #include <linux/genalloc.h>
  #include <linux/dma-mapping.h>
  #include <linux/dma-contiguous.h>
-#include <linux/dma-iommu.h>
  #include <linux/vmalloc.h>
  #include <linux/swiotlb.h>

@@ -438,6 +437,7 @@ fs_initcall(dma_debug_do_init);


  #ifdef CONFIG_IOMMU_DMA
+#include <linux/iommu.h>

  static struct page **__atomic_get_pages(void *addr)
  {
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 19027bb..6615cfd 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -22,7 +22,7 @@ config OF_IOMMU

  # IOMMU-agnostic DMA-mapping layer
  config IOMMU_DMA
-	def_bool n
+	bool
  	depends on NEED_SG_DMA_LENGTH
  	select IOMMU_API
  	select IOMMU_IOVA
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index b97cc0b9..6c7ca0b 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -23,6 +23,7 @@

  #include <linux/dma-contiguous.h>
  #include <linux/dma-iommu.h>
+#include <linux/iommu.h>
  #include <linux/iova.h>

  int iommu_dma_init(void)
@@ -38,8 +39,10 @@ struct iommu_dma_domain {

  static inline dma_addr_t dev_dma_addr(struct device *dev, dma_addr_t addr)
  {
-	BUG_ON(addr < dev->dma_pfn_offset);
-	return addr - ((dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT);
+	dma_addr_t offset = (dma_addr_t)dev->dma_pfn_offset << PAGE_SHIFT;
+
+	BUG_ON(addr < offset);
+	return addr - offset;
  }

  static int __dma_direction_to_prot(enum dma_data_direction dir, bool 
coherent)
@@ -65,9 +68,9 @@ static struct iova *__alloc_iova(struct device *dev, 
size_t size, bool coherent)
  	unsigned long shift = iova_shift(iovad);
  	unsigned long length = iova_align(iovad, size) >> shift;
  	unsigned long limit_pfn = iovad->dma_32bit_pfn;
-	u64 dma_limit = coherent ? dev->coherent_dma_mask : *dev->dma_mask;
+	u64 dma_limit = coherent ? dev->coherent_dma_mask : dma_get_mask(dev);

-	limit_pfn = min(limit_pfn, (unsigned long)(dma_limit >> shift));
+	limit_pfn = min_t(unsigned long, limit_pfn, dma_limit >> shift);
  	/* Alignment should probably come from a domain/device attribute... */
  	return alloc_iova(iovad, length, limit_pfn, false);
  }
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 4bba85a..8de28ad 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -18,8 +18,7 @@

  #ifdef __KERNEL__

-#include <linux/types.h>
-#include <linux/iommu.h>
+#include <linux/dma-mapping.h>

  #ifdef CONFIG_IOMMU_DMA

@@ -39,8 +38,9 @@ void iommu_dma_detach_device(struct device *dev);
   * Implementation of these is left to arch code - it can associate domains
   * with devices however it likes, provided the lookup is efficient.
   */
-struct iommu_dma_domain *get_dma_domain(struct device *dev);
-void set_dma_domain(struct device *dev, struct iommu_dma_domain 
*dma_domain);
+static inline struct iommu_dma_domain *get_dma_domain(struct device *dev);
+static inline void set_dma_domain(struct device *dev,
+		struct iommu_dma_domain *dma_domain);


  dma_addr_t iommu_dma_create_iova_mapping(struct device *dev,
@@ -86,7 +86,9 @@ static inline struct iommu_dma_domain 
*get_dma_domain(struct device *dev)
  	return NULL;
  }

-void set_dma_domain(struct device *dev, struct iommu_dma_domain 
*dma_domain) { }
+static inline void set_dma_domain(struct device *dev,
+		struct iommu_dma_domain *dma_domain)
+{ }

  #endif  /* CONFIG_IOMMU_DMA */

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Yong Wu]

On Tue, 2015-02-10 at 12:07 +0000, Robin Murphy wrote:
> On 10/02/15 04:39, Yingjoe Chen wrote:
> > On Fri, 2015-02-06 at 14:55 +0000, Robin Murphy wrote
> > <...>
> >> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> >> index 6932bb5..c1b271f 100644
> >> --- a/arch/arm64/include/asm/dma-mapping.h
> >> +++ b/arch/arm64/include/asm/dma-mapping.h
> >> @@ -62,13 +62,30 @@ static inline bool is_device_dma_coherent(struct device *dev)
> >>
> >>   #include <asm-generic/dma-mapping-common.h>
> >>
> >> +#ifdef CONFIG_IOMMU_DMA
> >> +static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
> >> +{
> >> +	return dev->archdata.dma_domain;
> >> +}
> >> +
> >> +static inline void set_dma_domain(struct device *dev,
> >> +				  struct iommu_dma_domain *dma_domain)
> >> +{
> >> +	dev->archdata.dma_domain = dma_domain;
> >> +}
> >> +#endif
> >> +
> >>   static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
> >>   {
> >> +	if (WARN_ON(dev && get_dma_domain(dev)))
> >> +		return DMA_ERROR_CODE;
> >>   	return (dma_addr_t)paddr;
> >>   }
> >
> >
> > Hi Robin,
> >
> > Build fail if CONFIG_IOMMU_DMA is not enabled.
> >
> > In file included from ../include/linux/dma-mapping.h:82:0,
> >                   from ../arch/arm64/kernel/asm-offsets.c:23:
> > ../arch/arm64/include/asm/dma-mapping.h: In function 'phys_to_dma':
> > ../arch/arm64/include/asm/dma-mapping.h:81:2: error: implicit declaration of function 'get_dma_domain' [-Werror=implicit-function-declaration]
> >    if (WARN_ON(dev && get_dma_domain(dev)))
> >    ^
> >
> > Joe.C
> 
> Bah, how did I manage to make such a half-finished mess of the includes? 
> Current fixup diff below.
> 
> Thanks,
> Robin.
> 
Dear Robin,
     We have test this patch on our mt8173, it also could work well.
Tested-by: Yong Wu <yong.wu@mediatek.com>

     And I have 2 questiones about how to use this iommu.
     1)if We call "arch_setup_dma_ops" to create a iommu domain, then is
this function "bus_set_iommu" who also need "struct iommu_ops *"
necessary to be called or not in your design?
     2)int (*domain_init)(struct iommu_domain *domain);
       About this function, it will alloc pagetable for the iommu
domain. And We expect the pagetable memory is uncacheable, so try to
call "dma_alloc_coherent", unfortunately the "struct device *" can't be
passed into this function. so is it possible if adding a parameter in
this function.

 Thanks.

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Robin Murphy]

On 14/02/15 08:03, Yong Wu wrote:
> On Tue, 2015-02-10 at 12:07 +0000, Robin Murphy wrote:
>> On 10/02/15 04:39, Yingjoe Chen wrote:
>>> On Fri, 2015-02-06 at 14:55 +0000, Robin Murphy wrote
>>> <...>
>>>> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
>>>> index 6932bb5..c1b271f 100644
>>>> --- a/arch/arm64/include/asm/dma-mapping.h
>>>> +++ b/arch/arm64/include/asm/dma-mapping.h
>>>> @@ -62,13 +62,30 @@ static inline bool is_device_dma_coherent(struct device *dev)
>>>>
>>>>    #include <asm-generic/dma-mapping-common.h>
>>>>
>>>> +#ifdef CONFIG_IOMMU_DMA
>>>> +static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
>>>> +{
>>>> +	return dev->archdata.dma_domain;
>>>> +}
>>>> +
>>>> +static inline void set_dma_domain(struct device *dev,
>>>> +				  struct iommu_dma_domain *dma_domain)
>>>> +{
>>>> +	dev->archdata.dma_domain = dma_domain;
>>>> +}
>>>> +#endif
>>>> +
>>>>    static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
>>>>    {
>>>> +	if (WARN_ON(dev && get_dma_domain(dev)))
>>>> +		return DMA_ERROR_CODE;
>>>>    	return (dma_addr_t)paddr;
>>>>    }
>>>
>>>
>>> Hi Robin,
>>>
>>> Build fail if CONFIG_IOMMU_DMA is not enabled.
>>>
>>> In file included from ../include/linux/dma-mapping.h:82:0,
>>>                    from ../arch/arm64/kernel/asm-offsets.c:23:
>>> ../arch/arm64/include/asm/dma-mapping.h: In function 'phys_to_dma':
>>> ../arch/arm64/include/asm/dma-mapping.h:81:2: error: implicit declaration of function 'get_dma_domain' [-Werror=implicit-function-declaration]
>>>     if (WARN_ON(dev && get_dma_domain(dev)))
>>>     ^
>>>
>>> Joe.C
>>
>> Bah, how did I manage to make such a half-finished mess of the includes?
>> Current fixup diff below.
>>
>> Thanks,
>> Robin.
>>
> Dear Robin,
>       We have test this patch on our mt8173, it also could work well.
> Tested-by: Yong Wu <yong.wu@mediatek.com>
>
>       And I have 2 questiones about how to use this iommu.
>       1)if We call "arch_setup_dma_ops" to create a iommu domain, then is
> this function "bus_set_iommu" who also need "struct iommu_ops *"
> necessary to be called or not in your design?

I've intentionally bypassed bus_set_iommu() because it doesn't 
necessarily work well for platform devices - I've got 7 separate IOMMUs 
here and I need to know which is which, so the notion of "the IOMMU for 
the platform bus" doesn't make a whole lot of sense. Thus 
iommu_dma_create_domain() creates the iommu_domain directly using the 
iommu_ops provided, because...

>       2)int (*domain_init)(struct iommu_domain *domain);
>         About this function, it will alloc pagetable for the iommu
> domain. And We expect the pagetable memory is uncacheable, so try to
> call "dma_alloc_coherent", unfortunately the "struct device *" can't be
> passed into this function. so is it possible if adding a parameter in
> this function.

...one of the ideas of the new of_iommu_configure framework is that 
iommu_ops structures can represent individual IOMMU devices if 
necessary. The ops->priv pointer was included for that purpose, but 
isn't very clean so Will has plans to remove it again - it's easy enough 
to achieve the same effect by having the driver embed the ops in its 
private instance data instead. I've done that with the ARM SMMU driver 
which has a similar issue of needing hardware details at domain_init() 
time (no patches ready yet but I have an iommu/dev branch on top of the 
iommu/dma branch with some current work-in-progress bits)

Thanks,
Robin.

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Yong Wu]

On Mon, 2015-02-16 at 20:04 +0000, Robin Murphy wrote:

> >       2)int (*domain_init)(struct iommu_domain *domain);
> >         About this function, it will alloc pagetable for the iommu
> > domain. And We expect the pagetable memory is uncacheable, so try to
> > call "dma_alloc_coherent", unfortunately the "struct device *" can't be
> > passed into this function. so is it possible if adding a parameter in
> > this function.
> 
> ...one of the ideas of the new of_iommu_configure framework is that 
> iommu_ops structures can represent individual IOMMU devices if 
> necessary. The ops->priv pointer was included for that purpose, but 
> isn't very clean so Will has plans to remove it again - it's easy enough 
> to achieve the same effect by having the driver embed the ops in its 
> private instance data instead. I've done that with the ARM SMMU driver 
> which has a similar issue of needing hardware details at domain_init() 
> time (no patches ready yet but I have an iommu/dev branch on top of the 
> iommu/dma branch with some current work-in-progress bits)
> 
> Thanks,
> Robin.
> 
Dear Robin,
        Thanks very much for your suggestion.

        I have a more question: how to do cache maintenance in arm64.

        a) "__dma_flush_range" can not be expected to used directly.
        b) Following Documentation\DMA-API.txt, section: Streaming DMA
mappings. dma_map_single should be used, But I am not sure how to pass
the first "struct device *".
         We have a device which call arch_setup_dma_ops to create the
iommu domain, if we use this device, it will enter __iommu_map_page;
        If we input NULL for dmap_map_single, it will assert at
dma_capable in swiotlb_map_page...

        And normally, we always need do cache maintenance only for some
bytes in the pagetable but not whole a page. Then is there a easy way to
do the cache maintenance?

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Robin Murphy]

On 03/03/15 03:38, Yong Wu wrote:
> On Mon, 2015-02-16 at 20:04 +0000, Robin Murphy wrote:
>
>>>        2)int (*domain_init)(struct iommu_domain *domain);
>>>          About this function, it will alloc pagetable for the iommu
>>> domain. And We expect the pagetable memory is uncacheable, so try to
>>> call "dma_alloc_coherent", unfortunately the "struct device *" can't be
>>> passed into this function. so is it possible if adding a parameter in
>>> this function.
>>
>> ...one of the ideas of the new of_iommu_configure framework is that
>> iommu_ops structures can represent individual IOMMU devices if
>> necessary. The ops->priv pointer was included for that purpose, but
>> isn't very clean so Will has plans to remove it again - it's easy enough
>> to achieve the same effect by having the driver embed the ops in its
>> private instance data instead. I've done that with the ARM SMMU driver
>> which has a similar issue of needing hardware details at domain_init()
>> time (no patches ready yet but I have an iommu/dev branch on top of the
>> iommu/dma branch with some current work-in-progress bits)
>>
>> Thanks,
>> Robin.
>>
> Dear Robin,
>          Thanks very much for your suggestion.
>
>          I have a more question: how to do cache maintenance in arm64.
>
>          a) "__dma_flush_range" can not be expected to used directly.
>          b) Following Documentation\DMA-API.txt, section: Streaming DMA
> mappings. dma_map_single should be used, But I am not sure how to pass
> the first "struct device *".
>           We have a device which call arch_setup_dma_ops to create the
> iommu domain, if we use this device, it will enter __iommu_map_page;
>          If we input NULL for dmap_map_single, it will assert at
> dma_capable in swiotlb_map_page...

Consider that the IOMMU's page table walker is a DMA master in its own 
right, and that is the device you're mapping the page tables for. 
Therefore your IOMMU driver needs to have access to the struct device of 
the IOMMU itself to use for the page-table-related mappings. Also, be 
sure to set the IOMMU's DMA mask correctly to prevent SWIOTLB bounce 
buffers being created in the process (which as I've found generally ends 
in disaster).

>          And normally, we always need do cache maintenance only for some
> bytes in the pagetable but not whole a page. Then is there a easy way to
> do the cache maintenance?

For a noncoherent device, dma_map_single() will end up calling 
__dma_map_area() with the page offset and size of the original request, 
so the updated part gets flushed by VA, and the rest of the page isn't 
touched if it doesn't need to be. On the other hand if the page tables 
were allocated with dma_alloc_coherent() in the first place, then just 
calling dma_sync_single_for_device() for the updated region should suffice.

Robin.

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Laura Abbott]

On 3/3/2015 4:15 AM, Robin Murphy wrote:
> On 03/03/15 03:38, Yong Wu wrote:
>> On Mon, 2015-02-16 at 20:04 +0000, Robin Murphy wrote:
>>
>>>>        2)int (*domain_init)(struct iommu_domain *domain);
>>>>          About this function, it will alloc pagetable for the iommu
>>>> domain. And We expect the pagetable memory is uncacheable, so try to
>>>> call "dma_alloc_coherent", unfortunately the "struct device *" can't be
>>>> passed into this function. so is it possible if adding a parameter in
>>>> this function.
>>>
>>> ...one of the ideas of the new of_iommu_configure framework is that
>>> iommu_ops structures can represent individual IOMMU devices if
>>> necessary. The ops->priv pointer was included for that purpose, but
>>> isn't very clean so Will has plans to remove it again - it's easy enough
>>> to achieve the same effect by having the driver embed the ops in its
>>> private instance data instead. I've done that with the ARM SMMU driver
>>> which has a similar issue of needing hardware details at domain_init()
>>> time (no patches ready yet but I have an iommu/dev branch on top of the
>>> iommu/dma branch with some current work-in-progress bits)
>>>
>>> Thanks,
>>> Robin.
>>>
>> Dear Robin,
>>          Thanks very much for your suggestion.
>>
>>          I have a more question: how to do cache maintenance in arm64.
>>
>>          a) "__dma_flush_range" can not be expected to used directly.
>>          b) Following Documentation\DMA-API.txt, section: Streaming DMA
>> mappings. dma_map_single should be used, But I am not sure how to pass
>> the first "struct device *".
>>           We have a device which call arch_setup_dma_ops to create the
>> iommu domain, if we use this device, it will enter __iommu_map_page;
>>          If we input NULL for dmap_map_single, it will assert at
>> dma_capable in swiotlb_map_page...
>
> Consider that the IOMMU's page table walker is a DMA master in its own
right, and that is the device you're mapping the page tables for.
Therefore your IOMMU driver needs to have access to the struct device
of the IOMMU itself to use for the page-table-related mappings. Also,
be sure to set the IOMMU's DMA mask correctly to prevent SWIOTLB bounce
buffers being created in the process (which as I've found generally ends
in disaster).
>
>>          And normally, we always need do cache maintenance only for some
>> bytes in the pagetable but not whole a page. Then is there a easy way to
>> do the cache maintenance?
>
> For a noncoherent device, dma_map_single() will end up calling
__dma_map_area() with the page offset and size of the original request, so
the updated part gets flushed by VA, and the rest of the page isn't touched
if it doesn't need to be. On the other hand if the page tables were
allocated with dma_alloc_coherent() in the first place, then just calling
dma_sync_single_for_device() for the updated region should suffice.
>

Where exactly would you call the dma_unmap? It seems a bit strange to
be repeatedly calling dma_map and never calling dma_unmap. I don't see it
explicitly forbidden in the docs anywhere to do this but it seems like
it would be violating the implicit handoff of dma_map/dma_unmap.

> Robin.
>

Thanks,
Laura

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Robin Murphy]

Hi Laura,

On 05/03/15 00:19, Laura Abbott wrote:
[...]
>> Consider that the IOMMU's page table walker is a DMA master in its own
> right, and that is the device you're mapping the page tables for.
> Therefore your IOMMU driver needs to have access to the struct device
> of the IOMMU itself to use for the page-table-related mappings. Also,
> be sure to set the IOMMU's DMA mask correctly to prevent SWIOTLB bounce
> buffers being created in the process (which as I've found generally ends
> in disaster).
>>
>>>           And normally, we always need do cache maintenance only for some
>>> bytes in the pagetable but not whole a page. Then is there a easy way to
>>> do the cache maintenance?
>>
>> For a noncoherent device, dma_map_single() will end up calling
> __dma_map_area() with the page offset and size of the original request, so
> the updated part gets flushed by VA, and the rest of the page isn't touched
> if it doesn't need to be. On the other hand if the page tables were
> allocated with dma_alloc_coherent() in the first place, then just calling
> dma_sync_single_for_device() for the updated region should suffice.
>>
>
> Where exactly would you call the dma_unmap? It seems a bit strange to
> be repeatedly calling dma_map and never calling dma_unmap. I don't see it
> explicitly forbidden in the docs anywhere to do this but it seems like
> it would be violating the implicit handoff of dma_map/dma_unmap.

I think ideally you'd call dma_map_page when you first create the page 
table, dma_sync_single_for_device on any update, and dma_unmap_page when 
you tear it down, and you'd also use the appropriate DMA addresses 
everywhere instead of physical addresses.

I wouldn't compare that with what we do in the ARM SMMU driver, because 
that's a lot more hacky; there we're actually _relying_ on the mapping 
aspect of dma_map_page being a no-op so we just get the implicit sync 
part of it, thus we know an unmap would do absolutely nothing (since the 
SMMU doesn't write to the page tables we've no need to sync them back to 
the CPU), so we can get away with skipping it. Of course, as both Mitch 
and I have apparently discovered recently, things end up going wrong 
when that no-op assumption isn't true and bounce buffers happen...

Robin.

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Russell King - ARM Linux]

On Thu, Mar 05, 2015 at 11:16:28AM +0000, Robin Murphy wrote:
> Hi Laura,
> 
> On 05/03/15 00:19, Laura Abbott wrote:
> [...]
> >>Consider that the IOMMU's page table walker is a DMA master in its own
> >right, and that is the device you're mapping the page tables for.
> >Therefore your IOMMU driver needs to have access to the struct device
> >of the IOMMU itself to use for the page-table-related mappings. Also,
> >be sure to set the IOMMU's DMA mask correctly to prevent SWIOTLB bounce
> >buffers being created in the process (which as I've found generally ends
> >in disaster).
> >>
> >>>          And normally, we always need do cache maintenance only for some
> >>>bytes in the pagetable but not whole a page. Then is there a easy way to
> >>>do the cache maintenance?
> >>
> >>For a noncoherent device, dma_map_single() will end up calling
> >__dma_map_area() with the page offset and size of the original request, so
> >the updated part gets flushed by VA, and the rest of the page isn't touched
> >if it doesn't need to be. On the other hand if the page tables were
> >allocated with dma_alloc_coherent() in the first place, then just calling
> >dma_sync_single_for_device() for the updated region should suffice.

That's wrong.  dma_sync_single_*() is not permitted to be called on
coherently allocated memory.  Where coherent memory needs to be remapped,
dma_sync_single_*() will panic the kernel.

If it's in coherent memory, all you should need is the appropriate
memory barrier to ensure that the DMA agent can see the writes.

> >Where exactly would you call the dma_unmap? It seems a bit strange to
> >be repeatedly calling dma_map and never calling dma_unmap. I don't see it
> >explicitly forbidden in the docs anywhere to do this but it seems like
> >it would be violating the implicit handoff of dma_map/dma_unmap.
> 
> I think ideally you'd call dma_map_page when you first create the page
> table, dma_sync_single_for_device on any update, and dma_unmap_page when you
> tear it down, and you'd also use the appropriate DMA addresses everywhere
> instead of physical addresses.

No.

dma_map_page()			ownership changes CPU->DMA
dma_sync_single_for_cpu()	ownership changes DMA->CPU
dma_sync_single_for_device()	ownership changes CPU->DMA
dma_unmap_page()		ownership changes DMA->CPU

It's invalid to miss out the pairing that give those ownership changes.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Robin Murphy]

Hi Russell,

On 09/03/15 17:59, Russell King - ARM Linux wrote:
[...]
>>>> For a noncoherent device, dma_map_single() will end up calling
>>> __dma_map_area() with the page offset and size of the original request, so
>>> the updated part gets flushed by VA, and the rest of the page isn't touched
>>> if it doesn't need to be. On the other hand if the page tables were
>>> allocated with dma_alloc_coherent() in the first place, then just calling
>>> dma_sync_single_for_device() for the updated region should suffice.
>
> That's wrong.  dma_sync_single_*() is not permitted to be called on
> coherently allocated memory.  Where coherent memory needs to be remapped,
> dma_sync_single_*() will panic the kernel.
>
> If it's in coherent memory, all you should need is the appropriate
> memory barrier to ensure that the DMA agent can see the writes.

You're quite right, that's the whole point of *coherent* allocations 
after all. I got my syncs and barriers muddled there.

>>> Where exactly would you call the dma_unmap? It seems a bit strange to
>>> be repeatedly calling dma_map and never calling dma_unmap. I don't see it
>>> explicitly forbidden in the docs anywhere to do this but it seems like
>>> it would be violating the implicit handoff of dma_map/dma_unmap.
>>
>> I think ideally you'd call dma_map_page when you first create the page
>> table, dma_sync_single_for_device on any update, and dma_unmap_page when you
>> tear it down, and you'd also use the appropriate DMA addresses everywhere
>> instead of physical addresses.
>
> No.
>
> dma_map_page()			ownership changes CPU->DMA
> dma_sync_single_for_cpu()	ownership changes DMA->CPU
> dma_sync_single_for_device()	ownership changes CPU->DMA
> dma_unmap_page()		ownership changes DMA->CPU
>
> It's invalid to miss out the pairing that give those ownership changes.

Thanks for the clarification - the wording in DMA-API.txt rather implies 
that in the DMA_TO_DEVICE case you only have to sync the updated data 
/after/ writing it. For the sake of purely getting pages flushed, would 
it be more reasonable then to call dma_map_single() followed immediately 
by dma_unmap_single_attrs() with DMA_ATTR_SKIP_CPU_SYNC? Since we know 
the IOMMU can never write back to memory  (ones that can are a different 
issue) it would be nice to be able to skip the extra invalidations 
somehow, without too heinously abusing the API.

Robin.

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Robin Murphy]

On 09/03/15 20:09, Robin Murphy wrote:
[...]
>>> I think ideally you'd call dma_map_page when you first create the page
>>> table, dma_sync_single_for_device on any update, and dma_unmap_page when you
>>> tear it down, and you'd also use the appropriate DMA addresses everywhere
>>> instead of physical addresses.
>>
>> No.
>>
>> dma_map_page()			ownership changes CPU->DMA
>> dma_sync_single_for_cpu()	ownership changes DMA->CPU
>> dma_sync_single_for_device()	ownership changes CPU->DMA
>> dma_unmap_page()		ownership changes DMA->CPU
>>
>> It's invalid to miss out the pairing that give those ownership changes.
>
> Thanks for the clarification - the wording in DMA-API.txt rather implies
> that in the DMA_TO_DEVICE case you only have to sync the updated data
> /after/ writing it. For the sake of purely getting pages flushed, would
> it be more reasonable then to call dma_map_single() followed immediately
> by dma_unmap_single_attrs() with DMA_ATTR_SKIP_CPU_SYNC? Since we know
> the IOMMU can never write back to memory  (ones that can are a different
> issue) it would be nice to be able to skip the extra invalidations
> somehow, without too heinously abusing the API.

Scratch that, I'm being a massive idiot (again). Of course the actual 
invalidations will only happen if they need to, based on the DMA 
direction. The overhead of dma_sync_*_for_cpu() and dma_unmap() is then 
only a handful of function calls, which is probably an acceptable price 
to pay for making sure things work as correctly as possible.

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

[RFC PATCH v2 2/3] arm64: add IOMMU dma_ops

[Marek Szyprowski]

Hello Robin,

On 2015-02-06 15:55, Robin Murphy wrote:
> Taking some inspiration from the arch/arm code, implement the
> arch-specific side of the DMA mapping ops using the new IOMMU-DMA layer.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>   arch/arm64/include/asm/device.h      |   3 +
>   arch/arm64/include/asm/dma-mapping.h |  17 ++
>   arch/arm64/mm/dma-mapping.c          | 320 +++++++++++++++++++++++++++++++++++
>   3 files changed, 340 insertions(+)
>
> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
> index 243ef25..510cee1 100644
> --- a/arch/arm64/include/asm/device.h
> +++ b/arch/arm64/include/asm/device.h
> @@ -20,6 +20,9 @@ struct dev_archdata {
>   	struct dma_map_ops *dma_ops;
>   #ifdef CONFIG_IOMMU_API
>   	void *iommu;			/* private IOMMU data */
> +#ifdef CONFIG_IOMMU_DMA
> +	struct iommu_dma_domain *dma_domain;
> +#endif
>   #endif
>   	bool dma_coherent;
>   };
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index 6932bb5..c1b271f 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -62,13 +62,30 @@ static inline bool is_device_dma_coherent(struct device *dev)
>   
>   #include <asm-generic/dma-mapping-common.h>
>   
> +#ifdef CONFIG_IOMMU_DMA
> +static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
> +{
> +	return dev->archdata.dma_domain;
> +}
> +
> +static inline void set_dma_domain(struct device *dev,
> +				  struct iommu_dma_domain *dma_domain)
> +{
> +	dev->archdata.dma_domain = dma_domain;
> +}
> +#endif
> +
>   static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
>   {
> +	if (WARN_ON(dev && get_dma_domain(dev)))
> +		return DMA_ERROR_CODE;
>   	return (dma_addr_t)paddr;
>   }
>   
>   static inline phys_addr_t dma_to_phys(struct device *dev, dma_addr_t dev_addr)
>   {
> +	if (WARN_ON(dev && get_dma_domain(dev)))
> +		return 0;
>   	return (phys_addr_t)dev_addr;
>   }
>   
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 0a24b9b..28e771c 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -23,6 +23,7 @@
>   #include <linux/genalloc.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/dma-contiguous.h>
> +#include <linux/dma-iommu.h>
>   #include <linux/vmalloc.h>
>   #include <linux/swiotlb.h>
>   
> @@ -426,6 +427,7 @@ static int __init arm64_dma_init(void)
>   
>   	ret |= swiotlb_late_init();
>   	ret |= atomic_pool_init();
> +	ret |= iommu_dma_init();
>   
>   	return ret;
>   }
> @@ -439,3 +441,321 @@ static int __init dma_debug_do_init(void)
>   	return 0;
>   }
>   fs_initcall(dma_debug_do_init);
> +
> +
> +#ifdef CONFIG_IOMMU_DMA
> +
> +static struct page **__atomic_get_pages(void *addr)
> +{
> +	struct page *page;
> +	phys_addr_t phys;
> +
> +	phys = gen_pool_virt_to_phys(atomic_pool, (unsigned long)addr);
> +	page = phys_to_page(phys);
> +
> +	return (struct page **)page;
> +}
> +
> +static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
> +{
> +	struct vm_struct *area;
> +
> +	if (__in_atomic_pool(cpu_addr, PAGE_SIZE))
> +		return __atomic_get_pages(cpu_addr);
> +
> +	area = find_vm_area(cpu_addr);
> +	if (!area)
> +		return NULL;
> +
> +	return area->pages;
> +}
> +
> +static void *__iommu_alloc_atomic(struct device *dev, size_t size,
> +				  dma_addr_t *handle, bool coherent)
> +{
> +	struct page *page;
> +	void *addr;
> +
> +	addr = __alloc_from_pool(size, &page);
> +	if (!addr)
> +		return NULL;
> +
> +	*handle = iommu_dma_create_iova_mapping(dev, &page, size, coherent);
> +	if (*handle == DMA_ERROR_CODE) {
> +		__free_from_pool(addr, size);
> +		return NULL;
> +	}
> +	return addr;
> +}
> +
> +static void __iommu_free_atomic(struct device *dev, void *cpu_addr,
> +				dma_addr_t handle, size_t size)
> +{
> +	iommu_dma_release_iova_mapping(dev, handle, size);
> +	__free_from_pool(cpu_addr, size);
> +}
> +
> +static void __dma_clear_buffer(struct page *page, size_t size)
> +{
> +	void *ptr = page_address(page);
> +
> +	memset(ptr, 0, size);
> +	__dma_flush_range(ptr, ptr + size);
> +}
> +
> +static void *__iommu_alloc_attrs(struct device *dev, size_t size,
> +				 dma_addr_t *handle, gfp_t gfp,
> +				 struct dma_attrs *attrs)
> +{
> +	bool coherent = is_device_dma_coherent(dev);
> +	pgprot_t prot = coherent ? __pgprot(PROT_NORMAL) :
> +				   __pgprot(PROT_NORMAL_NC);
> +	struct page **pages;
> +	void *addr = NULL;
> +
> +	*handle = DMA_ERROR_CODE;
> +	size = PAGE_ALIGN(size);
> +
> +	if (!(gfp & __GFP_WAIT))
> +		return __iommu_alloc_atomic(dev, size, handle, coherent);
> +	/*
> +	 * FIXME: This isn't even true any more!
> +	 *
> +	 * Following is a work-around (a.k.a. hack) to prevent pages
> +	 * with __GFP_COMP being passed to split_page() which cannot
> +	 * handle them.  The real problem is that this flag probably
> +	 * should be 0 on ARM as it is not supported on this
> +	 * platform; see CONFIG_HUGETLBFS.
> +	 */
> +	gfp &= ~(__GFP_COMP);
> +
> +	pages = iommu_dma_alloc_buffer(dev, size, gfp, attrs,
> +			__dma_clear_buffer);
> +	if (!pages)
> +		return NULL;
> +
> +	*handle = iommu_dma_create_iova_mapping(dev, pages, size, coherent);
> +	if (*handle == DMA_ERROR_CODE)
> +		goto err_mapping;
> +
> +	addr = dma_common_pages_remap(pages, size, VM_USERMAP,
> +				      __get_dma_pgprot(attrs, prot, coherent),
> +				      __builtin_return_address(0));
> +	if (addr)
> +		return addr;
> +
> +	iommu_dma_release_iova_mapping(dev, *handle, size);
> +err_mapping:
> +	iommu_dma_free_buffer(dev, pages, size, attrs);
> +	return NULL;
> +}
> +
> +static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
> +			       dma_addr_t handle, struct dma_attrs *attrs)
> +{
> +	struct page **pages;
> +
> +	size = PAGE_ALIGN(size);
> +	if (__in_atomic_pool(cpu_addr, size)) {
> +		__iommu_free_atomic(dev, cpu_addr, handle, size);
> +		return;
> +	}
> +
> +	pages = __iommu_get_pages(cpu_addr, attrs);
> +	if (!pages) {
> +		WARN(1, "trying to free invalid coherent area: %p\n", cpu_addr);
> +		return;
> +	}
> +
> +	dma_common_free_remap(cpu_addr, size, VM_USERMAP);
> +
> +	iommu_dma_release_iova_mapping(dev, handle, size);
> +	iommu_dma_free_buffer(dev, pages, size, attrs);
> +}
> +
> +static int __iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
> +			      void *cpu_addr, dma_addr_t dma_addr, size_t size,
> +			      struct dma_attrs *attrs)
> +{
> +	unsigned long uaddr = vma->vm_start;
> +	unsigned long usize = vma->vm_end - vma->vm_start;
> +	struct page **pages = __iommu_get_pages(cpu_addr, attrs);
> +	int ret;
> +
> +	if (!pages)
> +		return -ENXIO;
> +
> +	vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot,
> +					     is_device_dma_coherent(dev));
> +
> +	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
> +		return ret;
> +
> +	do {
> +		ret = vm_insert_page(vma, uaddr, *pages++);
> +		if (ret) {
> +			pr_err("Remapping memory failed: %d\n", ret);
> +			return ret;
> +		}
> +		uaddr += PAGE_SIZE;
> +		usize -= PAGE_SIZE;
> +	} while (usize > 0);
> +
> +	return 0;
> +}
> +
> +static void __iommu_sync_single_for_cpu(struct device *dev,
> +					dma_addr_t dev_addr, size_t size,
> +					enum dma_data_direction dir)
> +{
> +	if (!is_device_dma_coherent(dev) && (dev_addr != DMA_ERROR_CODE)) {
> +		struct iommu_dma_domain *dma_domain = get_dma_domain(dev);
> +		struct iommu_domain *domain = iommu_dma_raw_domain(dma_domain);
> +		phys_addr_t phys = iommu_iova_to_phys(domain, dev_addr);
> +
> +		__dma_unmap_area(phys_to_virt(phys), size, dir);
> +	}
> +}
> +
> +static void __iommu_sync_single_for_device(struct device *dev,
> +					   dma_addr_t dev_addr, size_t size,
> +					   enum dma_data_direction dir)
> +{
> +	if (!is_device_dma_coherent(dev) && (dev_addr != DMA_ERROR_CODE)) {
> +		struct iommu_dma_domain *dma_domain = get_dma_domain(dev);
> +		struct iommu_domain *domain = iommu_dma_raw_domain(dma_domain);
> +		phys_addr_t phys = iommu_iova_to_phys(domain, dev_addr);
> +
> +		__dma_map_area(phys_to_virt(phys), size, dir);
> +	}
> +}
> +
> +static dma_addr_t __iommu_map_page(struct device *dev, struct page *page,
> +				   unsigned long offset, size_t size,
> +				   enum dma_data_direction dir,
> +				   struct dma_attrs *attrs)
> +{
> +	dma_addr_t dev_addr;
> +
> +	if (is_device_dma_coherent(dev))
> +		return iommu_dma_coherent_map_page(dev, page, offset, size,
> +				dir, attrs);
> +
> +	dev_addr = iommu_dma_map_page(dev, page, offset, size, dir, attrs);
> +
> +	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
> +		__iommu_sync_single_for_device(dev, dev_addr, size, dir);
> +
> +	return dev_addr;
> +}
> +
> +static void __iommu_unmap_page(struct device *dev, dma_addr_t dev_addr,
> +			       size_t size, enum dma_data_direction dir,
> +			       struct dma_attrs *attrs)
> +{
> +	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
> +		__iommu_sync_single_for_cpu(dev, dev_addr, size, dir);
> +
> +	iommu_dma_unmap_page(dev, dev_addr, size, dir, attrs);
> +}
> +
> +static void __iommu_sync_sg_for_cpu(struct device *dev,
> +				    struct scatterlist *sgl, int nelems,
> +				    enum dma_data_direction dir)
> +{
> +	struct scatterlist *sg;
> +	int i;
> +
> +	if (is_device_dma_coherent(dev))
> +		return;
> +
> +	for_each_sg(sgl, sg, nelems, i)
> +		__dma_unmap_area(sg_virt(sg), sg->length, dir);
> +}
> +
> +static void __iommu_sync_sg_for_device(struct device *dev,
> +				       struct scatterlist *sgl, int nelems,
> +				       enum dma_data_direction dir)
> +{
> +	struct scatterlist *sg;
> +	int i;
> +
> +	if (is_device_dma_coherent(dev))
> +		return;
> +
> +	for_each_sg(sgl, sg, nelems, i)
> +		__dma_map_area(sg_virt(sg), sg->length, dir);
> +}
> +
> +static int __iommu_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
> +				int nelems, enum dma_data_direction dir,
> +				struct dma_attrs *attrs)
> +{
> +	if (is_device_dma_coherent(dev))
> +		return iommu_dma_coherent_map_sg(dev, sgl, nelems, dir, attrs);
> +
> +	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
> +		__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
> +
> +	return iommu_dma_map_sg(dev, sgl, nelems, dir, attrs);
> +}
> +
> +static void __iommu_unmap_sg_attrs(struct device *dev,
> +				   struct scatterlist *sgl, int nelems,
> +				   enum dma_data_direction dir,
> +				   struct dma_attrs *attrs)
> +{
> +	if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
> +		__iommu_sync_sg_for_cpu(dev, sgl, nelems, dir);
> +
> +	iommu_dma_unmap_sg(dev, sgl, nelems, dir, attrs);
> +}
> +

Could you also add the following function to make the implementation 
complete?

static int __iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
                                void *cpu_addr, dma_addr_t dma_addr,
                                size_t size, struct dma_attrs *attrs)
{
         unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
         struct page **pages = __iommu_get_pages(cpu_addr, attrs);

         if (!pages)
                 return -ENXIO;

         return sg_alloc_table_from_pages(sgt, pages, count, 0, size,
                                          GFP_KERNEL);
}

> +static struct dma_map_ops iommu_dma_ops = {
> +	.alloc = __iommu_alloc_attrs,
> +	.free = __iommu_free_attrs,
> +	.mmap = __iommu_mmap_attrs,
> +	.map_page = __iommu_map_page,
> +	.unmap_page = __iommu_unmap_page,
> +	.map_sg = __iommu_map_sg_attrs,
> +	.unmap_sg = __iommu_unmap_sg_attrs,
> +	.sync_single_for_cpu = __iommu_sync_single_for_cpu,
> +	.sync_single_for_device = __iommu_sync_single_for_device,
> +	.sync_sg_for_cpu = __iommu_sync_sg_for_cpu,
> +	.sync_sg_for_device = __iommu_sync_sg_for_device,
> +	.dma_supported = iommu_dma_supported,
> +	.mapping_error = iommu_dma_mapping_error,
> +};
> +
> +static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> +				  const struct iommu_ops *ops)
> +{
> +	struct iommu_dma_domain *dma_domain;
> +
> +	if (!ops)
> +		return;
> +
> +	dma_domain = iommu_dma_create_domain(ops, dma_base, size);
> +	if (!dma_domain) {
> +		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
> +				size, dev_name(dev));
> +		return;
> +	}
> +
> +	if (iommu_dma_attach_device(dev, dma_domain))
> +		pr_warn("Failed to attach device %s to IOMMU mapping\n",
> +				dev_name(dev));
> +	else
> +		dev->archdata.dma_ops = &iommu_dma_ops;
> +
> +	/* drop the initial mapping refcount */
> +	iommu_dma_release_domain(dma_domain);
> +}
> +
> +#else
> +
> +static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> +				  struct iommu_ops *iommu)
> +{ }
> +
> +#endif  /* CONFIG_IOMMU_DMA */

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFC PATCH v2 3/3] arm64: hook up IOMMU dma_ops

[Robin Murphy]

With iommu_dma_ops in place, hook them up to the configuration code, so
IOMMU-fronted devices will get them automatically.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/dma-mapping.h | 11 ++++++-----
 arch/arm64/mm/dma-mapping.c          | 15 +++++++++++++++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b1f9a20..e2abcdc 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -66,6 +66,7 @@ config ARM64
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_RCU_TABLE_FREE
 	select HAVE_SYSCALL_TRACEPOINTS
+	select IOMMU_DMA if IOMMU_SUPPORT
 	select IRQ_DOMAIN
 	select MODULES_USE_ELF_RELA
 	select NO_BOOTMEM
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index c1b271f..5246d1a 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -45,11 +45,8 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 		return __generic_dma_ops(dev);
 }
 
-static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-				      struct iommu_ops *iommu, bool coherent)
-{
-	dev->archdata.dma_coherent = coherent;
-}
+void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+			struct iommu_ops *iommu, bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
 /* do not use this function in a driver */
@@ -63,6 +60,10 @@ static inline bool is_device_dma_coherent(struct device *dev)
 #include <asm-generic/dma-mapping-common.h>
 
 #ifdef CONFIG_IOMMU_DMA
+
+void arch_teardown_dma_ops(struct device *dev);
+#define arch_teardown_dma_ops	arch_teardown_dma_ops
+
 static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
 {
 	return dev->archdata.dma_domain;
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 28e771c..2a30673 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -752,6 +752,14 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	iommu_dma_release_domain(dma_domain);
 }
 
+void arch_teardown_dma_ops(struct device *dev)
+{
+	if (dev->archdata.dma_domain) {
+		iommu_dma_detach_device(dev);
+		dev->archdata.dma_ops = NULL;
+	}
+}
+
 #else
 
 static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
@@ -759,3 +767,10 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 { }
 
 #endif  /* CONFIG_IOMMU_DMA */
+
+void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+			struct iommu_ops *iommu, bool coherent)
+{
+	dev->archdata.dma_coherent = coherent;
+	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
+}
-- 
1.9.1

[RFC PATCH v2 3/3] arm64: hook up IOMMU dma_ops

[leizhen]

On 2015/2/6 22:55, Robin Murphy wrote:
> With iommu_dma_ops in place, hook them up to the configuration code, so
> IOMMU-fronted devices will get them automatically.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  arch/arm64/Kconfig                   |  1 +
>  arch/arm64/include/asm/dma-mapping.h | 11 ++++++-----
>  arch/arm64/mm/dma-mapping.c          | 15 +++++++++++++++
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b1f9a20..e2abcdc 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -66,6 +66,7 @@ config ARM64
>  	select HAVE_PERF_USER_STACK_DUMP
>  	select HAVE_RCU_TABLE_FREE
>  	select HAVE_SYSCALL_TRACEPOINTS
> +	select IOMMU_DMA if IOMMU_SUPPORT

IOMMU_SUPPORT is selected above, so "if IOMMU_SUPPORT" can be omitted.
And I suggest putting "select IOMMU_DMA" next to "select IOMMU_SUPPORT".

>  	select IRQ_DOMAIN
>  	select MODULES_USE_ELF_RELA
>  	select NO_BOOTMEM
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index c1b271f..5246d1a 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -45,11 +45,8 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
>  		return __generic_dma_ops(dev);
>  }
>  
> -static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> -				      struct iommu_ops *iommu, bool coherent)
> -{
> -	dev->archdata.dma_coherent = coherent;
> -}
> +void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> +			struct iommu_ops *iommu, bool coherent);
>  #define arch_setup_dma_ops	arch_setup_dma_ops
>  
>  /* do not use this function in a driver */
> @@ -63,6 +60,10 @@ static inline bool is_device_dma_coherent(struct device *dev)
>  #include <asm-generic/dma-mapping-common.h>
>  
>  #ifdef CONFIG_IOMMU_DMA
> +
> +void arch_teardown_dma_ops(struct device *dev);
> +#define arch_teardown_dma_ops	arch_teardown_dma_ops
> +
>  static inline struct iommu_dma_domain *get_dma_domain(struct device *dev)
>  {
>  	return dev->archdata.dma_domain;
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index 28e771c..2a30673 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -752,6 +752,14 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  	iommu_dma_release_domain(dma_domain);
>  }
>  
> +void arch_teardown_dma_ops(struct device *dev)
> +{
> +	if (dev->archdata.dma_domain) {
> +		iommu_dma_detach_device(dev);
> +		dev->archdata.dma_ops = NULL;
> +	}
> +}
> +
>  #else
>  
>  static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> @@ -759,3 +767,10 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  { }
>  
>  #endif  /* CONFIG_IOMMU_DMA */
> +
> +void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> +			struct iommu_ops *iommu, bool coherent)
> +{
> +	dev->archdata.dma_coherent = coherent;
> +	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
> +}
> 

[RFC PATCH v2 3/3] arm64: hook up IOMMU dma_ops

[Robin Murphy]

On 03/03/15 11:05, leizhen wrote:
> On 2015/2/6 22:55, Robin Murphy wrote:
>> With iommu_dma_ops in place, hook them up to the configuration code, so
>> IOMMU-fronted devices will get them automatically.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   arch/arm64/Kconfig                   |  1 +
>>   arch/arm64/include/asm/dma-mapping.h | 11 ++++++-----
>>   arch/arm64/mm/dma-mapping.c          | 15 +++++++++++++++
>>   3 files changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index b1f9a20..e2abcdc 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -66,6 +66,7 @@ config ARM64
>>   	select HAVE_PERF_USER_STACK_DUMP
>>   	select HAVE_RCU_TABLE_FREE
>>   	select HAVE_SYSCALL_TRACEPOINTS
>> +	select IOMMU_DMA if IOMMU_SUPPORT
>
> IOMMU_SUPPORT is selected above, so "if IOMMU_SUPPORT" can be omitted.
> And I suggest putting "select IOMMU_DMA" next to "select IOMMU_SUPPORT".

Not in this patch set it isn't ;)

Omitting the "if IOMMU_SUPPORT" at this point results in this for me:

*** Default configuration is based on 'defconfig'
warning: (ARM64) selects IOMMU_DMA which has unmet direct dependencies 
(IOMMU_SUPPORT && NEED_SG_DMA_LENGTH)

Furthermore, if IOMMU_SUPPORT is selected anywhere I think it should be 
in defconfig, rather than here - if the user wants to leave out all the 
IOMMU drivers, then they should be able to turn off IOMMU_SUPPORT as 
well (since it makes no sense on its own), at which point the 
architecture then needs to make sure that IOMMU_DMA is deselected. 
Unless there's a more elegant way to enforce that, I think this 
"select...if..." needs to stay as it is.

Robin.