Hi Laurent, Thanks for the review.
Laurent Vivier writes: > Le 25/01/2017 à 01:10, Pranith Kumar a écrit : >> Adopted from a previous patch posting: >> https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg02079.html >> >> CC: Allan Wirth <awi...@akamai.com> >> CC: Peter Maydell <peter.mayd...@linaro.org> >> Signed-off-by: Pranith Kumar <bobby.pr...@gmail.com> >> --- >> linux-user/signal.c | 264 >> ++++++++++++++++++++++++++++++++++++++++------- >> target/i386/cpu.h | 2 + >> target/i386/fpu_helper.c | 12 +++ >> 3 files changed, 242 insertions(+), 36 deletions(-) >> >> diff --git a/linux-user/signal.c b/linux-user/signal.c >> index 0a5bb4e26b..0248621d66 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. >> */ >> @@ -512,7 +511,7 @@ void signal_init(void) >> } >> } >> >> -#if !(defined(TARGET_X86_64) || defined(TARGET_UNICORE32)) >> +#ifndef TARGET_UNICORE32 >> /* Force a synchronously taken signal. The kernel force_sig() function >> * also forces the signal to "not blocked, not ignored", but for QEMU >> * that work is done in process_pending_signals(). >> @@ -819,9 +818,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]; >> @@ -835,7 +833,7 @@ struct target_fpxreg { >> }; >> >> struct target_xmmreg { >> - abi_ulong element[4]; >> + uint32_t element[4]; >> }; >> >> struct target_fpstate { >> @@ -860,33 +858,117 @@ struct target_fpstate { >> abi_ulong padding[56]; >> }; > > I think you should remove the definition of the target_fpstate structure > as you overwrite it with #define below: > ... >> + >> +#ifndef TARGET_X86_64 >> +# define target_fpstate target_fpstate_32 >> +#else >> +# define target_fpstate target_fpstate_64 >> +#endif >> + Good catch. I'll do that. > ... >> @@ -959,12 +1052,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 > > This part would be more readable if the registers were in the same order > as in the kernel function setup_sigcontext(). OK, noted this. I'll make the change. > ... >> + if (info) { >> + tswap_siginfo(&frame->info, info); >> + } > > kernel checks "ksig->ka.sa.sa_flags & SA_SIGINFO" to know if there is > siginfo structure. > OK. I'll do the same as what the kernel is doing. > ... >> /* Set up registers for signal handler */ >> env->regs[R_ESP] = frame_addr; >> + env->regs[R_EAX] = 0; >> + env->regs[R_EDI] = sig; >> + env->regs[R_ESI] = (unsigned long)&frame->info; >> + env->regs[R_EDX] = (unsigned long)&frame->uc; >> env->eip = ka->_sa_handler; > > In kernel, 32bit handler seems to not use the same registers as 64bit > handler, for instance ax is sig, info is dx and uc is cx. > Hmm.. how do I reproduce the same here? Should I check if we are in 32-bit environment? > ... >> @@ -6181,11 +6371,13 @@ static void handle_pending_signal(CPUArchState >> *cpu_env, int sig, >> || defined(TARGET_PPC64) || defined(TARGET_HPPA) >> /* These targets do not have traditional signals. */ >> setup_rt_frame(sig, sa, &k->info, &target_old_set, cpu_env); >> -#else >> +#elif !defined(TARGET_X86_64) >> if (sa->sa_flags & TARGET_SA_SIGINFO) >> setup_rt_frame(sig, sa, &k->info, &target_old_set, cpu_env); >> else >> setup_frame(sig, sa, &target_old_set, cpu_env); >> +#else >> + setup_rt_frame(sig, sa, 0, &target_old_set, cpu_env); > > Why do we use "0" instead of "&k->info"? That is a miss by me. I'll fix this. Thanks, -- Pranith
