On 10/11/2017 03:59 AM, Matt Redfearn wrote:
Commit 9fef68686317b ("MIPS: Make SAVE_SOME more standard") made several
changes to the order in which registers are saved in the SAVE_SOME
macro, used by exception handlers to save the processor state. In
particular, it removed the
move k1, sp
in the delay slot of the branch testing if the processor is already in
kernel mode. This is replaced later in the macro by a
move k0, sp
When CONFIG_EVA is disabled, this instruction actually appears in the
delay slot of the branch. However, when CONFIG_EVA is enabled, instead
the RPS workaround of
MFC0 k0, CP0_ENTRYHI
appears in the delay slot. This results in k0 not containing the stack
pointer, but some unrelated value, which is then saved to the kernel
stack. On exit from the exception, this bogus value is restored to the
stack pointer, resulting in an OOPS.
Fix this by moving the save of SP in k0 explicitly in the delay slot of
the branch, outside of the CONFIG_EVA section, restoring the expected
instruction ordering when CONFIG_EVA is active.
Fixes: 9fef68686317b ("MIPS: Make SAVE_SOME more standard")
Signed-off-by: Matt Redfearn <matt.redfearn@xxxxxxxx>
Reported-by: Vladimir Kondratiev <vladimir.kondratiev@xxxxxxxxx>
I looked this over pretty carefully and it looks correct to me. It
makes no difference
in the instructions generated by the non-EVA case. I shouldn't have
missed this :(.
Reviewed-by: Corey Minyard <cminyard@xxxxxxxxxx>
---
Note that some of our compiler people are dubious about putting frame
related instructions in conditionally executed blocks of code. In this
case, presuming that we only care about unwinding the kernel stack, then
we only care about the case in which the branch is taken, and k0 always
contains the SP to be saved. There is also a question about putting
frame related instructions in branch delay slots. Again, in this case,
we think it's OK to use them since the only path that ought to be
unwound will be the "branch taken" route where we are already on the
kernel stack.
Since the compiler can put frame-related instructions in delay slots (see
aee16625b19 MIPS: Fix issues in backtraces), it's probably ok. I have
tested
this before with kernel dumps and gdb, and gdb had no issues with this.
That said, this is a tricky case. But looking at the generated unwinding
info, it seems to do the right thing.
Not having access to a CFI based kernel stack unwinder makes this change
difficult to verify, but since the same construct already existed when
CONFIG_EVA is disabled, I don't think this change is likely to break the
unwinder, and fixes exception entry when CONFIG_EVA is enabled.
Agreed. Thanks for fixing this.
-corey
Thanks,
Matt
---
arch/mips/include/asm/stackframe.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
index 5d3563c55e0c..2161357cc68f 100644
--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -199,6 +199,10 @@
sll k0, 3 /* extract cu0 bit */
.set noreorder
bltz k0, 8f
+ move k0, sp
+ .if \docfi
+ .cfi_register sp, k0
+ .endif
#ifdef CONFIG_EVA
/*
* Flush interAptiv's Return Prediction Stack (RPS) by writing
@@ -225,10 +229,6 @@
MTC0 k0, CP0_ENTRYHI
#endif
.set reorder
- move k0, sp
- .if \docfi
- .cfi_register sp, k0
- .endif
/* Called from user mode, new stack. */
get_saved_sp docfi=\docfi tosp=1
8:
