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

[laurent.pinchart@ideasonboard.com (Laurent Pinchart)]

Hi Will,

On Wednesday 19 November 2014 11:41:50 Will Deacon wrote:
> On Wed, Nov 19, 2014 at 11:21:26AM +0000, 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.szyprowski@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 (dma_map_ops pointer) with the
> - * IOMMU aware version. More than one client might be attached to
> - * the same io address space mapping.
> + * More than one client might be attached to the same io address space
> + * mapping.
>   */
>  int arm_iommu_attach_device(struct device *dev,
>  			    struct dma_iommu_mapping *mapping)
> @@ -1963,7 +1975,6 @@ int arm_iommu_attach_device(struct device *dev,
> 
>  	kref_get(&mapping->kref);
>  	dev->archdata.mapping = mapping;
> -	set_dma_ops(dev, &iommu_ops);
> 
>  	pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev));
>  	return 0;
> @@ -1975,7 +1986,6 @@ EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
>   * @dev: valid struct device pointer
>   *
>   * Detaches the provided device from a previously attached map.
> - * This voids the dma operations (dma_map_ops pointer)
>   */
>  void arm_iommu_detach_device(struct device *dev)
>  {
> @@ -1990,10 +2000,141 @@ void arm_iommu_detach_device(struct device *dev)
>  	iommu_detach_device(mapping->domain, dev);
>  	kref_put(&mapping->kref, release_iommu_mapping);
>  	dev->archdata.mapping = NULL;
> -	set_dma_ops(dev, NULL);
> 
>  	pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev));
>  }
>  EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
> 
> -#endif
> +static struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
> +{
> +	return coherent ? &iommu_coherent_ops : &iommu_ops;
> +}
> +
> +struct dma_iommu_mapping_entry {
> +	struct list_head		list;
> +	struct dma_iommu_mapping	*mapping;
> +	struct iommu_domain		*domain;
> +	u64				dma_base;
> +	u64				size;
> +	struct kref			kref;
> +};
> +
> +static DEFINE_SPINLOCK(dma_iommu_mapping_lock);
> +static LIST_HEAD(dma_iommu_mapping_table);
> +
> +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 ?

> +	/* Load entry->mapping after entry  -- not strictly necessary for ARM */
> +	smp_read_barrier_depends();
> +
> +	if (!entry) {
> +		struct dma_iommu_mapping *mapping;
> +
> +		entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
> +		if (!entry)
> +			goto err_unlock;
> +
> +		entry->domain	= domain;
> +		entry->dma_base	= dma_base;
> +		entry->size	= size;
> +		kref_init(&entry->kref);
> +		list_add(&entry->list, &dma_iommu_mapping_table);
> +		spin_unlock(&dma_iommu_mapping_lock);
> +
> +		mapping = __arm_iommu_create_mapping(domain, dma_base, size);
> +		if (!IS_ERR(mapping))
> +			return false;
> +
> +		smp_wmb();
> +		entry->mapping = mapping;
> +	} else if (entry->mapping) {
> +		if (entry->dma_base > dma_base || entry->size > size)
> +			goto err_unlock;
> +
> +		kref_get(&entry->kref);
> +		spin_unlock(&dma_iommu_mapping_lock);
> +	} else {
> +		/* Racing on the same IOMMU */
> +		goto err_unlock;
> +	}
> +
> +	if (arm_iommu_attach_device(dev, entry->mapping)) {
> +		int entry_dead;
> +
> +		pr_warn("Failed to attached device %s to IOMMU mapping\n",
> +				dev_name(dev));
> +		spin_lock(&dma_iommu_mapping_lock);
> +		entry_dead = kref_put(&entry->kref,
> +				      __remove_iommu_mapping_entry);
> +		spin_unlock(&dma_iommu_mapping_lock);
> +
> +		if (entry_dead) {
> +			entry->mapping->domain = NULL;
> +			arm_iommu_release_mapping(entry->mapping);
> +		}
> +
> +		return false;
> +	}
> +
> +	return true;
> +
> +err_unlock:
> +	spin_unlock(&dma_iommu_mapping_lock);
> +	return false;
> +}
> +
> +#else
> +
> +static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64
> size, +				    struct iommu_ops *iommu)
> +{
> +	return false;
> +}
> +
> +#define arm_get_iommu_dma_map_ops arm_get_dma_map_ops
> +
> +#endif	/* CONFIG_ARM_DMA_USE_IOMMU */
> +
> +static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
> +{
> +	return coherent ? &arm_coherent_dma_ops : &arm_dma_ops;
> +}
> +
> +void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> +			struct iommu_ops *iommu, bool coherent)
> +{
> +	struct dma_map_ops *dma_ops;
> +
> +	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
> +		dma_ops = arm_get_iommu_dma_map_ops(coherent);
> +	else
> +		dma_ops = arm_get_dma_map_ops(coherent);
> +
> +	set_dma_ops(dev, dma_ops);
> +}

-- 
Regards,

Laurent Pinchart