From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757399AbdJKNMg (ORCPT ); Wed, 11 Oct 2017 09:12:36 -0400 Received: from mail-pg0-f43.google.com ([74.125.83.43]:54119 "EHLO mail-pg0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752066AbdJKNMe (ORCPT ); Wed, 11 Oct 2017 09:12:34 -0400 X-Google-Smtp-Source: AOwi7QC+qOHhDDo6PP+T8adhrM89wmoMPfQrPfrR+mnM+3xlJg47roBAyjFhlnQ7IdwmdVdOJVj8MA== Subject: Re: [PATCH] MIPS: Fix exception entry when CONFIG_EVA enabled To: Matt Redfearn , Ralf Baechle , James Hogan Cc: Matthew Fortune , linux-mips@linux-mips.org, linux-kernel@vger.kernel.org, "Jason A. Donenfeld" , Paul Burton References: <1507712360-20657-1-git-send-email-matt.redfearn@mips.com> From: Corey Minyard Message-ID: <605f6a96-a843-085c-efc6-a2c0f2afd84a@mvista.com> Date: Wed, 11 Oct 2017 08:12:31 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1507712360-20657-1-git-send-email-matt.redfearn@mips.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Reported-by: Vladimir Kondratiev 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 > > --- > > 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: