[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] linux-user: Add signal handling for x86_64
From: |
Pranith Kumar |
Subject: |
Re: [Qemu-devel] [RFC PATCH] linux-user: Add signal handling for x86_64 |
Date: |
Mon, 06 Feb 2017 02:04:51 -0500 |
User-agent: |
mu4e 0.9.17; emacs 25.1.1 |
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 <address@hidden>
>> CC: Peter Maydell <address@hidden>
>> Signed-off-by: Pranith Kumar <address@hidden>
>> ---
>> 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