Re: [PATCH v3 2/8] arm64, unwind: build kernel with sframe V3 info

[Dylan Hatch]

On Tue, Apr 14, 2026 at 5:43 AM Jens Remus <jremus@xxxxxxxxxxxxx> 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)
> jremus@xxxxxxxxxx / jremus@xxxxxxxxxxxxx
>
> 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