[U-Boot] [PATCH] Save/restore global data pointer on API boundary

[U-Boot] [PATCH] Save/restore global data pointer on API boundary

[Leif Lindholm]

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,

[U-Boot] [PATCH] Save/restore global data pointer on API boundary

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If the odds are a million to one against something occuring,  chances
are 50-50 it will.

[U-Boot] [PATCH] Save/restore global data pointer on API boundary

[Leif Lindholm]

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