From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Subject: Re: [PATCH 09/10] arch: introduce memremap() Date: Mon, 20 Jul 2015 13:00:41 +0100 Message-ID: <20150720120040.GC19239@leverpostej> References: <20150720001614.30857.89063.stgit@dwillia2-desk3.amr.corp.intel.com> <20150720001822.30857.65670.stgit@dwillia2-desk3.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <20150720001822.30857.65670.stgit@dwillia2-desk3.amr.corp.intel.com> Sender: linux-kernel-owner@vger.kernel.org To: Dan Williams Cc: "tglx@linutronix.de" , "mingo@kernel.org" , "hpa@zytor.com" , "linux-arch@vger.kernel.org" , "tony.luck@intel.com" , "linux@arm.linux.org.uk" , "arnd@arndb.de" , "benh@kernel.crashing.org" , "mcgrof@suse.com" , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "ralf@linux-mips.org" , Andy Shevchenko , "geert@linux-m68k.org" , "toshi.kani@hp.com" , "ross.zwisler@linux.intel.com" , "hch@lst.de" , "linux-arm-kernel@lists.infradead.org" , will.deacon@arm.com, catalin.marinas List-Id: linux-arch.vger.kernel.org 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. >=20 > The ARCH_HAS_MEMREMAP kconfig symbol is introduced for archs to asser= t > that it is safe to recast / reuse the return value from ioremap as a > normal pointer to memory. In other words, archs that mandate specifi= c > accessors for __iomem are not memremap() capable and drivers that car= e, > like pmem, can add a dependency to disable themselves on these archs. >=20 > Note, that memremap is a break from the ioremap implementation patter= n > of adding a new memremap_() for each mapping type. Instead, > the implementation defines flags that are passed to the central > memremap() implementation. >=20 > Outside of ioremap() and ioremap_nocache(), the expectation is that m= ost > calls to ioremap_() are seeking memory-like semantics (e.g. > speculative reads, and prefetching permitted). These callsites can b= e > moved to memremap() over time. >=20 > Cc: Arnd Bergmann > Acked-by: Andy Shevchenko > Signed-off-by: Dan Williams > --- > 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 , no= r 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_BROADCA= ST > 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(v= oid) > pr_info("Remapping and enabling EFI services.\n"); >=20 > mapsize =3D memmap.map_end - memmap.map; > - memmap.map =3D (__force void *)ioremap_cache((phys_addr_t)mem= map.phys_map, > - mapsize); > + memmap.map =3D memremap(memmap.phys_map, mapsize, MEMREMAP_CA= CHE); memmap.phys_map is a void*, so we need to retain a cast to a numeric ty= pe or we'll get splats like: arch/arm64/kernel/efi.c: In function =E2=80=98arm64_enable_runtime_serv= ices=E2=80=99: arch/arm64/kernel/efi.c:294:24: warning: passing argument 1 of =E2=80=98= memremap=E2=80=99 makes integer from pointer without a cast memmap.map =3D 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 =E2=80=98resource_size_t=E2=80= =99 but argument is of type =E2=80=98void *=E2=80=99 extern void *memremap(resource_size_t offset, size_t size, unsigned lo= ng 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: [] dump_backtrace+0x0/0x124 [] show_stack+0x10/0x1c [] dump_stack+0x84/0xc8 [] warn_slowpath_common+0x98/0xd0 [] warn_slowpath_fmt+0x4c/0x58 [] memremap+0xac/0xbc [] arm64_enable_runtime_services+0x90/0x208 [] do_one_initcall+0x88/0x1a0 [] kernel_init_freeable+0x8c/0x1f8 [] kernel_init+0xc/0xd8 ---[ end trace cb88537fdc8fa200 ]--- =46ailed 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(v= oid) > memmap.map_end =3D memmap.map + mapsize; > efi.memmap =3D &memmap; >=20 > - efi.systab =3D (__force void *)ioremap_cache(efi_system_table= , > - sizeof(efi_system_= table_t)); > + efi.systab =3D memremap(efi_system_table, sizeof(efi_system_t= able_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/s= mp_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 c= pu) >=20 > static int smp_spin_table_cpu_prepare(unsigned int cpu) > { > - __le64 __iomem *release_addr; > + __le64 *release_addr; >=20 > if (!cpu_release_addr[cpu]) > return -ENODEV; >=20 > /* > * The cpu-release-addr may or may not be inside the linear m= apping. > - * As ioremap_cache will either give us a new mapping or reus= e the > - * existing linear mapping, we can use it to cover both cases= =2E 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 eith= er case > + * the memory will be MT_NORMAL. > */ > - release_addr =3D ioremap_cache(cpu_release_addr[cpu], > - sizeof(*release_addr)); > + release_addr =3D memremap(cpu_release_addr[cpu], sizeof(*rele= ase_addr), > + MEMREMAP_CACHE); Likewise, the comment here is contradicted by the code in memremap_vali= d (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 reso= urce > + * 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) !=3D 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 fl= ags) > +{ > + void __iomem *addr =3D 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 =3D strict_ioremap_cache(offset, size); > + else > + addr =3D ioremap_cache(offset, size); > + } > + > + if (!addr && (flags & MEMREMAP_WT)) { > + if (flags & MEMREMAP_STRICT) > + addr =3D strict_ioremap_wt(offset, size); > + else > + addr =3D ioremap_wt(offset, size); > + } > + > + return (void __force *) addr; > +} > +EXPORT_SYMBOL(memremap); Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com ([217.140.101.70]:55284 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753669AbbGTMBJ (ORCPT ); Mon, 20 Jul 2015 08:01:09 -0400 Date: Mon, 20 Jul 2015 13:00:41 +0100 From: Mark Rutland Subject: Re: [PATCH 09/10] arch: introduce memremap() Message-ID: <20150720120040.GC19239@leverpostej> References: <20150720001614.30857.89063.stgit@dwillia2-desk3.amr.corp.intel.com> <20150720001822.30857.65670.stgit@dwillia2-desk3.amr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150720001822.30857.65670.stgit@dwillia2-desk3.amr.corp.intel.com> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dan Williams Cc: "tglx@linutronix.de" , "mingo@kernel.org" , "hpa@zytor.com" , "linux-arch@vger.kernel.org" , "tony.luck@intel.com" , "linux@arm.linux.org.uk" , "arnd@arndb.de" , "benh@kernel.crashing.org" , "mcgrof@suse.com" , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "ralf@linux-mips.org" , Andy Shevchenko , "geert@linux-m68k.org" , "toshi.kani@hp.com" , "ross.zwisler@linux.intel.com" , "hch@lst.de" , "linux-arm-kernel@lists.infradead.org" , will.deacon@arm.com, catalin.marinas@arm.com Message-ID: <20150720120041.IjtYb62AWivXmgXC7fvd1N9mr-CRrHsKswNPfP24hyQ@z> 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_() 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_() are seeking memory-like semantics (e.g. > speculative reads, and prefetching permitted). These callsites can be > moved to memremap() over time. > > Cc: Arnd Bergmann > Acked-by: Andy Shevchenko > Signed-off-by: Dan Williams > --- > 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 , 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: [] dump_backtrace+0x0/0x124 [] show_stack+0x10/0x1c [] dump_stack+0x84/0xc8 [] warn_slowpath_common+0x98/0xd0 [] warn_slowpath_fmt+0x4c/0x58 [] memremap+0xac/0xbc [] arm64_enable_runtime_services+0x90/0x208 [] do_one_initcall+0x88/0x1a0 [] kernel_init_freeable+0x8c/0x1f8 [] 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.