Re: [PATCH 5/8] unwind: arm64: Add sframe unwinder on arm64

Prasanna Kumar T S M
2025-01-30T10:35:03Z

On 28-01-2025 03:03, Weinan Liu wrote:
> Add unwind_next_frame_sframe() function to unwind by sframe info.
> Built with GNU Binutils 2.42 to verify that this sframe unwinder can
> backtrace correctly on arm64.
>
> Signed-off-by: Weinan Liu <wnliu@google.com>
> ---
> arch/arm64/include/asm/stacktrace/common.h | 4 ++
> arch/arm64/kernel/setup.c | 2 +
> arch/arm64/kernel/stacktrace.c | 59 ++++++++++++++++++++++
> 3 files changed, 65 insertions(+)
>
> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> index 821a8fdd31af..19edae8a5b1a 100644
> --- a/arch/arm64/include/asm/stacktrace/common.h
> +++ b/arch/arm64/include/asm/stacktrace/common.h
> @@ -25,6 +25,7 @@ struct stack_info {
> * @stack: The stack currently being unwound.
> * @stacks: An array of stacks which can be unwound.
> * @nr_stacks: The number of stacks in @stacks.
> + * @cfa: The sp value at the call site of the current function.
> */
> struct unwind_state {
> unsigned long fp;
> @@ -33,6 +34,9 @@ struct unwind_state {
> struct stack_info stack;
> struct stack_info *stacks;
> int nr_stacks;
> +#ifdef CONFIG_SFRAME_UNWINDER
> + unsigned long cfa;
> +#endif
> };
>
> static inline struct stack_info stackinfo_get_unknown(void)
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 4f613e8e0745..d3ac92b624f3 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -32,6 +32,7 @@
> #include <linux/sched/task.h>
> #include <linux/scs.h>
> #include <linux/mm.h>
> +#include <linux/sframe_lookup.h>
>
> #include <asm/acpi.h>
> #include <asm/fixmap.h>
> @@ -377,6 +378,7 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
> "This indicates a broken bootloader or old kernel\n",
> boot_args[1], boot_args[2], boot_args[3]);
> }
> + init_sframe_table();
> }
>
> static inline bool cpu_can_disable(unsigned int cpu)
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 1d9d51d7627f..c035adb8fe8a 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -14,6 +14,7 @@
> #include <linux/sched/debug.h>
> #include <linux/sched/task_stack.h>
> #include <linux/stacktrace.h>
> +#include <linux/sframe_lookup.h>
>
> #include <asm/efi.h>
> #include <asm/irq.h>
> @@ -242,6 +243,53 @@ kunwind_next_frame_record(struct kunwind_state *state)
> return 0;
> }
>
> +#ifdef CONFIG_SFRAME_UNWINDER
> +/*
> + * Unwind to the next frame according to sframe.
> + */
> +static __always_inline int
> +unwind_next_frame_sframe(struct unwind_state *state)
> +{
> + unsigned long fp = state->fp, ip = state->pc;
> + unsigned long base_reg, cfa;
> + unsigned long pc_addr, fp_addr;
> + struct sframe_ip_entry entry;
> + struct stack_info *info;
> + struct frame_record *record = (struct frame_record *)fp;
> +
> + int err;
> +
> + /* frame record alignment 8 bytes */
> + if (fp & 0x7)
> + return -EINVAL;
> +
> + info = unwind_find_stack(state, fp, sizeof(*record));
> + if (!info)
> + return -EINVAL;
> +
> + err = sframe_find_pc(ip, &entry);
> + if (err)
> + return -EINVAL;
> +
> + unwind_consume_stack(state, info, fp, sizeof(*record));
> +
> + base_reg = entry.use_fp ? fp : state->cfa;
> +
> + /* Set up the initial CFA using fp based info if CFA is not set */
> + if (!state->cfa)
> + cfa = fp - entry.fp_offset;
> + else
> + cfa = base_reg + entry.cfa_offset;
> + fp_addr = cfa + entry.fp_offset;
> + pc_addr = cfa + entry.ra_offset;
> + state->cfa = cfa;
> + state->fp = READ_ONCE(*(unsigned long *)(fp_addr));
> + state->pc = READ_ONCE(*(unsigned long *)(pc_addr));
> +
> + return 0;
> +}
> +#endif
> +
> /*
> * Unwind from one frame record (A) to the next frame record (B).
> *
> @@ -261,7 +309,15 @@ kunwind_next(struct kunwind_state *state)
> case KUNWIND_SOURCE_CALLER:
> case KUNWIND_SOURCE_TASK:
> case KUNWIND_SOURCE_REGS_PC:
> +#ifdef CONFIG_SFRAME_UNWINDER
> + err = unwind_next_frame_sframe(&state->common);
> +
> + /* Fallback to FP based unwinder */
> + if (err)
> err = kunwind_next_frame_record(state);
> +#else
> + err = kunwind_next_frame_record(state);
> +#endif
> break;
> default:
> err = -EINVAL;
> @@ -347,6 +403,9 @@ kunwind_stack_walk(kunwind_consume_fn consume_state,
> .common = {
> .stacks = stacks,
> .nr_stacks = ARRAY_SIZE(stacks),
> +#ifdef CONFIG_SFRAME_UNWINDER
> + .cfa = 0,
> +#endif
> },
> };
>

Looks good to me.
Reviewed-by: Prasanna Kumar T S M <ptsm@linux.microsoft.com>.