* fix default dma_mmap_* pgprot @ 2019-08-01 14:21 Christoph Hellwig 2019-08-01 14:21 ` [PATCH] dma-mapping: fix page attributes for dma_mmap_* Christoph Hellwig 0 siblings, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2019-08-01 14:21 UTC (permalink / raw) To: iommu Cc: Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Will Deacon, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel Hi all, As Shawn pointed out we've had issues with the dma mmap pgprots ever since the dma_common_mmap helper was added beyong the initial architectures - we default to uncached mappings, but for devices that are DMA coherent, or if the DMA_ATTR_NON_CONSISTENT is set (and supported) this can lead to aliasing of cache attributes. This patch fixes that. My explanation of why this hasn't been much of an issue is that the dma_mmap_ helpers aren't used widely and mostly just in architecture specific drivers. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-01 14:21 fix default dma_mmap_* pgprot Christoph Hellwig @ 2019-08-01 14:21 ` Christoph Hellwig 2019-08-01 16:23 ` Will Deacon 0 siblings, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2019-08-01 14:21 UTC (permalink / raw) To: iommu Cc: Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Will Deacon, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel All the way back to introducing dma_common_mmap we've defaulyed to mark the pages as uncached. But this is wrong for DMA coherent devices or if using DMA_ATTR_NON_CONSISTENT. Later on DMA_ATTR_WRITE_COMBINE also got incorrect treatment as that flag is only treated special on the alloc side for non-coherent devices. Introduce a new dma_mmap_pgprot helper that deals with the check for coherent devices and DMA_ATTR_NON_CONSISTENT so that only the remapping cases even reach arch_dma_mmap_pgprot and we thus ensure no aliasing of page attributes happens. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/arm/mm/dma-mapping.c | 4 +--- arch/arm64/mm/dma-mapping.c | 4 +--- arch/powerpc/kernel/Makefile | 3 +-- arch/powerpc/kernel/dma-common.c | 17 ----------------- drivers/iommu/dma-iommu.c | 6 +++--- include/linux/dma-mapping.h | 1 + include/linux/dma-noncoherent.h | 5 ----- kernel/dma/mapping.c | 11 ++++++++++- kernel/dma/remap.c | 2 +- 9 files changed, 18 insertions(+), 35 deletions(-) delete mode 100644 arch/powerpc/kernel/dma-common.c diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 9c9a23e5600d..cfe44df169c5 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -2397,9 +2397,7 @@ long arch_dma_coherent_to_pfn(struct device *dev, void *cpu_addr, pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs) { - if (!dev_is_dma_coherent(dev)) - return __get_dma_pgprot(attrs, prot); - return prot; + return __get_dma_pgprot(attrs, prot); } void *arch_dma_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle, diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index 1d3f0b5a9940..bd2b039f43a6 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -14,9 +14,7 @@ pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs) { - if (!dev_is_dma_coherent(dev) || (attrs & DMA_ATTR_WRITE_COMBINE)) - return pgprot_writecombine(prot); - return prot; + return pgprot_writecombine(prot); } void arch_sync_dma_for_device(struct device *dev, phys_addr_t paddr, diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile index ea0c69236789..56dfa7a2a6f2 100644 --- a/arch/powerpc/kernel/Makefile +++ b/arch/powerpc/kernel/Makefile @@ -49,8 +49,7 @@ obj-y := cputable.o ptrace.o syscalls.o \ signal.o sysfs.o cacheinfo.o time.o \ prom.o traps.o setup-common.o \ udbg.o misc.o io.o misc_$(BITS).o \ - of_platform.o prom_parse.o \ - dma-common.o + of_platform.o prom_parse.o obj-$(CONFIG_PPC64) += setup_64.o sys_ppc32.o \ signal_64.o ptrace32.o \ paca.o nvram_64.o firmware.o diff --git a/arch/powerpc/kernel/dma-common.c b/arch/powerpc/kernel/dma-common.c deleted file mode 100644 index dc7ef6b17b69..000000000000 --- a/arch/powerpc/kernel/dma-common.c +++ /dev/null @@ -1,17 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-or-later -/* - * Contains common dma routines for all powerpc platforms. - * - * Copyright (C) 2019 Shawn Anastasio. - */ - -#include <linux/mm.h> -#include <linux/dma-noncoherent.h> - -pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot, - unsigned long attrs) -{ - if (!dev_is_dma_coherent(dev)) - return pgprot_noncached(prot); - return prot; -} diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index a7f9c3edbcb2..703de9a7553f 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -574,7 +574,7 @@ static void *iommu_dma_alloc_remap(struct device *dev, size_t size, struct iova_domain *iovad = &cookie->iovad; bool coherent = dev_is_dma_coherent(dev); int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs); - pgprot_t prot = arch_dma_mmap_pgprot(dev, PAGE_KERNEL, attrs); + pgprot_t prot = dma_mmap_pgprot(dev, PAGE_KERNEL, attrs); unsigned int count, min_size, alloc_sizes = domain->pgsize_bitmap; struct page **pages; struct sg_table sgt; @@ -975,7 +975,7 @@ static void *iommu_dma_alloc_pages(struct device *dev, size_t size, return NULL; if (IS_ENABLED(CONFIG_DMA_REMAP) && (!coherent || PageHighMem(page))) { - pgprot_t prot = arch_dma_mmap_pgprot(dev, PAGE_KERNEL, attrs); + pgprot_t prot = dma_mmap_pgprot(dev, PAGE_KERNEL, attrs); cpu_addr = dma_common_contiguous_remap(page, alloc_size, VM_USERMAP, prot, __builtin_return_address(0)); @@ -1035,7 +1035,7 @@ static int iommu_dma_mmap(struct device *dev, struct vm_area_struct *vma, unsigned long pfn, off = vma->vm_pgoff; int ret; - vma->vm_page_prot = arch_dma_mmap_pgprot(dev, vma->vm_page_prot, attrs); + vma->vm_page_prot = dma_mmap_pgprot(dev, vma->vm_page_prot, attrs); if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret)) return ret; diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 633dae466097..c61d7870277f 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -611,6 +611,7 @@ static inline void dma_sync_single_range_for_device(struct device *dev, #define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t, v, h, s, 0) #define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h, s, 0) +pgprot_t dma_mmap_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs); extern int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs); diff --git a/include/linux/dma-noncoherent.h b/include/linux/dma-noncoherent.h index 3813211a9aad..9ae5cee543c4 100644 --- a/include/linux/dma-noncoherent.h +++ b/include/linux/dma-noncoherent.h @@ -42,13 +42,8 @@ void arch_dma_free(struct device *dev, size_t size, void *cpu_addr, dma_addr_t dma_addr, unsigned long attrs); long arch_dma_coherent_to_pfn(struct device *dev, void *cpu_addr, dma_addr_t dma_addr); - -#ifdef CONFIG_ARCH_HAS_DMA_MMAP_PGPROT pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs); -#else -# define arch_dma_mmap_pgprot(dev, prot, attrs) pgprot_noncached(prot) -#endif #ifdef CONFIG_DMA_NONCOHERENT_CACHE_SYNC void arch_dma_cache_sync(struct device *dev, void *vaddr, size_t size, diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c index 815446f76995..357ae5cdb91b 100644 --- a/kernel/dma/mapping.c +++ b/kernel/dma/mapping.c @@ -162,6 +162,15 @@ int dma_get_sgtable_attrs(struct device *dev, struct sg_table *sgt, } EXPORT_SYMBOL(dma_get_sgtable_attrs); +pgprot_t dma_mmap_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs) +{ + if (dev_is_dma_coherent(dev) || (attrs & DMA_ATTR_NON_CONSISTENT)) + return prot; + if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_MMAP_PGPROT)) + return arch_dma_mmap_pgprot(dev, prot, attrs); + return pgprot_noncached(prot); +} + /* * Create userspace mapping for the DMA-coherent memory. */ @@ -175,7 +184,7 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, unsigned long pfn; int ret = -ENXIO; - vma->vm_page_prot = arch_dma_mmap_pgprot(dev, vma->vm_page_prot, attrs); + vma->vm_page_prot = dma_mmap_pgprot(dev, vma->vm_page_prot, attrs); if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret)) return ret; diff --git a/kernel/dma/remap.c b/kernel/dma/remap.c index a594aec07882..3d75c79b124c 100644 --- a/kernel/dma/remap.c +++ b/kernel/dma/remap.c @@ -218,7 +218,7 @@ void *arch_dma_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle, /* create a coherent mapping */ ret = dma_common_contiguous_remap(page, size, VM_USERMAP, - arch_dma_mmap_pgprot(dev, PAGE_KERNEL, attrs), + dma_mmap_pgprot(dev, PAGE_KERNEL, attrs), __builtin_return_address(0)); if (!ret) { __dma_direct_free_pages(dev, size, page); -- 2.20.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-01 14:21 ` [PATCH] dma-mapping: fix page attributes for dma_mmap_* Christoph Hellwig @ 2019-08-01 16:23 ` Will Deacon 2019-08-01 16:34 ` Christoph Hellwig 0 siblings, 1 reply; 12+ messages in thread From: Will Deacon @ 2019-08-01 16:23 UTC (permalink / raw) To: Christoph Hellwig Cc: iommu, Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Thu, Aug 01, 2019 at 05:21:18PM +0300, Christoph Hellwig wrote: > All the way back to introducing dma_common_mmap we've defaulyed to mark > the pages as uncached. But this is wrong for DMA coherent devices or > if using DMA_ATTR_NON_CONSISTENT. Later on DMA_ATTR_WRITE_COMBINE > also got incorrect treatment as that flag is only treated special on > the alloc side for non-coherent devices. > > Introduce a new dma_mmap_pgprot helper that deals with the check > for coherent devices and DMA_ATTR_NON_CONSISTENT so that only the > remapping cases even reach arch_dma_mmap_pgprot and we thus ensure > no aliasing of page attributes happens. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > arch/arm/mm/dma-mapping.c | 4 +--- > arch/arm64/mm/dma-mapping.c | 4 +--- > arch/powerpc/kernel/Makefile | 3 +-- > arch/powerpc/kernel/dma-common.c | 17 ----------------- > drivers/iommu/dma-iommu.c | 6 +++--- > include/linux/dma-mapping.h | 1 + > include/linux/dma-noncoherent.h | 5 ----- > kernel/dma/mapping.c | 11 ++++++++++- > kernel/dma/remap.c | 2 +- > 9 files changed, 18 insertions(+), 35 deletions(-) > delete mode 100644 arch/powerpc/kernel/dma-common.c > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index 9c9a23e5600d..cfe44df169c5 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -2397,9 +2397,7 @@ long arch_dma_coherent_to_pfn(struct device *dev, void *cpu_addr, > pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot, > unsigned long attrs) > { > - if (!dev_is_dma_coherent(dev)) > - return __get_dma_pgprot(attrs, prot); > - return prot; > + return __get_dma_pgprot(attrs, prot); > } > > void *arch_dma_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle, > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c > index 1d3f0b5a9940..bd2b039f43a6 100644 > --- a/arch/arm64/mm/dma-mapping.c > +++ b/arch/arm64/mm/dma-mapping.c > @@ -14,9 +14,7 @@ > pgprot_t arch_dma_mmap_pgprot(struct device *dev, pgprot_t prot, > unsigned long attrs) > { > - if (!dev_is_dma_coherent(dev) || (attrs & DMA_ATTR_WRITE_COMBINE)) > - return pgprot_writecombine(prot); > - return prot; > + return pgprot_writecombine(prot); > } Seems like a sensible cleanup to me: Acked-by: Will Deacon <will@kernel.org> Although arch_dma_mmap_pgprot() is a bit of a misnomer now that it only gets involved in the non-coherent case. Will ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-01 16:23 ` Will Deacon @ 2019-08-01 16:34 ` Christoph Hellwig 2019-08-01 16:44 ` Will Deacon 0 siblings, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2019-08-01 16:34 UTC (permalink / raw) To: Will Deacon Cc: Christoph Hellwig, iommu, Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Thu, Aug 01, 2019 at 05:23:06PM +0100, Will Deacon wrote: > > - if (!dev_is_dma_coherent(dev) || (attrs & DMA_ATTR_WRITE_COMBINE)) > > - return pgprot_writecombine(prot); > > - return prot; > > + return pgprot_writecombine(prot); > > } > > Seems like a sensible cleanup to me: > > Acked-by: Will Deacon <will@kernel.org> > > Although arch_dma_mmap_pgprot() is a bit of a misnomer now that it only > gets involved in the non-coherent case. A better name is welcome. My other idea would be to just remove it entirely and do something like: #ifndef pgprot_dmacoherent #define pgprot_dmacoherent pgprot_noncached #endif pgprot_t dma_mmap_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs) { if (dev_is_dma_coherent(dev) || (attrs & DMA_ATTR_NON_CONSISTENT)) return prot; #ifdef pgprot_writecombine if (attrs & DMA_ATTR_WRITE_COMBINE) return pgprot_writecombine(prot); #endif return pgprot_dmacoherent(prot); } But my worry is how this interacts with architectures that have an uncached segment (mips, nios2, microblaze, extensa) where we'd have the kernel access DMA_ATTR_WRITE_COMBINE mappigns using the uncached segment, and userspace mmaps using pgprot_writecombine, which could lead to aliasing issues. But then again mips already supports DMA_ATTR_WRITE_COMBINE, so this must be ok somehow. I guess I'll need to field that question to the relevant parties. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-01 16:34 ` Christoph Hellwig @ 2019-08-01 16:44 ` Will Deacon 2019-08-02 8:14 ` Christoph Hellwig 0 siblings, 1 reply; 12+ messages in thread From: Will Deacon @ 2019-08-01 16:44 UTC (permalink / raw) To: Christoph Hellwig Cc: iommu, Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Thu, Aug 01, 2019 at 06:34:57PM +0200, Christoph Hellwig wrote: > On Thu, Aug 01, 2019 at 05:23:06PM +0100, Will Deacon wrote: > > > - if (!dev_is_dma_coherent(dev) || (attrs & DMA_ATTR_WRITE_COMBINE)) > > > - return pgprot_writecombine(prot); > > > - return prot; > > > + return pgprot_writecombine(prot); > > > } > > > > Seems like a sensible cleanup to me: > > > > Acked-by: Will Deacon <will@kernel.org> > > > > Although arch_dma_mmap_pgprot() is a bit of a misnomer now that it only > > gets involved in the non-coherent case. > > A better name is welcome. How about arch_dma_noncoherent_mmap_pgprot() ? Too long? > My other idea would be to just remove it entirely and do something like: > > #ifndef pgprot_dmacoherent > #define pgprot_dmacoherent pgprot_noncached > #endif > > pgprot_t dma_mmap_pgprot(struct device *dev, pgprot_t prot, unsigned long attrs) > { > if (dev_is_dma_coherent(dev) || (attrs & DMA_ATTR_NON_CONSISTENT)) > return prot; > #ifdef pgprot_writecombine > if (attrs & DMA_ATTR_WRITE_COMBINE) > return pgprot_writecombine(prot); > #endif > return pgprot_dmacoherent(prot); > } Oh, I prefer that! > But my worry is how this interacts with architectures that have an > uncached segment (mips, nios2, microblaze, extensa) where we'd have > the kernel access DMA_ATTR_WRITE_COMBINE mappigns using the uncached > segment, and userspace mmaps using pgprot_writecombine, which could > lead to aliasing issues. But then again mips already supports > DMA_ATTR_WRITE_COMBINE, so this must be ok somehow. I guess I'll > need to field that question to the relevant parties. Or it's always been busted and happens to work out in practice... Will ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-01 16:44 ` Will Deacon @ 2019-08-02 8:14 ` Christoph Hellwig 2019-08-02 10:38 ` Will Deacon 0 siblings, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2019-08-02 8:14 UTC (permalink / raw) To: Will Deacon Cc: Christoph Hellwig, iommu, Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Thu, Aug 01, 2019 at 05:44:12PM +0100, Will Deacon wrote: > > > Although arch_dma_mmap_pgprot() is a bit of a misnomer now that it only > > > gets involved in the non-coherent case. > > > > A better name is welcome. > > How about arch_dma_noncoherent_mmap_pgprot() ? Too long? Sounds a little long yes. And doesn't fix the additional problem that we don't just it for mmap but also for the in-kernel remapping these days. > > But my worry is how this interacts with architectures that have an > > uncached segment (mips, nios2, microblaze, extensa) where we'd have > > the kernel access DMA_ATTR_WRITE_COMBINE mappigns using the uncached > > segment, and userspace mmaps using pgprot_writecombine, which could > > lead to aliasing issues. But then again mips already supports > > DMA_ATTR_WRITE_COMBINE, so this must be ok somehow. I guess I'll > > need to field that question to the relevant parties. > > Or it's always been busted and happens to work out in practice... I've sent a ping to the mips folks. While we'are at it: arm64 and arm32 (optionally) map dma coherent allocations as write combine. I suspect this hasn't always just been busted but intentional (of course!), but is there any chance to get a quote from the arm architecture spec on why this is fine as it looks rather confusion? Also if we assume mips is buggy DMA_ATTR_WRITE_COMBINE really just seems to be there for old arm platforms, which makes the scope pretty limited. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-02 8:14 ` Christoph Hellwig @ 2019-08-02 10:38 ` Will Deacon 2019-08-03 6:48 ` Christoph Hellwig 0 siblings, 1 reply; 12+ messages in thread From: Will Deacon @ 2019-08-02 10:38 UTC (permalink / raw) To: Christoph Hellwig Cc: iommu, Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Fri, Aug 02, 2019 at 10:14:41AM +0200, Christoph Hellwig wrote: > On Thu, Aug 01, 2019 at 05:44:12PM +0100, Will Deacon wrote: > > > > Although arch_dma_mmap_pgprot() is a bit of a misnomer now that it only > > > > gets involved in the non-coherent case. > > > > > > A better name is welcome. > > > > How about arch_dma_noncoherent_mmap_pgprot() ? Too long? > > Sounds a little long yes. And doesn't fix the additional problem that > we don't just it for mmap but also for the in-kernel remapping these > days. Hmm. Maybe just arch_dma_noncoherent_pgprot() then. > > > But my worry is how this interacts with architectures that have an > > > uncached segment (mips, nios2, microblaze, extensa) where we'd have > > > the kernel access DMA_ATTR_WRITE_COMBINE mappigns using the uncached > > > segment, and userspace mmaps using pgprot_writecombine, which could > > > lead to aliasing issues. But then again mips already supports > > > DMA_ATTR_WRITE_COMBINE, so this must be ok somehow. I guess I'll > > > need to field that question to the relevant parties. > > > > Or it's always been busted and happens to work out in practice... > > I've sent a ping to the mips folks. While we'are at it: arm64 > and arm32 (optionally) map dma coherent allocations as write combine. > I suspect this hasn't always just been busted but intentional (of course!), > but is there any chance to get a quote from the arm architecture spec > on why this is fine as it looks rather confusion? So this boils down to a terminology mismatch. The Arm architecture doesn't have anything called "write combine", so in Linux we instead provide what the Arm architecture calls "Normal non-cacheable" memory for pgprot_writecombine(). Amongst other things, this memory type permits speculation, unaligned accesses and merging of writes. I found something in the architecture spec about non-cachable memory, but it's written in Armglish[1]. pgprot_noncached(), on the other hand, provides what the architecture calls Strongly Ordered or Device-nGnRnE memory. This is intended for mapping MMIO (i.e. PCI config space) and therefore forbids speculation, preserves access size, requires strict alignment and also forces write responses to come from the endpoint. I think the naming mismatch is historical, but on arm64 we wanted to use the same names as arm32 so that any drivers using these things directly would get the same behaviour. Will [1] B2.4.4 Implication of caches for the application programmer [...] Data coherency issues Software can ensure the data coherency of caches in the following ways: * By not using the caches in situations where coherency issues can arise. This can be achieved by: - Using Non-cacheable or, in some cases, Write-Through Cacheable memory. - Not enabling caches in the system. * By using cache maintenance instructions to manage the coherency issues in software. * By using hardware coherency mechanisms to ensure the coherency of data accesses to memory for cacheable locations by observers within the different shareability domains. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-02 10:38 ` Will Deacon @ 2019-08-03 6:48 ` Christoph Hellwig 2019-08-06 16:08 ` Will Deacon 0 siblings, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2019-08-03 6:48 UTC (permalink / raw) To: Will Deacon Cc: Christoph Hellwig, iommu, Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Fri, Aug 02, 2019 at 11:38:03AM +0100, Will Deacon wrote: > > So this boils down to a terminology mismatch. The Arm architecture doesn't have > anything called "write combine", so in Linux we instead provide what the Arm > architecture calls "Normal non-cacheable" memory for pgprot_writecombine(). > Amongst other things, this memory type permits speculation, unaligned accesses > and merging of writes. I found something in the architecture spec about > non-cachable memory, but it's written in Armglish[1]. > > pgprot_noncached(), on the other hand, provides what the architecture calls > Strongly Ordered or Device-nGnRnE memory. This is intended for mapping MMIO > (i.e. PCI config space) and therefore forbids speculation, preserves access > size, requires strict alignment and also forces write responses to come from > the endpoint. > > I think the naming mismatch is historical, but on arm64 we wanted to use the > same names as arm32 so that any drivers using these things directly would get > the same behaviour. That all makes sense, but it totally needs a comment. I'll try to draft one based on this. I've also looked at the arm32 code a bit more, and it seems arm always (?) supported Normal non-cacheable attribute, but Linux only optionally uses it for arm v6+ because of fears of drivers missing barriers. The other really weird things is that in arm32 pgprot_dmacoherent incudes the L_PTE_XN bit, which from my understanding is the no-execture bit, but pgprot_writecombine does not. This seems to not very unintentional. So minus that the whole DMA_ATTR_WRITE_COMBІNE seems to be about flagging old arm specific drivers as having the proper barriers in places and otherwise is a no-op. Here is my tentative plan: - respin this patch with a small fix to handle the DMA_ATTR_NON_CONSISTENT (as in ignore it unless actually supported), but keep the name as-is to avoid churn. This should allow 5.3 inclusion and backports - remove DMA_ATTR_WRITE_COMBINE support from mips, probably also 5.3 material. - move all architectures but arm over to just define pgprot_dmacoherent, including a comment with the above explanation for arm64. - make DMA_ATTR_WRITE_COMBINE a no-op and schedule it for removal, thus removing the last instances of arch_dma_mmap_pgprot ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-03 6:48 ` Christoph Hellwig @ 2019-08-06 16:08 ` Will Deacon 2019-08-06 16:45 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 12+ messages in thread From: Will Deacon @ 2019-08-06 16:08 UTC (permalink / raw) To: Christoph Hellwig Cc: iommu, Shawn Anastasio, Michael Ellerman, Russell King, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Sat, Aug 03, 2019 at 08:48:12AM +0200, Christoph Hellwig wrote: > On Fri, Aug 02, 2019 at 11:38:03AM +0100, Will Deacon wrote: > > > > So this boils down to a terminology mismatch. The Arm architecture doesn't have > > anything called "write combine", so in Linux we instead provide what the Arm > > architecture calls "Normal non-cacheable" memory for pgprot_writecombine(). > > Amongst other things, this memory type permits speculation, unaligned accesses > > and merging of writes. I found something in the architecture spec about > > non-cachable memory, but it's written in Armglish[1]. > > > > pgprot_noncached(), on the other hand, provides what the architecture calls > > Strongly Ordered or Device-nGnRnE memory. This is intended for mapping MMIO > > (i.e. PCI config space) and therefore forbids speculation, preserves access > > size, requires strict alignment and also forces write responses to come from > > the endpoint. > > > > I think the naming mismatch is historical, but on arm64 we wanted to use the > > same names as arm32 so that any drivers using these things directly would get > > the same behaviour. > > That all makes sense, but it totally needs a comment. I'll try to draft > one based on this. I've also looked at the arm32 code a bit more, and > it seems arm always (?) supported Normal non-cacheable attribute, but > Linux only optionally uses it for arm v6+ because of fears of drivers > missing barriers. I think it was also to do with aliasing, but I don't recall all of the details. > The other really weird things is that in arm32 > pgprot_dmacoherent incudes the L_PTE_XN bit, which from my understanding > is the no-execture bit, but pgprot_writecombine does not. This seems to > not very unintentional. So minus that the whole DMA_ATTR_WRITE_COMBІNE > seems to be about flagging old arm specific drivers as having the proper > barriers in places and otherwise is a no-op. I think it only matters for Armv7 CPUs, but yes, we should probably be setting L_PTE_XN for both of these memory types. > Here is my tentative plan: > > - respin this patch with a small fix to handle the > DMA_ATTR_NON_CONSISTENT (as in ignore it unless actually supported), > but keep the name as-is to avoid churn. This should allow 5.3 > inclusion and backports > - remove DMA_ATTR_WRITE_COMBINE support from mips, probably also 5.3 > material. > - move all architectures but arm over to just define > pgprot_dmacoherent, including a comment with the above explanation > for arm64. That would be great, thanks. > - make DMA_ATTR_WRITE_COMBINE a no-op and schedule it for removal, > thus removing the last instances of arch_dma_mmap_pgprot All sounds good to me, although I suppose 32-bit Arm platforms without CONFIG_ARM_DMA_MEM_BUFFERABLE may run into issues if DMA_ATTR_WRITE_COMBINE disappears. Only one way to find out... Will ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-06 16:08 ` Will Deacon @ 2019-08-06 16:45 ` Russell King - ARM Linux admin 2019-08-06 16:48 ` Russell King - ARM Linux admin 2019-08-07 6:14 ` Christoph Hellwig 0 siblings, 2 replies; 12+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-06 16:45 UTC (permalink / raw) To: Will Deacon Cc: Christoph Hellwig, iommu, Shawn Anastasio, Michael Ellerman, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Tue, Aug 06, 2019 at 05:08:54PM +0100, Will Deacon wrote: > On Sat, Aug 03, 2019 at 08:48:12AM +0200, Christoph Hellwig wrote: > > On Fri, Aug 02, 2019 at 11:38:03AM +0100, Will Deacon wrote: > > > > > > So this boils down to a terminology mismatch. The Arm architecture doesn't have > > > anything called "write combine", so in Linux we instead provide what the Arm > > > architecture calls "Normal non-cacheable" memory for pgprot_writecombine(). > > > Amongst other things, this memory type permits speculation, unaligned accesses > > > and merging of writes. I found something in the architecture spec about > > > non-cachable memory, but it's written in Armglish[1]. > > > > > > pgprot_noncached(), on the other hand, provides what the architecture calls > > > Strongly Ordered or Device-nGnRnE memory. This is intended for mapping MMIO > > > (i.e. PCI config space) and therefore forbids speculation, preserves access > > > size, requires strict alignment and also forces write responses to come from > > > the endpoint. > > > > > > I think the naming mismatch is historical, but on arm64 we wanted to use the > > > same names as arm32 so that any drivers using these things directly would get > > > the same behaviour. > > > > That all makes sense, but it totally needs a comment. I'll try to draft > > one based on this. I've also looked at the arm32 code a bit more, and > > it seems arm always (?) supported Normal non-cacheable attribute, but > > Linux only optionally uses it for arm v6+ because of fears of drivers > > missing barriers. > > I think it was also to do with aliasing, but I don't recall all of the > details. ARMv6+ is where the architecture significantly changed to introduce the idea of [Normal, Device, Strongly Ordered] where Normal has the cache attributes. Before that, we had just "uncached/unbuffered, uncached/buffered, cached/unbuffered, cached/buffered" modes. The write buffer (enabled by buffered modes) has no architected guarantees about how long writes will sit in it, and there is only the "drain write buffer" instruction to push writes out. Up to and including ARMv5, we took the easy approach of just using the "uncached/unbuffered" mode since that is (a) the safest, and (b) avoids write buffers that alias when there are multiple different mappings. We could have used a different approach, making all IO writes contain a "drain write buffer" instruction, and map DMA memory as "buffered", but as there were no Linux barriers defined to order memory accesses to DMA memory (so, for example, ring buffers can be updated in the correct order) back in those days, using the uncached/unbuffered mode was the sanest and most reliable solution. > > > The other really weird things is that in arm32 > > pgprot_dmacoherent incudes the L_PTE_XN bit, which from my understanding > > is the no-execture bit, but pgprot_writecombine does not. This seems to > > not very unintentional. So minus that the whole DMA_ATTR_WRITE_COMBІNE > > seems to be about flagging old arm specific drivers as having the proper > > barriers in places and otherwise is a no-op. > > I think it only matters for Armv7 CPUs, but yes, we should probably be > setting L_PTE_XN for both of these memory types. Conventionally, pgprot_writecombine() has only been used to change the memory type and not the permissions. Since writecombine memory is still capable of being executed, I don't see any reason to set XN for it. If the user wishes to mmap() using PROT_READ|PROT_EXEC, then is there really a reason for writecombine to set XN overriding the user? That said, pgprot_writecombine() is mostly used for framebuffers, which arguably shouldn't be executable anyway - but who'd want to mmap() the framebuffer with PROT_EXEC? > > > Here is my tentative plan: > > > > - respin this patch with a small fix to handle the > > DMA_ATTR_NON_CONSISTENT (as in ignore it unless actually supported), > > but keep the name as-is to avoid churn. This should allow 5.3 > > inclusion and backports > > - remove DMA_ATTR_WRITE_COMBINE support from mips, probably also 5.3 > > material. > > - move all architectures but arm over to just define > > pgprot_dmacoherent, including a comment with the above explanation > > for arm64. > > That would be great, thanks. > > > - make DMA_ATTR_WRITE_COMBINE a no-op and schedule it for removal, > > thus removing the last instances of arch_dma_mmap_pgprot > > All sounds good to me, although I suppose 32-bit Arm platforms without > CONFIG_ARM_DMA_MEM_BUFFERABLE may run into issues if DMA_ATTR_WRITE_COMBINE > disappears. Only one way to find out... Looking at the results of grep, I think only OMAP2+ and Exynos may be affected. However, removing writecombine support from the DMA API is going to have a huge impact for framebuffers on earlier ARMs - that's where we do expect framebuffers to be mapped "uncached/buffered" for performance reasons and not "uncached/unbuffered". It's quite literally the difference between console scrolling being usable and totally unusable. Given what I've said above, switching to using buffered mode for normal DMA mappings is data-corrupting risky - as in your filesystem could get fried. I don't think we should play fast and loose with people's data by randomly changing that "because we'd like to", and I don't see that screwing the console is really an option either. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-06 16:45 ` Russell King - ARM Linux admin @ 2019-08-06 16:48 ` Russell King - ARM Linux admin 2019-08-07 6:14 ` Christoph Hellwig 1 sibling, 0 replies; 12+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-06 16:48 UTC (permalink / raw) To: Will Deacon Cc: Shawn Anastasio, Michael Ellerman, linuxppc-dev, linux-kernel, iommu, Catalin Marinas, Robin Murphy, Christoph Hellwig, linux-arm-kernel On Tue, Aug 06, 2019 at 05:45:03PM +0100, Russell King - ARM Linux admin wrote: > On Tue, Aug 06, 2019 at 05:08:54PM +0100, Will Deacon wrote: > > On Sat, Aug 03, 2019 at 08:48:12AM +0200, Christoph Hellwig wrote: > > > On Fri, Aug 02, 2019 at 11:38:03AM +0100, Will Deacon wrote: > > > > > > > > So this boils down to a terminology mismatch. The Arm architecture doesn't have > > > > anything called "write combine", so in Linux we instead provide what the Arm > > > > architecture calls "Normal non-cacheable" memory for pgprot_writecombine(). > > > > Amongst other things, this memory type permits speculation, unaligned accesses > > > > and merging of writes. I found something in the architecture spec about > > > > non-cachable memory, but it's written in Armglish[1]. > > > > > > > > pgprot_noncached(), on the other hand, provides what the architecture calls > > > > Strongly Ordered or Device-nGnRnE memory. This is intended for mapping MMIO > > > > (i.e. PCI config space) and therefore forbids speculation, preserves access > > > > size, requires strict alignment and also forces write responses to come from > > > > the endpoint. > > > > > > > > I think the naming mismatch is historical, but on arm64 we wanted to use the > > > > same names as arm32 so that any drivers using these things directly would get > > > > the same behaviour. > > > > > > That all makes sense, but it totally needs a comment. I'll try to draft > > > one based on this. I've also looked at the arm32 code a bit more, and > > > it seems arm always (?) supported Normal non-cacheable attribute, but > > > Linux only optionally uses it for arm v6+ because of fears of drivers > > > missing barriers. > > > > I think it was also to do with aliasing, but I don't recall all of the > > details. > > ARMv6+ is where the architecture significantly changed to introduce > the idea of [Normal, Device, Strongly Ordered] where Normal has the > cache attributes. > > Before that, we had just "uncached/unbuffered, uncached/buffered, > cached/unbuffered, cached/buffered" modes. > > The write buffer (enabled by buffered modes) has no architected > guarantees about how long writes will sit in it, and there is only > the "drain write buffer" instruction to push writes out. > > Up to and including ARMv5, we took the easy approach of just using > the "uncached/unbuffered" mode since that is (a) the safest, and (b) > avoids write buffers that alias when there are multiple different > mappings. > > We could have used a different approach, making all IO writes contain > a "drain write buffer" instruction, and map DMA memory as "buffered", > but as there were no Linux barriers defined to order memory accesses > to DMA memory (so, for example, ring buffers can be updated in the > correct order) back in those days, using the uncached/unbuffered mode > was the sanest and most reliable solution. > > > > > > The other really weird things is that in arm32 > > > pgprot_dmacoherent incudes the L_PTE_XN bit, which from my understanding > > > is the no-execture bit, but pgprot_writecombine does not. This seems to > > > not very unintentional. So minus that the whole DMA_ATTR_WRITE_COMBІNE > > > seems to be about flagging old arm specific drivers as having the proper > > > barriers in places and otherwise is a no-op. > > > > I think it only matters for Armv7 CPUs, but yes, we should probably be > > setting L_PTE_XN for both of these memory types. > > Conventionally, pgprot_writecombine() has only been used to change > the memory type and not the permissions. Since writecombine memory > is still capable of being executed, I don't see any reason to set XN > for it. > > If the user wishes to mmap() using PROT_READ|PROT_EXEC, then is there > really a reason for writecombine to set XN overriding the user? > > That said, pgprot_writecombine() is mostly used for framebuffers, which > arguably shouldn't be executable anyway - but who'd want to mmap() the > framebuffer with PROT_EXEC? > > > > > > Here is my tentative plan: > > > > > > - respin this patch with a small fix to handle the > > > DMA_ATTR_NON_CONSISTENT (as in ignore it unless actually supported), > > > but keep the name as-is to avoid churn. This should allow 5.3 > > > inclusion and backports > > > - remove DMA_ATTR_WRITE_COMBINE support from mips, probably also 5.3 > > > material. > > > - move all architectures but arm over to just define > > > pgprot_dmacoherent, including a comment with the above explanation > > > for arm64. > > > > That would be great, thanks. > > > > > - make DMA_ATTR_WRITE_COMBINE a no-op and schedule it for removal, > > > thus removing the last instances of arch_dma_mmap_pgprot > > > > All sounds good to me, although I suppose 32-bit Arm platforms without > > CONFIG_ARM_DMA_MEM_BUFFERABLE may run into issues if DMA_ATTR_WRITE_COMBINE > > disappears. Only one way to find out... > > Looking at the results of grep, I think only OMAP2+ and Exynos may be > affected. > > However, removing writecombine support from the DMA API is going to > have a huge impact for framebuffers on earlier ARMs - that's where we > do expect framebuffers to be mapped "uncached/buffered" for performance > reasons and not "uncached/unbuffered". It's quite literally the > difference between console scrolling being usable and totally unusable. > > Given what I've said above, switching to using buffered mode for normal > DMA mappings is data-corrupting risky - as in your filesystem could get > fried. I don't think we should play fast and loose with people's data > by randomly changing that "because we'd like to", and I don't see that > screwing the console is really an option either. Sorry, I forgot to explain - the reason is dma_alloc_writecombine() internally uses DMA_ATTR_WRITE_COMBINE, which I'd forgotten about when grepping - so there's potentially way more users than my greps above found. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] dma-mapping: fix page attributes for dma_mmap_* 2019-08-06 16:45 ` Russell King - ARM Linux admin 2019-08-06 16:48 ` Russell King - ARM Linux admin @ 2019-08-07 6:14 ` Christoph Hellwig 1 sibling, 0 replies; 12+ messages in thread From: Christoph Hellwig @ 2019-08-07 6:14 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Will Deacon, Christoph Hellwig, iommu, Shawn Anastasio, Michael Ellerman, Catalin Marinas, Robin Murphy, linuxppc-dev, linux-arm-kernel, linux-kernel On Tue, Aug 06, 2019 at 05:45:03PM +0100, Russell King - ARM Linux admin wrote: > We could have used a different approach, making all IO writes contain > a "drain write buffer" instruction, and map DMA memory as "buffered", > but as there were no Linux barriers defined to order memory accesses > to DMA memory (so, for example, ring buffers can be updated in the > correct order) back in those days, using the uncached/unbuffered mode > was the sanest and most reliable solution. Absolutely makes sense so far. > > > The other really weird things is that in arm32 > > > pgprot_dmacoherent incudes the L_PTE_XN bit, which from my understanding > > > is the no-execture bit, but pgprot_writecombine does not. This seems to > > > not very unintentional. So minus that the whole DMA_ATTR_WRITE_COMBІNE > > > seems to be about flagging old arm specific drivers as having the proper > > > barriers in places and otherwise is a no-op. > > > > I think it only matters for Armv7 CPUs, but yes, we should probably be > > setting L_PTE_XN for both of these memory types. > > Conventionally, pgprot_writecombine() has only been used to change > the memory type and not the permissions. Since writecombine memory > is still capable of being executed, I don't see any reason to set XN > for it. > > If the user wishes to mmap() using PROT_READ|PROT_EXEC, then is there > really a reason for writecombine to set XN overriding the user? > > That said, pgprot_writecombine() is mostly used for framebuffers, which > arguably shouldn't be executable anyway - but who'd want to mmap() the > framebuffer with PROT_EXEC? Well, I was mostly taking about DMA_ATTR_WRITE_COMBINE, which really should include the NX bit even if pgprot_writecombine doesn't, right? > > > - make DMA_ATTR_WRITE_COMBINE a no-op and schedule it for removal, > > > thus removing the last instances of arch_dma_mmap_pgprot > > > > All sounds good to me, although I suppose 32-bit Arm platforms without > > CONFIG_ARM_DMA_MEM_BUFFERABLE may run into issues if DMA_ATTR_WRITE_COMBINE > > disappears. Only one way to find out... > > Looking at the results of grep, I think only OMAP2+ and Exynos may be > affected. As you mentioned later we also have the dma_alloc_wc wrapper, and a single instance of dma_alloc_writecombine. Exynos looks like purely ARM v7 from Kconfig, so it shouldn't even be affected. > However, removing writecombine support from the DMA API is going to > have a huge impact for framebuffers on earlier ARMs - that's where we > do expect framebuffers to be mapped "uncached/buffered" for performance > reasons and not "uncached/unbuffered". It's quite literally the > difference between console scrolling being usable and totally unusable. > > Given what I've said above, switching to using buffered mode for normal > DMA mappings is data-corrupting risky - as in your filesystem could get > fried. I don't think we should play fast and loose with people's data > by randomly changing that "because we'd like to", and I don't see that > screwing the console is really an option either. Oh well. If we can't make dma_alloc_wc generally safe I fear we'll have to keep it, but maybe literally limit it to the pre ARM v6 platforms. While pretty much all callers seems platform specific, there actually are a decent number that can only work on ARM v7 or arm64. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-08-07 6:14 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-01 14:21 fix default dma_mmap_* pgprot Christoph Hellwig 2019-08-01 14:21 ` [PATCH] dma-mapping: fix page attributes for dma_mmap_* Christoph Hellwig 2019-08-01 16:23 ` Will Deacon 2019-08-01 16:34 ` Christoph Hellwig 2019-08-01 16:44 ` Will Deacon 2019-08-02 8:14 ` Christoph Hellwig 2019-08-02 10:38 ` Will Deacon 2019-08-03 6:48 ` Christoph Hellwig 2019-08-06 16:08 ` Will Deacon 2019-08-06 16:45 ` Russell King - ARM Linux admin 2019-08-06 16:48 ` Russell King - ARM Linux admin 2019-08-07 6:14 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).