[PATCH v6 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V6

[PATCH v6 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V6

[Magnus Damm]

iommu/ipmmu-vmsa: IPMMU multi-arch update V6

[PATCH v6 01/07] iommu/ipmmu-vmsa: Remove platform data handling
[PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context
[PATCH v6 03/07] iommu/ipmmu-vmsa: Break out utlb parsing code
[PATCH v6 04/07] iommu/ipmmu-vmsa: Break out domain allocation code
[PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
[PATCH v6 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access
[PATCH v6 07/07] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency

These patches update the IPMMU driver with a couple of changes
to support build on multiple architectures. In the process of
doing so the interrupt code gets reworked and the foundation
for supporting multiple contexts are added.

Changes since V5:
 - Rebased series on top of next-20161019
 - Updated patch 5/7 to simplify domain allocation/free code - thanks Joerg!
 - Added reviewed-by tag from Joerg for patch 1-4 and 6-7.
 
Changes since V4:
 - Updated patch 3/7 to work on top on the following commit in next-20160920:
   b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device
 - Add Kconfig hunk to patch 5/7 to avoid undeclared ipmmu_ops if COMPILE_TEST
 - Rebased patch 7/7 to fit on top of new Kconfig bits in 5/7

Changes since V3:
 - Updated patch 3/7 to fix hang-on-boot issue on 32-bit ARM - thanks Geert!
 - Reworked group parameter handling in patch 3/7 and 5/7.
 - Added patch 6/7 to fix build of the driver on s390/tile/um architectures

Changes since V2:
 - Got rid of patch 3 from the V2 however patch 1, 2 and 4 are kept.
 - V3 patch 3, 4 and 5 come from
   [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
 - Patch 5 has been reworked to include patch 3 of the V1 of this series 

Changes since V1:
 - Got rid of patch 2 and 3 from initial series
 - Updated bitmap code locking and also used lighter bitop functions
 - Updated the Kconfig bits to apply on top of ARCH_RENESAS

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Built on top next-20161019:

 drivers/iommu/Kconfig      |    2 
 drivers/iommu/ipmmu-vmsa.c |  311 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 254 insertions(+), 59 deletions(-)

[PATCH v6 01/07] iommu/ipmmu-vmsa: Remove platform data handling

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

The IPMMU driver is using DT these days, and platform data is no longer
used by the driver. Remove unused code.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---

 Changes since V5:
 - None

 Changes since V4:
 - None

 Changes since V3:
 - None

 Changes since V2:
 - None

 Changes since V1:
 - Added Reviewed-by from Laurent

 drivers/iommu/ipmmu-vmsa.c |    5 -----
 1 file changed, 5 deletions(-)

--- 0002/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:46:26.000000000 +0900
@@ -766,11 +766,6 @@ static int ipmmu_probe(struct platform_d
 	int irq;
 	int ret;
 
-	if (!IS_ENABLED(CONFIG_OF) && !pdev->dev.platform_data) {
-		dev_err(&pdev->dev, "missing platform data\n");
-		return -EINVAL;
-	}
-
 	mmu = devm_kzalloc(&pdev->dev, sizeof(*mmu), GFP_KERNEL);
 	if (!mmu) {
 		dev_err(&pdev->dev, "cannot allocate device data\n");

[PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Introduce a bitmap for context handing and convert the
interrupt routine to handle all registered contexts.

At this point the number of contexts are still limited.

Also remove the use of the ARM specific mapping variable
from ipmmu_irq() to allow compile on ARM64.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---

 Changes since V5:
 - None

 Changes since V4:
 - None

 Changes since V3:
 - None

 Changes since V2: (Thanks again to Laurent!)
 - Introduce a spinlock together with the bitmap and domain array.
 - Break out code into separate functions for alloc and free.
 - Perform free after (instead of before) configuring hardware registers.
 - Use the spinlock to protect the domain array in the interrupt handler.

 Changes since V1: (Thanks to Laurent for feedback!)
 - Use simple find_first_zero()/set_bit()/clear_bit() for context management.
 - For allocation rely on spinlock held when calling ipmmu_domain_init_context()
 - For test/free use atomic bitops
 - Return IRQ_HANDLED if any of the contexts generated interrupts

 drivers/iommu/ipmmu-vmsa.c |   76 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 66 insertions(+), 10 deletions(-)

--- 0004/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:48:23.770607110 +0900
@@ -8,6 +8,7 @@
  * the Free Software Foundation; version 2 of the License.
  */
 
+#include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
@@ -26,12 +27,17 @@
 
 #include "io-pgtable.h"
 
+#define IPMMU_CTX_MAX 1
+
 struct ipmmu_vmsa_device {
 	struct device *dev;
 	void __iomem *base;
 	struct list_head list;
 
 	unsigned int num_utlbs;
+	spinlock_t lock;			/* Protects ctx and domains[] */
+	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
+	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
 
 	struct dma_iommu_mapping *mapping;
 };
@@ -293,9 +299,29 @@ static struct iommu_gather_ops ipmmu_gat
  * Domain/Context Management
  */
 
+static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
+					 struct ipmmu_vmsa_domain *domain)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&mmu->lock, flags);
+
+	ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
+	if (ret != IPMMU_CTX_MAX) {
+		mmu->domains[ret] = domain;
+		set_bit(ret, mmu->ctx);
+	}
+
+	spin_unlock_irqrestore(&mmu->lock, flags);
+
+	return ret;
+}
+
 static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
 {
 	u64 ttbr;
+	int ret;
 
 	/*
 	 * Allocate the page table operations.
@@ -325,10 +351,15 @@ static int ipmmu_domain_init_context(str
 		return -EINVAL;
 
 	/*
-	 * TODO: When adding support for multiple contexts, find an unused
-	 * context.
+	 * Find an unused context.
 	 */
-	domain->context_id = 0;
+	ret = ipmmu_domain_allocate_context(domain->mmu, domain);
+	if (ret == IPMMU_CTX_MAX) {
+		free_io_pgtable_ops(domain->iop);
+		return -EBUSY;
+	}
+
+	domain->context_id = ret;
 
 	/* TTBR0 */
 	ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
@@ -370,6 +401,19 @@ static int ipmmu_domain_init_context(str
 	return 0;
 }
 
+static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
+				      unsigned int context_id)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&mmu->lock, flags);
+
+	clear_bit(context_id, mmu->ctx);
+	mmu->domains[context_id] = NULL;
+
+	spin_unlock_irqrestore(&mmu->lock, flags);
+}
+
 static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
 {
 	/*
@@ -380,6 +424,7 @@ static void ipmmu_domain_destroy_context
 	 */
 	ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
 	ipmmu_tlb_sync(domain);
+	ipmmu_domain_free_context(domain->mmu, domain->context_id);
 }
 
 /* -----------------------------------------------------------------------------
@@ -437,16 +482,25 @@ static irqreturn_t ipmmu_domain_irq(stru
 static irqreturn_t ipmmu_irq(int irq, void *dev)
 {
 	struct ipmmu_vmsa_device *mmu = dev;
-	struct iommu_domain *io_domain;
-	struct ipmmu_vmsa_domain *domain;
+	irqreturn_t status = IRQ_NONE;
+	unsigned int i;
+	unsigned long flags;
 
-	if (!mmu->mapping)
-		return IRQ_NONE;
+	spin_lock_irqsave(&mmu->lock, flags);
+
+	/*
+	 * Check interrupts for all active contexts.
+	 */
+	for (i = 0; i < IPMMU_CTX_MAX; i++) {
+		if (!mmu->domains[i])
+			continue;
+		if (ipmmu_domain_irq(mmu->domains[i]) == IRQ_HANDLED)
+			status = IRQ_HANDLED;
+	}
 
-	io_domain = mmu->mapping->domain;
-	domain = to_vmsa_domain(io_domain);
+	spin_unlock_irqrestore(&mmu->lock, flags);
 
-	return ipmmu_domain_irq(domain);
+	return status;
 }
 
 /* -----------------------------------------------------------------------------
@@ -774,6 +828,8 @@ static int ipmmu_probe(struct platform_d
 
 	mmu->dev = &pdev->dev;
 	mmu->num_utlbs = 32;
+	spin_lock_init(&mmu->lock);
+	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
 
 	/* Map I/O memory and request IRQ. */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

[PATCH v6 03/07] iommu/ipmmu-vmsa: Break out utlb parsing code

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Break out the utlb parsing code and dev_data allocation into a
separate function. This is preparation for future code sharing.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---

 Changes since V5:
 - None

 Changes since V4:
 - Dropped hunk with fix to apply on top of:
   b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device
 
 Changes since V3:
 - Initialize "mmu" to NULL, check before accessing
 - Removed group parameter from ipmmu_init_platform_device()

 Changes since V2:
 - Included this new patch from the following series:
   [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
 - Reworked code to fit on top on previous two patches in current series.

 drivers/iommu/ipmmu-vmsa.c |   58 ++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 21 deletions(-)

--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:49:34.580607110 +0900
@@ -647,22 +647,15 @@ static int ipmmu_find_utlbs(struct ipmmu
 	return 0;
 }
 
-static int ipmmu_add_device(struct device *dev)
+static int ipmmu_init_platform_device(struct device *dev)
 {
 	struct ipmmu_vmsa_archdata *archdata;
 	struct ipmmu_vmsa_device *mmu;
-	struct iommu_group *group = NULL;
 	unsigned int *utlbs;
 	unsigned int i;
 	int num_utlbs;
 	int ret = -ENODEV;
 
-	if (dev->archdata.iommu) {
-		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
-			 dev_name(dev));
-		return -EINVAL;
-	}
-
 	/* Find the master corresponding to the device. */
 
 	num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
@@ -699,6 +692,36 @@ static int ipmmu_add_device(struct devic
 		}
 	}
 
+	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
+	if (!archdata) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	archdata->mmu = mmu;
+	archdata->utlbs = utlbs;
+	archdata->num_utlbs = num_utlbs;
+	dev->archdata.iommu = archdata;
+	return 0;
+
+error:
+	kfree(utlbs);
+	return ret;
+}
+
+static int ipmmu_add_device(struct device *dev)
+{
+	struct ipmmu_vmsa_archdata *archdata;
+	struct ipmmu_vmsa_device *mmu = NULL;
+	struct iommu_group *group;
+	int ret;
+
+	if (dev->archdata.iommu) {
+		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
+			 dev_name(dev));
+		return -EINVAL;
+	}
+
 	/* Create a device group and add the device to it. */
 	group = iommu_group_alloc();
 	if (IS_ERR(group)) {
@@ -716,16 +739,9 @@ static int ipmmu_add_device(struct devic
 		goto error;
 	}
 
-	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
-	if (!archdata) {
-		ret = -ENOMEM;
+	ret = ipmmu_init_platform_device(dev);
+	if (ret < 0)
 		goto error;
-	}
-
-	archdata->mmu = mmu;
-	archdata->utlbs = utlbs;
-	archdata->num_utlbs = num_utlbs;
-	dev->archdata.iommu = archdata;
 
 	/*
 	 * Create the ARM mapping, used by the ARM DMA mapping core to allocate
@@ -736,6 +752,8 @@ static int ipmmu_add_device(struct devic
 	 * - Make the mapping size configurable ? We currently use a 2GB mapping
 	 *   at a 1GB offset to ensure that NULL VAs will fault.
 	 */
+	archdata = dev->archdata.iommu;
+	mmu = archdata->mmu;
 	if (!mmu->mapping) {
 		struct dma_iommu_mapping *mapping;
 
@@ -760,10 +778,8 @@ static int ipmmu_add_device(struct devic
 	return 0;
 
 error:
-	arm_iommu_release_mapping(mmu->mapping);
-
-	kfree(dev->archdata.iommu);
-	kfree(utlbs);
+	if (mmu)
+		arm_iommu_release_mapping(mmu->mapping);
 
 	dev->archdata.iommu = NULL;
 

[PATCH v6 04/07] iommu/ipmmu-vmsa: Break out domain allocation code

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Break out the domain allocation code into a separate function.
This is preparation for future code sharing.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---

 Changes since V5:
 - None

 Changes since V4:
 - None

 Changes since V3:
 - None

 Changes since V2:
 - Included this new patch as-is from the following series:
   [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update

 drivers/iommu/ipmmu-vmsa.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

--- 0008/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:56:59.220607110 +0900
@@ -507,13 +507,10 @@ static irqreturn_t ipmmu_irq(int irq, vo
  * IOMMU Operations
  */
 
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
 {
 	struct ipmmu_vmsa_domain *domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
@@ -523,6 +520,14 @@ static struct iommu_domain *ipmmu_domain
 	return &domain->io_domain;
 }
 
+static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+{
+	if (type != IOMMU_DOMAIN_UNMANAGED)
+		return NULL;
+
+	return __ipmmu_domain_alloc(type);
+}
+
 static void ipmmu_domain_free(struct iommu_domain *io_domain)
 {
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);

[PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Introduce an alternative set of iommu_ops suitable for 64-bit ARM
as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
Kconfig to depend on ARM or IOMMU_DMA.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V5:
 - Made domain allocation/free code more consistent - thanks Joerg!

 Changes since V4:
 - Added Kconfig hunk to depend on ARM or IOMMU_DMA

 Changes since V3:
 - Removed group parameter from ipmmu_init_platform_device()

 Changes since V2:
 - Included this new patch from the following series:
   [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
 - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
 - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
 - of_xlate() is now used without #ifdefs
 - Made sure code compiles on both 32-bit and 64-bit ARM.

 drivers/iommu/Kconfig      |    1 
 drivers/iommu/ipmmu-vmsa.c |  122 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 115 insertions(+), 8 deletions(-)

--- 0001/drivers/iommu/Kconfig
+++ work/drivers/iommu/Kconfig	2016-10-20 08:16:40.980607110 +0900
@@ -274,6 +274,7 @@ config EXYNOS_IOMMU_DEBUG
 
 config IPMMU_VMSA
 	bool "Renesas VMSA-compatible IPMMU"
+	depends on ARM || IOMMU_DMA
 	depends on ARM_LPAE
 	depends on ARCH_RENESAS || COMPILE_TEST
 	select IOMMU_API
--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-10-20 08:16:48.440607110 +0900
@@ -10,6 +10,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/delay.h>
+#include <linux/dma-iommu.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/export.h>
@@ -22,8 +23,10 @@
 #include <linux/sizes.h>
 #include <linux/slab.h>
 
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 #include <asm/dma-iommu.h>
 #include <asm/pgalloc.h>
+#endif
 
 #include "io-pgtable.h"
 
@@ -520,14 +523,6 @@ static struct iommu_domain *__ipmmu_doma
 	return &domain->io_domain;
 }
 
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
-	return __ipmmu_domain_alloc(type);
-}
-
 static void ipmmu_domain_free(struct iommu_domain *io_domain)
 {
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
@@ -714,6 +709,8 @@ error:
 	return ret;
 }
 
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
+
 static int ipmmu_add_device(struct device *dev)
 {
 	struct ipmmu_vmsa_archdata *archdata;
@@ -807,6 +804,14 @@ static void ipmmu_remove_device(struct d
 	dev->archdata.iommu = NULL;
 }
 
+static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+{
+	if (type != IOMMU_DOMAIN_UNMANAGED)
+		return NULL;
+
+	return __ipmmu_domain_alloc(type);
+}
+
 static const struct iommu_ops ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc,
 	.domain_free = ipmmu_domain_free,
@@ -821,6 +826,105 @@ static const struct iommu_ops ipmmu_ops
 	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
 };
 
+#endif /* !CONFIG_ARM && CONFIG_IOMMU_DMA */
+
+#ifdef CONFIG_IOMMU_DMA
+
+static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
+{
+	struct iommu_domain *io_domain = NULL;
+
+	switch (type) {
+	case IOMMU_DOMAIN_UNMANAGED:
+		io_domain = __ipmmu_domain_alloc(type);
+		break;
+
+	case IOMMU_DOMAIN_DMA:
+		io_domain = __ipmmu_domain_alloc(type);
+		if (io_domain)
+			iommu_get_dma_cookie(io_domain);
+		break;
+	}
+
+	return io_domain;
+}
+
+static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
+{
+	switch (io_domain->type) {
+	case IOMMU_DOMAIN_DMA:
+		iommu_put_dma_cookie(io_domain);
+		/* fall-through */
+	default:
+		ipmmu_domain_free(io_domain);
+		break;
+	}
+}
+
+static int ipmmu_add_device_dma(struct device *dev)
+{
+	struct iommu_group *group;
+
+	/* only accept devices with iommus property */
+	if (of_count_phandle_with_args(dev->of_node, "iommus",
+				       "#iommu-cells") < 0)
+		return -ENODEV;
+
+	group = iommu_group_get_for_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	return 0;
+}
+
+static void ipmmu_remove_device_dma(struct device *dev)
+{
+	iommu_group_remove_device(dev);
+}
+
+static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
+{
+	struct iommu_group *group;
+	int ret;
+
+	group = generic_device_group(dev);
+	if (IS_ERR(group))
+		return group;
+
+	ret = ipmmu_init_platform_device(dev);
+	if (ret) {
+		iommu_group_put(group);
+		group = ERR_PTR(ret);
+	}
+
+	return group;
+}
+
+static int ipmmu_of_xlate_dma(struct device *dev,
+			      struct of_phandle_args *spec)
+{
+	/* dummy callback to satisfy of_iommu_configure() */
+	return 0;
+}
+
+static const struct iommu_ops ipmmu_ops = {
+	.domain_alloc = ipmmu_domain_alloc_dma,
+	.domain_free = ipmmu_domain_free_dma,
+	.attach_dev = ipmmu_attach_device,
+	.detach_dev = ipmmu_detach_device,
+	.map = ipmmu_map,
+	.unmap = ipmmu_unmap,
+	.map_sg = default_iommu_map_sg,
+	.iova_to_phys = ipmmu_iova_to_phys,
+	.add_device = ipmmu_add_device_dma,
+	.remove_device = ipmmu_remove_device_dma,
+	.device_group = ipmmu_device_group_dma,
+	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
+	.of_xlate = ipmmu_of_xlate_dma,
+};
+
+#endif /* CONFIG_IOMMU_DMA */
+
 /* -----------------------------------------------------------------------------
  * Probe/remove and init
  */
@@ -910,7 +1014,9 @@ static int ipmmu_remove(struct platform_
 	list_del(&mmu->list);
 	spin_unlock(&ipmmu_devices_lock);
 
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 	arm_iommu_release_mapping(mmu->mapping);
+#endif
 
 	ipmmu_device_reset(mmu);
 

[PATCH v6 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Not all architectures have an iommu member in their archdata, so
use #ifdefs support build wit COMPILE_TEST on any architecture.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---

 Changes since V5:
 - None

 Changes since V4:
 - None

 Changes since V3:
 - New patch

 drivers/iommu/ipmmu-vmsa.c |   37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

--- 0012/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:59:21.690607110 +0900
@@ -70,6 +70,25 @@ static struct ipmmu_vmsa_domain *to_vmsa
 	return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
 }
 
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
+{
+	return dev->archdata.iommu;
+}
+static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p)
+{
+	dev->archdata.iommu = p;
+}
+#else
+static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
+{
+	return NULL;
+}
+static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p)
+{
+}
+#endif
+
 #define TLB_LOOP_TIMEOUT		100	/* 100us */
 
 /* -----------------------------------------------------------------------------
@@ -539,7 +558,7 @@ static void ipmmu_domain_free(struct iom
 static int ipmmu_attach_device(struct iommu_domain *io_domain,
 			       struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
 	struct ipmmu_vmsa_device *mmu = archdata->mmu;
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned long flags;
@@ -581,7 +600,7 @@ static int ipmmu_attach_device(struct io
 static void ipmmu_detach_device(struct iommu_domain *io_domain,
 				struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned int i;
 
@@ -701,7 +720,7 @@ static int ipmmu_init_platform_device(st
 	archdata->mmu = mmu;
 	archdata->utlbs = utlbs;
 	archdata->num_utlbs = num_utlbs;
-	dev->archdata.iommu = archdata;
+	set_archdata(dev, archdata);
 	return 0;
 
 error:
@@ -713,12 +732,11 @@ error:
 
 static int ipmmu_add_device(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata;
 	struct ipmmu_vmsa_device *mmu = NULL;
 	struct iommu_group *group;
 	int ret;
 
-	if (dev->archdata.iommu) {
+	if (to_archdata(dev)) {
 		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
 			 dev_name(dev));
 		return -EINVAL;
@@ -754,8 +772,7 @@ static int ipmmu_add_device(struct devic
 	 * - Make the mapping size configurable ? We currently use a 2GB mapping
 	 *   at a 1GB offset to ensure that NULL VAs will fault.
 	 */
-	archdata = dev->archdata.iommu;
-	mmu = archdata->mmu;
+	mmu = to_archdata(dev)->mmu;
 	if (!mmu->mapping) {
 		struct dma_iommu_mapping *mapping;
 
@@ -783,7 +800,7 @@ error:
 	if (mmu)
 		arm_iommu_release_mapping(mmu->mapping);
 
-	dev->archdata.iommu = NULL;
+	set_archdata(dev, NULL);
 
 	if (!IS_ERR_OR_NULL(group))
 		iommu_group_remove_device(dev);
@@ -793,7 +810,7 @@ error:
 
 static void ipmmu_remove_device(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
 
 	arm_iommu_detach_device(dev);
 	iommu_group_remove_device(dev);
@@ -801,7 +818,7 @@ static void ipmmu_remove_device(struct d
 	kfree(archdata->utlbs);
 	kfree(archdata);
 
-	dev->archdata.iommu = NULL;
+	set_archdata(dev, NULL);
 }
 
 static struct iommu_domain *ipmmu_domain_alloc(unsigned type)

[PATCH v6 07/07] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Neither the ARM page table code enabled by IOMMU_IO_PGTABLE_LPAE
nor the IPMMU_VMSA driver actually depends on ARM_LPAE, so get
rid of the dependency.

Tested with ipmmu-vmsa on r8a7794 ALT and a kernel config using:
 # CONFIG_ARM_LPAE is not set

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Joerg Roedel <jroedel@suse.de>
---

 Changes since V5:
 - None

 Changes since V4:
 - Rebased patch to fit on top of earlier Kconfig changes in series

 Changes since V3:
 - None

 Changes since V2:
 - None

 Changes since V1:
 - Rebased on top of ARCH_RENESAS change
 - Added Acked-by from Laurent

 drivers/iommu/Kconfig |    1 -
 1 file changed, 1 deletion(-)

--- 0008/drivers/iommu/Kconfig
+++ work/drivers/iommu/Kconfig	2016-09-20 22:13:03.500607110 +0900
@@ -275,7 +275,6 @@ config EXYNOS_IOMMU_DEBUG
 config IPMMU_VMSA
 	bool "Renesas VMSA-compatible IPMMU"
 	depends on ARM || IOMMU_DMA
-	depends on ARM_LPAE
 	depends on ARCH_RENESAS || COMPILE_TEST
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE

Re: [PATCH v6 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access

[Robin Murphy]

Hi Magnus,

On 20/10/16 00:36, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Not all architectures have an iommu member in their archdata, so
> use #ifdefs support build wit COMPILE_TEST on any architecture.

As an alternative to this we could now use iommu_fwspec in place of the
custom ipmmu_vmsa_archdata - that's deliberately
architecture-independent. I converted the Mediatek drivers[1] as an
example to stand separately from the big SMMU rework, as those are the
ones I'm most familiar with, but it looks like the IPMMU is a similarly
perfect fit.

Of course, that could always come afterwards as a cleanup - I wouldn't
consider it a blocker at this point - but it does look like it might
simplify some of the code being moved around in this series if it were
to be done beforehand.

Robin.

[1]:http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14577.html

> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
> 
>  Changes since V5:
>  - None
> 
>  Changes since V4:
>  - None
> 
>  Changes since V3:
>  - New patch
> 
>  drivers/iommu/ipmmu-vmsa.c |   37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> --- 0012/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:59:21.690607110 +0900
> @@ -70,6 +70,25 @@ static struct ipmmu_vmsa_domain *to_vmsa
>  	return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
>  }
>  
> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
> +{
> +	return dev->archdata.iommu;
> +}
> +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p)
> +{
> +	dev->archdata.iommu = p;
> +}
> +#else
> +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
> +{
> +	return NULL;
> +}
> +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p)
> +{
> +}
> +#endif
> +
>  #define TLB_LOOP_TIMEOUT		100	/* 100us */
>  
>  /* -----------------------------------------------------------------------------
> @@ -539,7 +558,7 @@ static void ipmmu_domain_free(struct iom
>  static int ipmmu_attach_device(struct iommu_domain *io_domain,
>  			       struct device *dev)
>  {
> -	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> +	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
>  	struct ipmmu_vmsa_device *mmu = archdata->mmu;
>  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
>  	unsigned long flags;
> @@ -581,7 +600,7 @@ static int ipmmu_attach_device(struct io
>  static void ipmmu_detach_device(struct iommu_domain *io_domain,
>  				struct device *dev)
>  {
> -	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> +	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
>  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
>  	unsigned int i;
>  
> @@ -701,7 +720,7 @@ static int ipmmu_init_platform_device(st
>  	archdata->mmu = mmu;
>  	archdata->utlbs = utlbs;
>  	archdata->num_utlbs = num_utlbs;
> -	dev->archdata.iommu = archdata;
> +	set_archdata(dev, archdata);
>  	return 0;
>  
>  error:
> @@ -713,12 +732,11 @@ error:
>  
>  static int ipmmu_add_device(struct device *dev)
>  {
> -	struct ipmmu_vmsa_archdata *archdata;
>  	struct ipmmu_vmsa_device *mmu = NULL;
>  	struct iommu_group *group;
>  	int ret;
>  
> -	if (dev->archdata.iommu) {
> +	if (to_archdata(dev)) {
>  		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
>  			 dev_name(dev));
>  		return -EINVAL;
> @@ -754,8 +772,7 @@ static int ipmmu_add_device(struct devic
>  	 * - Make the mapping size configurable ? We currently use a 2GB mapping
>  	 *   at a 1GB offset to ensure that NULL VAs will fault.
>  	 */
> -	archdata = dev->archdata.iommu;
> -	mmu = archdata->mmu;
> +	mmu = to_archdata(dev)->mmu;
>  	if (!mmu->mapping) {
>  		struct dma_iommu_mapping *mapping;
>  
> @@ -783,7 +800,7 @@ error:
>  	if (mmu)
>  		arm_iommu_release_mapping(mmu->mapping);
>  
> -	dev->archdata.iommu = NULL;
> +	set_archdata(dev, NULL);
>  
>  	if (!IS_ERR_OR_NULL(group))
>  		iommu_group_remove_device(dev);
> @@ -793,7 +810,7 @@ error:
>  
>  static void ipmmu_remove_device(struct device *dev)
>  {
> -	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> +	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
>  
>  	arm_iommu_detach_device(dev);
>  	iommu_group_remove_device(dev);
> @@ -801,7 +818,7 @@ static void ipmmu_remove_device(struct d
>  	kfree(archdata->utlbs);
>  	kfree(archdata);
>  
> -	dev->archdata.iommu = NULL;
> +	set_archdata(dev, NULL);
>  }
>  
>  static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> 

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Robin Murphy]

On 20/10/16 00:36, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Introduce an alternative set of iommu_ops suitable for 64-bit ARM
> as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
> Kconfig to depend on ARM or IOMMU_DMA.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Changes since V5:
>  - Made domain allocation/free code more consistent - thanks Joerg!
> 
>  Changes since V4:
>  - Added Kconfig hunk to depend on ARM or IOMMU_DMA
> 
>  Changes since V3:
>  - Removed group parameter from ipmmu_init_platform_device()
> 
>  Changes since V2:
>  - Included this new patch from the following series:
>    [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
>  - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
>  - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
>  - of_xlate() is now used without #ifdefs
>  - Made sure code compiles on both 32-bit and 64-bit ARM.
> 
>  drivers/iommu/Kconfig      |    1 
>  drivers/iommu/ipmmu-vmsa.c |  122 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 115 insertions(+), 8 deletions(-)
> 
> --- 0001/drivers/iommu/Kconfig
> +++ work/drivers/iommu/Kconfig	2016-10-20 08:16:40.980607110 +0900
> @@ -274,6 +274,7 @@ config EXYNOS_IOMMU_DEBUG
>  
>  config IPMMU_VMSA
>  	bool "Renesas VMSA-compatible IPMMU"
> +	depends on ARM || IOMMU_DMA
>  	depends on ARM_LPAE
>  	depends on ARCH_RENESAS || COMPILE_TEST
>  	select IOMMU_API
> --- 0006/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-10-20 08:16:48.440607110 +0900
> @@ -10,6 +10,7 @@
>  
>  #include <linux/bitmap.h>
>  #include <linux/delay.h>
> +#include <linux/dma-iommu.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
> @@ -22,8 +23,10 @@
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
>  
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
>  #include <asm/dma-iommu.h>
>  #include <asm/pgalloc.h>
> +#endif
>  
>  #include "io-pgtable.h"
>  
> @@ -520,14 +523,6 @@ static struct iommu_domain *__ipmmu_doma
>  	return &domain->io_domain;
>  }
>  
> -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> -{
> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> -		return NULL;

I *think* that if we did the initial check thus:

	if (type != IOMMU_DOMAIN_UNMANAGED ||
	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
		return NULL;

it shouldn't be necessary to split the function at all - we then just
wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
in the 32-bit ARM case they just don't run as that can never be true.

> -
> -	return __ipmmu_domain_alloc(type);
> -}
> -
>  static void ipmmu_domain_free(struct iommu_domain *io_domain)
>  {
>  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> @@ -714,6 +709,8 @@ error:
>  	return ret;
>  }
>  
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> +
>  static int ipmmu_add_device(struct device *dev)
>  {
>  	struct ipmmu_vmsa_archdata *archdata;
> @@ -807,6 +804,14 @@ static void ipmmu_remove_device(struct d
>  	dev->archdata.iommu = NULL;
>  }
>  
> +static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> +{
> +	if (type != IOMMU_DOMAIN_UNMANAGED)
> +		return NULL;
> +
> +	return __ipmmu_domain_alloc(type);
> +}
> +
>  static const struct iommu_ops ipmmu_ops = {
>  	.domain_alloc = ipmmu_domain_alloc,
>  	.domain_free = ipmmu_domain_free,
> @@ -821,6 +826,105 @@ static const struct iommu_ops ipmmu_ops
>  	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
>  };
>  
> +#endif /* !CONFIG_ARM && CONFIG_IOMMU_DMA */
> +
> +#ifdef CONFIG_IOMMU_DMA
> +
> +static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
> +{
> +	struct iommu_domain *io_domain = NULL;
> +
> +	switch (type) {
> +	case IOMMU_DOMAIN_UNMANAGED:
> +		io_domain = __ipmmu_domain_alloc(type);
> +		break;
> +
> +	case IOMMU_DOMAIN_DMA:
> +		io_domain = __ipmmu_domain_alloc(type);
> +		if (io_domain)
> +			iommu_get_dma_cookie(io_domain);

Either way, this can fail (in fact if !CONFIG_IOMMU_DMA it can be relied
upon to).

Robin.

> +		break;
> +	}
> +
> +	return io_domain;
> +}
> +
> +static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
> +{
> +	switch (io_domain->type) {
> +	case IOMMU_DOMAIN_DMA:
> +		iommu_put_dma_cookie(io_domain);
> +		/* fall-through */
> +	default:
> +		ipmmu_domain_free(io_domain);
> +		break;
> +	}
> +}
> +
> +static int ipmmu_add_device_dma(struct device *dev)
> +{
> +	struct iommu_group *group;
> +
> +	/* only accept devices with iommus property */
> +	if (of_count_phandle_with_args(dev->of_node, "iommus",
> +				       "#iommu-cells") < 0)
> +		return -ENODEV;
> +
> +	group = iommu_group_get_for_dev(dev);
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
> +
> +	return 0;
> +}
> +
> +static void ipmmu_remove_device_dma(struct device *dev)
> +{
> +	iommu_group_remove_device(dev);
> +}
> +
> +static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
> +{
> +	struct iommu_group *group;
> +	int ret;
> +
> +	group = generic_device_group(dev);
> +	if (IS_ERR(group))
> +		return group;
> +
> +	ret = ipmmu_init_platform_device(dev);
> +	if (ret) {
> +		iommu_group_put(group);
> +		group = ERR_PTR(ret);
> +	}
> +
> +	return group;
> +}
> +
> +static int ipmmu_of_xlate_dma(struct device *dev,
> +			      struct of_phandle_args *spec)
> +{
> +	/* dummy callback to satisfy of_iommu_configure() */
> +	return 0;
> +}
> +
> +static const struct iommu_ops ipmmu_ops = {
> +	.domain_alloc = ipmmu_domain_alloc_dma,
> +	.domain_free = ipmmu_domain_free_dma,
> +	.attach_dev = ipmmu_attach_device,
> +	.detach_dev = ipmmu_detach_device,
> +	.map = ipmmu_map,
> +	.unmap = ipmmu_unmap,
> +	.map_sg = default_iommu_map_sg,
> +	.iova_to_phys = ipmmu_iova_to_phys,
> +	.add_device = ipmmu_add_device_dma,
> +	.remove_device = ipmmu_remove_device_dma,
> +	.device_group = ipmmu_device_group_dma,
> +	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
> +	.of_xlate = ipmmu_of_xlate_dma,
> +};
> +
> +#endif /* CONFIG_IOMMU_DMA */
> +
>  /* -----------------------------------------------------------------------------
>   * Probe/remove and init
>   */
> @@ -910,7 +1014,9 @@ static int ipmmu_remove(struct platform_
>  	list_del(&mmu->list);
>  	spin_unlock(&ipmmu_devices_lock);
>  
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
>  	arm_iommu_release_mapping(mmu->mapping);
> +#endif
>  
>  	ipmmu_device_reset(mmu);
>  
> 

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Joerg Roedel]

On Fri, Oct 21, 2016 at 06:52:53PM +0100, Robin Murphy wrote:
> > -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> > -{
> > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > -		return NULL;
> 
> I *think* that if we did the initial check thus:
> 
> 	if (type != IOMMU_DOMAIN_UNMANAGED ||
> 	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> 		return NULL;
> 
> it shouldn't be necessary to split the function at all - we then just
> wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
> in the 32-bit ARM case they just don't run as that can never be true.

This would be a good improvement. Magnus, Robin, can either of you send
a follow-on patch to implement this suggestion? I have applied these
patches to my arm/renesas branch (not pushed yet). The patch can be
based on it.


	Joerg

Re: [PATCH v6 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V6

[Joerg Roedel]

On Thu, Oct 20, 2016 at 08:35:33AM +0900, Magnus Damm wrote:
> iommu/ipmmu-vmsa: IPMMU multi-arch update V6
> 
> [PATCH v6 01/07] iommu/ipmmu-vmsa: Remove platform data handling
> [PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context
> [PATCH v6 03/07] iommu/ipmmu-vmsa: Break out utlb parsing code
> [PATCH v6 04/07] iommu/ipmmu-vmsa: Break out domain allocation code
> [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
> [PATCH v6 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access
> [PATCH v6 07/07] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency

Applied to arm/renesas, thanks.

Re: [PATCH v6 02/07] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Thursday 20 Oct 2016 08:35:53 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Introduce a bitmap for context handing and convert the
> interrupt routine to handle all registered contexts.
> 
> At this point the number of contexts are still limited.
> 
> Also remove the use of the ARM specific mapping variable
> from ipmmu_irq() to allow compile on ARM64.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
> 
>  Changes since V5:
>  - None
> 
>  Changes since V4:
>  - None
> 
>  Changes since V3:
>  - None
> 
>  Changes since V2: (Thanks again to Laurent!)
>  - Introduce a spinlock together with the bitmap and domain array.
>  - Break out code into separate functions for alloc and free.
>  - Perform free after (instead of before) configuring hardware registers.
>  - Use the spinlock to protect the domain array in the interrupt handler.
> 
>  Changes since V1: (Thanks to Laurent for feedback!)
>  - Use simple find_first_zero()/set_bit()/clear_bit() for context
> management. - For allocation rely on spinlock held when calling
> ipmmu_domain_init_context() - For test/free use atomic bitops
>  - Return IRQ_HANDLED if any of the contexts generated interrupts
> 
>  drivers/iommu/ipmmu-vmsa.c |   76 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 66 insertions(+), 10 deletions(-)
> 
> --- 0004/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:48:23.770607110 +0900
> @@ -8,6 +8,7 @@
>   * the Free Software Foundation; version 2 of the License.
>   */
> 
> +#include <linux/bitmap.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/err.h>
> @@ -26,12 +27,17 @@
> 
>  #include "io-pgtable.h"
> 
> +#define IPMMU_CTX_MAX 1
> +
>  struct ipmmu_vmsa_device {
>  	struct device *dev;
>  	void __iomem *base;
>  	struct list_head list;
> 
>  	unsigned int num_utlbs;
> +	spinlock_t lock;			/* Protects ctx and domains[] 
*/
> +	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> +	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> 
>  	struct dma_iommu_mapping *mapping;
>  };
> @@ -293,9 +299,29 @@ static struct iommu_gather_ops ipmmu_gat
>   * Domain/Context Management
>   */
> 
> +static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
> +					 struct ipmmu_vmsa_domain *domain)

Nitpicking, I'd name this ipmmu_domain_alloc_context() as the driver uses the 
alloc abbreviation already.

> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&mmu->lock, flags);
> +
> +	ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
> +	if (ret != IPMMU_CTX_MAX) {
> +		mmu->domains[ret] = domain;
> +		set_bit(ret, mmu->ctx);
> +	}

How about returning a negative error code on error instead of IPMMU_CTX_MAX ? 
I think it would make the API clearer, avoiding the need to think about 
special error handling for this function.

Having said that, I find that the init/alloc and destroy/free function names 
don't carry a very clear semantic. Given the size of the alloc and free 
functions, how about inlining them in their single caller ?

> +
> +	spin_unlock_irqrestore(&mmu->lock, flags);
> +
> +	return ret;
> +}
> +
>  static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>  {
>  	u64 ttbr;
> +	int ret;
> 
>  	/*
>  	 * Allocate the page table operations.
> @@ -325,10 +351,15 @@ static int ipmmu_domain_init_context(str
>  		return -EINVAL;
> 
>  	/*
> -	 * TODO: When adding support for multiple contexts, find an unused
> -	 * context.
> +	 * Find an unused context.
>  	 */

The comment now holds on one line.

> -	domain->context_id = 0;
> +	ret = ipmmu_domain_allocate_context(domain->mmu, domain);
> +	if (ret == IPMMU_CTX_MAX) {
> +		free_io_pgtable_ops(domain->iop);
> +		return -EBUSY;
> +	}
> +
> +	domain->context_id = ret;
> 
>  	/* TTBR0 */
>  	ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> @@ -370,6 +401,19 @@ static int ipmmu_domain_init_context(str
>  	return 0;
>  }
> 
> +static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
> +				      unsigned int context_id)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mmu->lock, flags);
> +
> +	clear_bit(context_id, mmu->ctx);
> +	mmu->domains[context_id] = NULL;
> +
> +	spin_unlock_irqrestore(&mmu->lock, flags);
> +}
> +
>  static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>  {
>  	/*
> @@ -380,6 +424,7 @@ static void ipmmu_domain_destroy_context
>  	 */
>  	ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
>  	ipmmu_tlb_sync(domain);
> +	ipmmu_domain_free_context(domain->mmu, domain->context_id);
>  }
> 
>  /*
> ---------------------------------------------------------------------------
> -- @@ -437,16 +482,25 @@ static irqreturn_t ipmmu_domain_irq(stru
>  static irqreturn_t ipmmu_irq(int irq, void *dev)
>  {
>  	struct ipmmu_vmsa_device *mmu = dev;
> -	struct iommu_domain *io_domain;
> -	struct ipmmu_vmsa_domain *domain;
> +	irqreturn_t status = IRQ_NONE;
> +	unsigned int i;
> +	unsigned long flags;

Nitpicking again I like to arrange variable declarations by decreasing line 
length when there's no reason not to :-)

> -	if (!mmu->mapping)
> -		return IRQ_NONE;
> +	spin_lock_irqsave(&mmu->lock, flags);
> +
> +	/*
> +	 * Check interrupts for all active contexts.
> +	 */

This comment holds on a single line too.

With all these small comments addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	for (i = 0; i < IPMMU_CTX_MAX; i++) {
> +		if (!mmu->domains[i])
> +			continue;
> +		if (ipmmu_domain_irq(mmu->domains[i]) == IRQ_HANDLED)
> +			status = IRQ_HANDLED;
> +	}
> 
> -	io_domain = mmu->mapping->domain;
> -	domain = to_vmsa_domain(io_domain);
> +	spin_unlock_irqrestore(&mmu->lock, flags);
> 
> -	return ipmmu_domain_irq(domain);
> +	return status;
>  }
> 
>  /*
> ---------------------------------------------------------------------------
> -- @@ -774,6 +828,8 @@ static int ipmmu_probe(struct platform_d
> 
>  	mmu->dev = &pdev->dev;
>  	mmu->num_utlbs = 32;
> +	spin_lock_init(&mmu->lock);
> +	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
> 
>  	/* Map I/O memory and request IRQ. */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 03/07] iommu/ipmmu-vmsa: Break out utlb parsing code

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Thursday 20 Oct 2016 08:36:03 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Break out the utlb parsing code and dev_data allocation into a
> separate function. This is preparation for future code sharing.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> ---
> 
>  Changes since V5:
>  - None
> 
>  Changes since V4:
>  - Dropped hunk with fix to apply on top of:
>    b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device
> 
>  Changes since V3:
>  - Initialize "mmu" to NULL, check before accessing
>  - Removed group parameter from ipmmu_init_platform_device()
> 
>  Changes since V2:
>  - Included this new patch from the following series:
>    [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
>  - Reworked code to fit on top on previous two patches in current series.
> 
>  drivers/iommu/ipmmu-vmsa.c |   58 ++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 21 deletions(-)
> 
> --- 0006/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:49:34.580607110 +0900
> @@ -647,22 +647,15 @@ static int ipmmu_find_utlbs(struct ipmmu
>  	return 0;
>  }
> 
> -static int ipmmu_add_device(struct device *dev)
> +static int ipmmu_init_platform_device(struct device *dev)

The device doesn't have to be a platform device, how about calling this 
ipmmu_init_device_archdata() ?

>  {
>  	struct ipmmu_vmsa_archdata *archdata;
>  	struct ipmmu_vmsa_device *mmu;
> -	struct iommu_group *group = NULL;
>  	unsigned int *utlbs;
>  	unsigned int i;
>  	int num_utlbs;
>  	int ret = -ENODEV;
> 
> -	if (dev->archdata.iommu) {
> -		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
> -			 dev_name(dev));
> -		return -EINVAL;
> -	}
> -
>  	/* Find the master corresponding to the device. */
> 
>  	num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
> @@ -699,6 +692,36 @@ static int ipmmu_add_device(struct devic
>  		}
>  	}
> 
> +	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
> +	if (!archdata) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	archdata->mmu = mmu;
> +	archdata->utlbs = utlbs;
> +	archdata->num_utlbs = num_utlbs;
> +	dev->archdata.iommu = archdata;
> +	return 0;
> +
> +error:
> +	kfree(utlbs);
> +	return ret;
> +}
> +
> +static int ipmmu_add_device(struct device *dev)
> +{
> +	struct ipmmu_vmsa_archdata *archdata;
> +	struct ipmmu_vmsa_device *mmu = NULL;
> +	struct iommu_group *group;
> +	int ret;
> +
> +	if (dev->archdata.iommu) {
> +		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
> +			 dev_name(dev));
> +		return -EINVAL;
> +	}
> +
>  	/* Create a device group and add the device to it. */
>  	group = iommu_group_alloc();
>  	if (IS_ERR(group)) {
> @@ -716,16 +739,9 @@ static int ipmmu_add_device(struct devic
>  		goto error;
>  	}
> 
> -	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
> -	if (!archdata) {
> -		ret = -ENOMEM;
> +	ret = ipmmu_init_platform_device(dev);
> +	if (ret < 0)
>  		goto error;
> -	}
> -
> -	archdata->mmu = mmu;
> -	archdata->utlbs = utlbs;
> -	archdata->num_utlbs = num_utlbs;
> -	dev->archdata.iommu = archdata;
> 
>  	/*
>  	 * Create the ARM mapping, used by the ARM DMA mapping core to 
allocate
> @@ -736,6 +752,8 @@ static int ipmmu_add_device(struct devic
>  	 * - Make the mapping size configurable ? We currently use a 2GB 
mapping
>  	 *   at a 1GB offset to ensure that NULL VAs will fault.
>  	 */
> +	archdata = dev->archdata.iommu;
> +	mmu = archdata->mmu;
>  	if (!mmu->mapping) {
>  		struct dma_iommu_mapping *mapping;
> 
> @@ -760,10 +778,8 @@ static int ipmmu_add_device(struct devic
>  	return 0;
> 
>  error:
> -	arm_iommu_release_mapping(mmu->mapping);
> -
> -	kfree(dev->archdata.iommu);
> -	kfree(utlbs);
> +	if (mmu)
> +		arm_iommu_release_mapping(mmu->mapping);

Looking at the surrounding code, shouldn't the mapping only be destroyed here 
if it has been created by the same call to the function ? If there's an 
existing mapping for the IPMMU instance and the arm_iommu_attach_device() call 
fails, releasing the mapping seems to be a bug. As the bug isn't introduced by 
this patch, we can fix it separately, but if you end up resubmitting due to 
the first comment you could also add a fix.

> 
>  	dev->archdata.iommu = NULL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 04/07] iommu/ipmmu-vmsa: Break out domain allocation code

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Thursday 20 Oct 2016 08:36:13 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Break out the domain allocation code into a separate function.
> This is preparation for future code sharing.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> Reviewed-by: Joerg Roedel <jroedel@suse.de>

This looks good to me,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

(assuming my review of the next patches in the series won't make me conclude 
that this patch isn't needed :-))

> ---
> 
>  Changes since V5:
>  - None
> 
>  Changes since V4:
>  - None
> 
>  Changes since V3:
>  - None
> 
>  Changes since V2:
>  - Included this new patch as-is from the following series:
>    [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> 
>  drivers/iommu/ipmmu-vmsa.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> --- 0008/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:56:59.220607110 +0900
> @@ -507,13 +507,10 @@ static irqreturn_t ipmmu_irq(int irq, vo
>   * IOMMU Operations
>   */
> 
> -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> +static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
>  {
>  	struct ipmmu_vmsa_domain *domain;
> 
> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> -		return NULL;
> -
>  	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>  	if (!domain)
>  		return NULL;
> @@ -523,6 +520,14 @@ static struct iommu_domain *ipmmu_domain
>  	return &domain->io_domain;
>  }
> 
> +static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> +{
> +	if (type != IOMMU_DOMAIN_UNMANAGED)
> +		return NULL;
> +
> +	return __ipmmu_domain_alloc(type);
> +}
> +
>  static void ipmmu_domain_free(struct iommu_domain *io_domain)
>  {
>  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Laurent Pinchart]

Hello,

On Thursday 10 Nov 2016 12:42:06 Joerg Roedel wrote:
> On Fri, Oct 21, 2016 at 06:52:53PM +0100, Robin Murphy wrote:
> > > -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> > > -{
> > > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > > -		return NULL;
> > 
> > I *think* that if we did the initial check thus:
> > 	if (type != IOMMU_DOMAIN_UNMANAGED ||
> > 	
> > 	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> > 		
> > 		return NULL;
> > 
> > it shouldn't be necessary to split the function at all - we then just
> > wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
> > in the 32-bit ARM case they just don't run as that can never be true.
> 
> This would be a good improvement. Magnus, Robin, can either of you send
> a follow-on patch to implement this suggestion? I have applied these
> patches to my arm/renesas branch (not pushed yet). The patch can be
> based on it.

I like the suggestion too, a patch is on its way.

Joerg, as I've sent a few comments about the other patches (sorry for the late 
review, I got delayed by KS and LPC), the follow-up patch should probably be 
squashed into this one when Magnus addresses my comments. Could you please 
hold off pushing the arm/renesas branch until Magnus replies to this ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Thursday 20 Oct 2016 08:36:23 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Introduce an alternative set of iommu_ops suitable for 64-bit ARM
> as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
> Kconfig to depend on ARM or IOMMU_DMA.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Changes since V5:
>  - Made domain allocation/free code more consistent - thanks Joerg!
> 
>  Changes since V4:
>  - Added Kconfig hunk to depend on ARM or IOMMU_DMA
> 
>  Changes since V3:
>  - Removed group parameter from ipmmu_init_platform_device()
> 
>  Changes since V2:
>  - Included this new patch from the following series:
>    [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
>  - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
>  - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
>  - of_xlate() is now used without #ifdefs
>  - Made sure code compiles on both 32-bit and 64-bit ARM.
> 
>  drivers/iommu/Kconfig      |    1
>  drivers/iommu/ipmmu-vmsa.c |  122 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 115 insertions(+), 8 deletions(-)
> 
> --- 0001/drivers/iommu/Kconfig
> +++ work/drivers/iommu/Kconfig	2016-10-20 08:16:40.980607110 +0900
> @@ -274,6 +274,7 @@ config EXYNOS_IOMMU_DEBUG
> 
>  config IPMMU_VMSA
>  	bool "Renesas VMSA-compatible IPMMU"
> +	depends on ARM || IOMMU_DMA
>  	depends on ARM_LPAE
>  	depends on ARCH_RENESAS || COMPILE_TEST
>  	select IOMMU_API
> --- 0006/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-10-20 08:16:48.440607110 +0900
> @@ -10,6 +10,7 @@
> 
>  #include <linux/bitmap.h>
>  #include <linux/delay.h>
> +#include <linux/dma-iommu.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
> @@ -22,8 +23,10 @@
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> 
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
>  #include <asm/dma-iommu.h>
>  #include <asm/pgalloc.h>
> +#endif
> 
>  #include "io-pgtable.h"
> 
> @@ -520,14 +523,6 @@ static struct iommu_domain *__ipmmu_doma
>  	return &domain->io_domain;
>  }
> 
> -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> -{
> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> -		return NULL;
> -
> -	return __ipmmu_domain_alloc(type);
> -}
> -
>  static void ipmmu_domain_free(struct iommu_domain *io_domain)
>  {
>  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> @@ -714,6 +709,8 @@ error:
>  	return ret;
>  }
> 
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
> +
>  static int ipmmu_add_device(struct device *dev)
>  {
>  	struct ipmmu_vmsa_archdata *archdata;
> @@ -807,6 +804,14 @@ static void ipmmu_remove_device(struct d
>  	dev->archdata.iommu = NULL;
>  }
> 
> +static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> +{
> +	if (type != IOMMU_DOMAIN_UNMANAGED)
> +		return NULL;
> +
> +	return __ipmmu_domain_alloc(type);
> +}
> +
>  static const struct iommu_ops ipmmu_ops = {
>  	.domain_alloc = ipmmu_domain_alloc,
>  	.domain_free = ipmmu_domain_free,
> @@ -821,6 +826,105 @@ static const struct iommu_ops ipmmu_ops
>  	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
>  };
> 
> +#endif /* !CONFIG_ARM && CONFIG_IOMMU_DMA */
> +
> +#ifdef CONFIG_IOMMU_DMA
> +
> +static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
> +{
> +	struct iommu_domain *io_domain = NULL;
> +
> +	switch (type) {
> +	case IOMMU_DOMAIN_UNMANAGED:
> +		io_domain = __ipmmu_domain_alloc(type);
> +		break;
> +
> +	case IOMMU_DOMAIN_DMA:
> +		io_domain = __ipmmu_domain_alloc(type);
> +		if (io_domain)
> +			iommu_get_dma_cookie(io_domain);
> +		break;
> +	}
> +
> +	return io_domain;
> +}
> +
> +static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
> +{
> +	switch (io_domain->type) {
> +	case IOMMU_DOMAIN_DMA:
> +		iommu_put_dma_cookie(io_domain);
> +		/* fall-through */
> +	default:
> +		ipmmu_domain_free(io_domain);
> +		break;
> +	}
> +}
> +
> +static int ipmmu_add_device_dma(struct device *dev)
> +{
> +	struct iommu_group *group;
> +
> +	/* only accept devices with iommus property */

Nitpicking, comments in the driver are capitalized and end with a period.

> +	if (of_count_phandle_with_args(dev->of_node, "iommus",
> +				       "#iommu-cells") < 0)
> +		return -ENODEV;
> +
> +	group = iommu_group_get_for_dev(dev);
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
> +
> +	return 0;
> +}
> +
> +static void ipmmu_remove_device_dma(struct device *dev)
> +{
> +	iommu_group_remove_device(dev);
> +}
> +
> +static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
> +{
> +	struct iommu_group *group;
> +	int ret;
> +
> +	group = generic_device_group(dev);
> +	if (IS_ERR(group))
> +		return group;
> +
> +	ret = ipmmu_init_platform_device(dev);
> +	if (ret) {
> +		iommu_group_put(group);
> +		group = ERR_PTR(ret);
> +	}
> +
> +	return group;
> +}
> +
> +static int ipmmu_of_xlate_dma(struct device *dev,
> +			      struct of_phandle_args *spec)
> +{
> +	/* dummy callback to satisfy of_iommu_configure() */

This is actually where you should parse the utlbs. If you switch to the 
iommu_fwspec API as Robin proposed in his review of patch 6/7, I think you can 
use iommu_fwspec_add_ids() as a helper function for this. See the arm-smmu-v3 
driver for an example.

> +	return 0;
> +}
> +
> +static const struct iommu_ops ipmmu_ops = {
> +	.domain_alloc = ipmmu_domain_alloc_dma,
> +	.domain_free = ipmmu_domain_free_dma,
> +	.attach_dev = ipmmu_attach_device,
> +	.detach_dev = ipmmu_detach_device,
> +	.map = ipmmu_map,
> +	.unmap = ipmmu_unmap,
> +	.map_sg = default_iommu_map_sg,
> +	.iova_to_phys = ipmmu_iova_to_phys,
> +	.add_device = ipmmu_add_device_dma,
> +	.remove_device = ipmmu_remove_device_dma,
> +	.device_group = ipmmu_device_group_dma,
> +	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
> +	.of_xlate = ipmmu_of_xlate_dma,

The ARM implementation should switch to .of_xlate() too. Have you given it a 
try by any chance ?

> +};
> +
> +#endif /* CONFIG_IOMMU_DMA */
> +
>  /* ------------------------------------------------------------------------
>   * Probe/remove and init
>   */
> @@ -910,7 +1014,9 @@ static int ipmmu_remove(struct platform_
>  	list_del(&mmu->list);
>  	spin_unlock(&ipmmu_devices_lock);
> 
> +#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
>  	arm_iommu_release_mapping(mmu->mapping);
> +#endif
> 
>  	ipmmu_device_reset(mmu);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 06/07] iommu/ipmmu-vmsa: ARM and ARM64 archdata access

[Laurent Pinchart]

Hello,

On Friday 21 Oct 2016 18:32:54 Robin Murphy wrote:
> On 20/10/16 00:36, Magnus Damm wrote:
> > From: Magnus Damm <damm+renesas@opensource.se>
> > 
> > Not all architectures have an iommu member in their archdata, so
> > use #ifdefs support build wit COMPILE_TEST on any architecture.
> 
> As an alternative to this we could now use iommu_fwspec in place of the
> custom ipmmu_vmsa_archdata - that's deliberately
> architecture-independent. I converted the Mediatek drivers[1] as an
> example to stand separately from the big SMMU rework, as those are the
> ones I'm most familiar with, but it looks like the IPMMU is a similarly
> perfect fit.
> 
> Of course, that could always come afterwards as a cleanup - I wouldn't
> consider it a blocker at this point - but it does look like it might
> simplify some of the code being moved around in this series if it were
> to be done beforehand.

I like the suggestion, it looks much cleaner. It would also allow implementing 
.of_xlate() in a cleaner fashion. I don't think it needs to block this series, 
but if Magnus ends up sending a v7 to address all the other small comments, it 
would be worth a shot.

> [1]:http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14577.ht
> ml
> > Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> > Reviewed-by: Joerg Roedel <jroedel@suse.de>
> > ---
> > 
> >  Changes since V5:
> >  - None
> >  
> >  Changes since V4:
> >  - None
> >  
> >  Changes since V3:
> >  - New patch
> >  
> >  drivers/iommu/ipmmu-vmsa.c |   37 +++++++++++++++++++++++++++----------
> >  1 file changed, 27 insertions(+), 10 deletions(-)
> > 
> > --- 0012/drivers/iommu/ipmmu-vmsa.c
> > +++ work/drivers/iommu/ipmmu-vmsa.c	2016-09-20 21:59:21.690607110 +0900
> > @@ -70,6 +70,25 @@ static struct ipmmu_vmsa_domain *to_vmsa
> > 
> >  	return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
> >  
> >  }
> > 
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
> > +{
> > +	return dev->archdata.iommu;
> > +}
> > +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata
> > *p) +{
> > +	dev->archdata.iommu = p;
> > +}
> > +#else
> > +static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
> > +{
> > +	return NULL;
> > +}
> > +static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata
> > *p) +{
> > +}
> > +#endif
> > +
> > 
> >  #define TLB_LOOP_TIMEOUT		100	/* 100us */
> >  
> >  /*
> >  ------------------------------------------------------------------------
> >  -----> 
> > @@ -539,7 +558,7 @@ static void ipmmu_domain_free(struct iom
> > 
> >  static int ipmmu_attach_device(struct iommu_domain *io_domain,
> >  
> >  			       struct device *dev)
> >  
> >  {
> > 
> > -	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> > +	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
> > 
> >  	struct ipmmu_vmsa_device *mmu = archdata->mmu;
> >  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> >  	unsigned long flags;
> > 
> > @@ -581,7 +600,7 @@ static int ipmmu_attach_device(struct io
> > 
> >  static void ipmmu_detach_device(struct iommu_domain *io_domain,
> >  
> >  				struct device *dev)
> >  
> >  {
> > 
> > -	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> > +	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
> > 
> >  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> >  	unsigned int i;
> > 
> > @@ -701,7 +720,7 @@ static int ipmmu_init_platform_device(st
> > 
> >  	archdata->mmu = mmu;
> >  	archdata->utlbs = utlbs;
> >  	archdata->num_utlbs = num_utlbs;
> > 
> > -	dev->archdata.iommu = archdata;
> > +	set_archdata(dev, archdata);
> > 
> >  	return 0;
> >  
> >  error:
> > @@ -713,12 +732,11 @@ error:
> >  static int ipmmu_add_device(struct device *dev)
> >  {
> > 
> > -	struct ipmmu_vmsa_archdata *archdata;
> > 
> >  	struct ipmmu_vmsa_device *mmu = NULL;
> >  	struct iommu_group *group;
> >  	int ret;
> > 
> > -	if (dev->archdata.iommu) {
> > +	if (to_archdata(dev)) {
> > 
> >  		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
> >  		
> >  			 dev_name(dev));
> >  		
> >  		return -EINVAL;
> > 
> > @@ -754,8 +772,7 @@ static int ipmmu_add_device(struct devic
> > 
> >  	 * - Make the mapping size configurable ? We currently use a 2GB 
mapping
> >  	 *   at a 1GB offset to ensure that NULL VAs will fault.
> >  	 */
> > 
> > -	archdata = dev->archdata.iommu;
> > -	mmu = archdata->mmu;
> > +	mmu = to_archdata(dev)->mmu;
> > 
> >  	if (!mmu->mapping) {
> >  	
> >  		struct dma_iommu_mapping *mapping;
> > 
> > @@ -783,7 +800,7 @@ error:
> >  	if (mmu)
> >  	
> >  		arm_iommu_release_mapping(mmu->mapping);
> > 
> > -	dev->archdata.iommu = NULL;
> > +	set_archdata(dev, NULL);
> > 
> >  	if (!IS_ERR_OR_NULL(group))
> >  	
> >  		iommu_group_remove_device(dev);
> > 
> > @@ -793,7 +810,7 @@ error:
> >  static void ipmmu_remove_device(struct device *dev)
> >  {
> > 
> > -	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> > +	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
> > 
> >  	arm_iommu_detach_device(dev);
> >  	iommu_group_remove_device(dev);
> > 
> > @@ -801,7 +818,7 @@ static void ipmmu_remove_device(struct d
> > 
> >  	kfree(archdata->utlbs);
> >  	kfree(archdata);
> > 
> > -	dev->archdata.iommu = NULL;
> > +	set_archdata(dev, NULL);
> > 
> >  }
> >  
> >  static struct iommu_domain *ipmmu_domain_alloc(unsigned type)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Laurent Pinchart]

Hi Robin,

On Friday 21 Oct 2016 18:52:53 Robin Murphy wrote:
> On 20/10/16 00:36, Magnus Damm wrote:
> > From: Magnus Damm <damm+renesas@opensource.se>
> > 
> > Introduce an alternative set of iommu_ops suitable for 64-bit ARM
> > as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
> > Kconfig to depend on ARM or IOMMU_DMA.
> > 
> > Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> > ---
> > 
> >  Changes since V5:
> >  - Made domain allocation/free code more consistent - thanks Joerg!
> >  
> >  Changes since V4:
> >  - Added Kconfig hunk to depend on ARM or IOMMU_DMA
> >  
> >  Changes since V3:
> >  - Removed group parameter from ipmmu_init_platform_device()
> >  
> >  Changes since V2:
> >  
> >  - Included this new patch from the following series:
> >    [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> >  
> >  - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
> >  - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
> >  - of_xlate() is now used without #ifdefs
> >  - Made sure code compiles on both 32-bit and 64-bit ARM.
> >  
> >  drivers/iommu/Kconfig      |    1
> >  drivers/iommu/ipmmu-vmsa.c |  122 ++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 115 insertions(+), 8 deletions(-)

[snip]

> > --- 0006/drivers/iommu/ipmmu-vmsa.c
> > +++ work/drivers/iommu/ipmmu-vmsa.c	2016-10-20 08:16:48.440607110 +0900

[snip]

> > -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> > -{
> > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > -		return NULL;
> 
> I *think* that if we did the initial check thus:
> 
> 	if (type != IOMMU_DOMAIN_UNMANAGED ||
> 	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> 		return NULL;

I assume you meant

 	if (type != IOMMU_DOMAIN_UNMANAGED &&
 	    (!IS_ENABLED(CONFIG_IOMMU_DMA) || type != IOMMU_DOMAIN_DMA))
 		return NULL;

But how about just

 	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA))
 		return NULL;

as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?

> it shouldn't be necessary to split the function at all - we then just
> wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
> in the 32-bit ARM case they just don't run as that can never be true.
> 
> > -
> > -	return __ipmmu_domain_alloc(type);
> > -}
> > -

-- 
Regards,

Laurent Pinchart

[PATCH] iommu/ipmmu-vmsa: Unify domain alloc/free implementations

[Laurent Pinchart]

The ARM and ARM64 implementations of the IOMMU domain alloc/free
operations can be unified with the correct combination of type checking,
as the domain type can never be IOMMU_DOMAIN_DMA on 32-bit ARM due to
the driver calling iommu_group_get_for_dev() on ARM64 only. Do so.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/iommu/ipmmu-vmsa.c | 58 +++++++++++-----------------------------------
 1 file changed, 14 insertions(+), 44 deletions(-)

Hi Magnus,

This cleans up patch 5/7, making patch 4/7 unnecessary. I proposed squashing
4/7, 5/7 and this one in v7.

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 11550ac90d65..827aa72aaf28 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -529,16 +529,22 @@ static irqreturn_t ipmmu_irq(int irq, void *dev)
  * IOMMU Operations
  */
 
-static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
+static struct iommu_domain *ipmmu_domain_alloc(unsigned int type)
 {
 	struct ipmmu_vmsa_domain *domain;
 
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+		return NULL;
+
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
 
 	spin_lock_init(&domain->lock);
 
+	if (type == IOMMU_DOMAIN_DMA)
+		iommu_get_dma_cookie(&domain->io_domain);
+
 	return &domain->io_domain;
 }
 
@@ -546,6 +552,9 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain)
 {
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 
+	if (io_domain->type)
+		iommu_put_dma_cookie(io_domain);
+
 	/*
 	 * Free the domain resources. We assume that all devices have already
 	 * been detached.
@@ -821,14 +830,6 @@ static void ipmmu_remove_device(struct device *dev)
 	set_archdata(dev, NULL);
 }
 
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
-	if (type != IOMMU_DOMAIN_UNMANAGED)
-		return NULL;
-
-	return __ipmmu_domain_alloc(type);
-}
-
 static const struct iommu_ops ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc,
 	.domain_free = ipmmu_domain_free,
@@ -847,42 +848,11 @@ static const struct iommu_ops ipmmu_ops = {
 
 #ifdef CONFIG_IOMMU_DMA
 
-static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
-{
-	struct iommu_domain *io_domain = NULL;
-
-	switch (type) {
-	case IOMMU_DOMAIN_UNMANAGED:
-		io_domain = __ipmmu_domain_alloc(type);
-		break;
-
-	case IOMMU_DOMAIN_DMA:
-		io_domain = __ipmmu_domain_alloc(type);
-		if (io_domain)
-			iommu_get_dma_cookie(io_domain);
-		break;
-	}
-
-	return io_domain;
-}
-
-static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
-{
-	switch (io_domain->type) {
-	case IOMMU_DOMAIN_DMA:
-		iommu_put_dma_cookie(io_domain);
-		/* fall-through */
-	default:
-		ipmmu_domain_free(io_domain);
-		break;
-	}
-}
-
 static int ipmmu_add_device_dma(struct device *dev)
 {
 	struct iommu_group *group;
 
-	/* only accept devices with iommus property */
+	/* Only accept devices with an iommus property. */
 	if (of_count_phandle_with_args(dev->of_node, "iommus",
 				       "#iommu-cells") < 0)
 		return -ENODEV;
@@ -920,13 +890,13 @@ static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
 static int ipmmu_of_xlate_dma(struct device *dev,
 			      struct of_phandle_args *spec)
 {
-	/* dummy callback to satisfy of_iommu_configure() */
+	/* Dummy callback to satisfy of_iommu_configure(). */
 	return 0;
 }
 
 static const struct iommu_ops ipmmu_ops = {
-	.domain_alloc = ipmmu_domain_alloc_dma,
-	.domain_free = ipmmu_domain_free_dma,
+	.domain_alloc = ipmmu_domain_alloc,
+	.domain_free = ipmmu_domain_free,
 	.attach_dev = ipmmu_attach_device,
 	.detach_dev = ipmmu_detach_device,
 	.map = ipmmu_map,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Joerg Roedel]

On Fri, Nov 11, 2016 at 03:13:32AM +0200, Laurent Pinchart wrote:
> Joerg, as I've sent a few comments about the other patches (sorry for the late 
> review, I got delayed by KS and LPC), the follow-up patch should probably be 
> squashed into this one when Magnus addresses my comments. Could you please 
> hold off pushing the arm/renesas branch until Magnus replies to this ?

Okay, I wait for a re-post and replace the patches in my tree then.



	Joerg

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Robin Murphy]

Hi Laurent,

On 11/11/16 01:50, Laurent Pinchart wrote:
> Hi Robin,
> 
> On Friday 21 Oct 2016 18:52:53 Robin Murphy wrote:
>> On 20/10/16 00:36, Magnus Damm wrote:
>>> From: Magnus Damm <damm+renesas@opensource.se>
>>>
>>> Introduce an alternative set of iommu_ops suitable for 64-bit ARM
>>> as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
>>> Kconfig to depend on ARM or IOMMU_DMA.
>>>
>>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>>> ---
>>>
>>>  Changes since V5:
>>>  - Made domain allocation/free code more consistent - thanks Joerg!
>>>  
>>>  Changes since V4:
>>>  - Added Kconfig hunk to depend on ARM or IOMMU_DMA
>>>  
>>>  Changes since V3:
>>>  - Removed group parameter from ipmmu_init_platform_device()
>>>  
>>>  Changes since V2:
>>>  
>>>  - Included this new patch from the following series:
>>>    [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
>>>  
>>>  - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
>>>  - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
>>>  - of_xlate() is now used without #ifdefs
>>>  - Made sure code compiles on both 32-bit and 64-bit ARM.
>>>  
>>>  drivers/iommu/Kconfig      |    1
>>>  drivers/iommu/ipmmu-vmsa.c |  122 ++++++++++++++++++++++++++++++++++++---
>>>  2 files changed, 115 insertions(+), 8 deletions(-)
> 
> [snip]
> 
>>> --- 0006/drivers/iommu/ipmmu-vmsa.c
>>> +++ work/drivers/iommu/ipmmu-vmsa.c	2016-10-20 08:16:48.440607110 +0900
> 
> [snip]
> 
>>> -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
>>> -{
>>> -	if (type != IOMMU_DOMAIN_UNMANAGED)
>>> -		return NULL;
>>
>> I *think* that if we did the initial check thus:
>>
>> 	if (type != IOMMU_DOMAIN_UNMANAGED ||
>> 	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
>> 		return NULL;
> 
> I assume you meant
> 
>  	if (type != IOMMU_DOMAIN_UNMANAGED &&
>  	    (!IS_ENABLED(CONFIG_IOMMU_DMA) || type != IOMMU_DOMAIN_DMA))
>  		return NULL;
> 
> But how about just
> 
>  	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA))
>  		return NULL;
> 
> as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?

Actually it can be, but *only* at this point, because
iommu_group_get_for_dev() will always attempt to allocate a default
domain. Having the additional check up-front just saves going through
the whole IOVA domain allocation only to tear it all down again when
get_cookie() returns -ENODEV. You're right that it's not strictly
necessary (and that I got my DeMorganning wrong), though.

Robin.

>> it shouldn't be necessary to split the function at all - we then just
>> wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
>> in the 32-bit ARM case they just don't run as that can never be true.
>>
>>> -
>>> -	return __ipmmu_domain_alloc(type);
>>> -}
>>> -
> 

Re: [PATCH v6 05/07] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

[Laurent Pinchart]

Hi Robin,

On Friday 11 Nov 2016 14:44:29 Robin Murphy wrote:
> On 11/11/16 01:50, Laurent Pinchart wrote:
> > On Friday 21 Oct 2016 18:52:53 Robin Murphy wrote:
> >> On 20/10/16 00:36, Magnus Damm wrote:
> >>> From: Magnus Damm <damm+renesas@opensource.se>
> >>> 
> >>> Introduce an alternative set of iommu_ops suitable for 64-bit ARM
> >>> as well as 32-bit ARM when CONFIG_IOMMU_DMA=y. Also adjust the
> >>> Kconfig to depend on ARM or IOMMU_DMA.
> >>> 
> >>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> >>> ---
> >>> 
> >>>  Changes since V5:
> >>>  - Made domain allocation/free code more consistent - thanks Joerg!
> >>>  
> >>>  Changes since V4:
> >>>  - Added Kconfig hunk to depend on ARM or IOMMU_DMA
> >>>  
> >>>  Changes since V3:
> >>>  - Removed group parameter from ipmmu_init_platform_device()
> >>>  
> >>>  Changes since V2:
> >>>  
> >>>  - Included this new patch from the following series:
> >>>    [PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
> >>>  
> >>>  - Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
> >>>  - Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
> >>>  - of_xlate() is now used without #ifdefs
> >>>  - Made sure code compiles on both 32-bit and 64-bit ARM.
> >>>  
> >>>  drivers/iommu/Kconfig      |    1
> >>>  drivers/iommu/ipmmu-vmsa.c |  122 +++++++++++++++++++++++++++++++++---
> >>>  2 files changed, 115 insertions(+), 8 deletions(-)
> > 
> > [snip]
> > 
> >>> --- 0006/drivers/iommu/ipmmu-vmsa.c
> >>> +++ work/drivers/iommu/ipmmu-vmsa.c
> >>> 2016-10-20 08:16:48.440607110 +0900
> > 
> > [snip]
> > 
> >>> -static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
> >>> -{
> >>> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> >>> -		return NULL;
> >> 
> >> I *think* that if we did the initial check thus:
> >> 	if (type != IOMMU_DOMAIN_UNMANAGED ||
> >> 	    (IS_ENABLED(CONFIG_IOMMU_DMA) && type != IOMMU_DOMAIN_DMA))
> >> 		return NULL;
> > 
> > I assume you meant
> > 
> >  	if (type != IOMMU_DOMAIN_UNMANAGED &&
> >  	    (!IS_ENABLED(CONFIG_IOMMU_DMA) || type != IOMMU_DOMAIN_DMA))
> >  		return NULL;
> > 
> > But how about just
> > 
> >  	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA))
> >  		return NULL;
> > 
> > as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?
> 
> Actually it can be, but *only* at this point, because
> iommu_group_get_for_dev() will always attempt to allocate a default
> domain.

If I'm not mistaken iommu_group_get_for_dev() is the only function that tries 
to create an IOMMU_DOMAIN_DMA domain, and is only called in core code by 
iommu_request_dm_for_dev(). That function isn't called by the IPMMU driver, 
and with Magnus' patches the IPMMU driver calls iommu_group_get_for_dev() on 
ARM64 platforms only (in ipmmu_add_device_dma(), only compiled in when 
CONFIG_IOMMU_DMA is set, which only happens on ARM64).

> Having the additional check up-front just saves going through
> the whole IOVA domain allocation only to tear it all down again when
> get_cookie() returns -ENODEV. You're right that it's not strictly
> necessary (and that I got my DeMorganning wrong), though.
> 
> Robin.
> 
> >> it shouldn't be necessary to split the function at all - we then just
> >> wrap the {get,put}_cookie() bits in "if (type ==  IOMMU_DOMAIN_DMA)" and
> >> in the 32-bit ARM case they just don't run as that can never be true.
> >> 
> >>> -
> >>> -	return __ipmmu_domain_alloc(type);
> >>> -}
> >>> -

-- 
Regards,

Laurent Pinchart