DMA Atomic pool for arm64

One of the more common algorithms used for allocation is to align the start address of the allocation to the order of size requested. Add this as an algorithm option for genalloc.

Signed-off-by Laura Abbott <[email protected]>

---
 include/linux/genalloc.h |  4 ++++
 lib/genalloc.c           | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

include/linux/genalloc.h +0 -0

index 1c2fdaa..3cd0934 100644

Quoted 20 lines · Laura Abbott

One of the more common algorithms used for allocation is to align the start address of the allocation to the order of size requested. Add this as an algorithm option for genalloc.

Signed-off-by Laura Abbott <[email protected]>

---
 include/linux/genalloc.h |  4 ++++
 lib/genalloc.c           | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

include/linux/genalloc.h +0 -0

index 1c2fdaa..3cd0934 100644
@@ -110,6 +110,10 @@ extern void gen_pool_set_algo(struct gen_pool *pool, genpool_algo_t algo,
 extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
              unsigned long start, unsigned int nr, void *data);
 

include/linux/genalloc.h extern void gen_pool_set_algo(struct gen_pool *pool, genpool_algo_t algo, · line 116

Quoted 4 lines

include/linux/genalloc.h +4 -0

+extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
+             unsigned long size, unsigned long start, unsigned int nr,
+             void *data);
+

You could also update gen_pool_first_fit to call this new function instead.

After allocating an address from a particular genpool, there is no good way to verify if that address actually belongs to a genpool. Introduce addr_in_gen_pool which will return if an address plus size falls completely within the genpool range.

Signed-off-by Laura Abbott <[email protected]>

---

After allocating an address from a particular genpool, there is no good way to verify if that address actually belongs to a genpool. Introduce addr_in_gen_pool which will return if an address plus size falls completely within the genpool range.

Signed-off-by Laura Abbott <[email protected]>

Quoted 7 lines · Laura Abbott

After allocating an address from a particular genpool, there is no good way to verify if that address actually belongs to a genpool. Introduce addr_in_gen_pool which will return if an address plus size falls completely within the genpool range.

Quoted 1 line

Signed-off-by Laura Abbott <[email protected]>

Reviewed-by Olof Johansson <[email protected]>

What's the merge path for this code? Part of the arm64 code that needs it, I presume?

Quoted 10 lines · Olof Johansson

Quoted 8 lines · Laura Abbott

After allocating an address from a particular genpool, there is no good way to verify if that address actually belongs to a genpool. Introduce addr_in_gen_pool which will return if an address plus size falls completely within the genpool range.

Signed-off-by Laura Abbott <[email protected]>

Quoted 4 lines

Reviewed-by Olof Johansson <[email protected]>

What's the merge path for this code? Part of the arm64 code that needs it, I presume?

My plan was to have the entire series go through the arm64 tree unless someone has a better idea.

For architectures without coherent DMA, memory for DMA may need to be remapped with coherent attributes. Factor out the the remapping code from arm and put it in a common location to reduced code duplication.

Signed-off-by Laura Abbott <[email protected]>

Quoted 6 lines · Laura Abbott

For architectures without coherent DMA, memory for DMA may need to be remapped with coherent attributes. Factor out the the remapping code from arm and put it in a common location to reduced code duplication.

Signed-off-by Laura Abbott <[email protected]>

Hm. The switch from ioremap to map_vm_area() here seems to imply that lib/ioremap can/should be reworked to use just wrap the vmalloc functions too?

drivers/base/dma-mapping.c +7 -0

+void *dma_common_pages_remap(struct page **pages, size_t size,
+			unsigned long vm_flags, pgprot_t prot,
+			const void *caller)
+{
+	struct vm_struct *area;
+
+	area = get_vm_area_caller(size, vm_flags, caller);

Quoted 11 lines · Laura Abbott

drivers/base/dma-mapping.c +11 -0

+void *dma_common_pages_remap(struct page **pages, size_t size,
+			unsigned long vm_flags, pgprot_t prot,
+			const void *caller)
+{
+	struct vm_struct *area;
+
+	area = get_vm_area_caller(size, vm_flags, caller);
+	if (!area)
+		return NULL;
+
+	if (map_vm_area(area, prot, &pages)) {

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 320

Quoted 6 lines

drivers/base/dma-mapping.c +6 -0

+		vunmap(area->addr);
+		return NULL;
+	}
+
+	return area->addr;
+}

Why not just replace this function with vmap()? It is nearly identical.

Quoted 12 lines · Catalin Marinas

Quoted 11 lines · Laura Abbott

drivers/base/dma-mapping.c +11 -0

+void *dma_common_pages_remap(struct page **pages, size_t size,
+			unsigned long vm_flags, pgprot_t prot,
+			const void *caller)
+{
+	struct vm_struct *area;
+
+	area = get_vm_area_caller(size, vm_flags, caller);
+	if (!area)
+		return NULL;
+
+	if (map_vm_area(area, prot, &pages)) {

Quoted 8 lines

drivers/base/dma-mapping.c int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, · line 320

Quoted 6 lines

drivers/base/dma-mapping.c +6 -0

+		vunmap(area->addr);
+		return NULL;
+	}
+
+	return area->addr;
+}

Why not just replace this function with vmap()? It is nearly identical.

With this version, the caller stored and printed via /proc/vmallocinfo is the actual caller of the DMA API whereas if we just call vmap we don't get any useful caller information. Going to vmap would change the existing behavior on ARM so it seems unwise to switch.

Another option is to move this into vmalloc.c and add vmap_caller.

arch/arm/mm/dma-mapping.c +0 -0

On Wed, Jul 02, 2014 at 11:03:37AM -0700, Laura Abbott wrote:
[...]

Quoted 1 line

arch/arm/mm/dma-mapping.c +0 -0

[...]

arch/arm/mm/dma-mapping.c line 29

Quoted 8 lines

index f5190ac..02a1939 100644

arch/arm/mm/dma-mapping.c +1 -0

@@ -26,6 +26,7 @@
 #include <linux/io.h>
 #include <linux/vmalloc.h>
 #include <linux/sizes.h>
+#include <linux/genalloc.h>

Includes should be sorted alphabetically. I realize that's not the case for this particular file, but the downside of that is that your patch no longer applies cleanly on top of linux-next because some other patch did add linux/cma.h at the same location.

Quoted 8 lines

arch/arm/mm/dma-mapping.c +1 -1

 static int __init early_coherent_pool(char *p)
 {
-	atomic_pool.size = memparse(p, &p);
+	atomic_pool_size = memparse(p, &p);

return 0;
}
early_param("coherent_pool", early_coherent_pool);

Quoted 1 line

+

There's a gratuituous blank line her.

I also need the below hunk on top of you patch to make this compile on ARM.

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

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

 		struct page *page;
 
 		size = PAGE_ALIGN(size);

@@ -73,50 +124,56 @@ static void __dma_free_coherent(struct device *dev, size_t size,
 				void *vaddr, dma_addr_t dma_handle,
 				struct dma_attrs *attrs)
 {
+	bool freed;
+	phys_addr_t paddr = dma_to_phys(dev, dma_handle);

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

+	if (page) {
+		int ret;
+
+		atomic_pool = gen_pool_create(PAGE_SHIFT, -1);
+		if (!atomic_pool)
+			goto free_page;
+
+		addr = dma_common_contiguous_remap(page, atomic_pool_size,
+					VM_USERMAP, prot, atomic_pool_init);
+
+		if (!addr)
+			goto destroy_genpool;
+
+		memset(addr, 0, atomic_pool_size);
+		__dma_flush_range(addr, addr + atomic_pool_size);
+

On Wed, Jul 02, 2014 at 11:03:38AM -0700, Laura Abbott wrote:
[...]

Quoted 1 line

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

[...]

arch/arm64/mm/dma-mapping.c static pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot, · line 48

Quoted 4 lines

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

+static struct gen_pool *atomic_pool;
+
+#define DEFAULT_DMA_COHERENT_POOL_SIZE  SZ_256K
+static size_t atomic_pool_size = DEFAULT_DMA_COHERENT_POOL_SIZE;

There doesn't seem to be much use for this since it can't be overridden via init_dma_coherent_pool_size like on ARM.

Quoted 21 lines

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

+	if (page) {
+		int ret;
+
+		atomic_pool = gen_pool_create(PAGE_SHIFT, -1);
+		if (!atomic_pool)
+			goto free_page;
+
+		addr = dma_common_contiguous_remap(page, atomic_pool_size,
+					VM_USERMAP, prot, atomic_pool_init);
+
+		if (!addr)
+			goto destroy_genpool;
+
+		memset(addr, 0, atomic_pool_size);
+		__dma_flush_range(addr, addr + atomic_pool_size);
+
+		ret = gen_pool_add_virt(atomic_pool, (unsigned long)addr,
+					page_to_phys(page),
+					atomic_pool_size, -1);
+		if (ret)
+			goto remove_mapping;

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 434

Quoted 5 lines

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

+
+		gen_pool_set_algo(atomic_pool,
+				  gen_pool_first_fit_order_align, NULL);
+
+		pr_info("DMA: preallocated %zd KiB pool for atomic allocations\n",

I think this should be "%zu" because atomic_pool_size is a size_t, not a ssize_t.

Quoted 4 lines

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

+			atomic_pool_size / 1024);
+		return 0;
+	}
+	goto out;

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 444

Quoted 6 lines

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

+
+remove_mapping:
+	dma_common_free_remap(addr, atomic_pool_size, VM_USERMAP);
+destroy_genpool:
+	gen_pool_destroy(atomic_pool);
+	atomic_pool == NULL;

This probably doesn't belong here.

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 447

Quoted 3 lines

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

+free_page:
+	if (!dma_release_from_contiguous(NULL, page, nr_pages))
+		__free_pages(page, get_order(atomic_pool_size));

You use get_order(atomic_pool_size) a lot, perhaps it should be a temporary variable?

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 450

Quoted 3 lines

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

+out:
+	pr_err("DMA: failed to allocate %zx KiB pool for atomic coherent allocation\n",
+		atomic_pool_size / 1024);

Print in decimal rather than hexadecimal?

Thierry

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

index 4164c5a..a2487f1 100644

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

@@ -73,50 +124,56 @@ static void __dma_free_coherent(struct device *dev, size_t size,
 				void *vaddr, dma_addr_t dma_handle,
 				struct dma_attrs *attrs)
 {
+	bool freed;
+	phys_addr_t paddr = dma_to_phys(dev, dma_handle);
+
 	if (dev == NULL) {
 		WARN_ONCE(1, "Use an actual device structure for DMA allocation\n");
 		return;
 	}
 
-	if (IS_ENABLED(CONFIG_DMA_CMA)) {
-		phys_addr_t paddr = dma_to_phys(dev, dma_handle);

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

 static void *__dma_alloc_noncoherent(struct device *dev, size_t size,
 				     dma_addr_t *dma_handle, gfp_t flags,
 				     struct dma_attrs *attrs)
 {
-	struct page *page, **map;
+	struct page *page;
 	void *ptr, *coherent_ptr;
-	int order, i;
 
 	size = PAGE_ALIGN(size);
-	order = get_order(size);
+

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

+	if (page) {
+		int ret;
+
+		atomic_pool = gen_pool_create(PAGE_SHIFT, -1);
+		if (!atomic_pool)

Quoted 4 lines · Laura Abbott

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

index 4164c5a..a2487f1 100644

[...]

arch/arm64/mm/dma-mapping.c static void *__dma_alloc_coherent(struct device *dev, size_t size, · line 110

Quoted 13 lines

static void *__dma_alloc_coherent(struct device *dev, size_t size,

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

 				  dma_addr_t *dma_handle, gfp_t flags,
 				  struct dma_attrs *attrs)

@@ -53,7 +103,8 @@ static void *__dma_alloc_coherent(struct device *dev, size_t size,
 	if (IS_ENABLED(CONFIG_ZONE_DMA) &&
 	    dev->coherent_dma_mask <= DMA_BIT_MASK(32))
 		flags |= GFP_DMA;
-	if (IS_ENABLED(CONFIG_DMA_CMA)) {
+
+	if (!(flags & __GFP_WAIT) && IS_ENABLED(CONFIG_DMA_CMA)) {
 		struct page *page;
 
 		size = PAGE_ALIGN(size);

I think that's the wrong condition here. You want to use CMA if (flags & __GFP_WAIT). CMA does not support atomic allocations so it can fall back to swiotlb_alloc_coherent().

Quoted 14 lines

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

@@ -73,50 +124,56 @@ static void __dma_free_coherent(struct device *dev, size_t size,
 				void *vaddr, dma_addr_t dma_handle,
 				struct dma_attrs *attrs)
 {
+	bool freed;
+	phys_addr_t paddr = dma_to_phys(dev, dma_handle);
+
 	if (dev == NULL) {
 		WARN_ONCE(1, "Use an actual device structure for DMA allocation\n");
 		return;
 	}
 
-	if (IS_ENABLED(CONFIG_DMA_CMA)) {
-		phys_addr_t paddr = dma_to_phys(dev, dma_handle);

arch/arm64/mm/dma-mapping.c static void __dma_free_coherent(struct device *dev, size_t size, · line 141

Quoted 10 lines

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

 
-		dma_release_from_contiguous(dev,
+	freed = dma_release_from_contiguous(dev,
 					phys_to_page(paddr),
 					size >> PAGE_SHIFT);
-	} else {
+	if (!freed)
 		swiotlb_free_coherent(dev, size, vaddr, dma_handle);
-	}
 }

Is __dma_free_coherent() ever called in atomic context? If yes, the dma_release_from_contiguous() may not like it since it tries to acquire a mutex. But since we don't have the gfp flags here, we don't have an easy way to know what to call.

So the initial idea of always calling __alloc_from_pool() for both coherent/non-coherent cases would work better (but still with a single shared pool, see below).

Quoted 12 lines

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

 static void *__dma_alloc_noncoherent(struct device *dev, size_t size,
 				     dma_addr_t *dma_handle, gfp_t flags,
 				     struct dma_attrs *attrs)
 {
-	struct page *page, **map;
+	struct page *page;
 	void *ptr, *coherent_ptr;
-	int order, i;
 
 	size = PAGE_ALIGN(size);
-	order = get_order(size);
+

arch/arm64/mm/dma-mapping.c static void __dma_free_coherent(struct device *dev, size_t size, · line 161

Quoted 10 lines

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

+	if (!(flags & __GFP_WAIT)) {
+		struct page *page = NULL;
+		void *addr = __alloc_from_pool(size, &page);
+
+		if (addr)
+			*dma_handle = phys_to_dma(dev, page_to_phys(page));
+
+		return addr;
+
+	}

If we do the above for the __dma_alloc_coherent() case, we could use the same pool but instead of returning addr it could just return page_address(page). The downside of sharing the pool is that you need cache flushing for every allocation (which we already do for the non-atomic case).

Quoted 4 lines

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

@@ -332,6 +391,67 @@ static struct notifier_block amba_bus_nb = {
 
 extern int swiotlb_late_init_with_default_size(size_t default_size);
 

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 402

Quoted 9 lines

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

+static int __init atomic_pool_init(void)
+{
+	pgprot_t prot = __pgprot(PROT_NORMAL_NC);
+	unsigned long nr_pages = atomic_pool_size >> PAGE_SHIFT;
+	struct page *page;
+	void *addr;
+
+
+	if (dev_get_cma_area(NULL))

Is it worth using this condition for other places where we check IS_ENABLED(CONFIG_DMA_CMA) (maybe as a separate patch).

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 406

Quoted 4 lines

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

+		page = dma_alloc_from_contiguous(NULL, nr_pages,
+					get_order(atomic_pool_size));
+	else
+		page = alloc_pages(GFP_KERNEL, get_order(atomic_pool_size));

One problem here is that the atomic pool wouldn't be able to honour GFP_DMA (in the latest kernel, CMA is by default in ZONE_DMA). You should probably pass GFP_KERNEL|GFP_DMA here. You could also use the swiotlb_alloc_coherent() which, with a NULL dev, assumes 32-bit DMA mask but it still expects GFP_DMA to be passed.

Quoted 5 lines

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

+	if (page) {
+		int ret;
+
+		atomic_pool = gen_pool_create(PAGE_SHIFT, -1);
+		if (!atomic_pool)

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 423

Quoted 10 lines

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

+			goto free_page;
+
+		addr = dma_common_contiguous_remap(page, atomic_pool_size,
+					VM_USERMAP, prot, atomic_pool_init);
+
+		if (!addr)
+			goto destroy_genpool;
+
+		memset(addr, 0, atomic_pool_size);
+		__dma_flush_range(addr, addr + atomic_pool_size);

If you add the flushing in the __dma_alloc_noncoherent(), it won't be needed here (of course, more efficient here but it would not work if we share the pool).

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 453

Quoted 1 line

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

+postcore_initcall(atomic_pool_init);

Why not arch_initcall? Or even better, we could have a common DMA init function that calls swiotlb_late_init() and atomic_pool_init() (in this order if you decide to use swiotlb allocation above).

Quoted 31 lines · Catalin Marinas

Quoted 14 lines · Laura Abbott

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

@@ -73,50 +124,56 @@ static void __dma_free_coherent(struct device *dev, size_t size,
 				void *vaddr, dma_addr_t dma_handle,
 				struct dma_attrs *attrs)
 {
+	bool freed;
+	phys_addr_t paddr = dma_to_phys(dev, dma_handle);
+
 	if (dev == NULL) {
 		WARN_ONCE(1, "Use an actual device structure for DMA allocation\n");
 		return;
 	}
 
-	if (IS_ENABLED(CONFIG_DMA_CMA)) {
-		phys_addr_t paddr = dma_to_phys(dev, dma_handle);

arch/arm64/mm/dma-mapping.c static void __dma_free_coherent(struct device *dev, size_t size, · line 141

Quoted 10 lines

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

 
-		dma_release_from_contiguous(dev,
+	freed = dma_release_from_contiguous(dev,
 					phys_to_page(paddr),
 					size >> PAGE_SHIFT);
-	} else {
+	if (!freed)
 		swiotlb_free_coherent(dev, size, vaddr, dma_handle);
-	}
 }

Is __dma_free_coherent() ever called in atomic context? If yes, the dma_release_from_contiguous() may not like it since it tries to acquire a mutex. But since we don't have the gfp flags here, we don't have an easy way to know what to call.

Quoted 3 lines

So the initial idea of always calling __alloc_from_pool() for both coherent/non-coherent cases would work better (but still with a single shared pool, see below).

We should be okay

__dma_free_coherent -> dma_release_from_contiguous -> cma_release which bounds checks the CMA region before taking any mutexes unless I missed something.

The existing behavior on arm is to not allow non-atomic allocations to be freed atomic context when CMA is enabled so we'd be giving arm64 more leeway there. Is being able to free non-atomic allocations in atomic context really necessary?

Quoted 11 lines

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 406

Quoted 4 lines

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

+		page = dma_alloc_from_contiguous(NULL, nr_pages,
+					get_order(atomic_pool_size));
+	else
+		page = alloc_pages(GFP_KERNEL, get_order(atomic_pool_size));

One problem here is that the atomic pool wouldn't be able to honour GFP_DMA (in the latest kernel, CMA is by default in ZONE_DMA). You should probably pass GFP_KERNEL|GFP_DMA here. You could also use the swiotlb_alloc_coherent() which, with a NULL dev, assumes 32-bit DMA mask but it still expects GFP_DMA to be passed.

I think I missed updating this to GFP_DMA. The only advantage I would see to using swiotlb_alloc_coherent vs. alloc_pages directly would be to allow the fallback to using a bounce buffer if __get_free_pages failed. I'll keep this as alloc_pages for now; it can be changed later if there is a particular need for swiotlb behavior.

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

+       pgprot_t prot = __pgprot(PROT_NORMAL_NC);
+       unsigned long nr_pages = atomic_pool_size >> PAGE_SHIFT;
+       struct page *page;
+       void *addr;
+
+
+       if (dev_get_cma_area(NULL))
+               page = dma_alloc_from_contiguous(NULL, nr_pages,
+                                       get_order(atomic_pool_size));
+       else
+               page = alloc_pages(GFP_KERNEL, get_order(atomic_pool_size));
+
+

Quoted 22 lines · Laura Abbott

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

+       pgprot_t prot = __pgprot(PROT_NORMAL_NC);
+       unsigned long nr_pages = atomic_pool_size >> PAGE_SHIFT;
+       struct page *page;
+       void *addr;
+
+
+       if (dev_get_cma_area(NULL))
+               page = dma_alloc_from_contiguous(NULL, nr_pages,
+                                       get_order(atomic_pool_size));
+       else
+               page = alloc_pages(GFP_KERNEL, get_order(atomic_pool_size));
+
+
+       if (page) {
+               int ret;
+
+               atomic_pool = gen_pool_create(PAGE_SHIFT, -1);
+               if (!atomic_pool)
+                       goto free_page;
+
+               addr = dma_common_contiguous_remap(page, atomic_pool_size,
+                                       VM_USERMAP, prot, atomic_pool_init);

Quoted 1 line

+

I just stumbled over this thread and noticed the code here: When you do alloc_pages() above, you actually get pages that are already mapped into the linear kernel mapping as cacheable pages. Your new dma_common_contiguous_remap tries to map them as noncacheable. This seems broken because it allows the CPU to treat both mappings as cacheable, and that won't be coherent with device DMA.

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 423

Quoted 5 lines

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

+               if (!addr)
+                       goto destroy_genpool;
+
+               memset(addr, 0, atomic_pool_size);
+               __dma_flush_range(addr, addr + atomic_pool_size);

It also seems weird to flush the cache on a virtual address of an uncacheable mapping. Is that well-defined?

In the CMA case, the
original mapping should already be uncached here, so you don't need to flush it.

In the alloc_pages() case, I think you need to unmap the pages from the linear mapping instead.

On Tue, Jul 22, 2014 at 07:06:44PM +0100, Arnd Bergmann wrote:

Quoted 8 lines

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 423

Quoted 5 lines

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

+               if (!addr)
+                       goto destroy_genpool;
+
+               memset(addr, 0, atomic_pool_size);
+               __dma_flush_range(addr, addr + atomic_pool_size);

It also seems weird to flush the cache on a virtual address of an uncacheable mapping. Is that well-defined?

Yes. According to D5.8.1 (Data and unified caches), "if cache maintenance is performed on a memory location, the effect of that cache maintenance is visible to all aliases of that physical memory location. These properties are consistent with implementing all caches that can handle data accesses as Physically-indexed, physically-tagged (PIPT) caches".

Quoted 3 lines

In the CMA case, the
original mapping should already be uncached here, so you don't need to flush it.

I don't think it is non-cacheable already, at least not for arm64 (CMA can be used on coherent architectures as well).

Quoted 2 lines

In the alloc_pages() case, I think you need to unmap the pages from the linear mapping instead.

Even if unmapped, it would not remove dirty cache lines (which are associated with physical addresses anyway). But we don't need to worry about unmapping anyway, see above (that's unless we find some architecture implementation where having such cacheable/non-cacheable aliases is not efficient enough, the efficiency is not guaranteed by the ARM ARM, just the correct behaviour).

Quoted 23 lines · Arnd Bergmann

Quoted 22 lines · Laura Abbott

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

+       pgprot_t prot = __pgprot(PROT_NORMAL_NC);
+       unsigned long nr_pages = atomic_pool_size >> PAGE_SHIFT;
+       struct page *page;
+       void *addr;
+
+
+       if (dev_get_cma_area(NULL))
+               page = dma_alloc_from_contiguous(NULL, nr_pages,
+                                       get_order(atomic_pool_size));
+       else
+               page = alloc_pages(GFP_KERNEL, get_order(atomic_pool_size));
+
+
+       if (page) {
+               int ret;
+
+               atomic_pool = gen_pool_create(PAGE_SHIFT, -1);
+               if (!atomic_pool)
+                       goto free_page;
+
+               addr = dma_common_contiguous_remap(page, atomic_pool_size,
+                                       VM_USERMAP, prot, atomic_pool_init);

Quoted 8 lines

Quoted 1 line

+

I just stumbled over this thread and noticed the code here: When you do alloc_pages() above, you actually get pages that are already mapped into the linear kernel mapping as cacheable pages. Your new dma_common_contiguous_remap tries to map them as noncacheable. This seems broken because it allows the CPU to treat both mappings as cacheable, and that won't be coherent with device DMA.

It does *not* allow the CPU to treat both as cacheable. It treats the non-cacheable mapping as non-cacheable (and the cacheable one as cacheable). The only requirements the ARM ARM makes in this situation (B2.9 point 5 in the ARMv8 ARM):

- Before writing to a location not using the Write-Back attribute, software must invalidate, or clean, a location from the caches if any agent might have written to the location with the Write-Back attribute. This avoids the possibility of overwriting the location with stale data.
- After writing to a location with the Write-Back attribute, software must clean the location from the caches, to make the write visible to external memory.
- Before reading the location with a cacheable attribute, software must invalidate the location from the caches, to ensure that any value held in the caches reflects the last value made visible in external memory.

So we as long as the CPU accesses such memory only via the non-cacheable mapping, the only requirement is to flush the cache so that there are no dirty lines that could be evicted.

(if the mismatched attributes were for example Normal vs Device, the Device guarantees would be lost but in the cacheable vs non-cacheable it's not too bad; same for ARMv7).

Quoted 8 lines

arch/arm64/mm/dma-mapping.c static struct notifier_block amba_bus_nb = { · line 423

Quoted 5 lines

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

+               if (!addr)
+                       goto destroy_genpool;
+
+               memset(addr, 0, atomic_pool_size);
+               __dma_flush_range(addr, addr + atomic_pool_size);

It also seems weird to flush the cache on a virtual address of an uncacheable mapping. Is that well-defined?

Yes. According to D5.8.1 (Data and unified caches), "if cache maintenance is performed on a memory location, the effect of that cache maintenance is visible to all aliases of that physical memory location. These properties are consistent with implementing all caches that can handle data accesses as Physically-indexed, physically-tagged (PIPT) caches".

Quoted 3 lines

In the CMA case, the
original mapping should already be uncached here, so you don't need to flush it.

I don't think it is non-cacheable already, at least not for arm64 (CMA can be used on coherent architectures as well).