From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:38:44
Hi all,
this series switches the powerpc port to use the generic swiotlb
and noncoherent dma ops, and to use more generic code for the
coherent direct mapping, as well as removing dead code.
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:38:43
We need to take the DMA offset and encryption bit into account when selecting
a zone. Add a helper that takes those into account and use it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
kernel/dma/direct.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
@@ -70,9 +78,9 @@ void *dma_direct_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,gfp&=~__GFP_ZERO;/* GFP_DMA32 and GFP_DMA are no ops without the corresponding zones: */-if(dev->coherent_dma_mask<=DMA_BIT_MASK(ARCH_ZONE_DMA_BITS))+if(dma_coherent_below(dev,DMA_BIT_MASK(ARCH_ZONE_DMA_BITS)))gfp|=GFP_DMA;-if(dev->coherent_dma_mask<=DMA_BIT_MASK(32)&&!(gfp&GFP_DMA))+if(dma_coherent_below(dev,DMA_BIT_MASK(32)&&!(gfp&GFP_DMA)))gfp|=GFP_DMA32;again:
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:38:45
When a device has a DMA offset the dma capable result will change due
to the difference between the physical and DMA address. Take that into
account.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
kernel/dma/direct.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:38:47
This save some duplication for ia64. In the long run this method will
need some additional work including moving over to kernel/dma, but that
will require some additional prep work, so let's do this minimal change
for now.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/base/platform.c | 11 ++++++++++-
include/linux/dma-mapping.h | 2 --
2 files changed, 10 insertions(+), 3 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:38:49
ia64 can use the generic implementation in general, and SN2 can just
override it in the dma_map_ops now.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/ia64/include/asm/dma-mapping.h | 2 --
arch/ia64/include/asm/machvec.h | 7 -------
arch/ia64/include/asm/machvec_init.h | 1 -
arch/ia64/include/asm/machvec_sn2.h | 2 --
arch/ia64/pci/pci.c | 26 --------------------------
arch/ia64/sn/pci/pci_dma.c | 4 ++--
6 files changed, 2 insertions(+), 40 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:38:50
For now this allows consolidating the powerpc code. In the long run
we should grow a generic implementation of dma_get_required_mask that
returns the dma mask required to avoid bounce buffering.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
kernel/dma/swiotlb.c | 4 ++++
1 file changed, 4 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:38:54
This is need for powerpc for now. Hopefully we can come up with a clean
generic implementation mid-term.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/dma-noncoherent.h | 6 ++++++
kernel/dma/Kconfig | 4 ++++
kernel/dma/noncoherent.c | 1 +
3 files changed, 11 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:06
The implemementation for the CONFIG_NOT_COHERENT_CACHE case doesn't share
any code with the one for systems with coherent caches. Split it off
and merge it with the helpers in dma-noncoherent.c that have no other
callers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/include/asm/dma-mapping.h | 5 -----
arch/powerpc/kernel/dma.c | 14 ++------------
arch/powerpc/mm/dma-noncoherent.c | 15 +++++++--------
arch/powerpc/platforms/44x/warp.c | 2 +-
4 files changed, 10 insertions(+), 26 deletions(-)
@@ -47,7 +47,7 @@ static int __init warp_probe(void)if(!of_machine_is_compatible("pika,warp"))return0;-/* For __dma_alloc_coherent */+/* For __dma_nommu_alloc_coherent */ISA_DMA_THRESHOLD=~0L;return1;
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:08
The requirement to disable local irqs over kmap_atomic is long gone,
so remove those calls.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/mm/dma-noncoherent.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:09
Use the standard portable helper instead of the powerpc specific one,
which is about to go away.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/kernel/dma-swiotlb.c | 5 ++---
arch/powerpc/kernel/dma.c | 12 ++++++------
2 files changed, 8 insertions(+), 9 deletions(-)
@@ -43,7 +43,7 @@ static u64 __maybe_unused get_pfn_limit(struct device *dev)staticintdma_nommu_dma_supported(structdevice*dev,u64mask){#ifdef CONFIG_PPC64-u64limit=get_dma_offset(dev)+(memblock_end_of_DRAM()-1);+u64limit=phys_to_dma(dev,(memblock_end_of_DRAM()-1));/* Limit fits in the mask, we are good */if(mask>=limit)
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:16
Just fold the calculation into __phys_to_dma/__dma_to_phys as those are
the only places that should know about it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/include/asm/dma-direct.h | 8 ++++++--
arch/powerpc/include/asm/dma-mapping.h | 16 ----------------
2 files changed, 6 insertions(+), 18 deletions(-)
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:17
The ppc32 case of dma_nommu_dma_supported already was a no-op, and the
64-bit case came to the same conclusion as dma_direct_supported, so
replace it with the generic version.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/kernel/dma.c | 28 +++-------------------------
2 files changed, 4 insertions(+), 25 deletions(-)
@@ -40,28 +40,6 @@ static u64 __maybe_unused get_pfn_limit(struct device *dev)returnpfn;}-staticintdma_nommu_dma_supported(structdevice*dev,u64mask)-{-#ifdef CONFIG_PPC64-u64limit=phys_to_dma(dev,(memblock_end_of_DRAM()-1));--/* Limit fits in the mask, we are good */-if(mask>=limit)-return1;--#ifdef CONFIG_FSL_SOC-/* Freescale gets another chance via ZONE_DMA/ZONE_DMA32, however-*thatwillhavetoberefinedif/whentheysupportiommus-*/-return1;-#endif-/* Sorry ... */-return0;-#else-return1;-#endif-}-#ifndef CONFIG_NOT_COHERENT_CACHEvoid*__dma_nommu_alloc_coherent(structdevice*dev,size_tsize,dma_addr_t*dma_handle,gfp_tflag,
@@ -126,7 +104,7 @@ static void *dma_nommu_alloc_coherent(struct device *dev, size_t size,/* The coherent mask may be smaller than the real mask, check if*wecanreallyusethedirectops*/-if(dma_nommu_dma_supported(dev,dev->coherent_dma_mask))+if(dma_direct_supported(dev,dev->coherent_dma_mask))return__dma_nommu_alloc_coherent(dev,size,dma_handle,flag,attrs);
@@ -148,7 +126,7 @@ static void dma_nommu_free_coherent(struct device *dev, size_t size,structiommu_table*iommu;/* See comments in dma_nommu_alloc_coherent() */-if(dma_nommu_dma_supported(dev,dev->coherent_dma_mask))+if(dma_direct_supported(dev,dev->coherent_dma_mask))return__dma_nommu_free_coherent(dev,size,vaddr,dma_handle,attrs);/* Maybe we used an iommu ... */
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:20
These do the same functionality as the existing helpers, but do it
simpler, and also allow the (optional) use of CMA.
Note that the swiotlb code now calls into the dma_direct code directly,
given that it doesn't work with noncoherent caches at all, and isn't called
when we have an iommu either, so the iommu special case in
dma_nommu_alloc_coherent isn't required for swiotlb.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/include/asm/pgtable.h | 1 -
arch/powerpc/kernel/dma-swiotlb.c | 4 +-
arch/powerpc/kernel/dma.c | 78 ++++--------------------------
arch/powerpc/mm/mem.c | 19 --------
4 files changed, 11 insertions(+), 91 deletions(-)
@@ -26,75 +26,6 @@*cansetarchdata.dma_datatoanunsignedlongholdingtheoffset.By*defaulttheoffsetisPCI_DRAM_OFFSET.*/--staticu64__maybe_unusedget_pfn_limit(structdevice*dev)-{-u64pfn=(dev->coherent_dma_mask>>PAGE_SHIFT)+1;-structdev_archdata__maybe_unused*sd=&dev->archdata;--#ifdef CONFIG_SWIOTLB-if(sd->max_direct_dma_addr&&dev->dma_ops==&powerpc_swiotlb_dma_ops)-pfn=min_t(u64,pfn,sd->max_direct_dma_addr>>PAGE_SHIFT);-#endif--returnpfn;-}--#ifndef CONFIG_NOT_COHERENT_CACHE-void*__dma_nommu_alloc_coherent(structdevice*dev,size_tsize,-dma_addr_t*dma_handle,gfp_tflag,-unsignedlongattrs)-{-void*ret;-structpage*page;-intnode=dev_to_node(dev);-#ifdef CONFIG_FSL_SOC-u64pfn=get_pfn_limit(dev);-intzone;--/*-*ThiscodeshouldbeOKonotherplatforms,butwehavedriversthat-*don'tsetcoherent_dma_mask.Asaworkaroundwejustifdefit.This-*wholeroutineneedssomeseriouscleanup.-*/--zone=dma_pfn_limit_to_zone(pfn);-if(zone<0){-dev_err(dev,"%s: No suitable zone for pfn %#llx\n",-__func__,pfn);-returnNULL;-}--switch(zone){-caseZONE_DMA:-flag|=GFP_DMA;-break;-#ifdef CONFIG_ZONE_DMA32-caseZONE_DMA32:-flag|=GFP_DMA32;-break;-#endif-};-#endif /* CONFIG_FSL_SOC */--page=alloc_pages_node(node,flag,get_order(size));-if(page==NULL)-returnNULL;-ret=page_address(page);-memset(ret,0,size);-*dma_handle=phys_to_dma(dev,__pa(ret));--returnret;-}--void__dma_nommu_free_coherent(structdevice*dev,size_tsize,-void*vaddr,dma_addr_tdma_handle,-unsignedlongattrs)-{-free_pages((unsignedlong)vaddr,get_order(size));-}-#endif /* !CONFIG_NOT_COHERENT_CACHE */-staticvoid*dma_nommu_alloc_coherent(structdevice*dev,size_tsize,dma_addr_t*dma_handle,gfp_tflag,unsignedlongattrs)
@@ -105,8 +36,12 @@ static void *dma_nommu_alloc_coherent(struct device *dev, size_t size,*wecanreallyusethedirectops*/if(dma_direct_supported(dev,dev->coherent_dma_mask))+#ifdef CONFIG_NOT_COHERENT_CACHEreturn__dma_nommu_alloc_coherent(dev,size,dma_handle,flag,attrs);+#else+returndma_direct_alloc(dev,size,dma_handle,flag,attrs);+#endif/* Ok we can't ... do we have an iommu ? If not, fail */iommu=get_iommu_table_base(dev);
@@ -127,8 +62,13 @@ static void dma_nommu_free_coherent(struct device *dev, size_t size,/* See comments in dma_nommu_alloc_coherent() */if(dma_direct_supported(dev,dev->coherent_dma_mask))+#ifdef CONFIG_NOT_COHERENT_CACHEreturn__dma_nommu_free_coherent(dev,size,vaddr,dma_handle,attrs);+#else+returndma_direct_free(dev,size,vaddr,dma_handle,attrs);+#endif+/* Maybe we used an iommu ... */iommu=get_iommu_table_base(dev);
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:21
These methods are optional to start with, no need to implement no-op
versions.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/kernel/dma.c | 16 ----------------
1 file changed, 16 deletions(-)
@@ -88,7 +64,7 @@ static int ppc_swiotlb_bus_notify(struct notifier_block *nb,/* May need to bounce if the device can't address all of DRAM */if((dma_get_mask(dev)+1)<memblock_end_of_DRAM())-set_dma_ops(dev,&powerpc_swiotlb_dma_ops);+set_dma_ops(dev,&swiotlb_dma_ops);returnNOTIFY_DONE;}
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:27
The generic dma-noncoherent code provides all that is needed by powerpc.
Note that the cache maintainance in the existing code is a bit odd
as it implements both the sync_to_device and sync_to_cpu callouts,
but never flushes caches when unmapping. This patch keeps both
directions arounds, which will lead to more flushing than the previous
implementation. Someone more familar with the required CPUs should
eventually take a look and optimize the cache flush handling if needed.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/dma-mapping.h | 29 -------------
arch/powerpc/kernel/dma.c | 59 +++-----------------------
arch/powerpc/kernel/pci-common.c | 5 ++-
arch/powerpc/kernel/setup-common.c | 4 ++
arch/powerpc/mm/dma-noncoherent.c | 52 +++++++++++++++++------
arch/powerpc/platforms/44x/warp.c | 2 +-
arch/powerpc/platforms/Kconfig.cputype | 6 ++-
8 files changed, 60 insertions(+), 99 deletions(-)
@@ -19,40 +19,11 @@#include<asm/swiotlb.h>/* Some dma direct funcs must be visible for use in other dma_ops */-externvoid*__dma_nommu_alloc_coherent(structdevice*dev,size_tsize,-dma_addr_t*dma_handle,gfp_tflag,-unsignedlongattrs);-externvoid__dma_nommu_free_coherent(structdevice*dev,size_tsize,-void*vaddr,dma_addr_tdma_handle,-unsignedlongattrs);externintdma_nommu_mmap_coherent(structdevice*dev,structvm_area_struct*vma,void*cpu_addr,dma_addr_thandle,size_tsize,unsignedlongattrs);-#ifdef CONFIG_NOT_COHERENT_CACHE-/*-*DMA-consistentmappingfunctionsforPowerPCsthatdon'tsupport-*cachesnooping.Theseallocate/freearegionofuncachedmapped-*memoryspaceforusewithDMAdevices.Alternatively,youcould-*allocatethespace"normally"andusethecachemanagementfunctions-*toensureitisconsistent.-*/-structdevice;-externvoid__dma_sync(void*vaddr,size_tsize,intdirection);-externvoid__dma_sync_page(structpage*page,unsignedlongoffset,-size_tsize,intdirection);-externunsignedlong__dma_get_coherent_pfn(unsignedlongcpu_addr);--#else /* ! CONFIG_NOT_COHERENT_CACHE */-/*-*Cachecoherentcores.-*/--#define __dma_sync(addr, size, rw) ((void)0)-#define __dma_sync_page(pg, off, sz, rw) ((void)0)--#endif /* ! CONFIG_NOT_COHERENT_CACHE */staticinlineunsignedlongdevice_to_mask(structdevice*dev){
@@ -36,12 +36,7 @@ static void *dma_nommu_alloc_coherent(struct device *dev, size_t size,*wecanreallyusethedirectops*/if(dma_direct_supported(dev,dev->coherent_dma_mask))-#ifdef CONFIG_NOT_COHERENT_CACHE-return__dma_nommu_alloc_coherent(dev,size,dma_handle,-flag,attrs);-#elsereturndma_direct_alloc(dev,size,dma_handle,flag,attrs);-#endif/* Ok we can't ... do we have an iommu ? If not, fail */iommu=get_iommu_table_base(dev);
@@ -62,12 +57,7 @@ static void dma_nommu_free_coherent(struct device *dev, size_t size,/* See comments in dma_nommu_alloc_coherent() */if(dma_direct_supported(dev,dev->coherent_dma_mask))-#ifdef CONFIG_NOT_COHERENT_CACHE-return__dma_nommu_free_coherent(dev,size,vaddr,dma_handle,-attrs);-#elsereturndma_direct_free(dev,size,vaddr,dma_handle,attrs);-#endif/* Maybe we used an iommu ... */iommu=get_iommu_table_base(dev);
@@ -108,17 +92,13 @@ static int dma_nommu_map_sg(struct device *dev, struct scatterlist *sgl,for_each_sg(sgl,sg,nents,i){sg->dma_address=phys_to_dma(dev,sg_phys(sg));sg->dma_length=sg->length;--if(attrs&DMA_ATTR_SKIP_CPU_SYNC)-continue;--__dma_sync_page(sg_page(sg),sg->offset,sg->length,direction);}returnnents;}-staticu64dma_nommu_get_required_mask(structdevice*dev)+/* note: needs to be called arch_get_required_mask for dma-noncoherent.c */+u64arch_get_required_mask(structdevice*dev){u64end,mask;
@@ -382,23 +382,36 @@ static inline void __dma_sync_page_highmem(struct page *page,*__dma_sync_pagemakesmemoryconsistent.identicalto__dma_sync,but*takesastructpageinsteadofavirtualaddress*/-void__dma_sync_page(structpage*page,unsignedlongoffset,-size_tsize,intdirection)+staticvoid__dma_sync_page(phys_addr_tpaddr,size_tsize,intdir){+structpage*page=pfn_to_page(paddr>>PAGE_SHIFT);+unsignedoffset=paddr&~PAGE_MASK;+#ifdef CONFIG_HIGHMEM-__dma_sync_page_highmem(page,offset,size,direction);+__dma_sync_page_highmem(page,offset,size,dir);#elseunsignedlongstart=(unsignedlong)page_address(page)+offset;-__dma_sync((void*)start,size,direction);+__dma_sync((void*)start,size,dir);#endif}-EXPORT_SYMBOL(__dma_sync_page);++voidarch_sync_dma_for_device(structdevice*dev,phys_addr_tpaddr,+size_tsize,enumdma_data_directiondir)+{+__dma_sync_page(paddr,size,dir);+}++voidarch_sync_dma_for_cpu(structdevice*dev,phys_addr_tpaddr,+size_tsize,enumdma_data_directiondir)+{+__dma_sync_page(paddr,size,dir);+}/*-*ReturnthePFNforagivencpuvirtualaddressreturnedby-*__dma_nommu_alloc_coherent.Thisisusedbydma_mmap_coherent()+*ReturnthePFNforagivencpuvirtualaddressreturnedby__arch_dma_alloc.+*Thisisusedbydma_mmap_coherent()*/-unsignedlong__dma_get_coherent_pfn(unsignedlongcpu_addr)+staticunsignedlong__dma_get_coherent_pfn(unsignedlongcpu_addr){/* This should always be populated, so we don't test every*level.Ifthatfails,we'llhaveanicecrashwhich
@@ -413,3 +426,16 @@ unsigned long __dma_get_coherent_pfn(unsigned long cpu_addr)return0;returnpte_pfn(*ptep);}++intarch_dma_mmap(structdevice*dev,structvm_area_struct*vma,+void*cpu_addr,dma_addr_thandle,size_tsize,+unsignedlongattrs)+{+unsignedlongpfn=__dma_get_coherent_pfn((unsignedlong)cpu_addr);++vma->vm_page_prot=pgprot_noncached(vma->vm_page_prot);+returnremap_pfn_range(vma,vma->vm_start,+pfn+vma->vm_pgoff,+vma->vm_end-vma->vm_start,+vma->vm_page_prot);+}
@@ -47,7 +47,7 @@ static int __init warp_probe(void)if(!of_machine_is_compatible("pika,warp"))return0;-/* For __dma_nommu_alloc_coherent */+/* For arch_dma_alloc */ISA_DMA_THRESHOLD=~0L;return1;
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-30 16:39:29
These are indentical except for additional error checking, so migrate
to the common code, and wire up the get_mapping_error method as well.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/kernel/dma.c | 32 ++++----------------------------
1 file changed, 4 insertions(+), 28 deletions(-)
@@ -82,21 +82,6 @@ int dma_nommu_mmap_coherent(struct device *dev, struct vm_area_struct *vma,vma->vm_page_prot);}-staticintdma_nommu_map_sg(structdevice*dev,structscatterlist*sgl,-intnents,enumdma_data_directiondirection,-unsignedlongattrs)-{-structscatterlist*sg;-inti;--for_each_sg(sgl,sg,nents,i){-sg->dma_address=phys_to_dma(dev,sg_phys(sg));-sg->dma_length=sg->length;-}--returnnents;-}-/* note: needs to be called arch_get_required_mask for dma-noncoherent.c */u64arch_get_required_mask(structdevice*dev){
@@ -18,13 +18,6 @@#include<asm/io.h>#include<asm/swiotlb.h>-/* Some dma direct funcs must be visible for use in other dma_ops */-externintdma_nommu_mmap_coherent(structdevice*dev,-structvm_area_struct*vma,-void*cpu_addr,dma_addr_thandle,-size_tsize,unsignedlongattrs);--staticinlineunsignedlongdevice_to_mask(structdevice*dev){if(dev->dma_mask&&*dev->dma_mask)
@@ -70,18 +70,6 @@ static void dma_nommu_free_coherent(struct device *dev, size_t size,iommu_free_coherent(iommu,size,vaddr,dma_handle);}-intdma_nommu_mmap_coherent(structdevice*dev,structvm_area_struct*vma,-void*cpu_addr,dma_addr_thandle,size_tsize,-unsignedlongattrs)-{-unsignedlongpfn=page_to_pfn(virt_to_page(cpu_addr));--returnremap_pfn_range(vma,vma->vm_start,-pfn+vma->vm_pgoff,-vma->vm_end-vma->vm_start,-vma->vm_page_prot);-}-/* note: needs to be called arch_get_required_mask for dma-noncoherent.c */u64arch_get_required_mask(structdevice*dev){
From: Christoph Hellwig <hch@lst.de> Date: 2018-07-31 12:12:03
It turns out cxl actually uses it. So for now skip this patch,
although random code in drivers messing with dma ops will need to
be sorted out sooner or later.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-08 23:44:40
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
When a device has a DMA offset the dma capable result will change due
to the difference between the physical and DMA address. Take that into
account.
The patch in itself makes sense.
However, there are a number of things in that dma_direct.c file that I
don't quite get:
- looking more generally at what that function does, I worry about the
switch of ppc32 to this later on:
We do have the occasional device with things like 31-bit DMA
limitation. We know they happens to work because those systems
can't have enough memory to be a problem. This is why our current
DMA direct ops in powerpc just unconditionally return true on ppc32.
The test against a full 32-bit mask here will break them I think.
Thing is, I'm not sure I still have access to one of these things
to test, I'll have to dig (from memory things like b43 wifi).
Also those platforms don't have an iommu.
- What is this trying to achieve ?
/*
* Various PCI/PCIe bridges have broken support for > 32bit DMA even
* if the device itself might support it.
*/
if (dev->dma_32bit_limit && mask > phys_to_dma(dev, DMA_BIT_MASK(32)))
return 0;
IE, if the device has a 32-bit limit, we fail an attempt at checking
if a >32-bit mask works ? That doesn't quite seem to be the right thing
to do... Shouldn't this be in dma_set_mask() and just clamp the mask down ?
IE, dma_set_mask() is what a driver uses to establish the device capability,
so it makes sense tot have dma_32bit_limit just reduce that capability, not
fail because the device can do more than what the bridge can....
Sorry if I'm a bit confused here.
- How is that file supposed to work on 64-bit platforms ? From what I can
tell, dma_supported() will unconditionally return true if the mask is
32-bit or larger (appart from the above issue). This doesn't look right,
the mask needs to be compared to the max memory address. There are a bunch
of devices out there with masks anywhere bettween 40 and 64 bits, and
some of these will not work "out of the box" if the offseted top
of memory is beyond the mask limit. Or am I missing something ?
Cheers,
Ben.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-08 23:55:00
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
We need to take the DMA offset and encryption bit into account when selecting
a zone. Add a helper that takes those into account and use it.
That whole "encryption" stuff seems to be completely specific to the
way x86 does memory encryption, or am I mistaken ? It's not clear to me
what that does in practice and how it relates to DMA mappings.
I'm also not sure about that whole business with ZONE_DMA and
ARCH_ZONE_DMA_BITS...
On ppc64, unless you enable swiotlb (which we only do currently on
some embedded platforms), you have all of memory in ZONE_DMA.
[ 0.000000] Zone ranges:
[ 0.000000] DMA [mem 0x0000000000000000-0x0000001fffffffff]
[ 0.000000] DMA32 empty
[ 0.000000] Normal empty
[ 0.000000] Device empty
I'm not sure how this will work with that dma direct code.
I also see a number of tests against a 64-bit mask rather than the
top of memory...
Ben.
@@ -70,9 +78,9 @@ void *dma_direct_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,gfp&=~__GFP_ZERO;/* GFP_DMA32 and GFP_DMA are no ops without the corresponding zones: */-if(dev->coherent_dma_mask<=DMA_BIT_MASK(ARCH_ZONE_DMA_BITS))+if(dma_coherent_below(dev,DMA_BIT_MASK(ARCH_ZONE_DMA_BITS)))gfp|=GFP_DMA;-if(dev->coherent_dma_mask<=DMA_BIT_MASK(32)&&!(gfp&GFP_DMA))+if(dma_coherent_below(dev,DMA_BIT_MASK(32)&&!(gfp&GFP_DMA)))gfp|=GFP_DMA32;again:
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 00:01:37
On Tue, 2018-07-31 at 14:16 +0200, Christoph Hellwig wrote:
It turns out cxl actually uses it. So for now skip this patch,
although random code in drivers messing with dma ops will need to
be sorted out sooner or later.
CXL devices are "special", they bypass the classic iommu in favor of
allowing the device to operate using the main processor page tables
using an MMU context (so basically the device can use userspace
addresses directly), akin to ATS.
I think the code currently uses the nommu ops as a way to do a simple
kernel mapping for kernel drivers using CXL (not userspace stuff)
though.
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 00:28:07
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
The requirement to disable local irqs over kmap_atomic is long gone,
so remove those calls.
Really ? I'm trying to verify that and getting lost in a mess of macros
from hell in the per-cpu stuff but if you look at our implementation
of kmap_atomic_prot(), all it does is a preempt_disable(), and then
it uses kmap_atomic_idx_push():
int idx = __this_cpu_inc_return(__kmap_atomic_idx) - 1;
Note the use of __this_cpu_inc_return(), not this_cpu_inc_return(),
ie this is the non-interrupt safe version...
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 00:40:35
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
The implemementation for the CONFIG_NOT_COHERENT_CACHE case doesn't share
any code with the one for systems with coherent caches. Split it off
and merge it with the helpers in dma-noncoherent.c that have no other
callers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
@@ -47,7 +47,7 @@ static int __init warp_probe(void)if(!of_machine_is_compatible("pika,warp"))return0;-/* For __dma_alloc_coherent */+/* For __dma_nommu_alloc_coherent */ISA_DMA_THRESHOLD=~0L;return1;
@@ -43,7 +43,7 @@ static u64 __maybe_unused get_pfn_limit(struct device *dev)staticintdma_nommu_dma_supported(structdevice*dev,u64mask){#ifdef CONFIG_PPC64-u64limit=get_dma_offset(dev)+(memblock_end_of_DRAM()-1);+u64limit=phys_to_dma(dev,(memblock_end_of_DRAM()-1));/* Limit fits in the mask, we are good */if(mask>=limit)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 00:45:44
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
Just fold the calculation into __phys_to_dma/__dma_to_phys as those are
the only places that should know about it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 00:49:48
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
The ppc32 case of dma_nommu_dma_supported already was a no-op, and the
64-bit case came to the same conclusion as dma_direct_supported, so
replace it with the generic version.
It's not at all equivalent (see my review on your earlier patch) or
am I missing something ?
- ppc32 always return 1, but dma_direct_supported() will not for
devices with a <32-bit mask (and yes ppc32 isn't quite right to do
so, it should check against memory size, but in practice it worked
as the only limited devices we deal with on systems we still support
have a 31-bit limitation)
- ppc64 needs to check against the end of DRAM as some devices will
fail the check, dma_direct_supported() doesn't seem to be doing that.
Also as I mentioned, I'm not sure about the business with ZONE_DMA,
and that arbitrary 24-bit limit since our entire memory is in ZONE_DMA
but that's a different can of worms I suppose.
@@ -40,28 +40,6 @@ static u64 __maybe_unused get_pfn_limit(struct device *dev)returnpfn;}-staticintdma_nommu_dma_supported(structdevice*dev,u64mask)-{-#ifdef CONFIG_PPC64-u64limit=phys_to_dma(dev,(memblock_end_of_DRAM()-1));--/* Limit fits in the mask, we are good */-if(mask>=limit)-return1;--#ifdef CONFIG_FSL_SOC-/* Freescale gets another chance via ZONE_DMA/ZONE_DMA32, however-*thatwillhavetoberefinedif/whentheysupportiommus-*/-return1;-#endif-/* Sorry ... */-return0;-#else-return1;-#endif-}-#ifndef CONFIG_NOT_COHERENT_CACHEvoid*__dma_nommu_alloc_coherent(structdevice*dev,size_tsize,dma_addr_t*dma_handle,gfp_tflag,
@@ -126,7 +104,7 @@ static void *dma_nommu_alloc_coherent(struct device *dev, size_t size,/* The coherent mask may be smaller than the real mask, check if*wecanreallyusethedirectops*/-if(dma_nommu_dma_supported(dev,dev->coherent_dma_mask))+if(dma_direct_supported(dev,dev->coherent_dma_mask))return__dma_nommu_alloc_coherent(dev,size,dma_handle,flag,attrs);
@@ -148,7 +126,7 @@ static void dma_nommu_free_coherent(struct device *dev, size_t size,structiommu_table*iommu;/* See comments in dma_nommu_alloc_coherent() */-if(dma_nommu_dma_supported(dev,dev->coherent_dma_mask))+if(dma_direct_supported(dev,dev->coherent_dma_mask))return__dma_nommu_free_coherent(dev,size,vaddr,dma_handle,attrs);/* Maybe we used an iommu ... */
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 00:53:20
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
These do the same functionality as the existing helpers, but do it
simpler, and also allow the (optional) use of CMA.
Note that the swiotlb code now calls into the dma_direct code directly,
given that it doesn't work with noncoherent caches at all, and isn't called
when we have an iommu either, so the iommu special case in
dma_nommu_alloc_coherent isn't required for swiotlb.
I am not convinced that this will produce the same results due to
the way the zone picking works.
As for the interaction with swiotlb, we'll need the FSL guys to have
a look. Scott, do you remember what this is about ?
@@ -26,75 +26,6 @@*cansetarchdata.dma_datatoanunsignedlongholdingtheoffset.By*defaulttheoffsetisPCI_DRAM_OFFSET.*/--staticu64__maybe_unusedget_pfn_limit(structdevice*dev)-{-u64pfn=(dev->coherent_dma_mask>>PAGE_SHIFT)+1;-structdev_archdata__maybe_unused*sd=&dev->archdata;--#ifdef CONFIG_SWIOTLB-if(sd->max_direct_dma_addr&&dev->dma_ops==&powerpc_swiotlb_dma_ops)-pfn=min_t(u64,pfn,sd->max_direct_dma_addr>>PAGE_SHIFT);-#endif--returnpfn;-}--#ifndef CONFIG_NOT_COHERENT_CACHE-void*__dma_nommu_alloc_coherent(structdevice*dev,size_tsize,-dma_addr_t*dma_handle,gfp_tflag,-unsignedlongattrs)-{-void*ret;-structpage*page;-intnode=dev_to_node(dev);-#ifdef CONFIG_FSL_SOC-u64pfn=get_pfn_limit(dev);-intzone;--/*-*ThiscodeshouldbeOKonotherplatforms,butwehavedriversthat-*don'tsetcoherent_dma_mask.Asaworkaroundwejustifdefit.This-*wholeroutineneedssomeseriouscleanup.-*/--zone=dma_pfn_limit_to_zone(pfn);-if(zone<0){-dev_err(dev,"%s: No suitable zone for pfn %#llx\n",-__func__,pfn);-returnNULL;-}--switch(zone){-caseZONE_DMA:-flag|=GFP_DMA;-break;-#ifdef CONFIG_ZONE_DMA32-caseZONE_DMA32:-flag|=GFP_DMA32;-break;-#endif-};-#endif /* CONFIG_FSL_SOC */--page=alloc_pages_node(node,flag,get_order(size));-if(page==NULL)-returnNULL;-ret=page_address(page);-memset(ret,0,size);-*dma_handle=phys_to_dma(dev,__pa(ret));--returnret;-}--void__dma_nommu_free_coherent(structdevice*dev,size_tsize,-void*vaddr,dma_addr_tdma_handle,-unsignedlongattrs)-{-free_pages((unsignedlong)vaddr,get_order(size));-}-#endif /* !CONFIG_NOT_COHERENT_CACHE */-staticvoid*dma_nommu_alloc_coherent(structdevice*dev,size_tsize,dma_addr_t*dma_handle,gfp_tflag,unsignedlongattrs)
@@ -105,8 +36,12 @@ static void *dma_nommu_alloc_coherent(struct device *dev, size_t size,*wecanreallyusethedirectops*/if(dma_direct_supported(dev,dev->coherent_dma_mask))+#ifdef CONFIG_NOT_COHERENT_CACHEreturn__dma_nommu_alloc_coherent(dev,size,dma_handle,flag,attrs);+#else+returndma_direct_alloc(dev,size,dma_handle,flag,attrs);+#endif/* Ok we can't ... do we have an iommu ? If not, fail */iommu=get_iommu_table_base(dev);
@@ -127,8 +62,13 @@ static void dma_nommu_free_coherent(struct device *dev, size_t size,/* See comments in dma_nommu_alloc_coherent() */if(dma_direct_supported(dev,dev->coherent_dma_mask))+#ifdef CONFIG_NOT_COHERENT_CACHEreturn__dma_nommu_free_coherent(dev,size,vaddr,dma_handle,attrs);+#else+returndma_direct_free(dev,size,vaddr,dma_handle,attrs);+#endif+/* Maybe we used an iommu ... */iommu=get_iommu_table_base(dev);
@@ -88,7 +64,7 @@ static int ppc_swiotlb_bus_notify(struct notifier_block *nb,/* May need to bounce if the device can't address all of DRAM */if((dma_get_mask(dev)+1)<memblock_end_of_DRAM())-set_dma_ops(dev,&powerpc_swiotlb_dma_ops);+set_dma_ops(dev,&swiotlb_dma_ops);returnNOTIFY_DONE;}
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 01:00:50
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
The generic dma-noncoherent code provides all that is needed by powerpc.
Note that the cache maintainance in the existing code is a bit odd
as it implements both the sync_to_device and sync_to_cpu callouts,
but never flushes caches when unmapping. This patch keeps both
directions arounds, which will lead to more flushing than the previous
implementation. Someone more familar with the required CPUs should
eventually take a look and optimize the cache flush handling if needed.
The original code looks bogus indeed.
I think we got away with it because those older CPUs wouldn't speculate
or prefetch aggressively enough (or at all) so the flush on map was
sufficient, the stuff wouldn't come back into the cache.
But safe is better than sorry, so ... tentative Ack, I do need to try
to dig one of these things to test, which might take a while.
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
@@ -19,40 +19,11 @@#include<asm/swiotlb.h>/* Some dma direct funcs must be visible for use in other dma_ops */-externvoid*__dma_nommu_alloc_coherent(structdevice*dev,size_tsize,-dma_addr_t*dma_handle,gfp_tflag,-unsignedlongattrs);-externvoid__dma_nommu_free_coherent(structdevice*dev,size_tsize,-void*vaddr,dma_addr_tdma_handle,-unsignedlongattrs);externintdma_nommu_mmap_coherent(structdevice*dev,structvm_area_struct*vma,void*cpu_addr,dma_addr_thandle,size_tsize,unsignedlongattrs);-#ifdef CONFIG_NOT_COHERENT_CACHE-/*-*DMA-consistentmappingfunctionsforPowerPCsthatdon'tsupport-*cachesnooping.Theseallocate/freearegionofuncachedmapped-*memoryspaceforusewithDMAdevices.Alternatively,youcould-*allocatethespace"normally"andusethecachemanagementfunctions-*toensureitisconsistent.-*/-structdevice;-externvoid__dma_sync(void*vaddr,size_tsize,intdirection);-externvoid__dma_sync_page(structpage*page,unsignedlongoffset,-size_tsize,intdirection);-externunsignedlong__dma_get_coherent_pfn(unsignedlongcpu_addr);--#else /* ! CONFIG_NOT_COHERENT_CACHE */-/*-*Cachecoherentcores.-*/--#define __dma_sync(addr, size, rw) ((void)0)-#define __dma_sync_page(pg, off, sz, rw) ((void)0)--#endif /* ! CONFIG_NOT_COHERENT_CACHE */staticinlineunsignedlongdevice_to_mask(structdevice*dev){
@@ -36,12 +36,7 @@ static void *dma_nommu_alloc_coherent(struct device *dev, size_t size,*wecanreallyusethedirectops*/if(dma_direct_supported(dev,dev->coherent_dma_mask))-#ifdef CONFIG_NOT_COHERENT_CACHE-return__dma_nommu_alloc_coherent(dev,size,dma_handle,-flag,attrs);-#elsereturndma_direct_alloc(dev,size,dma_handle,flag,attrs);-#endif/* Ok we can't ... do we have an iommu ? If not, fail */iommu=get_iommu_table_base(dev);
@@ -62,12 +57,7 @@ static void dma_nommu_free_coherent(struct device *dev, size_t size,/* See comments in dma_nommu_alloc_coherent() */if(dma_direct_supported(dev,dev->coherent_dma_mask))-#ifdef CONFIG_NOT_COHERENT_CACHE-return__dma_nommu_free_coherent(dev,size,vaddr,dma_handle,-attrs);-#elsereturndma_direct_free(dev,size,vaddr,dma_handle,attrs);-#endif/* Maybe we used an iommu ... */iommu=get_iommu_table_base(dev);
@@ -108,17 +92,13 @@ static int dma_nommu_map_sg(struct device *dev, struct scatterlist *sgl,for_each_sg(sgl,sg,nents,i){sg->dma_address=phys_to_dma(dev,sg_phys(sg));sg->dma_length=sg->length;--if(attrs&DMA_ATTR_SKIP_CPU_SYNC)-continue;--__dma_sync_page(sg_page(sg),sg->offset,sg->length,direction);}returnnents;}-staticu64dma_nommu_get_required_mask(structdevice*dev)+/* note: needs to be called arch_get_required_mask for dma-noncoherent.c */+u64arch_get_required_mask(structdevice*dev){u64end,mask;
@@ -382,23 +382,36 @@ static inline void __dma_sync_page_highmem(struct page *page,*__dma_sync_pagemakesmemoryconsistent.identicalto__dma_sync,but*takesastructpageinsteadofavirtualaddress*/-void__dma_sync_page(structpage*page,unsignedlongoffset,-size_tsize,intdirection)+staticvoid__dma_sync_page(phys_addr_tpaddr,size_tsize,intdir){+structpage*page=pfn_to_page(paddr>>PAGE_SHIFT);+unsignedoffset=paddr&~PAGE_MASK;+#ifdef CONFIG_HIGHMEM-__dma_sync_page_highmem(page,offset,size,direction);+__dma_sync_page_highmem(page,offset,size,dir);#elseunsignedlongstart=(unsignedlong)page_address(page)+offset;-__dma_sync((void*)start,size,direction);+__dma_sync((void*)start,size,dir);#endif}-EXPORT_SYMBOL(__dma_sync_page);++voidarch_sync_dma_for_device(structdevice*dev,phys_addr_tpaddr,+size_tsize,enumdma_data_directiondir)+{+__dma_sync_page(paddr,size,dir);+}++voidarch_sync_dma_for_cpu(structdevice*dev,phys_addr_tpaddr,+size_tsize,enumdma_data_directiondir)+{+__dma_sync_page(paddr,size,dir);+}/*-*ReturnthePFNforagivencpuvirtualaddressreturnedby-*__dma_nommu_alloc_coherent.Thisisusedbydma_mmap_coherent()+*ReturnthePFNforagivencpuvirtualaddressreturnedby__arch_dma_alloc.+*Thisisusedbydma_mmap_coherent()*/-unsignedlong__dma_get_coherent_pfn(unsignedlongcpu_addr)+staticunsignedlong__dma_get_coherent_pfn(unsignedlongcpu_addr){/* This should always be populated, so we don't test every*level.Ifthatfails,we'llhaveanicecrashwhich
@@ -413,3 +426,16 @@ unsigned long __dma_get_coherent_pfn(unsigned long cpu_addr)return0;returnpte_pfn(*ptep);}++intarch_dma_mmap(structdevice*dev,structvm_area_struct*vma,+void*cpu_addr,dma_addr_thandle,size_tsize,+unsignedlongattrs)+{+unsignedlongpfn=__dma_get_coherent_pfn((unsignedlong)cpu_addr);++vma->vm_page_prot=pgprot_noncached(vma->vm_page_prot);+returnremap_pfn_range(vma,vma->vm_start,+pfn+vma->vm_pgoff,+vma->vm_end-vma->vm_start,+vma->vm_page_prot);+}
@@ -47,7 +47,7 @@ static int __init warp_probe(void)if(!of_machine_is_compatible("pika,warp"))return0;-/* For __dma_nommu_alloc_coherent */+/* For arch_dma_alloc */ISA_DMA_THRESHOLD=~0L;return1;
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 01:05:44
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
The remaining implementation for coherent caches is functionally
identical to the default provided in common code.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
@@ -18,13 +18,6 @@#include<asm/io.h>#include<asm/swiotlb.h>-/* Some dma direct funcs must be visible for use in other dma_ops */-externintdma_nommu_mmap_coherent(structdevice*dev,-structvm_area_struct*vma,-void*cpu_addr,dma_addr_thandle,-size_tsize,unsignedlongattrs);--staticinlineunsignedlongdevice_to_mask(structdevice*dev){if(dev->dma_mask&&*dev->dma_mask)
@@ -70,18 +70,6 @@ static void dma_nommu_free_coherent(struct device *dev, size_t size,iommu_free_coherent(iommu,size,vaddr,dma_handle);}-intdma_nommu_mmap_coherent(structdevice*dev,structvm_area_struct*vma,-void*cpu_addr,dma_addr_thandle,size_tsize,-unsignedlongattrs)-{-unsignedlongpfn=page_to_pfn(virt_to_page(cpu_addr));--returnremap_pfn_range(vma,vma->vm_start,-pfn+vma->vm_pgoff,-vma->vm_end-vma->vm_start,-vma->vm_page_prot);-}-/* note: needs to be called arch_get_required_mask for dma-noncoherent.c */u64arch_get_required_mask(structdevice*dev){
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-09 01:58:16
On Thu, 2018-08-09 at 10:54 +1000, Benjamin Herrenschmidt wrote:
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
quoted
These are identical to the arch specific ones, so remove them.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Note: We will still need to implement some custom variant of this
for our secure VMs ...
Basically we'll need to use the existing bounce bufferring as-is but
the condition will be different, it won't be whether the address is
below a certain limit, it will be *always*.
Cheers,
Ben.
@@ -88,7 +64,7 @@ static int ppc_swiotlb_bus_notify(struct notifier_block *nb,/* May need to bounce if the device can't address all of DRAM */if((dma_get_mask(dev)+1)<memblock_end_of_DRAM())-set_dma_ops(dev,&powerpc_swiotlb_dma_ops);+set_dma_ops(dev,&swiotlb_dma_ops);returnNOTIFY_DONE;}
From: Christoph Hellwig <hch@lst.de> Date: 2018-08-22 06:43:52
On Thu, Aug 09, 2018 at 10:01:16AM +1000, Benjamin Herrenschmidt wrote:
On Tue, 2018-07-31 at 14:16 +0200, Christoph Hellwig wrote:
quoted
It turns out cxl actually uses it. So for now skip this patch,
although random code in drivers messing with dma ops will need to
be sorted out sooner or later.
CXL devices are "special", they bypass the classic iommu in favor of
allowing the device to operate using the main processor page tables
using an MMU context (so basically the device can use userspace
addresses directly), akin to ATS.
I think the code currently uses the nommu ops as a way to do a simple
kernel mapping for kernel drivers using CXL (not userspace stuff)
though.
Its still a horrible idea to have this in drivers/, we need some
core API to mediate this behavior. Also if the device supports
using virtual addresses dma_nommu_ops seems wrong as it won't do
the right thing for e.g. vmalloc addresses not mapped into the
kernel linear mapping (which I guess can't currently happen on
powerpc, but still..)
From: Christoph Hellwig <hch@lst.de> Date: 2018-08-22 06:52:19
On Thu, Aug 09, 2018 at 09:44:18AM +1000, Benjamin Herrenschmidt wrote:
We do have the occasional device with things like 31-bit DMA
limitation. We know they happens to work because those systems
can't have enough memory to be a problem. This is why our current
DMA direct ops in powerpc just unconditionally return true on ppc32.
The test against a full 32-bit mask here will break them I think.
Thing is, I'm not sure I still have access to one of these things
to test, I'll have to dig (from memory things like b43 wifi).
Yeah, the other platforms that support these devices support ZONE_DMA
to reliably handle these devices. But there is two other ways the
current code would actually handle these fine despite the dma_direct
checks:
1) if the device only has physical addresses up to 31-bit anyway
2) by trying again to find a lower address. But this only works
for coherent allocations and not streaming maps (unless we have
swiotlb with a buffer below 31-bits).
It seems powerpc can have ZONE_DMA, though and we will cover these
devices just fine. If it didn't have that the current powerpc
code would not work either.
- What is this trying to achieve ?
/*
* Various PCI/PCIe bridges have broken support for > 32bit DMA even
* if the device itself might support it.
*/
if (dev->dma_32bit_limit && mask > phys_to_dma(dev, DMA_BIT_MASK(32)))
return 0;
IE, if the device has a 32-bit limit, we fail an attempt at checking
if a >32-bit mask works ? That doesn't quite seem to be the right thing
to do... Shouldn't this be in dma_set_mask() and just clamp the mask down ?
IE, dma_set_mask() is what a driver uses to establish the device capability,
so it makes sense tot have dma_32bit_limit just reduce that capability, not
fail because the device can do more than what the bridge can....
If your PCI bridge / PCIe root port doesn't support dma to addresses
larger than 32-bit the device capabilities above that don't matter, it
just won't work. We have this case at least for some old VIA x86 chipsets
and some relatively modern Xilinx FPGAs with PCIe.
- How is that file supposed to work on 64-bit platforms ? From what I can
tell, dma_supported() will unconditionally return true if the mask is
32-bit or larger (appart from the above issue). This doesn't look right,
the mask needs to be compared to the max memory address. There are a bunch
of devices out there with masks anywhere bettween 40 and 64 bits, and
some of these will not work "out of the box" if the offseted top
of memory is beyond the mask limit. Or am I missing something ?
Your are not missing anything except for the history of this code.
Your observation is right, but there always has been the implicit
assumption that architectures with more than 4GB of physical address
space must either support and iommu or swiotlb and use that. It's
never been document anywhere, but I'm working on integrating all
this code to make more sense.
From: Christoph Hellwig <hch@lst.de> Date: 2018-08-22 06:57:16
On Thu, Aug 09, 2018 at 09:54:33AM +1000, Benjamin Herrenschmidt wrote:
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
quoted
We need to take the DMA offset and encryption bit into account when selecting
a zone. Add a helper that takes those into account and use it.
That whole "encryption" stuff seems to be completely specific to the
way x86 does memory encryption, or am I mistaken ? It's not clear to me
what that does in practice and how it relates to DMA mappings.
Not even all of x86, but AMD in particular, Intel does it yet another
way. But it still is easier to take this into the core with a few
overrides than duplicating all the code.
I'm also not sure about that whole business with ZONE_DMA and
ARCH_ZONE_DMA_BITS...
ZONE_DMA usually (but not always) maps to 24-bits of address space,
if it doesn't (I mostly through about s390 with it's odd 31-bits)
the architecture can override it if it cares).
On ppc64, unless you enable swiotlb (which we only do currently on
some embedded platforms), you have all of memory in ZONE_DMA.
[ 0.000000] Zone ranges:
[ 0.000000] DMA [mem 0x0000000000000000-0x0000001fffffffff]
[ 0.000000] DMA32 empty
[ 0.000000] Normal empty
[ 0.000000] Device empty
This is really weird. Why would you wire up ZONE_DMA like this?
The general scheme that architectures should implement is:
ZONE_DMA: Any memory below a magic threshold that is lower than
32-bit. Only enabled if actually required (usually
either 24-bit for ISA, or some other weird architecture
specific value like 32-bit for S/390)
ZONE_DMA32: Memory <= 32-bit if the architecture supports more than
32-bits worth of physical address space. Should generally
be enabled on all 64-bit architectures unless you have
a very good reason not to.
ZONE_NORMAL: Everything above 32-bit not falling into HIGHMEM or
MOVEABLE.
From: Christoph Hellwig <hch@lst.de> Date: 2018-08-22 07:01:02
On Thu, Aug 09, 2018 at 10:27:46AM +1000, Benjamin Herrenschmidt wrote:
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
quoted
The requirement to disable local irqs over kmap_atomic is long gone,
so remove those calls.
Really ? I'm trying to verify that and getting lost in a mess of macros
from hell in the per-cpu stuff but if you look at our implementation
of kmap_atomic_prot(), all it does is a preempt_disable(), and then
it uses kmap_atomic_idx_push():
int idx = __this_cpu_inc_return(__kmap_atomic_idx) - 1;
Note the use of __this_cpu_inc_return(), not this_cpu_inc_return(),
ie this is the non-interrupt safe version...
Looks like the powerpc variant indeed isn't save.
I did look a bit more through the code and history, and it seems
like we remove the need to disable irqs when called from process
context a while ago, but we still require disabling irqs when called
from irq context. Given that this code can also be called from
irq context we'll have to keep the local_irq_save.
From: Christoph Hellwig <hch@lst.de> Date: 2018-08-22 07:02:28
On Thu, Aug 09, 2018 at 11:57:53AM +1000, Benjamin Herrenschmidt wrote:
Note: We will still need to implement some custom variant of this
for our secure VMs ...
Basically we'll need to use the existing bounce bufferring as-is but
the condition will be different, it won't be whether the address is
below a certain limit, it will be *always*.
The conditions are in the dma_capable() helper that the architecture
can override (and which powerpc already does override).
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-22 23:46:10
On Wed, 2018-08-22 at 09:02 +0200, Christoph Hellwig wrote:
On Thu, Aug 09, 2018 at 10:27:46AM +1000, Benjamin Herrenschmidt wrote:
quoted
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
quoted
The requirement to disable local irqs over kmap_atomic is long gone,
so remove those calls.
Really ? I'm trying to verify that and getting lost in a mess of macros
from hell in the per-cpu stuff but if you look at our implementation
of kmap_atomic_prot(), all it does is a preempt_disable(), and then
it uses kmap_atomic_idx_push():
int idx = __this_cpu_inc_return(__kmap_atomic_idx) - 1;
Note the use of __this_cpu_inc_return(), not this_cpu_inc_return(),
ie this is the non-interrupt safe version...
Looks like the powerpc variant indeed isn't save.
I did look a bit more through the code and history, and it seems
like we remove the need to disable irqs when called from process
context a while ago, but we still require disabling irqs when called
from irq context. Given that this code can also be called from
irq context we'll have to keep the local_irq_save.
This is the same with x86 no ?
32-bit x86 kmap_atomic_prot is the same as ours...
In fact I wonder why the preempt_disable() in there since it needs to
be protected against interrupt ?
Or is it that we never actually call kmap_atomic_* these days from
interrupt, and the atomic versions are just about dealing with
spinlocks ?
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-22 23:51:11
On Wed, 2018-08-22 at 08:45 +0200, Christoph Hellwig wrote:
On Thu, Aug 09, 2018 at 10:01:16AM +1000, Benjamin Herrenschmidt wrote:
quoted
On Tue, 2018-07-31 at 14:16 +0200, Christoph Hellwig wrote:
quoted
It turns out cxl actually uses it. So for now skip this patch,
although random code in drivers messing with dma ops will need to
be sorted out sooner or later.
CXL devices are "special", they bypass the classic iommu in favor of
allowing the device to operate using the main processor page tables
using an MMU context (so basically the device can use userspace
addresses directly), akin to ATS.
I think the code currently uses the nommu ops as a way to do a simple
kernel mapping for kernel drivers using CXL (not userspace stuff)
though.
Its still a horrible idea to have this in drivers/, we need some
core API to mediate this behavior. Also if the device supports
using virtual addresses dma_nommu_ops seems wrong as it won't do
the right thing for e.g. vmalloc addresses not mapped into the
kernel linear mapping (which I guess can't currently happen on
powerpc, but still..)
You are right it won't do the right thing, but neither will standard
DMA ops, will they ? Drivers know not to try to dma_map vmalloc
addresses without first getting the underlying page, nothing unusal
there.
Yes I agree having this in drivers somewhat sucks though.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-22 23:59:45
On Wed, 2018-08-22 at 08:53 +0200, Christoph Hellwig wrote:
On Thu, Aug 09, 2018 at 09:44:18AM +1000, Benjamin Herrenschmidt wrote:
quoted
We do have the occasional device with things like 31-bit DMA
limitation. We know they happens to work because those systems
can't have enough memory to be a problem. This is why our current
DMA direct ops in powerpc just unconditionally return true on ppc32.
The test against a full 32-bit mask here will break them I think.
Thing is, I'm not sure I still have access to one of these things
to test, I'll have to dig (from memory things like b43 wifi).
Yeah, the other platforms that support these devices support ZONE_DMA
to reliably handle these devices. But there is two other ways the
current code would actually handle these fine despite the dma_direct
checks:
1) if the device only has physical addresses up to 31-bit anyway
2) by trying again to find a lower address. But this only works
for coherent allocations and not streaming maps (unless we have
swiotlb with a buffer below 31-bits).
It seems powerpc can have ZONE_DMA, though and we will cover these
devices just fine. If it didn't have that the current powerpc
code would not work either.
Not exactly. powerpc has ZONE_DMA covering all of system memory.
What happens in ppc32 is that we somewhat "know" that none of the
systems with those stupid 31-bit limited pieces of HW is capable of
having more than 2GB of memory anyway.
So we get away with just returning "1".
quoted
- What is this trying to achieve ?
/*
* Various PCI/PCIe bridges have broken support for > 32bit DMA even
* if the device itself might support it.
*/
if (dev->dma_32bit_limit && mask > phys_to_dma(dev, DMA_BIT_MASK(32)))
return 0;
IE, if the device has a 32-bit limit, we fail an attempt at checking
if a >32-bit mask works ? That doesn't quite seem to be the right thing
to do... Shouldn't this be in dma_set_mask() and just clamp the mask down ?
IE, dma_set_mask() is what a driver uses to establish the device capability,
so it makes sense tot have dma_32bit_limit just reduce that capability, not
fail because the device can do more than what the bridge can....
If your PCI bridge / PCIe root port doesn't support dma to addresses
larger than 32-bit the device capabilities above that don't matter, it
just won't work. We have this case at least for some old VIA x86 chipsets
and some relatively modern Xilinx FPGAs with PCIe.
Hrm... that's the usual confusion dma_capable() vs. dma_set_mask().
It's always been perfectly fine for a driver to do a dma_set_mask(64-
bit) on a system where the bridge can only do 32-bits ...
We shouldn't fail there, we should instead "clamp" the mask to 32-bit,
see what I mean ? It doesn't matter that the device itself is capable
of issuing >32 addresses, I agree, but what we need to express is that
the combination device+bridge doesn't want addresses above 32-bit, so
it's equivalent to making the device do a set_mask(32-bit).
This will succeed if the system can limit the addresses (for example
because memory is never above 32-bit) and will fail if the system
can't.
So that's equivalent of writing
if (dev->dma_32bit_limit && mask > phys_to_dma(dev, DMA_BIT_MASK(32)))
mask = phys_to_dma(dev, DMA_BIT_MASK(32));
Effectively meaning "don't give me addresses aboe 32-bit".
Still, your code doesn't check the mask against the memory size. Which
means it will fail for 32-bit masks even on systems that do not have
memory above 4G.
quoted
- How is that file supposed to work on 64-bit platforms ? From what I can
tell, dma_supported() will unconditionally return true if the mask is
32-bit or larger (appart from the above issue). This doesn't look right,
the mask needs to be compared to the max memory address. There are a bunch
of devices out there with masks anywhere bettween 40 and 64 bits, and
some of these will not work "out of the box" if the offseted top
of memory is beyond the mask limit. Or am I missing something ?
Your are not missing anything except for the history of this code.
Your observation is right, but there always has been the implicit
assumption that architectures with more than 4GB of physical address
space must either support and iommu or swiotlb and use that. It's
never been document anywhere, but I'm working on integrating all
this code to make more sense.
Well, iommus can have bypass regions, which we also use for
performance, so we do at dma_set_mask() time "swap" the ops around, and
in that case, we do want to check the mask against the actual top of
memory...
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-23 00:02:07
On Wed, 2018-08-22 at 08:58 +0200, Christoph Hellwig wrote:
On Thu, Aug 09, 2018 at 09:54:33AM +1000, Benjamin Herrenschmidt wrote:
quoted
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
quoted
We need to take the DMA offset and encryption bit into account when selecting
a zone. Add a helper that takes those into account and use it.
That whole "encryption" stuff seems to be completely specific to the
way x86 does memory encryption, or am I mistaken ? It's not clear to me
what that does in practice and how it relates to DMA mappings.
Not even all of x86, but AMD in particular, Intel does it yet another
way. But it still is easier to take this into the core with a few
overrides than duplicating all the code.
quoted
I'm also not sure about that whole business with ZONE_DMA and
ARCH_ZONE_DMA_BITS...
ZONE_DMA usually (but not always) maps to 24-bits of address space,
if it doesn't (I mostly through about s390 with it's odd 31-bits)
the architecture can override it if it cares).
quoted
On ppc64, unless you enable swiotlb (which we only do currently on
some embedded platforms), you have all of memory in ZONE_DMA.
[ 0.000000] Zone ranges:
[ 0.000000] DMA [mem 0x0000000000000000-0x0000001fffffffff]
[ 0.000000] DMA32 empty
[ 0.000000] Normal empty
[ 0.000000] Device empty
This is really weird. Why would you wire up ZONE_DMA like this?
We always did :-) It predates my involvement and I think it predates
even Pauls. It's quite silly actually since the first powerpc machines
actually had ISA devices in them, but that's how it's been for ever. I
suppose we could change it but that would mean digging out some old
stuff to test.
The general scheme that architectures should implement is:
ZONE_DMA: Any memory below a magic threshold that is lower than
32-bit. Only enabled if actually required (usually
either 24-bit for ISA, or some other weird architecture
specific value like 32-bit for S/390)
It should have been ZONE_ISA_DMA :-)
ZONE_DMA32: Memory <= 32-bit if the architecture supports more than
32-bits worth of physical address space. Should generally
be enabled on all 64-bit architectures unless you have
a very good reason not to.
Yeah so we sort-of enable the config option but only populate the zone
on platforms using swiotlb (freescale stuff). It's a bit messy at the
moment I must admit.
ZONE_NORMAL: Everything above 32-bit not falling into HIGHMEM or
MOVEABLE.
From: Christoph Hellwig <hch@lst.de> Date: 2018-08-23 05:22:16
On Thu, Aug 23, 2018 at 09:59:18AM +1000, Benjamin Herrenschmidt wrote:
quoted
Yeah, the other platforms that support these devices support ZONE_DMA
to reliably handle these devices. But there is two other ways the
current code would actually handle these fine despite the dma_direct
checks:
1) if the device only has physical addresses up to 31-bit anyway
2) by trying again to find a lower address. But this only works
for coherent allocations and not streaming maps (unless we have
swiotlb with a buffer below 31-bits).
It seems powerpc can have ZONE_DMA, though and we will cover these
devices just fine. If it didn't have that the current powerpc
code would not work either.
Not exactly. powerpc has ZONE_DMA covering all of system memory.
What happens in ppc32 is that we somewhat "know" that none of the
systems with those stupid 31-bit limited pieces of HW is capable of
having more than 2GB of memory anyway.
So we get away with just returning "1".
I think I can up with a proper way of handling that by checking
the actual amount of physical memory present instead of the hard coded
32-bit.
quoted
If your PCI bridge / PCIe root port doesn't support dma to addresses
larger than 32-bit the device capabilities above that don't matter, it
just won't work. We have this case at least for some old VIA x86 chipsets
and some relatively modern Xilinx FPGAs with PCIe.
Hrm... that's the usual confusion dma_capable() vs. dma_set_mask().
It's always been perfectly fine for a driver to do a dma_set_mask(64-
bit) on a system where the bridge can only do 32-bits ...
No, it hasn't. That's why we have this pattern of trying a 64-bit
mask first and then setting a 32-bit mask if that fails all over
drivers/. However with all the work we've done over the last month
we are getting really close to a world where:
- the driver just does one dma_set_mask for the capabilities and
stores that in the dma_mask
- other limitations go elsewhere and will be automatically taken
into account.
Which is I guess what you always wanted, but which wasn't how things
actually worked before.
We shouldn't fail there, we should instead "clamp" the mask to 32-bit,
see what I mean ? It doesn't matter that the device itself is capable
of issuing >32 addresses, I agree, but what we need to express is that
the combination device+bridge doesn't want addresses above 32-bit, so
it's equivalent to making the device do a set_mask(32-bit).
As said, we'll get there (but with the new separate bus_dma_mask in 4.19),
but this is not how things currently work.
quoted
Your observation is right, but there always has been the implicit
assumption that architectures with more than 4GB of physical address
space must either support and iommu or swiotlb and use that. It's
never been document anywhere, but I'm working on integrating all
this code to make more sense.
Well, iommus can have bypass regions, which we also use for
performance, so we do at dma_set_mask() time "swap" the ops around, and
in that case, we do want to check the mask against the actual top of
memory...
That is a bit of a powerpc special case (we also had one other arch
doing that, but it got removed in the great purge, can't rember which
one right now). Everyone else has one set of ops, and they just switch
to the direct mapping inside the iommu ops.
From: Christoph Hellwig <hch@lst.de> Date: 2018-08-23 05:24:17
On Thu, Aug 23, 2018 at 10:01:45AM +1000, Benjamin Herrenschmidt wrote:
quoted
The general scheme that architectures should implement is:
ZONE_DMA: Any memory below a magic threshold that is lower than
32-bit. Only enabled if actually required (usually
either 24-bit for ISA, or some other weird architecture
specific value like 32-bit for S/390)
It should have been ZONE_ISA_DMA :-)
For most of these use cases it should have been indeed, and that
would avoid a lot of confusion where people use GFP_DMA just because
they do DMA.
Anyway, switching powerpc to this scheme would be great, but I don't
think it is required - GFP_KERNEL will silently fall back to ZONE_DMA,
so except for an additional GFP_DMA fallback allocation when the
GFP_KERNEL one fails the code should just work.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-08-23 05:25:10
On Thu, 2018-08-23 at 07:24 +0200, Christoph Hellwig wrote:
quoted
Well, iommus can have bypass regions, which we also use for
performance, so we do at dma_set_mask() time "swap" the ops around, and
in that case, we do want to check the mask against the actual top of
memory...
That is a bit of a powerpc special case (we also had one other arch
doing that, but it got removed in the great purge, can't rember which
one right now). Everyone else has one set of ops, and they just switch
to the direct mapping inside the iommu ops.
We more or less do that too in some of ours these days bcs of the whole
coherent_mask vs mask where a given device might need either depending
on the type of mapping.
Ben.
From: Scott Wood <oss@buserror.net> Date: 2018-08-27 08:56:11
On Thu, 2018-08-09 at 10:52 +1000, Benjamin Herrenschmidt wrote:
On Mon, 2018-07-30 at 18:38 +0200, Christoph Hellwig wrote:
quoted
These do the same functionality as the existing helpers, but do it
simpler, and also allow the (optional) use of CMA.
Note that the swiotlb code now calls into the dma_direct code directly,
given that it doesn't work with noncoherent caches at all, and isn't
called
when we have an iommu either, so the iommu special case in
dma_nommu_alloc_coherent isn't required for swiotlb.
I am not convinced that this will produce the same results due to
the way the zone picking works.
As for the interaction with swiotlb, we'll need the FSL guys to have
a look. Scott, do you remember what this is about ?
dma_direct_alloc() has similar (though not identical[1]) zone picking, so I
think it will work. Needs testing though, and I no longer have a book3e
machine with a PCIe card in it.
The odd thing about this platform (fsl book3e) is the 31-bit[2] limitation on
PCI. We currently use ZONE_DMA32 for this, rather than ZONE_DMA, at Ben's
request[3]. dma_direct_alloc() regards ZONE_DMA32 as being fixed at 32-bits,
but it doesn't really matter as long as limit_zone_pfn() still works, and the
allocation is made below 2 GiB. If we were to switch to ZONE_DMA, and have
both 31-bit and 32-bit zones, then dma_direct_alloc() would have a problem
knowing when to use the 31-bit zone since it's based on a non-power-of-2 limit
that isn't reflected in the dma mask.
-Scott
[1] The logic in dma_direct_alloc() seems wrong -- the zone should need to fit
in the mask, not the other way around. If ARCH_ZONE_DMA_BITS is 24, then
0x007fffff should be a failure rather than GFP_DMA, 0x7fffffff should be
GFP_DMA rather than GFP_DMA32, and 0x3ffffffff should be GFP_DMA32 rather than
an unrestricted allocation (in each case assuming that the end of RAM is
beyond the mask).
[2] The actual limit is closer to 4 GiB, but not quite due to special windows.
swiotlb still uses the real limit when deciding whether to bounce, so the dma
mask is still 32 bits.
[3] https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-July/099593.html
From: Konrad Rzeszutek Wilk <hidden> Date: 2018-08-27 16:07:12
On Mon, Jul 30, 2018 at 06:38:09PM +0200, Christoph Hellwig wrote:
For now this allows consolidating the powerpc code. In the long run
we should grow a generic implementation of dma_get_required_mask that
returns the dma mask required to avoid bounce buffering.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Konrad Rzeszutek Wilk <redacted>
Thank you!