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

[Nicholas Piggin <npiggin@gmail.com>]

On Thu, 16 Mar 2017 18:10:48 +0530
Mahesh Jagannath Salgaonkar <mahesh@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@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 ?

Hmm, on second look, I don't think any non-volatile GPRs are overwritten
in this path. But this MCE is a slow path, and it is a much longer path
than the system reset idle wakeup... So I'll add the napstatelost with
a comment.

Thanks,
Nick