[PATCH v2 0/7] Generic DT bindings for PCI IOMMUs and ARM SMMUv3

[PATCH v2 0/7] Generic DT bindings for PCI IOMMUs and ARM SMMUv3

[Robin Murphy]

Hi all,

Compared to v1[1] this is more or less a repost of the core parts, although
patch 1 is new. I still need to take the horrible SMMUv2 code out back and
shoot it (there turned out to be some subtle nasties in v1), so in the
meantime I picked up the SMMUv3 ticket to fill in as it was rather more
straightforward. I'll be reworking SMMUv2 on top of these patches (modulo
any feedback) to post in another week or so.

Note that patch 6 might not be plausible to queue just yet as it ends up
looking like a regression due to deficiencies elsewhere - the virtio block
device on the SMMUv3 Fast Model blows up because virtio doesn't use the DMA
API appropriately on a host (although hacking vring_use_dma_api() suffices),
and MSIs are still an open problem - I could really do with focusing on that
with Eric, so it'd be nice to get as much of this out of the way as I can :)

Branch at git://linux-arm.org/linux-rm iommu/generic-v2

Robin.

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

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

Robin Murphy (6):
  iommu/of: Respect disabled IOMMUs
  of/irq: Break out msi-map lookup (again)
  iommu/of: Handle iommu-map property for PCI
  iommu/arm-smmu: Implement of_xlate() for SMMUv3
  iommu/arm-smmu: Finish off SMMUv3 default domain support
  iommu/arm-smmu: Support non-PCI devices with SMMUv3

 .../devicetree/bindings/pci/pci-iommu.txt          | 171 ++++++++++++
 drivers/iommu/Kconfig                              |   2 +-
 drivers/iommu/arm-smmu-v3.c                        | 300 +++++++++------------
 drivers/iommu/of_iommu.c                           |  73 +++--
 drivers/of/irq.c                                   |  70 +----
 drivers/of/of_pci.c                                | 102 +++++++
 include/linux/of_pci.h                             |   8 +
 7 files changed, 460 insertions(+), 266 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt

-- 
2.8.1.dirty

[PATCH v2 1/7] iommu/of: Respect disabled IOMMUs

[Robin Murphy]

If an IOMMU node is present in the DT but marked as disabled, we should
avoid trying to do anything with it. We currently sort-of get away with
this by virtue of a disabled device probably not having called
of_iommu_set_ops(), but that is hardly safe to rely upon in general, and
either way we don't want to treat it as an error condition with the
resulting "Failed to initialise IOMMU" message.

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

v2: New.

 drivers/iommu/of_iommu.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index af499aea0a1a..662f9a600f4f 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -140,7 +140,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 	struct of_phandle_args iommu_spec;
 	struct device_node *np;
 	const struct iommu_ops *ops = NULL;
-	int idx = 0;
+	int idx;
 
 	/*
 	 * We can't do much for PCI devices without knowing how
@@ -154,24 +154,25 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 	 * See the `Notes:' section of
 	 * Documentation/devicetree/bindings/iommu/iommu.txt
 	 */
-	while (!of_parse_phandle_with_args(master_np, "iommus",
-					   "#iommu-cells", idx,
-					   &iommu_spec)) {
+	for (idx = 0;
+	     !of_parse_phandle_with_args(master_np, "iommus", "#iommu-cells",
+					 idx, &iommu_spec);
+	     of_node_put(np), idx++) {
 		np = iommu_spec.np;
+		if (!of_device_is_available(np))
+			continue;
+
 		ops = of_iommu_get_ops(np);
+		if (!ops || !ops->of_xlate)
+			continue;
 
-		if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
-			goto err_put_node;
-
-		of_node_put(np);
-		idx++;
+		if (ops->of_xlate(dev, &iommu_spec)) {
+			of_node_put(np);
+			return NULL;
+		}
 	}
 
 	return ops;
-
-err_put_node:
-	of_node_put(np);
-	return NULL;
 }
 
 void __init of_iommu_init(void)
@@ -182,7 +183,7 @@ void __init of_iommu_init(void)
 	for_each_matching_node_and_match(np, matches, &match) {
 		const of_iommu_init_fn init_fn = match->data;
 
-		if (init_fn(np))
+		if (of_device_is_available(np) && init_fn(np))
 			pr_err("Failed to initialise IOMMU %s\n",
 				of_node_full_name(np));
 	}
-- 
2.8.1.dirty

Re: [PATCH v2 1/7] iommu/of: Respect disabled IOMMUs

[Will Deacon]

On Fri, Jun 03, 2016 at 06:15:36PM +0100, Robin Murphy wrote:
> If an IOMMU node is present in the DT but marked as disabled, we should
> avoid trying to do anything with it. We currently sort-of get away with
> this by virtue of a disabled device probably not having called
> of_iommu_set_ops(), but that is hardly safe to rely upon in general, and
> either way we don't want to treat it as an error condition with the
> resulting "Failed to initialise IOMMU" message.

What's the use-case for this? I ask because epapr says that the device
binding should provide details as to exactly what "disabled" means for
that device.

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 v2 1/7] iommu/of: Respect disabled IOMMUs

[Robin Murphy]

On 14/06/16 15:11, Will Deacon wrote:
> On Fri, Jun 03, 2016 at 06:15:36PM +0100, Robin Murphy wrote:
>> If an IOMMU node is present in the DT but marked as disabled, we should
>> avoid trying to do anything with it. We currently sort-of get away with
>> this by virtue of a disabled device probably not having called
>> of_iommu_set_ops(), but that is hardly safe to rely upon in general, and
>> either way we don't want to treat it as an error condition with the
>> resulting "Failed to initialise IOMMU" message.
>
> What's the use-case for this? I ask because epapr says that the device
> binding should provide details as to exactly what "disabled" means for
> that device.

Right now, it's mostly a case of making things behave as expected - the 
OF platform code is already assigning the implicit meaning of "don't 
probe or do anything with this device", so playing along with that 
prevents slightly confusing errors or potential null-dereference-style 
crashes as above.

Longer term, I was also thinking back to the probe ordering problem, and 
the sticky case of differentiating "this IOMMU isn't probed yet" from 
"this IOMMU will never probe, continue without it". Being able to make 
that decision from the bootloader (by disabling nodes) in at least a 
subset of cases seemed like a useful tool. I'm still open to other 
points of view, though.

Robin.

>
> Will
>

[PATCH v2 2/7] 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>
Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
---

v2: +Rob'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

Re: [PATCH v2 2/7] Docs: dt: add PCI IOMMU map bindings

[Will Deacon]

On Fri, Jun 03, 2016 at 06:15:37PM +0100, Robin Murphy wrote:
> 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>
> Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> v2: +Rob's ack.
> 
>  .../devicetree/bindings/pci/pci-iommu.txt          | 171 +++++++++++++++++++++
>  1 file changed, 171 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt

This looks great, thanks!

Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Will

[PATCH v2 3/7] 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>
CC: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v2: No change.

 drivers/of/irq.c       |  70 ++-------------------------------
 drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pci.h |   8 ++++
 3 files changed, 114 insertions(+), 66 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e7bfc175b8e1..0c9118d849ee 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>
 
@@ -586,13 +587,7 @@ 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
@@ -602,71 +597,14 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
 		if (!parent_dev->of_node)
 			continue;
 
-		msi_map = of_get_property(parent_dev->of_node,
-					  "msi-map", &msi_map_len);
-		if (!msi_map)
+		if (!of_property_read_bool(parent_dev->of_node, "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. */
+		of_pci_map_rid(parent_dev->of_node, "msi-map", rid_in, np,
+			       &rid_out);
 		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;
-			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..20bf5a0c57fd 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 */
+
+#define MASK_NAME_LEN	32	/* Safely longer than "iommu-map-mask" */
+
+/**
+ * of_pci_map_rid - Translate a requester ID through a downstream mapping.
+ * @np: root complex device node.
+ * @map_name: property name of the map 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. @target or @id_out may
+ * be NULL if not required. If @target points to a device node pointer, only
+ * entries targeting that node will be matched; if it points to a NULL
+ * value, it will receive the device node for the first matching target entry,
+ * with a reference held.
+ *
+ * Return: 0 on success or a standard error code on failure.
+ */
+int of_pci_map_rid(struct device_node *np, const char *map_name, u32 rid_in,
+		   struct device_node **target, u32 *rid_out)
+{
+	u32 map_mask, masked_rid;
+	int map_len;
+	const __be32 *map = NULL;
+	char mask_name[MASK_NAME_LEN];
+
+	if (!np || !map_name || (!target && !rid_out))
+		return -EINVAL;
+
+	map = of_get_property(np, map_name, &map_len);
+	if (!map) {
+		if (target)
+			return -ENODEV;
+		/* Otherwise, no map implies no translation */
+		*rid_out = rid_in;
+		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.
+	 */
+	snprintf(mask_name, MASK_NAME_LEN, "%s-mask", map_name);
+	of_property_read_u32(np, mask_name, &map_mask);
+
+	masked_rid = map_mask & rid_in;
+	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 -EINVAL;
+		}
+
+		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 (rid_out)
+			*rid_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_in, *rid_out);
+		return 0;
+	}
+
+	pr_err("%s: Invalid %s translation - no match for rid 0x%x on %s\n",
+		np->full_name, map_name, rid_in,
+		target && *target ? (*target)->full_name : "any target");
+	return -EINVAL;
+}
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index f6e9e85164e8..070f8835780b 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -17,6 +17,8 @@ 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, const char *map_name, u32 rid_in,
+		   struct device_node **target, u32 *rid_out);
 #else
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {
@@ -52,6 +54,12 @@ of_get_pci_domain_nr(struct device_node *node)
 	return -1;
 }
 
+static inline int of_pci_map_rid(struct device_node *np, const char *map_name,
+			u32 rid_in, struct device_node **target, u32 *rid_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

Re: [PATCH v2 3/7] of/irq: Break out msi-map lookup (again)

[Marc Zyngier]

On Fri, 3 Jun 2016 18:15:38 +0100
Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> CC: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

Acked-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

	M.
-- 
Jazz is not dead. It just smells funny.
--
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 v2 3/7] of/irq: Break out msi-map lookup (again)

[Will Deacon]

On Fri, Jun 03, 2016 at 06:15:38PM +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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> CC: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> v2: No change.
> 
>  drivers/of/irq.c       |  70 ++-------------------------------
>  drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_pci.h |   8 ++++
>  3 files changed, 114 insertions(+), 66 deletions(-)

[...]

> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 13f4fed38048..20bf5a0c57fd 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 */
> +
> +#define MASK_NAME_LEN	32	/* Safely longer than "iommu-map-mask" */
> +
> +/**
> + * of_pci_map_rid - Translate a requester ID through a downstream mapping.
> + * @np: root complex device node.
> + * @map_name: property name of the map 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. @target or @id_out may
> + * be NULL if not required. If @target points to a device node pointer, only
> + * entries targeting that node will be matched; if it points to a NULL
> + * value, it will receive the device node for the first matching target entry,
> + * with a reference held.
> + *
> + * Return: 0 on success or a standard error code on failure.
> + */
> +int of_pci_map_rid(struct device_node *np, const char *map_name, u32 rid_in,
> +		   struct device_node **target, u32 *rid_out)
> +{
> +	u32 map_mask, masked_rid;
> +	int map_len;
> +	const __be32 *map = NULL;
> +	char mask_name[MASK_NAME_LEN];

Couldn't you avoid this if you just took const char *mask_name as a
parameter too?

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 v2 3/7] of/irq: Break out msi-map lookup (again)

[Robin Murphy]

On 14/06/16 15:37, Will Deacon wrote:
> On Fri, Jun 03, 2016 at 06:15:38PM +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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> CC: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> v2: No change.
>>
>>   drivers/of/irq.c       |  70 ++-------------------------------
>>   drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of_pci.h |   8 ++++
>>   3 files changed, 114 insertions(+), 66 deletions(-)
>
> [...]
>
>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>> index 13f4fed38048..20bf5a0c57fd 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 */
>> +
>> +#define MASK_NAME_LEN	32	/* Safely longer than "iommu-map-mask" */
>> +
>> +/**
>> + * of_pci_map_rid - Translate a requester ID through a downstream mapping.
>> + * @np: root complex device node.
>> + * @map_name: property name of the map 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. @target or @id_out may
>> + * be NULL if not required. If @target points to a device node pointer, only
>> + * entries targeting that node will be matched; if it points to a NULL
>> + * value, it will receive the device node for the first matching target entry,
>> + * with a reference held.
>> + *
>> + * Return: 0 on success or a standard error code on failure.
>> + */
>> +int of_pci_map_rid(struct device_node *np, const char *map_name, u32 rid_in,
>> +		   struct device_node **target, u32 *rid_out)
>> +{
>> +	u32 map_mask, masked_rid;
>> +	int map_len;
>> +	const __be32 *map = NULL;
>> +	char mask_name[MASK_NAME_LEN];
>
> Couldn't you avoid this if you just took const char *mask_name as a
> parameter too?

Having started out with that, I considered two strings essentially 
duplicating each other, possibly duplicated per callsite, and thought it 
might be prudent to trade off a little runtime work for a permanent 
reduction in static data. I like the "never save anything you can 
recalculate" philosophy, but it is possible I got a bit carried away 
here. If you hate it I can just add the extra argument back.

Robin.

>
> Will
>

Re: [PATCH v2 3/7] of/irq: Break out msi-map lookup (again)

[Will Deacon]

On Tue, Jun 14, 2016 at 07:12:46PM +0100, Robin Murphy wrote:
> On 14/06/16 15:37, Will Deacon wrote:
> >On Fri, Jun 03, 2016 at 06:15:38PM +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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>CC: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> >>Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> >>---
> >>
> >>v2: No change.
> >>
> >>  drivers/of/irq.c       |  70 ++-------------------------------
> >>  drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/of_pci.h |   8 ++++
> >>  3 files changed, 114 insertions(+), 66 deletions(-)
> >
> >[...]
> >
> >>diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> >>index 13f4fed38048..20bf5a0c57fd 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 */
> >>+
> >>+#define MASK_NAME_LEN	32	/* Safely longer than "iommu-map-mask" */
> >>+
> >>+/**
> >>+ * of_pci_map_rid - Translate a requester ID through a downstream mapping.
> >>+ * @np: root complex device node.
> >>+ * @map_name: property name of the map 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. @target or @id_out may
> >>+ * be NULL if not required. If @target points to a device node pointer, only
> >>+ * entries targeting that node will be matched; if it points to a NULL
> >>+ * value, it will receive the device node for the first matching target entry,
> >>+ * with a reference held.
> >>+ *
> >>+ * Return: 0 on success or a standard error code on failure.
> >>+ */
> >>+int of_pci_map_rid(struct device_node *np, const char *map_name, u32 rid_in,
> >>+		   struct device_node **target, u32 *rid_out)
> >>+{
> >>+	u32 map_mask, masked_rid;
> >>+	int map_len;
> >>+	const __be32 *map = NULL;
> >>+	char mask_name[MASK_NAME_LEN];
> >
> >Couldn't you avoid this if you just took const char *mask_name as a
> >parameter too?
> 
> Having started out with that, I considered two strings essentially
> duplicating each other, possibly duplicated per callsite, and thought it
> might be prudent to trade off a little runtime work for a permanent
> reduction in static data. I like the "never save anything you can
> recalculate" philosophy, but it is possible I got a bit carried away here.
> If you hate it I can just add the extra argument back.

Yeah, please do. The idea is that this can be used with different bindings,
and I don't think we gain anything from constructing the mask string other
than and ugliness complexity.

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 v2 3/7] of/irq: Break out msi-map lookup (again)

[Rob Herring]

On Fri, Jun 3, 2016 at 12:15 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> CC: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>
> v2: No change.
>
>  drivers/of/irq.c       |  70 ++-------------------------------
>  drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_pci.h |   8 ++++
>  3 files changed, 114 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index e7bfc175b8e1..0c9118d849ee 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>
>
> @@ -586,13 +587,7 @@ 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
> @@ -602,71 +597,14 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
>                 if (!parent_dev->of_node)
>                         continue;
>
> -               msi_map = of_get_property(parent_dev->of_node,
> -                                         "msi-map", &msi_map_len);
> -               if (!msi_map)
> +               if (!of_property_read_bool(parent_dev->of_node, "msi-map"))

But msi-map is not bool, right? I think we allow bools to have values
for historical reasons, but really bool with a value should be an
error. So don't rely on current behavior.

>                         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. */
> +               of_pci_map_rid(parent_dev->of_node, "msi-map", rid_in, np,
> +                              &rid_out);

Seems like this could return an error code and then you could continue
based on the return code. Then you wouldn't be looking up msi-map
twice. Probably could get rid of the !parent_dev->of_node check too.

Rob

Re: [PATCH v2 3/7] of/irq: Break out msi-map lookup (again)

[Robin Murphy]

On 14/06/16 18:01, Rob Herring wrote:
> On Fri, Jun 3, 2016 at 12:15 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> 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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> CC: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> v2: No change.
>>
>>   drivers/of/irq.c       |  70 ++-------------------------------
>>   drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of_pci.h |   8 ++++
>>   3 files changed, 114 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index e7bfc175b8e1..0c9118d849ee 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>
>>
>> @@ -586,13 +587,7 @@ 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
>> @@ -602,71 +597,14 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
>>                  if (!parent_dev->of_node)
>>                          continue;
>>
>> -               msi_map = of_get_property(parent_dev->of_node,
>> -                                         "msi-map", &msi_map_len);
>> -               if (!msi_map)
>> +               if (!of_property_read_bool(parent_dev->of_node, "msi-map"))
>
> But msi-map is not bool, right? I think we allow bools to have values
> for historical reasons, but really bool with a value should be an
> error. So don't rely on current behavior.

Right, I now have no idea why that wasn't of_find_property() in the 
first place...

>>                          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. */
>> +               of_pci_map_rid(parent_dev->of_node, "msi-map", rid_in, np,
>> +                              &rid_out);
>
> Seems like this could return an error code and then you could continue
> based on the return code. Then you wouldn't be looking up msi-map
> twice. Probably could get rid of the !parent_dev->of_node check too.

...but I agree that approach does sound better overall, so I'll take a 
closer look at this whole patch again.

Thanks,
Robin.

>
> Rob
>

--
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 v2 4/7] 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.

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

v2: Skip disabled IOMMUs.

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

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 662f9a600f4f..5716131199b3 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,49 @@ 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;
 
-	/*
-	 * 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-map", iommu_spec.args[0],
+				   &np, iommu_spec.args) ||
+		    !of_device_is_available(np))
+			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 v2 4/7] iommu/of: Handle iommu-map property for PCI

[Will Deacon]

On Fri, Jun 03, 2016 at 06:15:39PM +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.
> 
> CC: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> v2: Skip disabled IOMMUs.
> 
>  drivers/iommu/of_iommu.c | 44 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 662f9a600f4f..5716131199b3 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,49 @@ 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;
>  
> -	/*
> -	 * 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-map", iommu_spec.args[0],
> +				   &np, iommu_spec.args) ||
> +		    !of_device_is_available(np))
> +			return NULL;

Hmm, do we need to of_node_put(np) in the case that it's not available?

Will

Re: [PATCH v2 4/7] iommu/of: Handle iommu-map property for PCI

[Robin Murphy]

On 14/06/16 15:45, Will Deacon wrote:
> On Fri, Jun 03, 2016 at 06:15:39PM +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.
>>
>> CC: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> CC: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> v2: Skip disabled IOMMUs.
>>
>>   drivers/iommu/of_iommu.c | 44 +++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 37 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index 662f9a600f4f..5716131199b3 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,49 @@ 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;
>>
>> -	/*
>> -	 * 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-map", iommu_spec.args[0],
>> +				   &np, iommu_spec.args) ||
>> +		    !of_device_is_available(np))
>> +			return NULL;
>
> Hmm, do we need to of_node_put(np) in the case that it's not available?

Ah yes, now I see; I moved the put into the loop iteration for the 
non-PCI case so that a "continue" does the job, but I've not handled the 
equivalent here.

Robin.

>
> Will
>

[PATCH v2 5/7] 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. Initially, this just gets rid of the misuse of the "iommus"
binding without changing the driver's existing level of functionality,
but does at least pave the way to extending it more easily in future.

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

v2: New.

 drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++-----------------
 1 file changed, 73 insertions(+), 46 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 94b68213c50d..7631639cc209 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>
@@ -638,6 +639,12 @@ struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };
 
+/* SMMU private data for each master */
+struct arm_smmu_master_data {
+	struct arm_smmu_device		*smmu;
+	u32				sid;
+};
+
 struct arm_smmu_option_prop {
 	u32 opt;
 	const char *prop;
@@ -1754,40 +1761,19 @@ 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)
-{
-	*(u32 *)sidp = alias;
-	return 0; /* Continue walking */
-}
-
 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)
+static struct arm_smmu_device *arm_smmu_get_by_node(struct device_node *np)
 {
-	struct device_node *of_node;
-	struct platform_device *smmu_pdev;
-	struct arm_smmu_device *smmu = NULL;
-	struct pci_bus *bus = pdev->bus;
+	struct platform_device *smmu_pdev = of_find_device_by_node(np);
 
-	/* 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)
@@ -1804,23 +1790,41 @@ static int arm_smmu_add_device(struct device *dev)
 {
 	int i, ret;
 	u32 sid, *sids;
-	struct pci_dev *pdev;
 	struct iommu_group *group;
+	struct device_node *np;
 	struct arm_smmu_group *smmu_group;
-	struct arm_smmu_device *smmu;
+	struct arm_smmu_device *smmu = NULL;
+	struct arm_smmu_master_data *data = dev->archdata.iommu;
 
-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
+	if (!data)
 		return -ENODEV;
 
-	pdev = to_pci_dev(dev);
+	np = (struct device_node *)data->smmu;
+	smmu = data->smmu = arm_smmu_get_by_node(np);
+	of_node_put(np);
+	if (!smmu)
+		return -ENODEV;
+
+	sid = data->sid;
+
+	/* Check the SID is in range of the SMMU and our stream table */
+	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;
+	}
+
 	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);
+		smmu = arm_smmu_get_by_node(np);
 		if (!smmu) {
 			ret = -ENOENT;
 			goto out_remove_dev;
@@ -1840,27 +1844,12 @@ static int arm_smmu_add_device(struct device *dev)
 		smmu = smmu_group->smmu;
 	}
 
-	/* 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 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;
-	}
-
-	/* 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),
@@ -1887,6 +1876,7 @@ out_remove_dev:
 
 static void arm_smmu_remove_device(struct device *dev)
 {
+	kfree(dev->archdata.iommu);
 	iommu_group_remove_device(dev);
 }
 
@@ -1934,6 +1924,34 @@ out_unlock:
 	return ret;
 }
 
+static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	struct arm_smmu_master_data *data;
+
+	/* We only support PCI, for now */
+	if (!dev_is_pci(dev))
+		return -ENODEV;
+
+	if (dev->archdata.iommu)
+		return -EEXIST;
+
+	data = kmalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/*
+	 * The device_node stands in for the SMMU device at this point
+	 * since there's no guarantee the SMMU itself has probed yet.
+	 * By the time we see this again in an add_device callback, we'll
+	 * be in a position to fix it up with the real thing.
+	 */
+	data->smmu = (struct arm_smmu_device *)args->np;
+	data->sid = args->args[0];
+	dev->archdata.iommu = data;
+
+	return 0;
+}
+
 static struct iommu_ops arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
@@ -1947,6 +1965,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 +2716,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

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

[Will Deacon]

On Fri, Jun 03, 2016 at 06:15:40PM +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. Initially, this just gets rid of the misuse of the "iommus"
> binding without changing the driver's existing level of functionality,
> but does at least pave the way to extending it more easily in future.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> v2: New.
> 
>  drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++-----------------
>  1 file changed, 73 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 94b68213c50d..7631639cc209 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>
> @@ -638,6 +639,12 @@ struct arm_smmu_domain {
>  	struct iommu_domain		domain;
>  };
>  
> +/* SMMU private data for each master */
> +struct arm_smmu_master_data {
> +	struct arm_smmu_device		*smmu;
> +	u32				sid;
> +};

Hmm, we already have this information in arm_smmu_group so I'd rather
avoid the duplication if we can.

> @@ -1804,23 +1790,41 @@ static int arm_smmu_add_device(struct device *dev)
>  {
>  	int i, ret;
>  	u32 sid, *sids;
> -	struct pci_dev *pdev;
>  	struct iommu_group *group;
> +	struct device_node *np;
>  	struct arm_smmu_group *smmu_group;
> -	struct arm_smmu_device *smmu;
> +	struct arm_smmu_device *smmu = NULL;
> +	struct arm_smmu_master_data *data = dev->archdata.iommu;
>  
> -	/* We only support PCI, for now */
> -	if (!dev_is_pci(dev))
> +	if (!data)
>  		return -ENODEV;
>  
> -	pdev = to_pci_dev(dev);
> +	np = (struct device_node *)data->smmu;
> +	smmu = data->smmu = arm_smmu_get_by_node(np);
> +	of_node_put(np);
> +	if (!smmu)
> +		return -ENODEV;

Why can't we continue to use the group here?

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 v2 5/7] iommu/arm-smmu: Implement of_xlate() for SMMUv3

[Robin Murphy]

On 14/06/16 16:07, Will Deacon wrote:
> On Fri, Jun 03, 2016 at 06:15:40PM +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. Initially, this just gets rid of the misuse of the "iommus"
>> binding without changing the driver's existing level of functionality,
>> but does at least pave the way to extending it more easily in future.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> v2: New.
>>
>>   drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++-----------------
>>   1 file changed, 73 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 94b68213c50d..7631639cc209 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>
>> @@ -638,6 +639,12 @@ struct arm_smmu_domain {
>>   	struct iommu_domain		domain;
>>   };
>>
>> +/* SMMU private data for each master */
>> +struct arm_smmu_master_data {
>> +	struct arm_smmu_device		*smmu;
>> +	u32				sid;
>> +};
>
> Hmm, we already have this information in arm_smmu_group so I'd rather
> avoid the duplication if we can.

For of_xlate() to work at all, we need to associate the IDs with their 
device long before groups are even available (because the device is only 
partially constructed), so at a bare minimum we need something to carry 
the data through until we see it again in add_device, where we can 
allocate a group.

>> @@ -1804,23 +1790,41 @@ static int arm_smmu_add_device(struct device *dev)
>>   {
>>   	int i, ret;
>>   	u32 sid, *sids;
>> -	struct pci_dev *pdev;
>>   	struct iommu_group *group;
>> +	struct device_node *np;
>>   	struct arm_smmu_group *smmu_group;
>> -	struct arm_smmu_device *smmu;
>> +	struct arm_smmu_device *smmu = NULL;
>> +	struct arm_smmu_master_data *data = dev->archdata.iommu;
>>
>> -	/* We only support PCI, for now */
>> -	if (!dev_is_pci(dev))
>> +	if (!data)
>>   		return -ENODEV;
>>
>> -	pdev = to_pci_dev(dev);
>> +	np = (struct device_node *)data->smmu;
>> +	smmu = data->smmu = arm_smmu_get_by_node(np);
>> +	of_node_put(np);
>> +	if (!smmu)
>> +		return -ENODEV;
>
> Why can't we continue to use the group here?

I don't follow; at this point we've still not even allocated/found a 
group for this device yet. The code a few dozen lines later that takes 
the IDs and stashes them in the groupdata once it _does_ exist is 
unchanged in this patch, all that's changing is how those IDs got here 
in the first place.

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 v2 6/7] iommu/arm-smmu: Finish off SMMUv3 default domain support

[Robin Murphy]

The driver's current reliance on attaching/detaching an entire group
for the first device it sees is at odds with the IOMMU core's initial
construction of a group by adding each device and attaching it to the
default domain in turn. As it turns out, we can happily do away with the
whole palaver by simply letting each device be in charge of its own
stream ID/stream table entry, and reducing the problem of tracking
duplicate IDs and domains down to "Is the STE already associated with
the appropriate context?", which is easily done by just looking at the
stream table itself.

With an of_xlate() callback in place, devices attached to default
domains will now get appropriate DMA ops installed, so make sure we
enable translation again to stop them getting horribly confused - this
reverts the SMMUv3 portion of cbf8277ef456 ("iommu/arm-smmu: Treat
IOMMU_DOMAIN_DMA as bypass for now")

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

v2: New.

 drivers/iommu/arm-smmu-v3.c | 183 +++++++++-----------------------------------
 1 file changed, 38 insertions(+), 145 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 7631639cc209..28dcc5ca237e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -607,15 +607,6 @@ struct arm_smmu_device {
 	struct arm_smmu_strtab_cfg	strtab_cfg;
 };
 
-/* SMMU private data for an IOMMU group */
-struct arm_smmu_group {
-	struct arm_smmu_device		*smmu;
-	struct arm_smmu_domain		*domain;
-	int				num_sids;
-	u32				*sids;
-	struct arm_smmu_strtab_ent	ste;
-};
-
 /* SMMU private data for an IOMMU domain */
 enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
@@ -642,6 +633,8 @@ struct arm_smmu_domain {
 /* SMMU private data for each master */
 struct arm_smmu_master_data {
 	struct arm_smmu_device		*smmu;
+
+	struct arm_smmu_strtab_ent	ste;
 	u32				sid;
 };
 
@@ -1020,9 +1013,15 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 	 * 1. Update Config, return (init time STEs aren't live)
 	 * 2. Write everything apart from dword 0, sync, write dword 0, sync
 	 * 3. Update Config, sync
+	 *
+	 * Note that with the departure of the explicit detach callback from
+	 * the API, we may be doing 3 & 2 back-to-back in the same call.
 	 */
 	u64 val = le64_to_cpu(dst[0]);
 	bool ste_live = false;
+	bool ste_ok = false;
+	dma_addr_t s1ctxptr = ste->s1_cfg ? ste->s1_cfg->cdptr_dma : 0;
+	u16 vmid = ste->s2_cfg ? ste->s2_cfg->vmid : 0;
 	struct arm_smmu_cmdq_ent prefetch_cmd = {
 		.opcode		= CMDQ_OP_PREFETCH_CFG,
 		.prefetch	= {
@@ -1035,17 +1034,27 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 
 		cfg = val & STRTAB_STE_0_CFG_MASK << STRTAB_STE_0_CFG_SHIFT;
 		switch (cfg) {
+		case STRTAB_STE_0_CFG_ABORT:
 		case STRTAB_STE_0_CFG_BYPASS:
+			ste_ok = ste->bypass;
 			break;
 		case STRTAB_STE_0_CFG_S1_TRANS:
 		case STRTAB_STE_0_CFG_S2_TRANS:
 			ste_live = true;
+			ste_ok = ((val & STRTAB_STE_0_S1CTXPTR_MASK <<
+				   STRTAB_STE_0_S1CTXPTR_SHIFT) == s1ctxptr) &&
+				 ((le64_to_cpu(dst[2]) &
+				  STRTAB_STE_2_S2VMID_MASK) == vmid);
 			break;
 		default:
 			BUG(); /* STE corruption */
 		}
 	}
 
+	/* No point rewriting things to the exact same state... */
+	if (ste_ok)
+		return;
+
 	/* Nuke the existing Config, as we're going to rewrite it */
 	val &= ~(STRTAB_STE_0_CFG_MASK << STRTAB_STE_0_CFG_SHIFT);
 
@@ -1054,7 +1063,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 	else
 		val &= ~STRTAB_STE_0_V;
 
-	if (ste->bypass) {
+	if (ste_live || ste->bypass) {
 		val |= disable_bypass ? STRTAB_STE_0_CFG_ABORT
 				      : STRTAB_STE_0_CFG_BYPASS;
 		dst[0] = cpu_to_le64(val);
@@ -1063,11 +1072,11 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 		dst[2] = 0; /* Nuke the VMID */
 		if (ste_live)
 			arm_smmu_sync_ste_for_sid(smmu, sid);
-		return;
+		if (ste->bypass)
+			return;
 	}
 
 	if (ste->s1_cfg) {
-		BUG_ON(ste_live);
 		dst[1] = cpu_to_le64(
 			 STRTAB_STE_1_S1C_CACHE_WBRA
 			 << STRTAB_STE_1_S1CIR_SHIFT |
@@ -1082,16 +1091,15 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 		if (smmu->features & ARM_SMMU_FEAT_STALLS)
 			dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
 
-		val |= (ste->s1_cfg->cdptr_dma & STRTAB_STE_0_S1CTXPTR_MASK
+		val |= (s1ctxptr & STRTAB_STE_0_S1CTXPTR_MASK
 		        << STRTAB_STE_0_S1CTXPTR_SHIFT) |
 			STRTAB_STE_0_CFG_S1_TRANS;
 
 	}
 
 	if (ste->s2_cfg) {
-		BUG_ON(ste_live);
 		dst[2] = cpu_to_le64(
-			 ste->s2_cfg->vmid << STRTAB_STE_2_S2VMID_SHIFT |
+			 vmid << STRTAB_STE_2_S2VMID_SHIFT |
 			 (ste->s2_cfg->vtcr & STRTAB_STE_2_VTCR_MASK)
 			  << STRTAB_STE_2_VTCR_SHIFT |
 #ifdef __BIG_ENDIAN
@@ -1582,20 +1590,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;
@@ -1618,12 +1612,12 @@ 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 void arm_smmu_install_ste(struct arm_smmu_master_data *master,
+				struct arm_smmu_domain *smmu_domain)
 {
-	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_device *smmu = master->smmu;
+	struct arm_smmu_strtab_ent *ste = &master->ste;
+	__le64 *step = arm_smmu_get_step_for_sid(smmu, master->sid);
 
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		ste->s1_cfg = &smmu_domain->s1_cfg;
@@ -1634,42 +1628,16 @@ static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
 		ste->s2_cfg = &smmu_domain->s2_cfg;
 	}
 
-	for (i = 0; i < smmu_group->num_sids; ++i) {
-		u32 sid = smmu_group->sids[i];
-		__le64 *step = arm_smmu_get_step_for_sid(smmu, sid);
-
-		arm_smmu_write_strtab_ent(smmu, sid, step, ste);
-	}
-
-	return 0;
-}
-
-static void arm_smmu_detach_dev(struct device *dev)
-{
-	struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
-
-	smmu_group->ste.bypass = true;
-	if (arm_smmu_install_ste_for_group(smmu_group) < 0)
-		dev_warn(dev, "failed to install bypass STE\n");
-
-	smmu_group->domain = NULL;
+	arm_smmu_write_strtab_ent(smmu, master->sid, step, ste);
 }
 
 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 = dev->archdata.iommu;
+	struct arm_smmu_device *smmu = master->smmu;
 
-	if (!smmu_group)
-		return -ENOENT;
-
-	/* Already attached to a different domain? */
-	if (smmu_group->domain && smmu_group->domain != smmu_domain)
-		arm_smmu_detach_dev(dev);
-
-	smmu = smmu_group->smmu;
 	mutex_lock(&smmu_domain->init_mutex);
 
 	if (!smmu_domain->smmu) {
@@ -1688,21 +1656,9 @@ 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;
+	master->ste.bypass = false;
 
-	smmu_group->domain	= smmu_domain;
-
-	/*
-	 * 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);
-	if (ret < 0)
-		smmu_group->domain = NULL;
+	arm_smmu_install_ste(master, smmu_domain);
 
 out_unlock:
 	mutex_unlock(&smmu_domain->init_mutex);
@@ -1761,11 +1717,6 @@ arm_smmu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova)
 	return ret;
 }
 
-static void __arm_smmu_release_pci_iommudata(void *data)
-{
-	kfree(data);
-}
-
 static struct arm_smmu_device *arm_smmu_get_by_node(struct device_node *np)
 {
 	struct platform_device *smmu_pdev = of_find_device_by_node(np);
@@ -1788,12 +1739,8 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 
 static int arm_smmu_add_device(struct device *dev)
 {
-	int i, ret;
-	u32 sid, *sids;
-	struct iommu_group *group;
 	struct device_node *np;
-	struct arm_smmu_group *smmu_group;
-	struct arm_smmu_device *smmu = NULL;
+	struct arm_smmu_device *smmu;
 	struct arm_smmu_master_data *data = dev->archdata.iommu;
 
 	if (!data)
@@ -1805,73 +1752,19 @@ static int arm_smmu_add_device(struct device *dev)
 	if (!smmu)
 		return -ENODEV;
 
-	sid = data->sid;
-
 	/* Check the SID is in range of the SMMU and our stream table */
-	if (!arm_smmu_sid_in_range(smmu, sid))
+	if (!arm_smmu_sid_in_range(smmu, data->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);
+		int ret = arm_smmu_init_l2_strtab(smmu, data->sid);
+
 		if (ret)
 			return ret;
 	}
 
-	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_by_node(np);
-		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);
-	} else {
-		smmu = smmu_group->smmu;
-	}
-
-	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;
-	}
-
-	/* 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(iommu_group_get_for_dev(dev));
 }
 
 static void arm_smmu_remove_device(struct device *dev)
-- 
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 v2 6/7] iommu/arm-smmu: Finish off SMMUv3 default domain support

[Jean-Philippe Brucker]

Hi Robin,

I quite like this change, the result looks pretty clean. I rebased my
current work and didn't notice any major issue. Some nits below.

On Fri, Jun 03, 2016 at 06:15:41PM +0100, Robin Murphy wrote:
> The driver's current reliance on attaching/detaching an entire group
> for the first device it sees is at odds with the IOMMU core's initial
> construction of a group by adding each device and attaching it to the
> default domain in turn. As it turns out, we can happily do away with the
> whole palaver by simply letting each device be in charge of its own
> stream ID/stream table entry, and reducing the problem of tracking
> duplicate IDs and domains down to "Is the STE already associated with
> the appropriate context?", which is easily done by just looking at the
> stream table itself.
> 
> With an of_xlate() callback in place, devices attached to default
> domains will now get appropriate DMA ops installed, so make sure we
> enable translation again to stop them getting horribly confused - this
> reverts the SMMUv3 portion of cbf8277ef456 ("iommu/arm-smmu: Treat
> IOMMU_DOMAIN_DMA as bypass for now")
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
...
>  
> -static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
> +static void arm_smmu_install_ste(struct arm_smmu_master_data *master,
> +				struct arm_smmu_domain *smmu_domain)

(Second line is misaligned here)

>  {
> -	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_device *smmu = master->smmu;
> +	struct arm_smmu_strtab_ent *ste = &master->ste;
> +	__le64 *step = arm_smmu_get_step_for_sid(smmu, master->sid);
>  
>  	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>  		ste->s1_cfg = &smmu_domain->s1_cfg;
> @@ -1634,42 +1628,16 @@ static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
>  		ste->s2_cfg = &smmu_domain->s2_cfg;
>  	}
>  
> -	for (i = 0; i < smmu_group->num_sids; ++i) {
> -		u32 sid = smmu_group->sids[i];
> -		__le64 *step = arm_smmu_get_step_for_sid(smmu, sid);
> -
> -		arm_smmu_write_strtab_ent(smmu, sid, step, ste);
> -	}
> -
> -	return 0;
> -}
> -
> -static void arm_smmu_detach_dev(struct device *dev)
> -{
> -	struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
> -
> -	smmu_group->ste.bypass = true;
> -	if (arm_smmu_install_ste_for_group(smmu_group) < 0)
> -		dev_warn(dev, "failed to install bypass STE\n");
> -
> -	smmu_group->domain = NULL;
> +	arm_smmu_write_strtab_ent(smmu, master->sid, step, ste);
>  }
>  
>  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 = dev->archdata.iommu;

(calling this member 'master' or 'smmu_master' consistently, instead of
'data', would make the driver easier to grep)

> +	struct arm_smmu_device *smmu = master->smmu;
>  
> -	if (!smmu_group)
> -		return -ENOENT;
> -
> -	/* Already attached to a different domain? */
> -	if (smmu_group->domain && smmu_group->domain != smmu_domain)
> -		arm_smmu_detach_dev(dev);
> -
> -	smmu = smmu_group->smmu;
>  	mutex_lock(&smmu_domain->init_mutex);
>  
>  	if (!smmu_domain->smmu) {
> @@ -1688,21 +1656,9 @@ 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;
> +	master->ste.bypass = false;

Should also set master->ste.valid = true. It worked out of the box
during my first test, because master is allocated with kmalloc and
initialised with garbage. Could we also use kzalloc, in the of_xlate
patch?

Thanks,
Jean-Philippe
--
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 v2 6/7] iommu/arm-smmu: Finish off SMMUv3 default domain support

[Robin Murphy]

On 06/06/16 16:47, Jean-Philippe Brucker wrote:
> Hi Robin,
>
> I quite like this change, the result looks pretty clean. I rebased my
> current work and didn't notice any major issue. Some nits below.

Thanks for looking!

> On Fri, Jun 03, 2016 at 06:15:41PM +0100, Robin Murphy wrote:
>> The driver's current reliance on attaching/detaching an entire group
>> for the first device it sees is at odds with the IOMMU core's initial
>> construction of a group by adding each device and attaching it to the
>> default domain in turn. As it turns out, we can happily do away with the
>> whole palaver by simply letting each device be in charge of its own
>> stream ID/stream table entry, and reducing the problem of tracking
>> duplicate IDs and domains down to "Is the STE already associated with
>> the appropriate context?", which is easily done by just looking at the
>> stream table itself.
>>
>> With an of_xlate() callback in place, devices attached to default
>> domains will now get appropriate DMA ops installed, so make sure we
>> enable translation again to stop them getting horribly confused - this
>> reverts the SMMUv3 portion of cbf8277ef456 ("iommu/arm-smmu: Treat
>> IOMMU_DOMAIN_DMA as bypass for now")
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
> ...
>>
>> -static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
>> +static void arm_smmu_install_ste(struct arm_smmu_master_data *master,
>> +				struct arm_smmu_domain *smmu_domain)
>
> (Second line is misaligned here)

Fixed.

>>   {
>> -	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_device *smmu = master->smmu;
>> +	struct arm_smmu_strtab_ent *ste = &master->ste;
>> +	__le64 *step = arm_smmu_get_step_for_sid(smmu, master->sid);
>>
>>   	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>>   		ste->s1_cfg = &smmu_domain->s1_cfg;
>> @@ -1634,42 +1628,16 @@ static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
>>   		ste->s2_cfg = &smmu_domain->s2_cfg;
>>   	}
>>
>> -	for (i = 0; i < smmu_group->num_sids; ++i) {
>> -		u32 sid = smmu_group->sids[i];
>> -		__le64 *step = arm_smmu_get_step_for_sid(smmu, sid);
>> -
>> -		arm_smmu_write_strtab_ent(smmu, sid, step, ste);
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -static void arm_smmu_detach_dev(struct device *dev)
>> -{
>> -	struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
>> -
>> -	smmu_group->ste.bypass = true;
>> -	if (arm_smmu_install_ste_for_group(smmu_group) < 0)
>> -		dev_warn(dev, "failed to install bypass STE\n");
>> -
>> -	smmu_group->domain = NULL;
>> +	arm_smmu_write_strtab_ent(smmu, master->sid, step, ste);
>>   }
>>
>>   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 = dev->archdata.iommu;
>
> (calling this member 'master' or 'smmu_master' consistently, instead of
> 'data', would make the driver easier to grep)

Good point; it's now called "master" everywhere.

>> +	struct arm_smmu_device *smmu = master->smmu;
>>
>> -	if (!smmu_group)
>> -		return -ENOENT;
>> -
>> -	/* Already attached to a different domain? */
>> -	if (smmu_group->domain && smmu_group->domain != smmu_domain)
>> -		arm_smmu_detach_dev(dev);
>> -
>> -	smmu = smmu_group->smmu;
>>   	mutex_lock(&smmu_domain->init_mutex);
>>
>>   	if (!smmu_domain->smmu) {
>> @@ -1688,21 +1656,9 @@ 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;
>> +	master->ste.bypass = false;
>
> Should also set master->ste.valid = true. It worked out of the box
> during my first test, because master is allocated with kmalloc and
> initialised with garbage. Could we also use kzalloc, in the of_xlate
> patch?

Yikes! I think this is the of_xlate directly cherry-picked from a more 
aggressive attempt to remove arm_smmu_strtab_ent along with 
arm_smmu_group, and those bits of initialisation never got added back; 
both fixed.

Thanks for catching all those - I've recreated the unversioned 
iommu/generic branch with a fixup patch on top for the time being.

Robin.

>
> Thanks,
> Jean-Philippe
>

--
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 v2 6/7] iommu/arm-smmu: Finish off SMMUv3 default domain support

[Will Deacon]

On Fri, Jun 03, 2016 at 06:15:41PM +0100, Robin Murphy wrote:
> The driver's current reliance on attaching/detaching an entire group
> for the first device it sees is at odds with the IOMMU core's initial
> construction of a group by adding each device and attaching it to the
> default domain in turn. As it turns out, we can happily do away with the
> whole palaver by simply letting each device be in charge of its own
> stream ID/stream table entry, and reducing the problem of tracking
> duplicate IDs and domains down to "Is the STE already associated with
> the appropriate context?", which is easily done by just looking at the
> stream table itself.

Hmm, it would be nice if Joerg could confirm this before we remove the
driver code.

Joerg: can an IOMMU driver rely on not receiving contradictory ->attach
callbacks for devices in the same group? Robin is proposing that we
remove the group tracking code from the ARM SMMU driver which is a nice
cleanup but leaves us open to silent misconfiguration if we are asked to
violate the group constraints.

> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 7631639cc209..28dcc5ca237e 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -607,15 +607,6 @@ struct arm_smmu_device {
>  	struct arm_smmu_strtab_cfg	strtab_cfg;
>  };
>  
> -/* SMMU private data for an IOMMU group */
> -struct arm_smmu_group {
> -	struct arm_smmu_device		*smmu;
> -	struct arm_smmu_domain		*domain;
> -	int				num_sids;
> -	u32				*sids;
> -	struct arm_smmu_strtab_ent	ste;
> -};

I'm not keen on losing this structure. How about we instead change the
sids array to be a linked list of {struct device, sid[]} and then change
arm_smmu_install_ste_for_group to take a device pointer as well?

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 v2 7/7] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Robin Murphy]

With the device <-> stream ID relationship suitably abstracted and
of_xlate() hooked up, we no longer have any PCI-specifics in play,
so adding support for the simpler kinds of platform device (a single
unique stream ID each) becomes trivial; let's do it!

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

v2: New. Consider this one "extra bonus material" as I'm not sure there
    are even any suitable devices on our model to test it with (it
    _should_ be OK, given that I know the basic infrastructure on either
    side works...)

 drivers/iommu/Kconfig       |  2 +-
 drivers/iommu/arm-smmu-v3.c | 36 ++++++++++++++++++++++++++++++------
 2 files changed, 31 insertions(+), 7 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 28dcc5ca237e..6379f0ab24fc 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 */
@@ -1773,6 +1775,22 @@ static void arm_smmu_remove_device(struct device *dev)
 	iommu_group_remove_device(dev);
 }
 
+static struct iommu_group *arm_smmu_device_group(struct device *dev)
+{
+	struct iommu_group *group;
+
+	/*
+	 * We've currently no means of grouping non-PCI masters, so
+	 * there'd better not be any non-unique stream IDs in the DT...
+	 */
+	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)
 {
@@ -1821,10 +1839,6 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 {
 	struct arm_smmu_master_data *data;
 
-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
-		return -ENODEV;
-
 	if (dev->archdata.iommu)
 		return -EEXIST;
 
@@ -1855,7 +1869,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,
@@ -2598,7 +2612,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 v2 7/7] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Will Deacon]

[adding ThunderTown, since he might be able to test this for us]

On Fri, Jun 03, 2016 at 06:15:42PM +0100, Robin Murphy wrote:
> With the device <-> stream ID relationship suitably abstracted and
> of_xlate() hooked up, we no longer have any PCI-specifics in play,
> so adding support for the simpler kinds of platform device (a single
> unique stream ID each) becomes trivial; let's do it!
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> v2: New. Consider this one "extra bonus material" as I'm not sure there
>     are even any suitable devices on our model to test it with (it
>     _should_ be OK, given that I know the basic infrastructure on either
>     side works...)
> 
>  drivers/iommu/Kconfig       |  2 +-
>  drivers/iommu/arm-smmu-v3.c | 36 ++++++++++++++++++++++++++++++------
>  2 files changed, 31 insertions(+), 7 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 28dcc5ca237e..6379f0ab24fc 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 */
> @@ -1773,6 +1775,22 @@ static void arm_smmu_remove_device(struct device *dev)
>  	iommu_group_remove_device(dev);
>  }
>  
> +static struct iommu_group *arm_smmu_device_group(struct device *dev)
> +{
> +	struct iommu_group *group;
> +
> +	/*
> +	 * We've currently no means of grouping non-PCI masters, so
> +	 * there'd better not be any non-unique stream IDs in the DT...
> +	 */

Worse: what if a SID in the DT aliases with a PCI master? It might be
nice to have some basic snity checking, at least.

Will

Re: [PATCH v2 7/7] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Leizhen (ThunderTown)]

On 2016/6/14 23:16, Will Deacon wrote:
> [adding ThunderTown, since he might be able to test this for us]

OK. I'm so glad to do it.

> 
> On Fri, Jun 03, 2016 at 06:15:42PM +0100, Robin Murphy wrote:
>> With the device <-> stream ID relationship suitably abstracted and
>> of_xlate() hooked up, we no longer have any PCI-specifics in play,
>> so adding support for the simpler kinds of platform device (a single
>> unique stream ID each) becomes trivial; let's do it!
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> v2: New. Consider this one "extra bonus material" as I'm not sure there
>>     are even any suitable devices on our model to test it with (it
>>     _should_ be OK, given that I know the basic infrastructure on either
>>     side works...)
>>
>>  drivers/iommu/Kconfig       |  2 +-
>>  drivers/iommu/arm-smmu-v3.c | 36 ++++++++++++++++++++++++++++++------
>>  2 files changed, 31 insertions(+), 7 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 28dcc5ca237e..6379f0ab24fc 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 */
>> @@ -1773,6 +1775,22 @@ static void arm_smmu_remove_device(struct device *dev)
>>  	iommu_group_remove_device(dev);
>>  }
>>  
>> +static struct iommu_group *arm_smmu_device_group(struct device *dev)
>> +{
>> +	struct iommu_group *group;
>> +
>> +	/*
>> +	 * We've currently no means of grouping non-PCI masters, so
>> +	 * there'd better not be any non-unique stream IDs in the DT...
>> +	 */
> 
> Worse: what if a SID in the DT aliases with a PCI master? It might be
> nice to have some basic snity checking, at least.
> 
> Will
> 
> .
> 

Re: [PATCH v2 7/7] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Leizhen (ThunderTown)]

Hi,
I only applied these patch series on lastest 4.7-rc3, is there any patches I missed?
According to my test, it seems can not work. The problem is:

[    0.916536] Call trace:
[    0.918967] [<ffff00000808882c>] dump_backtrace+0x0/0x1ac
[    0.924361] [<ffff0000080889ec>] show_stack+0x14/0x1c
[    0.929384] [<ffff00000833c290>] dump_stack+0x8c/0xac
[    0.934425] [<ffff000008093550>] do_iommu_attach.isra.11+0x24/0x7c				//called iommu_get_domain_for_dev, but currently dev->iommu_group is NULL
[    0.940589] [<ffff000008093604>] __iommu_attach_notifier.part.13+0x5c/0xc8
[    0.947443] [<ffff000008093690>] __iommu_attach_notifier+0x20/0x2c
[    0.953608] [<ffff0000080d4c88>] notifier_call_chain+0x4c/0x88
[    0.959408] [<ffff0000080d5010>] __blocking_notifier_call_chain+0x44/0x74
[    0.966159] [<ffff0000080d5054>] blocking_notifier_call_chain+0x14/0x1c
[    0.972754] [<ffff000008489600>] device_add+0x3ac/0x53c
[    0.977959] [<ffff000008671810>] of_device_add+0x58/0x64
[    0.983240] [<ffff0000086720c8>] of_platform_device_create_pdata+0x90/0xe8
[    0.990077] [<ffff00000867220c>] of_platform_bus_create+0xd0/0x31c
[    0.996265] [<ffff000008672574>] of_platform_populate+0x48/0xac
[    1.002169] [<ffff000008af2654>] arm64_device_init+0x30/0x58
[    1.007813] [<ffff000008081a14>] do_one_initcall+0x38/0x128
[    1.013354] [<ffff000008af0cc4>] kernel_init_freeable+0x14c/0x1ec
[    1.019439] [<ffff0000087a92d0>] kernel_init+0x10/0xfc
[    1.024556] [<ffff000008084e10>] ret_from_fork+0x10/0x40

[    0.906207] Failed to set up IOMMU for device tst_smmu; retaining platform DMA ops		//error info

[    0.963782] HugeTLB registered 2 MB page size, pre-allocated 0 pages
[    0.970442] ACPI: Interpreter disabled.
[    0.974429] arm-smmu-v3 d0040000.smmu_alg: ias 44-bit, oas 44-bit (features 0x00000f0d)

[    0.996948] Call trace:
[    0.999400] [<ffff00000808882c>] dump_backtrace+0x0/0x1ac
[    1.004787] [<ffff0000080889ec>] show_stack+0x14/0x1c
[    1.009846] [<ffff00000833c290>] dump_stack+0x8c/0xac
[    1.014905] [<ffff0000084829d4>] arm_smmu_add_device+0x44/0x160
[    1.020801] [<ffff00000847a218>] add_iommu_group+0x20/0x44
[    1.026291] [<ffff00000848a43c>] bus_for_each_dev+0x58/0x98
[    1.031866] [<ffff00000847b220>] bus_set_iommu+0x9c/0xf8
[    1.037150] [<ffff000008b1b420>] arm_smmu_init+0x78/0x84
[    1.042475] [<ffff000008081a14>] do_one_initcall+0x38/0x128
[    1.048043] [<ffff000008af0cc4>] kernel_init_freeable+0x14c/0x1ec
[    1.054138] [<ffff0000087a931c>] kernel_init+0x10/0xfc
[    1.059264] [<ffff000008084e10>] ret_from_fork+0x10/0x40

[    1.171026] iommu: Adding device tst_smmu to group 0						//It's already late.

On 2016/6/15 9:22, Leizhen (ThunderTown) wrote:
> 
> 
> On 2016/6/14 23:16, Will Deacon wrote:
>> [adding ThunderTown, since he might be able to test this for us]
> 
> OK. I'm so glad to do it.
> 
>>
>> On Fri, Jun 03, 2016 at 06:15:42PM +0100, Robin Murphy wrote:
>>> With the device <-> stream ID relationship suitably abstracted and
>>> of_xlate() hooked up, we no longer have any PCI-specifics in play,
>>> so adding support for the simpler kinds of platform device (a single
>>> unique stream ID each) becomes trivial; let's do it!
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>>> ---
>>>
>>> v2: New. Consider this one "extra bonus material" as I'm not sure there
>>>     are even any suitable devices on our model to test it with (it
>>>     _should_ be OK, given that I know the basic infrastructure on either
>>>     side works...)
>>>
>>>  drivers/iommu/Kconfig       |  2 +-
>>>  drivers/iommu/arm-smmu-v3.c | 36 ++++++++++++++++++++++++++++++------
>>>  2 files changed, 31 insertions(+), 7 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 28dcc5ca237e..6379f0ab24fc 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 */
>>> @@ -1773,6 +1775,22 @@ static void arm_smmu_remove_device(struct device *dev)
>>>  	iommu_group_remove_device(dev);
>>>  }
>>>  
>>> +static struct iommu_group *arm_smmu_device_group(struct device *dev)
>>> +{
>>> +	struct iommu_group *group;
>>> +
>>> +	/*
>>> +	 * We've currently no means of grouping non-PCI masters, so
>>> +	 * there'd better not be any non-unique stream IDs in the DT...
>>> +	 */
>>
>> Worse: what if a SID in the DT aliases with a PCI master? It might be
>> nice to have some basic snity checking, at least.
>>
>> Will
>>
>> .
>>

Re: [PATCH v2 7/7] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Robin Murphy]

On 17/06/16 02:54, Leizhen (ThunderTown) wrote:
> Hi,
> I only applied these patch series on lastest 4.7-rc3, is there any patches I missed?
> According to my test, it seems can not work. The problem is:

Thanks, that's helpful (if irritating) confirmation.

Would you mind trying this patch on top to see if that helps: 
http://article.gmane.org/gmane.linux.kernel.iommu/13991

Failing that, we could go back to the forced-device-creation workaround 
(although I was hoping we'd be able to avoid it):

-----8<-----
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index e5acd601ee96..556a48f836b3 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2683,9 +2683,15 @@ module_exit(arm_smmu_exit);

  static int __init arm_smmu_of_init(struct device_node *np)
  {
+	static bool registered;
+
  	of_iommu_set_ops(np, &arm_smmu_ops);

-	return 0;
+	if (!registered)
+		registered = !arm_smmu_init();
+
+	return PTR_ERR_OR_ZERO(of_platform_device_create(np, NULL,
+					platform_bus_type.dev_root));
  }
  IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", arm_smmu_of_init);

-----8<-----

Robin.

>
> [    0.916536] Call trace:
> [    0.918967] [<ffff00000808882c>] dump_backtrace+0x0/0x1ac
> [    0.924361] [<ffff0000080889ec>] show_stack+0x14/0x1c
> [    0.929384] [<ffff00000833c290>] dump_stack+0x8c/0xac
> [    0.934425] [<ffff000008093550>] do_iommu_attach.isra.11+0x24/0x7c				//called iommu_get_domain_for_dev, but currently dev->iommu_group is NULL
> [    0.940589] [<ffff000008093604>] __iommu_attach_notifier.part.13+0x5c/0xc8
> [    0.947443] [<ffff000008093690>] __iommu_attach_notifier+0x20/0x2c
> [    0.953608] [<ffff0000080d4c88>] notifier_call_chain+0x4c/0x88
> [    0.959408] [<ffff0000080d5010>] __blocking_notifier_call_chain+0x44/0x74
> [    0.966159] [<ffff0000080d5054>] blocking_notifier_call_chain+0x14/0x1c
> [    0.972754] [<ffff000008489600>] device_add+0x3ac/0x53c
> [    0.977959] [<ffff000008671810>] of_device_add+0x58/0x64
> [    0.983240] [<ffff0000086720c8>] of_platform_device_create_pdata+0x90/0xe8
> [    0.990077] [<ffff00000867220c>] of_platform_bus_create+0xd0/0x31c
> [    0.996265] [<ffff000008672574>] of_platform_populate+0x48/0xac
> [    1.002169] [<ffff000008af2654>] arm64_device_init+0x30/0x58
> [    1.007813] [<ffff000008081a14>] do_one_initcall+0x38/0x128
> [    1.013354] [<ffff000008af0cc4>] kernel_init_freeable+0x14c/0x1ec
> [    1.019439] [<ffff0000087a92d0>] kernel_init+0x10/0xfc
> [    1.024556] [<ffff000008084e10>] ret_from_fork+0x10/0x40
>
> [    0.906207] Failed to set up IOMMU for device tst_smmu; retaining platform DMA ops		//error info
>
> [    0.963782] HugeTLB registered 2 MB page size, pre-allocated 0 pages
> [    0.970442] ACPI: Interpreter disabled.
> [    0.974429] arm-smmu-v3 d0040000.smmu_alg: ias 44-bit, oas 44-bit (features 0x00000f0d)
>
> [    0.996948] Call trace:
> [    0.999400] [<ffff00000808882c>] dump_backtrace+0x0/0x1ac
> [    1.004787] [<ffff0000080889ec>] show_stack+0x14/0x1c
> [    1.009846] [<ffff00000833c290>] dump_stack+0x8c/0xac
> [    1.014905] [<ffff0000084829d4>] arm_smmu_add_device+0x44/0x160
> [    1.020801] [<ffff00000847a218>] add_iommu_group+0x20/0x44
> [    1.026291] [<ffff00000848a43c>] bus_for_each_dev+0x58/0x98
> [    1.031866] [<ffff00000847b220>] bus_set_iommu+0x9c/0xf8
> [    1.037150] [<ffff000008b1b420>] arm_smmu_init+0x78/0x84
> [    1.042475] [<ffff000008081a14>] do_one_initcall+0x38/0x128
> [    1.048043] [<ffff000008af0cc4>] kernel_init_freeable+0x14c/0x1ec
> [    1.054138] [<ffff0000087a931c>] kernel_init+0x10/0xfc
> [    1.059264] [<ffff000008084e10>] ret_from_fork+0x10/0x40
>
> [    1.171026] iommu: Adding device tst_smmu to group 0						//It's already late.
>
> On 2016/6/15 9:22, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2016/6/14 23:16, Will Deacon wrote:
>>> [adding ThunderTown, since he might be able to test this for us]
>>
>> OK. I'm so glad to do it.
>>
>>>
>>> On Fri, Jun 03, 2016 at 06:15:42PM +0100, Robin Murphy wrote:
>>>> With the device <-> stream ID relationship suitably abstracted and
>>>> of_xlate() hooked up, we no longer have any PCI-specifics in play,
>>>> so adding support for the simpler kinds of platform device (a single
>>>> unique stream ID each) becomes trivial; let's do it!
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>>>> ---
>>>>
>>>> v2: New. Consider this one "extra bonus material" as I'm not sure there
>>>>      are even any suitable devices on our model to test it with (it
>>>>      _should_ be OK, given that I know the basic infrastructure on either
>>>>      side works...)
>>>>
>>>>   drivers/iommu/Kconfig       |  2 +-
>>>>   drivers/iommu/arm-smmu-v3.c | 36 ++++++++++++++++++++++++++++++------
>>>>   2 files changed, 31 insertions(+), 7 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 28dcc5ca237e..6379f0ab24fc 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 */
>>>> @@ -1773,6 +1775,22 @@ static void arm_smmu_remove_device(struct device *dev)
>>>>   	iommu_group_remove_device(dev);
>>>>   }
>>>>
>>>> +static struct iommu_group *arm_smmu_device_group(struct device *dev)
>>>> +{
>>>> +	struct iommu_group *group;
>>>> +
>>>> +	/*
>>>> +	 * We've currently no means of grouping non-PCI masters, so
>>>> +	 * there'd better not be any non-unique stream IDs in the DT...
>>>> +	 */
>>>
>>> Worse: what if a SID in the DT aliases with a PCI master? It might be
>>> nice to have some basic snity checking, at least.
>>>
>>> Will
>>>
>>> .
>>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: [PATCH v2 7/7] iommu/arm-smmu: Support non-PCI devices with SMMUv3

[Leizhen (ThunderTown)]

On 2016/6/17 17:14, Robin Murphy wrote:
> On 17/06/16 02:54, Leizhen (ThunderTown) wrote:
>> Hi,
>> I only applied these patch series on lastest 4.7-rc3, is there any patches I missed?
>> According to my test, it seems can not work. The problem is:
> 
> Thanks, that's helpful (if irritating) confirmation.
> 
> Would you mind trying this patch on top to see if that helps: http://article.gmane.org/gmane.linux.kernel.iommu/13991

Although it can eliminate the warning, but still can not work.

> 
> Failing that, we could go back to the forced-device-creation workaround (although I was hoping we'd be able to avoid it):
> 
> -----8<-----
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index e5acd601ee96..556a48f836b3 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2683,9 +2683,15 @@ module_exit(arm_smmu_exit);
> 
>  static int __init arm_smmu_of_init(struct device_node *np)
>  {
> +    static bool registered;
> +
>      of_iommu_set_ops(np, &arm_smmu_ops);
> 
> -    return 0;
> +    if (!registered)
> +        registered = !arm_smmu_init();
> +
> +    return PTR_ERR_OR_ZERO(of_platform_device_create(np, NULL,
> +                    platform_bus_type.dev_root));
>  }
>  IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", arm_smmu_of_init);

This is work well. With or without the above.

> 
> -----8<-----
> 
> Robin.
> 
>>
>> [    0.916536] Call trace:
>> [    0.918967] [<ffff00000808882c>] dump_backtrace+0x0/0x1ac
>> [    0.924361] [<ffff0000080889ec>] show_stack+0x14/0x1c
>> [    0.929384] [<ffff00000833c290>] dump_stack+0x8c/0xac
>> [    0.934425] [<ffff000008093550>] do_iommu_attach.isra.11+0x24/0x7c                //called iommu_get_domain_for_dev, but currently dev->iommu_group is NULL
>> [    0.940589] [<ffff000008093604>] __iommu_attach_notifier.part.13+0x5c/0xc8
>> [    0.947443] [<ffff000008093690>] __iommu_attach_notifier+0x20/0x2c
>> [    0.953608] [<ffff0000080d4c88>] notifier_call_chain+0x4c/0x88
>> [    0.959408] [<ffff0000080d5010>] __blocking_notifier_call_chain+0x44/0x74
>> [    0.966159] [<ffff0000080d5054>] blocking_notifier_call_chain+0x14/0x1c
>> [    0.972754] [<ffff000008489600>] device_add+0x3ac/0x53c
>> [    0.977959] [<ffff000008671810>] of_device_add+0x58/0x64
>> [    0.983240] [<ffff0000086720c8>] of_platform_device_create_pdata+0x90/0xe8
>> [    0.990077] [<ffff00000867220c>] of_platform_bus_create+0xd0/0x31c
>> [    0.996265] [<ffff000008672574>] of_platform_populate+0x48/0xac
>> [    1.002169] [<ffff000008af2654>] arm64_device_init+0x30/0x58
>> [    1.007813] [<ffff000008081a14>] do_one_initcall+0x38/0x128
>> [    1.013354] [<ffff000008af0cc4>] kernel_init_freeable+0x14c/0x1ec
>> [    1.019439] [<ffff0000087a92d0>] kernel_init+0x10/0xfc
>> [    1.024556] [<ffff000008084e10>] ret_from_fork+0x10/0x40
>>
>> [    0.906207] Failed to set up IOMMU for device tst_smmu; retaining platform DMA ops        //error info
>>
>> [    0.963782] HugeTLB registered 2 MB page size, pre-allocated 0 pages
>> [    0.970442] ACPI: Interpreter disabled.
>> [    0.974429] arm-smmu-v3 d0040000.smmu_alg: ias 44-bit, oas 44-bit (features 0x00000f0d)
>>
>> [    0.996948] Call trace:
>> [    0.999400] [<ffff00000808882c>] dump_backtrace+0x0/0x1ac
>> [    1.004787] [<ffff0000080889ec>] show_stack+0x14/0x1c
>> [    1.009846] [<ffff00000833c290>] dump_stack+0x8c/0xac
>> [    1.014905] [<ffff0000084829d4>] arm_smmu_add_device+0x44/0x160
>> [    1.020801] [<ffff00000847a218>] add_iommu_group+0x20/0x44
>> [    1.026291] [<ffff00000848a43c>] bus_for_each_dev+0x58/0x98
>> [    1.031866] [<ffff00000847b220>] bus_set_iommu+0x9c/0xf8
>> [    1.037150] [<ffff000008b1b420>] arm_smmu_init+0x78/0x84
>> [    1.042475] [<ffff000008081a14>] do_one_initcall+0x38/0x128
>> [    1.048043] [<ffff000008af0cc4>] kernel_init_freeable+0x14c/0x1ec
>> [    1.054138] [<ffff0000087a931c>] kernel_init+0x10/0xfc
>> [    1.059264] [<ffff000008084e10>] ret_from_fork+0x10/0x40
>>
>> [    1.171026] iommu: Adding device tst_smmu to group 0                        //It's already late.
>>
>> On 2016/6/15 9:22, Leizhen (ThunderTown) wrote:
>>>
>>>
>>> On 2016/6/14 23:16, Will Deacon wrote:
>>>> [adding ThunderTown, since he might be able to test this for us]
>>>
>>> OK. I'm so glad to do it.
>>>
>>>>
>>>> On Fri, Jun 03, 2016 at 06:15:42PM +0100, Robin Murphy wrote:
>>>>> With the device <-> stream ID relationship suitably abstracted and
>>>>> of_xlate() hooked up, we no longer have any PCI-specifics in play,
>>>>> so adding support for the simpler kinds of platform device (a single
>>>>> unique stream ID each) becomes trivial; let's do it!
>>>>>
>>>>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>>>>> ---
>>>>>
>>>>> v2: New. Consider this one "extra bonus material" as I'm not sure there
>>>>>      are even any suitable devices on our model to test it with (it
>>>>>      _should_ be OK, given that I know the basic infrastructure on either
>>>>>      side works...)
>>>>>
>>>>>   drivers/iommu/Kconfig       |  2 +-
>>>>>   drivers/iommu/arm-smmu-v3.c | 36 ++++++++++++++++++++++++++++++------
>>>>>   2 files changed, 31 insertions(+), 7 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 28dcc5ca237e..6379f0ab24fc 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 */
>>>>> @@ -1773,6 +1775,22 @@ static void arm_smmu_remove_device(struct device *dev)
>>>>>       iommu_group_remove_device(dev);
>>>>>   }
>>>>>
>>>>> +static struct iommu_group *arm_smmu_device_group(struct device *dev)
>>>>> +{
>>>>> +    struct iommu_group *group;
>>>>> +
>>>>> +    /*
>>>>> +     * We've currently no means of grouping non-PCI masters, so
>>>>> +     * there'd better not be any non-unique stream IDs in the DT...
>>>>> +     */
>>>>
>>>> Worse: what if a SID in the DT aliases with a PCI master? It might be
>>>> nice to have some basic snity checking, at least.
>>>>
>>>> Will
>>>>
>>>> .
>>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> 
> .
>