On Tue, Apr 14, 2026 at 5:43 AM Jens Remus <[email protected]> wrote:
>
>
> Good catch! Should we rather add the following in the series you are
> basing on, as there are already arch-specific unwind_user.h and
> unwind_user_sframe.h?
>
> F: arch/*/include/asm/unwind*.h
Thanks for the suggestion, this works for me.
>
> On the other hand I wonder whether the arch-specific headers should
> remain maintained by the respective arch maintainers? How is that
> handled in general?
I had the same question. My scan of MAINTAINERS shows both patterns
are present, so I defer to those who know more about this kind of
maintainership configuration.
>
> > diff --git a/arch/Kconfig b/arch/Kconfig
>
> > @@ -520,6 +520,13 @@ config SFRAME_VALIDATION
> >
> > If unsure, say N.
> >
> > +config ARCH_SUPPORTS_SFRAME_UNWINDER
> > + bool
> > + help
> > + An architecture can select this if it enables the sframe (Simple
> > + Frame) unwinder for unwinding kernel stack traces. It uses unwind
> > + table that is directly generatedby toolchain based on DWARF CFI
> > information.
>
> Nit: s/sframe/SFrame/
>
> > +
> > config HAVE_PERF_REGS
> > bool
> > help
>
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>
> > @@ -112,6 +112,7 @@ config ARM64
> > select ARCH_SUPPORTS_SCHED_SMT
> > select ARCH_SUPPORTS_SCHED_CLUSTER
> > select ARCH_SUPPORTS_SCHED_MC
> > + select ARCH_SUPPORTS_SFRAME_UNWINDER
> > select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
> > select ARCH_WANT_COMPAT_IPC_PARSE_VERSION if COMPAT
> > select ARCH_WANT_DEFAULT_BPF_JIT
>
> > diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>
> > @@ -20,4 +20,17 @@ config ARM64_RELOC_TEST
> > depends on m
> > tristate "Relocation testing module"
> >
> > +config SFRAME_UNWINDER
>
> Why do you introduce this for arm64 (and debug) only? If s390 were to
> add support (as replacement for s390 backchain), this would have to be
> moved or duplicated. It would not suffice to enable
> ARCH_SUPPORTS_SFRAME_UNWINDER to also enable SFRAME_UNWINDER.
Makes sense, I'll look into introducing this as arch-generic.
>
> As mentioned in my feedback on the previous patch in this series:
> Would it make sense to align the naming to the existing
> HAVE_UNWIND_USER_SFRAME, for instance:
>
> HAVE_UNWIND_KERNEL_SFRAME
>
> > + bool "Sframe unwinder"
> > + depends on AS_SFRAME3
> > + depends on 64BIT
> > + depends on ARCH_SUPPORTS_SFRAME_UNWINDER
> > + select SFRAME_LOOKUP
> > + help
> > + This option enables the sframe (Simple Frame) unwinder for unwinding
> > + kernel stack traces. It uses unwind table that is directly generated
> > + by toolchain based on DWARF CFI information. In, practice this can
> > + provide more reliable stacktrace results than unwinding with frame
> > + pointers alone.
>
> Nit: s/sframe/SFrame/
>
> > +
> > source "drivers/hwtracing/coresight/Kconfig"
>
> You are introducing two new Kconfig options (SFRAME_UNWINDER and
> ARCH_SUPPORTS_SFRAME_UNWINDER). I wonder whether they could somehow be
> combined into a single new option. Although I am not sure how an option
> can be both selectable and depending at the same time, so that the ARM64
> config could select it, but it would also depend on the above.
I don't think this is recommended, since the behavior of 'select'
appears to override a 'depends' requirement.
>From Documentation/kbuild/kconfig-language.rst: "select should be used
with care. select will force a symbol to a value without visiting the
dependencies. By abusing select you are able to select a symbol FOO
even if FOO depends on BAR that is not set. In general use select only
for non-visible symbols (no prompts anywhere) and for symbols with no
dependencies. That will limit the usefulness but on the other hand
avoid the illegal configurations all over."
>
> > diff --git a/include/asm-generic/vmlinux.lds.h
> > b/include/asm-generic/vmlinux.lds.h
>
> > @@ -491,6 +491,8 @@
> > *(.rodata1) \
> > } \
> > \
> > + SFRAME \
> > + \
> > /* PCI quirks */ \
> > .pci_fixup : AT(ADDR(.pci_fixup) - LOAD_OFFSET) { \
> > BOUNDED_SECTION_PRE_LABEL(.pci_fixup_early,
> > _pci_fixups_early, __start, __end) \
> > @@ -911,6 +913,19 @@
> > #define TRACEDATA
> > #endif
> >
> > +#ifdef CONFIG_SFRAME_UNWINDER
> > +#define SFRAME \
> > + /* sframe */ \
> > + .sframe : AT(ADDR(.sframe) - LOAD_OFFSET) { \
> > + __start_sframe_header = .; \
>
> __start_sframe[_section] = .;
>
> > + KEEP(*(.sframe)) \
> > + KEEP(*(.init.sframe)) \
> > + __stop_sframe_header = .; \
>
> __stop_sframe[_section] = .;
>
> Unless I am missing something both are not the start/stop of the .sframe
> header (in the .sframe section) but the .sframe section itself (see also
> your subsequent "[PATCH v3 4/8] sframe: Provide PC lookup for vmlinux
> .sframe section." where you assign both to kernel_sfsec.sframe_start
> and kernel_sfsec.sframe_end.
>
> > + }
> > +#else
> > +#define SFRAME
> > +#endif
> > +
> > #ifdef CONFIG_PRINTK_INDEX
> > #define PRINTK_INDEX \
> > .printk_index : AT(ADDR(.printk_index) - LOAD_OFFSET) { \
>
> Regards,
> Jens
> --
> Jens Remus
> Linux on Z Development (D3303)
> [email protected] / [email protected]
>
> IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats:
> Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft:
> Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
> IBM Data Privacy Statement: https://www.ibm.com/privacy/
>
I'm working on a new version that I'm hoping to be able to send
sometime next week, which should address your other comments.
Thanks,
Dylan
