[Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall

Richard Henderson posted 1 patch 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180718200648.22529-1-richard.henderson@linaro.org
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
linux-user/qemu.h       |  2 ++
linux-user/ppc/signal.c | 56 +++++++++++++++++++++++++++++++++++++++++
linux-user/syscall.c    |  6 +++++
3 files changed, 64 insertions(+)
[Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall
Posted by Richard Henderson 5 years, 9 months ago
This allows the tests generated by debian-powerpc-user-cross
to function properly, especially tests/test-coroutine.

Technically this syscall is available to both ppc32 and ppc64,
but only ppc32 glibc actually uses it.  Thus the ppc64 path is
untested.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/qemu.h       |  2 ++
 linux-user/ppc/signal.c | 56 +++++++++++++++++++++++++++++++++++++++++
 linux-user/syscall.c    |  6 +++++
 3 files changed, 64 insertions(+)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index bb85c81aa4..e0963676c7 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -395,6 +395,8 @@ long do_sigreturn(CPUArchState *env);
 long do_rt_sigreturn(CPUArchState *env);
 abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp);
 int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset);
+abi_long do_swapcontext(CPUArchState *env, abi_ulong uold_ctx,
+                        abi_ulong unew_ctx, abi_long ctx_size);
 /**
  * block_signals: block all signals while handling this guest syscall
  *
diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index ef4c518f11..2ae120a2bc 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -675,3 +675,59 @@ sigsegv:
     force_sig(TARGET_SIGSEGV);
     return -TARGET_QEMU_ESIGRETURN;
 }
+
+/* This syscall implements {get,set,swap}context for userland.  */
+abi_long do_swapcontext(CPUArchState *env, abi_ulong uold_ctx,
+                        abi_ulong unew_ctx, abi_long ctx_size)
+{
+    struct target_ucontext *uctx;
+    struct target_mcontext *mctx;
+
+    /* For ppc32, ctx_size is "reserved for future use".
+     * For ppc64, we do not yet support the VSX extension.
+     */
+    if (ctx_size < sizeof(struct target_ucontext)) {
+        return -TARGET_EINVAL;
+    }
+
+    if (uold_ctx) {
+        TaskState *ts = (TaskState *)thread_cpu->opaque;
+
+        if (!lock_user_struct(VERIFY_WRITE, uctx, uold_ctx, 1)) {
+            return -TARGET_EFAULT;
+        }
+
+#ifdef TARGET_PPC64
+        mctx = &uctx->tuc_sigcontext.mcontext;
+#else
+        /* ??? The kernel aligns the pointer down here into padding, but
+         * in setup_rt_frame we don't.  Be self-compatible for now.
+         */
+        mctx = &uctx->tuc_mcontext;
+        __put_user(h2g(mctx), &uctx->tuc_regs);
+#endif
+
+        save_user_regs(env, mctx);
+        host_to_target_sigset(&uctx->tuc_sigmask, &ts->signal_mask);
+
+        unlock_user_struct(uctx, uold_ctx, 1);
+    }
+
+    if (unew_ctx) {
+        int err;
+
+        if (!lock_user_struct(VERIFY_READ, uctx, unew_ctx, 1)) {
+            return -TARGET_EFAULT;
+        }
+        err = do_setcontext(uctx, env, 0);
+        unlock_user_struct(uctx, unew_ctx, 1);
+
+        if (err) {
+            /* We cannot return to a partially updated context.  */
+            force_sig(TARGET_SIGSEGV);
+        }
+        return -TARGET_QEMU_ESIGRETURN;
+    }
+
+    return 0;
+}
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3df3bdffb2..dfc851cc35 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -12790,6 +12790,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         ret = get_errno(kcmp(arg1, arg2, arg3, arg4, arg5));
         break;
 #endif
+#ifdef TARGET_NR_swapcontext
+    case TARGET_NR_swapcontext:
+        /* PowerPC specific.  */
+        ret = do_swapcontext(cpu_env, arg1, arg2, arg3);
+        break;
+#endif
 
     default:
     unimplemented:
-- 
2.17.1


Re: [Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
Hi Richard,

On 07/18/2018 05:06 PM, Richard Henderson wrote:
> This allows the tests generated by debian-powerpc-user-cross
> to function properly, especially tests/test-coroutine.
> 
> Technically this syscall is available to both ppc32 and ppc64,
> but only ppc32 glibc actually uses it.  Thus the ppc64 path is
> untested.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/qemu.h       |  2 ++
>  linux-user/ppc/signal.c | 56 +++++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall.c    |  6 +++++
>  3 files changed, 64 insertions(+)
> 
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index bb85c81aa4..e0963676c7 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -395,6 +395,8 @@ long do_sigreturn(CPUArchState *env);
>  long do_rt_sigreturn(CPUArchState *env);
>  abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp);
>  int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset);
> +abi_long do_swapcontext(CPUArchState *env, abi_ulong uold_ctx,
> +                        abi_ulong unew_ctx, abi_long ctx_size);
>  /**
>   * block_signals: block all signals while handling this guest syscall
>   *
> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> index ef4c518f11..2ae120a2bc 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -675,3 +675,59 @@ sigsegv:
>      force_sig(TARGET_SIGSEGV);
>      return -TARGET_QEMU_ESIGRETURN;
>  }
> +
> +/* This syscall implements {get,set,swap}context for userland.  */

This comment confuses me because do_setcontext() is available at line 625.

> +abi_long do_swapcontext(CPUArchState *env, abi_ulong uold_ctx,
> +                        abi_ulong unew_ctx, abi_long ctx_size)
> +{
> +    struct target_ucontext *uctx;
> +    struct target_mcontext *mctx;
> +
> +    /* For ppc32, ctx_size is "reserved for future use".
> +     * For ppc64, we do not yet support the VSX extension.
> +     */
> +    if (ctx_size < sizeof(struct target_ucontext)) {
> +        return -TARGET_EINVAL;

Shouldn't this be -TARGET_ENOMEM?

swapcontext(3):
ERRORS
       ENOMEM Insufficient stack space left.

> +    }
> +
> +    if (uold_ctx) {
> +        TaskState *ts = (TaskState *)thread_cpu->opaque;
> +
> +        if (!lock_user_struct(VERIFY_WRITE, uctx, uold_ctx, 1)) {
> +            return -TARGET_EFAULT;
> +        }
> +
> +#ifdef TARGET_PPC64
> +        mctx = &uctx->tuc_sigcontext.mcontext;
> +#else
> +        /* ??? The kernel aligns the pointer down here into padding, but
> +         * in setup_rt_frame we don't.  Be self-compatible for now.
> +         */
> +        mctx = &uctx->tuc_mcontext;
> +        __put_user(h2g(mctx), &uctx->tuc_regs);
> +#endif
> +
> +        save_user_regs(env, mctx);
> +        host_to_target_sigset(&uctx->tuc_sigmask, &ts->signal_mask);
> +
> +        unlock_user_struct(uctx, uold_ctx, 1);
> +    }
> +
> +    if (unew_ctx) {
> +        int err;
> +
> +        if (!lock_user_struct(VERIFY_READ, uctx, unew_ctx, 1)) {
> +            return -TARGET_EFAULT;
> +        }
> +        err = do_setcontext(uctx, env, 0);
> +        unlock_user_struct(uctx, unew_ctx, 1);
> +
> +        if (err) {
> +            /* We cannot return to a partially updated context.  */
> +            force_sig(TARGET_SIGSEGV);
> +        }
> +        return -TARGET_QEMU_ESIGRETURN;
> +    }
> +
> +    return 0;
> +}
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 3df3bdffb2..dfc851cc35 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -12790,6 +12790,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          ret = get_errno(kcmp(arg1, arg2, arg3, arg4, arg5));
>          break;
>  #endif
> +#ifdef TARGET_NR_swapcontext
> +    case TARGET_NR_swapcontext:
> +        /* PowerPC specific.  */
> +        ret = do_swapcontext(cpu_env, arg1, arg2, arg3);
> +        break;
> +#endif
>  
>      default:
>      unimplemented:
> 

Re: [Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall
Posted by Richard Henderson 5 years, 9 months ago
On 07/18/2018 03:56 PM, Philippe Mathieu-Daudé wrote:
>> +
>> +/* This syscall implements {get,set,swap}context for userland.  */
> 
> This comment confuses me because do_setcontext() is available at line 625.

But that's not wired up as a syscall.

>> +    /* For ppc32, ctx_size is "reserved for future use".
>> +     * For ppc64, we do not yet support the VSX extension.
>> +     */
>> +    if (ctx_size < sizeof(struct target_ucontext)) {
>> +        return -TARGET_EINVAL;
> 
> Shouldn't this be -TARGET_ENOMEM?
> 
> swapcontext(3):
> ERRORS
>        ENOMEM Insufficient stack space left.

No.  Please compare against the syscall, not the libc interface.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/signal_32.c#n1045


r~

Re: [Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
On 07/19/2018 08:13 AM, Richard Henderson wrote:
> On 07/18/2018 03:56 PM, Philippe Mathieu-Daudé wrote:
>>> +
>>> +/* This syscall implements {get,set,swap}context for userland.  */
>>
>> This comment confuses me because do_setcontext() is available at line 625.
> 
> But that's not wired up as a syscall.
> 
>>> +    /* For ppc32, ctx_size is "reserved for future use".
>>> +     * For ppc64, we do not yet support the VSX extension.
>>> +     */
>>> +    if (ctx_size < sizeof(struct target_ucontext)) {
>>> +        return -TARGET_EINVAL;
>>
>> Shouldn't this be -TARGET_ENOMEM?
>>
>> swapcontext(3):
>> ERRORS
>>        ENOMEM Insufficient stack space left.
> 
> No.  Please compare against the syscall, not the libc interface.

Thanks, I felt I was missing something with the 3rd syscall argument.

> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/signal_32.c#n1045
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


Re: [Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall
Posted by Alex Bennée 5 years, 9 months ago
Richard Henderson <richard.henderson@linaro.org> writes:

> This allows the tests generated by debian-powerpc-user-cross
> to function properly, especially tests/test-coroutine.
>
> Technically this syscall is available to both ppc32 and ppc64,
> but only ppc32 glibc actually uses it.  Thus the ppc64 path is
> untested.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Tested-by: Alex Bennée <alex.bennee@linaro.org>

and a caveat-ed:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

I'm confused by the lock_user_struct/unlock_user_struct which AFAICT are
basically access checks. Is there an implied locking I'm missing?

> ---
>  linux-user/qemu.h       |  2 ++
>  linux-user/ppc/signal.c | 56 +++++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall.c    |  6 +++++
>  3 files changed, 64 insertions(+)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index bb85c81aa4..e0963676c7 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -395,6 +395,8 @@ long do_sigreturn(CPUArchState *env);
>  long do_rt_sigreturn(CPUArchState *env);
>  abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp);
>  int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset);
> +abi_long do_swapcontext(CPUArchState *env, abi_ulong uold_ctx,
> +                        abi_ulong unew_ctx, abi_long ctx_size);
>  /**
>   * block_signals: block all signals while handling this guest syscall
>   *
> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> index ef4c518f11..2ae120a2bc 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -675,3 +675,59 @@ sigsegv:
>      force_sig(TARGET_SIGSEGV);
>      return -TARGET_QEMU_ESIGRETURN;
>  }
> +
> +/* This syscall implements {get,set,swap}context for userland.  */
> +abi_long do_swapcontext(CPUArchState *env, abi_ulong uold_ctx,
> +                        abi_ulong unew_ctx, abi_long ctx_size)
> +{
> +    struct target_ucontext *uctx;
> +    struct target_mcontext *mctx;
> +
> +    /* For ppc32, ctx_size is "reserved for future use".
> +     * For ppc64, we do not yet support the VSX extension.
> +     */
> +    if (ctx_size < sizeof(struct target_ucontext)) {
> +        return -TARGET_EINVAL;
> +    }
> +
> +    if (uold_ctx) {
> +        TaskState *ts = (TaskState *)thread_cpu->opaque;
> +
> +        if (!lock_user_struct(VERIFY_WRITE, uctx, uold_ctx, 1)) {
> +            return -TARGET_EFAULT;
> +        }
> +
> +#ifdef TARGET_PPC64
> +        mctx = &uctx->tuc_sigcontext.mcontext;
> +#else
> +        /* ??? The kernel aligns the pointer down here into padding, but
> +         * in setup_rt_frame we don't.  Be self-compatible for now.
> +         */
> +        mctx = &uctx->tuc_mcontext;
> +        __put_user(h2g(mctx), &uctx->tuc_regs);
> +#endif
> +
> +        save_user_regs(env, mctx);
> +        host_to_target_sigset(&uctx->tuc_sigmask, &ts->signal_mask);
> +
> +        unlock_user_struct(uctx, uold_ctx, 1);
> +    }
> +
> +    if (unew_ctx) {
> +        int err;
> +
> +        if (!lock_user_struct(VERIFY_READ, uctx, unew_ctx, 1)) {
> +            return -TARGET_EFAULT;
> +        }
> +        err = do_setcontext(uctx, env, 0);
> +        unlock_user_struct(uctx, unew_ctx, 1);
> +
> +        if (err) {
> +            /* We cannot return to a partially updated context.  */
> +            force_sig(TARGET_SIGSEGV);
> +        }
> +        return -TARGET_QEMU_ESIGRETURN;
> +    }
> +
> +    return 0;
> +}
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 3df3bdffb2..dfc851cc35 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -12790,6 +12790,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          ret = get_errno(kcmp(arg1, arg2, arg3, arg4, arg5));
>          break;
>  #endif
> +#ifdef TARGET_NR_swapcontext
> +    case TARGET_NR_swapcontext:
> +        /* PowerPC specific.  */
> +        ret = do_swapcontext(cpu_env, arg1, arg2, arg3);
> +        break;
> +#endif
>
>      default:
>      unimplemented:


--
Alex Bennée

Re: [Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall
Posted by Richard Henderson 5 years, 9 months ago
On 07/19/2018 06:05 AM, Alex Bennée wrote:
> I'm confused by the lock_user_struct/unlock_user_struct which AFAICT are
> basically access checks. Is there an implied locking I'm missing?

I have no idea why those have that name.  There's no lock,
just a validation of the underlying pages and a transform to
the corresponding host address.


r~

Re: [Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall
Posted by Peter Maydell 5 years, 9 months ago
On 19 July 2018 at 16:24, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 07/19/2018 06:05 AM, Alex Bennée wrote:
>> I'm confused by the lock_user_struct/unlock_user_struct which AFAICT are
>> basically access checks. Is there an implied locking I'm missing?
>
> I have no idea why those have that name.  There's no lock,
> just a validation of the underlying pages and a transform to
> the corresponding host address.

Mmm, it's unclear. It came in via 53a5960aadd542dd, and the
impression I get is that the idea was to try to decouple from
knowing that the guest memory layout is flat, as a preparation
for possible support for softmmu-in-usermode...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user/ppc: Implement swapcontext syscall
Posted by Laurent Vivier 5 years, 9 months ago
Le 18/07/2018 à 22:06, Richard Henderson a écrit :
> This allows the tests generated by debian-powerpc-user-cross
> to function properly, especially tests/test-coroutine.
> 
> Technically this syscall is available to both ppc32 and ppc64,
> but only ppc32 glibc actually uses it.  Thus the ppc64 path is
> untested.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/qemu.h       |  2 ++
>  linux-user/ppc/signal.c | 56 +++++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall.c    |  6 +++++
>  3 files changed, 64 insertions(+)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>