Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1622031 > unrolled thread
| Started by | "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> |
|---|---|
| First post | 2017-04-12 13:50 +0200 |
| Last post | 2017-04-13 14:10 +0200 |
| Articles | 8 — 7 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> - 2017-04-12 13:50 +0200
Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2017-04-13 06:10 +0200
Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-04-13 06:20 +0200
Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop Michael Neuling <mikey@neuling.org> - 2017-04-13 08:30 +0200
Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop Nicholas Piggin <npiggin@gmail.com> - 2017-04-13 09:20 +0200
Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop Michael Ellerman <mpe@ellerman.id.au> - 2017-04-13 12:10 +0200
Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2017-04-13 14:00 +0200
Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop Nicholas Piggin <npiggin@gmail.com> - 2017-04-13 14:10 +0200
| From | "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-04-12 13:50 +0200 |
| Subject | [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop |
| Message-ID | <tvoWC-3Wi-9@gated-at.bofh.it> |
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) hypervisor_state_restored: -- 1.9.4
[toc] | [next] | [standalone]
| From | "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-04-13 06:10 +0200 |
| Message-ID | <tvEeZ-5SJ-3@gated-at.bofh.it> |
| In reply to | #1622031 |
"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
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Herrenschmidt <benh@kernel.crashing.org> |
|---|---|
| Date | 2017-04-13 06:20 +0200 |
| Message-ID | <tvEoF-5Wg-3@gated-at.bofh.it> |
| In reply to | #1622702 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Michael Neuling <mikey@neuling.org> |
|---|---|
| Date | 2017-04-13 08:30 +0200 |
| Message-ID | <tvGqt-7kR-3@gated-at.bofh.it> |
| In reply to | #1622703 |
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
[toc] | [prev] | [next] | [standalone]
| From | Nicholas Piggin <npiggin@gmail.com> |
|---|---|
| Date | 2017-04-13 09:20 +0200 |
| Subject | Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop |
| Message-ID | <tvHcR-7Xz-3@gated-at.bofh.it> |
| In reply to | #1622741 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Michael Ellerman <mpe@ellerman.id.au> |
|---|---|
| Date | 2017-04-13 12:10 +0200 |
| Message-ID | <tvJRo-1sn-19@gated-at.bofh.it> |
| In reply to | #1622769 |
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
[toc] | [prev] | [next] | [standalone]
| From | Gautham R Shenoy <ego@linux.vnet.ibm.com> |
|---|---|
| Date | 2017-04-13 14:00 +0200 |
| Message-ID | <tvLzP-2vl-9@gated-at.bofh.it> |
| In reply to | #1622769 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Nicholas Piggin <npiggin@gmail.com> |
|---|---|
| Date | 2017-04-13 14:10 +0200 |
| Subject | Re: [PATCH 3/3] powernv:idle: Set LPCR_UPRT on wakeup from deep-stop |
| Message-ID | <tvLJw-2Oy-9@gated-at.bofh.it> |
| In reply to | #1622952 |
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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web