core: arm32: handle aborts in system mode #1703

jenswi-linaro · 2017-07-18T14:55:21Z

Switch to handle aborts in system mode in order to be able to give a
stack trace in case an abort occurs in the abort handler.

In a manner similar to the AArch64 implementation are abort and undef
mode stack pointers pointing to the struct thread_core_local of
corresponding cpu.

Tested-by: Jens Wiklander jens.wiklander@linaro.org (Hikey)
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org

prime-zeng · 2017-07-19T01:41:29Z

Can this patch handle the abort which occurs in native interrupt handler(IRQ/FIQ MODE)?

jenswi-linaro · 2017-07-19T07:14:19Z

With regards to IRQ/FIQ mode it's the same behavior as today, that is, either the abort is reported as an unhandled abort or there's a panic in thread_get_id() depending on configuration (pager and vfp).

etienne-lms · 2017-08-11T11:35:45Z

I have tested this on my b2260 (ARMv7). I get the exact same backtrace with and without this change when generating an abort from the abort handler :( The initial abort backtrace is fine but the backtrace from the abort-in-abort is buggy (always shows twice the current function and the thread_core_local structure address).

Same status with initial abort being issued either from core service or from user TA execution.
Same status with and without pager.

This is not a very constructive comment but I did not manage to find where's the issue...

etienne-lms · 2017-08-11T14:59:26Z

core/arch/arm/kernel/thread_a32.S

+	 * r1 is initialized as a temporary stack pointer until we've
+	 * switched to system mode.
+	 */
+	tst	r1, #(THREAD_CLF_SAVED_SHIFT + THREAD_CLF_ABORT_SHIFT)


should be tst r1, #(THREAD_CLF_ABORT << THREAD_CLF_SAVED_SHIFT)

Thanks, I'll fix.

etienne-lms · 2017-08-11T15:01:37Z

core/arch/arm/kernel/thread_a32.S

+	 * switched to system mode.
+	 */
+	tst	r1, #(THREAD_CLF_SAVED_SHIFT + THREAD_CLF_ABORT_SHIFT)
+	orrne	r1, r1, #THREAD_CLF_TMP /* flags |= THREAD_CLF_TMP; */


THREAD_CLF_TMP seems not being used anywhere (but set here).

True, but this is the same for AArch64. This flag is kept to be able to tell which state we're executing in. AArch64 doesn't have as many mode bits as AArch32 in CPSR so it's mostly to compensate for that.

It's probably only useful for debug purposes, but I'd prefer to keep it reliable in either case for the time being.

jenswi-linaro · 2017-08-17T06:54:29Z

Please test again with the added fix.

etienne-lms · 2017-08-17T12:53:58Z

Stack info and link register value are still wrong in abort-in-abort case, because read from current exec mode context through read_mode_sp/lr(). Proposed change below gets the right sp and lr for traces and stack unwind. But i don't think it is ok for all execution cases (boot thread, pager, ...)

--- a/core/arch/arm/kernel/abort.c
+++ b/core/arch/arm/kernel/abort.c
@@ -101,8 +101,13 @@ static void __print_stack_unwind_arm32(struct abort_info *ai)
 	state.registers[10] = ai->regs->r10;
 	state.registers[11] = ai->regs->r11;
 
-	state.registers[13] = read_mode_sp(ai->regs->spsr & CPSR_MODE_MASK);
-	state.registers[14] = read_mode_lr(ai->regs->spsr & CPSR_MODE_MASK);
+	if (thread_is_from_abort_mode(NULL)) {
+		state.registers[13] = ai->regs->usr_sp;
+		state.registers[14] = ai->regs->usr_lr;
+	} else {
+		state.registers[13] = read_mode_sp(ai->regs->spsr & CPSR_MODE_MASK);
+		state.registers[14] = read_mode_lr(ai->regs->spsr & CPSR_MODE_MASK);
+	}
 	state.registers[15] = ai->pc;
 
 	EMSG_RAW("Call stack:");
@@ -245,9 +250,11 @@ static __maybe_unused void __print_abort_info(
 		 ai->regs->r0, ai->regs->r4, ai->regs->r8, ai->regs->ip);
 	EMSG_RAW(" r1 0x%08x      r5 0x%08x    r9 0x%08x    sp 0x%08x",
 		 ai->regs->r1, ai->regs->r5, ai->regs->r9,
+		 thread_is_from_abort_mode(NULL) ? ai->regs->usr_sp :
 		 read_mode_sp(ai->regs->spsr & CPSR_MODE_MASK));
 	EMSG_RAW(" r2 0x%08x      r6 0x%08x   r10 0x%08x    lr 0x%08x",
 		 ai->regs->r2, ai->regs->r6, ai->regs->r10,
+		 thread_is_from_abort_mode(NULL) ? ai->regs->usr_lr :
 		 read_mode_lr(ai->regs->spsr & CPSR_MODE_MASK));
 	EMSG_RAW(" r3 0x%08x      r7 0x%08x   r11 0x%08x    pc 0x%08x",
 		 ai->regs->r3, ai->regs->r7, ai->regs->r11, ai->pc);

jenswi-linaro · 2017-08-18T06:50:36Z

Thanks for the patch @etienne-lms, I ended up doing it slightly different. As far as I can see in QEMU it's OK for aborts in pager and in boot thread.

etienne-lms · 2017-08-18T08:22:31Z

Agree with the changes. Tested ok on b2260 on various abort conditions.

Note an build error in 64bit mode reported by travis.
entry of __print_abort_info must be conditioned to ARM32:

 static __maybe_unused void __print_abort_info(struct abort_info *ai,
 					      const char *ctx __maybe_unused)
 {
+#ifdef ARM32
 	uint32_t mode = ai->regs->spsr & CPSR_MODE_MASK;
 	__maybe_unused uint32_t sp;
 	__maybe_unused uint32_t lr;
 
 	if (mode == CPSR_MODE_USR || mode == CPSR_MODE_SYS) {
 		sp = ai->regs->usr_sp;
 		lr = ai->regs->usr_lr;
 	} else {
 		sp = read_mode_sp(mode);
 		lr = read_mode_lr(mode);
 	}
+#endif
 
 	EMSG_RAW("");

jenswi-linaro · 2017-08-18T08:32:30Z

Updated

etienne-lms · 2017-08-18T08:47:34Z

core/arch/arm/kernel/abort.c

-static __maybe_unused void __print_abort_info(
-				struct abort_info *ai __maybe_unused,
-				const char *ctx __maybe_unused)
+static __maybe_unused void __print_abort_info(struct abort_info *ai,


must preserve __maybe_unused for case ARM64 and LOG_LEVEL==0

etienne-lms · 2017-08-18T08:52:17Z

core/arch/arm/kernel/thread.c

@@ -644,14 +644,9 @@ size_t thread_stack_size(void)

 bool thread_is_from_abort_mode(struct thread_abort_regs __maybe_unused *regs)


As argument regs is useless now, maybe remove it within this P-R.

etienne-lms · 2017-08-18T08:56:16Z

core/arch/arm/kernel/thread_a32.S

+	 * cpsr is in mode undef or abort
+	 * sp is still pointing to struct thread_core_local belonging to
+	 * this core.
+	 * {r0-r3} are saved in struct thread_core_local pointed to by sp


s/{r0-r3}/{r0,r1}/

etienne-lms · 2017-08-18T08:56:38Z

core/arch/arm/kernel/thread_a32.S

+	 * sp is still pointing to struct thread_core_local belonging to
+	 * this core.
+	 * {r0-r3} are saved in struct thread_core_local pointed to by sp
+	 * {r4-r11, ip} are untouched.


s/r4-r11/r2-r11/

jenswi-linaro · 2017-08-18T10:01:47Z

Comments addressed

etienne-lms · 2017-08-18T11:18:42Z

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

Switch to handle aborts in system mode in order to be able to give a stack trace in case an abort occurs in the abort handler. In a manner similar to the AArch64 implementation are abort and undef mode stack pointers pointing to the struct thread_core_local of corresponding cpu. Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (QEMU) Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2017-08-18T11:34:47Z

Squashed, tags applied and rebased.

etienne-lms reviewed Aug 11, 2017

View reviewed changes

etienne-lms reviewed Aug 18, 2017

View reviewed changes

jenswi-linaro force-pushed the system_mode branch from 119145c to ccf8290 Compare August 18, 2017 11:34

jforissier merged commit 935ac3e into OP-TEE:master Aug 18, 2017

jenswi-linaro deleted the system_mode branch August 18, 2017 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: arm32: handle aborts in system mode #1703

core: arm32: handle aborts in system mode #1703

jenswi-linaro commented Jul 18, 2017

prime-zeng commented Jul 19, 2017 •

edited

Loading

jenswi-linaro commented Jul 19, 2017

etienne-lms commented Aug 11, 2017

etienne-lms Aug 11, 2017

jenswi-linaro Aug 17, 2017

etienne-lms Aug 11, 2017

jenswi-linaro Aug 17, 2017

etienne-lms Aug 17, 2017

jenswi-linaro commented Aug 17, 2017

etienne-lms commented Aug 17, 2017

jenswi-linaro commented Aug 18, 2017

etienne-lms commented Aug 18, 2017

jenswi-linaro commented Aug 18, 2017

etienne-lms Aug 18, 2017

etienne-lms Aug 18, 2017

etienne-lms Aug 18, 2017

etienne-lms Aug 18, 2017

jenswi-linaro commented Aug 18, 2017

etienne-lms commented Aug 18, 2017

jenswi-linaro commented Aug 18, 2017

		@@ -644,14 +644,9 @@ size_t thread_stack_size(void)

		bool thread_is_from_abort_mode(struct thread_abort_regs __maybe_unused *regs)

core: arm32: handle aborts in system mode #1703

core: arm32: handle aborts in system mode #1703

Conversation

jenswi-linaro commented Jul 18, 2017

prime-zeng commented Jul 19, 2017 • edited Loading

jenswi-linaro commented Jul 19, 2017

etienne-lms commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Aug 17, 2017

etienne-lms commented Aug 17, 2017

jenswi-linaro commented Aug 18, 2017

etienne-lms commented Aug 18, 2017

jenswi-linaro commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Aug 18, 2017

etienne-lms commented Aug 18, 2017

jenswi-linaro commented Aug 18, 2017

prime-zeng commented Jul 19, 2017 •

edited

Loading