Re: [RFC PATCH v2 2/8] arm64: Implement frame types

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On Mon, Mar 15, 2021 at 11:57:54AM -0500, madvenka@linux.microsoft.com wrote:

> To summarize, pt_regs->stackframe is used (or will be used) as a marker
> frame in stack traces. To enable the unwinder to detect these frames, tag
> each pt_regs->stackframe with a type. To record the type, use the unused2
> field in struct pt_regs and rename it to frame_type. The types are:

Unless I'm misreading what's going on here this is more trying to set a
type for the stack as a whole than for a specific stack frame.  I'm also
finding this a bit confusing as the unwinder already tracks things it
calls frame types and it handles types that aren't covered here like
SDEI.  At the very least there's a naming issue here.

Taking a step back though do we want to be tracking this via pt_regs?
It's reliant on us robustly finding the correct pt_regs and on having
the things that make the stack unreliable explicitly go in and set the
appropriate type.  That seems like it will be error prone, I'd been
expecting to do something more like using sections to filter code for
unreliable features based on the addresses of the functions we find on
the stack or similar.  This could still go wrong of course but there's
fewer moving pieces, and especially fewer moving pieces specific to
reliable stack trace.

I'm wary of tracking data that only ever gets used for the reliable
stack trace path given that it's going to be fairly infrequently used
and hence tested, especially things that only crop up in cases that are
hard to provoke reliably.  If there's a way to detect things that
doesn't use special data that seems safer.

> EL1_FRAME
> 	EL1 exception frame.

We do trap into EL2 as well, the patch will track EL2 frames as EL1
frames.  Even if we can treat them the same the naming ought to be
clear.

> FTRACE_FRAME
>         FTRACE frame.

This is implemented later in the series.  If using this approach I'd
suggest pulling the change in entry-ftrace.S that sets this into this
patch, it's easier than adding a note about this being added later and
should help with any bisect issues.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]