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));
> +		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 */