On Wed, 19 Jul 2017 10:34:59 +0530 Gautham R Shenoy <e...@linux.vnet.ibm.com> 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