Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > + > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > +{ > + char name[MAX_NAME_LEN + 1]; > + struct dentry *d_top; > + > + if (!debugfs_initialized()) Probably not needed. > + return; > + > + mutex_lock(&amd_iommu_debugfs_lock); > + if (!amd_iommu_debugfs) { > + d_top = iommu_debugfs_setup(); > + if (d_top) > + amd_iommu_debugfs = > debugfs_create_dir("amd", d_top); > + } > + mutex_unlock(&amd_iommu_debugfs_lock); You can do the above only once if you iterate over the IOMMUs here instead of doing it in amd_iommu_init. > + if (amd_iommu_debugfs) { > + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu- > >index); > + iommu->debugfs = debugfs_create_dir(name, > + amd_iommu_debugf > s); > + if (!iommu->debugfs) { > + debugfs_remove_recursive(amd_iommu_debugfs); > + amd_iommu_debugfs = NULL; > + } > + } > +} -Sohil ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/2] iommu - Enable debugfs exposure of the IOMMU
On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > > +struct dentry *iommu_debugfs_setup(void) > +{ > + if (!debugfs_initialized()) This check is probably not needed. > + return NULL; > + > + if (!iommu_debugfs_dir) > + iommu_debugfs_dir = debugfs_create_dir("iommu", > NULL); > + > + if (iommu_debugfs_dir) > + pr_warn("WARNING: IOMMU DEBUGFS SUPPORT HAS BEEN > ENABLED IN THIS KERNEL\n"); > + As this gets called for each IOMMU, do you want to use pr_warn_once? > + return iommu_debugfs_dir; > +} > +EXPORT_SYMBOL_GPL(iommu_debugfs_setup); -Sohil ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] dt-bindings: iommu/rockchip: Make clock properties optional
On Tue, Apr 10, 2018 at 01:26:41PM +0200, Heiko Stuebner wrote: > Hi Robin, > > Am Dienstag, 10. April 2018, 13:18:48 CEST schrieb Robin Murphy: > > On 10/04/18 10:26, Heiko Stuebner wrote: > > > Rockchip IOMMUs are used without explicit clock handling for 4 years > > > now, so we should keep compatibility with old devicetrees if possible. > > > Therefore make iommu clocks optional. > > > > Do we need to touch the binding itself? Obviously the driver has to > > treat clocks as optional in existing DTs (and I feel a bit dumb now for > > managing to overlook that in review), but the binding effectively only > > covers future DTs, and I'd assume we want to encourage the clocks to be > > correctly specified there. I'd prefer the DT docs reflect what is correct for new/current dts files. That's the only way the docs can validate the dts files. > I guess that depends on your definition of the timespan for backwards > compatibility. I'm always starting out at indefinite till convinced > otherwise ;-). Hence the clocks would need to stay optional for (nearly) > forever. > > Also, having the binding claim them as required but the code handling > them as optional just calls for someone to remove the optional handling :-D A comment in the code saying why missing clocks are allowed should suffice. > Not sure if there is a established way of saying > "we want this for all future devices, but allow it to be missing for old dts". We don't really... Rob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] base: dma-mapping: Postpone cpu addr translation on mmap()
Postpone calling virt_to_page() translation on memory locations not guaranteed to be backed by a struct page. Try first to map memory from device's coherent memory pool, then perform translation if that fails. On some architectures, specifically SH when configured with SPARSEMEM memory model, assuming a struct page is always assigned to a memory address lead to unexpected hangs during the virtual to page address translation. This patch fixes that specific issue but applies in the general case too. Suggested-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- It has now been clarified this patch does not resolve the issue, but only mitigate it on platforms where dma_mmap_from_dev_coherent() succeeds and delay page_to_pfn() faulty conversion. A suggested proper solution would be not relying on dma_common_mmap() but require all platforms to implement an mmap methods known to work, as noted by Christoph in v1 review. v1 -> v2: - Save the 'pfn' temp variable performing the page_to_pfn() conversion in the remap_pfn_range() function call as suggested by Christoph. --- drivers/base/dma-mapping.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c index 3b11835..d82566d 100644 --- a/drivers/base/dma-mapping.c +++ b/drivers/base/dma-mapping.c @@ -226,7 +226,6 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, #ifndef CONFIG_ARCH_NO_COHERENT_DMA_MMAP unsigned long user_count = vma_pages(vma); unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); unsigned long off = vma->vm_pgoff; vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); @@ -234,12 +233,11 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret)) return ret; - if (off < count && user_count <= (count - off)) { + if (off < count && user_count <= (count - off)) ret = remap_pfn_range(vma, vma->vm_start, - pfn + off, + page_to_pfn(virt_to_page(cpu_addr)) + off, user_count << PAGE_SHIFT, vma->vm_page_prot); - } #endif /* !CONFIG_ARCH_NO_COHERENT_DMA_MMAP */ return ret; -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] base: dma-mapping: Postpone cpu addr translation on mmap()
On 13/04/18 18:25, Jacopo Mondi wrote: Postpone calling virt_to_page() translation on memory locations not guaranteed to be backed by a struct page. Try first to map memory from device's coherent memory pool, then perform translation if that fails. On some architectures, specifically SH when configured with SPARSEMEM memory model, assuming a struct page is always assigned to a memory address lead to unexpected hangs during the virtual to page address translation. This patch fixes that specific issue but applies in the general case too. Reviewed-by: Robin Murphy Suggested-by: Laurent Pinchart Signed-off-by: Jacopo Mondi --- It has now been clarified this patch does not resolve the issue, but only mitigate it on platforms where dma_mmap_from_dev_coherent() succeeds and delay page_to_pfn() faulty conversion. A suggested proper solution would be not relying on dma_common_mmap() but require all platforms to implement an mmap methods known to work, as noted by Christoph in v1 review. Note that that "proper solution" should still involve having dma_common_mmap() since we certainly don't want an explosion of code duplication. It just means that architectures that do use it should be defining their dma_map_ops with an explicit ".mmap = dma_common_mmap" instead of relying on dma_mmap_attrs() calling it by default. Thus the more architectures this implementation *is* definitely safe for, the better :) Robin. v1 -> v2: - Save the 'pfn' temp variable performing the page_to_pfn() conversion in the remap_pfn_range() function call as suggested by Christoph. --- drivers/base/dma-mapping.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c index 3b11835..d82566d 100644 --- a/drivers/base/dma-mapping.c +++ b/drivers/base/dma-mapping.c @@ -226,7 +226,6 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, #ifndef CONFIG_ARCH_NO_COHERENT_DMA_MMAP unsigned long user_count = vma_pages(vma); unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); unsigned long off = vma->vm_pgoff; vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); @@ -234,12 +233,11 @@ int dma_common_mmap(struct device *dev, struct vm_area_struct *vma, if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret)) return ret; - if (off < count && user_count <= (count - off)) { + if (off < count && user_count <= (count - off)) ret = remap_pfn_range(vma, vma->vm_start, - pfn + off, + page_to_pfn(virt_to_page(cpu_addr)) + off, user_count << PAGE_SHIFT, vma->vm_page_prot); - } #endif/* !CONFIG_ARCH_NO_COHERENT_DMA_MMAP */ return ret; -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()
Hello again, On Tue, Apr 10, 2018 at 09:57:52AM +0200, jacopo mondi wrote: > Hi Christoph, > > On Mon, Apr 09, 2018 at 10:52:51AM -0700, Christoph Hellwig wrote: > > On Mon, Apr 09, 2018 at 06:59:08PM +0200, Jacopo Mondi wrote: > > > I'm still a bit puzzled on what happens if dma_mmap_from_dev_coherent() > > > fails. > > > Does a dma_mmap_from_dev_coherent() failure guarantee anyhow that the > > > successive virt_to_page() isn't problematic as it is today? > > > Or is it the > > > if (off < count && user_count <= (count - off)) > > > check that makes the translation safe? > > > > It doesn't. I think one major issue is that we should not simply fall > > to dma_common_mmap if no method is required, but need every instance of > > dma_map_ops to explicitly opt into an mmap method that is known to work. > > I see.. this patch thus just postpones the problem... > > > > > > #ifndef CONFIG_ARCH_NO_COHERENT_DMA_MMAP > > > unsigned long user_count = vma_pages(vma); > > > unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT; > > > - unsigned long pfn = page_to_pfn(virt_to_page(cpu_addr)); > > > unsigned long off = vma->vm_pgoff; > > > + unsigned long pfn; > > > > > > vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); > > > > > > @@ -235,6 +235,7 @@ int dma_common_mmap(struct device *dev, struct > > > vm_area_struct *vma, > > > return ret; > > > > > > if (off < count && user_count <= (count - off)) { > > > + pfn = page_to_pfn(virt_to_page(cpu_addr)); > > > ret = remap_pfn_range(vma, vma->vm_start, > > > pfn + off, > > > user_count << PAGE_SHIFT, > > > > Why not: > > > > ret = remap_pfn_range(vma, vma->vm_start, > > page_to_pfn(virt_to_page(cpu_addr)) + off, > > > > and save the temp variable? > > Sure, it's better... Should I send a v2 or considering your above > comment this patch is just a mitigation and should be ditched in > favour of a proper solution (which requires a much more considerable amount > of work though)? Don't want to be insistent, but I didn't get from your reply if a v2 is welcome or not :) Thanks j signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] base: dma-mapping: Postpone cpu addr translation on mmap()
Please send a v2. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: amd: hide unused iommu_table_lock
On 2018-04-04 12:56:59 [+0200], Arnd Bergmann wrote: > The newly introduced lock is only used when CONFIG_IRQ_REMAP is enabled: > > drivers/iommu/amd_iommu.c:86:24: error: 'iommu_table_lock' defined but not > used [-Werror=unused-variable] > static DEFINE_SPINLOCK(iommu_table_lock); > > This moves the definition next to the user, within the #ifdef protected > section of the file. > > Fixes: ea6166f4b83e ("iommu/amd: Split irq_lookup_table out of the > amd_iommu_devtable_lock") > Signed-off-by: Arnd Bergmann Acked-by: Sebastian Andrzej Siewior Thank you Arnd. Sebastian ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 03/37] iommu/sva: Manage process address spaces
On 10/04/18 19:53, Sinan Kaya wrote: > On 2/12/2018 1:33 PM, Jean-Philippe Brucker wrote: >> +static void io_mm_detach_all_locked(struct iommu_bond *bond) >> +{ >> +while (!io_mm_detach_locked(bond)); >> +} >> + > > I don't remember if I mentioned this before or not but I think this loop > needs a little bit relaxation with yield and maybe an informational message > with might help if wait exceeds some time. Right, at the very least we should have a cpu_relax here. I think this bit is going away, though, because I want to lift the possibility of calling bind() for the same dev/mm pair multiple times. It's not useful in my opinion because that call could only be issued by a given driver. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu