aarch64: Save correct value of x18 on trapframe for nested faults
ClosedPublic
Actions

Authored by jhb on Wed, Sep 10, 2:28 PM.

Details

Reviewers

andrew
def
manu

Commits

rG99213b3c352c: aarch64: Save correct value of x18 on trapframe for nested faults

Summary

x18 is overwritten with a temporary copy of the kernel stack pointer
when it is saved in the trapframe. This does not matter in terms of
function since nested exception return does not restore x18 from the
trapframe, but it does mean that examining x18 in a debugger in stack
frames above a nested fault outputs the wrong register value.

To fix, compute the value of the original stack pointer to save in x18
later after the trapframe has been constructed.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jhb created this revision.Wed, Sep 10, 2:28 PM

Herald added a reviewer: manu. · View Herald TranscriptWed, Sep 10, 2:28 PM

Herald added subscribers: emaste, imp. · View Herald Transcript

jhb requested review of this revision.Wed, Sep 10, 2:28 PM

Harbormaster completed remote builds in B66949: Diff 161856.Wed, Sep 10, 2:28 PM

I had worked on this patch a while ago due to the weird kgdb issue, but @def has some downstream changes in CheriBSD where the wrong saved value in the trapframe matters outside of just debugging I believe.

Thanks for fixing this in upstream!

This revision is now accepted and ready to land.Wed, Sep 10, 2:32 PM

Are you receiving the exception within the exception entry asm? I can easily see functional issues if they happen before the stp on line 73, although it may not be until later to be fully safe.

sys/arm64/arm64/exception.S
64	Not a `sub` with a positive value?

The point is any nested kernel exception saves the "wrong" value in the x18 stack in the trapframe. That value isn't used during exception exit so it doesn't matter currently (though Konrad I think does depend on changing x18 in trapframes downstream). The way this is visible in kgdb is that when you unwind across a nested exception, gdb believes that x18's value is saved in the trapframe (it just always believes the trapframe layout for any exception) so if you do p $x18 in the frames "above" a nested exception it will not reflect the actual value the CPU was using. So this is true for any nested exception (e.g. a device interrupt taking while handling a system call).

sys/arm64/arm64/exception.S
64	Hmm, I could do that if that is cleaner. Probably does read better in fact.

jhb added inline comments.Fri, Sep 12, 1:35 PM

sys/arm64/arm64/exception.S
64	One thing to keep in mind is that TF_X (48) - TF_SIZE (288) is actually negative, so this actually ends up being: add x18, sp, #368 (And logically, `sp` should be moving down the stack so getting back to the original value should be an `add`.)

def added inline comments.Fri, Sep 12, 3:20 PM

sys/arm64/arm64/exception.S
64	I like having `add` here to indicate that it "reverts" the stack allocation and that you used "-(the original value)" to easily correlate that with the stack allocation at the beginning of this macro.

I'm not opposed to the current change, but think it would be less confusing if we pass a value that is more obvious it is negative to the stp instructions.

sys/arm64/arm64/exception.S
64	I think the EL1 `stp` would be clearer if it used `#-(TF_SIZE - TF_X + 128)` (and similar for the EL0 case) & this to the positive version. A more intrusive change would be to reorder `struct trapframe` so `tf_x` is first & we could just set sp to the base of the trapframe. This would need more testing as we reuse it in vmm.ko.

jhb added inline comments.Fri, Sep 12, 7:16 PM

sys/arm64/arm64/exception.S
64	Changing trapframe is a bit fraught as kgdb hardcodes that knowledge. I would be fine with fixing the arguments to stp to be clearer (and in fact I would prefer that quite a bit). I can add that as a commit before this and post a new review for that.