From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934571AbcCOLpi (ORCPT ); Tue, 15 Mar 2016 07:45:38 -0400 Received: from foss.arm.com ([217.140.101.70]:35941 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932428AbcCOLp3 (ORCPT ); Tue, 15 Mar 2016 07:45:29 -0400 Subject: Re: [RFC 3/3] iommu: dma-iommu: use common implementation also on ARM architecture To: Magnus Damm , Marek Szyprowski References: <1455870164-25337-1-git-send-email-m.szyprowski@samsung.com> <1455870164-25337-4-git-send-email-m.szyprowski@samsung.com> Cc: linaro-mm-sig@lists.linaro.org, Krzysztof Kozlowski , Russell King - ARM Linux , Heiko Stuebner , Arnd Bergmann , Bartlomiej Zolnierkiewicz , Catalin Marinas , Will Deacon , linux-kernel , dri-devel@lists.freedesktop.org, Inki Dae , iommu@lists.linux-foundation.org, Sakari Ailus , Laurent Pinchart , "linux-arm-kernel@lists.infradead.org" , Mark Yao From: Robin Murphy Message-ID: <56E7F5D4.1020201@arm.com> Date: Tue, 15 Mar 2016 11:45:24 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Magnus, On 15/03/16 11:18, Magnus Damm wrote: > Hi Marek, > > On Fri, Feb 19, 2016 at 5:22 PM, Marek Szyprowski > wrote: >> This patch replaces ARM-specific IOMMU-based DMA-mapping implementation >> with generic IOMMU DMA-mapping code shared with ARM64 architecture. The >> side-effect of this change is a switch from bitmap-based IO address space >> management to tree-based code. There should be no functional changes >> for drivers, which rely on initialization from generic arch_setup_dna_ops() >> interface. Code, which used old arm_iommu_* functions must be updated to >> new interface. >> >> Signed-off-by: Marek Szyprowski >> --- > > Thanks for your efforts and my apologies for late comments. Just FYI > I'll try your patch (and this series) with the ipmmu-vmsa.c driver on > 32-bit ARM and see how it goes. Nice not to have to support multiple > interfaces depending on architecture! > > One question that comes to mind is how to handle features. > > For instance, the 32-bit ARM code supports DMA_ATTR_FORCE_CONTIGUOUS > while the shared code in drivers/iommu/dma-iommu.c does not. I assume > existing users may rely on such features so from my point of view it > probably makes sense to carry over features from the 32-bit ARM code > into the shared code before pulling the plug. Indeed - the patch I posted the other day doing proper scatterlist merging in the common code is largely to that end. > I also wonder if it is possible to do a step-by-step migration and > support both old and new interfaces in the same binary? That may make > things easier for multiplatform enablement. So far I've managed to > make one IOMMU driver support both 32-bit ARM and 64-bit ARM with some > ugly magic, so adjusting 32-bit ARM dma-mapping code to coexist with > the shared code in drivers/iommu/dma-iommu.c may also be possible. And > probably involving even more ugly magic. =) That was also my thought when I tried to look at this a while ago - I started on some patches moving the bitmap from dma_iommu_mapping into the iommu_domain->iova_cookie so that the existing code and users could then be converted to just passing iommu_domains around, after which it should be fairly painless to swap out the back-end implementation transparently. That particular effort ground to a halt upon realising the number of the IOMMU and DRM drivers I'd have no way of testing - if you're interested I've dug out the diff below from an old work-in-progress branch (which probably doesn't even compile). Robin. > > Cheers, > > / magnus --->8--- diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h index 4111592..6ea939c 100644 --- a/arch/arm/include/asm/device.h +++ b/arch/arm/include/asm/device.h @@ -14,9 +14,6 @@ struct dev_archdata { #ifdef CONFIG_IOMMU_API void *iommu; /* private IOMMU data */ #endif -#ifdef CONFIG_ARM_DMA_USE_IOMMU - struct dma_iommu_mapping *mapping; -#endif bool dma_coherent; }; @@ -28,10 +25,4 @@ struct pdev_archdata { #endif }; -#ifdef CONFIG_ARM_DMA_USE_IOMMU -#define to_dma_iommu_mapping(dev) ((dev)->archdata.mapping) -#else -#define to_dma_iommu_mapping(dev) NULL -#endif - #endif diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h index 2ef282f..e15197d 100644 --- a/arch/arm/include/asm/dma-iommu.h +++ b/arch/arm/include/asm/dma-iommu.h @@ -24,13 +24,12 @@ struct dma_iommu_mapping { struct kref kref; }; -struct dma_iommu_mapping * +struct iommu_domain * arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size); -void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping); +void arm_iommu_release_mapping(struct iommu_domain *mapping); -int arm_iommu_attach_device(struct device *dev, - struct dma_iommu_mapping *mapping); +int arm_iommu_attach_device(struct device *dev, struct iommu_domain *mapping); void arm_iommu_detach_device(struct device *dev); #endif /* __KERNEL__ */ diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index e62400e..dfb5001 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1246,7 +1246,8 @@ __iommu_alloc_remap(struct page **pages, size_t size, gfp_t gfp, pgprot_t prot, static dma_addr_t __iommu_create_mapping(struct device *dev, struct page **pages, size_t size) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *dom = iommu_get_domain_for_dev(dev); + struct dma_iommu_mapping *mapping = dom->iova_cookie; unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT; dma_addr_t dma_addr, iova; int i; @@ -1268,8 +1269,7 @@ __iommu_create_mapping(struct device *dev, struct page **pages, size_t size) break; len = (j - i) << PAGE_SHIFT; - ret = iommu_map(mapping->domain, iova, phys, len, - IOMMU_READ|IOMMU_WRITE); + ret = iommu_map(dom, iova, phys, len, IOMMU_READ|IOMMU_WRITE); if (ret < 0) goto fail; iova += len; @@ -1277,14 +1277,14 @@ __iommu_create_mapping(struct device *dev, struct page **pages, size_t size) } return dma_addr; fail: - iommu_unmap(mapping->domain, dma_addr, iova-dma_addr); + iommu_unmap(dom, dma_addr, iova-dma_addr); __free_iova(mapping, dma_addr, size); return DMA_ERROR_CODE; } static int __iommu_remove_mapping(struct device *dev, dma_addr_t iova, size_t size) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *dom = iommu_get_domain_for_dev(dev); /* * add optional in-page offset from iova to size and align @@ -1293,8 +1293,8 @@ static int __iommu_remove_mapping(struct device *dev, dma_addr_t iova, size_t si size = PAGE_ALIGN((iova & ~PAGE_MASK) + size); iova &= PAGE_MASK; - iommu_unmap(mapping->domain, iova, size); - __free_iova(mapping, iova, size); + iommu_unmap(dom, iova, size); + __free_iova(dom->iova_cookie, iova, size); return 0; } @@ -1506,7 +1506,8 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, enum dma_data_direction dir, struct dma_attrs *attrs, bool is_coherent) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *dom = iommu_get_domain_for_dev(dev); + struct dma_iommu_mapping *mapping = dom->iova_cookie; dma_addr_t iova, iova_base; int ret = 0; unsigned int count; @@ -1530,7 +1531,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, prot = __dma_direction_to_prot(dir); - ret = iommu_map(mapping->domain, iova, phys, len, prot); + ret = iommu_map(dom, iova, phys, len, prot); if (ret < 0) goto fail; count += len >> PAGE_SHIFT; @@ -1540,7 +1541,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, return 0; fail: - iommu_unmap(mapping->domain, iova_base, count * PAGE_SIZE); + iommu_unmap(dom, iova_base, count * PAGE_SIZE); __free_iova(mapping, iova_base, size); return ret; } @@ -1727,7 +1728,8 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p unsigned long offset, size_t size, enum dma_data_direction dir, struct dma_attrs *attrs) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *dom = iommu_get_domain_for_dev(dev); + struct dma_iommu_mapping *mapping = dom->iova_cookie; dma_addr_t dma_addr; int ret, prot, len = PAGE_ALIGN(size + offset); @@ -1737,7 +1739,7 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p prot = __dma_direction_to_prot(dir); - ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot); + ret = iommu_map(dom, dma_addr, page_to_phys(page), len, prot); if (ret < 0) goto fail; @@ -1780,7 +1782,7 @@ static void arm_coherent_iommu_unmap_page(struct device *dev, dma_addr_t handle, size_t size, enum dma_data_direction dir, struct dma_attrs *attrs) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *dom = iommu_get_domain_for_dev(dev); dma_addr_t iova = handle & PAGE_MASK; int offset = handle & ~PAGE_MASK; int len = PAGE_ALIGN(size + offset); @@ -1788,8 +1790,8 @@ static void arm_coherent_iommu_unmap_page(struct device *dev, dma_addr_t handle, if (!iova) return; - iommu_unmap(mapping->domain, iova, len); - __free_iova(mapping, iova, len); + iommu_unmap(dom, iova, len); + __free_iova(dom->iova_cookie, iova, len); } /** @@ -1805,9 +1807,9 @@ static void arm_iommu_unmap_page(struct device *dev, dma_addr_t handle, size_t size, enum dma_data_direction dir, struct dma_attrs *attrs) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *dom = iommu_get_domain_for_dev(dev); dma_addr_t iova = handle & PAGE_MASK; - struct page *page = phys_to_page(iommu_iova_to_phys(mapping->domain, iova)); + struct page *page = phys_to_page(iommu_iova_to_phys(dom, iova)); int offset = handle & ~PAGE_MASK; int len = PAGE_ALIGN(size + offset); @@ -1817,16 +1819,16 @@ static void arm_iommu_unmap_page(struct device *dev, dma_addr_t handle, if (!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs)) __dma_page_dev_to_cpu(page, offset, size, dir); - iommu_unmap(mapping->domain, iova, len); - __free_iova(mapping, iova, len); + iommu_unmap(dom, iova, len); + __free_iova(dom->iova_cookie, iova, len); } static void arm_iommu_sync_single_for_cpu(struct device *dev, dma_addr_t handle, size_t size, enum dma_data_direction dir) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *dom = iommu_get_domain_for_dev(dev); dma_addr_t iova = handle & PAGE_MASK; - struct page *page = phys_to_page(iommu_iova_to_phys(mapping->domain, iova)); + struct page *page = phys_to_page(iommu_iova_to_phys(dom, iova)); unsigned int offset = handle & ~PAGE_MASK; if (!iova) @@ -1838,9 +1840,9 @@ static void arm_iommu_sync_single_for_cpu(struct device *dev, static void arm_iommu_sync_single_for_device(struct device *dev, dma_addr_t handle, size_t size, enum dma_data_direction dir) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *dom = iommu_get_domain_for_dev(dev); dma_addr_t iova = handle & PAGE_MASK; - struct page *page = phys_to_page(iommu_iova_to_phys(mapping->domain, iova)); + struct page *page = phys_to_page(iommu_iova_to_phys(dom, iova)); unsigned int offset = handle & ~PAGE_MASK; if (!iova) @@ -1896,12 +1898,13 @@ struct dma_map_ops iommu_coherent_ops = { * The client device need to be attached to the mapping with * arm_iommu_attach_device function. */ -struct dma_iommu_mapping * +struct iommu_domain * arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size) { unsigned int bits = size >> PAGE_SHIFT; unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long); struct dma_iommu_mapping *mapping; + struct iommu_domain *dom; int extensions = 1; int err = -ENOMEM; @@ -1938,12 +1941,14 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, u64 size) spin_lock_init(&mapping->lock); - mapping->domain = iommu_domain_alloc(bus); - if (!mapping->domain) + dom = iommu_domain_alloc(bus); + if (!dom) goto err4; + mapping->domain = dom; + dom->iova_cookie = mapping; kref_init(&mapping->kref); - return mapping; + return dom; err4: kfree(mapping->bitmaps[0]); err3: @@ -1986,24 +1991,27 @@ static int extend_iommu_mapping(struct dma_iommu_mapping *mapping) return 0; } -void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping) +void arm_iommu_release_mapping(struct iommu_domain *domain) { - if (mapping) + if (domain) { + struct dma_iommu_mapping *mapping = domain->iova_cookie; + kref_put(&mapping->kref, release_iommu_mapping); + } } EXPORT_SYMBOL_GPL(arm_iommu_release_mapping); static int __arm_iommu_attach_device(struct device *dev, - struct dma_iommu_mapping *mapping) + struct iommu_domain *domain) { int err; + struct dma_iommu_mapping *mapping = domain->iova_cookie; - err = iommu_attach_device(mapping->domain, dev); + err = iommu_attach_device(domain, dev); if (err) return err; kref_get(&mapping->kref); - to_dma_iommu_mapping(dev) = mapping; pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev)); return 0; @@ -2023,7 +2031,7 @@ static int __arm_iommu_attach_device(struct device *dev, * mapping. */ int arm_iommu_attach_device(struct device *dev, - struct dma_iommu_mapping *mapping) + struct iommu_domain *mapping) { int err; @@ -2039,16 +2047,17 @@ EXPORT_SYMBOL_GPL(arm_iommu_attach_device); static void __arm_iommu_detach_device(struct device *dev) { struct dma_iommu_mapping *mapping; + struct iommu_domain *dom; - mapping = to_dma_iommu_mapping(dev); - if (!mapping) { + dom = iommu_get_domain_for_dev(dev); + if (!dom) { dev_warn(dev, "Not attached\n"); return; } - iommu_detach_device(mapping->domain, dev); + mapping = dom->iova_cookie; + iommu_detach_device(dom, dev); kref_put(&mapping->kref, release_iommu_mapping); - to_dma_iommu_mapping(dev) = NULL; pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev)); } @@ -2075,7 +2084,7 @@ static struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent) static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size, struct iommu_ops *iommu) { - struct dma_iommu_mapping *mapping; + struct iommu_domain *mapping; if (!iommu) return false; @@ -2099,13 +2108,13 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size, static void arm_teardown_iommu_dma_ops(struct device *dev) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); + struct iommu_domain *mapping = iommu_get_domain_for_dev(dev); if (!mapping) return; __arm_iommu_detach_device(dev); - arm_iommu_release_mapping(mapping); + arm_iommu_release_mapping(mapping->iova_cookie); } #else