Re: [PATCH] ppc/pnv: Fix NMI system reset SRR1 value

[Nicholas Piggin]

Excerpts from Cédric Le Goater's message of May 8, 2020 3:14 am:
> On 5/7/20 1:48 PM, Nicholas Piggin wrote:
>> Commit a77fed5bd926 ("ppc/pnv: Add support for NMI interface") got the
>> SRR1 setting wrong for sresets that hit outside of power-save states.
>> 
>> Fix this, better documenting the source for the bit definitions.
>> 
>> Fixes: a77fed5bd926 ("ppc/pnv: Add support for NMI interface") got the
>> Cc: Cédric Le Goater <address@hidden>
>> Cc: David Gibson <address@hidden>
>> Signed-off-by: Nicholas Piggin <address@hidden>
> 
> We should introduce some defines like the SRR1_WAKE ones in Linux and 
> cleanup powerpc_reset_wakeup(). This function uses cryptic values. 
> That can be done later on as a followup.
> 
> Reviewed-by: Cédric Le Goater <address@hidden>

Thanks.

>> ---
>> 
>> Thanks to Cedric for pointing out concerns with a previous MCE patch
>> that unearthed this as well. Linux does not actually care what these
>> SRR1[42:45] bits look like for non-powersave sresets, but we should
>> follow documented behaviour as far as possible.
> 
> We should introduce some defines like the SRR1_WAKE ones in Linux and 
> cleanup powerpc_reset_wakeup(). This function uses cryptic values. 
> That can be done later on as a followup.
> 
> 
> I am currently after a bug which results in a CPU hard lockup because 
> of a pending interrupt. It occurs on a SMP PowerNV machine when it is 
> stressed with IO, such as scp of a big file. 
> 
> I am suspecting more and more an issue with an interrupt being handled 
> when the CPU is coming out of idle. I haven't seen anything wrong in

So you can't hit it when booting Linux with powersave=off?

Do we model stop with EC=0 properly? Looks like helper_pminsn seems to
be doing the right thing there.

> the models. Unless this maybe :
> 
>     /* Pretend to be returning from doze always as we don't lose state */
>     *msr |= (0x1ull << (63 - 47));
> 
> I am not sure how in sync it is with PSSCR.

That should be okay, the hardware can always enter a shallower state 
than was asked for. Linux will handle it. For testing purpose, we could
model deeper states by scribbling on registers and indicating state loss.

Aide from SRR1 sleep state value, Linux uses the SRR1 wake reason value 
to run the interrupt handler, but even if we got SRR1 wrong, Linux 
eventually enables MSR[EE] so the interrupt should get replayed then 
(this is what Linux used to do until we added the wake-reason processing 
for improved performance).

But we do appear to get those right in powerpc_reset_wakeup().

Thanks,
Nick