Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU

2018-04-13 Thread Mehta, Sohil 
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(_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(_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

2018-04-13 Thread Mehta, Sohil 
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

2018-04-13 Thread Rob Herring 
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()

2018-04-13 Thread Jacopo Mondi 
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, ))
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()

2018-04-13 Thread Robin Murphy 

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, ))
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()

2018-04-13 Thread jacopo mondi 
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()

2018-04-13 Thread Christoph Hellwig 
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

2018-04-13 Thread Sebastian Andrzej Siewior 
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

2018-04-13 Thread Jean-Philippe Brucker 
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