[PATCH 0/0] RFC: ARM: Thumb-2: Symbol manipulation macros for function body copying

[PATCH 0/0] RFC: ARM: Thumb-2: Symbol manipulation macros for function body copying

[Dave Martin]

For at least one board (omap3), some functions are copied from
their link-time location into other memory at run-time.

This is a plausible thing to do if, for example, the board
might need to do something like manipulating the SDRAM 
controller configuration during power management operations.
Such code may not be able to execute from the SDRAM itself.


In Thumb-2, copying function bodies is not straightforward:
for Thumb symbols, bit 0 is set by the toolchain, and so
a function symbol can't be used directly as a base address
for memcpy: this leads to an off-by-one error, resulting in
garbage instructions in the destination buffer.


The obvious solution is to mask off this bit when calling
memcpy() and then insert the bit into the address of the
target buffer, in order to derive a pointer which can be
used to call the copied function in the correct instruction
set.  However, in practice the compiler may optimise this
operation away.  This seems wrong, but having discussed this
with compiler folks I believe it's not a compiler bug: rather,
C doesn't specifiy what happens when casting function pointers
and attempting to do arithmetic on them.  So some surprising
optimisations can happen.


To make it easier to deal with cases like this, I've had a
go at writing some macros to make copying function bodies
easier, while being robust for ARM and Thumb-2.

In particular, the required type-casts are implemented as
empty asm() blocks, to ensure that the compiler makes no
assumptions about the result.

These macros just help with the address manipulations: e.g.:

    extern int scary_function(int a, char *b);
    extern const int size_of_scary_function;
    extern void *scary_memory_buf;

    int (*runtime_scary_function)(int a, char *b);

    runtime_scary_function = FSYM_REBASE(
        scary_function, 
	memcpy(scary_memory_buf, FSYM_BASE(scary_function),
            size_of_scary_function));

This is quite a lot more readable than the explicit code,
and gives the correct result (modulo bugs in my patch...)

It's still necessary to do the appropriate cache-flushing
before runtime_scary_function is actually called.  Also,
it's not possible to determine the size of a function from
C code.  This must be done by other means, such as adding
extra symbols in the assembler code where scary_function is
defined.


I can't comment on how widespread the requirement for function
body copying is in the arch/arm/ tree, however.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
KernelVersion: next-20110111

 arch/arm/include/asm/unified.h |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/unified.h b/arch/arm/include/asm/unified.h
index bc63116..636a765 100644
--- a/arch/arm/include/asm/unified.h
+++ b/arch/arm/include/asm/unified.h
@@ -24,6 +24,32 @@
 	.syntax unified
 #endif
 
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+#define __funcp_to_uint(funcp) ({				\
+		uintptr_t __result;				\
+								\
+		asm("" : "=r" (__result) : "0" (funcp));	\
+		__result;					\
+	})
+#define __uint_to_funcp(i, funcp) ({			\
+		typeof(funcp) __result;			\
+							\
+		asm("" : "=r" (__result) : "0" (i));	\
+		__result;				\
+	})
+#define FSYM_REBASE(funcp, dest_buf)					\
+	__uint_to_funcp((uintptr_t)(dest_buf) | FSYM_TYPE(funcp), funcp)
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define FSYM_BASE(funcp) ((void *)(__funcp_to_uint(funcp) & ~(uintptr_t)1))
+#define FSYM_TYPE(funcp) (__funcp_to_uint(funcp) & 1)
+#else /* !CONFIG_THUMB2_KERNEL */
+#define FSYM_BASE(funcp) ((void *)__funcp_to_uint(funcp))
+#define FSYM_TYPE(funcp) 0
+#endif /* !CONFIG_THUMB2_KERNEL */
+#endif /* !__ASSEMBLY__ */
+
 #ifdef CONFIG_THUMB2_KERNEL
 
 #if __GNUC__ < 4
-- 
1.7.1

*** BLURB HERE ***

Dave Martin (1):
  ARM: Thumb-2: Symbol manipulation macros for function body copying

 arch/arm/include/asm/unified.h |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

[PATCH 1/1] ARM: Thumb-2: Symbol manipulation macros for function body copying

[Dave Martin]

In low-level board support code, there is sometimes a need to
copy a function body to another location at run-time.

A straightforward call to memcpy doesn't work in Thumb-2,
because bit 0 of external Thumb function symbols is set to 1,
indicating that the function is Thumb.  Without corrective
measures, this will cause an off-by-one copy, and the copy
may be called using the wrong instruction set.

This patch adds macros to help with such cases.

Particular care is needed, because C doesn't guarantee any
defined behaviour when casting a function pointer to any other
type.  This has been observed to lead to strange optimisation
side-effects when doing the arithmetic which is required in
order to copy/move function bodies correctly in Thumb-2.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
KernelVersion: next-20110111

 arch/arm/include/asm/unified.h |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/unified.h b/arch/arm/include/asm/unified.h
index bc63116..636a765 100644
--- a/arch/arm/include/asm/unified.h
+++ b/arch/arm/include/asm/unified.h
@@ -24,6 +24,32 @@
 	.syntax unified
 #endif
 
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+#define __funcp_to_uint(funcp) ({				\
+		uintptr_t __result;				\
+								\
+		asm("" : "=r" (__result) : "0" (funcp));	\
+		__result;					\
+	})
+#define __uint_to_funcp(i, funcp) ({			\
+		typeof(funcp) __result;			\
+							\
+		asm("" : "=r" (__result) : "0" (i));	\
+		__result;				\
+	})
+#define FSYM_REBASE(funcp, dest_buf)					\
+	__uint_to_funcp((uintptr_t)(dest_buf) | FSYM_TYPE(funcp), funcp)
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define FSYM_BASE(funcp) ((void *)(__funcp_to_uint(funcp) & ~(uintptr_t)1))
+#define FSYM_TYPE(funcp) (__funcp_to_uint(funcp) & 1)
+#else /* !CONFIG_THUMB2_KERNEL */
+#define FSYM_BASE(funcp) ((void *)__funcp_to_uint(funcp))
+#define FSYM_TYPE(funcp) 0
+#endif /* !CONFIG_THUMB2_KERNEL */
+#endif /* !__ASSEMBLY__ */
+
 #ifdef CONFIG_THUMB2_KERNEL
 
 #if __GNUC__ < 4
-- 
1.7.1

[PATCH 0/0] RFC: ARM: Thumb-2: Symbol manipulation macros for function body copying

[Russell King - ARM Linux]

On Tue, Jan 11, 2011 at 06:02:30PM -0600, Dave Martin wrote:
> To make it easier to deal with cases like this, I've had a
> go at writing some macros to make copying function bodies
> easier, while being robust for ARM and Thumb-2.

How about instead providing some infrastructure which coes the
copy too?  Something like:

#define copy_fn_to_sram(to, fn, size) ({		\
	__typeof__(fn) f;				\
	unsigned long ptr;				\
	__asm__("" : "=r" (ptr) : "0" (fn));		\
	memcpy(to, (void *)(ptr & ~1), size);		\
	ptr = (ptr & 1) | (unsigned long)to;		\
	__asm__("" : "=r" (f) : "0" (ptr));		\
	f;						\
})

Used by:
extern void my_func(int foo);
extern int my_func_size;

void call_my_func(void *to, int arg)
{
	void (*fn)(int);

	fn = copy_fn_to_sram(to, my_func, my_func_size);
	fn(arg);
}

Then if you need to fix the way the copies are done for some
architectural reason, there's only one place to do it.

I'm not sure asm/unified.h is the right place - I don't think this has
anything to do with the unified assembler syntax.  Please create a new
header for this.

[PATCH 0/0] RFC: ARM: Thumb-2: Symbol manipulation macros for function body copying

[Dave Martin]

Hi,

On Wed, Jan 12, 2011 at 3:32 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jan 11, 2011 at 06:02:30PM -0600, Dave Martin wrote:
>> To make it easier to deal with cases like this, I've had a
>> go at writing some macros to make copying function bodies
>> easier, while being robust for ARM and Thumb-2.
>
> How about instead providing some infrastructure which coes the
> copy too? ?Something like:
>
> #define copy_fn_to_sram(to, fn, size) ({ ? ? ? ? ? ? ? ?\
> ? ? ? ?__typeof__(fn) f; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? ? ?unsigned long ptr; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? ?__asm__("" : "=r" (ptr) : "0" (fn)); ? ? ? ? ? ?\
> ? ? ? ?memcpy(to, (void *)(ptr & ~1), size); ? ? ? ? ? \
> ? ? ? ?ptr = (ptr & 1) | (unsigned long)to; ? ? ? ? ? ?\
> ? ? ? ?__asm__("" : "=r" (f) : "0" (ptr)); ? ? ? ? ? ? \
> ? ? ? ?f; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> })

omap provides some infrastructure for both allocating SRAM space and
doing the copy, using omap_sram_push() and friends.  So I wasn't sure
what the correct level of abstraction was for the new helpers.
Certainly, providing a sort of "function memcpy" macro like your
copy_fn_to_sram makes sense.  I think this should still be safe from a
type system perspective: providing the "blind" type casts using asm()
appear somewhere in the execution flow C shouldn't make silly
assumptions even if Linux ends up enabling multifile optimisation
sometime in the future.

Would you agree with that assertion?

>
> Used by:
> extern void my_func(int foo);
> extern int my_func_size;

Potentially, we could define, an extra assembler macro to complement
ENDPROC() which records the size of a function automatically.  What do
you think?

>
> void call_my_func(void *to, int arg)
> {
> ? ? ? ?void (*fn)(int);
>
> ? ? ? ?fn = copy_fn_to_sram(to, my_func, my_func_size);
> ? ? ? ?fn(arg);
> }
>
> Then if you need to fix the way the copies are done for some
> architectural reason, there's only one place to do it.

The model used in the omap code is to copy some functions into SRAM
ahead of time and stash the pointers away to be called later: for that
model, it's not so useful to have something like call_my_func
directly.  Also, I wasn't sure whether conflating other functionality
such as cache flushing into the new macros would be a good idea -- is
might be cleaner and more maintainable, but might result in less
efficient usage.  Any thoughts?

>
> I'm not sure asm/unified.h is the right place - I don't think this has
> anything to do with the unified assembler syntax. ?Please create a new
> header for this.

I suspected unified.h might not be right--- thanks for the feedback.

Cheers
---Dave

[PATCH 0/0] RFC: ARM: Thumb-2: Symbol manipulation macros for function body copying

[Russell King - ARM Linux]

On Wed, Jan 12, 2011 at 10:00:25AM -0600, Dave Martin wrote:
> omap provides some infrastructure for both allocating SRAM space and
> doing the copy, using omap_sram_push() and friends.  So I wasn't sure
> what the correct level of abstraction was for the new helpers.
> Certainly, providing a sort of "function memcpy" macro like your
> copy_fn_to_sram makes sense.

It'd just be a matter of splitting the copying out of omap_sram_push().

> I think this should still be safe from a type system perspective:
> providing the "blind" type casts using asm() appear somewhere in
> the execution flow C shouldn't make silly assumptions even if Linux
> ends up enabling multifile optimisation sometime in the future.

Yes.  The only thing that is missing from my version is the
flush_icache_range() which should also be there.

> > Used by:
> > extern void my_func(int foo);
> > extern int my_func_size;
> 
> Potentially, we could define, an extra assembler macro to complement
> ENDPROC() which records the size of a function automatically.  What do
> you think?

That would pad the code out with a fair number of additional integers.
That's probably not a good idea.

> The model used in the omap code is to copy some functions into SRAM
> ahead of time and stash the pointers away to be called later: for that
> model, it's not so useful to have something like call_my_func
> directly.  Also, I wasn't sure whether conflating other functionality
> such as cache flushing into the new macros would be a good idea -- is
> might be cleaner and more maintainable, but might result in less
> efficient usage.  Any thoughts?

My example was only that - an example.  You can also use it in the
way you describe too:

	to = omap_sram_push(size);
	_omap_sram_reprogram_clock = copy_fn_to_sram(to,
					omap1_sram_reprogram_clock, size);

and it'll also ensure type-safety between the omap1_sram_reprogram_clock
and _omap_sram_reprogram_clock symbols, which the current code doesn't
do.

[PATCH 0/0] RFC: ARM: Thumb-2: Symbol manipulation macros for function body copying

[Dave Martin]

On Wed, Jan 12, 2011 at 10:11 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jan 12, 2011 at 10:00:25AM -0600, Dave Martin wrote:
>> omap provides some infrastructure for both allocating SRAM space and
>> doing the copy, using omap_sram_push() and friends. ?So I wasn't sure
>> what the correct level of abstraction was for the new helpers.
>> Certainly, providing a sort of "function memcpy" macro like your
>> copy_fn_to_sram makes sense.
>
> It'd just be a matter of splitting the copying out of omap_sram_push().
>
>> I think this should still be safe from a type system perspective:
>> providing the "blind" type casts using asm() appear somewhere in
>> the execution flow C shouldn't make silly assumptions even if Linux
>> ends up enabling multifile optimisation sometime in the future.
>
> Yes. ?The only thing that is missing from my version is the
> flush_icache_range() which should also be there.
>
>> > Used by:
>> > extern void my_func(int foo);
>> > extern int my_func_size;
>>
>> Potentially, we could define, an extra assembler macro to complement
>> ENDPROC() which records the size of a function automatically. ?What do
>> you think?

Sure -- we shouldn't change ENDPROC() itself, but we could have, say,
and ENDPROC_SZ() macro which people should use strictly when they know
they need it.

>
> That would pad the code out with a fair number of additional integers.
> That's probably not a good idea.
>
>> The model used in the omap code is to copy some functions into SRAM
>> ahead of time and stash the pointers away to be called later: for that
>> model, it's not so useful to have something like call_my_func
>> directly. ?Also, I wasn't sure whether conflating other functionality
>> such as cache flushing into the new macros would be a good idea -- is
>> might be cleaner and more maintainable, but might result in less
>> efficient usage. ?Any thoughts?
>
> My example was only that - an example. ?You can also use it in the
> way you describe too:
>
> ? ? ? ?to = omap_sram_push(size);
> ? ? ? ?_omap_sram_reprogram_clock = copy_fn_to_sram(to,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?omap1_sram_reprogram_clock, size);
>
> and it'll also ensure type-safety between the omap1_sram_reprogram_clock
> and _omap_sram_reprogram_clock symbols, which the current code doesn't
> do.
>

Ah, OK -- I'd interpreted call_my_func() as part of the API, rather
than a usage example.

I'll have a think and update the patch.

Cheers
---Dave