[PATCH 10/10] arm64: stacktrace: unwind exception boundaries

[Miroslav Benes]

On Fri, 11 Oct 2024, Mark Rutland wrote:

> On Fri, Oct 11, 2024 at 05:16:23PM +0200, Miroslav Benes wrote:
>
> > >  	switch (state->source) {
> > >  	case KUNWIND_SOURCE_FRAME:
> > >  	case KUNWIND_SOURCE_CALLER:
> > >  	case KUNWIND_SOURCE_TASK:
> > > +	case KUNWIND_SOURCE_REGS_LR:
> > > +		err = kunwind_next_frame_record(state);
> > > +		break;
> > >  	case KUNWIND_SOURCE_REGS_PC:
> > > -		err = unwind_next_frame_record(&state->common);
> > > -		if (err)
> > > -			return err;
> > > -		state->source = KUNWIND_SOURCE_FRAME;
> > > +		err = kunwind_next_regs_lr(state);
> > 
> > the remaining users of unwind_next_frame_record() after this change are in 
> > KVM. How does it work there?
> 
> The short of it is those are unchanged by this series, and the behaviour
> of the KVM stacktrace code is unchanged -- it will continue to not
> report exception boundaries.
> 
> The KVM hyp code doesn't (currently) use the frame_record_meta records
> at exception boundaries, so the KVM stacktrace code won't see those and
> only need to unwind regular frame records.

Yes.
 
> It would be nice to improve that, but IIUC it shouldn't matter for
> RELAIBLE_STACKTRACE; all calls to hyp code from the main kernel are
> synchronous, and aside from a (fatal) hyp_panic(), executing in a
> regular kernel context ensures there's no hyp context to unwind.

I agree. I was mainly asking if there was a reason (how exceptions are 
processed for example) why it could not be unified because the first 
thought was that the differences are not so big on the code level.

But yeah, out of scope for this patch set.

> > What is the difference?
> 
> The kunwind_next_frame_record() function will identify and unwind any
> frame_record_meta frames, while unwind_next_frame_record() only handles
> regular frame records. Since the KVM hyp code doesn't use
> frame_record_meta frames, using unwind_next_frame_record() is
> sufficient.
> 
> I originally wanted the kunwind code call unwind_next_frame_record() and
> then handle any frame_record_meta, but that was painful due to the state
> that alters (e.g. the way we track which stack ranges have been
> consumed), and duplicating the easrly parts of
> unwind_next_frame_record() was simpler overall.

Ok.

> As a general naming convention, the unwind_*() functions are things that
> can be shared between the main kernel image and KVM, and the kunwind_*()
> functions are specific to the main kernel image.
> 
> I'm happy to change the name to more clearly distinguish
> kunwind_next_frame_record() from unwind_next_frame_record(), if that
> would help?

Nope, as far as I am concerned the convention makes sense and I have no 
problem with that.

Thank you,
Miroslav