arm: fix pmd flushing in map_init_section

PATCH New Mark Rutland 3 participants 8 messages Jun 14, 2013 Arm (32-bit)

Share & tools

Mark Rutland

author

2013-06-14 16:22 UTC #0 lore raw

PATCH 1/1 arm: fix pmd flushing in map_init_section

In e651eab0af: "ARM: 7677/1: LPAE: Fix mapping in alloc_init_section for unaligned addresses", the pmd flushing was broken when split out to map_init_section. At the end of the final iteration of the while loop, pmd will point at the pmd_t immediately after the pmds we updated, and thus flush_pmd_entry(pmd) won't flush the newly modified pmds. This has been observed to prevent an 11MPCore system from booting.

This patch fixes this by remembering the address of the first pmd we update and using this as the argument to flush_pmd_entry.

Signed-off-by Mark Rutland <[email protected]>

Cc R Sricharan <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Russell King <[email protected]>

Cc stable at vger.kernel.org

---
 arch/arm/mm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm/mm/mmu.c +2 -1

index e0d8565..22bc0ff 100644
@@ -620,6 +620,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p = pmd;
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to

@@ -638,7 +639,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);
 
-	flush_pmd_entry(pmd);
+	flush_pmd_entry(p);
 }
 
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,

Signature · 1 line

1.8.1.1

Christoffer Dall

2013-06-14 16:34 UTC #1 lore raw

PATCH 1/1 arm: fix pmd flushing in map_init_section

Quoted 34 lines · Mark Rutland

In e651eab0af: "ARM: 7677/1: LPAE: Fix mapping in alloc_init_section for unaligned addresses", the pmd flushing was broken when split out to map_init_section. At the end of the final iteration of the while loop, pmd will point at the pmd_t immediately after the pmds we updated, and thus flush_pmd_entry(pmd) won't flush the newly modified pmds. This has been observed to prevent an 11MPCore system from booting.
This patch fixes this by remembering the address of the first pmd we update and using this as the argument to flush_pmd_entry.
Signed-off-by Mark Rutland <[email protected]>
Cc R Sricharan <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Russell King <[email protected]>
Cc stable at vger.kernel.org
---
 arch/arm/mm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/mm/mmu.c +1 -0
index e0d8565..22bc0ff 100644
@@ -620,6 +620,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p = pmd;
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to

arch/arm/mm/mmu.c static void __init map_init_section(pmd_t *pmd, unsigned long addr, · line 645

Quoted 8 lines

arch/arm/mm/mmu.c +1 -1

@@ -638,7 +639,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);
 
-	flush_pmd_entry(pmd);
+	flush_pmd_entry(p);
 }
 
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,

Refresh my memory here again, why are we not flushing every pmd entry we update? Is it because we assume the cache lines cover the maximum span between addr and end?

Theoretically, shouldn't you also increment p in the non-LPAE case?

-Christoffer

Mark Rutland

author

2013-06-14 16:48 UTC #2 lore raw

PATCH 1/1 arm: fix pmd flushing in map_init_section

Quoted 41 lines · Christoffer Dall

Quoted 40 lines · Mark Rutland
In e651eab0af: "ARM: 7677/1: LPAE: Fix mapping in alloc_init_section for unaligned addresses", the pmd flushing was broken when split out to map_init_section. At the end of the final iteration of the while loop, pmd will point at the pmd_t immediately after the pmds we updated, and thus flush_pmd_entry(pmd) won't flush the newly modified pmds. This has been observed to prevent an 11MPCore system from booting.
This patch fixes this by remembering the address of the first pmd we update and using this as the argument to flush_pmd_entry.
Signed-off-by Mark Rutland <[email protected]>
Cc R Sricharan <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Russell King <[email protected]>
Cc stable at vger.kernel.org
---
 arch/arm/mm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/mm/mmu.c +2 -1
index e0d8565..22bc0ff 100644
@@ -620,6 +620,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p = pmd;
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to
@@ -638,7 +639,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);
 
-	flush_pmd_entry(pmd);
+	flush_pmd_entry(p);
 }
 

Quoted 6 lines

arch/arm/mm/mmu.c static void __init map_init_section(pmd_t *pmd, unsigned long addr, · line 645
Quoted 2 lines
arch/arm/mm/mmu.c +0 -0
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
Refresh my memory here again, why are we not flushing every pmd entry we update? Is it because we assume the cache lines cover the maximum span between addr and end?

Yup, we assume a minimum cache line size of 8 bytes. I'm not so keen on this, but I suspect others might not be happy with moving the flush into the loop.

Quoted 2 lines

Theoretically, shouldn't you also increment p in the non-LPAE case?

Yes, I should. v2 shortly...

Thanks,
Mark.

Christoffer Dall

2013-06-14 16:54 UTC #3 lore raw

PATCH 1/1 arm: fix pmd flushing in map_init_section

Quoted 49 lines · Mark Rutland

Quoted 41 lines · Christoffer Dall
Quoted 40 lines · Mark Rutland
In e651eab0af: "ARM: 7677/1: LPAE: Fix mapping in alloc_init_section for unaligned addresses", the pmd flushing was broken when split out to map_init_section. At the end of the final iteration of the while loop, pmd will point at the pmd_t immediately after the pmds we updated, and thus flush_pmd_entry(pmd) won't flush the newly modified pmds. This has been observed to prevent an 11MPCore system from booting.
This patch fixes this by remembering the address of the first pmd we update and using this as the argument to flush_pmd_entry.
Signed-off-by Mark Rutland <[email protected]>
Cc R Sricharan <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Russell King <[email protected]>
Cc stable at vger.kernel.org
---
 arch/arm/mm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/mm/mmu.c +2 -1
index e0d8565..22bc0ff 100644
@@ -620,6 +620,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p = pmd;
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to
@@ -638,7 +639,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);
 
-	flush_pmd_entry(pmd);
+	flush_pmd_entry(p);
 }
 
Quoted 6 lines
arch/arm/mm/mmu.c static void __init map_init_section(pmd_t *pmd, unsigned long addr, · line 645
Quoted 2 lines
arch/arm/mm/mmu.c +0 -0
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
Refresh my memory here again, why are we not flushing every pmd entry we update? Is it because we assume the cache lines cover the maximum span between addr and end?

Quoted 3 lines

Yup, we assume a minimum cache line size of 8 bytes. I'm not so keen on this, but I suspect others might not be happy with moving the flush into the loop.

A comment on the call to flush_pmd_entry could solve it.

Quoted 7 lines

Quoted 2 lines
Theoretically, shouldn't you also increment p in the non-LPAE case?
Yes, I should. v2 shortly...
Thanks,
Mark.

Mark Rutland

author

2013-06-14 16:57 UTC #4 lore raw

PATCH v2 1/1 arm: fix pmd flushing in map_init_section

This patch fixes this by remembering the address of the first pmd we update and using this as the argument to flush_pmd_entry.

Signed-off-by Mark Rutland <[email protected]>

Cc R Sricharan <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Russell King <[email protected]>

Cc stable at vger.kernel.org

---
Since v1:
* Take the incremented value of pmd for !LPAE.
* Comment why only one cache flush is necessary.

 arch/arm/mm/mmu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

arch/arm/mm/mmu.c +8 -1

index e0d8565..1c66f51 100644
@@ -620,6 +620,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p;
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to

@@ -633,12 +634,18 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 	if (addr & SECTION_SIZE)
 		pmd++;
 #endif
+	p = pmd;
+
 	do {
 		*pmd = __pmd(phys | type->prot_sect);
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);
 
-	flush_pmd_entry(pmd);
+	/*
+	 * We expect a minimum cache line of 8 bytes, so this will flush both
+	 * pmd entries with classic tables, and will be a nop for LPAE systems.
+	 */
+	flush_pmd_entry(p);
 }
 
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,

Signature · 1 line

1.8.1.1

Christoffer Dall

2013-06-14 17:04 UTC #5 lore raw

PATCH v2 1/1 arm: fix pmd flushing in map_init_section

Quoted 47 lines · Mark Rutland

In e651eab0af: "ARM: 7677/1: LPAE: Fix mapping in alloc_init_section for unaligned addresses", the pmd flushing was broken when split out to map_init_section. At the end of the final iteration of the while loop, pmd will point at the pmd_t immediately after the pmds we updated, and thus flush_pmd_entry(pmd) won't flush the newly modified pmds. This has been observed to prevent an 11MPCore system from booting.
This patch fixes this by remembering the address of the first pmd we update and using this as the argument to flush_pmd_entry.
Signed-off-by Mark Rutland <[email protected]>
Cc R Sricharan <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Russell King <[email protected]>
Cc stable at vger.kernel.org
---
Since v1:
* Take the incremented value of pmd for !LPAE.
* Comment why only one cache flush is necessary.
 arch/arm/mm/mmu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/mm/mmu.c +3 -0
index e0d8565..1c66f51 100644
@@ -620,6 +620,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p;
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to
@@ -633,12 +634,18 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 	if (addr & SECTION_SIZE)
 		pmd++;
 #endif
+	p = pmd;
+
 	do {
 		*pmd = __pmd(phys | type->prot_sect);
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);

arch/arm/mm/mmu.c static void __init map_init_section(pmd_t *pmd, unsigned long addr, · line 651

Quoted 10 lines

arch/arm/mm/mmu.c +5 -1

 
-	flush_pmd_entry(pmd);
+	/*
+	 * We expect a minimum cache line of 8 bytes, so this will flush both
+	 * pmd entries with classic tables, and will be a nop for LPAE systems.
+	 */
+	flush_pmd_entry(p);
 }
 
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,

Reviewed-by Christoffer Dall <[email protected]>

Catalin Marinas

2013-06-14 17:29 UTC #6 lore raw

PATCH v2 1/1 arm: fix pmd flushing in map_init_section

In reply to Mark Rutland

Quoted 10 lines · Mark Rutland

In e651eab0af: "ARM: 7677/1: LPAE: Fix mapping in alloc_init_section for unaligned addresses", the pmd flushing was broken when split out to map_init_section. At the end of the final iteration of the while loop, pmd will point at the pmd_t immediately after the pmds we updated, and thus flush_pmd_entry(pmd) won't flush the newly modified pmds. This has been observed to prevent an 11MPCore system from booting.
This patch fixes this by remembering the address of the first pmd we update and using this as the argument to flush_pmd_entry.

Quoted 6 lines

Signed-off-by Mark Rutland <[email protected]>
Cc R Sricharan <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Russell King <[email protected]>
Cc stable at vger.kernel.org

Reviewed-by Catalin Marinas <[email protected]>

Catalin Marinas

2013-06-14 17:28 UTC #7 lore raw

PATCH 1/1 arm: fix pmd flushing in map_init_section

In reply to Christoffer Dall

Quoted 41 lines · Christoffer Dall

Quoted 40 lines · Mark Rutland
In e651eab0af: "ARM: 7677/1: LPAE: Fix mapping in alloc_init_section for unaligned addresses", the pmd flushing was broken when split out to map_init_section. At the end of the final iteration of the while loop, pmd will point at the pmd_t immediately after the pmds we updated, and thus flush_pmd_entry(pmd) won't flush the newly modified pmds. This has been observed to prevent an 11MPCore system from booting.
This patch fixes this by remembering the address of the first pmd we update and using this as the argument to flush_pmd_entry.
Signed-off-by Mark Rutland <[email protected]>
Cc R Sricharan <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Russell King <[email protected]>
Cc stable at vger.kernel.org
---
 arch/arm/mm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/mm/mmu.c +2 -1
index e0d8565..22bc0ff 100644
@@ -620,6 +620,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p = pmd;
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to
@@ -638,7 +639,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);
 
-	flush_pmd_entry(pmd);
+	flush_pmd_entry(p);
 }
 

Quoted 8 lines

arch/arm/mm/mmu.c static void __init map_init_section(pmd_t *pmd, unsigned long addr, · line 645
Quoted 2 lines
arch/arm/mm/mmu.c +0 -0
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
Refresh my memory here again, why are we not flushing every pmd entry we update? Is it because we assume the cache lines cover the maximum span between addr and end?
Theoretically, shouldn't you also increment p in the non-LPAE case?

It wouldn't make any difference. With classic MMU we assume that we write 2 pmds at the same time (to form a pgd covering 2MB) but the above increment is a workaround to only allow 1MB section mappings. Either way, it's harmless.

Signature · 1 line

Catalin