From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg Ungerer Subject: Re: [PATCH v2] m68k: use conventional function parameters for do_sigreturn Date: Wed, 24 Feb 2016 11:10:52 +1000 Message-ID: <56CD031C.5040101@uclinux.org> References: <1455518189-12478-1-git-send-email-gerg@uclinux.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: Received: from icp-osb-irony-out2.external.iinet.net.au ([203.59.1.155]:58188 "EHLO icp-osb-irony-out2.external.iinet.net.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751129AbcBXBKJ (ORCPT ); Tue, 23 Feb 2016 20:10:09 -0500 In-Reply-To: <1455518189-12478-1-git-send-email-gerg@uclinux.org> Sender: linux-m68k-owner@vger.kernel.org List-Id: linux-m68k@vger.kernel.org To: Geert Uytterhoeven Cc: linux-m68k@vger.kernel.org Hi Geert, Do you have any objections or issues with this change? Andreas and Philippe kindly gave feedback, which I have addressed (I think to everyone's satisfaction?) I would very much like to see this applied to resolve the compilation issue for non-MMU with modern versions of gcc. I don't mind pushing through my tree. I just want to make sure first that nobody objects to this change or has any remaining issues with it. Regards Greg On 15/02/16 16:36, Greg Ungerer wrote: > Create conventional stack parameters for the calls to do_sigreturn and > do_rt_sigreturn. The current C code for do_sigreturn and do_rt_sigreturn > dig into the stack to create local pointers to the saved switch stack > and the pt_regs structs. > > The motivation for this change is a problem with non-MMU targets that > have broken signal return paths on newer versions of gcc. It appears as > though gcc has determined that the pointers into the saved stack structs, > and the saved structs themselves, are function parameters and updates to > them will be lost on function return, so they are optimized away. This > results in large parts of restore_sigcontext() and mangle_kernel_stack() > functions being removed. Of course this results in non-functional code > causing kernel oops. This problem has been observed with gcc version > 5.2 and 5.3, and probably exists in earlier versions as well. > > Using conventional stack parameter pointers passed to these functions has > the advantage of the code here not needing to know the exact details of > how the underlying entry handler layed these structs out on the stack. > So the rather ugly pointer setup casting and arg referencing can be > removed. > > The resulting code after this change is a few bytes larger (due to the > overhead of creating the stack args and their tear down). Not being hot > paths I don't think this is too much of a problem here. > > An alternative solution is to put a barrier() in the do_sigreturn() code, > but this doesn't feel quite as clean as this solution. > > This change has been compile tested on all defconfigs, and run tested on > Atari (through aranym), ColdFire with MMU (M5407EVB) and ColdFire with > no-MMU (QEMU and M5208EVB). > > Signed-off-by: Greg Ungerer > --- > v2: reworded the commit log message with better description of problem > > arch/m68k/kernel/entry.S | 6 ++++++ > arch/m68k/kernel/signal.c | 8 ++------ > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S > index b54ac7a..97cd3ea 100644 > --- a/arch/m68k/kernel/entry.S > +++ b/arch/m68k/kernel/entry.S > @@ -71,13 +71,19 @@ ENTRY(__sys_vfork) > > ENTRY(sys_sigreturn) > SAVE_SWITCH_STACK > + movel %sp,%sp@- | switch_stack pointer > + pea %sp@(SWITCH_STACK_SIZE+4) | pt_regs pointer > jbsr do_sigreturn > + addql #8,%sp > RESTORE_SWITCH_STACK > rts > > ENTRY(sys_rt_sigreturn) > SAVE_SWITCH_STACK > + movel %sp,%sp@- | switch_stack pointer > + pea %sp@(SWITCH_STACK_SIZE+4) | pt_regs pointer > jbsr do_rt_sigreturn > + addql #8,%sp > RESTORE_SWITCH_STACK > rts > > diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c > index af1c4f3..2dcee3a 100644 > --- a/arch/m68k/kernel/signal.c > +++ b/arch/m68k/kernel/signal.c > @@ -737,10 +737,8 @@ badframe: > return 1; > } > > -asmlinkage int do_sigreturn(unsigned long __unused) > +asmlinkage int do_sigreturn(struct pt_regs *regs, struct switch_stack *sw) > { > - struct switch_stack *sw = (struct switch_stack *) &__unused; > - struct pt_regs *regs = (struct pt_regs *) (sw + 1); > unsigned long usp = rdusp(); > struct sigframe __user *frame = (struct sigframe __user *)(usp - 4); > sigset_t set; > @@ -764,10 +762,8 @@ badframe: > return 0; > } > > -asmlinkage int do_rt_sigreturn(unsigned long __unused) > +asmlinkage int do_rt_sigreturn(struct pt_regs *regs, struct switch_stack *sw) > { > - struct switch_stack *sw = (struct switch_stack *) &__unused; > - struct pt_regs *regs = (struct pt_regs *) (sw + 1); > unsigned long usp = rdusp(); > struct rt_sigframe __user *frame = (struct rt_sigframe __user *)(usp - 4); > sigset_t set; >