[PATCH v4 0/8] Generic DT bindings for PCI IOMMUs and ARM SMMUv3

[PATCH v4 0/8] Generic DT bindings for PCI IOMMUs and ARM SMMUv3

[Robin Murphy]

Here's a quick repost to address the comments on v3[1], in the hope of
making some progress while I'm away next week.

I've dropped the workaround consolidation patch (and added the
equivalent to patch 6) since it's as much of a step sideways as in the
right direction, so we may as well hold off until _really_ fixing probe
ordering. Beyond that, a few minor tweaks per Will's comments.

Robin.

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

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 (6):
  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                        | 313 ++++++++++-----------
 drivers/iommu/of_iommu.c                           |  95 ++++++-
 drivers/of/irq.c                                   |  78 +----
 drivers/of/of_pci.c                                | 102 +++++++
 include/linux/of_iommu.h                           |  15 +
 include/linux/of_pci.h                             |  10 +
 9 files changed, 549 insertions(+), 259 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt

-- 
2.8.1.dirty

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

[Robin Murphy]

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

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@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
[rm: get rid of other now-redundant bits]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change.

 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

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

[Will Deacon]

Hi Catalin,

On Fri, Jul 01, 2016 at 05:50:10PM +0100, Robin Murphy wrote:
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> 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@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> [rm: get rid of other now-redundant bits]
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v4: No change.
> 
>  arch/arm64/mm/dma-mapping.c | 22 +++++-----------------
>  1 file changed, 5 insertions(+), 17 deletions(-)

Whilst this series is unlikely to make it for 4.8, this patch is
independent and it would be good to see it queued in the arm64 tree for
4.8, if possible. It shouldn't change anything on its own, but it's a
prerequisite for this series and anything on the IORT side from Lorenzo,
so it makes sense to me to keep the delta down if we can.

Will

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

[Catalin Marinas]

On Fri, Jul 08, 2016 at 03:55:21PM +0100, Will Deacon wrote:
> On Fri, Jul 01, 2016 at 05:50:10PM +0100, Robin Murphy wrote:
> > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > 
> > 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@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > [rm: get rid of other now-redundant bits]
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> > 
> > v4: No change.
> > 
> >  arch/arm64/mm/dma-mapping.c | 22 +++++-----------------
> >  1 file changed, 5 insertions(+), 17 deletions(-)
> 
> Whilst this series is unlikely to make it for 4.8, this patch is
> independent and it would be good to see it queued in the arm64 tree for
> 4.8, if possible. It shouldn't change anything on its own, but it's a
> prerequisite for this series and anything on the IORT side from Lorenzo,
> so it makes sense to me to keep the delta down if we can.

I queued it for 4.8. Thanks.

-- 
Catalin

[PATCH v4 2/8] Docs: dt: add PCI IOMMU map bindings

[Robin Murphy]

From: Mark Rutland <mark.rutland@arm.com>

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@kernel.org>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---

v4: No change.

 .../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 at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	pci: pci at 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 at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	pci: pci at 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 at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	pci: pci at 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 at a {
+		reg = <0xa 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	iommu_b: iommu at b {
+		reg = <0xb 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	iommu_c: iommu at c {
+		reg = <0xc 0x1>;
+		compatible = "vendor,some-iommu";
+		#iommu-cells = <1>;
+	};
+
+	pci: pci at 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

[PATCH v4 3/8] 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@kernel.org>
CC: Frank Rowand <frowand.list@gmail.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change.

 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

[PATCH v4 3/8] of/irq: Break out msi-map lookup (again)

[Will Deacon]

On Fri, Jul 01, 2016 at 05:50:12PM +0100, Robin Murphy wrote:
> 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@kernel.org>
> CC: Frank Rowand <frowand.list@gmail.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v4: No change.
> 
>  drivers/of/irq.c       |  78 ++-----------------------------------
>  drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_pci.h |  10 +++++
>  3 files changed, 116 insertions(+), 74 deletions(-)

Whilst this looks good to me, I think we need an Ack from the DT
maintainers.

Rob, could you have a look please?

Thanks,

Will

> 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
> 

[PATCH v4 3/8] of/irq: Break out msi-map lookup (again)

[Rob Herring]

On Fri, Jul 1, 2016 at 11:50 AM, Robin Murphy <robin.murphy@arm.com> wrote:
> 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@kernel.org>
> CC: Frank Rowand <frowand.list@gmail.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

I've only skimmed thru it, but looks fine to me:

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

[PATCH v4 4/8] 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@arm.com>
---

v4: No change.

 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 af499aea0a1a..1fe1f620f79d 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/slab.h>
 
 static const struct of_device_id __iommu_of_table_sentinel
@@ -134,20 +135,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

[PATCH v4 5/8] 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@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: Move dev_iommu_fwspec() definition out-of-line.

 drivers/iommu/of_iommu.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 15 ++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 1fe1f620f79d..4618e89d6a37 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -216,3 +216,55 @@ 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;
+}
+
+inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
+{
+	return dev->archdata.iommu;
+}
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index bd02b44902d0..308791fca32d 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,15 @@ 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);
+struct iommu_fwspec *dev_iommu_fwspec(struct device *dev);
+
 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

[PATCH v4 5/8] iommu/of: Introduce iommu_fwspec

[Lorenzo Pieralisi]

On Fri, Jul 01, 2016 at 05:50:14PM +0100, Robin Murphy wrote:

[...]

> +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]);
								^^^^

+ num_ids ?

Lorenzo

> +	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;
> +}
> +
> +inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
> +{
> +	return dev->archdata.iommu;
> +}
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index bd02b44902d0..308791fca32d 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,15 @@ 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);
> +struct iommu_fwspec *dev_iommu_fwspec(struct device *dev);
> +
>  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
> 

[PATCH v4 5/8] iommu/of: Introduce iommu_fwspec

[Robin Murphy]

On 07/07/16 17:56, Lorenzo Pieralisi wrote:
> On Fri, Jul 01, 2016 at 05:50:14PM +0100, Robin Murphy wrote:
> 
> [...]
> 
>> +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]);
> 								^^^^
> 
> + num_ids ?

Oops, quite right. Thanks!

Robin.

> Lorenzo
> 
>> +	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;
>> +}
>> +
>> +inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
>> +{
>> +	return dev->archdata.iommu;
>> +}
>> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
>> index bd02b44902d0..308791fca32d 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,15 @@ 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);
>> +struct iommu_fwspec *dev_iommu_fwspec(struct device *dev);
>> +
>>  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
>>
> 

[PATCH v4 6/8] 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@arm.com>
---

v4: Add explicit device probe in of_init callback.

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

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 94b68213c50d..fb438664b9f0 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,22 @@ 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)
+{
+	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;
+}
+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@arm.com>");
 MODULE_LICENSE("GPL v2");
-- 
2.8.1.dirty

[PATCH v4 6/8] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Lorenzo Pieralisi]

On Fri, Jul 01, 2016 at 05:50:15PM +0100, Robin Murphy wrote:

[...]

> +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;

Given that a) the check above is removed in a later patch and b)
code below does not depend on SMMU v3, I think the aim should
be to make this a core function (ie I am asking this since I will
need it in IORT based translation and I do not want to add yet another
*_xlate hook to iommu_op), iommu_fwspec_xlate() ?

What I will do with my next RFC is move the iommu_fwspec out of
OF_IOMMU code in a separate compilation unit and we will take the
discussion from there.

[...]

> +
> +	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,22 @@ 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)
> +{
> +	static bool registered;
> +
> +	if (!registered)
> +		registered = !arm_smmu_init();

We also need a static variable in arm_smmu_init() to make sure
we do not try to execute it multiple times :( (here and
subsys_initcall).

Thanks,
Lorenzo

> +
> +	if (!of_platform_device_create(np, NULL, platform_bus_type.dev_root))
> +		return -ENODEV;
> +
> +	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@arm.com>");
>  MODULE_LICENSE("GPL v2");
> -- 
> 2.8.1.dirty
> 

[PATCH v4 6/8] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Robin Murphy]

On 15/07/16 14:55, Lorenzo Pieralisi wrote:
> On Fri, Jul 01, 2016 at 05:50:15PM +0100, Robin Murphy wrote:
> 
> [...]
> 
>> +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;
> 
> Given that a) the check above is removed in a later patch and b)
> code below does not depend on SMMU v3, I think the aim should
> be to make this a core function (ie I am asking this since I will
> need it in IORT based translation and I do not want to add yet another
> *_xlate hook to iommu_op), iommu_fwspec_xlate() ?

Indeed, this is only tied to OF by the current datatypes, and that's
straightforward to change. Ultimately the purpose is just for
firmware/bus code to pass in some words of configuration data, and the
driver to respond with what corresponding runtime data it wants to
associate with the device. As I suggested over on the fsl-mc discussion,
the caller might not even really be 'firmware' at all.

> What I will do with my next RFC is move the iommu_fwspec out of
> OF_IOMMU code in a separate compilation unit and we will take the
> discussion from there.

Sounds good. If the end result starts looking clear, it might be an idea
to squash some patches and skip this intermediate OF-specific step
entirely (I was just hesitant to do that myself without a clear view of
the IORT side).

>> +
>> +	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,22 @@ 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)
>> +{
>> +	static bool registered;
>> +
>> +	if (!registered)
>> +		registered = !arm_smmu_init();
> 
> We also need a static variable in arm_smmu_init() to make sure
> we do not try to execute it multiple times :( (here and
> subsys_initcall).

Strictly, yes, although since there didn't seem to be any real issue
with just letting the initcall fail when register_driver() detects the
collision, I'd hoped we might be able to keep this bodge together in one
place. I guess it might end up printing some unwanted failure message
though, so I'll take another look.

Thanks,
Robin.

> Thanks,
> Lorenzo
> 
>> +
>> +	if (!of_platform_device_create(np, NULL, platform_bus_type.dev_root))
>> +		return -ENODEV;
>> +
>> +	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@arm.com>");
>>  MODULE_LICENSE("GPL v2");
>> -- 
>> 2.8.1.dirty
>>
> 

[PATCH v4 6/8] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Jean-Philippe Brucker]

Hi Robin,

Very sorry about the delay, I forgot about this minor comment, below

On Fri, Jul 01, 2016 at 05:50:15PM +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@arm.com>
> ---
> 
> v4: Add explicit device probe in of_init callback.
> 
>  drivers/iommu/arm-smmu-v3.c | 278 +++++++++++++++++++-------------------------
>  1 file changed, 121 insertions(+), 157 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 94b68213c50d..fb438664b9f0 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
[...]
> @@ -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;

It's probably best to initialise master->ste.bypass = true here, to
reflect the initial state of STEs. Otherwise attach_dev always calls
detach_dev first for nothing.

Apart from that, this version works fine with my twisted setup. Note
that we might have to add a master->fwspec reference in the future, to
access those SIDs from various places. If I understood correctly, it
should be fine as those objects have the same lifetime after the
add_device call.

Thanks,
Jean-Philippe

[PATCH v4 6/8] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Robin Murphy]

On 29/07/16 15:46, Jean-Philippe Brucker wrote:
> Hi Robin,
> 
> Very sorry about the delay, I forgot about this minor comment, below
> 
> On Fri, Jul 01, 2016 at 05:50:15PM +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@arm.com>
>> ---
>>
>> v4: Add explicit device probe in of_init callback.
>>
>>  drivers/iommu/arm-smmu-v3.c | 278 +++++++++++++++++++-------------------------
>>  1 file changed, 121 insertions(+), 157 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 94b68213c50d..fb438664b9f0 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
> [...]
>> @@ -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;
> 
> It's probably best to initialise master->ste.bypass = true here, to
> reflect the initial state of STEs. Otherwise attach_dev always calls
> detach_dev first for nothing.

I'm actually now thinking that that check in attach_dev() should be for
ste->valid, rather than ste->bypass. That matches the similar check in
remove_device(), and looking at old local branches I apparently did have
it that way at some point, and I now can't quite remember why it ended
up differently. I'll have a proper dig into it next week.

> Apart from that, this version works fine with my twisted setup. Note
> that we might have to add a master->fwspec reference in the future, to
> access those SIDs from various places. If I understood correctly, it
> should be fine as those objects have the same lifetime after the
> add_device call.

That sounds reasonable, although I can't think offhand where we might
have a master_data without having got it via the containing device
(unless we also add some other means of keeping track of them). Since
this series doesn't need any kind of reverse-lookup capabilities I'm
just keeping things as simple as possible for the time being.

Robin.

> 
> Thanks,
> Jean-Philippe
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v4 6/8] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Robin Murphy]

On 29/07/16 19:55, Robin Murphy wrote:
> On 29/07/16 15:46, Jean-Philippe Brucker wrote:
>> Hi Robin,
>>
>> Very sorry about the delay, I forgot about this minor comment, below
>>
>> On Fri, Jul 01, 2016 at 05:50:15PM +0100, Robin Murphy wrote:
[...]
>>> +		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;
>>
>> It's probably best to initialise master->ste.bypass = true here, to
>> reflect the initial state of STEs. Otherwise attach_dev always calls
>> detach_dev first for nothing.
> 
> I'm actually now thinking that that check in attach_dev() should be for
> ste->valid, rather than ste->bypass. That matches the similar check in
> remove_device(), and looking at old local branches I apparently did have
> it that way at some point, and I now can't quite remember why it ended
> up differently. I'll have a proper dig into it next week.

[for a value of "next week" relative to "last week", at least...]

Having now remembered, the reason it is this way is a subtle concession
to the nasty PCI RID alias case. When you come to probe the second
device behind a non-transparent bridge, the first device is already
attached to the default domain so the underlying STE is no longer a
bypass entry, but we've got no way of knowing that - we can't tell we're
going to be in an alias group until we call iommu_group_get_for_dev(),
and by the time that returns it's already too late, since the attach to
the default domain (and thus the attempt to write a now-valid STE with
the same data again) will have happened.

The least complicated ways around that that I can think of are 1)
inspect the stream table itself on attach, 2) maintain a semi-redundant
list within the group of exactly which ID is attached to which domain at
any point in time, 3) treat the initial per-device state as undefined
(!valid, !bypass) so that the initial domain attach is always a safe
break-before-make on the stream table, or 4) disallow aliasing IDs
entirely and just let the BUG() in write_strtab_ent() fire if a
non-transparent bridge ever comes along.

Discounting #4 as unreasonable, #3 is by far the least complicated, so
I've kept this as it is for now. #2 might seem reasonable at a glance,
but it wouldn't be useful for anything _except_ this specific situation,
which in practice we'd expect to encounter rarely if ever.

Robin.

>> Apart from that, this version works fine with my twisted setup. Note
>> that we might have to add a master->fwspec reference in the future, to
>> access those SIDs from various places. If I understood correctly, it
>> should be fine as those objects have the same lifetime after the
>> add_device call.
> 
> That sounds reasonable, although I can't think offhand where we might
> have a master_data without having got it via the containing device
> (unless we also add some other means of keeping track of them). Since
> this series doesn't need any kind of reverse-lookup capabilities I'm
> just keeping things as simple as possible for the time being.
> 
> Robin.
> 
>>
>> Thanks,
>> Jean-Philippe
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v4 7/8] 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@arm.com>
---

v4: Shorten rambling comment about why we can't share IDs

 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/arm-smmu-v3.c | 40 ++++++++++++++++++++++++++++++++--------
 2 files changed, 33 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 fb438664b9f0..72056afdb0d7 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,23 @@ 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;
+
+	/*
+	 * We don't support devices sharing stream IDs other than PCI RID
+	 * aliases, since the necessary ID-to-device lookup becomes rather
+	 * impractical given a potential sparse 32-bit stream ID space.
+	 */
+	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 +1887,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 +1905,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 +2648,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

[PATCH v4 8/8] 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@arm.com>
---

v4: No change.

 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 72056afdb0d7..98e64414ea6c 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