[U-Boot] [PATCH] Save/restore global data pointer on API boundary
Most architectures keep the global data pointer (gd) in a register. When using the external app API, because they are calling us rather than we calling them, this register can be corrupted.
The attached (trivial) patch saves the gd pointer at api_init(), and restores it on every entry to syscall(). This may want to be put behind an ifdef for those architectures that don't use a dedicated register.
Signed-off-by: Leif Lindholm leif.lindholm@arm.com --- diff --git a/api/api.c b/api/api.c index a3bf60a..b911270 100644 --- a/api/api.c +++ b/api/api.c @@ -33,6 +33,8 @@
#include "api_private.h"
+DECLARE_GLOBAL_DATA_PTR; + #define DEBUG #undef DEBUG
@@ -600,6 +602,13 @@ static int API_display_clear(va_list ap) static cfp_t calls_table[API_MAXCALL] = { NULL, };
/* + * The global data pointer is held in a register on most if not all + * architectures. Its value is not retained across the API boundary, + * so must be manually restored. + */ +static void volatile *gd_backup; + +/* * The main syscall entry point - this is not reentrant, only one call is * serviced until finished. * @@ -620,6 +629,8 @@ int syscall(int call, int *retval, ...) va_list ap; int rv;
+ gd = gd_backup; + if (call < 0 || call >= calls_no) { debugf("invalid call #%d\n", call); return 0; @@ -686,6 +697,7 @@ void api_init(void) sig->checksum = crc32(0, (unsigned char *)sig, sizeof(struct api_signature)); debugf("syscall entry: 0x%08x\n", sig->syscall); + gd_backup = gd; }
void platform_set_mr(struct sys_info *si, unsigned long start, unsigned long size,
Dear Leif Lindholm,
In message 50214A38.3000305@arm.com you wrote:
Most architectures keep the global data pointer (gd) in a register.
This may, or may not be. You should not make any assumptions on how gd is implemented.
When using the external app API, because they are calling us rather than we calling them, this register can be corrupted.
How can this be? The caller should always use the same register convention as we do - otherwise we are in a much deeper trouble. It is up to the caller to make sure it uses the published API (resp. ABI).
The attached (trivial) patch saves the gd pointer at api_init(), and restores it on every entry to syscall(). This may want to be put behind an ifdef for those architectures that don't use a dedicated register.
This is wrong. You make assumptions here why may be correct, but may be wrong as well. If we were to implement this, you would have to make sure it always works, no matter how gd is implemented on a specific board / platform. But I doubt this is the right approach. This should be done by the caller.
BTW: Please also note that there are a few coding style errors. Checkpatch says:
WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt WARNING: please, no spaces at the start of a line (2 x)
Best regards,
Wolfgang Denk
On 08/07/12 19:30, Wolfgang Denk wrote:
Most architectures keep the global data pointer (gd) in a register.
This may, or may not be. You should not make any assumptions on how gd is implemented.
The comment did, the code didn't, as long as gd was a pointer (which should be a safe assumption). But that's irrelevant as I seem to have gotten the wrong end of the stick.
When using the external app API, because they are calling us rather than we calling them, this register can be corrupted.
How can this be? The caller should always use the same register convention as we do - otherwise we are in a much deeper trouble. It is up to the caller to make sure it uses the published API (resp. ABI).
Hmm, ok - this was not clear to me from the docs and example.
Indeed, the example does not reserve r8 (on ARM) or r2 (on PPC). Nor does it save/restore it on entry/syscall. I don't know the exact ABI semantics of r2 for PPC, but on ARM this is an error.
This could be worked around by something like:
diff --git a/examples/api/crt0.S b/examples/api/crt0.S index 6daf127..5f956e4 100644 --- a/examples/api/crt0.S +++ b/examples/api/crt0.S @@ -49,13 +49,21 @@ syscall: _start: ldr ip, =search_hint str sp, [ip] + ldr ip, =gd_backup + str r8, [ip] b main
.globl syscall syscall: + push {r6-r8, lr} + ldr r6, =gd_backup + ldr r8, [r6] ldr ip, =syscall_ptr + mov lr, pc ldr pc, [ip] + str r8, [r6] + pop {r6-r8, pc}
#else #error No support for this arch! @@ -69,3 +77,7 @@ syscall_ptr: .globl search_hint search_hint: .long 0 + + .globl gd_backup +gd_backup: + .long 0
An alternative would be to mandate DECLARE_GLOBAL_DATA_PTR for all API applications, but I'd frankly prefer not providing direct access to gd.
Best Regards,
Leif
participants (2)
-
Leif Lindholm -
Wolfgang Denk