Re: [PATCH 2/2] memory: ti-emif-sram: introduce relocatable suspend/resume handlers

[Dave Gerlach]

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@xxxxxxxxxxxxxxx> [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