[PATCH 00/06] iommu/ipmmu-vmsa: IPMMU multi-arch update

[PATCH 00/06] iommu/ipmmu-vmsa: IPMMU multi-arch update

[Magnus Damm]

iommu/ipmmu-vmsa: IPMMU multi-arch update

[PATCH 01/06] iommu/ipmmu-vmsa: Remove platform data handling
[PATCH 02/06] iommu/ipmmu-vmsa: Convert to dev_data
[PATCH 03/06] iommu/ipmmu-vmsa: Break out utlb control function
[PATCH 04/06] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context
[PATCH 05/06] iommu/ipmmu-vmsa: Break out 32-bit ARM mapping code
[PATCH 06/06] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency

These patches update the IPMMU driver with a few minor changes
to support multiple architectures. With these patches applied
the driver is known to compile without issues on 32-bit ARM,
64-bit ARM and x86_64.

With the series applied the r8a7790 IPMMU still gets invoked
when the modetest utility is used to display a test pattern
on the VGA port driven by the DU device.

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

 Developed on top of renesas-drivers-2015-12-08-v4.4-rc4

 drivers/iommu/Kconfig      |    1 
 drivers/iommu/ipmmu-vmsa.c |  224 +++++++++++++++++++++++++++-----------------
 2 files changed, 139 insertions(+), 86 deletions(-)

[PATCH 01/06] 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>
---

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

--- 0001/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 11:35:00.490513000 +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 02/06] iommu/ipmmu-vmsa: Convert to dev_data

[Magnus Damm]

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

Rename ipmmu_vmsa_archdata to ipmmu_vmsa_dev_data to avoid
confusion when using the driver on multiple architectures.

The data now stored in ipmmu_vmsa_dev_data is used to point
various on-chip devices to the actual IPMMU instances.

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

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

--- 0003/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 13:16:27.160513000 +0900
@@ -47,7 +47,7 @@ struct ipmmu_vmsa_domain {
 	spinlock_t lock;			/* Protects mappings */
 };
 
-struct ipmmu_vmsa_archdata {
+struct ipmmu_vmsa_dev_data {
 	struct ipmmu_vmsa_device *mmu;
 	unsigned int *utlbs;
 	unsigned int num_utlbs;
@@ -182,6 +182,20 @@ static struct ipmmu_vmsa_domain *to_vmsa
 #define IMUASID_ASID0_SHIFT		0
 
 /* -----------------------------------------------------------------------------
+ * Consumer device side private data handling
+ */
+
+static struct ipmmu_vmsa_dev_data *get_dev_data(struct device *dev)
+{
+	return dev->archdata.iommu;
+}
+
+static void set_dev_data(struct device *dev, struct ipmmu_vmsa_dev_data *data)
+{
+	dev->archdata.iommu = data;
+}
+
+/* -----------------------------------------------------------------------------
  * Read/Write Access
  */
 
@@ -485,8 +499,8 @@ 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_device *mmu = archdata->mmu;
+	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
+	struct ipmmu_vmsa_device *mmu = dev_data->mmu;
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned long flags;
 	unsigned int i;
@@ -518,8 +532,8 @@ static int ipmmu_attach_device(struct io
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < archdata->num_utlbs; ++i)
-		ipmmu_utlb_enable(domain, archdata->utlbs[i]);
+	for (i = 0; i < dev_data->num_utlbs; ++i)
+		ipmmu_utlb_enable(domain, dev_data->utlbs[i]);
 
 	return 0;
 }
@@ -527,12 +541,12 @@ 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_dev_data *dev_data = get_dev_data(dev);
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned int i;
 
-	for (i = 0; i < archdata->num_utlbs; ++i)
-		ipmmu_utlb_disable(domain, archdata->utlbs[i]);
+	for (i = 0; i < dev_data->num_utlbs; ++i)
+		ipmmu_utlb_disable(domain, dev_data->utlbs[i]);
 
 	/*
 	 * TODO: Optimize by disabling the context when no device is attached.
@@ -595,7 +609,7 @@ static int ipmmu_find_utlbs(struct ipmmu
 
 static int ipmmu_add_device(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata;
+	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
 	struct ipmmu_vmsa_device *mmu;
 	struct iommu_group *group = NULL;
 	unsigned int *utlbs;
@@ -603,7 +617,7 @@ static int ipmmu_add_device(struct devic
 	int num_utlbs;
 	int ret = -ENODEV;
 
-	if (dev->archdata.iommu) {
+	if (dev_data) {
 		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
 			 dev_name(dev));
 		return -EINVAL;
@@ -662,16 +676,16 @@ static int ipmmu_add_device(struct devic
 		goto error;
 	}
 
-	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
-	if (!archdata) {
+	dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
+	if (!dev_data) {
 		ret = -ENOMEM;
 		goto error;
 	}
 
-	archdata->mmu = mmu;
-	archdata->utlbs = utlbs;
-	archdata->num_utlbs = num_utlbs;
-	dev->archdata.iommu = archdata;
+	dev_data->mmu = mmu;
+	dev_data->utlbs = utlbs;
+	dev_data->num_utlbs = num_utlbs;
+	set_dev_data(dev, dev_data);
 
 	/*
 	 * Create the ARM mapping, used by the ARM DMA mapping core to allocate
@@ -708,10 +722,10 @@ static int ipmmu_add_device(struct devic
 error:
 	arm_iommu_release_mapping(mmu->mapping);
 
-	kfree(dev->archdata.iommu);
+	kfree(dev_data);
 	kfree(utlbs);
 
-	dev->archdata.iommu = NULL;
+	set_dev_data(dev, NULL);
 
 	if (!IS_ERR_OR_NULL(group))
 		iommu_group_remove_device(dev);
@@ -721,15 +735,15 @@ error:
 
 static void ipmmu_remove_device(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
 
 	arm_iommu_detach_device(dev);
 	iommu_group_remove_device(dev);
 
-	kfree(archdata->utlbs);
-	kfree(archdata);
+	kfree(dev_data->utlbs);
+	kfree(dev_data);
 
-	dev->archdata.iommu = NULL;
+	set_dev_data(dev, NULL);
 }
 
 static const struct iommu_ops ipmmu_ops = {

[PATCH 03/06] iommu/ipmmu-vmsa: Break out utlb control function

[Magnus Damm]

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

Introduce the function ipmmu_utlb_ctrl() that shares code
to enable or disable utlbs.

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

 drivers/iommu/ipmmu-vmsa.c |   30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

--- 0004/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 13:17:40.580513000 +0900
@@ -279,9 +279,18 @@ static void ipmmu_utlb_enable(struct ipm
 static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
 			       unsigned int utlb)
 {
-	struct ipmmu_vmsa_device *mmu = domain->mmu;
+	ipmmu_write(domain->mmu, IMUCTR(utlb), 0);
+}
+
+static void ipmmu_utlb_ctrl(struct ipmmu_vmsa_domain *domain,
+			    void (*fn)(struct ipmmu_vmsa_domain *,
+				       unsigned int utlb), struct device *dev)
+{
+	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
+	unsigned int i;
 
-	ipmmu_write(mmu, IMUCTR(utlb), 0);
+	for (i = 0; i < dev_data->num_utlbs; ++i)
+		fn(domain, dev_data->utlbs[i]);
 }
 
 static void ipmmu_tlb_flush_all(void *cookie)
@@ -503,7 +512,6 @@ static int ipmmu_attach_device(struct io
 	struct ipmmu_vmsa_device *mmu = dev_data->mmu;
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned long flags;
-	unsigned int i;
 	int ret = 0;
 
 	if (!mmu) {
@@ -529,24 +537,16 @@ static int ipmmu_attach_device(struct io
 
 	spin_unlock_irqrestore(&domain->lock, flags);
 
-	if (ret < 0)
-		return ret;
+	if (!ret)
+		ipmmu_utlb_ctrl(domain, ipmmu_utlb_enable, dev);
 
-	for (i = 0; i < dev_data->num_utlbs; ++i)
-		ipmmu_utlb_enable(domain, dev_data->utlbs[i]);
-
-	return 0;
+	return ret;
 }
 
 static void ipmmu_detach_device(struct iommu_domain *io_domain,
 				struct device *dev)
 {
-	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
-	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
-	unsigned int i;
-
-	for (i = 0; i < dev_data->num_utlbs; ++i)
-		ipmmu_utlb_disable(domain, dev_data->utlbs[i]);
+	ipmmu_utlb_ctrl(to_vmsa_domain(io_domain), ipmmu_utlb_disable, dev);
 
 	/*
 	 * TODO: Optimize by disabling the context when no device is attached.

[PATCH 04/06] 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 go handle all registered contexts.

At this point the number of contexts are still limited.

The purpose of this patch is to remove the use of the
ARM specific mapping variable from ipmmu_irq(). 

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

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

--- 0007/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 13:14:35.540513000 +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,16 @@
 
 #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;
+	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
+	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
 
 	struct dma_iommu_mapping *mapping;
 };
@@ -319,6 +324,7 @@ static struct iommu_gather_ops ipmmu_gat
 static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
 {
 	phys_addr_t ttbr;
+	int ret;
 
 	/*
 	 * Allocate the page table operations.
@@ -348,10 +354,16 @@ 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 = bitmap_find_free_region(domain->mmu->ctx, IPMMU_CTX_MAX, 0);
+	if (ret < 0) {
+		free_io_pgtable_ops(domain->iop);
+		return ret;
+	}
+
+	domain->context_id = ret;
+	domain->mmu->domains[ret] = domain;
 
 	/* TTBR0 */
 	ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
@@ -395,6 +407,8 @@ static int ipmmu_domain_init_context(str
 
 static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
 {
+	bitmap_release_region(domain->mmu->ctx, domain->context_id, 0);
+
 	/*
 	 * Disable the context. Flush the TLB as required when modifying the
 	 * context registers.
@@ -460,16 +474,16 @@ 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;
-
-	if (!mmu->mapping)
-		return IRQ_NONE;
+	irqreturn_t status = IRQ_NONE;
+	unsigned int k;
 
-	io_domain = mmu->mapping->domain;
-	domain = to_vmsa_domain(io_domain);
+	/* Check interrupts for all active contexts */
+	for (k = find_first_bit(mmu->ctx, IPMMU_CTX_MAX);
+	     k < IPMMU_CTX_MAX && status == IRQ_NONE;
+	     k = find_next_bit(mmu->ctx, IPMMU_CTX_MAX, k))
+		status = ipmmu_domain_irq(mmu->domains[k]);
 
-	return ipmmu_domain_irq(domain);
+	return status;
 }
 
 /* -----------------------------------------------------------------------------
@@ -788,6 +802,7 @@ static int ipmmu_probe(struct platform_d
 
 	mmu->dev = &pdev->dev;
 	mmu->num_utlbs = 32;
+	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
 
 	/* Map I/O memory and request IRQ. */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

[PATCH 05/06] iommu/ipmmu-vmsa: Break out 32-bit ARM mapping code

[Magnus Damm]

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

Make the driver compile on more than just 32-bit ARM
by breaking out and wrapping ARM specific functions
in #ifdefs. Needed to be able to use the driver on
other architectures.

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

 drivers/iommu/ipmmu-vmsa.c |   94 +++++++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 32 deletions(-)

--- 0007/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 13:20:26.580513000 +0900
@@ -22,8 +22,10 @@
 #include <linux/sizes.h>
 #include <linux/slab.h>
 
+#ifdef CONFIG_ARM
 #include <asm/dma-iommu.h>
 #include <asm/pgalloc.h>
+#endif
 
 #include "io-pgtable.h"
 
@@ -38,7 +40,9 @@ struct ipmmu_vmsa_device {
 	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
 	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
 
+#ifdef CONFIG_ARM
 	struct dma_iommu_mapping *mapping;
+#endif
 };
 
 struct ipmmu_vmsa_domain {
@@ -621,6 +625,60 @@ static int ipmmu_find_utlbs(struct ipmmu
 	return 0;
 }
 
+#ifdef CONFIG_ARM
+static int ipmmu_map_attach(struct device *dev, struct ipmmu_vmsa_device *mmu)
+{
+	int ret;
+
+	/*
+	 * Create the ARM mapping, used by the ARM DMA mapping core to allocate
+	 * VAs. This will allocate a corresponding IOMMU domain.
+	 *
+	 * TODO:
+	 * - Create one mapping per context (TLB).
+	 * - Make the mapping size configurable ? We currently use a 2GB mapping
+	 *   at a 1GB offset to ensure that NULL VAs will fault.
+	 */
+	if (!mmu->mapping) {
+		struct dma_iommu_mapping *mapping;
+
+		mapping = arm_iommu_create_mapping(&platform_bus_type,
+						   SZ_1G, SZ_2G);
+		if (IS_ERR(mapping)) {
+			dev_err(mmu->dev, "failed to create ARM IOMMU mapping\n");
+			return PTR_ERR(mapping);
+		}
+
+		mmu->mapping = mapping;
+	}
+
+	/* Attach the ARM VA mapping to the device. */
+	ret = arm_iommu_attach_device(dev, mmu->mapping);
+	if (ret < 0) {
+		dev_err(dev, "Failed to attach device to VA mapping\n");
+		arm_iommu_release_mapping(mmu->mapping);
+	}
+
+	return ret;
+}
+static inline void ipmmu_detach(struct device *dev)
+{
+	arm_iommu_detach_device(dev);
+}
+static inline void ipmmu_release_mapping(struct ipmmu_vmsa_device *mmu)
+{
+	arm_iommu_release_mapping(mmu->mapping);
+}
+#else
+static inline int ipmmu_map_attach(struct device *dev,
+				   struct ipmmu_vmsa_device *mmu)
+{
+	return 0;
+}
+static inline void ipmmu_detach(struct device *dev) {}
+static inline void ipmmu_release_mapping(struct ipmmu_vmsa_device *mmu) {}
+#endif
+
 static int ipmmu_add_device(struct device *dev)
 {
 	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
@@ -701,41 +759,13 @@ static int ipmmu_add_device(struct devic
 	dev_data->num_utlbs = num_utlbs;
 	set_dev_data(dev, dev_data);
 
-	/*
-	 * Create the ARM mapping, used by the ARM DMA mapping core to allocate
-	 * VAs. This will allocate a corresponding IOMMU domain.
-	 *
-	 * TODO:
-	 * - Create one mapping per context (TLB).
-	 * - Make the mapping size configurable ? We currently use a 2GB mapping
-	 *   at a 1GB offset to ensure that NULL VAs will fault.
-	 */
-	if (!mmu->mapping) {
-		struct dma_iommu_mapping *mapping;
-
-		mapping = arm_iommu_create_mapping(&platform_bus_type,
-						   SZ_1G, SZ_2G);
-		if (IS_ERR(mapping)) {
-			dev_err(mmu->dev, "failed to create ARM IOMMU mapping\n");
-			ret = PTR_ERR(mapping);
-			goto error;
-		}
-
-		mmu->mapping = mapping;
-	}
-
-	/* Attach the ARM VA mapping to the device. */
-	ret = arm_iommu_attach_device(dev, mmu->mapping);
-	if (ret < 0) {
-		dev_err(dev, "Failed to attach device to VA mapping\n");
+	ret = ipmmu_map_attach(dev, mmu);
+	if (ret < 0)
 		goto error;
-	}
 
 	return 0;
 
 error:
-	arm_iommu_release_mapping(mmu->mapping);
-
 	kfree(dev_data);
 	kfree(utlbs);
 
@@ -751,7 +781,7 @@ static void ipmmu_remove_device(struct d
 {
 	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
 
-	arm_iommu_detach_device(dev);
+	ipmmu_detach(dev);
 	iommu_group_remove_device(dev);
 
 	kfree(dev_data->utlbs);
@@ -862,7 +892,7 @@ static int ipmmu_remove(struct platform_
 	list_del(&mmu->list);
 	spin_unlock(&ipmmu_devices_lock);
 
-	arm_iommu_release_mapping(mmu->mapping);
+	ipmmu_release_mapping(mmu);
 
 	ipmmu_device_reset(mmu);
 

[PATCH 06/06] 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>
---

 This time the result also compiles on x86. Need to be
 applied as last patch in the following series:
 [PATCH 00/06] iommu/ipmmu-vmsa: IPMMU multi-arch update

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

--- 0001/drivers/iommu/Kconfig
+++ work/drivers/iommu/Kconfig	2015-10-18 14:58:09.080513000 +0900
@@ -324,7 +324,6 @@ config SHMOBILE_IOMMU_L1SIZE
 
 config IPMMU_VMSA
 	bool "Renesas VMSA-compatible IPMMU"
-	depends on ARM_LPAE
 	depends on ARCH_SHMOBILE || COMPILE_TEST
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE

Re: [PATCH 03/06] iommu/ipmmu-vmsa: Break out utlb control function

[Geert Uytterhoeven]

Hi Magnus,

On Tue, Dec 15, 2015 at 1:02 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> --- 0004/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c     2015-12-15 13:17:40.580513000 +0900
> @@ -279,9 +279,18 @@ static void ipmmu_utlb_enable(struct ipm
>  static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
>                                unsigned int utlb)
>  {
> -       struct ipmmu_vmsa_device *mmu = domain->mmu;
> +       ipmmu_write(domain->mmu, IMUCTR(utlb), 0);
> +}
> +
> +static void ipmmu_utlb_ctrl(struct ipmmu_vmsa_domain *domain,
> +                           void (*fn)(struct ipmmu_vmsa_domain *,
> +                                      unsigned int utlb), struct device *dev)
> +{
> +       struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> +       unsigned int i;
>
> -       ipmmu_write(mmu, IMUCTR(utlb), 0);
> +       for (i = 0; i < dev_data->num_utlbs; ++i)
> +               fn(domain, dev_data->utlbs[i]);
>  }

Unless you have further plans with the "fn" parameter, I would simply pass
a bool enable/disable flag instead of a function pointer.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 01/06] iommu/ipmmu-vmsa: Remove platform data handling

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Tuesday 15 December 2015 21:02:21 Magnus Damm wrote:
> 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>

> ---
> 
>  drivers/iommu/ipmmu-vmsa.c |    5 -----
>  1 file changed, 5 deletions(-)
> 
> --- 0001/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 11:35:00.490513000 +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");

-- 
Regards,

Laurent Pinchart

Re: [PATCH 02/06] iommu/ipmmu-vmsa: Convert to dev_data

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Tuesday 15 December 2015 21:02:30 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Rename ipmmu_vmsa_archdata to ipmmu_vmsa_dev_data to avoid
> confusion when using the driver on multiple architectures.

I'm sorry but this patches makes the driver more confusing to me. archdata is 
a well established concept for IOMMU on ARM, I don't see how hiding help would 
help.

> The data now stored in ipmmu_vmsa_dev_data is used to point
> various on-chip devices to the actual IPMMU instances.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  drivers/iommu/ipmmu-vmsa.c |   58 +++++++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 22 deletions(-)
> 
> --- 0003/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 13:16:27.160513000 +0900
> @@ -47,7 +47,7 @@ struct ipmmu_vmsa_domain {
>  	spinlock_t lock;			/* Protects mappings */
>  };
> 
> -struct ipmmu_vmsa_archdata {
> +struct ipmmu_vmsa_dev_data {
>  	struct ipmmu_vmsa_device *mmu;
>  	unsigned int *utlbs;
>  	unsigned int num_utlbs;
> @@ -182,6 +182,20 @@ static struct ipmmu_vmsa_domain *to_vmsa
>  #define IMUASID_ASID0_SHIFT		0
> 
>  /* ------------------------------------------------------------------------
> + * Consumer device side private data handling
> + */
> +
> +static struct ipmmu_vmsa_dev_data *get_dev_data(struct device *dev)
> +{
> +	return dev->archdata.iommu;
> +}
> +
> +static void set_dev_data(struct device *dev, struct ipmmu_vmsa_dev_data
> *data)
> +{
> +	dev->archdata.iommu = data;
> +}
> +
> +/* ------------------------------------------------------------------------
>   * Read/Write Access
>   */
> 
> @@ -485,8 +499,8 @@ 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_device *mmu = archdata->mmu;
> +	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> +	struct ipmmu_vmsa_device *mmu = dev_data->mmu;
>  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
>  	unsigned long flags;
>  	unsigned int i;
> @@ -518,8 +532,8 @@ static int ipmmu_attach_device(struct io
>  	if (ret < 0)
>  		return ret;
> 
> -	for (i = 0; i < archdata->num_utlbs; ++i)
> -		ipmmu_utlb_enable(domain, archdata->utlbs[i]);
> +	for (i = 0; i < dev_data->num_utlbs; ++i)
> +		ipmmu_utlb_enable(domain, dev_data->utlbs[i]);
> 
>  	return 0;
>  }
> @@ -527,12 +541,12 @@ 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_dev_data *dev_data = get_dev_data(dev);
>  	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
>  	unsigned int i;
> 
> -	for (i = 0; i < archdata->num_utlbs; ++i)
> -		ipmmu_utlb_disable(domain, archdata->utlbs[i]);
> +	for (i = 0; i < dev_data->num_utlbs; ++i)
> +		ipmmu_utlb_disable(domain, dev_data->utlbs[i]);
> 
>  	/*
>  	 * TODO: Optimize by disabling the context when no device is attached.
> @@ -595,7 +609,7 @@ static int ipmmu_find_utlbs(struct ipmmu
> 
>  static int ipmmu_add_device(struct device *dev)
>  {
> -	struct ipmmu_vmsa_archdata *archdata;
> +	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
>  	struct ipmmu_vmsa_device *mmu;
>  	struct iommu_group *group = NULL;
>  	unsigned int *utlbs;
> @@ -603,7 +617,7 @@ static int ipmmu_add_device(struct devic
>  	int num_utlbs;
>  	int ret = -ENODEV;
> 
> -	if (dev->archdata.iommu) {
> +	if (dev_data) {
>  		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
>  			 dev_name(dev));
>  		return -EINVAL;
> @@ -662,16 +676,16 @@ static int ipmmu_add_device(struct devic
>  		goto error;
>  	}
> 
> -	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
> -	if (!archdata) {
> +	dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
> +	if (!dev_data) {
>  		ret = -ENOMEM;
>  		goto error;
>  	}
> 
> -	archdata->mmu = mmu;
> -	archdata->utlbs = utlbs;
> -	archdata->num_utlbs = num_utlbs;
> -	dev->archdata.iommu = archdata;
> +	dev_data->mmu = mmu;
> +	dev_data->utlbs = utlbs;
> +	dev_data->num_utlbs = num_utlbs;
> +	set_dev_data(dev, dev_data);
> 
>  	/*
>  	 * Create the ARM mapping, used by the ARM DMA mapping core to allocate
> @@ -708,10 +722,10 @@ static int ipmmu_add_device(struct devic
>  error:
>  	arm_iommu_release_mapping(mmu->mapping);
> 
> -	kfree(dev->archdata.iommu);
> +	kfree(dev_data);
>  	kfree(utlbs);
> 
> -	dev->archdata.iommu = NULL;
> +	set_dev_data(dev, NULL);
> 
>  	if (!IS_ERR_OR_NULL(group))
>  		iommu_group_remove_device(dev);
> @@ -721,15 +735,15 @@ error:
> 
>  static void ipmmu_remove_device(struct device *dev)
>  {
> -	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> +	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> 
>  	arm_iommu_detach_device(dev);
>  	iommu_group_remove_device(dev);
> 
> -	kfree(archdata->utlbs);
> -	kfree(archdata);
> +	kfree(dev_data->utlbs);
> +	kfree(dev_data);
> 
> -	dev->archdata.iommu = NULL;
> +	set_dev_data(dev, NULL);
>  }
> 
>  static const struct iommu_ops ipmmu_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/06] iommu/ipmmu-vmsa: Break out utlb control function

[Laurent Pinchart]

Hi Magnus,

On Tuesday 15 December 2015 16:48:42 Geert Uytterhoeven wrote:
> On Tue, Dec 15, 2015 at 1:02 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> > --- 0004/drivers/iommu/ipmmu-vmsa.c
> > +++ work/drivers/iommu/ipmmu-vmsa.c     2015-12-15 13:17:40.580513000
> > +0900
> > @@ -279,9 +279,18 @@ static void ipmmu_utlb_enable(struct ipm
> >  static void ipmmu_utlb_disable(struct ipmmu_vmsa_domain *domain,
> >                                unsigned int utlb)
> >  {
> > -       struct ipmmu_vmsa_device *mmu = domain->mmu;
> > +       ipmmu_write(domain->mmu, IMUCTR(utlb), 0);
> > +}
> > +
> > +static void ipmmu_utlb_ctrl(struct ipmmu_vmsa_domain *domain,
> > +                           void (*fn)(struct ipmmu_vmsa_domain *,
> > +                                      unsigned int utlb), struct device
> > *dev)
> > +{
> > +       struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> > +       unsigned int i;
> > 
> > -       ipmmu_write(mmu, IMUCTR(utlb), 0);
> > +       for (i = 0; i < dev_data->num_utlbs; ++i)
> > +               fn(domain, dev_data->utlbs[i]);
> > 
> >  }
> 
> Unless you have further plans with the "fn" parameter, I would simply pass
> a bool enable/disable flag instead of a function pointer.

I agree with Geert. What's your plan here ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 04/06] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Tuesday 15 December 2015 21:02:49 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Introduce a bitmap for context handing and convert the
> interrupt routine to go handle all registered contexts.
> 
> At this point the number of contexts are still limited.

That's all nice, but without seeing support for multiple contexts it's hard to 
tell if the implementation is correct for multiple context purpose.

> The purpose of this patch is to remove the use of the
> ARM specific mapping variable from ipmmu_irq().

Why do you want to do that ?

> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  drivers/iommu/ipmmu-vmsa.c |   37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> --- 0007/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 13:14:35.540513000 +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,16 @@
> 
>  #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;
> +	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);

We have up to 4 context on Gen2 and 8 on Gen3, a bitmap might be slightly 
overkill.

> +	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> 
>  	struct dma_iommu_mapping *mapping;
>  };
> @@ -319,6 +324,7 @@ static struct iommu_gather_ops ipmmu_gat
>  static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>  {
>  	phys_addr_t ttbr;
> +	int ret;
> 
>  	/*
>  	 * Allocate the page table operations.
> @@ -348,10 +354,16 @@ static int ipmmu_domain_init_context(str
>  		return -EINVAL;
> 
>  	/*
> -	 * TODO: When adding support for multiple contexts, find an unused
> -	 * context.
> +	 * Find an unused context.

We need to support multiple devices per context or we will very soon run out 
of contexts. How to pick a proper context is a topic that needs to be 
researched, I believe IOMMU groups might come into play.

>  	 */
> -	domain->context_id = 0;
> +	ret = bitmap_find_free_region(domain->mmu->ctx, IPMMU_CTX_MAX, 0);
> +	if (ret < 0) {
> +		free_io_pgtable_ops(domain->iop);
> +		return ret;
> +	}
> +
> +	domain->context_id = ret;
> +	domain->mmu->domains[ret] = domain;

This requires locking to protect against races with the interrupt handler.

> 
>  	/* TTBR0 */
>  	ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
> @@ -395,6 +407,8 @@ static int ipmmu_domain_init_context(str
> 
>  static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>  {
> +	bitmap_release_region(domain->mmu->ctx, domain->context_id, 0);
> +
>  	/*
>  	 * Disable the context. Flush the TLB as required when modifying the
>  	 * context registers.
> @@ -460,16 +474,16 @@ 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;
> -
> -	if (!mmu->mapping)
> -		return IRQ_NONE;
> +	irqreturn_t status = IRQ_NONE;
> +	unsigned int k;

i is a perfectly fine loop counter :-)

> -	io_domain = mmu->mapping->domain;
> -	domain = to_vmsa_domain(io_domain);
> +	/* Check interrupts for all active contexts */
> +	for (k = find_first_bit(mmu->ctx, IPMMU_CTX_MAX);
> +	     k < IPMMU_CTX_MAX && status == IRQ_NONE;
> +	     k = find_next_bit(mmu->ctx, IPMMU_CTX_MAX, k))

You can just loop over mmu->domains and skip NULL entries.

> +		status = ipmmu_domain_irq(mmu->domains[k]);

Only the status of the last domain is taken into account.

> -	return ipmmu_domain_irq(domain);
> +	return status;
>  }
> 
>  /* ------------------------------------------------------------------------
> @@ -788,6 +802,7 @@ static int ipmmu_probe(struct platform_d
> 
>  	mmu->dev = &pdev->dev;
>  	mmu->num_utlbs = 32;
> +	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 05/06] iommu/ipmmu-vmsa: Break out 32-bit ARM mapping code

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Tuesday 15 December 2015 21:02:58 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Make the driver compile on more than just 32-bit ARM
> by breaking out and wrapping ARM specific functions
> in #ifdefs. Needed to be able to use the driver on
> other architectures.

The only other architecture where we need to use the driver is ARM64, and I'm 
not sure this is the right fix to correctly support ARM and ARM64. I believe 
we should instead improve the ARM implementation to get rid of 
arm_iommu_create_mapping() and arm_iommu_attach_device() the same way that 
ARM64 did.

> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
> drivers/iommu/ipmmu-vmsa.c |   94 ++++++++++++++++++++++++++---------------
> 1 file changed, 62 insertions(+), 32 deletions(-)
> 
> --- 0007/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2015-12-15 13:20:26.580513000 +0900
> @@ -22,8 +22,10 @@
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> 
> +#ifdef CONFIG_ARM
>  #include <asm/dma-iommu.h>
>  #include <asm/pgalloc.h>
> +#endif
> 
>  #include "io-pgtable.h"
> 
> @@ -38,7 +40,9 @@ struct ipmmu_vmsa_device {
>  	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>  	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> 
> +#ifdef CONFIG_ARM
>  	struct dma_iommu_mapping *mapping;
> +#endif
>  };
> 
>  struct ipmmu_vmsa_domain {
> @@ -621,6 +625,60 @@ static int ipmmu_find_utlbs(struct ipmmu
>  	return 0;
>  }
> 
> +#ifdef CONFIG_ARM
> +static int ipmmu_map_attach(struct device *dev, struct ipmmu_vmsa_device
> *mmu)
> +{
> +	int ret;
> +
> +	/*
> +	 * Create the ARM mapping, used by the ARM DMA mapping core to allocate
> +	 * VAs. This will allocate a corresponding IOMMU domain.
> +	 *
> +	 * TODO:
> +	 * - Create one mapping per context (TLB).
> +	 * - Make the mapping size configurable ? We currently use a 2GB mapping
> +	 *   at a 1GB offset to ensure that NULL VAs will fault.
> +	 */
> +	if (!mmu->mapping) {
> +		struct dma_iommu_mapping *mapping;
> +
> +		mapping = arm_iommu_create_mapping(&platform_bus_type,
> +						   SZ_1G, SZ_2G);
> +		if (IS_ERR(mapping)) {
> +			dev_err(mmu->dev, "failed to create ARM IOMMU mapping\n");
> +			return PTR_ERR(mapping);
> +		}
> +
> +		mmu->mapping = mapping;
> +	}
> +
> +	/* Attach the ARM VA mapping to the device. */
> +	ret = arm_iommu_attach_device(dev, mmu->mapping);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to attach device to VA mapping\n");
> +		arm_iommu_release_mapping(mmu->mapping);
> +	}
> +
> +	return ret;
> +}
> +static inline void ipmmu_detach(struct device *dev)
> +{
> +	arm_iommu_detach_device(dev);
> +}
> +static inline void ipmmu_release_mapping(struct ipmmu_vmsa_device *mmu)
> +{
> +	arm_iommu_release_mapping(mmu->mapping);
> +}
> +#else
> +static inline int ipmmu_map_attach(struct device *dev,
> +				   struct ipmmu_vmsa_device *mmu)
> +{
> +	return 0;
> +}
> +static inline void ipmmu_detach(struct device *dev) {}
> +static inline void ipmmu_release_mapping(struct ipmmu_vmsa_device *mmu) {}
> +#endif
> +
>  static int ipmmu_add_device(struct device *dev)
>  {
>  	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> @@ -701,41 +759,13 @@ static int ipmmu_add_device(struct devic
>  	dev_data->num_utlbs = num_utlbs;
>  	set_dev_data(dev, dev_data);
> 
> -	/*
> -	 * Create the ARM mapping, used by the ARM DMA mapping core to allocate
> -	 * VAs. This will allocate a corresponding IOMMU domain.
> -	 *
> -	 * TODO:
> -	 * - Create one mapping per context (TLB).
> -	 * - Make the mapping size configurable ? We currently use a 2GB mapping
> -	 *   at a 1GB offset to ensure that NULL VAs will fault.
> -	 */
> -	if (!mmu->mapping) {
> -		struct dma_iommu_mapping *mapping;
> -
> -		mapping = arm_iommu_create_mapping(&platform_bus_type,
> -						   SZ_1G, SZ_2G);
> -		if (IS_ERR(mapping)) {
> -			dev_err(mmu->dev, "failed to create ARM IOMMU mapping\n");
> -			ret = PTR_ERR(mapping);
> -			goto error;
> -		}
> -
> -		mmu->mapping = mapping;
> -	}
> -
> -	/* Attach the ARM VA mapping to the device. */
> -	ret = arm_iommu_attach_device(dev, mmu->mapping);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to attach device to VA mapping\n");
> +	ret = ipmmu_map_attach(dev, mmu);
> +	if (ret < 0)
>  		goto error;
> -	}
> 
>  	return 0;
> 
>  error:
> -	arm_iommu_release_mapping(mmu->mapping);
> -
>  	kfree(dev_data);
>  	kfree(utlbs);
> 
> @@ -751,7 +781,7 @@ static void ipmmu_remove_device(struct d
>  {
>  	struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> 
> -	arm_iommu_detach_device(dev);
> +	ipmmu_detach(dev);
>  	iommu_group_remove_device(dev);
> 
>  	kfree(dev_data->utlbs);
> @@ -862,7 +892,7 @@ static int ipmmu_remove(struct platform_
>  	list_del(&mmu->list);
>  	spin_unlock(&ipmmu_devices_lock);
> 
> -	arm_iommu_release_mapping(mmu->mapping);
> +	ipmmu_release_mapping(mmu);
> 
>  	ipmmu_device_reset(mmu);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 06/06] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Tuesday 15 December 2015 21:03:08 Magnus Damm wrote:
> 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>

> ---
> 
>  This time the result also compiles on x86. Need to be
>  applied as last patch in the following series:
>  [PATCH 00/06] iommu/ipmmu-vmsa: IPMMU multi-arch update
> 
>  drivers/iommu/Kconfig |    1 -
>  1 file changed, 1 deletion(-)
> 
> --- 0001/drivers/iommu/Kconfig
> +++ work/drivers/iommu/Kconfig	2015-10-18 14:58:09.080513000 +0900
> @@ -324,7 +324,6 @@ config SHMOBILE_IOMMU_L1SIZE
> 
>  config IPMMU_VMSA
>  	bool "Renesas VMSA-compatible IPMMU"
> -	depends on ARM_LPAE
>  	depends on ARCH_SHMOBILE || COMPILE_TEST
>  	select IOMMU_API
>  	select IOMMU_IO_PGTABLE_LPAE

-- 
Regards,

Laurent Pinchart

Re: [PATCH 04/06] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context

[Magnus Damm]

Hi Laurent,

On Tue, Dec 29, 2015 at 9:14 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.

Thanks for your feedback!

> On Tuesday 15 December 2015 21:02:49 Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Introduce a bitmap for context handing and convert the
>> interrupt routine to go handle all registered contexts.
>>
>> At this point the number of contexts are still limited.
>
> That's all nice, but without seeing support for multiple contexts it's hard to
> tell if the implementation is correct for multiple context purpose.
>
>> The purpose of this patch is to remove the use of the
>> ARM specific mapping variable from ipmmu_irq().
>
> Why do you want to do that ?

The purpose of this series is to be able to use the IPMMU driver on
other architectures than 32-bit ARM. The main goal is to use the
driver on 64-bit ARM where the mapping variable does not exist.

>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  drivers/iommu/ipmmu-vmsa.c |   37 ++++++++++++++++++++++++++-----------
>>  1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> --- 0007/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c   2015-12-15 13:14:35.540513000 +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,16 @@
>>
>>  #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;
>> +     DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>
> We have up to 4 context on Gen2 and 8 on Gen3, a bitmap might be slightly
> overkill.

Maybe so, but I'd rather use something standard than rolling my own.
Can you think of a better data structure?

>> +     struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
>>
>>       struct dma_iommu_mapping *mapping;
>>  };
>> @@ -319,6 +324,7 @@ static struct iommu_gather_ops ipmmu_gat
>>  static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>>  {
>>       phys_addr_t ttbr;
>> +     int ret;
>>
>>       /*
>>        * Allocate the page table operations.
>> @@ -348,10 +354,16 @@ static int ipmmu_domain_init_context(str
>>               return -EINVAL;
>>
>>       /*
>> -      * TODO: When adding support for multiple contexts, find an unused
>> -      * context.
>> +      * Find an unused context.
>
> We need to support multiple devices per context or we will very soon run out
> of contexts. How to pick a proper context is a topic that needs to be
> researched, I believe IOMMU groups might come into play.

The experimental 64-bit ARM patches for this driver (on top of this
series) that I've posted makes use of IOMMU groups.

>>        */
>> -     domain->context_id = 0;
>> +     ret = bitmap_find_free_region(domain->mmu->ctx, IPMMU_CTX_MAX, 0);
>> +     if (ret < 0) {
>> +             free_io_pgtable_ops(domain->iop);
>> +             return ret;
>> +     }
>> +
>> +     domain->context_id = ret;
>> +     domain->mmu->domains[ret] = domain;
>
> This requires locking to protect against races with the interrupt handler.

Hm, it seems that I mistakenly assumed that bitmap_find_free_region()
was built on top of atomic set_bit() and managed the bitmap in an
atomic way. I believe you are correct that locking is needed. Will
fix.

>>
>>       /* TTBR0 */
>>       ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
>> @@ -395,6 +407,8 @@ static int ipmmu_domain_init_context(str
>>
>>  static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>>  {
>> +     bitmap_release_region(domain->mmu->ctx, domain->context_id, 0);
>> +
>>       /*
>>        * Disable the context. Flush the TLB as required when modifying the
>>        * context registers.
>> @@ -460,16 +474,16 @@ 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;
>> -
>> -     if (!mmu->mapping)
>> -             return IRQ_NONE;
>> +     irqreturn_t status = IRQ_NONE;
>> +     unsigned int k;
>
> i is a perfectly fine loop counter :-)
>
>> -     io_domain = mmu->mapping->domain;
>> -     domain = to_vmsa_domain(io_domain);
>> +     /* Check interrupts for all active contexts */
>> +     for (k = find_first_bit(mmu->ctx, IPMMU_CTX_MAX);
>> +          k < IPMMU_CTX_MAX && status == IRQ_NONE;
>> +          k = find_next_bit(mmu->ctx, IPMMU_CTX_MAX, k))
>
> You can just loop over mmu->domains and skip NULL entries.

You are right, that may be easier!

>> +             status = ipmmu_domain_irq(mmu->domains[k]);
>
> Only the status of the last domain is taken into account.

Will fix, thanks!

/ magnus