| Message ID | 20180718200648.22529-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux-user/ppc: Implement swapcontext syscall | expand |
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: >
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~
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
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~
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
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>
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>
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:
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(+) -- 2.17.1