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

[Andreas Larsson <andreas@gaisler.com>]

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