[PATCH for-5.2 0/3] linux-user: fix various sparc64 guest bugs

[PATCH for-5.2 0/3] linux-user: fix various sparc64 guest bugs

[Peter Maydell]

This set of patches fixes bugs which were preventing the
Debian sparc64 /bin/bash from running:
 * the target_ucontext structure put the registers in the
   wrong place (missing alignment specifier, mostly)
 * the set_context and get_context traps weren't saving fp
   and i7, which meant that guest code that did a longjmp would
   crash shortly afterwards (SPARC64 uses these traps to
   implement setjmp/longjmp)
 * we were trying to stuff a 64-bit PC into a uint32_t in
   sigreturn, which caused a SEGV on return from a signal handler

Review very much desired in particular from anybody who understands
SPARC register windows and how we handle them in linux-user for
patch 2! The other patches are straightforward.

This patchset is sufficient that I can at least chroot into
a Debian sparc64 chroot and run basic commands like 'ls' from
the shell prompt (together with Giuseppe Musacchio's patch that
fixes the stack_t struct).

There are clearly a bunch of other bugs in sparc signal handling
(starting with the fact that rt_frame support is simply not
implemented, but there are also some XXX/FIXME comments about TSTATE
save/restore in set/get_context and about the FPU state in the signal
frame code). There's also a Coverity issue about accessing off the
end of the sregs[] array in the target_mc_fpu struct -- the error is
actually harmless (we're accessing into the space in the union for
dregs[16..31] which is what we want to be doing) but I'll probably
put together a patch to make Coverity happier.

thanks
-- PMM

Peter Maydell (3):
  linux-user/sparc: Fix errors in target_ucontext structures
  linux-user/sparc: Correct set/get_context handling of fp and i7
  linux-user/sparc: Don't zero high half of PC, NPC, PSR in sigreturn

 linux-user/sparc/signal.c | 62 ++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

-- 
2.20.1

[PATCH for-5.2 1/3] linux-user/sparc: Fix errors in target_ucontext structures

[Peter Maydell]

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;
-- 
2.20.1

Re: [PATCH for-5.2 1/3] linux-user/sparc: Fix errors in target_ucontext structures

[Richard Henderson]

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~

Re: [PATCH for-5.2 1/3] linux-user/sparc: Fix errors in target_ucontext structures

[Peter Maydell]

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

Re: [PATCH for-5.2 1/3] linux-user/sparc: Fix errors in target_ucontext structures

[Laurent Vivier]

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

Re: [PATCH for-5.2 1/3] linux-user/sparc: Fix errors in target_ucontext structures

[LemonBoy]

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
> 

Re: [PATCH for-5.2 1/3] linux-user/sparc: Fix errors in target_ucontext structures

[Laurent Vivier]

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

[PATCH for-5.2 2/3] linux-user/sparc: Correct set/get_context handling of fp and i7

[Peter Maydell]

Because QEMU's user-mode emulation just directly accesses guest CPU
state, for SPARC the guest register window state is not the same in
the sparc64_get_context() and sparc64_set_context() functions as it
is for the real kernel's versions of those functions.  Specifically,
for the kernel it has saved the user space state such that the O*
registers go into a pt_regs struct as UREG_I*, and the I* registers
have been spilled onto the userspace stack.  For QEMU, we haven't
done that, so the guest's O* registers are still in WREG_O* and the
I* registers in WREG_I*.

The code was already accessing the O* registers correctly for QEMU,
but had copied the kernel code for accessing the I* registers off the
userspace stack.  Replace this with direct accesses to fp and i7 in
the CPU state, and add a comment explaining why we differ from the
kernel code here.

This fix is sufficient to get bash to a shell prompt.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I'm really pretty unsure about our handling of SPARC register
windows here. This fix works, but should we instead be
ensuring that the flush_windows() call cpu_loop() does
before handling this trap has written the I* regs to the
stack ???
---
 linux-user/sparc/signal.c | 47 ++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index 57ea1593bfc..c315704b389 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -403,7 +403,6 @@ void sparc64_set_context(CPUSPARCState *env)
     struct target_ucontext *ucp;
     target_mc_gregset_t *grp;
     abi_ulong pc, npc, tstate;
-    abi_ulong fp, i7, w_addr;
     unsigned int i;
 
     ucp_addr = env->regwptr[WREG_O0];
@@ -447,6 +446,15 @@ void sparc64_set_context(CPUSPARCState *env)
     __get_user(env->gregs[5], (&(*grp)[SPARC_MC_G5]));
     __get_user(env->gregs[6], (&(*grp)[SPARC_MC_G6]));
     __get_user(env->gregs[7], (&(*grp)[SPARC_MC_G7]));
+
+    /*
+     * Note that unlike the kernel, we didn't need to mess with the
+     * guest register window state to save it into a pt_regs to run
+     * the kernel. So for us the guest's O regs are still in WREG_O*
+     * (unlike the kernel which has put them in UREG_I* in a pt_regs)
+     * and the fp and i7 are still in WREG_I6 and WREG_I7 and don't
+     * need to be written back to userspace memory.
+     */
     __get_user(env->regwptr[WREG_O0], (&(*grp)[SPARC_MC_O0]));
     __get_user(env->regwptr[WREG_O1], (&(*grp)[SPARC_MC_O1]));
     __get_user(env->regwptr[WREG_O2], (&(*grp)[SPARC_MC_O2]));
@@ -456,18 +464,9 @@ void sparc64_set_context(CPUSPARCState *env)
     __get_user(env->regwptr[WREG_O6], (&(*grp)[SPARC_MC_O6]));
     __get_user(env->regwptr[WREG_O7], (&(*grp)[SPARC_MC_O7]));
 
-    __get_user(fp, &(ucp->tuc_mcontext.mc_fp));
-    __get_user(i7, &(ucp->tuc_mcontext.mc_i7));
+    __get_user(env->regwptr[WREG_FP], &(ucp->tuc_mcontext.mc_fp));
+    __get_user(env->regwptr[WREG_I7], &(ucp->tuc_mcontext.mc_i7));
 
-    w_addr = TARGET_STACK_BIAS + env->regwptr[WREG_O6];
-    if (put_user(fp, w_addr + offsetof(struct target_reg_window, ins[6]),
-                 abi_ulong) != 0) {
-        goto do_sigsegv;
-    }
-    if (put_user(i7, w_addr + offsetof(struct target_reg_window, ins[7]),
-                 abi_ulong) != 0) {
-        goto do_sigsegv;
-    }
     /* FIXME this does not match how the kernel handles the FPU in
      * its sparc64_set_context implementation. In particular the FPU
      * is only restored if fenab is non-zero in:
@@ -501,7 +500,6 @@ void sparc64_get_context(CPUSPARCState *env)
     struct target_ucontext *ucp;
     target_mc_gregset_t *grp;
     target_mcontext_t *mcp;
-    abi_ulong fp, i7, w_addr;
     int err;
     unsigned int i;
     target_sigset_t target_set;
@@ -553,6 +551,15 @@ void sparc64_get_context(CPUSPARCState *env)
     __put_user(env->gregs[5], &((*grp)[SPARC_MC_G5]));
     __put_user(env->gregs[6], &((*grp)[SPARC_MC_G6]));
     __put_user(env->gregs[7], &((*grp)[SPARC_MC_G7]));
+
+    /*
+     * Note that unlike the kernel, we didn't need to mess with the
+     * guest register window state to save it into a pt_regs to run
+     * the kernel. So for us the guest's O regs are still in WREG_O*
+     * (unlike the kernel which has put them in UREG_I* in a pt_regs)
+     * and the fp and i7 are still in WREG_I6 and WREG_I7 and don't
+     * need to be fished out of userspace memory.
+     */
     __put_user(env->regwptr[WREG_O0], &((*grp)[SPARC_MC_O0]));
     __put_user(env->regwptr[WREG_O1], &((*grp)[SPARC_MC_O1]));
     __put_user(env->regwptr[WREG_O2], &((*grp)[SPARC_MC_O2]));
@@ -562,18 +569,8 @@ void sparc64_get_context(CPUSPARCState *env)
     __put_user(env->regwptr[WREG_O6], &((*grp)[SPARC_MC_O6]));
     __put_user(env->regwptr[WREG_O7], &((*grp)[SPARC_MC_O7]));
 
-    w_addr = TARGET_STACK_BIAS + env->regwptr[WREG_O6];
-    fp = i7 = 0;
-    if (get_user(fp, w_addr + offsetof(struct target_reg_window, ins[6]),
-                 abi_ulong) != 0) {
-        goto do_sigsegv;
-    }
-    if (get_user(i7, w_addr + offsetof(struct target_reg_window, ins[7]),
-                 abi_ulong) != 0) {
-        goto do_sigsegv;
-    }
-    __put_user(fp, &(mcp->mc_fp));
-    __put_user(i7, &(mcp->mc_i7));
+    __put_user(env->regwptr[WREG_FP], &(mcp->mc_fp));
+    __put_user(env->regwptr[WREG_I7], &(mcp->mc_i7));
 
     {
         uint32_t *dst = ucp->tuc_mcontext.mc_fpregs.mcfpu_fregs.sregs;
-- 
2.20.1

Re: [PATCH for-5.2 2/3] linux-user/sparc: Correct set/get_context handling of fp and i7

[Richard Henderson]

On 11/5/20 1:23 PM, Peter Maydell wrote:
> Because QEMU's user-mode emulation just directly accesses guest CPU
> state, for SPARC the guest register window state is not the same in
> the sparc64_get_context() and sparc64_set_context() functions as it
> is for the real kernel's versions of those functions.  Specifically,
> for the kernel it has saved the user space state such that the O*
> registers go into a pt_regs struct as UREG_I*, and the I* registers
> have been spilled onto the userspace stack.  For QEMU, we haven't
> done that, so the guest's O* registers are still in WREG_O* and the
> I* registers in WREG_I*.
> 
> The code was already accessing the O* registers correctly for QEMU,
> but had copied the kernel code for accessing the I* registers off the
> userspace stack.  Replace this with direct accesses to fp and i7 in
> the CPU state, and add a comment explaining why we differ from the
> kernel code here.
> 
> This fix is sufficient to get bash to a shell prompt.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I'm really pretty unsure about our handling of SPARC register
> windows here. This fix works, but should we instead be
> ensuring that the flush_windows() call cpu_loop() does
> before handling this trap has written the I* regs to the
> stack ???
> ---

Ach, I was so close to being right the last time I tried to clean up this code.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-5.2 2/3] linux-user/sparc: Correct set/get_context handling of fp and i7

[Laurent Vivier]

Le 05/11/2020 à 22:23, Peter Maydell a écrit :
> Because QEMU's user-mode emulation just directly accesses guest CPU
> state, for SPARC the guest register window state is not the same in
> the sparc64_get_context() and sparc64_set_context() functions as it
> is for the real kernel's versions of those functions.  Specifically,
> for the kernel it has saved the user space state such that the O*
> registers go into a pt_regs struct as UREG_I*, and the I* registers
> have been spilled onto the userspace stack.  For QEMU, we haven't
> done that, so the guest's O* registers are still in WREG_O* and the
> I* registers in WREG_I*.
> 
> The code was already accessing the O* registers correctly for QEMU,
> but had copied the kernel code for accessing the I* registers off the
> userspace stack.  Replace this with direct accesses to fp and i7 in
> the CPU state, and add a comment explaining why we differ from the
> kernel code here.
> 
> This fix is sufficient to get bash to a shell prompt.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I'm really pretty unsure about our handling of SPARC register
> windows here. This fix works, but should we instead be
> ensuring that the flush_windows() call cpu_loop() does
> before handling this trap has written the I* regs to the
> stack ???
> ---
>  linux-user/sparc/signal.c | 47 ++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
> index 57ea1593bfc..c315704b389 100644
> --- a/linux-user/sparc/signal.c
> +++ b/linux-user/sparc/signal.c
> @@ -403,7 +403,6 @@ void sparc64_set_context(CPUSPARCState *env)
>      struct target_ucontext *ucp;
>      target_mc_gregset_t *grp;
>      abi_ulong pc, npc, tstate;
> -    abi_ulong fp, i7, w_addr;
>      unsigned int i;
>  
>      ucp_addr = env->regwptr[WREG_O0];
> @@ -447,6 +446,15 @@ void sparc64_set_context(CPUSPARCState *env)
>      __get_user(env->gregs[5], (&(*grp)[SPARC_MC_G5]));
>      __get_user(env->gregs[6], (&(*grp)[SPARC_MC_G6]));
>      __get_user(env->gregs[7], (&(*grp)[SPARC_MC_G7]));
> +
> +    /*
> +     * Note that unlike the kernel, we didn't need to mess with the
> +     * guest register window state to save it into a pt_regs to run
> +     * the kernel. So for us the guest's O regs are still in WREG_O*
> +     * (unlike the kernel which has put them in UREG_I* in a pt_regs)
> +     * and the fp and i7 are still in WREG_I6 and WREG_I7 and don't
> +     * need to be written back to userspace memory.
> +     */
>      __get_user(env->regwptr[WREG_O0], (&(*grp)[SPARC_MC_O0]));
>      __get_user(env->regwptr[WREG_O1], (&(*grp)[SPARC_MC_O1]));
>      __get_user(env->regwptr[WREG_O2], (&(*grp)[SPARC_MC_O2]));
> @@ -456,18 +464,9 @@ void sparc64_set_context(CPUSPARCState *env)
>      __get_user(env->regwptr[WREG_O6], (&(*grp)[SPARC_MC_O6]));
>      __get_user(env->regwptr[WREG_O7], (&(*grp)[SPARC_MC_O7]));
>  
> -    __get_user(fp, &(ucp->tuc_mcontext.mc_fp));
> -    __get_user(i7, &(ucp->tuc_mcontext.mc_i7));
> +    __get_user(env->regwptr[WREG_FP], &(ucp->tuc_mcontext.mc_fp));
> +    __get_user(env->regwptr[WREG_I7], &(ucp->tuc_mcontext.mc_i7));
>  
> -    w_addr = TARGET_STACK_BIAS + env->regwptr[WREG_O6];
> -    if (put_user(fp, w_addr + offsetof(struct target_reg_window, ins[6]),
> -                 abi_ulong) != 0) {
> -        goto do_sigsegv;
> -    }
> -    if (put_user(i7, w_addr + offsetof(struct target_reg_window, ins[7]),
> -                 abi_ulong) != 0) {
> -        goto do_sigsegv;
> -    }
>      /* FIXME this does not match how the kernel handles the FPU in
>       * its sparc64_set_context implementation. In particular the FPU
>       * is only restored if fenab is non-zero in:
> @@ -501,7 +500,6 @@ void sparc64_get_context(CPUSPARCState *env)
>      struct target_ucontext *ucp;
>      target_mc_gregset_t *grp;
>      target_mcontext_t *mcp;
> -    abi_ulong fp, i7, w_addr;
>      int err;
>      unsigned int i;
>      target_sigset_t target_set;
> @@ -553,6 +551,15 @@ void sparc64_get_context(CPUSPARCState *env)
>      __put_user(env->gregs[5], &((*grp)[SPARC_MC_G5]));
>      __put_user(env->gregs[6], &((*grp)[SPARC_MC_G6]));
>      __put_user(env->gregs[7], &((*grp)[SPARC_MC_G7]));
> +
> +    /*
> +     * Note that unlike the kernel, we didn't need to mess with the
> +     * guest register window state to save it into a pt_regs to run
> +     * the kernel. So for us the guest's O regs are still in WREG_O*
> +     * (unlike the kernel which has put them in UREG_I* in a pt_regs)
> +     * and the fp and i7 are still in WREG_I6 and WREG_I7 and don't
> +     * need to be fished out of userspace memory.
> +     */
>      __put_user(env->regwptr[WREG_O0], &((*grp)[SPARC_MC_O0]));
>      __put_user(env->regwptr[WREG_O1], &((*grp)[SPARC_MC_O1]));
>      __put_user(env->regwptr[WREG_O2], &((*grp)[SPARC_MC_O2]));
> @@ -562,18 +569,8 @@ void sparc64_get_context(CPUSPARCState *env)
>      __put_user(env->regwptr[WREG_O6], &((*grp)[SPARC_MC_O6]));
>      __put_user(env->regwptr[WREG_O7], &((*grp)[SPARC_MC_O7]));
>  
> -    w_addr = TARGET_STACK_BIAS + env->regwptr[WREG_O6];
> -    fp = i7 = 0;
> -    if (get_user(fp, w_addr + offsetof(struct target_reg_window, ins[6]),
> -                 abi_ulong) != 0) {
> -        goto do_sigsegv;
> -    }
> -    if (get_user(i7, w_addr + offsetof(struct target_reg_window, ins[7]),
> -                 abi_ulong) != 0) {
> -        goto do_sigsegv;
> -    }
> -    __put_user(fp, &(mcp->mc_fp));
> -    __put_user(i7, &(mcp->mc_i7));
> +    __put_user(env->regwptr[WREG_FP], &(mcp->mc_fp));
> +    __put_user(env->regwptr[WREG_I7], &(mcp->mc_i7));
>  
>      {
>          uint32_t *dst = ucp->tuc_mcontext.mc_fpregs.mcfpu_fregs.sregs;
> 

Applied to my linux-user-for-5.2 branch.

Thanks,
Laurent

[PATCH for-5.2 3/3] linux-user/sparc: Don't zero high half of PC, NPC, PSR in sigreturn

[Peter Maydell]

The function do_sigreturn() tries to store the PC, NPC and PSR in
uint32_t local variables, which implicitly drops the high half of
these fields for 64-bit guests.

The usual effect was that a guest which used signals would crash on
return from a signal unless it was lucky enough to take it while the
PC was in the low 4GB of the address space.  In particular, Debian
/bin/dash and /bin/bash would segfault after executing external
commands.

Use abi_ulong, which is the type these fields all have in the
__siginfo_t struct.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/sparc/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
index c315704b389..d12adc8e6ff 100644
--- a/linux-user/sparc/signal.c
+++ b/linux-user/sparc/signal.c
@@ -247,7 +247,7 @@ long do_sigreturn(CPUSPARCState *env)
 {
     abi_ulong sf_addr;
     struct target_signal_frame *sf;
-    uint32_t up_psr, pc, npc;
+    abi_ulong up_psr, pc, npc;
     target_sigset_t set;
     sigset_t host_set;
     int i;
-- 
2.20.1

Re: [PATCH for-5.2 3/3] linux-user/sparc: Don't zero high half of PC, NPC, PSR in sigreturn

[Richard Henderson]

On 11/5/20 1:23 PM, Peter Maydell wrote:
> The function do_sigreturn() tries to store the PC, NPC and PSR in
> uint32_t local variables, which implicitly drops the high half of
> these fields for 64-bit guests.
> 
> The usual effect was that a guest which used signals would crash on
> return from a signal unless it was lucky enough to take it while the
> PC was in the low 4GB of the address space.  In particular, Debian
> /bin/dash and /bin/bash would segfault after executing external
> commands.
> 
> Use abi_ulong, which is the type these fields all have in the
> __siginfo_t struct.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/sparc/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-5.2 3/3] linux-user/sparc: Don't zero high half of PC, NPC, PSR in sigreturn

[Laurent Vivier]

Le 05/11/2020 à 22:23, Peter Maydell a écrit :
> The function do_sigreturn() tries to store the PC, NPC and PSR in
> uint32_t local variables, which implicitly drops the high half of
> these fields for 64-bit guests.
> 
> The usual effect was that a guest which used signals would crash on
> return from a signal unless it was lucky enough to take it while the
> PC was in the low 4GB of the address space.  In particular, Debian
> /bin/dash and /bin/bash would segfault after executing external
> commands.
> 
> Use abi_ulong, which is the type these fields all have in the
> __siginfo_t struct.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/sparc/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/sparc/signal.c b/linux-user/sparc/signal.c
> index c315704b389..d12adc8e6ff 100644
> --- a/linux-user/sparc/signal.c
> +++ b/linux-user/sparc/signal.c
> @@ -247,7 +247,7 @@ long do_sigreturn(CPUSPARCState *env)
>  {
>      abi_ulong sf_addr;
>      struct target_signal_frame *sf;
> -    uint32_t up_psr, pc, npc;
> +    abi_ulong up_psr, pc, npc;
>      target_sigset_t set;
>      sigset_t host_set;
>      int i;
> 

Applied to my linux-user-for-5.2 branch.

Thanks,
Laurent

Re: [PATCH for-5.2 0/3] linux-user: fix various sparc64 guest bugs

[Mark Cave-Ayland]

On 05/11/2020 21:23, Peter Maydell wrote:

> This set of patches fixes bugs which were preventing the
> Debian sparc64 /bin/bash from running:
>   * the target_ucontext structure put the registers in the
>     wrong place (missing alignment specifier, mostly)
>   * the set_context and get_context traps weren't saving fp
>     and i7, which meant that guest code that did a longjmp would
>     crash shortly afterwards (SPARC64 uses these traps to
>     implement setjmp/longjmp)
>   * we were trying to stuff a 64-bit PC into a uint32_t in
>     sigreturn, which caused a SEGV on return from a signal handler
> 
> Review very much desired in particular from anybody who understands
> SPARC register windows and how we handle them in linux-user for
> patch 2! The other patches are straightforward.
> 
> This patchset is sufficient that I can at least chroot into
> a Debian sparc64 chroot and run basic commands like 'ls' from
> the shell prompt (together with Giuseppe Musacchio's patch that
> fixes the stack_t struct).
> 
> There are clearly a bunch of other bugs in sparc signal handling
> (starting with the fact that rt_frame support is simply not
> implemented, but there are also some XXX/FIXME comments about TSTATE
> save/restore in set/get_context and about the FPU state in the signal
> frame code). There's also a Coverity issue about accessing off the
> end of the sregs[] array in the target_mc_fpu struct -- the error is
> actually harmless (we're accessing into the space in the union for
> dregs[16..31] which is what we want to be doing) but I'll probably
> put together a patch to make Coverity happier.

Thanks Peter! This has been broken for a very long time indeed. Once this is merged I 
should probably look at getting a test environment set up.


ATB,

Mark.

Re: [PATCH for-5.2 0/3] linux-user: fix various sparc64 guest bugs

[Laurent Vivier]

Le 10/11/2020 à 13:56, Mark Cave-Ayland a écrit :
> On 05/11/2020 21:23, Peter Maydell wrote:
> 
>> This set of patches fixes bugs which were preventing the
>> Debian sparc64 /bin/bash from running:
>>   * the target_ucontext structure put the registers in the
>>     wrong place (missing alignment specifier, mostly)
>>   * the set_context and get_context traps weren't saving fp
>>     and i7, which meant that guest code that did a longjmp would
>>     crash shortly afterwards (SPARC64 uses these traps to
>>     implement setjmp/longjmp)
>>   * we were trying to stuff a 64-bit PC into a uint32_t in
>>     sigreturn, which caused a SEGV on return from a signal handler
>>
>> Review very much desired in particular from anybody who understands
>> SPARC register windows and how we handle them in linux-user for
>> patch 2! The other patches are straightforward.
>>
>> This patchset is sufficient that I can at least chroot into
>> a Debian sparc64 chroot and run basic commands like 'ls' from
>> the shell prompt (together with Giuseppe Musacchio's patch that
>> fixes the stack_t struct).
>>
>> There are clearly a bunch of other bugs in sparc signal handling
>> (starting with the fact that rt_frame support is simply not
>> implemented, but there are also some XXX/FIXME comments about TSTATE
>> save/restore in set/get_context and about the FPU state in the signal
>> frame code). There's also a Coverity issue about accessing off the
>> end of the sregs[] array in the target_mc_fpu struct -- the error is
>> actually harmless (we're accessing into the space in the union for
>> dregs[16..31] which is what we want to be doing) but I'll probably
>> put together a patch to make Coverity happier.
> 
> Thanks Peter! This has been broken for a very long time indeed. Once
> this is merged I should probably look at getting a test environment set up.

+1

With these patches,

on sparc, debootstrap (wheezy) has some issues but after some hacks
around the packages I've been able to build and run LTP.

on sparc64, debootstrap (sid) seems to work well but after that there
are some issues with apt (URI error)

Thanks,
Laurent