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

[Prasanna Kumar T S M <ptsm@linux.microsoft.com>]

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>.