First stab at iommu consolidation:
- Migrate OMAP's iommu driver to the generic iommu API. With this in hand,
users can now start using the generic iommu layer instead of calling
omap-specific iommu API.
New code that requires functionality missing from the generic iommu api,
will add that functionality in the generic framework (e.g. adding framework
awareness to multi page sizes, supported by the underlying hardware, will
avoid the otherwise-inevitable code duplication when mapping a memory
region).
OMAP-specific api that is still exposed in the omap iommu driver can
now be either moved to the generic iommu framework, or just removed (if not
used).
This api (and other omap-specific primitives like struct iommu) needs to
be omapified (i.e. renamed to include an 'omap_' prefix). At this early
point of this patch set this is too much churn though, so I'll do that
in the following iteration, after (and if), the general direction is
accepted.
- Migrate OMAP's iovmm (virtual memory manager) driver to the generic
iommu API. With this in hand, iovmm no longer uses omap-specific api
for mapping/unmapping operations. Nevertheless, iovmm is still coupled
with omap's iommu even with this change: it assumes omap page sizes,
and it uses omap's iommu objects to maintain its internal state.
Further generalizing of iovmm strongly depends on our broader plans for
providing a generic virtual memory manager and allocation framework
(which, as discussed, should be separated from a specific mapper).
iovmm has a mainline user: omap3isp, and therefore must be maintained,
but new potential users will either have to generalize it, or come up
with a different generic framework that will replace it.
- Migrate OMAP's iommu mainline user, omap3isp, to the generic API as well
(so it doesn't break). As with iovmm, omap3isp still depends on
omap's iommu, mainly because iovmm depends on it, but also for
iommu context saving and restoring.
It is definitely desirable to completely remove omap3isp's dependency
on the omap-specific iommu layer, and that will be possible as the
required functionality will be added to generic framework.
- Create a dedicated iommu drivers folder (and put the base iommu code there)
- Move OMAP's and MSM's iommu drivers to that drivers iommu folder
Putting all iommu drivers together will ease finding similarities
between different platforms, with the intention of solving problems once,
in a generic framework which everyone can use.
I've only moved the omap and msm implementations for now, to demonstrate
the idea (and support the ARM diet :), but if this is found desirable,
we can bring in intel-iommu.c and amd_iommu.c as well.
Meta:
- This patch set is not bisectable; it was splitted (and ordered) this way
to ease its review. Later iterations of this patch set will fix that
(most likely by squashing the first three patches)
- Based on and tested with 3.0-rc1
- OMAP's iommu code was tested on both OMAP3 and OMAP4
- omap3isp code was tested with a sensor-less OMAP3 (memory-to-memory only)
(thanks Laurent Pinchart for showing me the magic needed to test omap3isp :)
- MSM code was only compile tested
Ohad Ben-Cohen (6):
omap: iommu: generic iommu api migration
omap: iovmm: generic iommu api migration
media: omap3isp: generic iommu api migration
drivers: iommu: move to a dedicated folder
omap: iommu/iovmm: move to dedicated iommu folder
msm: iommu: move to dedicated iommu drivers folder
arch/arm/mach-msm/Kconfig | 15 -
arch/arm/mach-msm/Makefile | 2 +-
arch/arm/plat-omap/Kconfig | 12 -
arch/arm/plat-omap/Makefile | 2 -
arch/arm/plat-omap/include/plat/iommu.h | 3 +-
arch/arm/plat-omap/{ => include/plat}/iopgtable.h | 18 ++
arch/arm/plat-omap/include/plat/iovmm.h | 27 +-
arch/x86/Kconfig | 5 +-
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/base/Makefile | 1 -
drivers/iommu/Kconfig | 32 +++
drivers/iommu/Makefile | 5 +
drivers/{base => iommu}/iommu.c | 0
.../mach-msm/iommu.c => drivers/iommu/msm-iommu.c | 0
.../iommu/omap-iommu-debug.c | 2 +-
.../iommu.c => drivers/iommu/omap-iommu.c | 290 +++++++++++++++++---
.../iovmm.c => drivers/iommu/omap-iovmm.c | 113 +++++---
drivers/media/video/Kconfig | 2 +-
drivers/media/video/omap3isp/isp.c | 41 +++-
drivers/media/video/omap3isp/isp.h | 3 +
drivers/media/video/omap3isp/ispccdc.c | 16 +-
drivers/media/video/omap3isp/ispstat.c | 6 +-
drivers/media/video/omap3isp/ispvideo.c | 4 +-
24 files changed, 451 insertions(+), 151 deletions(-)
rename arch/arm/plat-omap/{ => include/plat}/iopgtable.h (84%)
create mode 100644 drivers/iommu/Kconfig
create mode 100644 drivers/iommu/Makefile
rename drivers/{base => iommu}/iommu.c (100%)
rename arch/arm/mach-msm/iommu.c => drivers/iommu/msm-iommu.c (100%)
rename arch/arm/plat-omap/iommu-debug.c => drivers/iommu/omap-iommu-debug.c (99%)
rename arch/arm/plat-omap/iommu.c => drivers/iommu/omap-iommu.c (77%)
rename arch/arm/plat-omap/iovmm.c => drivers/iommu/omap-iovmm.c (85%)
Migrate OMAP's iommu to the generic iommu api, so users can stay
generic, and non-omap-specific code can be removed and eventually
consolidated into a generic framework.
Tested on both OMAP3 and OMAP4.
Signed-off-by: Ohad Ben-Cohen <[email protected]>
---
arch/arm/plat-omap/Kconfig | 7 +-
arch/arm/plat-omap/include/plat/iommu.h | 3 +-
arch/arm/plat-omap/iommu.c | 288 +++++++++++++++++++++++++++----
arch/arm/plat-omap/iopgtable.h | 18 ++
4 files changed, 278 insertions(+), 38 deletions(-)
diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index 49a4c75..1c3acb5 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -131,8 +131,13 @@ config OMAP_MBOX_KFIFO_SIZE
This can also be changed at runtime (via the mbox_kfifo_size
module parameter).
+config IOMMU_API
+ bool
+
+#can't be tristate; iommu api doesn't support un-registration
config OMAP_IOMMU
- tristate
+ bool
+ select IOMMU_API
config OMAP_IOMMU_DEBUG
tristate "Export OMAP IOMMU internals in DebugFS"
diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
index 174f1b9..db1c492 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -167,8 +167,6 @@ extern void iopgtable_lookup_entry(struct iommu *obj, u32 da, u32 **ppgd,
extern size_t iopgtable_clear_entry(struct iommu *obj, u32 iova);
extern int iommu_set_da_range(struct iommu *obj, u32 start, u32 end);
-extern struct iommu *iommu_get(const char *name);
-extern void iommu_put(struct iommu *obj);
extern int iommu_set_isr(const char *name,
int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
void *priv),
@@ -185,5 +183,6 @@ extern int foreach_iommu_device(void *data,
extern ssize_t iommu_dump_ctx(struct iommu *obj, char *buf, ssize_t len);
extern size_t dump_tlb_entries(struct iommu *obj, char *buf, ssize_t len);
+struct device *omap_find_iommu_device(const char *name);
#endif /* __MACH_IOMMU_H */
diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index 34fc31e..f06e99c 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -18,6 +18,8 @@
#include <linux/ioport.h>
#include <linux/clk.h>
#include <linux/platform_device.h>
+#include <linux/iommu.h>
+#include <linux/mutex.h>
#include <asm/cacheflush.h>
@@ -30,6 +32,19 @@
(__i < (n)) && (cr = __iotlb_read_cr((obj), __i), true); \
__i++)
+/**
+ * struct omap_iommu_domain - omap iommu domain
+ * @pgtable: the page table
+ * @iommu_dev: an omap iommu device attached to this domain. only a single
+ * iommu device can be attached for now.
+ * @lock: domain lock, should be taken when attaching/detaching
+ */
+struct omap_iommu_domain {
+ u32 *pgtable;
+ struct iommu *iommu_dev;
+ struct mutex lock;
+};
+
/* accommodate the difference between omap1 and omap2/3 */
static const struct iommu_functions *arch_iommu;
@@ -852,31 +867,50 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 end)
EXPORT_SYMBOL_GPL(iommu_set_da_range);
/**
- * iommu_get - Get iommu handler
- * @name: target iommu name
+ * omap_find_iommu_device() - find an omap iommu device by name
+ * @name: name of the iommu device
+ *
+ * The generic iommu API requires the caller to provide the device
+ * he wishes to attach to a certain iommu domain. Users of that API
+ * may look up the device using PCI credentials when relevent, and when
+ * not, this helper should be used to find a specific iommu device by name.
+ *
+ * This may be relevant to other platforms as well (msm ?) so consider
+ * moving this to the generic iommu framework.
+ */
+struct device *omap_find_iommu_device(const char *name)
+{
+ return driver_find_device(&omap_iommu_driver.driver, NULL,
+ (void *)name,
+ device_match_by_alias);
+}
+EXPORT_SYMBOL_GPL(omap_find_iommu_device);
+
+/**
+ * omap_iommu_attach() - attach iommu device to an iommu domain
+ * @dev: target omap iommu device
+ * @iopgd: page table
**/
-struct iommu *iommu_get(const char *name)
+static struct iommu *omap_iommu_attach(struct device *dev, u32 *iopgd)
{
int err = -ENOMEM;
- struct device *dev;
- struct iommu *obj;
-
- dev = driver_find_device(&omap_iommu_driver.driver, NULL, (void *)name,
- device_match_by_alias);
- if (!dev)
- return ERR_PTR(-ENODEV);
-
- obj = to_iommu(dev);
+ struct iommu *obj = to_iommu(dev);
mutex_lock(&obj->iommu_lock);
- if (obj->refcount++ == 0) {
- err = iommu_enable(obj);
- if (err)
- goto err_enable;
- flush_iotlb_all(obj);
+ /* an iommu device can only be attached once */
+ if (++obj->refcount > 1) {
+ dev_err(dev, "%s: already attached!\n", obj->name);
+ err = -EBUSY;
+ goto err_enable;
}
+ obj->iopgd = iopgd;
+ err = iommu_enable(obj);
+ if (err)
+ goto err_enable;
+ flush_iotlb_all(obj);
+
if (!try_module_get(obj->owner))
goto err_module;
@@ -893,13 +927,12 @@ err_enable:
mutex_unlock(&obj->iommu_lock);
return ERR_PTR(err);
}
-EXPORT_SYMBOL_GPL(iommu_get);
/**
- * iommu_put - Put back iommu handler
+ * omap_iommu_detach - release iommu device
* @obj: target iommu
**/
-void iommu_put(struct iommu *obj)
+static void omap_iommu_detach(struct iommu *obj)
{
if (!obj || IS_ERR(obj))
return;
@@ -911,11 +944,12 @@ void iommu_put(struct iommu *obj)
module_put(obj->owner);
+ obj->iopgd = NULL;
+
mutex_unlock(&obj->iommu_lock);
dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name);
}
-EXPORT_SYMBOL_GPL(iommu_put);
int iommu_set_isr(const char *name,
int (*isr)(struct iommu *obj, u32 da, u32 iommu_errs,
@@ -950,7 +984,6 @@ EXPORT_SYMBOL_GPL(iommu_set_isr);
static int __devinit omap_iommu_probe(struct platform_device *pdev)
{
int err = -ENODEV;
- void *p;
int irq;
struct iommu *obj;
struct resource *res;
@@ -1009,22 +1042,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
goto err_irq;
platform_set_drvdata(pdev, obj);
- p = (void *)__get_free_pages(GFP_KERNEL, get_order(IOPGD_TABLE_SIZE));
- if (!p) {
- err = -ENOMEM;
- goto err_pgd;
- }
- memset(p, 0, IOPGD_TABLE_SIZE);
- clean_dcache_area(p, IOPGD_TABLE_SIZE);
- obj->iopgd = p;
-
- BUG_ON(!IS_ALIGNED((unsigned long)obj->iopgd, IOPGD_TABLE_SIZE));
-
dev_info(&pdev->dev, "%s registered\n", obj->name);
return 0;
-err_pgd:
- free_irq(irq, obj);
err_irq:
iounmap(obj->regbase);
err_ioremap:
@@ -1072,6 +1092,202 @@ static void iopte_cachep_ctor(void *iopte)
clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
}
+static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
+ phys_addr_t pa, int order, int prot)
+{
+ struct omap_iommu_domain *omap_domain = domain->priv;
+ struct iommu *oiommu = omap_domain->iommu_dev;
+ struct device *dev = oiommu->dev;
+ size_t bytes = PAGE_SIZE << order;
+ struct iotlb_entry e;
+ int omap_pgsz;
+ u32 ret, flags;
+
+ /* we only support mapping a single iommu page for now */
+ omap_pgsz = bytes_to_iopgsz(bytes);
+ if (omap_pgsz < 0) {
+ dev_err(dev, "invalid size to map: %d\n", bytes);
+ return -EINVAL;
+ }
+
+ dev_dbg(dev, "mapping da 0x%lx to pa 0x%x size 0x%x\n", da, pa, bytes);
+
+ flags = omap_pgsz | prot;
+
+ iotlb_init_entry(&e, da, pa, flags);
+
+ ret = iopgtable_store_entry(oiommu, &e);
+ if (ret) {
+ dev_err(dev, "iopgtable_store_entry failed: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int omap_iommu_unmap(struct iommu_domain *domain, unsigned long da,
+ int order)
+{
+ struct omap_iommu_domain *omap_domain = domain->priv;
+ struct iommu *oiommu = omap_domain->iommu_dev;
+ struct device *dev = oiommu->dev;
+ size_t bytes = PAGE_SIZE << order;
+ size_t ret;
+
+ dev_dbg(dev, "unmapping da 0x%lx size 0x%x\n", da, bytes);
+
+ ret = iopgtable_clear_entry(oiommu, da);
+ if (ret != bytes) {
+ dev_err(dev, "entry @ 0x%lx was %d; not %d\n", da, ret, bytes);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+omap_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
+{
+ struct omap_iommu_domain *omap_domain = domain->priv;
+ struct iommu *oiommu;
+ int ret = 0;
+
+ mutex_lock(&omap_domain->lock);
+
+ /* only a single device is supported per domain for now */
+ if (omap_domain->iommu_dev) {
+ dev_err(dev, "iommu domain is already attached\n");
+ ret = -EBUSY;
+ goto out;
+ }
+
+ /* get a handle to and enable the omap iommu */
+ oiommu = omap_iommu_attach(dev, omap_domain->pgtable);
+ if (IS_ERR(oiommu)) {
+ ret = PTR_ERR(oiommu);
+ dev_err(dev, "can't get omap iommu: %d\n", ret);
+ goto out;
+ }
+
+ omap_domain->iommu_dev = oiommu;
+
+out:
+ mutex_unlock(&omap_domain->lock);
+ return ret;
+}
+
+static void omap_iommu_detach_dev(struct iommu_domain *domain,
+ struct device *dev)
+{
+ struct omap_iommu_domain *omap_domain = domain->priv;
+ struct iommu *oiommu = to_iommu(dev);
+
+ mutex_lock(&omap_domain->lock);
+
+ /* only a single device is supported per domain for now */
+ if (omap_domain->iommu_dev != oiommu) {
+ dev_err(dev, "invalid iommu device\n");
+ goto out;
+ }
+
+ iopgtable_clear_entry_all(oiommu);
+
+ omap_iommu_detach(oiommu);
+
+ omap_domain->iommu_dev = NULL;
+
+out:
+ mutex_unlock(&omap_domain->lock);
+}
+
+static int omap_iommu_domain_init(struct iommu_domain *domain)
+{
+ struct omap_iommu_domain *omap_domain;
+
+ omap_domain = kzalloc(sizeof(*omap_domain), GFP_KERNEL);
+ if (!omap_domain) {
+ pr_err("kzalloc failed\n");
+ goto fail_nomem;
+ }
+
+ omap_domain->pgtable = (u32 *)__get_free_pages(GFP_KERNEL,
+ get_order(IOPGD_TABLE_SIZE));
+ if (!omap_domain->pgtable) {
+ pr_err("__get_free_pages failed\n");
+ goto fail_nomem;
+ }
+
+ BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));
+ memset(omap_domain->pgtable, 0, IOPGD_TABLE_SIZE);
+ clean_dcache_area(omap_domain->pgtable, IOPGD_TABLE_SIZE);
+ mutex_init(&omap_domain->lock);
+
+ domain->priv = omap_domain;
+
+ return 0;
+
+fail_nomem:
+ kfree(omap_domain);
+ return -ENOMEM;
+}
+
+/* assume device was already detached */
+static void omap_iommu_domain_destroy(struct iommu_domain *domain)
+{
+ struct omap_iommu_domain *omap_domain = domain->priv;
+
+ domain->priv = NULL;
+
+ kfree(omap_domain);
+}
+
+static phys_addr_t omap_iommu_iova_to_phys(struct iommu_domain *domain,
+ unsigned long da)
+{
+ struct omap_iommu_domain *omap_domain = domain->priv;
+ struct iommu *oiommu = omap_domain->iommu_dev;
+ struct device *dev = oiommu->dev;
+ u32 *pgd, *pte;
+ phys_addr_t ret = 0;
+
+ iopgtable_lookup_entry(oiommu, da, &pgd, &pte);
+
+ if (pte) {
+ if (iopte_is_small(*pte))
+ ret = omap_iommu_translate(*pte, da, IOPTE_MASK);
+ else if (iopte_is_large(*pte))
+ ret = omap_iommu_translate(*pte, da, IOLARGE_MASK);
+ else
+ dev_err(dev, "bogus pte 0x%x", *pte);
+ } else {
+ if (iopgd_is_section(*pgd))
+ ret = omap_iommu_translate(*pgd, da, IOSECTION_MASK);
+ else if (iopgd_is_super(*pgd))
+ ret = omap_iommu_translate(*pgd, da, IOSUPER_MASK);
+ else
+ dev_err(dev, "bogus pgd 0x%x", *pgd);
+ }
+
+ return ret;
+}
+
+static int omap_iommu_domain_has_cap(struct iommu_domain *domain,
+ unsigned long cap)
+{
+ return 0;
+}
+
+static struct iommu_ops omap_iommu_ops = {
+ .domain_init = omap_iommu_domain_init,
+ .domain_destroy = omap_iommu_domain_destroy,
+ .attach_dev = omap_iommu_attach_dev,
+ .detach_dev = omap_iommu_detach_dev,
+ .map = omap_iommu_map,
+ .unmap = omap_iommu_unmap,
+ .iova_to_phys = omap_iommu_iova_to_phys,
+ .domain_has_cap = omap_iommu_domain_has_cap,
+};
+
static int __init omap_iommu_init(void)
{
struct kmem_cache *p;
@@ -1084,6 +1300,8 @@ static int __init omap_iommu_init(void)
return -ENOMEM;
iopte_cachep = p;
+ register_iommu(&omap_iommu_ops);
+
return platform_driver_register(&omap_iommu_driver);
}
module_init(omap_iommu_init);
diff --git a/arch/arm/plat-omap/iopgtable.h b/arch/arm/plat-omap/iopgtable.h
index c3e93bb..33c7aa9 100644
--- a/arch/arm/plat-omap/iopgtable.h
+++ b/arch/arm/plat-omap/iopgtable.h
@@ -56,6 +56,19 @@
#define IOPAGE_MASK IOPTE_MASK
+/**
+ * omap_iommu_translate() - va to pa translation
+ * @d: omap iommu descriptor
+ * @va: virtual address
+ * @mask: omap iommu descriptor mask
+ *
+ * va to pa translation
+ */
+static inline phys_addr_t omap_iommu_translate(u32 d, u32 va, u32 mask)
+{
+ return (d & mask) | (va & (~mask));
+}
+
/*
* some descriptor attributes.
*/
@@ -64,10 +77,15 @@
#define IOPGD_SUPER (1 << 18 | 2 << 0)
#define iopgd_is_table(x) (((x) & 3) == IOPGD_TABLE)
+#define iopgd_is_section(x) (((x) & (1 << 18 | 3)) == IOPGD_SECTION)
+#define iopgd_is_super(x) (((x) & (1 << 18 | 3)) == IOPGD_SUPER)
#define IOPTE_SMALL (2 << 0)
#define IOPTE_LARGE (1 << 0)
+#define iopte_is_small(x) (((x) & 2) == IOPTE_SMALL)
+#define iopte_is_large(x) (((x) & 3) == IOPTE_LARGE)
+
/* to find an entry in a page-table-directory */
#define iopgd_index(da) (((da) >> IOPGD_SHIFT) & (PTRS_PER_IOPGD - 1))
#define iopgd_offset(obj, da) ((obj)->iopgd + iopgd_index(da))
--
1.7.1
Migrate omap's iovmm (virtual memory manager) to the generic iommu api.
This brings iovmm a step forward towards being completely non
omap-specific (it's still assuming omap's iommu page sizes, and also
maintaining state inside omap's internal iommu structure, but it no
longer calls omap-specific iommu map/unmap api).
Further generalizing of iovmm (or complete removal) should take place
together with broader plans of providing a generic virtual memory manager
and allocation framework (de-coupled from specific mappers).
Signed-off-by: Ohad Ben-Cohen <[email protected]>
---
arch/arm/plat-omap/include/plat/iovmm.h | 27 +++++---
arch/arm/plat-omap/iovmm.c | 111 ++++++++++++++++++-------------
2 files changed, 82 insertions(+), 56 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/iovmm.h b/arch/arm/plat-omap/include/plat/iovmm.h
index 32a2f6c..56ee2b9 100644
--- a/arch/arm/plat-omap/include/plat/iovmm.h
+++ b/arch/arm/plat-omap/include/plat/iovmm.h
@@ -13,6 +13,8 @@
#ifndef __IOMMU_MMAP_H
#define __IOMMU_MMAP_H
+#include <linux/iommu.h>
+
struct iovm_struct {
struct iommu *iommu; /* iommu object which this belongs to */
u32 da_start; /* area definition */
@@ -74,18 +76,21 @@ struct iovm_struct {
extern struct iovm_struct *find_iovm_area(struct iommu *obj, u32 da);
-extern u32 iommu_vmap(struct iommu *obj, u32 da,
+extern u32 iommu_vmap(struct iommu_domain *domain, struct iommu *obj, u32 da,
const struct sg_table *sgt, u32 flags);
-extern struct sg_table *iommu_vunmap(struct iommu *obj, u32 da);
-extern u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes,
- u32 flags);
-extern void iommu_vfree(struct iommu *obj, const u32 da);
-extern u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
- u32 flags);
-extern void iommu_kunmap(struct iommu *obj, u32 da);
-extern u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes,
- u32 flags);
-extern void iommu_kfree(struct iommu *obj, u32 da);
+extern struct sg_table *iommu_vunmap(struct iommu_domain *domain,
+ struct iommu *obj, u32 da);
+extern u32 iommu_vmalloc(struct iommu_domain *domain, struct iommu *obj,
+ u32 da, size_t bytes, u32 flags);
+extern void iommu_vfree(struct iommu_domain *domain, struct iommu *obj,
+ const u32 da);
+extern u32 iommu_kmap(struct iommu_domain *domain, struct iommu *obj, u32 da,
+ u32 pa, size_t bytes, u32 flags);
+extern void iommu_kunmap(struct iommu_domain *domain, struct iommu *obj,
+ u32 da);
+extern u32 iommu_kmalloc(struct iommu_domain *domain, struct iommu *obj,
+ u32 da, size_t bytes, u32 flags);
+extern void iommu_kfree(struct iommu_domain *domain, struct iommu *obj, u32 da);
extern void *da_to_va(struct iommu *obj, u32 da);
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 51ef43e..80bb2b6 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -15,6 +15,7 @@
#include <linux/vmalloc.h>
#include <linux/device.h>
#include <linux/scatterlist.h>
+#include <linux/iommu.h>
#include <asm/cacheflush.h>
#include <asm/mach/map.h>
@@ -456,15 +457,16 @@ static inline void sgtable_drain_kmalloc(struct sg_table *sgt)
}
/* create 'da' <-> 'pa' mapping from 'sgt' */
-static int map_iovm_area(struct iommu *obj, struct iovm_struct *new,
- const struct sg_table *sgt, u32 flags)
+static int map_iovm_area(struct iommu_domain *domain, struct iovm_struct *new,
+ const struct sg_table *sgt, u32 flags)
{
int err;
unsigned int i, j;
struct scatterlist *sg;
u32 da = new->da_start;
+ int order;
- if (!obj || !sgt)
+ if (!domain || !sgt)
return -EINVAL;
BUG_ON(!sgtable_ok(sgt));
@@ -473,22 +475,22 @@ static int map_iovm_area(struct iommu *obj, struct iovm_struct *new,
u32 pa;
int pgsz;
size_t bytes;
- struct iotlb_entry e;
pa = sg_phys(sg);
bytes = sg_dma_len(sg);
flags &= ~IOVMF_PGSZ_MASK;
+
pgsz = bytes_to_iopgsz(bytes);
if (pgsz < 0)
goto err_out;
- flags |= pgsz;
+
+ order = get_order(bytes);
pr_debug("%s: [%d] %08x %08x(%x)\n", __func__,
i, da, pa, bytes);
- iotlb_init_entry(&e, da, pa, flags);
- err = iopgtable_store_entry(obj, &e);
+ err = iommu_map(domain, da, pa, order, flags);
if (err)
goto err_out;
@@ -502,9 +504,11 @@ err_out:
for_each_sg(sgt->sgl, sg, i, j) {
size_t bytes;
- bytes = iopgtable_clear_entry(obj, da);
+ bytes = sg_dma_len(sg);
+ order = get_order(bytes);
- BUG_ON(!iopgsz_ok(bytes));
+ /* ignore failures.. we're already handling one */
+ iommu_unmap(domain, da, order);
da += bytes;
}
@@ -512,22 +516,31 @@ err_out:
}
/* release 'da' <-> 'pa' mapping */
-static void unmap_iovm_area(struct iommu *obj, struct iovm_struct *area)
+static void unmap_iovm_area(struct iommu_domain *domain, struct iommu *obj,
+ struct iovm_struct *area)
{
u32 start;
size_t total = area->da_end - area->da_start;
+ const struct sg_table *sgt = area->sgt;
+ struct scatterlist *sg;
+ int i, err;
+ BUG_ON(!sgtable_ok(sgt));
BUG_ON((!total) || !IS_ALIGNED(total, PAGE_SIZE));
start = area->da_start;
- while (total > 0) {
+ for_each_sg(sgt->sgl, sg, sgt->nents, i) {
size_t bytes;
+ int order;
+
+ bytes = sg_dma_len(sg);
+ order = get_order(bytes);
+
+ err = iommu_unmap(domain, start, order);
+ if (err)
+ break;
- bytes = iopgtable_clear_entry(obj, start);
- if (bytes == 0)
- bytes = PAGE_SIZE;
- else
- dev_dbg(obj->dev, "%s: unmap %08x(%x) %08x\n",
+ dev_dbg(obj->dev, "%s: unmap %08x(%x) %08x\n",
__func__, start, bytes, area->flags);
BUG_ON(!IS_ALIGNED(bytes, PAGE_SIZE));
@@ -539,7 +552,8 @@ static void unmap_iovm_area(struct iommu *obj, struct iovm_struct *area)
}
/* template function for all unmapping */
-static struct sg_table *unmap_vm_area(struct iommu *obj, const u32 da,
+static struct sg_table *unmap_vm_area(struct iommu_domain *domain,
+ struct iommu *obj, const u32 da,
void (*fn)(const void *), u32 flags)
{
struct sg_table *sgt = NULL;
@@ -565,7 +579,7 @@ static struct sg_table *unmap_vm_area(struct iommu *obj, const u32 da,
}
sgt = (struct sg_table *)area->sgt;
- unmap_iovm_area(obj, area);
+ unmap_iovm_area(domain, obj, area);
fn(area->va);
@@ -580,8 +594,9 @@ out:
return sgt;
}
-static u32 map_iommu_region(struct iommu *obj, u32 da,
- const struct sg_table *sgt, void *va, size_t bytes, u32 flags)
+static u32 map_iommu_region(struct iommu_domain *domain, struct iommu *obj,
+ u32 da, const struct sg_table *sgt, void *va,
+ size_t bytes, u32 flags)
{
int err = -ENOMEM;
struct iovm_struct *new;
@@ -596,7 +611,7 @@ static u32 map_iommu_region(struct iommu *obj, u32 da,
new->va = va;
new->sgt = sgt;
- if (map_iovm_area(obj, new, sgt, new->flags))
+ if (map_iovm_area(domain, new, sgt, new->flags))
goto err_map;
mutex_unlock(&obj->mmap_lock);
@@ -613,10 +628,11 @@ err_alloc_iovma:
return err;
}
-static inline u32 __iommu_vmap(struct iommu *obj, u32 da,
- const struct sg_table *sgt, void *va, size_t bytes, u32 flags)
+static inline u32 __iommu_vmap(struct iommu_domain *domain, struct iommu *obj,
+ u32 da, const struct sg_table *sgt,
+ void *va, size_t bytes, u32 flags)
{
- return map_iommu_region(obj, da, sgt, va, bytes, flags);
+ return map_iommu_region(domain, obj, da, sgt, va, bytes, flags);
}
/**
@@ -628,8 +644,8 @@ static inline u32 __iommu_vmap(struct iommu *obj, u32 da,
* Creates 1-n-1 mapping with given @sgt and returns @da.
* All @sgt element must be io page size aligned.
*/
-u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
- u32 flags)
+u32 iommu_vmap(struct iommu_domain *domain, struct iommu *obj, u32 da,
+ const struct sg_table *sgt, u32 flags)
{
size_t bytes;
void *va = NULL;
@@ -652,7 +668,7 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
flags |= IOVMF_DISCONT;
flags |= IOVMF_MMIO;
- da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
+ da = __iommu_vmap(domain, obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da))
vunmap_sg(va);
@@ -668,14 +684,16 @@ EXPORT_SYMBOL_GPL(iommu_vmap);
* Free the iommu virtually contiguous memory area starting at
* @da, which was returned by 'iommu_vmap()'.
*/
-struct sg_table *iommu_vunmap(struct iommu *obj, u32 da)
+struct sg_table *
+iommu_vunmap(struct iommu_domain *domain, struct iommu *obj, u32 da)
{
struct sg_table *sgt;
/*
* 'sgt' is allocated before 'iommu_vmalloc()' is called.
* Just returns 'sgt' to the caller to free
*/
- sgt = unmap_vm_area(obj, da, vunmap_sg, IOVMF_DISCONT | IOVMF_MMIO);
+ sgt = unmap_vm_area(domain, obj, da, vunmap_sg,
+ IOVMF_DISCONT | IOVMF_MMIO);
if (!sgt)
dev_dbg(obj->dev, "%s: No sgt\n", __func__);
return sgt;
@@ -692,7 +710,8 @@ EXPORT_SYMBOL_GPL(iommu_vunmap);
* Allocate @bytes linearly and creates 1-n-1 mapping and returns
* @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
-u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
+u32 iommu_vmalloc(struct iommu_domain *domain, struct iommu *obj, u32 da,
+ size_t bytes, u32 flags)
{
void *va;
struct sg_table *sgt;
@@ -717,7 +736,7 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
}
sgtable_fill_vmalloc(sgt, va);
- da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
+ da = __iommu_vmap(domain, obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da))
goto err_iommu_vmap;
@@ -740,19 +759,20 @@ EXPORT_SYMBOL_GPL(iommu_vmalloc);
* Frees the iommu virtually continuous memory area starting at
* @da, as obtained from 'iommu_vmalloc()'.
*/
-void iommu_vfree(struct iommu *obj, const u32 da)
+void iommu_vfree(struct iommu_domain *domain, struct iommu *obj, const u32 da)
{
struct sg_table *sgt;
- sgt = unmap_vm_area(obj, da, vfree, IOVMF_DISCONT | IOVMF_ALLOC);
+ sgt = unmap_vm_area(domain, obj, da, vfree,
+ IOVMF_DISCONT | IOVMF_ALLOC);
if (!sgt)
dev_dbg(obj->dev, "%s: No sgt\n", __func__);
sgtable_free(sgt);
}
EXPORT_SYMBOL_GPL(iommu_vfree);
-static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa, void *va,
- size_t bytes, u32 flags)
+static u32 __iommu_kmap(struct iommu_domain *domain, struct iommu *obj,
+ u32 da, u32 pa, void *va, size_t bytes, u32 flags)
{
struct sg_table *sgt;
@@ -762,7 +782,7 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa, void *va,
sgtable_fill_kmalloc(sgt, pa, da, bytes);
- da = map_iommu_region(obj, da, sgt, va, bytes, flags);
+ da = map_iommu_region(domain, obj, da, sgt, va, bytes, flags);
if (IS_ERR_VALUE(da)) {
sgtable_drain_kmalloc(sgt);
sgtable_free(sgt);
@@ -781,8 +801,8 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa, void *va,
* Creates 1-1-1 mapping and returns @da again, which can be
* adjusted if 'IOVMF_DA_FIXED' is not set.
*/
-u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
- u32 flags)
+u32 iommu_kmap(struct iommu_domain *domain, struct iommu *obj, u32 da, u32 pa,
+ size_t bytes, u32 flags)
{
void *va;
@@ -799,7 +819,7 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
flags |= IOVMF_LINEAR;
flags |= IOVMF_MMIO;
- da = __iommu_kmap(obj, da, pa, va, bytes, flags);
+ da = __iommu_kmap(domain, obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
iounmap(va);
@@ -815,12 +835,12 @@ EXPORT_SYMBOL_GPL(iommu_kmap);
* Frees the iommu virtually contiguous memory area starting at
* @da, which was passed to and was returned by'iommu_kmap()'.
*/
-void iommu_kunmap(struct iommu *obj, u32 da)
+void iommu_kunmap(struct iommu_domain *domain, struct iommu *obj, u32 da)
{
struct sg_table *sgt;
typedef void (*func_t)(const void *);
- sgt = unmap_vm_area(obj, da, (func_t)iounmap,
+ sgt = unmap_vm_area(domain, obj, da, (func_t)iounmap,
IOVMF_LINEAR | IOVMF_MMIO);
if (!sgt)
dev_dbg(obj->dev, "%s: No sgt\n", __func__);
@@ -838,7 +858,8 @@ EXPORT_SYMBOL_GPL(iommu_kunmap);
* Allocate @bytes linearly and creates 1-1-1 mapping and returns
* @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
*/
-u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
+u32 iommu_kmalloc(struct iommu_domain *domain, struct iommu *obj, u32 da,
+ size_t bytes, u32 flags)
{
void *va;
u32 pa;
@@ -857,7 +878,7 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
flags |= IOVMF_LINEAR;
flags |= IOVMF_ALLOC;
- da = __iommu_kmap(obj, da, pa, va, bytes, flags);
+ da = __iommu_kmap(domain, obj, da, pa, va, bytes, flags);
if (IS_ERR_VALUE(da))
kfree(va);
@@ -873,11 +894,11 @@ EXPORT_SYMBOL_GPL(iommu_kmalloc);
* Frees the iommu virtually contiguous memory area starting at
* @da, which was passed to and was returned by'iommu_kmalloc()'.
*/
-void iommu_kfree(struct iommu *obj, u32 da)
+void iommu_kfree(struct iommu_domain *domain, struct iommu *obj, u32 da)
{
struct sg_table *sgt;
- sgt = unmap_vm_area(obj, da, kfree, IOVMF_LINEAR | IOVMF_ALLOC);
+ sgt = unmap_vm_area(domain, obj, da, kfree, IOVMF_LINEAR | IOVMF_ALLOC);
if (!sgt)
dev_dbg(obj->dev, "%s: No sgt\n", __func__);
sgtable_free(sgt);
--
1.7.1
First step towards migrating omap3isp to the generic iommu api.
At this point we still need a handle of the omap-specific iommu, mainly
because we highly depend on omap's iovmm.
Migration will be fully completed only once omap's iovmm will be generalized
(or replaced by a generic virtual memory manager framework).
Signed-off-by: Ohad Ben-Cohen <[email protected]>
---
drivers/media/video/omap3isp/isp.c | 41 +++++++++++++++++++++++++-----
drivers/media/video/omap3isp/isp.h | 3 ++
drivers/media/video/omap3isp/ispccdc.c | 16 ++++++------
drivers/media/video/omap3isp/ispstat.c | 6 ++--
drivers/media/video/omap3isp/ispvideo.c | 4 +-
5 files changed, 50 insertions(+), 20 deletions(-)
diff --git a/drivers/media/video/omap3isp/isp.c b/drivers/media/video/omap3isp/isp.c
index 897a1cf..25bade9 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -80,6 +80,13 @@
#include "isph3a.h"
#include "isphist.h"
+/*
+ * this is provided as an interim solution until omap3isp doesn't need
+ * any omap-specific iommu API
+ */
+#define to_iommu(dev) \
+ (struct iommu *)platform_get_drvdata(to_platform_device(dev))
+
static unsigned int autoidle;
module_param(autoidle, int, 0444);
MODULE_PARM_DESC(autoidle, "Enable OMAP3ISP AUTOIDLE support");
@@ -1975,7 +1982,8 @@ static int isp_remove(struct platform_device *pdev)
isp_cleanup_modules(isp);
omap3isp_get(isp);
- iommu_put(isp->iommu);
+ iommu_detach_device(isp->domain, isp->iommu_dev);
+ iommu_domain_free(isp->domain);
omap3isp_put(isp);
free_irq(isp->irq_num, isp);
@@ -2123,25 +2131,41 @@ static int isp_probe(struct platform_device *pdev)
}
/* IOMMU */
- isp->iommu = iommu_get("isp");
- if (IS_ERR_OR_NULL(isp->iommu)) {
- isp->iommu = NULL;
+ isp->iommu_dev = omap_find_iommu_device("isp");
+ if (!isp->iommu_dev) {
+ dev_err(isp->dev, "omap_find_iommu_device failed\n");
ret = -ENODEV;
goto error_isp;
}
+ /* to be removed once iommu migration is complete */
+ isp->iommu = to_iommu(isp->iommu_dev);
+
+ isp->domain = iommu_domain_alloc();
+ if (!isp->domain) {
+ dev_err(isp->dev, "can't alloc iommu domain\n");
+ ret = -ENOMEM;
+ goto error_isp;
+ }
+
+ ret = iommu_attach_device(isp->domain, isp->iommu_dev);
+ if (ret) {
+ dev_err(&pdev->dev, "can't attach iommu device: %d\n", ret);
+ goto free_domain;
+ }
+
/* Interrupt */
isp->irq_num = platform_get_irq(pdev, 0);
if (isp->irq_num <= 0) {
dev_err(isp->dev, "No IRQ resource\n");
ret = -ENODEV;
- goto error_isp;
+ goto detach_dev;
}
if (request_irq(isp->irq_num, isp_isr, IRQF_SHARED, "OMAP3 ISP", isp)) {
dev_err(isp->dev, "Unable to request IRQ\n");
ret = -EINVAL;
- goto error_isp;
+ goto detach_dev;
}
/* Entities */
@@ -2162,8 +2186,11 @@ error_modules:
isp_cleanup_modules(isp);
error_irq:
free_irq(isp->irq_num, isp);
+detach_dev:
+ iommu_detach_device(isp->domain, isp->iommu_dev);
+free_domain:
+ iommu_domain_free(isp->domain);
error_isp:
- iommu_put(isp->iommu);
omap3isp_put(isp);
error:
isp_put_clocks(isp);
diff --git a/drivers/media/video/omap3isp/isp.h b/drivers/media/video/omap3isp/isp.h
index 2620c40..1b54aa4 100644
--- a/drivers/media/video/omap3isp/isp.h
+++ b/drivers/media/video/omap3isp/isp.h
@@ -32,6 +32,7 @@
#include <linux/io.h>
#include <linux/platform_device.h>
#include <linux/wait.h>
+#include <linux/iommu.h>
#include <plat/iommu.h>
#include <plat/iovmm.h>
@@ -289,6 +290,8 @@ struct isp_device {
unsigned int subclk_resources;
struct iommu *iommu;
+ struct iommu_domain *domain;
+ struct device *iommu_dev;
struct isp_platform_callback platform_cb;
};
diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
index 39d501b..b59b06f 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -365,7 +365,7 @@ static void ccdc_lsc_free_request(struct isp_ccdc_device *ccdc,
dma_unmap_sg(isp->dev, req->iovm->sgt->sgl,
req->iovm->sgt->nents, DMA_TO_DEVICE);
if (req->table)
- iommu_vfree(isp->iommu, req->table);
+ iommu_vfree(isp->domain, isp->iommu, req->table);
kfree(req);
}
@@ -437,8 +437,8 @@ static int ccdc_lsc_config(struct isp_ccdc_device *ccdc,
req->enable = 1;
- req->table = iommu_vmalloc(isp->iommu, 0, req->config.size,
- IOMMU_FLAG);
+ req->table = iommu_vmalloc(isp->domain, isp->iommu, 0,
+ req->config.size, IOMMU_FLAG);
if (IS_ERR_VALUE(req->table)) {
req->table = 0;
ret = -ENOMEM;
@@ -733,15 +733,15 @@ static int ccdc_config(struct isp_ccdc_device *ccdc,
* already done by iommu_vmalloc().
*/
size = ccdc->fpc.fpnum * 4;
- table_new = iommu_vmalloc(isp->iommu, 0, size,
- IOMMU_FLAG);
+ table_new = iommu_vmalloc(isp->domain, isp->iommu, 0,
+ size, IOMMU_FLAG);
if (IS_ERR_VALUE(table_new))
return -ENOMEM;
if (copy_from_user(da_to_va(isp->iommu, table_new),
(__force void __user *)
ccdc->fpc.fpcaddr, size)) {
- iommu_vfree(isp->iommu, table_new);
+ iommu_vfree(isp->domain, isp->iommu, table_new);
return -EFAULT;
}
@@ -751,7 +751,7 @@ static int ccdc_config(struct isp_ccdc_device *ccdc,
ccdc_configure_fpc(ccdc);
if (table_old != 0)
- iommu_vfree(isp->iommu, table_old);
+ iommu_vfree(isp->domain, isp->iommu, table_old);
}
return ccdc_lsc_config(ccdc, ccdc_struct);
@@ -2287,5 +2287,5 @@ void omap3isp_ccdc_cleanup(struct isp_device *isp)
ccdc_lsc_free_queue(ccdc, &ccdc->lsc.free_queue);
if (ccdc->fpc.fpcaddr != 0)
- iommu_vfree(isp->iommu, ccdc->fpc.fpcaddr);
+ iommu_vfree(isp->domain, isp->iommu, ccdc->fpc.fpcaddr);
}
diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
index b44cb68..afb7d78 100644
--- a/drivers/media/video/omap3isp/ispstat.c
+++ b/drivers/media/video/omap3isp/ispstat.c
@@ -366,7 +366,7 @@ static void isp_stat_bufs_free(struct ispstat *stat)
dma_unmap_sg(isp->dev, buf->iovm->sgt->sgl,
buf->iovm->sgt->nents,
DMA_FROM_DEVICE);
- iommu_vfree(isp->iommu, buf->iommu_addr);
+ iommu_vfree(isp->domain, isp->iommu, buf->iommu_addr);
} else {
if (!buf->virt_addr)
continue;
@@ -399,8 +399,8 @@ static int isp_stat_bufs_alloc_iommu(struct ispstat *stat, unsigned int size)
struct iovm_struct *iovm;
WARN_ON(buf->dma_addr);
- buf->iommu_addr = iommu_vmalloc(isp->iommu, 0, size,
- IOMMU_FLAG);
+ buf->iommu_addr = iommu_vmalloc(isp->domain, isp->iommu, 0,
+ size, IOMMU_FLAG);
if (IS_ERR((void *)buf->iommu_addr)) {
dev_err(stat->isp->dev,
"%s: Can't acquire memory for "
diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index 9cd8f1a..7bc7986 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -446,7 +446,7 @@ ispmmu_vmap(struct isp_device *isp, const struct scatterlist *sglist, int sglen)
sgt->nents = sglen;
sgt->orig_nents = sglen;
- da = iommu_vmap(isp->iommu, 0, sgt, IOMMU_FLAG);
+ da = iommu_vmap(isp->domain, isp->iommu, 0, sgt, IOMMU_FLAG);
if (IS_ERR_VALUE(da))
kfree(sgt);
@@ -462,7 +462,7 @@ static void ispmmu_vunmap(struct isp_device *isp, dma_addr_t da)
{
struct sg_table *sgt;
- sgt = iommu_vunmap(isp->iommu, (u32)da);
+ sgt = iommu_vunmap(isp->domain, isp->iommu, (u32)da);
kfree(sgt);
}
--
1.7.1
Create a dedicated folder for iommu drivers, and move the base
iommu implementation over there.
Grouping the varius iommu drivers in a single location will help
finding similar problems shared by different platforms, so they
could be solved once, in the iommu framework, instead of solved
differently (or duplicated) in each driver.
Signed-off-by: Ohad Ben-Cohen <[email protected]>
---
arch/arm/mach-msm/Kconfig | 3 ---
arch/arm/plat-omap/Kconfig | 3 ---
arch/x86/Kconfig | 5 ++---
drivers/Kconfig | 2 ++
drivers/Makefile | 1 +
drivers/base/Makefile | 1 -
drivers/iommu/Kconfig | 3 +++
drivers/iommu/Makefile | 1 +
drivers/{base => iommu}/iommu.c | 0
9 files changed, 9 insertions(+), 10 deletions(-)
create mode 100644 drivers/iommu/Kconfig
create mode 100644 drivers/iommu/Makefile
rename drivers/{base => iommu}/iommu.c (100%)
diff --git a/arch/arm/mach-msm/Kconfig b/arch/arm/mach-msm/Kconfig
index 1516896..efb7b7d 100644
--- a/arch/arm/mach-msm/Kconfig
+++ b/arch/arm/mach-msm/Kconfig
@@ -205,9 +205,6 @@ config MSM_GPIOMUX
config MSM_V2_TLMM
bool
-config IOMMU_API
- bool
-
config MSM_SCM
bool
endif
diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index 1c3acb5..1bb1981 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -131,9 +131,6 @@ config OMAP_MBOX_KFIFO_SIZE
This can also be changed at runtime (via the mbox_kfifo_size
module parameter).
-config IOMMU_API
- bool
-
#can't be tristate; iommu api doesn't support un-registration
config OMAP_IOMMU
bool
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index da34972..460d573 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -685,6 +685,7 @@ config AMD_IOMMU
select SWIOTLB
select PCI_MSI
select PCI_IOV
+ select IOMMU_API
depends on X86_64 && PCI && ACPI
---help---
With this option you can enable support for AMD IOMMU hardware in
@@ -720,9 +721,6 @@ config SWIOTLB
config IOMMU_HELPER
def_bool (CALGARY_IOMMU || GART_IOMMU || SWIOTLB || AMD_IOMMU)
-config IOMMU_API
- def_bool (AMD_IOMMU || DMAR)
-
config MAXSMP
bool "Enable Maximum number of SMP Processors and NUMA Nodes"
depends on X86_64 && SMP && DEBUG_KERNEL && EXPERIMENTAL
@@ -1945,6 +1943,7 @@ config PCI_CNB20LE_QUIRK
config DMAR
bool "Support for DMA Remapping Devices (EXPERIMENTAL)"
depends on PCI_MSI && ACPI && EXPERIMENTAL
+ select IOMMU_API
help
DMA remapping (DMAR) devices support enables independent address
translations for Direct Memory Access (DMA) from devices.
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 3bb154d..9d51318 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -126,4 +126,6 @@ source "drivers/hwspinlock/Kconfig"
source "drivers/clocksource/Kconfig"
+source "drivers/iommu/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 09f3232..2f7a71a 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -122,3 +122,4 @@ obj-y += ieee802154/
obj-y += clk/
obj-$(CONFIG_HWSPINLOCK) += hwspinlock/
+obj-$(CONFIG_IOMMU_API) += iommu/
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c5701c..5ab0d07 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -13,7 +13,6 @@ obj-$(CONFIG_FW_LOADER) += firmware_class.o
obj-$(CONFIG_NUMA) += node.o
obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
obj-$(CONFIG_SMP) += topology.o
-obj-$(CONFIG_IOMMU_API) += iommu.o
ifeq ($(CONFIG_SYSFS),y)
obj-$(CONFIG_MODULES) += module.o
endif
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
new file mode 100644
index 0000000..2c5dfb4
--- /dev/null
+++ b/drivers/iommu/Kconfig
@@ -0,0 +1,3 @@
+# IOMMU_API always gets selected by whoever wants it.
+config IOMMU_API
+ bool
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
new file mode 100644
index 0000000..241ba4c
--- /dev/null
+++ b/drivers/iommu/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_IOMMU_API) += iommu.o
diff --git a/drivers/base/iommu.c b/drivers/iommu/iommu.c
similarity index 100%
rename from drivers/base/iommu.c
rename to drivers/iommu/iommu.c
--
1.7.1
Move OMAP's iommu drivers to the dedicated iommu drivers folder.
While OMAP's iovmm (virtual memory manager) driver does not strictly
belong to the iommu drivers folder, move it there as well, because
it's by no means OMAP-specific (in concept. technically it is still
coupled with the omap iommu), and exposing it will ease its generalization,
consolidation, or removal (once a generic virtual memory manager and allocator
would materialize).
Move omap's iommu debug driver to the generic folder as well, for the
same reasons.
Signed-off-by: Ohad Ben-Cohen <[email protected]>
---
arch/arm/plat-omap/Kconfig | 14 --------------
arch/arm/plat-omap/Makefile | 2 --
arch/arm/plat-omap/{ => include/plat}/iopgtable.h | 0
drivers/iommu/Kconfig | 18 ++++++++++++++++++
drivers/iommu/Makefile | 3 +++
.../iommu/omap-iommu-debug.c | 2 +-
.../iommu.c => drivers/iommu/omap-iommu.c | 2 +-
.../iovmm.c => drivers/iommu/omap-iovmm.c | 2 +-
drivers/media/video/Kconfig | 2 +-
9 files changed, 25 insertions(+), 20 deletions(-)
rename arch/arm/plat-omap/{ => include/plat}/iopgtable.h (100%)
rename arch/arm/plat-omap/iommu-debug.c => drivers/iommu/omap-iommu-debug.c (99%)
rename arch/arm/plat-omap/iommu.c => drivers/iommu/omap-iommu.c (99%)
rename arch/arm/plat-omap/iovmm.c => drivers/iommu/omap-iovmm.c (99%)
diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index 1bb1981..14f067f 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -131,20 +131,6 @@ config OMAP_MBOX_KFIFO_SIZE
This can also be changed at runtime (via the mbox_kfifo_size
module parameter).
-#can't be tristate; iommu api doesn't support un-registration
-config OMAP_IOMMU
- bool
- select IOMMU_API
-
-config OMAP_IOMMU_DEBUG
- tristate "Export OMAP IOMMU internals in DebugFS"
- depends on OMAP_IOMMU && DEBUG_FS
- help
- Select this to see extensive information about
- the internal state of OMAP IOMMU in debugfs.
-
- Say N unless you know you need this.
-
config OMAP_IOMMU_IVA2
bool
diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
index f0233e6..9852622 100644
--- a/arch/arm/plat-omap/Makefile
+++ b/arch/arm/plat-omap/Makefile
@@ -18,8 +18,6 @@ obj-$(CONFIG_ARCH_OMAP3) += omap_device.o
obj-$(CONFIG_ARCH_OMAP4) += omap_device.o
obj-$(CONFIG_OMAP_MCBSP) += mcbsp.o
-obj-$(CONFIG_OMAP_IOMMU) += iommu.o iovmm.o
-obj-$(CONFIG_OMAP_IOMMU_DEBUG) += iommu-debug.o
obj-$(CONFIG_CPU_FREQ) += cpu-omap.o
obj-$(CONFIG_OMAP_DM_TIMER) += dmtimer.o
diff --git a/arch/arm/plat-omap/iopgtable.h b/arch/arm/plat-omap/include/plat/iopgtable.h
similarity index 100%
rename from arch/arm/plat-omap/iopgtable.h
rename to arch/arm/plat-omap/include/plat/iopgtable.h
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 2c5dfb4..57378ac 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -1,3 +1,21 @@
# IOMMU_API always gets selected by whoever wants it.
config IOMMU_API
bool
+
+# can't be tristate; iommu api doesn't support un-registration
+config OMAP_IOMMU
+ bool
+ select IOMMU_API
+
+config OMAP_IOVMM
+ tristate
+ select OMAP_IOMMU
+
+config OMAP_IOMMU_DEBUG
+ tristate "Export OMAP IOMMU internals in DebugFS"
+ depends on OMAP_IOVMM && DEBUG_FS
+ help
+ Select this to see extensive information about
+ the internal state of OMAP IOMMU in debugfs.
+
+ Say N unless you know you need this.
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 241ba4c..c094875 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -1 +1,4 @@
obj-$(CONFIG_IOMMU_API) += iommu.o
+obj-$(CONFIG_OMAP_IOMMU) += omap-iommu.o
+obj-$(CONFIG_OMAP_IOVMM) += omap-iovmm.o
+obj-$(CONFIG_OMAP_IOMMU_DEBUG) += omap-iommu-debug.o
diff --git a/arch/arm/plat-omap/iommu-debug.c b/drivers/iommu/omap-iommu-debug.c
similarity index 99%
rename from arch/arm/plat-omap/iommu-debug.c
rename to drivers/iommu/omap-iommu-debug.c
index f07cf2f..0f8c8dd 100644
--- a/arch/arm/plat-omap/iommu-debug.c
+++ b/drivers/iommu/omap-iommu-debug.c
@@ -21,7 +21,7 @@
#include <plat/iommu.h>
#include <plat/iovmm.h>
-#include "iopgtable.h"
+#include <plat/iopgtable.h>
#define MAXCOLUMN 100 /* for short messages */
diff --git a/arch/arm/plat-omap/iommu.c b/drivers/iommu/omap-iommu.c
similarity index 99%
rename from arch/arm/plat-omap/iommu.c
rename to drivers/iommu/omap-iommu.c
index f06e99c..1526fea 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -25,7 +25,7 @@
#include <plat/iommu.h>
-#include "iopgtable.h"
+#include <plat/iopgtable.h>
#define for_each_iotlb_cr(obj, n, __i, cr) \
for (__i = 0; \
diff --git a/arch/arm/plat-omap/iovmm.c b/drivers/iommu/omap-iovmm.c
similarity index 99%
rename from arch/arm/plat-omap/iovmm.c
rename to drivers/iommu/omap-iovmm.c
index 80bb2b6..42f4a2c 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/drivers/iommu/omap-iovmm.c
@@ -23,7 +23,7 @@
#include <plat/iommu.h>
#include <plat/iovmm.h>
-#include "iopgtable.h"
+#include <plat/iopgtable.h>
/*
* A device driver needs to create address mappings between:
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index bb53de7..4945f91 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -761,7 +761,7 @@ source "drivers/media/video/m5mols/Kconfig"
config VIDEO_OMAP3
tristate "OMAP 3 Camera support (EXPERIMENTAL)"
- select OMAP_IOMMU
+ select OMAP_IOVMM
depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3 && EXPERIMENTAL
---help---
Driver for an OMAP 3 camera controller.
--
1.7.1
This should ease finding similarities with other platforms,
with the intention of solving problems once in a generic framework
which everyone can use.
Signed-off-by: Ohad Ben-Cohen <[email protected]>
---
arch/arm/mach-msm/Kconfig | 12 ------------
arch/arm/mach-msm/Makefile | 2 +-
drivers/iommu/Kconfig | 11 +++++++++++
drivers/iommu/Makefile | 1 +
.../mach-msm/iommu.c => drivers/iommu/msm-iommu.c | 0
5 files changed, 13 insertions(+), 13 deletions(-)
rename arch/arm/mach-msm/iommu.c => drivers/iommu/msm-iommu.c (100%)
diff --git a/arch/arm/mach-msm/Kconfig b/arch/arm/mach-msm/Kconfig
index efb7b7d..14ebda1 100644
--- a/arch/arm/mach-msm/Kconfig
+++ b/arch/arm/mach-msm/Kconfig
@@ -148,18 +148,6 @@ config MACH_MSM8960_RUMI3
endmenu
-config MSM_IOMMU
- bool "MSM IOMMU Support"
- depends on ARCH_MSM8X60 || ARCH_MSM8960
- select IOMMU_API
- default n
- help
- Support for the IOMMUs found on certain Qualcomm SOCs.
- These IOMMUs allow virtualization of the address space used by most
- cores within the multimedia subsystem.
-
- If unsure, say N here.
-
config IOMMU_PGTABLES_L2
def_bool y
depends on MSM_IOMMU && MMU && SMP && CPU_DCACHE_DISABLE=n
diff --git a/arch/arm/mach-msm/Makefile b/arch/arm/mach-msm/Makefile
index 9519fd2..0bf4648 100644
--- a/arch/arm/mach-msm/Makefile
+++ b/arch/arm/mach-msm/Makefile
@@ -3,7 +3,7 @@ obj-y += clock.o
obj-$(CONFIG_DEBUG_FS) += clock-debug.o
obj-$(CONFIG_MSM_VIC) += irq-vic.o
-obj-$(CONFIG_MSM_IOMMU) += iommu.o iommu_dev.o devices-iommu.o
+obj-$(CONFIG_MSM_IOMMU) += iommu_dev.o devices-iommu.o
obj-$(CONFIG_ARCH_MSM7X00A) += dma.o irq.o acpuclock-arm11.o
obj-$(CONFIG_ARCH_MSM7X30) += dma.o
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 57378ac..e083ff0 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -19,3 +19,14 @@ config OMAP_IOMMU_DEBUG
the internal state of OMAP IOMMU in debugfs.
Say N unless you know you need this.
+
+config MSM_IOMMU
+ bool "MSM IOMMU Support"
+ depends on ARCH_MSM8X60 || ARCH_MSM8960
+ select IOMMU_API
+ help
+ Support for the IOMMUs found on certain Qualcomm SOCs.
+ These IOMMUs allow virtualization of the address space used by most
+ cores within the multimedia subsystem.
+
+ If unsure, say N here.
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index c094875..bfb000a 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_IOMMU_API) += iommu.o
obj-$(CONFIG_OMAP_IOMMU) += omap-iommu.o
obj-$(CONFIG_OMAP_IOVMM) += omap-iovmm.o
obj-$(CONFIG_OMAP_IOMMU_DEBUG) += omap-iommu-debug.o
+obj-$(CONFIG_MSM_IOMMU) += msm-iommu.o
diff --git a/arch/arm/mach-msm/iommu.c b/drivers/iommu/msm-iommu.c
similarity index 100%
rename from arch/arm/mach-msm/iommu.c
rename to drivers/iommu/msm-iommu.c
--
1.7.1
Hi,
Good approach.
CC'ed the Samsung IOMMU developer. Marek.
BTW, Russell wants to use the DMA based IOMMU?
Please see the RFC patch, ARM: DMA-mapping & IOMMU integration
http://www.spinics.net/lists/linux-mm/msg19856.html
Thank you,
Kyungmin Park
On Fri, Jun 3, 2011 at 7:27 AM, Ohad Ben-Cohen <[email protected]> wrote:
> First stab at iommu consolidation:
>
> - Migrate OMAP's iommu driver to the generic iommu API. With this in hand,
> ?users can now start using the generic iommu layer instead of calling
> ?omap-specific iommu API.
>
> ?New code that requires functionality missing from the generic iommu api,
> ?will add that functionality in the generic framework (e.g. adding framework
> ?awareness to multi page sizes, supported by the underlying hardware, will
> ?avoid the otherwise-inevitable code duplication when mapping a memory
> ?region).
>
> ?OMAP-specific api that is still exposed in the omap iommu driver can
> ?now be either moved to the generic iommu framework, or just removed (if not
> ?used).
>
> ?This api (and other omap-specific primitives like struct iommu) needs to
> ?be omapified (i.e. renamed to include an 'omap_' prefix). At this early
> ?point of this patch set this is too much churn though, so I'll do that
> ?in the following iteration, after (and if), the general direction is
> ?accepted.
>
> - Migrate OMAP's iovmm (virtual memory manager) driver to the generic
> ?iommu API. With this in hand, iovmm no longer uses omap-specific api
> ?for mapping/unmapping operations. Nevertheless, iovmm is still coupled
> ?with omap's iommu even with this change: it assumes omap page sizes,
> ?and it uses omap's iommu objects to maintain its internal state.
>
> ?Further generalizing of iovmm strongly depends on our broader plans for
> ?providing a generic virtual memory manager and allocation framework
> ?(which, as discussed, should be separated from a specific mapper).
>
> ?iovmm has a mainline user: omap3isp, and therefore must be maintained,
> ?but new potential users will either have to generalize it, or come up
> ?with a different generic framework that will replace it.
>
> - Migrate OMAP's iommu mainline user, omap3isp, to the generic API as well
> ?(so it doesn't break). As with iovmm, omap3isp still depends on
> ?omap's iommu, mainly because iovmm depends on it, but also for
> ?iommu context saving and restoring.
>
> ?It is definitely desirable to completely remove omap3isp's dependency
> ?on the omap-specific iommu layer, and that will be possible as the
> ?required functionality will be added to generic framework.
>
> - Create a dedicated iommu drivers folder (and put the base iommu code there)
> - Move OMAP's and MSM's iommu drivers to that drivers iommu folder
>
> ?Putting all iommu drivers together will ease finding similarities
> ?between different platforms, with the intention of solving problems once,
> ?in a generic framework which everyone can use.
>
> ?I've only moved the omap and msm implementations for now, to demonstrate
> ?the idea (and support the ARM diet :), but if this is found desirable,
> ?we can bring in intel-iommu.c and amd_iommu.c as well.
>
> Meta:
>
> - This patch set is not bisectable; it was splitted (and ordered) this way
> ?to ease its review. Later iterations of this patch set will fix that
> ?(most likely by squashing the first three patches)
> - Based on and tested with 3.0-rc1
> - OMAP's iommu code was tested on both OMAP3 and OMAP4
> - omap3isp code was tested with a sensor-less OMAP3 (memory-to-memory only)
> ?(thanks Laurent Pinchart for showing me the magic needed to test omap3isp :)
> - MSM code was only compile tested
>
> Ohad Ben-Cohen (6):
> ?omap: iommu: generic iommu api migration
> ?omap: iovmm: generic iommu api migration
> ?media: omap3isp: generic iommu api migration
> ?drivers: iommu: move to a dedicated folder
> ?omap: iommu/iovmm: move to dedicated iommu folder
> ?msm: iommu: move to dedicated iommu drivers folder
>
> ?arch/arm/mach-msm/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 15 -
> ?arch/arm/mach-msm/Makefile ? ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +-
> ?arch/arm/plat-omap/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? | ? 12 -
> ?arch/arm/plat-omap/Makefile ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 -
> ?arch/arm/plat-omap/include/plat/iommu.h ? ? ? ? ? ?| ? ?3 +-
> ?arch/arm/plat-omap/{ => include/plat}/iopgtable.h ?| ? 18 ++
> ?arch/arm/plat-omap/include/plat/iovmm.h ? ? ? ? ? ?| ? 27 +-
> ?arch/x86/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?5 +-
> ?drivers/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +
> ?drivers/Makefile ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
> ?drivers/base/Makefile ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 -
> ?drivers/iommu/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 32 +++
> ?drivers/iommu/Makefile ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?5 +
> ?drivers/{base => iommu}/iommu.c ? ? ? ? ? ? ? ? ? ?| ? ?0
> ?.../mach-msm/iommu.c => drivers/iommu/msm-iommu.c ?| ? ?0
> ?.../iommu/omap-iommu-debug.c ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +-
> ?.../iommu.c => drivers/iommu/omap-iommu.c ? ? ? ? ?| ?290 +++++++++++++++++---
> ?.../iovmm.c => drivers/iommu/omap-iovmm.c ? ? ? ? ?| ?113 +++++---
> ?drivers/media/video/Kconfig ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +-
> ?drivers/media/video/omap3isp/isp.c ? ? ? ? ? ? ? ? | ? 41 +++-
> ?drivers/media/video/omap3isp/isp.h ? ? ? ? ? ? ? ? | ? ?3 +
> ?drivers/media/video/omap3isp/ispccdc.c ? ? ? ? ? ? | ? 16 +-
> ?drivers/media/video/omap3isp/ispstat.c ? ? ? ? ? ? | ? ?6 +-
> ?drivers/media/video/omap3isp/ispvideo.c ? ? ? ? ? ?| ? ?4 +-
> ?24 files changed, 451 insertions(+), 151 deletions(-)
> ?rename arch/arm/plat-omap/{ => include/plat}/iopgtable.h (84%)
> ?create mode 100644 drivers/iommu/Kconfig
> ?create mode 100644 drivers/iommu/Makefile
> ?rename drivers/{base => iommu}/iommu.c (100%)
> ?rename arch/arm/mach-msm/iommu.c => drivers/iommu/msm-iommu.c (100%)
> ?rename arch/arm/plat-omap/iommu-debug.c => drivers/iommu/omap-iommu-debug.c (99%)
> ?rename arch/arm/plat-omap/iommu.c => drivers/iommu/omap-iommu.c (77%)
> ?rename arch/arm/plat-omap/iovmm.c => drivers/iommu/omap-iovmm.c (85%)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
On Friday 03 June 2011, Ohad Ben-Cohen wrote:
> First stab at iommu consolidation:
Hi Ohad,
Great to see your progress here!
> - Migrate OMAP's iommu driver to the generic iommu API. With this in hand,
> users can now start using the generic iommu layer instead of calling
> omap-specific iommu API.
>
> New code that requires functionality missing from the generic iommu api,
> will add that functionality in the generic framework (e.g. adding framework
> awareness to multi page sizes, supported by the underlying hardware, will
> avoid the otherwise-inevitable code duplication when mapping a memory
> region).
>
> OMAP-specific api that is still exposed in the omap iommu driver can
> now be either moved to the generic iommu framework, or just removed (if not
> used).
>
> This api (and other omap-specific primitives like struct iommu) needs to
> be omapified (i.e. renamed to include an 'omap_' prefix). At this early
> point of this patch set this is too much churn though, so I'll do that
> in the following iteration, after (and if), the general direction is
> accepted.
Sounds all good.
> - Migrate OMAP's iovmm (virtual memory manager) driver to the generic
> iommu API. With this in hand, iovmm no longer uses omap-specific api
> for mapping/unmapping operations. Nevertheless, iovmm is still coupled
> with omap's iommu even with this change: it assumes omap page sizes,
> and it uses omap's iommu objects to maintain its internal state.
>
> Further generalizing of iovmm strongly depends on our broader plans for
> providing a generic virtual memory manager and allocation framework
> (which, as discussed, should be separated from a specific mapper).
>
> iovmm has a mainline user: omap3isp, and therefore must be maintained,
> but new potential users will either have to generalize it, or come up
> with a different generic framework that will replace it.
I think the future of iovmm is looking not so good. Marek Szyprowski
is working on a generic version of the dma-mapping API (dma_map_ops)
based on the iommu API. As far as I can tell, once we have that in
place, we you can migrate omap3isp from iovmm to dma-mapping and
remove iovmm.
Of course if there are things missing from the dma-mapping API
that are present in iovmm, we should know about them so that we can
extend the dma-mapping API accordingly.
> - Migrate OMAP's iommu mainline user, omap3isp, to the generic API as well
> (so it doesn't break). As with iovmm, omap3isp still depends on
> omap's iommu, mainly because iovmm depends on it, but also for
> iommu context saving and restoring.
>
> It is definitely desirable to completely remove omap3isp's dependency
> on the omap-specific iommu layer, and that will be possible as the
> required functionality will be added to generic framework.
ok.
> - Create a dedicated iommu drivers folder (and put the base iommu code there)
>
> - Move OMAP's and MSM's iommu drivers to that drivers iommu folder
>
> Putting all iommu drivers together will ease finding similarities
> between different platforms, with the intention of solving problems once,
> in a generic framework which everyone can use.
>
> I've only moved the omap and msm implementations for now, to demonstrate
> the idea (and support the ARM diet :), but if this is found desirable,
> we can bring in intel-iommu.c and amd_iommu.c as well.
Yes, very good idea.
Arnd
Hi Arnd,
On Fri, Jun 3, 2011 at 6:53 PM, Arnd Bergmann <[email protected]> wrote:
> I think the future of iovmm is looking not so good. Marek Szyprowski
> is working on a generic version of the dma-mapping API (dma_map_ops)
> based on the iommu API.
Nice! I missed Marek's work somehow.
> As far as I can tell, once we have that in
> place, we you can migrate omap3isp from iovmm to dma-mapping and
> remove iovmm.
Sounds like a plan.
I'd still prefer us to take small steps here, and not gate the omap
iommu cleanups with Marek's generic dma_map_ops work though. Let's go
forward and migrate omap's iommu to the generic iommu API, so new code
will be able to use it (e.g. the long coming virtio-based IPC/AMP
framework).
We'll migrate iovmm/omap3isp just enough so they don't break, but once
the generic dma_map_ops work materializes, we'd be able to complete
the migration, remove iovmm, and decouple omap3isp from omap-specific
iommu APIs for good.
>> ? I've only moved the omap and msm implementations for now, to demonstrate
>> ? the idea (and support the ARM diet :), but if this is found desirable,
>> ? we can bring in intel-iommu.c and amd_iommu.c as well.
>
> Yes, very good idea.
Great!
(to move intel-iommu.c, we'll have to move the declaration of
pci_find_upstream_pcie_bridge() from drivers/pci/pci.h to
include/linux/pci.h, but that's probably not too bad).
Thanks,
Ohad.
Hi Kyungmin,
On Fri, Jun 3, 2011 at 2:57 AM, Kyungmin Park <[email protected]> wrote:
> Please see the RFC patch, ARM: DMA-mapping & IOMMU integration
> http://www.spinics.net/lists/linux-mm/msg19856.html
Marek's work somehow escaped me, thanks for the pointers !
Ohad.
On Sunday 05 June 2011, Ohad Ben-Cohen wrote:
> > As far as I can tell, once we have that in
> > place, we you can migrate omap3isp from iovmm to dma-mapping and
> > remove iovmm.
>
> Sounds like a plan.
>
> I'd still prefer us to take small steps here, and not gate the omap
> iommu cleanups with Marek's generic dma_map_ops work though. Let's go
> forward and migrate omap's iommu to the generic iommu API, so new code
> will be able to use it (e.g. the long coming virtio-based IPC/AMP
> framework).
Yes, of course. That's what I meant. Moving over omap to the IOMMU API
is required anyway, so there is no point delaying that.
> We'll migrate iovmm/omap3isp just enough so they don't break, but once
> the generic dma_map_ops work materializes, we'd be able to complete
> the migration, remove iovmm, and decouple omap3isp from omap-specific
> iommu APIs for good.
Ok, great!
Arnd
Hi,
On Thu, Jun 02, 2011 at 06:27:37PM -0400, Ohad Ben-Cohen wrote:
> First stab at iommu consolidation:
>
> - Migrate OMAP's iommu driver to the generic iommu API. With this in hand,
> users can now start using the generic iommu layer instead of calling
> omap-specific iommu API.
>
> New code that requires functionality missing from the generic iommu api,
> will add that functionality in the generic framework (e.g. adding framework
> awareness to multi page sizes, supported by the underlying hardware, will
> avoid the otherwise-inevitable code duplication when mapping a memory
> region).
The IOMMU-API already supports multiple page-sizes. See the
'order'-parameter of the map/unmap functions.
> Further generalizing of iovmm strongly depends on our broader plans for
> providing a generic virtual memory manager and allocation framework
> (which, as discussed, should be separated from a specific mapper).
The generic vmm for DMA is called DMA-API :) Any reason for not using
that (those reasons should be fixed)?
> iovmm has a mainline user: omap3isp, and therefore must be maintained,
> but new potential users will either have to generalize it, or come up
> with a different generic framework that will replace it.
Moving to the DMA-API should be considered here. If it is too much work
iovmm can stay for a while, but the goal should be to get rid of it and
only use the DMA-API.
> - Create a dedicated iommu drivers folder (and put the base iommu code there)
Very good idea.
Joerg
P.S.: Please also Cc the iommu-list ([email protected])
in the future for IOMMU related patches.
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
Hi Joerg,
On Mon, Jun 6, 2011 at 1:09 PM, Roedel, Joerg <[email protected]> wrote:
> The IOMMU-API already supports multiple page-sizes. See the
> 'order'-parameter of the map/unmap functions.
This is insufficient; users need somehow to tell what page sizes are
supported by the underlying hardware (we can't assume host page-sizes,
and we want to use bigger pages whenever possible, to relax the TLB
pressure).
>> ? Further generalizing of iovmm strongly depends on our broader plans for
>> ? providing a generic virtual memory manager and allocation framework
>> ? (which, as discussed, should be separated from a specific mapper).
>
> The generic vmm for DMA is called DMA-API :) Any reason for not using
> that (those reasons should be fixed)?
This is definitely something we will look into (dspbridge will need it
too, not only omap3isp).
Thanks,
Ohad.
On Mon, Jun 6, 2011 at 12:10 PM, Arnd Bergmann <[email protected]> wrote:
>> I'd still prefer us to take small steps here, and not gate the omap
>> iommu cleanups with Marek's generic dma_map_ops work though. Let's go
>> forward and migrate omap's iommu to the generic iommu API, so new code
>> will be able to use it (e.g. the long coming virtio-based IPC/AMP
>> framework).
>
> Yes, of course. That's what I meant. Moving over omap to the IOMMU API
> is required anyway, so there is no point delaying that.
Ok, great !
On Mon, Jun 06, 2011 at 11:15:30AM -0400, Ohad Ben-Cohen wrote:
> This is insufficient; users need somehow to tell what page sizes are
> supported by the underlying hardware (we can't assume host page-sizes,
> and we want to use bigger pages whenever possible, to relax the TLB
> pressure).
/
What does the IOMMU-API user need this info for? On the x86 IOMMUs these
details are handled transparently by the IOMMU driver.
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
On Mon, Jun 6, 2011 at 6:35 PM, Roedel, Joerg <[email protected]> wrote:
> On Mon, Jun 06, 2011 at 11:15:30AM -0400, Ohad Ben-Cohen wrote:
>
>> This is insufficient; users need somehow to tell what page sizes are
>> supported by the underlying hardware (we can't assume host page-sizes,
>> and we want to use bigger pages whenever possible, to relax the TLB
>> pressure).
> /
> What does the IOMMU-API user need this info for? On the x86 IOMMUs these
> details are handled transparently by the IOMMU driver.
That's one way to do that, but then it means duplicating this logic
inside the different IOMMU implementations.
Take the OMAP (and seemingly MSM too) example: we have 4KB, 64KB, 1MB
and 16MB page-table entries. When we map a memory region, we need to
break it up to a minimum number of pages (while validating
sizes/alignments are sane). It's not complicated, but it can be nice
if it'd be implemented only once.
In addition, unless we require 'va' and 'pa' to have the exact same
alignment, we might run into specific page configuration that the
IOMMU implementation cannot restore on ->unmap, since unmap only takes
'va' and 'order'. So we will either have to supply 'pa' too, or have
the implementation remember the mapping in order to unmap it later.
That begins to be a bit messy...
On Mon, Jun 06, 2011 at 12:36:13PM -0400, Ohad Ben-Cohen wrote:
> On Mon, Jun 6, 2011 at 6:35 PM, Roedel, Joerg <[email protected]> wrote:
> > On Mon, Jun 06, 2011 at 11:15:30AM -0400, Ohad Ben-Cohen wrote:
> >
> >> This is insufficient; users need somehow to tell what page sizes are
> >> supported by the underlying hardware (we can't assume host page-sizes,
> >> and we want to use bigger pages whenever possible, to relax the TLB
> >> pressure).
> > /
> > What does the IOMMU-API user need this info for? On the x86 IOMMUs these
> > details are handled transparently by the IOMMU driver.
>
> That's one way to do that, but then it means duplicating this logic
> inside the different IOMMU implementations.
>
> Take the OMAP (and seemingly MSM too) example: we have 4KB, 64KB, 1MB
> and 16MB page-table entries. When we map a memory region, we need to
> break it up to a minimum number of pages (while validating
> sizes/alignments are sane). It's not complicated, but it can be nice
> if it'd be implemented only once.
Well, it certainly makes sense to have a single implementation for this.
But I want to hide this complexity to the user of the IOMMU-API. The
best choice is to put this into the layer between the IOMMU-API and the
backend implementation.
> In addition, unless we require 'va' and 'pa' to have the exact same
> alignment, we might run into specific page configuration that the
> IOMMU implementation cannot restore on ->unmap, since unmap only takes
> 'va' and 'order'. So we will either have to supply 'pa' too, or have
> the implementation remember the mapping in order to unmap it later.
> That begins to be a bit messy...
That interface is not put into stone. There were other complains about
the ->unmap part recently, so there is certainly room for improvement
there.
Regards,
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
On Mon, Jun 6, 2011 at 10:20 PM, Roedel, Joerg <[email protected]> wrote:
> Well, it certainly makes sense to have a single implementation for this.
> But I want to hide this complexity to the user of the IOMMU-API. The
> best choice is to put this into the layer between the IOMMU-API and the
> backend implementation.
I agree.
The IOMMU API should take physically contiguous regions from the user,
split them up according to page-sizes (/alignment requirements)
supported by the hardware, and then tell the underlying implementation
what to map where.
> That interface is not put into stone. There were other complains about
> the ->unmap part recently, so there is certainly room for improvement
> there.
Once the supported page sizes are exposed to the framework, the
current ->unmap API should probably be enough. 'va' + 'order' sounds
like all the information an implementation needs to unmap a page.
On Mon, Jun 06, 2011 at 04:09:33PM -0400, Ohad Ben-Cohen wrote:
> On Mon, Jun 6, 2011 at 10:20 PM, Roedel, Joerg <[email protected]> wrote:
> > Well, it certainly makes sense to have a single implementation for this.
> > But I want to hide this complexity to the user of the IOMMU-API. The
> > best choice is to put this into the layer between the IOMMU-API and the
> > backend implementation.
>
> I agree.
>
> The IOMMU API should take physically contiguous regions from the user,
> split them up according to page-sizes (/alignment requirements)
> supported by the hardware, and then tell the underlying implementation
> what to map where.
Yup. Btw, is there any IOMMU hardware which supports non-natural
alignment? In this case we need to expose these requirements somehow.
Regards,
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
Hi Ohad,
Thanks for the patch.
On Friday 03 June 2011 00:27:39 Ohad Ben-Cohen wrote:
> Migrate omap's iovmm (virtual memory manager) to the generic iommu api.
>
> This brings iovmm a step forward towards being completely non
> omap-specific (it's still assuming omap's iommu page sizes, and also
> maintaining state inside omap's internal iommu structure, but it no
> longer calls omap-specific iommu map/unmap api).
>
> Further generalizing of iovmm (or complete removal) should take place
> together with broader plans of providing a generic virtual memory manager
> and allocation framework (de-coupled from specific mappers).
>
> Signed-off-by: Ohad Ben-Cohen <[email protected]>
[snip]
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 51ef43e..80bb2b6 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
[snip]
> @@ -473,22 +475,22 @@ static int map_iovm_area(struct iommu *obj, struct
> iovm_struct *new, u32 pa;
> int pgsz;
> size_t bytes;
> - struct iotlb_entry e;
>
> pa = sg_phys(sg);
> bytes = sg_dma_len(sg);
>
> flags &= ~IOVMF_PGSZ_MASK;
> +
> pgsz = bytes_to_iopgsz(bytes);
> if (pgsz < 0)
> goto err_out;
> - flags |= pgsz;
pgsz isn't used anymore, you can remove it.
> +
> + order = get_order(bytes);
Does iommu_map() handle offsets correctly, or does it expect pa to be aligned
to an order (or other) boundary ? Same comment for iommu_unmap() in
unmap_iovm_area().
> pr_debug("%s: [%d] %08x %08x(%x)\n", __func__,
> i, da, pa, bytes);
>
> - iotlb_init_entry(&e, da, pa, flags);
> - err = iopgtable_store_entry(obj, &e);
> + err = iommu_map(domain, da, pa, order, flags);
> if (err)
> goto err_out;
>
> @@ -502,9 +504,11 @@ err_out:
> for_each_sg(sgt->sgl, sg, i, j) {
> size_t bytes;
>
> - bytes = iopgtable_clear_entry(obj, da);
> + bytes = sg_dma_len(sg);
As Russell pointed out, we should use sg->length instead of sg_dma_length(sg).
sg_dma_length(sg) is only valid after the scatter list has been DMA-mapped,
which doesn't happen in the iovmm driver. This applies to all sg_dma_len(sg)
calls.
> + order = get_order(bytes);
>
> - BUG_ON(!iopgsz_ok(bytes));
> + /* ignore failures.. we're already handling one */
> + iommu_unmap(domain, da, order);
>
> da += bytes;
> }
--
Regards,
Laurent Pinchart
On Tue, Jun 7, 2011 at 10:52 AM, Roedel, Joerg <[email protected]> wrote:
> Yup. Btw, is there any IOMMU hardware which supports non-natural
> alignment? In this case we need to expose these requirements somehow.
Not sure there are. Let's start with natural alignment, and extend it
only if someone with additional requirements shows up.
Hi Ohad,
Thanks for the patch.
On Friday 03 June 2011 00:27:38 Ohad Ben-Cohen wrote:
> Migrate OMAP's iommu to the generic iommu api, so users can stay
> generic, and non-omap-specific code can be removed and eventually
> consolidated into a generic framework.
>
> Tested on both OMAP3 and OMAP4.
>
> Signed-off-by: Ohad Ben-Cohen <[email protected]>
[snip]
> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index 34fc31e..f06e99c 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
[snip]
> +static int omap_iommu_domain_init(struct iommu_domain *domain)
> +{
> + struct omap_iommu_domain *omap_domain;
> +
> + omap_domain = kzalloc(sizeof(*omap_domain), GFP_KERNEL);
> + if (!omap_domain) {
> + pr_err("kzalloc failed\n");
> + goto fail_nomem;
> + }
> +
> + omap_domain->pgtable = (u32 *)__get_free_pages(GFP_KERNEL,
> + get_order(IOPGD_TABLE_SIZE));
> + if (!omap_domain->pgtable) {
> + pr_err("__get_free_pages failed\n");
> + goto fail_nomem;
> + }
> +
> + BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));
Either __get_free_pages() guarantees that the allocated memory will be aligned
on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is unnecessary, or
doesn't offer such guarantee, in which case the BUG_ON() will oops randomly.
In both cases BUG_ON() should probably be avoided.
> + memset(omap_domain->pgtable, 0, IOPGD_TABLE_SIZE);
> + clean_dcache_area(omap_domain->pgtable, IOPGD_TABLE_SIZE);
> + mutex_init(&omap_domain->lock);
> +
> + domain->priv = omap_domain;
> +
> + return 0;
> +
> +fail_nomem:
> + kfree(omap_domain);
> + return -ENOMEM;
> +}
> +
> +/* assume device was already detached */
> +static void omap_iommu_domain_destroy(struct iommu_domain *domain)
> +{
> + struct omap_iommu_domain *omap_domain = domain->priv;
> +
> + domain->priv = NULL;
> +
> + kfree(omap_domain);
This leaks omap_domain->pgtable.
The free_pages() call in omap_iommu_remove() should be removed, as
omap_iommu_probe() doesn't allocate the pages table anymore. You can also
remove the the struct iommu::iopgd field.
> +}
> +
> +static phys_addr_t omap_iommu_iova_to_phys(struct iommu_domain *domain,
> + unsigned long da)
> +{
> + struct omap_iommu_domain *omap_domain = domain->priv;
> + struct iommu *oiommu = omap_domain->iommu_dev;
> + struct device *dev = oiommu->dev;
> + u32 *pgd, *pte;
> + phys_addr_t ret = 0;
> +
> + iopgtable_lookup_entry(oiommu, da, &pgd, &pte);
> +
> + if (pte) {
> + if (iopte_is_small(*pte))
> + ret = omap_iommu_translate(*pte, da, IOPTE_MASK);
> + else if (iopte_is_large(*pte))
> + ret = omap_iommu_translate(*pte, da, IOLARGE_MASK);
> + else
> + dev_err(dev, "bogus pte 0x%x", *pte);
> + } else {
> + if (iopgd_is_section(*pgd))
> + ret = omap_iommu_translate(*pgd, da, IOSECTION_MASK);
> + else if (iopgd_is_super(*pgd))
> + ret = omap_iommu_translate(*pgd, da, IOSUPER_MASK);
> + else
> + dev_err(dev, "bogus pgd 0x%x", *pgd);
> + }
> +
> + return ret;
You return 0 in the bogus pte/pgd cases. Is that intentional ?
> +}
--
Regards,
Laurent Pinchart
On Tue, Jun 07, 2011 at 05:22:11AM -0400, Ohad Ben-Cohen wrote:
> On Tue, Jun 7, 2011 at 10:52 AM, Roedel, Joerg <[email protected]> wrote:
> > Yup. Btw, is there any IOMMU hardware which supports non-natural
> > alignment? In this case we need to expose these requirements somehow.
>
> Not sure there are. Let's start with natural alignment, and extend it
> only if someone with additional requirements shows up.
Btw, mind to split out your changes which move the iommu-api into
drivers/iommu? I can merge them meanwhile into my iommu tree and start
working on a proposal for the generic large page-size support.
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
Hi Laurent,
On Tue, Jun 7, 2011 at 12:05 PM, Laurent Pinchart
<[email protected]> wrote:
> pgsz isn't used anymore, you can remove it.
Ok.
>> + ? ? ? ? ? ? order = get_order(bytes);
>
> Does iommu_map() handle offsets correctly, or does it expect pa to be aligned
> to an order (or other) boundary ?
Right now we have a BUG_ON if pa is unaligned, but that can be changed
if needed (do we want it to handle offsets ?).
> As Russell pointed out, we should use sg->length instead of sg_dma_length(sg).
> sg_dma_length(sg) is only valid after the scatter list has been DMA-mapped,
> which doesn't happen in the iovmm driver. This applies to all sg_dma_len(sg)
> calls.
I'll make sure I don't introduce such calls, but it sounds like a
separate patch should take care of the existing ones; pls tell me if
you want me to send one.
Thanks,
Ohad.
On Tue, Jun 7, 2011 at 12:58 PM, Roedel, Joerg <[email protected]> wrote:
> Btw, mind to split out your changes which move the iommu-api into
> drivers/iommu? I can merge them meanwhile into my iommu tree and start
> working on a proposal for the generic large page-size support.
Sure, that will be great. Thanks!
Hi Laurent,
On Tue, Jun 7, 2011 at 12:22 PM, Laurent Pinchart
<[email protected]> wrote:
>> + ? ? BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));
>
> Either __get_free_pages() guarantees that the allocated memory will be aligned
> on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is unnecessary, or
> doesn't offer such guarantee, in which case the BUG_ON() will oops randomly.
Curious, does it oops randomly today ?
(i just copied this from omap_iommu_probe, where it always existed).
It is a bit ugly though, and thinking on it again, 16KB is not that
big. We can just use kmalloc here, which does ensure the alignment
(or, better yet, kzalloc, and then ditch the memset).
> In both cases BUG_ON() should probably be avoided.
I disagree; we must check this so user data won't be harmed (hardware
requirement), and if a memory allocation API fails to meet its
requirements - that's really bad and user data is again at stake (much
more will break, not only the iommu driver).
> This leaks omap_domain->pgtable.
>
> The free_pages() call in omap_iommu_remove() should be removed, as
> omap_iommu_probe() doesn't allocate the pages table anymore.
thanks !
> You can also remove the the struct iommu::iopgd field.
No, I can't; it's used when the device is attached to an address space domain.
> You return 0 in the bogus pte/pgd cases. Is that intentional ?
Yes, that's probably the most (if any) reasonable value to return here
(all other iommu implementations are doing so too).
Thanks,
Ohad.
Hi Ohad,
On Tuesday 07 June 2011 12:28:53 Ohad Ben-Cohen wrote:
> On Tue, Jun 7, 2011 at 12:05 PM, Laurent Pinchart wrote:
> > pgsz isn't used anymore, you can remove it.
>
> Ok.
>
> >> + order = get_order(bytes);
> >
> > Does iommu_map() handle offsets correctly, or does it expect pa to be
> > aligned to an order (or other) boundary ?
>
> Right now we have a BUG_ON if pa is unaligned, but that can be changed
> if needed (do we want it to handle offsets ?).
At least for the OMAP3 ISP we need to, as video buffers don't necessarily
start on page boundaries.
> > As Russell pointed out, we should use sg->length instead of
> > sg_dma_length(sg). sg_dma_length(sg) is only valid after the scatter
> > list has been DMA-mapped, which doesn't happen in the iovmm driver. This
> > applies to all sg_dma_len(sg) calls.
>
> I'll make sure I don't introduce such calls, but it sounds like a
> separate patch should take care of the existing ones; pls tell me if
> you want me to send one.
A separate patch is indeed needed, yes. As you're already working on iommu it
might be simpler if you add it to your tree. Otherwise I can send it.
--
Regards,
Laurent Pinchart
Hi Ohad,
On Tuesday 07 June 2011 13:19:05 Ohad Ben-Cohen wrote:
> On Tue, Jun 7, 2011 at 12:22 PM, Laurent Pinchart wrote:
> >> + BUG_ON(!IS_ALIGNED((long)omap_domain->pgtable, IOPGD_TABLE_SIZE));
> >
> > Either __get_free_pages() guarantees that the allocated memory will be
> > aligned on an IOPGD_TABLE_SIZE boundary, in which case the BUG_ON() is
> > unnecessary, or doesn't offer such guarantee, in which case the BUG_ON()
> > will oops randomly.
>
> Curious, does it oops randomly today ?
> (i just copied this from omap_iommu_probe, where it always existed).
No that I know of :-)
> It is a bit ugly though, and thinking on it again, 16KB is not that
> big. We can just use kmalloc here, which does ensure the alignment
> (or, better yet, kzalloc, and then ditch the memset).
>
> > In both cases BUG_ON() should probably be avoided.
>
> I disagree; we must check this so user data won't be harmed (hardware
> requirement), and if a memory allocation API fails to meet its
> requirements - that's really bad and user data is again at stake (much
> more will break, not only the iommu driver).
My point is that if the allocator guarantees the alignment (not as a side
effect of the implementation, but per its API) there's no need to check it
again. As the alignement is required, we need an allocator that guarantees it
anyway.
> > This leaks omap_domain->pgtable.
> >
> > The free_pages() call in omap_iommu_remove() should be removed, as
> > omap_iommu_probe() doesn't allocate the pages table anymore.
>
> thanks !
>
> > You can also remove the the struct iommu::iopgd field.
>
> No, I can't; it's used when the device is attached to an address space
> domain.
Right, my bad.
> > You return 0 in the bogus pte/pgd cases. Is that intentional ?
>
> Yes, that's probably the most (if any) reasonable value to return here
> (all other iommu implementations are doing so too).
--
Regards,
Laurent Pinchart
On Tue, Jun 7, 2011 at 2:40 PM, Laurent Pinchart
<[email protected]> wrote:
> My point is that if the allocator guarantees the alignment (not as a side
> effect of the implementation, but per its API) there's no need to check it
> again. As the alignement is required, we need an allocator that guarantees it
> anyway.
I understand, but I'd still prefer to have an explicit check that the
hardware alignment requirement is met.
There's no cost in doing that (it's a cold path), and even if it would
only fail once and with an extremely broken kernel - it's worth it.
Will save huge amount of debugging pain (think of the poor guy that
will have to debug this...).
Thanks,
Ohad.
Hi Laurent,
On Tue, Jun 7, 2011 at 2:26 PM, Laurent Pinchart
<[email protected]> wrote:
>> Right now we have a BUG_ON if pa is unaligned, but that can be changed
>> if needed (do we want it to handle offsets ?).
>
> At least for the OMAP3 ISP we need to, as video buffers don't necessarily
> start on page boundaries.
Where do you take care of those potential offsets today ? Or do you
simply ignore the offsets and map the entire page ?
Seems like omap's iommu (mostly) rejects unaligned pa addresses, see:
4abb761749abfb4ec403e4054f9dae2ee604e54f "omap iommu: Reject unaligned
addresses at setting page table entry"
(this doesn't seem to cover 4KB entries though, only large pages,
sections and super sections)
> A separate patch is indeed needed, yes. As you're already working on iommu it
> might be simpler if you add it to your tree.
Sure, i'll send it.
Thanks,
Ohad.
Hi Ohad,
On Tuesday 07 June 2011 15:46:26 Ohad Ben-Cohen wrote:
> On Tue, Jun 7, 2011 at 2:26 PM, Laurent Pinchart wrote:
> >> Right now we have a BUG_ON if pa is unaligned, but that can be changed
> >> if needed (do we want it to handle offsets ?).
> >
> > At least for the OMAP3 ISP we need to, as video buffers don't necessarily
> > start on page boundaries.
>
> Where do you take care of those potential offsets today ? Or do you
> simply ignore the offsets and map the entire page ?
Here http://marc.info/?l=linux-omap&m=130693502326513&w=2 :-)
> Seems like omap's iommu (mostly) rejects unaligned pa addresses, see:
>
> 4abb761749abfb4ec403e4054f9dae2ee604e54f "omap iommu: Reject unaligned
> addresses at setting page table entry"
>
> (this doesn't seem to cover 4KB entries though, only large pages,
> sections and super sections)
>
> > A separate patch is indeed needed, yes. As you're already working on
> > iommu it might be simpler if you add it to your tree.
>
> Sure, i'll send it.
--
Regards,
Laurent Pinchart
On Wed, Jun 8, 2011 at 1:46 PM, Laurent Pinchart
<[email protected]> wrote:
> On Tuesday 07 June 2011 15:46:26 Ohad Ben-Cohen wrote:
>> Where do you take care of those potential offsets today ? Or do you
>> simply ignore the offsets and map the entire page ?
>
> Here http://marc.info/?l=linux-omap&m=130693502326513&w=2 :-)
:)
Ok so it seems iovmm will take care of that for now ?
Let's get the basics working with the IOMMU API and then revise this
when we switch from iovmm to the generic dma.
Thanks,
Ohad.