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

[Magnus Damm]

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

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

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

Changes since V5:
- Rebased series on top of next-20161019
- Updated patch 5/7 to simplify domain allocation/free code - thanks Joerg!
- Added reviewed-by tag from Joerg for patch 1-4 and 6-7.

Changes since V4:
- Updated patch 3/7 to work on top on the following commit in next-20160920:
b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device
- Add Kconfig hunk to patch 5/7 to avoid undeclared ipmmu_ops if COMPILE_TEST
- Rebased patch 7/7 to fit on top of new Kconfig bits in 5/7

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

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

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

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

Built on top next-20161019:

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

[Magnus Damm]

From: Magnus Damm <damm+***@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+***@opensource.se>
Reviewed-by: Laurent Pinchart <***@ideasonboard.com>
Reviewed-by: Joerg Roedel <***@suse.de>
---

Changes since V5:
- None

Changes since V4:
- None

Changes since V3:
- None

Changes since V2:
- None

Changes since V1:
- Added Reviewed-by from Laurent

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

--- 0002/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:46:26.000000000 +0900
@@ -766,11 +766,6 @@ static int ipmmu_probe(struct platform_d
int irq;
int ret;

- if (!IS_ENABLED(CONFIG_OF) && !pdev->dev.platform_data) {
- dev_err(&pdev->dev, "missing platform data\n");
- return -EINVAL;
- }
-
mmu = devm_kzalloc(&pdev->dev, sizeof(*mmu), GFP_KERNEL);
if (!mmu) {
dev_err(&pdev->dev, "cannot allocate device data\n");

[Magnus Damm]

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

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

At this point the number of contexts are still limited.

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

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

Changes since V5:
- None

Changes since V4:
- None

Changes since V3:
- None

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

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

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

--- 0004/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:48:23.770607110 +0900
@@ -8,6 +8,7 @@
* the Free Software Foundation; version 2 of the License.
*/

+#include <linux/bitmap.h>
#include <linux/delay.h>
#include <linux/dma-mapping.h>
#include <linux/err.h>
@@ -26,12 +27,17 @@

#include "io-pgtable.h"

+#define IPMMU_CTX_MAX 1
+
struct ipmmu_vmsa_device {
struct device *dev;
void __iomem *base;
struct list_head list;

unsigned int num_utlbs;
+ spinlock_t lock; /* Protects ctx and domains[] */
+ DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
+ struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];

struct dma_iommu_mapping *mapping;
};
@@ -293,9 +299,29 @@ static struct iommu_gather_ops ipmmu_gat
* Domain/Context Management
*/

+static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
+ struct ipmmu_vmsa_domain *domain)
+{
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&mmu->lock, flags);
+
+ ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
+ if (ret != IPMMU_CTX_MAX) {
+ mmu->domains[ret] = domain;
+ set_bit(ret, mmu->ctx);
+ }
+
+ spin_unlock_irqrestore(&mmu->lock, flags);
+
+ return ret;
+}
+
static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
{
u64 ttbr;
+ int ret;

/*
* Allocate the page table operations.
@@ -325,10 +351,15 @@ static int ipmmu_domain_init_context(str
return -EINVAL;

/*
- * TODO: When adding support for multiple contexts, find an unused
- * context.
+ * Find an unused context.
*/
- domain->context_id = 0;
+ ret = ipmmu_domain_allocate_context(domain->mmu, domain);
+ if (ret == IPMMU_CTX_MAX) {
+ free_io_pgtable_ops(domain->iop);
+ return -EBUSY;
+ }
+
+ domain->context_id = ret;

/* TTBR0 */
ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
@@ -370,6 +401,19 @@ static int ipmmu_domain_init_context(str
return 0;
}

+static void ipmmu_domain_free_context(struct ipmmu_vmsa_device *mmu,
+ unsigned int context_id)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&mmu->lock, flags);
+
+ clear_bit(context_id, mmu->ctx);
+ mmu->domains[context_id] = NULL;
+
+ spin_unlock_irqrestore(&mmu->lock, flags);
+}
+
static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
{
/*
@@ -380,6 +424,7 @@ static void ipmmu_domain_destroy_context
*/
ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
ipmmu_tlb_sync(domain);
+ ipmmu_domain_free_context(domain->mmu, domain->context_id);
}

/* -----------------------------------------------------------------------------
@@ -437,16 +482,25 @@ static irqreturn_t ipmmu_domain_irq(stru
static irqreturn_t ipmmu_irq(int irq, void *dev)
{
struct ipmmu_vmsa_device *mmu = dev;
- struct iommu_domain *io_domain;
- struct ipmmu_vmsa_domain *domain;
+ irqreturn_t status = IRQ_NONE;
+ unsigned int i;
+ unsigned long flags;

- if (!mmu->mapping)
- return IRQ_NONE;
+ spin_lock_irqsave(&mmu->lock, flags);
+
+ /*
+ * Check interrupts for all active contexts.
+ */
+ for (i = 0; i < IPMMU_CTX_MAX; i++) {
+ if (!mmu->domains[i])
+ continue;
+ if (ipmmu_domain_irq(mmu->domains[i]) == IRQ_HANDLED)
+ status = IRQ_HANDLED;
+ }

- io_domain = mmu->mapping->domain;
- domain = to_vmsa_domain(io_domain);
+ spin_unlock_irqrestore(&mmu->lock, flags);

- return ipmmu_domain_irq(domain);
+ return status;
}

/* -----------------------------------------------------------------------------
@@ -774,6 +828,8 @@ static int ipmmu_probe(struct platform_d

mmu->dev = &pdev->dev;
mmu->num_utlbs = 32;
+ spin_lock_init(&mmu->lock);
+ bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);

/* Map I/O memory and request IRQ. */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

Post by Magnus Damm
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.
---
- None
- None
- None
Changes since V2: (Thanks again to Laurent!)
- Introduce a spinlock together with the bitmap and domain array.
- Break out code into separate functions for alloc and free.
- Perform free after (instead of before) configuring hardware registers.
- Use the spinlock to protect the domain array in the interrupt handler.
Changes since V1: (Thanks to Laurent for feedback!)
- Use simple find_first_zero()/set_bit()/clear_bit() for context
management. - For allocation rely on spinlock held when calling
ipmmu_domain_init_context() - For test/free use atomic bitops
- Return IRQ_HANDLED if any of the contexts generated interrupts
drivers/iommu/ipmmu-vmsa.c | 76 +++++++++++++++++++++++++++++++++++------
1 file changed, 66 insertions(+), 10 deletions(-)
--- 0004/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:48:23.770607110 +0900
@@ -8,6 +8,7 @@
* the Free Software Foundation; version 2 of the License.
*/
+#include <linux/bitmap.h>
#include <linux/delay.h>
#include <linux/dma-mapping.h>
#include <linux/err.h>
@@ -26,12 +27,17 @@
#include "io-pgtable.h"
+#define IPMMU_CTX_MAX 1
+
struct ipmmu_vmsa_device {
struct device *dev;
void __iomem *base;
struct list_head list;
unsigned int num_utlbs;
+ spinlock_t lock; /* Protects ctx and domains[]

*/

Post by Magnus Damm
+ DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
+ struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
struct dma_iommu_mapping *mapping;
};
@@ -293,9 +299,29 @@ static struct iommu_gather_ops ipmmu_gat
* Domain/Context Management
*/
+static int ipmmu_domain_allocate_context(struct ipmmu_vmsa_device *mmu,
+ struct ipmmu_vmsa_domain *domain)

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

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

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

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

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

The comment now holds on one line.

Post by Magnus Damm
- 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);
}
/*
---------------------------------------------------------------------------
static irqreturn_t ipmmu_irq(int irq, void *dev)
{
struct ipmmu_vmsa_device *mmu = dev;
- struct iommu_domain *io_domain;
- struct ipmmu_vmsa_domain *domain;
+ irqreturn_t status = IRQ_NONE;
+ unsigned int i;
+ unsigned long flags;

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

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

This comment holds on a single line too.

With all these small comments addressed,

Post by Magnus Damm
+ 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;
}
/*
---------------------------------------------------------------------------
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);

[Magnus Damm]

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

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

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

Changes since V5:
- None

Changes since V4:
- Dropped hunk with fix to apply on top of:
b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device

Changes since V3:
- Initialize "mmu" to NULL, check before accessing
- Removed group parameter from ipmmu_init_platform_device()

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

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

--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:49:34.580607110 +0900
@@ -647,22 +647,15 @@ static int ipmmu_find_utlbs(struct ipmmu
return 0;
}

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

- if (dev->archdata.iommu) {
- dev_warn(dev, "IOMMU driver already assigned to device %s\n",
- dev_name(dev));
- return -EINVAL;
- }
-
/* Find the master corresponding to the device. */

num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
@@ -699,6 +692,36 @@ static int ipmmu_add_device(struct devic
}
}

+ archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
+ if (!archdata) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ archdata->mmu = mmu;
+ archdata->utlbs = utlbs;
+ archdata->num_utlbs = num_utlbs;
+ dev->archdata.iommu = archdata;
+ return 0;
+
+error:
+ kfree(utlbs);
+ return ret;
+}
+
+static int ipmmu_add_device(struct device *dev)
+{
+ struct ipmmu_vmsa_archdata *archdata;
+ struct ipmmu_vmsa_device *mmu = NULL;
+ struct iommu_group *group;
+ int ret;
+
+ if (dev->archdata.iommu) {
+ dev_warn(dev, "IOMMU driver already assigned to device %s\n",
+ dev_name(dev));
+ return -EINVAL;
+ }
+
/* Create a device group and add the device to it. */
group = iommu_group_alloc();
if (IS_ERR(group)) {
@@ -716,16 +739,9 @@ static int ipmmu_add_device(struct devic
goto error;
}

- archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
- if (!archdata) {
- ret = -ENOMEM;
+ ret = ipmmu_init_platform_device(dev);
+ if (ret < 0)
goto error;
- }
-
- archdata->mmu = mmu;
- archdata->utlbs = utlbs;
- archdata->num_utlbs = num_utlbs;
- dev->archdata.iommu = archdata;

/*
* Create the ARM mapping, used by the ARM DMA mapping core to allocate
@@ -736,6 +752,8 @@ static int ipmmu_add_device(struct devic
* - Make the mapping size configurable ? We currently use a 2GB mapping
* at a 1GB offset to ensure that NULL VAs will fault.
*/
+ archdata = dev->archdata.iommu;
+ mmu = archdata->mmu;
if (!mmu->mapping) {
struct dma_iommu_mapping *mapping;

@@ -760,10 +778,8 @@ static int ipmmu_add_device(struct devic
return 0;

error:
- arm_iommu_release_mapping(mmu->mapping);
-
- kfree(dev->archdata.iommu);
- kfree(utlbs);
+ if (mmu)
+ arm_iommu_release_mapping(mmu->mapping);

dev->archdata.iommu = NULL;

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

Post by Magnus Damm
Break out the utlb parsing code and dev_data allocation into a
separate function. This is preparation for future code sharing.
---
- None
b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device
- Initialize "mmu" to NULL, check before accessing
- Removed group parameter from ipmmu_init_platform_device()
[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
- Reworked code to fit on top on previous two patches in current series.
drivers/iommu/ipmmu-vmsa.c | 58 ++++++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 21 deletions(-)
--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:49:34.580607110 +0900
@@ -647,22 +647,15 @@ static int ipmmu_find_utlbs(struct ipmmu
return 0;
}
-static int ipmmu_add_device(struct device *dev)
+static int ipmmu_init_platform_device(struct device *dev)

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

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

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

Post by Magnus Damm
dev->archdata.iommu = NULL;

[Magnus Damm]

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

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

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

Changes since V5:
- None

Changes since V4:
- None

Changes since V3:
- None

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

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

--- 0008/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:56:59.220607110 +0900
@@ -507,13 +507,10 @@ static irqreturn_t ipmmu_irq(int irq, vo
* IOMMU Operations
*/

-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
{
struct ipmmu_vmsa_domain *domain;

- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;
-
domain = kzalloc(sizeof(*domain), GFP_KERNEL);
if (!domain)
return NULL;
@@ -523,6 +520,14 @@ static struct iommu_domain *ipmmu_domain
return &domain->io_domain;
}

+static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+{
+ if (type != IOMMU_DOMAIN_UNMANAGED)
+ return NULL;
+
+ return __ipmmu_domain_alloc(type);
+}
+
static void ipmmu_domain_free(struct iommu_domain *io_domain)
{
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

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

This looks good to me,

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

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

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

[Magnus Damm]

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

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

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

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

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

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

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

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

--- 0001/drivers/iommu/Kconfig
+++ work/drivers/iommu/Kconfig 2016-10-20 08:16:40.980607110 +0900
@@ -274,6 +274,7 @@ config EXYNOS_IOMMU_DEBUG

config IPMMU_VMSA
bool "Renesas VMSA-compatible IPMMU"
+ depends on ARM || IOMMU_DMA
depends on ARM_LPAE
depends on ARCH_RENESAS || COMPILE_TEST
select IOMMU_API
--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-10-20 08:16:48.440607110 +0900
@@ -10,6 +10,7 @@

#include <linux/bitmap.h>
#include <linux/delay.h>
+#include <linux/dma-iommu.h>
#include <linux/dma-mapping.h>
#include <linux/err.h>
#include <linux/export.h>
@@ -22,8 +23,10 @@
#include <linux/sizes.h>
#include <linux/slab.h>

+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
#include <asm/dma-iommu.h>
#include <asm/pgalloc.h>
+#endif

#include "io-pgtable.h"

@@ -520,14 +523,6 @@ static struct iommu_domain *__ipmmu_doma
return &domain->io_domain;
}

-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;
-
- return __ipmmu_domain_alloc(type);
-}
-
static void ipmmu_domain_free(struct iommu_domain *io_domain)
{
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
@@ -714,6 +709,8 @@ error:
return ret;
}

+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
+
static int ipmmu_add_device(struct device *dev)
{
struct ipmmu_vmsa_archdata *archdata;
@@ -807,6 +804,14 @@ static void ipmmu_remove_device(struct d
dev->archdata.iommu = NULL;
}

+static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
+{
+ if (type != IOMMU_DOMAIN_UNMANAGED)
+ return NULL;
+
+ return __ipmmu_domain_alloc(type);
+}
+
static const struct iommu_ops ipmmu_ops = {
.domain_alloc = ipmmu_domain_alloc,
.domain_free = ipmmu_domain_free,
@@ -821,6 +826,105 @@ static const struct iommu_ops ipmmu_ops
.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
};

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

+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
arm_iommu_release_mapping(mmu->mapping);
+#endif

ipmmu_device_reset(mmu);

[Robin Murphy]

Post by Magnus Damm
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.
---
- Made domain allocation/free code more consistent - thanks Joerg!
- Added Kconfig hunk to depend on ARM or IOMMU_DMA
- Removed group parameter from ipmmu_init_platform_device()
[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
- Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
- Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
- of_xlate() is now used without #ifdefs
- Made sure code compiles on both 32-bit and 64-bit ARM.
drivers/iommu/Kconfig | 1
drivers/iommu/ipmmu-vmsa.c | 122 +++++++++++++++++++++++++++++++++++++++++---
2 files changed, 115 insertions(+), 8 deletions(-)
--- 0001/drivers/iommu/Kconfig
+++ work/drivers/iommu/Kconfig 2016-10-20 08:16:40.980607110 +0900
@@ -274,6 +274,7 @@ config EXYNOS_IOMMU_DEBUG
config IPMMU_VMSA
bool "Renesas VMSA-compatible IPMMU"
+ depends on ARM || IOMMU_DMA
depends on ARM_LPAE
depends on ARCH_RENESAS || COMPILE_TEST
select IOMMU_API
--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-10-20 08:16:48.440607110 +0900
@@ -10,6 +10,7 @@
#include <linux/bitmap.h>
#include <linux/delay.h>
+#include <linux/dma-iommu.h>
#include <linux/dma-mapping.h>
#include <linux/err.h>
#include <linux/export.h>
@@ -22,8 +23,10 @@
#include <linux/sizes.h>
#include <linux/slab.h>
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
#include <asm/dma-iommu.h>
#include <asm/pgalloc.h>
+#endif
#include "io-pgtable.h"
@@ -520,14 +523,6 @@ static struct iommu_domain *__ipmmu_doma
return &domain->io_domain;
}
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;

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

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

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

Post by Magnus Damm
-
- 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);
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) {
+ io_domain = __ipmmu_domain_alloc(type);
+ break;
+
+ io_domain = __ipmmu_domain_alloc(type);
+ if (io_domain)
+ iommu_get_dma_cookie(io_domain);

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

Robin.

Post by Magnus Damm
+ break;
+ }
+
+ return io_domain;
+}
+
+static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
+{
+ switch (io_domain->type) {
+ iommu_put_dma_cookie(io_domain);
+ /* fall-through */
+ 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);

[Joerg Roedel]

Post by Robin Murphy

Post by Magnus Damm
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;

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

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


Joerg

[Laurent Pinchart]

Hello,

Post by Joerg Roedel

Post by Robin Murphy

Post by Magnus Damm
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;

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

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

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

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

[Joerg Roedel]

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

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



Joerg

[Laurent Pinchart]

Hi Robin,

Post by Robin Murphy

Post by Magnus Damm
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.
---
- Made domain allocation/free code more consistent - thanks Joerg!
- Added Kconfig hunk to depend on ARM or IOMMU_DMA
- Removed group parameter from ipmmu_init_platform_device()
[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]

Post by Robin Murphy

Post by Magnus Damm
--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-10-20 08:16:48.440607110 +0900

[snip]

Post by Robin Murphy

Post by Magnus Damm
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;

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

I assume you meant

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

But how about just

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

as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?

Post by Robin Murphy
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.

Post by Magnus Damm
-
- return __ipmmu_domain_alloc(type);
-}
-

[Robin Murphy]

Hi Laurent,

Post by Laurent Pinchart
Hi Robin,

Post by Robin Murphy

Post by Magnus Damm
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.
---
- Made domain allocation/free code more consistent - thanks Joerg!
- Added Kconfig hunk to depend on ARM or IOMMU_DMA
- Removed group parameter from ipmmu_init_platform_device()
[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]

Post by Robin Murphy

Post by Magnus Damm
--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-10-20 08:16:48.440607110 +0900

[snip]

Post by Robin Murphy

Post by Magnus Damm
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;

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

I assume you meant
if (type != IOMMU_DOMAIN_UNMANAGED &&
(!IS_ENABLED(CONFIG_IOMMU_DMA) || type != IOMMU_DOMAIN_DMA))
return NULL;
But how about just
if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA))
return NULL;
as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?

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

Robin.

Post by Laurent Pinchart

Post by Robin Murphy
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.

Post by Magnus Damm
-
- return __ipmmu_domain_alloc(type);
-}
-

[Laurent Pinchart]

Hi Robin,

Post by Robin Murphy

Post by Laurent Pinchart

Post by Robin Murphy

Post by Magnus Damm
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.
---
- Made domain allocation/free code more consistent - thanks Joerg!
- Added Kconfig hunk to depend on ARM or IOMMU_DMA
- Removed group parameter from ipmmu_init_platform_device()
[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]

Post by Robin Murphy

Post by Magnus Damm
--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c
2016-10-20 08:16:48.440607110 +0900

[snip]

Post by Robin Murphy

Post by Magnus Damm
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;

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

I assume you meant
if (type != IOMMU_DOMAIN_UNMANAGED &&
(!IS_ENABLED(CONFIG_IOMMU_DMA) || type != IOMMU_DOMAIN_DMA))
return NULL;
But how about just
if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA))
return NULL;
as type will never be set to IOMMU_DOMAIN_DMA on ARM32 ?

Actually it can be, but *only* at this point, because
iommu_group_get_for_dev() will always attempt to allocate a default
domain.

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

Post by Robin Murphy
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.

Post by Laurent Pinchart

Post by Robin Murphy
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.

Post by Magnus Damm
-
- return __ipmmu_domain_alloc(type);
-}
-

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

Post by Magnus Damm
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.
---
- Made domain allocation/free code more consistent - thanks Joerg!
- Added Kconfig hunk to depend on ARM or IOMMU_DMA
- Removed group parameter from ipmmu_init_platform_device()
[PATCH 00/04] iommu/ipmmu-vmsa: IPMMU CONFIG_IOMMU_DMA update
- Use only a single iommu_ops structure with #ifdef CONFIG_IOMMU_DMA
- Folded in #ifdefs to handle CONFIG_ARM and CONFIG_IOMMU_DMA
- of_xlate() is now used without #ifdefs
- Made sure code compiles on both 32-bit and 64-bit ARM.
drivers/iommu/Kconfig | 1
drivers/iommu/ipmmu-vmsa.c | 122 ++++++++++++++++++++++++++++++++++++++---
2 files changed, 115 insertions(+), 8 deletions(-)
--- 0001/drivers/iommu/Kconfig
+++ work/drivers/iommu/Kconfig 2016-10-20 08:16:40.980607110 +0900
@@ -274,6 +274,7 @@ config EXYNOS_IOMMU_DEBUG
config IPMMU_VMSA
bool "Renesas VMSA-compatible IPMMU"
+ depends on ARM || IOMMU_DMA
depends on ARM_LPAE
depends on ARCH_RENESAS || COMPILE_TEST
select IOMMU_API
--- 0006/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-10-20 08:16:48.440607110 +0900
@@ -10,6 +10,7 @@
#include <linux/bitmap.h>
#include <linux/delay.h>
+#include <linux/dma-iommu.h>
#include <linux/dma-mapping.h>
#include <linux/err.h>
#include <linux/export.h>
@@ -22,8 +23,10 @@
#include <linux/sizes.h>
#include <linux/slab.h>
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
#include <asm/dma-iommu.h>
#include <asm/pgalloc.h>
+#endif
#include "io-pgtable.h"
@@ -520,14 +523,6 @@ static struct iommu_domain *__ipmmu_doma
return &domain->io_domain;
}
-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;
-
- return __ipmmu_domain_alloc(type);
-}
-
static void ipmmu_domain_free(struct iommu_domain *io_domain)
{
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
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) {
+ io_domain = __ipmmu_domain_alloc(type);
+ break;
+
+ 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) {
+ iommu_put_dma_cookie(io_domain);
+ /* fall-through */
+ ipmmu_domain_free(io_domain);
+ break;
+ }
+}
+
+static int ipmmu_add_device_dma(struct device *dev)
+{
+ struct iommu_group *group;
+
+ /* only accept devices with iommus property */

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

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

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

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

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

Post by Magnus Damm
+};
+
+#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);

[Laurent Pinchart]

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

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

Hi Magnus,

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

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 11550ac90d65..827aa72aaf28 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -529,16 +529,22 @@ static irqreturn_t ipmmu_irq(int irq, void *dev)
* IOMMU Operations
*/

-static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
+static struct iommu_domain *ipmmu_domain_alloc(unsigned int type)
{
struct ipmmu_vmsa_domain *domain;

+ if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+ return NULL;
+
domain = kzalloc(sizeof(*domain), GFP_KERNEL);
if (!domain)
return NULL;

spin_lock_init(&domain->lock);

+ if (type == IOMMU_DOMAIN_DMA)
+ iommu_get_dma_cookie(&domain->io_domain);
+
return &domain->io_domain;
}

@@ -546,6 +552,9 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain)
{
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);

+ if (io_domain->type)
+ iommu_put_dma_cookie(io_domain);
+
/*
* Free the domain resources. We assume that all devices have already
* been detached.
@@ -821,14 +830,6 @@ static void ipmmu_remove_device(struct device *dev)
set_archdata(dev, NULL);
}

-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- if (type != IOMMU_DOMAIN_UNMANAGED)
- return NULL;
-
- return __ipmmu_domain_alloc(type);
-}
-
static const struct iommu_ops ipmmu_ops = {
.domain_alloc = ipmmu_domain_alloc,
.domain_free = ipmmu_domain_free,
@@ -847,42 +848,11 @@ static const struct iommu_ops ipmmu_ops = {

#ifdef CONFIG_IOMMU_DMA

-static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
-{
- struct iommu_domain *io_domain = NULL;
-
- switch (type) {
- case IOMMU_DOMAIN_UNMANAGED:
- io_domain = __ipmmu_domain_alloc(type);
- break;
-
- case IOMMU_DOMAIN_DMA:
- io_domain = __ipmmu_domain_alloc(type);
- if (io_domain)
- iommu_get_dma_cookie(io_domain);
- break;
- }
-
- return io_domain;
-}
-
-static void ipmmu_domain_free_dma(struct iommu_domain *io_domain)
-{
- switch (io_domain->type) {
- case IOMMU_DOMAIN_DMA:
- iommu_put_dma_cookie(io_domain);
- /* fall-through */
- default:
- ipmmu_domain_free(io_domain);
- break;
- }
-}
-
static int ipmmu_add_device_dma(struct device *dev)
{
struct iommu_group *group;

- /* only accept devices with iommus property */
+ /* Only accept devices with an iommus property. */
if (of_count_phandle_with_args(dev->of_node, "iommus",
"#iommu-cells") < 0)
return -ENODEV;
@@ -920,13 +890,13 @@ static struct iommu_group *ipmmu_device_group_dma(struct device *dev)
static int ipmmu_of_xlate_dma(struct device *dev,
struct of_phandle_args *spec)
{
- /* dummy callback to satisfy of_iommu_configure() */
+ /* Dummy callback to satisfy of_iommu_configure(). */
return 0;
}

static const struct iommu_ops ipmmu_ops = {
- .domain_alloc = ipmmu_domain_alloc_dma,
- .domain_free = ipmmu_domain_free_dma,
+ .domain_alloc = ipmmu_domain_alloc,
+ .domain_free = ipmmu_domain_free,
.attach_dev = ipmmu_attach_device,
.detach_dev = ipmmu_detach_device,
.map = ipmmu_map,

[Magnus Damm]

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

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

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

Changes since V5:
- None

Changes since V4:
- None

Changes since V3:
- New patch

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

--- 0012/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:59:21.690607110 +0900
@@ -70,6 +70,25 @@ static struct ipmmu_vmsa_domain *to_vmsa
return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
}

+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
+{
+ return dev->archdata.iommu;
+}
+static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p)
+{
+ dev->archdata.iommu = p;
+}
+#else
+static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
+{
+ return NULL;
+}
+static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p)
+{
+}
+#endif
+
#define TLB_LOOP_TIMEOUT 100 /* 100us */

/* -----------------------------------------------------------------------------
@@ -539,7 +558,7 @@ static void ipmmu_domain_free(struct iom
static int ipmmu_attach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
struct ipmmu_vmsa_device *mmu = archdata->mmu;
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned long flags;
@@ -581,7 +600,7 @@ static int ipmmu_attach_device(struct io
static void ipmmu_detach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned int i;

@@ -701,7 +720,7 @@ static int ipmmu_init_platform_device(st
archdata->mmu = mmu;
archdata->utlbs = utlbs;
archdata->num_utlbs = num_utlbs;
- dev->archdata.iommu = archdata;
+ set_archdata(dev, archdata);
return 0;

error:
@@ -713,12 +732,11 @@ error:

static int ipmmu_add_device(struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata;
struct ipmmu_vmsa_device *mmu = NULL;
struct iommu_group *group;
int ret;

- if (dev->archdata.iommu) {
+ if (to_archdata(dev)) {
dev_warn(dev, "IOMMU driver already assigned to device %s\n",
dev_name(dev));
return -EINVAL;
@@ -754,8 +772,7 @@ static int ipmmu_add_device(struct devic
* - Make the mapping size configurable ? We currently use a 2GB mapping
* at a 1GB offset to ensure that NULL VAs will fault.
*/
- archdata = dev->archdata.iommu;
- mmu = archdata->mmu;
+ mmu = to_archdata(dev)->mmu;
if (!mmu->mapping) {
struct dma_iommu_mapping *mapping;

@@ -783,7 +800,7 @@ error:
if (mmu)
arm_iommu_release_mapping(mmu->mapping);

- dev->archdata.iommu = NULL;
+ set_archdata(dev, NULL);

if (!IS_ERR_OR_NULL(group))
iommu_group_remove_device(dev);
@@ -793,7 +810,7 @@ error:

static void ipmmu_remove_device(struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);

arm_iommu_detach_device(dev);
iommu_group_remove_device(dev);
@@ -801,7 +818,7 @@ static void ipmmu_remove_device(struct d
kfree(archdata->utlbs);
kfree(archdata);

- dev->archdata.iommu = NULL;
+ set_archdata(dev, NULL);
}

static struct iommu_domain *ipmmu_domain_alloc(unsigned type)

[Robin Murphy]

Hi Magnus,

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

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

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

Robin.

Post by Magnus Damm
---
- None
- None
- New patch
drivers/iommu/ipmmu-vmsa.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)
--- 0012/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:59:21.690607110 +0900
@@ -70,6 +70,25 @@ static struct ipmmu_vmsa_domain *to_vmsa
return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
}
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
+{
+ return dev->archdata.iommu;
+}
+static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p)
+{
+ dev->archdata.iommu = p;
+}
+#else
+static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
+{
+ return NULL;
+}
+static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata *p)
+{
+}
+#endif
+
#define TLB_LOOP_TIMEOUT 100 /* 100us */
/* -----------------------------------------------------------------------------
@@ -539,7 +558,7 @@ static void ipmmu_domain_free(struct iom
static int ipmmu_attach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
struct ipmmu_vmsa_device *mmu = archdata->mmu;
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned long flags;
@@ -581,7 +600,7 @@ static int ipmmu_attach_device(struct io
static void ipmmu_detach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned int i;
@@ -701,7 +720,7 @@ static int ipmmu_init_platform_device(st
archdata->mmu = mmu;
archdata->utlbs = utlbs;
archdata->num_utlbs = num_utlbs;
- dev->archdata.iommu = archdata;
+ set_archdata(dev, archdata);
return 0;
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;
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);
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)

[Laurent Pinchart]

Hello,

Post by Robin Murphy

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

As an alternative to this we could now use iommu_fwspec in place of the
custom ipmmu_vmsa_archdata - that's deliberately
architecture-independent. I converted the Mediatek drivers[1] as an
example to stand separately from the big SMMU rework, as those are the
ones I'm most familiar with, but it looks like the IPMMU is a similarly
perfect fit.
Of course, that could always come afterwards as a cleanup - I wouldn't
consider it a blocker at this point - but it does look like it might
simplify some of the code being moved around in this series if it were
to be done beforehand.

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

Post by Robin Murphy
ml

Post by Magnus Damm
---
- None
- None
- New patch
drivers/iommu/ipmmu-vmsa.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)
--- 0012/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-20 21:59:21.690607110 +0900
@@ -70,6 +70,25 @@ static struct ipmmu_vmsa_domain *to_vmsa
return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
}
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
+{
+ return dev->archdata.iommu;
+}
+static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata
*p) +{
+ dev->archdata.iommu = p;
+}
+#else
+static struct ipmmu_vmsa_archdata *to_archdata(struct device *dev)
+{
+ return NULL;
+}
+static void set_archdata(struct device *dev, struct ipmmu_vmsa_archdata
*p) +{
+}
+#endif
+
#define TLB_LOOP_TIMEOUT 100 /* 100us */
/*
------------------------------------------------------------------------
----->
@@ -539,7 +558,7 @@ static void ipmmu_domain_free(struct iom
static int ipmmu_attach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
struct ipmmu_vmsa_device *mmu = archdata->mmu;
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned long flags;
@@ -581,7 +600,7 @@ static int ipmmu_attach_device(struct io
static void ipmmu_detach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned int i;
@@ -701,7 +720,7 @@ static int ipmmu_init_platform_device(st
archdata->mmu = mmu;
archdata->utlbs = utlbs;
archdata->num_utlbs = num_utlbs;
- dev->archdata.iommu = archdata;
+ set_archdata(dev, archdata);
return 0;
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;
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);
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)

[Magnus Damm]

From: Magnus Damm <damm+***@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+***@opensource.se>
Acked-by: Laurent Pinchart <***@ideasonboard.com>
Reviewed-by: Joerg Roedel <***@suse.de>
---

Changes since V5:
- None

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

Changes since V3:
- None

Changes since V2:
- None

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

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

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

[Joerg Roedel]

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

Applied to arm/renesas, thanks.