[PATCH] mips: Save all registers when saving the frame

[PATCH] mips: Save all registers when saving the frame

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The MIPS frame save code was just saving a few registers, enough to
do a backtrace if every function set up a frame.  However, this is
not working if you are using DWARF unwinding, because most of the
registers are wrong.  This was causing kdump backtraces to be short
or bogus.

So save all the registers.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 arch/mips/include/asm/stacktrace.h | 64 +++++++++++++++++++++++++++++---------
 1 file changed, 50 insertions(+), 14 deletions(-)

diff --git a/arch/mips/include/asm/stacktrace.h b/arch/mips/include/asm/stacktrace.h
index 780ee2c..05a2195 100644
--- a/arch/mips/include/asm/stacktrace.h
+++ b/arch/mips/include/asm/stacktrace.h
@@ -2,6 +2,8 @@
 #define _ASM_STACKTRACE_H
 
 #include <asm/ptrace.h>
+#include <asm/asm.h>
+#include <linux/stringify.h>
 
 #ifdef CONFIG_KALLSYMS
 extern int raw_show_trace;
@@ -20,6 +22,14 @@ static inline unsigned long unwind_stack(struct task_struct *task,
 }
 #endif
 
+#define STR_PTR_LA    __stringify(PTR_LA)
+#define STR_LONG_S    __stringify(LONG_S)
+#define STR_LONG_L    __stringify(LONG_L)
+#define STR_LONGSIZE  __stringify(LONGSIZE)
+
+#define STORE_ONE_REG(r) \
+    STR_LONG_S   " $" __stringify(r)",("STR_LONGSIZE"*"__stringify(r)")(%1)\n\t"
+
 static __always_inline void prepare_frametrace(struct pt_regs *regs)
 {
 #ifndef CONFIG_KALLSYMS
@@ -32,21 +42,47 @@ static __always_inline void prepare_frametrace(struct pt_regs *regs)
 	__asm__ __volatile__(
 		".set push\n\t"
 		".set noat\n\t"
-#ifdef CONFIG_64BIT
-		"1: dla $1, 1b\n\t"
-		"sd $1, %0\n\t"
-		"sd $29, %1\n\t"
-		"sd $31, %2\n\t"
-#else
-		"1: la $1, 1b\n\t"
-		"sw $1, %0\n\t"
-		"sw $29, %1\n\t"
-		"sw $31, %2\n\t"
-#endif
+		/* Store $1 so we can use it */
+		STR_LONG_S " $1,"STR_LONGSIZE"(%1)\n\t"
+		/* Store the PC */
+		"1: " STR_PTR_LA " $1, 1b\n\t"
+		STR_LONG_S " $1,%0\n\t"
+		STORE_ONE_REG(2)
+		STORE_ONE_REG(3)
+		STORE_ONE_REG(4)
+		STORE_ONE_REG(5)
+		STORE_ONE_REG(6)
+		STORE_ONE_REG(7)
+		STORE_ONE_REG(8)
+		STORE_ONE_REG(9)
+		STORE_ONE_REG(10)
+		STORE_ONE_REG(11)
+		STORE_ONE_REG(12)
+		STORE_ONE_REG(13)
+		STORE_ONE_REG(14)
+		STORE_ONE_REG(15)
+		STORE_ONE_REG(16)
+		STORE_ONE_REG(17)
+		STORE_ONE_REG(18)
+		STORE_ONE_REG(19)
+		STORE_ONE_REG(20)
+		STORE_ONE_REG(21)
+		STORE_ONE_REG(22)
+		STORE_ONE_REG(23)
+		STORE_ONE_REG(24)
+		STORE_ONE_REG(25)
+		STORE_ONE_REG(26)
+		STORE_ONE_REG(27)
+		STORE_ONE_REG(28)
+		STORE_ONE_REG(29)
+		STORE_ONE_REG(30)
+		STORE_ONE_REG(31)
+		/* Restore $1 */
+		STR_LONG_L " $1,(%1)\n\t"
 		".set pop\n\t"
-		: "=m" (regs->cp0_epc),
-		"=m" (regs->regs[29]), "=m" (regs->regs[31])
-		: : "memory");
+		: "=m" (regs->cp0_epc)
+		: "r" (regs->regs)
+		: "memory");
 }
 
 #endif /* _ASM_STACKTRACE_H */
-- 
1.8.3.1

Re: [PATCH] mips: Save all registers when saving the frame

[Corey Minyard]

Well, there's a bug I noticed in this patch, $1 is restored from the
wrong location.
I'm not sure $1 ($at) needs to be restored at all, really.  I guess make
this a RFC.

-corey


On 09/16/2014 04:45 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> The MIPS frame save code was just saving a few registers, enough to
> do a backtrace if every function set up a frame.  However, this is
> not working if you are using DWARF unwinding, because most of the
> registers are wrong.  This was causing kdump backtraces to be short
> or bogus.
>
> So save all the registers.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
>  arch/mips/include/asm/stacktrace.h | 64 +++++++++++++++++++++++++++++---------
>  1 file changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/arch/mips/include/asm/stacktrace.h b/arch/mips/include/asm/stacktrace.h
> index 780ee2c..05a2195 100644
> --- a/arch/mips/include/asm/stacktrace.h
> +++ b/arch/mips/include/asm/stacktrace.h
> @@ -2,6 +2,8 @@
>  #define _ASM_STACKTRACE_H
>  
>  #include <asm/ptrace.h>
> +#include <asm/asm.h>
> +#include <linux/stringify.h>
>  
>  #ifdef CONFIG_KALLSYMS
>  extern int raw_show_trace;
> @@ -20,6 +22,14 @@ static inline unsigned long unwind_stack(struct task_struct *task,
>  }
>  #endif
>  
> +#define STR_PTR_LA    __stringify(PTR_LA)
> +#define STR_LONG_S    __stringify(LONG_S)
> +#define STR_LONG_L    __stringify(LONG_L)
> +#define STR_LONGSIZE  __stringify(LONGSIZE)
> +
> +#define STORE_ONE_REG(r) \
> +    STR_LONG_S   " $" __stringify(r)",("STR_LONGSIZE"*"__stringify(r)")(%1)\n\t"
> +
>  static __always_inline void prepare_frametrace(struct pt_regs *regs)
>  {
>  #ifndef CONFIG_KALLSYMS
> @@ -32,21 +42,47 @@ static __always_inline void prepare_frametrace(struct pt_regs *regs)
>  	__asm__ __volatile__(
>  		".set push\n\t"
>  		".set noat\n\t"
> -#ifdef CONFIG_64BIT
> -		"1: dla $1, 1b\n\t"
> -		"sd $1, %0\n\t"
> -		"sd $29, %1\n\t"
> -		"sd $31, %2\n\t"
> -#else
> -		"1: la $1, 1b\n\t"
> -		"sw $1, %0\n\t"
> -		"sw $29, %1\n\t"
> -		"sw $31, %2\n\t"
> -#endif
> +		/* Store $1 so we can use it */
> +		STR_LONG_S " $1,"STR_LONGSIZE"(%1)\n\t"
> +		/* Store the PC */
> +		"1: " STR_PTR_LA " $1, 1b\n\t"
> +		STR_LONG_S " $1,%0\n\t"
> +		STORE_ONE_REG(2)
> +		STORE_ONE_REG(3)
> +		STORE_ONE_REG(4)
> +		STORE_ONE_REG(5)
> +		STORE_ONE_REG(6)
> +		STORE_ONE_REG(7)
> +		STORE_ONE_REG(8)
> +		STORE_ONE_REG(9)
> +		STORE_ONE_REG(10)
> +		STORE_ONE_REG(11)
> +		STORE_ONE_REG(12)
> +		STORE_ONE_REG(13)
> +		STORE_ONE_REG(14)
> +		STORE_ONE_REG(15)
> +		STORE_ONE_REG(16)
> +		STORE_ONE_REG(17)
> +		STORE_ONE_REG(18)
> +		STORE_ONE_REG(19)
> +		STORE_ONE_REG(20)
> +		STORE_ONE_REG(21)
> +		STORE_ONE_REG(22)
> +		STORE_ONE_REG(23)
> +		STORE_ONE_REG(24)
> +		STORE_ONE_REG(25)
> +		STORE_ONE_REG(26)
> +		STORE_ONE_REG(27)
> +		STORE_ONE_REG(28)
> +		STORE_ONE_REG(29)
> +		STORE_ONE_REG(30)
> +		STORE_ONE_REG(31)
> +		/* Restore $1 */
> +		STR_LONG_L " $1,(%1)\n\t"
>  		".set pop\n\t"
> -		: "=m" (regs->cp0_epc),
> -		"=m" (regs->regs[29]), "=m" (regs->regs[31])
> -		: : "memory");
> +		: "=m" (regs->cp0_epc)
> +		: "r" (regs->regs)
> +		: "memory");
>  }
>  
>  #endif /* _ASM_STACKTRACE_H */

Re: [PATCH] mips: Save all registers when saving the frame

[Ralf Baechle]

On Tue, Sep 16, 2014 at 04:45:25PM -0500, minyard@acm.org wrote:

> From: Corey Minyard <cminyard@mvista.com>
> 
> The MIPS frame save code was just saving a few registers, enough to
> do a backtrace if every function set up a frame.  However, this is
> not working if you are using DWARF unwinding, because most of the
> registers are wrong.  This was causing kdump backtraces to be short
> or bogus.
> 
> So save all the registers.

The stratey of partial and full stack frames was developed in '97 to bring
down the syscall overhead.  It certaily was very effective - it brought
down the syscall latency to the level of Alphas running at much higher
clock.

That certainly worked well back then for kernel 2.0 / 2.2.  But the syscall
code has become much more complex.  Since then support for 64 bit kernels,
two 32 bit ABIs running on a 64 bit kernels and numerous features that
changed the once simple syscall path have been implemented.  My gut feeling
is it might be worth to yank out the whole optimization to see how much
code complexity we get rid of in exchange for how much extra syscall
latency.

  Ralf

Re: [PATCH] mips: Save all registers when saving the frame

[Corey Minyard]

On 09/18/2014 04:58 AM, Ralf Baechle wrote:
> On Tue, Sep 16, 2014 at 04:45:25PM -0500, minyard@acm.org wrote:
>
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> The MIPS frame save code was just saving a few registers, enough to
>> do a backtrace if every function set up a frame.  However, this is
>> not working if you are using DWARF unwinding, because most of the
>> registers are wrong.  This was causing kdump backtraces to be short
>> or bogus.
>>
>> So save all the registers.
> The stratey of partial and full stack frames was developed in '97 to bring
> down the syscall overhead.  It certaily was very effective - it brought
> down the syscall latency to the level of Alphas running at much higher
> clock.
>
> That certainly worked well back then for kernel 2.0 / 2.2.  But the syscall
> code has become much more complex.  Since then support for 64 bit kernels,
> two 32 bit ABIs running on a 64 bit kernels and numerous features that
> changed the once simple syscall path have been implemented.  My gut feeling
> is it might be worth to yank out the whole optimization to see how much
> code complexity we get rid of in exchange for how much extra syscall
> latency.

I"m not sure I understand.  From what I can tell, this code is only
called by
things that print stack traces, kdb, and kexec/kdump.  So it shouldn't be in
any normal syscall path.

This patch will currently only help kdump, but it will be necessary if
anyone
adds MIPS support for DWARF unwinding for stack traces.  And you'd have
to fix some things in context switching, too, I think.

>From what I can tell the partial save for syscalls is a good idea.  You
don't have
to save half the registers and it doesn't affect tracebacks, kdump, or
anything else
like that.

-corey

Re: [PATCH] mips: Save all registers when saving the frame

[Corey Minyard]

Ping?  I haven't heard anything on this.

Thanks,

-corey

On 09/18/2014 07:58 AM, Corey Minyard wrote:
> On 09/18/2014 04:58 AM, Ralf Baechle wrote:
>> On Tue, Sep 16, 2014 at 04:45:25PM -0500, minyard@acm.org wrote:
>>
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> The MIPS frame save code was just saving a few registers, enough to
>>> do a backtrace if every function set up a frame.  However, this is
>>> not working if you are using DWARF unwinding, because most of the
>>> registers are wrong.  This was causing kdump backtraces to be short
>>> or bogus.
>>>
>>> So save all the registers.
>> The stratey of partial and full stack frames was developed in '97 to bring
>> down the syscall overhead.  It certaily was very effective - it brought
>> down the syscall latency to the level of Alphas running at much higher
>> clock.
>>
>> That certainly worked well back then for kernel 2.0 / 2.2.  But the syscall
>> code has become much more complex.  Since then support for 64 bit kernels,
>> two 32 bit ABIs running on a 64 bit kernels and numerous features that
>> changed the once simple syscall path have been implemented.  My gut feeling
>> is it might be worth to yank out the whole optimization to see how much
>> code complexity we get rid of in exchange for how much extra syscall
>> latency.
> I"m not sure I understand.  From what I can tell, this code is only
> called by
> things that print stack traces, kdb, and kexec/kdump.  So it shouldn't be in
> any normal syscall path.
>
> This patch will currently only help kdump, but it will be necessary if
> anyone
> adds MIPS support for DWARF unwinding for stack traces.  And you'd have
> to fix some things in context switching, too, I think.
>
> From what I can tell the partial save for syscalls is a good idea.  You
> don't have
> to save half the registers and it doesn't affect tracebacks, kdump, or
> anything else
> like that.
>
> -corey