Re: [PATCH 2/2] powernv/powerpc: Clear PECE1 in LPCR via stop-api only on Hotplug

From: Nicholas Piggin
Date: Wed Jul 19 2017 - 01:20:54 EST


On Wed, 19 Jul 2017 10:34:59 +0530
Gautham R Shenoy <ego@xxxxxxxxxxxxxxxxxx> wrote:

> Hello Nicholas,
>
> On Wed, Jul 19, 2017 at 12:14:12PM +1000, Nicholas Piggin wrote:
> > Thanks for working on these patches. We really need to get this stuff
> > merged and tested asap :)
> >
>
> > On Tue, 18 Jul 2017 19:58:49 +0530
>
> [..snip..]
>
> > > diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
> > > index 40dae96..5f2a712 100644
> > > --- a/arch/powerpc/platforms/powernv/smp.c
> > > +++ b/arch/powerpc/platforms/powernv/smp.c
> > > @@ -143,7 +143,8 @@ static void pnv_smp_cpu_kill_self(void)
> > > {
> > > unsigned int cpu;
> > > unsigned long srr1, wmask;
> > > -
> > > + uint64_t lpcr_val;
> > > + uint64_t pir;
> > > /* Standard hot unplug procedure */
> > > /*
> > > * This hard disables local interurpts, ensuring we have no lazy
> > > @@ -164,13 +165,17 @@ static void pnv_smp_cpu_kill_self(void)
> > > if (cpu_has_feature(CPU_FTR_ARCH_207S))
> > > wmask = SRR1_WAKEMASK_P8;
> > >
> > > + pir = get_hard_smp_processor_id(cpu);
> > > /* We don't want to take decrementer interrupts while we are offline,
> > > * so clear LPCR:PECE1. We keep PECE2 (and LPCR_PECE_HVEE on P9)
> > > * enabled as to let IPIs in.
> > > */
> > > - mtspr(SPRN_LPCR, mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1);
> > > + lpcr_val = mfspr(SPRN_LPCR) & ~(u64)LPCR_PECE1;
> > > + mtspr(SPRN_LPCR, lpcr_val);
> > >
> > > while (!generic_check_cpu_restart(cpu)) {
> > > +
> > > +
> > > /*
> > > * Clear IPI flag, since we don't handle IPIs while
> > > * offline, except for those when changing micro-threading
> > > @@ -180,8 +185,15 @@ static void pnv_smp_cpu_kill_self(void)
> > > */
> > > kvmppc_set_host_ipi(cpu, 0);
> > >
> > > + /*
> > > + * If the CPU gets woken up by a special wakeup,
> > > + * ensure that the SLW engine sets LPCR with
> > > + * decrementer bit cleared, else we will get spurious
> > > + * wakeups.
> > > + */
> > > + if (cpu_has_feature(CPU_FTR_ARCH_300))
> > > + opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
> >
> > Can you put these details into pnv_cpu_offline? Possibly even from there
> > into another special SPR save function? E.g.,
>
> We could move this to pnv_cpu_offline. Except that if we do get
> spurious interrupts, we will end up programming the the LPCR via
> stop-api again which is not needed.
>
> Even this patch above can be optimized further. We need to program the
> LPCR with the PECE1 bit cleared only once, before the while loop, and
> once again program the LPCR with PECE1 bit set once we are out of the
> while loop.
>
> But then, perhaps getting spurious interrupts when the CPU is
> hotplugged is an unlikely event. So I will move this to pnv_cpu_offline.

Yeah, I think it just tidies it up a bit. You're right, but as you say
it's not really a critical path.


> > pnv_save_sprs_for_deep_state_decrementer_wakeup(bool decrementer_wakeup)
> >
> > I'd like to put the LPCR manipulation for idle wake settings into idle.c
> > as well (pnv_cpu_offline), I think it fits better in there.
> >
>
> I agree. Will respin this,test and send out the v2.

Great thanks. The first patch seemed okay to me.

I wonder if we should think about a more structured kernel API for
modifying these kind of system registers so we always have the
up-to-date values stored in memory. Many of them do need to be
restored after sleep, but they don't need to be saved per-thread
or saved every time we go to sleep.

That's far more work of course, but it might be something we want
to think about for the future.

Thanks,
Nick