arm64: Avoid clobbering the stack pointer when returning to EL1

[markjdb]

No description provided.

[bsdjhb]

How has this not broken before? This would seem to indicate that in hybrid kernels, every nested exception return sets the stack pointer to the per-CPU pointer?

[markjdb]

How has this not broken before? This would seem to indicate that in hybrid kernels, every nested exception return sets the stack pointer to the per-CPU pointer?

Indeed, I don't quite understand how this worked. Is there some other magic which restores the stack pointer? I only noticed this problem when handling breakpoints exceptions where dtrace has to manually adjust sp in order to emulate the overwritten instruction.

[bsdjhb]

Oh, so in the save_registers_head, we actually save sp in the x18 slot in the trapframe (that's confusing):

.macro	save_registers_head el
.if \el == 1
	mov	PTR(18), PTRN(sp)
....
	stp	CAP(18), CAP(19), [PTRN(sp), #(18 * CAP_WIDTH)]

So in actual fact, the load here is re-loading the saved sp again. It must be that in your case with dtrace you have adjusted sp to not reflect the value it used on exception entry so the extra value cached in tf_x18 is "wrong". OTOH, this also means that the saved tf_x18 value in nested trapframes is wrong. That doesn't really matter except perhaps for stack unwinding in debuggers which will get confused.

[bsdjhb]

This is also broken in upstream FWIW. If we really cared we would just do another store of x18 in the trapframe in the el ==1 case after restoring its value from tpidr_el1. That might run us out of instructions though? We could just add it at the start of save_registers instead?

[markjdb]

This is also broken in upstream FWIW.

Are you sure? In upstream kernels we don't bother restoring x18 when returning to EL1, see the comment, "We only restore called-saved registers...".

If we really cared we would just do another store of x18 in the trapframe in the el ==1 case after restoring its value from tpidr_el1. That might run us out of instructions though? We could just add it at the start of save_registers instead?

[bsdjhb]

In fact, I'd be tempted to refine this a bit further even upstream to avoid stomping on x18 in the el == 1 case. That is, I would only load the pair of registers in the el == 0 case and just load LR:

.if \el == 0
     ldp  CAP(18), CAPN(lr), [PTRN(sp), #(TF_SP)]
     msr CAPN(sp_el0), CAP(18)
.else
     ldr  CAPN(lr), [PTRN(sp), #(TF_LR)]
.endif

then at the end, you have:

.if \el == 0
     add PTRN(sp), PTRN(sp), #(TF_SIZE)
.else
     ldr   PTRN(sp), [PTRN(sp), #(TF_SP)
.endif

[bsdjhb]

This is also broken in upstream FWIW.

Are you sure? In upstream kernels we don't bother restoring x18 when returning to EL1, see the comment, "We only restore called-saved registers...".

By broken I mean that the values in struct trapframe are broken, so if you are doing a stack trace in kgdb and walk up the stack of a nested exception and p $x18 above the nested exception you will see the wrong value (you will see an alias of the stack pointer, not the per-CPU pointer). That is mostly cosmetic though and doesn't impact the running system, just potential confusion when debugging.

[bsdjhb]

I'll work on adjusting x18 upstream, but this should be fine for now.