public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Jens Remus <jremus@linux.ibm.com>
Cc: Dylan Hatch <dylanbhatch@google.com>,
	Roman Gushchin <roman.gushchin@linux.dev>,
	Weinan Liu <wnliu@google.com>, Will Deacon <will@kernel.org>,
	Josh Poimboeuf <jpoimboe@kernel.org>,
	Indu Bhagat <ibhagatgnu@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Jiri Kosina <jikos@kernel.org>,
	Prasanna Kumar T S M <ptsm@linux.microsoft.com>,
	Puranjay Mohan <puranjay@kernel.org>, Song Liu <song@kernel.org>,
	joe.lawrence@redhat.com, linux-toolchains@vger.kernel.org,
	linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Randy Dunlap <rdunlap@infradead.org>,
	Heiko Carstens <hca@linux.ibm.com>
Subject: Re: [PATCH v5 8/8] unwind: arm64: Use sframe to unwind interrupt frames
Date: Tue, 5 May 2026 11:29:26 +0100	[thread overview]
Message-ID: <afnGhsCYEUG0LXR5@J2N7QTR9R3> (raw)
In-Reply-To: <bc3fb59b-9d80-4957-af51-20db38e3487e@linux.ibm.com>

On Mon, May 04, 2026 at 10:47:26AM +0200, Jens Remus wrote:
> Hello Mark,
> 
> I mostly have comments regarding your the SFrame related remarks.

Thanks for this; I have a few more questions and comments below.

> On 5/1/2026 6:46 PM, Mark Rutland wrote:
> > Thanks for putting this together. I think this is looking pretty good.
> > However, there are some things that aren't quite right and need some
> > work, which I've commented on below.
> 
> > (2) To make unwinding generally possible, we'll need to annotate some
> >     assembly functions as unwindable. We'll need to do that for string
> >     routines under lib/, and probably some crypto code, but we don't
> >     need to do that for most code in head.S, entry.S, etc.
> > 
> >     The vast majority of relevant assembly functions are leaf functions
> >     (where the return address is never moved out of the LR), so we can
> >     probably get away with a simple annotation for those that avoids the
> >     need for open-coded CFI directives everywhere.
> 
> Wrapping them in .cfi_startproc ... .cfi_endproc should do.  For instance
> by extending SYM_FUNC_START() and SYM_FUNC_END() or introducing flavors
> that do.  Or where you thinking of something else?

I was expecting we'd do something like that, either with distinct
versions, or some entirely separate annotation.

We can't override SYM_FUNC_START() or SYM_FUNC_END() since those are
also used for non-leaf functions. The bulk of the work is going to be
making sure we only annotate leaf functions specifically, which will
require some human analysis.

> > On Tue, Apr 28, 2026 at 06:36:43PM +0000, Dylan Hatch wrote:
> >> diff --git a/arch/arm64/include/asm/stacktrace/common.h b/arch/arm64/include/asm/stacktrace/common.h
> 
> >> @@ -21,6 +21,8 @@ struct stack_info {
> >>   *
> >>   * @fp:          The fp value in the frame record (or the real fp)
> >>   * @pc:          The lr value in the frame record (or the real lr)
> >> + * @sp:          The sp value at the call site of the current function.
> >> + * @unreliable:  Stacktrace is unreliable.
> >>   *
> >>   * @stack:       The stack currently being unwound.
> >>   * @stacks:      An array of stacks which can be unwound.
> >> @@ -29,7 +31,11 @@ struct stack_info {
> >>  struct unwind_state {
> >>  	unsigned long fp;
> >>  	unsigned long pc;
> >> +#ifdef CONFIG_HAVE_UNWIND_KERNEL_SFRAME
> >> +	unsigned long sp;
> >> +#endif
> > 
> > As this is only used by the kernel unwinder (and not the hyp unwinder),
> > this should live in struct kunwind_state in stacktrace.c.
> > 
> > That said, for unwinding across exception boundaries we should not need
> > this, as the SP value will be in the pt_regs. If we only use SFrame for
> > the exception boundary case, we can remove this entirely. I would
> > strongly prefer that we do that.

> >> +	/* Get the Canonical Frame Address (CFA) */
> >> +	switch (frame.cfa.rule) {
> >> +	case UNWIND_CFA_RULE_SP_OFFSET:
> >> +		cfa = state->common.sp;
> 
> IIUC you suggest this to be changed as follows?
> 
> 		cfa = regs->regs[31];

I was suggesting:

		cfa = regs->sp;

Note: arm64's struct pt_regs has:

	union {
		struct user_pt_regs user_regs;
		struct {
			u64 regs[31];
			u64 sp;
			u64 pc;
			u64 pstate;
		};
	};	

... so regs->regs[31] would be an out-of-bounds array access.

[...]

> >> +	case UNWIND_CFA_RULE_REG_OFFSET:
> >> +	case UNWIND_CFA_RULE_REG_OFFSET_DEREF:
> >> +		/* regs only available in topmost/interrupt frame */
> >> +		if (!regs || frame.cfa.regnum > 30)
> >> +			return -EINVAL;
> >> +		cfa = regs->regs[frame.cfa.regnum];
> >> +		break;
> > 
> > Do we ever expect to see UNWIND_CFA_RULE_REG_OFFSET or
> > UNWIND_CFA_RULE_REG_OFFSET_DEREF in practice for kernel code?
> 
> No.  Those can only occur with SFrame V3 flexible FDE, which are
> currently not generated by GNU assembler for arm64/aarch64, and thus
> could be omitted in the arm64-specific kernel sframe unwinder:
> 
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/config/tc-aarch64.h;hb=binutils-2_46#l342

Ok.

Do we know whether there are currently cases on aarch64 that cannot be
encoded in SFrame (without flexible FDE), or whether SFrame without
flexible FDE is sufficient for arm64 as-is? ... or do we have
counter-examples today?

Looking at:

  https://sourceware.org/binutils/docs/sframe-spec.html#Flexible-FDE-Type-Interpretation-1

For arm64 I'm not sure whether we'd encounter the DRAP or stack
realignment cases within the kernel (perhaps with SVE?), nor whether the
Register-based RA/FP Locations cases would apply if we assume that we
continue to use frame records.

[... ]

> I must admit that while reviewing I thought it would be future-proof to
> have support for rules that can only be represented with SFrame V3
> flexible FDE, even if they are currently not used on arm64.  Ideally
> kunwind_next_sframe() could be made common, if another architecture
> would implement kernel unwinding using sframe.

While I understand that principle, I think that for now it would be
better to keep this arch-specific and minimal:

* We have arch-specific concerns (e.g. the FRAME_META_TYPE_FINAL
  frames), and factoring that into generic code is going to be painful
  to adapt (which we're likely to need to do in the near future), and to
  maintain going forwards. Keeping that arch-specific for now will make
  it easier/quicker to get to a stable state.

* Code which isn't used is liable to be wrong or made wrong by accident.
  For example, with all the SP cases I mentioned in my initial reply.

We can certainly look at making that more generic in future, but for now
I'd prefer to omit the code that cannot be used (and have some sort of
build or boot/module-load time check that SFrame only has elements that
we expect), and make sure that we thoroughly test the cases that do
exist in practice.

Do we expect SFrame V3 flexible FDE to be generated by toolchains in the
near future?

[...]

> >> +	/* CFA alignment 8 bytes */
> >> +	if (cfa & 0x7)
> >> +		return -EINVAL;
> > 
> > If the CFA is the SP upon entry to the function, then per AAPCS64 rules
> > it should be aligned to 16 bytes. Otherwise, where has this 8 byte
> > alignment requirement come from? Does SFrame mandate that?
> 
> That originates from the common unwind user logic (see
> kernel/unwind/user.c, unwind_user_next_common()), which currently
> assumes 8-byte/4-byte SP alignment for all 64-bit/32-bit architectures.
> 
> So checking for 16-byte alignment here would make sense.

Just to confirm, am I correct to understand that the SFrame definition
of CFA is intended to be the same as the DWARF definition of CFA, and so
for arm64 the CFA is the SP when the function is called?

That's the case for DWARF on arm64:

  https://github.com/ARM-software/abi-aa/releases/download/2025Q4/aadwarf64.pdf
  https://github.com/ARM-software/abi-aa/blob/daa7a94ca55973736c0e434a67a6e4bbcd35d7fa/aadwarf64/aadwarf64.rst

| The CFA is the value of the stack pointer (sp) at the call site in the
| previous frame.

I couldn't find an explciit statement to that effect in:

  https://sourceware.org/binutils/docs/sframe-spec.html

... but I guess that is implied, given the other bits inherited from
DWARF.

I see that the documented behaviour for CFA on AMD64 and s390x are
consistent with their DWARF behaviour.

> >> +
> >> +	/* Get the Return Address (RA) */
> >> +	switch (frame.ra.rule) {
> >> +	case UNWIND_RULE_RETAIN:
> >> +		/* regs only available in topmost/interrupt frame */
> >> +		if (!regs)
> >> +			return -EINVAL;
> >> +		ra = regs->regs[30];
> >> +		source = KUNWIND_SOURCE_REGS_LR;
> >> +		break;
> >> +	/* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
> 
> Nit: s/UNWIND_USER_RULE_CFA_OFFSET/UNWIND_RULE_CFA_OFFSET/
> 
> > 
> > It would be better for the comment to say *why* that's not implemented.
> > 
> > I assume that's because UNWIND_USER_RULE_CFA_OFFSET would mean that the return
> > address is a stack address, and that's obviously not legitimate.
> 
> That and SFrame V3 currently cannot represent FP/RA as CFA + offset
> (i.e. UNWIND_RULE_CFA_OFFSET; .cfi_val_offset FP/RA).
> 
> The comment originates from the common unwind user logic (see
> kernel/unwind/user.c).  I am open to improve that.  What about:
> 
> 	/*
> 	 * UNWIND_RULE_CFA_OFFSET not implemented on purpose, as a stack
> 	 * address cannot be a legitimate return address value.  It is
> 	 * also not used (e.g. not represented in sframe).
> 	 */

I'd go with something simpler, e.g.

	/*
	 * UNWIND_RULE_CFA_OFFSET doesn't make sense for RA.
	 * The return address cannot legitimately be a stack addres.
	 */

[...]

> > 
> > I don't think we expect UNWIND_RULE_REG_OFFSET unless that's sometimes used
> > instead of UNWIND_RULE_RETAIN to express that the return address is in x30
> > (with zero offset).
> 
> No.  Unless there would be nonsense .cfi_register 30, 30, which would
> require SFrame V3 flexible FDE to be represented.

Ok.

> @Indu:  We may consider to treat .cfi_register <reg>, <reg> (for FP/RA)
> like .cfi_restore <reg> in the GNU assembler?
> 
> > Similarly, if the address is on the stack it should be in a frame
> > record. Would we ever expect UNWIND_RULE_REG_OFFSET_DEREF rather than
> > UNWIND_RULE_CFA_OFFSET_DEREF?
> 
> No.  See above (SFrame V3 flexible FDE).

Ok.

> >> +	default:
> >> +		WARN_ON_ONCE(1);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	/* Get the Frame Pointer (FP) */
> >> +	switch (frame.fp.rule) {
> >> +	case UNWIND_RULE_RETAIN:
> >> +		fp = state->common.fp;
> >> +		break;
> >> +	/* UNWIND_USER_RULE_CFA_OFFSET not implemented on purpose */
> > 
> > As for RA, the comment should explain why that's not implemented.
> 
> I am open to improve the comment in the the common unwind user logic.
> What about:
> 
> 	/*
> 	 * UNWIND_RULE_CFA_OFFSET not implemented on purpose, as it is
> 	 * not used (e.g. not represented in sframe).
> 	 */

For me, this wording raises more questions, e.g.

* Does 'not used' mean that toolchains don't use that, or that the spec
  doesn't permit that?

* Does 'not represented' mean that this is not represntable, or that
  toolchains currently don't generate SFrame with the appropriate
  elements.

IIUC you're saying that this *is* representable with flexible FDE, but
current toolchains don't generate that.

Mark.

  reply	other threads:[~2026-05-05 10:29 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-28 18:36 [PATCH v5 0/8] unwind, arm64: add sframe unwinder for kernel Dylan Hatch
2026-04-28 18:36 ` [PATCH v5 1/8] sframe: Allow kernelspace sframe sections Dylan Hatch
2026-04-28 18:36 ` [PATCH v5 2/8] arm64, unwind: build kernel with sframe V3 info Dylan Hatch
2026-04-28 18:36 ` [PATCH v5 3/8] arm64: entry: add unwind info for various kernel entries Dylan Hatch
2026-04-29 15:26   ` Mark Rutland
2026-04-28 18:36 ` [PATCH v5 4/8] sframe: Provide PC lookup for vmlinux .sframe section Dylan Hatch
2026-04-28 18:36 ` [PATCH v5 5/8] sframe: Allow unsorted FDEs Dylan Hatch
2026-04-30 10:04   ` Jens Remus
2026-04-28 18:36 ` [PATCH v5 6/8] arm64/module, sframe: Add sframe support for modules Dylan Hatch
2026-04-30 10:04   ` Jens Remus
2026-04-28 18:36 ` [PATCH v5 7/8] sframe: Introduce in-kernel SFRAME_VALIDATION Dylan Hatch
2026-04-30 10:04   ` Jens Remus
2026-04-28 18:36 ` [PATCH v5 8/8] unwind: arm64: Use sframe to unwind interrupt frames Dylan Hatch
2026-05-01 16:46   ` Mark Rutland
2026-05-04  8:47     ` Jens Remus
2026-05-05 10:29       ` Mark Rutland [this message]
2026-04-29 17:18 ` [PATCH v5 0/8] unwind, arm64: add sframe unwinder for kernel Mark Rutland
2026-04-30 10:11 ` Jens Remus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=afnGhsCYEUG0LXR5@J2N7QTR9R3 \
    --to=mark.rutland@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=dylanbhatch@google.com \
    --cc=hca@linux.ibm.com \
    --cc=ibhagatgnu@gmail.com \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=jpoimboe@kernel.org \
    --cc=jremus@linux.ibm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-toolchains@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=ptsm@linux.microsoft.com \
    --cc=puranjay@kernel.org \
    --cc=rdunlap@infradead.org \
    --cc=roman.gushchin@linux.dev \
    --cc=rostedt@goodmis.org \
    --cc=song@kernel.org \
    --cc=will@kernel.org \
    --cc=wnliu@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox