Messages in this thread | | | Subject | Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled | From | Corey Minyard <> | Date | Wed, 11 Oct 2017 08:12:31 -0500 |
| |
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@mips.com> > Reported-by: Vladimir Kondratiev <vladimir.kondratiev@intel.com>
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@mvista.com>
> > --- > > 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:
| |