diff mbox series

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

Message ID 20200407032105.26711-1-richard.henderson@linaro.org
State Superseded
Headers show
Series [for-5.0?] linux-user/ppc: Fix padding in mcontext_t for ppc64 | expand

Commit Message

Richard Henderson April 7, 2020, 3:21 a.m. UTC 
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.henderson@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.


r~

---
 linux-user/ppc/signal.c | 69 +++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 40 deletions(-)

-- 
2.20.1

Comments

Laurent Vivier April 7, 2020, 11:09 a.m. UTC | #1 
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.henderson@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 <laurent@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)

>
David Gibson April 8, 2020, 2:10 a.m. UTC | #2 
On Mon, Apr 06, 2020 at 08:21:05PM -0700, Richard Henderson wrote:
> 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.henderson@linaro.org>


Applied to ppc-for-5.0, thanks.

> ---

> 

> 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.


Yeah, I think so.  Last time we had a problem in this area, I couldn't
even figure out what ppc64abi32 was supposed to *be*, let alone what
the use case for it is.  Given that, it's hard to imagine it's been
working (whatever that means) any time recently.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Richard Henderson April 8, 2020, 3:44 a.m. UTC | #3 
On 4/7/20 7:10 PM, David Gibson wrote:
>> Should we in fact disable ppc64abi32?

>> I can't see how it could work enough to be useful as-is.

> 

> Yeah, I think so.  Last time we had a problem in this area, I couldn't

> even figure out what ppc64abi32 was supposed to *be*, let alone what

> the use case for it is.  Given that, it's hard to imagine it's been

> working (whatever that means) any time recently.


What it's *supposed* to be is a ppc32 binary running on a 64-bit cpu, e.g. a
32-bit binary on power7 with the kernel's compat syscalls.  You get to do
native 64-bit arithmetic and have 32-bit pointers.

I guess there's a kind of a use case there somewhere, but it's rather niche,
and someone has to care more than they have until now.


r~
David Gibson April 8, 2020, 4:33 a.m. UTC | #4 
On Tue, Apr 07, 2020 at 08:44:47PM -0700, Richard Henderson wrote:
> On 4/7/20 7:10 PM, David Gibson wrote:

> >> Should we in fact disable ppc64abi32?

> >> I can't see how it could work enough to be useful as-is.

> > 

> > Yeah, I think so.  Last time we had a problem in this area, I couldn't

> > even figure out what ppc64abi32 was supposed to *be*, let alone what

> > the use case for it is.  Given that, it's hard to imagine it's been

> > working (whatever that means) any time recently.

> 

> What it's *supposed* to be is a ppc32 binary running on a 64-bit cpu, e.g. a

> 32-bit binary on power7 with the kernel's compat syscalls.  You get to do

> native 64-bit arithmetic and have 32-bit pointers.


Right, I guessed that from the name, but I couldn't find anything that
explicitly confirmed that.

> I guess there's a kind of a use case there somewhere, but it's rather niche,


Extremely.

> and someone has to care more than they have until now.

> 

> 

> r~

> 


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
diff mbox series

Patch

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)