Re: [Qemu-ppc] [PATCH 3/3] ppc: Fix signal delivery in 64-bit usermode qemu

[Benjamin Herrenschmidt]

On Wed, 2016-08-03 at 13:16 +1000, Benjamin Herrenschmidt wrote:
> There were a number of bugs in the implementation. The structure
> alignment was wrong for 64-bit. Also 64-bit only does RT signals.
> 
> Finally on 64-bit, we need to put a pointer to the (aligned)
> vector registers in the frame.
> 
> This is still missing support for VSX which I will add separately.
> 
> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> ---

Dont apply this just yet ... the patch is correct but doesn't fix all
the bugs :-) There's some endian crap too. Will post a new one.

>  linux-user/ppc/syscall_nr.h |  2 ++
>  linux-user/signal.c         | 75 +++++++++++++++++++++++++--------
> ------------
>  2 files changed, 44 insertions(+), 33 deletions(-)
> 
> diff --git a/linux-user/ppc/syscall_nr.h b/linux-
> user/ppc/syscall_nr.h
> index 46ed8a6..afa3654 100644
> --- a/linux-user/ppc/syscall_nr.h
> +++ b/linux-user/ppc/syscall_nr.h
> @@ -120,7 +120,9 @@
>  #define TARGET_NR_sysinfo                116
>  #define TARGET_NR_ipc                    117
>  #define TARGET_NR_fsync                  118
> +#if !defined(TARGET_PPC64)
>  #define TARGET_NR_sigreturn              119
> +#endif
>  #define TARGET_NR_clone                  120
>  #define TARGET_NR_setdomainname          121
>  #define TARGET_NR_uname                  122
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 9a4d894..af80a3e 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -4408,7 +4408,12 @@ struct target_mcontext {
>      target_ulong mc_gregs[48];
>      /* Includes fpscr.  */
>      uint64_t mc_fregs[33];
> +#if defined(TARGET_PPC64)
> +    /* Pointer to the vector regs */
> +    target_ulong v_regs;
> +#else
>      target_ulong mc_pad[2];
> +#endif
>      /* We need to handle Altivec and SPE at the same time, which no
>         kernel needs to do.  Fortunately, the kernel defines this bit
> to
>         be Altivec-register-large all the time, rather than trying to
> @@ -4418,15 +4423,24 @@ struct target_mcontext {
>          uint32_t spe[33];
>          /* Altivec vector registers.  The packing of VSCR and VRSAVE
>             varies depending on whether we're PPC64 or not: PPC64
> splits
> -           them apart; PPC32 stuffs them together.  */
> +           them apart; PPC32 stuffs them together.
> +           We also need to account for the VSX registers on PPC64
> +        */
>  #if defined(TARGET_PPC64)
> -#define QEMU_NVRREG 34
> +#define QEMU_NVRREG (34 + 16)
> +        /* On ppc64, we need to align to 16 bytes by hand */
> +        target_ulong pad;
>  #else
> +        /* On ppc32, we are already aligned to 16 bytes */
>  #define QEMU_NVRREG 33
>  #endif
> -        ppc_avr_t altivec[QEMU_NVRREG];
> +        /* We cannot use ppc_avr_t here as we do *not* want the
> implied
> +         * 16-bytes alignment that would result from it. This would
> have
> +         * the effect of making. The 32-bit variant is already
> aligned.
> +         */
> +        uint64_t altivec[QEMU_NVRREG][2];
>  #undef QEMU_NVRREG
> -    } mc_vregs __attribute__((__aligned__(16)));
> +    } mc_vregs;
>  };
>  
>  /* See arch/powerpc/include/asm/sigcontext.h.  */
> @@ -4606,9 +4620,10 @@ static void save_user_regs(CPUPPCState *env,
> struct target_mcontext *frame)
>  
>      /* Save Altivec registers if necessary.  */
>      if (env->insns_flags & PPC_ALTIVEC) {
> +        uint32_t *vrsave;
>          for (i = 0; i < ARRAY_SIZE(env->avr); i++) {
>              ppc_avr_t *avr = &env->avr[i];
> -            ppc_avr_t *vreg = &frame->mc_vregs.altivec[i];
> +            ppc_avr_t *vreg = (ppc_avr_t *)&frame-
> >mc_vregs.altivec[i];
>  
>              __put_user(avr->u64[0], &vreg->u64[0]);
>              __put_user(avr->u64[1], &vreg->u64[1]);
> @@ -4616,8 +4631,14 @@ static void save_user_regs(CPUPPCState *env,
> struct target_mcontext *frame)
>          /* Set MSR_VR in the saved MSR value to indicate that
>             frame->mc_vregs contains valid data.  */
>          msr |= MSR_VR;
> -        __put_user((uint32_t)env->spr[SPR_VRSAVE],
> -                   &frame->mc_vregs.altivec[32].u32[3]);
> +#if defined(TARGET_PPC64)
> +        vrsave = (uint32_t *)&frame->mc_vregs.altivec[33];
> +        /* 64-bit needs to put a pointer to the vectors in the frame
> */
> +        __put_user(h2g(frame->mc_vregs.altivec), &frame->v_regs);
> +#else
> +        vrsave = (uint32_t *)&frame->mc_vregs.altivec[32];
> +#endif
> +        __put_user((uint32_t)env->spr[SPR_VRSAVE], vrsave);
>      }
>  
>      /* Save floating point registers.  */
> @@ -4697,17 +4718,22 @@ static void restore_user_regs(CPUPPCState
> *env,
>  
>      /* Restore Altivec registers if necessary.  */
>      if (env->insns_flags & PPC_ALTIVEC) {
> +        uint32_t *vrsave;
>          for (i = 0; i < ARRAY_SIZE(env->avr); i++) {
>              ppc_avr_t *avr = &env->avr[i];
> -            ppc_avr_t *vreg = &frame->mc_vregs.altivec[i];
> +            ppc_avr_t *vreg = (ppc_avr_t *)&frame-
> >mc_vregs.altivec[i];
>  
>              __get_user(avr->u64[0], &vreg->u64[0]);
>              __get_user(avr->u64[1], &vreg->u64[1]);
>          }
>          /* Set MSR_VEC in the saved MSR value to indicate that
>             frame->mc_vregs contains valid data.  */
> -        __get_user(env->spr[SPR_VRSAVE],
> -                   (target_ulong *)(&frame-
> >mc_vregs.altivec[32].u32[3]));
> +#if defined(TARGET_PPC64)
> +        vrsave = (uint32_t *)&frame->mc_vregs.altivec[33];
> +#else
> +        vrsave = (uint32_t *)&frame->mc_vregs.altivec[32];
> +#endif
> +        __get_user(env->spr[SPR_VRSAVE], vrsave);
>      }
>  
>      /* Restore floating point registers.  */
> @@ -4738,6 +4764,7 @@ static void restore_user_regs(CPUPPCState *env,
>      }
>  }
>  
> +#if !defined(TARGET_PPC64)
>  static void setup_frame(int sig, struct target_sigaction *ka,
>                          target_sigset_t *set, CPUPPCState *env)
>  {
> @@ -4745,9 +4772,6 @@ static void setup_frame(int sig, struct
> target_sigaction *ka,
>      struct target_sigcontext *sc;
>      target_ulong frame_addr, newsp;
>      int err = 0;
> -#if defined(TARGET_PPC64)
> -    struct image_info *image = ((TaskState *)thread_cpu->opaque)-
> >info;
> -#endif
>  
>      frame_addr = get_sigframe(ka, env, sizeof(*frame));
>      trace_user_setup_frame(env, frame_addr);
> @@ -4757,11 +4781,7 @@ static void setup_frame(int sig, struct
> target_sigaction *ka,
>  
>      __put_user(ka->_sa_handler, &sc->handler);
>      __put_user(set->sig[0], &sc->oldmask);
> -#if TARGET_ABI_BITS == 64
> -    __put_user(set->sig[0] >> 32, &sc->_unused[3]);
> -#else
>      __put_user(set->sig[1], &sc->_unused[3]);
> -#endif
>      __put_user(h2g(&frame->mctx), &sc->regs);
>      __put_user(sig, &sc->signal);
>  
> @@ -4790,22 +4810,7 @@ static void setup_frame(int sig, struct
> target_sigaction *ka,
>      env->gpr[3] = sig;
>      env->gpr[4] = frame_addr + offsetof(struct target_sigframe,
> sctx);
>  
> -#if defined(TARGET_PPC64)
> -    if (get_ppc64_abi(image) < 2) {
> -        /* ELFv1 PPC64 function pointers are pointers to OPD
> entries. */
> -        struct target_func_ptr *handler =
> -            (struct target_func_ptr *)g2h(ka->_sa_handler);
> -        env->nip = tswapl(handler->entry);
> -        env->gpr[2] = tswapl(handler->toc);
> -    } else {
> -        /* ELFv2 PPC64 function pointers are entry points, but R12
> -         * must also be set */
> -        env->nip = tswapl((target_ulong) ka->_sa_handler);
> -        env->gpr[12] = env->nip;
> -    }
> -#else
>      env->nip = (target_ulong) ka->_sa_handler;
> -#endif
>  
>      /* Signal handlers are entered in big-endian mode.  */
>      env->msr &= ~(1ull << MSR_LE);
> @@ -4817,6 +4822,7 @@ sigsegv:
>      unlock_user_struct(frame, frame_addr, 1);
>      force_sig(TARGET_SIGSEGV);
>  }
> +#endif /* !defined(TARGET_PPC64) */
>  
>  static void setup_rt_frame(int sig, struct target_sigaction *ka,
>                             target_siginfo_t *info,
> @@ -4914,6 +4920,7 @@ sigsegv:
>  
>  }
>  
> +#if !defined(TARGET_PPC64)
>  long do_sigreturn(CPUPPCState *env)
>  {
>      struct target_sigcontext *sc = NULL;
> @@ -4950,6 +4957,7 @@ sigsegv:
>      force_sig(TARGET_SIGSEGV);
>      return 0;
>  }
> +#endif /* !defined(TARGET_PPC64) */
>  
>  /* See arch/powerpc/kernel/signal_32.c.  */
>  static int do_setcontext(struct target_ucontext *ucp, CPUPPCState
> *env, int sig)
> @@ -5893,7 +5901,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_PPC64)
>          /* These targets do not have traditional signals.  */
>          setup_rt_frame(sig, sa, &k->info, &target_old_set, cpu_env);
>  #else