| Message ID | 20220308135047.478297-9-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/rtas: various cleanups and improvements | expand |
On 08/03/2022, 14:50:41, Nicholas Piggin wrote: > This moves MSR save/restore and some real-mode juggling out of asm and > into C code, simplifying things. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/kernel/rtas.c | 15 ++++++++++++--- > arch/powerpc/kernel/rtas_entry.S | 32 +++++--------------------------- > 2 files changed, 17 insertions(+), 30 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index 6b5892d6a56b..87ede1877816 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -47,13 +47,22 @@ > /* This is here deliberately so it's only used in this file */ > void enter_rtas(unsigned long); > > -static inline void do_enter_rtas(unsigned long args) > +static noinline void do_enter_rtas(unsigned long args) > { > BUG_ON(!irqs_disabled()); > > - hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */ > + if (IS_ENABLED(CONFIG_PPC64)) { > + unsigned long msr; > > - enter_rtas(args); > + hard_irq_disable(); > + > + msr = mfmsr(); > + mtmsr(msr & ~(MSR_IR|MSR_DR)); > + enter_rtas(args); > + mtmsr(msr); > + } else { > + enter_rtas(args); > + } > > srr_regs_clobbered(); /* rtas uses SRRs, invalidate */ > } > diff --git a/arch/powerpc/kernel/rtas_entry.S b/arch/powerpc/kernel/rtas_entry.S > index 5f65ea4436c6..292551684bbd 100644 > --- a/arch/powerpc/kernel/rtas_entry.S > +++ b/arch/powerpc/kernel/rtas_entry.S > @@ -84,14 +84,11 @@ _GLOBAL(enter_rtas) > li r0,0 > mtcr r0 > > - mfmsr r6 > - > - /* Unfortunately, the stack pointer and the MSR are also clobbered, > - * so they are saved in the PACA which allows us to restore > - * our original state after RTAS returns. > + /* > + * The stack pointer is clobbered, so it is saved in the PACA which > + * allows us to restore our original state after RTAS returns. > */ > std r1,PACAR1(r13) > - std r6,PACASAVEDMSR(r13) > > /* Setup our real return addr */ > LOAD_REG_ADDR(r4,rtas_return_loc) > @@ -100,7 +97,6 @@ _GLOBAL(enter_rtas) > > LOAD_REG_IMMEDIATE(r6, MSR_ME) > > -__enter_rtas: > LOAD_REG_ADDR(r4, rtas) > ld r5,RTASENTRY(r4) /* get the rtas->entry value */ > ld r4,RTASBASE(r4) /* get the rtas->base value */ > @@ -112,6 +108,7 @@ __enter_rtas: > mtspr SPRN_SRR1,r6 > RFI_TO_KERNEL > b . /* prevent speculative execution */ > +_ASM_NOKPROBE_SYMBOL(enter_rtas) > > rtas_return_loc: > FIXUP_ENDIAN > @@ -127,29 +124,10 @@ rtas_return_loc: > sync > mtmsrd r6 Since MSR plumbing is still needed in the asm, what is the benefit of doing the real mode switching in the C code? What if the MSR is saved in the PACA before switching to real mode, and restored in rtas_return_loc? > > - /* relocation is off at this point */ > GET_PACA(r13) > > - bcl 20,31,$+4 > -0: mflr r3 > - ld r3,(1f-0b)(r3) /* get &rtas_restore_regs */ > - > ld r1,PACAR1(r13) /* Restore our SP */ > - ld r4,PACASAVEDMSR(r13) /* Restore our MSR */ > > - mtspr SPRN_SRR0,r3 > - mtspr SPRN_SRR1,r4 > - RFI_TO_KERNEL rfid is not more called to restore MSR. Noob question, is there any impact of using mtmsrd instead of rfid to restore the MSR? > - b . /* prevent speculative execution */ > -_ASM_NOKPROBE_SYMBOL(enter_rtas) > -_ASM_NOKPROBE_SYMBOL(__enter_rtas) > -_ASM_NOKPROBE_SYMBOL(rtas_return_loc) > - > - .align 3 > -1: .8byte rtas_restore_regs > - > -rtas_restore_regs: > - /* relocation is on at this point */ > REST_GPR(2, r1) /* Restore the TOC */ > REST_NVGPRS(r1) /* Restore the non-volatiles */ > > @@ -169,5 +147,5 @@ rtas_restore_regs: > > mtlr r0 > blr /* return to caller */ > - > +_ASM_NOKPROBE_SYMBOL(rtas_return_loc) > #endif /* CONFIG_PPC32 */
On 08/03/2022, 14:50:41, Nicholas Piggin wrote: > This moves MSR save/restore and some real-mode juggling out of asm and > into C code, simplifying things. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/kernel/rtas.c | 15 ++++++++++++--- > arch/powerpc/kernel/rtas_entry.S | 32 +++++--------------------------- > 2 files changed, 17 insertions(+), 30 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index 6b5892d6a56b..87ede1877816 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -47,13 +47,22 @@ > /* This is here deliberately so it's only used in this file */ > void enter_rtas(unsigned long); > > -static inline void do_enter_rtas(unsigned long args) > +static noinline void do_enter_rtas(unsigned long args) > { > BUG_ON(!irqs_disabled()); > > - hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */ > + if (IS_ENABLED(CONFIG_PPC64)) { > + unsigned long msr; > > - enter_rtas(args); > + hard_irq_disable(); > + > + msr = mfmsr(); > + mtmsr(msr & ~(MSR_IR|MSR_DR)); Further test done on top this series shows that switching MSR_DR off before entering enter_rtas() is generating DSI when accessing the stack in enter_rtas(). This may happen if the task stack is mapped beyond the VRMA. Furthermore, there is no real need to run enter_rtas() in real mode (IR and DR unset) because the MSR will be set to real mode when doing rfid, see below. > + enter_rtas(args); > + mtmsr(msr); > + } else { > + enter_rtas(args); > + } > > srr_regs_clobbered(); /* rtas uses SRRs, invalidate */ > } > diff --git a/arch/powerpc/kernel/rtas_entry.S b/arch/powerpc/kernel/rtas_entry.S > index 5f65ea4436c6..292551684bbd 100644 > --- a/arch/powerpc/kernel/rtas_entry.S > +++ b/arch/powerpc/kernel/rtas_entry.S > @@ -84,14 +84,11 @@ _GLOBAL(enter_rtas) > li r0,0 > mtcr r0 > > - mfmsr r6 > - > - /* Unfortunately, the stack pointer and the MSR are also clobbered, > - * so they are saved in the PACA which allows us to restore > - * our original state after RTAS returns. > + /* > + * The stack pointer is clobbered, so it is saved in the PACA which > + * allows us to restore our original state after RTAS returns. > */ > std r1,PACAR1(r13) > - std r6,PACASAVEDMSR(r13) > > /* Setup our real return addr */ > LOAD_REG_ADDR(r4,rtas_return_loc) > @@ -100,7 +97,6 @@ _GLOBAL(enter_rtas) > > LOAD_REG_IMMEDIATE(r6, MSR_ME) > > -__enter_rtas: > LOAD_REG_ADDR(r4, rtas) > ld r5,RTASENTRY(r4) /* get the rtas->entry value */ > ld r4,RTASBASE(r4) /* get the rtas->base value */ > @@ -112,6 +108,7 @@ __enter_rtas: > mtspr SPRN_SRR1,r6 > RFI_TO_KERNEL rfid will load the MSR with the value stored in SRR1 (formely r6) and so switch to the real mode. This why there is no need to switch earlier in real mode. > b . /* prevent speculative execution */ > +_ASM_NOKPROBE_SYMBOL(enter_rtas) > > rtas_return_loc: > FIXUP_ENDIAN > @@ -127,29 +124,10 @@ rtas_return_loc: > sync > mtmsrd r6 > > - /* relocation is off at this point */> GET_PACA(r13) > > - bcl 20,31,$+4 > -0: mflr r3 > - ld r3,(1f-0b)(r3) /* get &rtas_restore_regs */ > - > ld r1,PACAR1(r13) /* Restore our SP */ > - ld r4,PACASAVEDMSR(r13) /* Restore our MSR */ > > - mtspr SPRN_SRR0,r3 > - mtspr SPRN_SRR1,r4 > - RFI_TO_KERNEL This was turning on MSR_DR and MSR_IR so rtas_restore() could access the stack even if it is beyond the VRMA. That patch is breaking this and generating panic when task's stack are below VRMA. > - b . /* prevent speculative execution */ > -_ASM_NOKPROBE_SYMBOL(enter_rtas) > -_ASM_NOKPROBE_SYMBOL(__enter_rtas) > -_ASM_NOKPROBE_SYMBOL(rtas_return_loc) > - > - .align 3 > -1: .8byte rtas_restore_regs > - > -rtas_restore_regs: > - /* relocation is on at this point */ > REST_GPR(2, r1) /* Restore the TOC */ > REST_NVGPRS(r1) /* Restore the non-volatiles */ > > @@ -169,5 +147,5 @@ rtas_restore_regs: > > mtlr r0 > blr /* return to caller */ > - > +_ASM_NOKPROBE_SYMBOL(rtas_return_loc) > #endif /* CONFIG_PPC32 */
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index 6b5892d6a56b..87ede1877816 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -47,13 +47,22 @@ /* This is here deliberately so it's only used in this file */ void enter_rtas(unsigned long); -static inline void do_enter_rtas(unsigned long args) +static noinline void do_enter_rtas(unsigned long args) { BUG_ON(!irqs_disabled()); - hard_irq_disable(); /* Ensure MSR[EE] is disabled on PPC64 */ + if (IS_ENABLED(CONFIG_PPC64)) { + unsigned long msr; - enter_rtas(args); + hard_irq_disable(); + + msr = mfmsr(); + mtmsr(msr & ~(MSR_IR|MSR_DR)); + enter_rtas(args); + mtmsr(msr); + } else { + enter_rtas(args); + } srr_regs_clobbered(); /* rtas uses SRRs, invalidate */ } diff --git a/arch/powerpc/kernel/rtas_entry.S b/arch/powerpc/kernel/rtas_entry.S index 5f65ea4436c6..292551684bbd 100644 --- a/arch/powerpc/kernel/rtas_entry.S +++ b/arch/powerpc/kernel/rtas_entry.S @@ -84,14 +84,11 @@ _GLOBAL(enter_rtas) li r0,0 mtcr r0 - mfmsr r6 - - /* Unfortunately, the stack pointer and the MSR are also clobbered, - * so they are saved in the PACA which allows us to restore - * our original state after RTAS returns. + /* + * The stack pointer is clobbered, so it is saved in the PACA which + * allows us to restore our original state after RTAS returns. */ std r1,PACAR1(r13) - std r6,PACASAVEDMSR(r13) /* Setup our real return addr */ LOAD_REG_ADDR(r4,rtas_return_loc) @@ -100,7 +97,6 @@ _GLOBAL(enter_rtas) LOAD_REG_IMMEDIATE(r6, MSR_ME) -__enter_rtas: LOAD_REG_ADDR(r4, rtas) ld r5,RTASENTRY(r4) /* get the rtas->entry value */ ld r4,RTASBASE(r4) /* get the rtas->base value */ @@ -112,6 +108,7 @@ __enter_rtas: mtspr SPRN_SRR1,r6 RFI_TO_KERNEL b . /* prevent speculative execution */ +_ASM_NOKPROBE_SYMBOL(enter_rtas) rtas_return_loc: FIXUP_ENDIAN @@ -127,29 +124,10 @@ rtas_return_loc: sync mtmsrd r6 - /* relocation is off at this point */ GET_PACA(r13) - bcl 20,31,$+4 -0: mflr r3 - ld r3,(1f-0b)(r3) /* get &rtas_restore_regs */ - ld r1,PACAR1(r13) /* Restore our SP */ - ld r4,PACASAVEDMSR(r13) /* Restore our MSR */ - mtspr SPRN_SRR0,r3 - mtspr SPRN_SRR1,r4 - RFI_TO_KERNEL - b . /* prevent speculative execution */ -_ASM_NOKPROBE_SYMBOL(enter_rtas) -_ASM_NOKPROBE_SYMBOL(__enter_rtas) -_ASM_NOKPROBE_SYMBOL(rtas_return_loc) - - .align 3 -1: .8byte rtas_restore_regs - -rtas_restore_regs: - /* relocation is on at this point */ REST_GPR(2, r1) /* Restore the TOC */ REST_NVGPRS(r1) /* Restore the non-volatiles */ @@ -169,5 +147,5 @@ rtas_restore_regs: mtlr r0 blr /* return to caller */ - +_ASM_NOKPROBE_SYMBOL(rtas_return_loc) #endif /* CONFIG_PPC32 */
This moves MSR save/restore and some real-mode juggling out of asm and into C code, simplifying things. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/kernel/rtas.c | 15 ++++++++++++--- arch/powerpc/kernel/rtas_entry.S | 32 +++++--------------------------- 2 files changed, 17 insertions(+), 30 deletions(-)