On 9 July 2016 at 21:07, Wirth, Allan <awi...@akamai.com> wrote: > Note that x86_64 has only _rt signal handlers. This implementation > attempts to share code with the x86_32 implementation. > > Reported-by: Timothy Pearson <tpear...@raptorengineering.com> > Suggested-by: Peter Maydell <peter.mayd...@linaro.org> > Signed-off-by: Allan Wirth <awi...@akamai.com>
Thanks. This mostly looks good; I have one comment below about a definite bug, and the rest are minor style comments. The commit message subject line should start with a "linux-user: " prefix. > --- > linux-user/signal.c | 344 > ++++++++++++++++++++++++++++++++++++----------- > target-i386/cpu.h | 2 + > target-i386/fpu_helper.c | 12 ++ > 3 files changed, 282 insertions(+), 76 deletions(-) > > diff --git a/linux-user/signal.c b/linux-user/signal.c > index 9d98045..edca9e4 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -253,8 +253,7 @@ int do_sigprocmask(int how, const sigset_t *set, sigset_t > *oldset) > return 0; > } > > -#if !defined(TARGET_OPENRISC) && !defined(TARGET_UNICORE32) && \ > - !defined(TARGET_X86_64) > +#if !defined(TARGET_OPENRISC) && !defined(TARGET_UNICORE32) > /* Just set the guest's signal mask to the specified value; the > * caller is assumed to have called block_signals() already. > */ > @@ -789,9 +788,8 @@ int do_sigaction(int sig, const struct target_sigaction > *act, > return ret; > } > > -#if defined(TARGET_I386) && TARGET_ABI_BITS == 32 > - > -/* from the Linux kernel */ > +#if defined(TARGET_I386) > +/* from the Linux kernel - /arch/x86/include/uapi/asm/sigcontext.h */ > > struct target_fpreg { > uint16_t significand[4]; > @@ -805,7 +803,7 @@ struct target_fpxreg { > }; > > struct target_xmmreg { > - abi_ulong element[4]; > + uint32_t element[4]; > }; > > struct target_fpstate { > @@ -830,33 +828,117 @@ struct target_fpstate { > abi_ulong padding[56]; > }; > > -#define X86_FXSR_MAGIC 0x0000 > +struct target_fpstate_32 { > + /* Regular FPU environment */ > + uint32_t cw; > + uint32_t sw; > + uint32_t tag; > + uint32_t ipoff; > + uint32_t cssel; > + uint32_t dataoff; > + uint32_t datasel; > + struct target_fpreg _st[8]; Better to avoid the leading underscores. > + uint16_t status; > + uint16_t magic; /* 0xffff = regular FPU data only */ > > -struct target_sigcontext { > + /* FXSR FPU environment */ > + uint32_t _fxsr_env[6]; /* FXSR FPU env is ignored */ > + uint32_t mxcsr; > + uint32_t reserved; > + struct target_fpxreg _fxsr_st[8]; /* FXSR FPU reg data is ignored */ > + struct target_xmmreg _xmm[8]; > + uint32_t padding[56]; > +}; > + > +struct target_fpstate_64 { > + /* FXSAVE format */ > + uint16_t cw; > + uint16_t sw; > + uint16_t twd; > + uint16_t fop; > + uint64_t rip; > + uint64_t rdp; > + uint32_t mxcsr; > + uint32_t mxcsr_mask; > + uint32_t st_space[32]; > + uint32_t xmm_space[64]; > + uint32_t reserved[24]; > +}; > + > +#ifndef TARGET_X86_64 > +# define target_fpstate target_fpstate_32 > +#else > +# define target_fpstate target_fpstate_64 > +#endif > + > +struct target_sigcontext_32 { > uint16_t gs, __gsh; > uint16_t fs, __fsh; > uint16_t es, __esh; > uint16_t ds, __dsh; > - abi_ulong edi; > - abi_ulong esi; > - abi_ulong ebp; > - abi_ulong esp; > - abi_ulong ebx; > - abi_ulong edx; > - abi_ulong ecx; > - abi_ulong eax; > - abi_ulong trapno; > - abi_ulong err; > - abi_ulong eip; > + uint32_t edi; > + uint32_t esi; > + uint32_t ebp; > + uint32_t esp; > + uint32_t ebx; > + uint32_t edx; > + uint32_t ecx; > + uint32_t eax; > + uint32_t trapno; > + uint32_t err; > + uint32_t eip; > uint16_t cs, __csh; > - abi_ulong eflags; > - abi_ulong esp_at_signal; > + uint32_t eflags; > + uint32_t esp_at_signal; > uint16_t ss, __ssh; > - abi_ulong fpstate; /* pointer */ > - abi_ulong oldmask; > - abi_ulong cr2; > + uint32_t fpstate; /* pointer */ > + uint32_t oldmask; > + uint32_t cr2; > }; > > +struct target_sigcontext_64 { > + uint64_t r8; > + uint64_t r9; > + uint64_t r10; > + uint64_t r11; > + uint64_t r12; > + uint64_t r13; > + uint64_t r14; > + uint64_t r15; > + > + uint64_t rdi; > + uint64_t rsi; > + uint64_t rbp; > + uint64_t rbx; > + uint64_t rdx; > + uint64_t rax; > + uint64_t rcx; > + uint64_t rsp; > + uint64_t rip; > + > + uint64_t eflags; > + > + uint16_t cs; > + uint16_t gs; > + uint16_t fs; > + uint16_t ss; > + > + uint64_t err; > + uint64_t trapno; > + uint64_t oldmask; > + uint64_t cr2; > + > + uint64_t fpstate; /* pointer */ > + uint64_t padding[8]; > +}; > + > +#ifndef TARGET_X86_64 > +# define target_sigcontext target_sigcontext_32 > +#else > +# define target_sigcontext target_sigcontext_64 > +#endif > + > +/* see Linux/include/uapi/asm-generic/ucontext.h */ > struct target_ucontext { > abi_ulong tuc_flags; > abi_ulong tuc_link; > @@ -865,6 +947,7 @@ struct target_ucontext { > target_sigset_t tuc_sigmask; /* mask last for extensibility */ > }; > > +#ifndef TARGET_X86_64 > struct sigframe > { > abi_ulong pretcode; > @@ -887,6 +970,18 @@ struct rt_sigframe > char retcode[8]; > }; > > +#else > + > +struct rt_sigframe > +{ This brace should be on the line before. If you run your patch through scripts/checkpatch.pl it will catch this nit (and another ten coding style issues that I'm not going to bother to list out below). > + abi_ulong pretcode; > + struct target_ucontext uc; > + struct target_siginfo info; > + struct target_fpstate fpstate; > +}; > + > +#endif > + > /* > * Set up a signal frame. > */ > @@ -897,6 +992,7 @@ static void setup_sigcontext(struct target_sigcontext *sc, > abi_ulong fpstate_addr) > { > CPUState *cs = CPU(x86_env_get_cpu(env)); > +#ifndef TARGET_X86_64 > uint16_t magic; > > /* already locked in setup_frame() */ > @@ -929,12 +1025,49 @@ static void setup_sigcontext(struct target_sigcontext > *sc, > /* non-iBCS2 extensions.. */ > __put_user(mask, &sc->oldmask); > __put_user(env->cr[2], &sc->cr2); > +#else > + __put_user(env->regs[8], &sc->r8); > + __put_user(env->regs[9], &sc->r9); > + __put_user(env->regs[10], &sc->r10); > + __put_user(env->regs[11], &sc->r11); > + __put_user(env->regs[12], &sc->r12); > + __put_user(env->regs[13], &sc->r13); > + __put_user(env->regs[14], &sc->r14); > + __put_user(env->regs[15], &sc->r15); > + > + __put_user(env->regs[R_EDI], &sc->rdi); > + __put_user(env->regs[R_ESI], &sc->rsi); > + __put_user(env->regs[R_EBP], &sc->rbp); > + __put_user(env->regs[R_EBX], &sc->rbx); > + __put_user(env->regs[R_EDX], &sc->rdx); > + __put_user(env->regs[R_EAX], &sc->rax); > + __put_user(env->regs[R_ECX], &sc->rcx); > + __put_user(env->regs[R_ESP], &sc->rsp); > + __put_user(env->eip, &sc->rip); > + > + __put_user(env->eflags, &sc->eflags); > + > + __put_user(env->segs[R_CS].selector, &sc->cs); > + __put_user((uint16_t)0, &sc->gs); > + __put_user((uint16_t)0, &sc->fs); > + __put_user(env->segs[R_SS].selector, &sc->ss); > + > + __put_user(env->error_code, &sc->err); > + __put_user(cs->exception_index, &sc->trapno); > + __put_user(mask, &sc->oldmask); > + __put_user(env->cr[2], &sc->cr2); > + > + /* fpstate_addr must be 16 byte aligned for fxsave */ > + assert (! (fpstate_addr & 0xf)); > + > + cpu_x86_fxsave(env, fpstate_addr); > + __put_user(fpstate_addr, &sc->fpstate); > +#endif > } > > /* > * Determine which stack to use.. > */ > - Stray whitespace change. > static inline abi_ulong > get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t > frame_size) > { > @@ -942,63 +1075,98 @@ get_sigframe(struct target_sigaction *ka, CPUX86State > *env, size_t frame_size) > > /* Default to using normal stack */ > esp = env->regs[R_ESP]; > +#ifdef TARGET_X86_64 > + esp -= 128; /* this is the redzone */ > +#endif > + > /* This is the X/Open sanctioned signal stack switching. */ > if (ka->sa_flags & TARGET_SA_ONSTACK) { > if (sas_ss_flags(esp) == 0) { > esp = target_sigaltstack_used.ss_sp + > target_sigaltstack_used.ss_size; > } > +#ifndef TARGET_X86_64 This would be better after the "} else {" so we don't have mismatched nesting of #if..#endif and if..else... > } else { > - > /* This is the legacy signal stack switching. */ > if ((env->segs[R_SS].selector & 0xffff) != __USER_DS && > !(ka->sa_flags & TARGET_SA_RESTORER) && > ka->sa_restorer) { > esp = (unsigned long) ka->sa_restorer; > } > +#endif > } > + > +#ifndef TARGET_X86_64 > return (esp - frame_size) & -8ul; > +#else > + return ((esp - frame_size) & (~15ul)) - 8; > +#endif > } > > -/* compare linux/arch/i386/kernel/signal.c:setup_frame() */ > -static void setup_frame(int sig, struct target_sigaction *ka, > - target_sigset_t *set, CPUX86State *env) > +/* compare linux/arch/i386/kernel/signal.c:setup_rt_frame() */ > +static void setup_rt_frame(int sig, struct target_sigaction *ka, > + target_siginfo_t *info, > + target_sigset_t *set, CPUX86State *env) Why have you switched the order of these two functions? It makes the diff very hard to read :-( > { > abi_ulong frame_addr; > - struct sigframe *frame; > +#ifndef TARGET_X86_64 > + abi_ulong addr; > +#endif > + struct rt_sigframe *frame; > int i; > > frame_addr = get_sigframe(ka, env, sizeof(*frame)); > - trace_user_setup_frame(env, frame_addr); > + trace_user_setup_rt_frame(env, frame_addr); > > if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) > goto give_sigsegv; > > + /* These fields are only in rt_sigframe on 32 bit...? */ Yes; you can remove the "?" from the comment. (They correspond to setting up arguments on the stack for the signal handler. x86-64's ABI passes arguments in registers; see later.) > +#ifndef TARGET_X86_64 > __put_user(sig, &frame->sig); > + addr = frame_addr + offsetof(struct rt_sigframe, info); > + __put_user(addr, &frame->pinfo); > + addr = frame_addr + offsetof(struct rt_sigframe, uc); > + __put_user(addr, &frame->puc); > +#endif > + if (info) { > + tswap_siginfo(&frame->info, info); > + } > > - setup_sigcontext(&frame->sc, &frame->fpstate, env, set->sig[0], > - frame_addr + offsetof(struct sigframe, fpstate)); > + /* Create the ucontext. */ > + __put_user(0, &frame->uc.tuc_flags); > + __put_user(0, &frame->uc.tuc_link); > + __put_user(target_sigaltstack_used.ss_sp, &frame->uc.tuc_stack.ss_sp); > + __put_user(sas_ss_flags(get_sp_from_cpustate(env)), > + &frame->uc.tuc_stack.ss_flags); > + __put_user(target_sigaltstack_used.ss_size, > + &frame->uc.tuc_stack.ss_size); > + setup_sigcontext(&frame->uc.tuc_mcontext, &frame->fpstate, env, > + set->sig[0], frame_addr + offsetof(struct rt_sigframe, fpstate)); > > - for(i = 1; i < TARGET_NSIG_WORDS; i++) { > - __put_user(set->sig[i], &frame->extramask[i - 1]); > + for(i = 0; i < TARGET_NSIG_WORDS; i++) { > + __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]); > } > > /* Set up to return from userspace. If provided, use a stub > already in userspace. */ > +#ifndef TARGET_X86_64 > if (ka->sa_flags & TARGET_SA_RESTORER) { > __put_user(ka->sa_restorer, &frame->pretcode); > } else { > uint16_t val16; > - abi_ulong retcode_addr; > - retcode_addr = frame_addr + offsetof(struct sigframe, retcode); > - __put_user(retcode_addr, &frame->pretcode); > - /* This is popl %eax ; movl $,%eax ; int $0x80 */ > - val16 = 0xb858; > - __put_user(val16, (uint16_t *)(frame->retcode+0)); > - __put_user(TARGET_NR_sigreturn, (int *)(frame->retcode+2)); > + addr = frame_addr + offsetof(struct rt_sigframe, retcode); > + __put_user(addr, &frame->pretcode); > + /* This is movl $,%eax ; int $0x80 */ > + __put_user(0xb8, (char *)(frame->retcode+0)); > + __put_user(TARGET_NR_rt_sigreturn, (int *)(frame->retcode+1)); > val16 = 0x80cd; > - __put_user(val16, (uint16_t *)(frame->retcode+6)); > + __put_user(val16, (uint16_t *)(frame->retcode+5)); > } > - > +#else > + /// XXX: Would be slightly better to return -EFAULT here if test fails > + assert(ka->sa_flags & TARGET_SA_RESTORER); Don't assert() on things the guest can provoke, please. > + __put_user(ka->sa_restorer, &frame->pretcode); > +#endif > > /* Set up registers for signal handler */ > env->regs[R_ESP] = frame_addr; If you try a test program that registers a handler and prints the arguments to the signal handler (either just the signal number, or all 3 arguments if you use sigaction and set SA_SIGINFO) you will find that your code doesn't correctly pass the parameters for x86-64, because you aren't setting the necessary registers here. Compare http://lxr.free-electrons.com/source/arch/x86/kernel/signal.c#L496 where we set EAX, EDI, ESI, and EDX. > @@ -1021,41 +1189,28 @@ give_sigsegv: > force_sig(TARGET_SIGSEGV /* , current */); > } > > -/* compare linux/arch/i386/kernel/signal.c:setup_rt_frame() */ > -static void setup_rt_frame(int sig, struct target_sigaction *ka, > - target_siginfo_t *info, > - target_sigset_t *set, CPUX86State *env) > +/* compare linux/arch/i386/kernel/signal.c:setup_frame() */ > +static void setup_frame(int sig, struct target_sigaction *ka, > + target_sigset_t *set, CPUX86State *env) > { > - abi_ulong frame_addr, addr; > - struct rt_sigframe *frame; > +#ifndef TARGET_X86_64 > + abi_ulong frame_addr; > + struct sigframe *frame; > int i; > > frame_addr = get_sigframe(ka, env, sizeof(*frame)); > - trace_user_setup_rt_frame(env, frame_addr); > + trace_user_setup_frame(env, frame_addr); > > if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) > goto give_sigsegv; > > __put_user(sig, &frame->sig); > - addr = frame_addr + offsetof(struct rt_sigframe, info); > - __put_user(addr, &frame->pinfo); > - addr = frame_addr + offsetof(struct rt_sigframe, uc); > - __put_user(addr, &frame->puc); > - tswap_siginfo(&frame->info, info); > > - /* Create the ucontext. */ > - __put_user(0, &frame->uc.tuc_flags); > - __put_user(0, &frame->uc.tuc_link); > - __put_user(target_sigaltstack_used.ss_sp, &frame->uc.tuc_stack.ss_sp); > - __put_user(sas_ss_flags(get_sp_from_cpustate(env)), > - &frame->uc.tuc_stack.ss_flags); > - __put_user(target_sigaltstack_used.ss_size, > - &frame->uc.tuc_stack.ss_size); > - setup_sigcontext(&frame->uc.tuc_mcontext, &frame->fpstate, env, > - set->sig[0], frame_addr + offsetof(struct rt_sigframe, fpstate)); > + setup_sigcontext(&frame->sc, &frame->fpstate, env, set->sig[0], > + frame_addr + offsetof(struct sigframe, fpstate)); > > - for(i = 0; i < TARGET_NSIG_WORDS; i++) { > - __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]); > + for(i = 1; i < TARGET_NSIG_WORDS; i++) { > + __put_user(set->sig[i], &frame->extramask[i - 1]); > } > > /* Set up to return from userspace. If provided, use a stub > @@ -1064,15 +1219,18 @@ static void setup_rt_frame(int sig, struct > target_sigaction *ka, > __put_user(ka->sa_restorer, &frame->pretcode); > } else { > uint16_t val16; > - addr = frame_addr + offsetof(struct rt_sigframe, retcode); > - __put_user(addr, &frame->pretcode); > - /* This is movl $,%eax ; int $0x80 */ > - __put_user(0xb8, (char *)(frame->retcode+0)); > - __put_user(TARGET_NR_rt_sigreturn, (int *)(frame->retcode+1)); > + abi_ulong retcode_addr; > + retcode_addr = frame_addr + offsetof(struct sigframe, retcode); > + __put_user(retcode_addr, &frame->pretcode); > + /* This is popl %eax ; movl $,%eax ; int $0x80 */ > + val16 = 0xb858; > + __put_user(val16, (uint16_t *)(frame->retcode+0)); > + __put_user(TARGET_NR_sigreturn, (int *)(frame->retcode+2)); > val16 = 0x80cd; > - __put_user(val16, (uint16_t *)(frame->retcode+5)); > + __put_user(val16, (uint16_t *)(frame->retcode+6)); > } > > + > /* Set up registers for signal handler */ > env->regs[R_ESP] = frame_addr; > env->eip = ka->_sa_handler; > @@ -1092,15 +1250,19 @@ give_sigsegv: > ka->_sa_handler = TARGET_SIG_DFL; > } > force_sig(TARGET_SIGSEGV /* , current */); > +#else > + setup_rt_frame(sig, ka, 0, set, env); Why is this here? setup_frame() should never be called if the architecture doesn't implement it -- you should adjust the #if in handle_pending_signal(), and then put the whole setup_frame() function definition inside an #ifndef TARGET_X86_64. > +#endif > } > > static int > restore_sigcontext(CPUX86State *env, struct target_sigcontext *sc) > { > - unsigned int err = 0; > abi_ulong fpstate_addr; > unsigned int tmpflags; > + unsigned int err = 0; > > +#ifndef TARGET_X86_64 > cpu_x86_load_seg(env, R_GS, tswap16(sc->gs)); > cpu_x86_load_seg(env, R_FS, tswap16(sc->fs)); > cpu_x86_load_seg(env, R_ES, tswap16(sc->es)); > @@ -1114,7 +1276,29 @@ restore_sigcontext(CPUX86State *env, struct > target_sigcontext *sc) > env->regs[R_EDX] = tswapl(sc->edx); > env->regs[R_ECX] = tswapl(sc->ecx); > env->regs[R_EAX] = tswapl(sc->eax); > + > env->eip = tswapl(sc->eip); > +#else > + env->regs[8] = tswapl(sc->r8); > + env->regs[9] = tswapl(sc->r9); > + env->regs[10] = tswapl(sc->r10); > + env->regs[11] = tswapl(sc->r11); > + env->regs[12] = tswapl(sc->r12); > + env->regs[13] = tswapl(sc->r13); > + env->regs[14] = tswapl(sc->r14); > + env->regs[15] = tswapl(sc->r15); > + > + env->regs[R_EDI] = tswapl(sc->rdi); > + env->regs[R_ESI] = tswapl(sc->rsi); > + env->regs[R_EBP] = tswapl(sc->rbp); > + env->regs[R_EBX] = tswapl(sc->rbx); > + env->regs[R_EDX] = tswapl(sc->rdx); > + env->regs[R_EAX] = tswapl(sc->rax); > + env->regs[R_ECX] = tswapl(sc->rcx); > + env->regs[R_ESP] = tswapl(sc->rsp); > + > + env->eip = tswapl(sc->rip); > +#endif > > cpu_x86_load_seg(env, R_CS, lduw_p(&sc->cs) | 3); > cpu_x86_load_seg(env, R_SS, lduw_p(&sc->ss) | 3); > @@ -1128,14 +1312,21 @@ restore_sigcontext(CPUX86State *env, struct > target_sigcontext *sc) > if (!access_ok(VERIFY_READ, fpstate_addr, > sizeof(struct target_fpstate))) > goto badframe; > +#ifndef TARGET_X86_64 > cpu_x86_frstor(env, fpstate_addr, 1); > +#else > + cpu_x86_fxrstor(env, fpstate_addr); > +#endif > } > > return err; > + Stray whitespace change. > badframe: > return 1; > } > > +/* Note: there is no sigreturn on x86_64, there is only rt_sigreturn */ > +#ifndef TARGET_X86_64 > long do_sigreturn(CPUX86State *env) > { > struct sigframe *frame; > @@ -1167,6 +1358,7 @@ badframe: > force_sig(TARGET_SIGSEGV); > return 0; > } > +#endif > > long do_rt_sigreturn(CPUX86State *env) > { > @@ -1174,7 +1366,7 @@ long do_rt_sigreturn(CPUX86State *env) > struct rt_sigframe *frame; > sigset_t set; > > - frame_addr = env->regs[R_ESP] - 4; > + frame_addr = env->regs[R_ESP] - sizeof(abi_ulong); > trace_user_do_rt_sigreturn(env, frame_addr); > if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1)) > goto badframe; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 474b0b9..90410d8 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1337,6 +1337,8 @@ floatx80 cpu_set_fp80(uint64_t mant, uint16_t upper); > void cpu_x86_load_seg(CPUX86State *s, int seg_reg, int selector); > void cpu_x86_fsave(CPUX86State *s, target_ulong ptr, int data32); > void cpu_x86_frstor(CPUX86State *s, target_ulong ptr, int data32); > +void cpu_x86_fxsave(CPUX86State *s, target_ulong ptr); > +void cpu_x86_fxrstor(CPUX86State *s, target_ulong ptr); > > /* you can call this signal handler from your SIGBUS and SIGSEGV > signal handlers to inform the virtual CPU of exceptions. non zero > diff --git a/target-i386/fpu_helper.c b/target-i386/fpu_helper.c > index 929489b..617fdf2 100644 > --- a/target-i386/fpu_helper.c > +++ b/target-i386/fpu_helper.c > @@ -1370,6 +1370,18 @@ void helper_fxrstor(CPUX86State *env, target_ulong ptr) > } > } > > +#if defined(CONFIG_USER_ONLY) > +void cpu_x86_fxsave(CPUX86State *env, target_ulong ptr) > +{ > + helper_fxsave(env, ptr); > +} > + > +void cpu_x86_fxrstor(CPUX86State *env, target_ulong ptr) > +{ > + helper_fxrstor(env, ptr); > +} > +#endif > + > void helper_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm) > { > uintptr_t ra = GETPC(); > -- > 1.9.1 thanks -- PMM