Introduce automatic DMA configuration for IOMMU masters

IOMMU drivers must be initialised before any of their upstream devices, otherwise the relevant iommu_ops won't be configured for the bus in question. To solve this, a number of IOMMU drivers use initcalls to initialise the driver before anything has a chance to be probed.

Whilst this solves the immediate problem, it leaves the job of probing the IOMMU completely separate from the iommu_ops to configure the IOMMU, which are called on a per-bus basis and require the driver to figure out exactly which instance of the IOMMU is being requested. In particular, the add_device callback simply passes a struct device to the driver, which then has to parse firmware tables or probe buses to identify the relevant IOMMU instance.

This patch takes the first step in addressing this problem by adding an early initialisation pass for IOMMU drivers, giving them the ability to store some per-instance data in their iommu_ops structure and store that in their of_node. This can later be used when parsing OF masters to identify the IOMMU instance in question.

Signed-off-by Will Deacon <[email protected]>

---
  drivers/iommu/of_iommu.c          | 17 +++++++++++++++++
  include/asm-generic/vmlinux.lds.h |  2 ++
  include/linux/iommu.h             |  2 ++
  include/linux/of_iommu.h          | 25 +++++++++++++++++++++++++
  4 files changed, 46 insertions(+)

drivers/iommu/of_iommu.c +17 -0

index e550ccb7634e..89b903406968 100644
@@ -22,6 +22,9 @@
  #include <linux/of.h>
  #include <linux/of_iommu.h>
 
+static const struct of_device_id __iommu_of_table_sentinel
+	__used __section(__iommu_of_table_end);
+
  /**
   * of_get_dma_window - Parse *dma-window property and returns 0 if found.
   *

@@ -89,3 +92,17 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
  	return 0;
  }
  EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+void __init of_iommu_init(void)
+{
+	struct device_node *np;
+	const struct of_device_id *match, *matches = &__iommu_of_table;
+
+	for_each_matching_node_and_match(np, matches, &match) {
+		const of_iommu_init_fn init_fn = match->data;
+
+		if (init_fn(np))
+			pr_err("Failed to initialise IOMMU %s\n",
+				of_node_full_name(np));
+	}
+}

include/asm-generic/vmlinux.lds.h +2 -0

index aa70cbda327c..bee5d683074d 100644
@@ -164,6 +164,7 @@
  #define CLKSRC_OF_TABLES()	OF_TABLE(CONFIG_CLKSRC_OF, clksrc)
  #define IRQCHIP_OF_MATCH_TABLE() OF_TABLE(CONFIG_IRQCHIP, irqchip)
  #define CLK_OF_TABLES()		OF_TABLE(CONFIG_COMMON_CLK, clk)
+#define IOMMU_OF_TABLES()	OF_TABLE(CONFIG_OF_IOMMU, iommu)
  #define RESERVEDMEM_OF_TABLES()	OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem)
  #define CPU_METHOD_OF_TABLES()	OF_TABLE(CONFIG_SMP, cpu_method)
  #define EARLYCON_OF_TABLES()	OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)

@@ -497,6 +498,7 @@
  	CLK_OF_TABLES()							\
  	RESERVEDMEM_OF_TABLES()						\
  	CLKSRC_OF_TABLES()						\
+	IOMMU_OF_TABLES()						\
  	CPU_METHOD_OF_TABLES()						\
  	KERNEL_DTB()							\
  	IRQCHIP_OF_MATCH_TABLE()					\

include/linux/iommu.h +2 -0

index e6a7c9ff72f2..7b83f9f8e11d 100644
@@ -103,6 +103,7 @@ enum iommu_attr {
   * @domain_get_attr: Query domain attributes
   * @domain_set_attr: Change domain attributes
   * @pgsize_bitmap: bitmap of supported page sizes
+ * @priv: per-instance data private to the iommu driver
   */
  struct iommu_ops {
  	bool (*capable)(enum iommu_cap);

@@ -133,6 +134,7 @@ struct iommu_ops {
  	u32 (*domain_get_windows)(struct iommu_domain *domain);
 
  	unsigned long pgsize_bitmap;
+	void *priv;
  };
 
  #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */

include/linux/of_iommu.h +5 -0

index 51a560f34bca..81447e0f68b5 100644
@@ -1,12 +1,17 @@
  #ifndef __OF_IOMMU_H
  #define __OF_IOMMU_H
 
+#include <linux/iommu.h>
+#include <linux/of.h>
+
  #ifdef CONFIG_OF_IOMMU
 
  extern int of_get_dma_window(struct device_node *dn, const char *prefix,
  			     int index, unsigned long *busno, dma_addr_t *addr,
  			     size_t *size);
 
+extern void of_iommu_init(void);
+
  #else
 
  static inline int of_get_dma_window(struct device_node *dn, const char *prefix,

include/linux/of_iommu.h +5 -0

+}
+
+static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+	return np->data;

of_dma_configure determines the size of the DMA range for a device by either parsing the dma-ranges property or inspecting the coherent DMA mask. This same information can be used to initialise the max segment size and boundary_mask to a default value.

Signed-off-by Will Deacon <[email protected]>

---
  drivers/of/platform.c           | 4 ++++
  include/linux/amba/bus.h        | 1 +
  include/linux/platform_device.h | 2 ++
  3 files changed, 7 insertions(+)

drivers/of/platform.c +4 -0

index b89caf8c7586..0a2842d91db4 100644
@@ -191,6 +191,8 @@ static void of_dma_configure(struct device *dev)
  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
  	}
  	dev->dma_pfn_offset = offset;
+	dma_set_max_seg_size(dev, size);
+	dma_set_seg_boundary(dev, size);
 
  	coherent = of_dma_is_coherent(dev->of_node);
  	dev_dbg(dev, "device is%sdma coherent\n",

@@ -236,6 +238,7 @@ static struct platform_device *of_platform_device_create_pdata(
 
  	dev->dev.bus = &platform_bus_type;
  	dev->dev.platform_data = platform_data;
+	dev->dev.dma_parms = &dev->dma_parms;
  	of_dma_configure(&dev->dev);
 
  	if (of_device_add(dev) != 0) {

@@ -295,6 +298,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
  	dev->dev.of_node = of_node_get(node);
  	dev->dev.parent = parent;
  	dev->dev.platform_data = platform_data;
+	dev->dev.dma_parms = &dev->dma_parms;
  	if (bus_id)
  		dev_set_name(&dev->dev, "%s", bus_id);
  	else

include/linux/amba/bus.h +1 -0

index c324f5700d1a..9b232eeb6c20 100644
@@ -32,6 +32,7 @@ struct amba_device {
  	struct clk		*pclk;
  	unsigned int		periphid;
  	unsigned int		irq[AMBA_NR_IRQS];
+	struct device_dma_parameters dma_parms;
  };
 
  struct amba_driver {

include/linux/platform_device.h +2 -0

index 153d303af7eb..8dc48487b34b 100644
@@ -27,6 +27,8 @@ struct platform_device {
  	u32		num_resources;
  	struct resource	*resource;
 
+	struct device_dma_parameters dma_parms;
+
  	const struct platform_device_id	*id_entry;
  	char *driver_override; /* Driver name to force a match */
 

Quoted 6 lines · Will Deacon

of_dma_configure determines the size of the DMA range for a device by either parsing the dma-ranges property or inspecting the coherent DMA mask. This same information can be used to initialise the max segment size and boundary_mask to a default value.

Signed-off-by Will Deacon <[email protected]>

Sadly, NAK on this one. After a thorough investigation and testing, generic code really shouldn't be touching dma_parms like this, and I have some corrupted files on my USB filesystem to prove it ;) It seems it's a bad idea to effectively change the default segment size universally unless you really want to go through the entire block layer (and who-knows-what-else) to find and fix everything that relies on it being 64k.

Simply dropping this patch and letting the existing defaults in dma_set_max_seg_size and dma_get_seg_boundary stand seems like the correct action.

Marek, I believe you may have some use case for this - if so, as I understand it your individual drivers should be setting up their own dma_parms directly, to tell the IOMMU they can handle it merging scatterlists into arbitrarily long segments, without affecting the other drivers that assume they'll never see >64k segments and go wrong if they do (including the generic USB stack, apparently).

This patch plumbs the existing ARM IOMMU DMA infrastructure (which isn't actually called outside of a few drivers) into arch_setup_dma_ops, so that we can use IOMMUs for DMA transfers in a more generic fashion.

Since this significantly complicates the arch_setup_dma_ops function, it is moved out of line into dma-mapping.c. If CONFIG_ARM_DMA_USE_IOMMU is not set, the iommu parameter is ignored and the normal ops are used instead.

Signed-off-by Will Deacon <[email protected]>

arch/arm/mm/dma-mapping.c +45 -0

+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				    struct iommu_ops *iommu)
+{
+	struct dma_iommu_mapping *mapping;
+
+	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
+	if (IS_ERR(mapping)) {
+		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
+				size, dev_name(dev));
+		return false;
+	}
+
+	if (arm_iommu_attach_device(dev, mapping)) {
+		pr_warn("Failed to attached device %s to IOMMU_mapping\n",
+				dev_name(dev));
+		arm_iommu_release_mapping(mapping);
+		return false;
+	}
+
+	return true;
+}
+
+static void arm_teardown_iommu_dma_ops(struct device *dev)
+{
+	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
+
+	arm_iommu_detach_device(dev);
+	arm_iommu_release_mapping(mapping);
+}
+
+#else
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				    struct iommu_ops *iommu)
+{
+	return false;
+}
+
+static void arm_teardown_iommu_dma_ops(struct device *dev) { }
+
+#define arm_get_iommu_dma_map_ops arm_get_dma_map_ops
+
+#endif	/* CONFIG_ARM_DMA_USE_IOMMU */
+
+static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)

arch/arm/mm/dma-mapping.c +11 -0

+		dma_ops = arm_get_iommu_dma_map_ops(coherent);
+	else
+		dma_ops = arm_get_dma_map_ops(coherent);
+
+	set_dma_ops(dev, dma_ops);
+}
+
+void arch_teardown_dma_ops(struct device *dev)
+{
+	arm_teardown_iommu_dma_ops(dev);
+}

Quoted 10 lines · Will Deacon

This patch plumbs the existing ARM IOMMU DMA infrastructure (which isn't actually called outside of a few drivers) into arch_setup_dma_ops, so that we can use IOMMUs for DMA transfers in a more generic fashion.

Since this significantly complicates the arch_setup_dma_ops function, it is moved out of line into dma-mapping.c. If CONFIG_ARM_DMA_USE_IOMMU is not set, the iommu parameter is ignored and the normal ops are used instead.

Signed-off-by Will Deacon <[email protected]>

[...]

Quoted 53 lines

arch/arm/mm/dma-mapping.c +53 -0

+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				    struct iommu_ops *iommu)
+{
+	struct dma_iommu_mapping *mapping;
+
+	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
+	if (IS_ERR(mapping)) {
+		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
+				size, dev_name(dev));
+		return false;
+	}
+
+	if (arm_iommu_attach_device(dev, mapping)) {
+		pr_warn("Failed to attached device %s to IOMMU_mapping\n",
+				dev_name(dev));
+		arm_iommu_release_mapping(mapping);
+		return false;
+	}
+
+	return true;
+}
+
+static void arm_teardown_iommu_dma_ops(struct device *dev)
+{
+	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
+
+	arm_iommu_detach_device(dev);
+	arm_iommu_release_mapping(mapping);
+}
+
+#else
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
+				    struct iommu_ops *iommu)
+{
+	return false;
+}
+
+static void arm_teardown_iommu_dma_ops(struct device *dev) { }
+
+#define arm_get_iommu_dma_map_ops arm_get_dma_map_ops
+
+#endif	/* CONFIG_ARM_DMA_USE_IOMMU */
+
+static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
+{
+	return coherent ? &arm_coherent_dma_ops : &arm_dma_ops;
+}
+
+void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+			struct iommu_ops *iommu, bool coherent)
+{
+	struct dma_map_ops *dma_ops;

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))

Is the loss of a null check on iommu (compared to previous versions) intentional? It looks like you're always going to call arm_setup_iommu_dma_ops here for everything regardless, and given that that doesn't even look at the iommu parameter, relying on it to somehow fail correctly smells a bit off.

Here is the fourth iteration of the RFC I've previously posted here:

RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

- Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops).

- iommu_ops is used to hold the per-instance IOMMU data

- Configuration of DMA segments added to of_dma_configure

All feedback welcome.

Quoted 16 lines · On Friday 14 November 2014 18:56:29 Will Deacon wrote:

Here is the fourth iteration of the RFC I've previously posted here:

RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

- Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops).

- iommu_ops is used to hold the per-instance IOMMU data

Quoted 5 lines

- Configuration of DMA segments added to of_dma_configure

All feedback welcome.

Overall I think this is really nice, and I don't mind this going in, I only have one issue with they way you use iommu_ops now:

At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance.

I think rather than adding a .priv pointer to iommu_ops, we should do the same thing that a lot of other subsystems have:

/* generic structure */

struct iommu {
	struct iommu_ops *ops;
	/* possibly other generic per-instance members */
};

/* driver specific structure */

struct arm_smmu {
	struct iommu iommu;

	/* smmu specific members */
};
static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu)
{
	return container_of(iommu, struct arm_smmu, iommu);
}

Quoted 4 lines

At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance.

I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

Quoted 10 lines

I think rather than adding a .priv pointer to iommu_ops, we should do the same thing that a lot of other subsystems have:

/* generic structure */

struct iommu {
      struct iommu_ops *ops;
      /* possibly other generic per-instance members */
};

/* driver specific structure */

Quoted 9 lines

struct arm_smmu {
      struct iommu iommu;

      /* smmu specific members */
};
static inline struct arm_smmu *to_arm_smmu(struct iommu *iommu)
{
      return container_of(iommu, struct arm_smmu, iommu);
}

Regardless of the arguments above, I think this layout is cleaner. We could also move the pgsize_bitmap into struct iommu in that case, however, that would be a more invasive patch series than I what I currently have.

If I do another version of the patch, I can easily add a struct iommu and stash that in the device_node data for the IOMMU instead of directly putting the ops there. That's at least a step in the right direction.

Quoted 7 lines · On Friday 14 November 2014 19:27:54 Will Deacon wrote:

Quoted 3 lines

At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance.

I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops.

(resurrecting an old thread)

Quoted 10 lines · Arnd Bergmann

Quoted 8 lines · On Friday 14 November 2014 19:27:54 Will Deacon wrote:

Quoted 3 lines

At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance.

I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

Quoted 2 lines

Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops.

Now that of_iommu_get_ops has its own list of iommu_ops, we can actually just move pgsize_bitmap into the iommu_domain, which I think makes a lot more sense anyway.

diff below seems to do the trick. The next step would be postponing the pgsize_bitmap initialisation until device attach for the ARM SMMU, since it's at that point that we know the active page table format (and therefore the supported page sizes).

Will

--->8

drivers/iommu/amd_iommu.c +1 -1

index 98024856df07..ab9702d01e9a 100644
@@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain
 *dom) dom->geometry.aperture_end   = ~0ULL;
 	dom->geometry.force_aperture = true;
 
+	dom->pgsize_bitmap = AMD_IOMMU_PGSIZES;
 	return 0;
 
 out_free:

@@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = {
 	.unmap = amd_iommu_unmap,
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = amd_iommu_iova_to_phys,
-	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
 };
 
 /**************************************************************************

*** diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 6cd47b75286f..898fab8d10a0 100644

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

@@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain
 *domain)
 
 	spin_lock_init(&smmu_domain->lock);
 	domain->priv = smmu_domain;
+	domain->pgsize_bitmap = (SECTION_SIZE |
+				 ARM_SMMU_PTE_CONT_SIZE |
+				 PAGE_SIZE);
 	return 0;
 

out_free_domain:

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

@@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = {
 	.remove_device		= arm_smmu_remove_device,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
-	.pgsize_bitmap		= (SECTION_SIZE |
-				   ARM_SMMU_PTE_CONT_SIZE |
-				   PAGE_SIZE),
 };
 
 static void arm_smmu_device_reset(struct arm_smmu_device *smmu)

drivers/iommu/exynos-iommu.c +2 -1

index 7ce52737c7a1..404cad25db9b 100644
@@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain
 *domain) domain->geometry.aperture_end   = ~0UL;
 	domain->geometry.force_aperture = true;
 
+	domain->pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE;
+
 	domain->priv = priv;
 	return 0;
 

@@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = {
 	.iova_to_phys = exynos_iommu_iova_to_phys,
 	.add_device = exynos_iommu_add_device,
 	.remove_device = exynos_iommu_remove_device,
-	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 };
 
 static int __init exynos_iommu_init(void)

drivers/iommu/intel-iommu.c +0 -0

index 40dfbc0444c0..e2b0f34baa9d 100644
@@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain
 *domain) domain->geometry.aperture_end   =

__DOMAIN_MAX_ADDR(dmar_domain->gaw); domain->geometry.force_aperture = true;

drivers/iommu/intel-iommu.c +1 -1

+	domain->pgsize_bitmap = INTEL_IOMMU_PGSIZES;
 	return 0;
 }

@@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = {
 	.iova_to_phys	= intel_iommu_iova_to_phys,
 	.add_device	= intel_iommu_add_device,
 	.remove_device	= intel_iommu_remove_device,
-	.pgsize_bitmap	= INTEL_IOMMU_PGSIZES,
 };
 
 static void quirk_iommu_g4x_gfx(struct pci_dev *dev)

drivers/iommu/iommu.c +8 -8

index f7718d73e984..b8b6b0bc6951 100644
@@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain
 *domain, pgsize = (1UL << (pgsize_idx + 1)) - 1;
 
 	/* throw away page sizes not supported by the hardware */
-	pgsize &= domain->ops->pgsize_bitmap;
+	pgsize &= domain->pgsize_bitmap;
 
 	/* make sure we're still sane */
 	BUG_ON(!pgsize);

@@ -1046,11 +1046,11 @@ int iommu_map(struct iommu_domain *domain, unsigned
 long iova, int ret = 0;
 
 	if (unlikely(domain->ops->map == NULL ||
-		     domain->ops->pgsize_bitmap == 0UL))
+		     domain->pgsize_bitmap == 0UL))
 		return -ENODEV;
 
 	/* find out the minimum page size supported */
-	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
 	/*
 	 * both the virtual address and the physical one, as well as

@@ -1096,11 +1096,11 @@ size_t iommu_unmap(struct iommu_domain *domain,
 unsigned long iova, size_t size) unsigned int min_pagesz;
 
 	if (unlikely(domain->ops->unmap == NULL ||
-		     domain->ops->pgsize_bitmap == 0UL))
+		     domain->pgsize_bitmap == 0UL))
 		return -ENODEV;
 
 	/* find out the minimum page size supported */
-	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
 	/*
 	 * The virtual address, as well as the size of the mapping, must be

@@ -1146,10 +1146,10 @@ size_t default_iommu_map_sg(struct iommu_domain
 *domain, unsigned long iova, unsigned int i, min_pagesz;
 	int ret;
 
-	if (unlikely(domain->ops->pgsize_bitmap == 0UL))
+	if (unlikely(domain->pgsize_bitmap == 0UL))
 		return 0;
 
-	min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+	min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
 	for_each_sg(sg, s, nents, i) {
 		phys_addr_t phys = page_to_phys(sg_page(s)) + s->offset;

@@ -1230,7 +1230,7 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
 break;
 	case DOMAIN_ATTR_PAGING:
 		paging  = data;
-		*paging = (domain->ops->pgsize_bitmap != 0UL);
+		*paging = (domain->pgsize_bitmap != 0UL);
 		break;
 	case DOMAIN_ATTR_WINDOWS:
 		count = data;

drivers/iommu/ipmmu-vmsa.c +1 -0

index 748693192c20..7e6c6a7afd81 100644
@@ -872,6 +872,7 @@ static int ipmmu_domain_init(struct iommu_domain
 *io_domain)
 
 	io_domain->priv = domain;
 	domain->io_domain = io_domain;
+	domain->pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K;
 
 	return 0;

}

drivers/iommu/ipmmu-vmsa.c +0 -3

@@ -1131,7 +1132,6 @@ static const struct iommu_ops ipmmu_ops = {
 	.iova_to_phys = ipmmu_iova_to_phys,
 	.add_device = ipmmu_add_device,
 	.remove_device = ipmmu_remove_device,
-	.pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K,
 };
 
 /*
---------------------------------------------------------------------------
-- diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c index

e1b05379ca0e..8b623dce8161 100644

drivers/iommu/msm_iommu.c +2 -1

@@ -230,6 +230,8 @@ static int msm_iommu_domain_init(struct iommu_domain
 *domain) domain->geometry.aperture_end   = (1ULL << 32) - 1;
 	domain->geometry.force_aperture = true;
 
+	domain->pgsize_bitmap = MSM_IOMMU_PGSIZES;
+
 	return 0;
 
 fail_nomem:

@@ -682,7 +684,6 @@ static const struct iommu_ops msm_iommu_ops = {
 	.unmap = msm_iommu_unmap,
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = msm_iommu_iova_to_phys,
-	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
 };
 
 static int __init get_tex_class(int icp, int ocp, int mt, int nos)

drivers/iommu/omap-iommu.c +2 -1

index bbb7dcef02d3..bc4054967f18 100644
@@ -1250,6 +1250,8 @@ static int omap_iommu_domain_init(struct iommu_domain
 *domain) domain->geometry.aperture_end   = (1ULL << 32) - 1;
 	domain->geometry.force_aperture = true;
 
+	domain->pgsize_bitmap = OMAP_IOMMU_PGSIZES;
+
 	return 0;
 
 fail_nomem:

@@ -1368,7 +1370,6 @@ static const struct iommu_ops omap_iommu_ops = {
 	.iova_to_phys	= omap_iommu_iova_to_phys,
 	.add_device	= omap_iommu_add_device,
 	.remove_device	= omap_iommu_remove_device,
-	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 };
 
 static int __init omap_iommu_init(void)

drivers/iommu/rockchip-iommu.c +1 -1

index 6a8b1ec4a48a..5ab63e51e1c0 100644
@@ -828,6 +828,7 @@ static int rk_iommu_domain_init(struct iommu_domain
 *domain) INIT_LIST_HEAD(&rk_domain->iommus);
 
 	domain->priv = rk_domain;
+	domain->pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP;
 
 	return 0;
 err_dt:

@@ -961,7 +962,6 @@ static const struct iommu_ops rk_iommu_ops = {
 	.add_device = rk_iommu_add_device,
 	.remove_device = rk_iommu_remove_device,
 	.iova_to_phys = rk_iommu_iova_to_phys,
-	.pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
 };
 
 static int rk_iommu_probe(struct platform_device *pdev)

drivers/iommu/shmobile-iommu.c +1 -1

index f1b00774e4de..552bde09a69b 100644
@@ -101,6 +101,7 @@ static int shmobile_iommu_domain_init(struct
 iommu_domain *domain) spin_lock_init(&sh_domain->attached_list_lock);
 	INIT_LIST_HEAD(&sh_domain->attached_list);
 	domain->priv = sh_domain;
+	domain->pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K;
 	return 0;
 }
 

@@ -364,7 +365,6 @@ static const struct iommu_ops shmobile_iommu_ops = {
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = shmobile_iommu_iova_to_phys,
 	.add_device = shmobile_iommu_add_device,
-	.pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K,
 };
 
 int ipmmu_iommu_init(struct shmobile_ipmmu *ipmmu)

drivers/iommu/tegra-gart.c +1 -1

index f722a0c466cf..6404aba9667a 100644
@@ -218,6 +218,7 @@ out:
 
 static int gart_iommu_domain_init(struct iommu_domain *domain)
 {
+	domain->pgsize_bitmap = GART_IOMMU_PGSIZES;
 	return 0;
 }
 

@@ -317,7 +318,6 @@ static const struct iommu_ops gart_iommu_ops = {
 	.map		= gart_iommu_map,
 	.unmap		= gart_iommu_unmap,
 	.iova_to_phys	= gart_iommu_iova_to_phys,
-	.pgsize_bitmap	= GART_IOMMU_PGSIZES,
 };
 
 static int tegra_gart_suspend(struct device *dev)

drivers/iommu/tegra-smmu.c +1 -2

index 6e134c7c227f..a423ed9a19da 100644
@@ -265,6 +265,7 @@ static int tegra_smmu_domain_init(struct iommu_domain
 *domain) pd[i] = 0;
 
 	domain->priv = as;
+	domain->pgsize_bitmap = SZ_4K;
 
 	return 0;
 }

@@ -643,8 +644,6 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.unmap = tegra_smmu_unmap,
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = tegra_smmu_iova_to_phys,
-
-	.pgsize_bitmap = SZ_4K,
 };
 
 static void tegra_smmu_ahb_enable(void)

(quoted diff) +0 -0

b/drivers/vfio/vfio_iommu_type1.c index 4a9d666f1e91..3bb72499e4e1 100644

drivers/vfio/vfio_iommu_type1.c +1 -1

@@ -386,7 +386,7 @@ static unsigned long vfio_pgsize_bitmap(struct
 vfio_iommu *iommu)
 
 	mutex_lock(&iommu->lock);
 	list_for_each_entry(domain, &iommu->domain_list, next)
-		bitmap &= domain->domain->ops->pgsize_bitmap;
+		bitmap &= domain->domain->pgsize_bitmap;
 	mutex_unlock(&iommu->lock);
 

drivers/vfio/vfio_iommu_type1.c +0 -0

 	return bitmap;

include/linux/iommu.h +1 -4

index 38daa453f2e5..ffc023e24b78 100644
@@ -53,6 +53,7 @@ struct iommu_domain_geometry {
 
 struct iommu_domain {
 	const struct iommu_ops *ops;
+	unsigned long pgsize_bitmap;	/* Bitmap of supported page sizes */
 	void *priv;
 	iommu_fault_handler_t handler;
 	void *handler_token;

@@ -108,7 +109,6 @@ enum iommu_attr {
  * @domain_get_attr: Query domain attributes
  * @domain_set_attr: Change domain attributes
  * @of_xlate: add OF master IDs to iommu grouping
- * @pgsize_bitmap: bitmap of supported page sizes
  * @priv: per-instance data private to the iommu driver
  */
 struct iommu_ops {

@@ -144,9 +144,6 @@ struct iommu_ops {
 #ifdef CONFIG_OF_IOMMU
 	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
 #endif
-
-	unsigned long pgsize_bitmap;
-	void *priv;
 };
 
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */

Hi Will,

Thank you for the patch.

Quoted 16 lines · On Monday 19 January 2015 16:06:20 Will Deacon wrote:

(resurrecting an old thread)

Quoted 12 lines · Arnd Bergmann

Quoted 8 lines · On Friday 14 November 2014 19:27:54 Will Deacon wrote:

Quoted 3 lines

At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance.

I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops.

Quoted 3 lines

Now that of_iommu_get_ops has its own list of iommu_ops, we can actually just move pgsize_bitmap into the iommu_domain, which I think makes a lot more sense anyway.

The code looks good to me, but what does it have to do with of_iommu_get_ops() having its own list of iommu_ops ?

Quoted 339 lines

diff below seems to do the trick. The next step would be postponing the pgsize_bitmap initialisation until device attach for the ARM SMMU, since it's at that point that we know the active page table format (and therefore the supported page sizes).

Will

--->8

drivers/iommu/amd_iommu.c +1 -1

index 98024856df07..ab9702d01e9a 100644
@@ -3257,6 +3257,7 @@ static int amd_iommu_domain_init(struct iommu_domain
 *dom) dom->geometry.aperture_end   = ~0ULL;
      dom->geometry.force_aperture = true;
 
+     dom->pgsize_bitmap = AMD_IOMMU_PGSIZES;
      return 0;
 
 out_free:

@@ -3428,7 +3429,6 @@ static const struct iommu_ops amd_iommu_ops = {
      .unmap = amd_iommu_unmap,
      .map_sg = default_iommu_map_sg,
      .iova_to_phys = amd_iommu_iova_to_phys,
-     .pgsize_bitmap  = AMD_IOMMU_PGSIZES,
 };
 
 /**************************************************************************

*** diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 6cd47b75286f..898fab8d10a0 100644

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

@@ -1033,6 +1033,9 @@ static int arm_smmu_domain_init(struct iommu_domain
 *domain)
 
      spin_lock_init(&smmu_domain->lock);
      domain->priv = smmu_domain;
+     domain->pgsize_bitmap = (SECTION_SIZE |
+                              ARM_SMMU_PTE_CONT_SIZE |
+                              PAGE_SIZE);
      return 0;
 

out_free_domain:

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

@@ -1729,9 +1732,6 @@ static const struct iommu_ops arm_smmu_ops = {
      .remove_device          = arm_smmu_remove_device,
      .domain_get_attr        = arm_smmu_domain_get_attr,
      .domain_set_attr        = arm_smmu_domain_set_attr,
-     .pgsize_bitmap          = (SECTION_SIZE |
-                                ARM_SMMU_PTE_CONT_SIZE |
-                                PAGE_SIZE),
 };
 
 static void arm_smmu_device_reset(struct arm_smmu_device *smmu)

drivers/iommu/exynos-iommu.c +2 -1

index 7ce52737c7a1..404cad25db9b 100644
@@ -735,6 +735,8 @@ static int exynos_iommu_domain_init(struct iommu_domain
 *domain) domain->geometry.aperture_end   = ~0UL;
      domain->geometry.force_aperture = true;
 
+     domain->pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE;
+
      domain->priv = priv;
      return 0;
 

@@ -1181,7 +1183,6 @@ static const struct iommu_ops exynos_iommu_ops = {
      .iova_to_phys = exynos_iommu_iova_to_phys,
      .add_device = exynos_iommu_add_device,
      .remove_device = exynos_iommu_remove_device,
-     .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 };
 
 static int __init exynos_iommu_init(void)

drivers/iommu/intel-iommu.c +0 -0

index 40dfbc0444c0..e2b0f34baa9d 100644
@@ -4386,6 +4386,7 @@ static int intel_iommu_domain_init(struct iommu_domain
 *domain) domain->geometry.aperture_end   =

__DOMAIN_MAX_ADDR(dmar_domain->gaw); domain->geometry.force_aperture = true;

drivers/iommu/intel-iommu.c +1 -1

+     domain->pgsize_bitmap = INTEL_IOMMU_PGSIZES;
      return 0;
 }

@@ -4628,7 +4629,6 @@ static const struct iommu_ops intel_iommu_ops = {
      .iova_to_phys   = intel_iommu_iova_to_phys,
      .add_device     = intel_iommu_add_device,
      .remove_device  = intel_iommu_remove_device,
-     .pgsize_bitmap  = INTEL_IOMMU_PGSIZES,
 };
 
 static void quirk_iommu_g4x_gfx(struct pci_dev *dev)

drivers/iommu/iommu.c +8 -8

index f7718d73e984..b8b6b0bc6951 100644
@@ -1025,7 +1025,7 @@ static size_t iommu_pgsize(struct iommu_domain
 *domain, pgsize = (1UL << (pgsize_idx + 1)) - 1;
 
      /* throw away page sizes not supported by the hardware */
-     pgsize &= domain->ops->pgsize_bitmap;
+     pgsize &= domain->pgsize_bitmap;
 
      /* make sure we're still sane */
      BUG_ON(!pgsize);

@@ -1046,11 +1046,11 @@ int iommu_map(struct iommu_domain *domain, unsigned
 long iova, int ret = 0;
 
      if (unlikely(domain->ops->map == NULL ||
-                  domain->ops->pgsize_bitmap == 0UL))
+                  domain->pgsize_bitmap == 0UL))
              return -ENODEV;
 
      /* find out the minimum page size supported */
-     min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+     min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
      /*
       * both the virtual address and the physical one, as well as

@@ -1096,11 +1096,11 @@ size_t iommu_unmap(struct iommu_domain *domain,
 unsigned long iova, size_t size) unsigned int min_pagesz;
 
      if (unlikely(domain->ops->unmap == NULL ||
-                  domain->ops->pgsize_bitmap == 0UL))
+                  domain->pgsize_bitmap == 0UL))
              return -ENODEV;
 
      /* find out the minimum page size supported */
-     min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+     min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
      /*
       * The virtual address, as well as the size of the mapping, must be

@@ -1146,10 +1146,10 @@ size_t default_iommu_map_sg(struct iommu_domain
 *domain, unsigned long iova, unsigned int i, min_pagesz;
      int ret;
 
-     if (unlikely(domain->ops->pgsize_bitmap == 0UL))
+     if (unlikely(domain->pgsize_bitmap == 0UL))
              return 0;
 
-     min_pagesz = 1 << __ffs(domain->ops->pgsize_bitmap);
+     min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
 
      for_each_sg(sg, s, nents, i) {
              phys_addr_t phys = page_to_phys(sg_page(s)) + s->offset;

@@ -1230,7 +1230,7 @@ int iommu_domain_get_attr(struct iommu_domain *domain,
 break;
      case DOMAIN_ATTR_PAGING:
              paging  = data;
-             *paging = (domain->ops->pgsize_bitmap != 0UL);
+             *paging = (domain->pgsize_bitmap != 0UL);
              break;
      case DOMAIN_ATTR_WINDOWS:
              count = data;

drivers/iommu/ipmmu-vmsa.c +1 -0

index 748693192c20..7e6c6a7afd81 100644
@@ -872,6 +872,7 @@ static int ipmmu_domain_init(struct iommu_domain
 *io_domain)
 
      io_domain->priv = domain;
      domain->io_domain = io_domain;
+     domain->pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K;
 
      return 0;

}

drivers/iommu/ipmmu-vmsa.c +0 -3

@@ -1131,7 +1132,6 @@ static const struct iommu_ops ipmmu_ops = {
      .iova_to_phys = ipmmu_iova_to_phys,
      .add_device = ipmmu_add_device,
      .remove_device = ipmmu_remove_device,
-     .pgsize_bitmap = SZ_2M | SZ_64K | SZ_4K,
 };
 
 /*
---------------------------------------------------------------------------
-- diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c index

e1b05379ca0e..8b623dce8161 100644

drivers/iommu/msm_iommu.c +2 -1

@@ -230,6 +230,8 @@ static int msm_iommu_domain_init(struct iommu_domain
 *domain) domain->geometry.aperture_end   = (1ULL << 32) - 1;
      domain->geometry.force_aperture = true;
 
+     domain->pgsize_bitmap = MSM_IOMMU_PGSIZES;
+
      return 0;
 
 fail_nomem:

@@ -682,7 +684,6 @@ static const struct iommu_ops msm_iommu_ops = {
      .unmap = msm_iommu_unmap,
      .map_sg = default_iommu_map_sg,
      .iova_to_phys = msm_iommu_iova_to_phys,
-     .pgsize_bitmap = MSM_IOMMU_PGSIZES,
 };
 
 static int __init get_tex_class(int icp, int ocp, int mt, int nos)

drivers/iommu/omap-iommu.c +2 -1

index bbb7dcef02d3..bc4054967f18 100644
@@ -1250,6 +1250,8 @@ static int omap_iommu_domain_init(struct iommu_domain
 *domain) domain->geometry.aperture_end   = (1ULL << 32) - 1;
      domain->geometry.force_aperture = true;
 
+     domain->pgsize_bitmap = OMAP_IOMMU_PGSIZES;
+
      return 0;
 
 fail_nomem:

@@ -1368,7 +1370,6 @@ static const struct iommu_ops omap_iommu_ops = {
      .iova_to_phys   = omap_iommu_iova_to_phys,
      .add_device     = omap_iommu_add_device,
      .remove_device  = omap_iommu_remove_device,
-     .pgsize_bitmap  = OMAP_IOMMU_PGSIZES,
 };
 
 static int __init omap_iommu_init(void)

drivers/iommu/rockchip-iommu.c +1 -1

index 6a8b1ec4a48a..5ab63e51e1c0 100644
@@ -828,6 +828,7 @@ static int rk_iommu_domain_init(struct iommu_domain
 *domain) INIT_LIST_HEAD(&rk_domain->iommus);
 
      domain->priv = rk_domain;
+     domain->pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP;
 
      return 0;
 err_dt:

@@ -961,7 +962,6 @@ static const struct iommu_ops rk_iommu_ops = {
      .add_device = rk_iommu_add_device,
      .remove_device = rk_iommu_remove_device,
      .iova_to_phys = rk_iommu_iova_to_phys,
-     .pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
 };
 
 static int rk_iommu_probe(struct platform_device *pdev)

drivers/iommu/shmobile-iommu.c +1 -1

index f1b00774e4de..552bde09a69b 100644
@@ -101,6 +101,7 @@ static int shmobile_iommu_domain_init(struct
 iommu_domain *domain) spin_lock_init(&sh_domain->attached_list_lock);
      INIT_LIST_HEAD(&sh_domain->attached_list);
      domain->priv = sh_domain;
+     domain->pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K;
      return 0;
 }
 

@@ -364,7 +365,6 @@ static const struct iommu_ops shmobile_iommu_ops = {
      .map_sg = default_iommu_map_sg,
      .iova_to_phys = shmobile_iommu_iova_to_phys,
      .add_device = shmobile_iommu_add_device,
-     .pgsize_bitmap = SZ_1M | SZ_64K | SZ_4K,
 };
 
 int ipmmu_iommu_init(struct shmobile_ipmmu *ipmmu)

drivers/iommu/tegra-gart.c +1 -1

index f722a0c466cf..6404aba9667a 100644
@@ -218,6 +218,7 @@ out:
 
 static int gart_iommu_domain_init(struct iommu_domain *domain)
 {
+     domain->pgsize_bitmap = GART_IOMMU_PGSIZES;
      return 0;
 }
 

@@ -317,7 +318,6 @@ static const struct iommu_ops gart_iommu_ops = {
      .map            = gart_iommu_map,
      .unmap          = gart_iommu_unmap,
      .iova_to_phys   = gart_iommu_iova_to_phys,
-     .pgsize_bitmap  = GART_IOMMU_PGSIZES,
 };
 
 static int tegra_gart_suspend(struct device *dev)

drivers/iommu/tegra-smmu.c +1 -2

index 6e134c7c227f..a423ed9a19da 100644
@@ -265,6 +265,7 @@ static int tegra_smmu_domain_init(struct iommu_domain
 *domain) pd[i] = 0;
 
      domain->priv = as;
+     domain->pgsize_bitmap = SZ_4K;
 
      return 0;
 }

@@ -643,8 +644,6 @@ static const struct iommu_ops tegra_smmu_ops = {
      .unmap = tegra_smmu_unmap,
      .map_sg = default_iommu_map_sg,
      .iova_to_phys = tegra_smmu_iova_to_phys,
-
-     .pgsize_bitmap = SZ_4K,
 };
 
 static void tegra_smmu_ahb_enable(void)

(quoted diff) +0 -0

b/drivers/vfio/vfio_iommu_type1.c index 4a9d666f1e91..3bb72499e4e1 100644

drivers/vfio/vfio_iommu_type1.c +1 -1

@@ -386,7 +386,7 @@ static unsigned long vfio_pgsize_bitmap(struct
 vfio_iommu *iommu)
 
      mutex_lock(&iommu->lock);
      list_for_each_entry(domain, &iommu->domain_list, next)
-             bitmap &= domain->domain->ops->pgsize_bitmap;
+             bitmap &= domain->domain->pgsize_bitmap;
      mutex_unlock(&iommu->lock);
 

      return bitmap;

include/linux/iommu.h +1 -1

index 38daa453f2e5..ffc023e24b78 100644
@@ -53,6 +53,7 @@ struct iommu_domain_geometry {
 
 struct iommu_domain {
      const struct iommu_ops *ops;
+     unsigned long pgsize_bitmap;    /* Bitmap of supported page sizes */
      void *priv;
      iommu_fault_handler_t handler;
      void *handler_token;

@@ -108,7 +109,6 @@ enum iommu_attr {
  * @domain_get_attr: Query domain attributes
  * @domain_set_attr: Change domain attributes
  * @of_xlate: add OF master IDs to iommu grouping
- * @pgsize_bitmap: bitmap of supported page sizes
  * @priv: per-instance data private to the iommu driver
  */
 struct iommu_ops {

include/linux/iommu.h struct iommu_ops { · line 149

Quoted 9 lines

include/linux/iommu.h +0 -3

@@ -144,9 +144,6 @@ struct iommu_ops {
 #ifdef CONFIG_OF_IOMMU
      int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
 #endif
-
-     unsigned long pgsize_bitmap;
-     void *priv;
 };
 
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE                1 /* Device added */

--
Regards,

Laurent Pinchart

Quoted 16 lines · Laurent Pinchart

Quoted 15 lines · On Monday 19 January 2015 16:06:20 Will Deacon wrote:

Quoted 13 lines · Arnd Bergmann

Quoted 9 lines · On Friday 14 November 2014 19:27:54 Will Deacon wrote:

Quoted 4 lines

At the moment, iommu_ops is a structure that can get used for any number of iommus of the same type, but by putting per-device private data into the same structure you have to duplicate it per instance.

I'm not sure I agree -- the pgsize_bitmap, for example, could vary between different implementations of the same IOMMU. I think we already have this in Juno (some SMMUs can only do 64k pages, whilst others can do 4k and 64k).

Ah, I hadn't noticed that, it should be in the 'struct iommu' then of course, not in iommu_ops.

Quoted 6 lines

Quoted 3 lines

Now that of_iommu_get_ops has its own list of iommu_ops, we can actually just move pgsize_bitmap into the iommu_domain, which I think makes a lot more sense anyway.

The code looks good to me, but what does it have to do with of_iommu_get_ops() having its own list of iommu_ops ?

So, the initial patches for of_iommu_configure/of_xlate made use of a void *priv field in struct iommu_ops (which actually made it to mainline but isn't used). This was replaced by the list maintained by of_iommu_get_ops, but the discussion highlighted that iommu_ops is not per-IOMMU instance and therefore shouldn't contain the pgsize_bitmap, which can (and does) vary between different IOMMU instances in a running system.

This patch is an effort to move the pgsize_bitmap field out of iommu_ops (leaving that structure to contain only function pointers) and into iommu_domain, where I think it makes more sense.

Quoted 4 lines

Quoted 4 lines

diff below seems to do the trick. The next step would be postponing the pgsize_bitmap initialisation until device attach for the ARM SMMU, since it's at that point that we know the active page table format (and therefore the supported page sizes).

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

   RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
   RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
   RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

- Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops).

- iommu_ops is used to hold the per-instance IOMMU data

- Configuration of DMA segments added to of_dma_configure

All feedback welcome.

Cheers,

Will

--->8

Will Deacon (8):
iommu: provide early initialisation hook for IOMMU drivers
dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
iommu: add new iommu_ops callback for adding an OF device
iommu: provide helper function to configure an IOMMU for an of master
dma-mapping: detect and configure IOMMU in of_dma_configure
dma-mapping: set dma segment properties in of_dma_configure
arm: call iommu_init before of_platform_populate
arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

  arch/arm/include/asm/dma-mapping.h | 12 +++---
  arch/arm/kernel/setup.c            |  2 +
  arch/arm/mm/dma-mapping.c          | 80 ++++++++++++++++++++++++++++++++++----
  drivers/iommu/Kconfig              |  2 +-
  drivers/iommu/of_iommu.c           | 50 ++++++++++++++++++++++++
  drivers/of/platform.c              | 54 +++++++++++++------------
  include/asm-generic/vmlinux.lds.h  |  2 +
  include/linux/amba/bus.h           |  1 +
  include/linux/dma-mapping.h        | 13 ++++---
  include/linux/iommu.h              |  8 ++++
  include/linux/of_iommu.h           | 31 +++++++++++++++
  include/linux/platform_device.h    |  2 +
  12 files changed, 214 insertions(+), 43 deletions(-)

Quoted 19 lines · Will Deacon

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

   RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
   RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
   RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

- Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops).

- iommu_ops is used to hold the per-instance IOMMU data

- Configuration of DMA segments added to of_dma_configure

Quoted 1 line

All feedback welcome.

I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine,
You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT
and DMA-mapping subsystem" thread.

You can add to all your patches:

Acked-by Marek Szyprowski <[email protected]>

I'm also interested in adding get_default_domain() callback, but I assume that this
can be done once the basic patchset get merged. Do you plan to work on it, do you want
me to implement it?

Hi Marek,

Quoted 20 lines · Marek Szyprowski

Quoted 19 lines · Will Deacon

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

    RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
    RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
    RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

- Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops).

- iommu_ops is used to hold the per-instance IOMMU data

- Configuration of DMA segments added to of_dma_configure

Quoted 6 lines

Quoted 1 line

All feedback welcome.

I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine,
You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT
and DMA-mapping subsystem" thread.

I just saw that and it looks great, thanks! FWIW, I'll take the first 3 patches you have into my series in some shape or another.

Quoted 2 lines

You can add to all your patches:

Acked-by Marek Szyprowski <[email protected]>

Cheers.

Quoted 5 lines

I'm also interested in adding get_default_domain() callback, but I assume that this
can be done once the basic patchset get merged. Do you plan to work on it, do you want
me to implement it?

If Joerg isn't working on it already (I don't think he is), then please do have a go if you have time. You'll probably want to avoid adding devices with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks) to the default domain, otherwise you'll run into issues initialising the iova allocator.

I had a go at getting ARM dma-mapping to use a hypothetical get_default_domain function, so I've included the diff I ended up with below, in case it's at all useful.

Quoted 27 lines · Will Deacon

Quoted 26 lines · Marek Szyprowski

Quoted 15 lines · Will Deacon

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

    RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
    RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
    RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

- Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops).

Quoted 5 lines

- iommu_ops is used to hold the per-instance IOMMU data

- Configuration of DMA segments added to of_dma_configure

All feedback welcome.

I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine,
You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT
and DMA-mapping subsystem" thread.

Quoted 2 lines

I just saw that and it looks great, thanks! FWIW, I'll take the first 3 patches you have into my series in some shape or another.

It would be great if the iommu integration patches were merged to -next to give
them a try for a few days. Joerg: do you plan to take those patches to v3.19 or
do you want to wait more?

Joerg, would you expect this to go via your tree or via something broader like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead?

Quoted 2 lines · Will Deacon

Joerg, would you expect this to go via your tree or via something broader like arm-soc, with your Ack on the IOMMU bits (patches 1, 3 and 4) instead?

Hmm, I don't like the idea of storing private data in iommu_ops. But given that this is already an improvement we can build on later, here is my

Acked-by Joerg Roedel <[email protected]>

To further improve this we should probably introduce a seperate iommu-descriptor data-structure later which then describes a single hardware iommu device.

Quoted 19 lines · Marek Szyprowski

Quoted 18 lines · Will Deacon

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

   RFCv1:
   http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/28
   3023.html RFCv2:
   http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September
   /283752.html RFCv3:
   http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September
   /287031.html> > 

Changes since RFCv3 include:
- Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops).

- iommu_ops is used to hold the per-instance IOMMU data

Quoted 8 lines

Quoted 3 lines

- Configuration of DMA segments added to of_dma_configure

All feedback welcome.

I've rebased my Exynos SYSMMU patches on top of this patchset and it works fine,
You can find them in the "[PATCH v3 00/19] Exynos SYSMMU (IOMMU) integration with DT and DMA-mapping subsystem" thread.

I just saw that and it looks great, thanks! FWIW, I'll take the first 3 patches you have into my series in some shape or another.

Quoted 2 lines

You can add to all your patches:

Acked-by Marek Szyprowski <[email protected]>

Cheers.

Quoted 5 lines

I'm also interested in adding get_default_domain() callback, but I assume that this
can be done once the basic patchset get merged. Do you plan to work on it, do you want
me to implement it?

If Joerg isn't working on it already (I don't think he is), then please do have a go if you have time. You'll probably want to avoid adding devices with addressing restrictions (i.e. non-zero dma_pfn_offset, weird dma masks) to the default domain, otherwise you'll run into issues initialising the iova allocator.

I had a go at getting ARM dma-mapping to use a hypothetical get_default_domain function, so I've included the diff I ended up with below, in case it's at all useful.

Will

--->8

(quoted diff) +0 -0

b/arch/arm/include/asm/dma-mapping.h index f3c0d953f6a2..5071553bf6b8 100644

arch/arm/include/asm/dma-mapping.h +2 -7

@@ -121,14 +121,9 @@ static inline unsigned long dma_max_pfn(struct device
 *dev) }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
-static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
-				      u64 size, struct iommu_ops *iommu,
-				      bool coherent)
-{
-	if (coherent)
-		set_dma_ops(dev, &arm_coherent_dma_ops);
-}
 #define arch_setup_dma_ops arch_setup_dma_ops
+extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+			       struct iommu_ops *iommu, bool coherent);
 
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {

arch/arm/mm/dma-mapping.c +0 -1

index c245d903927f..da2c2667bbb1 100644
@@ -1849,7 +1849,8 @@ struct dma_map_ops iommu_coherent_ops = {
  * arm_iommu_attach_device function.
  */
 struct dma_iommu_mapping *
-arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t

size) +__arm_iommu_create_mapping(struct iommu_domain *domain, dma_addr_t
base, + size_t size)
{

arch/arm/mm/dma-mapping.c +45 -13

 	unsigned int bits = size >> PAGE_SHIFT;
 	unsigned int bitmap_size = BITS_TO_LONGS(bits) * sizeof(long);

@@ -1883,17 +1884,12 @@ arm_iommu_create_mapping(struct bus_type *bus,
 dma_addr_t base, size_t size) mapping->extensions = extensions;
 	mapping->base = base;
 	mapping->bits = BITS_PER_BYTE * bitmap_size;
+	mapping->domain = domain;
 
 	spin_lock_init(&mapping->lock);
 
-	mapping->domain = iommu_domain_alloc(bus);
-	if (!mapping->domain)
-		goto err4;
-
 	kref_init(&mapping->kref);
 	return mapping;
-err4:
-	kfree(mapping->bitmaps[0]);
 err3:
 	kfree(mapping->bitmaps);
 err2:

@@ -1901,6 +1897,23 @@ err2:
 err:
 	return ERR_PTR(err);
 }
+
+struct dma_iommu_mapping *
+arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t
 size) +{
+	struct dma_iommu_mapping *mapping;
+	struct iommu_domain *domain;
+
+	domain = iommu_domain_alloc(bus);
+	if (!domain)
+		return ERR_PTR(-ENOMEM);
+
+	mapping = __arm_iommu_create_mapping(domain, base, size);
+	if (IS_ERR(mapping))
+		iommu_domain_free(domain);
+
+	return mapping;
+}
 EXPORT_SYMBOL_GPL(arm_iommu_create_mapping);
 
 static void release_iommu_mapping(struct kref *kref)

@@ -1948,9 +1961,8 @@ EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
  *	arm_iommu_create_mapping)
  *
  * Attaches specified io address space mapping to the provided device,
- * this replaces the dma operations (dma_map_ops pointer) with the
- * IOMMU aware version. More than one client might be attached to
- * the same io address space mapping.
+ * More than one client might be attached to the same io address space
+ * mapping.
  */
 int arm_iommu_attach_device(struct device *dev,
 			    struct dma_iommu_mapping *mapping)

@@ -1963,7 +1975,6 @@ int arm_iommu_attach_device(struct device *dev,
 
 	kref_get(&mapping->kref);
 	dev->archdata.mapping = mapping;
-	set_dma_ops(dev, &iommu_ops);
 
 	pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev));
 	return 0;

@@ -1975,7 +1986,6 @@ EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
  * @dev: valid struct device pointer
  *
  * Detaches the provided device from a previously attached map.
- * This voids the dma operations (dma_map_ops pointer)
  */
 void arm_iommu_detach_device(struct device *dev)
 {

@@ -1990,10 +2000,141 @@ void arm_iommu_detach_device(struct device *dev)
 	iommu_detach_device(mapping->domain, dev);
 	kref_put(&mapping->kref, release_iommu_mapping);
 	dev->archdata.mapping = NULL;
-	set_dma_ops(dev, NULL);
 
 	pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev));
 }
 EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
 
-#endif
+static struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
+{
+	return coherent ? &iommu_coherent_ops : &iommu_ops;
+}
+
+struct dma_iommu_mapping_entry {
+	struct list_head		list;
+	struct dma_iommu_mapping	*mapping;
+	struct iommu_domain		*domain;
+	u64				dma_base;
+	u64				size;
+	struct kref			kref;
+};
+
+static DEFINE_SPINLOCK(dma_iommu_mapping_lock);
+static LIST_HEAD(dma_iommu_mapping_table);
+
+static void __remove_iommu_mapping_entry(struct kref *kref)
+{
+	struct dma_iommu_mapping_entry *entry;
+
+	entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
+	list_del(&entry->list);
+}
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64

size,

arch/arm/mm/dma-mapping.c +8 -0

+				    struct iommu_ops *iommu)
+{
+	struct iommu_domain *domain;
+	struct dma_iommu_mapping_entry *entry = NULL;
+
+	if (!iommu->get_default_domain)
+		return false;
+

arch/arm/mm/dma-mapping.c +62 -0

+	/* Load entry->mapping after entry  -- not strictly necessary for ARM */
+	smp_read_barrier_depends();
+
+	if (!entry) {
+		struct dma_iommu_mapping *mapping;
+
+		entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
+		if (!entry)
+			goto err_unlock;
+
+		entry->domain	= domain;
+		entry->dma_base	= dma_base;
+		entry->size	= size;
+		kref_init(&entry->kref);
+		list_add(&entry->list, &dma_iommu_mapping_table);
+		spin_unlock(&dma_iommu_mapping_lock);
+
+		mapping = __arm_iommu_create_mapping(domain, dma_base, size);
+		if (!IS_ERR(mapping))
+			return false;
+
+		smp_wmb();
+		entry->mapping = mapping;
+	} else if (entry->mapping) {
+		if (entry->dma_base > dma_base || entry->size > size)
+			goto err_unlock;
+
+		kref_get(&entry->kref);
+		spin_unlock(&dma_iommu_mapping_lock);
+	} else {
+		/* Racing on the same IOMMU */
+		goto err_unlock;
+	}
+
+	if (arm_iommu_attach_device(dev, entry->mapping)) {
+		int entry_dead;
+
+		pr_warn("Failed to attached device %s to IOMMU mapping\n",
+				dev_name(dev));
+		spin_lock(&dma_iommu_mapping_lock);
+		entry_dead = kref_put(&entry->kref,
+				      __remove_iommu_mapping_entry);
+		spin_unlock(&dma_iommu_mapping_lock);
+
+		if (entry_dead) {
+			entry->mapping->domain = NULL;
+			arm_iommu_release_mapping(entry->mapping);
+		}
+
+		return false;
+	}
+
+	return true;
+
+err_unlock:
+	spin_unlock(&dma_iommu_mapping_lock);
+	return false;
+}
+
+#else
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64

size, + struct iommu_ops *iommu)

arch/arm/mm/dma-mapping.c +25 -0

+{
+	return false;
+}
+
+#define arm_get_iommu_dma_map_ops arm_get_dma_map_ops
+
+#endif	/* CONFIG_ARM_DMA_USE_IOMMU */
+
+static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
+{
+	return coherent ? &arm_coherent_dma_ops : &arm_dma_ops;
+}
+
+void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+			struct iommu_ops *iommu, bool coherent)
+{
+	struct dma_map_ops *dma_ops;
+
+	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
+		dma_ops = arm_get_iommu_dma_map_ops(coherent);
+	else
+		dma_ops = arm_get_dma_map_ops(coherent);
+
+	set_dma_ops(dev, dma_ops);
+}

Hi Will,

Quoted 25 lines

arch/arm/mm/dma-mapping.c +9 -0

+static void __remove_iommu_mapping_entry(struct kref *kref)
+{
+	struct dma_iommu_mapping_entry *entry;
+
+	entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
+	list_del(&entry->list);
+}
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64

size,

arch/arm/mm/dma-mapping.c +15 -0

+				    struct iommu_ops *iommu)
+{
+	struct iommu_domain *domain;
+	struct dma_iommu_mapping_entry *entry = NULL;
+
+	if (!iommu->get_default_domain)
+		return false;
+
+	domain = iommu->get_default_domain(dev);
+	if (!domain)
+		return false;
+
+	spin_lock(&dma_iommu_mapping_lock);
+
+	list_for_each_entry(entry, &dma_iommu_mapping_table, list) {

arch/arm/mm/dma-mapping.c void arm_iommu_detach_device(struct device *dev) · line 2051

Quoted 3 lines

arch/arm/mm/dma-mapping.c +3 -0

+		if (entry->domain == domain)
+			break;
+	}

That might be a stupid question (fighting my impostor syndrome again here), but is there a fundamental reason why we can't store the VA allocation data (probably using iova) in the domain instead of having to go through hoops and loops here to associate that data to the domain ?

Quoted 27 lines · Laurent Pinchart

Quoted 26 lines · On Wednesday 19 November 2014 11:41:50 Will Deacon wrote:

arch/arm/mm/dma-mapping.c +9 -0

+static void __remove_iommu_mapping_entry(struct kref *kref)
+{
+	struct dma_iommu_mapping_entry *entry;
+
+	entry = container_of(kref, struct dma_iommu_mapping_entry, kref);
+	list_del(&entry->list);
+}
+
+static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base,

u64
size,

arch/arm/mm/dma-mapping.c +15 -0

+				    struct iommu_ops *iommu)
+{
+	struct iommu_domain *domain;
+	struct dma_iommu_mapping_entry *entry = NULL;
+
+	if (!iommu->get_default_domain)
+		return false;
+
+	domain = iommu->get_default_domain(dev);
+	if (!domain)
+		return false;
+
+	spin_lock(&dma_iommu_mapping_lock);
+
+	list_for_each_entry(entry, &dma_iommu_mapping_table, list) {

Quoted 8 lines

arch/arm/mm/dma-mapping.c void arm_iommu_detach_device(struct device *dev) · line 2051

Quoted 3 lines

arch/arm/mm/dma-mapping.c +3 -0

+		if (entry->domain == domain)
+			break;
+	}

That might be a stupid question (fighting my impostor syndrome again here), but is there a fundamental reason why we can't store the VA allocation data (probably using iova) in the domain instead of having to go through hoops and loops here to associate that data to the domain ?

Well, this was very much a work-in-progress that I ended up abandoning :)

Ultimately, I'd really like to see the default domain management moved into core code, at which point extending struct iommu_domain seems perfectly plausible to me (not sure if we'd have a new structure for that).

Then we'd have a fairly clean device -> group -> domain -> allocator abstraction for each master.

Hello everybody,

Here is the fourth iteration of the RFC I've previously posted here:

   RFCv1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/283023.html
   RFCv2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283752.html
   RFCv3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/287031.html

Changes since RFCv3 include:

- Drastic simplification of the data structures, so that we no longer pass around lists of domains. Instead, dma-mapping is expected to allocate the domain (Joerg talked about adding a get_default_domain operation to iommu_ops).

- iommu_ops is used to hold the per-instance IOMMU data

- Configuration of DMA segments added to of_dma_configure

All feedback welcome.

Cheers,

Will

--->8

Will Deacon (8):
iommu: provide early initialisation hook for IOMMU drivers
dma-mapping: replace set_arch_dma_coherent_ops with arch_setup_dma_ops
iommu: add new iommu_ops callback for adding an OF device
iommu: provide helper function to configure an IOMMU for an of master
dma-mapping: detect and configure IOMMU in of_dma_configure
dma-mapping: set dma segment properties in of_dma_configure
arm: call iommu_init before of_platform_populate
arm: dma-mapping: plumb our iommu mapping ops into arch_setup_dma_ops

  arch/arm/include/asm/dma-mapping.h | 12 +++---
  arch/arm/kernel/setup.c            |  2 +
  arch/arm/mm/dma-mapping.c          | 80 ++++++++++++++++++++++++++++++++++----
  drivers/iommu/Kconfig              |  2 +-
  drivers/iommu/of_iommu.c           | 50 ++++++++++++++++++++++++
  drivers/of/platform.c              | 54 +++++++++++++------------
  include/asm-generic/vmlinux.lds.h  |  2 +
  include/linux/amba/bus.h           |  1 +
  include/linux/dma-mapping.h        | 13 ++++---
  include/linux/iommu.h              |  8 ++++
  include/linux/of_iommu.h           | 31 +++++++++++++++
  include/linux/platform_device.h    |  2 +
  12 files changed, 214 insertions(+), 43 deletions(-)