Re: [PATCH v2 4/6] unwind: Implement generic sframe unwinder library

[Indu Bhagat <indu.bhagat@oracle.com>]

On 9/9/25 9:44 AM, Puranjay Mohan wrote:
> Dylan Hatch <dylanbhatch@google.com> writes:
> 
>> From: Weinan Liu <wnliu@google.com>
>>
>> This change introduces a kernel space unwinder using sframe table for
>> architectures without ORC unwinder support.
>>
>> The implementation is adapted from Josh's userspace sframe unwinder
>> proposal[1] according to the sframe v2 spec[2].
>>
>> [1] https://lore.kernel.org/lkml/42c0a99236af65c09c8182e260af7bcf5aa1e158.1730150953.git.jpoimboe@kernel.org/
>> [2] https://sourceware.org/binutils/docs/sframe-spec.html
>>
>> Signed-off-by: Weinan Liu <wnliu@google.com>
>> Signed-off-by: Dylan Hatch <dylanbhatch@google.com>
>> Reviewed-by: Prasanna Kumar T S M <ptsm@linux.microsoft.com>
>> ---
>>   include/linux/sframe_lookup.h |  43 ++++++++
>>   kernel/Makefile               |   1 +
>>   kernel/sframe_lookup.c        | 196 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 240 insertions(+)
>>   create mode 100644 include/linux/sframe_lookup.h
>>   create mode 100644 kernel/sframe_lookup.c
>>
>> diff --git a/include/linux/sframe_lookup.h b/include/linux/sframe_lookup.h
>> new file mode 100644
>> index 000000000000..1c26cf1f38d4
>> --- /dev/null
>> +++ b/include/linux/sframe_lookup.h
>> @@ -0,0 +1,43 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _LINUX_SFRAME_LOOKUP_H
>> +#define _LINUX_SFRAME_LOOKUP_H
>> +
>> +/**
>> + * struct sframe_ip_entry - sframe unwind info for given ip
>> + * @cfa_offset: Offset for the Canonical Frame Address(CFA) from Frame
>> + *              Pointer(FP) or Stack Pointer(SP)
>> + * @ra_offset: Offset for the Return Address from CFA.
>> + * @fp_offset: Offset for the Frame Pointer (FP) from CFA.
>> + * @use_fp: Use FP to get next CFA or not
>> + */
>> +struct sframe_ip_entry {
>> +	int32_t cfa_offset;
>> +	int32_t ra_offset;
>> +	int32_t fp_offset;
>> +	bool use_fp;
>> +};
>> +
>> +/**
>> + * struct sframe_table - sframe struct of a table
>> + * @sfhdr_p: Pointer to sframe header
>> + * @fde_p: Pointer to the first of sframe frame description entry(FDE).
>> + * @fre_p: Pointer to the first of sframe frame row entry(FRE).
>> + */
>> +struct sframe_table {
>> +	struct sframe_header *sfhdr_p;
>> +	struct sframe_fde *fde_p;
>> +	char *fre_p;
>> +};
>> +
>> +#ifdef CONFIG_SFRAME_UNWINDER
>> +void init_sframe_table(void);
>> +int sframe_find_pc(unsigned long pc, struct sframe_ip_entry *entry);
>> +#else
>> +static inline void init_sframe_table(void) {}
>> +static inline int sframe_find_pc(unsigned long pc, struct sframe_ip_entry *entry)
>> +{
>> +	return -EINVAL;
>> +}
>> +#endif
>> +
>> +#endif /* _LINUX_SFRAME_LOOKUP_H */
>> diff --git a/kernel/Makefile b/kernel/Makefile
>> index c60623448235..17e9cfe09dc0 100644
>> --- a/kernel/Makefile
>> +++ b/kernel/Makefile
>> @@ -138,6 +138,7 @@ obj-$(CONFIG_WATCH_QUEUE) += watch_queue.o
>>   
>>   obj-$(CONFIG_RESOURCE_KUNIT_TEST) += resource_kunit.o
>>   obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o
>> +obj-$(CONFIG_SFRAME_UNWINDER) += sframe_lookup.o
>>   
>>   CFLAGS_kstack_erase.o += $(DISABLE_KSTACK_ERASE)
>>   CFLAGS_kstack_erase.o += $(call cc-option,-mgeneral-regs-only)
>> diff --git a/kernel/sframe_lookup.c b/kernel/sframe_lookup.c
>> new file mode 100644
>> index 000000000000..51cd24a75956
>> --- /dev/null
>> +++ b/kernel/sframe_lookup.c
>> @@ -0,0 +1,196 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +#define pr_fmt(fmt)	"sframe: " fmt
>> +
>> +#include <linux/module.h>
>> +#include <linux/sort.h>
>> +#include <linux/sframe_lookup.h>
>> +#include <linux/kallsyms.h>
>> +#include "sframe.h"
>> +
>> +extern char __start_sframe_header[];
>> +extern char __stop_sframe_header[];
>> +
>> +static bool sframe_init __ro_after_init;
>> +static struct sframe_table sftbl;
>> +
>> +#define SFRAME_READ_TYPE(in, out, type)					\
>> +({									\
>> +	type __tmp;							\
>> +	memcpy(&__tmp, in, sizeof(__tmp));				\
>> +	in += sizeof(__tmp);						\
>> +	out = __tmp;							\
>> +})
>> +
>> +#define SFRAME_READ_ROW_ADDR(in_addr, out_addr, type)			\
>> +({									\
>> +	switch (type) {							\
>> +	case SFRAME_FRE_TYPE_ADDR1:					\
>> +		SFRAME_READ_TYPE(in_addr, out_addr, u8);		\
>> +		break;							\
>> +	case SFRAME_FRE_TYPE_ADDR2:					\
>> +		SFRAME_READ_TYPE(in_addr, out_addr, u16);		\
>> +		break;							\
>> +	case SFRAME_FRE_TYPE_ADDR4:					\
>> +		SFRAME_READ_TYPE(in_addr, out_addr, u32);		\
>> +		break;							\
>> +	default:							\
>> +		break;							\
>> +	}								\
>> +})
>> +
>> +#define SFRAME_READ_ROW_OFFSETS(in_addr, out_addr, size)		\
>> +({									\
>> +	switch (size) {							\
>> +	case 1:								\
>> +		SFRAME_READ_TYPE(in_addr, out_addr, s8);		\
>> +		break;							\
>> +	case 2:								\
>> +		SFRAME_READ_TYPE(in_addr, out_addr, s16);		\
>> +		break;							\
>> +	case 4:								\
>> +		SFRAME_READ_TYPE(in_addr, out_addr, s32);		\
>> +		break;							\
>> +	default:							\
>> +		break;							\
>> +	}								\
>> +})
>> +
>> +static struct sframe_fde *find_fde(const struct sframe_table *tbl, unsigned long pc)
>> +{
>> +	int l, r, m, f;
>> +	int32_t ip;
>> +	struct sframe_fde *fdep;
>> +
>> +	if (!tbl || !tbl->sfhdr_p || !tbl->fde_p)
>> +		return NULL;
>> +
>> +	ip = (pc - (unsigned long)tbl->sfhdr_p);
>> +
>> +	/* Do a binary range search to find the rightmost FDE start_addr < ip */
>> +	l = m = f = 0;
>> +	r = tbl->sfhdr_p->num_fdes;
>> +	while (l < r) {
>> +		m = l + ((r - l) / 2);
>> +		fdep = tbl->fde_p + m;
>> +		if (fdep->start_addr > ip)
>> +			r = m;
>> +		else
>> +			l = m + 1;
>> +	}
> 
> The above logic doesn't correctly work for the new scheme with
> SFRAME_F_FDE_FUNC_START_PCREL, see [1]
> 
> If SFRAME_F_FDE_FUNC_START_PCREL is set in flags then function start
> address in SFrame FDE is encoded as the distance from the location of
> the sfde_func_start_address to the start PC of the function.
> 
> And for modules, sframes will only work if compiled with [1] with
> SFRAME_F_FDE_FUNC_START_PCREL flag set as ET_DYN, ET_EXEC, and ET_REL
> (relocatable links) generated by ld have sfde_func_start_address as
> offset from field itself. see [2] for more details.
> 

Yes.

The SFrame reader patches need to be refreshed with changes to do the 
right thing when SFRAME_F_FDE_FUNC_START_PCREL flag is set.

Jens had posted a patch sometime ago (patch to update the SFrame reader 
routines to work with Binutils 2.45) to serve as a starting point.

+CC: Jens Remus

> So, for the in kernel sframe unwinder that should support both normal
> links (kernel) and relocatable links (modules), we need to reject the
> sframe section if this flag is not set in init_sframe_table() and in
> sframe_module_init().
> 
> Then we can fix find_fde() like:
> 
> use pc in place of ip directly.
> 
> and the check will become
> 
> if (fdep->start_addr > (s32)(pc - fdep))
> 
> I hope I am not missing something,
> 
> Indu,
> Do you agree with my comments above?
> 
> Thanks,
> Puranjay
> 
> [1] https://sourceware.org/pipermail/binutils/2025-July/142222.html
> [2] https://sourceware.org/bugzilla/show_bug.cgi?id=32666
>