[Qemu-devel] [PULL 05/27] target-arm/arm-semi.c: Factor out repeated 'return env->regs[0]'

[Peter Maydell]

Factor out a repeated pattern in the semihosting code:

    gdb_do_syscall(arm_semi_cb, "system,%s", arg0, (int)arg1+1);
    /* arm_semi_cb sets env->regs[0] to the syscall return value */
    return env->regs[0];

For A64 the return value will go in a different register; pull
the sequence out into its own function that passes the return
value in a static variable rather than overloading regs[0]
for the purpose, so the code will work on both A32/T32 and A64.

Note that the lack-of-synchronization bug noted in the FIXME
comment is not introduced by this commit, but was already present.

Signed-off-by: Peter Maydell <address@hidden>
Reviewed-by: Christopher Covington <address@hidden>
Tested-by: Christopher Covington <address@hidden>
Message-id: address@hidden
---
 target-arm/arm-semi.c | 79 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 32 deletions(-)

diff --git a/target-arm/arm-semi.c b/target-arm/arm-semi.c
index 42522a7..dbdc211 100644
--- a/target-arm/arm-semi.c
+++ b/target-arm/arm-semi.c
@@ -134,6 +134,7 @@ static void arm_semi_cb(CPUState *cs, target_ulong ret, 
target_ulong err)
 #ifdef CONFIG_USER_ONLY
     TaskState *ts = cs->opaque;
 #endif
+    target_ulong reg0 = env->regs[0];
 
     if (ret == (target_ulong)-1) {
 #ifdef CONFIG_USER_ONLY
@@ -141,22 +142,23 @@ static void arm_semi_cb(CPUState *cs, target_ulong ret, 
target_ulong err)
 #else
        syscall_err = err;
 #endif
-        env->regs[0] = ret;
+        reg0 = ret;
     } else {
         /* Fixup syscalls that use nonstardard return conventions.  */
-        switch (env->regs[0]) {
+        switch (reg0) {
         case TARGET_SYS_WRITE:
         case TARGET_SYS_READ:
-            env->regs[0] = arm_semi_syscall_len - ret;
+            reg0 = arm_semi_syscall_len - ret;
             break;
         case TARGET_SYS_SEEK:
-            env->regs[0] = 0;
+            reg0 = 0;
             break;
         default:
-            env->regs[0] = ret;
+            reg0 = ret;
             break;
         }
     }
+    env->regs[0] = reg0;
 }
 
 static void arm_semi_flen_cb(CPUState *cs, target_ulong ret, target_ulong err)
@@ -175,6 +177,25 @@ static void arm_semi_flen_cb(CPUState *cs, target_ulong 
ret, target_ulong err)
 #endif
 }
 
+static target_ulong arm_gdb_syscall(ARMCPU *cpu, gdb_syscall_complete_cb cb,
+                                    const char *fmt, ...)
+{
+    va_list va;
+    CPUARMState *env = &cpu->env;
+
+    va_start(va, fmt);
+    gdb_do_syscallv(cb, fmt, va);
+    va_end(va);
+
+    /* FIXME: we are implicitly relying on the syscall completing
+     * before this point, which is not guaranteed. We should
+     * put in an explicit synchronization between this and
+     * the callback function.
+     */
+
+    return env->regs[0];
+}
+
 /* Read the input value from the argument block; fail the semihosting
  * call if the memory read fails.
  */
@@ -223,9 +244,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
             return result_fileno;
         }
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_cb, "open,%s,%x,1a4", arg0,
-                           (int)arg2+1, gdb_open_modeflags[arg1]);
-            ret = env->regs[0];
+            ret = arm_gdb_syscall(cpu, arm_semi_cb, "open,%s,%x,1a4", arg0,
+                                  (int)arg2+1, gdb_open_modeflags[arg1]);
         } else {
             ret = set_swi_errno(ts, open(s, open_modeflags[arg1], 0644));
         }
@@ -234,8 +254,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
     case TARGET_SYS_CLOSE:
         GET_ARG(0);
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_cb, "close,%x", arg0);
-            return env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_cb, "close,%x", arg0);
         } else {
             return set_swi_errno(ts, close(arg0));
         }
@@ -248,8 +267,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
               return (uint32_t)-1;
           /* Write to debug console.  stderr is near enough.  */
           if (use_gdb_syscalls()) {
-                gdb_do_syscall(arm_semi_cb, "write,2,%x,1", args);
-                return env->regs[0];
+                return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", args);
           } else {
                 return write(STDERR_FILENO, &c, 1);
           }
@@ -260,8 +278,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
             return (uint32_t)-1;
         len = strlen(s);
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_cb, "write,2,%x,%x", args, len);
-            ret = env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,%x",
+                                   args, len);
         } else {
             ret = write(STDERR_FILENO, s, len);
         }
@@ -274,8 +292,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
         len = arg2;
         if (use_gdb_syscalls()) {
             arm_semi_syscall_len = len;
-            gdb_do_syscall(arm_semi_cb, "write,%x,%x,%x", arg0, arg1, len);
-            return env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_cb, "write,%x,%x,%x",
+                                   arg0, arg1, len);
         } else {
             s = lock_user(VERIFY_READ, arg1, len, 1);
             if (!s) {
@@ -295,8 +313,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
         len = arg2;
         if (use_gdb_syscalls()) {
             arm_semi_syscall_len = len;
-            gdb_do_syscall(arm_semi_cb, "read,%x,%x,%x", arg0, arg1, len);
-            return env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_cb, "read,%x,%x,%x",
+                                   arg0, arg1, len);
         } else {
             s = lock_user(VERIFY_WRITE, arg1, len, 0);
             if (!s) {
@@ -317,8 +335,7 @@ uint32_t do_arm_semihosting(CPUARMState *env)
     case TARGET_SYS_ISTTY:
         GET_ARG(0);
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_cb, "isatty,%x", arg0);
-            return env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_cb, "isatty,%x", arg0);
         } else {
             return isatty(arg0);
         }
@@ -326,8 +343,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
         GET_ARG(0);
         GET_ARG(1);
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_cb, "lseek,%x,%x,0", arg0, arg1);
-            return env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_cb, "lseek,%x,%x,0",
+                                   arg0, arg1);
         } else {
             ret = set_swi_errno(ts, lseek(arg0, arg1, SEEK_SET));
             if (ret == (uint32_t)-1)
@@ -337,9 +354,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
     case TARGET_SYS_FLEN:
         GET_ARG(0);
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_flen_cb, "fstat,%x,%x",
-                           arg0, env->regs[13]-64);
-            return env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_flen_cb, "fstat,%x,%x",
+                                   arg0, env->regs[13]-64);
         } else {
             struct stat buf;
             ret = set_swi_errno(ts, fstat(arg0, &buf));
@@ -354,8 +370,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
         GET_ARG(0);
         GET_ARG(1);
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_cb, "unlink,%s", arg0, (int)arg1+1);
-            ret = env->regs[0];
+            ret = arm_gdb_syscall(cpu, arm_semi_cb, "unlink,%s",
+                                  arg0, (int)arg1+1);
         } else {
             s = lock_user_string(arg0);
             if (!s) {
@@ -372,9 +388,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
         GET_ARG(2);
         GET_ARG(3);
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_cb, "rename,%s,%s",
-                           arg0, (int)arg1+1, arg2, (int)arg3+1);
-            return env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_cb, "rename,%s,%s",
+                                   arg0, (int)arg1+1, arg2, (int)arg3+1);
         } else {
             char *s2;
             s = lock_user_string(arg0);
@@ -398,8 +413,8 @@ uint32_t do_arm_semihosting(CPUARMState *env)
         GET_ARG(0);
         GET_ARG(1);
         if (use_gdb_syscalls()) {
-            gdb_do_syscall(arm_semi_cb, "system,%s", arg0, (int)arg1+1);
-            return env->regs[0];
+            return arm_gdb_syscall(cpu, arm_semi_cb, "system,%s",
+                                   arg0, (int)arg1+1);
         } else {
             s = lock_user_string(arg0);
             if (!s) {
-- 
1.9.1