Re: [PATCH for-5.0?] linux-user/ppc: Fix padding in mcontext_t for ppc64

[Laurent Vivier]

Le 07/04/2020 à 05:21, Richard Henderson a écrit :
> The padding that was added in 95cda4c44ee was added to a union,
> and so it had no effect.  This fixes misalignment errors detected
> by clang sanitizers for ppc64 and ppc64le.
> 
> In addition, only ppc64 allocates space for VSX registers, so do
> not save them for ppc32.  The kernel only has references to
> CONFIG_SPE in signal_32.c, so do not attempt to save them for ppc64.
> 
> Fixes: 95cda4c44ee
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> 
> Note that ppc64abi32 is *not* fixed by this patch.  It looks to
> me that all of the defined(TARGET_PPC64) tests in this file are
> incorrect, and that we should instead be testing TARGET_ABI_BITS
> vs 32/64.  In addition, virtually all of the target_ulong structure
> members would need to be abi_ulong.
> 
> Should we in fact disable ppc64abi32?
> I can't see how it could work enough to be useful as-is.

Yes, this part needs definitively more cleanup, anyway:

Acked-by: Laurent Vivier <address@hidden>

Thanks,
Laurent
> 
> r~
> 
> ---
>  linux-user/ppc/signal.c | 69 +++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 40 deletions(-)
> 
> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> index ecd99736b7..20a02c197c 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -35,12 +35,26 @@ 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;
> +    /*
> +     * On ppc64, this mcontext structure is naturally *unaligned*,
> +     * or rather it is aligned on a 8 bytes boundary but not on
> +     * a 16 byte boundary.  This pad fixes it up.  This is why we
> +     * cannot use ppc_avr_t, which would force alignment.  This is
> +     * also why the vector regs are referenced in the ABI by the
> +     * v_regs pointer above so any amount of padding can be added here.
> +     */
> +    target_ulong pad;
> +    /* VSCR and VRSAVE are saved separately.  Also reserve space for VSX. */
> +    struct {
> +        uint64_t altivec[34 + 16][2];
> +    } mc_vregs;
>  #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
> @@ -48,32 +62,14 @@ struct target_mcontext {
>      union {
>          /* SPE vector registers.  One extra for SPEFSCR.  */
>          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.
> -           We also need to account for the VSX registers on PPC64
> -        */
> -#if defined(TARGET_PPC64)
> -#define QEMU_NVRREG (34 + 16)
> -        /* On ppc64, this mcontext structure is naturally *unaligned*,
> -         * or rather it is aligned on a 8 bytes boundary but not on
> -         * a 16 bytes one. This pad fixes it up. This is also why the
> -         * vector regs are referenced by the v_regs pointer above so
> -         * any amount of padding can be added here
> +        /*
> +         * Altivec vector registers.  One extra for VRSAVE.
> +         * On ppc32, we are already aligned to 16 bytes.  We could
> +         * use ppc_avr_t, but choose to share the same type as ppc64.
>           */
> -        target_ulong pad;
> -#else
> -        /* On ppc32, we are already aligned to 16 bytes */
> -#define QEMU_NVRREG 33
> -#endif
> -        /* 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 whole struct target_mcontext aligned
> -         * which breaks the layout of struct target_ucontext on ppc64.
> -         */
> -        uint64_t altivec[QEMU_NVRREG][2];
> -#undef QEMU_NVRREG
> +        uint64_t altivec[33][2];
>      } mc_vregs;
> +#endif
>  };
>  
>  /* See arch/powerpc/include/asm/sigcontext.h.  */
> @@ -278,6 +274,7 @@ static void save_user_regs(CPUPPCState *env, struct 
> target_mcontext *frame)
>          __put_user((uint32_t)env->spr[SPR_VRSAVE], vrsave);
>      }
>  
> +#if defined(TARGET_PPC64)
>      /* Save VSX second halves */
>      if (env->insns_flags2 & PPC2_VSX) {
>          uint64_t *vsregs = (uint64_t *)&frame->mc_vregs.altivec[34];
> @@ -286,6 +283,7 @@ static void save_user_regs(CPUPPCState *env, struct 
> target_mcontext *frame)
>              __put_user(*vsrl, &vsregs[i]);
>          }
>      }
> +#endif
>  
>      /* Save floating point registers.  */
>      if (env->insns_flags & PPC_FLOAT) {
> @@ -296,22 +294,18 @@ static void save_user_regs(CPUPPCState *env, struct 
> target_mcontext *frame)
>          __put_user((uint64_t) env->fpscr, &frame->mc_fregs[32]);
>      }
>  
> +#if !defined(TARGET_PPC64)
>      /* Save SPE registers.  The kernel only saves the high half.  */
>      if (env->insns_flags & PPC_SPE) {
> -#if defined(TARGET_PPC64)
> -        for (i = 0; i < ARRAY_SIZE(env->gpr); i++) {
> -            __put_user(env->gpr[i] >> 32, &frame->mc_vregs.spe[i]);
> -        }
> -#else
>          for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
>              __put_user(env->gprh[i], &frame->mc_vregs.spe[i]);
>          }
> -#endif
>          /* Set MSR_SPE in the saved MSR value to indicate that
>             frame->mc_vregs contains valid data.  */
>          msr |= MSR_SPE;
>          __put_user(env->spe_fscr, &frame->mc_vregs.spe[32]);
>      }
> +#endif
>  
>      /* Store MSR.  */
>      __put_user(msr, &frame->mc_gregs[TARGET_PT_MSR]);
> @@ -392,6 +386,7 @@ static void restore_user_regs(CPUPPCState *env,
>          __get_user(env->spr[SPR_VRSAVE], vrsave);
>      }
>  
> +#if defined(TARGET_PPC64)
>      /* Restore VSX second halves */
>      if (env->insns_flags2 & PPC2_VSX) {
>          uint64_t *vsregs = (uint64_t *)&frame->mc_vregs.altivec[34];
> @@ -400,6 +395,7 @@ static void restore_user_regs(CPUPPCState *env,
>              __get_user(*vsrl, &vsregs[i]);
>          }
>      }
> +#endif
>  
>      /* Restore floating point registers.  */
>      if (env->insns_flags & PPC_FLOAT) {
> @@ -412,22 +408,15 @@ static void restore_user_regs(CPUPPCState *env,
>          env->fpscr = (uint32_t) fpscr;
>      }
>  
> +#if !defined(TARGET_PPC64)
>      /* Save SPE registers.  The kernel only saves the high half.  */
>      if (env->insns_flags & PPC_SPE) {
> -#if defined(TARGET_PPC64)
> -        for (i = 0; i < ARRAY_SIZE(env->gpr); i++) {
> -            uint32_t hi;
> -
> -            __get_user(hi, &frame->mc_vregs.spe[i]);
> -            env->gpr[i] = ((uint64_t)hi << 32) | ((uint32_t) env->gpr[i]);
> -        }
> -#else
>          for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
>              __get_user(env->gprh[i], &frame->mc_vregs.spe[i]);
>          }
> -#endif
>          __get_user(env->spe_fscr, &frame->mc_vregs.spe[32]);
>      }
> +#endif
>  }
>  
>  #if !defined(TARGET_PPC64)
>