[Qemu-devel] [RFC] [PATCH 0/5] linux-user: refactor do_syscall()

[Qemu-devel] [RFC] [PATCH 0/5] linux-user: refactor do_syscall()

[riku.voipio]

From: Riku Voipio <riku.voipio@linaro.org>

Currently linux-user handles system calls with a 3000+ line switch/case construct
in do_syscall(). Some syscalls are implemented inline in the switch/case, others
as separate functions, and the rest as mix of both.

As the first step of the cleanup, I'd like to move implementation of each syscall
completely to their own functions. While at it, we define more standard interface between
do_syscall() and the functions implementing the system calls: system call functions take
parameter as raw abi_long, and leave the host to target errno conversion to do_syscall.

Once all syscall are converted to separate functions, we can convert to switch/case to
a table with syscall structs pointing to syscall implementation as well the strace definitions
currently in strace.list.

Before I proceed to convert all 300+ syscalls implemented, I'd like to hear if people think
it is a good idea, or if you have some improvements to suggest. To see how it would look in
practice, here are the first five syscalls cleaned up.

Riku Voipio (5):
  linux-user: move exit to own function
  linux-user: move read to own function
  linux-user: move write to own function
  linux-user: complete do_open function isolation
  linux-user: do_openat wrapper added

 linux-user/syscall.c |  182 +++++++++++++++++++++++++++++---------------------
 1 file changed, 106 insertions(+), 76 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/5] linux-user: move exit to own function

[riku.voipio]

From: Riku Voipio <riku.voipio@linaro.org>

In preparations for for refactoring the main switch/case out

Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/syscall.c |   92 ++++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 44 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 471d060..0b077e7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5083,6 +5083,53 @@ static int do_open(void *cpu_env, const char *pathname, int flags, mode_t mode)
 
     return get_errno(open(path(pathname), flags, mode));
 }
+static void do_exit(void *, abi_long) __attribute__ ((noreturn));
+static void do_exit(void *cpu_env, abi_long arg1)
+{
+#ifdef CONFIG_USE_NPTL
+    /* In old applications this may be used to implement _exit(2).
+       However in threaded applictions it is used for thread termination,
+       and _exit_group is used for application termination.
+       Do thread termination if we have more then one thread.  */
+    /* FIXME: This probably breaks if a signal arrives.  We should probably
+       be disabling signals.  */
+    if (first_cpu->next_cpu) {
+        TaskState *ts;
+        CPUArchState **lastp;
+        CPUArchState *p;
+
+        cpu_list_lock();
+        lastp = &first_cpu;
+        p = first_cpu;
+        while (p && p != (CPUArchState *)cpu_env) {
+            lastp = &p->next_cpu;
+            p = p->next_cpu;
+        }
+        /* If we didn't find the CPU for this thread then something is
+           horribly wrong.  */
+        if (!p)
+            abort();
+        /* Remove the CPU from the list.  */
+        *lastp = p->next_cpu;
+        cpu_list_unlock();
+        ts = ((CPUArchState *)cpu_env)->opaque;
+        if (ts->child_tidptr) {
+            put_user_u32(0, ts->child_tidptr);
+            sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
+                      NULL, NULL, 0);
+        }
+        thread_env = NULL;
+        object_delete(OBJECT(ENV_GET_CPU(cpu_env)));
+        g_free(ts);
+        pthread_exit(NULL);
+    }
+#endif
+#ifdef TARGET_GPROF
+    _mcleanup();
+#endif
+    gdb_exit(cpu_env, arg1);
+    _exit(arg1);
+}
 
 /* do_syscall() should always have a single exit point at the end so
    that actions, such as logging of syscall results, can be performed.
@@ -5105,50 +5152,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 
     switch(num) {
     case TARGET_NR_exit:
-#ifdef CONFIG_USE_NPTL
-      /* In old applications this may be used to implement _exit(2).
-         However in threaded applictions it is used for thread termination,
-         and _exit_group is used for application termination.
-         Do thread termination if we have more then one thread.  */
-      /* FIXME: This probably breaks if a signal arrives.  We should probably
-         be disabling signals.  */
-      if (first_cpu->next_cpu) {
-          TaskState *ts;
-          CPUArchState **lastp;
-          CPUArchState *p;
-
-          cpu_list_lock();
-          lastp = &first_cpu;
-          p = first_cpu;
-          while (p && p != (CPUArchState *)cpu_env) {
-              lastp = &p->next_cpu;
-              p = p->next_cpu;
-          }
-          /* If we didn't find the CPU for this thread then something is
-             horribly wrong.  */
-          if (!p)
-              abort();
-          /* Remove the CPU from the list.  */
-          *lastp = p->next_cpu;
-          cpu_list_unlock();
-          ts = ((CPUArchState *)cpu_env)->opaque;
-          if (ts->child_tidptr) {
-              put_user_u32(0, ts->child_tidptr);
-              sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
-                        NULL, NULL, 0);
-          }
-          thread_env = NULL;
-          object_delete(OBJECT(ENV_GET_CPU(cpu_env)));
-          g_free(ts);
-          pthread_exit(NULL);
-      }
-#endif
-#ifdef TARGET_GPROF
-        _mcleanup();
-#endif
-        gdb_exit(cpu_env, arg1);
-        _exit(arg1);
-        ret = 0; /* avoid warning */
+        do_exit(cpu_env, arg1);
         break;
     case TARGET_NR_read:
         if (arg3 == 0)
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/5] linux-user: move read to own function

[riku.voipio]

From: Riku Voipio <riku.voipio@linaro.org>

In preparations for for refactoring the main switch/case out

Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/syscall.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0b077e7..497cae5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5131,6 +5131,21 @@ static void do_exit(void *cpu_env, abi_long arg1)
     _exit(arg1);
 }
 
+static abi_long do_read(abi_long arg1, abi_long arg2, abi_long arg3)
+{
+    abi_long ret;
+    void *p;
+    if (arg3 == 0)
+        ret = 0;
+    else {
+        if (!(p = lock_user(VERIFY_WRITE, arg2, arg3, 0)))
+            return -EFAULT;
+        ret = read(arg1, p, arg3);
+        unlock_user(p, arg2, ret);
+    }
+    return ret;
+}
+
 /* do_syscall() should always have a single exit point at the end so
    that actions, such as logging of syscall results, can be performed.
    All errnos that do_syscall() returns must be -TARGET_<errcode>. */
@@ -5155,14 +5170,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         do_exit(cpu_env, arg1);
         break;
     case TARGET_NR_read:
-        if (arg3 == 0)
-            ret = 0;
-        else {
-            if (!(p = lock_user(VERIFY_WRITE, arg2, arg3, 0)))
-                goto efault;
-            ret = get_errno(read(arg1, p, arg3));
-            unlock_user(p, arg2, ret);
-        }
+        ret = get_errno(do_read(arg1, arg2, arg3));
         break;
     case TARGET_NR_write:
         if (!(p = lock_user(VERIFY_READ, arg2, arg3, 1)))
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/5] linux-user: move write to own function

[riku.voipio]

From: Riku Voipio <riku.voipio@linaro.org>

In preparations for for refactoring the main switch/case out

Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/syscall.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 497cae5..a9bfe8c 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5145,6 +5145,16 @@ static abi_long do_read(abi_long arg1, abi_long arg2, abi_long arg3)
     }
     return ret;
 }
+static abi_long do_write(abi_long arg1, abi_long arg2, abi_long arg3)
+{
+    abi_long ret;
+    void *p;
+    if (!(p = lock_user(VERIFY_READ, arg2, arg3, 1)))
+        return -EFAULT;
+    ret = write(arg1, p, arg3);
+    unlock_user(p, arg2, 0);
+    return ret;
+}
 
 /* do_syscall() should always have a single exit point at the end so
    that actions, such as logging of syscall results, can be performed.
@@ -5173,10 +5183,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = get_errno(do_read(arg1, arg2, arg3));
         break;
     case TARGET_NR_write:
-        if (!(p = lock_user(VERIFY_READ, arg2, arg3, 1)))
-            goto efault;
-        ret = get_errno(write(arg1, p, arg3));
-        unlock_user(p, arg2, 0);
+        ret = get_errno(do_write(arg1, arg2, arg3));
         break;
     case TARGET_NR_open:
         if (!(p = lock_user_string(arg1)))
-- 
1.7.9.5

[Qemu-devel] [PATCH 4/5] linux-user: complete do_open function isolation

[riku.voipio]

From: Riku Voipio <riku.voipio@linaro.org>

In preparations for for refactoring the main switch/case out

Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/syscall.c |   32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a9bfe8c..eebf219 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5035,8 +5035,10 @@ static int open_self_auxv(void *cpu_env, int fd)
     return 0;
 }
 
-static int do_open(void *cpu_env, const char *pathname, int flags, mode_t mode)
+static int do_open(void *cpu_env, abi_long arg1, abi_long arg2, abi_long arg3)
 {
+    char *pathname;
+    int ret,flags;
     struct fake_open {
         const char *filename;
         int (*fill)(void *cpu_env, int fd);
@@ -5048,6 +5050,9 @@ static int do_open(void *cpu_env, const char *pathname, int flags, mode_t mode)
         { "/proc/self/auxv", open_self_auxv },
         { NULL, NULL }
     };
+    flags=target_to_host_bitmask(arg2, fcntl_flags_tbl);
+    if (!(pathname = lock_user_string(arg1)))
+        return -EFAULT;
 
     for (fake_open = fakes; fake_open->filename; fake_open++) {
         if (!strncmp(pathname, fake_open->filename,
@@ -5059,7 +5064,7 @@ static int do_open(void *cpu_env, const char *pathname, int flags, mode_t mode)
     if (fake_open->filename) {
         const char *tmpdir;
         char filename[PATH_MAX];
-        int fd, r;
+        int fd;
 
         /* create temporary file to map stat to */
         tmpdir = getenv("TMPDIR");
@@ -5068,20 +5073,24 @@ static int do_open(void *cpu_env, const char *pathname, int flags, mode_t mode)
         snprintf(filename, sizeof(filename), "%s/qemu-open.XXXXXX", tmpdir);
         fd = mkstemp(filename);
         if (fd < 0) {
-            return fd;
+            ret = fd;
+            goto cleanup;
         }
         unlink(filename);
 
-        if ((r = fake_open->fill(cpu_env, fd))) {
+        if ((ret = fake_open->fill(cpu_env, fd))) {
             close(fd);
-            return r;
+            goto cleanup;
         }
         lseek(fd, 0, SEEK_SET);
-
-        return fd;
+        ret = fd;
+        goto cleanup;
     }
 
-    return get_errno(open(path(pathname), flags, mode));
+    ret = open(path(pathname), flags, arg3);
+cleanup:
+    unlock_user(pathname, arg1, 0);
+    return ret;
 }
 static void do_exit(void *, abi_long) __attribute__ ((noreturn));
 static void do_exit(void *cpu_env, abi_long arg1)
@@ -5186,12 +5195,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = get_errno(do_write(arg1, arg2, arg3));
         break;
     case TARGET_NR_open:
-        if (!(p = lock_user_string(arg1)))
-            goto efault;
-        ret = get_errno(do_open(cpu_env, p,
-                                target_to_host_bitmask(arg2, fcntl_flags_tbl),
-                                arg3));
-        unlock_user(p, arg1, 0);
+        ret = get_errno(do_open(cpu_env, arg1, arg2, arg3));
         break;
 #if defined(TARGET_NR_openat) && defined(__NR_openat)
     case TARGET_NR_openat:
-- 
1.7.9.5

[Qemu-devel] [PATCH 5/5] linux-user: do_openat wrapper added

[riku.voipio]

From: Riku Voipio <riku.voipio@linaro.org>

In preparations for for refactoring the main switch/case out

Signed-off-by: Riku Voipio <riku.voipio@linaro.org>
---
 linux-user/syscall.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index eebf219..61030f2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5092,6 +5092,19 @@ cleanup:
     unlock_user(pathname, arg1, 0);
     return ret;
 }
+
+static int do_openat(void *cpu_env, abi_long arg1, abi_long arg2, abi_long arg3, abi_long arg4)
+{
+    void *p;
+    int ret;
+    if (!(p = lock_user_string(arg2)))
+            return -EFAULT;
+    ret = sys_openat(arg1, path(p),
+            target_to_host_bitmask(arg3, fcntl_flags_tbl), arg4);
+    unlock_user(p, arg2, 0);
+    return ret;
+}
+
 static void do_exit(void *, abi_long) __attribute__ ((noreturn));
 static void do_exit(void *cpu_env, abi_long arg1)
 {
@@ -5199,13 +5212,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         break;
 #if defined(TARGET_NR_openat) && defined(__NR_openat)
     case TARGET_NR_openat:
-        if (!(p = lock_user_string(arg2)))
-            goto efault;
-        ret = get_errno(sys_openat(arg1,
-                                   path(p),
-                                   target_to_host_bitmask(arg3, fcntl_flags_tbl),
-                                   arg4));
-        unlock_user(p, arg2, 0);
+        ret = get_errno(do_openat(cpu_env, arg1, arg2, arg3, arg4));
         break;
 #endif
     case TARGET_NR_close:
-- 
1.7.9.5

Re: [Qemu-devel] [RFC] [PATCH 0/5] linux-user: refactor do_syscall()

[Richard Henderson]

On 10/12/2012 11:24 AM, riku.voipio@linaro.org wrote:
> From: Riku Voipio <riku.voipio@linaro.org>
> 
> Currently linux-user handles system calls with a 3000+ line switch/case construct
> in do_syscall(). Some syscalls are implemented inline in the switch/case, others
> as separate functions, and the rest as mix of both.
> 
> As the first step of the cleanup, I'd like to move implementation of each syscall
> completely to their own functions. While at it, we define more standard interface between
> do_syscall() and the functions implementing the system calls: system call functions take
> parameter as raw abi_long, and leave the host to target errno conversion to do_syscall.

My only concern is leaving the host-to-target conversion to do_syscall.
I think the return value from do_foo should be the proper target return value.

My concern here are those handful of syscalls for which we have target-specific
error reporting mechanisms, and for which those reporting mechanisms are exploited
in some nefarious way.  E.g. sigprocmask and getpriority.


r~

Re: [Qemu-devel] [RFC] [PATCH 0/5] linux-user: refactor do_syscall()

[Peter Maydell]

On 12 October 2012 21:47, Richard Henderson <rth@twiddle.net> wrote:
> On 10/12/2012 11:24 AM, riku.voipio@linaro.org wrote:
>> As the first step of the cleanup, I'd like to move implementation of each syscall
>> completely to their own functions. While at it, we define more standard interface between
>> do_syscall() and the functions implementing the system calls: system call functions take
>> parameter as raw abi_long, and leave the host to target errno conversion to do_syscall.
>
> My only concern is leaving the host-to-target conversion to do_syscall.
> I think the return value from do_foo should be the proper target return value.

Yes, I think I agree with this.

Also, maybe we should have all the do_foo() functions actually have the
same signature (ie take all 6 args)? Otherwise when we switch to the
dispatch-via-function-pointer-in-array-of-structs model we'll have to
change all those prototypes first.

-- PMM

Re: [Qemu-devel] [RFC] [PATCH 0/5] linux-user: refactor do_syscall()

[Blue Swirl]

On Fri, Oct 12, 2012 at 6:24 PM,  <riku.voipio@linaro.org> wrote:
> From: Riku Voipio <riku.voipio@linaro.org>
>
> Currently linux-user handles system calls with a 3000+ line switch/case construct
> in do_syscall(). Some syscalls are implemented inline in the switch/case, others
> as separate functions, and the rest as mix of both.
>
> As the first step of the cleanup, I'd like to move implementation of each syscall
> completely to their own functions. While at it, we define more standard interface between
> do_syscall() and the functions implementing the system calls: system call functions take
> parameter as raw abi_long, and leave the host to target errno conversion to do_syscall.
>
> Once all syscall are converted to separate functions, we can convert to switch/case to
> a table with syscall structs pointing to syscall implementation as well the strace definitions
> currently in strace.list.
>
> Before I proceed to convert all 300+ syscalls implemented, I'd like to hear if people think
> it is a good idea, or if you have some improvements to suggest. To see how it would look in
> practice, here are the first five syscalls cleaned up.

I think it will be a nice cleanup. However, please fix coding style
for the affected code.

>
> Riku Voipio (5):
>   linux-user: move exit to own function
>   linux-user: move read to own function
>   linux-user: move write to own function
>   linux-user: complete do_open function isolation
>   linux-user: do_openat wrapper added
>
>  linux-user/syscall.c |  182 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 106 insertions(+), 76 deletions(-)
>
> --
> 1.7.9.5
>
>