Re: [PATCH 1/2] dma-mapping: remove ->mapping_error
On Fri, Nov 09, 2018 at 02:41:18PM +, Robin Murphy wrote: >> - >> #define CMD_SET_TYPE(cmd, t) ((cmd)->data[1] |= ((t) << 28)) >> #define LOOP_TIMEOUT 10 >> @@ -2339,7 +2337,7 @@ static dma_addr_t __map_single(struct device *dev, >> paddr &= PAGE_MASK; >> address = dma_ops_alloc_iova(dev, dma_dom, pages, dma_mask); >> -if (address == AMD_IOMMU_MAPPING_ERROR) >> +if (address == DMA_MAPPING_ERROR) > > This for one is clearly broken, because the IOVA allocator still returns 0 > on failure here... Indeed. And that shows how the original code was making a mess of these different constants.. > I very much agree with the concept, but I think the way to go about it is > to convert the implementations which need it to the standardised > *_MAPPING_ERROR value one-by-one, and only then then do the big sweep to > remove them all. That has more of a chance of getting worthwhile review and > testing from the respective relevant parties (I'll confess I came looking > for this bug specifically, since I happened to recall amd_iommu having a > tricky implicit reliance on the old DMA_ERROR_CODE being 0 on x86). I'll see if I can split this out somehow, but I'm not sure it is going to be all that much more readable.. > In terms of really minimising the error-checking overhead it's a bit of a > shame that DMA_MAPPING_ERROR = 0 doesn't seem viable as the thing to > standardise on, since that has advantages at the micro-optimisation level > for many ISAs - fixing up the legacy IOMMU code doesn't seem > insurmountable, but I suspect there may well be non-IOMMU platforms where > DMA to physical address 0 is a thing :( Yes, that is what I'm more worried about. > (yeah, I know saving a couple of instructions and potential register > allocations is down in the noise when we're already going from an indirect > call to an inline comparison; I'm mostly just thinking out loud there) The nice bit of standardizing the value is that we get rid of an indirect call, which generally is much more of a problem at the micro-architecture level. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] dma-mapping: remove ->mapping_error
On 09/11/2018 08:46, Christoph Hellwig wrote: [...] diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index 1167ff0416cf..cfb422e17049 100644 --- a/drivers/iommu/amd_iommu.c +++ b/drivers/iommu/amd_iommu.c @@ -55,8 +55,6 @@ #include "amd_iommu_types.h" #include "irq_remapping.h" -#define AMD_IOMMU_MAPPING_ERROR 0 - #define CMD_SET_TYPE(cmd, t) ((cmd)->data[1] |= ((t) << 28)) #define LOOP_TIMEOUT 10 @@ -2339,7 +2337,7 @@ static dma_addr_t __map_single(struct device *dev, paddr &= PAGE_MASK; address = dma_ops_alloc_iova(dev, dma_dom, pages, dma_mask); - if (address == AMD_IOMMU_MAPPING_ERROR) + if (address == DMA_MAPPING_ERROR) This for one is clearly broken, because the IOVA allocator still returns 0 on failure here... goto out; prot = dir2prot(direction); @@ -2376,7 +2374,7 @@ static dma_addr_t __map_single(struct device *dev, dma_ops_free_iova(dma_dom, address, pages); - return AMD_IOMMU_MAPPING_ERROR; + return DMA_MAPPING_ERROR; } /* @@ -2427,7 +2425,7 @@ static dma_addr_t map_page(struct device *dev, struct page *page, if (PTR_ERR(domain) == -EINVAL) return (dma_addr_t)paddr; else if (IS_ERR(domain)) - return AMD_IOMMU_MAPPING_ERROR; + return DMA_MAPPING_ERROR; dma_mask = *dev->dma_mask; dma_dom = to_dma_ops_domain(domain); @@ -2504,7 +2502,7 @@ static int map_sg(struct device *dev, struct scatterlist *sglist, npages = sg_num_pages(dev, sglist, nelems); address = dma_ops_alloc_iova(dev, dma_dom, npages, dma_mask); - if (address == AMD_IOMMU_MAPPING_ERROR) + if (address == DMA_MAPPING_ERROR) ..and here. I very much agree with the concept, but I think the way to go about it is to convert the implementations which need it to the standardised *_MAPPING_ERROR value one-by-one, and only then then do the big sweep to remove them all. That has more of a chance of getting worthwhile review and testing from the respective relevant parties (I'll confess I came looking for this bug specifically, since I happened to recall amd_iommu having a tricky implicit reliance on the old DMA_ERROR_CODE being 0 on x86). In terms of really minimising the error-checking overhead it's a bit of a shame that DMA_MAPPING_ERROR = 0 doesn't seem viable as the thing to standardise on, since that has advantages at the micro-optimisation level for many ISAs - fixing up the legacy IOMMU code doesn't seem insurmountable, but I suspect there may well be non-IOMMU platforms where DMA to physical address 0 is a thing :( (yeah, I know saving a couple of instructions and potential register allocations is down in the noise when we're already going from an indirect call to an inline comparison; I'm mostly just thinking out loud there) Robin. goto out_err; prot = dir2prot(direction); @@ -2627,7 +2625,7 @@ static void *alloc_coherent(struct device *dev, size_t size, *dma_addr = __map_single(dev, dma_dom, page_to_phys(page), size, DMA_BIDIRECTIONAL, dma_mask); - if (*dma_addr == AMD_IOMMU_MAPPING_ERROR) + if (*dma_addr == DMA_MAPPING_ERROR) goto out_free; return page_address(page); @@ -2678,11 +2676,6 @@ static int amd_iommu_dma_supported(struct device *dev, u64 mask) return check_device(dev); } -static int amd_iommu_mapping_error(struct device *dev, dma_addr_t dma_addr) -{ - return dma_addr == AMD_IOMMU_MAPPING_ERROR; -} - static const struct dma_map_ops amd_iommu_dma_ops = { .alloc = alloc_coherent, .free = free_coherent, @@ -2691,7 +2684,6 @@ static const struct dma_map_ops amd_iommu_dma_ops = { .map_sg = map_sg, .unmap_sg = unmap_sg, .dma_supported = amd_iommu_dma_supported, - .mapping_error = amd_iommu_mapping_error, }; static int init_reserved_iova_ranges(void) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/2] dma-mapping: remove ->mapping_error
There is no need to perform an indirect function call to check if a DMA mapping resulted in an error, if we always return the last possible dma address as the error code. While that could in theory be a valid DMAable region, it would have to assume we want to support unaligned DMAs of size 1, which is rather pointless. Instead simplify the whole machinery by using (~(dma_addr_t)0x0) as the common error return. Signed-off-by: Christoph Hellwig --- arch/alpha/kernel/pci_iommu.c| 10 ++- arch/arm/common/dmabounce.c | 12 +++-- arch/arm/include/asm/dma-iommu.h | 2 -- arch/arm/mm/dma-mapping.c| 39 +++- arch/arm64/mm/dma-mapping.c | 15 +++ arch/ia64/hp/common/sba_iommu.c | 8 +- arch/ia64/sn/pci/pci_dma.c | 8 +- arch/mips/include/asm/jazzdma.h | 6 - arch/mips/jazz/jazzdma.c | 16 arch/powerpc/include/asm/iommu.h | 3 --- arch/powerpc/kernel/dma-iommu.c | 6 - arch/powerpc/kernel/dma-swiotlb.c| 1 - arch/powerpc/kernel/iommu.c | 28 ++-- arch/powerpc/platforms/cell/iommu.c | 1 - arch/powerpc/platforms/pseries/vio.c | 3 +-- arch/s390/pci/pci_dma.c | 18 - arch/sparc/kernel/iommu.c| 12 +++-- arch/sparc/kernel/iommu_common.h | 2 -- arch/sparc/kernel/pci_sun4v.c| 14 +++--- arch/x86/kernel/amd_gart_64.c| 19 -- arch/x86/kernel/pci-calgary_64.c | 24 ++--- drivers/iommu/amd_iommu.c| 18 - drivers/iommu/dma-iommu.c| 23 ++-- drivers/iommu/intel-iommu.c | 13 +- drivers/parisc/ccio-dma.c| 10 +-- drivers/parisc/sba_iommu.c | 10 +-- drivers/pci/controller/vmd.c | 6 - drivers/xen/swiotlb-xen.c| 12 ++--- include/linux/dma-direct.h | 3 --- include/linux/dma-iommu.h| 1 - include/linux/dma-mapping.h | 12 - kernel/dma/direct.c | 8 +- kernel/dma/swiotlb.c | 9 +++ 33 files changed, 105 insertions(+), 267 deletions(-) diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c index 46e08e0d9181..30339a0369ce 100644 --- a/arch/alpha/kernel/pci_iommu.c +++ b/arch/alpha/kernel/pci_iommu.c @@ -291,7 +291,7 @@ pci_map_single_1(struct pci_dev *pdev, void *cpu_addr, size_t size, use direct_map above, it now must be considered an error. */ if (! alpha_mv.mv_pci_tbi) { printk_once(KERN_WARNING "pci_map_single: no HW sg\n"); - return 0; + return DMA_MAPPING_ERROR; } arena = hose->sg_pci; @@ -307,7 +307,7 @@ pci_map_single_1(struct pci_dev *pdev, void *cpu_addr, size_t size, if (dma_ofs < 0) { printk(KERN_WARNING "pci_map_single failed: " "could not allocate dma page tables\n"); - return 0; + return DMA_MAPPING_ERROR; } paddr &= PAGE_MASK; @@ -935,11 +935,6 @@ iommu_unbind(struct pci_iommu_arena *arena, long pg_start, long pg_count) return 0; } -static int alpha_pci_mapping_error(struct device *dev, dma_addr_t dma_addr) -{ - return dma_addr == 0; -} - const struct dma_map_ops alpha_pci_ops = { .alloc = alpha_pci_alloc_coherent, .free = alpha_pci_free_coherent, @@ -947,7 +942,6 @@ const struct dma_map_ops alpha_pci_ops = { .unmap_page = alpha_pci_unmap_page, .map_sg = alpha_pci_map_sg, .unmap_sg = alpha_pci_unmap_sg, - .mapping_error = alpha_pci_mapping_error, .dma_supported = alpha_pci_supported, }; EXPORT_SYMBOL(alpha_pci_ops); diff --git a/arch/arm/common/dmabounce.c b/arch/arm/common/dmabounce.c index 9a92de63426f..5ba4622030ca 100644 --- a/arch/arm/common/dmabounce.c +++ b/arch/arm/common/dmabounce.c @@ -257,7 +257,7 @@ static inline dma_addr_t map_single(struct device *dev, void *ptr, size_t size, if (buf == NULL) { dev_err(dev, "%s: unable to map unsafe buffer %p!\n", __func__, ptr); - return ARM_MAPPING_ERROR; + return DMA_MAPPING_ERROR; } dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x) mapped to %p (dma=%#x)\n", @@ -327,7 +327,7 @@ static dma_addr_t dmabounce_map_page(struct device *dev, struct page *page, ret = needs_bounce(dev, dma_addr, size); if (ret < 0) - return ARM_MAPPING_ERROR; + return DMA_MAPPING_ERROR; if (ret == 0) { arm_dma_ops.sync_single_for_device(dev, dma_addr, size, dir); @@ -336,7 +336,7 @@ static dma_addr_t dmabounce_map_page(struct device *dev, struct page *page, if