[PATCH v2 0/2] arm64: unwind_frame for interrupt stack

[PATCH v2 0/2] arm64: unwind_frame for interrupt stack

[AKASHI Takahiro]

Jungseok is going to introduce a dedicated interrupt stack[1] for arm64.
This patch is my second proposal to support interrupt stack in
unwind_frame() and address an issue I mentioned in [2].

Patch[3] is also a prerequisite.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/378699.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/377723.html
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/378700.html

AKASHI Takahiro (2):
  arm64: revamp unwind_frame for interrupt stack
  arm64: fix dump_backtrace() to show correct pt_regs at interrupt

 arch/arm64/include/asm/exception.h |    7 +++----
 arch/arm64/include/asm/traps.h     |    7 -------
 arch/arm64/kernel/entry.S          |   22 ++++++++++++++++++++--
 arch/arm64/kernel/stacktrace.c     |   14 +++++++++++++-
 arch/arm64/kernel/traps.c          |    9 +++++++--
 arch/arm64/kernel/vmlinux.lds.S    |    7 +++++++
 6 files changed, 50 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/2] arm64: revamp unwind_frame for interrupt stack

[AKASHI Takahiro]

This patch allows unwind_frame() to traverse from interrupt stack
to process stack correctly by having a dummy stack frame for irq
exception entry created at its prologue.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/kernel/entry.S      |   22 ++++++++++++++++++++--
 arch/arm64/kernel/stacktrace.c |   14 +++++++++++++-
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index c8e0bcf..779f807 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -186,8 +186,26 @@ alternative_endif
 	and	x23, x23, #~(IRQ_STACK_SIZE - 1)
 	cmp	x20, x23			// check irq re-enterance
 	mov	x19, sp
-	csel	x23, x19, x24, eq		// x24 = top of irq stack
-	mov	sp, x23
+	beq	1f
+	mov	sp, x24				// x24 = top of irq stack
+	stp	x29, x19, [sp, #-16]!		// for sanity check
+	stp	x29, x22, [sp, #-16]!		// dummy stack frame
+	mov	x29, sp
+1:
+	/*
+	 * Layout of interrupt stack after this macro is invoked:
+	 *
+	 *     |                |
+	 *-0x20+----------------+ <= dummy stack frame
+	 *     |      fp        |    : fp on process stack
+	 *-0x18+----------------+
+	 *     |      lr        |    : return address
+	 *-0x10+----------------+
+	 *     |    fp (copy)   |    : for sanity check
+	 * -0x8+----------------+
+	 *     |      sp        |    : sp on process stack
+	 *  0x0+----------------+
+	 */
 	.endm
 
 	/*
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 407991b..03611a1 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -43,12 +43,24 @@ int notrace unwind_frame(struct stackframe *frame)
 	low  = frame->sp;
 	high = ALIGN(low, THREAD_SIZE);
 
-	if (fp < low || fp > high - 0x18 || fp & 0xf)
+	if (fp < low || fp > high - 0x20 || fp & 0xf)
 		return -EINVAL;
 
 	frame->sp = fp + 0x10;
 	frame->fp = *(unsigned long *)(fp);
 	/*
+	 * check whether we are going to walk trough from interrupt stack
+	 * to process stack
+	 * If the previous frame is the initial (dummy) stack frame on
+	 * interrupt stack, frame->sp now points to just below the frame
+	 * (dummy frame + 0x10).
+	 * See entry.S
+	 */
+#define STACK_LOW(addr) round_down((addr), THREAD_SIZE)
+	if ((STACK_LOW(frame->sp) != STACK_LOW(frame->fp)) &&
+			(frame->fp == *(unsigned long *)frame->sp))
+		frame->sp = *((unsigned long *)(frame->sp + 8));
+	/*
 	 * -4 here because we care about the PC at time of bl,
 	 * not where the return will go.
 	 */
-- 
1.7.9.5

[PATCH v2 1/2] arm64: revamp unwind_frame for interrupt stack

[Jungseok Lee]

On Oct 20, 2015, at 5:00 PM, AKASHI Takahiro wrote:
> This patch allows unwind_frame() to traverse from interrupt stack
> to process stack correctly by having a dummy stack frame for irq
> exception entry created at its prologue.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/kernel/entry.S      |   22 ++++++++++++++++++++--
> arch/arm64/kernel/stacktrace.c |   14 +++++++++++++-
> 2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index c8e0bcf..779f807 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -186,8 +186,26 @@ alternative_endif
> 	and	x23, x23, #~(IRQ_STACK_SIZE - 1)
> 	cmp	x20, x23			// check irq re-enterance
> 	mov	x19, sp
> -	csel	x23, x19, x24, eq		// x24 = top of irq stack
> -	mov	sp, x23
> +	beq	1f
> +	mov	sp, x24				// x24 = top of irq stack
> +	stp	x29, x19, [sp, #-16]!		// for sanity check
> +	stp	x29, x22, [sp, #-16]!		// dummy stack frame
> +	mov	x29, sp
> +1:
> +	/*
> +	 * Layout of interrupt stack after this macro is invoked:
> +	 *
> +	 *     |                |
> +	 *-0x20+----------------+ <= dummy stack frame
> +	 *     |      fp        |    : fp on process stack
> +	 *-0x18+----------------+
> +	 *     |      lr        |    : return address
> +	 *-0x10+----------------+
> +	 *     |    fp (copy)   |    : for sanity check
> +	 * -0x8+----------------+
> +	 *     |      sp        |    : sp on process stack
> +	 *  0x0+----------------+
> +	 */
> 	.endm
> 
> 	/*
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 407991b..03611a1 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -43,12 +43,24 @@ int notrace unwind_frame(struct stackframe *frame)
> 	low  = frame->sp;
> 	high = ALIGN(low, THREAD_SIZE);
> 
> -	if (fp < low || fp > high - 0x18 || fp & 0xf)
> +	if (fp < low || fp > high - 0x20 || fp & 0xf)
> 		return -EINVAL;
> 
> 	frame->sp = fp + 0x10;
> 	frame->fp = *(unsigned long *)(fp);
> 	/*
> +	 * check whether we are going to walk trough from interrupt stack
> +	 * to process stack
> +	 * If the previous frame is the initial (dummy) stack frame on
> +	 * interrupt stack, frame->sp now points to just below the frame
> +	 * (dummy frame + 0x10).
> +	 * See entry.S
> +	 */
> +#define STACK_LOW(addr) round_down((addr), THREAD_SIZE)
> +	if ((STACK_LOW(frame->sp) != STACK_LOW(frame->fp)) &&
> +			(frame->fp == *(unsigned long *)frame->sp))
> +		frame->sp = *((unsigned long *)(frame->sp + 8));
> +	/*
> 	 * -4 here because we care about the PC at time of bl,
> 	 * not where the return will go.
> 	 */
> -- 
> 1.7.9.5

How about folding the following hunk into this patch?
The comment would be helpful for people to follow this code.

----8<----

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index f1303c5..0ff7db3 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -122,7 +122,8 @@
         * x21 - aborted SP
         * x22 - aborted PC
         * x23 - aborted PSTATE
-       */
+        * x29 - aborted FP
+        */
        .endm
 
        .macro  kernel_exit, el

----8<----

Best Regards
Jungseok Lee

[PATCH v2 1/2] arm64: revamp unwind_frame for interrupt stack

[AKASHI Takahiro]

On 10/20/2015 10:26 PM, Jungseok Lee wrote:
> On Oct 20, 2015, at 5:00 PM, AKASHI Takahiro wrote:
>> This patch allows unwind_frame() to traverse from interrupt stack
>> to process stack correctly by having a dummy stack frame for irq
>> exception entry created at its prologue.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> arch/arm64/kernel/entry.S      |   22 ++++++++++++++++++++--
>> arch/arm64/kernel/stacktrace.c |   14 +++++++++++++-
>> 2 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index c8e0bcf..779f807 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -186,8 +186,26 @@ alternative_endif
>> 	and	x23, x23, #~(IRQ_STACK_SIZE - 1)
>> 	cmp	x20, x23			// check irq re-enterance
>> 	mov	x19, sp
>> -	csel	x23, x19, x24, eq		// x24 = top of irq stack
>> -	mov	sp, x23
>> +	beq	1f
>> +	mov	sp, x24				// x24 = top of irq stack
>> +	stp	x29, x19, [sp, #-16]!		// for sanity check
>> +	stp	x29, x22, [sp, #-16]!		// dummy stack frame
>> +	mov	x29, sp
>> +1:
>> +	/*
>> +	 * Layout of interrupt stack after this macro is invoked:
>> +	 *
>> +	 *     |                |
>> +	 *-0x20+----------------+ <= dummy stack frame
>> +	 *     |      fp        |    : fp on process stack
>> +	 *-0x18+----------------+
>> +	 *     |      lr        |    : return address
>> +	 *-0x10+----------------+
>> +	 *     |    fp (copy)   |    : for sanity check
>> +	 * -0x8+----------------+
>> +	 *     |      sp        |    : sp on process stack
>> +	 *  0x0+----------------+
>> +	 */
>> 	.endm
>>
>> 	/*
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 407991b..03611a1 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -43,12 +43,24 @@ int notrace unwind_frame(struct stackframe *frame)
>> 	low  = frame->sp;
>> 	high = ALIGN(low, THREAD_SIZE);
>>
>> -	if (fp < low || fp > high - 0x18 || fp & 0xf)
>> +	if (fp < low || fp > high - 0x20 || fp & 0xf)
>> 		return -EINVAL;
>>
>> 	frame->sp = fp + 0x10;
>> 	frame->fp = *(unsigned long *)(fp);
>> 	/*
>> +	 * check whether we are going to walk trough from interrupt stack
>> +	 * to process stack
>> +	 * If the previous frame is the initial (dummy) stack frame on
>> +	 * interrupt stack, frame->sp now points to just below the frame
>> +	 * (dummy frame + 0x10).
>> +	 * See entry.S
>> +	 */
>> +#define STACK_LOW(addr) round_down((addr), THREAD_SIZE)
>> +	if ((STACK_LOW(frame->sp) != STACK_LOW(frame->fp)) &&
>> +			(frame->fp == *(unsigned long *)frame->sp))
>> +		frame->sp = *((unsigned long *)(frame->sp + 8));
>> +	/*
>> 	 * -4 here because we care about the PC at time of bl,
>> 	 * not where the return will go.
>> 	 */
>> --
>> 1.7.9.5
>
> How about folding the following hunk into this patch?
> The comment would be helpful for people to follow this code.

Thanks.
A frame pointer(x29) is a frame pointer, and I'm not sure that the comment is
very useful.
But it's much better than "fp on process stack" in my ascii-art.

-Takahiro AKASHI

> ----8<----
>
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index f1303c5..0ff7db3 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -122,7 +122,8 @@
>           * x21 - aborted SP
>           * x22 - aborted PC
>           * x23 - aborted PSTATE
> -       */
> +        * x29 - aborted FP
> +        */
>          .endm
>
>          .macro  kernel_exit, el
>
> ----8<----
>
> Best Regards
> Jungseok Lee
>

[PATCH v2 1/2] arm64: revamp unwind_frame for interrupt stack

[Jungseok Lee]

[Only Akashi and James]

On Oct 21, 2015, at 3:38 PM, AKASHI Takahiro wrote:

Hi Akashi and James,

Am I the only person who have experienced kernel panic with this series?
I have observed NULL pointer kernel panic with the following two ways.

 - $ sudo echo c > /proc/sysrq-trigger
 - perf record -e mem:[address of do_softirq]:x -ag -- sleep 2

The kernel panic is triggered when the last stack frame of swapper is unwound.
At that time, the value of fp, low, high is 0, 0, 0, respectively.

My tree includes "Synchronise dump_backtrace() with perf call chain" patch
which is queued into for-next/core.

Best Regards
Jungseok Lee

> On 10/20/2015 10:26 PM, Jungseok Lee wrote:
>> On Oct 20, 2015, at 5:00 PM, AKASHI Takahiro wrote:
>>> This patch allows unwind_frame() to traverse from interrupt stack
>>> to process stack correctly by having a dummy stack frame for irq
>>> exception entry created at its prologue.
>>> 
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>> arch/arm64/kernel/entry.S      |   22 ++++++++++++++++++++--
>>> arch/arm64/kernel/stacktrace.c |   14 +++++++++++++-
>>> 2 files changed, 33 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>> index c8e0bcf..779f807 100644
>>> --- a/arch/arm64/kernel/entry.S
>>> +++ b/arch/arm64/kernel/entry.S
>>> @@ -186,8 +186,26 @@ alternative_endif
>>> 	and	x23, x23, #~(IRQ_STACK_SIZE - 1)
>>> 	cmp	x20, x23			// check irq re-enterance
>>> 	mov	x19, sp
>>> -	csel	x23, x19, x24, eq		// x24 = top of irq stack
>>> -	mov	sp, x23
>>> +	beq	1f
>>> +	mov	sp, x24				// x24 = top of irq stack
>>> +	stp	x29, x19, [sp, #-16]!		// for sanity check
>>> +	stp	x29, x22, [sp, #-16]!		// dummy stack frame
>>> +	mov	x29, sp
>>> +1:
>>> +	/*
>>> +	 * Layout of interrupt stack after this macro is invoked:
>>> +	 *
>>> +	 *     |                |
>>> +	 *-0x20+----------------+ <= dummy stack frame
>>> +	 *     |      fp        |    : fp on process stack
>>> +	 *-0x18+----------------+
>>> +	 *     |      lr        |    : return address
>>> +	 *-0x10+----------------+
>>> +	 *     |    fp (copy)   |    : for sanity check
>>> +	 * -0x8+----------------+
>>> +	 *     |      sp        |    : sp on process stack
>>> +	 *  0x0+----------------+
>>> +	 */
>>> 	.endm
>>> 
>>> 	/*
>>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>>> index 407991b..03611a1 100644
>>> --- a/arch/arm64/kernel/stacktrace.c
>>> +++ b/arch/arm64/kernel/stacktrace.c
>>> @@ -43,12 +43,24 @@ int notrace unwind_frame(struct stackframe *frame)
>>> 	low  = frame->sp;
>>> 	high = ALIGN(low, THREAD_SIZE);
>>> 
>>> -	if (fp < low || fp > high - 0x18 || fp & 0xf)
>>> +	if (fp < low || fp > high - 0x20 || fp & 0xf)
>>> 		return -EINVAL;
>>> 
>>> 	frame->sp = fp + 0x10;
>>> 	frame->fp = *(unsigned long *)(fp);
>>> 	/*
>>> +	 * check whether we are going to walk trough from interrupt stack
>>> +	 * to process stack
>>> +	 * If the previous frame is the initial (dummy) stack frame on
>>> +	 * interrupt stack, frame->sp now points to just below the frame
>>> +	 * (dummy frame + 0x10).
>>> +	 * See entry.S
>>> +	 */
>>> +#define STACK_LOW(addr) round_down((addr), THREAD_SIZE)
>>> +	if ((STACK_LOW(frame->sp) != STACK_LOW(frame->fp)) &&
>>> +			(frame->fp == *(unsigned long *)frame->sp))
>>> +		frame->sp = *((unsigned long *)(frame->sp + 8));
>>> +	/*
>>> 	 * -4 here because we care about the PC at time of bl,
>>> 	 * not where the return will go.
>>> 	 */
>>> --
>>> 1.7.9.5
>> 
>> How about folding the following hunk into this patch?
>> The comment would be helpful for people to follow this code.
> 
> Thanks.
> A frame pointer(x29) is a frame pointer, and I'm not sure that the comment is
> very useful.
> But it's much better than "fp on process stack" in my ascii-art.
> 
> -Takahiro AKASHI
> 
>> ----8<----
>> 
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index f1303c5..0ff7db3 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -122,7 +122,8 @@
>>          * x21 - aborted SP
>>          * x22 - aborted PC
>>          * x23 - aborted PSTATE
>> -       */
>> +        * x29 - aborted FP
>> +        */
>>         .endm
>> 
>>         .macro  kernel_exit, el
>> 
>> ----8<----
>> 
>> Best Regards
>> Jungseok Lee
>> 

[PATCH v2 1/2] arm64: revamp unwind_frame for interrupt stack

[Jungseok Lee]

On Oct 21, 2015, at 9:14 PM, Jungseok Lee wrote:

Whoops!

> [Only Akashi and James]
> 
> On Oct 21, 2015, at 3:38 PM, AKASHI Takahiro wrote:
> 
> Hi Akashi and James,
> 
> Am I the only person who have experienced kernel panic with this series?
> I have observed NULL pointer kernel panic with the following two ways.
> 
> - $ sudo echo c > /proc/sysrq-trigger
> - perf record -e mem:[address of do_softirq]:x -ag -- sleep 2
> 
> The kernel panic is triggered when the last stack frame of swapper is unwound.
> At that time, the value of fp, low, high is 0, 0, 0, respectively.
> 
> My tree includes "Synchronise dump_backtrace() with perf call chain" patch
> which is queued into for-next/core.
> 
> Best Regards
> Jungseok Lee

Really sorry for the noise.. It won't happen again..

Best Regards
Jungseok Lee

[PATCH v2 2/2] arm64: fix dump_backtrace() to show correct pt_regs at interrupt

[AKASHI Takahiro]

Adding an extra dummy stack frame for interrupt has one side-effect that
dump_backtrace() shows inccorect data of struct pt_regs at
"Exception stack ..." because we are still on an interrupt stack when
dump_backtrace() encounters an interrupt handler's frame.

This patch reuses __in_irqentry_text() macro, which was added by
  commit 9a5ad7d0e3e1 ("arm64: Add __exception_irq_entry definition for
                        function graph"),
and allows dump_backtrace() to recognize an interrupt handler's
frame and fetch a correct pointer (sp) to struct pt_regs on an process
stack.

One of drawbacks in this approach is that we will sometimes see
__irqentry_text_start and gic_handle_irq interchangeably, especially,
when "perf report --call-graph" shows stack traces because both symbols
has the same address.
(Please note that this can happen even without this patch if
CONFIG_FUNCTION_GRAPH_TRACER is enabled.)

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/exception.h |    7 +++----
 arch/arm64/include/asm/traps.h     |    7 -------
 arch/arm64/kernel/traps.c          |    9 +++++++--
 arch/arm64/kernel/vmlinux.lds.S    |    7 +++++++
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index 6cb7e1a..8415005 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -21,10 +21,9 @@
 #include <linux/ftrace.h>
 
 #define __exception	__attribute__((section(".exception.text")))
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+/* We always need this definition for dump_backtrace */
+#undef __irq_entry
+#define __irq_entry	__attribute__((__section__(".irqentry.text")))
 #define __exception_irq_entry	__irq_entry
-#else
-#define __exception_irq_entry	__exception
-#endif
 
 #endif	/* __ASM_EXCEPTION_H */
diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
index 0cc2f29..8f05d3b 100644
--- a/arch/arm64/include/asm/traps.h
+++ b/arch/arm64/include/asm/traps.h
@@ -34,7 +34,6 @@ struct undef_hook {
 void register_undef_hook(struct undef_hook *hook);
 void unregister_undef_hook(struct undef_hook *hook);
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
 static inline int __in_irqentry_text(unsigned long ptr)
 {
 	extern char __irqentry_text_start[];
@@ -43,12 +42,6 @@ static inline int __in_irqentry_text(unsigned long ptr)
 	return ptr >= (unsigned long)&__irqentry_text_start &&
 	       ptr < (unsigned long)&__irqentry_text_end;
 }
-#else
-static inline int __in_irqentry_text(unsigned long ptr)
-{
-	return 0;
-}
-#endif
 
 static inline int in_exception_text(unsigned long ptr)
 {
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index e9b9b53..8fc3d4b 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -179,10 +179,15 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 		ret = unwind_frame(&frame);
 		if (ret < 0)
 			break;
-		stack = frame.sp;
-		if (in_exception_text(where))
+		if (__in_irqentry_text(where)) {
+			stack = *(unsigned long *)(frame.fp + 0x18);
+			dump_mem("", "Interrupt stack", stack,
+				 stack + sizeof(struct pt_regs), false);
+		} else if (in_exception_text(where)) {
+			stack = frame.sp;
 			dump_mem("", "Exception stack", stack,
 				 stack + sizeof(struct pt_regs), false);
+		}
 	}
 }
 
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 9807333..0d16d02 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -44,6 +44,13 @@ jiffies = jiffies_64;
 	*(.idmap.text)					\
 	VMLINUX_SYMBOL(__idmap_text_end) = .;
 
+#undef IRQENTRY_TEXT
+#define IRQENTRY_TEXT						\
+		ALIGN_FUNCTION();				\
+		VMLINUX_SYMBOL(__irqentry_text_start) = .;	\
+		*(.irqentry.text)				\
+		VMLINUX_SYMBOL(__irqentry_text_end) = .;
+
 /*
  * The size of the PE/COFF section that covers the kernel image, which
  * runs from stext to _edata, must be a round multiple of the PE/COFF
-- 
1.7.9.5

[PATCH v2 2/2] arm64: fix dump_backtrace() to show correct pt_regs at interrupt

[Jungseok Lee]

On Oct 20, 2015, at 5:00 PM, AKASHI Takahiro wrote:

Hi Akashi,

> Adding an extra dummy stack frame for interrupt has one side-effect that
> dump_backtrace() shows inccorect data of struct pt_regs at
> "Exception stack ..." because we are still on an interrupt stack when
> dump_backtrace() encounters an interrupt handler's frame.
> 
> This patch reuses __in_irqentry_text() macro, which was added by
>  commit 9a5ad7d0e3e1 ("arm64: Add __exception_irq_entry definition for
>                        function graph"),
> and allows dump_backtrace() to recognize an interrupt handler's
> frame and fetch a correct pointer (sp) to struct pt_regs on an process
> stack.

A concern is the way __irq_entry and IRQENTRY_TEXT are implemented. As
you know well, they are bound to only function graph tracer. IMHO, it
would be better to factor them out generically and then utilize them
in arch and ftrace side. However, other hackers, ARM64 maintainers
or Arnd Bergmann, would leave more valuable comments than me ;)

Other than that, the approach is straightforwrd.

> One of drawbacks in this approach is that we will sometimes see
> __irqentry_text_start and gic_handle_irq interchangeably, especially,
> when "perf report --call-graph" shows stack traces because both symbols
> has the same address.
> (Please note that this can happen even without this patch if
> CONFIG_FUNCTION_GRAPH_TRACER is enabled.)
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/exception.h |    7 +++----
> arch/arm64/include/asm/traps.h     |    7 -------
> arch/arm64/kernel/traps.c          |    9 +++++++--
> arch/arm64/kernel/vmlinux.lds.S    |    7 +++++++
> 4 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index 6cb7e1a..8415005 100644
> --- a/arch/arm64/include/asm/exception.h
> +++ b/arch/arm64/include/asm/exception.h
> @@ -21,10 +21,9 @@
> #include <linux/ftrace.h>

This can be dropped.

> #define __exception	__attribute__((section(".exception.text")))
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +/* We always need this definition for dump_backtrace */
> +#undef __irq_entry
> +#define __irq_entry	__attribute__((__section__(".irqentry.text")))
> #define __exception_irq_entry	__irq_entry
> -#else
> -#define __exception_irq_entry	__exception
> -#endif
> 
> #endif	/* __ASM_EXCEPTION_H */
> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
> index 0cc2f29..8f05d3b 100644
> --- a/arch/arm64/include/asm/traps.h
> +++ b/arch/arm64/include/asm/traps.h
> @@ -34,7 +34,6 @@ struct undef_hook {
> void register_undef_hook(struct undef_hook *hook);
> void unregister_undef_hook(struct undef_hook *hook);
> 
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> static inline int __in_irqentry_text(unsigned long ptr)
> {
> 	extern char __irqentry_text_start[];
> @@ -43,12 +42,6 @@ static inline int __in_irqentry_text(unsigned long ptr)
> 	return ptr >= (unsigned long)&__irqentry_text_start &&
> 	       ptr < (unsigned long)&__irqentry_text_end;
> }
> -#else
> -static inline int __in_irqentry_text(unsigned long ptr)
> -{
> -	return 0;
> -}
> -#endif
> 
> static inline int in_exception_text(unsigned long ptr)
> {
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index e9b9b53..8fc3d4b 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -179,10 +179,15 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> 		ret = unwind_frame(&frame);
> 		if (ret < 0)
> 			break;
> -		stack = frame.sp;
> -		if (in_exception_text(where))
> +		if (__in_irqentry_text(where)) {
> +			stack = *(unsigned long *)(frame.fp + 0x18);
> +			dump_mem("", "Interrupt stack", stack,
> +				 stack + sizeof(struct pt_regs), false);

According to entry.S in case of \el == 1, the stack, might look as follows.

    ----------- <- High address (x21)
    |         |
    |         |
    | pt_regs |
    |         |
    |         |
    ----------- <- Low address

So, I think 'bottom' is stack-sizeof(struct pt_regs) and 'top' is stack.
Am I missing here?

Best Regards
Jungseok Lee