[PATCH v4 0/11] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000

[PATCH v4 0/11] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000

[Andreas Herrmann]

Hi,

Here is v4 of arm-smmu changes to support SMMUs on Calxeda ECX-2000.

Patches are based on v3.13-rc8.

Major change to previous version is new code that allows to extend an
existing IOMMU address mapping. (last two patches)

Changelog:
v4:
 - added support to extend the size of an IOMMU address mapping
 - check for duplicate stream IDs
 - Increase MAX_PHANDLE_ARGS
 - misc minor changes of patch
   "iommu/arm-smmu: Introduce automatic stream-id-masking"
v3:
  http://marc.info/?l=linux-arm-kernel&m=138212725606348
v2:
  http://marc.info/?l=linux-arm-kernel&m=138135834704855
v1:
   http://marc.info/?l=linux-arm-kernel&m=138122450023564


Regards,
Andreas

[PATCH 01/11] iommu/arm-smmu: Introduce driver option handling

[Andreas Herrmann]

Introduce handling of driver options. Options are set based on DT
information when probing an SMMU device. The first option introduced
is "arm,smmu-isolate-devices". (It will be used in the bus notifier
block.)

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index e46a887..0b97d03 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -47,6 +47,9 @@
 
 #include <asm/pgalloc.h>
 
+/* Driver options */
+#define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
+
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		8
 
@@ -348,6 +351,7 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
 #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
 	u32				features;
+	u32				options;
 	int				version;
 
 	u32				num_context_banks;
@@ -398,6 +402,29 @@ struct arm_smmu_domain {
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
 static LIST_HEAD(arm_smmu_devices);
 
+struct arm_smmu_option_prop {
+	u32 opt;
+	const char *prop;
+};
+
+static struct arm_smmu_option_prop arm_smmu_options [] = {
+	{ ARM_SMMU_OPT_ISOLATE_DEVICES, "arm,smmu-isolate-devices" },
+	{ 0, NULL},
+};
+
+static void check_driver_options(struct arm_smmu_device *smmu)
+{
+	int i = 0;
+	do {
+		if (of_property_read_bool(smmu->dev->of_node,
+						arm_smmu_options[i].prop)) {
+			smmu->options |= arm_smmu_options[i].opt;
+			dev_dbg(smmu->dev, "option %s\n",
+				arm_smmu_options[i].prop);
+		}
+	} while (arm_smmu_options[++i].opt);
+}
+
 static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
 						struct device_node *dev_node)
 {
@@ -1783,6 +1810,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	smmu->dev = dev;
 
+	check_driver_options(smmu);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	smmu->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(smmu->base))
-- 
1.7.9.5

[PATCH 01/11] iommu/arm-smmu: Introduce driver option handling

[Will Deacon]

Hi Andreas,

Couple of *tiny* comments.

On Thu, Jan 16, 2014 at 12:44:13PM +0000, Andreas Herrmann wrote:
> Introduce handling of driver options. Options are set based on DT
> information when probing an SMMU device. The first option introduced
> is "arm,smmu-isolate-devices". (It will be used in the bus notifier
> block.)
> 
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index e46a887..0b97d03 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -47,6 +47,9 @@
>  
>  #include <asm/pgalloc.h>
>  
> +/* Driver options */
> +#define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)

You can move this...

>  /* Maximum number of stream IDs assigned to a single device */
>  #define MAX_MASTER_STREAMIDS		8
>  
> @@ -348,6 +351,7 @@ struct arm_smmu_device {
>  #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
>  #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
>  	u32				features;

... to here. Like the *_FEAT_* defines above.

> +	u32				options;
>  	int				version;
>  
>  	u32				num_context_banks;
> @@ -398,6 +402,29 @@ struct arm_smmu_domain {
>  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
>  static LIST_HEAD(arm_smmu_devices);
>  
> +struct arm_smmu_option_prop {
> +	u32 opt;
> +	const char *prop;
> +};
> +
> +static struct arm_smmu_option_prop arm_smmu_options [] = {
> +	{ ARM_SMMU_OPT_ISOLATE_DEVICES, "arm,smmu-isolate-devices" },
> +	{ 0, NULL},
> +};
> +
> +static void check_driver_options(struct arm_smmu_device *smmu)
> +{
> +	int i = 0;
> +	do {
> +		if (of_property_read_bool(smmu->dev->of_node,
> +						arm_smmu_options[i].prop)) {
> +			smmu->options |= arm_smmu_options[i].opt;
> +			dev_dbg(smmu->dev, "option %s\n",
> +				arm_smmu_options[i].prop);
> +		}
> +	} while (arm_smmu_options[++i].opt);
> +}
> +
>  static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
>  						struct device_node *dev_node)
>  {
> @@ -1783,6 +1810,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  	}
>  	smmu->dev = dev;
>  
> +	check_driver_options(smmu);

I think parse_driver_opts is a better name. Also, if we called this after
arm_smmu_device_cfg_probe, we could replace the dev_dbg with a dev_notice,
since the user probably wants to know which options ended up getting
enabled. Is there a reason you need to probe the option so early?

Will

[PATCH 01/11] iommu/arm-smmu: Introduce driver option handling

[Andreas Herrmann]

On Wed, Jan 22, 2014 at 11:51:43AM +0000, Will Deacon wrote:
> Hi Andreas,
> 
> Couple of *tiny* comments.
> 
> On Thu, Jan 16, 2014 at 12:44:13PM +0000, Andreas Herrmann wrote:
> > Introduce handling of driver options. Options are set based on DT
> > information when probing an SMMU device. The first option introduced
> > is "arm,smmu-isolate-devices". (It will be used in the bus notifier
> > block.)
> > 
> > Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |   29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index e46a887..0b97d03 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -47,6 +47,9 @@
> >  
> >  #include <asm/pgalloc.h>
> >  
> > +/* Driver options */
> > +#define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
> 
> You can move this...
> 
> >  /* Maximum number of stream IDs assigned to a single device */
> >  #define MAX_MASTER_STREAMIDS		8
> >  
> > @@ -348,6 +351,7 @@ struct arm_smmu_device {
> >  #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
> >  #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
> >  	u32				features;
> 
> ... to here. Like the *_FEAT_* defines above.

Ok.

> > +	u32				options;
> >  	int				version;
> >  
> >  	u32				num_context_banks;
> > @@ -398,6 +402,29 @@ struct arm_smmu_domain {
> >  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> >  static LIST_HEAD(arm_smmu_devices);
> >  
> > +struct arm_smmu_option_prop {
> > +	u32 opt;
> > +	const char *prop;
> > +};
> > +
> > +static struct arm_smmu_option_prop arm_smmu_options [] = {
> > +	{ ARM_SMMU_OPT_ISOLATE_DEVICES, "arm,smmu-isolate-devices" },
> > +	{ 0, NULL},
> > +};
> > +
> > +static void check_driver_options(struct arm_smmu_device *smmu)
> > +{
> > +	int i = 0;
> > +	do {
> > +		if (of_property_read_bool(smmu->dev->of_node,
> > +						arm_smmu_options[i].prop)) {
> > +			smmu->options |= arm_smmu_options[i].opt;
> > +			dev_dbg(smmu->dev, "option %s\n",
> > +				arm_smmu_options[i].prop);
> > +		}
> > +	} while (arm_smmu_options[++i].opt);
> > +}
> > +
> >  static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
> >  						struct device_node *dev_node)
> >  {
> > @@ -1783,6 +1810,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> >  	}
> >  	smmu->dev = dev;
> >  
> > +	check_driver_options(smmu);
> 
> I think parse_driver_opts is a better name. Also, if we called this after
> arm_smmu_device_cfg_probe, we could replace the dev_dbg with a dev_notice,
> since the user probably wants to know which options ended up getting
> enabled.

Yes, that's right.
(I was undecided on adding additional output because the properties
can be checked under /proc/device-tree on a running system.)

> Is there a reason you need to probe the option so early?

There is no real requirement. Just the general consideration to be
done with option parsing as early as possible. (And I think once I had
some preliminary code in register_smmu_master that depended on it.)

I'll move the call.


Andreas

[PATCH 02/11] iommu/arm-smmu: Introduce bus notifier block

[Andreas Herrmann]

At the moment just handle BUS_NOTIFY_BIND_DRIVER to conditionally
isolate all master devices for an SMMU.

Depending on DT information 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 "arm,smmu-isolate-devices" in a DT. If this property is set
for an SMMU node, device isolation is performed.

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 fixed as 0 and
SZ_128M at the moment. Additional patches will address this
restriction and allow automatic growth of mapping size.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 0b97d03..bc81dd0 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>
 
 /* Driver options */
 #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
@@ -1964,6 +1965,48 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int arm_smmu_device_notifier(struct notifier_block *nb,
+				unsigned long action, void *data)
+{
+	struct device *dev = data;
+	struct dma_iommu_mapping *mapping;
+	struct arm_smmu_device *smmu;
+	int ret;
+
+	switch (action) {
+	case BUS_NOTIFY_BIND_DRIVER:
+
+		arm_smmu_add_device(dev);
+		smmu = dev->archdata.iommu;
+		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
+			break;
+
+		mapping = arm_iommu_create_mapping(&platform_bus_type,
+						0, SZ_128M, 0);
+		if (IS_ERR(mapping)) {
+			ret = PTR_ERR(mapping);
+			dev_info(dev, "arm_iommu_create_mapping failed\n");
+			break;
+		}
+
+		ret = arm_iommu_attach_device(dev, mapping);
+		if (ret < 0) {
+			dev_info(dev, "arm_iommu_attach_device failed\n");
+			arm_iommu_release_mapping(mapping);
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block device_nb = {
+	.notifier_call = arm_smmu_device_notifier,
+};
+
 #ifdef CONFIG_OF
 static struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", },
@@ -2000,6 +2043,8 @@ static int __init arm_smmu_init(void)
 	if (!iommu_present(&amba_bustype))
 		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
 
+	bus_register_notifier(&platform_bus_type, &device_nb);
+
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 02/11] iommu/arm-smmu: Introduce bus notifier block

[Varun Sethi]

> -----Original Message-----
> From: iommu-bounces at lists.linux-foundation.org [mailto:iommu-
> bounces at lists.linux-foundation.org] On Behalf Of Andreas Herrmann
> Sent: Thursday, January 16, 2014 6:14 PM
> To: Will Deacon
> Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org
> Subject: [PATCH 02/11] iommu/arm-smmu: Introduce bus notifier block
> 
> At the moment just handle BUS_NOTIFY_BIND_DRIVER to conditionally isolate
> all master devices for an SMMU.
> 
> Depending on DT information 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 "arm,smmu-isolate-devices" in a DT. If this property is set for
> an SMMU node, device isolation is performed.
[Sethi Varun-B16395] What if the devices have to be assigned to different virtual machines? Would the absence of this property indicate that devices can't
Be assigned to different virtual machines i.e. devices would be in the same iommu group?

> 
> W/o device isolation the driver detects SMMUs but no translation is
> configured (transactions just bypass translation process).
> 
[Sethi Varun-B16395] Would SMR and S2CR still be allocated?

> Note that for device isolation dma_base and size are fixed as 0 and
> SZ_128M at the moment. Additional patches will address this restriction
> and allow automatic growth of mapping size.
> 
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   45
> +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> 0b97d03..bc81dd0 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>
> 
>  /* Driver options */
>  #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
> @@ -1964,6 +1965,48 @@ static int arm_smmu_device_remove(struct
> platform_device *pdev)
>  	return 0;
>  }
> 
> +static int arm_smmu_device_notifier(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +	struct dma_iommu_mapping *mapping;
> +	struct arm_smmu_device *smmu;
> +	int ret;
> +
> +	switch (action) {
> +	case BUS_NOTIFY_BIND_DRIVER:
> +
> +		arm_smmu_add_device(dev);
[Sethi Varun-B16395] This would have already happened by virtue of the add_device iommu_ops.

> +		smmu = dev->archdata.iommu;
> +		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
> +			break;
> +
> +		mapping = arm_iommu_create_mapping(&platform_bus_type,
> +						0, SZ_128M, 0);
> +		if (IS_ERR(mapping)) {
> +			ret = PTR_ERR(mapping);
> +			dev_info(dev, "arm_iommu_create_mapping failed\n");
> +			break;
> +		}
> +
> +		ret = arm_iommu_attach_device(dev, mapping);
> +		if (ret < 0) {
> +			dev_info(dev, "arm_iommu_attach_device failed\n");
> +			arm_iommu_release_mapping(mapping);
> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block device_nb = {
> +	.notifier_call = arm_smmu_device_notifier, };
> +
>  #ifdef CONFIG_OF
>  static struct of_device_id arm_smmu_of_match[] = {
>  	{ .compatible = "arm,smmu-v1", },
> @@ -2000,6 +2043,8 @@ static int __init arm_smmu_init(void)
>  	if (!iommu_present(&amba_bustype))
>  		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
> 
> +	bus_register_notifier(&platform_bus_type, &device_nb);
> +
[Sethi Varun-B16395] Notifier was already registered for the platform bus via bus_set_iommu. You can actually register a iommu group (once iommu group gets created) notifier (iommu_group_register_notifier) and listen for the IOMMU_GROUP_NOTIFY_BIND_DRIVER event.

-Varun

[PATCH 02/11] iommu/arm-smmu: Introduce bus notifier block

[Andreas Herrmann]

On Sat, Jan 18, 2014 at 03:59:53PM -0500, Varun Sethi wrote:
> > -----Original Message-----
> > From: iommu-bounces at lists.linux-foundation.org [mailto:iommu-
> > bounces at lists.linux-foundation.org] On Behalf Of Andreas Herrmann
> > Sent: Thursday, January 16, 2014 6:14 PM
> > To: Will Deacon
> > Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; linux-arm-
> > kernel at lists.infradead.org
> > Subject: [PATCH 02/11] iommu/arm-smmu: Introduce bus notifier block
> > 
> > At the moment just handle BUS_NOTIFY_BIND_DRIVER to conditionally isolate
> > all master devices for an SMMU.
> > 
> > Depending on DT information 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 "arm,smmu-isolate-devices" in a DT. If this property is set for
> > an SMMU node, device isolation is performed.

> [Sethi Varun-B16395] What if the devices have to be assigned to
> different virtual machines? Would the absence of this property
> indicate that devices can't Be assigned to different virtual
> machines i.e. devices would be in the same iommu group?

The absence of this property implies that arm-smmu driver handling of
master devices is equivalent to the current handling.
(I.e. assignment to different VMs works or doesn't work as with the current
mainline driver.)

> > W/o device isolation the driver detects SMMUs but no translation is
> > configured (transactions just bypass translation process).
> > 
> [Sethi Varun-B16395] Would SMR and S2CR still be allocated?

That's similar to the current handling of the driver. SMRs will be
marked as invalid and all S2CRs are configured as bypass.

As long as nobody creates a domain and attaches a device to it this
won't change.

> > Note that for device isolation dma_base and size are fixed as 0 and
> > SZ_128M at the moment. Additional patches will address this restriction
> > and allow automatic growth of mapping size.
> > 
> > Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |   45
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> > 0b97d03..bc81dd0 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>
> > 
> >  /* Driver options */
> >  #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
> > @@ -1964,6 +1965,48 @@ static int arm_smmu_device_remove(struct
> > platform_device *pdev)
> >  	return 0;
> >  }
> > 
> > +static int arm_smmu_device_notifier(struct notifier_block *nb,
> > +				unsigned long action, void *data)
> > +{
> > +	struct device *dev = data;
> > +	struct dma_iommu_mapping *mapping;
> > +	struct arm_smmu_device *smmu;
> > +	int ret;
> > +
> > +	switch (action) {
> > +	case BUS_NOTIFY_BIND_DRIVER:
> > +
> > +		arm_smmu_add_device(dev);
> [Sethi Varun-B16395] This would have already happened by virtue of the add_device iommu_ops.

You are right. That call is there for no good reason.

> > +		smmu = dev->archdata.iommu;
> > +		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
> > +			break;
> > +
> > +		mapping = arm_iommu_create_mapping(&platform_bus_type,
> > +						0, SZ_128M, 0);
> > +		if (IS_ERR(mapping)) {
> > +			ret = PTR_ERR(mapping);
> > +			dev_info(dev, "arm_iommu_create_mapping failed\n");
> > +			break;
> > +		}
> > +
> > +		ret = arm_iommu_attach_device(dev, mapping);
> > +		if (ret < 0) {
> > +			dev_info(dev, "arm_iommu_attach_device failed\n");
> > +			arm_iommu_release_mapping(mapping);
> > +		}
> > +
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct notifier_block device_nb = {
> > +	.notifier_call = arm_smmu_device_notifier, };
> > +
> >  #ifdef CONFIG_OF
> >  static struct of_device_id arm_smmu_of_match[] = {
> >  	{ .compatible = "arm,smmu-v1", },
> > @@ -2000,6 +2043,8 @@ static int __init arm_smmu_init(void)
> >  	if (!iommu_present(&amba_bustype))
> >  		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
> > 
> > +	bus_register_notifier(&platform_bus_type, &device_nb);
> > +

> [Sethi Varun-B16395] Notifier was already registered for the
>  platform bus via bus_set_iommu. You can actually register a iommu
>  group (once iommu group gets created) notifier
>  (iommu_group_register_notifier) and listen for the
>  IOMMU_GROUP_NOTIFY_BIND_DRIVER event.

Sounds reasonable.

So far the driver didn't care about iommu_groups. But there is already
a patch "iommu/arm-smmu: add devices attached to the SMMU to an IOMMU
group" pending in Will's iommu/devel branch that fixes this.

I'll adapt my patch to use iommu_group notifier instead of bus
notifier.

Thanks for your review!


Andreas

[PATCH v2 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Andreas Herrmann]

At the moment just handle BUS_NOTIFY_BIND_DRIVER to conditionally
isolate all master devices for an SMMU.

Depending on DT information 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 "arm,smmu-isolate-devices" in a DT. If this property is set
for an SMMU node, device isolation is performed.

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 fixed as 0 and
SZ_128M at the moment. Additional patches will address this
restriction and allow automatic growth of mapping size.

Cc: Varun Sethi <Varun.Sethi@freescale.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

Hi Will,

This new patch addresses Varun's comments:
 - use iommu_group notifier instead of bus notifier
 - remove superfluous call to arm_smmu_add_device in
   notifier function

This patch depends on commit "iommu/arm-smmu: add devices attached to
the SMMU to an IOMMU group" as found in your git tree (e.g. in branch
iommu/devel or for-joerg/arm-smmu/updates).


Andreas

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 0a5649f..edd0ffb 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>
 
 /* Driver options */
 #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
@@ -402,6 +403,13 @@ struct arm_smmu_domain {
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
 static LIST_HEAD(arm_smmu_devices);
 
+static int arm_smmu_group_notifier(struct notifier_block *nb,
+				unsigned long action, void *data);
+
+static struct notifier_block group_nb = {
+	.notifier_call = arm_smmu_group_notifier,
+};
+
 struct arm_smmu_option_prop {
 	u32 opt;
 	const char *prop;
@@ -1566,6 +1574,8 @@ static int arm_smmu_add_device(struct device *dev)
 		return PTR_ERR(group);
 	}
 
+	iommu_group_register_notifier(group, &group_nb);
+
 	ret = iommu_group_add_device(group, dev);
 	iommu_group_put(group);
 	dev->archdata.iommu = smmu;
@@ -1981,6 +1991,43 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int arm_smmu_group_notifier(struct notifier_block *nb,
+				unsigned long action, void *data)
+{
+	struct device *dev = data;
+	struct dma_iommu_mapping *mapping;
+	struct arm_smmu_device *smmu;
+	int ret;
+
+	switch (action) {
+	case BUS_NOTIFY_BIND_DRIVER:
+
+		smmu = dev->archdata.iommu;
+		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
+			break;
+
+		mapping = arm_iommu_create_mapping(&platform_bus_type,
+						0, SZ_128M, 0);
+		if (IS_ERR(mapping)) {
+			ret = PTR_ERR(mapping);
+			dev_info(dev, "arm_iommu_create_mapping failed\n");
+			break;
+		}
+
+		ret = arm_iommu_attach_device(dev, mapping);
+		if (ret < 0) {
+			dev_info(dev, "arm_iommu_attach_device failed\n");
+			arm_iommu_release_mapping(mapping);
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_OF
 static struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", },
-- 
1.7.9.5

[PATCH v2 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Andreas Herrmann]

On Mon, Jan 20, 2014 at 10:53:51PM +0100, Andreas Herrmann wrote:
> 
> At the moment just handle BUS_NOTIFY_BIND_DRIVER to conditionally
> isolate all master devices for an SMMU.

Oops, sent out too early. Please ignore this crap.
(An iommu_group notifier that is handling BUS_NOTIFY_BIND_DRIVER ...)


Andreas

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Andreas Herrmann]

At the moment just handle IOMMU_GROUP_NOTIFY_BIND_DRIVER to
conditionally isolate all master devices for an SMMU.

Depending on DT information 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 "arm,smmu-isolate-devices" in a DT. If this property is set
for an SMMU node, device isolation is performed.

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 fixed as 0 and
SZ_128M at the moment. Additional patches will address this
restriction and allow automatic growth of mapping size.

Cc: Varun Sethi <Varun.Sethi@freescale.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Hi Will,

This new patch addresses Varun's comments:
 - use iommu_group notifier instead of bus notifier
 - remove superfluous call to arm_smmu_add_device in
   notifier function

This patch depends on commit "iommu/arm-smmu: add devices attached to
the SMMU to an IOMMU group" as found in your git tree (e.g. in branch
iommu/devel or for-joerg/arm-smmu/updates).


Andreas

PS: This time with a proper adaption of the notifier function.


diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 0a5649f..da19bd6 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>
 
 /* Driver options */
 #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
@@ -1517,6 +1518,47 @@ static int arm_smmu_domain_has_cap(struct iommu_domain *domain,
 	return !!(cap & caps);
 }
 
+static int arm_smmu_group_notifier(struct notifier_block *nb,
+				unsigned long action, void *data)
+{
+	struct device *dev = data;
+	struct dma_iommu_mapping *mapping;
+	struct arm_smmu_device *smmu;
+	int ret;
+
+	switch (action) {
+	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
+
+		smmu = dev->archdata.iommu;
+		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
+			break;
+
+		mapping = arm_iommu_create_mapping(&platform_bus_type,
+						0, SZ_128M, 0);
+		if (IS_ERR(mapping)) {
+			ret = PTR_ERR(mapping);
+			dev_info(dev, "arm_iommu_create_mapping failed\n");
+			break;
+		}
+
+		ret = arm_iommu_attach_device(dev, mapping);
+		if (ret < 0) {
+			dev_info(dev, "arm_iommu_attach_device failed\n");
+			arm_iommu_release_mapping(mapping);
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block group_nb = {
+	.notifier_call = arm_smmu_group_notifier,
+};
+
 static int arm_smmu_add_device(struct device *dev)
 {
 	struct arm_smmu_device *child, *parent, *smmu;
@@ -1566,6 +1608,8 @@ static int arm_smmu_add_device(struct device *dev)
 		return PTR_ERR(group);
 	}
 
+	iommu_group_register_notifier(group, &group_nb);
+
 	ret = iommu_group_add_device(group, dev);
 	iommu_group_put(group);
 	dev->archdata.iommu = smmu;
-- 
1.7.9.5

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Varun Sethi]

> -----Original Message-----
> From: Andreas Herrmann [mailto:andreas.herrmann at calxeda.com]
> Sent: Tuesday, January 21, 2014 3:58 AM
> To: Will Deacon
> Cc: iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; Andreas Herrmann; Sethi Varun-B16395
> Subject: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier
> block
> 
> 
> At the moment just handle IOMMU_GROUP_NOTIFY_BIND_DRIVER to conditionally
> isolate all master devices for an SMMU.
> 
> Depending on DT information 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 "arm,smmu-isolate-devices" in a DT. If this property is set for
> an SMMU node, device isolation is performed.
> 
> 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 fixed as 0 and
> SZ_128M at the moment. Additional patches will address this restriction
> and allow automatic growth of mapping size.
> 
> Cc: Varun Sethi <Varun.Sethi@freescale.com>
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   44
> ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> Hi Will,
> 
> This new patch addresses Varun's comments:
>  - use iommu_group notifier instead of bus notifier
>  - remove superfluous call to arm_smmu_add_device in
>    notifier function
> 
> This patch depends on commit "iommu/arm-smmu: add devices attached to the
> SMMU to an IOMMU group" as found in your git tree (e.g. in branch
> iommu/devel or for-joerg/arm-smmu/updates).
> 
> 
> Andreas
> 
> PS: This time with a proper adaption of the notifier function.
> 
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> 0a5649f..da19bd6 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>
> 
>  /* Driver options */
>  #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
> @@ -1517,6 +1518,47 @@ static int arm_smmu_domain_has_cap(struct
> iommu_domain *domain,
>  	return !!(cap & caps);
>  }
> 
> +static int arm_smmu_group_notifier(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +	struct dma_iommu_mapping *mapping;
> +	struct arm_smmu_device *smmu;
> +	int ret;
> +
> +	switch (action) {
> +	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
> +
> +		smmu = dev->archdata.iommu;
> +		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
> +			break;
[Sethi Varun-B16395] Should this check be really done here? The "Isolate devices" property would allow us to set up iommu groups. My understanding is that if we specify the isolate devices property, then each device would have a separate iommu group otherwise all devices connected to the SMMU would share the iommu group.

With that logic, we should link the mapping to the iommu group.

-Varun

> +
> +		mapping = arm_iommu_create_mapping(&platform_bus_type,
> +						0, SZ_128M, 0);
> +		if (IS_ERR(mapping)) {
> +			ret = PTR_ERR(mapping);
> +			dev_info(dev, "arm_iommu_create_mapping failed\n");
> +			break;
> +		}
> +
> +		ret = arm_iommu_attach_device(dev, mapping);
> +		if (ret < 0) {
> +			dev_info(dev, "arm_iommu_attach_device failed\n");
> +			arm_iommu_release_mapping(mapping);
> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block group_nb = {
> +	.notifier_call = arm_smmu_group_notifier, };
> +
>  static int arm_smmu_add_device(struct device *dev)  {
>  	struct arm_smmu_device *child, *parent, *smmu; @@ -1566,6 +1608,8
> @@ static int arm_smmu_add_device(struct device *dev)
>  		return PTR_ERR(group);
>  	}
> 
> +	iommu_group_register_notifier(group, &group_nb);
> +
>  	ret = iommu_group_add_device(group, dev);
>  	iommu_group_put(group);
>  	dev->archdata.iommu = smmu;
> --
> 1.7.9.5
> 
> 

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Will Deacon]

Hi Varun, Andreas,

On Tue, Jan 21, 2014 at 05:48:02PM +0000, Varun Sethi wrote:
> > +static int arm_smmu_group_notifier(struct notifier_block *nb,
> > +				unsigned long action, void *data)
> > +{
> > +	struct device *dev = data;
> > +	struct dma_iommu_mapping *mapping;
> > +	struct arm_smmu_device *smmu;
> > +	int ret;
> > +
> > +	switch (action) {
> > +	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
> > +
> > +		smmu = dev->archdata.iommu;
> > +		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
> > +			break;
> [Sethi Varun-B16395] Should this check be really done here? The "Isolate
> devices" property would allow us to set up iommu groups. My understanding
> is that if we specify the isolate devices property, then each device would
> have a separate iommu group otherwise all devices connected to the SMMU
> would share the iommu group.

That's not what currently happens (at least, in the patch I have queued for
groups). The code queued adds each device to its own group in
arm_smmu_add_device, which I think is the right thing to do.

> With that logic, we should link the mapping to the iommu group.

Ok, so are you suggesting that we perform the isolation mapping in
arm_smmu_add_device and drop the notifier altogether?

Will

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Varun Sethi]

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Wednesday, January 22, 2014 5:56 PM
> To: Sethi Varun-B16395
> Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; Andreas Herrmann
> Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> notifier block
> 
> Hi Varun, Andreas,
> 
> On Tue, Jan 21, 2014 at 05:48:02PM +0000, Varun Sethi wrote:
> > > +static int arm_smmu_group_notifier(struct notifier_block *nb,
> > > +				unsigned long action, void *data) {
> > > +	struct device *dev = data;
> > > +	struct dma_iommu_mapping *mapping;
> > > +	struct arm_smmu_device *smmu;
> > > +	int ret;
> > > +
> > > +	switch (action) {
> > > +	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
> > > +
> > > +		smmu = dev->archdata.iommu;
> > > +		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
> > > +			break;
> > [Sethi Varun-B16395] Should this check be really done here? The
> > "Isolate devices" property would allow us to set up iommu groups. My
> > understanding is that if we specify the isolate devices property, then
> > each device would have a separate iommu group otherwise all devices
> > connected to the SMMU would share the iommu group.
> 
> That's not what currently happens (at least, in the patch I have queued
> for groups). The code queued adds each device to its own group in
> arm_smmu_add_device, which I think is the right thing to do.
> 
> > With that logic, we should link the mapping to the iommu group.
> 
> Ok, so are you suggesting that we perform the isolation mapping in
> arm_smmu_add_device and drop the notifier altogether?
I think that should be fine, until we want to delay mapping creation till driver bind time. But the "isolate device" property should dictate iommu group creation.

-Varun

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Will Deacon]

On Wed, Jan 22, 2014 at 01:14:13PM +0000, Varun Sethi wrote:
> > On Tue, Jan 21, 2014 at 05:48:02PM +0000, Varun Sethi wrote:
> > > > +static int arm_smmu_group_notifier(struct notifier_block *nb,
> > > > +				unsigned long action, void *data) {
> > > > +	struct device *dev = data;
> > > > +	struct dma_iommu_mapping *mapping;
> > > > +	struct arm_smmu_device *smmu;
> > > > +	int ret;
> > > > +
> > > > +	switch (action) {
> > > > +	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
> > > > +
> > > > +		smmu = dev->archdata.iommu;
> > > > +		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
> > > > +			break;
> > > [Sethi Varun-B16395] Should this check be really done here? The
> > > "Isolate devices" property would allow us to set up iommu groups. My
> > > understanding is that if we specify the isolate devices property, then
> > > each device would have a separate iommu group otherwise all devices
> > > connected to the SMMU would share the iommu group.
> > 
> > That's not what currently happens (at least, in the patch I have queued
> > for groups). The code queued adds each device to its own group in
> > arm_smmu_add_device, which I think is the right thing to do.
> > 
> > > With that logic, we should link the mapping to the iommu group.
> > 
> > Ok, so are you suggesting that we perform the isolation mapping in
> > arm_smmu_add_device and drop the notifier altogether?
> I think that should be fine, until we want to delay mapping creation till
> driver bind time.

Is there a hard dependency on that?

> But the "isolate device" property should dictate iommu group creation.

The reason we added automatic group creation (per-device) is for VFIO, which
expects all devices to be in a group regardless of the device isolation
configuration.

Will

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Varun Sethi]

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Wednesday, January 22, 2014 7:10 PM
> To: Sethi Varun-B16395
> Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; Andreas Herrmann
> Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> notifier block
> 
> On Wed, Jan 22, 2014 at 01:14:13PM +0000, Varun Sethi wrote:
> > > On Tue, Jan 21, 2014 at 05:48:02PM +0000, Varun Sethi wrote:
> > > > > +static int arm_smmu_group_notifier(struct notifier_block *nb,
> > > > > +				unsigned long action, void *data) {
> > > > > +	struct device *dev = data;
> > > > > +	struct dma_iommu_mapping *mapping;
> > > > > +	struct arm_smmu_device *smmu;
> > > > > +	int ret;
> > > > > +
> > > > > +	switch (action) {
> > > > > +	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
> > > > > +
> > > > > +		smmu = dev->archdata.iommu;
> > > > > +		if (!smmu || !(smmu->options &
> ARM_SMMU_OPT_ISOLATE_DEVICES))
> > > > > +			break;
> > > > [Sethi Varun-B16395] Should this check be really done here? The
> > > > "Isolate devices" property would allow us to set up iommu groups.
> > > > My understanding is that if we specify the isolate devices
> > > > property, then each device would have a separate iommu group
> > > > otherwise all devices connected to the SMMU would share the iommu
> group.
> > >
> > > That's not what currently happens (at least, in the patch I have
> > > queued for groups). The code queued adds each device to its own
> > > group in arm_smmu_add_device, which I think is the right thing to do.
> > >
> > > > With that logic, we should link the mapping to the iommu group.
> > >
> > > Ok, so are you suggesting that we perform the isolation mapping in
> > > arm_smmu_add_device and drop the notifier altogether?
> > I think that should be fine, until we want to delay mapping creation
> > till driver bind time.
> 
> Is there a hard dependency on that?
> 
Not sure, may be Andreas can answer that.

> > But the "isolate device" property should dictate iommu group creation.
> 
> The reason we added automatic group creation (per-device) is for VFIO,
> which expects all devices to be in a group regardless of the device
> isolation configuration.
> 
IIUC, with the "isolate devices" property we ensure that there would be independent SMR and S2CR per device. Is that correct?

-Varun

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Will Deacon]

On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > Ok, so are you suggesting that we perform the isolation mapping in
> > > > arm_smmu_add_device and drop the notifier altogether?
> > > I think that should be fine, until we want to delay mapping creation
> > > till driver bind time.
> > 
> > Is there a hard dependency on that?
> > 
> Not sure, may be Andreas can answer that.

Ok. Andreas? I would have thought doing this *earlier* shouldn't be a
problem (the DMA ops must be swizzled before the driver is probed).

> > > But the "isolate device" property should dictate iommu group creation.
> > 
> > The reason we added automatic group creation (per-device) is for VFIO,
> > which expects all devices to be in a group regardless of the device
> > isolation configuration.
> > 
> IIUC, with the "isolate devices" property we ensure that there would be
> independent SMR and S2CR per device. Is that correct?

Yes, as part of giving them independent sets of page tables. Initially these
tables don't have any valid mappings, but the dma-mapping API will populate
them in response to dma_map_*/dma_alloc/etc.

Not sure what this has to do with us putting devices into their own
groups...

Will

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Varun Sethi]

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Wednesday, January 22, 2014 9:04 PM
> To: Sethi Varun-B16395
> Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; linux-arm-
> kernel at lists.infradead.org; Andreas Herrmann
> Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> notifier block
> 
> On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > > Ok, so are you suggesting that we perform the isolation mapping
> > > > > in arm_smmu_add_device and drop the notifier altogether?
> > > > I think that should be fine, until we want to delay mapping
> > > > creation till driver bind time.
> > >
> > > Is there a hard dependency on that?
> > >
> > Not sure, may be Andreas can answer that.
> 
> Ok. Andreas? I would have thought doing this *earlier* shouldn't be a
> problem (the DMA ops must be swizzled before the driver is probed).
> 
> > > > But the "isolate device" property should dictate iommu group
> creation.
> > >
> > > The reason we added automatic group creation (per-device) is for
> > > VFIO, which expects all devices to be in a group regardless of the
> > > device isolation configuration.
> > >
> > IIUC, with the "isolate devices" property we ensure that there would
> > be independent SMR and S2CR per device. Is that correct?
> 
> Yes, as part of giving them independent sets of page tables. Initially
> these tables don't have any valid mappings, but the dma-mapping API will
> populate them in response to dma_map_*/dma_alloc/etc.
> 
> Not sure what this has to do with us putting devices into their own
> groups...
[Sethi Varun-B16395] Devices in an iommu group would share the dma mapping, so shouldn't they be sharing the SMR and context registers? In case of the "isolate devices"  property, each device would have its own SMR and context registers, thus allowing the devices to have independent mappings and allowing them to fall in separate iommu groups.

-Varun

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Andreas Herrmann]

On Wed, Jan 22, 2014 at 07:07:40PM +0000, Varun Sethi wrote:
> > -----Original Message-----
> > From: Will Deacon [mailto:will.deacon at arm.com]
> > Sent: Wednesday, January 22, 2014 9:04 PM
> > To: Sethi Varun-B16395
> > Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; linux-arm-
> > kernel at lists.infradead.org; Andreas Herrmann
> > Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> > notifier block
> > 
> > On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > > > Ok, so are you suggesting that we perform the isolation mapping
> > > > > > in arm_smmu_add_device and drop the notifier altogether?
> > > > > I think that should be fine, until we want to delay mapping
> > > > > creation till driver bind time.
> > > >
> > > > Is there a hard dependency on that?
> > > >
> > > Not sure, may be Andreas can answer that.
> > 
> > Ok. Andreas? I would have thought doing this *earlier* shouldn't be a
> > problem (the DMA ops must be swizzled before the driver is probed).
> > 
> > > > > But the "isolate device" property should dictate iommu group
> > creation.
> > > >
> > > > The reason we added automatic group creation (per-device) is for
> > > > VFIO, which expects all devices to be in a group regardless of the
> > > > device isolation configuration.
> > > >
> > > IIUC, with the "isolate devices" property we ensure that there would
> > > be independent SMR and S2CR per device. Is that correct?
> > 
> > Yes, as part of giving them independent sets of page tables. Initially
> > these tables don't have any valid mappings, but the dma-mapping API will
> > populate them in response to dma_map_*/dma_alloc/etc.
> > 
> > Not sure what this has to do with us putting devices into their own
> > groups...

> [Sethi Varun-B16395] Devices in an iommu group would share the dma
> mapping, so shouldn't they be sharing the SMR and context registers?

I aggree with the context but SMRs won't be shared. I think you just
can say that a certain subset of the available SMRs will be used to
map all devices in an iommu group to the same context. Depending on
what streamIDs each device has you might have to use separate SMRs for
each device to map it to the same context.

> In case of the "isolate devices" property, each device would have
> its own SMR and context registers, thus allowing the devices to have
> independent mappings and allowing them to fall in separate iommu
> groups.

I aggree with Varun's view here. (Now that I take iommu groups into
consideration.)

But my goal with the "isolate devices" thing was twofold:

1. actual make use of SMMU for address translation for all master
  devices (instead of just bypassing the SMMU)

plus

2. put each master into it's own domain to isolate it

The latter matches usage of separate iommu groups for devices. If we
now use the isolate devices property to just control whether master
devices fall into the same or separate iommu groups it seems to me we
would need to have another mechanism that triggers the creation of a
mapping for a group.

What do you think?


Andreas

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Varun Sethi]

> -----Original Message-----
> From: iommu-bounces at lists.linux-foundation.org [mailto:iommu-
> bounces at lists.linux-foundation.org] On Behalf Of Andreas Herrmann
> Sent: Friday, January 24, 2014 1:27 AM
> To: Sethi Varun-B16395
> Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; Will Deacon;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> notifier block
> 
> On Wed, Jan 22, 2014 at 07:07:40PM +0000, Varun Sethi wrote:
> > > -----Original Message-----
> > > From: Will Deacon [mailto:will.deacon at arm.com]
> > > Sent: Wednesday, January 22, 2014 9:04 PM
> > > To: Sethi Varun-B16395
> > > Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; linux-arm-
> > > kernel at lists.infradead.org; Andreas Herrmann
> > > Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> > > notifier block
> > >
> > > On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > > > > Ok, so are you suggesting that we perform the isolation
> > > > > > > mapping in arm_smmu_add_device and drop the notifier
> altogether?
> > > > > > I think that should be fine, until we want to delay mapping
> > > > > > creation till driver bind time.
> > > > >
> > > > > Is there a hard dependency on that?
> > > > >
> > > > Not sure, may be Andreas can answer that.
> > >
> > > Ok. Andreas? I would have thought doing this *earlier* shouldn't be
> > > a problem (the DMA ops must be swizzled before the driver is probed).
> > >
> > > > > > But the "isolate device" property should dictate iommu group
> > > creation.
> > > > >
> > > > > The reason we added automatic group creation (per-device) is for
> > > > > VFIO, which expects all devices to be in a group regardless of
> > > > > the device isolation configuration.
> > > > >
> > > > IIUC, with the "isolate devices" property we ensure that there
> > > > would be independent SMR and S2CR per device. Is that correct?
> > >
> > > Yes, as part of giving them independent sets of page tables.
> > > Initially these tables don't have any valid mappings, but the
> > > dma-mapping API will populate them in response to
> dma_map_*/dma_alloc/etc.
> > >
> > > Not sure what this has to do with us putting devices into their own
> > > groups...
> 
> > [Sethi Varun-B16395] Devices in an iommu group would share the dma
> > mapping, so shouldn't they be sharing the SMR and context registers?
> 
> I aggree with the context but SMRs won't be shared. I think you just can
> say that a certain subset of the available SMRs will be used to map all
> devices in an iommu group to the same context. Depending on what
> streamIDs each device has you might have to use separate SMRs for each
> device to map it to the same context.
[Sethi Varun-B16395] IIUC the SMRs would unique per device, but there is a possibility where the number of context registers are restricted?
In that case IOMMU groups should correspond to unique stream IDs (and corresponding SMRs).

> 
> > In case of the "isolate devices" property, each device would have its
> > own SMR and context registers, thus allowing the devices to have
> > independent mappings and allowing them to fall in separate iommu
> > groups.
> 
> I aggree with Varun's view here. (Now that I take iommu groups into
> consideration.)
> 
> But my goal with the "isolate devices" thing was twofold:
> 
> 1. actual make use of SMMU for address translation for all master
>   devices (instead of just bypassing the SMMU)
[Sethi Varun-B16395]  even if masters have to share translation? But from the patch it seemed that we are creating mapping only if we find the isolate devices property.
> 
> plus
> 
> 2. put each master into it's own domain to isolate it
> 
> The latter matches usage of separate iommu groups for devices. If we now
> use the isolate devices property to just control whether master devices
> fall into the same or separate iommu groups it seems to me we would need
> to have another mechanism that triggers the creation of a mapping for a
> group.
> 
> What do you think?
> 
[Sethi Varun-B16395] As mentioned before, we should be having iommu group per device (having a unique stream id). Isolate devices property just ensures that each device has a unique context. Now, for the devices for which we don't have the isolate device property, can't we have the multiple devices point to a common mapping. 

-Varun

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Andreas Herrmann]

On Tue, Jan 28, 2014 at 11:00:35AM +0000, Varun Sethi wrote:
> 
> 
> > -----Original Message-----
> > From: iommu-bounces at lists.linux-foundation.org [mailto:iommu-
> > bounces at lists.linux-foundation.org] On Behalf Of Andreas Herrmann
> > Sent: Friday, January 24, 2014 1:27 AM
> > To: Sethi Varun-B16395
> > Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; Will Deacon;
> > linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> > notifier block
> > 
> > On Wed, Jan 22, 2014 at 07:07:40PM +0000, Varun Sethi wrote:
> > > > -----Original Message-----
> > > > From: Will Deacon [mailto:will.deacon at arm.com]
> > > > Sent: Wednesday, January 22, 2014 9:04 PM
> > > > To: Sethi Varun-B16395
> > > > Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; linux-arm-
> > > > kernel at lists.infradead.org; Andreas Herrmann
> > > > Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> > > > notifier block
> > > >
> > > > On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > > > > > Ok, so are you suggesting that we perform the isolation
> > > > > > > > mapping in arm_smmu_add_device and drop the notifier
> > altogether?
> > > > > > > I think that should be fine, until we want to delay mapping
> > > > > > > creation till driver bind time.
> > > > > >
> > > > > > Is there a hard dependency on that?
> > > > > >
> > > > > Not sure, may be Andreas can answer that.
> > > >
> > > > Ok. Andreas? I would have thought doing this *earlier* shouldn't be
> > > > a problem (the DMA ops must be swizzled before the driver is probed).
> > > >
> > > > > > > But the "isolate device" property should dictate iommu group
> > > > creation.
> > > > > >
> > > > > > The reason we added automatic group creation (per-device) is for
> > > > > > VFIO, which expects all devices to be in a group regardless of
> > > > > > the device isolation configuration.
> > > > > >
> > > > > IIUC, with the "isolate devices" property we ensure that there
> > > > > would be independent SMR and S2CR per device. Is that correct?
> > > >
> > > > Yes, as part of giving them independent sets of page tables.
> > > > Initially these tables don't have any valid mappings, but the
> > > > dma-mapping API will populate them in response to
> > dma_map_*/dma_alloc/etc.
> > > >
> > > > Not sure what this has to do with us putting devices into their own
> > > > groups...
> > 
> > > [Sethi Varun-B16395] Devices in an iommu group would share the dma
> > > mapping, so shouldn't they be sharing the SMR and context registers?
> > 
> > I aggree with the context but SMRs won't be shared. I think you just can
> > say that a certain subset of the available SMRs will be used to map all
> > devices in an iommu group to the same context. Depending on what
> > streamIDs each device has you might have to use separate SMRs for each
> > device to map it to the same context.

> [Sethi Varun-B16395] IIUC the SMRs would unique per device, but
> there is a possibility where the number of context registers are
> restricted?  In that case IOMMU groups should correspond to unique
> stream IDs (and corresponding SMRs).

> > > In case of the "isolate devices" property, each device would have its
> > > own SMR and context registers, thus allowing the devices to have
> > > independent mappings and allowing them to fall in separate iommu
> > > groups.
> > 
> > I aggree with Varun's view here. (Now that I take iommu groups into
> > consideration.)
> > 
> > But my goal with the "isolate devices" thing was twofold:
> > 
> > 1. actual make use of SMMU for address translation for all master
> >   devices (instead of just bypassing the SMMU)

> [Sethi Varun-B16395] even if masters have to share translation? But
> from the patch it seemed that we are creating mapping only if we
> find the isolate devices property.

Yes, the patch creates mappings only if isolate-devices property is
given. Currently there is no trigger to create a common mapping for
all devices attached to the same SMMU.

> > plus
> > 
> > 2. put each master into it's own domain to isolate it
> > 
> > The latter matches usage of separate iommu groups for devices. If we now
> > use the isolate devices property to just control whether master devices
> > fall into the same or separate iommu groups it seems to me we would need
> > to have another mechanism that triggers the creation of a mapping for a
> > group.
> > 
> > What do you think?
> > 

> [Sethi Varun-B16395] As mentioned before, we should be having iommu
> group per device (having a unique stream id). Isolate devices
> property just ensures that each device has a unique context. Now,
> for the devices for which we don't have the isolate device property,
> can't we have the multiple devices point to a common mapping.

Hmm, the isolate-devices option is per SMMU. So if this is set for an
SMMU the driver tries to create a mapping per device. (And this is
done for all master devices of this SMMU.)

Are you requesting to change the default behaviour of the driver to
create a shared mapping in case the isolate-devices property is not
specified in DT for an SMMU node?

Or maybe your concern is that you have x master devices for an SMMU
which support y number of contexts and x > y? Which would imply that
not all devices can be isolated and some need to share a context?


Andreas

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Varun Sethi]

> -----Original Message-----
> From: iommu-bounces at lists.linux-foundation.org [mailto:iommu-
> bounces at lists.linux-foundation.org] On Behalf Of Andreas Herrmann
> Sent: Wednesday, January 29, 2014 7:44 PM
> To: Sethi Varun-B16395
> Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; Will Deacon;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> notifier block
> 
> On Tue, Jan 28, 2014 at 11:00:35AM +0000, Varun Sethi wrote:
> >
> >
> > > -----Original Message-----
> > > From: iommu-bounces at lists.linux-foundation.org [mailto:iommu-
> > > bounces at lists.linux-foundation.org] On Behalf Of Andreas Herrmann
> > > Sent: Friday, January 24, 2014 1:27 AM
> > > To: Sethi Varun-B16395
> > > Cc: Andreas Herrmann; iommu at lists.linux-foundation.org; Will Deacon;
> > > linux-arm-kernel at lists.infradead.org
> > > Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group
> > > notifier block
> > >
> > > On Wed, Jan 22, 2014 at 07:07:40PM +0000, Varun Sethi wrote:
> > > > > -----Original Message-----
> > > > > From: Will Deacon [mailto:will.deacon at arm.com]
> > > > > Sent: Wednesday, January 22, 2014 9:04 PM
> > > > > To: Sethi Varun-B16395
> > > > > Cc: Andreas Herrmann; iommu at lists.linux-foundation.org;
> > > > > linux-arm- kernel at lists.infradead.org; Andreas Herrmann
> > > > > Subject: Re: [PATCH v3 02/11] iommu/arm-smmu: Introduce
> > > > > iommu_group notifier block
> > > > >
> > > > > On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > > > > > > Ok, so are you suggesting that we perform the isolation
> > > > > > > > > mapping in arm_smmu_add_device and drop the notifier
> > > altogether?
> > > > > > > > I think that should be fine, until we want to delay
> > > > > > > > mapping creation till driver bind time.
> > > > > > >
> > > > > > > Is there a hard dependency on that?
> > > > > > >
> > > > > > Not sure, may be Andreas can answer that.
> > > > >
> > > > > Ok. Andreas? I would have thought doing this *earlier* shouldn't
> > > > > be a problem (the DMA ops must be swizzled before the driver is
> probed).
> > > > >
> > > > > > > > But the "isolate device" property should dictate iommu
> > > > > > > > group
> > > > > creation.
> > > > > > >
> > > > > > > The reason we added automatic group creation (per-device) is
> > > > > > > for VFIO, which expects all devices to be in a group
> > > > > > > regardless of the device isolation configuration.
> > > > > > >
> > > > > > IIUC, with the "isolate devices" property we ensure that there
> > > > > > would be independent SMR and S2CR per device. Is that correct?
> > > > >
> > > > > Yes, as part of giving them independent sets of page tables.
> > > > > Initially these tables don't have any valid mappings, but the
> > > > > dma-mapping API will populate them in response to
> > > dma_map_*/dma_alloc/etc.
> > > > >
> > > > > Not sure what this has to do with us putting devices into their
> > > > > own groups...
> > >
> > > > [Sethi Varun-B16395] Devices in an iommu group would share the dma
> > > > mapping, so shouldn't they be sharing the SMR and context
> registers?
> > >
> > > I aggree with the context but SMRs won't be shared. I think you just
> > > can say that a certain subset of the available SMRs will be used to
> > > map all devices in an iommu group to the same context. Depending on
> > > what streamIDs each device has you might have to use separate SMRs
> > > for each device to map it to the same context.
> 
> > [Sethi Varun-B16395] IIUC the SMRs would unique per device, but there
> > is a possibility where the number of context registers are restricted?
> > In that case IOMMU groups should correspond to unique stream IDs (and
> > corresponding SMRs).
> 
> > > > In case of the "isolate devices" property, each device would have
> > > > its own SMR and context registers, thus allowing the devices to
> > > > have independent mappings and allowing them to fall in separate
> > > > iommu groups.
> > >
> > > I aggree with Varun's view here. (Now that I take iommu groups into
> > > consideration.)
> > >
> > > But my goal with the "isolate devices" thing was twofold:
> > >
> > > 1. actual make use of SMMU for address translation for all master
> > >   devices (instead of just bypassing the SMMU)
> 
> > [Sethi Varun-B16395] even if masters have to share translation? But
> > from the patch it seemed that we are creating mapping only if we find
> > the isolate devices property.
> 
> Yes, the patch creates mappings only if isolate-devices property is
> given. Currently there is no trigger to create a common mapping for all
> devices attached to the same SMMU.
> 
> > > plus
> > >
> > > 2. put each master into it's own domain to isolate it
> > >
> > > The latter matches usage of separate iommu groups for devices. If we
> > > now use the isolate devices property to just control whether master
> > > devices fall into the same or separate iommu groups it seems to me
> > > we would need to have another mechanism that triggers the creation
> > > of a mapping for a group.
> > >
> > > What do you think?
> > >
> 
> > [Sethi Varun-B16395] As mentioned before, we should be having iommu
> > group per device (having a unique stream id). Isolate devices property
> > just ensures that each device has a unique context. Now, for the
> > devices for which we don't have the isolate device property, can't we
> > have the multiple devices point to a common mapping.
> 
> Hmm, the isolate-devices option is per SMMU. So if this is set for an
> SMMU the driver tries to create a mapping per device. (And this is done
> for all master devices of this SMMU.)
> 
> Are you requesting to change the default behaviour of the driver to
> create a shared mapping in case the isolate-devices property is not
> specified in DT for an SMMU node?
> 
> Or maybe your concern is that you have x master devices for an SMMU which
> support y number of contexts and x > y? Which would imply that not all
> devices can be isolated and some need to share a context?
> 
[Sethi Varun-B16395] I am trying to understand the requirement for the "isolate devices" property. Currently no default in mapping is created in the SMMU driver. The new property allows default mapping to be created for all masters. Why can't we create default mappings for all masters without this property?

-Varun

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Andreas Herrmann]

On Wed, Jan 22, 2014 at 03:33:52PM +0000, Will Deacon wrote:
> On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > > Ok, so are you suggesting that we perform the isolation mapping in
> > > > > arm_smmu_add_device and drop the notifier altogether?
> > > > I think that should be fine, until we want to delay mapping creation
> > > > till driver bind time.
> > > 
> > > Is there a hard dependency on that?
> > > 
> > Not sure, may be Andreas can answer that.
> 
> Ok. Andreas? I would have thought doing this *earlier* shouldn't be a
> problem (the DMA ops must be swizzled before the driver is probed).

Yes, I think, there is no hard dependency.

(But still have to double check whether arm_smmu_add_device can be
used instead of the notifier. Will finally see this when doing some
tests.)

> > > > But the "isolate device" property should dictate iommu group creation.
> > > 
> > > The reason we added automatic group creation (per-device) is for VFIO,
> > > which expects all devices to be in a group regardless of the device
> > > isolation configuration.
> > > 
> > IIUC, with the "isolate devices" property we ensure that there would be
> > independent SMR and S2CR per device. Is that correct?
> 
> Yes, as part of giving them independent sets of page tables. Initially these
> tables don't have any valid mappings, but the dma-mapping API will populate
> them in response to dma_map_*/dma_alloc/etc.
> 
> Not sure what this has to do with us putting devices into their own
> groups...
> 
> Will


Andreas

[PATCH v3 02/11] iommu/arm-smmu: Introduce iommu_group notifier block

[Andreas Herrmann]

On Thu, Jan 23, 2014 at 08:24:29PM +0100, Andreas Herrmann wrote:
> On Wed, Jan 22, 2014 at 03:33:52PM +0000, Will Deacon wrote:
> > On Wed, Jan 22, 2014 at 01:54:11PM +0000, Varun Sethi wrote:
> > > > > > Ok, so are you suggesting that we perform the isolation mapping in
> > > > > > arm_smmu_add_device and drop the notifier altogether?
> > > > > I think that should be fine, until we want to delay mapping creation
> > > > > till driver bind time.
> > > > 
> > > > Is there a hard dependency on that?
> > > > 
> > > Not sure, may be Andreas can answer that.
> > 
> > Ok. Andreas? I would have thought doing this *earlier* shouldn't be a
> > problem (the DMA ops must be swizzled before the driver is probed).
> 
> Yes, I think, there is no hard dependency.
> 
> (But still have to double check whether arm_smmu_add_device can be
> used instead of the notifier. Will finally see this when doing some
> tests.)

I think we have to keep the notifier and delay mapping creation until
device is bound to a driver.

Otherwise we might create mappings for devices which might not be used
at all. Who guarantees that the running kernel will have driver
support for all devices? If there is no driver for a device why create
a mapping for it -- no DMA will happen.


Andreas

[PATCH 03/11] iommu/arm-smmu: Support buggy implementation where all config accesses are secure

[Andreas Herrmann]

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

This handling is switched on by DT property
"calxeda,smmu-secure-config-access" for an SMMU node.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index bc81dd0..823699e 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -50,6 +50,7 @@
 
 /* Driver options */
 #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
+#define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 1)
 
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		8
@@ -64,6 +65,15 @@
 #define ARM_SMMU_GR0(smmu)		((smmu)->base)
 #define ARM_SMMU_GR1(smmu)		((smmu)->base + (smmu)->pagesize)
 
+/*
+ * SMMU global address space with conditional offset to access secure aliases of
+ * non-secure registers (e.g. nsCR0: 0x400, nsGFSR: 0x448, nsGFSYNR0: 0x450)
+ */
+#define ARM_SMMU_GR0_NS(smmu)						\
+	((smmu)->base +							\
+		((smmu->options & ARM_SMMU_OPT_SECURE_CONFIG_ACCESS)	\
+			? 0x400 : 0))
+
 /* Page table bits */
 #define ARM_SMMU_PTE_PAGE		(((pteval_t)3) << 0)
 #define ARM_SMMU_PTE_CONT		(((pteval_t)1) << 52)
@@ -410,6 +420,7 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options [] = {
 	{ ARM_SMMU_OPT_ISOLATE_DEVICES, "arm,smmu-isolate-devices" },
+	{ ARM_SMMU_OPT_SECURE_CONFIG_ACCESS, "calxeda,smmu-secure-config-access" },
 	{ 0, NULL},
 };
 
@@ -639,16 +650,16 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
 {
 	u32 gfsr, gfsynr0, gfsynr1, gfsynr2;
 	struct arm_smmu_device *smmu = dev;
-	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+	void __iomem *gr0_base = ARM_SMMU_GR0_NS(smmu);
 
 	gfsr = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
-	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);
 
+	if (!gfsr)
+		return IRQ_NONE;
+
 	dev_err_ratelimited(smmu->dev,
 		"Unexpected global fault, this could be serious\n");
 	dev_err_ratelimited(smmu->dev,
@@ -1586,9 +1597,9 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	int i = 0;
 	u32 reg;
 
-	/* Clear Global FSR */
-	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
-	writel(reg, gr0_base + ARM_SMMU_GR0_sGFSR);
+	/* clear global FSR */
+	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
+	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 
 	/* Mark all SMRn as invalid and all S2CRn as bypass */
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
@@ -1608,7 +1619,7 @@ 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);
 
-	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 
 	/* Enable fault reporting */
 	reg |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
@@ -1627,7 +1638,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 
 	/* Push the button */
 	arm_smmu_tlb_sync(smmu);
-	writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_sCR0);
+	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 }
 
 static int arm_smmu_id_size_to_bits(int size)
@@ -1961,7 +1972,7 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 		free_irq(smmu->irqs[i], smmu);
 
 	/* Turn the thing off */
-	writel_relaxed(sCR0_CLIENTPD, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_sCR0);
+	writel(sCR0_CLIENTPD,ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 04/11] iommu/arm-smmu: Introduce automatic stream-id-masking

[Andreas Herrmann]

Try to determine a mask that can be used for all StreamIDs of a master
device. This allows to use just one SMR group instead of
number-of-streamids SMR groups for a master device.

Changelog:
* dropped "#define DEBUG"
* removed  "BUG_ON(!is_power_of_2(nr))" from determine_smr_mask
  by passing an order instead of the actual number of streamids
  to this function.
* added check for master->num_used_smrs being 0 to
  determine_smr_mapping
* renamed num_used_smrs to num_s2crs
* added validation of calculated SMR mask and id field (against number
  of implemented bits of SMR

Notes:

* Check for duplicate stream IDs
  - not implemented with this patch but in a separate patch
  - If the same stream ID is specified for 2 masters further behaviour
    is implementation defined (we'll end with more than one matching
    entry in the stream mapping table).
  - If one stream ID is specified twice for a master device, the
    determination of how many S2CR/SMR groups are required for stream
    mapping will fail. Esp. it can happen that more than one matching
    entry is created in the stream mapping table.

* Sorting of stream IDs (to make usage of S2CR independend of sequence of
  stream IDs in DT)
  - intentionally not implemented
  - code does not rely on sorting
  - in fact sorting might make things worse with this simple
    implementation
    + Example: master with stream IDs 4, 5, 6, 0xe, 0xf requires 3
      SMRs when IDs are specified in this sorted order (one to map 4,
      5, one to map 6, one to map 0xe, 0xf) but just 2 SMRs when
      specified as 4, 5, 0xe, 0xf, 6 (one to map 4, 5, 0xe, 0xf and
      one SMR to map 6)
  - thus by modifying the DT information you can affect the number of
    S2CRs required for stream matching
  => I'd say "use common sense" when specifying stream IDs for a master
   device in DT.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 823699e..02a871e 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -42,6 +42,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/bitops.h>
 
 #include <linux/amba/bus.h>
 
@@ -338,8 +339,9 @@ struct arm_smmu_master {
 	 * SMMU chain.
 	 */
 	struct rb_node			node;
-	int				num_streamids;
+	u32				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
+	int				num_s2crs;
 
 	/*
 	 * We only need to allocate these on the root SMMU, as we
@@ -381,6 +383,9 @@ struct arm_smmu_device {
 	u32				num_context_irqs;
 	unsigned int			*irqs;
 
+	u32				smr_mask_mask;
+	u32				smr_id_mask;
+
 	struct list_head		list;
 	struct rb_root			masters;
 };
@@ -1025,10 +1030,109 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
 	kfree(smmu_domain);
 }
 
+static int determine_smr_mask(struct arm_smmu_device *smmu,
+			struct arm_smmu_master *master,
+			struct arm_smmu_smr *smr, int start, int order)
+{
+	u16 i, zero_bits_mask, one_bits_mask, const_mask;
+	int nr;
+
+	nr = 1 << order;
+
+	if (nr == 1) {
+		/* no mask, use streamid to match and be done with it */
+		smr->mask = 0;
+		smr->id = master->streamids[start];
+		return 0;
+	}
+
+	zero_bits_mask = 0;
+	one_bits_mask = 0xffff;
+	for (i = start; i < start + nr; i++) {
+		zero_bits_mask |= master->streamids[i];   /* const 0 bits */
+		one_bits_mask &= master->streamids[i]; /* const 1 bits */
+	}
+	zero_bits_mask = ~zero_bits_mask;
+
+	/* bits having constant values (either 0 or 1) */
+	const_mask = zero_bits_mask | one_bits_mask;
+
+	i = hweight16(~const_mask);
+	if ((1 << i) == nr) {
+		smr->mask = ~const_mask;
+		smr->id = one_bits_mask;
+	} else {
+		/* no usable mask for this set of streamids */
+		return 1;
+	}
+
+	if (((smr->mask & smmu->smr_mask_mask) != smr->mask) ||
+		((smr->id & smmu->smr_id_mask) != smr->id))
+		/* insufficient number of mask/id bits */
+		return 1;
+
+	return 0;
+}
+
+static int determine_smr_mapping(struct arm_smmu_device *smmu,
+				struct arm_smmu_master *master,
+				struct arm_smmu_smr *smrs, int max_smrs)
+{
+	int nr_sid, nr, i, bit, start;
+
+	/*
+	 * This function is called only once -- when a master is added
+	 * to a domain. If master->num_s2crs != 0 then this master
+	 * was already added to a domain.
+	 */
+	BUG_ON(master->num_s2crs);
+
+	start = nr = 0;
+	nr_sid = master->num_streamids;
+	do {
+		/*
+		 * largest power-of-2 number of streamids for which to
+		 * determine a usable mask/id pair for stream matching
+		 */
+		bit = fls(nr_sid);
+		if (!bit)
+			return 0;
+
+		/*
+		 * iterate over power-of-2 numbers to determine
+		 * largest possible mask/id pair for stream matching
+		 * of next 2**i streamids
+		 */
+		for (i = bit - 1; i >= 0; i--) {
+			if(!determine_smr_mask(smmu, master,
+						&smrs[master->num_s2crs],
+						start, i))
+				break;
+		}
+
+		if (i < 0)
+			goto out;
+
+		nr = 1 << i;
+		nr_sid -= nr;
+		start += nr;
+		master->num_s2crs++;
+	} while (master->num_s2crs <= max_smrs);
+
+out:
+	if (nr_sid) {
+		/* not enough mapping groups available */
+		master->num_s2crs = 0;
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
 static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 					  struct arm_smmu_master *master)
 {
-	int i;
+	int i, max_smrs, ret;
 	struct arm_smmu_smr *smrs;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 
@@ -1038,42 +1142,45 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 	if (master->smrs)
 		return -EEXIST;
 
-	smrs = kmalloc(sizeof(*smrs) * master->num_streamids, GFP_KERNEL);
+	max_smrs = min(smmu->num_mapping_groups, master->num_streamids);
+	smrs = kmalloc(sizeof(*smrs) * max_smrs, GFP_KERNEL);
 	if (!smrs) {
 		dev_err(smmu->dev, "failed to allocate %d SMRs for master %s\n",
-			master->num_streamids, master->of_node->name);
+			max_smrs, master->of_node->name);
 		return -ENOMEM;
 	}
 
+	ret = determine_smr_mapping(smmu, master, smrs, max_smrs);
+	if (ret)
+		goto err_free_smrs;
+
 	/* Allocate the SMRs on the root SMMU */
-	for (i = 0; i < master->num_streamids; ++i) {
+	for (i = 0; i < master->num_s2crs; ++i) {
 		int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
 						  smmu->num_mapping_groups);
 		if (IS_ERR_VALUE(idx)) {
 			dev_err(smmu->dev, "failed to allocate free SMR\n");
-			goto err_free_smrs;
+			goto err_free_bitmap;
 		}
-
-		smrs[i] = (struct arm_smmu_smr) {
-			.idx	= idx,
-			.mask	= 0, /* We don't currently share SMRs */
-			.id	= master->streamids[i],
-		};
+		smrs[i].idx = idx;
 	}
 
 	/* It worked! Now, poke the actual hardware */
-	for (i = 0; i < master->num_streamids; ++i) {
+	for (i = 0; i < master->num_s2crs; ++i) {
 		u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
 			  smrs[i].mask << SMR_MASK_SHIFT;
+		dev_dbg(smmu->dev, "SMR%d: 0x%x\n", smrs[i].idx, reg);
 		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx));
 	}
 
 	master->smrs = smrs;
 	return 0;
 
-err_free_smrs:
+err_free_bitmap:
 	while (--i >= 0)
 		__arm_smmu_free_bitmap(smmu->smr_map, smrs[i].idx);
+	master->num_s2crs = 0;
+err_free_smrs:
 	kfree(smrs);
 	return -ENOSPC;
 }
@@ -1136,11 +1243,14 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	}
 
 	/* Now we're at the root, time to point at our context bank */
-	for (i = 0; i < master->num_streamids; ++i) {
+	if (!master->num_s2crs)
+		master->num_s2crs = master->num_streamids;
+	for (i = 0; i < master->num_s2crs; ++i) {
 		u32 idx, s2cr;
 		idx = master->smrs ? master->smrs[i].idx : master->streamids[i];
 		s2cr = (S2CR_TYPE_TRANS << S2CR_TYPE_SHIFT) |
 		       (smmu_domain->root_cfg.cbndx << S2CR_CBNDX_SHIFT);
+		dev_dbg(smmu->dev, "S2CR%d: 0x%x\n", idx, s2cr);
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
 
@@ -1733,6 +1843,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 				mask, sid);
 			return -ENODEV;
 		}
+		smmu->smr_mask_mask = mask;
+		smmu->smr_id_mask = sid;
 
 		dev_notice(smmu->dev,
 			   "\tstream matching with %u register groups, mask 0x%x",
-- 
1.7.9.5

[PATCH 04/11] iommu/arm-smmu: Introduce automatic stream-id-masking

[Will Deacon]

Hi Andreas,

This patch always requires some extra brain cycles when reviewing!

On Thu, Jan 16, 2014 at 12:44:16PM +0000, Andreas Herrmann wrote:
> Try to determine a mask that can be used for all StreamIDs of a master
> device. This allows to use just one SMR group instead of
> number-of-streamids SMR groups for a master device.
> 
> Changelog:

You can put the change log and notes after the '---' so they don't appear in
the commit log, although the commit message could probably use a brief
description of your algorithm.

> * Sorting of stream IDs (to make usage of S2CR independend of sequence of
>   stream IDs in DT)
>   - intentionally not implemented
>   - code does not rely on sorting
>   - in fact sorting might make things worse with this simple
>     implementation
>     + Example: master with stream IDs 4, 5, 6, 0xe, 0xf requires 3
>       SMRs when IDs are specified in this sorted order (one to map 4,
>       5, one to map 6, one to map 0xe, 0xf) but just 2 SMRs when
>       specified as 4, 5, 0xe, 0xf, 6 (one to map 4, 5, 0xe, 0xf and
>       one SMR to map 6)
>   - thus by modifying the DT information you can affect the number of
>     S2CRs required for stream matching
>   => I'd say "use common sense" when specifying stream IDs for a master
>    device in DT.

Then we probably want a comment in the driver helping people work out what
the best ordering is.

> @@ -1025,10 +1030,109 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
>  	kfree(smmu_domain);
>  }
>  
> +static int determine_smr_mask(struct arm_smmu_device *smmu,
> +			struct arm_smmu_master *master,
> +			struct arm_smmu_smr *smr, int start, int order)
> +{
> +	u16 i, zero_bits_mask, one_bits_mask, const_mask;
> +	int nr;
> +
> +	nr = 1 << order;
> +
> +	if (nr == 1) {
> +		/* no mask, use streamid to match and be done with it */
> +		smr->mask = 0;
> +		smr->id = master->streamids[start];
> +		return 0;
> +	}
> +
> +	zero_bits_mask = 0;
> +	one_bits_mask = 0xffff;
> +	for (i = start; i < start + nr; i++) {
> +		zero_bits_mask |= master->streamids[i];   /* const 0 bits */
> +		one_bits_mask &= master->streamids[i]; /* const 1 bits */
> +	}
> +	zero_bits_mask = ~zero_bits_mask;
> +
> +	/* bits having constant values (either 0 or 1) */
> +	const_mask = zero_bits_mask | one_bits_mask;
> +
> +	i = hweight16(~const_mask);
> +	if ((1 << i) == nr) {
> +		smr->mask = ~const_mask;
> +		smr->id = one_bits_mask;

This part always confuses me. Why do we check (1 << i) against nr? In fact,
in your example where we have SIDs {4,5,e,f,6}, then we'll call this
initially with start = 0, order = 2 and try to allocate an smr for
{4,5,e,f}. That will succeed with mask 1011b and id 0100b, but the mask has
a hamming weight of 3, which is != nr (2).

Where am I getting this wrong?

I also still need to convince myself that we can't end up generating smrs
which match the same SID. Is that what your check above is trying to handle?

> +static int determine_smr_mapping(struct arm_smmu_device *smmu,
> +				struct arm_smmu_master *master,
> +				struct arm_smmu_smr *smrs, int max_smrs)
> +{
> +	int nr_sid, nr, i, bit, start;
> +
> +	/*
> +	 * This function is called only once -- when a master is added
> +	 * to a domain. If master->num_s2crs != 0 then this master
> +	 * was already added to a domain.
> +	 */
> +	BUG_ON(master->num_s2crs);

I think I'd rather WARN and return -EINVAL. We needn't kill the kernel for
this.

> +
> +	start = nr = 0;
> +	nr_sid = master->num_streamids;
> +	do {
> +		/*
> +		 * largest power-of-2 number of streamids for which to
> +		 * determine a usable mask/id pair for stream matching
> +		 */
> +		bit = fls(nr_sid);

If you use __fls...

> +		if (!bit)
> +			return 0;
> +
> +		/*
> +		 * iterate over power-of-2 numbers to determine
> +		 * largest possible mask/id pair for stream matching
> +		 * of next 2**i streamids
> +		 */
> +		for (i = bit - 1; i >= 0; i--) {

... then you don't need this -1.

> +			if(!determine_smr_mask(smmu, master,

Cosmetic: space after 'if'.

>  	/* It worked! Now, poke the actual hardware */
> -	for (i = 0; i < master->num_streamids; ++i) {
> +	for (i = 0; i < master->num_s2crs; ++i) {
>  		u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
>  			  smrs[i].mask << SMR_MASK_SHIFT;
> +		dev_dbg(smmu->dev, "SMR%d: 0x%x\n", smrs[i].idx, reg);

I think we can drop the dev_dbg statements from this patch.

Will

[PATCH 04/11] iommu/arm-smmu: Introduce automatic stream-id-masking

[Andreas Herrmann]

On Wed, Jan 22, 2014 at 03:26:22PM +0000, Will Deacon wrote:
> Hi Andreas,
> 
> This patch always requires some extra brain cycles when reviewing!
> 
> On Thu, Jan 16, 2014 at 12:44:16PM +0000, Andreas Herrmann wrote:
> > Try to determine a mask that can be used for all StreamIDs of a master
> > device. This allows to use just one SMR group instead of
> > number-of-streamids SMR groups for a master device.
> > 
> > Changelog:
> 
> You can put the change log and notes after the '---' so they don't appear in
> the commit log, although the commit message could probably use a brief
> description of your algorithm.

Yep, will fix this.

> > * Sorting of stream IDs (to make usage of S2CR independend of sequence of
> >   stream IDs in DT)
> >   - intentionally not implemented
> >   - code does not rely on sorting
> >   - in fact sorting might make things worse with this simple
> >     implementation
> >     + Example: master with stream IDs 4, 5, 6, 0xe, 0xf requires 3
> >       SMRs when IDs are specified in this sorted order (one to map 4,
> >       5, one to map 6, one to map 0xe, 0xf) but just 2 SMRs when
> >       specified as 4, 5, 0xe, 0xf, 6 (one to map 4, 5, 0xe, 0xf and
> >       one SMR to map 6)
> >   - thus by modifying the DT information you can affect the number of
> >     S2CRs required for stream matching
> >   => I'd say "use common sense" when specifying stream IDs for a master
> >    device in DT.
> 
> Then we probably want a comment in the driver helping people work out what
> the best ordering is.

Hmm, yes indeed. Or maybe deliver some tool to calculate the best
ordering. (Doing this in kernel is overkill but calculate it once and
store the result in DTB seems to be the right approach.)

> > @@ -1025,10 +1030,109 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
> >  	kfree(smmu_domain);
> >  }
> >  
> > +static int determine_smr_mask(struct arm_smmu_device *smmu,
> > +			struct arm_smmu_master *master,
> > +			struct arm_smmu_smr *smr, int start, int order)
> > +{
> > +	u16 i, zero_bits_mask, one_bits_mask, const_mask;
> > +	int nr;
> > +
> > +	nr = 1 << order;
> > +
> > +	if (nr == 1) {
> > +		/* no mask, use streamid to match and be done with it */
> > +		smr->mask = 0;
> > +		smr->id = master->streamids[start];
> > +		return 0;
> > +	}
> > +
> > +	zero_bits_mask = 0;
> > +	one_bits_mask = 0xffff;
> > +	for (i = start; i < start + nr; i++) {
> > +		zero_bits_mask |= master->streamids[i];   /* const 0 bits */
> > +		one_bits_mask &= master->streamids[i]; /* const 1 bits */
> > +	}
> > +	zero_bits_mask = ~zero_bits_mask;
> > +
> > +	/* bits having constant values (either 0 or 1) */
> > +	const_mask = zero_bits_mask | one_bits_mask;
> > +
> > +	i = hweight16(~const_mask);
> > +	if ((1 << i) == nr) {
> > +		smr->mask = ~const_mask;
> > +		smr->id = one_bits_mask;
> 
> This part always confuses me. Why do we check (1 << i) against nr?

(I think, I could change this check to (i == order).)

The reason for this check is:

If there is a mismatch it means that the calculated mask/id pair
either covers

(1) more than nr stream IDs (that were used for the calculation)
    (then (1<<i) > nr)

or

(2) less than nr stream IDs (that were used for the calculation)
   (then (1<<i) < nr)

Only if there is a match, then we know that the calculated mask/id
pair match exactly nr stream IDs (and the matched IDs are those used
for the calculation).

> In fact, in your example where we have SIDs {4,5,e,f,6}, then we'll
> call this initially with start = 0, order = 2 and try to allocate an
> smr for {4,5,e,f}. That will succeed with mask 1011b and id 0100b,
> but the mask has a hamming weight of 3, which is != nr (2).

Arrgh, which means that the example is complete bogus:

4	0b00100
5	0b00101
0xe	0b01110
0xf	0b01111

calculation of const bit masks:
- bits being always zero:	0xfff0	(zero_bits_mask)
- bits being always one:	0x0004	(one_bits_mask)
- thus for const_mask we get:	0xfff4

hweight16(0x1011) != nr (1 << 2). So the algorithm would refuse to use
the calculated mask/id pair to program an SMR. Another iteration would
be done and one SMR for 4, 5 and after that another SMR for 0xe, 0xf
would be set up. The point is that with mask 1011b and id 0100b not
just 4, 5, 0xe, 0xf would be mapped but in fact all following stream
IDs would be mapped:

0b0100	(4)
0b0101  (5)
0b0110  (6)
0b0111  (7)
0b1100  (0xc)
0b1101  (0xd)
0b1110  (0xe)
0b1111  (0xf)

It's eight stream IDs that would be mapped but we considered only 4
stream IDs for the cacluation. We have to dismiss the mask/id pair.

I really should have written down the mask/id stuff to avoid
this wrong example. For a correct example I should have used
4, 5, 6, 0xc, 0xd.

4	0b00100
5	0b00101
0xc	0b01100
0xd	0b01101

calculation of const bit masks:
- bits being always zero:		 0xfff2	(zero_bits_mask)
- bits being always one:		 0x0004	(one_bits_mask)
- thus for const_mask we get:		 0xfff6

(hweight16(0b1001) = 2 which matches order = 2)

With mask 0b1001 and id 0b0100 following IDs match:
0100
0101
1100
1101

0-3, 8-0xb don't match (bit 2 is not set, but it should)
6, 7, 0xe, 0xf don't match (bit 1 is set, but it shouldn't)

> Where am I getting this wrong?

No.

But my example was crap. So providing a tool to correctly calculate
things is required I think.

> I also still need to convince myself that we can't end up generating
> smrs which match the same SID. Is that what your check above is
> trying to handle?

Yes, that should do the trick.

> > +static int determine_smr_mapping(struct arm_smmu_device *smmu,
> > +				struct arm_smmu_master *master,
> > +				struct arm_smmu_smr *smrs, int max_smrs)
> > +{
> > +	int nr_sid, nr, i, bit, start;
> > +
> > +	/*
> > +	 * This function is called only once -- when a master is added
> > +	 * to a domain. If master->num_s2crs != 0 then this master
> > +	 * was already added to a domain.
> > +	 */
> > +	BUG_ON(master->num_s2crs);
> 
> I think I'd rather WARN and return -EINVAL. We needn't kill the kernel for
> this.

Agreed.

> > +
> > +	start = nr = 0;
> > +	nr_sid = master->num_streamids;
> > +	do {
> > +		/*
> > +		 * largest power-of-2 number of streamids for which to
> > +		 * determine a usable mask/id pair for stream matching
> > +		 */
> > +		bit = fls(nr_sid);
> 
> If you use __fls...
> 
> > +		if (!bit)
> > +			return 0;
> > +
> > +		/*
> > +		 * iterate over power-of-2 numbers to determine
> > +		 * largest possible mask/id pair for stream matching
> > +		 * of next 2**i streamids
> > +		 */
> > +		for (i = bit - 1; i >= 0; i--) {
> 
> ... then you don't need this -1.

Ok.

> > +			if(!determine_smr_mask(smmu, master,
> 
> Cosmetic: space after 'if'.

Oops, sorry.

> >  	/* It worked! Now, poke the actual hardware */
> > -	for (i = 0; i < master->num_streamids; ++i) {
> > +	for (i = 0; i < master->num_s2crs; ++i) {
> >  		u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
> >  			  smrs[i].mask << SMR_MASK_SHIFT;
> > +		dev_dbg(smmu->dev, "SMR%d: 0x%x\n", smrs[i].idx, reg);
> 
> I think we can drop the dev_dbg statements from this patch.

Ok.

Andreas

[PATCH 05/11] iommu/arm-smmu: Check for duplicate stream IDs when registering master devices

[Andreas Herrmann]

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 02a871e..a4e0c93 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -56,6 +56,9 @@
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		8
 
+/* Maximum stream ID */
+#define ARM_SMMU_MAX_STREAMID		(SZ_64K - 1)
+
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
 
@@ -386,6 +389,8 @@ struct arm_smmu_device {
 	u32				smr_mask_mask;
 	u32				smr_id_mask;
 
+	unsigned long			*sids;
+
 	struct list_head		list;
 	struct rb_root			masters;
 };
@@ -491,7 +496,7 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
 				struct of_phandle_args *masterspec)
 {
-	int i;
+	int i, sid;
 	struct arm_smmu_master *master;
 
 	master = find_smmu_master(smmu, masterspec->np);
@@ -516,8 +521,14 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	master->of_node		= masterspec->np;
 	master->num_streamids	= masterspec->args_count;
 
-	for (i = 0; i < master->num_streamids; ++i)
-		master->streamids[i] = masterspec->args[i];
+	for (i = 0; i < master->num_streamids; ++i) {
+		sid = masterspec->args[i];
+		if (test_and_set_bit(sid, smmu->sids)) {
+			dev_err(dev, "duplicate stream ID (%d)\n", sid);
+			return -EEXIST;
+		}
+		master->streamids[i] = sid;
+	}
 
 	return insert_smmu_master(smmu, master);
 }
@@ -1934,6 +1945,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	smmu->dev = dev;
 
+	smmu->sids = devm_kzalloc(dev, BITS_TO_LONGS(ARM_SMMU_MAX_STREAMID) *
+				sizeof(long), GFP_KERNEL);
+	if (!smmu->sids) {
+		dev_err(dev,
+			"failed to allocate bitmap for stream ID tracking\n");
+		return -ENOMEM;
+	}
+
 	check_driver_options(smmu);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
1.7.9.5

[PATCH 05/11] iommu/arm-smmu: Check for duplicate stream IDs when registering master devices

[Will Deacon]

On Thu, Jan 16, 2014 at 12:44:17PM +0000, Andreas Herrmann wrote:
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 02a871e..a4e0c93 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -56,6 +56,9 @@
>  /* Maximum number of stream IDs assigned to a single device */
>  #define MAX_MASTER_STREAMIDS		8
>  
> +/* Maximum stream ID */
> +#define ARM_SMMU_MAX_STREAMID		(SZ_64K - 1)
> +
>  /* Maximum number of context banks per SMMU */
>  #define ARM_SMMU_MAX_CBS		128
>  
> @@ -386,6 +389,8 @@ struct arm_smmu_device {
>  	u32				smr_mask_mask;
>  	u32				smr_id_mask;
>  
> +	unsigned long			*sids;

DECLARE_BITMAP instead? However, that's an 8k bitmap just for sanity
checking, which I'm not too fond of. Given that the motivation for the
checking was your smr allocator, perhaps it's sufficient just to do the
checking on a per-master basis, which we can do with the exhaustive search
each time.

Will

[PATCH 05/11] iommu/arm-smmu: Check for duplicate stream IDs when registering master devices

[Andreas Herrmann]

On Wed, Jan 22, 2014 at 03:53:02PM +0000, Will Deacon wrote:
> On Thu, Jan 16, 2014 at 12:44:17PM +0000, Andreas Herrmann wrote:
> > Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |   25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 02a871e..a4e0c93 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -56,6 +56,9 @@
> >  /* Maximum number of stream IDs assigned to a single device */
> >  #define MAX_MASTER_STREAMIDS		8
> >  
> > +/* Maximum stream ID */
> > +#define ARM_SMMU_MAX_STREAMID		(SZ_64K - 1)
> > +
> >  /* Maximum number of context banks per SMMU */
> >  #define ARM_SMMU_MAX_CBS		128
> >  
> > @@ -386,6 +389,8 @@ struct arm_smmu_device {
> >  	u32				smr_mask_mask;
> >  	u32				smr_id_mask;
> >  
> > +	unsigned long			*sids;
> 
> DECLARE_BITMAP instead?

I wanted to allocate the bitmap which btw allows us to ... [? see end
of mail]

> However, that's an 8k bitmap just for sanity checking, which I'm not
> too fond of.

Yes, I thought the same. But finally I thought it's worth it.

> Given that the motivation for the checking was your smr allocator,
> perhaps it's sufficient just to do the checking on a per-master
> basis, which we can do with the exhaustive search each time.

A per-master basis is not sufficient. If you argue that someone could
have specified the same stream ID twice for one master in DT how are
chances that the same stream ID shows up twice but for different
master devices?

I think we have to check this (independent of my smr allocator).
If two masters accidentially share the/an ID:

* Stream ID matching (w/o a mask, just use one SMR to map one stream
  ID): It proably will cause a multi match with undifined behaviour.

* Stream ID indexing: You would overwrite an already used S2CR (if
  there are no further precautions).

That is why I think a check to reject duplicate stream IDs among all
masters for one SMMU is required.

If this is needed, exhaustive search doesn't seem to be a good idea,
because if you have a distributed SMMU -- one TCU (I think that's
where the SMRs are), and many TBUs -- you might have several master
devices attached (many more than in current systems). Each master
potentially has several stream IDs ...  Esp. in this case 8k for doing
the check isn't an issue.

Maybe a compromise is to 

... free the bitmap after the check is done.


Andreas

[PATCH 06/11] documentation/iommu: Update description of ARM System MMU binding

[Andreas Herrmann]

This patch adds descriptions fore new properties of device tree
binding for the ARM SMMU architecture. These properties control
arm-smmu driver options.

Cc: Rob Herring <robherring2@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index e34c6cd..7ad8ff0 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -48,6 +48,16 @@ conditions.
                   from the mmu-masters towards memory) node for this
                   SMMU.
 
+- arm,smmu-isolate-devices : Enable device isolation for all masters
+                   of this SMMU. Ie. each master will be attached to
+                   its own iommu domain.
+
+- 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 aliases of secure registers have to be
+                   used during SMMU configuration.
+
 Example:
 
         smmu {
@@ -67,4 +77,5 @@ Example:
                  */
                 mmu-masters = <&dma0 0xd01d 0xd01e>,
                               <&dma1 0xd11c>;
+                arm,smmu-isolate-devices;
         };
-- 
1.7.9.5

[PATCH 06/11] documentation/iommu: Update description of ARM System MMU binding

[Rob Herring]

On Thu, Jan 16, 2014 at 6:44 AM, Andreas Herrmann
<andreas.herrmann@calxeda.com> wrote:
> This patch adds descriptions fore new properties of device tree
> binding for the ARM SMMU architecture. These properties control
> arm-smmu driver options.
>
> Cc: Rob Herring <robherring2@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt         |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index e34c6cd..7ad8ff0 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -48,6 +48,16 @@ conditions.
>                    from the mmu-masters towards memory) node for this
>                    SMMU.
>
> +- arm,smmu-isolate-devices : Enable device isolation for all masters
> +                   of this SMMU. Ie. each master will be attached to
> +                   its own iommu domain.
> +
> +- 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 aliases of secure registers have to be
> +                   used during SMMU configuration.
> +
>  Example:
>
>          smmu {
> @@ -67,4 +77,5 @@ Example:
>                   */
>                  mmu-masters = <&dma0 0xd01d 0xd01e>,
>                                <&dma1 0xd11c>;
> +                arm,smmu-isolate-devices;
>          };
> --
> 1.7.9.5
>

[PATCH 07/11] iommu/arm-smmu: Set MAX_MASTER_STREAMIDS to MAX_PHANDLE_ARGS

[Andreas Herrmann]

The DT parsing code that determines stream IDs uses
of_parse_phandle_with_args and thus MAX_MASTER_STREAMIDS
is always bound by MAX_PHANDLE_ARGS.

Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
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 a4e0c93..68bbe45 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -54,7 +54,7 @@
 #define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 1)
 
 /* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		8
+#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
 
 /* Maximum stream ID */
 #define ARM_SMMU_MAX_STREAMID		(SZ_64K - 1)
-- 
1.7.9.5

[PATCH 08/11] of: Increase MAX_PHANDLE_ARGS

[Andreas Herrmann]

arm-smmu driver uses of_parse_phandle_with_args when parsing DT
information to determine stream IDs for a master device.
Thus the number of stream IDs per master device is bound by
MAX_PHANDLE_ARGS.

To support Calxeda ECX-2000 hardware arm-smmu driver requires a
slightly higher value for MAX_PHANDLE_ARGS as this hardware has 10
stream IDs for one master device.

Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree at vger.kernel.org
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 include/linux/of.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index 276c546..0807af8 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -67,7 +67,7 @@ struct device_node {
 #endif
 };
 
-#define MAX_PHANDLE_ARGS 8
+#define MAX_PHANDLE_ARGS 10
 struct of_phandle_args {
 	struct device_node *np;
 	int args_count;
-- 
1.7.9.5

[PATCH 08/11] of: Increase MAX_PHANDLE_ARGS

[Rob Herring]

On Thu, Jan 16, 2014 at 6:44 AM, Andreas Herrmann
<andreas.herrmann@calxeda.com> wrote:
> arm-smmu driver uses of_parse_phandle_with_args when parsing DT
> information to determine stream IDs for a master device.
> Thus the number of stream IDs per master device is bound by
> MAX_PHANDLE_ARGS.
>
> To support Calxeda ECX-2000 hardware arm-smmu driver requires a
> slightly higher value for MAX_PHANDLE_ARGS as this hardware has 10
> stream IDs for one master device.
>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree at vger.kernel.org
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>

Acked-by: Rob Herring <robh@kernel.org>

One comment below...

> ---
>  include/linux/of.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 276c546..0807af8 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -67,7 +67,7 @@ struct device_node {
>  #endif
>  };
>
> -#define MAX_PHANDLE_ARGS 8
> +#define MAX_PHANDLE_ARGS 10

Just bump this to 16. This is normally just a temporary on the stack
and 8 more words on the stack is not going to cost much.

Rob

[PATCH 08/11] of: Increase MAX_PHANDLE_ARGS

[Andreas Herrmann]

On Thu, Jan 16, 2014 at 09:25:59AM -0500, Rob Herring wrote:
> On Thu, Jan 16, 2014 at 6:44 AM, Andreas Herrmann
> <andreas.herrmann@calxeda.com> wrote:
> > arm-smmu driver uses of_parse_phandle_with_args when parsing DT
> > information to determine stream IDs for a master device.
> > Thus the number of stream IDs per master device is bound by
> > MAX_PHANDLE_ARGS.
> >
> > To support Calxeda ECX-2000 hardware arm-smmu driver requires a
> > slightly higher value for MAX_PHANDLE_ARGS as this hardware has 10
> > stream IDs for one master device.
> >
> > Cc: Grant Likely <grant.likely@linaro.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: devicetree at vger.kernel.org
> > Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
> One comment below...
> 
> > ---
> >  include/linux/of.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 276c546..0807af8 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -67,7 +67,7 @@ struct device_node {
> >  #endif
> >  };
> >
> > -#define MAX_PHANDLE_ARGS 8
> > +#define MAX_PHANDLE_ARGS 10
> 
> Just bump this to 16. This is normally just a temporary on the stack
> and 8 more words on the stack is not going to cost much.

Yes, that seems reasonable.


Thanks,

Andreas

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Andreas Herrmann]

arm-smmu driver uses of_parse_phandle_with_args when parsing DT
information to determine stream IDs for a master device.
Thus the number of stream IDs per master device is bound by
MAX_PHANDLE_ARGS.

To support Calxeda ECX-2000 hardware arm-smmu driver requires a
slightly higher value for MAX_PHANDLE_ARGS as this hardware has 10
stream IDs for one master device.

Increasing it to 16 seems a reasonable choice.

Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree at vger.kernel.org
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 include/linux/of.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index 276c546..24e1b28 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -67,7 +67,7 @@ struct device_node {
 #endif
 };
 
-#define MAX_PHANDLE_ARGS 8
+#define MAX_PHANDLE_ARGS 16
 struct of_phandle_args {
 	struct device_node *np;
 	int args_count;
-- 
1.7.9.5

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Suravee Suthikulanit]

On 1/17/2014 5:08 AM, Andreas Herrmann wrote:
>
> arm-smmu driver uses of_parse_phandle_with_args when parsing DT
> information to determine stream IDs for a master device.
> Thus the number of stream IDs per master device is bound by
> MAX_PHANDLE_ARGS.
>
> To support Calxeda ECX-2000 hardware arm-smmu driver requires a
> slightly higher value for MAX_PHANDLE_ARGS as this hardware has 10
> stream IDs for one master device.
>
> Increasing it to 16 seems a reasonable choice.
>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree at vger.kernel.org
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>   include/linux/of.h |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 276c546..24e1b28 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -67,7 +67,7 @@ struct device_node {
>   #endif
>   };
>
> -#define MAX_PHANDLE_ARGS 8
> +#define MAX_PHANDLE_ARGS 16

Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't 
this be changed to be able to support 128 StreamIDs as well?  Although I 
am not sure if this would be too big to have on the stack per Rob's 
comment in the previous patch set.

Thank you,

Suravee

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Rob Herring]

On Wed, Jan 29, 2014 at 10:11 AM, Suravee Suthikulanit
<suravee.suthikulpanit@amd.com> wrote:
> On 1/17/2014 5:08 AM, Andreas Herrmann wrote:
>>
>>
>> arm-smmu driver uses of_parse_phandle_with_args when parsing DT
>> information to determine stream IDs for a master device.
>> Thus the number of stream IDs per master device is bound by
>> MAX_PHANDLE_ARGS.
>>
>> To support Calxeda ECX-2000 hardware arm-smmu driver requires a
>> slightly higher value for MAX_PHANDLE_ARGS as this hardware has 10
>> stream IDs for one master device.
>>
>> Increasing it to 16 seems a reasonable choice.
>>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: devicetree at vger.kernel.org
>> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
>> ---
>>   include/linux/of.h |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 276c546..24e1b28 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -67,7 +67,7 @@ struct device_node {
>>   #endif
>>   };
>>
>> -#define MAX_PHANDLE_ARGS 8
>> +#define MAX_PHANDLE_ARGS 16
>
>
> Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't
> this be changed to be able to support 128 StreamIDs as well?  Although I am
> not sure if this would be too big to have on the stack per Rob's comment in
> the previous patch set.

Do you actually need 128 now? If not, then we can deal with that when
we get there. There are lots of things in spec's that are not actually
implemented or supported.

Rob

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Suravee Suthikulanit]

On 1/29/2014 10:57 AM, Rob Herring wrote:
>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>> >>index 276c546..24e1b28 100644
>>> >>--- a/include/linux/of.h
>>> >>+++ b/include/linux/of.h
>>> >>@@ -67,7 +67,7 @@ struct device_node {
>>> >>   #endif
>>> >>   };
>>> >>
>>> >>-#define MAX_PHANDLE_ARGS 8
>>> >>+#define MAX_PHANDLE_ARGS 16
>> >
>> >
>> >Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't
>> >this be changed to be able to support 128 StreamIDs as well?  Although I am
>> >not sure if this would be too big to have on the stack per Rob's comment in
>> >the previous patch set.
> Do you actually need 128 now? If not, then we can deal with that when
> we get there. There are lots of things in spec's that are not actually
> implemented or supported.

Actually, we are using 32 on the AMD system. So, do you think we can set 
this to 32 instead?

Thanks,

Suravee

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Andreas Herrmann]

On Wed, Jan 29, 2014 at 11:59:12AM -0500, Suravee Suthikulanit wrote:
> On 1/29/2014 10:57 AM, Rob Herring wrote:
> >>> diff --git a/include/linux/of.h b/include/linux/of.h
> >>> >>index 276c546..24e1b28 100644
> >>> >>--- a/include/linux/of.h
> >>> >>+++ b/include/linux/of.h
> >>> >>@@ -67,7 +67,7 @@ struct device_node {
> >>> >>   #endif
> >>> >>   };
> >>> >>
> >>> >>-#define MAX_PHANDLE_ARGS 8
> >>> >>+#define MAX_PHANDLE_ARGS 16
> >> >
> >> >
> >> >Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't
> >> >this be changed to be able to support 128 StreamIDs as well?  Although I am
> >> >not sure if this would be too big to have on the stack per Rob's comment in
> >> >the previous patch set.
> > Do you actually need 128 now? If not, then we can deal with that when
> > we get there. There are lots of things in spec's that are not actually
> > implemented or supported.
> 
> Actually, we are using 32 on the AMD system. So, do you think we can set 
> this to 32 instead?

I think that's ok.

But are we really talking about number of SMRs or number of StreamIDs
per master device here? Ie. are you just having 32 SMRs for an SMMU on
your AMD system or do you have master devices which have 32 StreamIDs?

If it's just number of SMRs we don't need to modify this macro.


Andreas

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Suravee Suthikulanit]

On 1/29/2014 11:16 AM, Andreas Herrmann wrote:
> On Wed, Jan 29, 2014 at 11:59:12AM -0500, Suravee Suthikulanit wrote:
>> On 1/29/2014 10:57 AM, Rob Herring wrote:
>>>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>>>>>> index 276c546..24e1b28 100644
>>>>>>> --- a/include/linux/of.h
>>>>>>> +++ b/include/linux/of.h
>>>>>>> @@ -67,7 +67,7 @@ struct device_node {
>>>>>>>    #endif
>>>>>>>    };
>>>>>>>
>>>>>>> -#define MAX_PHANDLE_ARGS 8
>>>>>>> +#define MAX_PHANDLE_ARGS 16
>>>>>
>>>>>
>>>>> Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't
>>>>> this be changed to be able to support 128 StreamIDs as well?  Although I am
>>>>> not sure if this would be too big to have on the stack per Rob's comment in
>>>>> the previous patch set.
>>> Do you actually need 128 now? If not, then we can deal with that when
>>> we get there. There are lots of things in spec's that are not actually
>>> implemented or supported.
>>
>> Actually, we are using 32 on the AMD system. So, do you think we can set
>> this to 32 instead?
>
> I think that's ok.
>
> But are we really talking about number of SMRs or number of StreamIDs
> per master device here? Ie. are you just having 32 SMRs for an SMMU on
> your AMD system or do you have master devices which have 32 StreamIDs?
>
> If it's just number of SMRs we don't need to modify this macro.
>
>
> Andreas
>

I am referring to the case where each mmu-master can have upto 32 streamID.

Suravee

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Will Deacon]

On Wed, Jan 29, 2014 at 05:26:35PM +0000, Suravee Suthikulanit wrote:
> On 1/29/2014 11:16 AM, Andreas Herrmann wrote:
> > On Wed, Jan 29, 2014 at 11:59:12AM -0500, Suravee Suthikulanit wrote:
> >> Actually, we are using 32 on the AMD system. So, do you think we can set
> >> this to 32 instead?
> >
> > I think that's ok.
> >
> > But are we really talking about number of SMRs or number of StreamIDs
> > per master device here? Ie. are you just having 32 SMRs for an SMMU on
> > your AMD system or do you have master devices which have 32 StreamIDs?
> >
> > If it's just number of SMRs we don't need to modify this macro.
> >
> 
> I am referring to the case where each mmu-master can have upto 32 streamID.

Crikey, how many SMRs do you have? Andreas and I have been struggling to
write a decent allocator for those, so if you have any algorithms that don't
require a quantum computer, we'd love to hear from you :)!

Will

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Suravee Suthikulanit]

On 1/29/2014 11:29 AM, Will Deacon wrote:
> On Wed, Jan 29, 2014 at 05:26:35PM +0000, Suravee Suthikulanit wrote:
>> On 1/29/2014 11:16 AM, Andreas Herrmann wrote:
>>> On Wed, Jan 29, 2014 at 11:59:12AM -0500, Suravee Suthikulanit wrote:
>>>> Actually, we are using 32 on the AMD system. So, do you think we can set
>>>> this to 32 instead?
>>>
>>> I think that's ok.
>>>
>>> But are we really talking about number of SMRs or number of StreamIDs
>>> per master device here? Ie. are you just having 32 SMRs for an SMMU on
>>> your AMD system or do you have master devices which have 32 StreamIDs?
>>>
>>> If it's just number of SMRs we don't need to modify this macro.
>>>
>>
>> I am referring to the case where each mmu-master can have upto 32 streamID.
>
> Crikey, how many SMRs do you have? Andreas and I have been struggling to
> write a decent allocator for those, so if you have any algorithms that don't
> require a quantum computer, we'd love to hear from you :)!
>
> Will
>

Are you talking about the __arm_smmu_alloc_bitmap()?

Currently, we have configured the each SMMU to have 32 SMRs and using 
15-bit streamID. However, we mostly have upto 32 streamID for each 
master, and most of the SMMU only have one master.  So it looks like the 
current logic should be ok.

Suravee

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Will Deacon]

On Wed, Jan 29, 2014 at 05:57:16PM +0000, Suravee Suthikulanit wrote:
> On 1/29/2014 11:29 AM, Will Deacon wrote:
> > On Wed, Jan 29, 2014 at 05:26:35PM +0000, Suravee Suthikulanit wrote:
> >> On 1/29/2014 11:16 AM, Andreas Herrmann wrote:
> >>> On Wed, Jan 29, 2014 at 11:59:12AM -0500, Suravee Suthikulanit wrote:
> >>>> Actually, we are using 32 on the AMD system. So, do you think we can set
> >>>> this to 32 instead?
> >>>
> >>> I think that's ok.
> >>>
> >>> But are we really talking about number of SMRs or number of StreamIDs
> >>> per master device here? Ie. are you just having 32 SMRs for an SMMU on
> >>> your AMD system or do you have master devices which have 32 StreamIDs?
> >>>
> >>> If it's just number of SMRs we don't need to modify this macro.
> >>>
> >>
> >> I am referring to the case where each mmu-master can have upto 32 streamID.
> >
> > Crikey, how many SMRs do you have? Andreas and I have been struggling to
> > write a decent allocator for those, so if you have any algorithms that don't
> > require a quantum computer, we'd love to hear from you :)!
> >
> > Will
> >
> 
> Are you talking about the __arm_smmu_alloc_bitmap()?
> 
> Currently, we have configured the each SMMU to have 32 SMRs and using 
> 15-bit streamID. However, we mostly have upto 32 streamID for each 
> master, and most of the SMMU only have one master.  So it looks like the 
> current logic should be ok.

Interesting... how does that work for PCI? Do you force all devices behind a
given RC into the same address space?

Will

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Suravee Suthikulanit]

On 1/29/2014 12:03 PM, Will Deacon wrote:
> On Wed, Jan 29, 2014 at 05:57:16PM +0000, Suravee Suthikulanit wrote:
>> On 1/29/2014 11:29 AM, Will Deacon wrote:
>>> On Wed, Jan 29, 2014 at 05:26:35PM +0000, Suravee Suthikulanit wrote:
>>>> On 1/29/2014 11:16 AM, Andreas Herrmann wrote:
>>>>> On Wed, Jan 29, 2014 at 11:59:12AM -0500, Suravee Suthikulanit wrote:
>>>>>> Actually, we are using 32 on the AMD system. So, do you think we can set
>>>>>> this to 32 instead?
>>>>>
>>>>> I think that's ok.
>>>>>
>>>>> But are we really talking about number of SMRs or number of StreamIDs
>>>>> per master device here? Ie. are you just having 32 SMRs for an SMMU on
>>>>> your AMD system or do you have master devices which have 32 StreamIDs?
>>>>>
>>>>> If it's just number of SMRs we don't need to modify this macro.
>>>>>
>>>>
>>>> I am referring to the case where each mmu-master can have upto 32 streamID.
>>>
>>> Crikey, how many SMRs do you have? Andreas and I have been struggling to
>>> write a decent allocator for those, so if you have any algorithms that don't
>>> require a quantum computer, we'd love to hear from you :)!
>>>
>>> Will
>>>
>>
>> Are you talking about the __arm_smmu_alloc_bitmap()?
>>
>> Currently, we have configured the each SMMU to have 32 SMRs and using
>> 15-bit streamID. However, we mostly have upto 32 streamID for each
>> master, and most of the SMMU only have one master.  So it looks like the
>> current logic should be ok.
>
> Interesting... how does that work for PCI? Do you force all devices behind a
> given RC into the same address space?
>
> Will
>

For PCI devices, we are using the bus, device, and function id to make 
up the 15-bit SID for devices behind a particular PCI root complex.

I also notice that we are currently not supporting the streamID mask in 
the SMR.  Is this something planed for the future?

Suravee

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Will Deacon]

On Thu, Jan 30, 2014 at 10:53:35PM +0000, Suravee Suthikulanit wrote:
> On 1/29/2014 12:03 PM, Will Deacon wrote:
> > Interesting... how does that work for PCI? Do you force all devices behind a
> > given RC into the same address space?
> >
> 
> For PCI devices, we are using the bus, device, and function id to make 
> up the 15-bit SID for devices behind a particular PCI root complex.

Very good!

> I also notice that we are currently not supporting the streamID mask in 
> the SMR.  Is this something planed for the future?

Andreas and I are curently working on this -- that's what I was referring to
above. Any feedback from you would be welcomed.

Will

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Andreas Herrmann]

On Wed, Jan 29, 2014 at 12:26:35PM -0500, Suravee Suthikulanit wrote:
> On 1/29/2014 11:16 AM, Andreas Herrmann wrote:
> > On Wed, Jan 29, 2014 at 11:59:12AM -0500, Suravee Suthikulanit wrote:
> >> On 1/29/2014 10:57 AM, Rob Herring wrote:
> >>>>> diff --git a/include/linux/of.h b/include/linux/of.h
> >>>>>>> index 276c546..24e1b28 100644
> >>>>>>> --- a/include/linux/of.h
> >>>>>>> +++ b/include/linux/of.h
> >>>>>>> @@ -67,7 +67,7 @@ struct device_node {
> >>>>>>>    #endif
> >>>>>>>    };
> >>>>>>>
> >>>>>>> -#define MAX_PHANDLE_ARGS 8
> >>>>>>> +#define MAX_PHANDLE_ARGS 16
> >>>>>
> >>>>>
> >>>>> Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't
> >>>>> this be changed to be able to support 128 StreamIDs as well?  Although I am
> >>>>> not sure if this would be too big to have on the stack per Rob's comment in
> >>>>> the previous patch set.
> >>> Do you actually need 128 now? If not, then we can deal with that when
> >>> we get there. There are lots of things in spec's that are not actually
> >>> implemented or supported.
> >>
> >> Actually, we are using 32 on the AMD system. So, do you think we can set
> >> this to 32 instead?
> >
> > I think that's ok.
> >
> > But are we really talking about number of SMRs or number of StreamIDs
> > per master device here? Ie. are you just having 32 SMRs for an SMMU on
> > your AMD system or do you have master devices which have 32 StreamIDs?
> >
> > If it's just number of SMRs we don't need to modify this macro.

> I am referring to the case where each mmu-master can have upto 32 streamID.

Rob,

Do you agree on increasing MAX_PHANDLE_ARGS to 32?

Or should this be done when someone (e.g. Suravee) submits a DTS
update with an SMMU node description containing more than 16 stream
IDs for a master device?


Andreas

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Rob Herring]

On Thu, Jan 30, 2014 at 11:45 AM, Andreas Herrmann
<andreas.herrmann@calxeda.com> wrote:
> On Wed, Jan 29, 2014 at 12:26:35PM -0500, Suravee Suthikulanit wrote:
>> On 1/29/2014 11:16 AM, Andreas Herrmann wrote:
>> > On Wed, Jan 29, 2014 at 11:59:12AM -0500, Suravee Suthikulanit wrote:
>> >> On 1/29/2014 10:57 AM, Rob Herring wrote:
>> >>>>> diff --git a/include/linux/of.h b/include/linux/of.h
>> >>>>>>> index 276c546..24e1b28 100644
>> >>>>>>> --- a/include/linux/of.h
>> >>>>>>> +++ b/include/linux/of.h
>> >>>>>>> @@ -67,7 +67,7 @@ struct device_node {
>> >>>>>>>    #endif
>> >>>>>>>    };
>> >>>>>>>
>> >>>>>>> -#define MAX_PHANDLE_ARGS 8
>> >>>>>>> +#define MAX_PHANDLE_ARGS 16
>> >>>>>
>> >>>>>
>> >>>>> Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't
>> >>>>> this be changed to be able to support 128 StreamIDs as well?  Although I am
>> >>>>> not sure if this would be too big to have on the stack per Rob's comment in
>> >>>>> the previous patch set.
>> >>> Do you actually need 128 now? If not, then we can deal with that when
>> >>> we get there. There are lots of things in spec's that are not actually
>> >>> implemented or supported.
>> >>
>> >> Actually, we are using 32 on the AMD system. So, do you think we can set
>> >> this to 32 instead?
>> >
>> > I think that's ok.
>> >
>> > But are we really talking about number of SMRs or number of StreamIDs
>> > per master device here? Ie. are you just having 32 SMRs for an SMMU on
>> > your AMD system or do you have master devices which have 32 StreamIDs?
>> >
>> > If it's just number of SMRs we don't need to modify this macro.
>
>> I am referring to the case where each mmu-master can have upto 32 streamID.
>
> Rob,
>
> Do you agree on increasing MAX_PHANDLE_ARGS to 32?

Yes, but more than that will require a closer look. Please get this
into next early in the cycle.

> Or should this be done when someone (e.g. Suravee) submits a DTS
> update with an SMMU node description containing more than 16 stream
> IDs for a master device?

Well, I am inclined to not care having seen no upstream activity for
AMD's platform.

Rob

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Will Deacon]

On Fri, Jan 31, 2014 at 04:24:09PM +0000, Rob Herring wrote:
> On Thu, Jan 30, 2014 at 11:45 AM, Andreas Herrmann
> <andreas.herrmann@calxeda.com> wrote:
> > Do you agree on increasing MAX_PHANDLE_ARGS to 32?
> 
> Yes, but more than that will require a closer look. Please get this
> into next early in the cycle.

If you want to get this patch in early, I suggest taking it via a tree other
than mine. I don't put my iommu queue into -next, since it goes to Joerg
around -rc4, who then sticks it in after that.

Will

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Grant Likely]

On Wed, 29 Jan 2014 10:59:12 -0600, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
> On 1/29/2014 10:57 AM, Rob Herring wrote:
> >>> diff --git a/include/linux/of.h b/include/linux/of.h
> >>> >>index 276c546..24e1b28 100644
> >>> >>--- a/include/linux/of.h
> >>> >>+++ b/include/linux/of.h
> >>> >>@@ -67,7 +67,7 @@ struct device_node {
> >>> >>   #endif
> >>> >>   };
> >>> >>
> >>> >>-#define MAX_PHANDLE_ARGS 8
> >>> >>+#define MAX_PHANDLE_ARGS 16
> >> >
> >> >
> >> >Since the MMU-500 specify "Number of SMRs" upto 128 registers, shouldn't
> >> >this be changed to be able to support 128 StreamIDs as well?  Although I am
> >> >not sure if this would be too big to have on the stack per Rob's comment in
> >> >the previous patch set.
> > Do you actually need 128 now? If not, then we can deal with that when
> > we get there. There are lots of things in spec's that are not actually
> > implemented or supported.
> 
> Actually, we are using 32 on the AMD system. So, do you think we can set 
> this to 32 instead?

The helper really wasn't designed for large number of arguments to a
phandle. If the phandle args are really that large, then it may be
better to have a parser that allocates the space needed and/or puts the
data directly into the destination data structure.

g.

[PATCH v2 08/11] of: Increase MAX_PHANDLE_ARGS

[Grant Likely]

On Fri, 17 Jan 2014 12:08:30 +0100, Andreas Herrmann <andreas.herrmann@calxeda.com> wrote:
> 
> arm-smmu driver uses of_parse_phandle_with_args when parsing DT
> information to determine stream IDs for a master device.
> Thus the number of stream IDs per master device is bound by
> MAX_PHANDLE_ARGS.
> 
> To support Calxeda ECX-2000 hardware arm-smmu driver requires a
> slightly higher value for MAX_PHANDLE_ARGS as this hardware has 10
> stream IDs for one master device.
> 
> Increasing it to 16 seems a reasonable choice.
> 
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree at vger.kernel.org
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>

I've merged this one, but I'm not excited about making it any larger
because this structure lives on the stack most of the time.

g.

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

[Andreas Herrmann]

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
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 2ccbb57f..722de49 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>;
@@ -109,6 +110,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 0 1>;
+			calxeda,smmu-secure-config-access;
+			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 0 1>;
+			calxeda,smmu-secure-config-access;
+			arm,smmu-isolate-devices;
+		};
+
+		smmu_sata: smmu at 920180000 {
+			compatible = "arm,mmu-400";
+			reg = <0x00000009 0x20180000 0x10000>;
+			mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
+			#global-interrupts = <1>;
+			interrupts = <0 114 4 0 114 4>;
+			calxeda,smmu-secure-config-access;
+			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 b90045a..ad9b2fd 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -33,7 +33,7 @@
 		compatible = "simple-bus";
 		interrupt-parent = <&intc>;
 
-		sata at ffe08000 {
+		sata: sata at ffe08000 {
 			compatible = "calxeda,hb-ahci";
 			reg = <0xffe08000 0x10000>;
 			interrupts = <0 83 4>;
@@ -43,6 +43,7 @@
 					     &combophy0 3>;
 			calxeda,sgpio-gpio =<&gpioh 5 1 &gpioh 6 1 &gpioh 7 1>;
 			calxeda,led-order = <4 0 1 2 3>;
+			#stream-id-cells = <10>;
 		};
 
 		sdhci at ffe0e000 {
@@ -210,18 +211,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

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

[Rob Herring]

On Thu, Jan 16, 2014 at 6:44 AM, Andreas Herrmann
<andreas.herrmann@calxeda.com> wrote:
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>

One minor comment, but otherwise:

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  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 2ccbb57f..722de49 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>;
> @@ -109,6 +110,45 @@
>                         interrupts = <0 76 4  0 75 4  0 74 4  0 73 4>;
>                 };
>         };
> +
> +       soc at 920000000 {

I believe this and the other > 32-bit addresses below should be
"soc at 9,20000000".

> +               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 0 1>;
> +                       calxeda,smmu-secure-config-access;
> +                       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 0 1>;
> +                       calxeda,smmu-secure-config-access;
> +                       arm,smmu-isolate-devices;
> +               };
> +
> +               smmu_sata: smmu at 920180000 {
> +                       compatible = "arm,mmu-400";
> +                       reg = <0x00000009 0x20180000 0x10000>;
> +                       mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
> +                       #global-interrupts = <1>;
> +                       interrupts = <0 114 4 0 114 4>;
> +                       calxeda,smmu-secure-config-access;
> +                       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 b90045a..ad9b2fd 100644
> --- a/arch/arm/boot/dts/ecx-common.dtsi
> +++ b/arch/arm/boot/dts/ecx-common.dtsi
> @@ -33,7 +33,7 @@
>                 compatible = "simple-bus";
>                 interrupt-parent = <&intc>;
>
> -               sata at ffe08000 {
> +               sata: sata at ffe08000 {
>                         compatible = "calxeda,hb-ahci";
>                         reg = <0xffe08000 0x10000>;
>                         interrupts = <0 83 4>;
> @@ -43,6 +43,7 @@
>                                              &combophy0 3>;
>                         calxeda,sgpio-gpio =<&gpioh 5 1 &gpioh 6 1 &gpioh 7 1>;
>                         calxeda,led-order = <4 0 1 2 3>;
> +                       #stream-id-cells = <10>;
>                 };
>
>                 sdhci at ffe0e000 {
> @@ -210,18 +211,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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andreas Herrmann]

On Thu, Jan 16, 2014 at 09:30:26AM -0500, Rob Herring wrote:
> On Thu, Jan 16, 2014 at 6:44 AM, Andreas Herrmann
> <andreas.herrmann@calxeda.com> wrote:
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> 
> One minor comment, but otherwise:
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
> > ---
> >  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 2ccbb57f..722de49 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>;
> > @@ -109,6 +110,45 @@
> >                         interrupts = <0 76 4  0 75 4  0 74 4  0 73 4>;
> >                 };
> >         };
> > +
> > +       soc at 920000000 {
> 
> I believe this and the other > 32-bit addresses below should be
> "soc at 9,20000000".

Will fix this.


Thanks,

Andreas

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

[Andreas Herrmann]

Add nodes for SMMUs on Calxeda ECX-2000.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Acked-by: Rob Herring <robh@kernel.org>
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 2ccbb57f..dde23bb 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>;
@@ -109,6 +110,45 @@
 			interrupts = <0 76 4  0 75 4  0 74 4  0 73 4>;
 		};
 	};
+
+	soc at 9,20000000 {
+		ranges = <0x9 0x20000000 0x9 0x20000000 0x290000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		interrupt-parent = <&intc>;
+
+		smmu_mac0: smmu at 9,20000000 {
+			compatible = "arm,mmu-400";
+			reg = <0x9 0x20000000 0x10000>;
+			#global-interrupts = <1>;
+			interrupts = <0 106 4 0 106 4>;
+			mmu-masters = <&mac0 0 1>;
+			calxeda,smmu-secure-config-access;
+			arm,smmu-isolate-devices;
+		};
+
+		smmu_mac1: smmu at 9,20080000 {
+			compatible = "arm,mmu-400";
+			reg = <0x9 0x20080000 0x10000>;
+			#global-interrupts = <1>;
+			interrupts = <0 108 4 0 108 4>;
+			mmu-masters = <&mac1 0 1>;
+			calxeda,smmu-secure-config-access;
+			arm,smmu-isolate-devices;
+		};
+
+		smmu_sata: smmu at 9,20180000 {
+			compatible = "arm,mmu-400";
+			reg = <0x9 0x20180000 0x10000>;
+			mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
+			#global-interrupts = <1>;
+			interrupts = <0 114 4 0 114 4>;
+			calxeda,smmu-secure-config-access;
+			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 b90045a..ad9b2fd 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -33,7 +33,7 @@
 		compatible = "simple-bus";
 		interrupt-parent = <&intc>;
 
-		sata at ffe08000 {
+		sata: sata at ffe08000 {
 			compatible = "calxeda,hb-ahci";
 			reg = <0xffe08000 0x10000>;
 			interrupts = <0 83 4>;
@@ -43,6 +43,7 @@
 					     &combophy0 3>;
 			calxeda,sgpio-gpio =<&gpioh 5 1 &gpioh 6 1 &gpioh 7 1>;
 			calxeda,led-order = <4 0 1 2 3>;
+			#stream-id-cells = <10>;
 		};
 
 		sdhci at ffe0e000 {
@@ -210,18 +211,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

[PATCH 10/11] arm: dma-mapping: Add additional parameters to arm_iommu_create_mapping

[Andreas Herrmann]

The new parameters are

  dma_addr_t grow_size

         Specifies the size by which the mapping will be extended in
         case that no sufficient space is left in the mapping to
         handle an iova allocation request. If a grow_size of 0 is
         specified the mapping is not extended.

  dma_addr_t max_size

         Specifies the maximum size for the entire mapping --
         including all extensions made over time. The mapping can only
         be extended if the entire size is less than or equal to
         max_size.

Adapt existing calls to arm_iommu_create_mapping in exynos_drm_iommu.c
and shmobile-iommu.c such that they do not make use of the extension
feature.

Adapt existing call to arm_iommu_create_mapping in arm-smmu.c such
that the extension feature will be used.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Hiroshi Doyu <hdoyu@nvidia.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 arch/arm/include/asm/dma-iommu.h          |    4 ++--
 arch/arm/mm/dma-mapping.c                 |    2 +-
 drivers/gpu/drm/exynos/exynos_drm_iommu.c |    4 ++--
 drivers/iommu/arm-smmu.c                  |    2 +-
 drivers/iommu/shmobile-iommu.c            |    2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
index a8c56ac..50edacd 100644
--- a/arch/arm/include/asm/dma-iommu.h
+++ b/arch/arm/include/asm/dma-iommu.h
@@ -23,8 +23,8 @@ struct dma_iommu_mapping {
 };
 
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
-			 int order);
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base,  size_t size,
+			int order, dma_addr_t grow_size, dma_addr_t max_size);
 
 void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
 
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index f61a570..ccea46a 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1887,7 +1887,7 @@ struct dma_map_ops iommu_coherent_ops = {
  */
 struct dma_iommu_mapping *
 arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
-			 int order)
+			int order, dma_addr_t grow_size, dma_addr_t max_size)
 {
 	unsigned int count = size >> (PAGE_SHIFT + order);
 	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_iommu.c b/drivers/gpu/drm/exynos/exynos_drm_iommu.c
index fb8db03..c1cd18b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_iommu.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_iommu.c
@@ -40,8 +40,8 @@ int drm_create_iommu_mapping(struct drm_device *drm_dev)
 		priv->da_space_order = EXYNOS_DEV_ADDR_ORDER;
 
 	mapping = arm_iommu_create_mapping(&platform_bus_type, priv->da_start,
-						priv->da_space_size,
-						priv->da_space_order);
+					priv->da_space_size,
+					priv->da_space_order, 0, 0);
 	if (IS_ERR(mapping))
 		return PTR_ERR(mapping);
 
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 68bbe45..31414e5 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2124,7 +2124,7 @@ static int arm_smmu_device_notifier(struct notifier_block *nb,
 			break;
 
 		mapping = arm_iommu_create_mapping(&platform_bus_type,
-						0, SZ_128M, 0);
+						0, SZ_128M, 0, SZ_128M, SZ_2G);
 		if (IS_ERR(mapping)) {
 			ret = PTR_ERR(mapping);
 			dev_info(dev, "arm_iommu_create_mapping failed\n");
diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
index d572863..1ba3a01 100644
--- a/drivers/iommu/shmobile-iommu.c
+++ b/drivers/iommu/shmobile-iommu.c
@@ -343,7 +343,7 @@ static int shmobile_iommu_add_device(struct device *dev)
 	mapping = archdata->iommu_mapping;
 	if (!mapping) {
 		mapping = arm_iommu_create_mapping(&platform_bus_type, 0,
-						   L1_LEN << 20, 0);
+						L1_LEN << 20, 0, 0, 0);
 		if (IS_ERR(mapping))
 			return PTR_ERR(mapping);
 		archdata->iommu_mapping = mapping;
-- 
1.7.9.5

[PATCH 10/11] arm: dma-mapping: Add additional parameters to arm_iommu_create_mapping

[Will Deacon]

On Thu, Jan 16, 2014 at 12:44:22PM +0000, Andreas Herrmann wrote:
> The new parameters are
> 
>   dma_addr_t grow_size
> 
>          Specifies the size by which the mapping will be extended in
>          case that no sufficient space is left in the mapping to
>          handle an iova allocation request. If a grow_size of 0 is
>          specified the mapping is not extended.

How can an IOMMU know what a sensible value is for grow size? I can only
think of two cases:

  (1) Growing is not permitted at all (0 in your current API)
  (2) Growing is permitted up to the maximum input address size of the
      IOMMU.

Maybe the latter should be probable, rather than waiting to get -ERANGE from
iommu_map? Actually, if we assume that the DMA master can't master a wider
address range the its IOMMU, then we could construct the grow_size from the
base and the dma mask already.

Will

[PATCH 11/11] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Andreas Herrmann]

Instead of using just one bitmap to keep track of IO virtual addresses
(handed out for IOMMU use) introduce a list of iova_ranges (each
having its own bitmap). This allows us to extend existing mappings
when running out of iova space for a mapping.

If there is not enough space in the mapping to service an IO virtual
address allocation request, __alloc_iova() tries to extend the mapping
-- by allocating another bitmap -- and makes another allocation
attempt using the freshly allocated bitmap.

This allows arm iommu drivers to start with a decent initial size when
an dma_iommu_mapping is created and still to avoid running out of IO
virtual addresses for the mapping.

Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
I've used SZ_512K both for initial mapping size and grow_size.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Hiroshi Doyu <hdoyu@nvidia.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 arch/arm/include/asm/dma-iommu.h |   17 ++++-
 arch/arm/mm/dma-mapping.c        |  147 ++++++++++++++++++++++++++++++++------
 2 files changed, 139 insertions(+), 25 deletions(-)

diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
index 50edacd..987d62c 100644
--- a/arch/arm/include/asm/dma-iommu.h
+++ b/arch/arm/include/asm/dma-iommu.h
@@ -8,15 +8,26 @@
 #include <linux/dma-debug.h>
 #include <linux/kmemcheck.h>
 #include <linux/kref.h>
+#include <linux/list.h>
+
+struct dma_iommu_iova_range {
+	struct list_head	list_head;
+	unsigned long		*bitmap;
+	size_t			bits;
+	dma_addr_t		base;
+	dma_addr_t		size;
+};
 
 struct dma_iommu_mapping {
 	/* iommu specific data */
 	struct iommu_domain	*domain;
 
-	void			*bitmap;
-	size_t			bits;
-	unsigned int		order;
+	struct list_head	iova_ranges;
 	dma_addr_t		base;
+	dma_addr_t		size;
+	dma_addr_t		grow_size;
+	dma_addr_t		max_size;
+	unsigned int		order;
 
 	spinlock_t		lock;
 	struct kref		kref;
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ccea46a..503e8d6 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -26,6 +26,7 @@
 #include <linux/io.h>
 #include <linux/vmalloc.h>
 #include <linux/sizes.h>
+#include <linux/list.h>
 
 #include <asm/memory.h>
 #include <asm/highmem.h>
@@ -1069,6 +1070,8 @@ fs_initcall(dma_debug_do_init);
 
 /* IOMMU */
 
+static int extend_iommu_mapping(struct dma_iommu_mapping *mapping);
+
 static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
 				      size_t size)
 {
@@ -1076,6 +1079,8 @@ static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
 	unsigned int align = 0;
 	unsigned int count, start;
 	unsigned long flags;
+	struct dma_iommu_iova_range *e;
+	bool area_found;
 
 	if (order > CONFIG_ARM_DMA_IOMMU_ALIGNMENT)
 		order = CONFIG_ARM_DMA_IOMMU_ALIGNMENT;
@@ -1086,32 +1091,80 @@ static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
 	if (order > mapping->order)
 		align = (1 << (order - mapping->order)) - 1;
 
+	area_found = false;
 	spin_lock_irqsave(&mapping->lock, flags);
-	start = bitmap_find_next_zero_area(mapping->bitmap, mapping->bits, 0,
-					   count, align);
-	if (start > mapping->bits) {
-		spin_unlock_irqrestore(&mapping->lock, flags);
-		return DMA_ERROR_CODE;
+	list_for_each_entry(e, &mapping->iova_ranges, list_head) {
+		start = bitmap_find_next_zero_area(e->bitmap, e->bits, 0,
+						count, align);
+		if (start > e->bits)
+			continue;
+
+		bitmap_set(e->bitmap, start, count);
+		area_found = true;
+		break;
 	}
 
-	bitmap_set(mapping->bitmap, start, count);
+	/*
+	 * Try to extend the existing mapping and perform a second
+	 * attempt to reserve an IO virtual address range of size
+	 * bytes.
+	 */
+	if (!area_found) {
+		if (extend_iommu_mapping(mapping)) {
+			spin_unlock_irqrestore(&mapping->lock, flags);
+			return DMA_ERROR_CODE;
+		}
+		e = list_entry(mapping->iova_ranges.prev,
+			struct dma_iommu_iova_range, list_head);
+		start = bitmap_find_next_zero_area(e->bitmap, e->bits, 0,
+						count, align);
+		if (start > e->bits) {
+			spin_unlock_irqrestore(&mapping->lock, flags);
+			return DMA_ERROR_CODE;
+		}
+		bitmap_set(e->bitmap, start, count);
+	}
 	spin_unlock_irqrestore(&mapping->lock, flags);
 
-	return mapping->base + (start << (mapping->order + PAGE_SHIFT));
+	return e->base + (start << (mapping->order + PAGE_SHIFT));
 }
 
 static inline void __free_iova(struct dma_iommu_mapping *mapping,
 			       dma_addr_t addr, size_t size)
 {
-	unsigned int start = (addr - mapping->base) >>
-			     (mapping->order + PAGE_SHIFT);
-	unsigned int count = ((size >> PAGE_SHIFT) +
-			      (1 << mapping->order) - 1) >> mapping->order;
+	struct dma_iommu_iova_range *e;
+	unsigned int start, count, tmp;
 	unsigned long flags;
 
-	spin_lock_irqsave(&mapping->lock, flags);
-	bitmap_clear(mapping->bitmap, start, count);
-	spin_unlock_irqrestore(&mapping->lock, flags);
+	list_for_each_entry(e, &mapping->iova_ranges, list_head) {
+		if (!size)
+			break;
+		if ((addr < e->base) || (addr >= e->base + e->size))
+			continue;
+
+		start = (addr - e->base) >> (mapping->order + PAGE_SHIFT);
+		if (addr + size > e->base + e->size) {
+			/*
+			 * The address range to be freed crosses an
+			 * iova_range boundary.
+			 * Hence calc count parameter to fit within
+			 * current iova_range and prepare addr and
+			 * size for next iteration.
+			 */
+			tmp = (e->base + e->size) - addr;
+			count = ((tmp >> PAGE_SHIFT) +
+				(1 << mapping->order) - 1) >> mapping->order;
+			size -= tmp;
+			addr += tmp;
+		} else {
+			count = ((size >> PAGE_SHIFT) +
+				(1 << mapping->order) - 1) >> mapping->order;
+			size -= size;
+		}
+		spin_lock_irqsave(&mapping->lock, flags);
+		bitmap_clear(e->bitmap, start, count);
+		spin_unlock_irqrestore(&mapping->lock, flags);
+	}
 }
 
 static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
@@ -1892,6 +1945,7 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
 	unsigned int count = size >> (PAGE_SHIFT + order);
 	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
 	struct dma_iommu_mapping *mapping;
+	struct dma_iommu_iova_range *iovar;
 	int err = -ENOMEM;
 
 	if (!count)
@@ -1901,23 +1955,37 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
 	if (!mapping)
 		goto err;
 
-	mapping->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
-	if (!mapping->bitmap)
+	INIT_LIST_HEAD(&mapping->iova_ranges);
+	spin_lock_init(&mapping->lock);
+
+	iovar = kzalloc(sizeof(struct dma_iommu_iova_range), GFP_KERNEL);
+	if (!iovar)
 		goto err2;
 
-	mapping->base = base;
-	mapping->bits = BITS_PER_BYTE * bitmap_size;
+	iovar->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
+	if (!iovar->bitmap)
+		goto err3;
+
+	iovar->bits = BITS_PER_BYTE * bitmap_size;
+	list_add_tail(&iovar->list_head, &mapping->iova_ranges);
+
+	mapping->base = iovar->base = base;
+	mapping->size = iovar->size = size;
+
 	mapping->order = order;
-	spin_lock_init(&mapping->lock);
+	mapping->grow_size = grow_size;
+	mapping->max_size = max_size;
 
 	mapping->domain = iommu_domain_alloc(bus);
 	if (!mapping->domain)
-		goto err3;
+		goto err4;
 
 	kref_init(&mapping->kref);
 	return mapping;
+err4:
+	kfree(iovar->bitmap);
 err3:
-	kfree(mapping->bitmap);
+	kfree(iovar);
 err2:
 	kfree(mapping);
 err:
@@ -1927,14 +1995,49 @@ EXPORT_SYMBOL_GPL(arm_iommu_create_mapping);
 
 static void release_iommu_mapping(struct kref *kref)
 {
+	struct dma_iommu_iova_range *e, *tmp;
 	struct dma_iommu_mapping *mapping =
 		container_of(kref, struct dma_iommu_mapping, kref);
 
 	iommu_domain_free(mapping->domain);
-	kfree(mapping->bitmap);
+	list_for_each_entry_safe(e, tmp, &mapping->iova_ranges, list_head) {
+		list_del(&e->list_head);
+		kfree(e->bitmap);
+		kfree(e);
+	}
 	kfree(mapping);
 }
 
+static int extend_iommu_mapping(struct dma_iommu_mapping *mapping)
+{
+	struct dma_iommu_iova_range *iovar;
+	unsigned int count = mapping->grow_size >> (PAGE_SHIFT + mapping->order);
+	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
+
+	if (!mapping->grow_size ||
+		(mapping->size + mapping->grow_size) >= mapping->max_size)
+		return -EINVAL;
+
+	iovar = kzalloc(sizeof(struct dma_iommu_iova_range), GFP_ATOMIC);
+	if (!iovar)
+		return -ENOMEM;
+
+	iovar->bitmap = kzalloc(bitmap_size, GFP_ATOMIC);
+	if (!iovar->bitmap) {
+		kfree(iovar);
+		return -ENOMEM;
+	}
+
+	iovar->bits = BITS_PER_BYTE * bitmap_size;
+	iovar->base = mapping->base + mapping->size;
+	iovar->size = mapping->grow_size;
+
+	mapping->size += mapping->grow_size;
+	list_add_tail(&iovar->list_head, &mapping->iova_ranges);
+
+	return 0;
+}
+
 void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping)
 {
 	if (mapping)
-- 
1.7.9.5

[PATCH 11/11] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Will Deacon]

On Thu, Jan 16, 2014 at 12:44:23PM +0000, Andreas Herrmann wrote:
> Instead of using just one bitmap to keep track of IO virtual addresses
> (handed out for IOMMU use) introduce a list of iova_ranges (each
> having its own bitmap). This allows us to extend existing mappings
> when running out of iova space for a mapping.
> 
> If there is not enough space in the mapping to service an IO virtual
> address allocation request, __alloc_iova() tries to extend the mapping
> -- by allocating another bitmap -- and makes another allocation
> attempt using the freshly allocated bitmap.
> 
> This allows arm iommu drivers to start with a decent initial size when
> an dma_iommu_mapping is created and still to avoid running out of IO
> virtual addresses for the mapping.
> 
> Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
> I've used SZ_512K both for initial mapping size and grow_size.

Aha, I thought grow_size was the *maximum* size, rather than the incremental
size. In which case, you probably want to pick the maximum supported IOMMU
page size that satisfies a fixed limit on the bitmap size.

> +static int extend_iommu_mapping(struct dma_iommu_mapping *mapping)
> +{
> +	struct dma_iommu_iova_range *iovar;
> +	unsigned int count = mapping->grow_size >> (PAGE_SHIFT + mapping->order);
> +	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
> +
> +	if (!mapping->grow_size ||
> +		(mapping->size + mapping->grow_size) >= mapping->max_size)
> +		return -EINVAL;
> +
> +	iovar = kzalloc(sizeof(struct dma_iommu_iova_range), GFP_ATOMIC);
> +	if (!iovar)
> +		return -ENOMEM;
> +
> +	iovar->bitmap = kzalloc(bitmap_size, GFP_ATOMIC);

Do these allocation really need to be atomic? I worry that's going to have
severe restrictions on our ability to allocate large address spaces.

Will

[PATCH 11/11] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Andreas Herrmann]

On Wed, Jan 22, 2014 at 04:10:11PM +0000, Will Deacon wrote:
> On Thu, Jan 16, 2014 at 12:44:23PM +0000, Andreas Herrmann wrote:
> > Instead of using just one bitmap to keep track of IO virtual addresses
> > (handed out for IOMMU use) introduce a list of iova_ranges (each
> > having its own bitmap). This allows us to extend existing mappings
> > when running out of iova space for a mapping.
> > 
> > If there is not enough space in the mapping to service an IO virtual
> > address allocation request, __alloc_iova() tries to extend the mapping
> > -- by allocating another bitmap -- and makes another allocation
> > attempt using the freshly allocated bitmap.
> > 
> > This allows arm iommu drivers to start with a decent initial size when
> > an dma_iommu_mapping is created and still to avoid running out of IO
> > virtual addresses for the mapping.
> > 
> > Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
> > I've used SZ_512K both for initial mapping size and grow_size.
> 
> Aha, I thought grow_size was the *maximum* size, rather than the incremental
> size. In which case, you probably want to pick the maximum supported IOMMU
> page size that satisfies a fixed limit on the bitmap size.
> 
> > +static int extend_iommu_mapping(struct dma_iommu_mapping *mapping)
> > +{
> > +	struct dma_iommu_iova_range *iovar;
> > +	unsigned int count = mapping->grow_size >> (PAGE_SHIFT + mapping->order);
> > +	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
> > +
> > +	if (!mapping->grow_size ||
> > +		(mapping->size + mapping->grow_size) >= mapping->max_size)
> > +		return -EINVAL;
> > +
> > +	iovar = kzalloc(sizeof(struct dma_iommu_iova_range), GFP_ATOMIC);
> > +	if (!iovar)
> > +		return -ENOMEM;
> > +
> > +	iovar->bitmap = kzalloc(bitmap_size, GFP_ATOMIC);
> 
> Do these allocation really need to be atomic? I worry that's going to have
> severe restrictions on our ability to allocate large address spaces.

Say some code acquired a lock before calling dma_map_single.
If we have to extend the mapping in such a path we should not s?eep.


Andreas

[PATCH 11/11] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Marek Szyprowski]

Hello,

I'm sorry for so late reply, but I was quite busy with other stuff.

On 2014-01-16 13:44, Andreas Herrmann wrote:
> Instead of using just one bitmap to keep track of IO virtual addresses
> (handed out for IOMMU use) introduce a list of iova_ranges (each
> having its own bitmap). This allows us to extend existing mappings
> when running out of iova space for a mapping.
>
> If there is not enough space in the mapping to service an IO virtual
> address allocation request, __alloc_iova() tries to extend the mapping
> -- by allocating another bitmap -- and makes another allocation
> attempt using the freshly allocated bitmap.
>
> This allows arm iommu drivers to start with a decent initial size when
> an dma_iommu_mapping is created and still to avoid running out of IO
> virtual addresses for the mapping.
>
> Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
> I've used SZ_512K both for initial mapping size and grow_size.

Thanks for implementing this feature! I remember it was discussed from
early beginning of arm dma iommu support, but I never had enough time
to actually implement it. I briefly checked the code and it look fine,
however I really wonder if we need separate grow_size parameter?
Personally I would simplify it to simply grow the bitmap by initial
size until it reaches the maximal size.

The whole concept of the simplified bitmap (where 1 bit != 1 page) for
iova allocation is a specific feature of this code and it has nothing
to the hardware. After thinking a bit more on the existing
implementation I've already observed that it is sometimes hard to
understand the parameters for arm_iommu_create_mapping() function,
especially the 'order' argument is ofter misunderstood. With your
patch we got two additional parameters. Maybe it will be much better
to use only 2 arguments: max_mapping_size and allocation_accuracy.
The initial bitmap size can be then calculated to fit it into single
memory page (that's quite important to avoid allocations larger that
a single memory page). 'allocation_accuracy' will serve the same way
as 'order' parameter now (but expressed in bytes rather than being
the multiplier for the number of pages). This way the
arm_iommu_create_mapping() function should be much easier to
understand, while keeping the implementation details hidden from the
caller.

> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
> Cc: Hiroshi Doyu <hdoyu@nvidia.com>
> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>   arch/arm/include/asm/dma-iommu.h |   17 ++++-
>   arch/arm/mm/dma-mapping.c        |  147 ++++++++++++++++++++++++++++++++------
>   2 files changed, 139 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
> index 50edacd..987d62c 100644
> --- a/arch/arm/include/asm/dma-iommu.h
> +++ b/arch/arm/include/asm/dma-iommu.h
> @@ -8,15 +8,26 @@
>   #include <linux/dma-debug.h>
>   #include <linux/kmemcheck.h>
>   #include <linux/kref.h>
> +#include <linux/list.h>
> +
> +struct dma_iommu_iova_range {
> +	struct list_head	list_head;
> +	unsigned long		*bitmap;
> +	size_t			bits;
> +	dma_addr_t		base;
> +	dma_addr_t		size;
> +};
>   
>   struct dma_iommu_mapping {
>   	/* iommu specific data */
>   	struct iommu_domain	*domain;
>   
> -	void			*bitmap;
> -	size_t			bits;
> -	unsigned int		order;
> +	struct list_head	iova_ranges;
>   	dma_addr_t		base;
> +	dma_addr_t		size;
> +	dma_addr_t		grow_size;
> +	dma_addr_t		max_size;
> +	unsigned int		order;
>   
>   	spinlock_t		lock;
>   	struct kref		kref;
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index ccea46a..503e8d6 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -26,6 +26,7 @@
>   #include <linux/io.h>
>   #include <linux/vmalloc.h>
>   #include <linux/sizes.h>
> +#include <linux/list.h>
>   
>   #include <asm/memory.h>
>   #include <asm/highmem.h>
> @@ -1069,6 +1070,8 @@ fs_initcall(dma_debug_do_init);
>   
>   /* IOMMU */
>   
> +static int extend_iommu_mapping(struct dma_iommu_mapping *mapping);
> +
>   static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
>   				      size_t size)
>   {
> @@ -1076,6 +1079,8 @@ static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
>   	unsigned int align = 0;
>   	unsigned int count, start;
>   	unsigned long flags;
> +	struct dma_iommu_iova_range *e;
> +	bool area_found;
>   
>   	if (order > CONFIG_ARM_DMA_IOMMU_ALIGNMENT)
>   		order = CONFIG_ARM_DMA_IOMMU_ALIGNMENT;
> @@ -1086,32 +1091,80 @@ static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
>   	if (order > mapping->order)
>   		align = (1 << (order - mapping->order)) - 1;
>   
> +	area_found = false;
>   	spin_lock_irqsave(&mapping->lock, flags);
> -	start = bitmap_find_next_zero_area(mapping->bitmap, mapping->bits, 0,
> -					   count, align);
> -	if (start > mapping->bits) {
> -		spin_unlock_irqrestore(&mapping->lock, flags);
> -		return DMA_ERROR_CODE;
> +	list_for_each_entry(e, &mapping->iova_ranges, list_head) {
> +		start = bitmap_find_next_zero_area(e->bitmap, e->bits, 0,
> +						count, align);
> +		if (start > e->bits)
> +			continue;
> +
> +		bitmap_set(e->bitmap, start, count);
> +		area_found = true;
> +		break;
>   	}
>   
> -	bitmap_set(mapping->bitmap, start, count);
> +	/*
> +	 * Try to extend the existing mapping and perform a second
> +	 * attempt to reserve an IO virtual address range of size
> +	 * bytes.
> +	 */
> +	if (!area_found) {
> +		if (extend_iommu_mapping(mapping)) {
> +			spin_unlock_irqrestore(&mapping->lock, flags);
> +			return DMA_ERROR_CODE;
> +		}
> +		e = list_entry(mapping->iova_ranges.prev,
> +			struct dma_iommu_iova_range, list_head);
> +		start = bitmap_find_next_zero_area(e->bitmap, e->bits, 0,
> +						count, align);
> +		if (start > e->bits) {
> +			spin_unlock_irqrestore(&mapping->lock, flags);
> +			return DMA_ERROR_CODE;
> +		}
> +		bitmap_set(e->bitmap, start, count);
> +	}
>   	spin_unlock_irqrestore(&mapping->lock, flags);
>   
> -	return mapping->base + (start << (mapping->order + PAGE_SHIFT));
> +	return e->base + (start << (mapping->order + PAGE_SHIFT));
>   }
>   
>   static inline void __free_iova(struct dma_iommu_mapping *mapping,
>   			       dma_addr_t addr, size_t size)
>   {
> -	unsigned int start = (addr - mapping->base) >>
> -			     (mapping->order + PAGE_SHIFT);
> -	unsigned int count = ((size >> PAGE_SHIFT) +
> -			      (1 << mapping->order) - 1) >> mapping->order;
> +	struct dma_iommu_iova_range *e;
> +	unsigned int start, count, tmp;
>   	unsigned long flags;
>   
> -	spin_lock_irqsave(&mapping->lock, flags);
> -	bitmap_clear(mapping->bitmap, start, count);
> -	spin_unlock_irqrestore(&mapping->lock, flags);
> +	list_for_each_entry(e, &mapping->iova_ranges, list_head) {
> +		if (!size)
> +			break;
> +		if ((addr < e->base) || (addr >= e->base + e->size))
> +			continue;
> +
> +		start = (addr - e->base) >> (mapping->order + PAGE_SHIFT);
> +		if (addr + size > e->base + e->size) {
> +			/*
> +			 * The address range to be freed crosses an
> +			 * iova_range boundary.
> +			 * Hence calc count parameter to fit within
> +			 * current iova_range and prepare addr and
> +			 * size for next iteration.
> +			 */
> +			tmp = (e->base + e->size) - addr;
> +			count = ((tmp >> PAGE_SHIFT) +
> +				(1 << mapping->order) - 1) >> mapping->order;
> +			size -= tmp;
> +			addr += tmp;
> +		} else {
> +			count = ((size >> PAGE_SHIFT) +
> +				(1 << mapping->order) - 1) >> mapping->order;
> +			size -= size;
> +		}
> +		spin_lock_irqsave(&mapping->lock, flags);
> +		bitmap_clear(e->bitmap, start, count);
> +		spin_unlock_irqrestore(&mapping->lock, flags);
> +	}
>   }
>   
>   static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
> @@ -1892,6 +1945,7 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
>   	unsigned int count = size >> (PAGE_SHIFT + order);
>   	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
>   	struct dma_iommu_mapping *mapping;
> +	struct dma_iommu_iova_range *iovar;
>   	int err = -ENOMEM;
>   
>   	if (!count)
> @@ -1901,23 +1955,37 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
>   	if (!mapping)
>   		goto err;
>   
> -	mapping->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> -	if (!mapping->bitmap)
> +	INIT_LIST_HEAD(&mapping->iova_ranges);
> +	spin_lock_init(&mapping->lock);
> +
> +	iovar = kzalloc(sizeof(struct dma_iommu_iova_range), GFP_KERNEL);
> +	if (!iovar)
>   		goto err2;
>   
> -	mapping->base = base;
> -	mapping->bits = BITS_PER_BYTE * bitmap_size;
> +	iovar->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> +	if (!iovar->bitmap)
> +		goto err3;
> +
> +	iovar->bits = BITS_PER_BYTE * bitmap_size;
> +	list_add_tail(&iovar->list_head, &mapping->iova_ranges);
> +
> +	mapping->base = iovar->base = base;
> +	mapping->size = iovar->size = size;
> +
>   	mapping->order = order;
> -	spin_lock_init(&mapping->lock);
> +	mapping->grow_size = grow_size;
> +	mapping->max_size = max_size;
>   
>   	mapping->domain = iommu_domain_alloc(bus);
>   	if (!mapping->domain)
> -		goto err3;
> +		goto err4;
>   
>   	kref_init(&mapping->kref);
>   	return mapping;
> +err4:
> +	kfree(iovar->bitmap);
>   err3:
> -	kfree(mapping->bitmap);
> +	kfree(iovar);
>   err2:
>   	kfree(mapping);
>   err:
> @@ -1927,14 +1995,49 @@ EXPORT_SYMBOL_GPL(arm_iommu_create_mapping);
>   
>   static void release_iommu_mapping(struct kref *kref)
>   {
> +	struct dma_iommu_iova_range *e, *tmp;
>   	struct dma_iommu_mapping *mapping =
>   		container_of(kref, struct dma_iommu_mapping, kref);
>   
>   	iommu_domain_free(mapping->domain);
> -	kfree(mapping->bitmap);
> +	list_for_each_entry_safe(e, tmp, &mapping->iova_ranges, list_head) {
> +		list_del(&e->list_head);
> +		kfree(e->bitmap);
> +		kfree(e);
> +	}
>   	kfree(mapping);
>   }
>   
> +static int extend_iommu_mapping(struct dma_iommu_mapping *mapping)
> +{
> +	struct dma_iommu_iova_range *iovar;
> +	unsigned int count = mapping->grow_size >> (PAGE_SHIFT + mapping->order);
> +	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
> +
> +	if (!mapping->grow_size ||
> +		(mapping->size + mapping->grow_size) >= mapping->max_size)
> +		return -EINVAL;
> +
> +	iovar = kzalloc(sizeof(struct dma_iommu_iova_range), GFP_ATOMIC);
> +	if (!iovar)
> +		return -ENOMEM;
> +
> +	iovar->bitmap = kzalloc(bitmap_size, GFP_ATOMIC);
> +	if (!iovar->bitmap) {
> +		kfree(iovar);
> +		return -ENOMEM;
> +	}
> +
> +	iovar->bits = BITS_PER_BYTE * bitmap_size;
> +	iovar->base = mapping->base + mapping->size;
> +	iovar->size = mapping->grow_size;
> +
> +	mapping->size += mapping->grow_size;
> +	list_add_tail(&iovar->list_head, &mapping->iova_ranges);
> +
> +	return 0;
> +}
> +
>   void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping)
>   {
>   	if (mapping)

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

[PATCH 11/11] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Will Deacon]

Hi Marek,

On Wed, Jan 29, 2014 at 10:57:01AM +0000, Marek Szyprowski wrote:
> On 2014-01-16 13:44, Andreas Herrmann wrote:
> > Instead of using just one bitmap to keep track of IO virtual addresses
> > (handed out for IOMMU use) introduce a list of iova_ranges (each
> > having its own bitmap). This allows us to extend existing mappings
> > when running out of iova space for a mapping.
> >
> > If there is not enough space in the mapping to service an IO virtual
> > address allocation request, __alloc_iova() tries to extend the mapping
> > -- by allocating another bitmap -- and makes another allocation
> > attempt using the freshly allocated bitmap.
> >
> > This allows arm iommu drivers to start with a decent initial size when
> > an dma_iommu_mapping is created and still to avoid running out of IO
> > virtual addresses for the mapping.
> >
> > Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
> > I've used SZ_512K both for initial mapping size and grow_size.
> 
> Thanks for implementing this feature! I remember it was discussed from
> early beginning of arm dma iommu support, but I never had enough time
> to actually implement it. I briefly checked the code and it look fine,
> however I really wonder if we need separate grow_size parameter?
> Personally I would simplify it to simply grow the bitmap by initial
> size until it reaches the maximal size.

That sounds sensible, but I also think it would be worth taking into account
the page sizes supported by the IOMMU as well, since aligning to those makes
sense from a TLB utilisation perspective.

> The whole concept of the simplified bitmap (where 1 bit != 1 page) for
> iova allocation is a specific feature of this code and it has nothing
> to the hardware. After thinking a bit more on the existing
> implementation I've already observed that it is sometimes hard to
> understand the parameters for arm_iommu_create_mapping() function,
> especially the 'order' argument is ofter misunderstood. With your
> patch we got two additional parameters. Maybe it will be much better
> to use only 2 arguments: max_mapping_size and allocation_accuracy.
> The initial bitmap size can be then calculated to fit it into single
> memory page (that's quite important to avoid allocations larger that
> a single memory page). 'allocation_accuracy' will serve the same way
> as 'order' parameter now (but expressed in bytes rather than being
> the multiplier for the number of pages). This way the
> arm_iommu_create_mapping() function should be much easier to
> understand, while keeping the implementation details hidden from the
> caller.

Hmm, I wouldn't guess the SI unit of accuracy to be bytes ;)

Will

[PATCH 11/11] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Andreas Herrmann]

Hi Will, Marek,

On Wed, Jan 29, 2014 at 06:05:37AM -0500, Will Deacon wrote:
> Hi Marek,
> 
> On Wed, Jan 29, 2014 at 10:57:01AM +0000, Marek Szyprowski wrote:
> > On 2014-01-16 13:44, Andreas Herrmann wrote:
> > > Instead of using just one bitmap to keep track of IO virtual addresses
> > > (handed out for IOMMU use) introduce a list of iova_ranges (each
> > > having its own bitmap). This allows us to extend existing mappings
> > > when running out of iova space for a mapping.
> > >
> > > If there is not enough space in the mapping to service an IO virtual
> > > address allocation request, __alloc_iova() tries to extend the mapping
> > > -- by allocating another bitmap -- and makes another allocation
> > > attempt using the freshly allocated bitmap.
> > >
> > > This allows arm iommu drivers to start with a decent initial size when
> > > an dma_iommu_mapping is created and still to avoid running out of IO
> > > virtual addresses for the mapping.
> > >
> > > Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
> > > I've used SZ_512K both for initial mapping size and grow_size.
> > 
> > Thanks for implementing this feature! I remember it was discussed from
> > early beginning of arm dma iommu support, but I never had enough time
> > to actually implement it. I briefly checked the code and it look fine,
> > however I really wonder if we need separate grow_size parameter?
> > Personally I would simplify it to simply grow the bitmap by initial
> > size until it reaches the maximal size.
> 
> That sounds sensible, but I also think it would be worth taking into account
> the page sizes supported by the IOMMU as well, since aligning to those makes
> sense from a TLB utilisation perspective.

Meanwhile I also think that the grow_size parameter is overkill. Only
the initial mapping size and the maximum size really matter. Then we
could try to extend the mapping by initial mapping size and if this
fails (we might not have enough pages to serve such an allocation) we
could/should even fall back to allocate a single page (which should
give us at least a 16MB range).

> > The whole concept of the simplified bitmap (where 1 bit != 1 page) for
> > iova allocation is a specific feature of this code and it has nothing
> > to the hardware. After thinking a bit more on the existing
> > implementation I've already observed that it is sometimes hard to
> > understand the parameters for arm_iommu_create_mapping() function,
> > especially the 'order' argument is ofter misunderstood. With your
> > patch we got two additional parameters. Maybe it will be much better
> > to use only 2 arguments: max_mapping_size and allocation_accuracy.
> > The initial bitmap size can be then calculated to fit it into single
> > memory page (that's quite important to avoid allocations larger that
> > a single memory page). 'allocation_accuracy' will serve the same way
> > as 'order' parameter now (but expressed in bytes rather than being
> > the multiplier for the number of pages). This way the
> > arm_iommu_create_mapping() function should be much easier to
> > understand, while keeping the implementation details hidden from the
> > caller.
> 
> Hmm, I wouldn't guess the SI unit of accuracy to be bytes ;)
 
Have to think about the alignment argument and the last paragraph for
a while.

All I can say now is that starting with a bitmap size that fits into a
single memory page doesn't sound right to me. The initial allocation
is "easy" (not GFP_ATOMIC) and thus I think we should start with a
larger bitmap. Say you have a device for which at runtime 512MB
mapping range are required and you use a 4k page for the bitmaps (each
tracking 16MB IOVA range) you'll have 32 bitmaps to maintain. Whereas
when you start with 128MB initial bitmap size and (successfully)
enhance the mapping by 128MB you just have to maintain 4 bitmaps.

But of course coming up with the right choice for the initial bitmap
size that fits all master devices is a hard thing to do ...
 


Andreas

[PATCH 11/11] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Marek Szyprowski]

Hello,

On 2014-01-29 15:40, Andreas Herrmann wrote:
> Hi Will, Marek,
>
> On Wed, Jan 29, 2014 at 06:05:37AM -0500, Will Deacon wrote:
> > Hi Marek,
> >
> > On Wed, Jan 29, 2014 at 10:57:01AM +0000, Marek Szyprowski wrote:
> > > On 2014-01-16 13:44, Andreas Herrmann wrote:
> > > > Instead of using just one bitmap to keep track of IO virtual addresses
> > > > (handed out for IOMMU use) introduce a list of iova_ranges (each
> > > > having its own bitmap). This allows us to extend existing mappings
> > > > when running out of iova space for a mapping.
> > > >
> > > > If there is not enough space in the mapping to service an IO virtual
> > > > address allocation request, __alloc_iova() tries to extend the mapping
> > > > -- by allocating another bitmap -- and makes another allocation
> > > > attempt using the freshly allocated bitmap.
> > > >
> > > > This allows arm iommu drivers to start with a decent initial size when
> > > > an dma_iommu_mapping is created and still to avoid running out of IO
> > > > virtual addresses for the mapping.
> > > >
> > > > Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
> > > > I've used SZ_512K both for initial mapping size and grow_size.
> > >
> > > Thanks for implementing this feature! I remember it was discussed from
> > > early beginning of arm dma iommu support, but I never had enough time
> > > to actually implement it. I briefly checked the code and it look fine,
> > > however I really wonder if we need separate grow_size parameter?
> > > Personally I would simplify it to simply grow the bitmap by initial
> > > size until it reaches the maximal size.
> >
> > That sounds sensible, but I also think it would be worth taking into account
> > the page sizes supported by the IOMMU as well, since aligning to those makes
> > sense from a TLB utilisation perspective.
>
> Meanwhile I also think that the grow_size parameter is overkill. Only
> the initial mapping size and the maximum size really matter. Then we
> could try to extend the mapping by initial mapping size and if this
> fails (we might not have enough pages to serve such an allocation) we
> could/should even fall back to allocate a single page (which should
> give us at least a 16MB range).
>
> > > The whole concept of the simplified bitmap (where 1 bit != 1 page) for
> > > iova allocation is a specific feature of this code and it has nothing
> > > to the hardware. After thinking a bit more on the existing
> > > implementation I've already observed that it is sometimes hard to
> > > understand the parameters for arm_iommu_create_mapping() function,
> > > especially the 'order' argument is ofter misunderstood. With your
> > > patch we got two additional parameters. Maybe it will be much better
> > > to use only 2 arguments: max_mapping_size and allocation_accuracy.
> > > The initial bitmap size can be then calculated to fit it into single
> > > memory page (that's quite important to avoid allocations larger that
> > > a single memory page). 'allocation_accuracy' will serve the same way
> > > as 'order' parameter now (but expressed in bytes rather than being
> > > the multiplier for the number of pages). This way the
> > > arm_iommu_create_mapping() function should be much easier to
> > > understand, while keeping the implementation details hidden from the
> > > caller.
> >
> > Hmm, I wouldn't guess the SI unit of accuracy to be bytes ;)
>   
> Have to think about the alignment argument and the last paragraph for
> a while.
>
> All I can say now is that starting with a bitmap size that fits into a
> single memory page doesn't sound right to me. The initial allocation
> is "easy" (not GFP_ATOMIC) and thus I think we should start with a
> larger bitmap. Say you have a device for which at runtime 512MB
> mapping range are required and you use a 4k page for the bitmaps (each
> tracking 16MB IOVA range) you'll have 32 bitmaps to maintain. Whereas
> when you start with 128MB initial bitmap size and (successfully)
> enhance the mapping by 128MB you just have to maintain 4 bitmaps.

Huh? In the 'worst' case scenario (1 bit represents 1 page) a bitmap
which occupies a single memory page holds 4096 bytes (PAGE SIZE) * 8
(bits/byte) = 32768 bits, what gives us 32768 * 4KiB (PAGE SIZE) = 128MiB
of address space, so for 512MiB mapping you just need at most 4 pages for
bitmap.

Maybe the whole concept of simplified bitmap was already an over
engineering from the beginning? I've added it to increase total supported
mapping size (in my case I worked with devices which allocates quite
large buffers), but now I see that it is not really needed if we have
dynamically extended bitmaps.

BTW, I thought a bit more one your implementation and I think that it will
be better to replace lists with simple arrays of bitmaps. We know the
maximum number of sub-bitmaps from beginning and such approach will
simplify iova freeing (no need for searching the address in a list).

> But of course coming up with the right choice for the initial bitmap
> size that fits all master devices is a hard thing to do ...

Right, but the dynamically extended bitmaps will adapt to particular use
cases, so the initial values doesn't really matter that much.

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

[PATCH 11/11] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Andreas Herrmann]

On Thu, Jan 30, 2014 at 03:28:51AM -0500, Marek Szyprowski wrote:
> Hello,
> 
> On 2014-01-29 15:40, Andreas Herrmann wrote:
> > Hi Will, Marek,
> >
> > On Wed, Jan 29, 2014 at 06:05:37AM -0500, Will Deacon wrote:
> > > Hi Marek,
> > >
> > > On Wed, Jan 29, 2014 at 10:57:01AM +0000, Marek Szyprowski wrote:
> > > > On 2014-01-16 13:44, Andreas Herrmann wrote:
> > > > > Instead of using just one bitmap to keep track of IO virtual addresses
> > > > > (handed out for IOMMU use) introduce a list of iova_ranges (each
> > > > > having its own bitmap). This allows us to extend existing mappings
> > > > > when running out of iova space for a mapping.
> > > > >
> > > > > If there is not enough space in the mapping to service an IO virtual
> > > > > address allocation request, __alloc_iova() tries to extend the mapping
> > > > > -- by allocating another bitmap -- and makes another allocation
> > > > > attempt using the freshly allocated bitmap.
> > > > >
> > > > > This allows arm iommu drivers to start with a decent initial size when
> > > > > an dma_iommu_mapping is created and still to avoid running out of IO
> > > > > virtual addresses for the mapping.
> > > > >
> > > > > Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
> > > > > I've used SZ_512K both for initial mapping size and grow_size.
> > > >
> > > > Thanks for implementing this feature! I remember it was discussed from
> > > > early beginning of arm dma iommu support, but I never had enough time
> > > > to actually implement it. I briefly checked the code and it look fine,
> > > > however I really wonder if we need separate grow_size parameter?
> > > > Personally I would simplify it to simply grow the bitmap by initial
> > > > size until it reaches the maximal size.
> > >
> > > That sounds sensible, but I also think it would be worth taking into account
> > > the page sizes supported by the IOMMU as well, since aligning to those makes
> > > sense from a TLB utilisation perspective.
> >
> > Meanwhile I also think that the grow_size parameter is overkill. Only
> > the initial mapping size and the maximum size really matter. Then we
> > could try to extend the mapping by initial mapping size and if this
> > fails (we might not have enough pages to serve such an allocation) we
> > could/should even fall back to allocate a single page (which should
> > give us at least a 16MB range).
> >
> > > > The whole concept of the simplified bitmap (where 1 bit != 1 page) for
> > > > iova allocation is a specific feature of this code and it has nothing
> > > > to the hardware. After thinking a bit more on the existing
> > > > implementation I've already observed that it is sometimes hard to
> > > > understand the parameters for arm_iommu_create_mapping() function,
> > > > especially the 'order' argument is ofter misunderstood. With your
> > > > patch we got two additional parameters. Maybe it will be much better
> > > > to use only 2 arguments: max_mapping_size and allocation_accuracy.
> > > > The initial bitmap size can be then calculated to fit it into single
> > > > memory page (that's quite important to avoid allocations larger that
> > > > a single memory page). 'allocation_accuracy' will serve the same way
> > > > as 'order' parameter now (but expressed in bytes rather than being
> > > > the multiplier for the number of pages). This way the
> > > > arm_iommu_create_mapping() function should be much easier to
> > > > understand, while keeping the implementation details hidden from the
> > > > caller.
> > >
> > > Hmm, I wouldn't guess the SI unit of accuracy to be bytes ;)
> >   
> > Have to think about the alignment argument and the last paragraph for
> > a while.
> >
> > All I can say now is that starting with a bitmap size that fits into a
> > single memory page doesn't sound right to me. The initial allocation
> > is "easy" (not GFP_ATOMIC) and thus I think we should start with a
> > larger bitmap. Say you have a device for which at runtime 512MB
> > mapping range are required and you use a 4k page for the bitmaps (each
> > tracking 16MB IOVA range) you'll have 32 bitmaps to maintain. Whereas
> > when you start with 128MB initial bitmap size and (successfully)
> > enhance the mapping by 128MB you just have to maintain 4 bitmaps.
> 
> Huh? In the 'worst' case scenario (1 bit represents 1 page) a bitmap
> which occupies a single memory page holds 4096 bytes (PAGE SIZE) * 8
> (bits/byte) = 32768 bits, what gives us 32768 * 4KiB (PAGE SIZE) = 128MiB
> of address space, so for 512MiB mapping you just need at most 4 pages for
> bitmap.

Arrgh, yes of course it's 128 and not 16 MB. (That's the reason why I
had chosen 128MB as the initial and grow size.) Not sure why I came up
with the 16MB thing yesterday. Sorry.

Of course that means that the number of bitmaps to maintain is
manageable (at least for most devices).

> Maybe the whole concept of simplified bitmap was already an over
> engineering from the beginning? I've added it to increase total supported
> mapping size (in my case I worked with devices which allocates quite
> large buffers), but now I see that it is not really needed if we have
> dynamically extended bitmaps.
> 
> BTW, I thought a bit more one your implementation and I think that it will
> be better to replace lists with simple arrays of bitmaps. We know the
> maximum number of sub-bitmaps from beginning and such approach will
> simplify iova freeing (no need for searching the address in a list).

Yep, that sounds reasonable.

> > But of course coming up with the right choice for the initial bitmap
> > size that fits all master devices is a hard thing to do ...
> 
> Right, but the dynamically extended bitmaps will adapt to particular use
> cases, so the initial values doesn't really matter that much.

Yes, (now that's again clear on my end that we cover 128MB ranges with
a bitmap of one page ;-) I don't see an issue here anymore.


Andreas

[PATCH] arm: dma-mapping: Add support to extend DMA IOMMU mappings

[Andreas Herrmann]

Instead of using just one bitmap to keep track of IO virtual addresses
(handed out for IOMMU use) introduce an array of bitmaps. This allows
us to extend existing mappings when running out of iova space in the
initial mapping etc.

If there is not enough space in the mapping to service an IO virtual
address allocation request, __alloc_iova() tries to extend the mapping
-- by allocating another bitmap -- and makes another allocation
attempt using the freshly allocated bitmap.

This allows arm iommu drivers to start with a decent initial size when
an dma_iommu_mapping is created and still to avoid running out of IO
virtual addresses for the mapping.

A new parameter is introduced for arm_iommu_create_mapping:

  int extensions

	Specifies how often the initial mapping of size bytes can be
	 extended by another address range of size bytes.  Thus if
	 extensions = 0 is passed the mapping won't be extended at
	 all.

Adapt existing calls to arm_iommu_create_mapping in exynos_drm_iommu.c
and shmobile-iommu.c such that they do not make use of the extension
feature.

Adapt existing call to arm_iommu_create_mapping in arm-smmu.c such
that the extension feature will be used. (The resulting maximum
mapping size is 4GB.)

Tests were done on Calxeda ECX-2000 with smmu for sata and xgmac.
I've used SZ_512K for the size of the mapping and 31 for the
extensions parameter.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Hiroshi Doyu <hdoyu@nvidia.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 arch/arm/include/asm/dma-iommu.h          |   12 ++-
 arch/arm/mm/dma-mapping.c                 |  135 ++++++++++++++++++++++++-----
 drivers/gpu/drm/exynos/exynos_drm_iommu.c |    4 +-
 drivers/iommu/arm-smmu.c                  |    2 +-
 drivers/iommu/shmobile-iommu.c            |    2 +-
 5 files changed, 126 insertions(+), 29 deletions(-)

Hi Marek, Will,

Here is a 2nd attempt to implement this feature. This time using
an array of bitmaps. The initial size for the mapping will be used
to extend the mapping if running out of unused IO addresses.

Comments welcome.


Regards,
Andreas


diff --git a/arch/arm/include/asm/dma-iommu.h b/arch/arm/include/asm/dma-iommu.h
index a8c56ac..e12c6c2 100644
--- a/arch/arm/include/asm/dma-iommu.h
+++ b/arch/arm/include/asm/dma-iommu.h
@@ -13,8 +13,12 @@ struct dma_iommu_mapping {
 	/* iommu specific data */
 	struct iommu_domain	*domain;
 
-	void			*bitmap;
-	size_t			bits;
+	unsigned long		**bitmaps;	/* array of bitmaps */
+	unsigned int		nr_bitmaps;	/* nr of elements in array */
+	unsigned int		extensions;
+	size_t			bitmap_size;	/* size of a sinble bitmap */
+	size_t			bits;		/* per bitmap */
+	unsigned int		size;		/* per bitmap */
 	unsigned int		order;
 	dma_addr_t		base;
 
@@ -23,8 +27,8 @@ struct dma_iommu_mapping {
 };
 
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
-			 int order);
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base,  size_t size,
+			int order, int extensions);
 
 void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping);
 
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index f61a570..bd9cd22 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1069,6 +1069,8 @@ fs_initcall(dma_debug_do_init);
 
 /* IOMMU */
 
+static int extend_iommu_mapping(struct dma_iommu_mapping *mapping);
+
 static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
 				      size_t size)
 {
@@ -1076,6 +1078,8 @@ static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
 	unsigned int align = 0;
 	unsigned int count, start;
 	unsigned long flags;
+	dma_addr_t iova;
+	int i;
 
 	if (order > CONFIG_ARM_DMA_IOMMU_ALIGNMENT)
 		order = CONFIG_ARM_DMA_IOMMU_ALIGNMENT;
@@ -1087,30 +1091,78 @@ static inline dma_addr_t __alloc_iova(struct dma_iommu_mapping *mapping,
 		align = (1 << (order - mapping->order)) - 1;
 
 	spin_lock_irqsave(&mapping->lock, flags);
-	start = bitmap_find_next_zero_area(mapping->bitmap, mapping->bits, 0,
-					   count, align);
-	if (start > mapping->bits) {
-		spin_unlock_irqrestore(&mapping->lock, flags);
-		return DMA_ERROR_CODE;
+	for (i = 0; i < mapping->nr_bitmaps; i++) {
+		start = bitmap_find_next_zero_area(mapping->bitmaps[i],
+				mapping->bits, 0, count, align);
+
+		if (start > mapping->bits)
+			continue;
+
+		bitmap_set(mapping->bitmaps[i], start, count);
+		break;
 	}
 
-	bitmap_set(mapping->bitmap, start, count);
+	/*
+	 * No unused range found. Try to extend the existing mapping
+	 * and perform a second attempt to reserve an IO virtual
+	 * address range of size bytes.
+	 */
+	if (i == mapping->nr_bitmaps) {
+		if (extend_iommu_mapping(mapping)) {
+			spin_unlock_irqrestore(&mapping->lock, flags);
+			return DMA_ERROR_CODE;
+		}
+		pr_err("extended iommu mapping\n");
+		start = bitmap_find_next_zero_area(mapping->bitmaps[i],
+				mapping->bits, 0, count, align);
+
+		if (start > mapping->bits) {
+			spin_unlock_irqrestore(&mapping->lock, flags);
+			return DMA_ERROR_CODE;
+		}
+
+		bitmap_set(mapping->bitmaps[i], start, count);
+	}
 	spin_unlock_irqrestore(&mapping->lock, flags);
 
-	return mapping->base + (start << (mapping->order + PAGE_SHIFT));
+	iova = mapping->base + (mapping->size * i);
+	iova += start << (mapping->order + PAGE_SHIFT);
+
+	return iova;
 }
 
 static inline void __free_iova(struct dma_iommu_mapping *mapping,
 			       dma_addr_t addr, size_t size)
 {
-	unsigned int start = (addr - mapping->base) >>
-			     (mapping->order + PAGE_SHIFT);
-	unsigned int count = ((size >> PAGE_SHIFT) +
-			      (1 << mapping->order) - 1) >> mapping->order;
+	unsigned int start, count;
 	unsigned long flags;
+	dma_addr_t bitmap_base;
+	u32 bitmap_index;
+
+	if (!size)
+		return;
+
+	bitmap_index = (u32) (addr - mapping->base) / (u32) mapping->size;
+	BUG_ON(addr < mapping->base || bitmap_index > mapping->extensions);
+
+	bitmap_base = mapping->base + mapping->size * bitmap_index;
+
+	start = (addr - bitmap_base) >>	(mapping->order + PAGE_SHIFT);
+
+	if (addr + size > bitmap_base + mapping->size) {
+		/*
+		 * The address range to be freed reaches into the iova
+		 * range of the next bitmap. This should not happen as
+		 * we don't allow this in __alloc_iova (at the
+		 * moment).
+		 */
+		BUG();
+	} else
+		count = ((size >> PAGE_SHIFT) +
+			(1 << mapping->order) - 1) >> mapping->order;
 
 	spin_lock_irqsave(&mapping->lock, flags);
-	bitmap_clear(mapping->bitmap, start, count);
+	bitmap_clear(mapping->bitmaps[bitmap_index], start, count);
 	spin_unlock_irqrestore(&mapping->lock, flags);
 }
 
@@ -1875,22 +1927,30 @@ struct dma_map_ops iommu_coherent_ops = {
  * arm_iommu_create_mapping
  * @bus: pointer to the bus holding the client device (for IOMMU calls)
  * @base: start address of the valid IO address space
- * @size: size of the valid IO address space
+ * @size: (initial) size of the valid IO address space
  * @order: accuracy of the IO addresses allocations
+ * @extensions: specifies how many times it's allowed to extend the initial
+ *              mapping (0 - no extension at all, 1 - mapping can be extended
+ *              once, 2 - mapping can be extended twice, etc.)
  *
  * Creates a mapping structure which holds information about used/unused
  * IO address ranges, which is required to perform memory allocation and
  * mapping with IOMMU aware functions.
  *
+ * If a positive extensions parameter is passed the mapping will be
+ * extended (by size bytes) when the mapping runs out of unused IO
+ * addresses.  Thus the maximum size of the mapping is
+ *
+ *      size * (1 + extensions)
+ *
  * The client device need to be attached to the mapping with
  * arm_iommu_attach_device function.
  */
 struct dma_iommu_mapping *
 arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
-			 int order)
+			int order, int extensions)
 {
 	unsigned int count = size >> (PAGE_SHIFT + order);
-	unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
 	struct dma_iommu_mapping *mapping;
 	int err = -ENOMEM;
 
@@ -1901,23 +1961,35 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size,
 	if (!mapping)
 		goto err;
 
-	mapping->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
-	if (!mapping->bitmap)
+	mapping->bitmap_size = BITS_TO_LONGS(count) * sizeof(long);
+	mapping->bitmaps = kzalloc((extensions + 1) * sizeof(unsigned long *),
+				GFP_KERNEL);
+	if (!mapping->bitmaps)
 		goto err2;
 
+	mapping->bitmaps[0] = kzalloc(mapping->bitmap_size, GFP_KERNEL);
+	if (!mapping->bitmaps[0])
+		goto err3;
+
+	mapping->nr_bitmaps = 1;
+	mapping->extensions = extensions;
 	mapping->base = base;
-	mapping->bits = BITS_PER_BYTE * bitmap_size;
+	mapping->size = size;
 	mapping->order = order;
+	mapping->bits = BITS_PER_BYTE * mapping->bitmap_size;
+
 	spin_lock_init(&mapping->lock);
 
 	mapping->domain = iommu_domain_alloc(bus);
 	if (!mapping->domain)
-		goto err3;
+		goto err4;
 
 	kref_init(&mapping->kref);
 	return mapping;
+err4:
+	kfree(mapping->bitmaps[0]);
 err3:
-	kfree(mapping->bitmap);
+	kfree(mapping->bitmaps);
 err2:
 	kfree(mapping);
 err:
@@ -1927,14 +1999,35 @@ EXPORT_SYMBOL_GPL(arm_iommu_create_mapping);
 
 static void release_iommu_mapping(struct kref *kref)
 {
+	int i;
 	struct dma_iommu_mapping *mapping =
 		container_of(kref, struct dma_iommu_mapping, kref);
 
 	iommu_domain_free(mapping->domain);
-	kfree(mapping->bitmap);
+	for (i = 0; i < mapping->nr_bitmaps; i++)
+		kfree(mapping->bitmaps[i]);
+	kfree(mapping->bitmaps);
 	kfree(mapping);
 }
 
+static int extend_iommu_mapping(struct dma_iommu_mapping *mapping)
+{
+	int next_bitmap;
+
+	if (mapping->nr_bitmaps > mapping->extensions)
+		return -EINVAL;
+
+	next_bitmap = mapping->nr_bitmaps;
+	mapping->bitmaps[next_bitmap] = kzalloc(mapping->bitmap_size,
+						GFP_ATOMIC);
+	if (!mapping->bitmaps[next_bitmap])
+		return -ENOMEM;
+
+	mapping->nr_bitmaps++;
+
+	return 0;
+}
+
 void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping)
 {
 	if (mapping)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_iommu.c b/drivers/gpu/drm/exynos/exynos_drm_iommu.c
index fb8db03..cc8389e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_iommu.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_iommu.c
@@ -40,8 +40,8 @@ int drm_create_iommu_mapping(struct drm_device *drm_dev)
 		priv->da_space_order = EXYNOS_DEV_ADDR_ORDER;
 
 	mapping = arm_iommu_create_mapping(&platform_bus_type, priv->da_start,
-						priv->da_space_size,
-						priv->da_space_order);
+					priv->da_space_size,
+					priv->da_space_order, 0);
 	if (IS_ERR(mapping))
 		return PTR_ERR(mapping);
 
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index bee88c8..67b3364 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1694,7 +1694,7 @@ static int arm_smmu_group_notifier(struct notifier_block *nb,
 			break;
 
 		mapping = arm_iommu_create_mapping(&platform_bus_type,
-						0, SZ_128M, 0);
+						0, SZ_128M, 0, 31);
 		if (IS_ERR(mapping)) {
 			ret = PTR_ERR(mapping);
 			dev_info(dev, "arm_iommu_create_mapping failed\n");
diff --git a/drivers/iommu/shmobile-iommu.c b/drivers/iommu/shmobile-iommu.c
index d572863..2540b7e 100644
--- a/drivers/iommu/shmobile-iommu.c
+++ b/drivers/iommu/shmobile-iommu.c
@@ -343,7 +343,7 @@ static int shmobile_iommu_add_device(struct device *dev)
 	mapping = archdata->iommu_mapping;
 	if (!mapping) {
 		mapping = arm_iommu_create_mapping(&platform_bus_type, 0,
-						   L1_LEN << 20, 0);
+						L1_LEN << 20, 0, 0);
 		if (IS_ERR(mapping))
 			return PTR_ERR(mapping);
 		archdata->iommu_mapping = mapping;
-- 
1.7.9.5