Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2015-01-21 Thread Laurent Pinchart
Hi Will,

On Wednesday 21 January 2015 14:48:35 Will Deacon wrote:
 On Tue, Jan 20, 2015 at 04:56:03PM +, Laurent Pinchart wrote:
  On Monday 19 January 2015 16:06:20 Will Deacon wrote:
   On Fri, Nov 14, 2014 at 08:01:56PM +, Arnd Bergmann wrote:
On Friday 14 November 2014 19:27:54 Will Deacon wrote:
  At the moment, iommu_ops is a structure that can get used for any
  number of iommus of the same type, but by putting per-device
  private data into the same structure you have to duplicate it per
  instance.
 
 I'm not sure I agree -- the pgsize_bitmap, for example, could vary
 between different implementations of the same IOMMU. I think we
 already have this in Juno (some SMMUs can only do 64k pages, whilst
 others can do 4k and 64k).

Ah, I hadn't noticed that, it should be in the 'struct iommu' then of
course, not in iommu_ops.
   
   Now that of_iommu_get_ops has its own list of iommu_ops, we can actually
   just move pgsize_bitmap into the iommu_domain, which I think makes a lot
   more sense anyway.
  
  The code looks good to me, but what does it have to do with
  of_iommu_get_ops() having its own list of iommu_ops ?
 
 So, the initial patches for of_iommu_configure/of_xlate made use of a void
 *priv field in struct iommu_ops (which actually made it to mainline but
 isn't used). This was replaced by the list maintained by of_iommu_get_ops,
 but the discussion highlighted that iommu_ops is not per-IOMMU instance and
 therefore shouldn't contain the pgsize_bitmap, which can (and does) vary
 between different IOMMU instances in a running system.
 
 This patch is an effort to move the pgsize_bitmap field out of iommu_ops
 (leaving that structure to contain only function pointers) and into
 iommu_domain, where I think it makes more sense.

OK, that's what I had understood. Thanks for the clarification. I was just 
puzzled by what I thought you meant was a dependency on of_iommu_get_ops().

   diff below seems to do the trick. The next step would be postponing the
   pgsize_bitmap initialisation until device attach for the ARM SMMU, since
   it's at that point that we know the active page table format (and
   therefore the supported page sizes).

-- 
Regards,

Laurent Pinchart

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2015-01-21 Thread Will Deacon
On Tue, Jan 20, 2015 at 04:56:03PM +, Laurent Pinchart wrote:
 Hi Will,

Hi Laurent,

 Thank you for the patch.
 
 On Monday 19 January 2015 16:06:20 Will Deacon wrote:
  (resurrecting an old thread)
 
  On Fri, Nov 14, 2014 at 08:01:56PM +, Arnd Bergmann wrote:
   On Friday 14 November 2014 19:27:54 Will Deacon wrote:
 At the moment, iommu_ops is a structure that can get used for any
 number of iommus of the same type, but by putting per-device private
 data into the same structure you have to duplicate it per instance.
   
I'm not sure I agree -- the pgsize_bitmap, for example, could vary
between different implementations of the same IOMMU. I think we already
have this in Juno (some SMMUs can only do 64k pages, whilst others can
do 4k and 64k).
  
   Ah, I hadn't noticed that, it should be in the 'struct iommu' then of
   course, not in iommu_ops.
 
  Now that of_iommu_get_ops has its own list of iommu_ops, we can actually
  just move pgsize_bitmap into the iommu_domain, which I think makes a lot
  more sense anyway.
 
 The code looks good to me, but what does it have to do with of_iommu_get_ops()
 having its own list of iommu_ops ?

So, the initial patches for of_iommu_configure/of_xlate made use of a void
*priv field in struct iommu_ops (which actually made it to mainline but
isn't used). This was replaced by the list maintained by of_iommu_get_ops,
but the discussion highlighted that iommu_ops is not per-IOMMU instance and
therefore shouldn't contain the pgsize_bitmap, which can (and does) vary
between different IOMMU instances in a running system.

This patch is an effort to move the pgsize_bitmap field out of iommu_ops
(leaving that structure to contain only function pointers) and into
iommu_domain, where I think it makes more sense.

Will

  diff below seems to do the trick. The next step would be postponing the
  pgsize_bitmap initialisation until device attach for the ARM SMMU, since
  it's at that point that we know the active page table format (and
  therefore the supported page sizes).
 
  Will
 
  ---8
 
  diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
  index 98024856df07..ab9702d01e9a 100644
  --- a/drivers/iommu/amd_iommu.c
  +++ b/drivers/iommu/amd_iommu.c
  @@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain
  *dom) dom-geometry.aperture_end   = ~0ULL;
dom-geometry.force_aperture = true;
 
  + dom-pgsize_bitmap = AMD_IOMMU_PGSIZES;
return 0;
 
   out_free:
  @@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = {
.unmap = amd_iommu_unmap,
.map_sg = default_iommu_map_sg,
.iova_to_phys = amd_iommu_iova_to_phys,
  - .pgsize_bitmap  = AMD_IOMMU_PGSIZES,
   };
 
   /**
  *** diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
  index 6cd47b75286f..898fab8d10a0 100644
  --- a/drivers/iommu/arm-smmu.c
  +++ b/drivers/iommu/arm-smmu.c
  @@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain
  *domain)
 
spin_lock_init(smmu_domain-lock);
domain-priv = smmu_domain;
  + domain-pgsize_bitmap = (SECTION_SIZE |
  +  ARM_SMMU_PTE_CONT_SIZE |
  +  PAGE_SIZE);
return 0;
 
   out_free_domain:
  @@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = {
.remove_device  = arm_smmu_remove_device,
.domain_get_attr= arm_smmu_domain_get_attr,
.domain_set_attr= arm_smmu_domain_set_attr,
  - .pgsize_bitmap  = (SECTION_SIZE |
  -ARM_SMMU_PTE_CONT_SIZE |
  -PAGE_SIZE),
   };
 
   static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
  diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
  index 7ce52737c7a1..404cad25db9b 100644
  --- a/drivers/iommu/exynos-iommu.c
  +++ b/drivers/iommu/exynos-iommu.c
  @@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain
  *domain) domain-geometry.aperture_end   = ~0UL;
domain-geometry.force_aperture = true;
 
  + domain-pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE;
  +
domain-priv = priv;
return 0;
 
  @@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = {
.iova_to_phys = exynos_iommu_iova_to_phys,
.add_device = exynos_iommu_add_device,
.remove_device = exynos_iommu_remove_device,
  - .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
   };
 
   static int __init exynos_iommu_init(void)
  diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
  index 40dfbc0444c0..e2b0f34baa9d 100644
  --- a/drivers/iommu/intel-iommu.c
  +++ b/drivers/iommu/intel-iommu.c
  @@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain
  *domain) domain-geometry.aperture_end   =
 

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2015-01-20 Thread Laurent Pinchart
Hi Will,

Thank you for the patch.

On Monday 19 January 2015 16:06:20 Will Deacon wrote:
 (resurrecting an old thread)
 
 On Fri, Nov 14, 2014 at 08:01:56PM +, Arnd Bergmann wrote:
  On Friday 14 November 2014 19:27:54 Will Deacon wrote:
At the moment, iommu_ops is a structure that can get used for any
number of iommus of the same type, but by putting per-device private
data into the same structure you have to duplicate it per instance.
   
   I'm not sure I agree -- the pgsize_bitmap, for example, could vary
   between different implementations of the same IOMMU. I think we already
   have this in Juno (some SMMUs can only do 64k pages, whilst others can
   do 4k and 64k).
 
  Ah, I hadn't noticed that, it should be in the 'struct iommu' then of
  course, not in iommu_ops.
 
 Now that of_iommu_get_ops has its own list of iommu_ops, we can actually
 just move pgsize_bitmap into the iommu_domain, which I think makes a lot
 more sense anyway.

The code looks good to me, but what does it have to do with of_iommu_get_ops() 
having its own list of iommu_ops ?

 diff below seems to do the trick. The next step would be postponing the
 pgsize_bitmap initialisation until device attach for the ARM SMMU, since
 it's at that point that we know the active page table format (and
 therefore the supported page sizes).
 
 Will
 
 ---8
 
 diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
 index 98024856df07..ab9702d01e9a 100644
 --- a/drivers/iommu/amd_iommu.c
 +++ b/drivers/iommu/amd_iommu.c
 @@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain
 *dom) dom-geometry.aperture_end   = ~0ULL;
   dom-geometry.force_aperture = true;
 
 + dom-pgsize_bitmap = AMD_IOMMU_PGSIZES;
   return 0;
 
  out_free:
 @@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = {
   .unmap = amd_iommu_unmap,
   .map_sg = default_iommu_map_sg,
   .iova_to_phys = amd_iommu_iova_to_phys,
 - .pgsize_bitmap  = AMD_IOMMU_PGSIZES,
  };
 
  /**
 *** diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
 index 6cd47b75286f..898fab8d10a0 100644
 --- a/drivers/iommu/arm-smmu.c
 +++ b/drivers/iommu/arm-smmu.c
 @@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain
 *domain)
 
   spin_lock_init(smmu_domain-lock);
   domain-priv = smmu_domain;
 + domain-pgsize_bitmap = (SECTION_SIZE |
 +  ARM_SMMU_PTE_CONT_SIZE |
 +  PAGE_SIZE);
   return 0;
 
  out_free_domain:
 @@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = {
   .remove_device  = arm_smmu_remove_device,
   .domain_get_attr= arm_smmu_domain_get_attr,
   .domain_set_attr= arm_smmu_domain_set_attr,
 - .pgsize_bitmap  = (SECTION_SIZE |
 -ARM_SMMU_PTE_CONT_SIZE |
 -PAGE_SIZE),
  };
 
  static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
 index 7ce52737c7a1..404cad25db9b 100644
 --- a/drivers/iommu/exynos-iommu.c
 +++ b/drivers/iommu/exynos-iommu.c
 @@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain
 *domain) domain-geometry.aperture_end   = ~0UL;
   domain-geometry.force_aperture = true;
 
 + domain-pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE;
 +
   domain-priv = priv;
   return 0;
 
 @@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = {
   .iova_to_phys = exynos_iommu_iova_to_phys,
   .add_device = exynos_iommu_add_device,
   .remove_device = exynos_iommu_remove_device,
 - .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
  };
 
  static int __init exynos_iommu_init(void)
 diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
 index 40dfbc0444c0..e2b0f34baa9d 100644
 --- a/drivers/iommu/intel-iommu.c
 +++ b/drivers/iommu/intel-iommu.c
 @@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain
 *domain) domain-geometry.aperture_end   =
 __DOMAIN_MAX_ADDR(dmar_domain-gaw); domain-geometry.force_aperture =
 true;
 
 + domain-pgsize_bitmap = INTEL_IOMMU_PGSIZES;
   return 0;
  }
 
 @@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = {
   .iova_to_phys   = intel_iommu_iova_to_phys,
   .add_device = intel_iommu_add_device,
   .remove_device  = intel_iommu_remove_device,
 - .pgsize_bitmap  = INTEL_IOMMU_PGSIZES,
  };
 
  static void quirk_iommu_g4x_gfx(struct pci_dev *dev)
 diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
 index f7718d73e984..b8b6b0bc6951 100644
 --- a/drivers/iommu/iommu.c
 +++ b/drivers/iommu/iommu.c
 @@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain
 *domain, pgsize = (1UL  (pgsize_idx + 1)) - 1;
 
   /* throw away page 

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2015-01-19 Thread Will Deacon
(resurrecting an old thread)

On Fri, Nov 14, 2014 at 08:01:56PM +, Arnd Bergmann wrote:
 On Friday 14 November 2014 19:27:54 Will Deacon wrote:
   At the moment, iommu_ops is a structure that can get used for any
   number of iommus of the same type, but by putting per-device private
   data into the same structure you have to duplicate it per instance.
  
  I'm not sure I agree -- the pgsize_bitmap, for example, could vary between
  different implementations of the same IOMMU. I think we already have this in
  Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).
 
 Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course,
 not in iommu_ops.

Now that of_iommu_get_ops has its own list of iommu_ops, we can actually
just move pgsize_bitmap into the iommu_domain, which I think makes a lot
more sense anyway.

diff below seems to do the trick. The next step would be postponing the
pgsize_bitmap initialisation until device attach for the ARM SMMU, since
it's at that point that we know the active page table format (and
therefore the supported page sizes).

Will

---8

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 98024856df07..ab9702d01e9a 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain *dom)
dom-geometry.aperture_end   = ~0ULL;
dom-geometry.force_aperture = true;
 
+   dom-pgsize_bitmap = AMD_IOMMU_PGSIZES;
return 0;
 
 out_free:
@@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = {
.unmap = amd_iommu_unmap,
.map_sg = default_iommu_map_sg,
.iova_to_phys = amd_iommu_iova_to_phys,
-   .pgsize_bitmap  = AMD_IOMMU_PGSIZES,
 };
 
 /*
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 6cd47b75286f..898fab8d10a0 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain 
*domain)
 
spin_lock_init(smmu_domain-lock);
domain-priv = smmu_domain;
+   domain-pgsize_bitmap = (SECTION_SIZE |
+ARM_SMMU_PTE_CONT_SIZE |
+PAGE_SIZE);
return 0;
 
 out_free_domain:
@@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = {
.remove_device  = arm_smmu_remove_device,
.domain_get_attr= arm_smmu_domain_get_attr,
.domain_set_attr= arm_smmu_domain_set_attr,
-   .pgsize_bitmap  = (SECTION_SIZE |
-  ARM_SMMU_PTE_CONT_SIZE |
-  PAGE_SIZE),
 };
 
 static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 7ce52737c7a1..404cad25db9b 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain 
*domain)
domain-geometry.aperture_end   = ~0UL;
domain-geometry.force_aperture = true;
 
+   domain-pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE;
+
domain-priv = priv;
return 0;
 
@@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = {
.iova_to_phys = exynos_iommu_iova_to_phys,
.add_device = exynos_iommu_add_device,
.remove_device = exynos_iommu_remove_device,
-   .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 };
 
 static int __init exynos_iommu_init(void)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 40dfbc0444c0..e2b0f34baa9d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain 
*domain)
domain-geometry.aperture_end   = __DOMAIN_MAX_ADDR(dmar_domain-gaw);
domain-geometry.force_aperture = true;
 
+   domain-pgsize_bitmap = INTEL_IOMMU_PGSIZES;
return 0;
 }
 
@@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = {
.iova_to_phys   = intel_iommu_iova_to_phys,
.add_device = intel_iommu_add_device,
.remove_device  = intel_iommu_remove_device,
-   .pgsize_bitmap  = INTEL_IOMMU_PGSIZES,
 };
 
 static void quirk_iommu_g4x_gfx(struct pci_dev *dev)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f7718d73e984..b8b6b0bc6951 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain *domain,
pgsize = (1UL  (pgsize_idx + 1)) - 1;
 
/* throw away page sizes not supported by the hardware */
-   pgsize = domain-ops-pgsize_bitmap;
+   pgsize = domain-pgsize_bitmap;
 
/* make sure we're still sane */
BUG_ON(!pgsize);
@@ -1046,11 +1046,11 

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-12-15 Thread Laurent Pinchart
Hi Will,

On Wednesday 19 November 2014 11:41:50 Will Deacon wrote:
 On Wed, Nov 19, 2014 at 11:21:26AM +, Marek Szyprowski wrote:
  On 2014-11-14 19:56, Will Deacon wrote:
   Hello everybody,
   
   Here is the fourth iteration of the RFC I've previously posted here:
  RFCv1:
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/28
  3023.html RFCv2:
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September
  /283752.html RFCv3:
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September
  /287031.html  
   Changes since RFCv3 include:
  - Drastic simplification of the data structures, so that we no longer
pass around lists of domains. Instead, dma-mapping is expected to
allocate the domain (Joerg talked about adding a get_default_domain
operation to iommu_ops).
  
  - iommu_ops is used to hold the per-instance IOMMU data
  
  - Configuration of DMA segments added to of_dma_configure
   
   All feedback welcome.
  
  I've rebased my Exynos SYSMMU patches on top of this patchset and it
  works fine,
  You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU)
  integration with DT and DMA-mapping subsystem thread.
 
 I just saw that and it looks great, thanks! FWIW, I'll take the first 3
 patches you have into my series in some shape or another.
 
  You can add to all your patches:
  Acked-by: Marek Szyprowski m.szyprow...@samsung.com
 
 Cheers.
 
  I'm also interested in adding get_default_domain() callback, but I
  assume that this
  can be done once the basic patchset get merged. Do you plan to work on
  it, do you want
  me to implement it?
 
 If Joerg isn't working on it already (I don't think he is), then please
 do have a go if you have time. You'll probably want to avoid adding devices
 with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks)
 to the default domain, otherwise you'll run into issues initialising the
 iova allocator.
 
 I had a go at getting ARM dma-mapping to use a hypothetical
 get_default_domain function, so I've included the diff I ended up with
 below, in case it's at all useful.
 
 Will
 
 ---8
 
 diff --git a/arch/arm/include/asm/dma-mapping.h
 b/arch/arm/include/asm/dma-mapping.h index f3c0d953f6a2..5071553bf6b8
 100644
 --- a/arch/arm/include/asm/dma-mapping.h
 +++ b/arch/arm/include/asm/dma-mapping.h
 @@ -121,14 +121,9 @@ static inline unsigned long dma_max_pfn(struct device
 *dev) }
  #define dma_max_pfn(dev) dma_max_pfn(dev)
 
 -static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
 -   u64 size, struct iommu_ops *iommu,
 -   bool coherent)
 -{
 - if (coherent)
 - set_dma_ops(dev, arm_coherent_dma_ops);
 -}
  #define arch_setup_dma_ops arch_setup_dma_ops
 +extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 +struct iommu_ops *iommu, bool coherent);
 
  static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
 diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
 index c245d903927f..da2c2667bbb1 100644
 --- a/arch/arm/mm/dma-mapping.c
 +++ b/arch/arm/mm/dma-mapping.c
 @@ -1849,7 +1849,8 @@ struct dma_map_ops iommu_coherent_ops = {
   * arm_iommu_attach_device function.
   */
  struct dma_iommu_mapping *
 -arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t
 size) +__arm_iommu_create_mapping(struct iommu_domain *domain, dma_addr_t
 base, +  size_t size)
  {
   unsigned int bits = size  PAGE_SHIFT;
   unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
 @@ -1883,17 +1884,12 @@ arm_iommu_create_mapping(struct bus_type *bus,
 dma_addr_t base, size_t size) mapping-extensions = extensions;
   mapping-base = base;
   mapping-bits = BITS_PER_BYTE * bitmap_size;
 + mapping-domain = domain;
 
   spin_lock_init(mapping-lock);
 
 - mapping-domain = iommu_domain_alloc(bus);
 - if (!mapping-domain)
 - goto err4;
 -
   kref_init(mapping-kref);
   return mapping;
 -err4:
 - kfree(mapping-bitmaps[0]);
  err3:
   kfree(mapping-bitmaps);
  err2:
 @@ -1901,6 +1897,23 @@ err2:
  err:
   return ERR_PTR(err);
  }
 +
 +struct dma_iommu_mapping *
 +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t
 size) +{
 + struct dma_iommu_mapping *mapping;
 + struct iommu_domain *domain;
 +
 + domain = iommu_domain_alloc(bus);
 + if (!domain)
 + return ERR_PTR(-ENOMEM);
 +
 + mapping = __arm_iommu_create_mapping(domain, base, size);
 + if (IS_ERR(mapping))
 + iommu_domain_free(domain);
 +
 + return mapping;
 +}
  EXPORT_SYMBOL_GPL(arm_iommu_create_mapping);
 
  static void release_iommu_mapping(struct kref *kref)
 @@ -1948,9 +1961,8 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
   *   

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-12-15 Thread Will Deacon
On Mon, Dec 15, 2014 at 05:21:16PM +, Laurent Pinchart wrote:
 Hi Will,

Hi Laurent,

 On Wednesday 19 November 2014 11:41:50 Will Deacon wrote:
  +static void __remove_iommu_mapping_entry(struct kref *kref)
  +{
  +   struct dma_iommu_mapping_entry *entry;
  +
  +   entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
  +   list_del(entry-list);
  +}
  +
  +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64
  size,
  +   struct iommu_ops *iommu)
  +{
  +   struct iommu_domain *domain;
  +   struct dma_iommu_mapping_entry *entry = NULL;
  +
  +   if (!iommu-get_default_domain)
  +   return false;
  +
  +   domain = iommu-get_default_domain(dev);
  +   if (!domain)
  +   return false;
  +
  +   spin_lock(dma_iommu_mapping_lock);
  +
  +   list_for_each_entry(entry, dma_iommu_mapping_table, list) {
  +   if (entry-domain == domain)
  +   break;
  +   }
 
 That might be a stupid question (fighting my impostor syndrome again here), 
 but is there a fundamental reason why we can't store the VA allocation data 
 (probably using iova) in the domain instead of having to go through hoops and 
 loops here to associate that data to the domain ?

Well, this was very much a work-in-progress that I ended up abandoning :)

Ultimately, I'd really like to see the default domain management moved into
core code, at which point extending struct iommu_domain seems perfectly
plausible to me (not sure if we'd have a new structure for that).

Then we'd have a fairly clean device - group - domain - allocator
abstraction for each master.

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-12-15 Thread Laurent Pinchart
Hi Will,

On Monday 15 December 2014 17:34:16 Will Deacon wrote:
 On Mon, Dec 15, 2014 at 05:21:16PM +, Laurent Pinchart wrote:
  On Wednesday 19 November 2014 11:41:50 Will Deacon wrote:
   +static void __remove_iommu_mapping_entry(struct kref *kref)
   +{
   + struct dma_iommu_mapping_entry *entry;
   +
   + entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
   + list_del(entry-list);
   +}
   +
   +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base,
   u64
   size,
   + struct iommu_ops *iommu)
   +{
   + struct iommu_domain *domain;
   + struct dma_iommu_mapping_entry *entry = NULL;
   +
   + if (!iommu-get_default_domain)
   + return false;
   +
   + domain = iommu-get_default_domain(dev);
   + if (!domain)
   + return false;
   +
   + spin_lock(dma_iommu_mapping_lock);
   +
   + list_for_each_entry(entry, dma_iommu_mapping_table, list) {
   + if (entry-domain == domain)
   + break;
   + }
  
  That might be a stupid question (fighting my impostor syndrome again
  here), but is there a fundamental reason why we can't store the VA
  allocation data (probably using iova) in the domain instead of having to
  go through hoops and loops here to associate that data to the domain ?
 
 Well, this was very much a work-in-progress that I ended up abandoning :)
 
 Ultimately, I'd really like to see the default domain management moved into
 core code, at which point extending struct iommu_domain seems perfectly
 plausible to me (not sure if we'd have a new structure for that).
 
 Then we'd have a fairly clean device - group - domain - allocator
 abstraction for each master.

I like that. Thank you for confirming that some of my questions are sensible 
:-)

-- 
Regards,

Laurent Pinchart

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-28 Thread Will Deacon
On Fri, Nov 28, 2014 at 01:03:36PM +, jroe...@suse.de wrote:
 On Wed, Nov 26, 2014 at 05:47:07PM +, Will Deacon wrote:
  Joerg, would you expect this to go via your tree or via something broader
  like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead?
 
 Hmm, I don't like the idea of storing private data in iommu_ops. But
 given that this is already an improvement we can build on later, here is
 my
 
   Acked-by: Joerg Roedel jroe...@suse.de

Thanks, Joerg! I'll repost this series shortly with the relevant acks.

 To further improve this we should probably introduce a seperate
 iommu-descriptor data-structure later which then describes a single
 hardware iommu device.

Yes, I agree. I'd like to put the pgsize_bitmap in there too, as the page
table series I posted yesterday has to play games to keep the mask
restricted based on the tables in use by any given domain.

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-26 Thread Will Deacon
Hi all,

On Tue, Nov 25, 2014 at 07:35:21AM +, Marek Szyprowski wrote:
 On 2014-11-19 12:41, Will Deacon wrote:
  On Wed, Nov 19, 2014 at 11:21:26AM +, Marek Szyprowski wrote:
  On 2014-11-14 19:56, Will Deacon wrote:
  Hello everybody,
 
  Here is the fourth iteration of the RFC I've previously posted here:
 
  RFCv1: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
  RFCv2: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
  RFCv3: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
 
  Changes since RFCv3 include:
 
  - Drastic simplification of the data structures, so that we no longer
pass around lists of domains. Instead, dma-mapping is expected to
allocate the domain (Joerg talked about adding a get_default_domain
operation to iommu_ops).
 
  - iommu_ops is used to hold the per-instance IOMMU data
 
  - Configuration of DMA segments added to of_dma_configure
 
  All feedback welcome.
  I've rebased my Exynos SYSMMU patches on top of this patchset and it
  works fine,
  You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU)
  integration with DT
  and DMA-mapping subsystem thread.
  I just saw that and it looks great, thanks! FWIW, I'll take the first 3
  patches you have into my series in some shape or another.
 
 It would be great if the iommu integration patches were merged to -next 
 to give
 them a try for a few days. Joerg: do you plan to take those patches to 
 v3.19 or
 do you want to wait more?

I don't know what the plan is to get this series upstream.

Joerg, would you expect this to go via your tree or via something broader
like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead?

I certainly don't see why patches 1-5 couldn't be queued, then we could add
the ARM bits later. I just need to know where to send them!

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-25 Thread Robin Murphy

Hi Will,

On 14/11/14 18:56, Will Deacon wrote:

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

   RFCv1: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
   RFCv2: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
   RFCv3: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

   - Drastic simplification of the data structures, so that we no longer
 pass around lists of domains. Instead, dma-mapping is expected to
 allocate the domain (Joerg talked about adding a get_default_domain
 operation to iommu_ops).

   - iommu_ops is used to hold the per-instance IOMMU data

   - Configuration of DMA segments added to of_dma_configure

All feedback welcome.



This proposal has been really useful in getting IOMMU DMA mapping 
running on arm64, so with the proviso that patch 6 definitely causes 
problems, and patches 7 and 8 ported to arm64 rather than used directly, 
for the series:


Tested-by: Robin Murphy robin.mur...@arm.com

Hopefully it's now stable enough that I can finish cleaning up my 
hacked-up SMMU driver (oh, the rebasing pain :P)


Thanks,
Robin.


Cheers,

Will

---8

Will Deacon (8):
   iommu: provide early initialisation hook for IOMMU drivers
   dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
   iommu: add new iommu_ops callback for adding an OF device
   iommu: provide helper function to configure an IOMMU for an of master
   dma-mapping: detect and configure IOMMU in of_dma_configure
   dma-mapping: set dma segment properties in of_dma_configure
   arm: call iommu_init before of_platform_populate
   arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

  arch/arm/include/asm/dma-mapping.h | 12 +++---
  arch/arm/kernel/setup.c|  2 +
  arch/arm/mm/dma-mapping.c  | 80 ++
  drivers/iommu/Kconfig  |  2 +-
  drivers/iommu/of_iommu.c   | 50 
  drivers/of/platform.c  | 54 +
  include/asm-generic/vmlinux.lds.h  |  2 +
  include/linux/amba/bus.h   |  1 +
  include/linux/dma-mapping.h| 13 ---
  include/linux/iommu.h  |  8 
  include/linux/of_iommu.h   | 31 +++
  include/linux/platform_device.h|  2 +
  12 files changed, 214 insertions(+), 43 deletions(-)




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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-24 Thread Marek Szyprowski

Hello,

On 2014-11-19 12:41, Will Deacon wrote:

Hi Marek,

On Wed, Nov 19, 2014 at 11:21:26AM +, Marek Szyprowski wrote:

On 2014-11-14 19:56, Will Deacon wrote:

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

RFCv1: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
RFCv2: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
RFCv3: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

- Drastic simplification of the data structures, so that we no longer
  pass around lists of domains. Instead, dma-mapping is expected to
  allocate the domain (Joerg talked about adding a get_default_domain
  operation to iommu_ops).

- iommu_ops is used to hold the per-instance IOMMU data

- Configuration of DMA segments added to of_dma_configure

All feedback welcome.

I've rebased my Exynos SYSMMU patches on top of this patchset and it
works fine,
You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU)
integration with DT
and DMA-mapping subsystem thread.

I just saw that and it looks great, thanks! FWIW, I'll take the first 3
patches you have into my series in some shape or another.


It would be great if the iommu integration patches were merged to -next 
to give
them a try for a few days. Joerg: do you plan to take those patches to 
v3.19 or

do you want to wait more?


You can add to all your patches:
Acked-by: Marek Szyprowski m.szyprow...@samsung.com

Cheers.


I'm also interested in adding get_default_domain() callback, but I
assume that this
can be done once the basic patchset get merged. Do you plan to work on
it, do you want
me to implement it?

If Joerg isn't working on it already (I don't think he is), then please
do have a go if you have time. You'll probably want to avoid adding devices
with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks)
to the default domain, otherwise you'll run into issues initialising the
iova allocator.

I had a go at getting ARM dma-mapping to use a hypothetical
get_default_domain function, so I've included the diff I ended up with below,
in case it's at all useful.


I will check that soon, but I hope this is not strictly needed to get 
basic iommu

and dma-mapping integration merged.

Best regards
--
Marek Szyprowski, PhD
Samsung RD Institute Poland

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-19 Thread Marek Szyprowski

Hello,

On 2014-11-14 19:56, Will Deacon wrote:

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

   RFCv1: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
   RFCv2: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
   RFCv3: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

   - Drastic simplification of the data structures, so that we no longer
 pass around lists of domains. Instead, dma-mapping is expected to
 allocate the domain (Joerg talked about adding a get_default_domain
 operation to iommu_ops).

   - iommu_ops is used to hold the per-instance IOMMU data

   - Configuration of DMA segments added to of_dma_configure

All feedback welcome.


I've rebased my Exynos SYSMMU patches on top of this patchset and it 
works fine,
You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU) 
integration with DT

and DMA-mapping subsystem thread.

You can add to all your patches:
Acked-by: Marek Szyprowski m.szyprow...@samsung.com

I'm also interested in adding get_default_domain() callback, but I 
assume that this
can be done once the basic patchset get merged. Do you plan to work on 
it, do you want

me to implement it?


Cheers,

Will

---8

Will Deacon (8):
   iommu: provide early initialisation hook for IOMMU drivers
   dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
   iommu: add new iommu_ops callback for adding an OF device
   iommu: provide helper function to configure an IOMMU for an of master
   dma-mapping: detect and configure IOMMU in of_dma_configure
   dma-mapping: set dma segment properties in of_dma_configure
   arm: call iommu_init before of_platform_populate
   arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

  arch/arm/include/asm/dma-mapping.h | 12 +++---
  arch/arm/kernel/setup.c|  2 +
  arch/arm/mm/dma-mapping.c  | 80 ++
  drivers/iommu/Kconfig  |  2 +-
  drivers/iommu/of_iommu.c   | 50 
  drivers/of/platform.c  | 54 +
  include/asm-generic/vmlinux.lds.h  |  2 +
  include/linux/amba/bus.h   |  1 +
  include/linux/dma-mapping.h| 13 ---
  include/linux/iommu.h  |  8 
  include/linux/of_iommu.h   | 31 +++
  include/linux/platform_device.h|  2 +
  12 files changed, 214 insertions(+), 43 deletions(-)



Best regards
--
Marek Szyprowski, PhD
Samsung RD Institute Poland

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-19 Thread Will Deacon
Hi Marek,

On Wed, Nov 19, 2014 at 11:21:26AM +, Marek Szyprowski wrote:
 On 2014-11-14 19:56, Will Deacon wrote:
  Hello everybody,
 
  Here is the fourth iteration of the RFC I've previously posted here:
 
 RFCv1: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
 RFCv2: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
 RFCv3: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
 
  Changes since RFCv3 include:
 
 - Drastic simplification of the data structures, so that we no longer
   pass around lists of domains. Instead, dma-mapping is expected to
   allocate the domain (Joerg talked about adding a get_default_domain
   operation to iommu_ops).
 
 - iommu_ops is used to hold the per-instance IOMMU data
 
 - Configuration of DMA segments added to of_dma_configure
 
  All feedback welcome.
 
 I've rebased my Exynos SYSMMU patches on top of this patchset and it 
 works fine,
 You can find them in the [PATCH v3 00/19] Exynos SYSMMU (IOMMU) 
 integration with DT
 and DMA-mapping subsystem thread.

I just saw that and it looks great, thanks! FWIW, I'll take the first 3
patches you have into my series in some shape or another.

 You can add to all your patches:
 Acked-by: Marek Szyprowski m.szyprow...@samsung.com

Cheers.

 I'm also interested in adding get_default_domain() callback, but I 
 assume that this
 can be done once the basic patchset get merged. Do you plan to work on 
 it, do you want
 me to implement it?

If Joerg isn't working on it already (I don't think he is), then please
do have a go if you have time. You'll probably want to avoid adding devices
with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks)
to the default domain, otherwise you'll run into issues initialising the
iova allocator.

I had a go at getting ARM dma-mapping to use a hypothetical
get_default_domain function, so I've included the diff I ended up with below,
in case it's at all useful.

Will

---8

diff --git a/arch/arm/include/asm/dma-mapping.h 
b/arch/arm/include/asm/dma-mapping.h
index f3c0d953f6a2..5071553bf6b8 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -121,14 +121,9 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
- u64 size, struct iommu_ops *iommu,
- bool coherent)
-{
-   if (coherent)
-   set_dma_ops(dev, arm_coherent_dma_ops);
-}
 #define arch_setup_dma_ops arch_setup_dma_ops
+extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+  struct iommu_ops *iommu, bool coherent);
 
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c245d903927f..da2c2667bbb1 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1849,7 +1849,8 @@ struct dma_map_ops iommu_coherent_ops = {
  * arm_iommu_attach_device function.
  */
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
+__arm_iommu_create_mapping(struct iommu_domain *domain, dma_addr_t base,
+  size_t size)
 {
unsigned int bits = size  PAGE_SHIFT;
unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);
@@ -1883,17 +1884,12 @@ arm_iommu_create_mapping(struct bus_type *bus, 
dma_addr_t base, size_t size)
mapping-extensions = extensions;
mapping-base = base;
mapping-bits = BITS_PER_BYTE * bitmap_size;
+   mapping-domain = domain;
 
spin_lock_init(mapping-lock);
 
-   mapping-domain = iommu_domain_alloc(bus);
-   if (!mapping-domain)
-   goto err4;
-
kref_init(mapping-kref);
return mapping;
-err4:
-   kfree(mapping-bitmaps[0]);
 err3:
kfree(mapping-bitmaps);
 err2:
@@ -1901,6 +1897,23 @@ err2:
 err:
return ERR_PTR(err);
 }
+
+struct dma_iommu_mapping *
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
+{
+   struct dma_iommu_mapping *mapping;
+   struct iommu_domain *domain;
+
+   domain = iommu_domain_alloc(bus);
+   if (!domain)
+   return ERR_PTR(-ENOMEM);
+
+   mapping = __arm_iommu_create_mapping(domain, base, size);
+   if (IS_ERR(mapping))
+   iommu_domain_free(domain);
+
+   return mapping;
+}
 EXPORT_SYMBOL_GPL(arm_iommu_create_mapping);
 
 static void release_iommu_mapping(struct kref *kref)
@@ -1948,9 +1961,8 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
  * arm_iommu_create_mapping)
  *
  * Attaches specified io address space mapping to the provided device,
- * this replaces the dma operations 

Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-14 Thread Arnd Bergmann
On Friday 14 November 2014 18:56:29 Will Deacon wrote:
 
 Here is the fourth iteration of the RFC I've previously posted here:
 
   RFCv1: 
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
   RFCv2: 
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
   RFCv3: 
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
 
 Changes since RFCv3 include:
 
   - Drastic simplification of the data structures, so that we no longer
 pass around lists of domains. Instead, dma-mapping is expected to
 allocate the domain (Joerg talked about adding a get_default_domain
 operation to iommu_ops).
 
   - iommu_ops is used to hold the per-instance IOMMU data
 
   - Configuration of DMA segments added to of_dma_configure
 
 All feedback welcome.
 
 

Overall I think this is really nice, and I don't mind this going in,
I only have one issue with they way you use iommu_ops now:

At the moment, iommu_ops is a structure that can get used for any
number of iommus of the same type, but by putting per-device private
data into the same structure you have to duplicate it per instance.

I think rather than adding a .priv pointer to iommu_ops, we should do
the same thing that a lot of other subsystems have:

/* generic structure */
struct iommu {
struct iommu_ops *ops;
/* possibly other generic per-instance members */
};

/* driver specific structure */
struct arm_smmu {
struct iommu iommu;

/* smmu specific members */
};
static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu)
{
return container_of(iommu, struct arm_smmu, iommu);
}

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-14 Thread Will Deacon
Hi Arnd,

Thanks for having a look.

On Fri, Nov 14, 2014 at 07:11:23PM +, Arnd Bergmann wrote:
 On Friday 14 November 2014 18:56:29 Will Deacon wrote:
  
  Here is the fourth iteration of the RFC I've previously posted here:
  
RFCv1: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
RFCv2: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
RFCv3: 
  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html
  
  Changes since RFCv3 include:
  
- Drastic simplification of the data structures, so that we no longer
  pass around lists of domains. Instead, dma-mapping is expected to
  allocate the domain (Joerg talked about adding a get_default_domain
  operation to iommu_ops).
  
- iommu_ops is used to hold the per-instance IOMMU data
  
- Configuration of DMA segments added to of_dma_configure
  
  All feedback welcome.
  
  
 
 Overall I think this is really nice, and I don't mind this going in,
 I only have one issue with they way you use iommu_ops now:

Hehe, I thought you might have something to say about that. I also had second
thoughts, but decided it wasn't worse than what we already have (more below).

 At the moment, iommu_ops is a structure that can get used for any
 number of iommus of the same type, but by putting per-device private
 data into the same structure you have to duplicate it per instance.

I'm not sure I agree -- the pgsize_bitmap, for example, could vary between
different implementations of the same IOMMU. I think we already have this in
Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

 I think rather than adding a .priv pointer to iommu_ops, we should do
 the same thing that a lot of other subsystems have:
 
 /* generic structure */
 struct iommu {
   struct iommu_ops *ops;
   /* possibly other generic per-instance members */
 };
 
 /* driver specific structure */
 struct arm_smmu {
   struct iommu iommu;
 
   /* smmu specific members */
 };
 static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu)
 {
   return container_of(iommu, struct arm_smmu, iommu);
 }

Regardless of the arguments above, I think this layout is cleaner. We could
also move the pgsize_bitmap into struct iommu in that case, however, that
would be a more invasive patch series than I what I currently have.

If I do another version of the patch, I can easily add a struct iommu and
stash that in the device_node data for the IOMMU instead of directly
putting the ops there. That's at least a step in the right direction.

Cheers,

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


Re: [RFC PATCH v4 0/8] Introduce automatic DMA configuration for IOMMU masters

2014-11-14 Thread Arnd Bergmann
On Friday 14 November 2014 19:27:54 Will Deacon wrote:
 
  At the moment, iommu_ops is a structure that can get used for any
  number of iommus of the same type, but by putting per-device private
  data into the same structure you have to duplicate it per instance.
 
 I'm not sure I agree -- the pgsize_bitmap, for example, could vary between
 different implementations of the same IOMMU. I think we already have this in
 Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course,
not in iommu_ops.

  I think rather than adding a .priv pointer to iommu_ops, we should do
  the same thing that a lot of other subsystems have:
  
  /* generic structure */
  struct iommu {
struct iommu_ops *ops;
/* possibly other generic per-instance members */
  };
  
  /* driver specific structure */
  struct arm_smmu {
struct iommu iommu;
  
/* smmu specific members */
  };
  static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu)
  {
return container_of(iommu, struct arm_smmu, iommu);
  }
 
 Regardless of the arguments above, I think this layout is cleaner. We could
 also move the pgsize_bitmap into struct iommu in that case, however, that
 would be a more invasive patch series than I what I currently have.

Right, it can be done as a follow-up. It's certainly not urgent.
 
 If I do another version of the patch, I can easily add a struct iommu and
 stash that in the device_node data for the IOMMU instead of directly
 putting the ops there. That's at least a step in the right direction.

Sounds good, yes. Alternatively, why not do the pointer in the opposite
direction and put a 'struct device_node *dn' and a list_head into
'struct iommu'. This means you will have to traverse the list of iommus
in of_iommu_get_ops, but I think it's safe to assume this is a short
list and it gets walked rarely. It would be somewhat cleaner not to have
to use device_node-data this way.

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