[Qemu-devel] [PATCH] linux-user: add ppoll syscall support

[Qemu-devel] [PATCH] linux-user: add ppoll syscall support

[Mike Frysinger]

Some architectures (like Blackfin) only implement ppoll (and skip poll).
So add support for it using existing poll code.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 linux-user/syscall.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6116ab5..5382662 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6230,18 +6230,42 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = do_select(arg1, arg2, arg3, arg4, arg5);
         break;
 #endif
-#ifdef TARGET_NR_poll
+#if defined(TARGET_NR_poll) || defined(TARGET_NR_ppoll)
+# ifdef TARGET_NR_poll
     case TARGET_NR_poll:
+# endif
+# ifdef TARGET_NR_ppoll
+    case TARGET_NR_ppoll:
+# endif
         {
             struct target_pollfd *target_pfd;
             unsigned int nfds = arg2;
             int timeout = arg3;
             struct pollfd *pfd;
             unsigned int i;
+            sigset_t origmask;
 
             target_pfd = lock_user(VERIFY_WRITE, arg1, sizeof(struct target_pollfd) * nfds, 1);
             if (!target_pfd)
                 goto efault;
+
+            if (num == TARGET_NR_ppoll) {
+                sigset_t set;
+                abi_ulong mask;
+
+                if (arg3) {
+                    struct timespec timeout_ts;
+                    target_to_host_timespec(&timeout_ts, arg3);
+                    timeout = timeout_ts.tv_sec * 1000 +
+                              timeout_ts.tv_nsec / 1000000;
+                } else
+                    timeout = -1;
+
+                mask = arg4;
+                target_to_host_old_sigset(&set, &mask);
+                sigprocmask(SIG_SETMASK, &set, &origmask);
+            }
+
             pfd = alloca(sizeof(struct pollfd) * nfds);
             for(i = 0; i < nfds; i++) {
                 pfd[i].fd = tswap32(target_pfd[i].fd);
@@ -6256,6 +6280,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                                - sizeof(struct pollfd));
             }
             unlock_user(target_pfd, arg1, ret);
+
+            if (num == TARGET_NR_ppoll)
+                sigprocmask(SIG_SETMASK, &origmask, NULL);
         }
         break;
 #endif
-- 
1.7.4.rc2

Re: [Qemu-devel] [PATCH] linux-user: add ppoll syscall support

[Peter Maydell]

On 23 January 2011 19:56, Mike Frysinger <vapier@gentoo.org> wrote:
> Some architectures (like Blackfin) only implement ppoll (and skip poll).
> So add support for it using existing poll code.

This looks wrong -- ppoll() is supposed to be atomic, but
your implementation isn't. Why can't we just implement this
by calling the host ppoll? (might need a configure test, but
that's straightforward.)

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: add ppoll syscall support

[Mike Frysinger]

On Sun, Jan 23, 2011 at 16:25, Peter Maydell wrote:
> On 23 January 2011 19:56, Mike Frysinger wrote:
>> Some architectures (like Blackfin) only implement ppoll (and skip poll).
>> So add support for it using existing poll code.
>
> This looks wrong -- ppoll() is supposed to be atomic, but
> your implementation isn't. Why can't we just implement this
> by calling the host ppoll? (might need a configure test, but
> that's straightforward.)

it's atomic from the apps' point of view, so what does it matter ?
-mike

Re: [Qemu-devel] [PATCH] linux-user: add ppoll syscall support

[Peter Maydell]

On 23 January 2011 21:35, Mike Frysinger <vapier@gentoo.org> wrote:
> On Sun, Jan 23, 2011 at 16:25, Peter Maydell wrote:
>> This looks wrong -- ppoll() is supposed to be atomic, but
>> your implementation isn't. Why can't we just implement this
>> by calling the host ppoll? (might need a configure test, but
>> that's straightforward.)
>
> it's atomic from the apps' point of view, so what does it matter ?

It's not atomic because signals can arrive in the gaps
between your calls to sigaction and poll (even if qemu doesn't
deliver them to the guest until we're done). ppoll() is supposed
to return EINTR if interrupted by a signal, but if a signal arrives
before you call poll() then you won't notice it so you won't
return, and the app might hang.

Looks like this same issue came up with a proposed pselect
patch last year:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg28339.html
(together with some remarks about it being better not to
trust buggy libcs and go straight for the host syscall.)

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: add ppoll syscall support

[Mike Frysinger]

On Sun, Jan 23, 2011 at 17:35, Peter Maydell wrote:
> On 23 January 2011 21:35, Mike Frysinger wrote:
>> On Sun, Jan 23, 2011 at 16:25, Peter Maydell wrote:
>>> This looks wrong -- ppoll() is supposed to be atomic, but
>>> your implementation isn't. Why can't we just implement this
>>> by calling the host ppoll? (might need a configure test, but
>>> that's straightforward.)
>>
>> it's atomic from the apps' point of view, so what does it matter ?
>
> It's not atomic because signals can arrive in the gaps
> between your calls to sigaction and poll (even if qemu doesn't
> deliver them to the guest until we're done). ppoll() is supposed
> to return EINTR if interrupted by a signal, but if a signal arrives
> before you call poll() then you won't notice it so you won't
> return, and the app might hang.

i know how host signals work, but my limited understanding of how qemu
works is that its guest signal handling would make this a non-issue.
if that isnt the case, then i can trivially convert it to ppoll.

> Looks like this same issue came up with a proposed pselect
> patch last year:
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg28339.html
> (together with some remarks about it being better not to
> trust buggy libcs and go straight for the host syscall.)

i also need pselect, and i have a local patch to support it (the same
way i've implemented ppoll).  returning ENOSYS is unacceptable as it
assumes the port in question can fall back to another select variant.
but if the arch *only* supports pselect6, then there is nothing to
fall back to if select/newselect are not supported by the arch.
-mike

[Qemu-devel] [PATCH v2] linux-user: add ppoll syscall support

[Mike Frysinger]

Some architectures (like Blackfin) only implement ppoll (and skip poll).
So add support for it using existing poll code.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- call host ppoll() directly

 linux-user/syscall.c |   28 ++++++++++++++++++++++++++--
 1 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6116ab5..690ee44 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6230,8 +6230,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = do_select(arg1, arg2, arg3, arg4, arg5);
         break;
 #endif
-#ifdef TARGET_NR_poll
+#if defined(TARGET_NR_poll) || defined(TARGET_NR_ppoll)
+# ifdef TARGET_NR_poll
     case TARGET_NR_poll:
+# endif
+# ifdef TARGET_NR_ppoll
+    case TARGET_NR_ppoll:
+# endif
         {
             struct target_pollfd *target_pfd;
             unsigned int nfds = arg2;
@@ -6242,12 +6247,31 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             target_pfd = lock_user(VERIFY_WRITE, arg1, sizeof(struct target_pollfd) * nfds, 1);
             if (!target_pfd)
                 goto efault;
+
             pfd = alloca(sizeof(struct pollfd) * nfds);
             for(i = 0; i < nfds; i++) {
                 pfd[i].fd = tswap32(target_pfd[i].fd);
                 pfd[i].events = tswap16(target_pfd[i].events);
             }
-            ret = get_errno(poll(pfd, nfds, timeout));
+
+# ifdef TARGET_NR_ppoll
+            if (num == TARGET_NR_ppoll) {
+                struct timespec _timeout_ts, *timeout_ts = &_timeout_ts;
+                abi_ulong mask = arg4;
+                sigset_t sigmask;
+
+                if (arg3)
+                    target_to_host_timespec(timeout_ts, arg3);
+                else
+                    timeout_ts = NULL;
+
+                target_to_host_old_sigset(&sigmask, &mask);
+
+                ret = get_errno(ppoll(pfd, nfds, timeout_ts, &sigmask));
+            } else
+# endif
+                ret = get_errno(poll(pfd, nfds, timeout));
+
             if (!is_error(ret)) {
                 for(i = 0; i < nfds; i++) {
                     target_pfd[i].revents = tswap16(pfd[i].revents);
-- 
1.7.4.rc2

Re: [Qemu-devel] [PATCH v2] linux-user: add ppoll syscall support

[Peter Maydell]

On 23 January 2011 23:32, Mike Frysinger <vapier@gentoo.org> wrote:
> +                if (arg3)
> +                    target_to_host_timespec(timeout_ts, arg3);
> +                else
> +                    timeout_ts = NULL;

Coding style mandates braces here. Also, target_to_host_timespec()
can return non-zero if the user handed us a bad pointer, which
you need to handle. (No, none of the other users of the
function do this; yes, I think they're all broken :-))

> +                target_to_host_old_sigset(&sigmask, &mask);

Are you sure this is right?
http://lxr.linux.no/#linux+v2.6.37/fs/select.c#L950
suggests the syscall takes a new sigset, not an old one.

You also need to lock_user() the memory for the sigset.
(target_to_host_timespec() does lock_user/unlock_user for
you but the target_to_host_*sigset() don't).

> +                ret = get_errno(ppoll(pfd, nfds, timeout_ts, &sigmask));
> +            } else
> +# endif
> +                ret = get_errno(poll(pfd, nfds, timeout));
> +
>             if (!is_error(ret)) {
>                 for(i = 0; i < nfds; i++) {
>                     target_pfd[i].revents = tswap16(pfd[i].revents);

The ppoll() manpage says
"The Linux ppoll() system call modifies its timeout argument."

Your implementation doesn't do this. Also, this means you
really do need to call the host ppoll syscall directly, because
glibc deliberately hides this behaviour and would prevent
us from implementing it.

thanks
-- PMM

[Qemu-devel] [PATCH v3] linux-user: add ppoll syscall support

[Mike Frysinger]

Some architectures (like Blackfin) only implement ppoll (and skip poll).
So add support for it using existing poll code.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v3
	- call ppoll syscall directly so timespec updates get passed back
	- tweak style
	- use new sigsets

 linux-user/syscall.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6116ab5..02aab0f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -529,6 +529,15 @@ static int sys_inotify_init1(int flags)
 #undef TARGET_NR_inotify_rm_watch
 #endif /* CONFIG_INOTIFY  */
 
+#if defined(TARGET_NR_ppoll)
+#ifndef __NR_ppoll
+# define __NR_ppoll -1
+#endif
+#define __NR_sys_ppoll __NR_ppoll
+_syscall5(int, sys_ppoll, struct pollfd *, fds, nfds_t, nfds,
+          struct timespec *, timeout, const __sigset_t *, sigmask,
+          size_t, sigsetsize)
+#endif
 
 extern int personality(int);
 extern int flock(int, int);
@@ -6230,8 +6239,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = do_select(arg1, arg2, arg3, arg4, arg5);
         break;
 #endif
-#ifdef TARGET_NR_poll
+#if defined(TARGET_NR_poll) || defined(TARGET_NR_ppoll)
+# ifdef TARGET_NR_poll
     case TARGET_NR_poll:
+# endif
+# ifdef TARGET_NR_ppoll
+    case TARGET_NR_ppoll:
+# endif
         {
             struct target_pollfd *target_pfd;
             unsigned int nfds = arg2;
@@ -6242,12 +6256,46 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             target_pfd = lock_user(VERIFY_WRITE, arg1, sizeof(struct target_pollfd) * nfds, 1);
             if (!target_pfd)
                 goto efault;
+
             pfd = alloca(sizeof(struct pollfd) * nfds);
             for(i = 0; i < nfds; i++) {
                 pfd[i].fd = tswap32(target_pfd[i].fd);
                 pfd[i].events = tswap16(target_pfd[i].events);
             }
-            ret = get_errno(poll(pfd, nfds, timeout));
+
+# ifdef TARGET_NR_ppoll
+            if (num == TARGET_NR_ppoll) {
+                struct timespec _timeout_ts, *timeout_ts = &_timeout_ts;
+                target_sigset_t *target_set;
+                sigset_t set;
+
+                if (arg3) {
+                    if (target_to_host_timespec(timeout_ts, arg3)) {
+                        unlock_user(target_pfd, arg1, 0);
+                        goto efault;
+                    }
+                } else {
+                    timeout_ts = NULL;
+                }
+
+                target_set = lock_user(VERIFY_READ, arg4, sizeof(target_sigset_t), 1);
+                if (!target_set) {
+                    unlock_user(target_pfd, arg1, 0);
+                    goto efault;
+                }
+                target_to_host_sigset(&set, target_set);
+
+                ret = get_errno(sys_ppoll(pfd, nfds, timeout_ts, &set, _NSIG/8));
+
+                if (!is_error(ret) && arg3) {
+                    host_to_target_timespec(arg3, timeout_ts);
+                }
+
+                unlock_user(target_set, arg4, 0);
+            } else
+# endif
+                ret = get_errno(poll(pfd, nfds, timeout));
+
             if (!is_error(ret)) {
                 for(i = 0; i < nfds; i++) {
                     target_pfd[i].revents = tswap16(pfd[i].revents);
-- 
1.7.4.rc2

Re: [Qemu-devel] [PATCH v3] linux-user: add ppoll syscall support

[Peter Maydell]

On 24 January 2011 03:57, Mike Frysinger <vapier@gentoo.org> wrote:
> Some architectures (like Blackfin) only implement ppoll (and skip poll).
> So add support for it using existing poll code.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> v3
>        - call ppoll syscall directly so timespec updates get passed back
>        - tweak style
>        - use new sigsets

This version looks OK to me and works for my fairly simple
testcase.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH v4] linux-user: add ppoll syscall support

[Mike Frysinger]

Some architectures (like Blackfin) only implement ppoll (and skip poll).
So add support for it using existing poll code.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v4
	- handle null signal set ... this wasn't failing as Blackfin ELFs default
	  to VMA of 0 and thus lock_user() on addr 0 did not fail ...

 linux-user/syscall.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6116ab5..bb6ef43 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -529,6 +529,15 @@ static int sys_inotify_init1(int flags)
 #undef TARGET_NR_inotify_rm_watch
 #endif /* CONFIG_INOTIFY  */
 
+#if defined(TARGET_NR_ppoll)
+#ifndef __NR_ppoll
+# define __NR_ppoll -1
+#endif
+#define __NR_sys_ppoll __NR_ppoll
+_syscall5(int, sys_ppoll, struct pollfd *, fds, nfds_t, nfds,
+          struct timespec *, timeout, const __sigset_t *, sigmask,
+          size_t, sigsetsize)
+#endif
 
 extern int personality(int);
 extern int flock(int, int);
@@ -6230,8 +6239,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = do_select(arg1, arg2, arg3, arg4, arg5);
         break;
 #endif
-#ifdef TARGET_NR_poll
+#if defined(TARGET_NR_poll) || defined(TARGET_NR_ppoll)
+# ifdef TARGET_NR_poll
     case TARGET_NR_poll:
+# endif
+# ifdef TARGET_NR_ppoll
+    case TARGET_NR_ppoll:
+# endif
         {
             struct target_pollfd *target_pfd;
             unsigned int nfds = arg2;
@@ -6242,12 +6256,51 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             target_pfd = lock_user(VERIFY_WRITE, arg1, sizeof(struct target_pollfd) * nfds, 1);
             if (!target_pfd)
                 goto efault;
+
             pfd = alloca(sizeof(struct pollfd) * nfds);
             for(i = 0; i < nfds; i++) {
                 pfd[i].fd = tswap32(target_pfd[i].fd);
                 pfd[i].events = tswap16(target_pfd[i].events);
             }
-            ret = get_errno(poll(pfd, nfds, timeout));
+
+# ifdef TARGET_NR_ppoll
+            if (num == TARGET_NR_ppoll) {
+                struct timespec _timeout_ts, *timeout_ts = &_timeout_ts;
+                target_sigset_t *target_set;
+                sigset_t _set, *set = &_set;
+
+                if (arg3) {
+                    if (target_to_host_timespec(timeout_ts, arg3)) {
+                        unlock_user(target_pfd, arg1, 0);
+                        goto efault;
+                    }
+                } else {
+                    timeout_ts = NULL;
+                }
+
+                if (arg4) {
+                    target_set = lock_user(VERIFY_READ, arg4, sizeof(target_sigset_t), 1);
+                    if (!target_set) {
+                        unlock_user(target_pfd, arg1, 0);
+                        goto efault;
+                    }
+                    target_to_host_sigset(set, target_set);
+                } else {
+                    set = NULL;
+                }
+
+                ret = get_errno(sys_ppoll(pfd, nfds, timeout_ts, set, _NSIG/8));
+
+                if (!is_error(ret) && arg3) {
+                    host_to_target_timespec(arg3, timeout_ts);
+                }
+                if (arg4) {
+                    unlock_user(target_set, arg4, 0);
+                }
+            } else
+# endif
+                ret = get_errno(poll(pfd, nfds, timeout));
+
             if (!is_error(ret)) {
                 for(i = 0; i < nfds; i++) {
                     target_pfd[i].revents = tswap16(pfd[i].revents);
-- 
1.7.4.rc2