[PATCH] sparc32: Page align size in arch_dma_alloc

[PATCH] sparc32: Page align size in arch_dma_alloc

[Andreas Larsson]

Commit 53b7670e5735 ("sparc: factor the dma coherent mapping into
helper") lost the page align for the calls to dma_make_coherent and
srmmu_unmapiorange. The latter cannot handle a non page aligned len
argument.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 arch/sparc/kernel/ioport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
index 8e1d72a16759..7ceae24b0ca9 100644
--- a/arch/sparc/kernel/ioport.c
+++ b/arch/sparc/kernel/ioport.c
@@ -356,7 +356,9 @@ void *arch_dma_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
 void arch_dma_free(struct device *dev, size_t size, void *cpu_addr,
 		dma_addr_t dma_addr, unsigned long attrs)
 {
-	if (!sparc_dma_free_resource(cpu_addr, PAGE_ALIGN(size)))
+	size = PAGE_ALIGN(size);
+
+	if (!sparc_dma_free_resource(cpu_addr, size))
 		return;
 
 	dma_make_coherent(dma_addr, size);
-- 
2.17.1

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[Christoph Hellwig]

On Wed, Sep 08, 2021 at 09:48:22AM +0200, Andreas Larsson wrote:
> Commit 53b7670e5735 ("sparc: factor the dma coherent mapping into
> helper") lost the page align for the calls to dma_make_coherent and
> srmmu_unmapiorange. The latter cannot handle a non page aligned len
> argument.
> 
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Andreas - while I've got your attention:  I've been looking into fully
converting sparc32 to the generic DMA code.  Do you have any
documentation for the Leon cache handling in dma_make_coherent,
and more importantly how that applies to the dma coherent handling?
I could see how a flush might be required for the streaming DMA mappings,
that is mapping normal cached memory for I/O.  But for the coherent
allocations which can be accessed from the device and the cpu without
another DMA mapping call this seems really strange.

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[Andreas Larsson]

On 2021-09-09 08:07, Christoph Hellwig wrote:
> Andreas - while I've got your attention:  I've been looking into fully
> converting sparc32 to the generic DMA code.  Do you have any
> documentation for the Leon cache handling in dma_make_coherent,
> and more importantly how that applies to the dma coherent handling?
> I could see how a flush might be required for the streaming DMA mappings,
> that is mapping normal cached memory for I/O.  But for the coherent
> allocations which can be accessed from the device and the cpu without
> another DMA mapping call this seems really strange.

As long as the area passed to arch_dma_free is mapped by
arch_dma_allocate, I don't see why the call to dma_make_coherent in
arch_dma_free should be needed. I am not sure if there are any current
(or historical paths) where we nevertheless have a cacheable mapping
when we reach arch_dma_free (or the historical pci32_free_coherent).

The usual case for LEON systems is that cache snooping on the CPU side
invalidates cache lines matching DMA that the CPU sees on the bus. Under
the assumption that DMA accesses are seen on the processor bus, this is
the reason for only flushing if snooping is not enabled in
dma_make_coherent.

-- 
Andreas Larsson

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[Christoph Hellwig]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Mon, Sep 13, 2021 at 03:18:38PM +0200, Andreas Larsson wrote:
>> Andreas - while I've got your attention:  I've been looking into fully
>> converting sparc32 to the generic DMA code.  Do you have any
>> documentation for the Leon cache handling in dma_make_coherent,
>> and more importantly how that applies to the dma coherent handling?
>> I could see how a flush might be required for the streaming DMA mappings,
>> that is mapping normal cached memory for I/O.  But for the coherent
>> allocations which can be accessed from the device and the cpu without
>> another DMA mapping call this seems really strange.
>
> As long as the area passed to arch_dma_free is mapped by
> arch_dma_allocate, I don't see why the call to dma_make_coherent in
> arch_dma_free should be needed. I am not sure if there are any current
> (or historical paths) where we nevertheless have a cacheable mapping
> when we reach arch_dma_free (or the historical pci32_free_coherent).

Note that the cacheable mapping in the kernel map still exists, but is
is not used for any access.

> The usual case for LEON systems is that cache snooping on the CPU side
> invalidates cache lines matching DMA that the CPU sees on the bus. Under
> the assumption that DMA accesses are seen on the processor bus, this is
> the reason for only flushing if snooping is not enabled in
> dma_make_coherent.

Thanks.  Can you take a look and test the two patches below on top of
your fix?  A git tree is also available here:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/sparc32-generic-dma

[-- Attachment #2: 0001-sparc32-remove-dma_make_coherent.patch --]
[-- Type: text/x-patch, Size: 2363 bytes --]

From 832183d0409a941788e4c27682b2ad5164aec5d9 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Mon, 13 Sep 2021 18:39:38 +0200
Subject: sparc32: remove dma_make_coherent

LEON only needs snooping when DMA accesses are not seen on the processor
bus.  Given that coherent allocations are mapped uncached this can't
happen for those, so open code the d-cache flushing logic in the only
remaing place that needs it, arch_sync_dma_for_cpu.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/sparc/kernel/ioport.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
index 7ceae24b0ca99..3eb748e862220 100644
--- a/arch/sparc/kernel/ioport.c
+++ b/arch/sparc/kernel/ioport.c
@@ -52,17 +52,6 @@
 #include <asm/io-unit.h>
 #include <asm/leon.h>
 
-/* This function must make sure that caches and memory are coherent after DMA
- * On LEON systems without cache snooping it flushes the entire D-CACHE.
- */
-static inline void dma_make_coherent(unsigned long pa, unsigned long len)
-{
-	if (sparc_cpu_model == sparc_leon) {
-		if (!sparc_leon3_snooping_enabled())
-			leon_flush_dcache_all();
-	}
-}
-
 static void __iomem *_sparc_ioremap(struct resource *res, u32 bus, u32 pa, int sz);
 static void __iomem *_sparc_alloc_io(unsigned int busno, unsigned long phys,
     unsigned long size, char *name);
@@ -361,18 +350,23 @@ void arch_dma_free(struct device *dev, size_t size, void *cpu_addr,
 	if (!sparc_dma_free_resource(cpu_addr, size))
 		return;
 
-	dma_make_coherent(dma_addr, size);
 	srmmu_unmapiorange((unsigned long)cpu_addr, size);
 	free_pages((unsigned long)phys_to_virt(dma_addr), get_order(size));
 }
 
-/* IIep is write-through, not flushing on cpu to device transfer. */
-
+/*
+ * IIep is write-through, not flushing on cpu to device transfer.
+ *
+ * On LEON systems without cache snooping, the entire D-CACHE must be flushed to
+ * make DMA to cacheable memory coherent.
+ */
 void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
 		enum dma_data_direction dir)
 {
-	if (dir != PCI_DMA_TODEVICE)
-		dma_make_coherent(paddr, PAGE_ALIGN(size));
+	if (dir != PCI_DMA_TODEVICE &&
+	    sparc_cpu_model == sparc_leon &&
+	    !sparc_leon3_snooping_enabled())
+		leon_flush_dcache_all();
 }
 
 #ifdef CONFIG_PROC_FS
-- 
2.30.2


[-- Attachment #3: 0002-sparc32-use-DMA_DIRECT_REMAP.patch --]
[-- Type: text/x-patch, Size: 3836 bytes --]

From 0f612347699290a8d2f604a7640b5568e3d9af57 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Mon, 13 Sep 2021 18:44:55 +0200
Subject: sparc32: use DMA_DIRECT_REMAP

Use the generic dma remapping allocator instead of open coding it.
This also avoids setting up page tables from irq context which is
generally dangerous and uses the atomic pool instead.

The only interesting part is the architecture specific pgprot_dmacoherent
definition that sets the SRMMU_PRIV bit as done by the old implementation.
(I have no idea what it is useful for, though).

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/sparc/Kconfig                  |  3 +-
 arch/sparc/include/asm/pgtable_32.h |  8 +++++
 arch/sparc/kernel/ioport.c          | 54 -----------------------------
 3 files changed, 10 insertions(+), 55 deletions(-)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index f0c0f955e1695..8089258d6cc97 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -54,8 +54,9 @@ config SPARC32
 	def_bool !64BIT
 	select ARCH_32BIT_OFF_T
 	select ARCH_HAS_SYNC_DMA_FOR_CPU
-	select GENERIC_ATOMIC64
 	select CLZ_TAB
+	select DMA_DIRECT_REMAP
+	select GENERIC_ATOMIC64
 	select HAVE_UID16
 	select OLD_SIGACTION
 	select ZONE_DMA
diff --git a/arch/sparc/include/asm/pgtable_32.h b/arch/sparc/include/asm/pgtable_32.h
index ffccfe3b22ed3..1e7984ff7b320 100644
--- a/arch/sparc/include/asm/pgtable_32.h
+++ b/arch/sparc/include/asm/pgtable_32.h
@@ -313,6 +313,14 @@ static inline pgprot_t pgprot_noncached(pgprot_t prot)
 	return prot;
 }
 
+#define pgprot_dmacoherent pgprot_dmacoherent
+static inline pgprot_t pgprot_dmacoherent(pgprot_t prot)
+{
+	pgprot_val(prot) &= ~pgprot_val(__pgprot(SRMMU_CACHE));
+	pgprot_val(prot) |= pgprot_val(__pgprot(SRMMU_PRIV));
+	return prot;
+}
+
 static pte_t pte_modify(pte_t pte, pgprot_t newprot) __attribute_const__;
 static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 {
diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
index 3eb748e862220..57a72c46eddb0 100644
--- a/arch/sparc/kernel/ioport.c
+++ b/arch/sparc/kernel/ioport.c
@@ -300,60 +300,6 @@ arch_initcall(sparc_register_ioport);
 
 #endif /* CONFIG_SBUS */
 
-
-/* Allocate and map kernel buffer using consistent mode DMA for a device.
- * hwdev should be valid struct pci_dev pointer for PCI devices.
- */
-void *arch_dma_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
-		gfp_t gfp, unsigned long attrs)
-{
-	unsigned long addr;
-	void *va;
-
-	if (!size || size > 256 * 1024)	/* __get_free_pages() limit */
-		return NULL;
-
-	size = PAGE_ALIGN(size);
-	va = (void *) __get_free_pages(gfp | __GFP_ZERO, get_order(size));
-	if (!va) {
-		printk("%s: no %zd pages\n", __func__, size >> PAGE_SHIFT);
-		return NULL;
-	}
-
-	addr = sparc_dma_alloc_resource(dev, size);
-	if (!addr)
-		goto err_nomem;
-
-	srmmu_mapiorange(0, virt_to_phys(va), addr, size);
-
-	*dma_handle = virt_to_phys(va);
-	return (void *)addr;
-
-err_nomem:
-	free_pages((unsigned long)va, get_order(size));
-	return NULL;
-}
-
-/* Free and unmap a consistent DMA buffer.
- * cpu_addr is what was returned arch_dma_alloc, size must be the same as what
- * was passed into arch_dma_alloc, and likewise dma_addr must be the same as
- * what *dma_ndler was set to.
- *
- * References to the memory and mappings associated with cpu_addr/dma_addr
- * past this call are illegal.
- */
-void arch_dma_free(struct device *dev, size_t size, void *cpu_addr,
-		dma_addr_t dma_addr, unsigned long attrs)
-{
-	size = PAGE_ALIGN(size);
-
-	if (!sparc_dma_free_resource(cpu_addr, size))
-		return;
-
-	srmmu_unmapiorange((unsigned long)cpu_addr, size);
-	free_pages((unsigned long)phys_to_virt(dma_addr), get_order(size));
-}
-
 /*
  * IIep is write-through, not flushing on cpu to device transfer.
  *
-- 
2.30.2

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[Andreas Larsson]

On 2021-09-14 08:17, Christoph Hellwig wrote:
> Thanks.  Can you take a look and test the two patches below on top of
> your fix?  A git tree is also available here:
> 
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/sparc32-generic-dma
> 

In a quick test, this seems to work on LEON for code paths previously 
going to arch_dma_alloc and arch_dma_free. However, this makes setting 
up these DMA mappings to not go through sparc_dma_alloc_resource, and it 
seems important that they do that on Sun systems. Hopefully, someone 
with more knowledge about that could chime in here.

The added pgprot_dmacoherent is problematic as it sets SRMMU_PRIV, which 
sets up kernel access only. This was fine for arch_dma_alloc that sets 
up kernel accesses only, but for user space DMA mmap this would make 
them kernel accessable only. Having no sparc-specific 
pgprot_dmacoherent, keeping it to default to pgprot_noncached, is 
probably better.

-- 
Andreas Larsson

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[Christoph Hellwig]

On Tue, Sep 14, 2021 at 10:51:51AM +0200, Andreas Larsson wrote:
> On 2021-09-14 08:17, Christoph Hellwig wrote:
>> Thanks.  Can you take a look and test the two patches below on top of
>> your fix?  A git tree is also available here:
>>
>> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/sparc32-generic-dma
>>
>
> In a quick test, this seems to work on LEON for code paths previously going 
> to arch_dma_alloc and arch_dma_free. However, this makes setting up these 
> DMA mappings to not go through sparc_dma_alloc_resource, and it seems 
> important that they do that on Sun systems. Hopefully, someone with more 
> knowledge about that could chime in here.

Does the hardware actually care about it?  The only thing it does is
to force allocating from a specific virtual address range, but how
would that have a special meaning?

> The added pgprot_dmacoherent is problematic as it sets SRMMU_PRIV, which 
> sets up kernel access only. This was fine for arch_dma_alloc that sets up 
> kernel accesses only, but for user space DMA mmap this would make them 
> kernel accessable only. Having no sparc-specific pgprot_dmacoherent, 
> keeping it to default to pgprot_noncached, is probably better.

I've just tried to keep the existing attributes.  If SRMMU_PRIV does
indeed mean that the page can't also be mapped into userspace page tables
it would be good to remove it in an incremental patch.  If OTOH it only
means that this PTE is a kernel mapping it should not affect a userspace
mapping as that will always use separate PTEs.

>
> -- 
> Andreas Larsson
---end quoted text---

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[Andreas Larsson]

Please consider the environment before printing this email
On 2021-09-14 12:42, Christoph Hellwig wrote:

>> The added pgprot_dmacoherent is problematic as it sets SRMMU_PRIV, which
>> sets up kernel access only. This was fine for arch_dma_alloc that sets up
>> kernel accesses only, but for user space DMA mmap this would make them
>> kernel accessable only. Having no sparc-specific pgprot_dmacoherent,
>> keeping it to default to pgprot_noncached, is probably better.
> 
> I've just tried to keep the existing attributes.  If SRMMU_PRIV does
> indeed mean that the page can't also be mapped into userspace page tables
> it would be good to remove it in an incremental patch.  If OTOH it only
> means that this PTE is a kernel mapping it should not affect a userspace
> mapping as that will always use separate PTEs.

Before the patch, arch_dma_alloc did via srmmu_mapiorange set up pages 
with SRMMU_PRIV, which is all fine as it sets up kernel buffers. With 
your patch we get PAGE_KERNEL as an argument to dma_pgprot in the 
corresponding call path that earlier lead to arch_dma_alloc. PAGE_KERNEL 
already includes SRMMU_PRIV so adding it again should not be necessary.

The problem I am pointing to is that adding a pgprot_dmacoherent that 
adds SRMMU_PRIV, changes the behaviour of other call paths that calls 
dma_pgprot but are not mapping in kernel pages.

Now this is not confirmed in execution from my side, but it seems that 
from following the code that e.g. this call path that is about mapping 
DMA pages accessible from user space:

dma_mmap_attrs ->  dma_direct_mmap -> dma_pgprot -> pgprot_dmacoherent

goes from making it merely uncacheable with the default

#ifndef pgprot_dmacoherent
#define pgprot_dmacoherent(prot)	pgprot_noncached(prot)
#endif

to also being non-user-accessible if we change to this  pgprot_dmacoherent

#define pgprot_dmacoherent pgprot_dmacoherent
static inline pgprot_t pgprot_dmacoherent(pgprot_t prot)
{
	pgprot_val(prot) &= ~pgprot_val(__pgprot(SRMMU_CACHE));
	pgprot_val(prot) |= pgprot_val(__pgprot(SRMMU_PRIV));
	return prot;
}

-- 
Andreas Larsson

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[Christoph Hellwig]

On Tue, Sep 14, 2021 at 01:16:16PM +0200, Andreas Larsson wrote:
> Before the patch, arch_dma_alloc did via srmmu_mapiorange set up pages with 
> SRMMU_PRIV, which is all fine as it sets up kernel buffers. With your patch 
> we get PAGE_KERNEL as an argument to dma_pgprot in the corresponding call 
> path that earlier lead to arch_dma_alloc. PAGE_KERNEL already includes 
> SRMMU_PRIV so adding it again should not be necessary.

You're right, I missed that PAGE_KERNEL already includes SRMMU_PRIV.

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[Christoph Hellwig]

On Wed, Sep 08, 2021 at 06:04:54PM +0200, Sam Ravnborg wrote:
> Hi Andreas,
> 
> On Wed, Sep 08, 2021 at 09:48:22AM +0200, Andreas Larsson wrote:
> > Commit 53b7670e5735 ("sparc: factor the dma coherent mapping into
> > helper") lost the page align for the calls to dma_make_coherent and
> > srmmu_unmapiorange. The latter cannot handle a non page aligned len
> > argument.
> 
> I wonder how you managed to track this down - well done.
> > 
> > Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> I assume David or Christoph picks it up.

I'll happily pick it up if that is ok.  Dave?

Re: [PATCH] sparc32: Page align size in arch_dma_alloc

[David Miller]

From: Christoph Hellwig <hch@lst.de>
Date: Tue, 14 Sep 2021 08:12:22 +0200

> On Wed, Sep 08, 2021 at 06:04:54PM +0200, Sam Ravnborg wrote:
>> Hi Andreas,
>> 
>> On Wed, Sep 08, 2021 at 09:48:22AM +0200, Andreas Larsson wrote:
>> > Commit 53b7670e5735 ("sparc: factor the dma coherent mapping into
>> > helper") lost the page align for the calls to dma_make_coherent and
>> > srmmu_unmapiorange. The latter cannot handle a non page aligned len
>> > argument.
>> 
>> I wonder how you managed to track this down - well done.
>> > 
>> > Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>> 
>> I assume David or Christoph picks it up.
> 
> I'll happily pick it up if that is ok.  Dave?

Yep, it's ok.