Re: [PATCH v2] dma-direct: do not allocate a single page from CMA area

[Christoph Hellwig]

On Wed, Feb 06, 2019 at 06:28:49PM -0800, Nicolin Chen wrote:
> So we will keep allocating single pages in dev->cma_area if it's
> present, in order to address your previous concern?

Yes.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: use generic DMA mapping code in powerpc V4

[Christian Zigotzky]

Hi Christoph,

I also didn’t notice the 32-bit DMA mask in your patch. I have to read your 
patches and descriptions carefully in the future. I will test your new patch at 
the weekend.

Thanks,
Christian

Sent from my iPhone

> On 6. Feb 2019, at 16:16, Christoph Hellwig  wrote:
> 
>> On Wed, Feb 06, 2019 at 04:15:05PM +0100, Christoph Hellwig wrote:
>> The last good one was 29e7e2287e196f48fe5d2a6e017617723ea979bf
>> ("dma-direct: we might need GFP_DMA for 32-bit dma masks"), if I
>> remember correctly.  powerpc/dma: use the dma_direct mapping routines
>> was the one that you said makes the pasemi ethernet stop working.
>> 
>> Can you post the dmesg from the failing runs?
> 
> But I just noticed I sent you a wrong patch - the pasemi ethernet
> should set a 64-bit DMA mask, not 32-bit.  Updated version below,
> 32-bit would just keep the previous status quo.
> 
> commit 6c8f88045dee3597b9ce2ea5371eee37073a
> Author: Christoph Hellwig 
> Date:   Mon Feb 4 13:38:22 2019 +0100
> 
>pasemi WIP
> 
> diff --git a/drivers/net/ethernet/pasemi/pasemi_mac.c 
> b/drivers/net/ethernet/pasemi/pasemi_mac.c
> index 8a31a02c9f47..2d7d1589490a 100644
> --- a/drivers/net/ethernet/pasemi/pasemi_mac.c
> +++ b/drivers/net/ethernet/pasemi/pasemi_mac.c
> @@ -1716,6 +1716,7 @@ pasemi_mac_probe(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>err = -ENODEV;
>goto out;
>}
> +dma_set_mask(>dma_pdev->dev, DMA_BIT_MASK(64));
> 
>mac->iob_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa001, NULL);
>if (!mac->iob_pdev) {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] dma-direct: do not allocate a single page from CMA area

[Nicolin Chen]

Hi Christoph,

On Wed, Feb 06, 2019 at 08:07:26AM +0100, Christoph Hellwig wrote:
> On Tue, Feb 05, 2019 at 03:05:30PM -0800, Nicolin Chen wrote:
> > > And my other concern is that this skips allocating from the per-device
> > > pool, which drivers might rely on.
> > 
> > Actually Robin had the same concern at v1 and suggested that we could
> > always use DMA_ATTR_FORCE_CONTIGUOUS to enforce into per-device pool.
> 
> That is both against the documented behavior of DMA_ATTR_FORCE_CONTIGUOUS
> and doesn't help existing drivers that specify their CMA area in DT.

OK. I will drop it.

> > > To be honest I'm not sure there is
> > > much of a point in the per-device CMA pool vs the traditional per-device
> > > coherent pool, but I'd rather change that behavior in a clearly documented
> > > commit with intentions rather as a side effect from a random optimization.
> > 
> > Hmm..sorry, I don't really follow this suggestion. Is it possible for
> > you to make it clear that what should I do for the change?
> 
> Something like this (plus proper comments):
> 
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index b2a87905846d..789d734f0f77 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -192,10 +192,19 @@ int __init dma_contiguous_reserve_area(phys_addr_t 
> size, phys_addr_t base,
>  struct page *dma_alloc_from_contiguous(struct device *dev, size_t count,
>  unsigned int align, bool no_warn)
>  {
> + struct cma *cma;
> +
>   if (align > CONFIG_CMA_ALIGNMENT)
>   align = CONFIG_CMA_ALIGNMENT;
>  
> - return cma_alloc(dev_get_cma_area(dev), count, align, no_warn);
> + if (dev && dev->cma_area)
> + cma = dev->cma_area;
> + else if (count > PAGE_SIZE)
> + cma = dma_contiguous_default_area;
> + else
> + return NULL;

So we will keep allocating single pages in dev->cma_area if it's
present, in order to address your previous concern?

Thanks
Nicolin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 03/19] dma-iommu: don't use a scatterlist in iommu_dma_alloc

[Robin Murphy]

On 01/02/2019 16:16, Christoph Hellwig wrote:

On Fri, Feb 01, 2019 at 03:24:45PM +, Robin Murphy wrote:

On 14/01/2019 09:41, Christoph Hellwig wrote:

Directly iterating over the pages makes the code a bit simpler and
prepares for the following changes.


It also defeats the whole purpose of __iommu_dma_alloc_pages(), so I'm not
really buying the simplification angle - you've *seen* that code, right? ;)


How does it defeat the purpose of __iommu_dma_alloc_pages?


Because if iommu_map() only gets called at PAGE_SIZE granularity, then 
the IOMMU PTEs will be created at PAGE_SIZE (or smaller) granularity, so 
any effort to get higher-order allocations matching larger IOMMU block 
sizes is wasted, and we may as well have just done this:


for (i = 0; i < count; i++) {
struct page *page = alloc_page(gfp);
...
iommu_map(..., page_to_phys(page), PAGE_SIZE, ...);
}

Really, it's a shame we have to split huge pages for the CPU remap, 
since in the common case the CPU MMU will have a matching block size, 
but IIRC there was something in vmap() or thereabouts that explicitly 
chokes on them.


Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: use generic DMA mapping code in powerpc V4

[Christoph Hellwig]

On Wed, Feb 06, 2019 at 04:15:05PM +0100, Christoph Hellwig wrote:
> The last good one was 29e7e2287e196f48fe5d2a6e017617723ea979bf
> ("dma-direct: we might need GFP_DMA for 32-bit dma masks"), if I
> remember correctly.  powerpc/dma: use the dma_direct mapping routines
> was the one that you said makes the pasemi ethernet stop working.
> 
> Can you post the dmesg from the failing runs?

But I just noticed I sent you a wrong patch - the pasemi ethernet
should set a 64-bit DMA mask, not 32-bit.  Updated version below,
32-bit would just keep the previous status quo.

commit 6c8f88045dee3597b9ce2ea5371eee37073a
Author: Christoph Hellwig 
Date:   Mon Feb 4 13:38:22 2019 +0100

pasemi WIP

diff --git a/drivers/net/ethernet/pasemi/pasemi_mac.c 
b/drivers/net/ethernet/pasemi/pasemi_mac.c
index 8a31a02c9f47..2d7d1589490a 100644
--- a/drivers/net/ethernet/pasemi/pasemi_mac.c
+++ b/drivers/net/ethernet/pasemi/pasemi_mac.c
@@ -1716,6 +1716,7 @@ pasemi_mac_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
err = -ENODEV;
goto out;
}
+   dma_set_mask(>dma_pdev->dev, DMA_BIT_MASK(64));
 
mac->iob_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa001, NULL);
if (!mac->iob_pdev) {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: use generic DMA mapping code in powerpc V4

[Christoph Hellwig]

On Wed, Feb 06, 2019 at 02:45:34PM +0100, Christian Zigotzky wrote:
> I patched the source code from the Git 'powerpc-dma.6' with your patch 
> today. Unfortunately the P.A. Semi Ethernet doesn't work with the patched 
> Git kernel.
>
> After that I tried it with the patch applied over the working setup again 
> (powerpc/dma: use the dma_direct mapping routines). Unfortunately after 
> compiling
> and booting, the P.A. Semi Ethernet doesn't work either.

The last good one was 29e7e2287e196f48fe5d2a6e017617723ea979bf
("dma-direct: we might need GFP_DMA for 32-bit dma masks"), if I
remember correctly.  powerpc/dma: use the dma_direct mapping routines
was the one that you said makes the pasemi ethernet stop working.

Can you post the dmesg from the failing runs?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 02/19] dma-iommu: cleanup dma-iommu.h

[Robin Murphy]

On 01/02/2019 16:13, Christoph Hellwig wrote:

On Fri, Feb 01, 2019 at 02:47:17PM +, Robin Murphy wrote:

On 14/01/2019 09:41, Christoph Hellwig wrote:

No need for a __KERNEL__ guard outside uapi, make sure we pull in the
includes unconditionally so users can rely on it, and add a missing
comment describing the #else cpp statement.  Last but not least include
 instead of the asm version, which is frowned upon.


I think the __KERNEL__ and asm/errno.h slip-ups are things I cargo-culted
from the arch code as a fresh-faced noob yet to learn the finer details, so
ack for those parts. The forward-declarations, though, were a deliberate
effort to minimise header dependencies and compilation bloat for includers
who absolutely wouldn't care, and specifically to try to avoid setting
transitive include expectations since they always seem to end up breaking
someone's config somewhere down the line. Admittedly this little backwater
is hardly comparable to the likes of the sched.h business, but I'm still
somewhat on the fence about that change :/


As far as I can tell almost all users of linux/dma-iommu.h require
CONFIG_DMA_IOMMU to be enabled anyway..


Other than dma-iommu.c itself, none of them *require* it - only 
arch/arm64 selects it (the one from MTK_IOMMU is just bogus), and a lot 
of the drivers also build for at least one other architecture (and/or 
arm64 with !IOMMU_API).


Either way, I have no vehement objection to the change, I just don't see 
any positive value in it.


Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: use generic DMA mapping code in powerpc V4

[Christian Zigotzky]

On 04 February 2019 at 01:38PM, Christoph Hellwig wrote:


It seems like the pasemi driver fails to set a DMA mask, but seems
otherwise 64-bit DMA capable.  The old PPC code didn't verify the
dma mask during the map operations, but the x86-derived generic
code does.

This patch just sets the DMA mask.

Olof: does this look ok?  The DMA device seems to not directly
bound by the net driver, but not really used by anything else in tree
either..

diff --git a/drivers/net/ethernet/pasemi/pasemi_mac.c 
b/drivers/net/ethernet/pasemi/pasemi_mac.c
index d21041554507..d98bd447c536 100644
--- a/drivers/net/ethernet/pasemi/pasemi_mac.c
+++ b/drivers/net/ethernet/pasemi/pasemi_mac.c
@@ -1716,6 +1716,7 @@ pasemi_mac_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
err = -ENODEV;
goto out;
}
+   dma_set_mask(>dma_pdev->dev, DMA_BIT_MASK(32));
  
  	mac->iob_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa001, NULL);

if (!mac->iob_pdev) {


Hello Christoph,

I patched the source code from the Git 'powerpc-dma.6' with your patch 
today. Unfortunately the P.A. Semi Ethernet doesn't work with the 
patched Git kernel.


After that I tried it with the patch applied over the working setup 
again (powerpc/dma: use the dma_direct mapping routines). Unfortunately 
after compiling

and booting, the P.A. Semi Ethernet doesn't work either.

Cheers,
Christian

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 18/19] arm64: switch copyright boilerplace to SPDX in dma-mapping.c

[Robin Murphy]

On 14/01/2019 09:41, Christoph Hellwig wrote:

Signed-off-by: Christoph Hellwig 


Acked-by: Robin Murphy 


---
  arch/arm64/mm/dma-mapping.c | 15 +--
  1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index fffba9426ee4..bdfb4e985a69 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -1,20 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
  /*
- * SWIOTLB-based DMA API implementation
- *
   * Copyright (C) 2012 ARM Ltd.
   * Author: Catalin Marinas 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see .
   */
  
  #include 



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 17/19] dma-iommu: switch copyright boilerplace to SPDX

[Robin Murphy]

On 14/01/2019 09:41, Christoph Hellwig wrote:

Signed-off-by: Christoph Hellwig 


Acked-by: Robin Murphy 


---
  drivers/iommu/dma-iommu.c | 13 +
  include/linux/dma-iommu.h | 13 +
  2 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index e27909771d55..1b76121df94e 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
  /*
   * A fairly generic DMA-API to IOMMU-API glue layer.
   *
@@ -5,18 +6,6 @@
   *
   * based in part on arch/arm/mm/dma-mapping.c:
   * Copyright (C) 2000-2004 Russell King
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see .
   */
  
  #include 

diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 5277aa8782bf..bfe9f19b1171 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -1,17 +1,6 @@
+/* SPDX-License-Identifier: GPL-2.0 */
  /*
   * Copyright (C) 2014-2015 ARM Ltd.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see .
   */
  #ifndef __DMA_IOMMU_H
  #define __DMA_IOMMU_H


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 16/19] dma-iommu: don't depend on CONFIG_DMA_DIRECT_REMAP

[Robin Murphy]

On 14/01/2019 09:41, Christoph Hellwig wrote:

For entirely dma coherent architectures there is no good reason to ever
remap dma coherent allocation.


Yes there is, namely assembling large buffers without the need for 
massive CMA areas and compaction overhead under memory fragmentation. 
That has always been a distinct concern from the DMA_DIRECT_REMAP cases; 
they've just been able to share a fair few code paths.



 Move all the remap and pool code under
CONFIG_DMA_DIRECT_REMAP ifdefs, and drop the Kconfig dependency.


As far as I'm concerned that splits things the wrong way. Logically, 
iommu_dma_alloc() should always have done its own vmap() instead of just 
returning the bare pages array, but that was tricky to resolve with the 
design of having the caller handle everything to do with coherency 
(forcing the caller to unpick that mapping just to remap it yet again in 
the noncoherent case didn't seem sensible).


Robin.


Signed-off-by: Christoph Hellwig 
---
  drivers/iommu/Kconfig |  1 -
  drivers/iommu/dma-iommu.c | 10 ++
  2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 8b13fb7d0263..d9a25715650e 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -94,7 +94,6 @@ config IOMMU_DMA
select IOMMU_API
select IOMMU_IOVA
select NEED_SG_DMA_LENGTH
-   depends on DMA_DIRECT_REMAP
  
  config FSL_PAMU

bool "Freescale IOMMU support"
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index fd25c995bde4..e27909771d55 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -502,6 +502,7 @@ static void *iommu_dma_alloc_contiguous(struct device *dev, 
size_t size,
return page_address(page);
  }
  
+#ifdef CONFIG_DMA_DIRECT_REMAP

  static void __iommu_dma_free_pages(struct page **pages, int count)
  {
while (count--)
@@ -775,6 +776,7 @@ static void *iommu_dma_alloc_noncoherent(struct device 
*dev, size_t size,
gfp, attrs);
return iommu_dma_alloc_remap(dev, size, dma_handle, gfp, attrs);
  }
+#endif /* CONFIG_DMA_DIRECT_REMAP */
  
  static void iommu_dma_sync_single_for_cpu(struct device *dev,

dma_addr_t dma_handle, size_t size, enum dma_data_direction dir)
@@ -1057,6 +1059,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t 
size,
 */
gfp |= __GFP_ZERO;
  
+#ifdef CONFIG_DMA_DIRECT_REMAP

if (!dev_is_dma_coherent(dev))
return iommu_dma_alloc_noncoherent(dev, size, dma_handle, gfp,
attrs);
@@ -1064,6 +1067,7 @@ static void *iommu_dma_alloc(struct device *dev, size_t 
size,
if (gfpflags_allow_blocking(gfp) &&
!(attrs & DMA_ATTR_FORCE_CONTIGUOUS))
return iommu_dma_alloc_remap(dev, size, dma_handle, gfp, attrs);
+#endif
  
  	return iommu_dma_alloc_contiguous(dev, size, dma_handle, gfp, attrs);

  }
@@ -1083,6 +1087,7 @@ static void iommu_dma_free(struct device *dev, size_t 
size, void *cpu_addr,
 *
 * Hence how dodgy the below logic looks...
 */
+#ifdef CONFIG_DMA_DIRECT_REMAP
if (dma_in_atomic_pool(cpu_addr, PAGE_ALIGN(size))) {
iommu_dma_free_pool(dev, size, cpu_addr, dma_handle);
return;
@@ -1096,6 +1101,7 @@ static void iommu_dma_free(struct device *dev, size_t 
size, void *cpu_addr,
page = vmalloc_to_page(cpu_addr);
dma_common_free_remap(cpu_addr, PAGE_ALIGN(size), VM_USERMAP);
} else
+#endif
page = virt_to_page(cpu_addr);
  
  	iommu_dma_free_contiguous(dev, size, page, dma_handle);

@@ -1119,11 +1125,13 @@ static int iommu_dma_mmap(struct device *dev, struct 
vm_area_struct *vma,
if (off >= count || user_count > count - off)
return -ENXIO;
  
+#ifdef CONFIG_DMA_DIRECT_REMAP

if (is_vmalloc_addr(cpu_addr)) {
if (!(attrs & DMA_ATTR_FORCE_CONTIGUOUS))
return iommu_dma_mmap_remap(cpu_addr, size, vma);
pfn = vmalloc_to_pfn(cpu_addr);
} else
+#endif
pfn = page_to_pfn(virt_to_page(cpu_addr));
  
  	return remap_pfn_range(vma, vma->vm_start, pfn + vma->vm_pgoff,

@@ -1137,11 +1145,13 @@ static int iommu_dma_get_sgtable(struct device *dev, 
struct sg_table *sgt,
struct page *page;
int ret;
  
+#ifdef CONFIG_DMA_DIRECT_REMAP

if (is_vmalloc_addr(cpu_addr)) {
if (!(attrs & DMA_ATTR_FORCE_CONTIGUOUS))
return iommu_dma_get_sgtable_remap(sgt, cpu_addr, size);
page = vmalloc_to_page(cpu_addr);
} else
+#endif
page = virt_to_page(cpu_addr);
  
  	ret = sg_alloc_table(sgt, 1, GFP_KERNEL);



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu