[PATCH 2/2] memory: ti-emif-sram: introduce relocatable suspend/resume handlers
d-gerlach at ti.com
Thu Apr 6 12:09:16 PDT 2017
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:
>> On 04/05/2017 09:33 AM, Tony Lindgren wrote:
>>> * Russell King - ARM Linux <linux at 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()
>>>> This needs resolving, and I'd like to see it resolved to the satisfaction
>>>> of architecture maintainers before we progress any further down this
>> 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;
> 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  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.
> 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.
More information about the linux-arm-kernel