| Message ID | 9be8410e0abe5ae1afa16a6f987c53046ef51757.1491996797.git.ego@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
"Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes: > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> > > On wakeup from a deep-stop used for CPU-Hotplug, we invoke > cur_cpu_spec->cpu_restore() which would set sane default values to > various SPRs including LPCR. > > On POWER9, the cpu_restore_power9() call would would restore LPCR to a > sane value that is set at early boot time, thereby clearing LPCR_UPRT. > > However, LPCR_UPRT is required to be set if we are running in Radix > mode. If this is not set we will end up with a crash when we enable > IR,DR. > > To fix this, after returning from cur_cpu_spec->cpu_restore() in the > idle exit path, set LPCR_UPRT if we are running in Radix mode. > > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> > --- > arch/powerpc/kernel/idle_book3s.S | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S > index 6a9bd28..39a9b63 100644 > --- a/arch/powerpc/kernel/idle_book3s.S > +++ b/arch/powerpc/kernel/idle_book3s.S > @@ -804,6 +804,19 @@ no_segments: > #endif > mtctr r12 > bctrl > +/* > + * cur_cpu_spec->cpu_restore would restore LPCR to a > + * sane value that is set at early boot time, > + * thereby clearing LPCR_UPRT. > + * LPCR_UPRT is required if we are running in Radix mode. > + * Set it here if that be the case. > + */ > +BEGIN_MMU_FTR_SECTION > + mfspr r3, SPRN_LPCR > + LOAD_REG_IMMEDIATE(r4, LPCR_UPRT) > + or r3, r3, r4 > + mtspr SPRN_LPCR, r3 > +END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) What about LPCR_HR ? > > hypervisor_state_restored: > > -- > 1.9.4 -aneesh
On Thu, 2017-04-13 at 09:28 +0530, Aneesh Kumar K.V wrote: > > #endif > > mtctr r12 > > bctrl > > +/* > > + * cur_cpu_spec->cpu_restore would restore LPCR to a > > + * sane value that is set at early boot time, > > + * thereby clearing LPCR_UPRT. > > + * LPCR_UPRT is required if we are running in Radix mode. > > + * Set it here if that be the case. > > + */ > > +BEGIN_MMU_FTR_SECTION > > + mfspr r3, SPRN_LPCR > > + LOAD_REG_IMMEDIATE(r4, LPCR_UPRT) > > + or r3, r3, r4 > > + mtspr SPRN_LPCR, r3 > > +END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) We are probably better off saving the value somewhere during boot and just "blasting" it whole back. Cheers Ben.
On Thu, 2017-04-13 at 14:12 +1000, Benjamin Herrenschmidt wrote: > On Thu, 2017-04-13 at 09:28 +0530, Aneesh Kumar K.V wrote: > > > #endif > > > mtctr r12 > > > bctrl > > > +/* > > > + * cur_cpu_spec->cpu_restore would restore LPCR to a > > > + * sane value that is set at early boot time, > > > + * thereby clearing LPCR_UPRT. > > > + * LPCR_UPRT is required if we are running in Radix mode. > > > + * Set it here if that be the case. > > > + */ > > > +BEGIN_MMU_FTR_SECTION > > > + mfspr r3, SPRN_LPCR > > > + LOAD_REG_IMMEDIATE(r4, LPCR_UPRT) > > > + or r3, r3, r4 > > > + mtspr SPRN_LPCR, r3 > > > +END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) > > We are probably better off saving the value somewhere during boot > and just "blasting" it whole back. We seem to touch LPCR in a bunch of places these days. Not sure when "sometimes during boot" should actually be. Mikey
On Thu, 13 Apr 2017 16:27:34 +1000 Michael Neuling <mikey@neuling.org> wrote: > On Thu, 2017-04-13 at 14:12 +1000, Benjamin Herrenschmidt wrote: > > On Thu, 2017-04-13 at 09:28 +0530, Aneesh Kumar K.V wrote: > > > > #endif > > > > mtctr r12 > > > > bctrl > > > > +/* > > > > + * cur_cpu_spec->cpu_restore would restore LPCR to a > > > > + * sane value that is set at early boot time, > > > > + * thereby clearing LPCR_UPRT. > > > > + * LPCR_UPRT is required if we are running in Radix mode. > > > > + * Set it here if that be the case. > > > > + */ > > > > +BEGIN_MMU_FTR_SECTION > > > > + mfspr r3, SPRN_LPCR > > > > + LOAD_REG_IMMEDIATE(r4, LPCR_UPRT) > > > > + or r3, r3, r4 > > > > + mtspr SPRN_LPCR, r3 > > > > +END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) > > > > We are probably better off saving the value somewhere during boot > > and just "blasting" it whole back. > > We seem to touch LPCR in a bunch of places these days. Not sure when "sometimes > during boot" should actually be. In the short term, what if we just save LPCR and restore it after calling cpu_restore? As you say there are a lot of things that touch LPCR we're not catching here.
Nicholas Piggin <npiggin@gmail.com> writes: > On Thu, 13 Apr 2017 16:27:34 +1000 > Michael Neuling <mikey@neuling.org> wrote: > >> On Thu, 2017-04-13 at 14:12 +1000, Benjamin Herrenschmidt wrote: >> > On Thu, 2017-04-13 at 09:28 +0530, Aneesh Kumar K.V wrote: >> > > > #endif >> > > > mtctr r12 >> > > > bctrl >> > > > +/* >> > > > + * cur_cpu_spec->cpu_restore would restore LPCR to a >> > > > + * sane value that is set at early boot time, >> > > > + * thereby clearing LPCR_UPRT. >> > > > + * LPCR_UPRT is required if we are running in Radix mode. >> > > > + * Set it here if that be the case. >> > > > + */ >> > > > +BEGIN_MMU_FTR_SECTION >> > > > + mfspr r3, SPRN_LPCR >> > > > + LOAD_REG_IMMEDIATE(r4, LPCR_UPRT) >> > > > + or r3, r3, r4 >> > > > + mtspr SPRN_LPCR, r3 >> > > > +END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) >> > >> > We are probably better off saving the value somewhere during boot >> > and just "blasting" it whole back. >> >> We seem to touch LPCR in a bunch of places these days. Not sure when "sometimes >> during boot" should actually be. > > In the short term, what if we just save LPCR and restore it after calling > cpu_restore? As you say there are a lot of things that touch LPCR we're > not catching here. Yeah can we save it on the way down and restore that value on the way back up? The real problem here is that cpu_restore() does not "restore" anything, it programs a set of fixed values. We should probably rework it so that it actually does a save/restore to avoid more bugs like this. cheers
On Thu, Apr 13, 2017 at 05:18:17PM +1000, Nicholas Piggin wrote: > On Thu, 13 Apr 2017 16:27:34 +1000 > Michael Neuling <mikey@neuling.org> wrote: > > > On Thu, 2017-04-13 at 14:12 +1000, Benjamin Herrenschmidt wrote: > > > On Thu, 2017-04-13 at 09:28 +0530, Aneesh Kumar K.V wrote: > > > > > #endif > > > > > mtctr r12 > > > > > bctrl > > > > > +/* > > > > > + * cur_cpu_spec->cpu_restore would restore LPCR to a > > > > > + * sane value that is set at early boot time, > > > > > + * thereby clearing LPCR_UPRT. > > > > > + * LPCR_UPRT is required if we are running in Radix mode. > > > > > + * Set it here if that be the case. > > > > > + */ > > > > > +BEGIN_MMU_FTR_SECTION > > > > > + mfspr r3, SPRN_LPCR > > > > > + LOAD_REG_IMMEDIATE(r4, LPCR_UPRT) > > > > > + or r3, r3, r4 > > > > > + mtspr SPRN_LPCR, r3 > > > > > +END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) > > > > > > We are probably better off saving the value somewhere during boot > > > and just "blasting" it whole back. > > > > We seem to touch LPCR in a bunch of places these days. Not sure when "sometimes > > during boot" should actually be. > > In the short term, what if we just save LPCR and restore it after calling > cpu_restore? As you say there are a lot of things that touch LPCR we're > not catching here. In that case can we skip calling cpu_restore in the idle_exit path altogether and simply restore LPCR to the value that the thread had before executing stop ? > -- Thanks and Regards gautham.
On Thu, 13 Apr 2017 17:24:34 +0530 Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote: > On Thu, Apr 13, 2017 at 05:18:17PM +1000, Nicholas Piggin wrote: > > On Thu, 13 Apr 2017 16:27:34 +1000 > > Michael Neuling <mikey@neuling.org> wrote: > > > > > On Thu, 2017-04-13 at 14:12 +1000, Benjamin Herrenschmidt wrote: > > > > On Thu, 2017-04-13 at 09:28 +0530, Aneesh Kumar K.V wrote: > > > > > > #endif > > > > > > mtctr r12 > > > > > > bctrl > > > > > > +/* > > > > > > + * cur_cpu_spec->cpu_restore would restore LPCR to a > > > > > > + * sane value that is set at early boot time, > > > > > > + * thereby clearing LPCR_UPRT. > > > > > > + * LPCR_UPRT is required if we are running in Radix mode. > > > > > > + * Set it here if that be the case. > > > > > > + */ > > > > > > +BEGIN_MMU_FTR_SECTION > > > > > > + mfspr r3, SPRN_LPCR > > > > > > + LOAD_REG_IMMEDIATE(r4, LPCR_UPRT) > > > > > > + or r3, r3, r4 > > > > > > + mtspr SPRN_LPCR, r3 > > > > > > +END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) > > > > > > > > We are probably better off saving the value somewhere during boot > > > > and just "blasting" it whole back. > > > > > > We seem to touch LPCR in a bunch of places these days. Not sure when "sometimes > > > during boot" should actually be. > > > > In the short term, what if we just save LPCR and restore it after calling > > cpu_restore? As you say there are a lot of things that touch LPCR we're > > not catching here. > > In that case can we skip calling cpu_restore in the idle_exit path > altogether and simply restore LPCR to the value that the thread had > before executing stop ? Good question. For a minimal fix I would keep calling cpu_restore. I'd like to get rid of it if we can though, but that might take a bit more work.
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S index 6a9bd28..39a9b63 100644 --- a/arch/powerpc/kernel/idle_book3s.S +++ b/arch/powerpc/kernel/idle_book3s.S @@ -804,6 +804,19 @@ no_segments: #endif mtctr r12 bctrl +/* + * cur_cpu_spec->cpu_restore would restore LPCR to a + * sane value that is set at early boot time, + * thereby clearing LPCR_UPRT. + * LPCR_UPRT is required if we are running in Radix mode. + * Set it here if that be the case. + */ +BEGIN_MMU_FTR_SECTION + mfspr r3, SPRN_LPCR + LOAD_REG_IMMEDIATE(r4, LPCR_UPRT) + or r3, r3, r4 + mtspr SPRN_LPCR, r3 +END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_RADIX) hypervisor_state_restored: