Re: [PATCH 09/10] arch: introduce memremap()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 20, 2015 at 5:00 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> 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 <arnd@xxxxxxxx>
>> Acked-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>>  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.

Thanks for testing!  I'll follow up and see why this failure wasn't
reported by 0day-kbuild.

> [...]
>
>> 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);
>               ^

Again, I need to check my compile scripts.

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

Really, "portions of", not the entire table?  Is it too early to use
the direct mapping like pfn_to_page(memmap.phys_map >> PAGE_SHIFT)?

In any event this splat mandates splitting this memremap() enabling
patch into per-instance conversions of ioremap_cache().

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

Ah, so ARM's ioremap_cache() silently handles calls to remap RAM with
a pointer to the direct mapping.  It think that's a useful property to
carry in the generic implementation.  Especially if the direct mapping
has established greater than PAGE_SIZE mappings.

>
>> +#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.

Thanks Mark!
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Newbies]     [x86 Platform Driver]     [Netdev]     [Linux Wireless]     [Netfilter]     [Bugtraq]     [Linux Filesystems]     [Yosemite Discussion]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux