arm64: mm: Fix cache maintenance for non-coherent streaming DMA

Hi all,

This pair of patches follows-up on the discussion we had on linux-arch earlier this week and addresses the reported problem for arm64 only.

Other architectures should be able to do something similar if they wish, but this all came out of discussion and code inspection rather than a concrete failure.

Cheers,

Will

--->8

Will Deacon (2):
arm64: mm: Don't invalidate FROM_DEVICE buffers at start of DMA
transfer
arm64: mm: Remove assembly DMA cache maintenance wrappers

 arch/arm64/include/asm/cacheflush.h |  7 -----
 arch/arm64/mm/cache.S               | 43 -----------------------------
 arch/arm64/mm/dma-mapping.c         | 19 +++++++++----
 3 files changed, 14 insertions(+), 55 deletions(-)

2.36.1.476.g0c4daa206d-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Invalidating the buffer memory in arch_sync_dma_for_device() for FROM_DEVICE transfers

When using the streaming DMA API to map a buffer prior to inbound non-coherent DMA (i.e. DMA_FROM_DEVICE), we invalidate any dirty CPU cachelines so that they will not be written back during the transfer and corrupt the buffer contents written by the DMA. This, however, poses two potential problems:

(1) If the DMA transfer does not write to every byte in the buffer, then the unwritten bytes will contain stale data once the transfer has completed.

(2) If the buffer has a virtual alias in userspace, then stale data may be visible via this alias during the period between performing the cache invalidation and the DMA writes landing in memory.

Address both of these issues by cleaning (aka writing-back) the dirty lines in arch_sync_dma_for_device(DMA_FROM_DEVICE) instead of discarding them using invalidation.

---
 arch/arm64/mm/cache.S | 2 --
 1 file changed, 2 deletions(-)

index 0ea6cc25dc66..21c907987080 100644
@@ -218,8 +218,6 @@ SYM_FUNC_ALIAS(__dma_flush_area, __pi___dma_flush_area)
  */
 SYM_FUNC_START(__pi___dma_map_area)
 	add	x1, x0, x1
-	cmp	w2, #DMA_FROM_DEVICE
-	b.eq	__pi_dcache_inval_poc
 	b	__pi_dcache_clean_poc
 SYM_FUNC_END(__pi___dma_map_area)
 SYM_FUNC_ALIAS(__dma_map_area, __pi___dma_map_area)

2.36.1.476.g0c4daa206d-goog

Remove the __dma_{flush,map,unmap}_area assembly wrappers and call the appropriate cache maintenance functions directly from the DMA mapping callbacks.

---
 arch/arm64/include/asm/cacheflush.h |  7 -----
 arch/arm64/mm/cache.S               | 41 -----------------------------
 arch/arm64/mm/dma-mapping.c         | 19 +++++++++----
 3 files changed, 14 insertions(+), 53 deletions(-)

index 5a228e203ef9..37185e978aeb 100644
@@ -104,13 +104,6 @@ static inline void flush_icache_range(unsigned long start, unsigned long end)
 }
 #define flush_icache_range flush_icache_range
 
-/*
- * Cache maintenance functions used by the DMA API. No to be used directly.
- */
-extern void __dma_map_area(const void *, size_t, int);
-extern void __dma_unmap_area(const void *, size_t, int);
-extern void __dma_flush_area(const void *, size_t);
-
 /*
  * Copy user data from/to a page which is mapped into a different
  * processes address space.  Really, we want to allow our "user

index 6719f9efea09..df0c488ae643 100644
@@ -14,20 +14,29 @@
 #include <asm/cacheflush.h>
 
 void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
-		enum dma_data_direction dir)
+			      enum dma_data_direction dir)
 {
-	__dma_map_area(phys_to_virt(paddr), size, dir);
+	unsigned long start = (unsigned long)phys_to_virt(paddr);
+
+	dcache_clean_poc(start, start + size);
 }
 
 void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
-		enum dma_data_direction dir)
+			   enum dma_data_direction dir)
 {
-	__dma_unmap_area(phys_to_virt(paddr), size, dir);
+	unsigned long start = (unsigned long)phys_to_virt(paddr);
+
+	if (dir == DMA_TO_DEVICE)
+		return;
+
+	dcache_inval_poc(start, start + size);
 }
 
 void arch_dma_prep_coherent(struct page *page, size_t size)
 {
-	__dma_flush_area(page_address(page), size);
+	unsigned long start = (unsigned long)page_address(page);
+
+	dcache_clean_inval_poc(start, start + size);
 }
 
 #ifdef CONFIG_IOMMU_DMA

2.36.1.476.g0c4daa206d-goog


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Thanks, this looks so much nicer. Is there any good reason why the C prototypes for the assembly function take an unsigned long instead of a void *, though? It seems like all or almost all of the callers have a pointer at hand an do silly casts for the assembly code that could work perfectly fine with a pointer and length.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

(I'll queue the first patch as a fix)

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Applied to arm64 (for-next/fixes), thanks!

[1/2] arm64: mm: Don't invalidate FROM_DEVICE buffers at start of DMA transfer

      https://git.kernel.org/arm64/c/c50f11c6196f

Catalin


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel