Re: [Qemu-devel] [PATCH 1/7] Add initial x86_64 signal handlers

[Peter Maydell]

On 19 June 2016 at 01:11, Timothy Pearson
<address@hidden> wrote:
> Note that x86_64 systems only offer the _rt signal handler variants,
> so the legacy signal handlers remain unimplemented on this platform.
>
> Signed-off-by: Timothy Pearson <address@hidden>
> ---
>  linux-user/signal.c | 302
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 299 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 61c1145..88d8fd3 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -26,6 +26,16 @@
>  #include "target_signal.h"
>  #include "trace.h"
>  +/*
> + * This looks more complex than it should be. But we need to
> + * get the type for the ~ right in round_down (it needs to be
> + * as wide as the result!), and we want to evaluate the macro
> + * arguments just once each.
> + */
> +#define __round_mask(x, y) ((__typeof__(x))((y)-1))
> +#define round_up(x, y) ((((x)-1) | __round_mask(x, y))+1)
> +#define round_down(x, y) ((x) & ~__round_mask(x, y))

You don't use round_up(), and only use round_down() once, in
  round_down(esp - frame_size, 16)

I think you should either
 (1) just write this as (esp - frame_size) & ~15ULL
or
 (2) provide a round-down function in the QEMU include headers
(probably calling it ROUND_DOWN to match our current ROUND_UP,
probably using the same "-(d)" mask our ROUND_UP does rather
than messing with typeof and using ~((d) - 1) as this does.

(1) is obviously easier...

>  static struct target_sigaltstack target_sigaltstack_used = {
>      .ss_sp = 0,
>      .ss_size = 0,
> @@ -256,8 +266,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.
>   */
> @@ -1185,6 +1194,292 @@ badframe:
>      return 0;
>  }
>  +#elif defined(TARGET_X86_64) && TARGET_ABI_BITS == 64

A lot of this is very similar to the 32-bit x86 code, so
perhaps we should do this by having one set of functions
with #if TARGET_ABI_BITS == 32 / #else / #endif conditionals
within those functions as needed?  (I don't insist on it.)

> +
> +struct target_fpxreg {
> +    uint16_t significand[4];
> +    uint16_t exponent;
> +    uint16_t padding[3];
> +};
> +
> +struct target_xmmreg {
> +    abi_ulong element[4];

The kernel headers say this is a 32-bit int, not a long (which is
64 bits for x86-64).

> +};
> +
> +struct target_fpstate {
> +    unsigned short cwd;
> +    unsigned short swd;
> +    unsigned short twd;
> +
> +    unsigned short fop;
> +    uint64_t rip;
> +    uint64_t rdp;
> +    abi_ulong mxcsr;
> +    abi_ulong mxcsr_mask;
> +    struct target_fpxreg _fxsr_st[8]; /* FXSR FPU reg data is ignored */
> +    struct target_xmmreg _xmm[16];
> +    __u32 padding[24];
> +};

I was expecting this struct layout to match the kernel 'struct _fpstate_64'.

> +
> +#define X86_FXSR_MAGIC         0x0000
> +
> +struct target_sigcontext {
> +    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 edi;
> +    uint64_t esi;
> +    uint64_t ebp;
> +    uint64_t ebx;
> +    uint64_t edx;
> +    uint64_t eax;
> +    uint64_t ecx;
> +    uint64_t esp;
> +    uint64_t eip;
> +    uint64_t eflags;
> +    uint16_t cs;
> +    uint16_t gs;
> +    uint16_t fs;
> +    uint16_t __pad0;
> +    uint64_t err;
> +    uint64_t trapno;
> +    uint64_t oldmask;
> +    uint64_t cr2;
> +
> +    uint64_t fpstate;
> +    uint64_t reserved1[8];
> +};
> +
> +struct target_ucontext {
> +    abi_ulong                  tuc_flags;
> +    struct target_sigcontext*  tuc_link;
> +    target_stack_t             tuc_stack;
> +    struct target_sigcontext   tuc_mcontext;
> +    target_sigset_t            tuc_sigmask;  /* mask last for extensibility 
> */
> +};
> +
> +struct rt_sigframe
> +{
> +    char* pretcode;
> +    struct target_ucontext uc;
> +    struct target_siginfo info;
> +    struct target_fpstate fpstate;
> +};
> +
> +/*
> + * Set up a signal frame.
> + */
> +
> +/* XXX: save x87 state */
> +static void setup_sigcontext(struct target_sigcontext *sc,
> +        struct target_fpstate *fpstate, CPUX86State *env, abi_ulong mask,
> +        abi_ulong fpstate_addr)
> +{
> +    CPUState *cs = CPU(x86_env_get_cpu(env));
> +
> +    /* already locked in setup_frame() */
> +    __put_user(env->regs[R_EDI], &sc->edi);
> +    __put_user(env->regs[R_ESI], &sc->esi);
> +    __put_user(env->regs[R_EBP], &sc->ebp);
> +    __put_user(env->regs[R_ESP], &sc->esp);
> +    __put_user(env->regs[R_EBX], &sc->ebx);
> +    __put_user(env->regs[R_EDX], &sc->edx);
> +    __put_user(env->regs[R_ECX], &sc->ecx);
> +    __put_user(env->regs[R_EAX], &sc->eax);
> +    __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(cs->exception_index, &sc->trapno);
> +    __put_user(env->error_code, &sc->err);
> +    __put_user(env->eip, &sc->eip);
> +    __put_user(env->eflags, &sc->eflags);
> +    __put_user(env->segs[R_CS].selector, (unsigned int *)&sc->cs);

64-bit shouldn't have this cast.

> +    __put_user(0, &sc->gs);
> +    __put_user(0, &sc->fs);
> +
> +    cpu_x86_fsave(env, fpstate_addr, 1);

The kernel seems to use an xsave state for 64-bit FPU state, not
the 32-bit structure, so this probably isn't correct.

> +    __put_user(fpstate_addr, &sc->fpstate);
> +
> +    /* non-iBCS2 extensions.. */
> +    __put_user(mask, &sc->oldmask);
> +    __put_user(env->cr[2], &sc->cr2);
> +}
> +
> +/*
> + * Determine which stack to use..
> + */
> +
> +static inline abi_ulong
> +get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t
> frame_size)
> +{
> +    unsigned long esp;
> +
> +    /* Default to using normal stack */
> +    esp = env->regs[R_ESP];
> +
> +    /* redzone */
> +    esp -= 128;
> +
> +    /* 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;
> +        }
> +    } 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;

I think this legacy stack switching is 32-bit only, looking
at the kernel get_sigframe().

> +        }
> +    }
> +    return round_down(esp - frame_size, 16) - 8;
> +}
> +
> +/* 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)
> +{
> +    abi_ulong frame_addr;
> +    struct rt_sigframe *frame;
> +    int i;
> +
> +    frame_addr = get_sigframe(ka, env, sizeof(*frame));
> +    trace_user_setup_rt_frame(env, frame_addr);
> +
> +    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> +        goto give_sigsegv;

Our coding style has braces on all if statements. (If you run
scripts/checkpatch.pl on your patch it will help spot things like
this.)

> +
> +    __put_user(target_sigaltstack_used.ss_size,
> +               &frame->uc.tuc_stack.ss_size);
> +
> +    /* 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);
> +
> +    setup_sigcontext(&frame->uc.tuc_mcontext, &frame->fpstate, env,
> +            set->sig[0], frame_addr + offsetof(struct rt_sigframe,
> fpstate));
> +
> +    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.  */
> +    if (ka->sa_flags & TARGET_SA_RESTORER) {
> +        __put_user(ka->sa_restorer, &frame->pretcode);
> +    } else {
> +        goto give_sigsegv;
> +    }
> +
> +    /* Set up registers for signal handler */
> +    env->regs[R_ESP] = frame_addr;
> +    env->eip = ka->_sa_handler;
> +
> +    env->eflags &= ~TF_MASK;
> +
> +    unlock_user_struct(frame, frame_addr, 1);
> +
> +    return;
> +
> +give_sigsegv:
> +    if (sig == TARGET_SIGSEGV) {
> +        ka->_sa_handler = TARGET_SIG_DFL;
> +    }
> +    force_sig(TARGET_SIGSEGV /* , current */);
> +}
> +
> +static int
> +restore_sigcontext(CPUX86State *env, struct target_sigcontext *sc)
> +{
> +    unsigned int err = 0;
> +    abi_ulong fpstate_addr;
> +    unsigned int tmpflags;
> +
> +    env->regs[R_EDI] = tswapl(sc->edi);
> +    env->regs[R_ESI] = tswapl(sc->esi);

Please use __get_user() for all these; it handles the case
where the value in guest memory is not sufficiently aligned
for the host CPU.

> +    env->regs[R_EBP] = tswapl(sc->ebp);
> +    env->regs[R_ESP] = tswapl(sc->esp);
> +    env->regs[R_EBX] = tswapl(sc->ebx);
> +    env->regs[R_EDX] = tswapl(sc->edx);
> +    env->regs[R_ECX] = tswapl(sc->ecx);
> +    env->regs[R_EAX] = tswapl(sc->eax);
> +    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->eip = tswapl(sc->eip);
> +
> +    cpu_x86_load_seg(env, R_CS, lduw_p(&sc->cs) | 3);
> +
> +    tmpflags = tswapl(sc->eflags);
> +    env->eflags = (env->eflags & ~0x40DD5) | (tmpflags & 0x40DD5);
> +    //         regs->orig_eax = -1;            /* disable syscall checks */
> +
> +    fpstate_addr = tswapl(sc->fpstate);
> +    if (fpstate_addr != 0) {
> +        if (!access_ok(VERIFY_READ, fpstate_addr,
> +                       sizeof(struct target_fpstate)))
> +            goto badframe;
> +        cpu_x86_frstor(env, fpstate_addr, 1);
> +    }

Again, I think this is assuming 32-bit fpstate layout.

> +
> +    return err;
> +badframe:
> +    return 1;
> +}
> +
> +long do_rt_sigreturn(CPUX86State *env)
> +{
> +    abi_ulong frame_addr;
> +    struct rt_sigframe *frame;
> +    sigset_t set;
> +
> +    frame_addr = env->regs[R_ESP] - 8;
> +    trace_user_do_rt_sigreturn(env, frame_addr);
> +    if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
> +        goto badframe;
> +    target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
> +    set_sigmask(&set);
> +
> +    if (restore_sigcontext(env, &frame->uc.tuc_mcontext)) {
> +        goto badframe;
> +    }
> +
> +    if (do_sigaltstack(frame_addr + offsetof(struct rt_sigframe,
> uc.tuc_stack), 0,
> +                       get_sp_from_cpustate(env)) == -EFAULT) {
> +        goto badframe;
> +    }
> +
> +    unlock_user_struct(frame, frame_addr, 0);
> +    return -TARGET_QEMU_ESIGRETURN;
> +
> +badframe:
> +    unlock_user_struct(frame, frame_addr, 0);
> +    force_sig(TARGET_SIGSEGV);
> +    return 0;
> +}
> +
>  #elif defined(TARGET_AARCH64)
>   struct target_sigcontext {
> @@ -5883,7 +6178,8 @@ static void handle_pending_signal(CPUArchState
> *cpu_env, int sig)
>  #endif
>          /* prepare the stack frame of the virtual CPU */
>  #if defined(TARGET_ABI_MIPSN32) || defined(TARGET_ABI_MIPSN64) \
> -    || defined(TARGET_OPENRISC) || defined(TARGET_TILEGX)
> +    || defined(TARGET_OPENRISC) || defined(TARGET_TILEGX) \
> +    || defined(TARGET_X86_64)
>          /* These targets do not have traditional signals.  */
>          setup_rt_frame(sig, sa, &k->info, &target_old_set, cpu_env);
>  #else
> --
> 2.1.4

thanks
-- PMM