Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1339493 > unrolled thread

[RFC PATCH 0/2] fix memremap on ARM

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/2] fix memremap on ARM Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-22 15:10 +0100
    [RFC PATCH 2/2] ARM: memremap: implement arch_memremap_wb() Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-22 15:10 +0100
    [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-22 15:10 +0100
      Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings Dan Williams <dan.j.williams@intel.com> - 2016-02-22 20:10 +0100
        Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-22 20:20 +0100
          Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings Dan Williams <dan.j.williams@intel.com> - 2016-02-22 21:00 +0100
          Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB  mappings Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-02-22 21:10 +0100
            Re: [RFC PATCH 1/2] memremap: add arch specific hook for MEMREMAP_WB mappings Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-22 21:40 +0100

#1339493 — [RFC PATCH 0/2] 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.

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

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

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/

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web