[PATCH 0/6] Hexagon: various signal and return path cleanups

[PATCH 0/6] Hexagon: various signal and return path cleanups

[Richard Kuo]

The following patches clean up various issues with the signal and return
path parts of the Hexagon arch port, which were pointed out by Al Viro:

https://lkml.org/lkml/2012/2/11/128

The work pending check was moved into a C routine to make it more
readable and to make sure we repeat the check when necessary.  Also, various
arguments and return values for signal handling should now be fixed.

The patches can also be viewed in the context of my next batch of cleanups
at my repo:

git://git.kernel.org/pub/scm/linux/kernel/git/rkuo/linux-hexagon-kernel.git


Any feedback is appreciated.


Thanks,
Richard Kuo




Richard Kuo (6):
  Hexagon: Signal and return path fixes
  Hexagon: fix up int enable/disable at ret_from_fork
  Hexagon: use correct work mask when checking for more work
  Hexagon: check to if we will overflow the signal stack
  Hexagon: break up user fn/arg register setting
  Hexagon: fix psp/sp macro

 arch/hexagon/include/uapi/asm/registers.h |    3 +-
 arch/hexagon/include/uapi/asm/signal.h    |    2 +
 arch/hexagon/kernel/process.c             |   44 ++++++++++++-
 arch/hexagon/kernel/signal.c              |   33 +++-------
 arch/hexagon/kernel/traps.c               |   13 +---
 arch/hexagon/kernel/vm_entry.S            |   96 +++++++++++++----------------
 6 files changed, 100 insertions(+), 91 deletions(-)

-- 
1.7.9.5


--

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/6] Hexagon: Signal and return path fixes

[Richard Kuo]

This fixes the return value of sigreturn and moves the work pending check
into a c routine for readability and fixes the loop for multiple pending
signals.  Based on feedback from Al Viro.

Signed-off-by: Richard Kuo <rkuo@codeaurora.org>
---
 arch/hexagon/include/uapi/asm/signal.h |    2 +
 arch/hexagon/kernel/process.c          |   41 +++++++++++++++
 arch/hexagon/kernel/signal.c           |   29 ++---------
 arch/hexagon/kernel/traps.c            |   13 +----
 arch/hexagon/kernel/vm_entry.S         |   87 +++++++++++++-------------------
 5 files changed, 84 insertions(+), 88 deletions(-)

diff --git a/arch/hexagon/include/uapi/asm/signal.h b/arch/hexagon/include/uapi/asm/signal.h
index 9395568..24b9988 100644
--- a/arch/hexagon/include/uapi/asm/signal.h
+++ b/arch/hexagon/include/uapi/asm/signal.h
@@ -21,6 +21,8 @@
 
 extern unsigned long __rt_sigtramp_template[2];
 
+void do_signal(struct pt_regs *regs);
+
 #include <asm-generic/signal.h>
 
 #endif
diff --git a/arch/hexagon/kernel/process.c b/arch/hexagon/kernel/process.c
index 06ae9ff..dc72ed5 100644
--- a/arch/hexagon/kernel/process.c
+++ b/arch/hexagon/kernel/process.c
@@ -24,6 +24,7 @@
 #include <linux/tick.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
+#include <linux/tracehook.h>
 
 /*
  * Program thread launch.  Often defined as a macro in processor.h,
@@ -202,3 +203,43 @@ int dump_fpu(struct pt_regs *regs, elf_fpregset_t *fpu)
 {
 	return 0;
 }
+
+
+/*
+ * Called on the exit path of event entry; see vm_entry.S
+ *
+ * Interrupts will already be disabled.
+ *
+ * Returns 0 if there's no need to re-check for more work.
+ */
+
+int do_work_pending(struct pt_regs *regs, u32 thread_info_flags)
+{
+	if (!(thread_info_flags & _TIF_ALLWORK_MASK)) {
+		return 0;
+	}  /* shortcut -- no work to be done */
+
+	local_irq_enable();
+
+	if (thread_info_flags & _TIF_NEED_RESCHED) {
+		schedule();
+		return 1;
+	}
+
+	if (thread_info_flags & _TIF_SIGPENDING) {
+		do_signal(regs);
+		return 1;
+	}
+
+	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
+		clear_thread_flag(TIF_NOTIFY_RESUME);
+		tracehook_notify_resume(regs);
+		if (current->replacement_session_keyring)
+			key_replace_session_keyring();
+		return 1;
+	}
+
+	/* Should not even reach here */
+	panic("%s: bad thread_info flags 0x%08x\n", __func__,
+		thread_info_flags);
+}
diff --git a/arch/hexagon/kernel/signal.c b/arch/hexagon/kernel/signal.c
index a1492a6..8a20e8e 100644
--- a/arch/hexagon/kernel/signal.c
+++ b/arch/hexagon/kernel/signal.c
@@ -199,7 +199,7 @@ static void handle_signal(int sig, siginfo_t *info, struct k_sigaction *ka,
 /*
  * Called from return-from-event code.
  */
-static void do_signal(struct pt_regs *regs)
+void do_signal(struct pt_regs *regs)
 {
 	struct k_sigaction sigact;
 	siginfo_t info;
@@ -216,8 +216,9 @@ static void do_signal(struct pt_regs *regs)
 	}
 
 	/*
-	 * If we came from a system call, handle the restart.
+	 * No (more) signals; if we came from a system call, handle the restart.
 	 */
+
 	if (regs->syscall_nr >= 0) {
 		switch (regs->r00) {
 		case -ERESTARTNOHAND:
@@ -240,17 +241,6 @@ no_restart:
 	restore_saved_sigmask();
 }
 
-void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
-{
-	if (thread_info_flags & _TIF_SIGPENDING)
-		do_signal(regs);
-
-	if (thread_info_flags & _TIF_NOTIFY_RESUME) {
-		clear_thread_flag(TIF_NOTIFY_RESUME);
-		tracehook_notify_resume(regs);
-	}
-}
-
 /*
  * Architecture-specific wrappers for signal-related system calls
  */
@@ -278,21 +268,12 @@ asmlinkage int sys_rt_sigreturn(void)
 	/* Restore the user's stack as well */
 	pt_psp(regs) = regs->r29;
 
-	/*
-	 * Leave a trace in the stack frame that this was a sigreturn.
-	 * If the system call is to replay, we've already restored the
-	 * number in the GPR slot and it will be regenerated on the
-	 * new system call trap entry. Note that if restore_sigcontext()
-	 * did something other than a bulk copy of the pt_regs struct,
-	 * we could avoid this assignment by simply not overwriting
-	 * regs->syscall_nr.
-	 */
-	regs->syscall_nr = __NR_rt_sigreturn;
+	regs->syscall_nr = -1;
 
 	if (restore_altstack(&frame->uc.uc_stack))
 		goto badframe;
 
-	return 0;
+	return regs->r00;
 
 badframe:
 	force_sig(SIGSEGV, current);
diff --git a/arch/hexagon/kernel/traps.c b/arch/hexagon/kernel/traps.c
index be5e2dd..d59ee62 100644
--- a/arch/hexagon/kernel/traps.c
+++ b/arch/hexagon/kernel/traps.c
@@ -356,7 +356,6 @@ long sys_syscall(void)
 
 void do_trap0(struct pt_regs *regs)
 {
-	unsigned long syscallret = 0;
 	syscall_fn syscall;
 
 	switch (pt_cause(regs)) {
@@ -396,21 +395,11 @@ void do_trap0(struct pt_regs *regs)
 		} else {
 			syscall = (syscall_fn)
 				  (sys_call_table[regs->syscall_nr]);
-			syscallret = syscall(regs->r00, regs->r01,
+			regs->r00 = syscall(regs->r00, regs->r01,
 				   regs->r02, regs->r03,
 				   regs->r04, regs->r05);
 		}
 
-		/*
-		 * If it was a sigreturn system call, don't overwrite
-		 * r0 value in stack frame with return value.
-		 *
-		 * __NR_sigreturn doesn't seem to exist in new unistd.h
-		 */
-
-		if (regs->syscall_nr != __NR_rt_sigreturn)
-			regs->r00 = syscallret;
-
 		/* allow strace to get the syscall return state  */
 		if (unlikely(test_thread_flag(TIF_SYSCALL_TRACE)))
 			tracehook_report_syscall_exit(regs, 0);
diff --git a/arch/hexagon/kernel/vm_entry.S b/arch/hexagon/kernel/vm_entry.S
index ffe4a42..053551e 100644
--- a/arch/hexagon/kernel/vm_entry.S
+++ b/arch/hexagon/kernel/vm_entry.S
@@ -274,6 +274,9 @@ event_dispatch:
 	callr	r1
 
 	/*
+	 * Coming back from the C-world, our thread info pointer
+	 * should be in the designated register (usually R19)
+	 *
 	 * If we were in kernel mode, we don't need to check scheduler
 	 * or signals if CONFIG_PREEMPT is not set.  If set, then it has
 	 * to jump to a need_resched kind of block.
@@ -286,67 +289,43 @@ event_dispatch:
 #endif
 
 	/*  "Nested control path" -- if the previous mode was kernel  */
-	R0 = memw(R29 + #_PT_ER_VMEST);
 	{
-		P0 = tstbit(R0, #HVM_VMEST_UM_SFT);
-		if (!P0.new) jump:nt restore_all;
+		R0 = memw(R29 + #_PT_ER_VMEST);
+		R16.L = #LO(do_work_pending);
 	}
-	/*
-	 * Returning from system call, normally coming back from user mode
-	 */
-return_from_syscall:
-	/*  Disable interrupts while checking TIF  */
-	R0 = #VM_INT_DISABLE
-	trap1(#HVM_TRAP1_VMSETIE)
-
-	/*
-	 * Coming back from the C-world, our thread info pointer
-	 * should be in the designated register (usually R19)
-	 */
-#if CONFIG_HEXAGON_ARCH_VERSION < 4
-	R1.L = #LO(_TIF_ALLWORK_MASK)
 	{
-		R1.H = #HI(_TIF_ALLWORK_MASK);
-		R0 = memw(THREADINFO_REG + #_THREAD_INFO_FLAGS);
-	}
-#else
-	{
-		R1 = ##_TIF_ALLWORK_MASK;
-		R0 = memw(THREADINFO_REG + #_THREAD_INFO_FLAGS);
+		P0 = tstbit(R0, #HVM_VMEST_UM_SFT);
+		if (!P0.new) jump:nt restore_all;
+		R16.H = #HI(do_work_pending);
+		R0 = #VM_INT_DISABLE;
 	}
-#endif
 
 	/*
-	 * Compare against the "return to userspace" _TIF_WORK_MASK
+	 * Check also the return from fork/system call, normally coming back from
+	 * user mode
+	 *
+	 * R16 needs to have do_work_pending, and R0 should have VM_INT_DISABLE
 	 */
-	R1 = and(R1,R0);
-	{ P0 = cmp.eq(R1,#0); if (!P0.new) jump:t work_pending;}
-	jump restore_all;  /*  we're outta here!  */
 
-work_pending:
+check_work_pending:
+	/*  Disable interrupts while checking TIF  */
+	trap1(#HVM_TRAP1_VMSETIE)
 	{
-		P0 = tstbit(R1, #TIF_NEED_RESCHED);
-		if (!P0.new) jump:nt work_notifysig;
+		R0 = R29;  /*  regs should still be at top of stack  */
+		R1 = memw(THREADINFO_REG + #_THREAD_INFO_FLAGS);
+		callr R16;
 	}
-	call schedule
-	jump return_from_syscall;  /*  check for more work  */
 
-work_notifysig:
-	/*  this is the part that's kind of fuzzy.  */
-	R1 = and(R0, #(_TIF_SIGPENDING | _TIF_NOTIFY_RESUME));
-	{
-		P0 = cmp.eq(R1, #0);
-		if P0.new jump:t restore_all;
-	}
 	{
-		R1 = R0; 	/* unsigned long thread_info_flags */
-		R0 = R29;	/* regs should still be at top of stack  */
+		P0 = cmp.eq(R0, #0); if (!P0.new) jump:nt check_work_pending;
+		R0 = #VM_INT_DISABLE;
 	}
-	call do_notify_resume
 
 restore_all:
-	/* Disable interrupts, if they weren't already, before reg restore.  */
-	R0 = #VM_INT_DISABLE
+	/*
+	 * Disable interrupts, if they weren't already, before reg restore.
+	 * R0 gets preloaded with #VM_INT_DISABLE before we get here.
+	 */
 	trap1(#HVM_TRAP1_VMSETIE)
 
 	/*  do the setregs here for VM 0.5  */
@@ -371,6 +350,7 @@ restore_all:
 	trap1(#HVM_TRAP1_VMRTE)
 	/* Notreached */
 
+
 	.globl _K_enter_genex
 _K_enter_genex:
 	vm_event_entry(do_genex)
@@ -390,9 +370,12 @@ _K_enter_machcheck:
 
 	.globl ret_from_fork
 ret_from_fork:
-	call schedule_tail
-	P0 = cmp.eq(R24, #0);
-	if P0 jump return_from_syscall
-	R0 = R25;
-	callr R24
-	jump return_from_syscall
+	{
+		call schedule_tail;
+		R16.H = #HI(do_work_pending);
+	}
+	{
+		R16.L = #LO(do_work_pending);
+		R0 = #VM_INT_DISABLE;
+		jump check_work_pending;
+	}
-- 
1.7.9.5

--

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 2/6] Hexagon: fix up int enable/disable at ret_from_fork

[Richard Kuo]

Check return coming out of check_work_pending, and if copy_thread
passed us a function in r24, call it.  Based on feedback from Al
Viro.

Signed-off-by: Richard Kuo <rkuo@codeaurora.org>
---
 arch/hexagon/kernel/vm_entry.S |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/hexagon/kernel/vm_entry.S b/arch/hexagon/kernel/vm_entry.S
index 053551e..0de8638 100644
--- a/arch/hexagon/kernel/vm_entry.S
+++ b/arch/hexagon/kernel/vm_entry.S
@@ -371,11 +371,20 @@ _K_enter_machcheck:
 	.globl ret_from_fork
 ret_from_fork:
 	{
-		call schedule_tail;
+		call schedule_tail
 		R16.H = #HI(do_work_pending);
 	}
 	{
+		P0 = cmp.eq(R24, #0);
 		R16.L = #LO(do_work_pending);
 		R0 = #VM_INT_DISABLE;
-		jump check_work_pending;
+	}
+	if P0 jump check_work_pending
+	{
+		R0 = R25;
+		callr R24
+	}
+	{
+		jump check_work_pending
+		R0 = #VM_INT_DISABLE;
 	}
-- 
1.7.9.5

--

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 3/6] Hexagon: use correct work mask when checking for more work

[Richard Kuo]

Signed-off-by: Richard Kuo <rkuo@codeaurora.org>
---
 arch/hexagon/kernel/process.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/hexagon/kernel/process.c b/arch/hexagon/kernel/process.c
index dc72ed5..8e90b0c 100644
--- a/arch/hexagon/kernel/process.c
+++ b/arch/hexagon/kernel/process.c
@@ -215,7 +215,7 @@ int dump_fpu(struct pt_regs *regs, elf_fpregset_t *fpu)
 
 int do_work_pending(struct pt_regs *regs, u32 thread_info_flags)
 {
-	if (!(thread_info_flags & _TIF_ALLWORK_MASK)) {
+	if (!(thread_info_flags & _TIF_WORK_MASK)) {
 		return 0;
 	}  /* shortcut -- no work to be done */
 
-- 
1.7.9.5

--

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 4/6] Hexagon: check to if we will overflow the signal stack

[Richard Kuo]

Signed-off-by: Richard Kuo <rkuo@codeaurora.org>
---
 arch/hexagon/kernel/signal.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/hexagon/kernel/signal.c b/arch/hexagon/kernel/signal.c
index 8a20e8e..097623c 100644
--- a/arch/hexagon/kernel/signal.c
+++ b/arch/hexagon/kernel/signal.c
@@ -41,6 +41,10 @@ static void __user *get_sigframe(struct k_sigaction *ka, struct pt_regs *regs,
 {
 	unsigned long sp = regs->r29;
 
+	/* check if we would overflow the alt stack */
+	if (on_sig_stack(sp) && !likely(on_sig_stack(sp - frame_size)))
+		return (void __user __force *)-1UL;
+
 	/* Switch to signal stack if appropriate */
 	if ((ka->sa.sa_flags & SA_ONSTACK) && (sas_ss_flags(sp) == 0))
 		sp = current->sas_ss_sp + current->sas_ss_size;
-- 
1.7.9.5

--

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 4/6] Hexagon: check to if we will overflow the signal stack

[Linas Vepstas]

On 3 April 2013 19:02, Richard Kuo <rkuo@codeaurora.org> wrote:

> +       /* check if we would overflow the alt stack */
> +       if (on_sig_stack(sp) && !likely(on_sig_stack(sp - frame_size)))
> +               return (void __user __force *)-1UL;

I found the !likely construction confusing, as its doing both a
'unlikely' (right?) and inverting the argument. It seems clearer,
to idiots like me, to write this as:

if (on_sig_stack(sp) && unlikely(!on_sig_stack(sp - frame_size)))

since where checking for overflow, and its unlikely that the overflow happened.

-- Linas

Re: [PATCH 4/6] Hexagon: check to if we will overflow the signal stack

[Richard Kuo]

On 04/04/2013 11:25 AM, Linas Vepstas wrote:
> On 3 April 2013 19:02, Richard Kuo <rkuo@codeaurora.org> wrote:
>
>> +       /* check if we would overflow the alt stack */
>> +       if (on_sig_stack(sp) && !likely(on_sig_stack(sp - frame_size)))
>> +               return (void __user __force *)-1UL;
> I found the !likely construction confusing, as its doing both a
> 'unlikely' (right?) and inverting the argument. It seems clearer,
> to idiots like me, to write this as:
>
> if (on_sig_stack(sp) && unlikely(!on_sig_stack(sp - frame_size)))
>
> since where checking for overflow, and its unlikely that the overflow happened.
>
> -- Linas

I'm not sure if putting a double negative in there will make it less not 
easy to understand...


-- 

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 5/6] Hexagon: break up user fn/arg register setting

[Richard Kuo]

Signed-off-by: Richard Kuo <rkuo@codeaurora.org>
---
 arch/hexagon/kernel/process.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/hexagon/kernel/process.c b/arch/hexagon/kernel/process.c
index 8e90b0c..2009377 100644
--- a/arch/hexagon/kernel/process.c
+++ b/arch/hexagon/kernel/process.c
@@ -113,7 +113,8 @@ int copy_thread(unsigned long clone_flags, unsigned long usp,
 	if (unlikely(p->flags & PF_KTHREAD)) {
 		memset(childregs, 0, sizeof(struct pt_regs));
 		/* r24 <- fn, r25 <- arg */
-		ss->r2524 = usp | ((u64)arg << 32);
+		ss->r24 = usp;
+		ss->r25 = arg;
 		pt_set_kmode(childregs);
 		return 0;
 	}
-- 
1.7.9.5

--

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 6/6] Hexagon: fix psp/sp macro

[Richard Kuo]

Based on feedback from Al Viro; previous-stack-pointer and
user reg for same should always be kept consistent.

Signed-off-by: Richard Kuo <rkuo@codeaurora.org>
---
 arch/hexagon/include/uapi/asm/registers.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/hexagon/include/uapi/asm/registers.h b/arch/hexagon/include/uapi/asm/registers.h
index 8050415..12005da 100644
--- a/arch/hexagon/include/uapi/asm/registers.h
+++ b/arch/hexagon/include/uapi/asm/registers.h
@@ -212,8 +212,7 @@ struct pt_regs {
 #define pt_badva(regs) ((regs)->hvmer.vmbadva)
 
 #define pt_set_rte_sp(regs, sp) do {\
-	pt_psp(regs) = (sp);\
-	(regs)->SP = (unsigned long) &((regs)->hvmer);\
+	pt_psp(regs) = (regs)->SP = (sp);\
 	} while (0)
 
 #define pt_set_kmode(regs) \
-- 
1.7.9.5

--

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation