Add support for ARM SMMU architectures 1 and 2

The PL330 driver probes the peripheral and primecell IDs of the device to make sure that it is indeed an AMBA PL330. However, it does this by making byte accesses to a device mapping of the word-aligned ID registers, which is either UNPREDICTABLE or generates an alignment fault (depending on the presence of the virtualisation extensions).

Rather than fix this code, we can actually rip most of it out and let the AMBA bus driver correctly do the probing for us.

Acked-by Jassi Brar <[email protected]>

The PL330 driver probes the peripheral and primecell IDs of the device to make sure that it is indeed an AMBA PL330. However, it does this by making byte accesses to a device mapping of the word-aligned ID registers, which is either UNPREDICTABLE or generates an alignment fault (depending on the presence of the virtualisation extensions).

Rather than fix this code, we can actually rip most of it out and let the AMBA bus driver correctly do the probing for us.

Quoted 9 lines · Will Deacon

The PL330 driver probes the peripheral and primecell IDs of the device to make sure that it is indeed an AMBA PL330. However, it does this by making byte accesses to a device mapping of the word-aligned ID registers, which is either UNPREDICTABLE or generates an alignment fault (depending on the presence of the virtualisation extensions).

Rather than fix this code, we can actually rip most of it out and let the AMBA bus driver correctly do the probing for us.

Acked-by Jassi Brar <[email protected]>

The PL330 driver probes the peripheral and primecell IDs of the device to make sure that it is indeed an AMBA PL330. However, it does this by making byte accesses to a device mapping of the word-aligned ID registers, which is either UNPREDICTABLE or generates an alignment fault (depending on the presence of the virtualisation extensions).

Rather than fix this code, we can actually rip most of it out and let the AMBA bus driver correctly do the probing for us.

Cc Jassi Brar <[email protected]>

Cc Vinod Koul <[email protected]>

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

---
 drivers/dma/pl330.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

drivers/dma/pl330.c +3 -24

index 24e0754..22e2a8f 100644
@@ -157,7 +157,6 @@ enum pl330_reqtype {
 #define PERIPH_REV_R0P0		0
 #define PERIPH_REV_R1P0		1
 #define PERIPH_REV_R1P1		2
-#define PCELL_ID		0xff0
 
 #define CR0_PERIPH_REQ_SET	(1 << 0)
 #define CR0_BOOT_EN_SET		(1 << 1)

@@ -193,8 +192,6 @@ enum pl330_reqtype {
 #define INTEG_CFG		0x0
 #define PERIPH_ID_VAL		((PART << 0) | (DESIGNER << 12))
 
-#define PCELL_ID_VAL		0xb105f00d
-
 #define PL330_STATE_STOPPED		(1 << 0)
 #define PL330_STATE_EXECUTING		(1 << 1)
 #define PL330_STATE_WFE			(1 << 2)

@@ -292,7 +289,6 @@ static unsigned cmd_line;
 /* Populated by the PL330 core driver for DMA API driver's info */
 struct pl330_config {
 	u32	periph_id;
-	u32	pcell_id;
 #define DMAC_MODE_NS	(1 << 0)
 	unsigned int	mode;
 	unsigned int	data_bus_width:10; /* In number of bits */

@@ -650,19 +646,6 @@ static inline bool _manager_ns(struct pl330_thread *thrd)
 	return (pl330->pinfo->pcfg.mode & DMAC_MODE_NS) ? true : false;
 }
 
-static inline u32 get_id(struct pl330_info *pi, u32 off)
-{
-	void __iomem *regs = pi->base;
-	u32 id = 0;
-
-	id |= (readb(regs + off + 0x0) << 0);
-	id |= (readb(regs + off + 0x4) << 8);
-	id |= (readb(regs + off + 0x8) << 16);
-	id |= (readb(regs + off + 0xc) << 24);
-
-	return id;
-}
-
 static inline u32 get_revision(u32 periph_id)
 {
 	return (periph_id >> PERIPH_REV_SHIFT) & PERIPH_REV_MASK;

@@ -1986,9 +1969,6 @@ static void read_dmac_config(struct pl330_info *pi)
 	pi->pcfg.num_events = val;
 
 	pi->pcfg.irq_ns = readl(regs + CR3);
-
-	pi->pcfg.periph_id = get_id(pi, PERIPH_ID);
-	pi->pcfg.pcell_id = get_id(pi, PCELL_ID);
 }
 
 static inline void _reset_thread(struct pl330_thread *thrd)

@@ -2098,10 +2078,8 @@ static int pl330_add(struct pl330_info *pi)
 	regs = pi->base;
 
 	/* Check if we can handle this DMAC */
-	if ((get_id(pi, PERIPH_ID) & 0xfffff) != PERIPH_ID_VAL
-	   || get_id(pi, PCELL_ID) != PCELL_ID_VAL) {
-		dev_err(pi->dev, "PERIPH_ID 0x%x, PCELL_ID 0x%x !\n",
-			get_id(pi, PERIPH_ID), get_id(pi, PCELL_ID));
+	if ((pi->pcfg.periph_id & 0xfffff) != PERIPH_ID_VAL) {
+		dev_err(pi->dev, "PERIPH_ID 0x%x !\n", pi->pcfg.periph_id);
 		return -EINVAL;
 	}
 

@@ -2922,6 +2900,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	if (ret)
 		return ret;
 
+	pi->pcfg.periph_id = adev->periphid;
 	ret = pl330_add(pi);
 	if (ret)
 		goto probe_err1;

Signature · 7 lines

1.8.2.2


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

The PL330 driver probes the peripheral and primecell IDs of the device to make sure that it is indeed an AMBA PL330. However, it does this by making byte accesses to a device mapping of the word-aligned ID registers, which is either UNPREDICTABLE or generates an alignment fault (depending on the presence of the virtualisation extensions).

Rather than fix this code, we can actually rip most of it out and let the AMBA bus driver correctly do the probing for us.

Cc Jassi Brar <[email protected]>

Cc Vinod Koul <[email protected]>

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

---
 drivers/dma/pl330.c | 27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

drivers/dma/pl330.c +3 -24

index 24e0754..22e2a8f 100644
@@ -157,7 +157,6 @@ enum pl330_reqtype {
 #define PERIPH_REV_R0P0		0
 #define PERIPH_REV_R1P0		1
 #define PERIPH_REV_R1P1		2
-#define PCELL_ID		0xff0
 
 #define CR0_PERIPH_REQ_SET	(1 << 0)
 #define CR0_BOOT_EN_SET		(1 << 1)

@@ -193,8 +192,6 @@ enum pl330_reqtype {
 #define INTEG_CFG		0x0
 #define PERIPH_ID_VAL		((PART << 0) | (DESIGNER << 12))
 
-#define PCELL_ID_VAL		0xb105f00d
-
 #define PL330_STATE_STOPPED		(1 << 0)
 #define PL330_STATE_EXECUTING		(1 << 1)
 #define PL330_STATE_WFE			(1 << 2)

@@ -292,7 +289,6 @@ static unsigned cmd_line;
 /* Populated by the PL330 core driver for DMA API driver's info */
 struct pl330_config {
 	u32	periph_id;
-	u32	pcell_id;
 #define DMAC_MODE_NS	(1 << 0)
 	unsigned int	mode;
 	unsigned int	data_bus_width:10; /* In number of bits */

@@ -650,19 +646,6 @@ static inline bool _manager_ns(struct pl330_thread *thrd)
 	return (pl330->pinfo->pcfg.mode & DMAC_MODE_NS) ? true : false;
 }
 
-static inline u32 get_id(struct pl330_info *pi, u32 off)
-{
-	void __iomem *regs = pi->base;
-	u32 id = 0;
-
-	id |= (readb(regs + off + 0x0) << 0);
-	id |= (readb(regs + off + 0x4) << 8);
-	id |= (readb(regs + off + 0x8) << 16);
-	id |= (readb(regs + off + 0xc) << 24);
-
-	return id;
-}
-
 static inline u32 get_revision(u32 periph_id)
 {
 	return (periph_id >> PERIPH_REV_SHIFT) & PERIPH_REV_MASK;

@@ -1986,9 +1969,6 @@ static void read_dmac_config(struct pl330_info *pi)
 	pi->pcfg.num_events = val;
 
 	pi->pcfg.irq_ns = readl(regs + CR3);
-
-	pi->pcfg.periph_id = get_id(pi, PERIPH_ID);
-	pi->pcfg.pcell_id = get_id(pi, PCELL_ID);
 }
 
 static inline void _reset_thread(struct pl330_thread *thrd)

@@ -2098,10 +2078,8 @@ static int pl330_add(struct pl330_info *pi)
 	regs = pi->base;
 
 	/* Check if we can handle this DMAC */
-	if ((get_id(pi, PERIPH_ID) & 0xfffff) != PERIPH_ID_VAL
-	   || get_id(pi, PCELL_ID) != PCELL_ID_VAL) {
-		dev_err(pi->dev, "PERIPH_ID 0x%x, PCELL_ID 0x%x !\n",
-			get_id(pi, PERIPH_ID), get_id(pi, PCELL_ID));
+	if ((pi->pcfg.periph_id & 0xfffff) != PERIPH_ID_VAL) {
+		dev_err(pi->dev, "PERIPH_ID 0x%x !\n", pi->pcfg.periph_id);
 		return -EINVAL;
 	}
 

@@ -2922,6 +2900,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	if (ret)
 		return ret;
 
+	pi->pcfg.periph_id = adev->periphid;
 	ret = pl330_add(pi);
 	if (ret)
 		goto probe_err1;

Signature · 1 line

1.8.2.2

The microcode bus address (pl330_dmac.mcode_bus) is currently a u32, which fails to compile when building on a system with 64-bit bus addresses.

This patch uses dma_addr_t to represent the address instead.

Acked-by Jassi Brar <[email protected]>

Cc Jassi Brar <[email protected]>

Cc Vinod Koul <[email protected]>

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

---
 drivers/dma/pl330.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dma/pl330.c +1 -2

index 22e2a8f..f1bc593 100644
@@ -501,7 +501,7 @@ struct pl330_dmac {
        /* Maximum possible events/irqs */
        int                     events[32];
        /* BUS address of MicroCode buffer */
-       u32                     mcode_bus;
+       dma_addr_t              mcode_bus;
        /* CPU address of MicroCode buffer */
        void                    *mcode_cpu;
        /* List of all Channel threads */
--

1.8.2.2

Quoted 4 lines · Will Deacon

The microcode bus address (pl330_dmac.mcode_bus) is currently a u32, which fails to compile when building on a system with 64-bit bus addresses.

Quoted 8 lines

This patch uses dma_addr_t to represent the address instead.

Acked-by Jassi Brar <[email protected]>

Cc Jassi Brar <[email protected]>

Cc Vinod Koul <[email protected]>

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

Quoted 21 lines

---
 drivers/dma/pl330.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dma/pl330.c +1 -2

index 22e2a8f..f1bc593 100644
@@ -501,7 +501,7 @@ struct pl330_dmac {
        /* Maximum possible events/irqs */
        int                     events[32];
        /* BUS address of MicroCode buffer */
-       u32                     mcode_bus;
+       dma_addr_t              mcode_bus;
        /* CPU address of MicroCode buffer */
        void                    *mcode_cpu;
        /* List of all Channel threads */
--

1.8.2.2

Signature · 3 lines

Linaro.org ? Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

The microcode bus address (pl330_dmac.mcode_bus) is currently a u32, which fails to compile when building on a system with 64-bit bus addresses.

This patch uses dma_addr_t to represent the address instead.

The microcode bus address (pl330_dmac.mcode_bus) is currently a u32, which fails to compile when building on a system with 64-bit bus addresses.

This patch uses dma_addr_t to represent the address instead.

Cc Jassi Brar <[email protected]>

Cc Vinod Koul <[email protected]>

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

---
 drivers/dma/pl330.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dma/pl330.c +1 -1

index 22e2a8f..f1bc593 100644
@@ -501,7 +501,7 @@ struct pl330_dmac {
 	/* Maximum possible events/irqs */
 	int			events[32];
 	/* BUS address of MicroCode buffer */
-	u32			mcode_bus;
+	dma_addr_t		mcode_bus;
 	/* CPU address of MicroCode buffer */
 	void			*mcode_cpu;
 	/* List of all Channel threads */

Signature · 6 lines

1.8.2.2

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

IOMMU mappings take a prot parameter, identifying the protection bits to enforce on the newly created mapping (READ or WRITE). The ARM dma-mapping framework currently just passes 0 as the prot argument, resulting in faulting mappings.

This patch infers the protection attributes based on the direction of the DMA transfer.

Cc Marek Szyprowski <[email protected]>

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

---
  arch/arm/mm/dma-mapping.c | 18 ++++++++++++++++--
  1 file changed, 16 insertions(+), 2 deletions(-)

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

index 6fb80cf..d119de7 100644
@@ -1636,13 +1636,27 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
  {
  	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
  	dma_addr_t dma_addr;
-	int ret, len = PAGE_ALIGN(size + offset);
+	int ret, prot, len = PAGE_ALIGN(size + offset);
 
  	dma_addr = __alloc_iova(mapping, len);
  	if (dma_addr == DMA_ERROR_CODE)
  		return dma_addr;
 
-	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0);
+	switch (dir) {
+	case DMA_BIDIRECTIONAL:
+		prot = IOMMU_READ | IOMMU_WRITE;
+		break;
+	case DMA_TO_DEVICE:
+		prot = IOMMU_READ;
+		break;
+	case DMA_FROM_DEVICE:

Hello,

Quoted 8 lines

IOMMU mappings take a prot parameter, identifying the protection bits to enforce on the newly created mapping (READ or WRITE). The ARM dma-mapping framework currently just passes 0 as the prot argument, resulting in faulting mappings.

This patch infers the protection attributes based on the direction of the DMA transfer.

Quoted 2 lines

Cc Marek Szyprowski <[email protected]>

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

Thanks for fixing this issue. Could I take this patch to my dma-mapping tree with other dma-mapping changes I've collected recently or do you want my ack and push it via other tree?

Quoted 1 line · Marek Szyprowski

Hello,

Hi Marek,

Quoted 12 lines · Will Deacon

Quoted 10 lines

IOMMU mappings take a prot parameter, identifying the protection bits to enforce on the newly created mapping (READ or WRITE). The ARM dma-mapping framework currently just passes 0 as the prot argument, resulting in faulting mappings.

This patch infers the protection attributes based on the direction of the DMA transfer.

Cc Marek Szyprowski <[email protected]>

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

Quoted 3 lines

Thanks for fixing this issue. Could I take this patch to my dma-mapping tree with other dma-mapping changes I've collected recently or do you want my ack and push it via other tree?

Please, feel free to take it via the dma-mapping tree! That's probably the best route for it and there's nothing that really depends on it anyway.

arch/arm/mm/dma-mapping.c +6 -2

@@ -1636,13 +1636,27 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
 {
 	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
 	dma_addr_t dma_addr;
-	int ret, len = PAGE_ALIGN(size + offset);
+	int ret, prot, len = PAGE_ALIGN(size + offset);
 
 	dma_addr = __alloc_iova(mapping, len);
 	if (dma_addr == DMA_ERROR_CODE)
 		return dma_addr;
 
-	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0);
+	switch (dir) {
+	case DMA_BIDIRECTIONAL:
+		prot = IOMMU_READ | IOMMU_WRITE;
+		break;
+	case DMA_TO_DEVICE:

Hi Will,

On Mon, 10 Jun 2013 20:34:39 +0200
Will Deacon <[email protected]> wrote:
...

Quoted 21 lines

arch/arm/mm/dma-mapping.c +10 -2

@@ -1636,13 +1636,27 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
 {
 	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
 	dma_addr_t dma_addr;
-	int ret, len = PAGE_ALIGN(size + offset);
+	int ret, prot, len = PAGE_ALIGN(size + offset);
 
 	dma_addr = __alloc_iova(mapping, len);
 	if (dma_addr == DMA_ERROR_CODE)
 		return dma_addr;
 
-	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, 0);
+	switch (dir) {
+	case DMA_BIDIRECTIONAL:
+		prot = IOMMU_READ | IOMMU_WRITE;
+		break;
+	case DMA_TO_DEVICE:
+		prot = IOMMU_READ;
+		break;
+	case DMA_FROM_DEVICE:
+		prot = IOMMU_WRITE;

arch/arm/mm/dma-mapping.c static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p · line 1659

Quoted 6 lines

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

+		break;
+	default:
+		prot = 0;
+	}
+
+	ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot);

Do we need similar changes for map_sg case as well? They still passes '0' as prot.

Quoted 1 line

Do we need similar changes for map_sg case as well? They still passes '0' as prot.

Yes, we could use the same trick there (probably worth moving the logic into a helper function for translating dma_data_direction into IOMMU_* values).

There are also iommu_map calls when allocating DMA buffers, but I think 0 is the right thing to pass there (i.e. no permission until pages have been explicitly mapped). Although, to be honest, I don't see why we need to map the buffer at all when we allocate it.

From 699e6bd4fef86383d197775486b47bcbdc594f4a Mon Sep 17 00:00:00 2001

Will Deacon <[email protected]> wrote @ Tue, 25 Jun 2013 13:37:14 +0200:
...

Quoted 5 lines

Quoted 1 line

Do we need similar changes for map_sg case as well? They still passes '0' as prot.

Yes, we could use the same trick there (probably worth moving the logic into a helper function for translating dma_data_direction into IOMMU_* values).

Quoted 4 lines

There are also iommu_map calls when allocating DMA buffers, but I think 0 is the right thing to pass there (i.e. no permission until pages have been explicitly mapped). Although, to be honest, I don't see why we need to map the buffer at all when we allocate it.

Yes, I thought too. I have a patch for that as below. If you like, I'll rebase and send for merge with the one which changes dma-mapping.c.

The current code only clobbers a local variable, so the device is left with a stale mapping pointer.

Hi Will,

Will Deacon <[email protected]> wrote @ Mon, 10 Jun 2013 20:34:40 +0200:

Quoted 2 lines

The current code only clobbers a local variable, so the device is left with a stale mapping pointer.

True. This's my bad. Thanks.

Quoted 8 lines · Hiroshi Doyu

Hi Will,

Will Deacon <[email protected]> wrote @ Mon, 10 Jun 2013 20:34:40 +0200:

Quoted 2 lines

The current code only clobbers a local variable, so the device is left with a stale mapping pointer.

True. This's my bad. Thanks.

That's alright, it's easy to fix. Mind if I add your ack please?

Quoted 5 lines · On Tue, 11 Jun 2013 10:50:15 +0200 Will Deacon <[email protected]> wrote:

Quoted 4 lines · Hiroshi Doyu

Hi Will,

Will Deacon <[email protected]> wrote @ Mon, 10 Jun 2013 20:34:40 +0200:

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

The current code only clobbers a local variable, so the device is left with a stale mapping pointer.

True. This's my bad. Thanks.

That's alright, it's easy to fix. Mind if I add your ack please?

Feel free to add: Acked-by: Hiroshi Doyu <[email protected]>

This patch adds a description of the device tree binding for the ARM System MMU architecture.

Cc Rob Herring <[email protected]>

Cc Andreas Herrmann <[email protected]>

Cc Joerg Roedel <[email protected]>

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

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

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +70 -0

new file mode 100644
index 0000000..e34c6cd
@@ -0,0 +1,70 @@
+* ARM System MMU Architecture Implementation
+
+ARM SoCs may contain an implementation of the ARM System Memory
+Management Unit Architecture, which can be used to provide 1 or 2 stages
+of address translation to bus masters external to the CPU.
+
+The SMMU may also raise interrupts in response to various fault
+conditions.
+
+** System MMU required properties:
+
+- compatible    : Should be one of:
+
+                        "arm,smmu-v1"
+                        "arm,smmu-v2"
+                        "arm,mmu-400"
+                        "arm,mmu-500"
+
+                  depending on the particular implementation and/or the
+                  version of the architecture implemented.
+
+- reg           : Base address and size of the SMMU.
+
+- #global-interrupts : The number of global interrupts exposed by the
+                       device.
+
+- interrupts    : Interrupt list, with the first #global-irqs entries
+                  corresponding to the global interrupts and any
+                  following entries corresponding to context interrupts,
+                  specified in order of their indexing by the SMMU.
+
+                  For SMMUv2 implementations, there must be exactly one
+                  interrupt per context bank. In the case of a single,
+                  combined interrupt, it must be listed multiple times.
+
+- mmu-masters   : A list of phandles to device nodes representing bus
+                  masters for which the SMMU can provide a translation
+                  and their corresponding StreamIDs (see example below).
+                  Each device node linked from this list must have a
+                  "#stream-id-cells" property, indicating the number of
+                  StreamIDs associated with it.
+
+** System MMU optional properties:
+
+- smmu-parent   : When multiple SMMUs are chained together, this
+                  property can be used to provide a phandle to the
+                  parent SMMU (that is the next SMMU on the path going
+                  from the mmu-masters towards memory) node for this
+                  SMMU.
+
+Example:
+
+        smmu {
+                compatible = "arm,smmu-v1";
+                reg = <0xba5e0000 0x10000>;
+                #global-interrupts = <2>;
+                interrupts = <0 32 4>,
+                             <0 33 4>,
+                             <0 34 4>, /* This is the first context interrupt */
+                             <0 35 4>,
+                             <0 36 4>,
+                             <0 37 4>;
+
+                /*
+                 * Two DMA controllers, the first with two StreamIDs (0xd01d
+                 * and 0xd01e) and the second with only one (0xd11c).
+                 */
+                mmu-masters = <&dma0 0xd01d 0xd01e>,
+                              <&dma1 0xd11c>;
+        };

Signature · 6 lines

1.8.2.2

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

This patch adds a description of the device tree binding for the ARM System MMU architecture.

Quoted 2 lines · Will Deacon

This patch adds a description of the device tree binding for the ARM System MMU architecture.

Interesting, will this be a common driver to replace existing ARM IOMMU drivers or is it a common driver for ARM IOMMUs found in future chips?

Documentation/devicetree/bindings/iommu/arm,smmu.txt line 49

Quoted 5 lines

+- smmu-parent : When multiple SMMUs are chained together, this

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +4 -0

+                  property can be used to provide a phandle to the
+                  parent SMMU (that is the next SMMU on the path going
+                  from the mmu-masters towards memory) node for this
+                  SMMU.

What happens when SMMUs are chained? Will the second SMMU seeing the DMA just pass it through or is it translated again?

This is a common driver that will support any IOMMUs compatible with v1 or v2 of the ARM SMMU architecture. Currently, that includes SMMUs known informatively as MMU-400, MMU-401 and MMU-500. I had a look at the other IOMMU drivers in the kernel and they seem to be driving incompatible IOMMUs, so I don't see how this driver can replace those.

However, we'll hopefully see ARM SMMU-compatible devices turning up soon (I know of many SoCs in development that are looking at them).

From NVidia is another driver with 'smmu' in its name, is that one also

Chaining is really horrible and exists as a hack to support virtualisation using two separate SMMUs, where neither of them can support nested translation.

Quoted 6 lines · Will Deacon

This is a common driver that will support any IOMMUs compatible with v1 or v2 of the ARM SMMU architecture. Currently, that includes SMMUs known informatively as MMU-400, MMU-401 and MMU-500. I had a look at the other IOMMU drivers in the kernel and they seem to be driving incompatible IOMMUs, so I don't see how this driver can replace those.

Quoted 2 lines

However, we'll hopefully see ARM SMMU-compatible devices turning up soon (I know of many SoCs in development that are looking at them).

That sounds great. So in the future there will be a single driver for most future SOCs. This is a real improvement over the current situation.

From NVidia is another driver with 'smmu' in its name, is that one also incompatible?

This patch adds a description of the device tree binding for the ARM System MMU architecture.

Cc Rob Herring <[email protected]>

Cc Andreas Herrmann <[email protected]>

Cc Joerg Roedel <[email protected]>

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

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

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +18 -0

new file mode 100644
index 0000000..e34c6cd
@@ -0,0 +1,70 @@
+* ARM System MMU Architecture Implementation
+
+ARM SoCs may contain an implementation of the ARM System Memory
+Management Unit Architecture, which can be used to provide 1 or 2 stages
+of address translation to bus masters external to the CPU.
+
+The SMMU may also raise interrupts in response to various fault
+conditions.
+
+** System MMU required properties:
+
+- compatible    : Should be one of:
+
+                        "arm,smmu-v1"
+                        "arm,smmu-v2"
+                        "arm,mmu-400"
+                        "arm,mmu-500"
+

and any

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +3 -0

+                  following entries corresponding to context interrupts,
+                  specified in order of their indexing by the SMMU.
+

Quoted 24 lines · Will Deacon

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +18 -0

new file mode 100644
index 0000000..e34c6cd
@@ -0,0 +1,70 @@
+* ARM System MMU Architecture Implementation
+
+ARM SoCs may contain an implementation of the ARM System Memory
+Management Unit Architecture, which can be used to provide 1 or 2 stages
+of address translation to bus masters external to the CPU.
+
+The SMMU may also raise interrupts in response to various fault
+conditions.
+
+** System MMU required properties:
+
+- compatible    : Should be one of:
+
+                        "arm,smmu-v1"
+                        "arm,smmu-v2"
+                        "arm,mmu-400"
+                        "arm,mmu-500"
+

Documentation/devicetree/bindings/iommu/arm,smmu.txt line 28

Quoted 10 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +10 -0

+                  depending on the particular implementation and/or the
+                  version of the architecture implemented.
+
+- reg           : Base address and size of the SMMU.
+
+- #global-interrupts : The number of global interrupts exposed by the
+                       device.
+
+- interrupts    : Interrupt list, with the first #global-irqs entries
+                  corresponding to the global interrupts

It seems like we don't have enough information here. It's not enough
for the OS to know that there are 2, 4, etc global interrupts, no? It needs
to know which hardware interrupt each corresponds to. That kind of
stuff is normally defined in the device binding.

What is it that determines the number of global interrupts?

Documentation/devicetree/bindings/iommu/arm,smmu.txt line 41

Quoted 6 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +6 -0

+- mmu-masters   : A list of phandles to device nodes representing bus
+                  masters for which the SMMU can provide a translation
+                  and their corresponding StreamIDs (see example below).
+                  Each device node linked from this list must have a
+                  "#stream-id-cells" property, indicating the number of
+                  StreamIDs associated with it.

So to find a the SMMU for a given device, I walk up the bus hierarchy until I find an SMMU?

Documentation/devicetree/bindings/iommu/arm,smmu.txt line 49

Quoted 7 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +7 -0

+** System MMU optional properties:
+
+- smmu-parent   : When multiple SMMUs are chained together, this
+                  property can be used to provide a phandle to the
+                  parent SMMU (that is the next SMMU on the path going
+                  from the mmu-masters towards memory) node for this
+                  SMMU.

Why is an explicit phandle link needed here when you don't need a
smmu-parent phandle in each mmu-master? Won't walking the bus
hierarchy identify the parent SMMU if things are chained?

Hi Stuart,

Quoted 41 lines · Stuart Yoder

Quoted 24 lines · Will Deacon

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +18 -0

new file mode 100644
index 0000000..e34c6cd
@@ -0,0 +1,70 @@
+* ARM System MMU Architecture Implementation
+
+ARM SoCs may contain an implementation of the ARM System Memory
+Management Unit Architecture, which can be used to provide 1 or 2 stages
+of address translation to bus masters external to the CPU.
+
+The SMMU may also raise interrupts in response to various fault
+conditions.
+
+** System MMU required properties:
+
+- compatible    : Should be one of:
+
+                        "arm,smmu-v1"
+                        "arm,smmu-v2"
+                        "arm,mmu-400"
+                        "arm,mmu-500"
+

Documentation/devicetree/bindings/iommu/arm,smmu.txt line 28

Quoted 10 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +10 -0

+                  depending on the particular implementation and/or the
+                  version of the architecture implemented.
+
+- reg           : Base address and size of the SMMU.
+
+- #global-interrupts : The number of global interrupts exposed by the
+                       device.
+
+- interrupts    : Interrupt list, with the first #global-irqs entries
+                  corresponding to the global interrupts

It seems like we don't have enough information here. It's not enough
for the OS to know that there are 2, 4, etc global interrupts, no? It needs
to know which hardware interrupt each corresponds to. That kind of
stuff is normally defined in the device binding.

Quoted 1 line

What is it that determines the number of global interrupts?

I'd suggest looking at the driver I posted to get a gist of how the parsing code works, but suffice to say that we describe both the number of interrupts and the actual interrupt numbers here.

Quoted 7 lines

Quoted 6 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +6 -0

+- mmu-masters   : A list of phandles to device nodes representing bus
+                  masters for which the SMMU can provide a translation
+                  and their corresponding StreamIDs (see example below).
+                  Each device node linked from this list must have a
+                  "#stream-id-cells" property, indicating the number of
+                  StreamIDs associated with it.

Quoted 2 lines

So to find a the SMMU for a given device, I walk up the bus hierarchy until I find an SMMU?

Again, the code is better than any explanation I can give here, but we basically construct a tree of masters for each SMMU in the system based on the mmu-masters property, which we can later search.

Quoted 11 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt line 49

Quoted 7 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt new file +7 -0

+** System MMU optional properties:
+
+- smmu-parent   : When multiple SMMUs are chained together, this
+                  property can be used to provide a phandle to the
+                  parent SMMU (that is the next SMMU on the path going
+                  from the mmu-masters towards memory) node for this
+                  SMMU.

Why is an explicit phandle link needed here when you don't need a
smmu-parent phandle in each mmu-master? Won't walking the bus
hierarchy identify the parent SMMU if things are chained?

What bus hierarchy? If I have two SMMU device nodes, how do I infer any topological information without an explicit linkage?

Quoted 3 lines · Will Deacon

I'd suggest looking at the driver I posted to get a gist of how the parsing code works, but suffice to say that we describe both the number of interrupts and the actual interrupt numbers here.

I understand that the number of interrupts and actual interrupt numbers are described here, but was referring to the _meaning_ of the interrupt
numbers. A binding for a device with 2 interrupts, a TX and RX would
normally identify which interrupt specifier is for TX and which is for RX.

Based on your code, the 2 global interrupts seem to be the secure and non-secure fault interrupts...which your driver does not differentiate. However, the device tree is describing hardware and you can't assume that all drivers don't care which is which.

If there are other interrupts added in the future like the performance interrupt the definition could be expanded to add that.

But a question.... why do you need the #global-interrupts property
at all? Is the number of "global" interrupts really implementation dependent?
Does't a specific compatible string imply the number of interrupts that there actually are?

Quoted 2 lines

What bus hierarchy? If I have two SMMU device nodes, how do I infer any topological information without an explicit linkage?

I really don't know anything about SMMU chaining, but am inferring what
this might look like. Does the "child" SMMU look just like another
mmu-master to the parent? If so, why not just use the mmu-masters
property to establish the relationship.

Quoted 10 lines · Stuart Yoder

Quoted 3 lines · Will Deacon

I'd suggest looking at the driver I posted to get a gist of how the parsing code works, but suffice to say that we describe both the number of interrupts and the actual interrupt numbers here.

I understand that the number of interrupts and actual interrupt numbers are described here, but was referring to the _meaning_ of the interrupt
numbers. A binding for a device with 2 interrupts, a TX and RX would
normally identify which interrupt specifier is for TX and which is for RX.

Quoted 4 lines

Based on your code, the 2 global interrupts seem to be the secure and non-secure fault interrupts...which your driver does not differentiate. However, the device tree is describing hardware and you can't assume that all drivers don't care which is which.

Currently, the driver only works when Linux is running as non-secure, which is becoming more and more common since it is required to be able to make use of hyp mode.

There are actually two global interrupts for SMMUv1 and SMMUv2, which correspond to configuration faults and `other' global faults.

Quoted 20 lines · Will Deacon

Quoted 10 lines · Stuart Yoder

Quoted 3 lines · Will Deacon

I'd suggest looking at the driver I posted to get a gist of how the parsing code works, but suffice to say that we describe both the number of interrupts and the actual interrupt numbers here.

I understand that the number of interrupts and actual interrupt numbers are described here, but was referring to the _meaning_ of the interrupt
numbers. A binding for a device with 2 interrupts, a TX and RX would
normally identify which interrupt specifier is for TX and which is for RX.

Quoted 4 lines

Based on your code, the 2 global interrupts seem to be the secure and non-secure fault interrupts...which your driver does not differentiate. However, the device tree is describing hardware and you can't assume that all drivers don't care which is which.

Currently, the driver only works when Linux is running as non-secure, which is becoming more and more common since it is required to be able to make use of hyp mode.

Quoted 2 lines

There are actually two global interrupts for SMMUv1 and SMMUv2, which correspond to configuration faults and `other' global faults.

So, why don't we define which interrupt is which in this binding? ...e.g. "The first
interrupt specifier is for the configuration access fault interrupt, the second interrupt specifier is for other global faults."

How else is software supposed to know which interrupt corresponds to what event.

Quoted 24 lines · Stuart Yoder

Quoted 22 lines · Will Deacon

Quoted 10 lines · Stuart Yoder

Quoted 3 lines · Will Deacon

I'd suggest looking at the driver I posted to get a gist of how the parsing code works, but suffice to say that we describe both the number of interrupts and the actual interrupt numbers here.

I understand that the number of interrupts and actual interrupt numbers are described here, but was referring to the _meaning_ of the interrupt
numbers. A binding for a device with 2 interrupts, a TX and RX would
normally identify which interrupt specifier is for TX and which is for RX.

Quoted 4 lines

Based on your code, the 2 global interrupts seem to be the secure and non-secure fault interrupts...which your driver does not differentiate. However, the device tree is describing hardware and you can't assume that all drivers don't care which is which.

Currently, the driver only works when Linux is running as non-secure, which is becoming more and more common since it is required to be able to make use of hyp mode.

There are actually two global interrupts for SMMUv1 and SMMUv2, which correspond to configuration faults and `other' global faults.

Quoted 4 lines

So, why don't we define which interrupt is which in this binding? ...e.g. "The first
interrupt specifier is for the configuration access fault interrupt, the second interrupt specifier is for other global faults."

Well, first of all, they're both global interrupts and the handler will have to go and access exactly the same registers to deal with the fault, so you don't really gain anything.

However, the main reason is that you need to be able to handle:

(1) Only one of the interrupts is routed to the interrupt controller
(2) The interrupts are ORd into a single line

This patch adds support for SMMUs implementing the ARM System MMU architecture versions 1 or 2. Both arm and arm64 are supported, although the v7s descriptor format is not used.

Cc Rob Herring <[email protected]>

Cc Andreas Herrmann <[email protected]>

Cc Olav Haugan <[email protected]>

Cc Joerg Roedel <[email protected]>

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

drivers/iommu/arm-smmu.c new file +5 -0

+
+	dev_warn(smmu->dev,
+		 "Failed to find SMMU parent despite parent in DT\n");
+	return NULL;
+}

drivers/iommu/arm-smmu.c new file +8 -0

+}
 
+static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
+			phys_addr_t paddr, size_t size, int flags)
+{
+	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_device *smmu = smmu_domain->leaf_smmu;
+
+	if (!smmu_domain || !smmu)

drivers/iommu/arm-smmu.c new file +4 -0

+
+	return arm_smmu_create_mapping(smmu_domain, iova, paddr, size, flags);
+}
+

drivers/iommu/arm-smmu.c new file +31 -0

+		smmu = parent;
+
+		/* Try to find a child of the current SMMU. */
+		list_for_each_entry_safe(child, tmp[1], &arm_smmu_devices, list) {
+			if (child->parent_of_node == parent->dev->of_node) {
+				/* Does the child sit above our master? */
+				master = find_smmu_master(child, dev->of_node);
+				if (master) {
+					smmu = NULL;
+					break;
+				}
+			}
+		}
+
+		/* We found some children, so keep searching. */
+		if (!smmu) {
+			master = NULL;
+			continue;
+		}
+
+		master = find_smmu_master(smmu, dev->of_node);
+		if (master)
+			break;
+	}
+
+	if (!master)
+		return -ENODEV;
+
+	dev->archdata.iommu = smmu;
+	return 0;
+}

Hi Will,

Quoted 9 lines

This patch adds support for SMMUs implementing the ARM System MMU architecture versions 1 or 2. Both arm and arm64 are supported, although the v7s descriptor format is not used.

Cc Rob Herring <[email protected]>

Cc Andreas Herrmann <[email protected]>

Cc Olav Haugan <[email protected]>

Cc Joerg Roedel <[email protected]>

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

A few general questions:

How have you tested this code? Has it been run on real hardware? What were the results?

The code looks good and clean in general, minus a few places mentioned below were I have questions and/or suggestions:

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+static struct arm_smmu_device *find_parent_smmu(struct arm_smmu_device *smmu)
+{
+	struct arm_smmu_device *parent, *tmp;
+
+	if (!smmu->parent_of_node)

drivers/iommu/arm-smmu.c line 485

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+		return NULL;
+
+	list_for_each_entry_safe(parent, tmp, &arm_smmu_devices, list)
+		if (parent->dev->of_node == smmu->parent_of_node)
+			return parent;

Why do you need the _safe variant here? You are not changing the list in this loop so you should be fine with list_for_each_entry().

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+
+	dev_warn(smmu->dev,
+		 "Failed to find SMMU parent despite parent in DT\n");
+	return NULL;
+}

drivers/iommu/arm-smmu.c line 539

Quoted 9 lines

drivers/iommu/arm-smmu.c new file +9 -0

+/* Wait for any pending TLB invalidations to complete */
+static void arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
+{
+	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+
+	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_sTLBGSYNC);
+	while (readl_relaxed(gr0_base + ARM_SMMU_GR0_sTLBGSTATUS)
+	       & sTLBGSTATUS_GSACTIVE)
+		cpu_relax();

Other IOMMU drivers have a timeout for this loop and report an error when the state does not change. I think this makes sense here too so that the kernel will not just stop spinning in that loop if something goes wrong but prints an error instead.

Quoted 9 lines

drivers/iommu/arm-smmu.c new file +9 -0

+}
+static void arm_smmu_flush_pgtable(struct arm_smmu_device *smmu, void *addr,
+				   size_t size)
+{
+	unsigned long offset = (unsigned long)addr & ~PAGE_MASK;
+
+	/*
+	 * If the SMMU can't walk tables in the CPU caches, treat them
+	 * like non-coherent DMA...

drivers/iommu/arm-smmu.c line 1162

Quoted 4 lines

drivers/iommu/arm-smmu.c new file +4 -0

+	 */
+	if (!(smmu->features & ARM_SMMU_FEAT_COHERENT_WALK))
+		dma_map_page(smmu->dev, virt_to_page(addr), offset, size,
+			     DMA_TO_DEVICE);

Why can you call into DMA-API here? A DMA-API implementation may call back into this IOMMU driver, no? So this looks a little bit like a layering violation.

Quoted 16 lines

drivers/iommu/arm-smmu.c new file +15 -0

+}
 
+static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
+			phys_addr_t paddr, size_t size, int flags)
+{
+	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_device *smmu = smmu_domain->leaf_smmu;
+
+	if (!smmu_domain || !smmu)
+		return -ENODEV;
+
+	/*
+	 * Check for silent address truncation up the SMMU chain.
+	 */
+	do {
+		phys_addr_t output_mask = (1ULL << smmu->s2_output_size) - 1;

drivers/iommu/arm-smmu.c line 1404

Quoted 3 lines

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

+		if ((phys_addr_t)iova & ~output_mask)
+			return -ERANGE;
+	} while ((smmu = find_parent_smmu(smmu)));

This looks a bit too expensive to have in the map path. How about saving something like an effective_output_mask (or output_size) which contains the logical OR of every mask up the path? This would make this check a lot cheaper.

Quoted 9 lines

drivers/iommu/arm-smmu.c new file +9 -0

+
+	return arm_smmu_create_mapping(smmu_domain, iova, paddr, size, flags);
+}
+
+static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
+			     size_t size)
+{
+	int ret;
+	struct arm_smmu_domain *smmu_domain = domain->priv;

drivers/iommu/arm-smmu.c line 1418

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+	struct arm_smmu_cfg *root_cfg = &smmu_domain->root_cfg;
+	struct arm_smmu_device *smmu = root_cfg->smmu;
+	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+
+	ret = arm_smmu_create_mapping(smmu_domain, iova, 0, size, 0);

Since this function does also unmapping, how about renaming it to arm_smmu_handle_mapping(). The 'create' part in there is misleading.

Quoted 6 lines

drivers/iommu/arm-smmu.c new file +6 -0

+	writel_relaxed(root_cfg->vmid, gr0_base + ARM_SMMU_GR0_TLBIVMID);
+	arm_smmu_tlb_sync(smmu);
+	return ret ? ret : size;
+}
+static int arm_smmu_add_device(struct device *dev)
+{

drivers/iommu/arm-smmu.c line 1485

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+	struct arm_smmu_device *child, *parent, *smmu;
+	struct arm_smmu_device *tmp[2];
+	struct arm_smmu_master *master = NULL;
+
+	list_for_each_entry_safe(parent, tmp[0], &arm_smmu_devices, list) {

Again, why do you use the _safe variant, you do not seem to change the lists traversed here.

The results were that the memory-to-memory DMA didn't show any corruption. I also managed to tickle access faults by messing around with the permissions, then remap the buffers and resume the transfers.

Quoted 5 lines

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+static struct arm_smmu_device *find_parent_smmu(struct arm_smmu_device *smmu)
+{
+	struct arm_smmu_device *parent, *tmp;
+
+	if (!smmu->parent_of_node)

Quoted 8 lines

drivers/iommu/arm-smmu.c line 485

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+		return NULL;
+
+	list_for_each_entry_safe(parent, tmp, &arm_smmu_devices, list)
+		if (parent->dev->of_node == smmu->parent_of_node)
+			return parent;

Why do you need the _safe variant here? You are not changing the list in this loop so you should be fine with list_for_each_entry().

For a system with multiple SMMUs (regardless of chaining), couldn't this code run in parallel with probing of another SMMU (which has to add to the arm_smmu_devices list)? The same applies for device removal, which could perhaps be driven from some power-managment code.

Quoted 10 lines

drivers/iommu/arm-smmu.c line 1404

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+	do {
+		phys_addr_t output_mask = (1ULL << smmu->s2_output_size) - 1;
+		if ((phys_addr_t)iova & ~output_mask)
+			return -ERANGE;
+	} while ((smmu = find_parent_smmu(smmu)));

This looks a bit too expensive to have in the map path. How about saving something like an effective_output_mask (or output_size) which contains the logical OR of every mask up the path? This would make this check a lot cheaper.

As mentioned in the DT binding thread, it's rare that this loop would execute more than once, and largely inconceivable that it would execute more than twice, so I don't know how much we need to worry about the cost.

Quoted 3 lines · Will Deacon

The results were that the memory-to-memory DMA didn't show any corruption. I also managed to tickle access faults by messing around with the permissions, then remap the buffers and resume the transfers.

That sounds pretty conclusive. So when real hardware shows up it should work reasonably well.

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+static struct arm_smmu_device *find_parent_smmu(struct arm_smmu_device *smmu)
+{
+	struct arm_smmu_device *parent, *tmp;
+
+	if (!smmu->parent_of_node)

Quoted 8 lines

drivers/iommu/arm-smmu.c line 485

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+		return NULL;
+
+	list_for_each_entry_safe(parent, tmp, &arm_smmu_devices, list)
+		if (parent->dev->of_node == smmu->parent_of_node)
+			return parent;

Why do you need the _safe variant here? You are not changing the list in this loop so you should be fine with list_for_each_entry().

Quoted 4 lines

For a system with multiple SMMUs (regardless of chaining), couldn't this code run in parallel with probing of another SMMU (which has to add to the arm_smmu_devices list)? The same applies for device removal, which could perhaps be driven from some power-managment code.

Well, the '_safe' does not mean it is safe from concurrent list manipulations. If you want to protect from that you still need a lock. The '_safe' variant only allows to remove the current element from the list while traversing it.

Quoted 11 lines

Quoted 10 lines

drivers/iommu/arm-smmu.c line 1404

Quoted 5 lines

drivers/iommu/arm-smmu.c new file +5 -0

+	do {
+		phys_addr_t output_mask = (1ULL << smmu->s2_output_size) - 1;
+		if ((phys_addr_t)iova & ~output_mask)
+			return -ERANGE;
+	} while ((smmu = find_parent_smmu(smmu)));

This looks a bit too expensive to have in the map path. How about saving something like an effective_output_mask (or output_size) which contains the logical OR of every mask up the path? This would make this check a lot cheaper.

Quoted 3 lines

As mentioned in the DT binding thread, it's rare that this loop would execute more than once, and largely inconceivable that it would execute more than twice, so I don't know how much we need to worry about the cost.

But still, this code is a challenge for the branch-predictor, plus the additional function calls to find_parent_smmu(). I still think it is worth to optimize this away. The map function is supposed to be a fast-path function.

Damn, I was hoping to avoid locking on the map path. In fact, this is a good argument to go with your suggestion below (otherwise I'd need to use reader/writer locks which seem to be frowned on).

Quoted 3 lines · Will Deacon

Damn, I was hoping to avoid locking on the map path. In fact, this is a good argument to go with your suggestion below (otherwise I'd need to use reader/writer locks which seem to be frowned on).

You should look into using rcu-lists instead. You still need a lock, but only when you actually manipulating the lists. For traversing them it is sufficient to take the rcu_read_lock() which has very low overhead.

Add myself as maintainer for the ARM system MMU driver.

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

---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

MAINTAINERS +6 -0

index 250dc97..84df4bd 100644
@@ -1310,6 +1310,12 @@ T:	git git://git.xilinx.com/linux-xlnx.git
 S:	Supported
 F:	arch/arm/mach-zynq/
 
+ARM SMMU DRIVER
+M:	Will Deacon <[email protected]>
+L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	drivers/iommu/arm-smmu.c
+
 ARM64 PORT (AARCH64 ARCHITECTURE)
 M:	Catalin Marinas <[email protected]>
 M:	Will Deacon <[email protected]>

Signature · 7 lines

1.8.2.2


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