RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper

2012-03-29 Thread Marek Szyprowski
Hello,

On Thursday, March 29, 2012 9:19 AM Hiroshi Doyu wrote:

 On Wed, 29 Feb 2012 16:04:22 +0100
 Marek Szyprowski m.szyprow...@samsung.com wrote:
 
  This patch add a complete implementation of DMA-mapping API for
  devices that have IOMMU support. All DMA-mapping calls are supported.
 
  This patch contains some of the code kindly provided by Krishna Reddy
  vdu...@nvidia.com and Andrzej Pietrasiewicz andrze...@samsung.com
 
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
  Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com
  ---
   arch/arm/Kconfig |8 +
   arch/arm/include/asm/device.h|3 +
   arch/arm/include/asm/dma-iommu.h |   34 ++
   arch/arm/mm/dma-mapping.c|  726 
  +-
   arch/arm/mm/vmregion.h   |2 +-
   5 files changed, 758 insertions(+), 15 deletions(-)
   create mode 100644 arch/arm/include/asm/dma-iommu.h
 
 
 snip
 
  +/*
  + * Map a part of the scatter-gather list into contiguous io address space
  + */
  +static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
  + size_t size, dma_addr_t *handle,
  + enum dma_data_direction dir)
  +{
  +   struct dma_iommu_mapping *mapping = dev-archdata.mapping;
  +   dma_addr_t iova, iova_base;
  +   int ret = 0;
  +   unsigned int count;
  +   struct scatterlist *s;
  +
  +   size = PAGE_ALIGN(size);
  +   *handle = ARM_DMA_ERROR;
  +
  +   iova_base = iova = __alloc_iova(mapping, size);
  +   if (iova == ARM_DMA_ERROR)
  +   return -ENOMEM;
  +
  +   for (count = 0, s = sg; count  (size  PAGE_SHIFT); s = 
  sg_next(s))
  +   {
  +   phys_addr_t phys = page_to_phys(sg_page(s));
  +   unsigned int len = PAGE_ALIGN(s-offset + s-length);
  +
  +   if (!arch_is_coherent())
  +   __dma_page_cpu_to_dev(sg_page(s), s-offset, 
  s-length, dir);
  +
  +   ret = iommu_map(mapping-domain, iova, phys, len, 0);
  +   if (ret  0)
  +   goto fail;
  +   count += len  PAGE_SHIFT;
  +   iova += len;
  +   }
  +   *handle = iova_base;
  +
  +   return 0;
  +fail:
  +   iommu_unmap(mapping-domain, iova_base, count * PAGE_SIZE);
  +   __free_iova(mapping, iova_base, size);
  +   return ret;
  +}
 
 Do we need to set dma_address as below?

Nope, this one is not correct. Please check the arm_iommu_map_sg() function. It 
calls 
__map_sg_chunk() and gives it dma-dma_address as one of the arguments, so the 
dma 
address is correctly stored in the scatter list. Please note that scatterlist 
is really
non-trivial structure and information about physical memory pages/chunks is 
disjoint 
from the information about dma address space chunks, although both are stored 
on the 
same list. In arm_iommu_map_sg() 's' pointer is used for physical memory chunks 
and 
'dma' pointer is used for dma address space chunks. 

The number of dma address space chunks (returned by arm_iommu_map_sq) can be 
lower 
than the number of physical memory chunks (given as nents argument).

In case of IOMMU you usually construct the scatter list in such a way, that you 
get
only one dma address chunk (so in fact you get a buffer mapped contiguously in 
virtual io address space).

 From e8bcc3cdac5375b5d7f5ac5b3f716a11c1008f38 Mon Sep 17 00:00:00 2001
 From: Hiroshi DOYU hd...@nvidia.com
 Date: Thu, 29 Mar 2012 09:59:04 +0300
 Subject: [PATCH 1/1] ARM: dma-mapping: Fix dma_address in sglist
 
 s-dma_address wasn't set at mapping.
 
 Signed-off-by: Hiroshi DOYU hd...@nvidia.com
 ---
  arch/arm/mm/dma-mapping.c |2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
 
 diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
 index 3347c37..11a9d65 100644
 --- a/arch/arm/mm/dma-mapping.c
 +++ b/arch/arm/mm/dma-mapping.c
 @@ -,6 +,8 @@ static int __map_sg_chunk(struct device *dev, struct 
 scatterlist *sg,
   ret = iommu_map(mapping-domain, iova, phys, len, 0);
   if (ret  0)
   goto fail;
 + s-dma_address = iova;
 +
   count += len  PAGE_SHIFT;
   iova += len;
   }

The above patch is not needed at all.

Best regards
-- 
Marek Szyprowski
Samsung Poland RD Center


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


Re: [PATCH 1/2] Isolation groups

2012-03-29 Thread David Gibson
On Tue, Mar 27, 2012 at 01:34:43PM -0600, Alex Williamson wrote:
[snip]
this case, it gets a bit complex.  When the FooBus isolation provider
is active, the FooBus devices would be in their own groups, not the
group of the FooBridge and its sibling.  When the FooBus isolation
provider is removed, it would have to configure the FooBus IOMMU to a
passthrough mode, and revert the FooBus devices to the parent's
group.  Hm.  Complicated.
   
   Yep.  I think we're arriving at the same point.  Groups are
   hierarchical, but ownership via a manager cannot be nested.  So to
   manage a group, we need to walk the entire tree of devices below each
   device checking that none of the groups are managed and all the devices
   are using the right driver, then walk up from the group to verify no
   group of a parent device is managed.  Thanks,
  
  Blargh.  I really, really hope we can come up with a simpler model
  than that.
 
 Yep, I'm pretty well at the end of this experiment.  Honestly, I think
 isolation groups are the wrong approach.  We're trying to wrap too many
 concepts together and it's completely unmanageable.  I cannot see adding
 the complexity we're talking about here to the core device model for
 such a niche usage.  I think we're better off going back to the
 iommu_device_group() and building that out into something more complete,
 starting with group based iommu ops and a dma quirk infrastructure.
 From there we can add some basic facilities to toggle driver autoprobe,
 maybe setup notifications for the group, and hopefully include a way to
 share iommu mappings between groups.  Anything much beyond that we
 should probably leave for something like the vfio driver.  Thanks,

Yes, well, I was hoping for a simpler model that didn't involve simply
sweeping all the issues under a rug.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu