Messages in this thread | | | Subject | Re: [PATCH 2/2] memory: ti-emif-sram: introduce relocatable suspend/resume handlers | From | Dave Gerlach <> | Date | Thu, 6 Apr 2017 14:09:16 -0500 |
| |
On 04/06/2017 02:00 PM, Russell King - ARM Linux wrote: > On Wed, Apr 05, 2017 at 09:48:26AM -0500, Dave Gerlach wrote: >> Russell, >> On 04/05/2017 09:33 AM, Tony Lindgren wrote: >>> * Russell King - ARM Linux <linux@armlinux.org.uk> [170405 07:02]: >>>> I'm not going to comment on this yet, but I'll instead comment on the >>>> newly appeared sram_exec_copy() stuff. >>>> >>>> So, a few years ago, we went to significant effort in ARM land to come >>>> up with a way to _safely_ copy assembler from the kernel into SRAM, >>>> because copying code to SRAM that is compiled in thumb mode and then >>>> executing it is _not_ as simple as memcpy(), cast the pointer to a >>>> function pointer, and then call the function pointer. >>>> >>>> The SRAM stuff throws all that out, instead preferring the dumb memcpy() >>>> approach. >>>> >>>> This needs resolving, and I'd like to see it resolved to the satisfaction >>>> of architecture maintainers before we progress any further down this >>>> route. >> >> I'm sure you are referring to fncpy, correct? This is what we used before >> with ARM specific code to do the copy, but we've moved into drivers now. > > Right, and as I explained above, fncpy() exists with very good reason. > The following does not work on ARM: > > sram = alloc(function_size); > > memcpy(sram, function, function_size); > > sram_ptr = (function_cast_t)sram; > > sram_ptr(args); > > when the function is Thumb. There are two problems with the above code > that fncpy() solves, both stemming from the same root cause: > > 1. The address of "function" will be offset by one byte, so the memcpy() > will miss copying the first byte of the function. > > 2. sram_ptr will not be offset by one byte. > > This is because, with Thumb functions, the "address" of the function is > offset by one byte - by the architecture requirements - to indicate that > it is to be called in Thumb mode.
Thanks for the explanation.
> >> What are your thoughts on exposing fncpy outside of arch/arm? > > You may use it by including asm/fncpy.h, but you may not move it out of > that file. fncpy() is there exactly because it's _architecture_ specific.
Yes agreed, and I already include asm/cacheflush.h for ARM arch to make use of the set_memory_* APIs you helped extend.
> > If you're looking to make this generic, then we need cross-arch agreement > on how we can copy functions, and I'd recommend that fncpy() becomes that > generic copy function. fncpy() has advantages over memcpy() besides > encoding the architecture specific knowledge - the biggest one is that > it guarantees type safety as well. It ensures that the function pointer > that it's returning conforms with the function it's being asked to copy.
I agree with this, I sent a patch yesterday here [1] that uses fncpy instead of memcpy. I also laid out the constraint that an arch will just need to define fncpy that guarantees safe copy of a function in order to make use of the sram_exec functionality. It is already only selected for CONFIG_ARM in Kconfig because of the dependency on set_memory_* APIs.
[1] http://www.spinics.net/lists/linux-omap/msg136517.html
> > It strikes me, looking at the SRAM stuff, that the baby has been > completely thrown out with the bath water... > > And really, this SRAM stuff _should_ have been through architecture > maintainer review before being merged into mainline so that these issues > could have been highlighted before hand. > > This looks to me like yet another huge big review failure in kernel land, > because people are insistant on continually dividing stuff up by > sub-directory. This has got to stop. >
This just seemed like a logical place for me to add this as executable SRAM is a shared resource and if the generic sram driver is handling allocation of memory regions it should handle the page attributes of these regions as well.
Regards, Dave
| |