[RFC PATCH 0/6] iommu/arm-smmu: Add support for DMA domains and instruction fetch

[RFC PATCH 0/6] iommu/arm-smmu: Add support for DMA domains and instruction fetch

[Anup Patel]

This patchset adds following to SMMUv1/SMMUv2 driver:
1. Support for domain type IOMMU_DOMAIN_DMA
2. Allow privilege instruction fetchs from MMU masters by having
a DT option to treat instruction fetch as data read

The patchset is based on '4.5-rc1' tag of linux mainline tree
and is available in smmu_v1 branch of
https://github.com/Broadcom/arm64-linux.git

All patches have been tested on Broadcom SoCs having SMMU-500.

Anup Patel (5):
  iommu/arm-smmu: Invoke DT probe from arm_smmu_of_setup()
  of: iommu: Increment DT node refcount in of_iommu_set_ops()
  iommu/arm-smmu: Add support for IOMMU_DOMAIN_DMA in SMMUv1/SMMUv2
    driver
  iommu/arm-smmu: Option to treat instruction fetch as data read for
    SMMUv2
  iommu/arm-smmu: Update bindings document for smmu-inst-as-data DT
    option

Sricharan R (1):
  iommu/arm-smmu: Init driver using IOMMU_OF_DECLARE

 .../devicetree/bindings/iommu/arm,smmu.txt         |  8 +++
 drivers/iommu/arm-smmu.c                           | 70 +++++++++++++++++-----
 drivers/iommu/of_iommu.c                           |  1 +
 3 files changed, 65 insertions(+), 14 deletions(-)

-- 
1.9.1

[RFC PATCH 1/6] iommu/arm-smmu: Init driver using IOMMU_OF_DECLARE

[Anup Patel]

From: Sricharan R <sricharan@codeaurora.org>

This patch uses IOMMU_OF_DECLARE to register the driver
and the iommu_ops. So when master devices of the iommu are
registered, of_xlate callback can be used to add the master
configurations to the smmu driver.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Tested-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/iommu/arm-smmu.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 59ee4b8..22b9668 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -38,6 +38,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_iommu.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -257,6 +258,8 @@
 #define FSYNR0_WNR			(1 << 4)
 
 static int force_stage;
+static bool init_done;
+
 module_param_named(force_stage, force_stage, int, S_IRUGO);
 MODULE_PARM_DESC(force_stage,
 	"Force SMMU mappings to be installed@a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
@@ -1885,20 +1888,8 @@ static struct platform_driver arm_smmu_driver = {
 
 static int __init arm_smmu_init(void)
 {
-	struct device_node *np;
 	int ret;
 
-	/*
-	 * Play nice with systems that don't have an ARM SMMU by checking that
-	 * an ARM SMMU exists in the system before proceeding with the driver
-	 * and IOMMU bus operation registration.
-	 */
-	np = of_find_matching_node(NULL, arm_smmu_of_match);
-	if (!np)
-		return 0;
-
-	of_node_put(np);
-
 	ret = platform_driver_register(&arm_smmu_driver);
 	if (ret)
 		return ret;
@@ -1917,15 +1908,33 @@ static int __init arm_smmu_init(void)
 		bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
 #endif
 
+	init_done = true;
+
 	return 0;
 }
 
+static int __init arm_smmu_of_setup(struct device_node *np)
+{
+
+	if (!init_done)
+		arm_smmu_init();
+
+	of_iommu_set_ops(np, &arm_smmu_ops);
+
+	return 0;
+}
+
+IOMMU_OF_DECLARE(arm_smmu_v1, "arm,smmu-v1", arm_smmu_of_setup);
+IOMMU_OF_DECLARE(arm_smmu_v2, "arm,smmu-v2", arm_smmu_of_setup);
+IOMMU_OF_DECLARE(arm_smmu_400, "arm,mmu-400", arm_smmu_of_setup);
+IOMMU_OF_DECLARE(arm_smmu_401, "arm,mmu-401", arm_smmu_of_setup);
+IOMMU_OF_DECLARE(arm_smmu_500, "arm,mmu-500", arm_smmu_of_setup);
+
 static void __exit arm_smmu_exit(void)
 {
 	return platform_driver_unregister(&arm_smmu_driver);
 }
 
-subsys_initcall(arm_smmu_init);
 module_exit(arm_smmu_exit);
 
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMU implementations");
-- 
1.9.1

[RFC PATCH 2/6] iommu/arm-smmu: Invoke DT probe from arm_smmu_of_setup()

[Anup Patel]

The SMMUv1/SMMUv2 driver is initialized very early using the
IOMMU_OF_DECLARE() but the actual platform device is probed
via normal DT probing.

This patch uses of_platform_device_create() from arm_smmu_of_setup()
to ensure that SMMU platform device is probed immediately.

Signed-off-by: Anup Patel <anup.patel@broadcom.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 22b9668..9bdf6b2 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -39,6 +39,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_iommu.h>
+#include <linux/of_platform.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -1915,10 +1916,15 @@ static int __init arm_smmu_init(void)
 
 static int __init arm_smmu_of_setup(struct device_node *np)
 {
+	struct platform_device *pdev;
 
 	if (!init_done)
 		arm_smmu_init();
 
+	pdev = of_platform_device_create(np, NULL, NULL);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
 	of_iommu_set_ops(np, &arm_smmu_ops);
 
 	return 0;
-- 
1.9.1

[RFC PATCH 3/6] of: iommu: Increment DT node refcount in of_iommu_set_ops()

[Anup Patel]

We are saving pointer to iommu DT node in of_iommu_set_ops()
hence we should increment DT node ref count.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/iommu/of_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 60ba238..5fea665 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -110,6 +110,7 @@ void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops)
 	if (WARN_ON(!iommu))
 		return;
 
+	of_node_get(np);
 	INIT_LIST_HEAD(&iommu->list);
 	iommu->np = np;
 	iommu->ops = ops;
-- 
1.9.1

[RFC PATCH 3/6] of: iommu: Increment DT node refcount in of_iommu_set_ops()

[Robin Murphy]

On 27/01/16 05:21, Anup Patel wrote:
> We are saving pointer to iommu DT node in of_iommu_set_ops()
> hence we should increment DT node ref count.

Oh man, shame on whoever wrote that code! :P

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>   drivers/iommu/of_iommu.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 60ba238..5fea665 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -110,6 +110,7 @@ void of_iommu_set_ops(struct device_node *np, struct iommu_ops *ops)
>   	if (WARN_ON(!iommu))
>   		return;
>
> +	of_node_get(np);
>   	INIT_LIST_HEAD(&iommu->list);
>   	iommu->np = np;
>   	iommu->ops = ops;
>

[RFC PATCH 4/6] iommu/arm-smmu: Add support for IOMMU_DOMAIN_DMA in SMMUv1/SMMUv2 driver

[Anup Patel]

To allow use of large memory (> 4Gb) with 32bit devices we need to use
some kind of iommu for such 32bit devices.

This patch extends SMMUv1/SMMUv2 driver to support DMA domains which
in-turn will allows us to use iommu based DMA mappings for 32bit devices.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/iommu/arm-smmu.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 9bdf6b2..43424fe 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -29,6 +29,7 @@
 #define pr_fmt(fmt) "arm-smmu: " fmt
 
 #include <linux/delay.h>
+#include <linux/dma-iommu.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -967,7 +968,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 	/*
 	 * Allocate the domain and initialise some of its data structures.
@@ -978,6 +979,12 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	if (!smmu_domain)
 		return NULL;
 
+	if (type == IOMMU_DOMAIN_DMA &&
+	    iommu_get_dma_cookie(&smmu_domain->domain)) {
+		kfree(smmu_domain);
+		return NULL;
+	}
+
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->pgtbl_lock);
 
@@ -992,6 +999,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	 * Free the domain resources. We assume that all devices have
 	 * already been detached.
 	 */
+	iommu_put_dma_cookie(domain);
 	arm_smmu_destroy_domain_context(domain);
 	kfree(smmu_domain);
 }
@@ -1361,6 +1369,16 @@ static int arm_smmu_init_platform_device(struct device *dev,
 	return 0;
 }
 
+int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	/*
+	 * Nothing to do here because SMMU is already aware of all
+	 * MMU masters and their stream IDs using mmu-master attibute
+	 * SMMU DT node.
+	 */
+	return 0;
+}
+
 static int arm_smmu_add_device(struct device *dev)
 {
 	struct iommu_group *group;
@@ -1458,6 +1476,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.unmap			= arm_smmu_unmap,
 	.map_sg			= default_iommu_map_sg,
 	.iova_to_phys		= arm_smmu_iova_to_phys,
+	.of_xlate		= arm_smmu_of_xlate,
 	.add_device		= arm_smmu_add_device,
 	.remove_device		= arm_smmu_remove_device,
 	.device_group		= arm_smmu_device_group,
-- 
1.9.1

[RFC PATCH 4/6] iommu/arm-smmu: Add support for IOMMU_DOMAIN_DMA in SMMUv1/SMMUv2 driver

[Robin Murphy]

On 27/01/16 05:21, Anup Patel wrote:
> To allow use of large memory (> 4Gb) with 32bit devices we need to use
> some kind of iommu for such 32bit devices.
>
> This patch extends SMMUv1/SMMUv2 driver to support DMA domains which
> in-turn will allows us to use iommu based DMA mappings for 32bit devices.
>
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>   drivers/iommu/arm-smmu.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 9bdf6b2..43424fe 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -29,6 +29,7 @@
>   #define pr_fmt(fmt) "arm-smmu: " fmt
>
>   #include <linux/delay.h>
> +#include <linux/dma-iommu.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/err.h>
>   #include <linux/interrupt.h>
> @@ -967,7 +968,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>   {
>   	struct arm_smmu_domain *smmu_domain;
>
> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> +	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
>   		return NULL;
>   	/*
>   	 * Allocate the domain and initialise some of its data structures.
> @@ -978,6 +979,12 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>   	if (!smmu_domain)
>   		return NULL;
>
> +	if (type == IOMMU_DOMAIN_DMA &&
> +	    iommu_get_dma_cookie(&smmu_domain->domain)) {
> +		kfree(smmu_domain);
> +		return NULL;
> +	}
> +
>   	mutex_init(&smmu_domain->init_mutex);
>   	spin_lock_init(&smmu_domain->pgtbl_lock);
>
> @@ -992,6 +999,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
>   	 * Free the domain resources. We assume that all devices have
>   	 * already been detached.
>   	 */
> +	iommu_put_dma_cookie(domain);
>   	arm_smmu_destroy_domain_context(domain);
>   	kfree(smmu_domain);
>   }
> @@ -1361,6 +1369,16 @@ static int arm_smmu_init_platform_device(struct device *dev,
>   	return 0;
>   }
>
> +int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> +{
> +	/*
> +	 * Nothing to do here because SMMU is already aware of all
> +	 * MMU masters and their stream IDs using mmu-master attibute
> +	 * SMMU DT node.
> +	 */

...but on the same hand this will also never get called if there's no 
"iommus" property on the master. Maintaining support for existing users 
of the "mmu-masters" binding is one thing (namely the thing that's been 
slowing down my efforts to clean up the really hacky generic binding 
support I did all the DMA stuff with), but having _both_ bindings in a 
single DT is something I don't think anybody wants to see - is that how 
you've tested this?

Robin.

> +	return 0;
> +}
> +
>   static int arm_smmu_add_device(struct device *dev)
>   {
>   	struct iommu_group *group;
> @@ -1458,6 +1476,7 @@ static struct iommu_ops arm_smmu_ops = {
>   	.unmap			= arm_smmu_unmap,
>   	.map_sg			= default_iommu_map_sg,
>   	.iova_to_phys		= arm_smmu_iova_to_phys,
> +	.of_xlate		= arm_smmu_of_xlate,
>   	.add_device		= arm_smmu_add_device,
>   	.remove_device		= arm_smmu_remove_device,
>   	.device_group		= arm_smmu_device_group,
>

[RFC PATCH 4/6] iommu/arm-smmu: Add support for IOMMU_DOMAIN_DMA in SMMUv1/SMMUv2 driver

[Mark Rutland]

On Thu, Jan 28, 2016 at 05:28:30PM +0000, Robin Murphy wrote:
> On 27/01/16 05:21, Anup Patel wrote:
> >To allow use of large memory (> 4Gb) with 32bit devices we need to use
> >some kind of iommu for such 32bit devices.
> >
> >This patch extends SMMUv1/SMMUv2 driver to support DMA domains which
> >in-turn will allows us to use iommu based DMA mappings for 32bit devices.
> >
> >Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> >Reviewed-by: Ray Jui <rjui@broadcom.com>
> >Reviewed-by: Scott Branden <sbranden@broadcom.com>
> >---
> >  drivers/iommu/arm-smmu.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >index 9bdf6b2..43424fe 100644
> >--- a/drivers/iommu/arm-smmu.c
> >+++ b/drivers/iommu/arm-smmu.c
> >@@ -29,6 +29,7 @@
> >  #define pr_fmt(fmt) "arm-smmu: " fmt
> >
> >  #include <linux/delay.h>
> >+#include <linux/dma-iommu.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/err.h>
> >  #include <linux/interrupt.h>
> >@@ -967,7 +968,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> >  {
> >  	struct arm_smmu_domain *smmu_domain;
> >
> >-	if (type != IOMMU_DOMAIN_UNMANAGED)
> >+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
> >  		return NULL;
> >  	/*
> >  	 * Allocate the domain and initialise some of its data structures.
> >@@ -978,6 +979,12 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> >  	if (!smmu_domain)
> >  		return NULL;
> >
> >+	if (type == IOMMU_DOMAIN_DMA &&
> >+	    iommu_get_dma_cookie(&smmu_domain->domain)) {
> >+		kfree(smmu_domain);
> >+		return NULL;
> >+	}
> >+
> >  	mutex_init(&smmu_domain->init_mutex);
> >  	spin_lock_init(&smmu_domain->pgtbl_lock);
> >
> >@@ -992,6 +999,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
> >  	 * Free the domain resources. We assume that all devices have
> >  	 * already been detached.
> >  	 */
> >+	iommu_put_dma_cookie(domain);
> >  	arm_smmu_destroy_domain_context(domain);
> >  	kfree(smmu_domain);
> >  }
> >@@ -1361,6 +1369,16 @@ static int arm_smmu_init_platform_device(struct device *dev,
> >  	return 0;
> >  }
> >
> >+int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> >+{
> >+	/*
> >+	 * Nothing to do here because SMMU is already aware of all
> >+	 * MMU masters and their stream IDs using mmu-master attibute
> >+	 * SMMU DT node.
> >+	 */
> 
> ...but on the same hand this will also never get called if there's
> no "iommus" property on the master. Maintaining support for existing
> users of the "mmu-masters" binding is one thing (namely the thing
> that's been slowing down my efforts to clean up the really hacky
> generic binding support I did all the DMA stuff with), but having
> _both_ bindings in a single DT is something I don't think anybody
> wants to see

Indeed. NAK to the mixed case.

Mark.

[RFC PATCH 4/6] iommu/arm-smmu: Add support for IOMMU_DOMAIN_DMA in SMMUv1/SMMUv2 driver

[Anup Patel]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy at arm.com]
> Sent: 28 January 2016 22:59
> To: Anup Patel; Catalin Marinas; Joerg Roedel; Will Deacon; Sricharan R; Linux
> IOMMU; Linux ARM Kernel
> Cc: Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Device
> Tree; Ray Jui; Scott Branden; Vikram Prakash; Linux Kernel; bcm-kernel-
> feedback-list
> Subject: Re: [RFC PATCH 4/6] iommu/arm-smmu: Add support for
> IOMMU_DOMAIN_DMA in SMMUv1/SMMUv2 driver
> 
> On 27/01/16 05:21, Anup Patel wrote:
> > To allow use of large memory (> 4Gb) with 32bit devices we need to use
> > some kind of iommu for such 32bit devices.
> >
> > This patch extends SMMUv1/SMMUv2 driver to support DMA domains which
> > in-turn will allows us to use iommu based DMA mappings for 32bit devices.
> >
> > Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> > Reviewed-by: Ray Jui <rjui@broadcom.com>
> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > ---
> >   drivers/iommu/arm-smmu.c | 21 ++++++++++++++++++++-
> >   1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> > 9bdf6b2..43424fe 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -29,6 +29,7 @@
> >   #define pr_fmt(fmt) "arm-smmu: " fmt
> >
> >   #include <linux/delay.h>
> > +#include <linux/dma-iommu.h>
> >   #include <linux/dma-mapping.h>
> >   #include <linux/err.h>
> >   #include <linux/interrupt.h>
> > @@ -967,7 +968,7 @@ static struct iommu_domain
> *arm_smmu_domain_alloc(unsigned type)
> >   {
> >   	struct arm_smmu_domain *smmu_domain;
> >
> > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > +	if (type != IOMMU_DOMAIN_UNMANAGED && type !=
> IOMMU_DOMAIN_DMA)
> >   		return NULL;
> >   	/*
> >   	 * Allocate the domain and initialise some of its data structures.
> > @@ -978,6 +979,12 @@ static struct iommu_domain
> *arm_smmu_domain_alloc(unsigned type)
> >   	if (!smmu_domain)
> >   		return NULL;
> >
> > +	if (type == IOMMU_DOMAIN_DMA &&
> > +	    iommu_get_dma_cookie(&smmu_domain->domain)) {
> > +		kfree(smmu_domain);
> > +		return NULL;
> > +	}
> > +
> >   	mutex_init(&smmu_domain->init_mutex);
> >   	spin_lock_init(&smmu_domain->pgtbl_lock);
> >
> > @@ -992,6 +999,7 @@ static void arm_smmu_domain_free(struct
> iommu_domain *domain)
> >   	 * Free the domain resources. We assume that all devices have
> >   	 * already been detached.
> >   	 */
> > +	iommu_put_dma_cookie(domain);
> >   	arm_smmu_destroy_domain_context(domain);
> >   	kfree(smmu_domain);
> >   }
> > @@ -1361,6 +1369,16 @@ static int arm_smmu_init_platform_device(struct
> device *dev,
> >   	return 0;
> >   }
> >
> > +int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args
> > +*args) {
> > +	/*
> > +	 * Nothing to do here because SMMU is already aware of all
> > +	 * MMU masters and their stream IDs using mmu-master attibute
> > +	 * SMMU DT node.
> > +	 */
> 
> ...but on the same hand this will also never get called if there's no "iommus"
> property on the master. Maintaining support for existing users of the "mmu-
> masters" binding is one thing (namely the thing that's been slowing down my
> efforts to clean up the really hacky generic binding support I did all the DMA
> stuff with), but having _both_ bindings in a single DT is something I don't think
> anybody wants to see - is that how you've tested this?

We want to use iommu aware DMA mapping APIs for certain 32bit devices
in our SoC without changing device driver of these 32bit devices
(such as PL330). Currently, to this we have to specifiy "iommus" attribute in
32bit device DT node. If we specify  "iommus" attribute in device DT node then
of_iommu_configure() expects "of_xlate" callback and it will fail if this
callback is not available.

If there is a way use DMA mappings APIs for 32bit devices without having
"of_xlate" in SMMU driver then I would prefer that approach. Suggestions??

> 
> Robin.
> 
> > +	return 0;
> > +}
> > +
> >   static int arm_smmu_add_device(struct device *dev)
> >   {
> >   	struct iommu_group *group;
> > @@ -1458,6 +1476,7 @@ static struct iommu_ops arm_smmu_ops = {
> >   	.unmap			= arm_smmu_unmap,
> >   	.map_sg			= default_iommu_map_sg,
> >   	.iova_to_phys		= arm_smmu_iova_to_phys,
> > +	.of_xlate		= arm_smmu_of_xlate,
> >   	.add_device		= arm_smmu_add_device,
> >   	.remove_device		= arm_smmu_remove_device,
> >   	.device_group		= arm_smmu_device_group,
> >
> 

Regards,
Anup

[RFC PATCH 5/6] iommu/arm-smmu: Option to treat instruction fetch as data read for SMMUv2

[Anup Patel]

Currently, the SMMU driver by default provides unprivilege read-write
permission in page table entries of stage1 page table. For SMMUv2 with
aarch64 long descriptor format, a privilege instruction fetch will
generate context fault. To allow privilege instruction fetch from a
MMU master we need to treat instruction fetch as data read.

This patch adds an optional DT attribute 'smmu-inst-as-data' to treat
privilege/unprivilege instruction fetch as data read for SMMUv2.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 drivers/iommu/arm-smmu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 43424fe..a14850b 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -169,6 +169,9 @@
 #define S2CR_TYPE_TRANS			(0 << S2CR_TYPE_SHIFT)
 #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
 #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
+#define S2CR_INSTCFG_SHIFT		26
+#define S2CR_INSTCFG_MASK		0x3
+#define S2CR_INSTCFG_DATA		(0x2 << S2CR_INSTCFG_SHIFT)
 
 /* Context bank attribute registers */
 #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
@@ -305,6 +308,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
+#define ARM_SMMU_OPT_INST_AS_DATA      (1 << 1)
 	u32				options;
 	enum arm_smmu_arch_version	version;
 
@@ -366,6 +370,7 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
+	{ ARM_SMMU_OPT_INST_AS_DATA, "smmu-inst-as-data" },
 	{ 0, NULL},
 };
 
@@ -1097,6 +1102,9 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
 		s2cr = S2CR_TYPE_TRANS |
 		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
+		if ((smmu->version == ARM_SMMU_V2) &&
+		    (smmu->options & ARM_SMMU_OPT_INST_AS_DATA))
+			s2cr |= S2CR_INSTCFG_DATA;
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
 
-- 
1.9.1

[RFC PATCH 5/6] iommu/arm-smmu: Option to treat instruction fetch as data read for SMMUv2

[Robin Murphy]

On 27/01/16 05:21, Anup Patel wrote:
> Currently, the SMMU driver by default provides unprivilege read-write
> permission in page table entries of stage1 page table. For SMMUv2 with
> aarch64 long descriptor format, a privilege instruction fetch will
> generate context fault. To allow privilege instruction fetch from a
> MMU master we need to treat instruction fetch as data read.
>
> This patch adds an optional DT attribute 'smmu-inst-as-data' to treat
> privilege/unprivilege instruction fetch as data read for SMMUv2.

What's the use-case for retaining the privilege attribute over the 
instruction attribute? The pagetable code still maps everything with 
unprivileged permissions, and it isn't going to be relevant for the vast 
majority of devices. Conversely, the instruction/data distinction is 
likely to be useful in a lot more cases - there's a veritable class of 
exploits around tricking a DSP/GPU/VPU/whatever into executing malicious 
firmware/shaders/etc. - and the IOMMU API does already have support for 
that which this change subtly undermines: if you override INSTCFG this 
way, you also need to stop the SMMU from claiming to support 
IOMMU_NOEXEC, because it no longer will.

> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>   drivers/iommu/arm-smmu.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 43424fe..a14850b 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -169,6 +169,9 @@
>   #define S2CR_TYPE_TRANS			(0 << S2CR_TYPE_SHIFT)
>   #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
>   #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
> +#define S2CR_INSTCFG_SHIFT		26
> +#define S2CR_INSTCFG_MASK		0x3
> +#define S2CR_INSTCFG_DATA		(0x2 << S2CR_INSTCFG_SHIFT)
>
>   /* Context bank attribute registers */
>   #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
> @@ -305,6 +308,7 @@ struct arm_smmu_device {
>   	u32				features;
>
>   #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
> +#define ARM_SMMU_OPT_INST_AS_DATA      (1 << 1)
>   	u32				options;
>   	enum arm_smmu_arch_version	version;
>
> @@ -366,6 +370,7 @@ struct arm_smmu_option_prop {
>
>   static struct arm_smmu_option_prop arm_smmu_options[] = {
>   	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
> +	{ ARM_SMMU_OPT_INST_AS_DATA, "smmu-inst-as-data" },
>   	{ 0, NULL},
>   };
>
> @@ -1097,6 +1102,9 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>   		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
>   		s2cr = S2CR_TYPE_TRANS |
>   		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
> +		if ((smmu->version == ARM_SMMU_V2) &&

I would say this is too broad a condition - there's still no need to do 
this for AArch32 contexts at stage 1, and there's no need for it at 
stage 2 at all.

Robin.

> +		    (smmu->options & ARM_SMMU_OPT_INST_AS_DATA))
> +			s2cr |= S2CR_INSTCFG_DATA;
>   		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
>   	}
>
>

[RFC PATCH 5/6] iommu/arm-smmu: Option to treat instruction fetch as data read for SMMUv2

[Anup Patel]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy at arm.com]
> Sent: 28 January 2016 22:41
> To: Anup Patel; Catalin Marinas; Joerg Roedel; Will Deacon; Robin Murphy;
> Sricharan R; Linux IOMMU; Linux ARM Kernel
> Cc: Mark Rutland; Device Tree; Scott Branden; Pawel Moll; Ian Campbell; Ray
> Jui; Linux Kernel; Vikram Prakash; Rob Herring; bcm-kernel-feedback-list; Kumar
> Gala
> Subject: Re: [RFC PATCH 5/6] iommu/arm-smmu: Option to treat instruction
> fetch as data read for SMMUv2
> 
> On 27/01/16 05:21, Anup Patel wrote:
> > Currently, the SMMU driver by default provides unprivilege read-write
> > permission in page table entries of stage1 page table. For SMMUv2 with
> > aarch64 long descriptor format, a privilege instruction fetch will
> > generate context fault. To allow privilege instruction fetch from a
> > MMU master we need to treat instruction fetch as data read.
> >
> > This patch adds an optional DT attribute 'smmu-inst-as-data' to treat
> > privilege/unprivilege instruction fetch as data read for SMMUv2.
> 
> What's the use-case for retaining the privilege attribute over the instruction
> attribute? The pagetable code still maps everything with unprivileged
> permissions, and it isn't going to be relevant for the vast majority of devices.
> Conversely, the instruction/data distinction is likely to be useful in a lot more
> cases - there's a veritable class of exploits around tricking a
> DSP/GPU/VPU/whatever into executing malicious firmware/shaders/etc. - and
> the IOMMU API does already have support for that which this change subtly
> undermines: if you override INSTCFG this way, you also need to stop the SMMU
> from claiming to support IOMMU_NOEXEC, because it no longer will.

Currently, there is no provision in IOMMU framework to specify privilege level
of a mapping. I thought in future someone might certainly add such a thing
because theoretically we can have accesses from a microcontroller or
coprocessor going through SMMU and microcontroller or coprocessor can
have two privilege levels.

Now, the list of all possible access types would be:
1. Privileged read
2. Privileged write
3. Privileged instruction fetch
4. Unprivileged read
5. Unprivileged write
6. Unprivileged instruction fetch

If we treat instruction fetch as data read then above would reduce to:
1. Privileged read
2. Privileged write
3. Unprivileged read
4. Unprivileged write

If we treat all privileged access as unprivileged then above would reduce to:
1. Unprivileged read
2. Unprivileged write
3. Unprivileged instruction fetch

The possible access types were reducing more if treated privileged accesses
as unprivileged accesses. This motivated me to prefer "treat instruction
fetch as data read" over "treat privileged access as unprivileged".

I am fine with both approaches. If we envision that IOMMU framework
no use for privileged accesses then we should go with "treat privileged
access as unprivileged" approach. In fact, I was already planning to base
this patchset upon your patchset and remove duplicate changes from
my patchset.

Regards,
Anup

[RFC PATCH 6/6] iommu/arm-smmu: Update bindings document for smmu-inst-as-data DT option

[Anup Patel]

This patch adds info about 'smmu-inst-as-data' DT option in ARM
SMMUv1/SMMUv2 driver bindings document.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index 7180745..4c4d03e 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -49,6 +49,14 @@ conditions.
                   NOTE: this only applies to the SMMU itself, not
                   masters connected upstream of the SMMU.
 
+- smmu-inst-as-data : Treat privilege/unprivilege instruction fetch as
+                  data read for SMMUv2. The SMMU driver by default provides
+                  unprivilege read-write permission in page table entries.
+                  For SMMUv2, privilege instruction fetch from MMU masters
+                  will cause a context fault for unprivilege read-write
+                  pages. To allow both privilege and unprivilege instruction
+                  fetch, we have to forcefully treat it as data read.
+
 - calxeda,smmu-secure-config-access : Enable proper handling of buggy
                   implementations that always use secure access to
                   SMMU configuration registers. In this case non-secure
-- 
1.9.1

[RFC PATCH 6/6] iommu/arm-smmu: Update bindings document for smmu-inst-as-data DT option

[Mark Rutland]

On Wed, Jan 27, 2016 at 10:51:19AM +0530, Anup Patel wrote:
> This patch adds info about 'smmu-inst-as-data' DT option in ARM
> SMMUv1/SMMUv2 driver bindings document.
> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
>  Documentation/devicetree/bindings/iommu/arm,smmu.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 7180745..4c4d03e 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -49,6 +49,14 @@ conditions.
>                    NOTE: this only applies to the SMMU itself, not
>                    masters connected upstream of the SMMU.
>  
> +- smmu-inst-as-data : Treat privilege/unprivilege instruction fetch as
> +                  data read for SMMUv2. The SMMU driver by default provides
> +                  unprivilege read-write permission in page table entries.
> +                  For SMMUv2, privilege instruction fetch from MMU masters
> +                  will cause a context fault for unprivilege read-write
> +                  pages. To allow both privilege and unprivilege instruction
> +                  fetch, we have to forcefully treat it as data read.

What is this needed for? Which masters do instruction fetches through
the SMMU, and when?

Surely this should only need to aplly to a subset of transactions?

Mark.

> +
>  - calxeda,smmu-secure-config-access : Enable proper handling of buggy
>                    implementations that always use secure access to
>                    SMMU configuration registers. In this case non-secure
> -- 
> 1.9.1
> 

[RFC PATCH 6/6] iommu/arm-smmu: Update bindings document for smmu-inst-as-data DT option

[Anup Patel]

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland at arm.com]
> Sent: 27 January 2016 17:59
> To: Anup Patel
> Cc: Catalin Marinas; Joerg Roedel; Will Deacon; Robin Murphy; Sricharan R;
> Linux IOMMU; Linux ARM Kernel; Rob Herring; Pawel Moll; Ian Campbell; Kumar
> Gala; Device Tree; Ray Jui; Scott Branden; Vikram Prakash; Linux Kernel; bcm-
> kernel-feedback-list
> Subject: Re: [RFC PATCH 6/6] iommu/arm-smmu: Update bindings document for
> smmu-inst-as-data DT option
> 
> On Wed, Jan 27, 2016 at 10:51:19AM +0530, Anup Patel wrote:
> > This patch adds info about 'smmu-inst-as-data' DT option in ARM
> > SMMUv1/SMMUv2 driver bindings document.
> >
> > Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> > Reviewed-by: Ray Jui <rjui@broadcom.com>
> > Reviewed-by: Vikram Prakash <vikramp@broadcom.com>
> > Reviewed-by: Scott Branden <sbranden@broadcom.com>
> > ---
> >  Documentation/devicetree/bindings/iommu/arm,smmu.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > index 7180745..4c4d03e 100644
> > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> > @@ -49,6 +49,14 @@ conditions.
> >                    NOTE: this only applies to the SMMU itself, not
> >                    masters connected upstream of the SMMU.
> >
> > +- smmu-inst-as-data : Treat privilege/unprivilege instruction fetch as
> > +                  data read for SMMUv2. The SMMU driver by default provides
> > +                  unprivilege read-write permission in page table entries.
> > +                  For SMMUv2, privilege instruction fetch from MMU masters
> > +                  will cause a context fault for unprivilege read-write
> > +                  pages. To allow both privilege and unprivilege instruction
> > +                  fetch, we have to forcefully treat it as data read.
> 
> What is this needed for? Which masters do instruction fetches through the
> SMMU, and when?
> 
> Surely this should only need to aplly to a subset of transactions?

The boot_manager of PL330 does privileged instruction fetches which
cause privilege-level context fault in SMMU because current SMMU
driver provides unprivileged read-write permissions.

Regards,
Anup