Re: [PATCH 4/8] powerpc/64s: fix POWER9 machine check handler from stop state

[Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com>]

On 03/14/2017 02:53 PM, Nicholas Piggin wrote:
> The ISA specifies power save wakeup can cause a machine check interrupt.
> The machine check handler currently has code to handle that for POWER8,
> but POWER9 crashes when trying to execute the P8 style sleep
> instructions.
> 
> So queue up the machine check, then call into the idle code to wake up
> as the system reset interrupt does, rather than attempting to sleep
> again without going through the main idle path.
> 
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/include/asm/reg.h       |  1 +
>  arch/powerpc/kernel/exceptions-64s.S | 69 ++++++++++++++++++------------------
>  2 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index fc879fd6bdae..8bbdfacce970 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -656,6 +656,7 @@
>  #define   SRR1_ISI_PROT		0x08000000 /* ISI: Other protection fault */
>  #define   SRR1_WAKEMASK		0x00380000 /* reason for wakeup */
>  #define   SRR1_WAKEMASK_P8	0x003c0000 /* reason for wakeup on POWER8 and 9 */
> +#define   SRR1_WAKEMCE_RESVD	0x003c0000 /* Unused/reserved value used by MCE wakeup to indicate cause to idle wakeup handler */
>  #define   SRR1_WAKESYSERR	0x00300000 /* System error */
>  #define   SRR1_WAKEEE		0x00200000 /* External interrupt */
>  #define   SRR1_WAKEHVI		0x00240000 /* Hypervisor Virtualization Interrupt (P9) */
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index e390fcd04bcb..5779d2d6a192 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -306,6 +306,33 @@ EXC_COMMON_BEGIN(machine_check_common)
>  	/* restore original r1. */			\
>  	ld	r1,GPR1(r1)
> 
> +#ifdef CONFIG_PPC_P7_NAP
> +EXC_COMMON_BEGIN(machine_check_idle_common)
> +	bl	machine_check_queue_event
> +	/*
> +	 * Queue the machine check, then reload SRR1 and use it to set
> +	 * CR3 according to pnv_powersave_wakeup convention.
> +	 */
> +	ld	r12,_MSR(r1)
> +	rlwinm	r11,r12,47-31,30,31
> +	cmpwi	cr3,r11,2
> +
> +	/*
> +	 * Now put SRR1_WAKEMCE_RESVD into SRR1, allows it to follow the
> +	 * system reset wakeup code.
> +	 */
> +	oris	r12,r12,SRR1_WAKEMCE_RESVD@h
> +	mtspr	SPRN_SRR1,r12
> +	std	r12,_MSR(r1)
> +
> +	/*
> +	 * Decrement MCE nesting after finishing with the stack.
> +	 */
> +	lhz	r11,PACA_IN_MCE(r13)
> +	subi	r11,r11,1
> +	sth	r11,PACA_IN_MCE(r13)

Looks like we are not winding up.. Shouldn't we ? What if we may end up
in pnv_wakeup_noloss() which assumes that no GPRs are lost. Am I missing
anything ?

> +	b	pnv_powersave_wakeup
> +#endif
>  	/*

[...]

Rest looks good to me.

Reviewed-by: Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com>

Thanks,
-Mahesh.