fix memremap on ARM

PATCH Superseded Ard Biesheuvel 3 participants 8 messages Mar 4, 2016 – Mar 5, 2016

Subsystems (2)

Arm (32-bit) Memory technology devices (MTD)

lore

Share & tools

lore

Ard Biesheuvel

author

2016-03-04 09:48 UTC #0 lore raw

v3 cover fix memremap on ARM

This is something I ran into while working on support for the UEFI memory attributes and ESRT tables. In both cases, these tables are passed to the kernel in memory which is guaranteed to be below 4 GB, but may be outside of the kernel direct mapping. (UEFI typically attempts to allocate from the top down, which means such tables are highly likely to be in highmem for any system with more than 760 MB of system RAM)

The recently introduced memremap() is a very useful abstraction for accessing such tables, because it is generic, and already attempts to do the right thing with respect to regions that may already have been mapped directly. However, it falls back to ioremap_cache() for mapping high memory, which is not allowed on ARM for system RAM, and also results in the region to be mapped with different attributes depending on whether it is covered by lowmem or not.

So instead, create an arch specific hook 'arch_memremap_wb(), and implement it for ARM using the same memory attributes used for the linear mapping. Note that memremap will only call this hook for regions that are not already mapped permanently.

Since this change results in memremap() to use attributes different from the ones used by ioremap_cache(), revert the change to pxa2xx-flash that moved it to memremap.

Changes since v2:
- add patch to bring back ioremap_cached() on ARM
- switch pxa2xx-flash back to ioremap_cached() not ioremap_cache()
- use arch_ioremap_caller not __arm_ioremap_caller() in patch #4
- deal with __iomem annotation of arch_ioremap_caller (patch #4)

Changes since v1/rfc:
- new patch #1 that reverts the ioremap_cache->memremap conversion for the pxa2xx-flash driver
- added Dan's ack to patch #2

Ard Biesheuvel (4):
ARM: reintroduce ioremap_cached() for creating cached I/O mappings
mtd: pxa2xx-flash: switch back from memremap to ioremap_cached
memremap: add arch specific hook for MEMREMAP_WB mappings
ARM: memremap: implement arch_memremap_wb()

 arch/arm/include/asm/io.h       | 12 ++++++++++++
 arch/arm/mm/ioremap.c           | 16 ++++++++++++++--
 drivers/mtd/maps/pxa2xx-flash.c |  6 +++---
 kernel/memremap.c               | 11 ++++++++---
 4 files changed, 37 insertions(+), 8 deletions(-)

Signature · 1 line

2.5.0

Ard Biesheuvel

author

2016-03-04 09:48 UTC #1 lore raw

PATCH v3 1/4 ARM: reintroduce ioremap_cached() for creating cached I/O mappings

The original ARM-only ioremap flavor 'ioremap_cached' has been renamed to 'ioremap_cache' to align with other architectures, and subsequently abused in generic code to map things like firmware tables in memory. For that reason, there is currently an effort underway to deprecate ioremap_cache, whose semantics are poorly defined, and which is typed with an __iomem annotation that is inappropriate for mappings of ordinary memory.

However, original users of ioremap_cached() used it in a context where the I/O connotation is appropriate, and replacing those instances with memremap() does not make sense. So let's revive ioremap_cached(), so that we can change back those original users before we drop ioremap_cache entirely in favor of memremap.

Cc Russell King <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/include/asm/io.h | 6 ++++++
 arch/arm/mm/ioremap.c     | 4 ++++
 2 files changed, 10 insertions(+)

arch/arm/include/asm/io.h +6 -0

index 485982084fe9..fb94de290edd 100644
@@ -395,6 +395,12 @@ void __iomem *ioremap(resource_size_t res_cookie, size_t size);
 void __iomem *ioremap_cache(resource_size_t res_cookie, size_t size);
 #define ioremap_cache ioremap_cache
 
+/*
+ * Do not use ioremap_cached in new code. Provided for the benefit of
+ * the pxa2xx-flash MTD driver only.
+ */
+void __iomem *ioremap_cached(resource_size_t res_cookie, size_t size);
+
 void __iomem *ioremap_wc(resource_size_t res_cookie, size_t size);
 #define ioremap_wc ioremap_wc
 #define ioremap_wt ioremap_wc

arch/arm/mm/ioremap.c +4 -0

index 66a978d05958..d5350f6af089 100644
@@ -380,11 +380,15 @@ void __iomem *ioremap(resource_size_t res_cookie, size_t size)
 EXPORT_SYMBOL(ioremap);
 
 void __iomem *ioremap_cache(resource_size_t res_cookie, size_t size)
+	__alias(ioremap_cached);
+
+void __iomem *ioremap_cached(resource_size_t res_cookie, size_t size)
 {
 	return arch_ioremap_caller(res_cookie, size, MT_DEVICE_CACHED,
 				   __builtin_return_address(0));
 }
 EXPORT_SYMBOL(ioremap_cache);
+EXPORT_SYMBOL(ioremap_cached);
 
 void __iomem *ioremap_wc(resource_size_t res_cookie, size_t size)
 {

Signature · 1 line

2.5.0

Ard Biesheuvel

author

2016-03-04 09:48 UTC #2 lore raw

PATCH v3 2/4 mtd: pxa2xx-flash: switch back from memremap to ioremap_cached

In reply to Ard Biesheuvel

This reverts commit 06968a54790d ("mtd: pxa2xx-flash: switch from ioremap_cache to memremap"), since NOR with memory semantics in array mode and RAM are not necessarily the same thing, and architectures may implement ioremap_cached() and memremap() with different memory attributes.

For this reason, ioremap_cached() has been brought back from the dead on the ARM side, so switch this driver back to using it instead of memremap().

Cc David Woodhouse <[email protected]>

Cc Brian Norris <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/mtd/maps/pxa2xx-flash.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/mtd/maps/pxa2xx-flash.c +3 -3

index 7497090e9900..2cde28ed95c9 100644
@@ -71,8 +71,8 @@ static int pxa2xx_flash_probe(struct platform_device *pdev)
 		       info->map.name);
 		return -ENOMEM;
 	}
-	info->map.cached = memremap(info->map.phys, info->map.size,
-			MEMREMAP_WB);
+	info->map.cached =
+		ioremap_cached(info->map.phys, info->map.size);
 	if (!info->map.cached)
 		printk(KERN_WARNING "Failed to ioremap cached %s\n",
 		       info->map.name);

@@ -111,7 +111,7 @@ static int pxa2xx_flash_remove(struct platform_device *dev)
 	map_destroy(info->mtd);
 	iounmap(info->map.virt);
 	if (info->map.cached)
-		memunmap(info->map.cached);
+		iounmap(info->map.cached);
 	kfree(info);
 	return 0;
 }

Signature · 1 line

2.5.0

Ard Biesheuvel

author

2016-03-04 09:48 UTC #3 lore raw

PATCH v3 3/4 memremap: add arch specific hook for MEMREMAP_WB mappings

In reply to Ard Biesheuvel

Currently, the memremap code serves MEMREMAP_WB mappings directly from the kernel direct mapping, unless the region is in high memory, in which case it falls back to using ioremap_cache(). However, the semantics of ioremap_cache() are not unambiguously defined, and on ARM, it will actually result in a mapping type that differs from the attributes used for the linear mapping, and for this reason, the ioremap_cache() call fails if the region is part of the memory managed by the kernel.

So instead, implement an optional hook 'arch_memremap_wb' whose default implementation calls ioremap_cache() as before, but which can be overridden by the architecture to do what is appropriate for it.

Acked-by Dan Williams <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 kernel/memremap.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

kernel/memremap.c +8 -3

index 7a1b5c3ef14e..77c41648ba16 100644
@@ -27,6 +27,13 @@ __weak void __iomem *ioremap_cache(resource_size_t offset, unsigned long size)
 }
 #endif
 
+#ifndef arch_memremap_wb
+static void *arch_memremap_wb(resource_size_t offset, unsigned long size)
+{
+	return (__force void *)ioremap_cache(offset, size);
+}
+#endif
+
 static void *try_ram_remap(resource_size_t offset, size_t size)
 {
 	struct page *page = pfn_to_page(offset >> PAGE_SHIFT);

@@ -34,7 +41,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size)
 	/* In the simple case just return the existing linear address */
 	if (!PageHighMem(page))
 		return __va(offset);
-	return NULL; /* fallback to ioremap_cache */
+	return arch_memremap_wb(offset, size);
 }
 
 /**

@@ -80,8 +87,6 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
 		 */
 		if (is_ram == REGION_INTERSECTS)
 			addr = try_ram_remap(offset, size);
-		if (!addr)
-			addr = ioremap_cache(offset, size);
 	}
 
 	/*

Signature · 1 line

2.5.0

Ard Biesheuvel

author

2016-03-04 09:48 UTC #4 lore raw

PATCH v3 4/4 ARM: memremap: implement arch_memremap_wb()

In reply to Ard Biesheuvel

The generic memremap() falls back to using ioremap_cache() to create MEMREMAP_WB mappings if the requested region is not already covered by the linear mapping, unless the architecture provides an implementation of arch_memremap_wb().

Since ioremap_cache() is not appropriate on ARM to map memory with the same attributes used for the linear mapping, implement arch_memremap_wb() which does exactly that. Also, relax the WARN() check to allow MT_MEMORY_RW mappings of pfn_valid() pages.

Cc Russell King <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm/include/asm/io.h |  6 ++++++
 arch/arm/mm/ioremap.c     | 12 ++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

arch/arm/include/asm/io.h +6 -0

index fb94de290edd..781ef5fe235d 100644
@@ -392,6 +392,9 @@ void __iomem *ioremap(resource_size_t res_cookie, size_t size);
 #define ioremap ioremap
 #define ioremap_nocache ioremap
 
+/*
+ * Do not use ioremap_cache for mapping memory. Use memremap instead.
+ */
 void __iomem *ioremap_cache(resource_size_t res_cookie, size_t size);
 #define ioremap_cache ioremap_cache

@@ -408,6 +411,9 @@ void __iomem *ioremap_wc(resource_size_t res_cookie, size_t size);
 void iounmap(volatile void __iomem *iomem_cookie);
 #define iounmap iounmap
 
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size);
+#define arch_memremap_wb arch_memremap_wb
+
 /*
  * io{read,write}{16,32}be() macros
  */

arch/arm/mm/ioremap.c +10 -2

index d5350f6af089..ff0eed23ddf1 100644
@@ -297,9 +297,10 @@ static void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 	}
 
 	/*
-	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
+	 * Don't allow RAM to be mapped with mismatched attributes - this
+	 * causes problems with ARMv6+
 	 */
-	if (WARN_ON(pfn_valid(pfn)))
+	if (WARN_ON(pfn_valid(pfn) && mtype != MT_MEMORY_RW))
 		return NULL;
 
 	area = get_vm_area_caller(size, VM_IOREMAP, caller);

@@ -418,6 +419,13 @@ __arm_ioremap_exec(phys_addr_t phys_addr, size_t size, bool cached)
 			__builtin_return_address(0));
 }
 
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size)
+{
+	return (__force void *)arch_ioremap_caller(phys_addr, size,
+						   MT_MEMORY_RW,
+						   __builtin_return_address(0));
+}
+
 void __iounmap(volatile void __iomem *io_addr)
 {
 	void *addr = (void *)(PAGE_MASK & (unsigned long)io_addr);

Signature · 1 line

2.5.0

Dan Williams

2016-03-04 18:34 UTC #5 lore raw

v3 cover fix memremap on ARM

In reply to Ard Biesheuvel

Quoted 42 lines · Ard Biesheuvel

This is something I ran into while working on support for the UEFI memory attributes and ESRT tables. In both cases, these tables are passed to the kernel in memory which is guaranteed to be below 4 GB, but may be outside of the kernel direct mapping. (UEFI typically attempts to allocate from the top down, which means such tables are highly likely to be in highmem for any system with more than 760 MB of system RAM)
The recently introduced memremap() is a very useful abstraction for accessing such tables, because it is generic, and already attempts to do the right thing with respect to regions that may already have been mapped directly. However, it falls back to ioremap_cache() for mapping high memory, which is not allowed on ARM for system RAM, and also results in the region to be mapped with different attributes depending on whether it is covered by lowmem or not.
So instead, create an arch specific hook 'arch_memremap_wb(), and implement it for ARM using the same memory attributes used for the linear mapping. Note that memremap will only call this hook for regions that are not already mapped permanently.
Since this change results in memremap() to use attributes different from the ones used by ioremap_cache(), revert the change to pxa2xx-flash that moved it to memremap.
Changes since v2:
- add patch to bring back ioremap_cached() on ARM
- switch pxa2xx-flash back to ioremap_cached() not ioremap_cache()
- use arch_ioremap_caller not __arm_ioremap_caller() in patch #4
- deal with __iomem annotation of arch_ioremap_caller (patch #4)
Changes since v1/rfc:
- new patch #1 that reverts the ioremap_cache->memremap conversion for the pxa2xx-flash driver
- added Dan's ack to patch #2
Ard Biesheuvel (4):
ARM: reintroduce ioremap_cached() for creating cached I/O mappings
mtd: pxa2xx-flash: switch back from memremap to ioremap_cached
memremap: add arch specific hook for MEMREMAP_WB mappings
ARM: memremap: implement arch_memremap_wb()

Quoted 6 lines

 arch/arm/include/asm/io.h       | 12 ++++++++++++
 arch/arm/mm/ioremap.c           | 16 ++++++++++++++--
 drivers/mtd/maps/pxa2xx-flash.c |  6 +++---
 kernel/memremap.c               | 11 ++++++++---
 4 files changed, 37 insertions(+), 8 deletions(-)

For the series:

Acked-by Dan Williams <[email protected]>

Ard Biesheuvel

author

2016-03-04 19:16 UTC #6 lore raw

v3 cover fix memremap on ARM

Quoted 51 lines · Dan Williams

Quoted 42 lines · Ard Biesheuvel
This is something I ran into while working on support for the UEFI memory attributes and ESRT tables. In both cases, these tables are passed to the kernel in memory which is guaranteed to be below 4 GB, but may be outside of the kernel direct mapping. (UEFI typically attempts to allocate from the top down, which means such tables are highly likely to be in highmem for any system with more than 760 MB of system RAM)
The recently introduced memremap() is a very useful abstraction for accessing such tables, because it is generic, and already attempts to do the right thing with respect to regions that may already have been mapped directly. However, it falls back to ioremap_cache() for mapping high memory, which is not allowed on ARM for system RAM, and also results in the region to be mapped with different attributes depending on whether it is covered by lowmem or not.
So instead, create an arch specific hook 'arch_memremap_wb(), and implement it for ARM using the same memory attributes used for the linear mapping. Note that memremap will only call this hook for regions that are not already mapped permanently.
Since this change results in memremap() to use attributes different from the ones used by ioremap_cache(), revert the change to pxa2xx-flash that moved it to memremap.
Changes since v2:
- add patch to bring back ioremap_cached() on ARM
- switch pxa2xx-flash back to ioremap_cached() not ioremap_cache()
- use arch_ioremap_caller not __arm_ioremap_caller() in patch #4
- deal with __iomem annotation of arch_ioremap_caller (patch #4)
Changes since v1/rfc:
- new patch #1 that reverts the ioremap_cache->memremap conversion for the pxa2xx-flash driver
- added Dan's ack to patch #2
Ard Biesheuvel (4):
ARM: reintroduce ioremap_cached() for creating cached I/O mappings
mtd: pxa2xx-flash: switch back from memremap to ioremap_cached
memremap: add arch specific hook for MEMREMAP_WB mappings
ARM: memremap: implement arch_memremap_wb()
Quoted 6 lines
 arch/arm/include/asm/io.h       | 12 ++++++++++++
 arch/arm/mm/ioremap.c           | 16 ++++++++++++++--
 drivers/mtd/maps/pxa2xx-flash.c |  6 +++---
 kernel/memremap.c               | 11 ++++++++---
 4 files changed, 37 insertions(+), 8 deletions(-)

Quoted 3 lines

For the series:
Acked-by Dan Williams <[email protected]>

Thanks Dan,

With the added patch #1, I think it now makes the most sense for Russell to take the whole series via the ARM tree.

@Brian, David: any objections?

Thanks,
Ard.

Brian Norris

2016-03-05 00:15 UTC #7 lore raw

v3 cover fix memremap on ARM

Quoted 54 lines · Ard Biesheuvel

Quoted 53 lines · Dan Williams
Quoted 42 lines · Ard Biesheuvel
This is something I ran into while working on support for the UEFI memory attributes and ESRT tables. In both cases, these tables are passed to the kernel in memory which is guaranteed to be below 4 GB, but may be outside of the kernel direct mapping. (UEFI typically attempts to allocate from the top down, which means such tables are highly likely to be in highmem for any system with more than 760 MB of system RAM)
The recently introduced memremap() is a very useful abstraction for accessing such tables, because it is generic, and already attempts to do the right thing with respect to regions that may already have been mapped directly. However, it falls back to ioremap_cache() for mapping high memory, which is not allowed on ARM for system RAM, and also results in the region to be mapped with different attributes depending on whether it is covered by lowmem or not.
So instead, create an arch specific hook 'arch_memremap_wb(), and implement it for ARM using the same memory attributes used for the linear mapping. Note that memremap will only call this hook for regions that are not already mapped permanently.
Since this change results in memremap() to use attributes different from the ones used by ioremap_cache(), revert the change to pxa2xx-flash that moved it to memremap.
Changes since v2:
- add patch to bring back ioremap_cached() on ARM
- switch pxa2xx-flash back to ioremap_cached() not ioremap_cache()
- use arch_ioremap_caller not __arm_ioremap_caller() in patch #4
- deal with __iomem annotation of arch_ioremap_caller (patch #4)
Changes since v1/rfc:
- new patch #1 that reverts the ioremap_cache->memremap conversion for the pxa2xx-flash driver
- added Dan's ack to patch #2
Ard Biesheuvel (4):
ARM: reintroduce ioremap_cached() for creating cached I/O mappings
mtd: pxa2xx-flash: switch back from memremap to ioremap_cached
memremap: add arch specific hook for MEMREMAP_WB mappings
ARM: memremap: implement arch_memremap_wb()
Quoted 6 lines
 arch/arm/include/asm/io.h       | 12 ++++++++++++
 arch/arm/mm/ioremap.c           | 16 ++++++++++++++--
 drivers/mtd/maps/pxa2xx-flash.c |  6 +++---
 kernel/memremap.c               | 11 ++++++++---
 4 files changed, 37 insertions(+), 8 deletions(-)
For the series:

Quoted 8 lines

Quoted 1 line
Acked-by Dan Williams <[email protected]>
Thanks Dan,
With the added patch #1, I think it now makes the most sense for Russell to take the whole series via the ARM tree.
@Brian, David: any objections?

Nope. For the MTD part:

Acked-by Brian Norris <[email protected]>

Regards,
Brian