Re: [PATCH 08/14] powerpc/rtas: call enter_rtas in real-mode on 64-bit

[Laurent Dufour <ldufour@linux.ibm.com>]

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 */