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 - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@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]

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] & 0xc000u) == 0xc000u) {
> +if ((regs->gregs[15] & 0xc000u) == 0xc000u &&
> +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 

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@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 
> ---
>  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] & 0xc000u) == 0xc000u) {

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] & 0xc000u) == 0xc000u) {
+if ((regs->gregs[15] & 0xc000u) == 0xc000u &&
+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

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 

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 - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@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 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 - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@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:
> 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 - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@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 à 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


0x3F2FBE3C.asc
Description: application/pgp-keys

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 - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@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 
> ---
>  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] & 0xc000u) == 0xc000u) {
> +/* 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