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

[PATCH v2 0/1] 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.

This patch provides a fncpy() macro which resembles memcpy().
It can be used as in this example:

    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 = fncpy(scary_memory_buf,
            &scary_function, 
            size_of_scary_function);

This is quite a lot more readable than the explicit code,
and should give the correct result.

fncpy() calls flush_icache_range() as necessary.

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.

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

 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(-)

*** BLURB HERE ***

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

 arch/arm/include/asm/fncpy.h |  111 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/fncpy.h

[PATCH 1/1 v2] 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.

Thanks to Russell King for his input on this patch.

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

 arch/arm/include/asm/fncpy.h |  111 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/fncpy.h

diff --git a/arch/arm/include/asm/fncpy.h b/arch/arm/include/asm/fncpy.h
new file mode 100644
index 0000000..a155fb7
--- /dev/null
+++ b/arch/arm/include/asm/fncpy.h
@@ -0,0 +1,111 @@
+/*
+ * arch/arm/include/asm/fncpy.h - helper macros for function body copying
+ *
+ * Copyright (C) 2011 Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+/*
+ * These macros are intended for use when there is a need to copy a low-level
+ * function body into special memory.
+ *
+ * For example, when reconfiguring the SDRAM controller, the code doing the
+ * reconfiguration may need to run from SRAM.
+ *
+ * NOTE: that the copied function body must be entirely self-contained and
+ * position-independent in order for this to work properly.
+ *
+ *
+ * Typical usage example:
+ *
+ * extern int f(args);
+ * extern uint32_t size_of_f;
+ * int (*copied_f)(args);
+ * void *sram_buffer;
+ *
+ * copied_f = fncpy(sram_buffer, &f, size_of_f);
+ *
+ * ... do any required D-side/I-side synchronisation ...
+ *
+ * ... later, call the function: ...
+ *
+ * copied_f(args);
+ *
+ * The size of the function to be copied can't be determined from C:
+ * this must be determined by other means, such as adding assmbler directives
+ * in the file where f is defined.
+ */
+
+ */
+
+#ifndef __ASM_FNCPY_H
+#define __ASM_FNCPY_H
+
+#include <linux/types.h>
+#include <linux/string.h>
+
+#include <asm/cacheflush.h>
+
+/* Function pointer casting macros */
+
+/* Cast function pointer to integer: */
+#define __funcp_to_uint(funcp) ({				\
+		uintptr_t __result;				\
+								\
+		asm("" : "=r" (__result) : "0" (funcp));	\
+		__result;					\
+	})
+
+/* Cast integer to function pointer with type matching funcp: */
+#define __uint_to_funcp(i, funcp) ({			\
+		typeof(funcp) __result;			\
+							\
+		asm("" : "=r" (__result) : "0" (i));	\
+		__result;				\
+	})
+
+
+/* Function symbol manipulation macros */
+
+/*
+ * FSYM_REBASE: Determine the correct function pointer for funcp,
+ * after the function has been copied to dest_buf:
+ */
+#define FSYM_REBASE(funcp, dest_buf)					\
+	__uint_to_funcp((uintptr_t)(dest_buf) | FSYM_TYPE(funcp), funcp)
+
+/*
+ * FSYM_BASE: Determine the base address in memory of the function funcp
+ * FSYM_TYPE: Determine the instruction set type (ARM/Thumb) of funcp
+ * (both defined below)
+ */
+
+#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 */
+
+/* Function copy helper */
+#define fncpy(dest_buf, funcp, size) ({					\
+		memcpy(dest_buf, FSYM_BASE(funcp), size);		\
+		flush_icache_range(dest_buf, (char *)(dest_buf) + size); \
+									\
+		FSYM_REBASE(funcp, dest_buf);				\
+	})
+
+#endif /* !__ASM_FNCPY_H */
-- 
1.7.1

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

[Russell King - ARM Linux]

On Thu, Jan 13, 2011 at 02:51:45PM -0600, Dave Martin wrote:
> +/* Cast function pointer to integer: */
> +#define __funcp_to_uint(funcp) ({				\

uint is confusing here - it suggests casting a pointer to an unsigned int,
rather than a uintptr_t.  Please use uintptr here.

> +/*
> + * FSYM_REBASE: Determine the correct function pointer for funcp,
> + * after the function has been copied to dest_buf:
> + */
> +#define FSYM_REBASE(funcp, dest_buf)					\
> +	__uint_to_funcp((uintptr_t)(dest_buf) | FSYM_TYPE(funcp), funcp)
> +
> +/*
> + * FSYM_BASE: Determine the base address in memory of the function funcp
> + * FSYM_TYPE: Determine the instruction set type (ARM/Thumb) of funcp
> + * (both defined below)
> + */
> +
> +#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 */

I'd really like to see these gone - otherwise they'll end up being used
in code inappropriately.  I like things to be kept as simple as possible
with as few opportunities for people to needlessly hook into internal
implementation details.

If you expose implementation details, people will use them, and then if
you need to change the implementation, you've got a lot of code to deal
with.

I don't think we need to make this conditional on THUMB2 either - we're
probably not wasting much by always clearing and copying the LSB.  And
this isn't particularly performance code.

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

[Dave Martin]

On Thu, Jan 13, 2011 at 5:55 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 13, 2011 at 02:51:45PM -0600, Dave Martin wrote:
>> +/* Cast function pointer to integer: */
>> +#define __funcp_to_uint(funcp) ({ ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>
> uint is confusing here - it suggests casting a pointer to an unsigned int,
> rather than a uintptr_t. ?Please use uintptr here.
>
>> +/*
>> + * FSYM_REBASE: Determine the correct function pointer for funcp,
>> + * after the function has been copied to dest_buf:
>> + */
>> +#define FSYM_REBASE(funcp, dest_buf) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>> + ? ? __uint_to_funcp((uintptr_t)(dest_buf) | FSYM_TYPE(funcp), funcp)
>> +
>> +/*
>> + * FSYM_BASE: Determine the base address in memory of the function funcp
>> + * FSYM_TYPE: Determine the instruction set type (ARM/Thumb) of funcp
>> + * (both defined below)
>> + */
>> +
>> +#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 */
>
> I'd really like to see these gone - otherwise they'll end up being used
> in code inappropriately. ?I like things to be kept as simple as possible
> with as few opportunities for people to needlessly hook into internal
> implementation details.
>
> If you expose implementation details, people will use them, and then if
> you need to change the implementation, you've got a lot of code to deal
> with.

I guess I agree with that now ... with fncpy() implemented, there's
little legitimate use for the other macros, including the casting
macros.

I'll fold it all into fncpy() and see how that looks.

>
> I don't think we need to make this conditional on THUMB2 either - we're
> probably not wasting much by always clearing and copying the LSB. ?And
> this isn't particularly performance code.
>

Agreed.  I originally tried to avoid impacting the ARM case, but that
adds complexity for little benefit.

Cheers
---Dave

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

[Dave Martin]

Hi again,

There's another problem which I hadn't spotted before:

In the Thumb case, we risk violating the alignment constraints of the
code which gets copied (actually, this is also true of the ARM case,
but less likely to bite).  This matters because the code may contain
literals and other data words -- quite likely given the requirement
for self-containedness.

In the simplest case, we could just require the caller to ensure that
there are is at least size+2 bytes of space at dest_buf: then we can
actually copy the function to dest_buf+2 if this is needed to preserve
word alignment.  But this does make the API harder to use.  For
Thumb-2, at least word alignment is always needed, even if the start
address of the function is only halfword-aligned, because of the way
literals are addressed.

There could also be a need for alignment greater than word alignment
in some cases; for example, where ldrd/strd or some hardware operation
needs an aligned buffer.  This would be rarer though; perhaps we could
get away without it.

We could provide a helper to determine how much space to request from
the allocator:

dest_size = fncpy_size(function, size_of_function, alignment);
dest_buf = allocate(dest_size);
fncpy(dest_buf, &function, size_of_function, alignment);

If we don't know what alignment guarantee is provided by the allocator
itself, we might have to define this conservatively, e.g.:

size_t fncpy_size(function, size_of_function, alignment)
{
    return size_of_function + alignment - 1;
}

...to guarantee the possibility of rounding the copied function
address up far enough to achieve the required alignment without
overrunning the buffer.  Of course, we can probably get simpler that
that...

Any thoughts?

Cheers
---Dave

On Fri, Jan 14, 2011 at 9:42 AM, Dave Martin <dave.martin@linaro.org> wrote:
> On Thu, Jan 13, 2011 at 5:55 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Jan 13, 2011 at 02:51:45PM -0600, Dave Martin wrote:
>>> +/* Cast function pointer to integer: */
>>> +#define __funcp_to_uint(funcp) ({ ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
>>
>> uint is confusing here - it suggests casting a pointer to an unsigned int,
>> rather than a uintptr_t. ?Please use uintptr here.
>>
>>> +/*
>>> + * FSYM_REBASE: Determine the correct function pointer for funcp,
>>> + * after the function has been copied to dest_buf:
>>> + */
>>> +#define FSYM_REBASE(funcp, dest_buf) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
>>> + ? ? __uint_to_funcp((uintptr_t)(dest_buf) | FSYM_TYPE(funcp), funcp)
>>> +
>>> +/*
>>> + * FSYM_BASE: Determine the base address in memory of the function funcp
>>> + * FSYM_TYPE: Determine the instruction set type (ARM/Thumb) of funcp
>>> + * (both defined below)
>>> + */
>>> +
>>> +#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 */
>>
>> I'd really like to see these gone - otherwise they'll end up being used
>> in code inappropriately. ?I like things to be kept as simple as possible
>> with as few opportunities for people to needlessly hook into internal
>> implementation details.
>>
>> If you expose implementation details, people will use them, and then if
>> you need to change the implementation, you've got a lot of code to deal
>> with.
>
> I guess I agree with that now ... with fncpy() implemented, there's
> little legitimate use for the other macros, including the casting
> macros.
>
> I'll fold it all into fncpy() and see how that looks.
>
>>
>> I don't think we need to make this conditional on THUMB2 either - we're
>> probably not wasting much by always clearing and copying the LSB. ?And
>> this isn't particularly performance code.
>>
>
> Agreed. ?I originally tried to avoid impacting the ARM case, but that
> adds complexity for little benefit.
>
> Cheers
> ---Dave
>

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

[Russell King - ARM Linux]

On Fri, Jan 14, 2011 at 11:15:19AM -0600, Dave Martin wrote:
> There's another problem which I hadn't spotted before:
> 
> In the Thumb case, we risk violating the alignment constraints of the
> code which gets copied (actually, this is also true of the ARM case,
> but less likely to bite).  This matters because the code may contain
> literals and other data words -- quite likely given the requirement
> for self-containedness.

There's a solution to this - require that the thumb function is
preceded by a .align 3 (which according to the GAS documentation I
have means for ARM, it aligns the PC to 1 << 3 not 3 bytes.)

Also require 8-byte alignment from the allocation function, and make
fncpy() bug if the destination isn't 8-byte aligned.  Same for the
source function argument (but ignoring bit 0 of course.)

We don't then have to mess with rounding allocation sizes up, nor worry
about aligning the destination according to the source or any other
games like that.

The down-side is wastage of maybe 7 bytes a function, but that's
probably going to happen anyway.

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

[Dave Martin]

On Fri, Jan 14, 2011 at 11:43 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 14, 2011 at 11:15:19AM -0600, Dave Martin wrote:
>> There's another problem which I hadn't spotted before:
>>
>> In the Thumb case, we risk violating the alignment constraints of the
>> code which gets copied (actually, this is also true of the ARM case,
>> but less likely to bite). ?This matters because the code may contain
>> literals and other data words -- quite likely given the requirement
>> for self-containedness.
>
> There's a solution to this - require that the thumb function is
> preceded by a .align 3 (which according to the GAS documentation I
> have means for ARM, it aligns the PC to 1 << 3 not 3 bytes.)
>
> Also require 8-byte alignment from the allocation function, and make
> fncpy() bug if the destination isn't 8-byte aligned. ?Same for the
> source function argument (but ignoring bit 0 of course.)
>
> We don't then have to mess with rounding allocation sizes up, nor worry
> about aligning the destination according to the source or any other
> games like that.
>
> The down-side is wastage of maybe 7 bytes a function, but that's
> probably going to happen anyway.
>

OK, that sounds like a practical compromise.

I'll roll this in and repost the patch.

Cheers
---Dave