Skip to contentSkip to conversation

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

PATCH Applied Magnus Damm 4 participants 13 messages May 17, 2017 – May 18, 2017 IOMMU

lore
Share & tools
lore

Magnus Damm

author
#0 lore raw

v8 cover iommu/ipmmu-vmsa: IPMMU multi-arch update V8

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

[PATCH v8 01/08] iommu/ipmmu-vmsa: Remove platform data handling [PATCH v8 02/08] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context [PATCH v8 03/08] iommu/ipmmu-vmsa: Break out utlb parsing code
[PATCH v8 04/08] iommu/ipmmu-vmsa: Break out domain allocation code [PATCH v8 05/08] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
[PATCH v8 06/08] iommu/ipmmu-vmsa: Use fwspec iommu_priv on ARM64 [PATCH v8 07/08] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency
[PATCH v8 08/08] iommu/ipmmu-vmsa: Fix pgsize_bitmap semicolon typo

These patches update the IPMMU driver with a modifications to support build on multiple architectures. In the process of doing so the interrupt code gets reworked, the foundation for supporting multiple contexts are added and in case of CONFIG_IOMMU_DMA=y (on 64-bit or 32-bit ARM) devices are grouped together and handled via ->xlate(). In this verison fwspec is used on 64-bit ARM instead of archdata. Support for existing 32-bit ARM SoCs from R-Car Gen2 is kept as-is.

Changes since V7:
- Rebased on top of v4.12-rc1
- Added Reviewed-by from Geert to patch 1/8 and 4/8, thanks!
- Reworked patch 6/8 to use fwspec, thanks Robin!
- Added patch 8/8

Changes since V6:
- Rebased on top of v4.11-rc1 and the following fast-tracked change:
3b6bb5b iommu/ipmmu-vmsa: Restrict IOMMU Domain Geometry to 32-bit address spac
- Updated patch 5/7 to roll in a few patches from other series
See individual patch for more details
- Build tested on 32-bit and 64-bit ARM
- Run time tested on 64-bit ARM (with additional SoC-specific patches)

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

---

Developed on top of a95cfad (v4.12-rc1 + fixes):

Compile tested on 32-bit and 64-bit ARM

Run time tested on 64-bit ARM r8a7796 Salvator-X

 drivers/iommu/Kconfig      |    2 
 drivers/iommu/ipmmu-vmsa.c |  421 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 331 insertions(+), 92 deletions(-)
author
#1 lore raw

PATCH v8 1/8 iommu/ipmmu-vmsa: Remove platform data handling

From: Magnus Damm <[email protected]>

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 <[email protected]>
Reviewed-by Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/[email protected]>
Reviewed-by Joerg Roedel <[email protected]>
Reviewed-by Geert Uytterhoeven <[email protected]>

---

Changes since V7:
- Added Reviewed-by from Geert - Thanks!

 drivers/iommu/ipmmu-vmsa.c |    5 -----
 1 file changed, 5 deletions(-)
drivers/iommu/ipmmu-vmsa.c +0 -5
@@ -768,11 +768,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");
author
#2 lore raw

PATCH v8 2/8 iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context

In reply to Magnus Damm

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 V7:
- None

 drivers/iommu/ipmmu-vmsa.c |   76 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 66 insertions(+), 10 deletions(-)
drivers/iommu/ipmmu-vmsa.c +66 -10
@@ -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.

@@ -327,10 +353,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];

@@ -372,6 +403,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)
 {
 	/*

@@ -382,6 +426,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);
 }
 
 /* -----------------------------------------------------------------------------

@@ -439,16 +484,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;
 }
 
 /* -----------------------------------------------------------------------------

@@ -776,6 +830,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);
author
#3 lore raw

PATCH v8 3/8 iommu/ipmmu-vmsa: Break out utlb parsing code

In reply to Magnus Damm

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 V7:
- Free archdata and utlbs in case of error

 drivers/iommu/ipmmu-vmsa.c |   64 ++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 23 deletions(-)
drivers/iommu/ipmmu-vmsa.c +41 -23
@@ -649,22 +649,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",

@@ -701,6 +694,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)) {

@@ -718,16 +741,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

@@ -738,6 +754,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;
 

@@ -762,16 +780,16 @@ static int ipmmu_add_device(struct devic
 	return 0;
 
 error:
-	arm_iommu_release_mapping(mmu->mapping);
-
-	kfree(dev->archdata.iommu);
-	kfree(utlbs);
-
-	dev->archdata.iommu = NULL;
+	if (mmu)
+		arm_iommu_release_mapping(mmu->mapping);
 
 	if (!IS_ERR_OR_NULL(group))
 		iommu_group_remove_device(dev);
 
+	kfree(archdata->utlbs);
+	kfree(archdata);
+	dev->archdata.iommu = NULL;
+
 	return ret;
 }
author
#4 lore raw

PATCH v8 4/8 iommu/ipmmu-vmsa: Break out domain allocation code

In reply to Magnus Damm

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]>
Reviewed-by Geert Uytterhoeven <[email protected]>

---

Changes since V7:
- Added Reviewed-by from Geert - Thanks!

 drivers/iommu/ipmmu-vmsa.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/iommu/ipmmu-vmsa.c +9 -4
@@ -509,13 +509,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;

@@ -525,6 +522,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);
author
#5 lore raw

PATCH v8 5/8 iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops

In reply to 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. Initialize the device from ->xlate() when CONFIG_IOMMU_DMA=y.

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

---

Changes since V7:
- None

 drivers/iommu/Kconfig      |    1 
 drivers/iommu/ipmmu-vmsa.c |  164 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 156 insertions(+), 9 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 +155 -9

221 lines not on this page · raw message

author
#6 lore raw

PATCH v8 6/8 iommu/ipmmu-vmsa: Use fwspec iommu_priv on ARM64

In reply to Magnus Damm

From: Magnus Damm <[email protected]>

Convert from archdata to iommu_priv via iommu_fwspec on ARM64 but let 32-bit ARM keep on using archdata for now.

Once the 32-bit ARM code and the IPMMU driver is able to move over to CONFIG_IOMMU_DMA=y then coverting to fwspec via ->of_xlate() will be easy.

For now fwspec ids and num_ids are not used to allow code sharing between 32-bit and 64-bit ARM code inside the driver.

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

---

Changes since V7:
- Rewrote code to use iommu_fwspec, thanks Robin!
- Dropped reviewed-by from Joerg, also skipped tag from Geert

 drivers/iommu/ipmmu-vmsa.c |   97 ++++++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 39 deletions(-)
drivers/iommu/ipmmu-vmsa.c +58 -39
@@ -56,7 +56,7 @@ struct ipmmu_vmsa_domain {
 	spinlock_t lock;			/* Protects mappings */
 };
 
-struct ipmmu_vmsa_archdata {
+struct ipmmu_vmsa_iommu_priv {
 	struct ipmmu_vmsa_device *mmu;
 	unsigned int *utlbs;
 	unsigned int num_utlbs;

@@ -72,6 +72,24 @@ static struct ipmmu_vmsa_domain *to_vmsa
 	return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
 }
 
+
+static struct ipmmu_vmsa_iommu_priv *to_priv(struct device *dev)
+{
+#if defined(CONFIG_ARM)
+	return dev->archdata.iommu;
+#else
+	return dev->iommu_fwspec->iommu_priv;
+#endif
+}
+static void set_priv(struct device *dev, struct ipmmu_vmsa_iommu_priv *p)
+{
+#if defined(CONFIG_ARM)
+	dev->archdata.iommu = p;
+#else
+	dev->iommu_fwspec->iommu_priv = p;
+#endif
+}
+
 #define TLB_LOOP_TIMEOUT		100	/* 100us */
 
 /* -----------------------------------------------------------------------------

@@ -543,8 +561,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_iommu_priv *priv = to_priv(dev);
+	struct ipmmu_vmsa_device *mmu = priv->mmu;
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned long flags;
 	unsigned int i;

@@ -577,8 +595,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 < priv->num_utlbs; ++i)
+		ipmmu_utlb_enable(domain, priv->utlbs[i]);
 
 	return 0;
 }

@@ -586,12 +604,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_iommu_priv *priv = to_priv(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 < priv->num_utlbs; ++i)
+		ipmmu_utlb_disable(domain, priv->utlbs[i]);
 
 	/*
 	 * TODO: Optimize by disabling the context when no device is attached.

@@ -654,7 +672,7 @@ static int ipmmu_find_utlbs(struct ipmmu
 
 static int ipmmu_init_platform_device(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata;
+	struct ipmmu_vmsa_iommu_priv *priv;
 	struct ipmmu_vmsa_device *mmu;
 	unsigned int *utlbs;
 	unsigned int i;

@@ -697,17 +715,17 @@ static int ipmmu_init_platform_device(st
 		}
 	}
 
-	archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
-	if (!archdata) {
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
 		ret = -ENOMEM;
 		goto error;
 	}
 
-	archdata->mmu = mmu;
-	archdata->utlbs = utlbs;
-	archdata->num_utlbs = num_utlbs;
-	archdata->dev = dev;
-	dev->archdata.iommu = archdata;
+	priv->mmu = mmu;
+	priv->utlbs = utlbs;
+	priv->num_utlbs = num_utlbs;
+	priv->dev = dev;
+	set_priv(dev, priv);
 	return 0;
 
 error:

@@ -727,12 +745,11 @@ static struct iommu_domain *ipmmu_domain
 
 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_priv(dev)) {
 		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
 			 dev_name(dev));
 		return -EINVAL;

@@ -768,8 +785,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_priv(dev)->mmu;
 	if (!mmu->mapping) {
 		struct dma_iommu_mapping *mapping;
 

@@ -800,24 +816,24 @@ error:
 	if (!IS_ERR_OR_NULL(group))
 		iommu_group_remove_device(dev);
 
-	kfree(archdata->utlbs);
-	kfree(archdata);
-	dev->archdata.iommu = NULL;
+	kfree(to_priv(dev)->utlbs);
+	kfree(to_priv(dev));
+	set_priv(dev, NULL);
 
 	return ret;
 }
 
 static void ipmmu_remove_device(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
 
 	arm_iommu_detach_device(dev);
 	iommu_group_remove_device(dev);
 
-	kfree(archdata->utlbs);
-	kfree(archdata);
+	kfree(priv->utlbs);
+	kfree(priv);
 
-	dev->archdata.iommu = NULL;
+	set_priv(dev, NULL);
 }
 
 static const struct iommu_ops ipmmu_ops = {

@@ -874,11 +890,14 @@ static void ipmmu_domain_free_dma(struct
 
 static int ipmmu_add_device_dma(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 	struct iommu_group *group;
 
-	/* The device has been verified in xlate() */
-	if (!archdata)
+	/*
+	 * Only let through devices that have been verified in xlate()
+	 * We may get called with dev->iommu_fwspec set to NULL.
+	 */
+	if (!fwspec || !fwspec->iommu_priv)
 		return -ENODEV;
 
 	group = iommu_group_get_for_dev(dev);

@@ -886,17 +905,17 @@ static int ipmmu_add_device_dma(struct d
 		return PTR_ERR(group);
 
 	spin_lock(&ipmmu_slave_devices_lock);
-	list_add(&archdata->list, &ipmmu_slave_devices);
+	list_add(&to_priv(dev)->list, &ipmmu_slave_devices);
 	spin_unlock(&ipmmu_slave_devices_lock);
 	return 0;
 }
 
 static void ipmmu_remove_device_dma(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
 
 	spin_lock(&ipmmu_slave_devices_lock);
-	list_del(&archdata->list);
+	list_del(&priv->list);
 	spin_unlock(&ipmmu_slave_devices_lock);
 
 	iommu_group_remove_device(dev);

@@ -904,16 +923,16 @@ static void ipmmu_remove_device_dma(stru
 
 static struct device *ipmmu_find_sibling_device(struct device *dev)
 {
-	struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
-	struct ipmmu_vmsa_archdata *sibling_archdata = NULL;
+	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
+	struct ipmmu_vmsa_iommu_priv *sibling_priv = NULL;
 	bool found = false;
 
 	spin_lock(&ipmmu_slave_devices_lock);
 
-	list_for_each_entry(sibling_archdata, &ipmmu_slave_devices, list) {
-		if (archdata == sibling_archdata)
+	list_for_each_entry(sibling_priv, &ipmmu_slave_devices, list) {
+		if (priv == sibling_priv)
 			continue;
-		if (sibling_archdata->mmu == archdata->mmu) {
+		if (sibling_priv->mmu == priv->mmu) {
 			found = true;
 			break;
 		}

@@ -921,7 +940,7 @@ static struct device *ipmmu_find_sibling
 
 	spin_unlock(&ipmmu_slave_devices_lock);
 
-	return found ? sibling_archdata->dev : NULL;
+	return found ? sibling_priv->dev : NULL;
 }
 
 static struct iommu_group *ipmmu_find_group_dma(struct device *dev)
reviewer
#7 lore raw

Re: [PATCH v8 06/08] iommu/ipmmu-vmsa: Use fwspec iommu_priv on ARM64

Hi Magnus,

Quoted 11 lines · Magnus Damm

From: Magnus Damm <[email protected]>

Convert from archdata to iommu_priv via iommu_fwspec on ARM64 but let 32-bit ARM keep on using archdata for now.

Once the 32-bit ARM code and the IPMMU driver is able to move over to CONFIG_IOMMU_DMA=y then coverting to fwspec via ->of_xlate() will be easy.

For now fwspec ids and num_ids are not used to allow code sharing between 32-bit and 64-bit ARM code inside the driver.

That's not what I meant at all - this just looks like a crazy unmaintainable mess that's making things that much harder to unpick in future.

Leaving the existing code fossilised and building up half of a second separate driver around it wrapped in ifdefs is not only hideous, it's more work in the end than simply pulling things into the right shape to begin with. What I meant was to start with the below (compile-tested only), and add the of_xlate support on top. AFAICS the arm/arm64 differences overall should only come down to a bit of special-casing in add_device(), and (optionally) whether you outright reject IOMMU_DOMAIN_DMA or not.

Sorry, but I just can't agree with the two-drivers-in-one approach.

Robin.

----->8-----
From: Robin Murphy <[email protected]>
Subject: [PATCH] iommu/ipmmu-vmsa: Convert to iommu_fwspec

The parent IPMMU pointer and set of uTLB IDs are, as intended, a perfect fit for the generic iommu_fwspec. Get rid of the architecture-specific archdata handling in favour of the common solution.

Signed-off-by Robin Murphy <[email protected]>
---
 drivers/iommu/ipmmu-vmsa.c | 68
++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 39 deletions(-)
drivers/iommu/ipmmu-vmsa.c +2 -6
index b7e14ee863f9..96479690269f 100644
@@ -47,12 +47,6 @@ struct ipmmu_vmsa_domain {
 	spinlock_t lock;			/* Protects mappings */
 };
 
-struct ipmmu_vmsa_archdata {
-	struct ipmmu_vmsa_device *mmu;
-	unsigned int *utlbs;
-	unsigned int num_utlbs;
-};
-
 static DEFINE_SPINLOCK(ipmmu_devices_lock);
 static LIST_HEAD(ipmmu_devices);
 

@@ -455,6 +449,8 @@ static irqreturn_t ipmmu_irq(int irq, void *dev)
  * IOMMU Operations
  */
 
+static const struct iommu_ops ipmmu_ops;
+
 static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
 {
 	struct ipmmu_vmsa_domain *domain;

@@ -487,18 +483,20 @@ static void ipmmu_domain_free(struct iommu_domain

*io_domain)
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 iommu_fwspec *fwspec = dev->iommu_fwspec;
+ struct ipmmu_vmsa_device *mmu;
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain); unsigned long flags;
unsigned int i;
int ret = 0;

- if (!mmu) {
+ if (!fwspec || fwspec->ops != &ipmmu_ops) {
dev_err(dev, "Cannot attach to IPMMU\n"); return -ENXIO;
}

+ mmu = fwspec->iommu_priv;
+
spin_lock_irqsave(&domain->lock, flags);

if (!domain->mmu) {
@@ -520,8 +518,8 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
if (ret < 0)
return ret;

- for (i = 0; i < archdata->num_utlbs; ++i)
- ipmmu_utlb_enable(domain, archdata->utlbs[i]);
+ for (i = 0; i < fwspec->num_ids; ++i)
+ ipmmu_utlb_enable(domain, fwspec->ids[i]);

return 0;
}
@@ -529,12 +527,15 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
static void ipmmu_detach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
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]);
+ if (!fwspec || fwspec->ops != &ipmmu_ops)
+ return;
+
+ for (i = 0; i < fwspec->num_ids; ++i)
+ ipmmu_utlb_disable(domain, fwspec->ids[i]);

/*
* TODO: Optimize by disabling the context when no device is attached.
@@ -597,7 +598,6 @@ static int ipmmu_find_utlbs(struct ipmmu_vmsa_device *mmu, struct device *dev,

static int ipmmu_add_device(struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata;
struct ipmmu_vmsa_device *mmu;
struct iommu_group *group = NULL;
unsigned int *utlbs;
@@ -605,7 +605,7 @@ static int ipmmu_add_device(struct device *dev)
int num_utlbs;
int ret = -ENODEV;

- if (dev->archdata.iommu) {
+ if (dev->iommu_fwspec) {
dev_warn(dev, "IOMMU driver already assigned to device %s\n",
dev_name(dev));
return -EINVAL;
@@ -638,13 +638,20 @@ static int ipmmu_add_device(struct device *dev)
spin_unlock(&ipmmu_devices_lock);

if (ret < 0)
- goto error;
+ return ret;
+
+ ret = iommu_fwspec_init(dev, mmu->dev->fwnode, &ipmmu_ops);
+ if (ret)
+ return ret;
+
+ dev->iommu_fwspec->iommu_priv = mmu;

for (i = 0; i < num_utlbs; ++i) {
if (utlbs[i] >= mmu->num_utlbs) {
ret = -EINVAL;
goto error;
}
+ iommu_fwspec_add_ids(dev, &utlbs[i], 1);
}

/* Create a device group and add the device to it. */
@@ -664,17 +671,6 @@ static int ipmmu_add_device(struct device *dev)
goto error;
}

- 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;
-
/*
* Create the ARM mapping, used by the ARM DMA mapping core to allocate
* VAs. This will allocate a corresponding IOMMU domain.
@@ -710,28 +706,22 @@ static int ipmmu_add_device(struct device *dev)
error:
arm_iommu_release_mapping(mmu->mapping);

- kfree(dev->archdata.iommu);
- kfree(utlbs);
-
- dev->archdata.iommu = NULL;
-
if (!IS_ERR_OR_NULL(group))
iommu_group_remove_device(dev);

+ iommu_fwspec_free(dev);
+
return ret;
}

static void ipmmu_remove_device(struct device *dev) {
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ if (!dev->iommu_fwspec || dev->iommu_fwspec->ops != &ipmmu_ops)
+ return;

arm_iommu_detach_device(dev);
iommu_group_remove_device(dev);
-
- kfree(archdata->utlbs);
- kfree(archdata);
-
- dev->archdata.iommu = NULL;
+ iommu_fwspec_free(dev);
}

static const struct iommu_ops ipmmu_ops = {

author
#8 lore raw

Re: [PATCH v8 06/08] iommu/ipmmu-vmsa: Use fwspec iommu_priv on ARM64

Hi Robin,

Quoted 28 lines · Robin Murphy

Hi Magnus,

Quoted 11 lines · Magnus Damm

From: Magnus Damm <[email protected]>

Convert from archdata to iommu_priv via iommu_fwspec on ARM64 but let 32-bit ARM keep on using archdata for now.

Once the 32-bit ARM code and the IPMMU driver is able to move over to CONFIG_IOMMU_DMA=y then coverting to fwspec via ->of_xlate() will be easy.

For now fwspec ids and num_ids are not used to allow code sharing between 32-bit and 64-bit ARM code inside the driver.

That's not what I meant at all - this just looks like a crazy unmaintainable mess that's making things that much harder to unpick in future.

Leaving the existing code fossilised and building up half of a second separate driver around it wrapped in ifdefs is not only hideous, it's more work in the end than simply pulling things into the right shape to begin with. What I meant was to start with the below (compile-tested only), and add the of_xlate support on top. AFAICS the arm/arm64 differences overall should only come down to a bit of special-casing in add_device(), and (optionally) whether you outright reject IOMMU_DOMAIN_DMA or not.

Quoted 1 line

Sorry, but I just can't agree with the two-drivers-in-one approach.

Thanks for checking the code and sorry to disappoint you by not using ->ids[] and ->num_ids right away. The two-drivers-on-one approach comes from wanting to use the same IOMMU driver on both 32-bit and 64-bit ARM architectures but the former is lacking IOMMU_DMA support upstream.

Obviously using ->ids[] and ->num_ids is the right forward, and in my mind it is only a question of time and merge order. I'm more than happy to make use of your patch but I'm wondering if it will work on 32-bit ARM and R-Car Gen2 that currently does not use ->of_xlate(). I can see that you're using iommu_fwspec_init() so it might work right out of the box. Thanks for your help cooking up the patch by the way.

Quoted 1 line

From my side I was hoping on doing one larger feature jump for 32-bit

ARM by moving over to IOMMU_DMA=y together with ->of_xlate()and fwspec at the same time. Doing minor increments of the legacy code that is still using special mapping code instead of OMMU_DMA=y in case of 32-bit ARM just feels like a lot of potential breakage and little gain to me.

What's the plan for supporting IOMMU_DMA=y on 32-bit ARM? Anything I can do to help? Or do you prefer minor increments on 32-bit ARM over larger feature jumps?

Let me know what you think. My plan is to revisit this topic early next week.

Cheers,

/ magnus

author
#9 lore raw

PATCH v8 7/8 iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency

In reply to Magnus Damm

From: Magnus Damm <[email protected]>

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 <[email protected]>
Acked-by Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/[email protected]>
Reviewed-by Joerg Roedel <[email protected]>

---

Changes since V7:
- None

 drivers/iommu/Kconfig |    1 -
 1 file changed, 1 deletion(-)
drivers/iommu/Kconfig +0 -1
@@ -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
author
#10 lore raw

PATCH v8 8/8 iommu/ipmmu-vmsa: Fix pgsize_bitmap semicolon typo

In reply to Magnus Damm

From: Magnus Damm <[email protected]>

Fix comma-instead-of-semicolon typo error present
in the latest version of the IPMMU driver.

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

---

Earlier posted as:
[PATCH] iommu/ipmmu-vmsa: Fix pgsize_bitmap semicolon typo

 drivers/iommu/ipmmu-vmsa.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/ipmmu-vmsa.c +1 -1
@@ -358,7 +358,7 @@ static int ipmmu_domain_init_context(str
 	 * non-secure mode.
 	 */
 	domain->cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS;
-	domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
+	domain->cfg.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K;
 	domain->cfg.ias = 32;
 	domain->cfg.oas = 40;
 	domain->cfg.tlb = &ipmmu_gather_ops;
Reviewed-by on v8 8/8
#11 lore raw

Re: [PATCH v8 08/08] iommu/ipmmu-vmsa: Fix pgsize_bitmap semicolon typo

Quoted 6 lines · Magnus Damm

From: Magnus Damm <[email protected]>

Fix comma-instead-of-semicolon typo error present
in the latest version of the IPMMU driver.

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

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

maintainer Applied
#12 lore raw
In reply to Magnus Damm
Quoted 10 lines · Magnus Damm

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

[PATCH v8 01/08] iommu/ipmmu-vmsa: Remove platform data handling [PATCH v8 02/08] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for context [PATCH v8 03/08] iommu/ipmmu-vmsa: Break out utlb parsing code
[PATCH v8 04/08] iommu/ipmmu-vmsa: Break out domain allocation code [PATCH v8 05/08] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
[PATCH v8 06/08] iommu/ipmmu-vmsa: Use fwspec iommu_priv on ARM64 [PATCH v8 07/08] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency
[PATCH v8 08/08] iommu/ipmmu-vmsa: Fix pgsize_bitmap semicolon typo

Applied, thanks.