[RFC PATCH 0/2] fix memremap on ARM

[RFC PATCH 0/2] fix memremap on ARM

[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.

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.

Ard Biesheuvel (2):
  memremap: add arch specific hook for MEMREMAP_WB mappings
  ARM: memremap: implement arch_memremap_wb()

 arch/arm/include/asm/io.h |  3 +++
 arch/arm/mm/ioremap.c     | 11 +++++++++--
 kernel/memremap.c         | 11 ++++++++---
 3 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.5.0

[RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[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.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 kernel/memremap.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/memremap.c b/kernel/memremap.c
index 7a1b5c3ef14e..77c41648ba16 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -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);
 	}
 
 	/*
-- 
2.5.0

[RFC PATCH 2/2] ARM: memremap: implement arch_memremap_wb()

[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 <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/io.h |  3 +++
 arch/arm/mm/ioremap.c     | 11 +++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 485982084fe9..7456638e6b3a 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -402,6 +402,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
  */
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 66a978d05958..d3a2b028c614 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -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);
@@ -414,6 +415,12 @@ __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 __arm_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);
-- 
2.5.0

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Dan Williams]

On Mon, Feb 22, 2016 at 6:02 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> 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 <dan.j.williams@intel.com>

I still have patches pending to delete ioremap_cache() from ARM and
require memremap() to be used for cacheable mappings.  Do you see any
use for ioremap_cache() on ARM after this change?

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Ard Biesheuvel]

On 22 February 2016 at 20:05, Dan Williams <dan.j.williams@intel.com> wrote:
> On Mon, Feb 22, 2016 at 6:02 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> 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 <dan.j.williams@intel.com>
>
> I still have patches pending to delete ioremap_cache() from ARM and
> require memremap() to be used for cacheable mappings.  Do you see any
> use for ioremap_cache() on ARM after this change?

I am not exactly sure why ioremap_cache() does not use MT_MEMORY_RW
attributes, but the ARM architecture simply does not allow mismatched
attributes, so we cannot simply replace each instance of
ioremap_cache() with memremap()

Perhaps Russell can explain?

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Dan Williams]

On Mon, Feb 22, 2016 at 11:17 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 22 February 2016 at 20:05, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Mon, Feb 22, 2016 at 6:02 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> 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 <dan.j.williams@intel.com>
>>
>> I still have patches pending to delete ioremap_cache() from ARM and
>> require memremap() to be used for cacheable mappings.  Do you see any
>> use for ioremap_cache() on ARM after this change?
>
> I am not exactly sure why ioremap_cache() does not use MT_MEMORY_RW
> attributes, but the ARM architecture simply does not allow mismatched
> attributes, so we cannot simply replace each instance of
> ioremap_cache() with memremap()
>
> Perhaps Russell can explain?

>From the x86 perspective mismatched mappings are also disallowed.  The
goal of deprecating ioremap_cache() in favor of memremap() is simply
that the __iomem annotation is misplaced [1], and to clean up the
calling convention to be explicit without silent fallbacks to
different mapping types.

[1]: https://lwn.net/Articles/653585/

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Russell King - ARM Linux]

On Mon, Feb 22, 2016 at 08:17:11PM +0100, Ard Biesheuvel wrote:
> I am not exactly sure why ioremap_cache() does not use MT_MEMORY_RW
> attributes, but the ARM architecture simply does not allow mismatched
> attributes, so we cannot simply replace each instance of
> ioremap_cache() with memremap()
> 
> Perhaps Russell can explain?

ARM has had ioremap_cached() for a while - it was introduced in the
bitkeeper times of 2.6, so pre-git.  In those kernels, and into the
git era, pre-dating ARMv6 support, it was merely:

+#define ioremap_cached(cookie,size)    __arch_ioremap((cookie),(size),L_PTE_CACHEABLE)

which means that we got write-through cache behaviour for mappings
created by this, where supported, or if not, read-allocate writeback.
This was completely independent of the system memory mapping
attributes, which could be specified on the kernel command line.

This was originally used by pxa2xx-flash to provide faster flash
access on those systems - in other words, it's created to remap
devices with cacheable attributes.

When creating ARMv6 support, I ended up completely rewriting how
the memory attributes were handled, and so it then became this:

+#define ioremap_cached(cookie,size)    __arch_ioremap((cookie), (size), MT_DEVICE_CACHED)

which gives very similar behaviour, though we now default to RAWB
mappings, which fall back to WT on CPUs that don't support RAWB.
Again, independent of the system memory mapping.

Then, in 2013, with the advent of Xen, ioremap_cached() became
ioremap_cache() so that Xen would build on ARM, and to align it
with other architectures.  Whether ioremap_cached() actually was
suitable to become ioremap_cache(), I'm not sure, but that's
what happened.

Since it was just renamed, it preserves the original goal which is
to remap device memory with cacheable attributes, which may differ
from the cacheable attributes of the system RAM.  It has never
been intended for remapping system memory: none of the ioremap_*
family of functions on ARM were ever intended for that purpose.

However, some people did use it for that purpose on ARMv5 and
earlier architectures, where, due to the virtual cache architecture,
you could get away with remapping the same memory with differing
attributes without any problem.  With the advent of ARMv6
(pre-dating 2013), this was clearly stated as being illegal at
architecture level, but people were married to the idea - despite
me telling them not to.

So eventually, I had no other option than to add a code check to
ioremap*() which prevents any ioremap*() function from being used
on system memory - iow, memory that Linux maps itself either as
part of lowmem or via the kmap*() API - since an ioremap*()
mapping would conflict with those.

That's basically where we are today: ioremap*() does not permit
system memory to be remapped, even ioremap_cache().

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Ard Biesheuvel]

On 22 February 2016 at 21:02, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Feb 22, 2016 at 08:17:11PM +0100, Ard Biesheuvel wrote:
>> I am not exactly sure why ioremap_cache() does not use MT_MEMORY_RW
>> attributes, but the ARM architecture simply does not allow mismatched
>> attributes, so we cannot simply replace each instance of
>> ioremap_cache() with memremap()
>>
>> Perhaps Russell can explain?
>
> ARM has had ioremap_cached() for a while - it was introduced in the
> bitkeeper times of 2.6, so pre-git.  In those kernels, and into the
> git era, pre-dating ARMv6 support, it was merely:
>
> +#define ioremap_cached(cookie,size)    __arch_ioremap((cookie),(size),L_PTE_CACHEABLE)
>
> which means that we got write-through cache behaviour for mappings
> created by this, where supported, or if not, read-allocate writeback.
> This was completely independent of the system memory mapping
> attributes, which could be specified on the kernel command line.
>
> This was originally used by pxa2xx-flash to provide faster flash
> access on those systems - in other words, it's created to remap
> devices with cacheable attributes.
>
> When creating ARMv6 support, I ended up completely rewriting how
> the memory attributes were handled, and so it then became this:
>
> +#define ioremap_cached(cookie,size)    __arch_ioremap((cookie), (size), MT_DEVICE_CACHED)
>
> which gives very similar behaviour, though we now default to RAWB
> mappings, which fall back to WT on CPUs that don't support RAWB.
> Again, independent of the system memory mapping.
>
> Then, in 2013, with the advent of Xen, ioremap_cached() became
> ioremap_cache() so that Xen would build on ARM, and to align it
> with other architectures.  Whether ioremap_cached() actually was
> suitable to become ioremap_cache(), I'm not sure, but that's
> what happened.
>
> Since it was just renamed, it preserves the original goal which is
> to remap device memory with cacheable attributes, which may differ
> from the cacheable attributes of the system RAM.  It has never
> been intended for remapping system memory: none of the ioremap_*
> family of functions on ARM were ever intended for that purpose.
>
> However, some people did use it for that purpose on ARMv5 and
> earlier architectures, where, due to the virtual cache architecture,
> you could get away with remapping the same memory with differing
> attributes without any problem.  With the advent of ARMv6
> (pre-dating 2013), this was clearly stated as being illegal at
> architecture level, but people were married to the idea - despite
> me telling them not to.
>
> So eventually, I had no other option than to add a code check to
> ioremap*() which prevents any ioremap*() function from being used
> on system memory - iow, memory that Linux maps itself either as
> part of lowmem or via the kmap*() API - since an ioremap*()
> mapping would conflict with those.
>
> That's basically where we are today: ioremap*() does not permit
> system memory to be remapped, even ioremap_cache().
>

OK, thanks for the historical context.

So what is your opinion on this series, i.e., to wire up memremap() to
remap arbitrary memory regions into the vmalloc area with MT_MEMORY_RW
attributes, and at the same time lift the restriction that the region
must be disjoint from memory covered by lowmem or kmap?

It would make my life a lot easier, since we can more easily share
code between x86, arm64 and ARM to permanently map memory regions that
have been populated by the firmware. As I noted in the commit log,
memremap() already does the right thing wrt lowmem, i.e., it returns
the existing mapping rather than creating a new one. For highmem, I
don't think kmap() is the way to go considering the unknown size and
the potentially permanent nature of the mappings (which resemble
ioremap more than they resemble kmap)

-- 
Ard.

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Russell King - ARM Linux]

On Mon, Feb 22, 2016 at 09:35:24PM +0100, Ard Biesheuvel wrote:
> OK, thanks for the historical context.
> 
> So what is your opinion on this series, i.e., to wire up memremap() to
> remap arbitrary memory regions into the vmalloc area with MT_MEMORY_RW
> attributes, and at the same time lift the restriction that the region
> must be disjoint from memory covered by lowmem or kmap?

The historical context is still present, because pxa2xx-flash has
been converted to use memremap() from ioremap_cache() - possibly
inappropriately.

I've already described the semantics of ioremap_cache(), which are
to always create a cacheable mapping irrespective of the system
memory mapping type.  However, memremap() says that MEMREMAP_WB
matches system RAM, which on ARM it doesn't right now.

Changing it to MT_MEMORY_RW would satisfy that comment against
memremap(), but at the same time changes what happens with
pxa2xx-flash - the memory region (which is not system RAM) then
changes with the cache status of system RAM.

So, I'm not that happy about the memremap() stuff right now, and
I don't like the idea of making memremap() conform to its stated
requirements without first preventing pxa2xx-flash being affected
by such a change.

Perhaps we need to reinstate the original ioremap_cached() API for
pxa2xx-flash, and then switch memremap() to MT_MEMORY_RW - that
would seem to result in the expected behaviour by all parties.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Ard Biesheuvel]

On 23 February 2016 at 12:58, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Feb 22, 2016 at 09:35:24PM +0100, Ard Biesheuvel wrote:
>> OK, thanks for the historical context.
>>
>> So what is your opinion on this series, i.e., to wire up memremap() to
>> remap arbitrary memory regions into the vmalloc area with MT_MEMORY_RW
>> attributes, and at the same time lift the restriction that the region
>> must be disjoint from memory covered by lowmem or kmap?
>
> The historical context is still present, because pxa2xx-flash has
> been converted to use memremap() from ioremap_cache() - possibly
> inappropriately.
>
> I've already described the semantics of ioremap_cache(), which are
> to always create a cacheable mapping irrespective of the system
> memory mapping type.  However, memremap() says that MEMREMAP_WB
> matches system RAM, which on ARM it doesn't right now.
>

Indeed. Hence this series, to decouple memremap(MEMREMAP_WB) from
ioremap_cache() for ARM

> Changing it to MT_MEMORY_RW would satisfy that comment against
> memremap(), but at the same time changes what happens with
> pxa2xx-flash - the memory region (which is not system RAM) then
> changes with the cache status of system RAM.
>
> So, I'm not that happy about the memremap() stuff right now, and
> I don't like the idea of making memremap() conform to its stated
> requirements without first preventing pxa2xx-flash being affected
> by such a change.
>

Actually, my change fixes this issue, since it will cause memremap()
to always create MT_MEMORY_RW mappings, and not fallback to
ioremap_cache() for ranges that are not covered by lowmem.

> Perhaps we need to reinstate the original ioremap_cached() API for
> pxa2xx-flash, and then switch memremap() to MT_MEMORY_RW - that
> would seem to result in the expected behaviour by all parties.
>

I think we can simply revert the change to pxa2xx-flash if it is
deemed inappropriate.

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Ard Biesheuvel]

On 23 February 2016 at 13:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 23 February 2016 at 12:58, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Mon, Feb 22, 2016 at 09:35:24PM +0100, Ard Biesheuvel wrote:
>>> OK, thanks for the historical context.
>>>
>>> So what is your opinion on this series, i.e., to wire up memremap() to
>>> remap arbitrary memory regions into the vmalloc area with MT_MEMORY_RW
>>> attributes, and at the same time lift the restriction that the region
>>> must be disjoint from memory covered by lowmem or kmap?
>>
>> The historical context is still present, because pxa2xx-flash has
>> been converted to use memremap() from ioremap_cache() - possibly
>> inappropriately.
>>
>> I've already described the semantics of ioremap_cache(), which are
>> to always create a cacheable mapping irrespective of the system
>> memory mapping type.  However, memremap() says that MEMREMAP_WB
>> matches system RAM, which on ARM it doesn't right now.
>>
>
> Indeed. Hence this series, to decouple memremap(MEMREMAP_WB) from
> ioremap_cache() for ARM
>
>> Changing it to MT_MEMORY_RW would satisfy that comment against
>> memremap(), but at the same time changes what happens with
>> pxa2xx-flash - the memory region (which is not system RAM) then
>> changes with the cache status of system RAM.
>>
>> So, I'm not that happy about the memremap() stuff right now, and
>> I don't like the idea of making memremap() conform to its stated
>> requirements without first preventing pxa2xx-flash being affected
>> by such a change.
>>
>
> Actually, my change fixes this issue, since it will cause memremap()
> to always create MT_MEMORY_RW mappings, and not fallback to
> ioremap_cache() for ranges that are not covered by lowmem.
>
>> Perhaps we need to reinstate the original ioremap_cached() API for
>> pxa2xx-flash, and then switch memremap() to MT_MEMORY_RW - that
>> would seem to result in the expected behaviour by all parties.
>>
>
> I think we can simply revert the change to pxa2xx-flash if it is
> deemed inappropriate.

OK, I see what you mean. I find it unfortunate that ioremap_cache()
instances are blindly being replaced with memremap(), and I wonder if
this wasted test by and/or cc'ed to people who can actually test this
driver. Dan?

Anyway, I don't think it makes sense to stipulate at the generic level
that ioremap_cache() and memremap(MEMREMAP_WB) shall be the same, and
deprecating it is a bit premature since the cross-architecturally
loosely defined semantics of ioremap_cache() can never be replaced 1:1
with what memremap() promises.

So what I suggest is that I revert the change to pxa2xx-flash as a new
1/3 in this series, and put these existing two on top to decouple
memremap(MEMREMAP_WB) from ioremap_cache() entirely.

Thanks,
Ard.

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Dan Williams]

On Tue, Feb 23, 2016 at 4:26 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 23 February 2016 at 13:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 23 February 2016 at 12:58, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Mon, Feb 22, 2016 at 09:35:24PM +0100, Ard Biesheuvel wrote:
>>>> OK, thanks for the historical context.
>>>>
>>>> So what is your opinion on this series, i.e., to wire up memremap() to
>>>> remap arbitrary memory regions into the vmalloc area with MT_MEMORY_RW
>>>> attributes, and at the same time lift the restriction that the region
>>>> must be disjoint from memory covered by lowmem or kmap?
>>>
>>> The historical context is still present, because pxa2xx-flash has
>>> been converted to use memremap() from ioremap_cache() - possibly
>>> inappropriately.
>>>
>>> I've already described the semantics of ioremap_cache(), which are
>>> to always create a cacheable mapping irrespective of the system
>>> memory mapping type.  However, memremap() says that MEMREMAP_WB
>>> matches system RAM, which on ARM it doesn't right now.
>>>
>>
>> Indeed. Hence this series, to decouple memremap(MEMREMAP_WB) from
>> ioremap_cache() for ARM
>>
>>> Changing it to MT_MEMORY_RW would satisfy that comment against
>>> memremap(), but at the same time changes what happens with
>>> pxa2xx-flash - the memory region (which is not system RAM) then
>>> changes with the cache status of system RAM.
>>>
>>> So, I'm not that happy about the memremap() stuff right now, and
>>> I don't like the idea of making memremap() conform to its stated
>>> requirements without first preventing pxa2xx-flash being affected
>>> by such a change.
>>>
>>
>> Actually, my change fixes this issue, since it will cause memremap()
>> to always create MT_MEMORY_RW mappings, and not fallback to
>> ioremap_cache() for ranges that are not covered by lowmem.
>>
>>> Perhaps we need to reinstate the original ioremap_cached() API for
>>> pxa2xx-flash, and then switch memremap() to MT_MEMORY_RW - that
>>> would seem to result in the expected behaviour by all parties.
>>>
>>
>> I think we can simply revert the change to pxa2xx-flash if it is
>> deemed inappropriate.
>
> OK, I see what you mean. I find it unfortunate that ioremap_cache()
> instances are blindly being replaced with memremap(), and I wonder if
> this wasted test by and/or cc'ed to people who can actually test this
> driver. Dan?

I included that change in my original "convert ARM to memremap"
patchset [1].  I admit I didn't see the problem initially, but in
hindsight I should have told Brian to hold off until the whole
approach was sanity checked by ARM core maintainers.  Since then I've
been deferring the deprecation of ioremap_cache() until we could have
a conversation like this one.

> Anyway, I don't think it makes sense to stipulate at the generic level
> that ioremap_cache() and memremap(MEMREMAP_WB) shall be the same, and
> deprecating it is a bit premature since the cross-architecturally
> loosely defined semantics of ioremap_cache() can never be replaced 1:1
> with what memremap() promises.

Ok, my goal was to clean all the cases the were mishandling the
__iomem annotation where the *accesses* did not have I/O side effects.
What I overlooked was the difference between varying flavors of
writeback cacheable mappings.

> So what I suggest is that I revert the change to pxa2xx-flash as a new
> 1/3 in this series, and put these existing two on top to decouple
> memremap(MEMREMAP_WB) from ioremap_cache() entirely.

Should we formalize the pxa2xx-flash case with a new MEMREMAP_<type>?
Part of the original confusion is that we have ioremap_cache() with
varying semantics across architectures.

[1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/360888.html

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Robert Jarzmik]

Dan Williams <dan.j.williams@intel.com> writes:

> On Tue, Feb 23, 2016 at 4:26 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 23 February 2016 at 13:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 23 February 2016 at 12:58, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk> wrote:
>>>> On Mon, Feb 22, 2016 at 09:35:24PM +0100, Ard Biesheuvel wrote:
>> OK, I see what you mean. I find it unfortunate that ioremap_cache()
>> instances are blindly being replaced with memremap(), and I wonder if
>> this wasted test by and/or cc'ed to people who can actually test this
>> driver. Dan?

Actually I have the hardware to test it.

And I also know what is behind :
 - it's a CFI NOR based memory
 - these are Intel StrataFlash 28F128J3A chips
 - as a CFI memory it is mapped on the system bus
 - from a read perspective, it behaves like a normal memory
 - but once the first write reaches the CFI, everything changes (the address
   space layout doesn't have the same meaning, be that becoming a status code or
   something else).
   In these conditions reordering of writes versus reads, merging reads after
   a write or coalescing writes is a recipe for disaster.

All of this to say I can make a small discrete number of tests (less than 10
write or erase ones to preserve the precious NOR).

Cheers.

--
Robert

Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings

[Ard Biesheuvel]

On 23 February 2016 at 23:23, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
>
>> On Tue, Feb 23, 2016 at 4:26 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 23 February 2016 at 13:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> On 23 February 2016 at 12:58, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> wrote:
>>>>> On Mon, Feb 22, 2016 at 09:35:24PM +0100, Ard Biesheuvel wrote:
>>> OK, I see what you mean. I find it unfortunate that ioremap_cache()
>>> instances are blindly being replaced with memremap(), and I wonder if
>>> this wasted test by and/or cc'ed to people who can actually test this
>>> driver. Dan?
>
> Actually I have the hardware to test it.
>
> And I also know what is behind :
>  - it's a CFI NOR based memory
>  - these are Intel StrataFlash 28F128J3A chips
>  - as a CFI memory it is mapped on the system bus
>  - from a read perspective, it behaves like a normal memory
>  - but once the first write reaches the CFI, everything changes (the address
>    space layout doesn't have the same meaning, be that becoming a status code or
>    something else).
>    In these conditions reordering of writes versus reads, merging reads after
>    a write or coalescing writes is a recipe for disaster.
>
> All of this to say I can make a small discrete number of tests (less than 10
> write or erase ones to preserve the precious NOR).
>

Thanks Robert.

But to be honest, I think we should simply revert the change, after
which we can wire up memremap() for ARM properly. And while I agree
that ioremap_cache() is often abused for mapping things like ACPI
tables in RAM (which forces you to cast away the __iomem annotation),
using ioremap_cache() to map NOR flash is totally different IMO, even
if it has memory semantics while in array mode.