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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>