Re: [Qemu-devel] [RFC PATCH] linux-user: Add signal handling for x86_64

[Pranith Kumar <bobby.prani@gmail.com>]

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 <awirth@akamai.com>
>> CC: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Pranith Kumar <bobby.prani@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