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: Mon, 15 Feb 2016 23:06:15 +1000 Message-ID: <56C1CD47.8000109@uclinux.org> References: <1455518189-12478-1-git-send-email-gerg@uclinux.org> <20160215100301.GA30557@frolo.macqel> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from icp-osb-irony-out4.external.iinet.net.au ([203.59.1.220]:51613 "EHLO icp-osb-irony-out4.external.iinet.net.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751278AbcBONGV (ORCPT ); Mon, 15 Feb 2016 08:06:21 -0500 In-Reply-To: <20160215100301.GA30557@frolo.macqel> Sender: linux-m68k-owner@vger.kernel.org List-Id: linux-m68k@vger.kernel.org To: Philippe De Muyter Cc: linux-m68k@vger.kernel.org Hi Philippe, On 15/02/16 20:03, Philippe De Muyter wrote: > On Mon, Feb 15, 2016 at 04:36:29PM +1000, 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. > > As an intermediate solution, I think that you can avoid part of that overhead ... > >> >> 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 > > if you remove the above asm instruction ... > >> jbsr do_sigreturn >> + addql #8,%sp > > change the 8 by 4 here ... > >> 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 > > ditto here ... > >> jbsr do_rt_sigreturn >> + addql #8,%sp > > and here ... > >> 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) > > and if you use the following prototype : > asmlinkage int do_sigreturn(struct switch_stack *sw) ... > >> { >> - struct switch_stack *sw = (struct switch_stack *) &__unused; >> - struct pt_regs *regs = (struct pt_regs *) (sw + 1); > > and keep the 'C' instruction above > >> 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) > same here >> { >> - struct switch_stack *sw = (struct switch_stack *) &__unused; >> - struct pt_regs *regs = (struct pt_regs *) (sw + 1); > and here >> unsigned long usp = rdusp(); >> struct rt_sigframe __user *frame = (struct rt_sigframe __user *)(usp - 4); >> sigset_t set; Those changes ultimately make no difference to code size. Although the entry point code is smaller the resulting generated code for do_sigreturn() and do_rt_sigreturn() is slightly larger. (At least that is the case with the gcc-5.3 compiler I am currently using). Personally I don't like having to keep that ugly stack referencing definition of regs based on sw either. I would like to remove that. "sw" isn't actually used in do_sigreturn() at all, so that could be removed from the its parameters. I chose to keep it there in this patch for consistency with do_rt_sigreturn(). Regards Greg