[parisc-linux] how to handle ERESTART_RESTARTBLOCK ?

[parisc-linux] how to handle ERESTART_RESTARTBLOCK ?

[Randolph Chung]

I commited some broken code into 2.6 cvs just now, so now i need some
help to fix it ;-) sorry for the verbose explanation below, but i need
to write this down to make sure i understand it myself :-)

Our signal handling code was not handling ERESTART_RESTARTBLOCK
correctly. as far as I can tell, the semantics of this is that if a
syscall function returns with that error code, we are supposed to
restart the syscall, but with a new syscall number
(__NR_restart_syscall -- which we don't define at the moment)

The problem is that our syscall mechanism doesn't really allow this
(afaict). Our syscall sequence looks like this:

    ble <address of gateway page>
    ldi <syscall number>, %r20
    [ we enter the kernel at this point ]

and the way we handle restarting other syscalls is to set the iaoq back
by two insns, so we do the ble again. there's a comment about this in
the code (signal.c)

                        /* Hooray for delayed branching.  We don't
                           have to restore %r20 (the system call
                           number) because it gets loaded in the delay
                           slot of the branch external instruction. */
                        regs->gr[31] -= 8;

which is all well and good, except i don't see how we can change the
syscall number to restart it for the ERESTART_RESTARTBLOCK case.

why is it bad to just call sys_restart_syscall directly from
do_signal()?

any suggestions on how to handle this properly?

thanks
randolph
-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

[parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[Randolph Chung]

> Our signal handling code was not handling ERESTART_RESTARTBLOCK
> correctly. as far as I can tell, the semantics of this is that if a
> syscall function returns with that error code, we are supposed to
> restart the syscall, but with a new syscall number
> (__NR_restart_syscall -- which we don't define at the moment)

ok, answering another one of my own emails :-)

here's a patch that implements the required logic. probably can be
improved some.... comments appreciated.

About __NR_restart_syscall, some archs use syscall #0 (unused for pa
afaict), others define a new syscall number. Any preferences? If it's
zero then i think i will remove the reassemble_14 logic.

thx
randolph

Index: arch/parisc/kernel/signal.c
===================================================================
RCS file: /var/cvs/linux-2.6/arch/parisc/kernel/signal.c,v
retrieving revision 1.11
diff -u -p -r1.11 signal.c
--- arch/parisc/kernel/signal.c	24 Sep 2003 17:54:31 -0000	1.11
+++ arch/parisc/kernel/signal.c	26 Oct 2003 07:17:08 -0000
@@ -451,6 +451,12 @@ handle_signal(unsigned long sig, siginfo
 	return 1;
 }
 
+static inline int reassemble_14(int as14)
+{
+	return (((as14 & 0x1fff) << 1) |
+		((as14 & 0x2000) >> 13));
+}
+
 /*
  * Note that 'init' is a special process: it doesn't get signals it doesn't
  * want to handle. Thus you cannot kill init even with a SIGKILL even by
@@ -530,10 +537,48 @@ do_signal(sigset_t *oldset, struct pt_re
 	/* Did we come from a system call? */
 	if (in_syscall) {
 		/* Restart the system call - no handlers present */
-		if (regs->gr[28] == -ERESTART_RESTARTBLOCK ||
-		    regs->gr[28] == -ERESTARTNOHAND ||
-		    regs->gr[28] == -ERESTARTSYS ||
-		    regs->gr[28] == -ERESTARTNOINTR) {
+		if (regs->gr[28] == -ERESTART_RESTARTBLOCK) {
+			unsigned int *usp = (unsigned int *)regs->gr[30];
+
+			/* Setup a trampoline to restart the syscall
+			 * with __NR_restart_syscall
+			 *
+			 *  0: ble 0x100(%%sr2, %%r0)
+			 *  4: ldi __NR_restart_syscall, %r20
+			 *  8: b,l .+8, %r21
+			 * 12: depi 0,31,2,%r21
+			 * 16: ldw 16(%r21), %rp
+			 * 20: bv %r0(%rp)
+			 * 24: ldo -64(%sp), %sp
+			 * 28: nop
+			 * 32: <return address> (2 slots for 64-bit)
+			 */
+			put_user(0xe4008200, &usp[0]);
+			put_user(0x34140000 | reassemble_14(__NR_restart_syscall), &usp[1]);
+			put_user(0xeaa00000, &usp[2]);
+  			put_user(0xd6a01c1e, &usp[3]);
+#ifndef __LP64__
+			put_user(0x4ab40020, &usp[4]);
+#else
+			put_user(0x52b40020, &usp[4]);
+#endif
+			put_user(0xea80c000, &usp[5]);
+			put_user(0x37de3f81, &usp[6]);
+			put_user(0x08000240, &usp[7]);
+			put_user(regs->gr[31] & ~3, &usp[8]); 
+
+			flush_user_dcache_range((unsigned long)usp,
+				                (unsigned long)(usp + 10));
+			flush_user_icache_range((unsigned long)usp,
+				                (unsigned long)(usp + 10));
+
+			regs->gr[31] = regs->gr[30];
+			regs->gr[30] += 64;
+			/* Preserve original r28. */
+			regs->gr[28] = regs->orig_r28;
+		} else if (regs->gr[28] == -ERESTARTNOHAND ||
+		           regs->gr[28] == -ERESTARTSYS ||
+		           regs->gr[28] == -ERESTARTNOINTR) {
 			/* Hooray for delayed branching.  We don't
                            have to restore %r20 (the system call
                            number) because it gets loaded in the delay


-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

[parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[Randolph Chung]

> +			 * 16: ldw 16(%r21), %rp
> +			 * 20: bv %r0(%rp)

btw, this comment is wrong, it's actually
> +			 * 16: ldw 16(%r21), %r20
> +			 * 20: bv %r0(%r20)

trashing rp in a syscall is a bad idea ;-)

thanks
randolph
-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

Re: [parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[Carlos O'Donell]

On Sun, Oct 26, 2003 at 12:37:51AM -0700, Randolph Chung wrote:
> here's a patch that implements the required logic. probably can be
> improved some.... comments appreciated.
> 
> About __NR_restart_syscall, some archs use syscall #0 (unused for pa
> afaict), others define a new syscall number. Any preferences? If it's
> zero then i think i will remove the reassemble_14 logic.

I don't see why #0 can't be used here. Then again, why can't you code
the syscall number directly into the hex? Are you expecting it to
change? :)

> +		if (regs->gr[28] == -ERESTART_RESTARTBLOCK) {
> +			unsigned int *usp = (unsigned int *)regs->gr[30];
> +
> +			/* Setup a trampoline to restart the syscall
> +			 * with __NR_restart_syscall
> +			 *
> +			 *  0: ble 0x100(%%sr2, %%r0)
> +			 *  4: ldi __NR_restart_syscall, %r20
> +			 *  8: b,l .+8, %r21
> +			 * 12: depi 0,31,2,%r21
> +			 * 16: ldw 16(%r21), %rp
> +			 * 20: bv %r0(%rp)
> +			 * 24: ldo -64(%sp), %sp

Adding stack here can be detremental to the userspace program. It's not
expecting you to have a stack, and it expects the syscall to live within 
its calling frame. When we setup the signal return trampoline we don't
mess with the stack either :) Though this context is different.

> +			 * 28: nop
> +			 * 32: <return address> (2 slots for 64-bit)
> +			 */

c.

Re: [parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[Randolph Chung]

> I don't see why #0 can't be used here. Then again, why can't you code
> the syscall number directly into the hex? Are you expecting it to
> change? :)

no, i can (will) just encode it into the hex.... 

> Adding stack here can be detremental to the userspace program. It's not
> expecting you to have a stack, and it expects the syscall to live within 
> its calling frame. When we setup the signal return trampoline we don't
> mess with the stack either :) Though this context is different.

userspace should not see this stack...

let's say your syscall sequence looks like this:

[...]
ble 0x100(%sr2, %r0)    /* call nanosleep */
ldi 0xa2,%r20
ldi -1000,r1
[...]

normally when you return from the kernel, you return at the second ldi
insn. In this case, you jump to a trampoline that happens to sit on your
stack, so the insn sequence becomes essentially

[...]
ble 0x100(%sr2, %r0)    /* call nanosleep */
ldi 0xa2,%r20
    ldo 64(%sp),%sp
    ble 0x100(%sr2, %r0)
    ldi 0,%r20
    b,l .+8, %r21
    [...]
    bv %r0(%r20)
    ldo -64(%sp),%sp
ldi -1000,r1
[...]

(indented part added by the kernel)

so it should be invisible to the syscall wrapper/application.

or am i missing something? :-)

randolph
-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

Re: [parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[John David Anglin]

> [...]
> ble 0x100(%sr2, %r0)    /* call nanosleep */
> ldi 0xa2,%r20
>     ldo 64(%sp),%sp
>     ble 0x100(%sr2, %r0)
>     ldi 0,%r20
>     b,l .+8, %r21
>     [...]
>     bv %r0(%r20)
>     ldo -64(%sp),%sp
> ldi -1000,r1
> [...]
> 
> (indented part added by the kernel)
> 
> so it should be invisible to the syscall wrapper/application.
> 
> or am i missing something? :-)

You probably have to be careful here with respect to signals, etc.
Technically, the frame marker is supposed to move when a dynamic
stack allocation is done.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: [parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[Randolph Chung]

> You probably have to be careful here with respect to signals, etc.
> Technically, the frame marker is supposed to move when a dynamic
> stack allocation is done.

ok, how about if i do it like this?


-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

Re: [parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[Randolph Chung]

In reference to a message from Randolph Chung, dated Oct 26:
> > You probably have to be careful here with respect to signals, etc.
> > Technically, the frame marker is supposed to move when a dynamic
> > stack allocation is done.
> 
> ok, how about if i do it like this?

hrm, oops, that's the magical disappearance act... :)

Index: arch/parisc/kernel/signal.c
===================================================================
RCS file: /var/cvs/linux-2.6/arch/parisc/kernel/signal.c,v
retrieving revision 1.11
diff -u -p -r1.11 signal.c
--- arch/parisc/kernel/signal.c	24 Sep 2003 17:54:31 -0000	1.11
+++ arch/parisc/kernel/signal.c	26 Oct 2003 20:53:14 -0000
@@ -530,10 +531,68 @@ do_signal(sigset_t *oldset, struct pt_re
 	/* Did we come from a system call? */
 	if (in_syscall) {
 		/* Restart the system call - no handlers present */
-		if (regs->gr[28] == -ERESTART_RESTARTBLOCK ||
-		    regs->gr[28] == -ERESTARTNOHAND ||
-		    regs->gr[28] == -ERESTARTSYS ||
-		    regs->gr[28] == -ERESTARTNOINTR) {
+		if (regs->gr[28] == -ERESTART_RESTARTBLOCK) {
+			unsigned int *usp = (unsigned int *)regs->gr[30];
+
+			/* Setup a trampoline to restart the syscall
+			 * with __NR_restart_syscall
+			 */
+#ifndef __LP64__
+			/* 32-bit version:
+			 *  0: <frame marker>
+			 *  4: <return address>
+			 *  8: ble 0x100(%%sr2, %%r0)
+			 * 12: ldi __NR_restart_syscall, %r20
+			 * 16: ldw -60(%sp), %r20
+			 * 20: bv %r0(%r20)
+			 * 24: ldwm -64(%sp), %r3
+			 */
+			put_user(regs->gr[3], &usp[0]); 
+			put_user(regs->gr[31], &usp[1]); 
+			put_user(0xe4008200, &usp[2]);
+			put_user(0x34140000, &usp[3]);
+			put_user(0x4bd43f89, &usp[4]);
+			put_user(0xea80c000, &usp[5]);
+			put_user(0x4fc33f81, &usp[6]);
+
+			regs->gr[31] = regs->gr[30] + 8;
+#else
+			/* 64-bit version:
+			 *  0: <frame marker>
+			 *  8: <return address>
+			 * 16: ble 0x100(%%sr2, %%r0)
+			 * 20: ldi __NR_restart_syscall, %r20
+			 * 24: ldd -56(%sp), %r20
+			 * 28: bv %r0(%r20)
+			 * 32: ldd,mb -64(%sp), %r3
+			 */
+			put_user(regs->gr[3] >> 32, &usp[0]); 
+			put_user(regs->gr[3] & 0xffffffff, &usp[1]); 
+			put_user(regs->gr[31] >> 32, &usp[2]); 
+			put_user(regs->gr[31] & 0xffffffff, &usp[3]); 
+			put_user(0xe4008200, &usp[4]);
+			put_user(0x34140000, &usp[5]);
+			put_user(0x53d43f91, &usp[6]);
+			put_user(0xea80c000, &usp[7]);
+			put_user(0x53c33f8d, &usp[8]);
+
+			regs->gr[31] = regs->gr[30] + 16;
+#endif
+			
+			/* Stack is 64-byte aligned, and we only 
+			 * need to flush 1 cache line */
+			asm("fdc 0(%%sr3, %0)\n"
+			    "fic 0(%%sr3, %0)\n"
+			    "sync\n"
+			    : : "r"(regs->gr[30]));
+
+			regs->gr[3] = regs->gr[30];
+			regs->gr[30] += 64;
+			/* Preserve original r28. */
+			regs->gr[28] = regs->orig_r28;
+		} else if (regs->gr[28] == -ERESTARTNOHAND ||
+		           regs->gr[28] == -ERESTARTSYS ||
+		           regs->gr[28] == -ERESTARTNOINTR) {
 			/* Hooray for delayed branching.  We don't
                            have to restore %r20 (the system call
                            number) because it gets loaded in the delay

-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

Re: [parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[Carlos O'Donell]

On Sun, Oct 26, 2003 at 01:02:01PM -0800, Randolph Chung wrote:
> hrm, oops, that's the magical disappearance act... :)
> 
> Index: arch/parisc/kernel/signal.c
> ===================================================================
> RCS file: /var/cvs/linux-2.6/arch/parisc/kernel/signal.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 signal.c
> --- arch/parisc/kernel/signal.c	24 Sep 2003 17:54:31 -0000	1.11
> +++ arch/parisc/kernel/signal.c	26 Oct 2003 20:53:14 -0000
> @@ -530,10 +531,68 @@ do_signal(sigset_t *oldset, struct pt_re
>  	/* Did we come from a system call? */
>  	if (in_syscall) {
>  		/* Restart the system call - no handlers present */
> -		if (regs->gr[28] == -ERESTART_RESTARTBLOCK ||
> -		    regs->gr[28] == -ERESTARTNOHAND ||
> -		    regs->gr[28] == -ERESTARTSYS ||
> -		    regs->gr[28] == -ERESTARTNOINTR) {
> +		if (regs->gr[28] == -ERESTART_RESTARTBLOCK) {
> +			unsigned int *usp = (unsigned int *)regs->gr[30];
> +
> +			/* Setup a trampoline to restart the syscall
> +			 * with __NR_restart_syscall
> +			 */
> +#ifndef __LP64__
> +			/* 32-bit version:
> +			 *  0: <frame marker>
> +			 *  4: <return address>
> +			 *  8: ble 0x100(%%sr2, %%r0)
> +			 * 12: ldi __NR_restart_syscall, %r20
> +			 * 16: ldw -60(%sp), %r20
> +			 * 20: bv %r0(%r20)
> +			 * 24: ldwm -64(%sp), %r3
> +			 */

What do you think of:

a. Copy the original rp into a temp register.
b. Setup our rp so we return to the stack trampoline.
c. Return to userspace.
d. Trampoline restores original rp.
e. Make call to restart syscall.
f. Kernel does the magic.
g. Return to the original rp.

You don't care if you get a signal because you are never going to return
to the trampoline, so you don't adjust 'sp' to protect the trampoline.

My fears:

a. Timer tick while on the trampoline before the syscall is made.
	= Signal is delivered... what happens?

As a mater of fact, I'm wholely afraid of stack trampolines for the
explicit reason that your timeslice might expire and odd things might
transpire.

c.

Re: [parisc-linux] Re: how to handle ERESTART_RESTARTBLOCK ?

[Randolph Chung]

> What do you think of:

here's a third try using Carlos' idea... seems cleaner (smaller) and
doesn't muck with the stack pointer.

thanks carlos! :-)
randolph

Index: arch/parisc/kernel/signal.c
===================================================================
RCS file: /var/cvs/linux-2.6/arch/parisc/kernel/signal.c,v
retrieving revision 1.11
diff -u -p -r1.11 signal.c
--- arch/parisc/kernel/signal.c	24 Sep 2003 17:54:31 -0000	1.11
+++ arch/parisc/kernel/signal.c	27 Oct 2003 05:01:52 -0000
@@ -530,10 +531,42 @@ do_signal(sigset_t *oldset, struct pt_re
 	/* Did we come from a system call? */
 	if (in_syscall) {
 		/* Restart the system call - no handlers present */
-		if (regs->gr[28] == -ERESTART_RESTARTBLOCK ||
-		    regs->gr[28] == -ERESTARTNOHAND ||
-		    regs->gr[28] == -ERESTARTSYS ||
-		    regs->gr[28] == -ERESTARTNOINTR) {
+		if (regs->gr[28] == -ERESTART_RESTARTBLOCK) {
+			unsigned int *usp = (unsigned int *)regs->gr[30];
+
+			/* Setup a trampoline to restart the syscall
+			 * with __NR_restart_syscall
+			 *
+			 *  0: <return address (orig r31)>
+			 *  8: <2nd half for 64-bit>
+			 * 12: ldw 0(%sp), %r31
+			 * 16: be 0x100(%sr2, %r0)
+			 * 20: ldi __NR_restart_syscall, %r20
+			 */
+#ifndef __LP64__
+			put_user(regs->gr[31], &usp[0]);
+			put_user(0x0fc0109f, &usp[2]);
+#else
+			put_user(regs->gr[31] >> 32, &usp[0]);
+			put_user(regs->gr[31] & 0xffffffff, &usp[1]);
+			put_user(0x0fc010df, &usp[2]);
+#endif
+			put_user(0xe0008200, &usp[3]);
+			put_user(0x34140000, &usp[4]);
+
+			/* Stack is 64-byte aligned, and we only 
+			 * need to flush 1 cache line */
+			asm("fdc 0(%%sr3, %0)\n"
+			    "fic 0(%%sr3, %0)\n"
+			    "sync\n"
+			    : : "r"(regs->gr[30]));
+
+			regs->gr[31] = regs->gr[30] + 8;
+			/* Preserve original r28. */
+			regs->gr[28] = regs->orig_r28;
+		} else if (regs->gr[28] == -ERESTARTNOHAND ||
+		           regs->gr[28] == -ERESTARTSYS ||
+		           regs->gr[28] == -ERESTARTNOINTR) {
 			/* Hooray for delayed branching.  We don't
                            have to restore %r20 (the system call
                            number) because it gets loaded in the delay