[PATCH 0/9] arm-smmu: Misc changes/Calxeda ECX-2000 support

[PATCH 0/9] arm-smmu: Misc changes/Calxeda ECX-2000 support

[Andreas Herrmann]

Hi,

Following a new patch set to fix some issues, that I've seen when
using arm-smmu driver with MMU-400 and to enable support for SMMUs on
Calxeda ECX-2000.

Patches are rebased on Joerg's latest joro/next branch (as of today).
I hope that I have addressed all (most) comments from the last
submission for following patches:

 [PATCH 1/9] iommu/arm-smmu: Switch to arch_initcall for driver ...
 [PATCH 2/9] iommu/arm-smmu: Calculate SMMU_CB_BASE from smmu ...
 [PATCH 3/9] ARM: dma-mapping: Always pass proper prot flags to ...
 [PATCH 4/9] iommu/arm-smmu: Check for num_context_irqs > 0 to avoid ...
 [PATCH 5/9] iommu/arm-smmu: Clear global and context bank fault ...

This time I've added patches for SMMU support on Calxeda ECX-2000.
Unfortunately this requires a quirk in the arm-smmu driver
(and that is the ugly part):

 [PATCH 6/9] iommu/arm-smmu: Support buggy implemenations where all
             config accesses are secure

I still kept the patch (slightly modified) to introduce a function to
isolate devices. I also merged the command line stuff into that patch
because it mainly belongs to it:

 [PATCH 7/9] iommu/arm-smmu: Add function that conditionally isolates
             all masters of all SMMUs

 (Maybe it's better to use a bus notifier to attach/detach devices
 instead. But I just started to look into this and code is not yet
 ready+tested.)

Also I think we need a default handler for context faults
Otherwise we won't notice when something goes wrong:

 [PATCH 8/9] iommu/arm-smmu: Introduce a default fault handler

And finally here's the dts-adaption to enable SMMUs

 [PATCH 9/9] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000

Would be nice if the noncontroversial patches (1-5?) could be applied.
(Esp. in case there are longer discussions about the rest.)

Comments welcome.


Thanks,

Andreas

[PATCH 1/9] 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 181c9ba..6808577 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1976,13 +1976,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 1/9] iommu/arm-smmu: Switch to arch_initcall for driver registration

[Will Deacon]

On Thu, Sep 26, 2013 at 11:36:13PM +0100, Andreas Herrmann wrote:
> 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.

Doesn't hurt to leave the exit function there though, right?

> 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 181c9ba..6808577 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1976,13 +1976,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);

Why not subsys_initcall, like the other ARM IOMMUs?

Will

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

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 04:58:12AM -0400, Will Deacon wrote:
> On Thu, Sep 26, 2013 at 11:36:13PM +0100, Andreas Herrmann wrote:
> > 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.
> 
> Doesn't hurt to leave the exit function there though, right?

Yes.

> > 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 181c9ba..6808577 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1976,13 +1976,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);
> 
> Why not subsys_initcall, like the other ARM IOMMUs?

No specific reason. I just tried to load the driver at the earliest
point during boot.

I just retested successful with subsys_initcall. The initialization
sequence looks fine (and yes, arch_initcall is overkill).

Will adapt this.


Andreas

[PATCH] iommu/arm-smmu: Switch to subsys_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.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 181c9ba..548f5d1 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1981,7 +1981,7 @@ static void __exit arm_smmu_exit(void)
 	return platform_driver_unregister(&arm_smmu_driver);
 }
 
-module_init(arm_smmu_init);
+subsys_initcall(arm_smmu_init);
 module_exit(arm_smmu_exit);
 
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMU implementations");
-- 
1.7.9.5

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

[Andreas Herrmann]

Currently it is derived from smmu resource size.  In case of a
mismatchin between the two calculations trust DT more than register
values and overwrite cb_base.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 6808577..4307fbc 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;
 
@@ -1622,6 +1623,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	unsigned long size;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 	u32 id;
+	unsigned long t;
 
 	dev_notice(smmu->dev, "probing hardware configuration...\n");
 
@@ -1702,12 +1704,23 @@ 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",
 			 size, smmu->size);
 
+	t = (unsigned long) smmu->base + (smmu->size >> 1);
+	if ((unsigned long)smmu->cb_base != t) {
+		dev_warn(smmu->dev, "address space mismatch, "
+			"overwriting cb_base (old: 0x%lx, new: 0x%lx)\n",
+			(unsigned long) smmu->cb_base, t);
+		smmu->cb_base = (void *) t;
+	}
+
 	smmu->num_s2_context_banks = (id >> ID1_NUMS2CB_SHIFT) &
 				      ID1_NUMS2CB_MASK;
 	smmu->num_context_banks = (id >> ID1_NUMCB_SHIFT) & ID1_NUMCB_MASK;
-- 
1.7.9.5

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

[Will Deacon]

Hi Andreas,

On Thu, Sep 26, 2013 at 11:36:14PM +0100, Andreas Herrmann wrote:
> Currently it is derived from smmu resource size.  In case of a
> mismatchin between the two calculations trust DT more than register
> values and overwrite cb_base.

I thought the driver already favoured the DT?

> @@ -1702,12 +1704,23 @@ 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",
>  			 size, smmu->size);
>  
> +	t = (unsigned long) smmu->base + (smmu->size >> 1);
> +	if ((unsigned long)smmu->cb_base != t) {
> +		dev_warn(smmu->dev, "address space mismatch, "
> +			"overwriting cb_base (old: 0x%lx, new: 0x%lx)\n",
> +			(unsigned long) smmu->cb_base, t);
> +		smmu->cb_base = (void *) t;
> +	}
> +

I expect I'm just being slow here (only one coffee in), but I can't see what
this gets us over the current use of resource_size (which goes and uses the
DT).

Will

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

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 05:51:57AM -0400, Will Deacon wrote:
> Hi Andreas,
> 
> On Thu, Sep 26, 2013 at 11:36:14PM +0100, Andreas Herrmann wrote:
> > Currently it is derived from smmu resource size.  In case of a
> > mismatchin between the two calculations trust DT more than register
> > values and overwrite cb_base.
> 
> I thought the driver already favoured the DT?
> 
> > @@ -1702,12 +1704,23 @@ 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",
> >  			 size, smmu->size);
> >  
> > +	t = (unsigned long) smmu->base + (smmu->size >> 1);
> > +	if ((unsigned long)smmu->cb_base != t) {
> > +		dev_warn(smmu->dev, "address space mismatch, "
> > +			"overwriting cb_base (old: 0x%lx, new: 0x%lx)\n",
> > +			(unsigned long) smmu->cb_base, t);
> > +		smmu->cb_base = (void *) t;
> > +	}
> > +
> 
> I expect I'm just being slow here (only one coffee in), but I can't see what
> this gets us over the current use of resource_size (which goes and uses the
> DT).

On balance it adds a warning if there is an inconsistency between the
resource size and the relevant registers describing the SMMU address
space.

If there is no such warning you might use a broken DT (with wrong
resource size) causing weird malfunction (because cb_base might be
wrong) and it will take some effort to root cause this.

But if you see such a warning it's clear what to check first.

I admit the commit message is misleading (somehow inherited from the
previous patch version) and maybe this kind of consistency check
should not be combined with cb_base calculation but moved into a
separate function called from arm_smmu_device_cfg_probe.


Andreas

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

[Will Deacon]

On Fri, Sep 27, 2013 at 11:23:59AM +0100, Andreas Herrmann wrote:
> On Fri, Sep 27, 2013 at 05:51:57AM -0400, Will Deacon wrote:
> > Hi Andreas,
> > 
> > On Thu, Sep 26, 2013 at 11:36:14PM +0100, Andreas Herrmann wrote:
> > > Currently it is derived from smmu resource size.  In case of a
> > > mismatchin between the two calculations trust DT more than register
> > > values and overwrite cb_base.
> > 
> > I thought the driver already favoured the DT?
> > 
> > > @@ -1702,12 +1704,23 @@ 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",
> > >  			 size, smmu->size);
> > >  
> > > +	t = (unsigned long) smmu->base + (smmu->size >> 1);
> > > +	if ((unsigned long)smmu->cb_base != t) {
> > > +		dev_warn(smmu->dev, "address space mismatch, "
> > > +			"overwriting cb_base (old: 0x%lx, new: 0x%lx)\n",
> > > +			(unsigned long) smmu->cb_base, t);
> > > +		smmu->cb_base = (void *) t;
> > > +	}
> > > +
> > 
> > I expect I'm just being slow here (only one coffee in), but I can't see what
> > this gets us over the current use of resource_size (which goes and uses the
> > DT).
> 
> On balance it adds a warning if there is an inconsistency between the
> resource size and the relevant registers describing the SMMU address
> space.

Well, we should already print the "device is 0x%lx bytes but only mapped
0x%lx!" message, which I think is enough to go and figure out what happened.

Will

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

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 06:51:53AM -0400, Will Deacon wrote:
> On Fri, Sep 27, 2013 at 11:23:59AM +0100, Andreas Herrmann wrote:
> > On Fri, Sep 27, 2013 at 05:51:57AM -0400, Will Deacon wrote:
> > > Hi Andreas,
> > > 
> > > On Thu, Sep 26, 2013 at 11:36:14PM +0100, Andreas Herrmann wrote:
> > > > Currently it is derived from smmu resource size.  In case of a
> > > > mismatchin between the two calculations trust DT more than register
> > > > values and overwrite cb_base.
> > > 
> > > I thought the driver already favoured the DT?
> > > 
> > > > @@ -1702,12 +1704,23 @@ 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",
> > > >  			 size, smmu->size);
> > > >  
> > > > +	t = (unsigned long) smmu->base + (smmu->size >> 1);
> > > > +	if ((unsigned long)smmu->cb_base != t) {
> > > > +		dev_warn(smmu->dev, "address space mismatch, "
> > > > +			"overwriting cb_base (old: 0x%lx, new: 0x%lx)\n",
> > > > +			(unsigned long) smmu->cb_base, t);
> > > > +		smmu->cb_base = (void *) t;
> > > > +	}
> > > > +
> > > 
> > > I expect I'm just being slow here (only one coffee in), but I can't see what
> > > this gets us over the current use of resource_size (which goes and uses the
> > > DT).
> > 
> > On balance it adds a warning if there is an inconsistency between the
> > resource size and the relevant registers describing the SMMU address
> > space.
> 
> Well, we should already print the "device is 0x%lx bytes but only mapped
> 0x%lx!" message, which I think is enough to go and figure out what happened.

No, you can map a larger region and still wrongly calculate cb_base
w/o this warning.
So the required check is something like

         /* Check for mismatch between SMMU address space size and size of mapped region */
         size = 1 << (((id >> ID1_NUMPAGENDXB_SHIFT) & ID1_NUMPAGENDXB_MASK) + 1);
         size *= (smmu->pagesize << 1);
         if (smmu->size != size)
                 dev_warn(smmu->dev,
                          "SMMU_GLOBAL_SIZE (0x%lx) differs from mapped "
			  region size (0x%lx)!\n", size, smmu->size);



Andreas

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

[Will Deacon]

On Fri, Sep 27, 2013 at 12:05:21PM +0100, Andreas Herrmann wrote:
> On Fri, Sep 27, 2013 at 06:51:53AM -0400, Will Deacon wrote:
> > Well, we should already print the "device is 0x%lx bytes but only mapped
> > 0x%lx!" message, which I think is enough to go and figure out what happened.
> 
> No, you can map a larger region and still wrongly calculate cb_base
> w/o this warning.

Gotcha.

> So the required check is something like
> 
>          /* Check for mismatch between SMMU address space size and size of mapped region */
>          size = 1 << (((id >> ID1_NUMPAGENDXB_SHIFT) & ID1_NUMPAGENDXB_MASK) + 1);
>          size *= (smmu->pagesize << 1);
>          if (smmu->size != size)
>                  dev_warn(smmu->dev,
>                           "SMMU_GLOBAL_SIZE (0x%lx) differs from mapped "
> 			  region size (0x%lx)!\n", size, smmu->size);

Sure, tightening up that check sounds like a good idea.
Looking forward to the next version of the patches!

(I need some more time to think about that secure config access patch :).

Will

[PATCH] iommu/arm-smmu: Refine check for proper size of mapped region

[Andreas Herrmann]

There is already a check to print a warning if the size of SMMU
address space (calculated from SMMU register values) is greater than
the size of the mapped memory region (e.g. passed via DT to the
driver).

Adapt this check to print also a warning in case the mapped region is
larger than the SMMU address space.

Such a mismatch could be intentional (to fix wrong register values).
If its not intentional (e.g. due to wrong DT information) this will
very likely cause a malfunction of the driver as SMMU_CB_BASE is
derived from the size of the mapped region. The warning helps to
identify the root cause in this case.

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

FYI, posted the patch to the mail thread for reference.
I'll send updated patches as a series asap.


Andreas

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 548f5d1..d19676c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1700,13 +1700,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID1);
 	smmu->pagesize = (id & ID1_PAGESIZE) ? SZ_64K : SZ_4K;
 
-	/* Check that we ioremapped enough */
+	/* Check for size mismatch of SMMU address space from mapped region */
 	size = 1 << (((id >> ID1_NUMPAGENDXB_SHIFT) & ID1_NUMPAGENDXB_MASK) + 1);
 	size *= (smmu->pagesize << 1);
-	if (smmu->size < size)
-		dev_warn(smmu->dev,
-			 "device is 0x%lx bytes but only mapped 0x%lx!\n",
-			 size, smmu->size);
+	if (smmu->size != size)
+		dev_warn(smmu->dev, "SMMU address space size (0x%lx) differs "
+			"from mapped region size (0x%lx)!\n", size, smmu->size);
 
 	smmu->num_s2_context_banks = (id >> ID1_NUMS2CB_SHIFT) &
 				      ID1_NUMS2CB_MASK;
-- 
1.7.9.5

[PATCH 3/9] 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 |   43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index f5e1a84..1272ed2 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;
@@ -1431,6 +1432,27 @@ static int arm_iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
 					 GFP_KERNEL);
 }
 
+static int __dma_direction_to_prot(enum dma_data_direction dir)
+{
+	int prot;
+
+	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;
+	}
+
+	return prot;
+}
+
 /*
  * Map a part of the scatter-gather list into contiguous io address space
  */
@@ -1444,6 +1466,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 +1483,9 @@ 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);
+		prot = __dma_direction_to_prot(dir);
+
+		ret = iommu_map(mapping->domain, iova, phys, len, prot);
 		if (ret < 0)
 			goto fail;
 		count += len >> PAGE_SHIFT;
@@ -1665,19 +1690,7 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
 	if (dma_addr == DMA_ERROR_CODE)
 		return dma_addr;
 
-	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;
-	}
+	prot = __dma_direction_to_prot(dir);
 
 	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot);
 	if (ret < 0)
-- 
1.7.9.5

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

[Will Deacon]

On Thu, Sep 26, 2013 at 11:36:15PM +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>

  Acked-by: Will Deacon <will.deacon@arm.com>

This one should go via the dma-mapping tree.

Will

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

[Marek Szyprowski]

Hello,

On 2013-09-27 00:36, 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>

Thanks pointing the issue and preparing the patch. I will push it to the 
dma-mapping fixes branch.

> ---
>   arch/arm/mm/dma-mapping.c |   43 ++++++++++++++++++++++++++++---------------
>   1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index f5e1a84..1272ed2 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;
> @@ -1431,6 +1432,27 @@ static int arm_iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
>   					 GFP_KERNEL);
>   }
>   
> +static int __dma_direction_to_prot(enum dma_data_direction dir)
> +{
> +	int prot;
> +
> +	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;
> +	}
> +
> +	return prot;
> +}
> +
>   /*
>    * Map a part of the scatter-gather list into contiguous io address space
>    */
> @@ -1444,6 +1466,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 +1483,9 @@ 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);
> +		prot = __dma_direction_to_prot(dir);
> +
> +		ret = iommu_map(mapping->domain, iova, phys, len, prot);
>   		if (ret < 0)
>   			goto fail;
>   		count += len >> PAGE_SHIFT;
> @@ -1665,19 +1690,7 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
>   	if (dma_addr == DMA_ERROR_CODE)
>   		return dma_addr;
>   
> -	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;
> -	}
> +	prot = __dma_direction_to_prot(dir);
>   
>   	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot);
>   	if (ret < 0)

Best regards
-- 
Marek Szyprowski
Samsung R&D Institute Poland

[PATCH 4/9] 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 when probing
for SMMU devices.

Rationale: Assuming that at least one context bank for non-secure
usage is provided per SMMU, it follows (from ARM SMMU Architecture
Spec) that at least one context interrupt must be available.

Also remove the line of code that derived num_context_irqs from
num_irqs and num_global_irqs. If DT is wrong and interrupt property
contains less interrupts than num_global_irqs this would set
num_context_irqs to a big u32 value which most likely causes trouble
in other parts of the driver.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 4307fbc..de9dd60 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1822,7 +1822,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 			 num_irqs, smmu->num_global_irqs);
 		smmu->num_global_irqs = num_irqs;
 	}
-	smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;
+
+	if (!smmu->num_context_irqs) {
+		dev_err(dev, "no context interrupt specified in DT\n");
+		return -ENODEV;
+	}
 
 	smmu->irqs = devm_kzalloc(dev, sizeof(*smmu->irqs) * num_irqs,
 				  GFP_KERNEL);
-- 
1.7.9.5

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

[Will Deacon]

On Thu, Sep 26, 2013 at 11:36:16PM +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 when probing
> for SMMU devices.
> 
> Rationale: Assuming that at least one context bank for non-secure
> usage is provided per SMMU, it follows (from ARM SMMU Architecture
> Spec) that at least one context interrupt must be available.

One problem with this reasoning is that the interrupt line might just not be
wired up to the GIC, despite existing on the SMMU. Still, we needn't solve
that now (let's wait for somebody to build it first...).

> Also remove the line of code that derived num_context_irqs from
> num_irqs and num_global_irqs. If DT is wrong and interrupt property
> contains less interrupts than num_global_irqs this would set
> num_context_irqs to a big u32 value which most likely causes trouble
> in other parts of the driver.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 4307fbc..de9dd60 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1822,7 +1822,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  			 num_irqs, smmu->num_global_irqs);
>  		smmu->num_global_irqs = num_irqs;
>  	}
> -	smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;

Why are you deleting this line?

> +
> +	if (!smmu->num_context_irqs) {
> +		dev_err(dev, "no context interrupt specified in DT\n");

I'd avoid mentioning "DT" in the log message, just in case this ever starts
probing from something else.

Will

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

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 04:41:54AM -0400, Will Deacon wrote:
> On Thu, Sep 26, 2013 at 11:36:16PM +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 when probing
> > for SMMU devices.
> > 
> > Rationale: Assuming that at least one context bank for non-secure
> > usage is provided per SMMU, it follows (from ARM SMMU Architecture
> > Spec) that at least one context interrupt must be available.
> 
> One problem with this reasoning is that the interrupt line might just not be
> wired up to the GIC, despite existing on the SMMU. Still, we needn't solve
> that now (let's wait for somebody to build it first...).
> 
> > Also remove the line of code that derived num_context_irqs from
> > num_irqs and num_global_irqs. If DT is wrong and interrupt property
> > contains less interrupts than num_global_irqs this would set
> > num_context_irqs to a big u32 value which most likely causes trouble
> > in other parts of the driver.
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 4307fbc..de9dd60 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1822,7 +1822,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> >  			 num_irqs, smmu->num_global_irqs);
> >  		smmu->num_global_irqs = num_irqs;
> >  	}
> > -	smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;
> 
> Why are you deleting this line?

Because I felt it's redundant in some cases and erroneously I thought
it could be bogus if num_irqs < num_global_irqs.
Of course the latter is wrong, as num_global_irqs is corrected two
lines above.

Now I think it's always redundant. num_context_irqs is only
incremented here

               if (num_irqs > smmu->num_global_irqs)
                        smmu->num_context_irqs++;

So either it is still 0 (and no fixup required for num_irqs <
num_global_irqs) or it contains already a positive value based on
(num_irqs - num_global_irqs).

But maybe I've missed something.
(At least I need to fix the commit message wrt to this removal.)

> > +
> > +	if (!smmu->num_context_irqs) {
> > +		dev_err(dev, "no context interrupt specified in DT\n");
> 
> I'd avoid mentioning "DT" in the log message, just in case this ever starts
> probing from something else.

Ok, will fix this.


Andreas

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

[Will Deacon]

On Fri, Sep 27, 2013 at 10:03:48AM +0100, Andreas Herrmann wrote:
> On Fri, Sep 27, 2013 at 04:41:54AM -0400, Will Deacon wrote:
> > On Thu, Sep 26, 2013 at 11:36:16PM +0100, Andreas Herrmann wrote:
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > index 4307fbc..de9dd60 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -1822,7 +1822,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> > >  			 num_irqs, smmu->num_global_irqs);
> > >  		smmu->num_global_irqs = num_irqs;
> > >  	}
> > > -	smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;
> > 
> > Why are you deleting this line?
> 
> Because I felt it's redundant in some cases and erroneously I thought
> it could be bogus if num_irqs < num_global_irqs.
> Of course the latter is wrong, as num_global_irqs is corrected two
> lines above.
> 
> Now I think it's always redundant. num_context_irqs is only
> incremented here
> 
>                if (num_irqs > smmu->num_global_irqs)
>                         smmu->num_context_irqs++;

Yes, I think you're right. This leaves us in a situation where getting the
#global-interrupts property wrong will trigger a warning followed
immediately by a failure to probe. That means we could simply augment the
current check and make it fatal instead:

--->8

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 2931921..03ffbae 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1799,12 +1799,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
                        smmu->num_context_irqs++;
        }
 
-       if (num_irqs < smmu->num_global_irqs) {
+       if (num_irqs <= smmu->num_global_irqs) {
                dev_warn(dev, "found %d interrupts but expected at least %d\n",
-                        num_irqs, smmu->num_global_irqs);
-               smmu->num_global_irqs = num_irqs;
+                        num_irqs, smmu->num_global_irqs + 1);
+               return -ENODEV;
        }
-       smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;
 
        smmu->irqs = devm_kzalloc(dev, sizeof(*smmu->irqs) * num_irqs,
                                  GFP_KERNEL);

8<---

What do you think?

Will

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

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 06:23:07AM -0400, Will Deacon wrote:
> On Fri, Sep 27, 2013 at 10:03:48AM +0100, Andreas Herrmann wrote:
> > On Fri, Sep 27, 2013 at 04:41:54AM -0400, Will Deacon wrote:
> > > On Thu, Sep 26, 2013 at 11:36:16PM +0100, Andreas Herrmann wrote:
> > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > > index 4307fbc..de9dd60 100644
> > > > --- a/drivers/iommu/arm-smmu.c
> > > > +++ b/drivers/iommu/arm-smmu.c
> > > > @@ -1822,7 +1822,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> > > >  			 num_irqs, smmu->num_global_irqs);
> > > >  		smmu->num_global_irqs = num_irqs;
> > > >  	}
> > > > -	smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;
> > > 
> > > Why are you deleting this line?
> > 
> > Because I felt it's redundant in some cases and erroneously I thought
> > it could be bogus if num_irqs < num_global_irqs.
> > Of course the latter is wrong, as num_global_irqs is corrected two
> > lines above.
> > 
> > Now I think it's always redundant. num_context_irqs is only
> > incremented here
> > 
> >                if (num_irqs > smmu->num_global_irqs)
> >                         smmu->num_context_irqs++;
> 
> Yes, I think you're right. This leaves us in a situation where getting the
> #global-interrupts property wrong will trigger a warning followed
> immediately by a failure to probe. That means we could simply augment the
> current check and make it fatal instead:
> 
> --->8
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 2931921..03ffbae 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1799,12 +1799,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>                         smmu->num_context_irqs++;
>         }
>  
> -       if (num_irqs < smmu->num_global_irqs) {
+         /* expect num_global_irqs plus at least 1 context interrupt */
> +       if (num_irqs <= smmu->num_global_irqs) {
>                 dev_warn(dev, "found %d interrupts but expected at least %d\n",
> -                        num_irqs, smmu->num_global_irqs);
> -               smmu->num_global_irqs = num_irqs;
> +                        num_irqs, smmu->num_global_irqs + 1);
> +               return -ENODEV;
>         }
> -       smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;
>  
>         smmu->irqs = devm_kzalloc(dev, sizeof(*smmu->irqs) * num_irqs,
>                                   GFP_KERNEL);
> 
> 8<---
> 
> What do you think?

Yes, that should suffice.

I know it's clear for us but what about a short comment to emphasize
that we expect to find at least one context irq?

-       if (num_irqs < smmu->num_global_irqs) {
+       /* expect num_global_irqs plus at least one context irq */
+       if (num_irqs <= smmu->num_global_irqs) {

which can be translated to

-       if (num_irqs < smmu->num_global_irqs) {
+       if (!smmu->num_context_irqs) {


I don't care -- every version is fine (at least I know what to look
for, if this warning message shows up). Most important thing is that a
wrong DT should not cause a division by zero in the driver.


Andreas

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

[Will Deacon]

On Fri, Sep 27, 2013 at 11:39:49AM +0100, Andreas Herrmann wrote:
> On Fri, Sep 27, 2013 at 06:23:07AM -0400, Will Deacon wrote:
> > What do you think?
> 
> Yes, that should suffice.
> 
> I know it's clear for us but what about a short comment to emphasize
> that we expect to find at least one context irq?
> 
> -       if (num_irqs < smmu->num_global_irqs) {
> +       /* expect num_global_irqs plus at least one context irq */
> +       if (num_irqs <= smmu->num_global_irqs) {
> 
> which can be translated to
> 
> -       if (num_irqs < smmu->num_global_irqs) {
> +       if (!smmu->num_context_irqs) {

Even better! Do you want to send a patch, or shall I just create one myself?

Will

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

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 06:48:02AM -0400, Will Deacon wrote:
> On Fri, Sep 27, 2013 at 11:39:49AM +0100, Andreas Herrmann wrote:
> > On Fri, Sep 27, 2013 at 06:23:07AM -0400, Will Deacon wrote:
> > > What do you think?
> > 
> > Yes, that should suffice.
> > 
> > I know it's clear for us but what about a short comment to emphasize
> > that we expect to find at least one context irq?
> > 
> > -       if (num_irqs < smmu->num_global_irqs) {
> > +       /* expect num_global_irqs plus at least one context irq */
> > +       if (num_irqs <= smmu->num_global_irqs) {
> > 
> > which can be translated to
> > 
> > -       if (num_irqs < smmu->num_global_irqs) {
> > +       if (!smmu->num_context_irqs) {
> 
> Even better! Do you want to send a patch, or shall I just create one myself?

I can do it.
(Have to update other patches anyway.)


Andreas

[PATCH] 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 when probing
for SMMU devices.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d19676c..7d07561 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1803,12 +1803,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 			smmu->num_context_irqs++;
 	}
 
-	if (num_irqs < smmu->num_global_irqs) {
+	if (!smmu->num_context_irqs) {
 		dev_warn(dev, "found %d interrupts but expected at least %d\n",
-			 num_irqs, smmu->num_global_irqs);
-		smmu->num_global_irqs = num_irqs;
+			 num_irqs, smmu->num_global_irqs + 1);
+		return -ENODEV;
 	}
-	smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;
 
 	smmu->irqs = devm_kzalloc(dev, sizeof(*smmu->irqs) * num_irqs,
 				  GFP_KERNEL);
-- 
1.7.9.5

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

[Andreas Herrmann]

After reset these registers have unknown values.
This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
in handlers for combined interrupts.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index de9dd60..9d31ad9 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -642,6 +642,9 @@ 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);
+
 	/* CBAR */
 	reg = root_cfg->cbar;
 	if (smmu->version == 1)
@@ -1564,6 +1567,9 @@ 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);
+
 	/* 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 5/9] iommu/arm-smmu: Clear global and context bank fault status registers

[Will Deacon]

On Thu, Sep 26, 2013 at 11:36:17PM +0100, Andreas Herrmann wrote:
> After reset these registers have unknown values.
> This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
> in handlers for combined interrupts.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |    6 ++++++
>  1 file changed, 6 insertions(+)

<pedantry> Your capitalisation of fsr vs FSR is inconsistent </pedantry>

> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index de9dd60..9d31ad9 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -642,6 +642,9 @@ 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);

I think this is too late, since we've already requested the IRQ line for
this context bank by the time we get here. There are two options (afaict):

 (1) Clear the CB FSRs during probe *and* during domain destruction

 (2) Delay request_irq until after the context bank has been initialised

Also, rather than 0xffffffff, FSR_IGN | FSR_FAULT is probably clearer.

>  	/* CBAR */
>  	reg = root_cfg->cbar;
>  	if (smmu->version == 1)
> @@ -1564,6 +1567,9 @@ 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);

For this guy, a read of the register and a write back is probably better than
writing 1s to all the reserved bits.

Will

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

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 04:52:56AM -0400, Will Deacon wrote:
> On Thu, Sep 26, 2013 at 11:36:17PM +0100, Andreas Herrmann wrote:
> > After reset these registers have unknown values.
> > This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
> > in handlers for combined interrupts.
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> 
> <pedantry> Your capitalisation of fsr vs FSR is inconsistent </pedantry>
> 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index de9dd60..9d31ad9 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -642,6 +642,9 @@ 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);
> 
> I think this is too late, since we've already requested the IRQ line for
> this context bank by the time we get here. There are two options (afaict):
> 
>  (1) Clear the CB FSRs during probe *and* during domain destruction
> 
>  (2) Delay request_irq until after the context bank has been initialised

I'd do kind of (2) and clear the FSR of that context bank before
request_irq is called.
 
> Also, rather than 0xffffffff, FSR_IGN | FSR_FAULT is probably clearer.

Yep.

> >  	/* CBAR */
> >  	reg = root_cfg->cbar;
> >  	if (smmu->version == 1)
> > @@ -1564,6 +1567,9 @@ 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);
> 
> For this guy, a read of the register and a write back is probably better than
> writing 1s to all the reserved bits.

Ok.

Andreas

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

[Andreas Herrmann]

After reset these registers have unknown values.
This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
in handlers for combined interrupts.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 579b6f8..cbbf597 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -631,6 +631,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static void arm_smmu_clear_cb_fsr(struct arm_smmu_device *smmu, u8 cbndx)
+{
+	void __iomem *cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
+	writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
+}
+
 static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 {
 	u32 reg;
@@ -838,6 +844,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 		root_cfg->irptndx = root_cfg->cbndx;
 	}
 
+	arm_smmu_clear_cb_fsr(smmu, 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);
@@ -1564,7 +1571,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 	void __iomem *sctlr_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB_SCTLR;
 	int i = 0;
-	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+	u32 val;
+
+	/* clear global FSR */
+	val = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
+	writel(val, gr0_base + ARM_SMMU_GR0_sGFSR);
 
 	/* Mark all SMRn as invalid and all S2CRn as bypass */
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
@@ -1581,24 +1592,26 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH);
 	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH);
 
+	val = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+
 	/* Enable fault reporting */
-	scr0 |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
+	val |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
 
 	/* Disable TLB broadcasting. */
-	scr0 |= (sCR0_VMIDPNE | sCR0_PTM);
+	val |= (sCR0_VMIDPNE | sCR0_PTM);
 
 	/* Enable client access, but bypass when no mapping is found */
-	scr0 &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
+	val &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
 
 	/* Disable forced broadcasting */
-	scr0 &= ~sCR0_FB;
+	val &= ~sCR0_FB;
 
 	/* Don't upgrade barriers */
-	scr0 &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
+	val &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
 
 	/* Push the button */
 	arm_smmu_tlb_sync(smmu);
-	writel(scr0, gr0_base + ARM_SMMU_GR0_sCR0);
+	writel(val, gr0_base + ARM_SMMU_GR0_sCR0);
 }
 
 static int arm_smmu_id_size_to_bits(int size)
-- 
1.7.9.5

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

[Will Deacon]

On Mon, Sep 30, 2013 at 02:56:21PM +0100, Andreas Herrmann wrote:
> 
> After reset these registers have unknown values.
> This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
> in handlers for combined interrupts.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 579b6f8..cbbf597 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -631,6 +631,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>  	return IRQ_HANDLED;
>  }
>  
> +static void arm_smmu_clear_cb_fsr(struct arm_smmu_device *smmu, u8 cbndx)
> +{
> +	void __iomem *cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
> +	writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
> +}

Hmm, why not just stick this in arm_smmu_init_context_bank...

>  static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
>  {
>  	u32 reg;
> @@ -838,6 +844,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  		root_cfg->irptndx = root_cfg->cbndx;
>  	}
>  
> +	arm_smmu_clear_cb_fsr(smmu, root_cfg->cbndx);

... then move that function call here instead of where it currently is?

>  	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);
> @@ -1564,7 +1571,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>  	void __iomem *sctlr_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB_SCTLR;
>  	int i = 0;
> -	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> +	u32 val;

I guess we have to dilute the meaning of the variable, but perhaps `reg' is
slightly more indicative than `val'?

Do you plan to post all your patches as a new series (it's hard to keep
track of what to pick)?

Cheers,

Will

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

[Andreas Herrmann]

On Mon, Sep 30, 2013 at 12:06:15PM -0400, Will Deacon wrote:
> On Mon, Sep 30, 2013 at 02:56:21PM +0100, Andreas Herrmann wrote:
> > 
> > After reset these registers have unknown values.
> > This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
> > in handlers for combined interrupts.
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |   27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 579b6f8..cbbf597 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -631,6 +631,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static void arm_smmu_clear_cb_fsr(struct arm_smmu_device *smmu, u8 cbndx)
> > +{
> > +	void __iomem *cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
> > +	writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
> > +}
> 
> Hmm, why not just stick this in arm_smmu_init_context_bank...

Because we should clear the FSR before we call request_irq.
Otherwise we might handle interrupts although the context bank is not
enabled.

Moving request_irq after arm_smmu_init_context_bank is not optimal
either. (We should have configured the context interrupt before
translation is enabled. Otherwise it's possible to miss a fault.)

> >  static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
> >  {
> >  	u32 reg;
> > @@ -838,6 +844,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >  		root_cfg->irptndx = root_cfg->cbndx;
> >  	}
> >  
> > +	arm_smmu_clear_cb_fsr(smmu, root_cfg->cbndx);
> 
> ... then move that function call here instead of where it currently is?

See above.

> >  	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);
> > @@ -1564,7 +1571,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >  	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> >  	void __iomem *sctlr_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB_SCTLR;
> >  	int i = 0;
> > -	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> > +	u32 val;
> 
> I guess we have to dilute the meaning of the variable, but perhaps `reg' is
> slightly more indicative than `val'?

Will change that.

> Do you plan to post all your patches as a new series (it's hard to keep
> track of what to pick)?

Yes.


Andreas

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

[Will Deacon]

On Mon, Sep 30, 2013 at 06:17:16PM +0100, Andreas Herrmann wrote:
> On Mon, Sep 30, 2013 at 12:06:15PM -0400, Will Deacon wrote:
> > On Mon, Sep 30, 2013 at 02:56:21PM +0100, Andreas Herrmann wrote:
> > > 
> > > After reset these registers have unknown values.
> > > This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
> > > in handlers for combined interrupts.
> > > 
> > > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > > ---
> > >  drivers/iommu/arm-smmu.c |   27 ++++++++++++++++++++-------
> > >  1 file changed, 20 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > index 579b6f8..cbbf597 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -631,6 +631,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > +static void arm_smmu_clear_cb_fsr(struct arm_smmu_device *smmu, u8 cbndx)
> > > +{
> > > +	void __iomem *cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
> > > +	writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
> > > +}
> > 
> > Hmm, why not just stick this in arm_smmu_init_context_bank...
> 
> Because we should clear the FSR before we call request_irq.
> Otherwise we might handle interrupts although the context bank is not
> enabled.
> 
> Moving request_irq after arm_smmu_init_context_bank is not optimal
> either. (We should have configured the context interrupt before
> translation is enabled. Otherwise it's possible to miss a fault.)

How would you miss a fault? If the device can start issuing transactions
before the SMMU has set up the mapping, then there's a race in the caller
code which we shouldn't attempt to resolve here.

Will

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

[Andreas Herrmann]

On Mon, Sep 30, 2013 at 02:30:06PM -0400, Will Deacon wrote:
> On Mon, Sep 30, 2013 at 06:17:16PM +0100, Andreas Herrmann wrote:
> > On Mon, Sep 30, 2013 at 12:06:15PM -0400, Will Deacon wrote:
> > > On Mon, Sep 30, 2013 at 02:56:21PM +0100, Andreas Herrmann wrote:
> > > > 
> > > > After reset these registers have unknown values.
> > > > This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
> > > > in handlers for combined interrupts.
> > > > 
> > > > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > > > ---
> > > >  drivers/iommu/arm-smmu.c |   27 ++++++++++++++++++++-------
> > > >  1 file changed, 20 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > > > index 579b6f8..cbbf597 100644
> > > > --- a/drivers/iommu/arm-smmu.c
> > > > +++ b/drivers/iommu/arm-smmu.c
> > > > @@ -631,6 +631,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> > > >  	return IRQ_HANDLED;
> > > >  }
> > > >  
> > > > +static void arm_smmu_clear_cb_fsr(struct arm_smmu_device *smmu, u8 cbndx)
> > > > +{
> > > > +	void __iomem *cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
> > > > +	writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
> > > > +}
> > > 
> > > Hmm, why not just stick this in arm_smmu_init_context_bank...
> > 
> > Because we should clear the FSR before we call request_irq.
> > Otherwise we might handle interrupts although the context bank is not
> > enabled.
> > 
> > Moving request_irq after arm_smmu_init_context_bank is not optimal
> > either. (We should have configured the context interrupt before
> > translation is enabled. Otherwise it's possible to miss a fault.)
> 
> How would you miss a fault?

> If the device can start issuing transactions before the SMMU has set
> up the mapping, then there's a race in the caller code which we
> shouldn't attempt to resolve here.

Broken device, broken driver code (maybe violating dma-api)
whatsoever. All that's needed is a device that is already doing DMA
when we enable SMMU handling for its transactions.
Yes, normally this should not happen. But if it happens we get a fault
and better should handle it.

I think, it's somehow logical to have fault handling set up before
fault reporting is switched on for a context bank.


Andreas

[PATCH 6/9] iommu/arm-smmu: Support buggy implemenations where all config accesses are secure

[Andreas Herrmann]

In such a case we have to use secure aliases of some non-secure
registers.

This behaviour is controlled via a flag in smmu->bugs. It is set
based on DT information when probing an SMMU device.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 9d31ad9..5fa34f9 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -159,6 +159,12 @@
 #define PIDR2_ARCH_SHIFT		4
 #define PIDR2_ARCH_MASK			0xf
 
+/* secure aliases for non-secure registers */
+#define ARM_SMMU_GR0_nsCR0		0x400
+#define ARM_SMMU_GR0_nsGFSR		0x448
+#define ARM_SMMU_GR0_nsGFSYNR0		0x450
+#define ARM_SMMU_GR0_nsGFSYNR1		0x454
+
 /* Global TLB invalidation */
 #define ARM_SMMU_GR0_STLBIALL		0x60
 #define ARM_SMMU_GR0_TLBIVMID		0x64
@@ -349,6 +355,8 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
 #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
 	u32				features;
+#define ARM_SMMU_BUG_SECURE_CFG_ACCESS	(1 << 0)
+	u32				bugs;
 	int				version;
 
 	u32				num_context_banks;
@@ -611,21 +619,32 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
 	struct arm_smmu_device *smmu = dev;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 
-	gfsr = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
+	if (smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS) {
+		gfsr = readl_relaxed(gr0_base + ARM_SMMU_GR0_nsGFSR);
+		gfsynr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_nsGFSYNR0);
+		gfsynr1 = readl_relaxed(gr0_base + ARM_SMMU_GR0_nsGFSYNR1);
+		gfsynr2 = 0;
+	} else {
+		gfsr = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
+		gfsynr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR0);
+		gfsynr1 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR1);
+		gfsynr2 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR2);
+	}
+
 	if (!gfsr)
 		return IRQ_NONE;
 
-	gfsynr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR0);
-	gfsynr1 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR1);
-	gfsynr2 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR2);
-
 	dev_err_ratelimited(smmu->dev,
 		"Unexpected global fault, this could be serious\n");
 	dev_err_ratelimited(smmu->dev,
 		"\tGFSR 0x%08x, GFSYNR0 0x%08x, GFSYNR1 0x%08x, GFSYNR2 0x%08x\n",
 		gfsr, gfsynr0, gfsynr1, gfsynr2);
 
-	writel(gfsr, gr0_base + ARM_SMMU_GR0_sGFSR);
+	if (smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS)
+		writel(gfsr, gr0_base + ARM_SMMU_GR0_nsGFSR);
+	else
+		writel(gfsr, gr0_base + ARM_SMMU_GR0_sGFSR);
+
 	return IRQ_HANDLED;
 }
 
@@ -1565,10 +1584,16 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 	void __iomem *sctlr_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB_SCTLR;
 	int i = 0;
-	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+	u32 cr0;
 
 	/* clear global FSRs */
-	writel(0xffffffff, gr0_base + ARM_SMMU_GR0_sGFSR);
+	if (smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS) {
+		writel(0xffffffff, gr0_base + ARM_SMMU_GR0_nsGFSR);
+		cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_nsCR0);
+	} else {
+		writel(0xffffffff, gr0_base + ARM_SMMU_GR0_sGFSR);
+		cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+	}
 
 	/* Mark all SMRn as invalid and all S2CRn as bypass */
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
@@ -1586,23 +1611,26 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH);
 
 	/* Enable fault reporting */
-	scr0 |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
+	cr0 |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
 
 	/* Disable TLB broadcasting. */
-	scr0 |= (sCR0_VMIDPNE | sCR0_PTM);
+	cr0 |= (sCR0_VMIDPNE | sCR0_PTM);
 
 	/* Enable client access, but bypass when no mapping is found */
-	scr0 &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
+	cr0 &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
 
 	/* Disable forced broadcasting */
-	scr0 &= ~sCR0_FB;
+	cr0 &= ~sCR0_FB;
 
 	/* Don't upgrade barriers */
-	scr0 &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
+	cr0 &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
 
 	/* Push the button */
 	arm_smmu_tlb_sync(smmu);
-	writel(scr0, gr0_base + ARM_SMMU_GR0_sCR0);
+	if (smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS)
+		writel(cr0, gr0_base + ARM_SMMU_GR0_nsCR0);
+	else
+		writel(cr0, gr0_base + ARM_SMMU_GR0_sCR0);
 }
 
 static int arm_smmu_id_size_to_bits(int size)
@@ -1894,6 +1922,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (of_property_read_bool(dev->of_node, "calxeda,smmu-secure-cfg-access"))
+		smmu->bugs |= ARM_SMMU_BUG_SECURE_CFG_ACCESS;
+
 	INIT_LIST_HEAD(&smmu->list);
 	spin_lock(&arm_smmu_devices_lock);
 	list_add(&smmu->list, &arm_smmu_devices);
@@ -1956,7 +1987,10 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 		free_irq(smmu->irqs[i], smmu);
 
 	/* Turn the thing off */
-	writel(sCR0_CLIENTPD, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_sCR0);
+	if (smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS)
+		writel(sCR0_CLIENTPD, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_nsCR0);
+	else
+		writel(sCR0_CLIENTPD, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_sCR0);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 6/9] iommu/arm-smmu: Support buggy implemenations where all config accesses are secure

[Will Deacon]

On Thu, Sep 26, 2013 at 11:36:18PM +0100, Andreas Herrmann wrote:
> In such a case we have to use secure aliases of some non-secure
> registers.
> 
> This behaviour is controlled via a flag in smmu->bugs. It is set
> based on DT information when probing an SMMU device.

:(

I guess my main comment is just some understanding on how things are wired.
It's *just* the configuration access that is secure, right? Device
transactions are non-secure, so the existing TLB invalidation code will work
correctly?

Will

[PATCH 6/9] iommu/arm-smmu: Support buggy implemenations where all config accesses are secure

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 09:05:28AM -0400, Will Deacon wrote:
> On Thu, Sep 26, 2013 at 11:36:18PM +0100, Andreas Herrmann wrote:
> > In such a case we have to use secure aliases of some non-secure
> > registers.
> > 
> > This behaviour is controlled via a flag in smmu->bugs. It is set
> > based on DT information when probing an SMMU device.
> 
> :(
> 
> I guess my main comment is just some understanding on how things are wired.
> It's *just* the configuration access that is secure, right?

Correct.

> Device transactions are non-secure, so the existing TLB invalidation
> code will work correctly?

Yes, transactions are non-secure.

The TLB invalidation code (using SMMU_TLBIVMID for stage2) also seems
too work -- no problems triggered so far (neither with sata nor with
xgmac).


Andreas

[PATCH 7/9] iommu/arm-smmu: Add function that conditionally isolates all masters of all SMMUs

[Andreas Herrmann]

(Depending on DT information and module parameters) each device is put
into its own protection domain (if possible).  For configuration with
one or just a few 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.

Default is that device isolation is contolled per SMMU with SMMU node
property "linux,arm-smmu-isolate-devices" in a DT. If the property is
set for an SMMU node device isolation is performed.

Also introduce a module parameter:

 arm-smmu=off|force_isolation|disable_isolation

  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.
                (Even for SMMUs that don't have the property
                "linux,arm-smmu-isolate-devices" set in device
                tree.)

        disable_isolation
                Disable device_isolation globally.
                (ie. no isolation even for SMMUs for which
                "linux,arm-smmu-isolate-devices" is set.)

W/o device isolation the driver detects SMMUs but no translation is
configured (transactions just bypass translation process).

Note that for device isolation dma_base and size are derived from
(coherent_)dma_mask of the master device.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5fa34f9..48f3bfb 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
@@ -354,6 +355,7 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_TRANS_S1		(1 << 2)
 #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
 #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
+#define ARM_SMMU_FEAT_ISOLATE_DEVICES	(1 << 5)
 	u32				features;
 #define ARM_SMMU_BUG_SECURE_CFG_ACCESS	(1 << 0)
 	u32				bugs;
@@ -407,6 +409,10 @@ 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 bool arm_smmu_disable_isolation;
+
 static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
 						struct device_node *dev_node)
 {
@@ -1810,6 +1816,74 @@ 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) {
+		if (arm_smmu_disable_isolation ||
+			(!(smmu->features & ARM_SMMU_FEAT_ISOLATE_DEVICES)
+				&& !arm_smmu_force_isolation))
+			continue;
+		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);
+		if (smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS) {
+			cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_nsCR0);
+			cr0 |= sCR0_USFCFG;
+			writel(cr0, gr0_base + ARM_SMMU_GR0_nsCR0);
+		} else {
+			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;
@@ -1820,6 +1894,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");
@@ -1925,6 +2002,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	if (of_property_read_bool(dev->of_node, "calxeda,smmu-secure-cfg-access"))
 		smmu->bugs |= ARM_SMMU_BUG_SECURE_CFG_ACCESS;
 
+	if (of_property_read_bool(dev->of_node, "linux,arm-smmu-isolate-devices"))
+		smmu->features |= ARM_SMMU_FEAT_ISOLATE_DEVICES;
+
 	INIT_LIST_HEAD(&smmu->list);
 	spin_lock(&arm_smmu_devices_lock);
 	list_add(&smmu->list, &arm_smmu_devices);
@@ -2015,6 +2095,25 @@ 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 if(!strncmp(str, "disable_isolation", 17))
+			arm_smmu_disable_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;
@@ -2030,6 +2129,8 @@ static int __init arm_smmu_init(void)
 	if (!iommu_present(&amba_bustype))
 		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
 
+	arm_smmu_isolate_devices();
+
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 7/9] iommu/arm-smmu: Add function that conditionally isolates all masters of all SMMUs

[Will Deacon]

Hi Andreas,

On Thu, Sep 26, 2013 at 11:36:19PM +0100, Andreas Herrmann wrote:
> (Depending on DT information and module parameters) each device is put
> into its own protection domain (if possible).  For configuration with
> one or just a few 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.
> 
> Default is that device isolation is contolled per SMMU with SMMU node
> property "linux,arm-smmu-isolate-devices" in a DT. If the property is
> set for an SMMU node device isolation is performed.
> 
> Also introduce a module parameter:

Actually, I think I'd rather do away with the module paramater / command
line option altogether in favour of DT.

> +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) {
> +		if (arm_smmu_disable_isolation ||
> +			(!(smmu->features & ARM_SMMU_FEAT_ISOLATE_DEVICES)
> +				&& !arm_smmu_force_isolation))
> +			continue;
> +		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;

Hmm, this could be *huge* with 64-bit capable DMA controllers (think LPAE).
Russell also has some pending dma mask cleanup, which might break some
assumptions here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/199397.html

(namely that we're offsetting everything from zero).

Will

[PATCH 7/9] iommu/arm-smmu: Add function that conditionally isolates all masters of all SMMUs

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 09:00:01AM -0400, Will Deacon wrote:
> Hi Andreas,
> 
> On Thu, Sep 26, 2013 at 11:36:19PM +0100, Andreas Herrmann wrote:
> > (Depending on DT information and module parameters) each device is put
> > into its own protection domain (if possible).  For configuration with
> > one or just a few 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.
> > 
> > Default is that device isolation is contolled per SMMU with SMMU node
> > property "linux,arm-smmu-isolate-devices" in a DT. If the property is
> > set for an SMMU node device isolation is performed.
> > 
> > Also introduce a module parameter:
> 
> Actually, I think I'd rather do away with the module paramater / command
> line option altogether in favour of DT.
> 
> > +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) {
> > +		if (arm_smmu_disable_isolation ||
> > +			(!(smmu->features & ARM_SMMU_FEAT_ISOLATE_DEVICES)
> > +				&& !arm_smmu_force_isolation))
> > +			continue;
> > +		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;
> 
> Hmm, this could be *huge* with 64-bit capable DMA controllers (think LPAE).

Yes, agreed.
(And even for 32-bit DMA this requires a large bitmap_size for the
mapping.)

> Russell also has some pending dma mask cleanup, which might break some
> assumptions here:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/199397.html
> 
> (namely that we're offsetting everything from zero).

What do you think is a reasonable general value for the size of a
mapping? (Do we need a DT property to specify this?)

What about a size of 128 MB -- if I'm not mistaken this requires a
bitmap_size of 4K.


Andreas

[PATCH 7/9] iommu/arm-smmu: Add function that conditionally isolates all masters of all SMMUs

[Will Deacon]

On Mon, Oct 07, 2013 at 04:42:27PM +0100, Andreas Herrmann wrote:
> On Fri, Sep 27, 2013 at 09:00:01AM -0400, Will Deacon wrote:
> > On Thu, Sep 26, 2013 at 11:36:19PM +0100, Andreas Herrmann wrote:
> > > +	list_for_each_entry(smmu, &arm_smmu_devices, list) {
> > > +		if (arm_smmu_disable_isolation ||
> > > +			(!(smmu->features & ARM_SMMU_FEAT_ISOLATE_DEVICES)
> > > +				&& !arm_smmu_force_isolation))
> > > +			continue;
> > > +		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;
> > 
> > Hmm, this could be *huge* with 64-bit capable DMA controllers (think LPAE).
> 
> Yes, agreed.
> (And even for 32-bit DMA this requires a large bitmap_size for the
> mapping.)
> 
> > Russell also has some pending dma mask cleanup, which might break some
> > assumptions here:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/199397.html
> > 
> > (namely that we're offsetting everything from zero).
> 
> What do you think is a reasonable general value for the size of a
> mapping? (Do we need a DT property to specify this?)
> 
> What about a size of 128 MB -- if I'm not mistaken this requires a
> bitmap_size of 4K.

I don't think we can reasonably pick a sane number for this. We *could* try
doing it lazily (i.e. driving the mapping off a fault handler) but that's
pretty scary and probably doesn't interact well with endpoints incompatible
with the stall model (e.g. PCIe).

So the right solution is probably to get this information from the device
drivers, but I don't know what the interaction is with the dma_mask...
You'll need to do some digging.

Will

[PATCH 8/9] iommu/arm-smmu: Introduce a default fault handler

[Andreas Herrmann]

And register the default handler for domains that are created during
device isolation.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 48f3bfb..380c2a0 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1816,6 +1816,13 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
+static int arm_smmu_context_fault_handler(struct iommu_domain *domain,
+		struct device *dev, unsigned long iova, int flags, void *arg)
+{
+	dev_warn(dev, "context fault: iova=0x%08lx, flags=0x%x\n", iova, flags);
+	return 0;
+}
+
 extern struct platform_device *of_find_device_by_node(struct device_node *np);
 
 static int arm_smmu_isolate_devices(void)
@@ -1859,6 +1866,9 @@ static int arm_smmu_isolate_devices(void)
 				goto out;
 			}
 
+			iommu_set_fault_handler(mapping->domain,
+					arm_smmu_context_fault_handler, dev);
+
 			ret = arm_iommu_attach_device(dev, mapping);
 			if (ret < 0) {
 				dev_info(dev, "arm_iommu_attach_device failed\n");
-- 
1.7.9.5

[PATCH 8/9] iommu/arm-smmu: Introduce a default fault handler

[Will Deacon]

On Thu, Sep 26, 2013 at 11:36:20PM +0100, Andreas Herrmann wrote:
> And register the default handler for domains that are created during
> device isolation.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 48f3bfb..380c2a0 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1816,6 +1816,13 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  	return 0;
>  }
>  
> +static int arm_smmu_context_fault_handler(struct iommu_domain *domain,
> +		struct device *dev, unsigned long iova, int flags, void *arg)
> +{
> +	dev_warn(dev, "context fault: iova=0x%08lx, flags=0x%x\n", iova, flags);
> +	return 0;

Maybe we're better off sandwiching this into arm_smmu_context_fault when
report_iommu_fault doesn't handle the fault.

Will

[PATCH 8/9] iommu/arm-smmu: Introduce a default fault handler

[Andreas Herrmann]

On Fri, Sep 27, 2013 at 06:09:02AM -0400, Will Deacon wrote:
> On Thu, Sep 26, 2013 at 11:36:20PM +0100, Andreas Herrmann wrote:
> > And register the default handler for domains that are created during
> > device isolation.
> > 
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 48f3bfb..380c2a0 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1816,6 +1816,13 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >  	return 0;
> >  }
> >  
> > +static int arm_smmu_context_fault_handler(struct iommu_domain *domain,
> > +		struct device *dev, unsigned long iova, int flags, void *arg)
> > +{
> > +	dev_warn(dev, "context fault: iova=0x%08lx, flags=0x%x\n", iova, flags);
> > +	return 0;
> 
> Maybe we're better off sandwiching this into arm_smmu_context_fault when
> report_iommu_fault doesn't handle the fault.

... which would allow us to dump the entire cb fault syndrome register.
(which has more info than what we get from above flags)

Fine with me.



Andreas

[PATCH] iommu/arm-smmu: Print context fault information

[Andreas Herrmann]

Print context fault information when the fault was not handled by
report_iommu_fault.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 2a9acc1..5e1dad1 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -590,6 +590,9 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 		ret = IRQ_HANDLED;
 		resume = RESUME_RETRY;
 	} else {
+		dev_err_ratelimited(smmu->dev, "context fault: iova=0x%08lx, "
+				"fsynr=0x%x, cb=%d\n",
+				iova, fsynr, root_cfg->cbndx);
 		ret = IRQ_NONE;
 		resume = RESUME_TERMINATE;
 	}
-- 
1.7.9.5

[PATCH 9/9] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000

[Andreas Herrmann]

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 arch/arm/boot/dts/ecx-2000.dts    |   44 +++++++++++++++++++++++++++++++++++--
 arch/arm/boot/dts/ecx-common.dtsi |    9 +++++---
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/ecx-2000.dts b/arch/arm/boot/dts/ecx-2000.dts
index 139b40c..d60beb0 100644
--- a/arch/arm/boot/dts/ecx-2000.dts
+++ b/arch/arm/boot/dts/ecx-2000.dts
@@ -76,10 +76,11 @@
 	};
 
 	soc {
-		ranges = <0x00000000 0x00000000 0x00000000 0xffffffff>;
+		ranges = <0x0 0x0 0x0 0xffffffff>;
 
 		timer {
-			compatible = "arm,cortex-a15-timer", "arm,armv7-timer"; 			interrupts = <1 13 0xf08>,
+			compatible = "arm,cortex-a15-timer", "arm,armv7-timer";
+			interrupts = <1 13 0xf08>,
 				<1 14 0xf08>,
 				<1 11 0xf08>,
 				<1 10 0xf08>;
@@ -103,6 +104,45 @@
 			interrupts = <0 76 4  0 75 4  0 74 4  0 73 4>;
 		};
 	};
+
+	soc at 920000000 {
+		ranges = <0x9 0x20000000 0x9 0x20000000 0x290000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		interrupt-parent = <&intc>;
+
+		smmu_mac0: smmu at 920000000 {
+			compatible = "arm,mmu-400";
+			reg = <0x9 0x20000000 0x10000>;
+			#global-interrupts = <1>;
+			interrupts = <0 106 4 0 106 4>;
+			mmu-masters = <&mac0 0x0 0x1>;
+			calxeda,smmu-secure-cfg-access;
+			linux,arm-smmu-isolate-devices;
+		};
+
+		smmu_mac1: smmu at 920080000 {
+			compatible = "arm,mmu-400";
+			reg = <0x9 0x20080000 0x10000>;
+			#global-interrupts = <1>;
+			interrupts = <0 108 4 0 108 4>;
+			mmu-masters = <&mac1 0x0 0x1>;
+			calxeda,smmu-secure-cfg-access;
+			linux,arm-smmu-isolate-devices;
+		};
+
+		smmu_sata: smmu at 920180000 {
+			compatible = "arm,mmu-400";
+			reg = <0x00000009 0x20180000 0x10000>;
+			mmu-masters = <&sata 0x0 0x1>;
+			#global-interrupts = <1>;
+			interrupts = <0 114 4 0 114 4>;
+			calxeda,smmu-secure-cfg-access;
+			linux,arm-smmu-isolate-devices;
+		};
+	};
+
 };
 
 /include/ "ecx-common.dtsi"
diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
index e8559b7..6658710 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -25,11 +25,12 @@
 		compatible = "simple-bus";
 		interrupt-parent = <&intc>;
 
-		sata at ffe08000 {
+		sata: sata at ffe08000 {
 			compatible = "calxeda,hb-ahci";
 			reg = <0xffe08000 0x10000>;
 			interrupts = <0 83 4>;
 			dma-coherent;
+			#stream-id-cells = <2>;
 			calxeda,port-phys = <&combophy5 0 &combophy0 0
 					     &combophy0 1 &combophy0 2
 					     &combophy0 3>;
@@ -208,18 +209,20 @@
 			clock-names = "apb_pclk";
 		};
 
-		ethernet at fff50000 {
+		mac0: ethernet at fff50000 {
 			compatible = "calxeda,hb-xgmac";
 			reg = <0xfff50000 0x1000>;
 			interrupts = <0 77 4  0 78 4  0 79 4>;
 			dma-coherent;
+			#stream-id-cells = <2>;
 		};
 
-		ethernet at fff51000 {
+		mac1: ethernet at fff51000 {
 			compatible = "calxeda,hb-xgmac";
 			reg = <0xfff51000 0x1000>;
 			interrupts = <0 80 4  0 81 4  0 82 4>;
 			dma-coherent;
+			#stream-id-cells = <2>;
 		};
 
 		combophy0: combo-phy at fff58000 {
-- 
1.7.9.5