RE: [Linaro-mm-sig] [PATCH 1/2] ARM: initial proof-of-concept IOMMU mapper for DMA-mapping

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hello,

On Thursday, September 08, 2011 6:42 PM Laura Abbott wrote:

> Hi, a few comments
> On Fri, September 2, 2011 6:56 am, Marek Szyprowski wrote:
> ...
> > +
> > +struct dma_iommu_mapping {
> > +	/* iommu specific data */
> > +	struct iommu_domain	*domain;
> > +
> > +	void			*bitmap;
> 
> In the earlier version of this patch you had this as a genpool instead of
> just doing the bitmaps manually. Is there a reason genpool can't be used
> to get the iova addresses?

IMHO genpool was a bit overkill in this case and required some additional
patches for aligned allocations. In the next version I also want to extend
this bitmap based allocator to dynamically resize the bitmap for more than
one page if the io address space gets exhausted. 

> > +	size_t			bits;
> > +	unsigned int		order;
> > +	dma_addr_t		base;
> > +
> > +	struct mutex		lock;
> > +};
> <snip>
> > +int arm_iommu_attach_device(struct device *dev, dma_addr_t base, size_t
> > size, int order)
> > +{
> > +	unsigned int count = (size >> PAGE_SHIFT) - order;
> > +	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
> > +	struct dma_iommu_mapping *mapping;
> > +	int err = -ENOMEM;
> > +
> > +	mapping = kzalloc(sizeof(struct dma_iommu_mapping), GFP_KERNEL);
> > +	if (!mapping)
> > +		goto err;
> > +
> > +	mapping->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > +	if (!mapping->bitmap)
> > +		goto err2;
> > +
> > +	mapping->base = base;
> > +	mapping->bits = bitmap_size;
> > +	mapping->order = order;
> > +	mutex_init(&mapping->lock);
> > +
> > +	mapping->domain = iommu_domain_alloc();
> > +	if (!mapping->domain)
> > +		goto err3;
> > +
> > +	err = iommu_attach_device(mapping->domain, dev);
> > +	if (err != 0)
> > +		goto err4;
> > +
> > +	dev->archdata.mapping = mapping;
> > +	set_dma_ops(dev, &iommu_ops);
> > +
> > +	printk(KERN_INFO "Attached IOMMU controller to %s device.\n",
> > dev_name(dev));
> > +	return 0;
> > +
> > +err4:
> > +	iommu_domain_free(mapping->domain);
> > +err3:
> > +	kfree(mapping->bitmap);
> > +err2:
> > +	kfree(mapping);
> > +err:
> > +	return -ENOMEM;
> > +}
> > +EXPORT_SYMBOL(arm_iommu_attach_device);
> > +
> > +#endif

> Attach makes the assumption that each iommu device will exist in a
> separate domain. What if multiple devices want to use the same iommu
> domain? The msm iommu implementation has many different iommu devices but
> many of these will need the same buffer to be mapped in each context so
> currently many devices share the same domain. Without this, the same map
> call would need to happen for each device, which creates extra map calls
> and overhead.

Ah, right. I forgot about the case when devices need to share one domain.
Moving iommu_domain_alloc out of arm_iommu_attach_device and giving that
function just a pointer to the iommu domain should solve this issue. I will
change this in the next version of the patches.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center