From: Gautham R Shenoy <ego@linux•vnet.ibm.com>
To: Nicholas Piggin <npiggin@gmail•com>
Cc: "Gautham R. Shenoy" <ego@linux•vnet.ibm.com>,
Michael Ellerman <mpe@ellerman•id.au>,
Michael Neuling <mikey@neuling•org>,
Benjamin Herrenschmidt <benh@kernel•crashing.org>,
"Shreyas B. Prabhu" <shreyasbp@gmail•com>,
Shilpasri G Bhat <shilpa.bhat@linux•vnet.ibm.com>,
Vaidyanathan Srinivasan <svaidy@linux•vnet.ibm.com>,
Anton Blanchard <anton@samba•org>,
Balbir Singh <bsingharora@gmail•com>,
Akshay Adiga <akshay.adiga@linux•vnet.ibm.com>,
linuxppc-dev@lists•ozlabs.org, linux-kernel@vger•kernel.org
Subject: Re: [v2 PATCH 4/4] powernv: Recover correct PACA on wakeup from a stop on P9 DD1
Date: Wed, 22 Mar 2017 11:28:46 +0530 [thread overview]
Message-ID: <20170322055846.GB8326@in.ibm.com> (raw)
In-Reply-To: <20170321025946.3c0b68c5@roar.ozlabs.ibm.com>
On Tue, Mar 21, 2017 at 02:59:46AM +1000, Nicholas Piggin wrote:
> On Mon, 20 Mar 2017 21:24:18 +0530
> "Gautham R. Shenoy" <ego@linux•vnet.ibm.com> wrote:
>
> > From: "Gautham R. Shenoy" <ego@linux•vnet.ibm.com>
> >
> > POWER9 DD1.0 hardware has an issue due to which the SPRs of a thread
> > waking up from stop 0,1,2 with ESL=1 can endup being misplaced in the
> > core. Thus the HSPRG0 of a thread waking up from can contain the paca
> > pointer of its sibling.
> >
> > This patch implements a context recovery framework within threads of a
> > core, by provisioning space in paca_struct for saving every sibling
> > threads's paca pointers. Basically, we should be able to arrive at the
> > right paca pointer from any of the thread's existing paca pointer.
> >
> > At bootup, during powernv idle-init, we save the paca address of every
> > CPU in each one its siblings paca_struct in the slot corresponding to
> > this CPU's index in the core.
> >
> > On wakeup from a stop, the thread will determine its index in the core
> > from the lower 2 bits of the PIR register and recover its PACA pointer
> > by indexing into the correct slot in the provisioned space in the
> > current PACA.
> >
> > Furthermore, ensure that the NVGPRs are restored from the stack on the
> > way out by setting the NAPSTATELOST in paca.
>
> Thanks for expanding on this, it makes the patch easier to follow :)
>
> As noted before, I think if we use PACA_EXNMI for system reset, then
> *hopefully* there should be minimal races with the initial use of other
> thread's PACA at the start of the exception. So I'll work on getting
> that in, but it need not prevent this patch from being merged first
> IMO.
>
> > [Changelog written with inputs from svaidy@linux•vnet.ibm.com]
> >
> > Signed-off-by: Gautham R. Shenoy <ego@linux•vnet.ibm.com>
> > ---
> > arch/powerpc/include/asm/paca.h | 5 ++++
> > arch/powerpc/kernel/asm-offsets.c | 1 +
> > arch/powerpc/kernel/idle_book3s.S | 49 ++++++++++++++++++++++++++++++++++-
> > arch/powerpc/platforms/powernv/idle.c | 22 ++++++++++++++++
> > 4 files changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> > index 708c3e5..4405630 100644
> > --- a/arch/powerpc/include/asm/paca.h
> > +++ b/arch/powerpc/include/asm/paca.h
> > @@ -172,6 +172,11 @@ struct paca_struct {
> > u8 thread_mask;
> > /* Mask to denote subcore sibling threads */
> > u8 subcore_sibling_mask;
> > + /*
> > + * Pointer to an array which contains pointer
> > + * to the sibling threads' paca.
> > + */
> > + struct paca_struct *thread_sibling_pacas[8];
>
> Is 8 the right number? I wonder if we have a define for it.
Thats the maximum number of threads per core that we have had on POWER
so far.
Perhaps, I can make this
struct paca_struct **thread_sibling_pacas;
and allocate threads_per_core number of slots in
pnv_init_idle_states. Sounds ok ?
>
> > #endif
> >
> > #ifdef CONFIG_PPC_BOOK3S_64
> > diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> > index 4367e7d..6ec5016 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -727,6 +727,7 @@ int main(void)
> > OFFSET(PACA_THREAD_IDLE_STATE, paca_struct, thread_idle_state);
> > OFFSET(PACA_THREAD_MASK, paca_struct, thread_mask);
> > OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask);
> > + OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas);
> > #endif
> >
> > DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);
> > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> > index 9957287..c2f2cfb 100644
> > --- a/arch/powerpc/kernel/idle_book3s.S
> > +++ b/arch/powerpc/kernel/idle_book3s.S
> > @@ -375,6 +375,46 @@ _GLOBAL(power9_idle_stop)
> > li r4,1
> > b pnv_powersave_common
> > /* No return */
> > +
> > +
> > +/*
> > + * On waking up from stop 0,1,2 with ESL=1 on POWER9 DD1,
> > + * HSPRG0 will be set to the HSPRG0 value of one of the
> > + * threads in this core. Thus the value we have in r13
> > + * may not be this thread's paca pointer.
> > + *
> > + * Fortunately, the PIR remains invariant. Since this thread's
> > + * paca pointer is recorded in all its sibling's paca, we can
> > + * correctly recover this thread's paca pointer if we
> > + * know the index of this thread in the core.
> > + *
> > + * This index can be obtained from the lower two bits of the PIR.
> > + *
> > + * i.e, thread's position in the core = PIR[62:63].
> > + * If this value is i, then this thread's paca is
> > + * paca->thread_sibling_pacas[i].
> > + */
> > +power9_dd1_recover_paca:
> > + mfspr r4, SPRN_PIR
> > + clrldi r4, r4, 62
>
> Does SPRN_TIR work?
I wasn't aware of SPRN_TIR!
I can check this. If my reading of the ISA is correct, TIR should
contain the thread number which are in the range [0..3].
>
> > + /*
> > + * Since each entry in thread_sibling_pacas is 8 bytes
> > + * we need to left-shift by 3 bits. Thus r4 = i * 8
> > + */
> > + sldi r4, r4, 3
> > + /* Get &paca->thread_sibling_pacas[0] in r5 */
> > + addi r5, r13, PACA_SIBLING_PACA_PTRS
> > + /* Load paca->thread_sibling_pacas[i] into r13 */
> > + ldx r13, r4, r5
> > + SET_PACA(r13)
> > + /*
> > + * Indicate that we have lost NVGPR state
> > + * which needs to be restored from the stack.
> > + */
> > + li r3, 1
> > + stb r0,PACA_NAPSTATELOST(r13)
> > + blr
> > +
> > /*
> > * Called from reset vector. Check whether we have woken up with
> > * hypervisor state loss. If yes, restore hypervisor state and return
> > @@ -385,7 +425,14 @@ _GLOBAL(power9_idle_stop)
> > */
> > _GLOBAL(pnv_restore_hyp_resource)
> > BEGIN_FTR_SECTION
> > - ld r2,PACATOC(r13);
> > +BEGIN_FTR_SECTION_NESTED(70)
> > + mflr r6
> > + bl power9_dd1_recover_paca
> > + mtlr r6
> > + ld r2, PACATOC(r13)
> > +FTR_SECTION_ELSE_NESTED(70)
> > + ld r2, PACATOC(r13)
> > +ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_POWER9_DD1, 70)
>
> This is quite neat now you've moved it to its own function. Nice.
> It will be only a trivial clash with my patches now, I think.
Sure!
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail•com>
>
Thanks for reviewing the patch.
--
Thanks and Regards
gautham.
next prev parent reply other threads:[~2017-03-22 5:59 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-20 15:54 [v2 PATCH 0/4] powernv:idle: Fixes for CPU-Hotplug on POWER DD1.0 Gautham R. Shenoy
2017-03-20 15:54 ` [v2 PATCH 1/4] powernv: Move CPU-Offline idle state invocation from smp.c to idle.c Gautham R. Shenoy
2017-03-20 16:35 ` Nicholas Piggin
2017-03-22 5:41 ` Gautham R Shenoy
2017-03-20 15:54 ` [v2 PATCH 2/4] powernv:smp: Add busy-wait loop as fall back for CPU-Hotplug Gautham R. Shenoy
2017-03-20 15:54 ` [v2 PATCH 3/4] powernv:idle: Don't override default/deepest directly in kernel Gautham R. Shenoy
2017-03-20 16:39 ` Nicholas Piggin
2017-03-22 5:43 ` Gautham R Shenoy
2017-03-20 15:54 ` [v2 PATCH 4/4] powernv: Recover correct PACA on wakeup from a stop on P9 DD1 Gautham R. Shenoy
2017-03-20 16:59 ` Nicholas Piggin
2017-03-22 3:29 ` Nicholas Piggin
2017-03-22 5:58 ` Gautham R Shenoy [this message]
2017-03-22 8:30 ` Nicholas Piggin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170322055846.GB8326@in.ibm.com \
--to=ego@linux$(echo .)vnet.ibm.com \
--cc=akshay.adiga@linux$(echo .)vnet.ibm.com \
--cc=anton@samba$(echo .)org \
--cc=benh@kernel$(echo .)crashing.org \
--cc=bsingharora@gmail$(echo .)com \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=mikey@neuling$(echo .)org \
--cc=mpe@ellerman$(echo .)id.au \
--cc=npiggin@gmail$(echo .)com \
--cc=shilpa.bhat@linux$(echo .)vnet.ibm.com \
--cc=shreyasbp@gmail$(echo .)com \
--cc=svaidy@linux$(echo .)vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox