| Message ID | 20201105212314.9628-2-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: fix various sparc64 guest bugs | expand |
On 11/5/20 1:23 PM, Peter Maydell wrote: > The various structs that make up the SPARC target_ucontext had some > errors: > * target structures must not include fields which are host pointers, > which might be the wrong size. These should be abi_ulong instead > * because we don't have the 'long double' part of the mcfpu_fregs > union in our version of the target_mc_fpu struct, we need to > manually force it to be 16-aligned > > In particular, the lack of 16-alignment caused sparc64_get_context() > and sparc64_set_context() to read and write all the registers at the > wrong offset, which triggered a guest glibc stack check in > siglongjmp: > *** longjmp causes uninitialized stack frame ***: terminated > when trying to run bash. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > +} __attribute__((aligned(16))); Hmph, 96 uses of the attribute directly, 20 uses of QEMU_ALIGNED. I suppose we should just remove the wrapper... r~
On Thu, 5 Nov 2020 at 22:15, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 11/5/20 1:23 PM, Peter Maydell wrote: > > +} __attribute__((aligned(16))); > > Hmph, 96 uses of the attribute directly, 20 uses of QEMU_ALIGNED. I suppose we > should just remove the wrapper... Oops, I forget about that. We're better at adhering to use of QEMU_SENTINEL and QEMU_NORETURN, at least. And a fair chunk of those 96 are in code-that's-not-ours like the headers imported from Linux or the pc-bios/s390-ccw code. I'm in two minds here -- the wrappers look less clunky than the __attribute__ syntax, but on the other hand "there is only one way this can be written" results in less inconsistency than "there are two ways". thanks -- PMM
Le 05/11/2020 à 22:23, Peter Maydell a écrit : > The various structs that make up the SPARC target_ucontext had some > errors: > * target structures must not include fields which are host pointers, > which might be the wrong size. These should be abi_ulong instead > * because we don't have the 'long double' part of the mcfpu_fregs > union in our version of the target_mc_fpu struct, we need to > manually force it to be 16-aligned > > In particular, the lack of 16-alignment caused sparc64_get_context() > and sparc64_set_context() to read and write all the registers at the > wrong offset, which triggered a guest glibc stack check in > siglongjmp: > *** longjmp causes uninitialized stack frame ***: terminated > when trying to run bash. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > linux-user/sparc/signal.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c > index d796f50f665..57ea1593bfc 100644 > --- a/linux-user/sparc/signal.c > +++ b/linux-user/sparc/signal.c > @@ -349,10 +349,15 @@ typedef abi_ulong target_mc_greg_t; > typedef target_mc_greg_t target_mc_gregset_t[SPARC_MC_NGREG]; > > struct target_mc_fq { > - abi_ulong *mcfq_addr; > + abi_ulong mcfq_addr; > uint32_t mcfq_insn; > }; > > +/* > + * Note the manual 16-alignment; the kernel gets this because it > + * includes a "long double qregs[16]" in the mcpu_fregs union, > + * which we can't do. > + */ > struct target_mc_fpu { > union { > uint32_t sregs[32]; > @@ -362,11 +367,11 @@ struct target_mc_fpu { > abi_ulong mcfpu_fsr; > abi_ulong mcfpu_fprs; > abi_ulong mcfpu_gsr; > - struct target_mc_fq *mcfpu_fq; > + abi_ulong mcfpu_fq; > unsigned char mcfpu_qcnt; > unsigned char mcfpu_qentsz; > unsigned char mcfpu_enab; > -}; > +} __attribute__((aligned(16))); > typedef struct target_mc_fpu target_mc_fpu_t; > > typedef struct { > @@ -377,7 +382,7 @@ typedef struct { > } target_mcontext_t; > > struct target_ucontext { > - struct target_ucontext *tuc_link; > + abi_ulong tuc_link; > abi_ulong tuc_flags; > target_sigset_t tuc_sigmask; > target_mcontext_t tuc_mcontext; > Applied to my linux-user-for-5.2 branch. Thanks, Laurent
Hello Laurent, you probably want to also apply my patch for stack_t definitions [1] that was mentioned in Peter Maydell's cover letter for the patch series. [1] https://patchew.org/QEMU/e9d47692-ee92-009f-6007-0abc3f502b97@gmail.com/ On 10/11/20 07:53, Laurent Vivier wrote: > Le 05/11/2020 à 22:23, Peter Maydell a écrit : >> The various structs that make up the SPARC target_ucontext had some >> errors: >> * target structures must not include fields which are host pointers, >> which might be the wrong size. These should be abi_ulong instead >> * because we don't have the 'long double' part of the mcfpu_fregs >> union in our version of the target_mc_fpu struct, we need to >> manually force it to be 16-aligned >> >> In particular, the lack of 16-alignment caused sparc64_get_context() >> and sparc64_set_context() to read and write all the registers at the >> wrong offset, which triggered a guest glibc stack check in >> siglongjmp: >> *** longjmp causes uninitialized stack frame ***: terminated >> when trying to run bash. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> linux-user/sparc/signal.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c >> index d796f50f665..57ea1593bfc 100644 >> --- a/linux-user/sparc/signal.c >> +++ b/linux-user/sparc/signal.c >> @@ -349,10 +349,15 @@ typedef abi_ulong target_mc_greg_t; >> typedef target_mc_greg_t target_mc_gregset_t[SPARC_MC_NGREG]; >> >> struct target_mc_fq { >> - abi_ulong *mcfq_addr; >> + abi_ulong mcfq_addr; >> uint32_t mcfq_insn; >> }; >> >> +/* >> + * Note the manual 16-alignment; the kernel gets this because it >> + * includes a "long double qregs[16]" in the mcpu_fregs union, >> + * which we can't do. >> + */ >> struct target_mc_fpu { >> union { >> uint32_t sregs[32]; >> @@ -362,11 +367,11 @@ struct target_mc_fpu { >> abi_ulong mcfpu_fsr; >> abi_ulong mcfpu_fprs; >> abi_ulong mcfpu_gsr; >> - struct target_mc_fq *mcfpu_fq; >> + abi_ulong mcfpu_fq; >> unsigned char mcfpu_qcnt; >> unsigned char mcfpu_qentsz; >> unsigned char mcfpu_enab; >> -}; >> +} __attribute__((aligned(16))); >> typedef struct target_mc_fpu target_mc_fpu_t; >> >> typedef struct { >> @@ -377,7 +382,7 @@ typedef struct { >> } target_mcontext_t; >> >> struct target_ucontext { >> - struct target_ucontext *tuc_link; >> + abi_ulong tuc_link; >> abi_ulong tuc_flags; >> target_sigset_t tuc_sigmask; >> target_mcontext_t tuc_mcontext; >> > > Applied to my linux-user-for-5.2 branch. > > Thanks, > Laurent >
Le 10/11/2020 à 10:02, LemonBoy a écrit : > Hello Laurent, > you probably want to also apply my patch for stack_t definitions [1] that > was mentioned in Peter Maydell's cover letter for the patch series. > > [1] https://patchew.org/QEMU/e9d47692-ee92-009f-6007-0abc3f502b97@gmail.com/ Yes, you're right. But as it touches more architectures it needs more test. I will send a pull request with your patch once this one will be merged. Thanks, Laurent
diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c index d796f50f665..57ea1593bfc 100644 --- a/linux-user/sparc/signal.c +++ b/linux-user/sparc/signal.c @@ -349,10 +349,15 @@ typedef abi_ulong target_mc_greg_t; typedef target_mc_greg_t target_mc_gregset_t[SPARC_MC_NGREG]; struct target_mc_fq { - abi_ulong *mcfq_addr; + abi_ulong mcfq_addr; uint32_t mcfq_insn; }; +/* + * Note the manual 16-alignment; the kernel gets this because it + * includes a "long double qregs[16]" in the mcpu_fregs union, + * which we can't do. + */ struct target_mc_fpu { union { uint32_t sregs[32]; @@ -362,11 +367,11 @@ struct target_mc_fpu { abi_ulong mcfpu_fsr; abi_ulong mcfpu_fprs; abi_ulong mcfpu_gsr; - struct target_mc_fq *mcfpu_fq; + abi_ulong mcfpu_fq; unsigned char mcfpu_qcnt; unsigned char mcfpu_qentsz; unsigned char mcfpu_enab; -}; +} __attribute__((aligned(16))); typedef struct target_mc_fpu target_mc_fpu_t; typedef struct { @@ -377,7 +382,7 @@ typedef struct { } target_mcontext_t; struct target_ucontext { - struct target_ucontext *tuc_link; + abi_ulong tuc_link; abi_ulong tuc_flags; target_sigset_t tuc_sigmask; target_mcontext_t tuc_mcontext;
The various structs that make up the SPARC target_ucontext had some errors: * target structures must not include fields which are host pointers, which might be the wrong size. These should be abi_ulong instead * because we don't have the 'long double' part of the mcfpu_fregs union in our version of the target_mc_fpu struct, we need to manually force it to be 16-aligned In particular, the lack of 16-alignment caused sparc64_get_context() and sparc64_set_context() to read and write all the registers at the wrong offset, which triggered a guest glibc stack check in siglongjmp: *** longjmp causes uninitialized stack frame ***: terminated when trying to run bash. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- linux-user/sparc/signal.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)