Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[John Paul Adrian Glaubitz]

Le 06/07/2017 à 02:23, Richard Henderson a écrit :
> kernel also checks PC < gUSA region end point,
> try this:
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 1e716a9..4e1e4f0 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -3477,7 +3477,8 @@ static abi_ulong get_sigframe(struct
> target_sigaction *ka,
>  static void unwind_gusa(CPUSH4State *regs)
>  {
>      /* If the stack pointer is sufficiently negative... */
> -    if ((regs->gregs[15] & 0xc0000000u) == 0xc0000000u) {
> +    if ((regs->gregs[15] & 0xc0000000u) == 0xc0000000u &&
> +        regs->pc < regs->gregs[0]) {
>          /* Reset the PC to before the gUSA region, as computed from
>             R0 = region end, SP = -(region size), plus one more insn
>             that actually sets SP to the region size.  */

This fixes the segfaults for me with newer chroots.

So, just in case:

Tested-By: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[John Paul Adrian Glaubitz]

On Thu, Jul 06, 2017 at 02:55:02PM +0200, John Paul Adrian Glaubitz wrote:
> This fixes the segfaults for me with newer chroots.

Ok, that was too fast. While some segfaults are now gone, there are
still some other commands segfaulting. Again, reverting patch 5
and Laurent's patch makes these segfaults go away.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

[Qemu-devel] [PATCH 00/11] target/sh4 improvments

[Richard Henderson]

This fixes two problems with atomic operations on sh4,
including an attempt at supporting the user-space atomics
technique used by most sh-linux-user binaries.

This is good enough to run the one occurrence in linux-user-test-0.3.
I'm still downloading enough of a cross environment to be able to run
more recent sh4 binaries.  Including the one in the LP bug report.

Thoughts and more extensive testing appreciated.


r~


Richard Henderson (11):
  target/sh4: Use cmpxchg for movco
  target/sh4: Consolidate end-of-TB tests
  target/sh4: Handle user-space atomics
  target/sh4: Recognize common gUSA sequences
  linux-user/sh4: Notice gUSA regions during signal delivery
  target/sh4: Hoist register bank selection
  target/sh4: Unify cpu_fregs into FREG
  target/sh4: Pass DisasContext to fpr64 routines
  target/sh4: Avoid a potential translator crash for malformed FPR64
  target/sh4: Hoist fp bank selection
  target/sh4: Eliminate DREG macro

 target/sh4/cpu.h       |  24 +-
 target/sh4/helper.h    |   1 +
 linux-user/signal.c    |  21 ++
 target/sh4/op_helper.c |   6 +
 target/sh4/translate.c | 724 ++++++++++++++++++++++++++++++++++++++-----------
 5 files changed, 621 insertions(+), 155 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[Richard Henderson]

We translate gUSA regions atomically in a parallel context.
But in a serial context a gUSA region may be interrupted.
In that case, restart the region as the kernel would.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/signal.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 3d18d1b..1e716a9 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -3471,6 +3471,23 @@ static abi_ulong get_sigframe(struct target_sigaction *ka,
     return (sp - frame_size) & -8ul;
 }
 
+/* Notice when we're in the middle of a gUSA region and reset.
+   Note that this will only occur for !parallel_cpus, as we will
+   translate such sequences differently in a parallel context.  */
+static void unwind_gusa(CPUSH4State *regs)
+{
+    /* If the stack pointer is sufficiently negative... */
+    if ((regs->gregs[15] & 0xc0000000u) == 0xc0000000u) {
+        /* Reset the PC to before the gUSA region, as computed from
+           R0 = region end, SP = -(region size), plus one more insn
+           that actually sets SP to the region size.  */
+        regs->pc = regs->gregs[0] + regs->gregs[15] - 2;
+
+        /* Reset the SP to the saved version in R1.  */
+        regs->gregs[15] = regs->gregs[1];
+    }
+}
+
 static void setup_sigcontext(struct target_sigcontext *sc,
                              CPUSH4State *regs, unsigned long mask)
 {
@@ -3534,6 +3551,8 @@ static void setup_frame(int sig, struct target_sigaction *ka,
     abi_ulong frame_addr;
     int i;
 
+    unwind_gusa(regs);
+
     frame_addr = get_sigframe(ka, regs->gregs[15], sizeof(*frame));
     trace_user_setup_frame(regs, frame_addr);
     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
@@ -3583,6 +3602,8 @@ static void setup_rt_frame(int sig, struct target_sigaction *ka,
     abi_ulong frame_addr;
     int i;
 
+    unwind_gusa(regs);
+
     frame_addr = get_sigframe(ka, regs->gregs[15], sizeof(*frame));
     trace_user_setup_rt_frame(regs, frame_addr);
     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
-- 
2.9.4

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[Laurent Vivier]

Le 06/07/2017 à 02:23, Richard Henderson a écrit :
> We translate gUSA regions atomically in a parallel context.
> But in a serial context a gUSA region may be interrupted.
> In that case, restart the region as the kernel would.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/signal.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 3d18d1b..1e716a9 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -3471,6 +3471,23 @@ static abi_ulong get_sigframe(struct target_sigaction *ka,
>      return (sp - frame_size) & -8ul;
>  }
>  
> +/* Notice when we're in the middle of a gUSA region and reset.
> +   Note that this will only occur for !parallel_cpus, as we will
> +   translate such sequences differently in a parallel context.  */
> +static void unwind_gusa(CPUSH4State *regs)
> +{
> +    /* If the stack pointer is sufficiently negative... */
> +    if ((regs->gregs[15] & 0xc0000000u) == 0xc0000000u) {
> +        /* Reset the PC to before the gUSA region, as computed from
> +           R0 = region end, SP = -(region size), plus one more insn
> +           that actually sets SP to the region size.  */
> +        regs->pc = regs->gregs[0] + regs->gregs[15] - 2;
> +
> +        /* Reset the SP to the saved version in R1.  */
> +        regs->gregs[15] = regs->gregs[1];
> +    }
> +}
> +
>  static void setup_sigcontext(struct target_sigcontext *sc,
>                               CPUSH4State *regs, unsigned long mask)
>  {
> @@ -3534,6 +3551,8 @@ static void setup_frame(int sig, struct target_sigaction *ka,
>      abi_ulong frame_addr;
>      int i;
>  
> +    unwind_gusa(regs);
> +
>      frame_addr = get_sigframe(ka, regs->gregs[15], sizeof(*frame));
>      trace_user_setup_frame(regs, frame_addr);
>      if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> @@ -3583,6 +3602,8 @@ static void setup_rt_frame(int sig, struct target_sigaction *ka,
>      abi_ulong frame_addr;
>      int i;
>  
> +    unwind_gusa(regs);
> +
>      frame_addr = get_sigframe(ka, regs->gregs[15], sizeof(*frame));
>      trace_user_setup_rt_frame(regs, frame_addr);
>      if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[John Paul Adrian Glaubitz]

Hi!

Wow, great to see that there is actually now support for handling gUSA
in qemu-user on sh4. I wonder whether this will help resolve the
lock-up issues on qemu-sh4-user when building Debian packages for
sh4.

The lockups occur fairly often when any process uses
multi-threading. Would this help here?

Was gUSA support recently added to qemu or is this just a fix?

I will give this a try soon. Thanks Laurent for letting me know.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[Laurent Vivier]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

Le 06/07/2017 à 10:10, John Paul Adrian Glaubitz a écrit :
> Hi!
> 
> Wow, great to see that there is actually now support for handling gUSA
> in qemu-user on sh4. I wonder whether this will help resolve the
> lock-up issues on qemu-sh4-user when building Debian packages for
> sh4.
> 
> The lockups occur fairly often when any process uses
> multi-threading. Would this help here?
> 
> Was gUSA support recently added to qemu or is this just a fix?

This is a patch series proposed by Richard, it is not included in qemu
at the moment:

http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01196.html

I think it would be interesting if you generate a qemu binary with it
and use it in your buildds to test it.

I guess the series can be pulled directly from Richard's branch:

git://github.com/rth7680/qemu.git tgt-sh4

Thanks,
Laurent

[-- Attachment #2: 0x3F2FBE3C.asc --]
[-- Type: application/pgp-keys, Size: 13126 bytes --]

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[John Paul Adrian Glaubitz]

On Thu, Jul 06, 2017 at 10:35:15AM +0200, Laurent Vivier wrote:
> This is a patch series proposed by Richard, it is not included in qemu
> at the moment:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg01196.html

Aye, awesome!

> I think it would be interesting if you generate a qemu binary with it
> and use it in your buildds to test it.
> 
> I guess the series can be pulled directly from Richard's branch:
> 
> git://github.com/rth7680/qemu.git tgt-sh4

You bet, I will!

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[John Paul Adrian Glaubitz]

On Thu, Jul 06, 2017 at 10:35:15AM +0200, Laurent Vivier wrote:
> I think it would be interesting if you generate a qemu binary with it
> and use it in your buildds to test it.
> 
> I guess the series can be pulled directly from Richard's branch:
> 
> git://github.com/rth7680/qemu.git tgt-sh4

Pull from this tree and built qemu-sh4-user as static from the tgt-sh4
branch. Copied in Debian/sh4 chroot and tried to enter it:

root@nofan:/local_scratch/sid-sh4-sbuild> chroot .
bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Segmentation fault
root@nofan:/local_scratch/sid-sh4-sbuild>

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[Laurent Vivier]

Le 06/07/2017 à 11:13, John Paul Adrian Glaubitz a écrit :
> On Thu, Jul 06, 2017 at 10:35:15AM +0200, Laurent Vivier wrote:
>> I think it would be interesting if you generate a qemu binary with it
>> and use it in your buildds to test it.
>>
>> I guess the series can be pulled directly from Richard's branch:
>>
>> git://github.com/rth7680/qemu.git tgt-sh4
> 
> Pull from this tree and built qemu-sh4-user as static from the tgt-sh4
> branch. Copied in Debian/sh4 chroot and tried to enter it:
> 
> root@nofan:/local_scratch/sid-sh4-sbuild> chroot .
> bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> Segmentation fault
> root@nofan:/local_scratch/sid-sh4-sbuild>

Could you try origin/master to see if the regression is introduced by
this series or by a previous one?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[John Paul Adrian Glaubitz]

On Thu, Jul 06, 2017 at 03:09:59AM +0200, Laurent Vivier wrote:
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>

I have identified this patch as reponsible for the
segfaults. Reverting this patch fixes the issue. The crashes are
random. Sometimes it crashes directly when entering the chroot,
sometimes only after a command like running "apt
update". Interestingly, the issue does not reproduce on an older
chroot from 2015.

I have uploaded two chroots for testing, one from 2015 [1] and one
freshly generated [2] which shows the crashes with patch nr. 5
applied.

Adrian

> [1] https://people.debian.org/~glaubitz/chroots/unstable-sh4-20150315.tar.gz
> [2] https://people.debian.org/~glaubitz/chroots/unstable-sh4-20170706.tgz

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [Qemu-devel] [PATCH 05/11] linux-user/sh4: Notice gUSA regions during signal delivery

[Laurent Vivier]

Le 06/07/2017 à 02:23, Richard Henderson a écrit :
> We translate gUSA regions atomically in a parallel context.
> But in a serial context a gUSA region may be interrupted.
> In that case, restart the region as the kernel would.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/signal.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 3d18d1b..1e716a9 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -3471,6 +3471,23 @@ static abi_ulong get_sigframe(struct target_sigaction *ka,
>      return (sp - frame_size) & -8ul;
>  }
>  
> +/* Notice when we're in the middle of a gUSA region and reset.
> +   Note that this will only occur for !parallel_cpus, as we will
> +   translate such sequences differently in a parallel context.  */
> +static void unwind_gusa(CPUSH4State *regs)
> +{
> +    /* If the stack pointer is sufficiently negative... */
> +    if ((regs->gregs[15] & 0xc0000000u) == 0xc0000000u) {

kernel also checks PC < gUSA region end point,
try this:

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 1e716a9..4e1e4f0 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -3477,7 +3477,8 @@ static abi_ulong get_sigframe(struct
target_sigaction *ka,
 static void unwind_gusa(CPUSH4State *regs)
 {
     /* If the stack pointer is sufficiently negative... */
-    if ((regs->gregs[15] & 0xc0000000u) == 0xc0000000u) {
+    if ((regs->gregs[15] & 0xc0000000u) == 0xc0000000u &&
+        regs->pc < regs->gregs[0]) {
         /* Reset the PC to before the gUSA region, as computed from
            R0 = region end, SP = -(region size), plus one more insn
            that actually sets SP to the region size.  */

Laurent