[PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2 mode

[PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2 mode

[Nikolay Borisov]

The thread_save_fp macro has been defined so that it always reads the fp member
of the cpu_context_save struct. However, in the case of THUMB2 the fp is saved
not in the fp (r11) member but rather in r7.

This patch changes the way the macro is defined such that FP is read from the
correct place depending on whether we are a THUMB2 kernel or not. This enables
the backtrace in sitaution such as "echo t > /proc/sysrq-trigger" or the
function in which a process sleeping when "ps -Al" is invoked.

Another place where similar change is requried is when working with pt_reg.
regs->ARM_fp is defined such that r11 is always referenced as the frame pointer,
so the definition of ARM_fp needs to change depending on the type of kernel we
are running.

Changes since v1:
* Added changes for the pt_reg structure.

Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>

handle the case when we've got regs present
---
 arch/arm/include/asm/thread_info.h | 6 ++++++
 arch/arm/include/uapi/asm/ptrace.h | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index f989d7c..e4e4208 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -114,8 +114,14 @@ static inline struct thread_info *current_thread_info(void)
 	((unsigned long)(task_thread_info(tsk)->cpu_context.pc))
 #define thread_saved_sp(tsk)	\
 	((unsigned long)(task_thread_info(tsk)->cpu_context.sp))
+
+#ifndef CONFIG_THUMB2_KERNEL
 #define thread_saved_fp(tsk)	\
 	((unsigned long)(task_thread_info(tsk)->cpu_context.fp))
+#else
+#define thread_saved_fp(tsk)	\
+	((unsigned long)(task_thread_info(tsk)->cpu_context.r7))
+#endif
 
 extern void crunch_task_disable(struct thread_info *);
 extern void crunch_task_copy(struct thread_info *, void *);
diff --git a/arch/arm/include/uapi/asm/ptrace.h b/arch/arm/include/uapi/asm/ptrace.h
index 5af0ed1..92e1bae 100644
--- a/arch/arm/include/uapi/asm/ptrace.h
+++ b/arch/arm/include/uapi/asm/ptrace.h
@@ -131,7 +131,11 @@ struct pt_regs {
 #define ARM_lr		uregs[14]
 #define ARM_sp		uregs[13]
 #define ARM_ip		uregs[12]
+#ifndef CONFIG_THUMB2_KERNEL
 #define ARM_fp		uregs[11]
+#else
+#define ARM_fp		uregs[7]
+#endif
 #define ARM_r10		uregs[10]
 #define ARM_r9		uregs[9]
 #define ARM_r8		uregs[8]
-- 
1.8.1.5

[PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2 mode

[Anurag Aggarwal]

Seems good.

I would like to know what kind of testing you have done that for this,
Other than that you can add my review statement

On Fri, May 2, 2014 at 9:07 PM, Nikolay Borisov <Nikolay.Borisov@arm.com> wrote:
> The thread_save_fp macro has been defined so that it always reads the fp member
> of the cpu_context_save struct. However, in the case of THUMB2 the fp is saved
> not in the fp (r11) member but rather in r7.
>
> This patch changes the way the macro is defined such that FP is read from the
> correct place depending on whether we are a THUMB2 kernel or not. This enables
> the backtrace in sitaution such as "echo t > /proc/sysrq-trigger" or the
> function in which a process sleeping when "ps -Al" is invoked.
>
> Another place where similar change is requried is when working with pt_reg.
> regs->ARM_fp is defined such that r11 is always referenced as the frame pointer,
> so the definition of ARM_fp needs to change depending on the type of kernel we
> are running.
>
> Changes since v1:
> * Added changes for the pt_reg structure.
>
> Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>

Reviewed-by: Anurag Aggarwal <anurag19aggarwal@gmail.com>

>
> handle the case when we've got regs present
> ---
>  arch/arm/include/asm/thread_info.h | 6 ++++++
>  arch/arm/include/uapi/asm/ptrace.h | 4 ++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index f989d7c..e4e4208 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -114,8 +114,14 @@ static inline struct thread_info *current_thread_info(void)
>         ((unsigned long)(task_thread_info(tsk)->cpu_context.pc))
>  #define thread_saved_sp(tsk)   \
>         ((unsigned long)(task_thread_info(tsk)->cpu_context.sp))
> +
> +#ifndef CONFIG_THUMB2_KERNEL
>  #define thread_saved_fp(tsk)   \
>         ((unsigned long)(task_thread_info(tsk)->cpu_context.fp))
> +#else
> +#define thread_saved_fp(tsk)   \
> +       ((unsigned long)(task_thread_info(tsk)->cpu_context.r7))
> +#endif
>
>  extern void crunch_task_disable(struct thread_info *);
>  extern void crunch_task_copy(struct thread_info *, void *);
> diff --git a/arch/arm/include/uapi/asm/ptrace.h b/arch/arm/include/uapi/asm/ptrace.h
> index 5af0ed1..92e1bae 100644
> --- a/arch/arm/include/uapi/asm/ptrace.h
> +++ b/arch/arm/include/uapi/asm/ptrace.h
> @@ -131,7 +131,11 @@ struct pt_regs {
>  #define ARM_lr         uregs[14]
>  #define ARM_sp         uregs[13]
>  #define ARM_ip         uregs[12]
> +#ifndef CONFIG_THUMB2_KERNEL
>  #define ARM_fp         uregs[11]
> +#else
> +#define ARM_fp         uregs[7]
> +#endif
>  #define ARM_r10                uregs[10]
>  #define ARM_r9         uregs[9]
>  #define ARM_r8         uregs[8]
> --
> 1.8.1.5
>
>

[PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2 mode

[Nikolay Borisov]

Hi, 
> -----Original Message-----
> From: Anurag Aggarwal [mailto:anurag19aggarwal at gmail.com]
> Sent: 03 May 2014 07:57
> To: Nikolay Borisov
> Cc: linux-arm-kernel at lists.infradead.org; Dave P Martin; Catalin
> Marinas
> Subject: Re: [PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2
> mode
> 
> Seems good.
> 
> I would like to know what kind of testing you have done that for this,
> Other than that you can add my review statement
>

Without this patch if you use the magic sysrq to print backtrace of all
the running tasks in the system those that are sleeping would not print 
a backtrace. With the patch this works correctly. Another case where you
can observe the change is in the invocation of the "ps -Al" which shows
(among other things) which function the sleeping task is blocked in. 

> On Fri, May 2, 2014 at 9:07 PM, Nikolay Borisov
> <Nikolay.Borisov@arm.com> wrote:
> > The thread_save_fp macro has been defined so that it always reads the
> fp member
> > of the cpu_context_save struct. However, in the case of THUMB2 the fp
> is saved
> > not in the fp (r11) member but rather in r7.
> >
> > This patch changes the way the macro is defined such that FP is read
> from the
> > correct place depending on whether we are a THUMB2 kernel or not.
> This enables
> > the backtrace in sitaution such as "echo t > /proc/sysrq-trigger" or
> the
> > function in which a process sleeping when "ps -Al" is invoked.
> >
> > Another place where similar change is requried is when working with
> pt_reg.
> > regs->ARM_fp is defined such that r11 is always referenced as the
> frame pointer,
> > so the definition of ARM_fp needs to change depending on the type of
> kernel we
> > are running.
> >
> > Changes since v1:
> > * Added changes for the pt_reg structure.
> >
> > Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
> 
> Reviewed-by: Anurag Aggarwal <anurag19aggarwal@gmail.com>
> 
> >
> > handle the case when we've got regs present
> > ---
> >  arch/arm/include/asm/thread_info.h | 6 ++++++
> >  arch/arm/include/uapi/asm/ptrace.h | 4 ++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/arch/arm/include/asm/thread_info.h
> b/arch/arm/include/asm/thread_info.h
> > index f989d7c..e4e4208 100644
> > --- a/arch/arm/include/asm/thread_info.h
> > +++ b/arch/arm/include/asm/thread_info.h
> > @@ -114,8 +114,14 @@ static inline struct thread_info
> *current_thread_info(void)
> >         ((unsigned long)(task_thread_info(tsk)->cpu_context.pc))
> >  #define thread_saved_sp(tsk)   \
> >         ((unsigned long)(task_thread_info(tsk)->cpu_context.sp))
> > +
> > +#ifndef CONFIG_THUMB2_KERNEL
> >  #define thread_saved_fp(tsk)   \
> >         ((unsigned long)(task_thread_info(tsk)->cpu_context.fp))
> > +#else
> > +#define thread_saved_fp(tsk)   \
> > +       ((unsigned long)(task_thread_info(tsk)->cpu_context.r7))
> > +#endif
> >
> >  extern void crunch_task_disable(struct thread_info *);
> >  extern void crunch_task_copy(struct thread_info *, void *);
> > diff --git a/arch/arm/include/uapi/asm/ptrace.h
> b/arch/arm/include/uapi/asm/ptrace.h
> > index 5af0ed1..92e1bae 100644
> > --- a/arch/arm/include/uapi/asm/ptrace.h
> > +++ b/arch/arm/include/uapi/asm/ptrace.h
> > @@ -131,7 +131,11 @@ struct pt_regs {
> >  #define ARM_lr         uregs[14]
> >  #define ARM_sp         uregs[13]
> >  #define ARM_ip         uregs[12]
> > +#ifndef CONFIG_THUMB2_KERNEL
> >  #define ARM_fp         uregs[11]
> > +#else
> > +#define ARM_fp         uregs[7]
> > +#endif
> >  #define ARM_r10                uregs[10]
> >  #define ARM_r9         uregs[9]
> >  #define ARM_r8         uregs[8]
> > --
> > 1.8.1.5
> >
> >

[PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2 mode

[Anurag Aggarwal]

The test cases seems good, although I haven't been able to verify them myself.
Other than that everything seems good.

On Tue, May 6, 2014 at 2:32 PM, Nikolay Borisov <nikolay.borisov@arm.com> wrote:
> Hi,
>> -----Original Message-----
>> From: Anurag Aggarwal [mailto:anurag19aggarwal at gmail.com]
>> Sent: 03 May 2014 07:57
>> To: Nikolay Borisov
>> Cc: linux-arm-kernel at lists.infradead.org; Dave P Martin; Catalin
>> Marinas
>> Subject: Re: [PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2
>> mode
>>
>> Seems good.
>>
>> I would like to know what kind of testing you have done that for this,
>> Other than that you can add my review statement
>>
>
> Without this patch if you use the magic sysrq to print backtrace of all
> the running tasks in the system those that are sleeping would not print
> a backtrace. With the patch this works correctly. Another case where you
> can observe the change is in the invocation of the "ps -Al" which shows
> (among other things) which function the sleeping task is blocked in.
>
>> On Fri, May 2, 2014 at 9:07 PM, Nikolay Borisov
>> <Nikolay.Borisov@arm.com> wrote:
>> > The thread_save_fp macro has been defined so that it always reads the
>> fp member
>> > of the cpu_context_save struct. However, in the case of THUMB2 the fp
>> is saved
>> > not in the fp (r11) member but rather in r7.
>> >
>> > This patch changes the way the macro is defined such that FP is read
>> from the
>> > correct place depending on whether we are a THUMB2 kernel or not.
>> This enables
>> > the backtrace in sitaution such as "echo t > /proc/sysrq-trigger" or
>> the
>> > function in which a process sleeping when "ps -Al" is invoked.
>> >
>> > Another place where similar change is requried is when working with
>> pt_reg.
>> > regs->ARM_fp is defined such that r11 is always referenced as the
>> frame pointer,
>> > so the definition of ARM_fp needs to change depending on the type of
>> kernel we
>> > are running.
>> >
>> > Changes since v1:
>> > * Added changes for the pt_reg structure.
>> >
>> > Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
>>
>> Reviewed-by: Anurag Aggarwal <anurag19aggarwal@gmail.com>
>>
>> >
>> > handle the case when we've got regs present
>> > ---
>> >  arch/arm/include/asm/thread_info.h | 6 ++++++
>> >  arch/arm/include/uapi/asm/ptrace.h | 4 ++++
>> >  2 files changed, 10 insertions(+)
>> >
>> > diff --git a/arch/arm/include/asm/thread_info.h
>> b/arch/arm/include/asm/thread_info.h
>> > index f989d7c..e4e4208 100644
>> > --- a/arch/arm/include/asm/thread_info.h
>> > +++ b/arch/arm/include/asm/thread_info.h
>> > @@ -114,8 +114,14 @@ static inline struct thread_info
>> *current_thread_info(void)
>> >         ((unsigned long)(task_thread_info(tsk)->cpu_context.pc))
>> >  #define thread_saved_sp(tsk)   \
>> >         ((unsigned long)(task_thread_info(tsk)->cpu_context.sp))
>> > +
>> > +#ifndef CONFIG_THUMB2_KERNEL
>> >  #define thread_saved_fp(tsk)   \
>> >         ((unsigned long)(task_thread_info(tsk)->cpu_context.fp))
>> > +#else
>> > +#define thread_saved_fp(tsk)   \
>> > +       ((unsigned long)(task_thread_info(tsk)->cpu_context.r7))
>> > +#endif
>> >
>> >  extern void crunch_task_disable(struct thread_info *);
>> >  extern void crunch_task_copy(struct thread_info *, void *);
>> > diff --git a/arch/arm/include/uapi/asm/ptrace.h
>> b/arch/arm/include/uapi/asm/ptrace.h
>> > index 5af0ed1..92e1bae 100644
>> > --- a/arch/arm/include/uapi/asm/ptrace.h
>> > +++ b/arch/arm/include/uapi/asm/ptrace.h
>> > @@ -131,7 +131,11 @@ struct pt_regs {
>> >  #define ARM_lr         uregs[14]
>> >  #define ARM_sp         uregs[13]
>> >  #define ARM_ip         uregs[12]
>> > +#ifndef CONFIG_THUMB2_KERNEL
>> >  #define ARM_fp         uregs[11]
>> > +#else
>> > +#define ARM_fp         uregs[7]
>> > +#endif
>> >  #define ARM_r10                uregs[10]
>> >  #define ARM_r9         uregs[9]
>> >  #define ARM_r8         uregs[8]
>> > --
>> > 1.8.1.5
>> >
>> >
>
>
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Anurag Aggarwal

[PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2 mode

[Dave Martin]

On Fri, May 02, 2014 at 04:37:36PM +0100, Nikolay Borisov wrote:
> The thread_save_fp macro has been defined so that it always reads the fp member
> of the cpu_context_save struct. However, in the case of THUMB2 the fp is saved
> not in the fp (r11) member but rather in r7.
> 
> This patch changes the way the macro is defined such that FP is read from the
> correct place depending on whether we are a THUMB2 kernel or not. This enables
> the backtrace in sitaution such as "echo t > /proc/sysrq-trigger" or the
> function in which a process sleeping when "ps -Al" is invoked.
> 
> Another place where similar change is requried is when working with pt_reg.
> regs->ARM_fp is defined such that r11 is always referenced as the frame pointer,
> so the definition of ARM_fp needs to change depending on the type of kernel we
> are running.

(Keep your S-o-b line here, but move the "change log" stuff after the
--- tearoff line so it doesn't form part of the commit message.)

> Changes since v1:
> * Added changes for the pt_reg structure.
> 
> Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
> 
> handle the case when we've got regs present
> ---
>  arch/arm/include/asm/thread_info.h | 6 ++++++
>  arch/arm/include/uapi/asm/ptrace.h | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index f989d7c..e4e4208 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -114,8 +114,14 @@ static inline struct thread_info *current_thread_info(void)
>  	((unsigned long)(task_thread_info(tsk)->cpu_context.pc))
>  #define thread_saved_sp(tsk)	\
>  	((unsigned long)(task_thread_info(tsk)->cpu_context.sp))
> +
> +#ifndef CONFIG_THUMB2_KERNEL
>  #define thread_saved_fp(tsk)	\
>  	((unsigned long)(task_thread_info(tsk)->cpu_context.fp))
> +#else
> +#define thread_saved_fp(tsk)	\
> +	((unsigned long)(task_thread_info(tsk)->cpu_context.r7))
> +#endif
>  
>  extern void crunch_task_disable(struct thread_info *);
>  extern void crunch_task_copy(struct thread_info *, void *);
> diff --git a/arch/arm/include/uapi/asm/ptrace.h b/arch/arm/include/uapi/asm/ptrace.h
> index 5af0ed1..92e1bae 100644
> --- a/arch/arm/include/uapi/asm/ptrace.h
> +++ b/arch/arm/include/uapi/asm/ptrace.h
> @@ -131,7 +131,11 @@ struct pt_regs {
>  #define ARM_lr		uregs[14]
>  #define ARM_sp		uregs[13]
>  #define ARM_ip		uregs[12]
> +#ifndef CONFIG_THUMB2_KERNEL
>  #define ARM_fp		uregs[11]
> +#else
> +#define ARM_fp		uregs[7]
> +#endif

I'm not sure about this.  There seem to be other bits of code that
assume the ARM_fp name maps to the fixed register defined by the PCS,
i.e. always r11 and not the "frame pointer".  For example, see
arch/arm/kernel/kgdb.c: I think we'll end up passing the wrong regs to
gdb in this case.

It may be better to work around the usage of these #defines locally in
the unwinder code instead.

Cheers
---Dave