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 <richard.hender...@linaro.org>
> ---
>
> 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 <laur...@vivier.eu>
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)
>
