Hi,
On Mon, Jul 20, 2015 at 01:18:23AM +0100, Dan Williams wrote:
> Existing users of ioremap_cache() are mapping memory that is known in
> advance to not have i/o side effects. These users are forced to cast
> away the __iomem annotation, or otherwise neglect to fix the sparse
> errors thrown when dereferencing pointers to this memory. Provide
> memremap() as a non __iomem annotated ioremap_*() in the case when
> ioremap is otherwise a pointer to memory.
>
> The ARCH_HAS_MEMREMAP kconfig symbol is introduced for archs to assert
> that it is safe to recast / reuse the return value from ioremap as a
> normal pointer to memory. In other words, archs that mandate specific
> accessors for __iomem are not memremap() capable and drivers that care,
> like pmem, can add a dependency to disable themselves on these archs.
>
> Note, that memremap is a break from the ioremap implementation pattern
> of adding a new memremap_<type>() for each mapping type. Instead,
> the implementation defines flags that are passed to the central
> memremap() implementation.
>
> Outside of ioremap() and ioremap_nocache(), the expectation is that most
> calls to ioremap_<type>() are seeking memory-like semantics (e.g.
> speculative reads, and prefetching permitted). These callsites can be
> moved to memremap() over time.
>
> Cc: Arnd Bergmann <a...@arndb.de>
> Acked-by: Andy Shevchenko <andy.shevche...@gmail.com>
> Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> ---
> arch/arm/Kconfig | 1 +
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/efi.c | 7 ++---
> arch/arm64/kernel/smp_spin_table.c | 17 +++++------
These last two files weren't updated in patch 2 to use <linux/io.h>, nor are
they updated here, so this patch breaks the build for arm64.
[...]
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 318175f62c24..305def28385f 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -6,6 +6,7 @@ config ARM64
> select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
> select ARCH_HAS_ELF_RANDOMIZE
> select ARCH_HAS_GCOV_PROFILE_ALL
> + select ARCH_HAS_MEMREMAP
> select ARCH_HAS_SG_CHAIN
> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> select ARCH_USE_CMPXCHG_LOCKREF
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 9d4aa18f2a82..ed363a0202b9 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -290,8 +290,7 @@ static int __init arm64_enable_runtime_services(void)
> pr_info("Remapping and enabling EFI services.\n");
>
> mapsize = memmap.map_end - memmap.map;
> - memmap.map = (__force void
> *)ioremap_cache((phys_addr_t)memmap.phys_map,
> - mapsize);
> + memmap.map = memremap(memmap.phys_map, mapsize, MEMREMAP_CACHE);
memmap.phys_map is a void*, so we need to retain a cast to a numeric type or
we'll get splats like:
arch/arm64/kernel/efi.c: In function ‘arm64_enable_runtime_services’:
arch/arm64/kernel/efi.c:294:24: warning: passing argument 1 of ‘memremap’ makes
integer from pointer without a cast
memmap.map = memremap(memmap.phys_map, mapsize, MEMREMAP_CACHE);
^
In file included from arch/arm64/kernel/efi.c:18:0:
include/linux/io.h:203:14: note: expected ‘resource_size_t’ but argument is of
type ‘void *’
extern void *memremap(resource_size_t offset, size_t size, unsigned long
flags);
^
Unfortunately, even with that fixed this patch breaks boot due to the
memremap_valid check. It's extremely likely that (portions of) the
tables are already in the linear mapping, and so page_is_ram will be
true.
At boot time I get the following:
Remapping and enabling EFI services.
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at kernel/resource.c:541 memremap+0xb0/0xbc()
memremap attempted on ram 0x00000009f9e4a018 size: 1200
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.2.0-rc3+ #201
Hardware name: ARM Juno development board (r0) (DT)
Call trace:
[<ffffffc000089954>] dump_backtrace+0x0/0x124
[<ffffffc000089a88>] show_stack+0x10/0x1c
[<ffffffc0005d15c8>] dump_stack+0x84/0xc8
[<ffffffc0000b3f44>] warn_slowpath_common+0x98/0xd0
[<ffffffc0000b3fc8>] warn_slowpath_fmt+0x4c/0x58
[<ffffffc0000b9470>] memremap+0xac/0xbc
[<ffffffc000814fe8>] arm64_enable_runtime_services+0x90/0x208
[<ffffffc000082864>] do_one_initcall+0x88/0x1a0
[<ffffffc000811a20>] kernel_init_freeable+0x8c/0x1f8
[<ffffffc0005ce370>] kernel_init+0xc/0xd8
---[ end trace cb88537fdc8fa200 ]---
Failed to remap EFI memory map
> if (!memmap.map) {
> pr_err("Failed to remap EFI memory map\n");
> return -1;
> @@ -299,8 +298,8 @@ static int __init arm64_enable_runtime_services(void)
> memmap.map_end = memmap.map + mapsize;
> efi.memmap = &memmap;
>
> - efi.systab = (__force void *)ioremap_cache(efi_system_table,
> -
> sizeof(efi_system_table_t));
> + efi.systab = memremap(efi_system_table, sizeof(efi_system_table_t),
> + MEMREMAP_CACHE);
> if (!efi.systab) {
> pr_err("Failed to remap EFI System Table\n");
> return -1;
> diff --git a/arch/arm64/kernel/smp_spin_table.c
> b/arch/arm64/kernel/smp_spin_table.c
> index aef3605a8c47..b9caf6cd44e6 100644
> --- a/arch/arm64/kernel/smp_spin_table.c
> +++ b/arch/arm64/kernel/smp_spin_table.c
> @@ -73,19 +73,19 @@ static int smp_spin_table_cpu_init(unsigned int cpu)
>
> static int smp_spin_table_cpu_prepare(unsigned int cpu)
> {
> - __le64 __iomem *release_addr;
> + __le64 *release_addr;
>
> if (!cpu_release_addr[cpu])
> return -ENODEV;
>
> /*
> * The cpu-release-addr may or may not be inside the linear mapping.
> - * As ioremap_cache will either give us a new mapping or reuse the
> - * existing linear mapping, we can use it to cover both cases. In
> - * either case the memory will be MT_NORMAL.
> + * As memremap will either give us a new mapping or reuse the existing
> + * linear mapping, we can use it to cover both cases. In either case
> + * the memory will be MT_NORMAL.
> */
> - release_addr = ioremap_cache(cpu_release_addr[cpu],
> - sizeof(*release_addr));
> + release_addr = memremap(cpu_release_addr[cpu], sizeof(*release_addr),
> + MEMREMAP_CACHE);
Likewise, the comment here is contradicted by the code in memremap_valid
(below). We'll fail to bring up secondaries if the release address fell
in the linear mapping.
> +#ifdef CONFIG_ARCH_HAS_MEMREMAP
> +/*
> + * memremap() is "ioremap" for cases where it is known that the resource
> + * being mapped does not have i/o side effects and the __iomem
> + * annotation is not applicable.
> + */
> +static bool memremap_valid(resource_size_t offset, size_t size)
> +{
> + if (region_is_ram(offset, size) != 0) {
> + WARN_ONCE(1, "memremap attempted on ram %pa size: %zu\n",
> + &offset, size);
> + return false;
> + }
> + return true;
> +}
> +
> +void *memremap(resource_size_t offset, size_t size, unsigned long flags)
> +{
> + void __iomem *addr = NULL;
> +
> + if (!memremap_valid(offset, size))
> + return NULL;
> +
> + /* try all mapping types requested until one returns non-NULL */
> + if (flags & MEMREMAP_CACHE) {
> + if (flags & MEMREMAP_STRICT)
> + addr = strict_ioremap_cache(offset, size);
> + else
> + addr = ioremap_cache(offset, size);
> + }
> +
> + if (!addr && (flags & MEMREMAP_WT)) {
> + if (flags & MEMREMAP_STRICT)
> + addr = strict_ioremap_wt(offset, size);
> + else
> + addr = ioremap_wt(offset, size);
> + }
> +
> + return (void __force *) addr;
> +}
> +EXPORT_SYMBOL(memremap);
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
