2022-09-20 15:24:47

by Chen Zhongjin

[permalink] [raw]
Subject: [PATCH -next 3/7] riscv: stacktrace: Save pt_regs in encoded fp on irq entry

To support stack unwinding at irq entry, the position of pt_regs saved
on stack is nessesary. Because for some functions, compiler only push
s0/fp on stack without ra.

As the situation described in
commit f766f77a74f5("riscv/stacktrace: Fix stack output without ra on the stack top")

When irq happens there, the the function frame looks like:

prev function | ... |
| |
normal function +-----------------+
| ra to prev |
| s0 of prev |
| ... |<-+
leaf function +-----------------+ |
| s0 of normal | |
| empty slot | |
irq pt_regs +-----------------+ |
| epc (ra to leaf)| |
| ra (ra to norm)| |
| ... | |
| s0 of leaf |--+
| ... |
+-----------------+

If the position of register in pt_regs is {epc, s0}, we can easily
unwind from irq frame to leaf function, as normal functions do.

However when unwinding from unwinding from leaf to normal, beacause
(ra to norm) is saved in pt_regs, but not stackframe of leaf, we
have to get pt_regs for that.

To get pt_regs position on stack, we can save the encoded *pt_regs
in s0, as x86 architecture did. Then we can get s0, epc and ra
easily.

Signed-off-by: Chen Zhongjin <[email protected]>
---
arch/riscv/include/asm/frame.h | 45 ++++++++++++++++++++++++++++++++++
arch/riscv/kernel/entry.S | 3 +++
2 files changed, 48 insertions(+)
create mode 100644 arch/riscv/include/asm/frame.h

diff --git a/arch/riscv/include/asm/frame.h b/arch/riscv/include/asm/frame.h
new file mode 100644
index 000000000000..2a1f45cf3a4e
--- /dev/null
+++ b/arch/riscv/include/asm/frame.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_RISCV_FRAME_H
+#define _ASM_RISCV_FRAME_H
+
+#include <asm/asm.h>
+
+#ifdef CONFIG_FRAME_POINTER
+
+#ifdef __ASSEMBLY__
+
+/*
+ * This is a sneaky trick to help the unwinder find pt_regs on the stack. The
+ * frame pointer is replaced with an encoded pointer to pt_regs. The encoding
+ * is just setting the LSB, which makes it an invalid stack address and is also
+ * a signal to the unwinder that it's a pt_regs pointer in disguise.
+ *
+ * This macro must be used when sp point to pt_regs
+ */
+.macro ENCODE_FRAME_POINTER
+ add s0, sp, 0x1
+.endm
+
+#else /* !__ASSEMBLY__ */
+
+#define ENCODE_FRAME_POINTER \
+ "add s0, sp, 0x1\n\t"
+
+#endif /* __ASSEMBLY__ */
+
+#else /* !CONFIG_FRAME_POINTER */
+
+#ifdef __ASSEMBLY__
+
+.macro ENCODE_FRAME_POINTER ptregs_offset=0
+.endm
+
+#else /* !__ASSEMBLY */
+
+#define ENCODE_FRAME_POINTER
+
+#endif /* !__ASSEMBLY */
+
+#endif /* CONFIG_FRAME_POINTER */
+
+#endif /* _ASM_RISCV_FRAME_H */
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index b9eda3fcbd6d..ecb15c7430b4 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -13,6 +13,7 @@
#include <asm/thread_info.h>
#include <asm/asm-offsets.h>
#include <asm/errata_list.h>
+#include <asm/frame.h>

#if !IS_ENABLED(CONFIG_PREEMPTION)
.set resume_kernel, restore_all
@@ -95,6 +96,8 @@ _save_context:
REG_S s4, PT_CAUSE(sp)
REG_S s5, PT_TP(sp)

+ ENCODE_FRAME_POINTER
+
/*
* Set the scratch register to 0, so that if a recursive exception
* occurs, the exception vector knows it came from the kernel
--
2.17.1


2022-09-21 02:32:25

by Guo Ren

[permalink] [raw]
Subject: Re: [PATCH -next 3/7] riscv: stacktrace: Save pt_regs in encoded fp on irq entry

Reviewed-by: Guo Ren <[email protected]>

ENCODE fp with BIT[0] is okay.

On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <[email protected]> wrote:
>
> To support stack unwinding at irq entry, the position of pt_regs saved
> on stack is nessesary. Because for some functions, compiler only push
> s0/fp on stack without ra.
>
> As the situation described in
> commit f766f77a74f5("riscv/stacktrace: Fix stack output without ra on the stack top")
>
> When irq happens there, the the function frame looks like:
>
> prev function | ... |
> | |
> normal function +-----------------+
> | ra to prev |
> | s0 of prev |
> | ... |<-+
> leaf function +-----------------+ |
> | s0 of normal | |
> | empty slot | |
> irq pt_regs +-----------------+ |
> | epc (ra to leaf)| |
> | ra (ra to norm)| |
> | ... | |
> | s0 of leaf |--+
> | ... |
> +-----------------+
>
> If the position of register in pt_regs is {epc, s0}, we can easily
> unwind from irq frame to leaf function, as normal functions do.
>
> However when unwinding from unwinding from leaf to normal, beacause
> (ra to norm) is saved in pt_regs, but not stackframe of leaf, we
> have to get pt_regs for that.
>
> To get pt_regs position on stack, we can save the encoded *pt_regs
> in s0, as x86 architecture did. Then we can get s0, epc and ra
> easily.
>
> Signed-off-by: Chen Zhongjin <[email protected]>
> ---
> arch/riscv/include/asm/frame.h | 45 ++++++++++++++++++++++++++++++++++
> arch/riscv/kernel/entry.S | 3 +++
> 2 files changed, 48 insertions(+)
> create mode 100644 arch/riscv/include/asm/frame.h
>
> diff --git a/arch/riscv/include/asm/frame.h b/arch/riscv/include/asm/frame.h
> new file mode 100644
> index 000000000000..2a1f45cf3a4e
> --- /dev/null
> +++ b/arch/riscv/include/asm/frame.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_RISCV_FRAME_H
> +#define _ASM_RISCV_FRAME_H
> +
> +#include <asm/asm.h>
> +
> +#ifdef CONFIG_FRAME_POINTER
> +
> +#ifdef __ASSEMBLY__
> +
> +/*
> + * This is a sneaky trick to help the unwinder find pt_regs on the stack. The
> + * frame pointer is replaced with an encoded pointer to pt_regs. The encoding
> + * is just setting the LSB, which makes it an invalid stack address and is also
> + * a signal to the unwinder that it's a pt_regs pointer in disguise.
> + *
> + * This macro must be used when sp point to pt_regs
> + */
> +.macro ENCODE_FRAME_POINTER
> + add s0, sp, 0x1
> +.endm
> +
> +#else /* !__ASSEMBLY__ */
> +
> +#define ENCODE_FRAME_POINTER \
> + "add s0, sp, 0x1\n\t"
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#else /* !CONFIG_FRAME_POINTER */
> +
> +#ifdef __ASSEMBLY__
> +
> +.macro ENCODE_FRAME_POINTER ptregs_offset=0
> +.endm
> +
> +#else /* !__ASSEMBLY */
> +
> +#define ENCODE_FRAME_POINTER
> +
> +#endif /* !__ASSEMBLY */
> +
> +#endif /* CONFIG_FRAME_POINTER */
> +
> +#endif /* _ASM_RISCV_FRAME_H */
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index b9eda3fcbd6d..ecb15c7430b4 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -13,6 +13,7 @@
> #include <asm/thread_info.h>
> #include <asm/asm-offsets.h>
> #include <asm/errata_list.h>
> +#include <asm/frame.h>
>
> #if !IS_ENABLED(CONFIG_PREEMPTION)
> .set resume_kernel, restore_all
> @@ -95,6 +96,8 @@ _save_context:
> REG_S s4, PT_CAUSE(sp)
> REG_S s5, PT_TP(sp)
>
> + ENCODE_FRAME_POINTER
> +
> /*
> * Set the scratch register to 0, so that if a recursive exception
> * occurs, the exception vector knows it came from the kernel
> --
> 2.17.1
>


--
Best Regards
Guo Ren