Generic DT bindings for PCI IOMMUs and ARM SMMUv3

From: Lorenzo Pieralisi <[email protected]>

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

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

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

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

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

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marek Szyprowski <[email protected]>

Signed-off-by Lorenzo Pieralisi <[email protected]>

[rm: get rid of other now-redundant bits]

Signed-off-by Robin Murphy <[email protected]>

---

v4: No change.

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

Quoted 43 lines · Robin Murphy

From: Lorenzo Pieralisi <[email protected]>

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

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

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

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

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

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Marek Szyprowski <[email protected]>

Signed-off-by Lorenzo Pieralisi <[email protected]>

[rm: get rid of other now-redundant bits]

Signed-off-by Robin Murphy <[email protected]>

---

v4: No change.

Quoted 2 lines

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

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

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 <[email protected]>

CC Frank Rowand <[email protected]>

Acked-by Marc Zyngier <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

v4: No change.

 drivers/of/irq.c       |  78 ++-----------------------------------
 drivers/of/of_pci.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pci.h |  10 +++++
 3 files changed, 116 insertions(+), 74 deletions(-)

drivers/of/irq.c +4 -74

index 6ec743faabe8..091d45e6d61d 100644
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
+#include <linux/of_pci.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 

@@ -587,87 +588,16 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
 			    u32 rid_in)
 {
 	struct device *parent_dev;
-	struct device_node *msi_controller_node;
-	struct device_node *msi_np = *np;
-	u32 map_mask, masked_rid, rid_base, msi_base, rid_len, phandle;
-	int msi_map_len;
-	bool matched;
 	u32 rid_out = rid_in;
-	const __be32 *msi_map = NULL;
 
 	/*
 	 * Walk up the device parent links looking for one with a
 	 * "msi-map" property.
 	 */
-	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
-		if (!parent_dev->of_node)
-			continue;
-
-		msi_map = of_get_property(parent_dev->of_node,
-					  "msi-map", &msi_map_len);
-		if (!msi_map)
-			continue;
-
-		if (msi_map_len % (4 * sizeof(__be32))) {
-			dev_err(parent_dev, "Error: Bad msi-map length: %d\n",
-				msi_map_len);
-			return rid_out;
-		}
-		/* We have a good parent_dev and msi_map, let's use them. */
-		break;
-	}
-	if (!msi_map)
-		return rid_out;
-
-	/* The default is to select all bits. */
-	map_mask = 0xffffffff;
-
-	/*
-	 * Can be overridden by "msi-map-mask" property.  If
-	 * of_property_read_u32() fails, the default is used.
-	 */
-	of_property_read_u32(parent_dev->of_node, "msi-map-mask", &map_mask);
-
-	masked_rid = map_mask & rid_in;
-	matched = false;
-	while (!matched && msi_map_len >= 4 * sizeof(__be32)) {
-		rid_base = be32_to_cpup(msi_map + 0);
-		phandle = be32_to_cpup(msi_map + 1);
-		msi_base = be32_to_cpup(msi_map + 2);
-		rid_len = be32_to_cpup(msi_map + 3);
-
-		if (rid_base & ~map_mask) {
-			dev_err(parent_dev,
-				"Invalid msi-map translation - msi-map-mask (0x%x) ignores rid-base (0x%x)\n",
-				map_mask, rid_base);
-			return rid_out;
-		}
-
-		msi_controller_node = of_find_node_by_phandle(phandle);
-
-		matched = (masked_rid >= rid_base &&
-			   masked_rid < rid_base + rid_len);
-		if (msi_np)
-			matched &= msi_np == msi_controller_node;
-
-		if (matched && !msi_np) {
-			*np = msi_np = msi_controller_node;
+	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
+		if (!of_pci_map_rid(parent_dev->of_node, rid_in, "msi-map",
+				    "msi-map-mask", np, &rid_out))
 			break;
-		}
-
-		of_node_put(msi_controller_node);
-		msi_map_len -= 4 * sizeof(__be32);
-		msi_map += 4;
-	}
-	if (!matched)
-		return rid_out;
-
-	rid_out = masked_rid - rid_base + msi_base;
-	dev_dbg(dev,
-		"msi-map at: %s, using mask %08x, rid-base: %08x, msi-base: %08x, length: %08x, rid: %08x -> %08x\n",
-		dev_name(parent_dev), map_mask, rid_base, msi_base,
-		rid_len, rid_in, rid_out);
-
 	return rid_out;
 }
 

drivers/of/of_pci.c +102 -0

index 13f4fed38048..e6465cf7aea9 100644
@@ -306,3 +306,105 @@ struct msi_controller *of_pci_find_msi_chip_by_node(struct device_node *of_node)
 EXPORT_SYMBOL_GPL(of_pci_find_msi_chip_by_node);
 
 #endif /* CONFIG_PCI_MSI */
+
+/**
+ * of_pci_map_rid - Translate a requester ID through a downstream mapping.
+ * @np: root complex device node.
+ * @rid: PCI requester ID to map.
+ * @map_name: property name of the map to use.
+ * @map_mask_name: optional property name of the mask to use.
+ * @target: optional pointer to a target device node.
+ * @id_out: optional pointer to receive the translated ID.
+ *
+ * Given a PCI requester ID, look up the appropriate implementation-defined
+ * platform ID and/or the target device which receives transactions on that
+ * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or
+ * @id_out may be NULL if only the other is required. If @target points to
+ * a non-NULL device node pointer, only entries targeting that node will be
+ * matched; if it points to a NULL value, it will receive the device node of
+ * the first matching target phandle, with a reference held.
+ *
+ * Return: 0 on success or a standard error code on failure.
+ */
+int of_pci_map_rid(struct device_node *np, u32 rid,
+		   const char *map_name, const char *map_mask_name,
+		   struct device_node **target, u32 *id_out)
+{
+	u32 map_mask, masked_rid;
+	int map_len;
+	const __be32 *map = NULL;
+
+	if (!np || !map_name || (!target && !id_out))
+		return -EINVAL;
+
+	map = of_get_property(np, map_name, &map_len);
+	if (!map) {
+		if (target)
+			return -ENODEV;
+		/* Otherwise, no map implies no translation */
+		*id_out = rid;
+		return 0;
+	}
+
+	if (!map_len || map_len % (4 * sizeof(*map))) {
+		pr_err("%s: Error: Bad %s length: %d\n", np->full_name,
+			map_name, map_len);
+		return -EINVAL;
+	}
+
+	/* The default is to select all bits. */
+	map_mask = 0xffffffff;
+
+	/*
+	 * Can be overridden by "{iommu,msi}-map-mask" property.
+	 * If of_property_read_u32() fails, the default is used.
+	 */
+	if (map_mask_name)
+		of_property_read_u32(np, map_mask_name, &map_mask);
+
+	masked_rid = map_mask & rid;
+	for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4) {
+		struct device_node *phandle_node;
+		u32 rid_base = be32_to_cpup(map + 0);
+		u32 phandle = be32_to_cpup(map + 1);
+		u32 out_base = be32_to_cpup(map + 2);
+		u32 rid_len = be32_to_cpup(map + 3);
+
+		if (rid_base & ~map_mask) {
+			pr_err("%s: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n",
+				np->full_name, map_name, map_name,
+				map_mask, rid_base);
+			return -EFAULT;
+		}
+
+		if (masked_rid < rid_base || masked_rid >= rid_base + rid_len)
+			continue;
+
+		phandle_node = of_find_node_by_phandle(phandle);
+		if (!phandle_node)
+			return -ENODEV;
+
+		if (target) {
+			if (*target)
+				of_node_put(phandle_node);
+			else
+				*target = phandle_node;
+
+			if (*target != phandle_node)
+				continue;
+		}
+
+		if (id_out)
+			*id_out = masked_rid - rid_base + out_base;
+
+		pr_debug("%s: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n",
+			np->full_name, map_name, map_mask, rid_base, out_base,
+			rid_len, rid, *id_out);
+		return 0;
+	}
+
+	pr_err("%s: Invalid %s translation - no match for rid 0x%x on %s\n",
+		np->full_name, map_name, rid,
+		target && *target ? (*target)->full_name : "any target");
+	return -EFAULT;
+}

include/linux/of_pci.h +10 -0

index b969e9443962..7fd5cfce9140 100644
@@ -17,6 +17,9 @@ int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin);
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
 void of_pci_check_probe_only(void);
+int of_pci_map_rid(struct device_node *np, u32 rid,
+		   const char *map_name, const char *map_mask_name,
+		   struct device_node **target, u32 *id_out);
 #else
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {

@@ -52,6 +55,13 @@ of_get_pci_domain_nr(struct device_node *node)
 	return -1;
 }
 
+static inline int of_pci_map_rid(struct device_node *np, u32 rid,
+			const char *map_name, const char *map_mask_name,
+			struct device_node **target, u32 *id_out)
+{
+	return -EINVAL;
+}
+
 static inline void of_pci_check_probe_only(void) { }
 #endif
 

Signature · 1 line

2.8.1.dirty

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 <[email protected]>

CC Frank Rowand <[email protected]>

Acked-by Marc Zyngier <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

drivers/iommu/of_iommu.c +15 -0

+	fwspec = krealloc(dev->archdata.iommu, size, GFP_KERNEL);
+	if (!fwspec)
+		return -ENOMEM;
+
+	while (num_ids--)
+		fwspec->ids[fwspec->num_ids++] = *ids++;
+
+	dev->archdata.iommu = fwspec;
+	return 0;
+}
+
+inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
+{
+	return dev->archdata.iommu;
+}

include/linux/of_iommu.h +15 -0

index bd02b44902d0..308791fca32d 100644
@@ -15,6 +15,14 @@ extern void of_iommu_init(void);
 extern const struct iommu_ops *of_iommu_configure(struct device *dev,
 					struct device_node *master_np);
 
+struct iommu_fwspec {
+	const struct iommu_ops	*iommu_ops;
+	struct device_node	*iommu_np;
+	void			*iommu_priv;
+	unsigned int		num_ids;
+	u32			ids[];
+};
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -31,8 +39,15 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 	return NULL;
 }
 
+struct iommu_fwspec;
+
 #endif	/* CONFIG_OF_IOMMU */
 
+int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+struct iommu_fwspec *dev_iommu_fwspec(struct device *dev);
+
 void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
 const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
 

Signature · 1 line

2.8.1.dirty

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

[...]

drivers/iommu/of_iommu.c void __init of_iommu_init(void) · line 255

Quoted 9 lines

drivers/iommu/of_iommu.c +9 -0

+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
+{
+	struct iommu_fwspec *fwspec = dev->archdata.iommu;
+	size_t size;
+
+	if (!fwspec)
+		return -EINVAL;
+
+	size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + 1]);

								^^^^

+ num_ids ?

Lorenzo

Quoted 53 lines

drivers/iommu/of_iommu.c +15 -0

+	fwspec = krealloc(dev->archdata.iommu, size, GFP_KERNEL);
+	if (!fwspec)
+		return -ENOMEM;
+
+	while (num_ids--)
+		fwspec->ids[fwspec->num_ids++] = *ids++;
+
+	dev->archdata.iommu = fwspec;
+	return 0;
+}
+
+inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
+{
+	return dev->archdata.iommu;
+}

include/linux/of_iommu.h +15 -0

index bd02b44902d0..308791fca32d 100644
@@ -15,6 +15,14 @@ extern void of_iommu_init(void);
 extern const struct iommu_ops *of_iommu_configure(struct device *dev,
 					struct device_node *master_np);
 
+struct iommu_fwspec {
+	const struct iommu_ops	*iommu_ops;
+	struct device_node	*iommu_np;
+	void			*iommu_priv;
+	unsigned int		num_ids;
+	u32			ids[];
+};
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

@@ -31,8 +39,15 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 	return NULL;
 }
 
+struct iommu_fwspec;
+
 #endif	/* CONFIG_OF_IOMMU */
 
+int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+struct iommu_fwspec *dev_iommu_fwspec(struct device *dev);
+
 void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
 const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
 

Signature · 1 line

2.8.1.dirty

drivers/iommu/arm-smmu-v3.c +9 -0

+
+	ret = iommu_fwspec_init(dev, args->np);
+	if (!ret)
+		ret = iommu_fwspec_add_ids(dev, &args->args[0], 1);
+
+	return ret;
+}
+
 static struct iommu_ops arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,

@@ -1947,6 +1894,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.device_group		= pci_device_group,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
+	.of_xlate		= arm_smmu_of_xlate,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 };
 

drivers/iommu/arm-smmu-v3.c +10 -0

+
+	if (!of_platform_device_create(np, NULL, platform_bus_type.dev_root))
+		return -ENODEV;
+
+	of_iommu_set_ops(np, &arm_smmu_ops);
+
+	return 0;
+}
+IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", arm_smmu_of_init);
+
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
 MODULE_AUTHOR("Will Deacon <[email protected]>");
 MODULE_LICENSE("GPL v2");

Signature · 1 line

2.8.1.dirty

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

[...]

drivers/iommu/arm-smmu-v3.c out_unlock: · line 1875

Quoted 7 lines

drivers/iommu/arm-smmu-v3.c +7 -0

+static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	int ret;
+
+	/* We only support PCI, for now */
+	if (!dev_is_pci(dev))
+		return -ENODEV;

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

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

Quoted 23 lines

drivers/iommu/arm-smmu-v3.c +9 -0

+
+	ret = iommu_fwspec_init(dev, args->np);
+	if (!ret)
+		ret = iommu_fwspec_add_ids(dev, &args->args[0], 1);
+
+	return ret;
+}
+
 static struct iommu_ops arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,

@@ -1947,6 +1894,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.device_group		= pci_device_group,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
+	.of_xlate		= arm_smmu_of_xlate,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 };
 

@@ -2697,6 +2645,22 @@ static void __exit arm_smmu_exit(void)
 subsys_initcall(arm_smmu_init);
 module_exit(arm_smmu_exit);
 

drivers/iommu/arm-smmu-v3.c static void __exit arm_smmu_exit(void) · line 2653

Quoted 6 lines

drivers/iommu/arm-smmu-v3.c +6 -0

+static int __init arm_smmu_of_init(struct device_node *np)
+{
+	static bool registered;
+
+	if (!registered)
+		registered = !arm_smmu_init();

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

Thanks,
Lorenzo

Quoted 16 lines

drivers/iommu/arm-smmu-v3.c +10 -0

+
+	if (!of_platform_device_create(np, NULL, platform_bus_type.dev_root))
+		return -ENODEV;
+
+	of_iommu_set_ops(np, &arm_smmu_ops);
+
+	return 0;
+}
+IOMMU_OF_DECLARE(arm_smmuv3, "arm,smmu-v3", arm_smmu_of_init);
+
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
 MODULE_AUTHOR("Will Deacon <[email protected]>");
 MODULE_LICENSE("GPL v2");

Signature · 1 line

2.8.1.dirty

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

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

Signed-off-by Robin Murphy <[email protected]>

---

v4: Add explicit device probe in of_init callback.

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

drivers/iommu/arm-smmu-v3.c +0 -0

index 94b68213c50d..fb438664b9f0 100644

drivers/iommu/arm-smmu-v3.c +14 -30

@@ -1800,94 +1752,74 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 	return sid < limit;
 }
 
+static struct iommu_ops arm_smmu_ops;
+
 static int arm_smmu_add_device(struct device *dev)
 {
 	int i, ret;
-	u32 sid, *sids;
-	struct pci_dev *pdev;
-	struct iommu_group *group;
-	struct arm_smmu_group *smmu_group;
 	struct arm_smmu_device *smmu;
+	struct arm_smmu_master_data *master;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
+	struct iommu_group *group;
 
-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
+	if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
 		return -ENODEV;
-
-	pdev = to_pci_dev(dev);
-	group = iommu_group_get_for_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
-	smmu_group = iommu_group_get_iommudata(group);
-	if (!smmu_group) {
-		smmu = arm_smmu_get_for_pci_dev(pdev);
-		if (!smmu) {
-			ret = -ENOENT;
-			goto out_remove_dev;
-		}
-
-		smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
-		if (!smmu_group) {
-			ret = -ENOMEM;
-			goto out_remove_dev;
-		}
-
-		smmu_group->ste.valid	= true;
-		smmu_group->smmu	= smmu;
-		iommu_group_set_iommudata(group, smmu_group,
-					  __arm_smmu_release_pci_iommudata);
+	/*
+	 * We _can_ actually withstand dodgy bus code re-calling add_device()
+	 * without an intervening remove_device()/of_xlate() sequence, but
+	 * we're not going to do so quietly...
+	 */
+	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
+		master = fwspec->iommu_priv;
+		smmu = master->smmu;
 	} else {

Hi Robin,

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

Quoted 24 lines · Robin Murphy

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

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

Signed-off-by Robin Murphy <[email protected]>

---

v4: Add explicit device probe in of_init callback.

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

drivers/iommu/arm-smmu-v3.c +0 -0

index 94b68213c50d..fb438664b9f0 100644

[...]

Quoted 61 lines

drivers/iommu/arm-smmu-v3.c +19 -31

@@ -1800,94 +1752,74 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 	return sid < limit;
 }
 
+static struct iommu_ops arm_smmu_ops;
+
 static int arm_smmu_add_device(struct device *dev)
 {
 	int i, ret;
-	u32 sid, *sids;
-	struct pci_dev *pdev;
-	struct iommu_group *group;
-	struct arm_smmu_group *smmu_group;
 	struct arm_smmu_device *smmu;
+	struct arm_smmu_master_data *master;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
+	struct iommu_group *group;
 
-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
+	if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
 		return -ENODEV;
-
-	pdev = to_pci_dev(dev);
-	group = iommu_group_get_for_dev(dev);
-	if (IS_ERR(group))
-		return PTR_ERR(group);
-
-	smmu_group = iommu_group_get_iommudata(group);
-	if (!smmu_group) {
-		smmu = arm_smmu_get_for_pci_dev(pdev);
-		if (!smmu) {
-			ret = -ENOENT;
-			goto out_remove_dev;
-		}
-
-		smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
-		if (!smmu_group) {
-			ret = -ENOMEM;
-			goto out_remove_dev;
-		}
-
-		smmu_group->ste.valid	= true;
-		smmu_group->smmu	= smmu;
-		iommu_group_set_iommudata(group, smmu_group,
-					  __arm_smmu_release_pci_iommudata);
+	/*
+	 * We _can_ actually withstand dodgy bus code re-calling add_device()
+	 * without an intervening remove_device()/of_xlate() sequence, but
+	 * we're not going to do so quietly...
+	 */
+	if (WARN_ON_ONCE(fwspec->iommu_priv)) {
+		master = fwspec->iommu_priv;
+		smmu = master->smmu;
 	} else {
-		smmu = smmu_group->smmu;
+		smmu = arm_smmu_get_by_node(fwspec->iommu_np);
+		if (!smmu)
+			return -ENODEV;
+		master = kzalloc(sizeof(*master), GFP_KERNEL);
+		if (!master)

drivers/iommu/arm-smmu-v3.c static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid) · line 1784

Quoted 4 lines

drivers/iommu/arm-smmu-v3.c +4 -0

+			return -ENOMEM;
+
+		master->smmu = smmu;
+		fwspec->iommu_priv = master;

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

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

Thanks,
Jean-Philippe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines · Jean-Philippe Brucker

Hi Robin,

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

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

Quoted 5 lines

Quoted 5 lines

drivers/iommu/arm-smmu-v3.c +5 -0

+		smmu = arm_smmu_get_by_node(fwspec->iommu_np);
+		if (!smmu)
+			return -ENODEV;
+		master = kzalloc(sizeof(*master), GFP_KERNEL);
+		if (!master)

Quoted 8 lines

drivers/iommu/arm-smmu-v3.c static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid) · line 1784

Quoted 4 lines

drivers/iommu/arm-smmu-v3.c +4 -0

+			return -ENOMEM;
+
+		master->smmu = smmu;
+		fwspec->iommu_priv = master;

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

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

Quoted 5 lines

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

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

Robin.

Quoted 10 lines

Thanks,
Jean-Philippe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel