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

[Mahesh Jagannath Salgaonkar]

On 03/16/2017 06:49 PM, Gautham R Shenoy wrote:
> Hi,
> 
> On Thu, Mar 16, 2017 at 11:05:20PM +1000, Nicholas Piggin wrote:
>> On Thu, 16 Mar 2017 18:10:48 +0530
>> Mahesh Jagannath Salgaonkar <mahesh at linux.vnet.ibm.com> wrote:
>>
>>> 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 at linux.vnet.ibm.com>
>>>> Signed-off-by: Nicholas Piggin <npiggin at 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 at 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 ?
> 
> Nice catch! This can occur if SRR1[46:47] == 0b01.
> 
>>
>> Hmm, no I think you're right. Thanks, good catch. But can we do it with
>> just setting PACA_NAPSTATELOST?
> 
> Unconditionally setting PACA_NAPSTATELOST should be sufficient.

Agree, that should take care.

> 
>>
>>>
>>>> +	b	pnv_powersave_wakeup
>>>> +#endif
>>>>  	/*  
>>>
>>> [...]
>>>
>>> Rest looks good to me.
>>>
>>> Reviewed-by: Mahesh J Salgaonkar <mahesh at linux.vnet.ibm.com>
>>
>> Thanks,
>> Nick
>>