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

[Nicholas Piggin <npiggin@gmail.com>]

Hi Gautham,

I'm just getting back to this. Sorry for the late reply, and
thanks for the reviews.

On Tue, 28 Feb 2017 22:45:46 +0530
Gautham R Shenoy <ego.lkml@gmail.com> wrote:

> Hi Nick,
> 
> On Fri, Feb 17, 2017 at 12:09 AM, Nicholas Piggin <npiggin@gmail.com> 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.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  arch/powerpc/kernel/exceptions-64s.S | 71 ++++++++++++++++++------------------
> >  1 file changed, 36 insertions(+), 35 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> > index 5f775783f744..0388843c8d12 100644
> > --- a/arch/powerpc/kernel/exceptions-64s.S
> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > @@ -329,6 +329,35 @@ 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 have to make SRR1 wake up reason look like a system reset
> > +        * interrupt. Put 0xf in there, which is reserved (and does not
> > +        * match HMI).  
> 
> The only places where the wakeup reason is presently checked on the way out
> of idle-exit are in KVM guest entry path in kvmppc_check_wake_reason()
>  and on the CPU-Hotplug exit path pnv_smp_cpu_kill_self(). In both places
> we do a positive check for EE, Doorbell, HVEE . The kvm case is also
> interested in
> HMI. We ignore all the other reasons at the moment.
> 
> So this should be fine.

Okay, thanks for confirming. We will have to be careful about this I
suppose if the wakeup reasons are expanded. I'll make a note of it
in asm/reg.h with the WAKEMASK definitions.

> > +        */
> > +       li      r11,0xf
> > +       insrdi  r12,r11,4,45  
> 
> 
> Shouldn't this be insrdi r12,r11,4,42? The exception bits are 42:45.
> I always have trouble wrapping my head around these nifty
> rotate-shift-mask-insert instructions. So I might as well be wrong!

Ah I think you're right, good catch. Maybe oris r12,r12,0x3c is a better
choice than that insrdi?


> 
> Otherwise, the patch looks correct to me.
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

Very much appreciate the reviews. I'm just getting some time to work on
the winkle count patch, so I'll repost with your suggestions when that's
done.

Thanks,
Nick