| Message ID | 1462830111-28172-2-git-send-email-d-gerlach@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 09, 2016 at 04:41:49PM -0500, Dave Gerlach wrote: > diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c > index 66a978d05958..c6eef3c98074 100644 > --- a/arch/arm/mm/ioremap.c > +++ b/arch/arm/mm/ioremap.c > @@ -400,6 +400,20 @@ EXPORT_SYMBOL(ioremap_wc); > * clocks that would affect normal memory for example. Please see > * CONFIG_GENERIC_ALLOCATOR for allocating external memory. > */ > +void __iomem *ioremap_exec(resource_size_t res_cookie, size_t size) > +{ > + return arch_ioremap_caller(res_cookie, size, MT_MEMORY_RWX, > + __builtin_return_address(0)); > +} > +EXPORT_SYMBOL(ioremap_exec); > + > +void __iomem *ioremap_exec_nocache(resource_size_t res_cookie, size_t size) > +{ > + return arch_ioremap_caller(res_cookie, size, MT_MEMORY_RWX_NONCACHED, > + __builtin_return_address(0)); > +} > +EXPORT_SYMBOL(ioremap_exec_nocache); I think these should be called memremap_exec() and similar. Please see the description of memremap() in kernel/memremap.c. If you're going to be executing code, the region must not have I/O side effects and according to the new definition of memremap() vs ioremap(), the memremap() interfaces fit better. Please also get these reviewed by Dan Williams who provided the memremap() API. Thanks.
Hi, On 05/12/2016 11:37 AM, Russell King - ARM Linux wrote: > On Mon, May 09, 2016 at 04:41:49PM -0500, Dave Gerlach wrote: >> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c >> index 66a978d05958..c6eef3c98074 100644 >> --- a/arch/arm/mm/ioremap.c >> +++ b/arch/arm/mm/ioremap.c >> @@ -400,6 +400,20 @@ EXPORT_SYMBOL(ioremap_wc); >> * clocks that would affect normal memory for example. Please see >> * CONFIG_GENERIC_ALLOCATOR for allocating external memory. >> */ >> +void __iomem *ioremap_exec(resource_size_t res_cookie, size_t size) >> +{ >> + return arch_ioremap_caller(res_cookie, size, MT_MEMORY_RWX, >> + __builtin_return_address(0)); >> +} >> +EXPORT_SYMBOL(ioremap_exec); >> + >> +void __iomem *ioremap_exec_nocache(resource_size_t res_cookie, size_t size) >> +{ >> + return arch_ioremap_caller(res_cookie, size, MT_MEMORY_RWX_NONCACHED, >> + __builtin_return_address(0)); >> +} >> +EXPORT_SYMBOL(ioremap_exec_nocache); > > I think these should be called memremap_exec() and similar. Please > see the description of memremap() in kernel/memremap.c. If you're > going to be executing code, the region must not have I/O side effects > and according to the new definition of memremap() vs ioremap(), the > memremap() interfaces fit better. > > Please also get these reviewed by Dan Williams who provided the > memremap() API. Ok thank you for the pointer. I agree, the memremap API looks like a better fit for this. I think it likely makes the most sense to still add these ioremap_exec and ioremap_exec_nocache and then call them through the memremap API based on new flags. This will fit into the current use model for memremap as it currently uses all of the other ioremap calls internally, and doing it how I just described will let this code evolve along with memremap. I will put v2 together this way and send it out. Regards, Dave > > Thanks. >
On Wed, May 18, 2016 at 09:12:20AM -0500, Dave Gerlach wrote: > Ok thank you for the pointer. I agree, the memremap API looks like a better > fit for this. I think it likely makes the most sense to still add these > ioremap_exec and ioremap_exec_nocache and then call them through the > memremap API based on new flags. This will fit into the current use model > for memremap as it currently uses all of the other ioremap calls internally, > and doing it how I just described will let this code evolve along with > memremap. I would _really_ prefer not to do that. Why? Because IO memory does not have the required properties to be executable. IO memory is normally memory which has side effects - and by side effects, I mean reading it can provoke hardware to perform some action. You don't want to be executing from such memory. So, in my mind, ioremap_exec makes absolutely no sense, and having it gives people a new interface to abuse - and abuse they will.
On Wednesday 18 May 2016 18:51:02 Russell King - ARM Linux wrote: > On Wed, May 18, 2016 at 09:12:20AM -0500, Dave Gerlach wrote: > > Ok thank you for the pointer. I agree, the memremap API looks like a better > > fit for this. I think it likely makes the most sense to still add these > > ioremap_exec and ioremap_exec_nocache and then call them through the > > memremap API based on new flags. This will fit into the current use model > > for memremap as it currently uses all of the other ioremap calls internally, > > and doing it how I just described will let this code evolve along with > > memremap. > > I would _really_ prefer not to do that. Why? Because IO memory does > not have the required properties to be executable. IO memory is normally > memory which has side effects - and by side effects, I mean reading it > can provoke hardware to perform some action. You don't want to be > executing from such memory. > > So, in my mind, ioremap_exec makes absolutely no sense, and having it > gives people a new interface to abuse - and abuse they will. Agreed, calling it ioremap when it is really memremap makes no sense. I also see another problem in the asm-generic portion: +#ifndef ARCH_HAS_IOREMAP_EXEC +#define ioremap_exec ioremap +#define ioremap_exec_nocache ioremap_nocache +#endif The ARM version of ioremap_exec() that gets added in this patch is cached (like memremap()), but then the asm-generic version is not? This is even more confusing, it should at least do roughly the same thing across architectures. There should also be some documentation about what the expected behavior is, e.g.: - is memremap_exec() by default cached or not? (I assume it would be like memremap()) - If we have an interface that does explicit uncached executable mapping, what about architectures on which this is not possible? Should they fall back to cached or non-executable, or cause a link error? Arnd
On Wed, May 18, 2016 at 10:25:03PM +0200, Arnd Bergmann wrote: > The ARM version of ioremap_exec() that gets added in this patch is cached > (like memremap()), but then the asm-generic version is not? This is > even more confusing, it should at least do roughly the same thing across > architectures. > > There should also be some documentation about what the expected behavior is, e.g.: > > - is memremap_exec() by default cached or not? (I assume it would > be like memremap()) > - If we have an interface that does explicit uncached executable mapping, > what about architectures on which this is not possible? Should they > fall back to cached or non-executable, or cause a link error? Another important point is whether atomic instructions / kernel locks can be located within the mapped memory.
On 05/18/2016 03:57 PM, Russell King - ARM Linux wrote: > On Wed, May 18, 2016 at 10:25:03PM +0200, Arnd Bergmann wrote: >> The ARM version of ioremap_exec() that gets added in this patch is cached >> (like memremap()), but then the asm-generic version is not? This is >> even more confusing, it should at least do roughly the same thing across >> architectures. >> >> There should also be some documentation about what the expected behavior is, e.g.: >> >> - is memremap_exec() by default cached or not? (I assume it would >> be like memremap()) >> - If we have an interface that does explicit uncached executable mapping, >> what about architectures on which this is not possible? Should they >> fall back to cached or non-executable, or cause a link error? Yes by default memremap_exec is cached, I do plan to add more explicit documentation. Well, I dont think that memremap_exec will be called directly but rather using a flag with memremap as arch_memremap_wb is now, to keep the memremap API unified, so a link error will prevent this. Also, the function may be present in code but not actually used in all cases, the example that comes to mind is the drivers/misc/sram.c code where other runtime options are perfectly valid for determining how to map memory even on architectures that can't memremap_exec_nocache. I think that a remap that can't deliver what you have asked for should return NULL here because if you are requesting executable, noncached memory you presumably will try to execute from it and fail, so the mapping should fail as it isn't actually valid if it can't do what you want. > > Another important point is whether atomic instructions / kernel locks > can be located within the mapped memory. > At this point I'd imagine most of the users of this would be copying small chunks of relocatable code (likely written in assembly) that would handle low-level tasks without the need for atomic instructions/locks, but is this something we should explicitly forbid in documentation? Regards, Dave
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index 485982084fe9..7d07a02cb7bc 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -399,6 +399,11 @@ void __iomem *ioremap_wc(resource_size_t res_cookie, size_t size); #define ioremap_wc ioremap_wc #define ioremap_wt ioremap_wc +void __iomem *ioremap_exec(resource_size_t res_cookie, size_t size); +void __iomem *ioremap_exec_nocache(resource_size_t res_cookie, size_t size); +#define ioremap_exec ioremap_exec +#define ioremap_exec_nocache ioremap_exec_nocache + void iounmap(volatile void __iomem *iomem_cookie); #define iounmap iounmap diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c index 66a978d05958..c6eef3c98074 100644 --- a/arch/arm/mm/ioremap.c +++ b/arch/arm/mm/ioremap.c @@ -400,6 +400,20 @@ EXPORT_SYMBOL(ioremap_wc); * clocks that would affect normal memory for example. Please see * CONFIG_GENERIC_ALLOCATOR for allocating external memory. */ +void __iomem *ioremap_exec(resource_size_t res_cookie, size_t size) +{ + return arch_ioremap_caller(res_cookie, size, MT_MEMORY_RWX, + __builtin_return_address(0)); +} +EXPORT_SYMBOL(ioremap_exec); + +void __iomem *ioremap_exec_nocache(resource_size_t res_cookie, size_t size) +{ + return arch_ioremap_caller(res_cookie, size, MT_MEMORY_RWX_NONCACHED, + __builtin_return_address(0)); +} +EXPORT_SYMBOL(ioremap_exec_nocache); + void __iomem * __arm_ioremap_exec(phys_addr_t phys_addr, size_t size, bool cached) { diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c index 1dd10936d68d..1d1d587340e6 100644 --- a/arch/arm/mm/nommu.c +++ b/arch/arm/mm/nommu.c @@ -380,6 +380,20 @@ void __iomem *ioremap_wc(resource_size_t res_cookie, size_t size) } EXPORT_SYMBOL(ioremap_wc); +void __iomem *ioremap_exec(resource_size_t res_cookie, size_t size) +{ + return __arm_ioremap_caller(res_cookie, size, 0, + __builtin_return_address(0)); +} +EXPORT_SYMBOL(ioremap_exec); + +void __iomem *ioremap_exec_nocache(resource_size_t res_cookie, size_t size) +{ + return __arm_ioremap_caller(res_cookie, size, 0, + __builtin_return_address(0)); +} +EXPORT_SYMBOL(ioremap_exec_nocache); + void __iounmap(volatile void __iomem *addr) { } diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h index d8f8622fa044..a789aad509c3 100644 --- a/include/asm-generic/iomap.h +++ b/include/asm-generic/iomap.h @@ -70,6 +70,11 @@ extern void ioport_unmap(void __iomem *); #define ioremap_wt ioremap_nocache #endif +#ifndef ARCH_HAS_IOREMAP_EXEC +#define ioremap_exec ioremap +#define ioremap_exec_nocache ioremap_nocache +#endif + #ifdef CONFIG_PCI /* Destroy a virtual mapping cookie for a PCI BAR (memory or IO) */ struct pci_dev;