arm64: IOMMU-backed DMA mapping

drivers/iommu/dma-iommu.c new file +35 -0

+/*
+ * The DMA API client is passing in a scatterlist which could describe
+ * any old buffer layout, but the IOMMU API requires everything to be
+ * aligned to IOMMU pages. Hence the need for this complicated bit of
+ * impedance-matching, to be able to hand off a suitably-aligned list,
+ * but still preserve the original offsets and sizes for the caller.
+ */
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+		int nents, int prot)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	struct iova_domain *iovad = domain->iova_cookie;
+	struct iova *iova;
+	struct scatterlist *s, *prev = NULL;
+	dma_addr_t dma_addr;
+	size_t iova_len = 0;
+	int i;
+
+	/*
+	 * 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-place, but reversibly, by
+	 * hiding the original data in the as-yet-unused DMA fields.
+	 */
+	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;
+
+		/*

drivers/iommu/dma-iommu.c new file +28 -0

+			iova_len += pad_len;
+		}
+
+		iova_len += s_length;
+		prev = s;
+	}
+
+	iova = __alloc_iova(iovad, iova_len, dma_get_mask(dev));
+	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(domain, dma_addr, sg, nents, prot) < iova_len)
+		goto out_free_iova;
+
+	return __finalise_sg(dev, sg, nents, dma_addr);
+
+out_free_iova:
+	__free_iova(iovad, iova);
+out_restore_sg:
+	__invalidate_sg(sg, nents);
+	return 0;
+}
+

On Thu, 2015-10-01 at 20:13 +0100, Robin Murphy wrote:
[...]

Quoted 35 lines

drivers/iommu/dma-iommu.c new file +35 -0

+/*
+ * The DMA API client is passing in a scatterlist which could describe
+ * any old buffer layout, but the IOMMU API requires everything to be
+ * aligned to IOMMU pages. Hence the need for this complicated bit of
+ * impedance-matching, to be able to hand off a suitably-aligned list,
+ * but still preserve the original offsets and sizes for the caller.
+ */
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+		int nents, int prot)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+	struct iova_domain *iovad = domain->iova_cookie;
+	struct iova *iova;
+	struct scatterlist *s, *prev = NULL;
+	dma_addr_t dma_addr;
+	size_t iova_len = 0;
+	int i;
+
+	/*
+	 * 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-place, but reversibly, by
+	 * hiding the original data in the as-yet-unused DMA fields.
+	 */
+	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;
+
+		/*

drivers/iommu/dma-iommu.c line 472

Quoted 10 lines

drivers/iommu/dma-iommu.c new file +10 -0

+		 * The simple way to avoid the rare case of a segment
+		 * crossing the boundary mask is to pad the previous one
+		 * to end at a naturally-aligned IOVA for this one's size,
+		 * at the cost of potentially over-allocating a little.
+		 */
+		if (prev) {
+			size_t pad_len = roundup_pow_of_two(s_length);
+
+			pad_len = (pad_len - iova_len) & (pad_len - 1);
+			prev->length += pad_len;

Hi Robin,
While our v4l2 testing, It seems that we met a problem here. Here we update prev->length again, Do we need update
sg_dma_len(prev) again too?

Some function like vb2_dc_get_contiguous_size[1] always get
sg_dma_len(s) to compare instead of s->length. so it may break unexpectedly while sg_dma_len(s) is not same with s->length.

[1]:
http://lxr.free-electrons.com/source/drivers/media/v4l2-core/videobuf2-dma-contig.c#L70

Quoted 58 lines · Yong Wu

On Thu, 2015-10-01 at 20:13 +0100, Robin Murphy wrote:
[...]

Quoted 41 lines

drivers/iommu/dma-iommu.c new file +20 -0

+/*
+ * The DMA API client is passing in a scatterlist which could describe
+ * any old buffer layout, but the IOMMU API requires everything to be
+ * aligned to IOMMU pages. Hence the need for this complicated bit of
+ * impedance-matching, to be able to hand off a suitably-aligned list,
+ * but still preserve the original offsets and sizes for the caller.
+ */
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+               int nents, int prot)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+       struct iova_domain *iovad = domain->iova_cookie;
+       struct iova *iova;
+       struct scatterlist *s, *prev = NULL;
+       dma_addr_t dma_addr;
+       size_t iova_len = 0;
+       int i;
+
+       /*
+        * Work out how much IOVA space we need, and align the segments

to

drivers/iommu/dma-iommu.c new file +18 -0

+        * IOVA granules for the IOMMU driver to handle. With some clever
+        * trickery we can modify the list in-place, but reversibly, by
+        * hiding the original data in the as-yet-unused DMA fields.
+        */
+       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;
+
+               /*
+                * The simple way to avoid the rare case of a segment
+                * crossing the boundary mask is to pad the previous one
+                * to end at a naturally-aligned IOVA for this one's

size,

drivers/iommu/dma-iommu.c line 472

Quoted 7 lines

drivers/iommu/dma-iommu.c new file +7 -0

+                * at the cost of potentially over-allocating a little.
+                */
+               if (prev) {
+                       size_t pad_len = roundup_pow_of_two(s_length);
+
+                       pad_len = (pad_len - iova_len) & (pad_len - 1);
+                       prev->length += pad_len;

Hi Robin,
While our v4l2 testing, It seems that we met a problem here. Here we update prev->length again, Do we need update
sg_dma_len(prev) again too?

Quoted 3 lines

Some function like vb2_dc_get_contiguous_size[1] always get
sg_dma_len(s) to compare instead of s->length. so it may break unexpectedly while sg_dma_len(s) is not same with s->length.

This is just tweaking the faked-up length that we hand off to iommu_map_sg() (see also the iova_align() above), to trick it into bumping this segment up to a suitable starting IOVA. The real length at this point is stashed in sg_dma_len(s), and will be copied back into s->length in __finalise_sg(), so both will hold the same true length once we return to the caller.

Yes, it does mean that if you have a list where the segment lengths are page aligned but not monotonically decreasing, e.g. {64k, 16k, 64k}, then you'll still end up with a gap between the second and third segments, but that's fine because the DMA API offers no guarantees about what the resulting DMA addresses will be (consider the no-IOMMU case where they would each just be "mapped" to their physical address). If that breaks v4l, then it's probably v4l's DMA API use that needs looking at (again).

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.

So, is the videobuf2-dma-contig.c based on an incorrect assumption about how the DMA API is supposed to work?

Quoted 4 lines · Daniel Kurtz

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.

If you are speaking of the dma_map_sg interface, than there is absolutly no guarantee from the API side that the buffers you pass in will end up mapped contiguously.
IOMMU drivers handle this differently, and when there is no IOMMU at all there is also no way to map these buffers together.

+linux-media & VIDEOBUF2 FRAMEWORK maintainers since this is about the v4l2-contig's usage of the DMA API.

Hi Robin,

Quoted 70 lines · Robin Murphy

Quoted 58 lines · Yong Wu

On Thu, 2015-10-01 at 20:13 +0100, Robin Murphy wrote:
[...]

Quoted 41 lines

drivers/iommu/dma-iommu.c new file +20 -0

+/*
+ * The DMA API client is passing in a scatterlist which could describe
+ * any old buffer layout, but the IOMMU API requires everything to be
+ * aligned to IOMMU pages. Hence the need for this complicated bit of
+ * impedance-matching, to be able to hand off a suitably-aligned list,
+ * but still preserve the original offsets and sizes for the caller.
+ */
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+               int nents, int prot)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+       struct iova_domain *iovad = domain->iova_cookie;
+       struct iova *iova;
+       struct scatterlist *s, *prev = NULL;
+       dma_addr_t dma_addr;
+       size_t iova_len = 0;
+       int i;
+
+       /*
+        * Work out how much IOVA space we need, and align the segments

to

drivers/iommu/dma-iommu.c new file +18 -0

+        * IOVA granules for the IOMMU driver to handle. With some clever
+        * trickery we can modify the list in-place, but reversibly, by
+        * hiding the original data in the as-yet-unused DMA fields.
+        */
+       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;
+
+               /*
+                * The simple way to avoid the rare case of a segment
+                * crossing the boundary mask is to pad the previous one
+                * to end at a naturally-aligned IOVA for this one's

size,

drivers/iommu/dma-iommu.c line 472

Quoted 7 lines

drivers/iommu/dma-iommu.c new file +7 -0

+                * at the cost of potentially over-allocating a little.
+                */
+               if (prev) {
+                       size_t pad_len = roundup_pow_of_two(s_length);
+
+                       pad_len = (pad_len - iova_len) & (pad_len - 1);
+                       prev->length += pad_len;

Hi Robin,
While our v4l2 testing, It seems that we met a problem here. Here we update prev->length again, Do we need update
sg_dma_len(prev) again too?

Quoted 3 lines

Some function like vb2_dc_get_contiguous_size[1] always get
sg_dma_len(s) to compare instead of s->length. so it may break unexpectedly while sg_dma_len(s) is not same with s->length.

This is just tweaking the faked-up length that we hand off to iommu_map_sg() (see also the iova_align() above), to trick it into bumping this segment up to a suitable starting IOVA. The real length at this point is stashed in sg_dma_len(s), and will be copied back into s->length in __finalise_sg(), so both will hold the same true length once we return to the caller.

Quoted 7 lines

Yes, it does mean that if you have a list where the segment lengths are page aligned but not monotonically decreasing, e.g. {64k, 16k, 64k}, then you'll still end up with a gap between the second and third segments, but that's fine because the DMA API offers no guarantees about what the resulting DMA addresses will be (consider the no-IOMMU case where they would each just be "mapped" to their physical address). If that breaks v4l, then it's probably v4l's DMA API use that needs looking at (again).

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.

IIUC, the change above breaks this model by inserting gaps in how the buffer is mapped to device memory, such that the buffer is no longer contiguous in dma address space.

Here is the code in question from
drivers/media/v4l2-core/videobuf2-dma-contig.c :

static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)

{
         struct scatterlist *s;
         dma_addr_t expected = sg_dma_address(sgt->sgl);
         unsigned int i;
         unsigned long size = 0;

         for_each_sg(sgt->sgl, s, sgt->nents, i) {
                 if (sg_dma_address(s) != expected)
                         break;
                 expected = sg_dma_address(s) + sg_dma_len(s);
                 size += sg_dma_len(s);
         }
         return size;
}

static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
unsigned long size, enum dma_data_direction dma_dir)

{
         struct vb2_dc_conf *conf = alloc_ctx;
         struct vb2_dc_buf *buf;
         struct frame_vector *vec;
         unsigned long offset;
         int n_pages, i;
         int ret = 0;
         struct sg_table *sgt;
         unsigned long contig_size;
         unsigned long dma_align = dma_get_cache_alignment();
         DEFINE_DMA_ATTRS(attrs);

         dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);

         buf = kzalloc(sizeof *buf, GFP_KERNEL);
         buf->dma_dir = dma_dir;

         offset = vaddr & ~PAGE_MASK;
         vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
         buf->vec = vec;
         n_pages = frame_vector_count(vec);

         sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);

         ret = sg_alloc_table_from_pages(sgt, frame_vector_pages(vec), n_pages,
                 offset, size, GFP_KERNEL);

         sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
                                       buf->dma_dir, &attrs);

         contig_size = vb2_dc_get_contiguous_size(sgt);

         if (contig_size < size) {

     <<<===   if the original buffer had sg entries that were not

aligned on the "natural" alignment for their size, the new arm64 iommu core code inserts a 'gap' in the iommu mapping, which causes vb2_dc_get_contiguous_size() to exit early (and return a smaller size than expected).

                 pr_err("contiguous mapping is too small %lu/%lu\n",
                         contig_size, size);
                 ret = -EFAULT;
                 goto fail_map_sg;
         }

So, is the videobuf2-dma-contig.c based on an incorrect assumption about how the DMA API is supposed to work?
Is it even possible to map a "contiguous-in-iova-range" mapping for a buffer given as an sg_table with an arbitrary set of pages?

Thanks for helping to move this forward.

-Dan

Hi Dan,

Quoted 91 lines · Daniel Kurtz

+linux-media & VIDEOBUF2 FRAMEWORK maintainers since this is about the v4l2-contig's usage of the DMA API.

Hi Robin,

Quoted 75 lines · Robin Murphy

Quoted 61 lines · Yong Wu

On Thu, 2015-10-01 at 20:13 +0100, Robin Murphy wrote:
[...]

Quoted 42 lines

drivers/iommu/dma-iommu.c new file +20 -0

+/*
+ * The DMA API client is passing in a scatterlist which could describe
+ * any old buffer layout, but the IOMMU API requires everything to be
+ * aligned to IOMMU pages. Hence the need for this complicated bit of
+ * impedance-matching, to be able to hand off a suitably-aligned list,
+ * but still preserve the original offsets and sizes for the caller.
+ */
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+               int nents, int prot)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+       struct iova_domain *iovad = domain->iova_cookie;
+       struct iova *iova;
+       struct scatterlist *s, *prev = NULL;
+       dma_addr_t dma_addr;
+       size_t iova_len = 0;
+       int i;
+
+       /*
+        * Work out how much IOVA space we need, and align the segments

to

drivers/iommu/dma-iommu.c new file +18 -0

+        * IOVA granules for the IOMMU driver to handle. With some clever
+        * trickery we can modify the list in-place, but reversibly, by
+        * hiding the original data in the as-yet-unused DMA fields.
+        */
+       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;
+
+               /*
+                * The simple way to avoid the rare case of a segment
+                * crossing the boundary mask is to pad the previous one
+                * to end at a naturally-aligned IOVA for this one's

size,

drivers/iommu/dma-iommu.c line 472

Quoted 7 lines

drivers/iommu/dma-iommu.c new file +7 -0

+                * at the cost of potentially over-allocating a little.
+                */
+               if (prev) {
+                       size_t pad_len = roundup_pow_of_two(s_length);
+
+                       pad_len = (pad_len - iova_len) & (pad_len - 1);
+                       prev->length += pad_len;

Hi Robin,
While our v4l2 testing, It seems that we met a problem here. Here we update prev->length again, Do we need update
sg_dma_len(prev) again too?

Quoted 3 lines

Some function like vb2_dc_get_contiguous_size[1] always get
sg_dma_len(s) to compare instead of s->length. so it may break unexpectedly while sg_dma_len(s) is not same with s->length.

This is just tweaking the faked-up length that we hand off to iommu_map_sg() (see also the iova_align() above), to trick it into bumping this segment up to a suitable starting IOVA. The real length at this point is stashed in sg_dma_len(s), and will be copied back into s->length in __finalise_sg(), so both will hold the same true length once we return to the caller.

Quoted 7 lines

Yes, it does mean that if you have a list where the segment lengths are page aligned but not monotonically decreasing, e.g. {64k, 16k, 64k}, then you'll still end up with a gap between the second and third segments, but that's fine because the DMA API offers no guarantees about what the resulting DMA addresses will be (consider the no-IOMMU case where they would each just be "mapped" to their physical address). If that breaks v4l, then it's probably v4l's DMA API use that needs looking at (again).

Quoted 4 lines

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.

Not at all. The streaming DMA API (dma_map_* and friends) has two responsibilities: performing any necessary cache maintenance to ensure the device will correctly see data from the CPU, and the CPU will correctly see data from the device; and working out an address for that buffer from the device's point of view to actually hand off to the hardware (which is perfectly well allowed to fail).

Consider SWIOTLB's implementation - segments which already lie at physical addresses within the device's DMA mask just get passed through, while those that lie outside it get mapped into the bounce buffer, but still as individual allocations (arch code just handles cache maintenance on the resulting physical addresses and can apply any hard-wired DMA offset for the device concerned).

Quoted 3 lines

IIUC, the change above breaks this model by inserting gaps in how the buffer is mapped to device memory, such that the buffer is no longer contiguous in dma address space.

Even the existing arch/arm IOMMU DMA code which I guess this implicitly relies on doesn't guarantee that behaviour - if the mapping happens to reach one of the segment length/boundary limits it won't just leave a gap, it'll start an entirely new IOVA allocation which could well start at a wildly different address[0].

Quoted 46 lines

Here is the code in question from
drivers/media/v4l2-core/videobuf2-dma-contig.c :

static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)

{
         struct scatterlist *s;
         dma_addr_t expected = sg_dma_address(sgt->sgl);
         unsigned int i;
         unsigned long size = 0;

         for_each_sg(sgt->sgl, s, sgt->nents, i) {
                 if (sg_dma_address(s) != expected)
                         break;
                 expected = sg_dma_address(s) + sg_dma_len(s);
                 size += sg_dma_len(s);
         }
         return size;
}

static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
unsigned long size, enum dma_data_direction dma_dir)

{
         struct vb2_dc_conf *conf = alloc_ctx;
         struct vb2_dc_buf *buf;
         struct frame_vector *vec;
         unsigned long offset;
         int n_pages, i;
         int ret = 0;
         struct sg_table *sgt;
         unsigned long contig_size;
         unsigned long dma_align = dma_get_cache_alignment();
         DEFINE_DMA_ATTRS(attrs);

         dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);

         buf = kzalloc(sizeof *buf, GFP_KERNEL);
         buf->dma_dir = dma_dir;

         offset = vaddr & ~PAGE_MASK;
         vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
         buf->vec = vec;
         n_pages = frame_vector_count(vec);

         sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);

Quoted 7 lines

         ret = sg_alloc_table_from_pages(sgt, frame_vector_pages(vec), n_pages,
                 offset, size, GFP_KERNEL);

         sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
                                       buf->dma_dir, &attrs);

         contig_size = vb2_dc_get_contiguous_size(sgt);

(as an aside, it's rather unintuitive that the handling of the dma_map_sg call actually failing is entirely implicit here)

Quoted 15 lines

         if (contig_size < size) {

     <<<===   if the original buffer had sg entries that were not

aligned on the "natural" alignment for their size, the new arm64 iommu core code inserts a 'gap' in the iommu mapping, which causes vb2_dc_get_contiguous_size() to exit early (and return a smaller size than expected).

                 pr_err("contiguous mapping is too small %lu/%lu\n",
                         contig_size, size);
                 ret = -EFAULT;
                 goto fail_map_sg;
         }

Quoted 4 lines

So, is the videobuf2-dma-contig.c based on an incorrect assumption about how the DMA API is supposed to work?
Is it even possible to map a "contiguous-in-iova-range" mapping for a buffer given as an sg_table with an arbitrary set of pages?

From the Streaming DMA mappings section of Documentation/DMA-API.txt:

Note also that the above constraints on physical contiguity and dma_mask may not apply if the platform has an IOMMU (a device which maps an I/O DMA address to a physical memory address). However, to be portable, device driver writers may *not* assume that such an IOMMU exists.

There's not strictly any harm in using the DMA API this way and *hoping* you get what you want, as long as you're happy for it to fail pretty much 100% of the time on some systems, and still in a minority of corner cases on any system. However, if there's a real dependency on IOMMUs and tight control of IOVA allocation here, then the DMA API isn't really the right tool for the job, and maybe it's time to start looking to how to better fit these multimedia-subsystem-type use cases into the IOMMU API - as far as I understand it there's at least some conceptual overlap with the HSA PASID stuff being prototyped in PCI/x86-land at the moment, so it could be an apposite time to try and bang out some common requirements.

Robin.

[0]:http://article.gmane.org/gmane.linux.kernel.iommu/11185

Quoted 5 lines

Thanks for helping to move this forward.

-Dan

(quoted diff) +2 -0

+Tomasz, so he can reply to the thread
+Marek and Russell as recommended by Tomasz

Quoted 52 lines · Robin Murphy

Hi Dan,

Quoted 48 lines · Daniel Kurtz

+linux-media & VIDEOBUF2 FRAMEWORK maintainers since this is about the v4l2-contig's usage of the DMA API.

Hi Robin,

Quoted 41 lines · Robin Murphy

Quoted 39 lines · Yong Wu

On Thu, 2015-10-01 at 20:13 +0100, Robin Murphy wrote:
[...]

Quoted 35 lines

drivers/iommu/dma-iommu.c new file +20 -0

+/*
+ * The DMA API client is passing in a scatterlist which could describe
+ * any old buffer layout, but the IOMMU API requires everything to be
+ * aligned to IOMMU pages. Hence the need for this complicated bit of
+ * impedance-matching, to be able to hand off a suitably-aligned list,
+ * but still preserve the original offsets and sizes for the caller.
+ */
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+               int nents, int prot)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+       struct iova_domain *iovad = domain->iova_cookie;
+       struct iova *iova;
+       struct scatterlist *s, *prev = NULL;
+       dma_addr_t dma_addr;
+       size_t iova_len = 0;
+       int i;
+
+       /*
+        * Work out how much IOVA space we need, and align the segments

to

drivers/iommu/dma-iommu.c new file +12 -0

+        * IOVA granules for the IOMMU driver to handle. With some clever
+        * trickery we can modify the list in-place, but reversibly, by
+        * hiding the original data in the as-yet-unused DMA fields.
+        */
+       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);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/iommu/dma-iommu.c new file +6 -0

+               s->length = s_length;
+
+               /*
+                * The simple way to avoid the rare case of a segment
+                * crossing the boundary mask is to pad the previous one
+                * to end at a naturally-aligned IOVA for this one's

size,
+ * at the cost of potentially over-allocating a little.

Quoted 35 lines

Quoted 35 lines

Quoted 33 lines

Quoted 9 lines

drivers/iommu/dma-iommu.c line 472

Quoted 6 lines

drivers/iommu/dma-iommu.c new file +6 -0

+                */
+               if (prev) {
+                       size_t pad_len = roundup_pow_of_two(s_length);
+
+                       pad_len = (pad_len - iova_len) & (pad_len - 1);
+                       prev->length += pad_len;

Quoted 8 lines

Hi Robin,
While our v4l2 testing, It seems that we met a problem here. Here we update prev->length again, Do we need update
sg_dma_len(prev) again too?

Some function like vb2_dc_get_contiguous_size[1] always get
sg_dma_len(s) to compare instead of s->length. so it may break unexpectedly while sg_dma_len(s) is not same with s->length.

This is just tweaking the faked-up length that we hand off to iommu_map_sg() (see also the iova_align() above), to trick it into bumping this segment up to a suitable starting IOVA. The real length at this point is stashed in sg_dma_len(s), and will be copied back into s->length in __finalise_sg(), so both will hold the same true length once we return to the caller.

Yes, it does mean that if you have a list where the segment lengths are page aligned but not monotonically decreasing, e.g. {64k, 16k, 64k}, then you'll still end up with a gap between the second and third segments, but that's fine because the DMA API offers no guarantees about what the resulting DMA addresses will be (consider the no-IOMMU case where they would each just be "mapped" to their physical address). If that breaks v4l, then it's probably v4l's DMA API use that needs looking at (again).

Quoted 7 lines

Quoted 4 lines

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.

Not at all. The streaming DMA API (dma_map_* and friends) has two responsibilities: performing any necessary cache maintenance to ensure the device will correctly see data from the CPU, and the CPU will correctly see data from the device; and working out an address for that buffer from the device's point of view to actually hand off to the hardware (which is perfectly well allowed to fail).

Quoted 2 lines

Consider SWIOTLB's implementation - segments which already lie at physical addresses within the device's DMA mask just get passed through, while those that lie outside it get mapped into the bounce buffer, but still as individual allocations (arch code just handles cache maintenance on the resulting physical addresses and can apply any hard-wired DMA offset for the device concerned).

Quoted 7 lines

Quoted 4 lines

IIUC, the change above breaks this model by inserting gaps in how the buffer is mapped to device memory, such that the buffer is no longer contiguous in dma address space.

Even the existing arch/arm IOMMU DMA code which I guess this implicitly relies on doesn't guarantee that behaviour - if the mapping happens to reach one of the segment length/boundary limits it won't just leave a gap, it'll start an entirely new IOVA allocation which could well start at a wildly different address[0].

Quoted 50 lines

Quoted 50 lines

Here is the code in question from
drivers/media/v4l2-core/videobuf2-dma-contig.c :

static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)

{
         struct scatterlist *s;
         dma_addr_t expected = sg_dma_address(sgt->sgl);
         unsigned int i;
         unsigned long size = 0;

         for_each_sg(sgt->sgl, s, sgt->nents, i) {
                 if (sg_dma_address(s) != expected)
                         break;
                 expected = sg_dma_address(s) + sg_dma_len(s);
                 size += sg_dma_len(s);
         }
         return size;
}

static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
unsigned long size, enum dma_data_direction dma_dir)

{
         struct vb2_dc_conf *conf = alloc_ctx;
         struct vb2_dc_buf *buf;
         struct frame_vector *vec;
         unsigned long offset;
         int n_pages, i;
         int ret = 0;
         struct sg_table *sgt;
         unsigned long contig_size;
         unsigned long dma_align = dma_get_cache_alignment();
         DEFINE_DMA_ATTRS(attrs);

         dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);

         buf = kzalloc(sizeof *buf, GFP_KERNEL);
         buf->dma_dir = dma_dir;

         offset = vaddr & ~PAGE_MASK;
         vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
         buf->vec = vec;
         n_pages = frame_vector_count(vec);

         sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);

         ret = sg_alloc_table_from_pages(sgt, frame_vector_pages(vec), n_pages,
                 offset, size, GFP_KERNEL);

Quoted 7 lines

Quoted 4 lines

         sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
                                       buf->dma_dir, &attrs);

         contig_size = vb2_dc_get_contiguous_size(sgt);

(as an aside, it's rather unintuitive that the handling of the dma_map_sg call actually failing is entirely implicit here)

Quoted 24 lines

Quoted 16 lines

         if (contig_size < size) {

     <<<===   if the original buffer had sg entries that were not

aligned on the "natural" alignment for their size, the new arm64 iommu core code inserts a 'gap' in the iommu mapping, which causes vb2_dc_get_contiguous_size() to exit early (and return a smaller size than expected).

                 pr_err("contiguous mapping is too small %lu/%lu\n",
                         contig_size, size);
                 ret = -EFAULT;
                 goto fail_map_sg;
         }

Quoted 4 lines

So, is the videobuf2-dma-contig.c based on an incorrect assumption about how the DMA API is supposed to work?
Is it even possible to map a "contiguous-in-iova-range" mapping for a buffer given as an sg_table with an arbitrary set of pages?

From the Streaming DMA mappings section of Documentation/DMA-API.txt:

Quoted 7 lines

Note also that the above constraints on physical contiguity and dma_mask may not apply if the platform has an IOMMU (a device which maps an I/O DMA address to a physical memory address). However, to be portable, device driver writers may *not* assume that such an IOMMU exists.

There's not strictly any harm in using the DMA API this way and *hoping* you get what you want, as long as you're happy for it to fail pretty much 100% of the time on some systems, and still in a minority of corner cases on any system.

Quoted 1 line

However, if there's a real dependency on IOMMUs and tight control of IOVA allocation here, then the DMA API isn't really the right tool for the job, and maybe it's time to start looking to how to better fit these multimedia-subsystem-type use cases into the IOMMU API - as far as I understand it there's at least some conceptual overlap with the HSA PASID stuff being prototyped in PCI/x86-land at the moment, so it could be an apposite time to try and bang out some common requirements.

I'd like to know what is the boundary mask and what hardware imposes requirements like this. The cost here is not only over-allocating a little, but making many, many buffers contiguously mappable on the CPU, unmappable contiguously in IOMMU, which just defeats the purpose of having an IOMMU, which I believe should be there for simple IP blocks taking one DMA address to be able to view the buffer the same way as the CPU.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.

Quoted 1 line

Quoted 1 line

Not at all. The streaming DMA API (dma_map_* and friends) has two responsibilities: performing any necessary cache maintenance to ensure the device will correctly see data from the CPU, and the CPU will correctly see data from the device; and working out an address for that buffer from the device's point of view to actually hand off to the hardware (which is perfectly well allowed to fail).

Agreed. The dma_map_*() API is not guaranteed to return a single contiguous part of virtual address space for any given SG list. However it was understood to be able to map buffers contiguously mappable by the CPU into a single segment and users, videobuf2-dma-contig in particular, relied on this.

Quoted 1 line

Quoted 1 line

Consider SWIOTLB's implementation - segments which already lie at physical addresses within the device's DMA mask just get passed through, while those that lie outside it get mapped into the bounce buffer, but still as individual allocations (arch code just handles cache maintenance on the resulting physical addresses and can apply any hard-wired DMA offset for the device concerned).

And this is fine for vb2-dma-contig, which was made for devices that require buffers contiguous in its address space. Without IOMMU it will allow only physically contiguous buffers and fails otherwise, which is fine, because it's a hardware requirement.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

IIUC, the change above breaks this model by inserting gaps in how the buffer is mapped to device memory, such that the buffer is no longer contiguous in dma address space.

Even the existing arch/arm IOMMU DMA code which I guess this implicitly relies on doesn't guarantee that behaviour - if the mapping happens to reach one of the segment length/boundary limits it won't just leave a gap, it'll start an entirely new IOVA allocation which could well start at a wildly different address[0].

Could you explain segment length/boundary limits and when buffers can reach them? Sorry, i haven't been following all the discussions, but I'm not aware of any similar requirements of the IOMMU hardware I worked with.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Here is the code in question from
drivers/media/v4l2-core/videobuf2-dma-contig.c :

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
unsigned long size, enum dma_data_direction dma_dir)
{

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

          sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
                                        buf->dma_dir, &attrs);

          contig_size = vb2_dc_get_contiguous_size(sgt);

(as an aside, it's rather unintuitive that the handling of the dma_map_sg call actually failing is entirely implicit here)

I'm not sure what you mean, please elaborate. The code considers only the case of contiguously mapping at least the requested size as a success, because anything else is useless with the hardware.

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

So, is the videobuf2-dma-contig.c based on an incorrect assumption about how the DMA API is supposed to work?
Is it even possible to map a "contiguous-in-iova-range" mapping for a buffer given as an sg_table with an arbitrary set of pages?

From the Streaming DMA mappings section of Documentation/DMA-API.txt:

Note also that the above constraints on physical contiguity and dma_mask may not apply if the platform has an IOMMU (a device which maps an I/O DMA address to a physical memory address). However, to be portable, device driver writers may *not* assume that such an IOMMU exists.

Quoted 1 line

Quoted 1 line

There's not strictly any harm in using the DMA API this way and *hoping* you get what you want, as long as you're happy for it to fail pretty much 100% of the time on some systems, and still in a minority of corner cases on any system.

Could you please elaborate? I'd like to see examples, because I can't really imagine buffers mappable contiguously on CPU, but not on IOMMU. Also, as I said, the hardware I worked with didn't suffer from problems like this.

Quoted 1 line

Quoted 1 line

However, if there's a real dependency on IOMMUs and tight control of IOVA allocation here, then the DMA API isn't really the right tool for the job, and maybe it's time to start looking to how to better fit these multimedia-subsystem-type use cases into the IOMMU API - as far as I understand it there's at least some conceptual overlap with the HSA PASID stuff being prototyped in PCI/x86-land at the moment, so it could be an apposite time to try and bang out some common requirements.

The DMA API is actually the only good tool to use here to keep the videobuf2-dma-contig code away from the knowledge about platform specific data, e.g. presence of IOMMU. The only thing it knows is that the target hardware requires a single contiguous buffer and it relies on the fact that in correct cases the buffer given to it will meet this requirement (i.e. physically contiguous w/o IOMMU; CPU mappable with IOMMU).

Hi Tomasz,

Quoted 5 lines · Tomasz Figa

Agreed. The dma_map_*() API is not guaranteed to return a single contiguous part of virtual address space for any given SG list. However it was understood to be able to map buffers contiguously mappable by the CPU into a single segment and users, videobuf2-dma-contig in particular, relied on this.

I don't follow that - _any_ buffer made of page-sized chunks is going to be mappable contiguously by the CPU; it's clearly impossible for the streaming DMA API itself to offer such a guarantee, because it's entirely orthogonal to the presence or otherwise of an IOMMU.

Quoted 9 lines · Robin Murphy

Hi Tomasz,

Quoted 5 lines · Tomasz Figa

Agreed. The dma_map_*() API is not guaranteed to return a single contiguous part of virtual address space for any given SG list. However it was understood to be able to map buffers contiguously mappable by the CPU into a single segment and users, videobuf2-dma-contig in particular, relied on this.

Quoted 4 lines

I don't follow that - _any_ buffer made of page-sized chunks is going to be mappable contiguously by the CPU; it's clearly impossible for the streaming DMA API itself to offer such a guarantee, because it's entirely orthogonal to the presence or otherwise of an IOMMU.

Tomasz's use of "virtual address space" above in combination with the DMA API is really confusing.

Quoted 10 lines · Russell King - ARM Linux

Quoted 9 lines · Robin Murphy

Hi Tomasz,

Quoted 5 lines · Tomasz Figa

Agreed. The dma_map_*() API is not guaranteed to return a single contiguous part of virtual address space for any given SG list. However it was understood to be able to map buffers contiguously mappable by the CPU into a single segment and users, videobuf2-dma-contig in particular, relied on this.

Quoted 7 lines

Quoted 4 lines

I don't follow that - _any_ buffer made of page-sized chunks is going to be mappable contiguously by the CPU; it's clearly impossible for the streaming DMA API itself to offer such a guarantee, because it's entirely orthogonal to the presence or otherwise of an IOMMU.

Tomasz's use of "virtual address space" above in combination with the DMA API is really confusing.

I suppose I must have mistakenly use "virtual address space" somewhere instead of "IO virtual address space". I'm sorry for causing confusion.

The thing being discussed here is mapping of buffers described by scatterlists into IO virtual address space, i.e. the operation happening when dma_map_sg() is called for an IOMMU-enabled device.

Hi Tomasz,

Quoted 8 lines · Tomasz Figa

I'd like to know what is the boundary mask and what hardware imposes requirements like this. The cost here is not only over-allocating a little, but making many, many buffers contiguously mappable on the CPU, unmappable contiguously in IOMMU, which just defeats the purpose of having an IOMMU, which I believe should be there for simple IP blocks taking one DMA address to be able to view the buffer the same way as the CPU.

The expectation with dma_map_sg() is that you're either going to be iterating over the buffer segments, handing off each address to the device to process one by one;

or you have a scatter-gather-capable device, in which case you hand off the whole list at once.

It's in the latter case where you
have to make sure the list doesn't exceed the hardware limitations of that device. I believe the original concern was disk controllers (the introduction of dma_parms seems to originate from the linux-scsi list), but most scatter-gather engines are going to have some limit on how much they can handle per entry (IMO the dmaengine drivers are the easiest example to look at).

Segment boundaries are a little more arcane, but my assumption is that they relate to the kind of devices whose addressing is not flat but relative to some separate segment register (The "64-bit" mode of USB EHCI is one concrete example I can think of) - since you cannot realistically change the segment register while the device is in the middle of accessing a single buffer entry, that entry must not fall across a segment boundary or at some point the device's accesses are going to overflow the offset address bits and wrap around to bogus addresses at the bottom of the segment.

Now yes, it will be possible under _most_ circumstances to use an IOMMU to lay out a list of segments with page-aligned lengths within a single IOVA allocation whilst still meeting all the necessary constraints. It just needs some unavoidably complicated calculations - quite likely significantly more complex than my v5 version of map_sg() that tried to do that and merge segments but failed to take the initial alignment into account properly - since there are much simpler ways to enforce just the _necessary_ behaviour for the DMA API, I put the complicated stuff to one side for now to prevent it holding up getting the basic functional support in place.

Quoted 16 lines

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.

Quoted 6 lines

Quoted 6 lines

Not at all. The streaming DMA API (dma_map_* and friends) has two
responsibilities: performing any necessary cache maintenance to ensure the device will correctly see data from the CPU, and the CPU will correctly see data from the device; and working out an address for that buffer from the device's point of view to actually hand off to the hardware (which is perfectly well allowed to fail).

Quoted 5 lines

Agreed. The dma_map_*() API is not guaranteed to return a single contiguous part of virtual address space for any given SG list. However it was understood to be able to map buffers contiguously mappable by the CPU into a single segment and users, videobuf2-dma-contig in particular, relied on this.

I don't follow that - _any_ buffer made of page-sized chunks is going to be mappable contiguously by the CPU;'

it's clearly impossible for the streaming
DMA API itself to offer such a guarantee, because it's entirely orthogonal to the presence or otherwise of an IOMMU.

Furthermore, I can't see any existing dma_map_sg implementation (between arm/64 and x86, at least), that _won't_ break that expectation under certain conditions (ranging from "relatively pathological" to "always"), so it still seems questionable to have a dependency on it.

Quoted 9 lines

Quoted 7 lines

Quoted 7 lines

Consider SWIOTLB's implementation - segments which already lie at physical addresses within the device's DMA mask just get passed through, while those that lie outside it get mapped into the bounce buffer, but still as individual allocations (arch code just handles cache maintenance on the resulting physical addresses and can apply any hard-wired DMA offset for the device concerned).

Quoted 4 lines

And this is fine for vb2-dma-contig, which was made for devices that require buffers contiguous in its address space. Without IOMMU it will allow only physically contiguous buffers and fails otherwise, which is fine, because it's a hardware requirement.

If it depends on having contiguous-from-the-device's-view DMA buffers either way, that's a sign it should perhaps be using the coherent DMA API instead, which _does_ give such a guarantee. I'm well aware of the "but the noncacheable mappings make userspace access unacceptably slow!" issue many folks have with that, though, and don't particularly fancy going off on that tangent here.

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

IIUC, the change above breaks this model by inserting gaps in how the buffer is mapped to device memory, such that the buffer is no longer contiguous in dma address space.

Quoted 5 lines

Quoted 5 lines

Even the existing arch/arm IOMMU DMA code which I guess this implicitly relies on doesn't guarantee that behaviour - if the mapping happens to reach one of the segment length/boundary limits it won't just leave a gap, it'll start an entirely new IOVA allocation which could well start at a wildly different address[0].

Quoted 4 lines

Could you explain segment length/boundary limits and when buffers can reach them? Sorry, i haven't been following all the discussions, but I'm not aware of any similar requirements of the IOMMU hardware I worked with.

I hope the explanation at the top makes sense - it's purely about the requirements of the DMA master device itself, nothing to do with the IOMMU (or lack of) in the middle. Devices with scatter-gather DMA limitations exist, therefore the API for scatter-gather DMA is designed to represent and respect such limitations.

Quoted 22 lines

Quoted 20 lines

Quoted 20 lines

Quoted 4 lines

So, is the videobuf2-dma-contig.c based on an incorrect assumption about how the DMA API is supposed to work?
Is it even possible to map a "contiguous-in-iova-range" mapping for a buffer given as an sg_table with an arbitrary set of pages?

From the Streaming DMA mappings section of Documentation/DMA-API.txt:

Note also that the above constraints on physical contiguity and dma_mask may not apply if the platform has an IOMMU (a device which maps an I/O DMA address to a physical memory address). However, to
be
portable, device driver writers may *not* assume that such an IOMMU exists.

There's not strictly any harm in using the DMA API this way and *hoping* you get what you want, as long as you're happy for it to fail pretty much 100% of the time on some systems, and still in a minority of corner cases on any system.

Quoted 4 lines

Could you please elaborate? I'd like to see examples, because I can't really imagine buffers mappable contiguously on CPU, but not on IOMMU. Also, as I said, the hardware I worked with didn't suffer from problems like this.

"...device driver writers may *not* assume that such an IOMMU exists."

Quoted 9 lines

Quoted 7 lines

Quoted 7 lines

However, if there's a real dependency on IOMMUs and tight control of IOVA allocation here, then the DMA API isn't really the right tool for the job, and maybe it's time to start looking to how to better fit these multimedia-subsystem-type use cases into the IOMMU API - as far as I understand it there's at least some conceptual overlap with the HSA PASID stuff being prototyped in PCI/x86-land at the moment, so it could be an apposite time to try and bang out some common requirements.

Quoted 7 lines

The DMA API is actually the only good tool to use here to keep the videobuf2-dma-contig code away from the knowledge about platform specific data, e.g. presence of IOMMU. The only thing it knows is that the target hardware requires a single contiguous buffer and it relies on the fact that in correct cases the buffer given to it will meet this requirement (i.e. physically contiguous w/o IOMMU; CPU mappable with IOMMU).

As above; the DMA API guarantees only what the DMA API guarantees. An IOMMU-based implementation of streaming DMA is free to identity-map pages if it only cares about device isolation; a non-IOMMU implementation is free to provide streaming DMA remapping via some elaborate bounce-buffering scheme

if it really wants to. GART-type IOMMUs... let's not even go there.

If v4l needs a guarantee of a single contiguous DMA buffer, then it needs to use dma_alloc_coherent() for that, not streaming mappings.

My understanding of a scatterlist was that it represents a buffer as a whole, by joining together its physically discontinuous segments.

I don't see how single segments (layout of which is completely up to the allocator; often just single pages) would be usable for hardware that needs to do some work more serious than just writing a byte stream continuously to subsequent buffers. In case of such simple devices you don't even need an IOMMU (for means other than protection and/or getting over address space limitations).

However, IMHO the most important use case of an IOMMU is to make buffers, which are contiguous in CPU virtual address space (VA), contiguous in device's address space (IOVA).

Quoted 2 lines · Tomasz Figa

My understanding of a scatterlist was that it represents a buffer as a whole, by joining together its physically discontinuous segments.

Correct, and it may also be scattered in CPU virtual space as well.

Quoted 6 lines

I don't see how single segments (layout of which is completely up to the allocator; often just single pages) would be usable for hardware that needs to do some work more serious than just writing a byte stream continuously to subsequent buffers. In case of such simple devices you don't even need an IOMMU (for means other than protection and/or getting over address space limitations).

All that's required is that the addresses described in the scatterlist are accessed as an apparently contiguous series of bytes. They don't have to be contiguous in any address view, provided the device access appears to be contiguous. How that is done is really neither here nor there.

IOMMUs are normally there as an address translator - for example, the underlying device may not have the capability to address a scatterlist (eg, because it makes effectively random access) and in order to be accessible to the device, it needs to be made contiguous in device address space.

Another scenario is that you have more bits of physical address than a device can generate itself for DMA purposes, and you need an IOMMU to create a (possibly scattered) mapping in device address space within the ability of the device to address.

The requirements here depend on the device behind the IOMMU.

Quoted 3 lines

However, IMHO the most important use case of an IOMMU is to make buffers, which are contiguous in CPU virtual address space (VA), contiguous in device's address space (IOVA).

No - there is no requirement for CPU virtual contiguous buffers to also be contiguous in the device address space.

There is no requirement, but shouldn't it be desired for the mapping code to map them as such? Otherwise, how could the IOMMU use case you described above (address translator for devices which don't have the capability to address a scatterlist) be handled properly?

Is the general conclusion now that dma_map_sg() should not be used to create IOMMU mappings and we should make a step backwards making all drivers (or frameworks, such as videobuf2) do that manually? That would be really backwards, because code not aware of IOMMU existence at all would have to become aware of it.

Quoted 13 lines · Robin Murphy

Hi Tomasz,

Quoted 8 lines · Tomasz Figa

I'd like to know what is the boundary mask and what hardware imposes requirements like this. The cost here is not only over-allocating a little, but making many, many buffers contiguously mappable on the CPU, unmappable contiguously in IOMMU, which just defeats the purpose of having an IOMMU, which I believe should be there for simple IP blocks taking one DMA address to be able to view the buffer the same way as the CPU.

Quoted 3 lines

The expectation with dma_map_sg() is that you're either going to be iterating over the buffer segments, handing off each address to the device to process one by one;

My understanding of a scatterlist was that it represents a buffer as a whole, by joining together its physically discontinuous segments.

I don't see how single segments (layout of which is completely up to the allocator; often just single pages) would be usable for hardware that needs to do some work more serious than just writing a byte stream continuously to subsequent buffers. In case of such simple devices you don't even need an IOMMU (for means other than protection and/or getting over address space limitations).

However, IMHO the most important use case of an IOMMU is to make buffers, which are contiguous in CPU virtual address space (VA), contiguous in device's address space (IOVA). Your implementation of dma_map_sg() effectively breaks this ability, so I'm not really following why it's located under drivers/iommu and supposed to be used with IOMMU-enabled platforms...

Quoted 2 lines

or you have a scatter-gather-capable device, in which case you hand off the whole list at once.

No need for mapping ability of the IOMMU here as well (except for working around address space issues, as I mentioned above).

Quoted 8 lines

It's in the latter case where you
have to make sure the list doesn't exceed the hardware limitations of that device. I believe the original concern was disk controllers (the introduction of dma_parms seems to originate from the linux-scsi list), but most scatter-gather engines are going to have some limit on how much they can handle per entry (IMO the dmaengine drivers are the easiest example to look at).

Quoted 8 lines

Segment boundaries are a little more arcane, but my assumption is that they relate to the kind of devices whose addressing is not flat but relative to some separate segment register (The "64-bit" mode of USB EHCI is one concrete example I can think of) - since you cannot realistically change the segment register while the device is in the middle of accessing a single buffer entry, that entry must not fall across a segment boundary or at some point the device's accesses are going to overflow the offset address bits and wrap around to bogus addresses at the bottom of the segment.

The two requirements above sound like something really specific to scatter-gather-capable hardware, which as I pointed above, barely need an IOMMU (at least its mapping capabilities). We are talking here about very IOMMU-specific code, though...

Now, while I see that on some systems there might be IOMMU used for improving protection and working around addressing issues with SG-capable hardware, the code shouldn't be breaking the majority of systems with IOMMU used as the only possible way to make physically discontinuous appear (IO-virtually) continuous to devices incapable of scatter-gather.

Quoted 9 lines

Now yes, it will be possible under _most_ circumstances to use an IOMMU to lay out a list of segments with page-aligned lengths within a single IOVA allocation whilst still meeting all the necessary constraints. It just needs some unavoidably complicated calculations - quite likely significantly more complex than my v5 version of map_sg() that tried to do that and merge segments but failed to take the initial alignment into account properly - since there are much simpler ways to enforce just the _necessary_ behaviour for the DMA API, I put the complicated stuff to one side for now to prevent it holding up getting the basic functional support in place.

Somehow just whatever currently done in arch/arm/mm/dma-mapping.c was sufficient and not overly complicated.

See http://lxr.free-electrons.com/source/arch/arm/mm/dma-mapping.c#L1547 .

I can see that the code there at least tries to comply with maximum segment size constraint. Segment boundary seems to be ignored, though.

However, I'm convinced that in most (if not all) cases where IOMMU IOVA-contiguous mapping is needed, those two requirements don't exist. Do we really have to break the good hardware only because the bad^Wlimited one is broken?

Couldn't we preserve the ARM-like behavior whenever dma_parms->segment_boundary_mask is set to all 1s and dma_parms->max_segment_size to UINT_MAX (what currently drivers used to set) or 0 (sounds more logical for the meaning of "no maximum given")?

Quoted 22 lines

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.

Quoted 6 lines

Quoted 6 lines

Not at all. The streaming DMA API (dma_map_* and friends) has two
responsibilities: performing any necessary cache maintenance to ensure the device will correctly see data from the CPU, and the CPU will correctly see data from the device; and working out an address for that buffer from the device's point of view to actually hand off to the hardware (which is perfectly well allowed to fail).

Quoted 5 lines

Agreed. The dma_map_*() API is not guaranteed to return a single contiguous part of virtual address space for any given SG list. However it was understood to be able to map buffers contiguously mappable by the CPU into a single segment and users, videobuf2-dma-contig in particular, relied on this.

Quoted 2 lines

I don't follow that - _any_ buffer made of page-sized chunks is going to be mappable contiguously by the CPU;'

Yes it is. Actually the last chunk might not even need to be page-sized. However I believe we can have a scatterlist consisting of non-page-sized chunks in the middle as well, which is obviously not mappable in a contiguous way even for the CPU.

Quoted 3 lines

it's clearly impossible for the streaming
DMA API itself to offer such a guarantee, because it's entirely orthogonal to the presence or otherwise of an IOMMU.

But we are talking here about the very IOMMU-specific implementation of DMA API.

Quoted 4 lines

Furthermore, I can't see any existing dma_map_sg implementation (between arm/64 and x86, at least), that _won't_ break that expectation under certain conditions (ranging from "relatively pathological" to "always"), so it still seems questionable to have a dependency on it.

The current implementation for arch/arm doesn't break that expectation. As long as we fit inside the maximum segment size (which in most, if not all, cases of the hardware that actually requires such contiguous mapping to be created, is UINT_MAX).

http://lxr.free-electrons.com/source/arch/arm/mm/dma-mapping.c#L1547

Quoted 15 lines

Quoted 9 lines

Quoted 7 lines

Quoted 7 lines

Consider SWIOTLB's implementation - segments which already lie at physical addresses within the device's DMA mask just get passed through, while those that lie outside it get mapped into the bounce buffer, but still as individual allocations (arch code just handles cache maintenance on the resulting physical addresses and can apply any hard-wired DMA offset for the device concerned).

Quoted 4 lines

And this is fine for vb2-dma-contig, which was made for devices that require buffers contiguous in its address space. Without IOMMU it will allow only physically contiguous buffers and fails otherwise, which is fine, because it's a hardware requirement.

Quoted 6 lines

If it depends on having contiguous-from-the-device's-view DMA buffers either way, that's a sign it should perhaps be using the coherent DMA API instead, which _does_ give such a guarantee. I'm well aware of the "but the noncacheable mappings make userspace access unacceptably slow!" issue many folks have with that, though, and don't particularly fancy going off on that tangent here.

The keywords here are DMA-BUF and user pointer. Neither of these cases can use coherent DMA API, because the buffer is already allocated, so it just needs to be mapped into another device's (or its IOMMU's) address space. Obviously we can't guarantee mappability of such buffers, e.g. in case of importing non-contiguous buffers to a device without an IOMMU, However we expect the pipelines to be sane (physically contiguous buffers or both devices IOMMU-enabled), so that such things won't happen.

Quoted 19 lines

Quoted 13 lines

Quoted 11 lines

Quoted 11 lines

Quoted 3 lines

IIUC, the change above breaks this model by inserting gaps in how the buffer is mapped to device memory, such that the buffer is no longer contiguous in dma address space.

Even the existing arch/arm IOMMU DMA code which I guess this implicitly relies on doesn't guarantee that behaviour - if the mapping happens to reach one of the segment length/boundary limits it won't just leave a gap, it'll start an entirely new IOVA allocation which could well start at a wildly different address[0].

Quoted 4 lines

Could you explain segment length/boundary limits and when buffers can reach them? Sorry, i haven't been following all the discussions, but I'm not aware of any similar requirements of the IOMMU hardware I worked with.

Quoted 5 lines

I hope the explanation at the top makes sense - it's purely about the requirements of the DMA master device itself, nothing to do with the IOMMU (or lack of) in the middle. Devices with scatter-gather DMA limitations exist, therefore the API for scatter-gather DMA is designed to represent and respect such limitations.

Yes, it makes sense, thanks for the explanation. However there also exist devices with no scatter-gather capability, but behind an IOMMU without such fancy mapping limitations. I believe we should also respect the limitation of such setups, which is the lack of support for multiple IOVA segments.

Quoted 28 lines

Quoted 26 lines

Quoted 16 lines

Quoted 16 lines

Quoted 4 lines

So, is the videobuf2-dma-contig.c based on an incorrect assumption about how the DMA API is supposed to work?
Is it even possible to map a "contiguous-in-iova-range" mapping for a buffer given as an sg_table with an arbitrary set of pages?

From the Streaming DMA mappings section of Documentation/DMA-API.txt:

Note also that the above constraints on physical contiguity and dma_mask may not apply if the platform has an IOMMU (a device which maps an I/O DMA address to a physical memory address). However, to
be
portable, device driver writers may *not* assume that such an IOMMU exists.

Quoted 4 lines

Quoted 4 lines

There's not strictly any harm in using the DMA API this way and *hoping* you get what you want, as long as you're happy for it to fail pretty much 100% of the time on some systems, and still in a minority of corner cases on any system.

Could you please elaborate? I'd like to see examples, because I can't really imagine buffers mappable contiguously on CPU, but not on IOMMU. Also, as I said, the hardware I worked with didn't suffer from problems like this.

Quoted 2 lines

"...device driver writers may *not* assume that such an IOMMU exists."

And this is exactly why they _should_ use dma_map_sg(), because it was supposed to work correctly for both physically contiguous (i.e. 1 segment) buffers and non-IOMMU-enabled devices, as well as with non-contiguous (i.e. > 1 segment) buffers and IOMMU-enabled devices.

Quoted 18 lines

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

However, if there's a real dependency on IOMMUs and tight control of IOVA allocation here, then the DMA API isn't really the right tool for the job, and maybe it's time to start looking to how to better fit these multimedia-subsystem-type use cases into the IOMMU API - as far as I understand it there's at least some conceptual overlap with the HSA PASID stuff being prototyped in PCI/x86-land at the moment, so it could be an apposite time to try and bang out some common requirements.

The DMA API is actually the only good tool to use here to keep the videobuf2-dma-contig code away from the knowledge about platform specific data, e.g. presence of IOMMU. The only thing it knows is that the target hardware requires a single contiguous buffer and it relies on the fact that in correct cases the buffer given to it will meet this requirement (i.e. physically contiguous w/o IOMMU; CPU mappable with IOMMU).

Quoted 4 lines

As above; the DMA API guarantees only what the DMA API guarantees. An IOMMU-based implementation of streaming DMA is free to identity-map pages if it only cares about device isolation; a non-IOMMU implementation is free to provide streaming DMA remapping via some elaborate bounce-buffering scheme

I guess this is the area where our understandings of IOMMU-backed DMA API differ.

Quoted 1 line

if it really wants to. GART-type IOMMUs... let's not even go there.

I believe that's how IOMMU-based implementation of DMA API was supposed to work when first implemented for ARM...

Quoted 2 lines

If v4l needs a guarantee of a single contiguous DMA buffer, then it needs to use dma_alloc_coherent() for that, not streaming mappings.

Except that it can't use it, because the buffers are already allocated by another entity.

+linux-media & VIDEOBUF2 FRAMEWORK maintainers since this is about the v4l2-contig's usage of the DMA API.

Hi Robin,

Quoted 66 lines · Robin Murphy

Quoted 55 lines · Yong Wu

On Thu, 2015-10-01 at 20:13 +0100, Robin Murphy wrote:
[...]

Quoted 40 lines

drivers/iommu/dma-iommu.c new file +20 -0

+/*
+ * The DMA API client is passing in a scatterlist which could describe
+ * any old buffer layout, but the IOMMU API requires everything to be
+ * aligned to IOMMU pages. Hence the need for this complicated bit of
+ * impedance-matching, to be able to hand off a suitably-aligned list,
+ * but still preserve the original offsets and sizes for the caller.
+ */
+int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
+               int nents, int prot)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+       struct iova_domain *iovad = domain->iova_cookie;
+       struct iova *iova;
+       struct scatterlist *s, *prev = NULL;
+       dma_addr_t dma_addr;
+       size_t iova_len = 0;
+       int i;
+
+       /*
+        * Work out how much IOVA space we need, and align the segments

to

drivers/iommu/dma-iommu.c new file +18 -0

+        * IOVA granules for the IOMMU driver to handle. With some clever
+        * trickery we can modify the list in-place, but reversibly, by
+        * hiding the original data in the as-yet-unused DMA fields.
+        */
+       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;
+
+               /*
+                * The simple way to avoid the rare case of a segment
+                * crossing the boundary mask is to pad the previous one
+                * to end at a naturally-aligned IOVA for this one's

size,

drivers/iommu/dma-iommu.c line 472

Quoted 7 lines

drivers/iommu/dma-iommu.c new file +7 -0

+                * at the cost of potentially over-allocating a little.
+                */
+               if (prev) {
+                       size_t pad_len = roundup_pow_of_two(s_length);
+
+                       pad_len = (pad_len - iova_len) & (pad_len - 1);
+                       prev->length += pad_len;

Hi Robin,
While our v4l2 testing, It seems that we met a problem here. Here we update prev->length again, Do we need update
sg_dma_len(prev) again too?

Quoted 3 lines

Some function like vb2_dc_get_contiguous_size[1] always get
sg_dma_len(s) to compare instead of s->length. so it may break unexpectedly while sg_dma_len(s) is not same with s->length.

This is just tweaking the faked-up length that we hand off to iommu_map_sg() (see also the iova_align() above), to trick it into bumping this segment up to a suitable starting IOVA. The real length at this point is stashed in sg_dma_len(s), and will be copied back into s->length in __finalise_sg(), so both will hold the same true length once we return to the caller.

Quoted 7 lines

Yes, it does mean that if you have a list where the segment lengths are page aligned but not monotonically decreasing, e.g. {64k, 16k, 64k}, then you'll still end up with a gap between the second and third segments, but that's fine because the DMA API offers no guarantees about what the resulting DMA addresses will be (consider the no-IOMMU case where they would each just be "mapped" to their physical address). If that breaks v4l, then it's probably v4l's DMA API use that needs looking at (again).

Hmm, I thought the DMA API maps a (possibly) non-contiguous set of memory pages into a contiguous block in device memory address space. This would allow passing a dma mapped buffer to device dma using just a device address and length.
IIUC, the change above breaks this model by inserting gaps in how the buffer is mapped to device memory, such that the buffer is no longer contiguous in dma address space.

Here is the code in question from
drivers/media/v4l2-core/videobuf2-dma-contig.c :

static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)

{
         struct scatterlist *s;
         dma_addr_t expected = sg_dma_address(sgt->sgl);
         unsigned int i;
         unsigned long size = 0;

         for_each_sg(sgt->sgl, s, sgt->nents, i) {
                 if (sg_dma_address(s) != expected)
                         break;
                 expected = sg_dma_address(s) + sg_dma_len(s);
                 size += sg_dma_len(s);
         }
         return size;
}

static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
unsigned long size, enum dma_data_direction dma_dir)

{
         struct vb2_dc_conf *conf = alloc_ctx;
         struct vb2_dc_buf *buf;
         struct frame_vector *vec;
         unsigned long offset;
         int n_pages, i;
         int ret = 0;
         struct sg_table *sgt;
         unsigned long contig_size;
         unsigned long dma_align = dma_get_cache_alignment();
         DEFINE_DMA_ATTRS(attrs);

         dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);

         buf = kzalloc(sizeof *buf, GFP_KERNEL);
         buf->dma_dir = dma_dir;

         offset = vaddr & ~PAGE_MASK;
         vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
         buf->vec = vec;
         n_pages = frame_vector_count(vec);

         sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);

         ret = sg_alloc_table_from_pages(sgt, frame_vector_pages(vec), n_pages,
                 offset, size, GFP_KERNEL);

         sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
                                       buf->dma_dir, &attrs);

         contig_size = vb2_dc_get_contiguous_size(sgt);
         if (contig_size < size) {

     <<<===   if the original buffer had sg entries that were not

aligned on the "natural" alignment for their size, the new arm64 iommu core code inserts a 'gap' in the iommu mapping, which causes vb2_dc_get_contiguous_size() to exit early (and return a smaller size than expected).

                 pr_err("contiguous mapping is too small %lu/%lu\n",
                         contig_size, size);
                 ret = -EFAULT;
                 goto fail_map_sg;
         }

So, is the videobuf2-dma-contig.c based on an incorrect assumption about how the DMA API is supposed to work?
Is it even possible to map a "contiguous-in-iova-range" mapping for a buffer given as an sg_table with an arbitrary set of pages?

Thanks for helping to move this forward.

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

Since there is still work to do elsewhere to make DMA configuration happen in a more appropriate order and properly support platform devices in the IOMMU core, the device setup code unfortunately starts out carrying some workarounds to ensure it works correctly in the current state of things.

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

---
 arch/arm64/mm/dma-mapping.c | 435 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 435 insertions(+)

+/*
+ * TODO: Right now __iommu_setup_dma_ops() gets called too early to do
+ * everything it needs to - the device is only partially created and the
+ * IOMMU driver hasn't seen it yet, so it can't have a group. Thus we
+ * need this delayed attachment dance. Once IOMMU probe ordering is sorted
+ * to move the arch_setup_dma_ops() call later, all the notifier bits below
+ * become unnecessary, and will go away.
+ */

arch/arm64/mm/dma-mapping.c +62 -0

+struct iommu_dma_notifier_data {
+	struct list_head list;
+	struct device *dev;
+	const struct iommu_ops *ops;
+	u64 dma_base;
+	u64 size;
+};
+static LIST_HEAD(iommu_dma_masters);
+static DEFINE_MUTEX(iommu_dma_notifier_lock);
+
+/*
+ * Temporarily "borrow" a domain feature flag to to tell if we had to resort
+ * to creating our own domain here, in case we need to clean it up again.
+ */
+#define __IOMMU_DOMAIN_FAKE_DEFAULT		(1U << 31)
+
+static bool do_iommu_attach(struct device *dev, const struct iommu_ops *ops,
+			   u64 dma_base, u64 size)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	/*
+	 * Best case: The device is either part of a group which was
+	 * already attached to a domain in a previous call, or it's
+	 * been put in a default DMA domain by the IOMMU core.
+	 */
+	if (!domain) {
+		/*
+		 * Urgh. The IOMMU core isn't going to do default domains
+		 * for non-PCI devices anyway, until it has some means of
+		 * abstracting the entirely implementation-specific
+		 * sideband data/SoC topology/unicorn dust that may or
+		 * may not differentiate upstream masters.
+		 * So until then, HORRIBLE HACKS!
+		 */
+		domain = ops->domain_alloc(IOMMU_DOMAIN_DMA);
+		if (!domain)
+			goto out_no_domain;
+
+		domain->ops = ops;
+		domain->type = IOMMU_DOMAIN_DMA | __IOMMU_DOMAIN_FAKE_DEFAULT;
+
+		if (iommu_attach_device(domain, dev))
+			goto out_put_domain;
+	}
+
+	if (iommu_dma_init_domain(domain, dma_base, size))
+		goto out_detach;
+
+	dev->archdata.dma_ops = &iommu_dma_ops;
+	return true;
+
+out_detach:
+	iommu_detach_device(domain, dev);
+out_put_domain:
+	if (domain->type & __IOMMU_DOMAIN_FAKE_DEFAULT)
+		iommu_domain_free(domain);
+out_no_domain:
+	pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA ops\n",
+		dev_name(dev));
+	return false;
+}

arch/arm64/mm/dma-mapping.c +4 -0

+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				  const struct iommu_ops *ops)
+{
+	struct iommu_group *group;

arch/arm64/mm/dma-mapping.c +16 -0

+	if (group) {
+		do_iommu_attach(dev, ops, dma_base, size);
+		iommu_group_put(group);
+	} else {
+		queue_iommu_attach(dev, ops, dma_base, size);
+	}
+}
+
+#else
+
+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				  struct iommu_ops *iommu)
+{ }
+
+#endif  /* CONFIG_IOMMU_DMA */
+

Quoted 13 lines · 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.

Since there is still work to do elsewhere to make DMA configuration happen in a more appropriate order and properly support platform devices in the IOMMU core, the device setup code unfortunately starts out carrying some workarounds to ensure it works correctly in the current state of things.

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

---
  arch/arm64/mm/dma-mapping.c | 435 ++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 435 insertions(+)

[...]

Quoted 8 lines

+/*
+ * TODO: Right now __iommu_setup_dma_ops() gets called too early to do
+ * everything it needs to - the device is only partially created and the
+ * IOMMU driver hasn't seen it yet, so it can't have a group. Thus we
+ * need this delayed attachment dance. Once IOMMU probe ordering is sorted
+ * to move the arch_setup_dma_ops() call later, all the notifier bits below
+ * become unnecessary, and will go away.
+ */

Hi Robin,
Could I ask a question about the plan in the future:
How to move arch_setup_dma_ops() call later than IOMMU probe?

arch_setup_dma_ops is from of_dma_configure which is from
arm64_device_init, and IOMMU probe is subsys_init. So arch_setup_dma_ops will run before IOMMU probe normally, is it right?

Does Laurent's probe-deferral series could help do this? what's
the state of this series.

Quoted 62 lines

arch/arm64/mm/dma-mapping.c +62 -0

+struct iommu_dma_notifier_data {
+	struct list_head list;
+	struct device *dev;
+	const struct iommu_ops *ops;
+	u64 dma_base;
+	u64 size;
+};
+static LIST_HEAD(iommu_dma_masters);
+static DEFINE_MUTEX(iommu_dma_notifier_lock);
+
+/*
+ * Temporarily "borrow" a domain feature flag to to tell if we had to resort
+ * to creating our own domain here, in case we need to clean it up again.
+ */
+#define __IOMMU_DOMAIN_FAKE_DEFAULT		(1U << 31)
+
+static bool do_iommu_attach(struct device *dev, const struct iommu_ops *ops,
+			   u64 dma_base, u64 size)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	/*
+	 * Best case: The device is either part of a group which was
+	 * already attached to a domain in a previous call, or it's
+	 * been put in a default DMA domain by the IOMMU core.
+	 */
+	if (!domain) {
+		/*
+		 * Urgh. The IOMMU core isn't going to do default domains
+		 * for non-PCI devices anyway, until it has some means of
+		 * abstracting the entirely implementation-specific
+		 * sideband data/SoC topology/unicorn dust that may or
+		 * may not differentiate upstream masters.
+		 * So until then, HORRIBLE HACKS!
+		 */
+		domain = ops->domain_alloc(IOMMU_DOMAIN_DMA);
+		if (!domain)
+			goto out_no_domain;
+
+		domain->ops = ops;
+		domain->type = IOMMU_DOMAIN_DMA | __IOMMU_DOMAIN_FAKE_DEFAULT;
+
+		if (iommu_attach_device(domain, dev))
+			goto out_put_domain;
+	}
+
+	if (iommu_dma_init_domain(domain, dma_base, size))
+		goto out_detach;
+
+	dev->archdata.dma_ops = &iommu_dma_ops;
+	return true;
+
+out_detach:
+	iommu_detach_device(domain, dev);
+out_put_domain:
+	if (domain->type & __IOMMU_DOMAIN_FAKE_DEFAULT)
+		iommu_domain_free(domain);
+out_no_domain:
+	pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA ops\n",
+		dev_name(dev));
+	return false;
+}

[...]

Quoted 4 lines

arch/arm64/mm/dma-mapping.c +4 -0

+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				  const struct iommu_ops *ops)
+{
+	struct iommu_group *group;

arch/arm64/mm/dma-mapping.c static int __init dma_debug_do_init(void) · line 954

Quoted 10 lines

arch/arm64/mm/dma-mapping.c +10 -0

+
+	if (!ops)
+		return;
+	/*
+	 * TODO: As a concession to the future, we're ready to handle being
+	 * called both early and late (i.e. after bus_add_device). Once all
+	 * the platform bus code is reworked to call us late and the notifier
+	 * junk above goes away, move the body of do_iommu_attach here.
+	 */
+	group = iommu_group_get(dev);

If iommu_setup_dma_ops run after bus_add_device, then the device has
its group here. It will enter do_iommu_attach which will alloc a default iommu domain and attach this device to the new iommu domain.
But mtk-iommu don't expect like this, we would like to attach to the
same domain. So we should alloc a default iommu domain(if there is no iommu domain at that time) and attach the device to the same domain in our xx_add_device, is it right?

Quoted 16 lines

arch/arm64/mm/dma-mapping.c +16 -0

+	if (group) {
+		do_iommu_attach(dev, ops, dma_base, size);
+		iommu_group_put(group);
+	} else {
+		queue_iommu_attach(dev, ops, dma_base, size);
+	}
+}
+
+#else
+
+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				  struct iommu_ops *iommu)
+{ }
+
+#endif  /* CONFIG_IOMMU_DMA */
+

Quoted 28 lines · Yong Wu

Quoted 13 lines · 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.

Since there is still work to do elsewhere to make DMA configuration happen in a more appropriate order and properly support platform devices in the IOMMU core, the device setup code unfortunately starts out carrying some workarounds to ensure it works correctly in the current state of things.

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

---
  arch/arm64/mm/dma-mapping.c | 435 ++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 435 insertions(+)

[...]

Quoted 8 lines

+/*
+ * TODO: Right now __iommu_setup_dma_ops() gets called too early to do
+ * everything it needs to - the device is only partially created and the
+ * IOMMU driver hasn't seen it yet, so it can't have a group. Thus we
+ * need this delayed attachment dance. Once IOMMU probe ordering is sorted
+ * to move the arch_setup_dma_ops() call later, all the notifier bits below
+ * become unnecessary, and will go away.
+ */

Hi Robin,
Could I ask a question about the plan in the future:
How to move arch_setup_dma_ops() call later than IOMMU probe?

Quoted 3 lines

arch_setup_dma_ops is from of_dma_configure which is from
arm64_device_init, and IOMMU probe is subsys_init. So arch_setup_dma_ops will run before IOMMU probe normally, is it right?

Yup, hence the need to call of_platform_device_create() manually in your IOMMU_OF_DECLARE init function if you need the actual device instance to be ready before the root of_platform_populate() runs.

Quoted 3 lines

Does Laurent's probe-deferral series could help do this? what's
the state of this series.

What Laurent's patches do is to leave the DMA mask configuration where it is early in device creation, but split out the dma_ops configuration to be called just before the actual driver probe, and defer that if the IOMMU device hasn't probed yet. At the moment, those patches (plus a bit of my own development on top) are working fairly well in the simple case, but I've seen things start falling apart if the client driver then requests its own probe deferral, and there are probably other troublesome edge cases to find - I need to dig into that further, but sorting out my ARM SMMU driver patches is currently looking like a higher priority.

Quoted 7 lines

Quoted 7 lines

arch/arm64/mm/dma-mapping.c +7 -0

+struct iommu_dma_notifier_data {
+	struct list_head list;
+	struct device *dev;
+	const struct iommu_ops *ops;
+	u64 dma_base;
+	u64 size;
+};

[...]

Quoted 15 lines

Quoted 4 lines

arch/arm64/mm/dma-mapping.c +4 -0

+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				  const struct iommu_ops *ops)
+{
+	struct iommu_group *group;

arch/arm64/mm/dma-mapping.c static int __init dma_debug_do_init(void) · line 954

Quoted 10 lines

arch/arm64/mm/dma-mapping.c +10 -0

+
+	if (!ops)
+		return;
+	/*
+	 * TODO: As a concession to the future, we're ready to handle being
+	 * called both early and late (i.e. after bus_add_device). Once all
+	 * the platform bus code is reworked to call us late and the notifier
+	 * junk above goes away, move the body of do_iommu_attach here.
+	 */
+	group = iommu_group_get(dev);

Quoted 7 lines

If iommu_setup_dma_ops run after bus_add_device, then the device has
its group here. It will enter do_iommu_attach which will alloc a default iommu domain and attach this device to the new iommu domain.
But mtk-iommu don't expect like this, we would like to attach to the
same domain. So we should alloc a default iommu domain(if there is no iommu domain at that time) and attach the device to the same domain in our xx_add_device, is it right?

Yes, if you attach the device to your own 'real' default domain after setting up the group in add_device, then do_iommu_attach() will now pick that domain up and use it instead of trying to create a new one, and the arch code will stop short of tearing the domain down if the device probe fails and it gets detached again. Additionally, since from add_device you should hopefully have all the information you need to get back to the relevant m4u instance, it should now be OK to keep the default domain there and finally get rid of that pesky global variable.

Robin.

Quoted 18 lines

Quoted 17 lines

arch/arm64/mm/dma-mapping.c +16 -0

+	if (group) {
+		do_iommu_attach(dev, ops, dma_base, size);
+		iommu_group_put(group);
+	} else {
+		queue_iommu_attach(dev, ops, dma_base, size);
+	}
+}
+
+#else
+
+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				  struct iommu_ops *iommu)
+{ }
+
+#endif  /* CONFIG_IOMMU_DMA */
+

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

Since there is still work to do elsewhere to make DMA configuration happen in a more appropriate order and properly support platform devices in the IOMMU core, the device setup code unfortunately starts out carrying some workarounds to ensure it works correctly in the current state of things.

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

---
 arch/arm64/mm/dma-mapping.c | 435 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 435 insertions(+)

arch/arm64/mm/dma-mapping.c +293 -0

index 0bcc4bc..dd2d6e6 100644
@@ -533,3 +533,438 @@ static int __init dma_debug_do_init(void)
        return 0;
 }
 fs_initcall(dma_debug_do_init);
+
+
+#ifdef CONFIG_IOMMU_DMA
+#include <linux/dma-iommu.h>
+#include <linux/platform_device.h>
+#include <linux/amba/bus.h>
+
+/* Thankfully, all cache ops are by VA so we can ignore phys here */
+static void flush_page(struct device *dev, const void *virt, phys_addr_t phys)
+{
+       __dma_flush_range(virt, virt + PAGE_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);
+       int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+       void *addr;
+
+       if (WARN(!dev, "cannot create IOMMU mapping for unknown device\n"))
+               return NULL;
+       /*
+        * Some drivers rely on this, and we probably don't want the
+        * possibility of stale kernel data being read by devices anyway.
+        */
+       gfp |= __GFP_ZERO;
+
+       if (gfp & __GFP_WAIT) {
+               struct page **pages;
+               pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
+
+               pages = iommu_dma_alloc(dev, size, gfp, ioprot, handle,
+                                       flush_page);
+               if (!pages)
+                       return NULL;
+
+               addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
+                                             __builtin_return_address(0));
+               if (!addr)
+                       iommu_dma_free(dev, pages, size, handle);
+       } else {
+               struct page *page;
+               /*
+                * In atomic context we can't remap anything, so we'll only
+                * get the virtually contiguous buffer we need by way of a
+                * physically contiguous allocation.
+                */
+               if (coherent) {
+                       page = alloc_pages(gfp, get_order(size));
+                       addr = page ? page_address(page) : NULL;
+               } else {
+                       addr = __alloc_from_pool(size, &page, gfp);
+               }
+               if (!addr)
+                       return NULL;
+
+               *handle = iommu_dma_map_page(dev, page, 0, size, ioprot);
+               if (iommu_dma_mapping_error(dev, *handle)) {
+                       if (coherent)
+                               __free_pages(page, get_order(size));
+                       else
+                               __free_from_pool(addr, size);
+                       addr = NULL;
+               }
+       }
+       return addr;
+}
+
+static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
+                              dma_addr_t handle, struct dma_attrs *attrs)
+{
+       /*
+        * @cpu_addr will be one of 3 things depending on how it was allocated:
+        * - A remapped array of pages from iommu_dma_alloc(), for all
+        *   non-atomic allocations.
+        * - A non-cacheable alias from the atomic pool, for atomic
+        *   allocations by non-coherent devices.
+        * - A normal lowmem address, for atomic allocations by
+        *   coherent devices.
+        * Hence how dodgy the below logic looks...
+        */
+       if (__in_atomic_pool(cpu_addr, size)) {
+               iommu_dma_unmap_page(dev, handle, size, 0, NULL);
+               __free_from_pool(cpu_addr, size);
+       } else if (is_vmalloc_addr(cpu_addr)){
+               struct vm_struct *area = find_vm_area(cpu_addr);
+
+               if (WARN_ON(!area || !area->pages))
+                       return;
+               iommu_dma_free(dev, area->pages, size, &handle);
+               dma_common_free_remap(cpu_addr, size, VM_USERMAP);
+       } else {
+               iommu_dma_unmap_page(dev, handle, size, 0, NULL);
+               __free_pages(virt_to_page(cpu_addr), get_order(size));
+       }
+}
+
+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)
+{
+       struct vm_struct *area;
+       int ret;
+
+       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;
+
+       area = find_vm_area(cpu_addr);
+       if (WARN_ON(!area || !area->pages))
+               return -ENXIO;
+
+       return iommu_dma_mmap(area->pages, size, vma);
+}
+
+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 vm_struct *area = find_vm_area(cpu_addr);
+
+       if (WARN_ON(!area || !area->pages))
+               return -ENXIO;
+
+       return sg_alloc_table_from_pages(sgt, area->pages, count, 0, size,
+                                        GFP_KERNEL);
+}
+
+static void __iommu_sync_single_for_cpu(struct device *dev,
+                                       dma_addr_t dev_addr, size_t size,
+                                       enum dma_data_direction dir)
+{
+       phys_addr_t phys;
+
+       if (is_device_dma_coherent(dev))
+               return;
+
+       phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), 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)
+{
+       phys_addr_t phys;
+
+       if (is_device_dma_coherent(dev))
+               return;
+
+       phys = iommu_iova_to_phys(iommu_get_domain_for_dev(dev), 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)
+{
+       bool coherent = is_device_dma_coherent(dev);
+       int prot = dma_direction_to_prot(dir, coherent);
+       dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
+
+       if (!iommu_dma_mapping_error(dev, dev_addr) &&
+           !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)
+{
+       bool coherent = is_device_dma_coherent(dev);
+
+       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,
+                       dma_direction_to_prot(dir, coherent));
+}
+
+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,
+       .get_sgtable = __iommu_get_sgtable,
+       .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,
+};
+
+/*
+ * TODO: Right now __iommu_setup_dma_ops() gets called too early to do
+ * everything it needs to - the device is only partially created and the
+ * IOMMU driver hasn't seen it yet, so it can't have a group. Thus we
+ * need this delayed attachment dance. Once IOMMU probe ordering is sorted
+ * to move the arch_setup_dma_ops() call later, all the notifier bits below
+ * become unnecessary, and will go away.
+ */
+struct iommu_dma_notifier_data {
+       struct list_head list;
+       struct device *dev;
+       const struct iommu_ops *ops;
+       u64 dma_base;
+       u64 size;
+};
+static LIST_HEAD(iommu_dma_masters);
+static DEFINE_MUTEX(iommu_dma_notifier_lock);
+
+/*
+ * Temporarily "borrow" a domain feature flag to to tell if we had to resort
+ * to creating our own domain here, in case we need to clean it up again.
+ */
+#define __IOMMU_DOMAIN_FAKE_DEFAULT            (1U << 31)
+
+static bool do_iommu_attach(struct device *dev, const struct iommu_ops *ops,
+                          u64 dma_base, u64 size)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+       /*
+        * Best case: The device is either part of a group which was
+        * already attached to a domain in a previous call, or it's
+        * been put in a default DMA domain by the IOMMU core.
+        */
+       if (!domain) {
+               /*
+                * Urgh. The IOMMU core isn't going to do default domains
+                * for non-PCI devices anyway, until it has some means of
+                * abstracting the entirely implementation-specific

arch/arm64/mm/dma-mapping.c +132 -1

+
+               if (iommu_attach_device(domain, dev))
+                       goto out_put_domain;
+       }
+
+       if (iommu_dma_init_domain(domain, dma_base, size))
+               goto out_detach;
+
+       dev->archdata.dma_ops = &iommu_dma_ops;
+       return true;
+
+out_detach:
+       iommu_detach_device(domain, dev);
+out_put_domain:
+       if (domain->type & __IOMMU_DOMAIN_FAKE_DEFAULT)
+               iommu_domain_free(domain);
+out_no_domain:
+       pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA ops\n",
+               dev_name(dev));
+       return false;
+}
+
+static void queue_iommu_attach(struct device *dev, const struct iommu_ops *ops,
+                             u64 dma_base, u64 size)
+{
+       struct iommu_dma_notifier_data *iommudata;
+
+       iommudata = kzalloc(sizeof(*iommudata), GFP_KERNEL);
+       if (!iommudata)
+               return;
+
+       iommudata->dev = dev;
+       iommudata->ops = ops;
+       iommudata->dma_base = dma_base;
+       iommudata->size = size;
+
+       mutex_lock(&iommu_dma_notifier_lock);
+       list_add(&iommudata->list, &iommu_dma_masters);
+       mutex_unlock(&iommu_dma_notifier_lock);
+}
+
+static int __iommu_attach_notifier(struct notifier_block *nb,
+                                  unsigned long action, void *data)
+{
+       struct iommu_dma_notifier_data *master, *tmp;
+
+       if (action != BUS_NOTIFY_ADD_DEVICE)
+               return 0;
+
+       mutex_lock(&iommu_dma_notifier_lock);
+       list_for_each_entry_safe(master, tmp, &iommu_dma_masters, list) {
+               if (do_iommu_attach(master->dev, master->ops,
+                               master->dma_base, master->size)) {
+                       list_del(&master->list);
+                       kfree(master);
+               }
+       }
+       mutex_unlock(&iommu_dma_notifier_lock);
+       return 0;
+}
+
+static int register_iommu_dma_ops_notifier(struct bus_type *bus)
+{
+       struct notifier_block *nb = kzalloc(sizeof(*nb), GFP_KERNEL);
+       int ret;
+
+       if (!nb)
+               return -ENOMEM;
+       /*
+        * The device must be attached to a domain before the driver probe
+        * routine gets a chance to start allocating DMA buffers. However,
+        * the IOMMU driver also needs a chance to configure the iommu_group
+        * via its add_device callback first, so we need to make the attach
+        * happen between those two points. Since the IOMMU core uses a bus
+        * notifier with default priority for add_device, do the same but
+        * with a lower priority to ensure the appropriate ordering.
+        */
+       nb->notifier_call = __iommu_attach_notifier;
+       nb->priority = -100;
+
+       ret = bus_register_notifier(bus, nb);
+       if (ret) {
+               pr_warn("Failed to register DMA domain notifier; IOMMU DMA ops unavailable on bus '%s'\n",
+                       bus->name);
+               kfree(nb);
+       }
+       return ret;
+}
+
+static int __init __iommu_dma_init(void)
+{
+       int ret;
+
+       ret = iommu_dma_init();
+       if (!ret)
+               ret = register_iommu_dma_ops_notifier(&platform_bus_type);
+       if (!ret)
+               ret = register_iommu_dma_ops_notifier(&amba_bustype);
+       return ret;
+}
+arch_initcall(__iommu_dma_init);
+
+static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+                                 const struct iommu_ops *ops)
+{
+       struct iommu_group *group;
+
+       if (!ops)
+               return;
+       /*
+        * TODO: As a concession to the future, we're ready to handle being
+        * called both early and late (i.e. after bus_add_device). Once all
+        * the platform bus code is reworked to call us late and the notifier
+        * junk above goes away, move the body of do_iommu_attach here.
+        */
+       group = iommu_group_get(dev);
+       if (group) {
+               do_iommu_attach(dev, ops, dma_base, size);
+               iommu_group_put(group);
+       } else {
+               queue_iommu_attach(dev, ops, dma_base, size);
+       }
+}
+
+#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

Quoted 20 lines

arch/arm64/mm/dma-mapping.c +20 -0

+static bool do_iommu_attach(struct device *dev, const struct iommu_ops *ops,
+                          u64 dma_base, u64 size)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+       /*
+        * Best case: The device is either part of a group which was
+        * already attached to a domain in a previous call, or it's
+        * been put in a default DMA domain by the IOMMU core.
+        */
+       if (!domain) {
+               /*
+                * Urgh. The IOMMU core isn't going to do default domains
+                * for non-PCI devices anyway, until it has some means of
+                * abstracting the entirely implementation-specific
+                * sideband data/SoC topology/unicorn dust that may or
+                * may not differentiate upstream masters.
+                * So until then, HORRIBLE HACKS!
+                */
+               domain = ops->domain_alloc(IOMMU_DOMAIN_DMA);

arch/arm64/mm/dma-mapping.c static int __init dma_debug_do_init(void) · line 838

Quoted 5 lines

arch/arm64/mm/dma-mapping.c +5 -0

+               if (!domain)
+                       goto out_no_domain;
+
+               domain->ops = ops;
+               domain->type = IOMMU_DOMAIN_DMA | __IOMMU_DOMAIN_FAKE_DEFAULT;

We require iommu_get_dma_cookie(domain) here. If we dont allocate iommu cookie then iommu_dma_init_domain() will fail.

On 07/10/15 10:03, Anup Patel wrote:
[...]

Quoted 23 lines

Quoted 23 lines

+static bool do_iommu_attach(struct device *dev, const struct iommu_ops *ops,

arch/arm64/mm/dma-mapping.c +21 -0

+                          u64 dma_base, u64 size)
+{
+       struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+       /*
+        * Best case: The device is either part of a group which was
+        * already attached to a domain in a previous call, or it's
+        * been put in a default DMA domain by the IOMMU core.
+        */
+       if (!domain) {
+               /*
+                * Urgh. The IOMMU core isn't going to do default domains
+                * for non-PCI devices anyway, until it has some means of
+                * abstracting the entirely implementation-specific
+                * sideband data/SoC topology/unicorn dust that may or
+                * may not differentiate upstream masters.
+                * So until then, HORRIBLE HACKS!
+                */
+               domain = ops->domain_alloc(IOMMU_DOMAIN_DMA);
+               if (!domain)
+                       goto out_no_domain;

Quoted 8 lines

Quoted 4 lines

arch/arm64/mm/dma-mapping.c +3 -0

+
+               domain->ops = ops;
+               domain->type = IOMMU_DOMAIN_DMA |

__IOMMU_DOMAIN_FAKE_DEFAULT;

We require iommu_get_dma_cookie(domain) here. If we dont allocate iommu cookie then iommu_dma_init_domain() will fail.

The iova cookie is tightly coupled with the domain, so it really only makes sense for the IOMMU driver to deal with it as part of its domain_alloc/domain_free callbacks.

Doing that here was one of the nastier 'compatibility' hacks which I've now taken out; trying to make things work without modifying existing IOMMU drivers was just too impractical. Drivers which want to play are now required to support the IOMMU_DOMAIN_DMA type appropriately, but it's still only a minimal change, as per the example diff for the ARM SMMU driver below (big pile of further patches necessary to make said driver compatible in other respects notwithstanding).

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

Since there is still work to do elsewhere to make DMA configuration happen in a more appropriate order and properly support platform devices in the IOMMU core, the device setup code unfortunately starts out carrying some workarounds to ensure it works correctly in the current state of things.

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

---
 arch/arm64/mm/dma-mapping.c | 435 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 435 insertions(+)

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

Since there is still work to do elsewhere to make DMA configuration happen in a more appropriate order and properly support platform devices in the IOMMU core, the device setup code unfortunately starts out carrying some workarounds to ensure it works correctly in the current state of things.

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

Quoted 8 lines · 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.

Since there is still work to do elsewhere to make DMA configuration happen in a more appropriate order and properly support platform devices in the IOMMU core, the device setup code unfortunately starts out carrying some workarounds to ensure it works correctly in the current state of things.

Quoted 1 line

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

Sorry, I reviewed this patch before but forgot to ack it, so here it is:

Acked-by Catalin Marinas <[email protected]>

(and I'm fined for the arm64 patches here to go in via the iommu tree)

I assume part of this patch will disappear at some point when the device probing order is sorted.

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

arch/arm64/mm/dma-mapping.c +70 -0

+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);
+	int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+	void *addr;
+
+	if (WARN(!dev, "cannot create IOMMU mapping for unknown device\n"))
+		return NULL;
+	/*
+	 * Some drivers rely on this, and we probably don't want the
+	 * possibility of stale kernel data being read by devices anyway.
+	 */
+	gfp |= __GFP_ZERO;
+
+	if (gfp & __GFP_WAIT) {
+		struct page **pages;
+		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
+
+		pages = iommu_dma_alloc(dev, size, gfp, ioprot,	handle,
+					flush_page);
+		if (!pages)
+			return NULL;
+
+		addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
+					      __builtin_return_address(0));
+		if (!addr)
+			iommu_dma_free(dev, pages, size, handle);
+	} else {
+		struct page *page;
+		/*
+		 * In atomic context we can't remap anything, so we'll only
+		 * get the virtually contiguous buffer we need by way of a
+		 * physically contiguous allocation.
+		 */
+		if (coherent) {
+			page = alloc_pages(gfp, get_order(size));
+			addr = page ? page_address(page) : NULL;
+		} else {
+			addr = __alloc_from_pool(size, &page, gfp);
+		}
+		if (!addr)
+			return NULL;
+
+		*handle = iommu_dma_map_page(dev, page, 0, size, ioprot);
+		if (iommu_dma_mapping_error(dev, *handle)) {
+			if (coherent)
+				__free_pages(page, get_order(size));
+			else
+				__free_from_pool(addr, size);
+			addr = NULL;
+		}
+	}
+	return addr;
+}
+
+static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
+			       dma_addr_t handle, struct dma_attrs *attrs)
+{
+	/*
+	 * @cpu_addr will be one of 3 things depending on how it was allocated:
+	 * - A remapped array of pages from iommu_dma_alloc(), for all
+	 *   non-atomic allocations.
+	 * - A non-cacheable alias from the atomic pool, for atomic
+	 *   allocations by non-coherent devices.
+	 * - A normal lowmem address, for atomic allocations by
+	 *   coherent devices.
+	 * Hence how dodgy the below logic looks...
+	 */

arch/arm64/mm/dma-mapping.c +6 -0

+	} else {
+		iommu_dma_unmap_page(dev, handle, size, 0, NULL);
+		__free_pages(virt_to_page(cpu_addr), get_order(size));
+	}
+}
+

Quoted 2 lines · 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.

[...]

Quoted 70 lines

arch/arm64/mm/dma-mapping.c +70 -0

+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);
+	int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+	void *addr;
+
+	if (WARN(!dev, "cannot create IOMMU mapping for unknown device\n"))
+		return NULL;
+	/*
+	 * Some drivers rely on this, and we probably don't want the
+	 * possibility of stale kernel data being read by devices anyway.
+	 */
+	gfp |= __GFP_ZERO;
+
+	if (gfp & __GFP_WAIT) {
+		struct page **pages;
+		pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
+
+		pages = iommu_dma_alloc(dev, size, gfp, ioprot,	handle,
+					flush_page);
+		if (!pages)
+			return NULL;
+
+		addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
+					      __builtin_return_address(0));
+		if (!addr)
+			iommu_dma_free(dev, pages, size, handle);
+	} else {
+		struct page *page;
+		/*
+		 * In atomic context we can't remap anything, so we'll only
+		 * get the virtually contiguous buffer we need by way of a
+		 * physically contiguous allocation.
+		 */
+		if (coherent) {
+			page = alloc_pages(gfp, get_order(size));
+			addr = page ? page_address(page) : NULL;
+		} else {
+			addr = __alloc_from_pool(size, &page, gfp);
+		}
+		if (!addr)
+			return NULL;
+
+		*handle = iommu_dma_map_page(dev, page, 0, size, ioprot);
+		if (iommu_dma_mapping_error(dev, *handle)) {
+			if (coherent)
+				__free_pages(page, get_order(size));
+			else
+				__free_from_pool(addr, size);
+			addr = NULL;
+		}
+	}
+	return addr;
+}
+
+static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
+			       dma_addr_t handle, struct dma_attrs *attrs)
+{
+	/*
+	 * @cpu_addr will be one of 3 things depending on how it was allocated:
+	 * - A remapped array of pages from iommu_dma_alloc(), for all
+	 *   non-atomic allocations.
+	 * - A non-cacheable alias from the atomic pool, for atomic
+	 *   allocations by non-coherent devices.
+	 * - A normal lowmem address, for atomic allocations by
+	 *   coherent devices.
+	 * Hence how dodgy the below logic looks...
+	 */

arch/arm64/mm/dma-mapping.c static int __init dma_debug_do_init(void) · line 628

Quoted 10 lines

arch/arm64/mm/dma-mapping.c +10 -0

+	if (__in_atomic_pool(cpu_addr, size)) {
+		iommu_dma_unmap_page(dev, handle, size, 0, NULL);
+		__free_from_pool(cpu_addr, size);
+	} else if (is_vmalloc_addr(cpu_addr)){
+		struct vm_struct *area = find_vm_area(cpu_addr);
+
+		if (WARN_ON(!area || !area->pages))
+			return;
+		iommu_dma_free(dev, area->pages, size, &handle);
+		dma_common_free_remap(cpu_addr, size, VM_USERMAP);

Hi Robin,
We get a WARN issue while the size is not aligned here.

The WARN log is:

[  206.852002] WARNING: CPU: 0 PID: 23329
at /mnt/host/source/src/third_party/kernel/v3.18/mm/vmalloc.c:65
vunmap_page_range+0x190/0x1b4()
[  206.864438] Modules linked in: nls_iso8859_1 nls_cp437 vfat fat

rfcomm i2c_dev uinput dm9601 uvcvideo btmrvl_sdio mwifiex_sdio mwifiex btmrvl bluetooth zram fuse cfg80211 nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables cdc_ether usbnet mii joydev snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device ppp_async ppp_generic slhc tun

[  206.902983] CPU: 0 PID: 23329 Comm: chrome Not tainted 3.18.0 #17
[  206.910430] Hardware name: Mediatek Oak rev3 board (DT)
[  206.920018] Call trace:
[  206.925537] [<ffffffc000208c00>] dump_backtrace+0x0/0x140
[  206.931905] [<ffffffc000208d5c>] show_stack+0x1c/0x28
[  206.939158] [<ffffffc000870f80>] dump_stack+0x74/0x94
[  206.947459] [<ffffffc0002219a4>] warn_slowpath_common+0x90/0xb8
[  206.954100] [<ffffffc000221b58>] warn_slowpath_null+0x34/0x44
[  206.961537] [<ffffffc000321358>] vunmap_page_range+0x18c/0x1b4
[  206.967630] [<ffffffc0003213e4>] unmap_kernel_range+0x2c/0x78
[  206.976977] [<ffffffc000582224>] dma_common_free_remap+0x68/0x80
[  206.983581] [<ffffffc000217260>] __iommu_free_attrs+0x14c/0x160
[  206.989646] [<ffffffc00066fc1c>] mtk_vcodec_mem_free+0xa0/0x15c
[  206.996481] [<ffffffc00067e278>] vp9_free_work_buf+0x54/0x70
[  207.002260] [<ffffffc00067f168>] vdec_vp9_deinit+0x7c/0xe8
[  207.008134] [<ffffffc0006787d8>] vdec_if_deinit+0x84/0xec
[  207.013820] [<ffffffc000677898>] mtk_vcodec_vdec_release+0x54/0x6c
[  207.020672] [<ffffffc000673e3c>] fops_vcodec_release+0x7c/0xf8
[  207.026607] [<ffffffc000652b78>] v4l2_release+0x3c/0x84
[  207.031824] [<ffffffc00033b218>] __fput+0xf8/0x1c0
[  207.036599] [<ffffffc00033b350>] ____fput+0x1c/0x2c
[  207.041454] [<ffffffc00023ed78>] task_work_run+0xb0/0xd4
[  207.046756] [<ffffffc00020872c>] do_notify_resume+0x54/0x6c

From the log I get in this fail case, the size of unmap here is
0x10080, and its map size of dma_common_pages_remap in __iommu_alloc_attrs is 0x10080, and the corresponding dma-map size is 0x11000(after iova_align). I think all the parameters of map and unmap are good, it look like not a DMA issue. but I don't know why we get this warning.
Have you met this problem and give us some advices, Thanks.

(If we add PAGE_ALIGN for the size in dma_alloc and dma_free, It is OK.)

Quoted 7 lines

arch/arm64/mm/dma-mapping.c +6 -0

+	} else {
+		iommu_dma_unmap_page(dev, handle, size, 0, NULL);
+		__free_pages(virt_to_page(cpu_addr), get_order(size));
+	}
+}
+

[...]

Quoted 131 lines · Yong Wu

Quoted 2 lines · 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.

[...]

Quoted 70 lines

arch/arm64/mm/dma-mapping.c +70 -0

+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);
+    int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+    void *addr;
+
+    if (WARN(!dev, "cannot create IOMMU mapping for unknown device\n"))
+        return NULL;
+    /*
+     * Some drivers rely on this, and we probably don't want the
+     * possibility of stale kernel data being read by devices anyway.
+     */
+    gfp |= __GFP_ZERO;
+
+    if (gfp & __GFP_WAIT) {
+        struct page **pages;
+        pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);
+
+        pages = iommu_dma_alloc(dev, size, gfp, ioprot,    handle,
+                    flush_page);
+        if (!pages)
+            return NULL;
+
+        addr = dma_common_pages_remap(pages, size, VM_USERMAP, prot,
+                          __builtin_return_address(0));
+        if (!addr)
+            iommu_dma_free(dev, pages, size, handle);
+    } else {
+        struct page *page;
+        /*
+         * In atomic context we can't remap anything, so we'll only
+         * get the virtually contiguous buffer we need by way of a
+         * physically contiguous allocation.
+         */
+        if (coherent) {
+            page = alloc_pages(gfp, get_order(size));
+            addr = page ? page_address(page) : NULL;
+        } else {
+            addr = __alloc_from_pool(size, &page, gfp);
+        }
+        if (!addr)
+            return NULL;
+
+        *handle = iommu_dma_map_page(dev, page, 0, size, ioprot);
+        if (iommu_dma_mapping_error(dev, *handle)) {
+            if (coherent)
+                __free_pages(page, get_order(size));
+            else
+                __free_from_pool(addr, size);
+            addr = NULL;
+        }
+    }
+    return addr;
+}
+
+static void __iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
+                   dma_addr_t handle, struct dma_attrs *attrs)
+{
+    /*
+     * @cpu_addr will be one of 3 things depending on how it was allocated:
+     * - A remapped array of pages from iommu_dma_alloc(), for all
+     *   non-atomic allocations.
+     * - A non-cacheable alias from the atomic pool, for atomic
+     *   allocations by non-coherent devices.
+     * - A normal lowmem address, for atomic allocations by
+     *   coherent devices.
+     * Hence how dodgy the below logic looks...
+     */

arch/arm64/mm/dma-mapping.c static int __init dma_debug_do_init(void) · line 628

Quoted 10 lines

arch/arm64/mm/dma-mapping.c +10 -0

+    if (__in_atomic_pool(cpu_addr, size)) {
+        iommu_dma_unmap_page(dev, handle, size, 0, NULL);
+        __free_from_pool(cpu_addr, size);
+    } else if (is_vmalloc_addr(cpu_addr)){
+        struct vm_struct *area = find_vm_area(cpu_addr);
+
+        if (WARN_ON(!area || !area->pages))
+            return;
+        iommu_dma_free(dev, area->pages, size, &handle);
+        dma_common_free_remap(cpu_addr, size, VM_USERMAP);

Hi Robin,
We get a WARN issue while the size is not aligned here.

The WARN log is:

[  206.852002] WARNING: CPU: 0 PID: 23329
at /mnt/host/source/src/third_party/kernel/v3.18/mm/vmalloc.c:65
vunmap_page_range+0x190/0x1b4()
[  206.864438] Modules linked in: nls_iso8859_1 nls_cp437 vfat fat

rfcomm i2c_dev uinput dm9601 uvcvideo btmrvl_sdio mwifiex_sdio mwifiex btmrvl bluetooth zram fuse cfg80211 nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables cdc_ether usbnet mii joydev snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device ppp_async ppp_generic slhc tun

[  206.902983] CPU: 0 PID: 23329 Comm: chrome Not tainted 3.18.0 #17
[  206.910430] Hardware name: Mediatek Oak rev3 board (DT)
[  206.920018] Call trace:
[  206.925537] [<ffffffc000208c00>] dump_backtrace+0x0/0x140
[  206.931905] [<ffffffc000208d5c>] show_stack+0x1c/0x28
[  206.939158] [<ffffffc000870f80>] dump_stack+0x74/0x94
[  206.947459] [<ffffffc0002219a4>] warn_slowpath_common+0x90/0xb8
[  206.954100] [<ffffffc000221b58>] warn_slowpath_null+0x34/0x44
[  206.961537] [<ffffffc000321358>] vunmap_page_range+0x18c/0x1b4
[  206.967630] [<ffffffc0003213e4>] unmap_kernel_range+0x2c/0x78
[  206.976977] [<ffffffc000582224>] dma_common_free_remap+0x68/0x80
[  206.983581] [<ffffffc000217260>] __iommu_free_attrs+0x14c/0x160
[  206.989646] [<ffffffc00066fc1c>] mtk_vcodec_mem_free+0xa0/0x15c
[  206.996481] [<ffffffc00067e278>] vp9_free_work_buf+0x54/0x70
[  207.002260] [<ffffffc00067f168>] vdec_vp9_deinit+0x7c/0xe8
[  207.008134] [<ffffffc0006787d8>] vdec_if_deinit+0x84/0xec
[  207.013820] [<ffffffc000677898>] mtk_vcodec_vdec_release+0x54/0x6c
[  207.020672] [<ffffffc000673e3c>] fops_vcodec_release+0x7c/0xf8
[  207.026607] [<ffffffc000652b78>] v4l2_release+0x3c/0x84
[  207.031824] [<ffffffc00033b218>] __fput+0xf8/0x1c0
[  207.036599] [<ffffffc00033b350>] ____fput+0x1c/0x2c
[  207.041454] [<ffffffc00023ed78>] task_work_run+0xb0/0xd4
[  207.046756] [<ffffffc00020872c>] do_notify_resume+0x54/0x6c

From the log I get in this fail case, the size of unmap here is
0x10080, and its map size of dma_common_pages_remap in __iommu_alloc_attrs is 0x10080, and the corresponding dma-map size is 0x11000(after iova_align). I think all the parameters of map and unmap are good, it look like not a DMA issue. but I don't know why we get this warning.
Have you met this problem and give us some advices, Thanks.

Quoted 1 line

(If we add PAGE_ALIGN for the size in dma_alloc and dma_free, It is OK.)

OK, having dug into this it looks like the root cause comes from some asymmetry in the common code: dma_common_pages remap() just passes the size through to get_vm_area_caller(), and the first thing that does is to page-align it. On the other hand, neither dma_common_free_remap() nor unmap_kernel_range() does anything with the size, so we wind up giving an unaligned end address to vunmap_page_range() and messing up the vmalloc page tables.

I wonder if dma_common_free_remap() should be page-aligning the size to match expectations (i.e. make it correctly unmap any request the other functions happily mapped), or conversely, perhaps both the map and unmap functions should have a WARN_ON(size & PAGE_MASK) to enforce being called as actually intended. Laura?

Either way, I'll send out a patch to make the arm64 side deal with it explicitly.

Robin.

Hi all,

Here's the latest, and hopefully last, revision of the initial arm64 IOMMU dma_ops support.

There are a couple of dependencies still currently in -next and the intel-iommu tree[0]: "iommu: iova: Move iova cache management to the iova library" is necessary for the rename of iova_cache_get(), and "iommu/iova: Avoid over-allocating when size-aligned" will be needed with some IOMMU drivers to prevent unmapping errors.

Changes from v5[1]:
- Change __iommu_dma_unmap() from BUG to WARN when things go wrong, and prevent a NULL dereference on double-free.
- Fix iommu_dma_map_sg() to ensure segments can never inadvertently end mapped across a segment boundary. As a result, we have to lose the segment-merging optimisation from before (I might revisit that if there's some evidence it's really worthwhile, though).
- Cleaned up the platform device workarounds for config order and default domains, and removed the other hacks. Demanding that the IOMMU drivers assign groups, and support IOMMU_DOMAIN_DMA via the methods provided, keeps things bearable, and the behaviour should now be consistent across all cases.

As a bonus, whilst the underlying of_iommu_configure() code only supports platform devices at the moment, I can also say that this has now been tested to work for PCI devices too, via some horrible hacks on a Juno r1.

Thanks,
Robin.

[0]:http://thread.gmane.org/gmane.linux.kernel.iommu/11033 [1]:http://thread.gmane.org/gmane.linux.kernel.iommu/10439

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/dma-mapping.h |  15 +-
  arch/arm64/mm/dma-mapping.c          | 457 ++++++++++++++++++++++++++++++
  drivers/iommu/Kconfig                |   7 +
  drivers/iommu/Makefile               |   1 +
  drivers/iommu/dma-iommu.c            | 524 +++++++++++++++++++++++++++++++++++
  include/linux/dma-iommu.h            |  85 ++++++
  include/linux/iommu.h                |   1 +
  8 files changed, 1083 insertions(+), 8 deletions(-)

create mode 100644 drivers/iommu/dma-iommu.c
create mode 100644 include/linux/dma-iommu.h

Anyway, what are your thoughts on taking this for 4.4? Since the dependencies are now in and we're at -rc5 already, I'm on the verge of reposting a self-contained version to go through arm64, as we really need to unblock all the follow-on development there (it's a shame that of the people I'm aware want this, it's only me and the Mediatek/Chrome guys here on the list saying so).

Hi Robin,

Quoted 6 lines · Robin Murphy

Anyway, what are your thoughts on taking this for 4.4? Since the dependencies are now in and we're at -rc5 already, I'm on the verge of reposting a self-contained version to go through arm64, as we really need to unblock all the follow-on development there (it's a shame that of the people I'm aware want this, it's only me and the Mediatek/Chrome guys here on the list saying so).

I plan to take it for 4.4, given the missing ack arrives. What are your plans to resolve the remaining issues, like with the probe deferral patch-set?

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/dma-mapping.h |  15 +-
 arch/arm64/mm/dma-mapping.c          | 457 ++++++++++++++++++++++++++++++
 drivers/iommu/Kconfig                |   7 +
 drivers/iommu/Makefile               |   1 +
 drivers/iommu/dma-iommu.c            | 524 +++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h            |  85 ++++++
 include/linux/iommu.h                |   1 +
 8 files changed, 1083 insertions(+), 8 deletions(-)
 create mode 100644 drivers/iommu/dma-iommu.c
 create mode 100644 include/linux/dma-iommu.h