[PATCH v3 0/9] Generic DT bindings for PCI IOMMUs and ARM SMMUv3

[PATCH v3 0/9] Generic DT bindings for PCI IOMMUs and ARM SMMUv3

[Robin Murphy]

I didn't want the ongoing SMMUv2 rework delaying getting this out any
longer, so here's the SMMUv3-flavoured parts of the respin to address
the last round of comments[1] plus some new bits which fell out of the
woodwork along the way. This time I've actually fully tested the
platform device/multiple ID support as well (turns out I was being a
bit thick and our model does have some suitable peripherals after all).

As ever, branch at git://linux-arm.org/linux-rm iommu/generic-v3

Robin.

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/13936

Lorenzo Pieralisi (1):
  arm64: mm: change IOMMU notifier action to attach DMA ops

Mark Rutland (1):
  Docs: dt: add PCI IOMMU map bindings

Robin Murphy (7):
  iommu/of: Consolidate device creation workarounds
  of/irq: Break out msi-map lookup (again)
  iommu/of: Handle iommu-map property for PCI
  iommu/of: Introduce iommu_fwspec
  iommu/arm-smmu: Implement of_xlate() for SMMUv3
  iommu/arm-smmu: Support non-PCI devices with SMMUv3
  iommu/arm-smmu: Set PRIVCFG in stage 1 STEs

 .../devicetree/bindings/pci/pci-iommu.txt          | 171 ++++++++++++
 arch/arm64/mm/dma-mapping.c                        |  22 +-
 drivers/iommu/Kconfig                              |   2 +-
 drivers/iommu/arm-smmu-v3.c                        | 307 ++++++++++-----------
 drivers/iommu/exynos-iommu.c                       |  20 +-
 drivers/iommu/mtk_iommu.c                          |   8 +-
 drivers/iommu/of_iommu.c                           |  96 ++++++-
 drivers/of/irq.c                                   |  78 +-----
 drivers/of/of_pci.c                                | 102 +++++++
 include/linux/of_iommu.h                           |  18 ++
 include/linux/of_pci.h                             |  10 +
 11 files changed, 554 insertions(+), 280 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt

-- 
2.8.1.dirty

[PATCH v3 1/9] arm64: mm: change IOMMU notifier action to attach DMA ops

[Robin Murphy]

From: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>

Current bus notifier in ARM64 (__iommu_attach_notifier)
attempts to attach dma_ops to a device on BUS_NOTIFY_ADD_DEVICE
action notification.

This will cause issues on ACPI based systems, where PCI devices
can be added before the IOMMUs the devices are attached to
had a chance to be probed, causing failures on attempts to
attach dma_ops in that the domain for the respective IOMMU
may not be set-up yet by the time the bus notifier is run.

Devices dma_ops do not require to be set-up till the matching
device drivers are probed. This means that instead of running
the notifier attaching dma_ops to devices (__iommu_attach_notifier)
on BUS_NOTIFY_ADD_DEVICE action, it can be run just before the
device driver is bound to the device in question (on action
BUS_NOTIFY_BIND_DRIVER) so that it is certain that its IOMMU
group and domain are set-up accordingly at the time the
notifier is triggered.

This patch changes the notifier action upon which dma_ops
are attached to devices and defer it to driver binding time,
so that IOMMU devices have a chance to be probed and to register
their bus notifiers before the dma_ops attach sequence for a
device is actually carried out.

As a result we also no longer need worry about racing with
iommu_bus_notifier(), or about retrying the queue in case devices
were added too early on DT-based systems, so clean up the notifier
itself plus the additional workaround from 722ec35f7fae ("arm64:
dma-mapping: fix handling of devices registered before arch_initcall")

Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Cc: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
Cc: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
[rm: get rid of other now-redundant bits]
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v3: New, co-opted from http://article.gmane.org/gmane.linux.kernel.iommu/13991

 arch/arm64/mm/dma-mapping.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index c566ec83719f..e384b76203cc 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -848,15 +848,16 @@ static int __iommu_attach_notifier(struct notifier_block *nb,
 {
 	struct iommu_dma_notifier_data *master, *tmp;
 
-	if (action != BUS_NOTIFY_ADD_DEVICE)
+	if (action != BUS_NOTIFY_BIND_DRIVER)
 		return 0;
 
 	mutex_lock(&iommu_dma_notifier_lock);
 	list_for_each_entry_safe(master, tmp, &iommu_dma_masters, list) {
-		if (do_iommu_attach(master->dev, master->ops,
-				master->dma_base, master->size)) {
+		if (data == master->dev && do_iommu_attach(master->dev,
+				master->ops, master->dma_base, master->size)) {
 			list_del(&master->list);
 			kfree(master);
+			break;
 		}
 	}
 	mutex_unlock(&iommu_dma_notifier_lock);
@@ -870,17 +871,8 @@ static int __init register_iommu_dma_ops_notifier(struct bus_type *bus)
 
 	if (!nb)
 		return -ENOMEM;
-	/*
-	 * The device must be attached to a domain before the driver probe
-	 * routine gets a chance to start allocating DMA buffers. However,
-	 * the IOMMU driver also needs a chance to configure the iommu_group
-	 * via its add_device callback first, so we need to make the attach
-	 * happen between those two points. Since the IOMMU core uses a bus
-	 * notifier with default priority for add_device, do the same but
-	 * with a lower priority to ensure the appropriate ordering.
-	 */
+
 	nb->notifier_call = __iommu_attach_notifier;
-	nb->priority = -100;
 
 	ret = bus_register_notifier(bus, nb);
 	if (ret) {
@@ -904,10 +896,6 @@ static int __init __iommu_dma_init(void)
 	if (!ret)
 		ret = register_iommu_dma_ops_notifier(&pci_bus_type);
 #endif
-
-	/* handle devices queued before this arch_initcall */
-	if (!ret)
-		__iommu_attach_notifier(NULL, BUS_NOTIFY_ADD_DEVICE, NULL);
 	return ret;
 }
 arch_initcall(__iommu_dma_init);
-- 
2.8.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/9] iommu/of: Consolidate device creation workarounds

[Robin Murphy]

So far, all the users of the generic of_xlate configuration mechanism
are resorting to explicit platform device creation to ensure the IOMMU
device is ready before anything tries to refer to it. As I'm about to
convert two more drivers that will need exactly the same thing, let's
nip that proliferation in the bud and move it to a single place.

A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is
instantiated before giving out its associated ops, since it's a fairly
safe assumption that the ops aren't going to be much use if the IOMMU
can't or won't exist to back them up.

CC: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
CC: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v3: New.

 drivers/iommu/exynos-iommu.c | 20 +++++++-------------
 drivers/iommu/mtk_iommu.c    |  8 +-------
 drivers/iommu/of_iommu.c     |  6 +++++-
 3 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 5ecc86cb74c8..97380ee56d71 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -665,6 +665,13 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
+	/*
+	 * use the first registered sysmmu device for performing
+	 * dma mapping operations on iommu page tables (cpu cache flush)
+	 */
+	if (!dma_dev)
+		dma_dev = dev;
+
 	return 0;
 }
 
@@ -1341,22 +1348,9 @@ err_reg_driver:
 
 static int __init exynos_iommu_of_setup(struct device_node *np)
 {
-	struct platform_device *pdev;
-
 	if (!init_done)
 		exynos_iommu_init();
 
-	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
-	if (IS_ERR(pdev))
-		return PTR_ERR(pdev);
-
-	/*
-	 * use the first registered sysmmu device for performing
-	 * dma mapping operations on iommu page tables (cpu cache flush)
-	 */
-	if (!dma_dev)
-		dma_dev = &pdev->dev;
-
 	of_iommu_set_ops(np, &exynos_iommu_ops);
 	return 0;
 }
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index c3043d8754e3..f7ae87abea99 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -724,14 +724,8 @@ static struct platform_driver mtk_iommu_driver = {
 
 static int mtk_iommu_init_fn(struct device_node *np)
 {
-	int ret;
-	struct platform_device *pdev;
+	int ret = platform_driver_register(&mtk_iommu_driver);
 
-	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
-	if (!pdev)
-		return -ENOMEM;
-
-	ret = platform_driver_register(&mtk_iommu_driver);
 	if (ret) {
 		pr_err("%s: Failed to register driver\n", __func__);
 		return ret;
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index af499aea0a1a..7e6369cffc95 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,7 @@
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
+#include <linux/of_platform.h>
 #include <linux/slab.h>
 
 static const struct of_device_id __iommu_of_table_sentinel
@@ -127,7 +128,10 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
 	spin_lock(&of_iommu_lock);
 	list_for_each_entry(node, &of_iommu_list, list)
 		if (node->np == np) {
-			ops = node->ops;
+			if (of_node_check_flag(np, OF_POPULATED) ||
+			    of_platform_device_create(np, NULL,
+					    platform_bus_type.dev_root))
+				ops = node->ops;
 			break;
 		}
 	spin_unlock(&of_iommu_lock);
-- 
2.8.1.dirty

Re: [PATCH v3 2/9] iommu/of: Consolidate device creation workarounds

[Will Deacon]

On Tue, Jun 28, 2016 at 04:48:21PM +0100, Robin Murphy wrote:
> So far, all the users of the generic of_xlate configuration mechanism
> are resorting to explicit platform device creation to ensure the IOMMU
> device is ready before anything tries to refer to it. As I'm about to
> convert two more drivers that will need exactly the same thing, let's
> nip that proliferation in the bud and move it to a single place.
> 
> A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is
> instantiated before giving out its associated ops, since it's a fairly
> safe assumption that the ops aren't going to be much use if the IOMMU
> can't or won't exist to back them up.
> 
> CC: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> CC: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---

Marek, Yong, can I have your acks on this please? It's a fairly
straightforward change, but the subsequent arm-smmu patches rely on it.

Thanks,

Will

>  drivers/iommu/exynos-iommu.c | 20 +++++++-------------
>  drivers/iommu/mtk_iommu.c    |  8 +-------
>  drivers/iommu/of_iommu.c     |  6 +++++-
>  3 files changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 5ecc86cb74c8..97380ee56d71 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -665,6 +665,13 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(dev);
>  
> +	/*
> +	 * use the first registered sysmmu device for performing
> +	 * dma mapping operations on iommu page tables (cpu cache flush)
> +	 */
> +	if (!dma_dev)
> +		dma_dev = dev;
> +
>  	return 0;
>  }
>  
> @@ -1341,22 +1348,9 @@ err_reg_driver:
>  
>  static int __init exynos_iommu_of_setup(struct device_node *np)
>  {
> -	struct platform_device *pdev;
> -
>  	if (!init_done)
>  		exynos_iommu_init();
>  
> -	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
> -	if (IS_ERR(pdev))
> -		return PTR_ERR(pdev);
> -
> -	/*
> -	 * use the first registered sysmmu device for performing
> -	 * dma mapping operations on iommu page tables (cpu cache flush)
> -	 */
> -	if (!dma_dev)
> -		dma_dev = &pdev->dev;
> -
>  	of_iommu_set_ops(np, &exynos_iommu_ops);
>  	return 0;
>  }
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index c3043d8754e3..f7ae87abea99 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -724,14 +724,8 @@ static struct platform_driver mtk_iommu_driver = {
>  
>  static int mtk_iommu_init_fn(struct device_node *np)
>  {
> -	int ret;
> -	struct platform_device *pdev;
> +	int ret = platform_driver_register(&mtk_iommu_driver);
>  
> -	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
> -	if (!pdev)
> -		return -ENOMEM;
> -
> -	ret = platform_driver_register(&mtk_iommu_driver);
>  	if (ret) {
>  		pr_err("%s: Failed to register driver\n", __func__);
>  		return ret;
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index af499aea0a1a..7e6369cffc95 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -22,6 +22,7 @@
>  #include <linux/limits.h>
>  #include <linux/of.h>
>  #include <linux/of_iommu.h>
> +#include <linux/of_platform.h>
>  #include <linux/slab.h>
>  
>  static const struct of_device_id __iommu_of_table_sentinel
> @@ -127,7 +128,10 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>  	spin_lock(&of_iommu_lock);
>  	list_for_each_entry(node, &of_iommu_list, list)
>  		if (node->np == np) {
> -			ops = node->ops;
> +			if (of_node_check_flag(np, OF_POPULATED) ||
> +			    of_platform_device_create(np, NULL,
> +					    platform_bus_type.dev_root))
> +				ops = node->ops;
>  			break;
>  		}
>  	spin_unlock(&of_iommu_lock);
> -- 
> 2.8.1.dirty
> 

Re: [PATCH v3 2/9] iommu/of: Consolidate device creation workarounds

[Marek Szyprowski]

Hi Robin,


On 2016-06-28 17:48, Robin Murphy wrote:
> So far, all the users of the generic of_xlate configuration mechanism
> are resorting to explicit platform device creation to ensure the IOMMU
> device is ready before anything tries to refer to it. As I'm about to
> convert two more drivers that will need exactly the same thing, let's
> nip that proliferation in the bud and move it to a single place.
>
> A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is
> instantiated before giving out its associated ops, since it's a fairly
> safe assumption that the ops aren't going to be much use if the IOMMU
> can't or won't exist to back them up.
>
> CC: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> CC: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

Frankly, I would avoid moving this workaround to the iommu core. IMHO the
best solution would be to let IOMMU controllers to be instantiated as normal
devices and implement proper support in the device core for waiting for the
iommu controller. Then the workaround can be removed from exynos and mtk
iommu drivers. What's the status of IOMMU deferred probe patches?

I've encountered a serious problems with current code (the one which
instantiates iommu controller devices from iommu driver) and its integration
with power domains, clocks and runtime pm, which were not possible to 
resolve
without iommu deferred probe.


> ---
>
> v3: New.
>
>   drivers/iommu/exynos-iommu.c | 20 +++++++-------------
>   drivers/iommu/mtk_iommu.c    |  8 +-------
>   drivers/iommu/of_iommu.c     |  6 +++++-
>   3 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 5ecc86cb74c8..97380ee56d71 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -665,6 +665,13 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
>   
>   	pm_runtime_enable(dev);
>   
> +	/*
> +	 * use the first registered sysmmu device for performing
> +	 * dma mapping operations on iommu page tables (cpu cache flush)
> +	 */
> +	if (!dma_dev)
> +		dma_dev = dev;
> +
>   	return 0;
>   }
>   
> @@ -1341,22 +1348,9 @@ err_reg_driver:
>   
>   static int __init exynos_iommu_of_setup(struct device_node *np)
>   {
> -	struct platform_device *pdev;
> -
>   	if (!init_done)
>   		exynos_iommu_init();
>   
> -	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
> -	if (IS_ERR(pdev))
> -		return PTR_ERR(pdev);
> -
> -	/*
> -	 * use the first registered sysmmu device for performing
> -	 * dma mapping operations on iommu page tables (cpu cache flush)
> -	 */
> -	if (!dma_dev)
> -		dma_dev = &pdev->dev;
> -
>   	of_iommu_set_ops(np, &exynos_iommu_ops);
>   	return 0;
>   }
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index c3043d8754e3..f7ae87abea99 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -724,14 +724,8 @@ static struct platform_driver mtk_iommu_driver = {
>   
>   static int mtk_iommu_init_fn(struct device_node *np)
>   {
> -	int ret;
> -	struct platform_device *pdev;
> +	int ret = platform_driver_register(&mtk_iommu_driver);
>   
> -	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
> -	if (!pdev)
> -		return -ENOMEM;
> -
> -	ret = platform_driver_register(&mtk_iommu_driver);
>   	if (ret) {
>   		pr_err("%s: Failed to register driver\n", __func__);
>   		return ret;
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index af499aea0a1a..7e6369cffc95 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -22,6 +22,7 @@
>   #include <linux/limits.h>
>   #include <linux/of.h>
>   #include <linux/of_iommu.h>
> +#include <linux/of_platform.h>
>   #include <linux/slab.h>
>   
>   static const struct of_device_id __iommu_of_table_sentinel
> @@ -127,7 +128,10 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>   	spin_lock(&of_iommu_lock);
>   	list_for_each_entry(node, &of_iommu_list, list)
>   		if (node->np == np) {
> -			ops = node->ops;
> +			if (of_node_check_flag(np, OF_POPULATED) ||
> +			    of_platform_device_create(np, NULL,
> +					    platform_bus_type.dev_root))
> +				ops = node->ops;
>   			break;
>   		}
>   	spin_unlock(&of_iommu_lock);

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

Re: [PATCH v3 2/9] iommu/of: Consolidate device creation workarounds

[Robin Murphy]

Hi Marek,

On 01/07/16 11:32, Marek Szyprowski wrote:
> Hi Robin,
> 
> 
> On 2016-06-28 17:48, Robin Murphy wrote:
>> So far, all the users of the generic of_xlate configuration mechanism
>> are resorting to explicit platform device creation to ensure the IOMMU
>> device is ready before anything tries to refer to it. As I'm about to
>> convert two more drivers that will need exactly the same thing, let's
>> nip that proliferation in the bud and move it to a single place.
>>
>> A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is
>> instantiated before giving out its associated ops, since it's a fairly
>> safe assumption that the ops aren't going to be much use if the IOMMU
>> can't or won't exist to back them up.
>>
>> CC: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> CC: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> 
> Frankly, I would avoid moving this workaround to the iommu core. IMHO the
> best solution would be to let IOMMU controllers to be instantiated as
> normal
> devices and implement proper support in the device core for waiting for the
> iommu controller. Then the workaround can be removed from exynos and mtk
> iommu drivers. What's the status of IOMMU deferred probe patches?

I think revisiting probe ordering is now second-from-top on my to-do
list after this lot. This patch was kind of thinking ahead to get the
"touch all the drivers" aspect out of the way before it grows any
bigger, and all the development can then happen in the core code alone,
but I admit it's not a particularly strong argument.

> I've encountered a serious problems with current code (the one which
> instantiates iommu controller devices from iommu driver) and its
> integration
> with power domains, clocks and runtime pm, which were not possible to
> resolve
> without iommu deferred probe.

OK. Do you have any plans to try tweaking the current workaround, or is
it really not worth it? FWIW I do have an Exynos 5410 (Odroid-XU) on my
desk which I could theoretically test things on, but I suspect it would
take a fair amount of work to get the SYSMMUs and relevant media bits up
and running on top of Krzysztof's basic support.

Will: for the time being, the alternative to this patch would be to
squash the following change into patch 7/9 (without either, patch 8/9
doesn't really work).

Robin.

-----8<-----
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index e4c3916efba9..c002ff06b625 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2678,6 +2678,14 @@ module_exit(arm_smmu_exit);

 static int __init arm_smmu_of_init(struct device_node *np)
 {
+	static bool registered;
+
+	if (!registered)
+		registered = !arm_smmu_init();
+
+	if (!of_platform_device_create(np, NULL, platform_bus_type.dev_root))
+		return -ENODEV;
+
 	of_iommu_set_ops(np, &arm_smmu_ops);

 	return 0;

Re: [PATCH v3 2/9] iommu/of: Consolidate device creation workarounds

[Marek Szyprowski]

Hi Robin,


On 2016-07-01 13:19, Robin Murphy wrote:
> Hi Marek,
>
> On 01/07/16 11:32, Marek Szyprowski wrote:
>> Hi Robin,
>>
>>
>> On 2016-06-28 17:48, Robin Murphy wrote:
>>> So far, all the users of the generic of_xlate configuration mechanism
>>> are resorting to explicit platform device creation to ensure the IOMMU
>>> device is ready before anything tries to refer to it. As I'm about to
>>> convert two more drivers that will need exactly the same thing, let's
>>> nip that proliferation in the bud and move it to a single place.
>>>
>>> A neat solution is to make of_get_iommu_ops() ensure an IOMMU device is
>>> instantiated before giving out its associated ops, since it's a fairly
>>> safe assumption that the ops aren't going to be much use if the IOMMU
>>> can't or won't exist to back them up.
>>>
>>> CC: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>> CC: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> Frankly, I would avoid moving this workaround to the iommu core. IMHO the
>> best solution would be to let IOMMU controllers to be instantiated as
>> normal
>> devices and implement proper support in the device core for waiting for the
>> iommu controller. Then the workaround can be removed from exynos and mtk
>> iommu drivers. What's the status of IOMMU deferred probe patches?
> I think revisiting probe ordering is now second-from-top on my to-do
> list after this lot. This patch was kind of thinking ahead to get the
> "touch all the drivers" aspect out of the way before it grows any
> bigger, and all the development can then happen in the core code alone,
> but I admit it's not a particularly strong argument.
>
>> I've encountered a serious problems with current code (the one which
>> instantiates iommu controller devices from iommu driver) and its
>> integration
>> with power domains, clocks and runtime pm, which were not possible to
>> resolve
>> without iommu deferred probe.
> OK. Do you have any plans to try tweaking the current workaround, or is
> it really not worth it?

I will keep it as is until the core will be ready for initializing Exynos
iommu driver simply from the core_initcall (or other), which will just do
platform_driver_register(&exynos_sysmmu_driver).

>   FWIW I do have an Exynos 5410 (Odroid-XU) on my
> desk which I could theoretically test things on, but I suspect it would
> take a fair amount of work to get the SYSMMUs and relevant media bits up
> and running on top of Krzysztof's basic support.

It should be quite easy to get Exynos DRM with HDMI display working on
Odroid XU1, I can take a look in that after getting back from holidays
(probably after 17th July).

> Will: for the time being, the alternative to this patch would be to
> squash the following change into patch 7/9 (without either, patch 8/9
> doesn't really work).
>
> Robin.
>
> -----8<-----
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index e4c3916efba9..c002ff06b625 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2678,6 +2678,14 @@ module_exit(arm_smmu_exit);
>
>   static int __init arm_smmu_of_init(struct device_node *np)
>   {
> +	static bool registered;
> +
> +	if (!registered)
> +		registered = !arm_smmu_init();
> +
> +	if (!of_platform_device_create(np, NULL, platform_bus_type.dev_root))
> +		return -ENODEV;
> +
>   	of_iommu_set_ops(np, &arm_smmu_ops);
>
>   	return 0;
>
>

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

Re: [PATCH v3 2/9] iommu/of: Consolidate device creation workarounds

[Will Deacon]

On Fri, Jul 01, 2016 at 12:19:51PM +0100, Robin Murphy wrote:
> On 01/07/16 11:32, Marek Szyprowski wrote:
> > Frankly, I would avoid moving this workaround to the iommu core. IMHO the
> > best solution would be to let IOMMU controllers to be instantiated as
> > normal
> > devices and implement proper support in the device core for waiting for the
> > iommu controller. Then the workaround can be removed from exynos and mtk
> > iommu drivers. What's the status of IOMMU deferred probe patches?
> 
> I think revisiting probe ordering is now second-from-top on my to-do
> list after this lot. This patch was kind of thinking ahead to get the
> "touch all the drivers" aspect out of the way before it grows any
> bigger, and all the development can then happen in the core code alone,
> but I admit it's not a particularly strong argument.
> 
> > I've encountered a serious problems with current code (the one which
> > instantiates iommu controller devices from iommu driver) and its
> > integration
> > with power domains, clocks and runtime pm, which were not possible to
> > resolve
> > without iommu deferred probe.
> 
> OK. Do you have any plans to try tweaking the current workaround, or is
> it really not worth it? FWIW I do have an Exynos 5410 (Odroid-XU) on my
> desk which I could theoretically test things on, but I suspect it would
> take a fair amount of work to get the SYSMMUs and relevant media bits up
> and running on top of Krzysztof's basic support.
> 
> Will: for the time being, the alternative to this patch would be to
> squash the following change into patch 7/9 (without either, patch 8/9
> doesn't really work).

It's ugly, but I'm fine with it for the moment. No need to wait for the
probe deferral stuff before getting this in.

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/9] Docs: dt: add PCI IOMMU map bindings

[Robin Murphy]

From: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>

The existing IOMMU bindings are able to specify the relationship between
masters and IOMMUs, but they are insufficient for describing the general
case of hotpluggable busses such as PCI where the set of masters is not
known until runtime, and the relationship between masters and IOMMUs is
a property of the integration of the system.

This patch adds a generic binding for mapping PCI devices to IOMMUs,
using a new iommu-map property (specific to PCI*) which may be used to
map devices (identified by their Requester ID) to sideband data for the
IOMMU which they master through.

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
---

v3: +Will's ack.

 .../devicetree/bindings/pci/pci-iommu.txt          | 171 +++++++++++++++++++++
 1 file changed, 171 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt

diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt b/Documentation/devicetree/bindings/pci/pci-iommu.txt
new file mode 100644
index 000000000000..56c829621b9a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
@@ -0,0 +1,171 @@
+This document describes the generic device tree binding for describing the
+relationship between PCI(e) devices and IOMMU(s).
+
+Each PCI(e) device under a root complex is uniquely identified by its Requester
+ID (AKA RID). A Requester ID is a triplet of a Bus number, Device number, and
+Function number.
+
+For the purpose of this document, when treated as a numeric value, a RID is
+formatted such that:
+
+* Bits [15:8] are the Bus number.
+* Bits [7:3] are the Device number.
+* Bits [2:0] are the Function number.
+* Any other bits required for padding must be zero.
+
+IOMMUs may distinguish PCI devices through sideband data derived from the
+Requester ID. While a given PCI device can only master through one IOMMU, a
+root complex may split masters across a set of IOMMUs (e.g. with one IOMMU per
+bus).
+
+The generic 'iommus' property is insufficient to describe this relationship,
+and a mechanism is required to map from a PCI device to its IOMMU and sideband
+data.
+
+For generic IOMMU bindings, see
+Documentation/devicetree/bindings/iommu/iommu.txt.
+
+
+PCI root complex
+================
+
+Optional properties
+-------------------
+
+- iommu-map: Maps a Requester ID to an IOMMU and associated iommu-specifier
+  data.
+
+  The property is an arbitrary number of tuples of
+  (rid-base,iommu,iommu-base,length).
+
+  Any RID r in the interval [rid-base, rid-base + length) is associated with
+  the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).
+
+- iommu-map-mask: A mask to be applied to each Requester ID prior to being
+  mapped to an iommu-specifier per the iommu-map property.
+
+
+Example (1)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	iommu: iommu@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	pci: pci@f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the IOMMU is the RID,
+		 * identity-mapped.
+		 */
+		iommu-map = <0x0 &iommu 0x0 0x10000>;
+	};
+};
+
+
+Example (2)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	iommu: iommu@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	pci: pci@f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the IOMMU is the RID with the
+		 * function bits masked out.
+		 */
+		iommu-map = <0x0 &iommu 0x0 0x10000>;
+		iommu-map-mask = <0xfff8>;
+	};
+};
+
+
+Example (3)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	iommu: iommu@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	pci: pci@f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * The sideband data provided to the IOMMU is the RID,
+		 * but the high bits of the bus number are flipped.
+		 */
+		iommu-map = <0x0000 &iommu 0x8000 0x8000>,
+			    <0x8000 &iommu 0x0000 0x8000>;
+	};
+};
+
+
+Example (4)
+===========
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	iommu_a: iommu@a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	iommu_b: iommu@b {
+		reg = <0xb 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	iommu_c: iommu@c {
+		reg = <0xc 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	pci: pci@f {
+		reg = <0xf 0x1>;
+		compatible = "vendor,pcie-root-complex";
+		device_type = "pci";
+
+		/*
+		 * Devices with bus number 0-127 are mastered via IOMMU
+		 * a, with sideband data being RID[14:0].
+		 * Devices with bus number 128-255 are mastered via
+		 * IOMMU b, with sideband data being RID[14:0].
+		 * No devices master via IOMMU c.
+		 */
+		iommu-map = <0x0000 &iommu_a 0x0000 0x8000>,
+			    <0x8000 &iommu_b 0x0000 0x8000>;
+	};
+};
-- 
2.8.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/9] of/irq: Break out msi-map lookup (again)

[Robin Murphy]

The PCI msi-map code is already doing double-duty translating IDs and
retrieving MSI parents, which unsurprisingly is the same functionality
we need for the identically-formatted PCI iommu-map property. Drag the
core parsing routine up yet another layer into the general OF-PCI code,
and further generalise it for either kind of lookup in either flavour
of map property.

CC: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v3: +Marc's ack, pass mask name explicitly [Will], further simplify
    __of_msi_map_rid() [Rob].
    
 drivers/of/irq.c       |  78 ++-----------------------------------
 drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pci.h |  10 +++++
 3 files changed, 116 insertions(+), 74 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 6ec743faabe8..091d45e6d61d 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
+#include <linux/of_pci.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 
@@ -587,87 +588,16 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
 			    u32 rid_in)
 {
 	struct device *parent_dev;
-	struct device_node *msi_controller_node;
-	struct device_node *msi_np = *np;
-	u32 map_mask, masked_rid, rid_base, msi_base, rid_len, phandle;
-	int msi_map_len;
-	bool matched;
 	u32 rid_out = rid_in;
-	const __be32 *msi_map = NULL;
 
 	/*
 	 * Walk up the device parent links looking for one with a
 	 * "msi-map" property.
 	 */
-	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
-		if (!parent_dev->of_node)
-			continue;
-
-		msi_map = of_get_property(parent_dev->of_node,
-					  "msi-map", &msi_map_len);
-		if (!msi_map)
-			continue;
-
-		if (msi_map_len % (4 * sizeof(__be32))) {
-			dev_err(parent_dev, "Error: Bad msi-map length: %d\n",
-				msi_map_len);
-			return rid_out;
-		}
-		/* We have a good parent_dev and msi_map, let's use them. */
-		break;
-	}
-	if (!msi_map)
-		return rid_out;
-
-	/* The default is to select all bits. */
-	map_mask = 0xffffffff;
-
-	/*
-	 * Can be overridden by "msi-map-mask" property.  If
-	 * of_property_read_u32() fails, the default is used.
-	 */
-	of_property_read_u32(parent_dev->of_node, "msi-map-mask", &map_mask);
-
-	masked_rid = map_mask & rid_in;
-	matched = false;
-	while (!matched && msi_map_len >= 4 * sizeof(__be32)) {
-		rid_base = be32_to_cpup(msi_map + 0);
-		phandle = be32_to_cpup(msi_map + 1);
-		msi_base = be32_to_cpup(msi_map + 2);
-		rid_len = be32_to_cpup(msi_map + 3);
-
-		if (rid_base & ~map_mask) {
-			dev_err(parent_dev,
-				"Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-base (0x%x)\n",
-				map_mask, rid_base);
-			return rid_out;
-		}
-
-		msi_controller_node = of_find_node_by_phandle(phandle);
-
-		matched = (masked_rid >= rid_base &&
-			   masked_rid < rid_base + rid_len);
-		if (msi_np)
-			matched &= msi_np == msi_controller_node;
-
-		if (matched && !msi_np) {
-			*np = msi_np = msi_controller_node;
+	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
+		if (!of_pci_map_rid(parent_dev->of_node, rid_in, "msi-map",
+				    "msi-map-mask", np, &rid_out))
 			break;
-		}
-
-		of_node_put(msi_controller_node);
-		msi_map_len -= 4 * sizeof(__be32);
-		msi_map += 4;
-	}
-	if (!matched)
-		return rid_out;
-
-	rid_out = masked_rid - rid_base + msi_base;
-	dev_dbg(dev,
-		"msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
-		dev_name(parent_dev), map_mask, rid_base, msi_base,
-		rid_len, rid_in, rid_out);
-
 	return rid_out;
 }
 
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 13f4fed38048..e6465cf7aea9 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -306,3 +306,105 @@ struct msi_controller *of_pci_find_msi_chip_by_node(struct device_node *of_node)
 EXPORT_SYMBOL_GPL(of_pci_find_msi_chip_by_node);
 
 #endif /* CONFIG_PCI_MSI */
+
+/**
+ * of_pci_map_rid - Translate a requester ID through a downstream mapping.
+ * @np: root complex device node.
+ * @rid: PCI requester ID to map.
+ * @map_name: property name of the map to use.
+ * @map_mask_name: optional property name of the mask to use.
+ * @target: optional pointer to a target device node.
+ * @id_out: optional pointer to receive the translated ID.
+ *
+ * Given a PCI requester ID, look up the appropriate implementation-defined
+ * platform ID and/or the target device which receives transactions on that
+ * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
+ * @id_out may be NULL if only the other is required. If @target points to
+ * a non-NULL device node pointer, only entries targeting that node will be
+ * matched; if it points to a NULL value, it will receive the device node of
+ * the first matching target phandle, with a reference held.
+ *
+ * Return: 0 on success or a standard error code on failure.
+ */
+int of_pci_map_rid(struct device_node *np, u32 rid,
+		   const char *map_name, const char *map_mask_name,
+		   struct device_node **target, u32 *id_out)
+{
+	u32 map_mask, masked_rid;
+	int map_len;
+	const __be32 *map = NULL;
+
+	if (!np || !map_name || (!target && !id_out))
+		return -EINVAL;
+
+	map = of_get_property(np, map_name, &map_len);
+	if (!map) {
+		if (target)
+			return -ENODEV;
+		/* Otherwise, no map implies no translation */
+		*id_out = rid;
+		return 0;
+	}
+
+	if (!map_len || map_len % (4 * sizeof(*map))) {
+		pr_err("%s: Error: Bad %s length: %d\n", np->full_name,
+			map_name, map_len);
+		return -EINVAL;
+	}
+
+	/* The default is to select all bits. */
+	map_mask = 0xffffffff;
+
+	/*
+	 * Can be overridden by "{iommu,msi}-map-mask" property.
+	 * If of_property_read_u32() fails, the default is used.
+	 */
+	if (map_mask_name)
+		of_property_read_u32(np, map_mask_name, &map_mask);
+
+	masked_rid = map_mask & rid;
+	for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4) {
+		struct device_node *phandle_node;
+		u32 rid_base = be32_to_cpup(map + 0);
+		u32 phandle = be32_to_cpup(map + 1);
+		u32 out_base = be32_to_cpup(map + 2);
+		u32 rid_len = be32_to_cpup(map + 3);
+
+		if (rid_base & ~map_mask) {
+			pr_err("%s: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n",
+				np->full_name, map_name, map_name,
+				map_mask, rid_base);
+			return -EFAULT;
+		}
+
+		if (masked_rid < rid_base || masked_rid >= rid_base + rid_len)
+			continue;
+
+		phandle_node = of_find_node_by_phandle(phandle);
+		if (!phandle_node)
+			return -ENODEV;
+
+		if (target) {
+			if (*target)
+				of_node_put(phandle_node);
+			else
+				*target = phandle_node;
+
+			if (*target != phandle_node)
+				continue;
+		}
+
+		if (id_out)
+			*id_out = masked_rid - rid_base + out_base;
+
+		pr_debug("%s: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n",
+			np->full_name, map_name, map_mask, rid_base, out_base,
+			rid_len, rid, *id_out);
+		return 0;
+	}
+
+	pr_err("%s: Invalid %s translation - no match for rid 0x%x on %s\n",
+		np->full_name, map_name, rid,
+		target && *target ? (*target)->full_name : "any target");
+	return -EFAULT;
+}
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index b969e9443962..7fd5cfce9140 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -17,6 +17,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin);
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
 void of_pci_check_probe_only(void);
+int of_pci_map_rid(struct device_node *np, u32 rid,
+		   const char *map_name, const char *map_mask_name,
+		   struct device_node **target, u32 *id_out);
 #else
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {
@@ -52,6 +55,13 @@ of_get_pci_domain_nr(struct device_node *node)
 	return -1;
 }
 
+static inline int of_pci_map_rid(struct device_node *np, u32 rid,
+			const char *map_name, const char *map_mask_name,
+			struct device_node **target, u32 *id_out)
+{
+	return -EINVAL;
+}
+
 static inline void of_pci_check_probe_only(void) { }
 #endif
 
-- 
2.8.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/9] iommu/of: Handle iommu-map property for PCI

[Robin Murphy]

Now that we have a way to pick up the RID translation and target IOMMU,
hook up of_iommu_configure() to bring PCI devices into the of_xlate
mechanism and allow them IOMMU-backed DMA ops without the need for
driver-specific handling.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v3: Use explicit "iommu-map-mask", drop of_device_is_available() check.

 drivers/iommu/of_iommu.c | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 7e6369cffc95..25406a8b9d4e 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -22,6 +22,7 @@
 #include <linux/limits.h>
 #include <linux/of.h>
 #include <linux/of_iommu.h>
+#include <linux/of_pci.h>
 #include <linux/of_platform.h>
 #include <linux/slab.h>
 
@@ -138,20 +139,48 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
 	return ops;
 }
 
+static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
+{
+	struct of_phandle_args *iommu_spec = data;
+
+	iommu_spec->args[0] = alias;
+	return iommu_spec->np == pdev->bus->dev.of_node;
+}
+
 const struct iommu_ops *of_iommu_configure(struct device *dev,
 					   struct device_node *master_np)
 {
 	struct of_phandle_args iommu_spec;
-	struct device_node *np;
+	struct device_node *np = NULL;
 	const struct iommu_ops *ops = NULL;
 	int idx = 0;
 
-	/*
-	 * We can't do much for PCI devices without knowing how
-	 * device IDs are wired up from the PCI bus to the IOMMU.
-	 */
-	if (dev_is_pci(dev))
-		return NULL;
+	if (dev_is_pci(dev)) {
+		/*
+		 * Start by tracing the RID alias down the PCI topology as
+		 * far as the host bridge whose OF node we have...
+		 */
+		iommu_spec.np = master_np;
+		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
+				       &iommu_spec);
+		/*
+		 * ...then find out what that becomes once it escapes the PCI
+		 * bus into the system beyond, and which IOMMU it ends up at.
+		 */
+		if (of_pci_map_rid(master_np, iommu_spec.args[0], "iommu-map",
+				    "iommu-map-mask", &np, iommu_spec.args))
+			return NULL;
+
+		/* We're not attempting to handle multi-alias devices yet */
+		iommu_spec.np = np;
+		iommu_spec.args_count = 1;
+		ops = of_iommu_get_ops(np);
+		if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
+			ops = NULL;
+
+		of_node_put(np);
+		return ops;
+	}
 
 	/*
 	 * We don't currently walk up the tree looking for a parent IOMMU.
-- 
2.8.1.dirty

Re: [PATCH v3 5/9] iommu/of: Handle iommu-map property for PCI

[Will Deacon]

On Tue, Jun 28, 2016 at 04:48:24PM +0100, Robin Murphy wrote:
> Now that we have a way to pick up the RID translation and target IOMMU,
> hook up of_iommu_configure() to bring PCI devices into the of_xlate
> mechanism and allow them IOMMU-backed DMA ops without the need for
> driver-specific handling.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> v3: Use explicit "iommu-map-mask", drop of_device_is_available() check.
> 
>  drivers/iommu/of_iommu.c | 43 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 7e6369cffc95..25406a8b9d4e 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -22,6 +22,7 @@
>  #include <linux/limits.h>
>  #include <linux/of.h>
>  #include <linux/of_iommu.h>
> +#include <linux/of_pci.h>
>  #include <linux/of_platform.h>
>  #include <linux/slab.h>
>  
> @@ -138,20 +139,48 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>  	return ops;
>  }
>  
> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
> +{
> +	struct of_phandle_args *iommu_spec = data;
> +
> +	iommu_spec->args[0] = alias;
> +	return iommu_spec->np == pdev->bus->dev.of_node;
> +}
> +
>  const struct iommu_ops *of_iommu_configure(struct device *dev,
>  					   struct device_node *master_np)
>  {
>  	struct of_phandle_args iommu_spec;
> -	struct device_node *np;
> +	struct device_node *np = NULL;

This sets off some alarm bells...

>  	const struct iommu_ops *ops = NULL;
>  	int idx = 0;
>  
> -	/*
> -	 * We can't do much for PCI devices without knowing how
> -	 * device IDs are wired up from the PCI bus to the IOMMU.
> -	 */
> -	if (dev_is_pci(dev))
> -		return NULL;
> +	if (dev_is_pci(dev)) {
> +		/*
> +		 * Start by tracing the RID alias down the PCI topology as
> +		 * far as the host bridge whose OF node we have...
> +		 */
> +		iommu_spec.np = master_np;
> +		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
> +				       &iommu_spec);
> +		/*
> +		 * ...then find out what that becomes once it escapes the PCI
> +		 * bus into the system beyond, and which IOMMU it ends up at.
> +		 */
> +		if (of_pci_map_rid(master_np, iommu_spec.args[0], "iommu-map",
> +				    "iommu-map-mask", &np, iommu_spec.args))
> +			return NULL;

... because you're assumedly initialising np to NULL in case of_pci_map_rid
returns 0, but doesn't set np...

> +
> +		/* We're not attempting to handle multi-alias devices yet */
> +		iommu_spec.np = np;
> +		iommu_spec.args_count = 1;
> +		ops = of_iommu_get_ops(np);

... and then we call of_iommu_get_ops(NULL). Does that make any sense?

Will

Re: [PATCH v3 5/9] iommu/of: Handle iommu-map property for PCI

[Robin Murphy]

On 01/07/16 11:31, Will Deacon wrote:
> On Tue, Jun 28, 2016 at 04:48:24PM +0100, Robin Murphy wrote:
>> Now that we have a way to pick up the RID translation and target IOMMU,
>> hook up of_iommu_configure() to bring PCI devices into the of_xlate
>> mechanism and allow them IOMMU-backed DMA ops without the need for
>> driver-specific handling.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> v3: Use explicit "iommu-map-mask", drop of_device_is_available() check.
>>
>>  drivers/iommu/of_iommu.c | 43 ++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index 7e6369cffc95..25406a8b9d4e 100644
>> --- a/drivers/iommu/of_iommu.c
>> +++ b/drivers/iommu/of_iommu.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/limits.h>
>>  #include <linux/of.h>
>>  #include <linux/of_iommu.h>
>> +#include <linux/of_pci.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/slab.h>
>>  
>> @@ -138,20 +139,48 @@ const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>>  	return ops;
>>  }
>>  
>> +static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>> +{
>> +	struct of_phandle_args *iommu_spec = data;
>> +
>> +	iommu_spec->args[0] = alias;
>> +	return iommu_spec->np == pdev->bus->dev.of_node;
>> +}
>> +
>>  const struct iommu_ops *of_iommu_configure(struct device *dev,
>>  					   struct device_node *master_np)
>>  {
>>  	struct of_phandle_args iommu_spec;
>> -	struct device_node *np;
>> +	struct device_node *np = NULL;
> 
> This sets off some alarm bells...
> 
>>  	const struct iommu_ops *ops = NULL;
>>  	int idx = 0;
>>  
>> -	/*
>> -	 * We can't do much for PCI devices without knowing how
>> -	 * device IDs are wired up from the PCI bus to the IOMMU.
>> -	 */
>> -	if (dev_is_pci(dev))
>> -		return NULL;
>> +	if (dev_is_pci(dev)) {
>> +		/*
>> +		 * Start by tracing the RID alias down the PCI topology as
>> +		 * far as the host bridge whose OF node we have...
>> +		 */
>> +		iommu_spec.np = master_np;
>> +		pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
>> +				       &iommu_spec);
>> +		/*
>> +		 * ...then find out what that becomes once it escapes the PCI
>> +		 * bus into the system beyond, and which IOMMU it ends up at.
>> +		 */
>> +		if (of_pci_map_rid(master_np, iommu_spec.args[0], "iommu-map",
>> +				    "iommu-map-mask", &np, iommu_spec.args))
>> +			return NULL;
> 
> ... because you're assumedly initialising np to NULL in case of_pci_map_rid
> returns 0, but doesn't set np...

Not quite; we're passing &np as the optional "target" argument to
of_pci_map_rid - since that argument is provided, it will either find a
matching translation (and fill in np in the process) or return -EFAULT.

>> +
>> +		/* We're not attempting to handle multi-alias devices yet */
>> +		iommu_spec.np = np;
>> +		iommu_spec.args_count = 1;
>> +		ops = of_iommu_get_ops(np);
> 
> ... and then we call of_iommu_get_ops(NULL). Does that make any sense?

It would actually work out, since nobody should have registered any ops
for a NULL node, but as above won't happen in practice anyway.

Robin.

> 
> Will
> 

[PATCH v3 6/9] iommu/of: Introduce iommu_fwspec

[Robin Murphy]

Introduce a common structure to hold the per-device firmware data that
non-architectural IOMMU drivers generally need to keep track of.
Initially this is DT-specific to complement the existing of_iommu
support code, but will generalise further once other firmware methods
(e.g. ACPI IORT) come along.

Ultimately the aim is to promote the fwspec to a first-class member of
struct device, and handle the init/free automatically in the firmware
code. That way we can have API calls look for dev->fwspec->iommu_ops
before falling back to dev->bus->iommu_ops, and thus gracefully handle
those troublesome multi-IOMMU systems which we currently cannot. To
start with, though, make use of the existing archdata field and delegate
the init/free to drivers to allow an incremental conversion rather than
the impractical pain of trying to attempt everything in one go.

Suggested-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v3: New.

 drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 18 ++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 25406a8b9d4e..2b90c1f56ff2 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -220,3 +220,50 @@ void __init of_iommu_init(void)
 				of_node_full_name(np));
 	}
 }
+
+int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)
+{
+	struct iommu_fwspec *fwspec = dev->archdata.iommu;
+
+	if (fwspec)
+		return 0;
+
+	fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
+	if (!fwspec)
+		return -ENOMEM;
+
+	fwspec->iommu_np = of_node_get(iommu_np);
+	fwspec->iommu_ops = of_iommu_get_ops(iommu_np);
+	dev->archdata.iommu = fwspec;
+	return 0;
+}
+
+void iommu_fwspec_free(struct device *dev)
+{
+	struct iommu_fwspec *fwspec = dev->archdata.iommu;
+
+	if (fwspec) {
+		of_node_put(fwspec->iommu_np);
+		kfree(fwspec);
+	}
+}
+
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
+{
+	struct iommu_fwspec *fwspec = dev->archdata.iommu;
+	size_t size;
+
+	if (!fwspec)
+		return -EINVAL;
+
+	size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + 1]);
+	fwspec = krealloc(dev->archdata.iommu, size, GFP_KERNEL);
+	if (!fwspec)
+		return -ENOMEM;
+
+	while (num_ids--)
+		fwspec->ids[fwspec->num_ids++] = *ids++;
+
+	dev->archdata.iommu = fwspec;
+	return 0;
+}
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index bd02b44902d0..5a4f516cfcfe 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -15,6 +15,14 @@ extern void of_iommu_init(void);
 extern const struct iommu_ops *of_iommu_configure(struct device *dev,
 					struct device_node *master_np);
 
+struct iommu_fwspec {
+	const struct iommu_ops	*iommu_ops;
+	struct device_node	*iommu_np;
+	void			*iommu_priv;
+	unsigned int		num_ids;
+	u32			ids[];
+};
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -31,8 +39,18 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 	return NULL;
 }
 
+struct iommu_fwspec;
+
 #endif	/* CONFIG_OF_IOMMU */
 
+int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+static inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
+{
+	return dev->archdata.iommu;
+}
+
 void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
 const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
 
-- 
2.8.1.dirty

Re: [PATCH v3 6/9] iommu/of: Introduce iommu_fwspec

[Will Deacon]

On Tue, Jun 28, 2016 at 04:48:25PM +0100, Robin Murphy wrote:
> Introduce a common structure to hold the per-device firmware data that
> non-architectural IOMMU drivers generally need to keep track of.
> Initially this is DT-specific to complement the existing of_iommu
> support code, but will generalise further once other firmware methods
> (e.g. ACPI IORT) come along.
> 
> Ultimately the aim is to promote the fwspec to a first-class member of
> struct device, and handle the init/free automatically in the firmware
> code. That way we can have API calls look for dev->fwspec->iommu_ops
> before falling back to dev->bus->iommu_ops, and thus gracefully handle
> those troublesome multi-IOMMU systems which we currently cannot. To
> start with, though, make use of the existing archdata field and delegate
> the init/free to drivers to allow an incremental conversion rather than
> the impractical pain of trying to attempt everything in one go.
> 
> Suggested-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> v3: New.
> 
>  drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_iommu.h | 18 ++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 25406a8b9d4e..2b90c1f56ff2 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -220,3 +220,50 @@ void __init of_iommu_init(void)
>  				of_node_full_name(np));
>  	}
>  }
> +
> +int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)

These functions might want an of_* prefix to match the rest of the file,
or do you plan to re-use these later for ACPI?

> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index bd02b44902d0..5a4f516cfcfe 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -15,6 +15,14 @@ extern void of_iommu_init(void);
>  extern const struct iommu_ops *of_iommu_configure(struct device *dev,
>  					struct device_node *master_np);
>  
> +struct iommu_fwspec {
> +	const struct iommu_ops	*iommu_ops;
> +	struct device_node	*iommu_np;
> +	void			*iommu_priv;
> +	unsigned int		num_ids;
> +	u32			ids[];
> +};
> +
>  #else
>  
>  static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
> @@ -31,8 +39,18 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
>  	return NULL;
>  }
>  
> +struct iommu_fwspec;
> +
>  #endif	/* CONFIG_OF_IOMMU */
>  
> +int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
> +void iommu_fwspec_free(struct device *dev);
> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> +static inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
> +{
> +	return dev->archdata.iommu;
> +}

I'm a bit nervous about putting this inline in a header file, since not
all architectures unconditionally provide the iommu field in dev_archdata.

Will

Re: [PATCH v3 6/9] iommu/of: Introduce iommu_fwspec

[Robin Murphy]

On 01/07/16 11:55, Will Deacon wrote:
> On Tue, Jun 28, 2016 at 04:48:25PM +0100, Robin Murphy wrote:
>> Introduce a common structure to hold the per-device firmware data that
>> non-architectural IOMMU drivers generally need to keep track of.
>> Initially this is DT-specific to complement the existing of_iommu
>> support code, but will generalise further once other firmware methods
>> (e.g. ACPI IORT) come along.
>>
>> Ultimately the aim is to promote the fwspec to a first-class member of
>> struct device, and handle the init/free automatically in the firmware
>> code. That way we can have API calls look for dev->fwspec->iommu_ops
>> before falling back to dev->bus->iommu_ops, and thus gracefully handle
>> those troublesome multi-IOMMU systems which we currently cannot. To
>> start with, though, make use of the existing archdata field and delegate
>> the init/free to drivers to allow an incremental conversion rather than
>> the impractical pain of trying to attempt everything in one go.
>>
>> Suggested-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> v3: New.
>>
>>  drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of_iommu.h | 18 ++++++++++++++++++
>>  2 files changed, 65 insertions(+)
>>
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index 25406a8b9d4e..2b90c1f56ff2 100644
>> --- a/drivers/iommu/of_iommu.c
>> +++ b/drivers/iommu/of_iommu.c
>> @@ -220,3 +220,50 @@ void __init of_iommu_init(void)
>>  				of_node_full_name(np));
>>  	}
>>  }
>> +
>> +int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)
> 
> These functions might want an of_* prefix to match the rest of the file,
> or do you plan to re-use these later for ACPI?

Indeed, the naming is a deliberate concession to the future - I was
trying to pre-empt the idea that the interface might be intended to grow
into separate of_* and iort_* versions. I'm just using raw device_nodes
for the moment as I don't think we'd reached a consensus on whether
fwnode_handle or some other abstraction was the appropriate way to go.

>> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
>> index bd02b44902d0..5a4f516cfcfe 100644
>> --- a/include/linux/of_iommu.h
>> +++ b/include/linux/of_iommu.h
>> @@ -15,6 +15,14 @@ extern void of_iommu_init(void);
>>  extern const struct iommu_ops *of_iommu_configure(struct device *dev,
>>  					struct device_node *master_np);
>>  
>> +struct iommu_fwspec {
>> +	const struct iommu_ops	*iommu_ops;
>> +	struct device_node	*iommu_np;
>> +	void			*iommu_priv;
>> +	unsigned int		num_ids;
>> +	u32			ids[];
>> +};
>> +
>>  #else
>>  
>>  static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
>> @@ -31,8 +39,18 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
>>  	return NULL;
>>  }
>>  
>> +struct iommu_fwspec;
>> +
>>  #endif	/* CONFIG_OF_IOMMU */
>>  
>> +int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
>> +void iommu_fwspec_free(struct device *dev);
>> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>> +static inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
>> +{
>> +	return dev->archdata.iommu;
>> +}
> 
> I'm a bit nervous about putting this inline in a header file, since not
> all architectures unconditionally provide the iommu field in dev_archdata.

Ah yes, you're right, this shouldn't be defined without OF_IOMMU (or the
IORT equivalent) anyway. Clearly the premature optimisation demons had
begun whispering "A measly one-line accessor in a separate compilation
unit? Won't somebody think of the chil^Wbranch predictors!?"

Robin.

> 
> Will
> 

Re: [PATCH v3 6/9] iommu/of: Introduce iommu_fwspec

[Lorenzo Pieralisi]

On Fri, Jul 01, 2016 at 01:04:46PM +0100, Robin Murphy wrote:
> On 01/07/16 11:55, Will Deacon wrote:
> > On Tue, Jun 28, 2016 at 04:48:25PM +0100, Robin Murphy wrote:
> >> Introduce a common structure to hold the per-device firmware data that
> >> non-architectural IOMMU drivers generally need to keep track of.
> >> Initially this is DT-specific to complement the existing of_iommu
> >> support code, but will generalise further once other firmware methods
> >> (e.g. ACPI IORT) come along.
> >>
> >> Ultimately the aim is to promote the fwspec to a first-class member of
> >> struct device, and handle the init/free automatically in the firmware
> >> code. That way we can have API calls look for dev->fwspec->iommu_ops
> >> before falling back to dev->bus->iommu_ops, and thus gracefully handle
> >> those troublesome multi-IOMMU systems which we currently cannot. To
> >> start with, though, make use of the existing archdata field and delegate
> >> the init/free to drivers to allow an incremental conversion rather than
> >> the impractical pain of trying to attempt everything in one go.
> >>
> >> Suggested-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> >> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> >> ---
> >>
> >> v3: New.
> >>
> >>  drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/of_iommu.h | 18 ++++++++++++++++++
> >>  2 files changed, 65 insertions(+)
> >>
> >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> >> index 25406a8b9d4e..2b90c1f56ff2 100644
> >> --- a/drivers/iommu/of_iommu.c
> >> +++ b/drivers/iommu/of_iommu.c
> >> @@ -220,3 +220,50 @@ void __init of_iommu_init(void)
> >>  				of_node_full_name(np));
> >>  	}
> >>  }
> >> +
> >> +int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)
> > 
> > These functions might want an of_* prefix to match the rest of the file,
> > or do you plan to re-use these later for ACPI?
> 
> Indeed, the naming is a deliberate concession to the future - I was
> trying to pre-empt the idea that the interface might be intended to grow
> into separate of_* and iort_* versions. I'm just using raw device_nodes
> for the moment as I don't think we'd reached a consensus on whether
> fwnode_handle or some other abstraction was the appropriate way to go.

Yes, for the time being you could just consider the whole thing
OF specific and in my series providing IORT support I will have to
find a place for this code to live, likely to be a new compilation
unit anyway (which means that the raw device_node will become
fwnode_handle types too in the process).

> >> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> >> index bd02b44902d0..5a4f516cfcfe 100644
> >> --- a/include/linux/of_iommu.h
> >> +++ b/include/linux/of_iommu.h
> >> @@ -15,6 +15,14 @@ extern void of_iommu_init(void);
> >>  extern const struct iommu_ops *of_iommu_configure(struct device *dev,
> >>  					struct device_node *master_np);
> >>  
> >> +struct iommu_fwspec {
> >> +	const struct iommu_ops	*iommu_ops;
> >> +	struct device_node	*iommu_np;
> >> +	void			*iommu_priv;
> >> +	unsigned int		num_ids;
> >> +	u32			ids[];
> >> +};
> >> +
> >>  #else
> >>  
> >>  static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
> >> @@ -31,8 +39,18 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
> >>  	return NULL;
> >>  }
> >>  
> >> +struct iommu_fwspec;
> >> +
> >>  #endif	/* CONFIG_OF_IOMMU */
> >>  
> >> +int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
> >> +void iommu_fwspec_free(struct device *dev);
> >> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
> >> +static inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
> >> +{
> >> +	return dev->archdata.iommu;
> >> +}
> > 
> > I'm a bit nervous about putting this inline in a header file, since not
> > all architectures unconditionally provide the iommu field in dev_archdata.
> 
> Ah yes, you're right, this shouldn't be defined without OF_IOMMU (or the
> IORT equivalent) anyway. Clearly the premature optimisation demons had
> begun whispering "A measly one-line accessor in a separate compilation
> unit? Won't somebody think of the chil^Wbranch predictors!?"

Well, moving this inline to of_iommu.c (as per v4) improves things a
bit, but it looks a bit incosistent to me anyway given that we would
*assume* archdata.iommu exists if CONFIG_OF_IOMMU, which should be
true (is it on eg powerpc ?) but it is still an assumption AFAIK.

An option could be to add the generic iommu_fwspec API but for
the time being decoupling it from struct device and leaving it
to IOMMU drivers to decide where the iommu_fwspec structure pointer
should be stored (ie on ARM systems dev->archdata.iommu).

Understood, you want to keep struct device part of the API so
that, when the iommu_fwspec will be integrated into struct device
we won't have to change the iommu_fwspec API, still, it does not
look right.

Other option would be adding this code to a separate compilation
unit that is compiled only on (ARM || ARM64), not sure what's the
most reasonable option here.

Lorenzo

[PATCH v3 7/9] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Robin Murphy]

Now that we can properly describe the mapping between PCI RIDs and
stream IDs via "iommu-map", and have it fed it to the driver
automatically via of_xlate(), rework the SMMUv3 driver to benefit from
that, and get rid of the current misuse of the "iommus" binding.

Since having of_xlate wired up means that masters will now be given the
appropriate DMA ops, we also need to make sure that default domains work
properly. This necessitates dispensing with the "whole group at a time"
notion for attaching to a domain, as devices which share a group get
attached to the group's default domain one by one as they are initially
probed.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v3: Significant rework, now based around iommu_fwspec; avoid touching
    low-level STE code; drop separate "finish off default domains" patch

 drivers/iommu/arm-smmu-v3.c | 270 ++++++++++++++++++--------------------------
 1 file changed, 113 insertions(+), 157 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 94b68213c50d..8ce2a4f9342b 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -30,6 +30,7 @@
 #include <linux/msi.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_iommu.h>
 #include <linux/of_platform.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
@@ -606,12 +607,9 @@ struct arm_smmu_device {
 	struct arm_smmu_strtab_cfg	strtab_cfg;
 };
 
-/* SMMU private data for an IOMMU group */
-struct arm_smmu_group {
+/* SMMU private data for each master */
+struct arm_smmu_master_data {
 	struct arm_smmu_device		*smmu;
-	struct arm_smmu_domain		*domain;
-	int				num_sids;
-	u32				*sids;
 	struct arm_smmu_strtab_ent	ste;
 };
 
@@ -1575,20 +1573,6 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 	return ret;
 }
 
-static struct arm_smmu_group *arm_smmu_group_get(struct device *dev)
-{
-	struct iommu_group *group;
-	struct arm_smmu_group *smmu_group;
-
-	group = iommu_group_get(dev);
-	if (!group)
-		return NULL;
-
-	smmu_group = iommu_group_get_iommudata(group);
-	iommu_group_put(group);
-	return smmu_group;
-}
-
 static __le64 *arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid)
 {
 	__le64 *step;
@@ -1611,27 +1595,17 @@ static __le64 *arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid)
 	return step;
 }
 
-static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
+static int arm_smmu_install_ste_for_dev(struct iommu_fwspec *fwspec)
 {
 	int i;
-	struct arm_smmu_domain *smmu_domain = smmu_group->domain;
-	struct arm_smmu_strtab_ent *ste = &smmu_group->ste;
-	struct arm_smmu_device *smmu = smmu_group->smmu;
+	struct arm_smmu_master_data *master = fwspec->iommu_priv;
+	struct arm_smmu_device *smmu = master->smmu;
 
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		ste->s1_cfg = &smmu_domain->s1_cfg;
-		ste->s2_cfg = NULL;
-		arm_smmu_write_ctx_desc(smmu, ste->s1_cfg);
-	} else {
-		ste->s1_cfg = NULL;
-		ste->s2_cfg = &smmu_domain->s2_cfg;
-	}
-
-	for (i = 0; i < smmu_group->num_sids; ++i) {
-		u32 sid = smmu_group->sids[i];
+	for (i = 0; i < fwspec->num_ids; ++i) {
+		u32 sid = fwspec->ids[i];
 		__le64 *step = arm_smmu_get_step_for_sid(smmu, sid);
 
-		arm_smmu_write_strtab_ent(smmu, sid, step, ste);
+		arm_smmu_write_strtab_ent(smmu, sid, step, &master->ste);
 	}
 
 	return 0;
@@ -1639,13 +1613,12 @@ static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
 
 static void arm_smmu_detach_dev(struct device *dev)
 {
-	struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
+	struct arm_smmu_master_data *master = fwspec->iommu_priv;
 
-	smmu_group->ste.bypass = true;
-	if (arm_smmu_install_ste_for_group(smmu_group) < 0)
+	master->ste.bypass = true;
+	if (arm_smmu_install_ste_for_dev(fwspec) < 0)
 		dev_warn(dev, "failed to install bypass STE\n");
-
-	smmu_group->domain = NULL;
 }
 
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -1653,16 +1626,21 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	int ret = 0;
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
+	struct arm_smmu_master_data *master;
+	struct arm_smmu_strtab_ent *ste;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
 
-	if (!smmu_group)
+	if (!fwspec)
 		return -ENOENT;
 
+	master = fwspec->iommu_priv;
+	smmu = master->smmu;
+	ste = &master->ste;
+
 	/* Already attached to a different domain? */
-	if (smmu_group->domain && smmu_group->domain != smmu_domain)
+	if (!ste->bypass)
 		arm_smmu_detach_dev(dev);
 
-	smmu = smmu_group->smmu;
 	mutex_lock(&smmu_domain->init_mutex);
 
 	if (!smmu_domain->smmu) {
@@ -1681,21 +1659,21 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		goto out_unlock;
 	}
 
-	/* Group already attached to this domain? */
-	if (smmu_group->domain)
-		goto out_unlock;
+	ste->bypass = false;
+	ste->valid = true;
 
-	smmu_group->domain	= smmu_domain;
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
+		ste->s1_cfg = &smmu_domain->s1_cfg;
+		ste->s2_cfg = NULL;
+		arm_smmu_write_ctx_desc(smmu, ste->s1_cfg);
+	} else {
+		ste->s1_cfg = NULL;
+		ste->s2_cfg = &smmu_domain->s2_cfg;
+	}
 
-	/*
-	 * FIXME: This should always be "false" once we have IOMMU-backed
-	 * DMA ops for all devices behind the SMMU.
-	 */
-	smmu_group->ste.bypass	= domain->type == IOMMU_DOMAIN_DMA;
-
-	ret = arm_smmu_install_ste_for_group(smmu_group);
+	ret = arm_smmu_install_ste_for_dev(fwspec);
 	if (ret < 0)
-		smmu_group->domain = NULL;
+		ste->valid = false;
 
 out_unlock:
 	mutex_unlock(&smmu_domain->init_mutex);
@@ -1754,40 +1732,14 @@ arm_smmu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
 	return ret;
 }
 
-static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *sidp)
+static struct arm_smmu_device *arm_smmu_get_by_node(struct device_node *np)
 {
-	*(u32 *)sidp = alias;
-	return 0; /* Continue walking */
-}
+	struct platform_device *smmu_pdev = of_find_device_by_node(np);
 
-static void __arm_smmu_release_pci_iommudata(void *data)
-{
-	kfree(data);
-}
-
-static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
-{
-	struct device_node *of_node;
-	struct platform_device *smmu_pdev;
-	struct arm_smmu_device *smmu = NULL;
-	struct pci_bus *bus = pdev->bus;
-
-	/* Walk up to the root bus */
-	while (!pci_is_root_bus(bus))
-		bus = bus->parent;
-
-	/* Follow the "iommus" phandle from the host controller */
-	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
-	if (!of_node)
+	if (!smmu_pdev)
 		return NULL;
 
-	/* See if we can find an SMMU corresponding to the phandle */
-	smmu_pdev = of_find_device_by_node(of_node);
-	if (smmu_pdev)
-		smmu = platform_get_drvdata(smmu_pdev);
-
-	of_node_put(of_node);
-	return smmu;
+	return platform_get_drvdata(smmu_pdev);
 }
 
 static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
@@ -1800,94 +1752,74 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 	return sid < limit;
 }
 
+static struct iommu_ops arm_smmu_ops;
+
 static int arm_smmu_add_device(struct device *dev)
 {
 	int i, ret;
-	u32 sid, *sids;
-	struct pci_dev *pdev;
-	struct iommu_group *group;
-	struct arm_smmu_group *smmu_group;
 	struct arm_smmu_device *smmu;
+	struct arm_smmu_master_data *master;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
+	struct iommu_group *group;
 
-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
+	if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
 		return -ENODEV;
-
-	pdev = to_pci_dev(dev);
-	group = iommu_group_get_for_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
-	smmu_group = iommu_group_get_iommudata(group);
-	if (!smmu_group) {
-		smmu = arm_smmu_get_for_pci_dev(pdev);
-		if (!smmu) {
-			ret = -ENOENT;
-			goto out_remove_dev;
-		}
-
-		smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
-		if (!smmu_group) {
-			ret = -ENOMEM;
-			goto out_remove_dev;
-		}
-
-		smmu_group->ste.valid	= true;
-		smmu_group->smmu	= smmu;
-		iommu_group_set_iommudata(group, smmu_group,
-					  __arm_smmu_release_pci_iommudata);
+	/*
+	 * We _can_ actually withstand dodgy bus code re-calling add_device()
+	 * without an intervening remove_device()/of_xlate() sequence, but
+	 * we're not going to do so quietly...
+	 */
+	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
+		master = fwspec->iommu_priv;
+		smmu = master->smmu;
 	} else {
-		smmu = smmu_group->smmu;
+		smmu = arm_smmu_get_by_node(fwspec->iommu_np);
+		if (!smmu)
+			return -ENODEV;
+		master = kzalloc(sizeof(*master), GFP_KERNEL);
+		if (!master)
+			return -ENOMEM;
+
+		master->smmu = smmu;
+		fwspec->iommu_priv = master;
 	}
 
-	/* Assume SID == RID until firmware tells us otherwise */
-	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
-	for (i = 0; i < smmu_group->num_sids; ++i) {
-		/* If we already know about this SID, then we're done */
-		if (smmu_group->sids[i] == sid)
-			goto out_put_group;
+	/* Check the SIDs are in range of the SMMU and our stream table */
+	for (i = 0; i < fwspec->num_ids; i++) {
+		u32 sid = fwspec->ids[i];
+
+		if (!arm_smmu_sid_in_range(smmu, sid))
+			return -ERANGE;
+
+		/* Ensure l2 strtab is initialised */
+		if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
+			ret = arm_smmu_init_l2_strtab(smmu, sid);
+			if (ret)
+				return ret;
+		}
 	}
 
-	/* Check the SID is in range of the SMMU and our stream table */
-	if (!arm_smmu_sid_in_range(smmu, sid)) {
-		ret = -ERANGE;
-		goto out_remove_dev;
-	}
+	group = iommu_group_get_for_dev(dev);
+	if (!IS_ERR(group))
+		iommu_group_put(group);
 
-	/* Ensure l2 strtab is initialised */
-	if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
-		ret = arm_smmu_init_l2_strtab(smmu, sid);
-		if (ret)
-			goto out_remove_dev;
-	}
-
-	/* Resize the SID array for the group */
-	smmu_group->num_sids++;
-	sids = krealloc(smmu_group->sids, smmu_group->num_sids * sizeof(*sids),
-			GFP_KERNEL);
-	if (!sids) {
-		smmu_group->num_sids--;
-		ret = -ENOMEM;
-		goto out_remove_dev;
-	}
-
-	/* Add the new SID */
-	sids[smmu_group->num_sids - 1] = sid;
-	smmu_group->sids = sids;
-
-out_put_group:
-	iommu_group_put(group);
-	return 0;
-
-out_remove_dev:
-	iommu_group_remove_device(dev);
-	iommu_group_put(group);
-	return ret;
+	return PTR_ERR_OR_ZERO(group);
 }
 
 static void arm_smmu_remove_device(struct device *dev)
 {
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
+	struct arm_smmu_master_data *master;
+
+	if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
+		return;
+
+	master = fwspec->iommu_priv;
+	if (master && master->ste.valid)
+		arm_smmu_detach_dev(dev);
 	iommu_group_remove_device(dev);
+	kfree(master);
+	iommu_fwspec_free(dev);
 }
 
 static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
@@ -1934,6 +1866,21 @@ out_unlock:
 	return ret;
 }
 
+static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	int ret;
+
+	/* We only support PCI, for now */
+	if (!dev_is_pci(dev))
+		return -ENODEV;
+
+	ret = iommu_fwspec_init(dev, args->np);
+	if (!ret)
+		ret = iommu_fwspec_add_ids(dev, &args->args[0], 1);
+
+	return ret;
+}
+
 static struct iommu_ops arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
@@ -1947,6 +1894,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.device_group		= pci_device_group,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
+	.of_xlate		= arm_smmu_of_xlate,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 };
 
@@ -2697,6 +2645,14 @@ static void __exit arm_smmu_exit(void)
 subsys_initcall(arm_smmu_init);
 module_exit(arm_smmu_exit);
 
+static int __init arm_smmu_of_init(struct device_node *np)
+{
+	of_iommu_set_ops(np, &arm_smmu_ops);
+
+	return 0;
+}
+IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", arm_smmu_of_init);
+
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
 MODULE_AUTHOR("Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>");
 MODULE_LICENSE("GPL v2");
-- 
2.8.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/9] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Will Deacon]

On Tue, Jun 28, 2016 at 04:48:26PM +0100, Robin Murphy wrote:
> Now that we can properly describe the mapping between PCI RIDs and
> stream IDs via "iommu-map", and have it fed it to the driver
> automatically via of_xlate(), rework the SMMUv3 driver to benefit from
> that, and get rid of the current misuse of the "iommus" binding.
> 
> Since having of_xlate wired up means that masters will now be given the
> appropriate DMA ops, we also need to make sure that default domains work
> properly. This necessitates dispensing with the "whole group at a time"
> notion for attaching to a domain, as devices which share a group get
> attached to the group's default domain one by one as they are initially
> probed.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---

[...]

>  static int arm_smmu_add_device(struct device *dev)
>  {
>  	int i, ret;
> -	u32 sid, *sids;
> -	struct pci_dev *pdev;
> -	struct iommu_group *group;
> -	struct arm_smmu_group *smmu_group;
>  	struct arm_smmu_device *smmu;
> +	struct arm_smmu_master_data *master;
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
> +	struct iommu_group *group;
>  
> -	/* We only support PCI, for now */
> -	if (!dev_is_pci(dev))
> +	if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
>  		return -ENODEV;
> -
> -	pdev = to_pci_dev(dev);
> -	group = iommu_group_get_for_dev(dev);
> -	if (IS_ERR(group))
> -		return PTR_ERR(group);
> -
> -	smmu_group = iommu_group_get_iommudata(group);
> -	if (!smmu_group) {
> -		smmu = arm_smmu_get_for_pci_dev(pdev);
> -		if (!smmu) {
> -			ret = -ENOENT;
> -			goto out_remove_dev;
> -		}
> -
> -		smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
> -		if (!smmu_group) {
> -			ret = -ENOMEM;
> -			goto out_remove_dev;
> -		}
> -
> -		smmu_group->ste.valid	= true;
> -		smmu_group->smmu	= smmu;
> -		iommu_group_set_iommudata(group, smmu_group,
> -					  __arm_smmu_release_pci_iommudata);
> +	/*
> +	 * We _can_ actually withstand dodgy bus code re-calling add_device()
> +	 * without an intervening remove_device()/of_xlate() sequence, but
> +	 * we're not going to do so quietly...
> +	 */
> +	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
> +		master = fwspec->iommu_priv;
> +		smmu = master->smmu;
>  	} else {
> -		smmu = smmu_group->smmu;
> +		smmu = arm_smmu_get_by_node(fwspec->iommu_np);
> +		if (!smmu)
> +			return -ENODEV;
> +		master = kzalloc(sizeof(*master), GFP_KERNEL);
> +		if (!master)
> +			return -ENOMEM;
> +
> +		master->smmu = smmu;
> +		fwspec->iommu_priv = master;
>  	}
>  
> -	/* Assume SID == RID until firmware tells us otherwise */
> -	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
> -	for (i = 0; i < smmu_group->num_sids; ++i) {
> -		/* If we already know about this SID, then we're done */
> -		if (smmu_group->sids[i] == sid)
> -			goto out_put_group;
> +	/* Check the SIDs are in range of the SMMU and our stream table */
> +	for (i = 0; i < fwspec->num_ids; i++) {
> +		u32 sid = fwspec->ids[i];
> +
> +		if (!arm_smmu_sid_in_range(smmu, sid))
> +			return -ERANGE;
> +
> +		/* Ensure l2 strtab is initialised */
> +		if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
> +			ret = arm_smmu_init_l2_strtab(smmu, sid);
> +			if (ret)
> +				return ret;
> +		}
>  	}

Do you not need to do some cleanup here, rather than just returning (same
for the -ERANGE above)? For example, freeing the master?

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/9] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Robin Murphy]

On 01/07/16 13:35, Will Deacon wrote:
> On Tue, Jun 28, 2016 at 04:48:26PM +0100, Robin Murphy wrote:
>> Now that we can properly describe the mapping between PCI RIDs and
>> stream IDs via "iommu-map", and have it fed it to the driver
>> automatically via of_xlate(), rework the SMMUv3 driver to benefit from
>> that, and get rid of the current misuse of the "iommus" binding.
>>
>> Since having of_xlate wired up means that masters will now be given the
>> appropriate DMA ops, we also need to make sure that default domains work
>> properly. This necessitates dispensing with the "whole group at a time"
>> notion for attaching to a domain, as devices which share a group get
>> attached to the group's default domain one by one as they are initially
>> probed.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
> 
> [...]
> 
>>  static int arm_smmu_add_device(struct device *dev)
>>  {
>>  	int i, ret;
>> -	u32 sid, *sids;
>> -	struct pci_dev *pdev;
>> -	struct iommu_group *group;
>> -	struct arm_smmu_group *smmu_group;
>>  	struct arm_smmu_device *smmu;
>> +	struct arm_smmu_master_data *master;
>> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
>> +	struct iommu_group *group;
>>  
>> -	/* We only support PCI, for now */
>> -	if (!dev_is_pci(dev))
>> +	if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
>>  		return -ENODEV;
>> -
>> -	pdev = to_pci_dev(dev);
>> -	group = iommu_group_get_for_dev(dev);
>> -	if (IS_ERR(group))
>> -		return PTR_ERR(group);
>> -
>> -	smmu_group = iommu_group_get_iommudata(group);
>> -	if (!smmu_group) {
>> -		smmu = arm_smmu_get_for_pci_dev(pdev);
>> -		if (!smmu) {
>> -			ret = -ENOENT;
>> -			goto out_remove_dev;
>> -		}
>> -
>> -		smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
>> -		if (!smmu_group) {
>> -			ret = -ENOMEM;
>> -			goto out_remove_dev;
>> -		}
>> -
>> -		smmu_group->ste.valid	= true;
>> -		smmu_group->smmu	= smmu;
>> -		iommu_group_set_iommudata(group, smmu_group,
>> -					  __arm_smmu_release_pci_iommudata);
>> +	/*
>> +	 * We _can_ actually withstand dodgy bus code re-calling add_device()
>> +	 * without an intervening remove_device()/of_xlate() sequence, but
>> +	 * we're not going to do so quietly...
>> +	 */
>> +	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
>> +		master = fwspec->iommu_priv;
>> +		smmu = master->smmu;
>>  	} else {
>> -		smmu = smmu_group->smmu;
>> +		smmu = arm_smmu_get_by_node(fwspec->iommu_np);
>> +		if (!smmu)
>> +			return -ENODEV;
>> +		master = kzalloc(sizeof(*master), GFP_KERNEL);
>> +		if (!master)
>> +			return -ENOMEM;
>> +
>> +		master->smmu = smmu;
>> +		fwspec->iommu_priv = master;
>>  	}
>>  
>> -	/* Assume SID == RID until firmware tells us otherwise */
>> -	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
>> -	for (i = 0; i < smmu_group->num_sids; ++i) {
>> -		/* If we already know about this SID, then we're done */
>> -		if (smmu_group->sids[i] == sid)
>> -			goto out_put_group;
>> +	/* Check the SIDs are in range of the SMMU and our stream table */
>> +	for (i = 0; i < fwspec->num_ids; i++) {
>> +		u32 sid = fwspec->ids[i];
>> +
>> +		if (!arm_smmu_sid_in_range(smmu, sid))
>> +			return -ERANGE;
>> +
>> +		/* Ensure l2 strtab is initialised */
>> +		if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
>> +			ret = arm_smmu_init_l2_strtab(smmu, sid);
>> +			if (ret)
>> +				return ret;
>> +		}
>>  	}
> 
> Do you not need to do some cleanup here, rather than just returning (same
> for the -ERANGE above)? For example, freeing the master?

The master will be freed in arm_smmu_remove_device() if and when the
device is removed from the bus. By the time we get this add_device call,
we're already past the point of it being added to the bus and there's
nothing we can do (all the return value here actually does is terminate
the notifier chain). By bailing out before assigning the group, though,
we prevent the device from being valid for any VFIO/IOMMU API usage.

That does make me think, however, that it would be worth trying to
detect in arch_setup_dma_ops() if a device has been rejected by an IOMMU
it was supposed to be associated with, and in such cases prevent it from
doing DMA *at all* rather than just leaving it with the default SWIOTLB
ops. I'll have a think about how feasible that is (with any luck it
should slot fairly neatly into the probe ordering work touching those
areas).

Robin.

> 
> Will
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 8/9] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Robin Murphy]

With the device <-> stream ID relationship suitably abstracted and
of_xlate() hooked up, the PCI dependency now looks, and is, entirely
arbitrary. Any bus using the of_dma_configure() mechanism will work,
so extend support to the platform and AMBA buses which do just that.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v3: Now actually tested; improve comment about duplicate IDs.

 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/arm-smmu-v3.c | 42 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index ad0860383cb3..d1c66afefeed 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -308,7 +308,7 @@ config ARM_SMMU
 
 config ARM_SMMU_V3
 	bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
-	depends on ARM64 && PCI
+	depends on ARM64
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select GENERIC_MSI_IRQ_DOMAIN
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 8ce2a4f9342b..735690e03818 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -35,6 +35,8 @@
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 
+#include <linux/amba/bus.h>
+
 #include "io-pgtable.h"
 
 /* MMIO registers */
@@ -1822,6 +1824,25 @@ static void arm_smmu_remove_device(struct device *dev)
 	iommu_fwspec_free(dev);
 }
 
+static struct iommu_group *arm_smmu_device_group(struct device *dev)
+{
+	struct iommu_group *group;
+
+	/*
+	 * The difficulty of efficient stream-ID-to-device lookup prevents us
+	 * from reasonably detecting aliasing outside of PCI buses, but for the
+	 * the same underlying reason (a sparse 32-bit ID space) there's also
+	 * little excuse for systems to be wired up with non-unique IDs in the
+	 * first place; consider them unsupported.
+	 */
+	if (dev_is_pci(dev))
+		group = pci_device_group(dev);
+	else
+		group = generic_device_group(dev);
+
+	return group;
+}
+
 static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
 				    enum iommu_attr attr, void *data)
 {
@@ -1868,13 +1889,8 @@ out_unlock:
 
 static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 {
-	int ret;
+	int ret = iommu_fwspec_init(dev, args->np);
 
-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
-		return -ENODEV;
-
-	ret = iommu_fwspec_init(dev, args->np);
 	if (!ret)
 		ret = iommu_fwspec_add_ids(dev, &args->args[0], 1);
 
@@ -1891,7 +1907,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.iova_to_phys		= arm_smmu_iova_to_phys,
 	.add_device		= arm_smmu_add_device,
 	.remove_device		= arm_smmu_remove_device,
-	.device_group		= pci_device_group,
+	.device_group		= arm_smmu_device_group,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
 	.of_xlate		= arm_smmu_of_xlate,
@@ -2634,7 +2650,17 @@ static int __init arm_smmu_init(void)
 	if (ret)
 		return ret;
 
-	return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
+#ifdef CONFIG_PCI
+	ret = bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
+	if (ret)
+		return ret;
+#endif
+#ifdef CONFIG_ARM_AMBA
+	ret = bus_set_iommu(&amba_bustype, &arm_smmu_ops);
+	if (ret)
+		return ret;
+#endif
+	return bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
 }
 
 static void __exit arm_smmu_exit(void)
-- 
2.8.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 8/9] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Will Deacon]

On Tue, Jun 28, 2016 at 04:48:27PM +0100, Robin Murphy wrote:
> With the device <-> stream ID relationship suitably abstracted and
> of_xlate() hooked up, the PCI dependency now looks, and is, entirely
> arbitrary. Any bus using the of_dma_configure() mechanism will work,
> so extend support to the platform and AMBA buses which do just that.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> v3: Now actually tested; improve comment about duplicate IDs.
> 
>  drivers/iommu/Kconfig       |  2 +-
>  drivers/iommu/arm-smmu-v3.c | 42 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index ad0860383cb3..d1c66afefeed 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -308,7 +308,7 @@ config ARM_SMMU
>  
>  config ARM_SMMU_V3
>  	bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
> -	depends on ARM64 && PCI
> +	depends on ARM64
>  	select IOMMU_API
>  	select IOMMU_IO_PGTABLE_LPAE
>  	select GENERIC_MSI_IRQ_DOMAIN
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 8ce2a4f9342b..735690e03818 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -35,6 +35,8 @@
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  
> +#include <linux/amba/bus.h>
> +
>  #include "io-pgtable.h"
>  
>  /* MMIO registers */
> @@ -1822,6 +1824,25 @@ static void arm_smmu_remove_device(struct device *dev)
>  	iommu_fwspec_free(dev);
>  }
>  
> +static struct iommu_group *arm_smmu_device_group(struct device *dev)
> +{
> +	struct iommu_group *group;
> +
> +	/*
> +	 * The difficulty of efficient stream-ID-to-device lookup prevents us
> +	 * from reasonably detecting aliasing outside of PCI buses, but for the
> +	 * the same underlying reason (a sparse 32-bit ID space) there's also

extra 'the'

> +	 * little excuse for systems to be wired up with non-unique IDs in the
> +	 * first place; consider them unsupported.
> +	 */

This is a long-winded way of saying "we don't support aliasing SIDs outside
of PCI".

Will

Re: [PATCH v3 8/9] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Robin Murphy]

On 01/07/16 13:40, Will Deacon wrote:
> On Tue, Jun 28, 2016 at 04:48:27PM +0100, Robin Murphy wrote:
>> With the device <-> stream ID relationship suitably abstracted and
>> of_xlate() hooked up, the PCI dependency now looks, and is, entirely
>> arbitrary. Any bus using the of_dma_configure() mechanism will work,
>> so extend support to the platform and AMBA buses which do just that.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> v3: Now actually tested; improve comment about duplicate IDs.
>>
>>  drivers/iommu/Kconfig       |  2 +-
>>  drivers/iommu/arm-smmu-v3.c | 42 ++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index ad0860383cb3..d1c66afefeed 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -308,7 +308,7 @@ config ARM_SMMU
>>  
>>  config ARM_SMMU_V3
>>  	bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support"
>> -	depends on ARM64 && PCI
>> +	depends on ARM64
>>  	select IOMMU_API
>>  	select IOMMU_IO_PGTABLE_LPAE
>>  	select GENERIC_MSI_IRQ_DOMAIN
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 8ce2a4f9342b..735690e03818 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -35,6 +35,8 @@
>>  #include <linux/pci.h>
>>  #include <linux/platform_device.h>
>>  
>> +#include <linux/amba/bus.h>
>> +
>>  #include "io-pgtable.h"
>>  
>>  /* MMIO registers */
>> @@ -1822,6 +1824,25 @@ static void arm_smmu_remove_device(struct device *dev)
>>  	iommu_fwspec_free(dev);
>>  }
>>  
>> +static struct iommu_group *arm_smmu_device_group(struct device *dev)
>> +{
>> +	struct iommu_group *group;
>> +
>> +	/*
>> +	 * The difficulty of efficient stream-ID-to-device lookup prevents us
>> +	 * from reasonably detecting aliasing outside of PCI buses, but for the
>> +	 * the same underlying reason (a sparse 32-bit ID space) there's also
> 
> extra 'the'

Oops, thanks.

>> +	 * little excuse for systems to be wired up with non-unique IDs in the
>> +	 * first place; consider them unsupported.
>> +	 */
> 
> This is a long-winded way of saying "we don't support aliasing SIDs outside
> of PCI".

It is. Someone said of the original terse 2-line comment "Worse: what if
a SID in the DT aliases with a PCI master? It might be nice to have some
basic sanity checking, at least.", so I expanded it to better clarify
why there isn't. I'll try making it a little less florid.

Robin.

> 
> Will
> 

[PATCH v3 9/9] iommu/arm-smmu: Set PRIVCFG in stage 1 STEs

[Robin Murphy]

Implement the SMMUv3 equivalent of d346180e70b9 ("iommu/arm-smmu: Treat
all device transactions as unprivileged"), so that once again those
pesky DMA controllers with their privileged instruction fetches don't
unexpectedly fault in stage 1 domains due to VMSAv8 rules.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v3: New.

 drivers/iommu/arm-smmu-v3.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 735690e03818..e4c3916efba9 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -263,6 +263,9 @@
 #define STRTAB_STE_1_SHCFG_INCOMING	1UL
 #define STRTAB_STE_1_SHCFG_SHIFT	44
 
+#define STRTAB_STE_1_PRIVCFG_UNPRIV	2UL
+#define STRTAB_STE_1_PRIVCFG_SHIFT	48
+
 #define STRTAB_STE_2_S2VMID_SHIFT	0
 #define STRTAB_STE_2_S2VMID_MASK	0xffffUL
 #define STRTAB_STE_2_VTCR_SHIFT		32
@@ -1070,7 +1073,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 #ifdef CONFIG_PCI_ATS
 			 STRTAB_STE_1_EATS_TRANS << STRTAB_STE_1_EATS_SHIFT |
 #endif
-			 STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT);
+			 STRTAB_STE_1_STRW_NSEL1 << STRTAB_STE_1_STRW_SHIFT |
+			 STRTAB_STE_1_PRIVCFG_UNPRIV <<
+			 STRTAB_STE_1_PRIVCFG_SHIFT);
 
 		if (smmu->features & ARM_SMMU_FEAT_STALLS)
 			dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
-- 
2.8.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html