[PATCH 0/7]: iommu/arm-smmu: Misc fixes/adaptions

[PATCH 0/7]: iommu/arm-smmu: Misc fixes/adaptions

[Andreas Herrmann]

Hi,

Following patches fix misc issues, that I've seen when using arm-smmu
driver with MMU-400.

I also suggest to add an option to automatically create domains for
each master of detected SMMUs. (Similar to what is available for other
iommu driver(s).)

Comments welcome.


Regards,

Andreas

PS: Patches are against v3.12-rc1-336-gd8524ae (and yes I probably
    need to rebase against Joerg's or your smmu tree but I wanted to
    sent this out to get feedback as soon as possible.)

[PATCH 1/7] iommu/arm-smmu: Switch to arch_initcall for driver registration

[Andreas Herrmann]

This should ensure that arm-smmu is initialized before other drivers
start handling devices that propably need smmu support.

Also remove module_exit function as we most likely never want to
unload this driver.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index be56846..97b764b 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1975,13 +1975,7 @@ static int __init arm_smmu_init(void)
 	return 0;
 }
 
-static void __exit arm_smmu_exit(void)
-{
-	return platform_driver_unregister(&arm_smmu_driver);
-}
-
-module_init(arm_smmu_init);
-module_exit(arm_smmu_exit);
+arch_initcall(arm_smmu_init);
 
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMU implementations");
 MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
-- 
1.7.9.5

[PATCH 2/7] iommu/arm-smmu: Calculate SMMU_CB_BASE from smmu register values

[Andreas Herrmann]

Currently it is derived from smmu resource size. If the resource size
is wrongly specified (e.g. too large) this leads to a miscalculation
and can cause undefined behaviour when context bank registers are
modified.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 97b764b..f5a856e 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -207,7 +207,7 @@
 #define CBA2R_RW64_64BIT		(1 << 0)
 
 /* Translation context bank */
-#define ARM_SMMU_CB_BASE(smmu)		((smmu)->base + ((smmu)->size >> 1))
+#define ARM_SMMU_CB_BASE(smmu)		((smmu)->cb_base)
 #define ARM_SMMU_CB(smmu, n)		((n) * (smmu)->pagesize)
 
 #define ARM_SMMU_CB_SCTLR		0x0
@@ -339,6 +339,7 @@ struct arm_smmu_device {
 	struct device_node		*parent_of_node;
 
 	void __iomem			*base;
+	void __iomem			*cb_base;
 	unsigned long			size;
 	unsigned long			pagesize;
 
@@ -1701,7 +1702,9 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 
 	/* Check that we ioremapped enough */
 	size = 1 << (((id >> ID1_NUMPAGENDXB_SHIFT) & ID1_NUMPAGENDXB_MASK) + 1);
-	size *= (smmu->pagesize << 1);
+	size *= smmu->pagesize;
+	smmu->cb_base = smmu->base + size;
+	size *= 2;
 	if (smmu->size < size)
 		dev_warn(smmu->dev,
 			 "device is 0x%lx bytes but only mapped 0x%lx!\n",
-- 
1.7.9.5

[PATCH 3/7] ARM: dma-mapping: Always pass proper prot flags to iommu_map()

[Andreas Herrmann]

... otherwise it is impossible for the low level iommu driver to
figure out which pte flags should be used.

In __map_sg_chunk we can derive the flags from dma_data_direction.

In __iommu_create_mapping we should treat the memory like
DMA_BIDIRECTIONAL and pass both IOMMU_READ and IOMMU_WRITE to
iommu_map.
__iommu_create_mapping is used during dma_alloc_coherent (via
arm_iommu_alloc_attrs).  AFAIK dma_alloc_coherent is responsible for
allocation _and_ mapping.  I think this implies that access to the
mapped pages should be allowed.

Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 arch/arm/mm/dma-mapping.c |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index f5e1a84..95abed8 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1232,7 +1232,8 @@ __iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
 				break;
 
 		len = (j - i) << PAGE_SHIFT;
-		ret = iommu_map(mapping->domain, iova, phys, len, 0);
+		ret = iommu_map(mapping->domain, iova, phys, len,
+				IOMMU_READ|IOMMU_WRITE);
 		if (ret < 0)
 			goto fail;
 		iova += len;
@@ -1444,6 +1445,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
 	int ret = 0;
 	unsigned int count;
 	struct scatterlist *s;
+	int prot;
 
 	size = PAGE_ALIGN(size);
 	*handle = DMA_ERROR_CODE;
@@ -1460,7 +1462,21 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
 			!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
 			__dma_page_cpu_to_dev(sg_page(s), s->offset, s->length, dir);
 
-		ret = iommu_map(mapping->domain, iova, phys, len, 0);
+		switch (dir) {
+		case DMA_BIDIRECTIONAL:
+			prot = IOMMU_READ | IOMMU_WRITE;
+			break;
+		case DMA_TO_DEVICE:
+			prot = IOMMU_READ;
+			break;
+		case DMA_FROM_DEVICE:
+			prot = IOMMU_WRITE;
+			break;
+		default:
+			prot = 0;
+		}
+
+		ret = iommu_map(mapping->domain, iova, phys, len, prot);
 		if (ret < 0)
 			goto fail;
 		count += len >> PAGE_SHIFT;
-- 
1.7.9.5

[PATCH 4/7] iommu/arm-smmu: Check for num_context_irqs > 0 to avoid divide by zero exception

[Andreas Herrmann]

With the right (or wrong;-) definition of v1 SMMU node in DTB it is
possible to trigger a division by zero in arm_smmu_init_domain_context
(if number of context irqs is 0):

       if (smmu->version == 1) {
               root_cfg->irptndx = atomic_inc_return(&smmu->irptndx);
 ?             root_cfg->irptndx %= smmu->num_context_irqs;
       } else {

Avoid this by checking for num_context_irqs > 0 before trying to
assign interrupts to contexts.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |   31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index f5a856e..0dfd255 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -828,21 +828,24 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 		return ret;
 
 	root_cfg->cbndx = ret;
-	if (smmu->version == 1) {
-		root_cfg->irptndx = atomic_inc_return(&smmu->irptndx);
-		root_cfg->irptndx %= smmu->num_context_irqs;
-	} else {
-		root_cfg->irptndx = root_cfg->cbndx;
-	}
 
-	irq = smmu->irqs[smmu->num_global_irqs + root_cfg->irptndx];
-	ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED,
-			  "arm-smmu-context-fault", domain);
-	if (IS_ERR_VALUE(ret)) {
-		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
-			root_cfg->irptndx, irq);
-		root_cfg->irptndx = -1;
-		goto out_free_context;
+	if (smmu->num_context_irqs) {
+		if (smmu->version == 1) {
+			root_cfg->irptndx = atomic_inc_return(&smmu->irptndx);
+			root_cfg->irptndx %= smmu->num_context_irqs;
+		} else {
+			root_cfg->irptndx = root_cfg->cbndx;
+		}
+
+		irq = smmu->irqs[smmu->num_global_irqs + root_cfg->irptndx];
+		ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED,
+				"arm-smmu-context-fault", domain);
+		if (IS_ERR_VALUE(ret)) {
+			dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
+				root_cfg->irptndx, irq);
+			root_cfg->irptndx = -1;
+			goto out_free_context;
+		}
 	}
 
 	root_cfg->smmu = smmu;
-- 
1.7.9.5

[PATCH 5/7] iommu/arm-smmu: Add function that isolates all masters for all SMMUs

[Andreas Herrmann]

Each device is put into its own protection domain (if possible).
For configuration with one or just a view masters per SMMU that
is easy to achieve.

In case of many devices per SMMU (e.g. MMU-500 with it's distributed
translation support) isolation of each device might not be possible --
depending on number of available SMR groups and/or context banks.

Derive dma_base and size from (coherent_)dma_mask of device

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 0dfd255..3eb2259 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -46,6 +46,7 @@
 #include <linux/amba/bus.h>
 
 #include <asm/pgalloc.h>
+#include <asm/dma-iommu.h>
 
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		8
@@ -1768,6 +1769,64 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
+extern struct platform_device *of_find_device_by_node(struct device_node *np);
+
+static int arm_smmu_isolate_devices(void)
+{
+	struct dma_iommu_mapping *mapping;
+	struct arm_smmu_device *smmu;
+	struct rb_node *rbn;
+	struct arm_smmu_master *master;
+	struct platform_device *pdev;
+	struct device *dev;
+	void __iomem *gr0_base;
+	u32 cr0;
+	int ret = 0;
+	size_t size;
+
+	list_for_each_entry(smmu, &arm_smmu_devices, list) {
+		rbn = rb_first(&smmu->masters);
+		while (rbn) {
+			master = container_of(rbn, struct arm_smmu_master, node);
+			pdev = of_find_device_by_node(master->of_node);
+			if (!pdev)
+				break;
+			dev = &pdev->dev;
+
+			size = (size_t) dev->coherent_dma_mask;
+			size = size ? : (unsigned long) dev->dma_mask;
+			if (!size) {
+				dev_warn(dev, "(coherent_)dma_mask not set\n");
+				continue;
+			}
+
+			mapping = arm_iommu_create_mapping(&platform_bus_type,
+							0, size, 0);
+			if (IS_ERR(mapping)) {
+				ret = PTR_ERR(mapping);
+				dev_info(dev, "arm_iommu_create_mapping failed\n");
+				goto out;
+			}
+
+			ret = arm_iommu_attach_device(dev, mapping);
+			if (ret < 0) {
+				dev_info(dev, "arm_iommu_attach_device failed\n");
+				arm_iommu_release_mapping(mapping);
+			}
+			rbn = rb_next(rbn);
+		}
+
+		gr0_base = ARM_SMMU_GR0(smmu);
+		cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+		cr0 |= sCR0_USFCFG;
+		writel(cr0, gr0_base + ARM_SMMU_GR0_sCR0);
+	}
+
+out:
+	return ret;
+}
+
+
 static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 {
 	struct resource *res;
-- 
1.7.9.5

[PATCH 6/7] iommu/arm-smmu: Add module parameter arm-smmu=off|force_isolation

[Andreas Herrmann]

  arm-smmu=	arm-smmu driver option

	off	Disable arm-smmu driver (ie. ignore available SMMUs)

	force_isolation
		Try to attach each master device on every SMMU to a
		separate iommu_domain.

Default is that driver detects SMMUs but no translation is configured
(transactions just bypass translation process).

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 3eb2259..251564e 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -399,6 +399,9 @@ struct arm_smmu_domain {
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
 static LIST_HEAD(arm_smmu_devices);
 
+static bool arm_smmu_disabled;
+static bool arm_smmu_force_isolation;
+
 static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
 						struct device_node *dev_node)
 {
@@ -1837,6 +1840,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	struct of_phandle_args masterspec;
 	int num_irqs, i, err;
 
+	if (arm_smmu_disabled)
+		return -ENODEV;
+
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
 	if (!smmu) {
 		dev_err(dev, "failed to allocate arm_smmu_device\n");
@@ -2022,6 +2028,23 @@ static struct platform_driver arm_smmu_driver = {
 	.remove	= arm_smmu_device_remove,
 };
 
+static int __init arm_smmu_parse_options(char *str)
+{
+	if (*str) {
+		str++;
+		if (!strncmp(str, "off", 3))
+			arm_smmu_disabled = true;
+		else if(!strncmp(str, "force_isolation", 15))
+			arm_smmu_force_isolation = true;
+		else {
+			pr_warn("arm_smmu: invalid parameter (\"%s\")\n", str);
+			return 0;
+		}
+	}
+	return 1;
+}
+__setup("arm-smmu", arm_smmu_parse_options);
+
 static int __init arm_smmu_init(void)
 {
 	int ret;
@@ -2037,6 +2060,9 @@ static int __init arm_smmu_init(void)
 	if (!iommu_present(&amba_bustype))
 		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
 
+	if (arm_smmu_force_isolation)
+		arm_smmu_isolate_devices();
+
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 7/7] iommu/arm-smmu: Clear global and context bank fault status and syndrome registers

[Andreas Herrmann]

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 251564e..a499146 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -645,6 +645,10 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 	stage1 = root_cfg->cbar != CBAR_TYPE_S2_TRANS;
 	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, root_cfg->cbndx);
 
+	/* clear fsr */
+	writel_relaxed(0xffffffff, cb_base + ARM_SMMU_CB_FSR);
+	writel_relaxed(0, cb_base + ARM_SMMU_CB_FSYNR0);
+
 	/* CBAR */
 	reg = root_cfg->cbar;
 	if (smmu->version == 1)
@@ -1570,6 +1574,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	int i = 0;
 	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
 
+	/* clear global FSRs */
+	writel(0xffffffff, gr0_base + ARM_SMMU_GR0_sGFSR);
+	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR0);
+	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR1);
+
 	/* Mark all SMRn as invalid and all S2CRn as bypass */
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
 		writel_relaxed(~SMR_VALID, gr0_base + ARM_SMMU_GR0_SMR(i));
-- 
1.7.9.5

[PATCH 1/7] iommu/arm-smmu: Switch to arch_initcall for driver registration

[Andreas Herrmann]

iommu/arm-smmu: Remove bogus semicolon from if conditions

Those prevented proper registration of arm_smmu_ops.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Will,

It seems that I've managed to exclude the very first patch of the
series. Here it is.


Regards,

Andreas

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index f417e89..be56846 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1966,10 +1966,10 @@ static int __init arm_smmu_init(void)
 		return ret;
 
 	/* Oh, for a proper bus abstraction */
-	if (!iommu_present(&platform_bus_type));
+	if (!iommu_present(&platform_bus_type))
 		bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
 
-	if (!iommu_present(&amba_bustype));
+	if (!iommu_present(&amba_bustype))
 		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
 
 	return 0;
-- 
1.7.9.5

[PATCH 1/7] iommu/arm-smmu: Switch to arch_initcall for driver registration

[Will Deacon]

On Tue, Sep 24, 2013 at 04:14:30PM +0100, Andreas Herrmann wrote:
> iommu/arm-smmu: Remove bogus semicolon from if conditions
> 
> Those prevented proper registration of arm_smmu_ops.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Will,
> 
> It seems that I've managed to exclude the very first patch of the
> series. Here it is.

No worries; I already have a fix for this and Joerg pulled it today.

Will

[PATCH 0/7]: iommu/arm-smmu: Misc fixes/adaptions

[Will Deacon]

On Tue, Sep 24, 2013 at 04:06:54PM +0100, Andreas Herrmann wrote:
> Hi,

Hi Andreas,

> Following patches fix misc issues, that I've seen when using arm-smmu
> driver with MMU-400.

Thanks for the patches! I'll respond to each one in turn.

Will

[PATCH 2/7] iommu/arm-smmu: Calculate SMMU_CB_BASE from smmu register values

[Will Deacon]

On Tue, Sep 24, 2013 at 04:06:56PM +0100, Andreas Herrmann wrote:
> Currently it is derived from smmu resource size. If the resource size
> is wrongly specified (e.g. too large) this leads to a miscalculation
> and can cause undefined behaviour when context bank registers are
> modified.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 97b764b..f5a856e 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -207,7 +207,7 @@
>  #define CBA2R_RW64_64BIT		(1 << 0)
>  
>  /* Translation context bank */
> -#define ARM_SMMU_CB_BASE(smmu)		((smmu)->base + ((smmu)->size >> 1))
> +#define ARM_SMMU_CB_BASE(smmu)		((smmu)->cb_base)
>  #define ARM_SMMU_CB(smmu, n)		((n) * (smmu)->pagesize)
>  
>  #define ARM_SMMU_CB_SCTLR		0x0
> @@ -339,6 +339,7 @@ struct arm_smmu_device {
>  	struct device_node		*parent_of_node;
>  
>  	void __iomem			*base;
> +	void __iomem			*cb_base;
>  	unsigned long			size;
>  	unsigned long			pagesize;
>  
> @@ -1701,7 +1702,9 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  
>  	/* Check that we ioremapped enough */
>  	size = 1 << (((id >> ID1_NUMPAGENDXB_SHIFT) & ID1_NUMPAGENDXB_MASK) + 1);
> -	size *= (smmu->pagesize << 1);
> +	size *= smmu->pagesize;
> +	smmu->cb_base = smmu->base + size;
> +	size *= 2;
>  	if (smmu->size < size)
>  		dev_warn(smmu->dev,
>  			 "device is 0x%lx bytes but only mapped 0x%lx!\n",

Hmm, this is a tricky one. We know that we have an inconsistency (i.e. the
DT and the hardware don't agree on the size of the device) but we warn and
attempt to continue with the value from the DT. I don't think that trusting
the hardware is the right thing to do in this case, since it's not possible
to change so we should let the DT act as an override.

In other words: if the device tree is wrong, go fix it.

Will

[PATCH 3/7] ARM: dma-mapping: Always pass proper prot flags to iommu_map()

[Will Deacon]

On Tue, Sep 24, 2013 at 04:06:57PM +0100, Andreas Herrmann wrote:
> ... otherwise it is impossible for the low level iommu driver to
> figure out which pte flags should be used.
> 
> In __map_sg_chunk we can derive the flags from dma_data_direction.
> 
> In __iommu_create_mapping we should treat the memory like
> DMA_BIDIRECTIONAL and pass both IOMMU_READ and IOMMU_WRITE to
> iommu_map.
> __iommu_create_mapping is used during dma_alloc_coherent (via
> arm_iommu_alloc_attrs).  AFAIK dma_alloc_coherent is responsible for
> allocation _and_ mapping.  I think this implies that access to the
> mapped pages should be allowed.
> 
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  arch/arm/mm/dma-mapping.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index f5e1a84..95abed8 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1232,7 +1232,8 @@ __iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
>  				break;
>  
>  		len = (j - i) << PAGE_SHIFT;
> -		ret = iommu_map(mapping->domain, iova, phys, len, 0);
> +		ret = iommu_map(mapping->domain, iova, phys, len,
> +				IOMMU_READ|IOMMU_WRITE);
>  		if (ret < 0)
>  			goto fail;
>  		iova += len;
> @@ -1444,6 +1445,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
>  	int ret = 0;
>  	unsigned int count;
>  	struct scatterlist *s;
> +	int prot;
>  
>  	size = PAGE_ALIGN(size);
>  	*handle = DMA_ERROR_CODE;
> @@ -1460,7 +1462,21 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
>  			!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
>  			__dma_page_cpu_to_dev(sg_page(s), s->offset, s->length, dir);
>  
> -		ret = iommu_map(mapping->domain, iova, phys, len, 0);
> +		switch (dir) {
> +		case DMA_BIDIRECTIONAL:
> +			prot = IOMMU_READ | IOMMU_WRITE;
> +			break;
> +		case DMA_TO_DEVICE:
> +			prot = IOMMU_READ;
> +			break;
> +		case DMA_FROM_DEVICE:
> +			prot = IOMMU_WRITE;
> +			break;
> +		default:
> +			prot = 0;
> +		}
> +
> +		ret = iommu_map(mapping->domain, iova, phys, len, prot);

I already added this code to arm_coherent_iommu_map_page but forgot to
update the rest of the time. Could you shift the switch into a helper and
call that instead of inlining it explicitly?

Cheers,

Will

[PATCH 4/7] iommu/arm-smmu: Check for num_context_irqs > 0 to avoid divide by zero exception

[Will Deacon]

On Tue, Sep 24, 2013 at 04:06:58PM +0100, Andreas Herrmann wrote:
> With the right (or wrong;-) definition of v1 SMMU node in DTB it is
> possible to trigger a division by zero in arm_smmu_init_domain_context
> (if number of context irqs is 0):
> 
>        if (smmu->version == 1) {
>                root_cfg->irptndx = atomic_inc_return(&smmu->irptndx);
>  ?             root_cfg->irptndx %= smmu->num_context_irqs;
>        } else {
> 
> Avoid this by checking for num_context_irqs > 0 before trying to
> assign interrupts to contexts.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index f5a856e..0dfd255 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -828,21 +828,24 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  		return ret;
>  
>  	root_cfg->cbndx = ret;
> -	if (smmu->version == 1) {
> -		root_cfg->irptndx = atomic_inc_return(&smmu->irptndx);
> -		root_cfg->irptndx %= smmu->num_context_irqs;
> -	} else {
> -		root_cfg->irptndx = root_cfg->cbndx;
> -	}
>  
> -	irq = smmu->irqs[smmu->num_global_irqs + root_cfg->irptndx];
> -	ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED,
> -			  "arm-smmu-context-fault", domain);
> -	if (IS_ERR_VALUE(ret)) {
> -		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
> -			root_cfg->irptndx, irq);
> -		root_cfg->irptndx = -1;
> -		goto out_free_context;
> +	if (smmu->num_context_irqs) {

Can we move this check to probe time, to avoid re-evaluating it every time
we initialise a new domain?

Will

[PATCH 6/7] iommu/arm-smmu: Add module parameter arm-smmu=off|force_isolation

[Will Deacon]

On Tue, Sep 24, 2013 at 04:07:00PM +0100, Andreas Herrmann wrote:
>   arm-smmu=	arm-smmu driver option
> 
> 	off	Disable arm-smmu driver (ie. ignore available SMMUs)
> 
> 	force_isolation
> 		Try to attach each master device on every SMMU to a
> 		separate iommu_domain.
> 
> Default is that driver detects SMMUs but no translation is configured
> (transactions just bypass translation process).
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 3eb2259..251564e 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -399,6 +399,9 @@ struct arm_smmu_domain {
>  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
>  static LIST_HEAD(arm_smmu_devices);
>  
> +static bool arm_smmu_disabled;
> +static bool arm_smmu_force_isolation;
> +
>  static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
>  						struct device_node *dev_node)
>  {
> @@ -1837,6 +1840,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  	struct of_phandle_args masterspec;
>  	int num_irqs, i, err;
>  
> +	if (arm_smmu_disabled)
> +		return -ENODEV;
> +
>  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
>  	if (!smmu) {
>  		dev_err(dev, "failed to allocate arm_smmu_device\n");
> @@ -2022,6 +2028,23 @@ static struct platform_driver arm_smmu_driver = {
>  	.remove	= arm_smmu_device_remove,
>  };
>  
> +static int __init arm_smmu_parse_options(char *str)
> +{
> +	if (*str) {
> +		str++;
> +		if (!strncmp(str, "off", 3))
> +			arm_smmu_disabled = true;
> +		else if(!strncmp(str, "force_isolation", 15))
> +			arm_smmu_force_isolation = true;
> +		else {
> +			pr_warn("arm_smmu: invalid parameter (\"%s\")\n", str);
> +			return 0;
> +		}
> +	}
> +	return 1;
> +}
> +__setup("arm-smmu", arm_smmu_parse_options);

If this is going to be a common function for IOMMUs, let's instead move the
command-line parsing out into the generic IOMMU layer, then have an optional
callback into the low-level IOMMU driver for enabling/disabling it.

Will

[PATCH 7/7] iommu/arm-smmu: Clear global and context bank fault status and syndrome registers

[Will Deacon]

On Tue, Sep 24, 2013 at 04:07:01PM +0100, Andreas Herrmann wrote:
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 251564e..a499146 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -645,6 +645,10 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
>  	stage1 = root_cfg->cbar != CBAR_TYPE_S2_TRANS;
>  	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, root_cfg->cbndx);
>  
> +	/* clear fsr */
> +	writel_relaxed(0xffffffff, cb_base + ARM_SMMU_CB_FSR);
> +	writel_relaxed(0, cb_base + ARM_SMMU_CB_FSYNR0);
> +
>  	/* CBAR */
>  	reg = root_cfg->cbar;
>  	if (smmu->version == 1)
> @@ -1570,6 +1574,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  	int i = 0;
>  	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
>  
> +	/* clear global FSRs */
> +	writel(0xffffffff, gr0_base + ARM_SMMU_GR0_sGFSR);
> +	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR0);
> +	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR1);

Why do you need this?

Will

[PATCH 3/7] ARM: dma-mapping: Always pass proper prot flags to iommu_map()

[Andreas Herrmann]

On Tue, Sep 24, 2013 at 11:36:25AM -0400, Will Deacon wrote:
> On Tue, Sep 24, 2013 at 04:06:57PM +0100, Andreas Herrmann wrote:
> > ... otherwise it is impossible for the low level iommu driver to
> > figure out which pte flags should be used.
> > 
> > In __map_sg_chunk we can derive the flags from dma_data_direction.
> > 
> > In __iommu_create_mapping we should treat the memory like
> > DMA_BIDIRECTIONAL and pass both IOMMU_READ and IOMMU_WRITE to
> > iommu_map.
> > __iommu_create_mapping is used during dma_alloc_coherent (via
> > arm_iommu_alloc_attrs).  AFAIK dma_alloc_coherent is responsible for
> > allocation _and_ mapping.  I think this implies that access to the
> > mapped pages should be allowed.
> > 
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  arch/arm/mm/dma-mapping.c |   20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index f5e1a84..95abed8 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -1232,7 +1232,8 @@ __iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
> >  				break;
> >  
> >  		len = (j - i) << PAGE_SHIFT;
> > -		ret = iommu_map(mapping->domain, iova, phys, len, 0);
> > +		ret = iommu_map(mapping->domain, iova, phys, len,
> > +				IOMMU_READ|IOMMU_WRITE);
> >  		if (ret < 0)
> >  			goto fail;
> >  		iova += len;
> > @@ -1444,6 +1445,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
> >  	int ret = 0;
> >  	unsigned int count;
> >  	struct scatterlist *s;
> > +	int prot;
> >  
> >  	size = PAGE_ALIGN(size);
> >  	*handle = DMA_ERROR_CODE;
> > @@ -1460,7 +1462,21 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
> >  			!dma_get_attr(DMA_ATTR_SKIP_CPU_SYNC, attrs))
> >  			__dma_page_cpu_to_dev(sg_page(s), s->offset, s->length, dir);
> >  
> > -		ret = iommu_map(mapping->domain, iova, phys, len, 0);
> > +		switch (dir) {
> > +		case DMA_BIDIRECTIONAL:
> > +			prot = IOMMU_READ | IOMMU_WRITE;
> > +			break;
> > +		case DMA_TO_DEVICE:
> > +			prot = IOMMU_READ;
> > +			break;
> > +		case DMA_FROM_DEVICE:
> > +			prot = IOMMU_WRITE;
> > +			break;
> > +		default:
> > +			prot = 0;
> > +		}
> > +
> > +		ret = iommu_map(mapping->domain, iova, phys, len, prot);
> 
> I already added this code to arm_coherent_iommu_map_page but forgot to
> update the rest of the time. Could you shift the switch into a helper and
> call that instead of inlining it explicitly?

Yes, will do so.


Andreas

[PATCH 2/7] iommu/arm-smmu: Calculate SMMU_CB_BASE from smmu register values

[Andreas Herrmann]

On Tue, Sep 24, 2013 at 11:34:57AM -0400, Will Deacon wrote:
> On Tue, Sep 24, 2013 at 04:06:56PM +0100, Andreas Herrmann wrote:
> > Currently it is derived from smmu resource size. If the resource size
> > is wrongly specified (e.g. too large) this leads to a miscalculation
> > and can cause undefined behaviour when context bank registers are
> > modified.
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 97b764b..f5a856e 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -207,7 +207,7 @@
> >  #define CBA2R_RW64_64BIT		(1 << 0)
> >  
> >  /* Translation context bank */
> > -#define ARM_SMMU_CB_BASE(smmu)		((smmu)->base + ((smmu)->size >> 1))
> > +#define ARM_SMMU_CB_BASE(smmu)		((smmu)->cb_base)
> >  #define ARM_SMMU_CB(smmu, n)		((n) * (smmu)->pagesize)
> >  
> >  #define ARM_SMMU_CB_SCTLR		0x0
> > @@ -339,6 +339,7 @@ struct arm_smmu_device {
> >  	struct device_node		*parent_of_node;
> >  
> >  	void __iomem			*base;
> > +	void __iomem			*cb_base;
> >  	unsigned long			size;
> >  	unsigned long			pagesize;
> >  
> > @@ -1701,7 +1702,9 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >  
> >  	/* Check that we ioremapped enough */
> >  	size = 1 << (((id >> ID1_NUMPAGENDXB_SHIFT) & ID1_NUMPAGENDXB_MASK) + 1);
> > -	size *= (smmu->pagesize << 1);
> > +	size *= smmu->pagesize;
> > +	smmu->cb_base = smmu->base + size;
> > +	size *= 2;
> >  	if (smmu->size < size)
> >  		dev_warn(smmu->dev,
> >  			 "device is 0x%lx bytes but only mapped 0x%lx!\n",
> 
> Hmm, this is a tricky one. We know that we have an inconsistency (i.e. the
> DT and the hardware don't agree on the size of the device) but we warn and
> attempt to continue with the value from the DT. I don't think that trusting
> the hardware is the right thing to do in this case, since it's not possible
> to change so we should let the DT act as an override.

> In other words: if the device tree is wrong, go fix it.

Yes, I've found this issue with a wrong DT. With the original code
there was some weirdness when setting certain context bank
registers. (Identifying the root cause was not straight forward.)

I think it's somehow odd not to trust the hardware values in the first
place and to add (right from the beginning) a quirk for potential
implementation bugs. Are there already implementations that use wrong
register values that are required to determine the partitioning of the
SMMU address space?

If there is a mismatch it's hard to say which value is the correct
one. I think there are three options:
(1) just print a warning about the mismatch
(2) print a warning + override based on DT
(3) print a warning + override based on DT + have an option to switch
    off the override

So, what's your choice?


Andreas

[PATCH 7/7] iommu/arm-smmu: Clear global and context bank fault status and syndrome registers

[Andreas Herrmann]

On Tue, Sep 24, 2013 at 11:42:52AM -0400, Will Deacon wrote:
> On Tue, Sep 24, 2013 at 04:07:01PM +0100, Andreas Herrmann wrote:
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 251564e..a499146 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -645,6 +645,10 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
> >  	stage1 = root_cfg->cbar != CBAR_TYPE_S2_TRANS;
> >  	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, root_cfg->cbndx);
> >  
> > +	/* clear fsr */
> > +	writel_relaxed(0xffffffff, cb_base + ARM_SMMU_CB_FSR);
> > +	writel_relaxed(0, cb_base + ARM_SMMU_CB_FSYNR0);
> > +
> >  	/* CBAR */
> >  	reg = root_cfg->cbar;
> >  	if (smmu->version == 1)
> > @@ -1570,6 +1574,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >  	int i = 0;
> >  	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> >  
> > +	/* clear global FSRs */
> > +	writel(0xffffffff, gr0_base + ARM_SMMU_GR0_sGFSR);
> > +	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR0);
> > +	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR1);
> 
> Why do you need this?

According to the spec the status and syndrome registers have
unknown/unpredictable reset values. So better set known values before
we start to use these registers (ie. handle faults where we read
them). No?


Andreas

[PATCH 4/7] iommu/arm-smmu: Check for num_context_irqs > 0 to avoid divide by zero exception

[Andreas Herrmann]

On Tue, Sep 24, 2013 at 11:40:48AM -0400, Will Deacon wrote:
> On Tue, Sep 24, 2013 at 04:06:58PM +0100, Andreas Herrmann wrote:
> > With the right (or wrong;-) definition of v1 SMMU node in DTB it is
> > possible to trigger a division by zero in arm_smmu_init_domain_context
> > (if number of context irqs is 0):
> > 
> >        if (smmu->version == 1) {
> >                root_cfg->irptndx = atomic_inc_return(&smmu->irptndx);
> >  ?             root_cfg->irptndx %= smmu->num_context_irqs;
> >        } else {
> > 
> > Avoid this by checking for num_context_irqs > 0 before trying to
> > assign interrupts to contexts.
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |   31 +++++++++++++++++--------------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index f5a856e..0dfd255 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -828,21 +828,24 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >  		return ret;
> >  
> >  	root_cfg->cbndx = ret;
> > -	if (smmu->version == 1) {
> > -		root_cfg->irptndx = atomic_inc_return(&smmu->irptndx);
> > -		root_cfg->irptndx %= smmu->num_context_irqs;
> > -	} else {
> > -		root_cfg->irptndx = root_cfg->cbndx;
> > -	}
> >  
> > -	irq = smmu->irqs[smmu->num_global_irqs + root_cfg->irptndx];
> > -	ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED,
> > -			  "arm-smmu-context-fault", domain);
> > -	if (IS_ERR_VALUE(ret)) {
> > -		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
> > -			root_cfg->irptndx, irq);
> > -		root_cfg->irptndx = -1;
> > -		goto out_free_context;
> > +	if (smmu->num_context_irqs) {
> 
> Can we move this check to probe time, to avoid re-evaluating it every time
> we initialise a new domain?

Yes, I'll move this check and issue an error message when there is not
at least one context interrupt available.

Andreas

[PATCH 6/7] iommu/arm-smmu: Add module parameter arm-smmu=off|force_isolation

[Andreas Herrmann]

On Tue, Sep 24, 2013 at 11:42:18AM -0400, Will Deacon wrote:
> On Tue, Sep 24, 2013 at 04:07:00PM +0100, Andreas Herrmann wrote:
> >   arm-smmu=	arm-smmu driver option
> > 
> > 	off	Disable arm-smmu driver (ie. ignore available SMMUs)
> > 
> > 	force_isolation
> > 		Try to attach each master device on every SMMU to a
> > 		separate iommu_domain.
> > 
> > Default is that driver detects SMMUs but no translation is configured
> > (transactions just bypass translation process).
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |   26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 3eb2259..251564e 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -399,6 +399,9 @@ struct arm_smmu_domain {
> >  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> >  static LIST_HEAD(arm_smmu_devices);
> >  
> > +static bool arm_smmu_disabled;
> > +static bool arm_smmu_force_isolation;
> > +
> >  static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
> >  						struct device_node *dev_node)
> >  {
> > @@ -1837,6 +1840,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> >  	struct of_phandle_args masterspec;
> >  	int num_irqs, i, err;
> >  
> > +	if (arm_smmu_disabled)
> > +		return -ENODEV;
> > +
> >  	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> >  	if (!smmu) {
> >  		dev_err(dev, "failed to allocate arm_smmu_device\n");
> > @@ -2022,6 +2028,23 @@ static struct platform_driver arm_smmu_driver = {
> >  	.remove	= arm_smmu_device_remove,
> >  };
> >  
> > +static int __init arm_smmu_parse_options(char *str)
> > +{
> > +	if (*str) {
> > +		str++;
> > +		if (!strncmp(str, "off", 3))
> > +			arm_smmu_disabled = true;
> > +		else if(!strncmp(str, "force_isolation", 15))
> > +			arm_smmu_force_isolation = true;
> > +		else {
> > +			pr_warn("arm_smmu: invalid parameter (\"%s\")\n", str);
> > +			return 0;
> > +		}
> > +	}
> > +	return 1;
> > +}
> > +__setup("arm-smmu", arm_smmu_parse_options);
> 
> If this is going to be a common function for IOMMUs, let's instead move the
> command-line parsing out into the generic IOMMU layer, then have an optional
> callback into the low-level IOMMU driver for enabling/disabling it.

Makes sense and I am currently looking into it.


Andreas

[PATCH 6/7] iommu/arm-smmu: Add module parameter arm-smmu=off|force_isolation

[Joerg Roedel]

On Tue, Sep 24, 2013 at 04:42:18PM +0100, Will Deacon wrote:
> On Tue, Sep 24, 2013 at 04:07:00PM +0100, Andreas Herrmann wrote:
> > +__setup("arm-smmu", arm_smmu_parse_options);
> 
> If this is going to be a common function for IOMMUs, let's instead move the
> command-line parsing out into the generic IOMMU layer, then have an optional
> callback into the low-level IOMMU driver for enabling/disabling it.

Hmm, actually the force-isolation parameters that different IOMMU
drivers implement are for their version of the DMA-API which is not yet
available in the generic IOMMU layer. Unless this changes I think it is
a good idea to keep these parameters close to the actual DMA-API
implementation used and not move them to the IOMMU layer.


	Joerg

[PATCH 2/7] iommu/arm-smmu: Calculate SMMU_CB_BASE from smmu register values

[Will Deacon]

On Tue, Sep 24, 2013 at 07:07:20PM +0100, Andreas Herrmann wrote:
> On Tue, Sep 24, 2013 at 11:34:57AM -0400, Will Deacon wrote:
> > On Tue, Sep 24, 2013 at 04:06:56PM +0100, Andreas Herrmann wrote:
> > > Currently it is derived from smmu resource size. If the resource size
> > > is wrongly specified (e.g. too large) this leads to a miscalculation
> > > and can cause undefined behaviour when context bank registers are
> > > modified.

[...]

> > Hmm, this is a tricky one. We know that we have an inconsistency (i.e. the
> > DT and the hardware don't agree on the size of the device) but we warn and
> > attempt to continue with the value from the DT. I don't think that trusting
> > the hardware is the right thing to do in this case, since it's not possible
> > to change so we should let the DT act as an override.
> 
> > In other words: if the device tree is wrong, go fix it.
> 
> Yes, I've found this issue with a wrong DT. With the original code
> there was some weirdness when setting certain context bank
> registers. (Identifying the root cause was not straight forward.)
> 
> I think it's somehow odd not to trust the hardware values in the first
> place and to add (right from the beginning) a quirk for potential
> implementation bugs. Are there already implementations that use wrong
> register values that are required to determine the partitioning of the
> SMMU address space?

I don't know of any, but you can bet that people will want to run old
kernels on future hardware, so we should try and get this right from day
one.

> If there is a mismatch it's hard to say which value is the correct
> one. I think there are three options:
> (1) just print a warning about the mismatch
> (2) print a warning + override based on DT
> (3) print a warning + override based on DT + have an option to switch
>     off the override
> 
> So, what's your choice?

I had gone for (2), on the assumption that fixing a broken DT shouldn't be
too hard as well as allowing people to work around broken hardware. Yes, it
means we treat the DT as golden, but that's already the case in the absence
of fully probable hardware.

Will

[PATCH 7/7] iommu/arm-smmu: Clear global and context bank fault status and syndrome registers

[Will Deacon]

On Tue, Sep 24, 2013 at 07:32:47PM +0100, Andreas Herrmann wrote:
> On Tue, Sep 24, 2013 at 11:42:52AM -0400, Will Deacon wrote:
> > On Tue, Sep 24, 2013 at 04:07:01PM +0100, Andreas Herrmann wrote:
> > > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > > ---
> > >  drivers/iommu/arm-smmu.c |    9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > index 251564e..a499146 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -645,6 +645,10 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
> > >  	stage1 = root_cfg->cbar != CBAR_TYPE_S2_TRANS;
> > >  	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, root_cfg->cbndx);
> > >  
> > > +	/* clear fsr */
> > > +	writel_relaxed(0xffffffff, cb_base + ARM_SMMU_CB_FSR);
> > > +	writel_relaxed(0, cb_base + ARM_SMMU_CB_FSYNR0);
> > > +
> > >  	/* CBAR */
> > >  	reg = root_cfg->cbar;
> > >  	if (smmu->version == 1)
> > > @@ -1570,6 +1574,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> > >  	int i = 0;
> > >  	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> > >  
> > > +	/* clear global FSRs */
> > > +	writel(0xffffffff, gr0_base + ARM_SMMU_GR0_sGFSR);
> > > +	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR0);
> > > +	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR1);
> > 
> > Why do you need this?
> 
> According to the spec the status and syndrome registers have
> unknown/unpredictable reset values. So better set known values before
> we start to use these registers (ie. handle faults where we read
> them). No?

Sure, but the only time these are made visible should be if we take a fault,
in which case the registers should contain something meaningful. I guess I
could see a problem if there is a shared interrupt line for faults though,
so a cut-down version of your patch (just clearing ARM_SMMU_GR0_sGFSR and
the ARM_SMMU_CB_FSR registers) should be enough.

Cheers,

Will

[PATCH 7/7] iommu/arm-smmu: Clear global and context bank fault status and syndrome registers

[Andreas Herrmann]

On Wed, Sep 25, 2013 at 12:49:18PM -0400, Will Deacon wrote:
> On Tue, Sep 24, 2013 at 07:32:47PM +0100, Andreas Herrmann wrote:
> > On Tue, Sep 24, 2013 at 11:42:52AM -0400, Will Deacon wrote:
> > > On Tue, Sep 24, 2013 at 04:07:01PM +0100, Andreas Herrmann wrote:
> > > > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > > > ---
> > > >  drivers/iommu/arm-smmu.c |    9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > > index 251564e..a499146 100644
> > > > --- a/drivers/iommu/arm-smmu.c
> > > > +++ b/drivers/iommu/arm-smmu.c
> > > > @@ -645,6 +645,10 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
> > > >  	stage1 = root_cfg->cbar != CBAR_TYPE_S2_TRANS;
> > > >  	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, root_cfg->cbndx);
> > > >  
> > > > +	/* clear fsr */
> > > > +	writel_relaxed(0xffffffff, cb_base + ARM_SMMU_CB_FSR);
> > > > +	writel_relaxed(0, cb_base + ARM_SMMU_CB_FSYNR0);
> > > > +
> > > >  	/* CBAR */
> > > >  	reg = root_cfg->cbar;
> > > >  	if (smmu->version == 1)
> > > > @@ -1570,6 +1574,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> > > >  	int i = 0;
> > > >  	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> > > >  
> > > > +	/* clear global FSRs */
> > > > +	writel(0xffffffff, gr0_base + ARM_SMMU_GR0_sGFSR);
> > > > +	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR0);
> > > > +	writel(0, gr0_base + ARM_SMMU_GR0_sGFSYNR1);
> > > 
> > > Why do you need this?
> > 
> > According to the spec the status and syndrome registers have
> > unknown/unpredictable reset values. So better set known values before
> > we start to use these registers (ie. handle faults where we read
> > them). No?
> 
> Sure, but the only time these are made visible should be if we take a fault,
> in which case the registers should contain something meaningful. I guess I
> could see a problem if there is a shared interrupt line for faults though,
> so a cut-down version of your patch (just clearing ARM_SMMU_GR0_sGFSR and
> the ARM_SMMU_CB_FSR registers) should be enough.

Agreed.

Andreas