Re: [RESEND][PATCH] powerpc/powernv : Save/Restore SPRG3 on entry/exit from stop.

From: Gautham R Shenoy <ego@linux.vnet.ibm.com>
To: Michael Neuling <mikey@neuling.org>
Cc: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>,
	linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
	Florian Weimer <fweimer@redhat.com>,
	Oleg Nesterov <oleg@redhat.com>
Subject: Re: [RESEND][PATCH] powerpc/powernv : Save/Restore SPRG3 on entry/exit from stop.
Date: Wed, 18 Jul 2018 13:42:49 +0530	[thread overview]
Message-ID: <20180718081249.GA17700@in.ibm.com> (raw)
In-Reply-To: <80bbdf47081e3e302ab5f28b5ddc9e2faabba842.camel@neuling.org>

Hello Mikey,

On Wed, Jul 18, 2018 at 09:24:19AM +1000, Michael Neuling wrote:
> 
> >  	DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);
> > diff --git a/arch/powerpc/kernel/idle_book3s.S
> > b/arch/powerpc/kernel/idle_book3s.S
> > index d85d551..5069d42 100644
> > --- a/arch/powerpc/kernel/idle_book3s.S
> > +++ b/arch/powerpc/kernel/idle_book3s.S
> > @@ -120,6 +120,9 @@ power9_save_additional_sprs:
> >  	mfspr	r4, SPRN_MMCR2
> >  	std	r3, STOP_MMCR1(r13)
> >  	std	r4, STOP_MMCR2(r13)
> > +
> > +	mfspr	r3, SPRN_SPRG3
> > +	std	r3, STOP_SPRG3(r13)
> 
> We don't need to save it.  Just restore it from paca->sprg_vdso which should
> never change.

Ok. I will respin a patch to restore SPRG3 from paca->sprg_vdso.

> 
> How can we do better at catching these missing SPRGs?

We can go through the list of SPRs from the POWER9 User Manual and
document explicitly why we don't have to save/restore certain SPRs
during the execution of the stop instruction. Does this sound ok ?

(Ref: Table 4-8, Section 4.7.3.4 from the POWER9 User Manual
accessible from
https://openpowerfoundation.org/?resource_lib=power9-processor-users-manual)

> 
> We missed this one and looking at c1b25a17d249 we missed the AMOR a couple of
> months back. I'd rather we had some systematic way of finding the ones we are
> missing, rather than playing wake-a-mole.

I agree, we need something more systematic than the try-catch method
thing we have now.

For deep stop states on POWER9, we looked at the list of SPRs that
were being lost and restored during winkle on POWER8. The additional
SPRs that we took care of were the ones related to the Radix and IMC.

Now since winkle was used only in the context of CPU-Hotplug, the
CPU-online code would reinit some of the SPRs such as SPRG3, which is
why we didn't see this problem on POWER8. So, that is one obvious
place to audit.

AMOR was a bad miss. It was being restored in POWER8 as part of the
subcore restore code, so like RPR, it should have been restored along
with the other per-core SPRs.

> 
> Mikey 
> 
> >  	blr
> >  
> >  power9_restore_additional_sprs:
> > @@ -144,7 +147,9 @@ power9_restore_additional_sprs:
> >  	mtspr	SPRN_MMCR1, r4
> >  
> >  	ld	r3, STOP_MMCR2(r13)
> > +	ld	r4, STOP_SPRG3(r13)
> >  	mtspr	SPRN_MMCR2, r3
> > +	mtspr	SPRN_SPRG3, r4
> >  	blr
> >  
> >  /*