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

From: Magnus Damm <[email protected]>

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 <[email protected]>

Reviewed-by Joerg Roedel <[email protected]>

---

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(-)

drivers/iommu/ipmmu-vmsa.c +1 -0

@@ -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"

drivers/iommu/ipmmu-vmsa.c +7 -0

+
+	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.

drivers/iommu/ipmmu-vmsa.c +21 -1

-	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);
 }
 
 /*

drivers/iommu/ipmmu-vmsa.c +10 -5

+	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);

From: Magnus Damm <[email protected]>

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 <[email protected]>

Reviewed-by Joerg Roedel <[email protected]>

---

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(-)

drivers/iommu/ipmmu-vmsa.c +32 -10

 {
 	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)

drivers/iommu/ipmmu-vmsa.c +2 -0

 	 *   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;
 

drivers/iommu/ipmmu-vmsa.c +0 -0

 	dev->archdata.iommu = NULL;

From: Magnus Damm <[email protected]>

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

Signed-off-by Magnus Damm <[email protected]>

Reviewed-by Joerg Roedel <[email protected]>

---

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(-)

drivers/iommu/ipmmu-vmsa.c +9 -4

@@ -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);

From: Magnus Damm <[email protected]>

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 <[email protected]>

---

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(-)

drivers/iommu/Kconfig +1 -0

@@ -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

drivers/iommu/ipmmu-vmsa.c +3 -0

@@ -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"
 

drivers/iommu/ipmmu-vmsa.c +17 -4

-
-	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;

drivers/iommu/ipmmu-vmsa.c +84 -0

+		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);
 

drivers/iommu/ipmmu-vmsa.c static struct iommu_domain *__ipmmu_doma · line 526

Quoted 4 lines

drivers/iommu/ipmmu-vmsa.c +0 -4

-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.

Quoted 12 lines · Robin Murphy

drivers/iommu/ipmmu-vmsa.c static struct iommu_domain *__ipmmu_doma · line 526

Quoted 4 lines

drivers/iommu/ipmmu-vmsa.c +0 -4

-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;

Quoted 3 lines

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, 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 ?

Quoted 31 lines · Magnus Damm

From: Magnus Damm <[email protected]>

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 <[email protected]>

---

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(-)

Quoted 2 lines

drivers/iommu/ipmmu-vmsa.c +0 -0

drivers/iommu/ipmmu-vmsa.c static struct iommu_domain *__ipmmu_doma · line 526

Quoted 4 lines

drivers/iommu/ipmmu-vmsa.c +0 -4

-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.

Quoted 4 lines

drivers/iommu/ipmmu-vmsa.c +0 -4

-
-	return __ipmmu_domain_alloc(type);
-}
-

Hi Robin,

Quoted 32 lines · On Friday 21 Oct 2016 18:52:53 Robin Murphy wrote:

Quoted 31 lines · Magnus Damm

From: Magnus Damm <[email protected]>

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 <[email protected]>

---

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]

Quoted 2 lines

Quoted 2 lines

drivers/iommu/ipmmu-vmsa.c +0 -0

[snip]

Quoted 10 lines

drivers/iommu/ipmmu-vmsa.c static struct iommu_domain *__ipmmu_doma · line 526

Quoted 4 lines

drivers/iommu/ipmmu-vmsa.c +0 -4

-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

(quoted diff) +0 -0

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

But how about just

(quoted diff) +0 -0

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

as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?

Quoted 8 lines

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.

Quoted 4 lines

drivers/iommu/ipmmu-vmsa.c +0 -4

-
-	return __ipmmu_domain_alloc(type);
-}
-

Quoted 60 lines · Laurent Pinchart

Quoted 32 lines · On Friday 21 Oct 2016 18:52:53 Robin Murphy wrote:

Quoted 31 lines · Magnus Damm

From: Magnus Damm <[email protected]>

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 <[email protected]>

---

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]

Quoted 3 lines

Quoted 3 lines

drivers/iommu/ipmmu-vmsa.c +0 -0

2016-10-20 08:16:48.440607110 +0900

[snip]

Quoted 9 lines

drivers/iommu/ipmmu-vmsa.c static struct iommu_domain *__ipmmu_doma · line 526

Quoted 4 lines

drivers/iommu/ipmmu-vmsa.c +0 -4

-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

(quoted diff) +0 -0

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

But how about just

Quoted 4 lines

(quoted diff) +0 -0

 	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.

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

drivers/iommu/ipmmu-vmsa.c +0 -4

-
-	return __ipmmu_domain_alloc(type);
-}
-

From: Magnus Damm <[email protected]>

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 <[email protected]>

---

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(-)

drivers/iommu/Kconfig +1 -0

@@ -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

drivers/iommu/ipmmu-vmsa.c +43 -8

@@ -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:

drivers/iommu/ipmmu-vmsa.c +28 -0

+	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);

drivers/iommu/ipmmu-vmsa.c +7 -0

+	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,

drivers/iommu/ipmmu-vmsa.c +6 -0

+};
+
+#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);

From: Magnus Damm <[email protected]>

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 <[email protected]>

Reviewed-by Joerg Roedel <[email protected]>

---

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(-)

drivers/iommu/ipmmu-vmsa.c +27 -10

@@ -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)

Quoted 4 lines · Magnus Damm

From: Magnus Damm <[email protected]>

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.

[1]:http://www.mail-archive.com/[email protected]/msg14577.ht ml

Quoted 105 lines

Signed-off-by Magnus Damm <[email protected]>

Reviewed-by Joerg Roedel <[email protected]>

---

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(-)

drivers/iommu/ipmmu-vmsa.c +14 -0

@@ -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) +{

drivers/iommu/ipmmu-vmsa.c +6 -3

+}
+#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);
 

drivers/iommu/ipmmu-vmsa.c +1 -2

 	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",

drivers/iommu/ipmmu-vmsa.c static int ipmmu_add_device(struct devic · line 773

Quoted 7 lines

drivers/iommu/ipmmu-vmsa.c +0 -0

 			 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 

Quoted 44 lines

drivers/iommu/ipmmu-vmsa.c +1 -2

 	 *   at a 1GB offset to ensure that NULL VAs will fault.
 	 */
 
-	archdata = dev->archdata.iommu;
-	mmu = archdata->mmu;
+	mmu = to_archdata(dev)->mmu;
 

drivers/iommu/ipmmu-vmsa.c +1 -1

 	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))

drivers/iommu/ipmmu-vmsa.c +2 -2

 		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)

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