Re: [PATCH 10/13] powerpc/64s: idle simplify KVM idle on POWER9

[Nicholas Piggin <npiggin@gmail.com>]

On Tue, 8 Aug 2017 16:06:43 +0530
Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:

> Hi Nicholas,
> 
> On Sun, Aug 06, 2017 at 03:02:38AM +1000, Nicholas Piggin wrote:
> > POWER9 CPUs have independent MMU contexts per thread so KVM
> > does not have to bring sibling threads into real-mode when
> > switching MMU mode to guest. This can simplify POWER9 sleep/wake
> > paths and avoids hwsyncs.
> > 


> > @@ -444,6 +439,7 @@ ALT_FTR_SECTION_END_IFSET(CPU_FTR_ARCH_300)
> >  	beq	1f
> >  	b	kvm_start_guest
> >  1:
> > +END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)  
> 
> This would be 7 nops on power9. Should we move this to a different
> function and do a bl to that?

Yes that's a good idea.

> > +static void kvmppc_release_hwthread_secondary(int cpu)
> > +{
> > +	struct paca_struct *tpaca;
> > +
> > +	if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> > +		WARN_ON(1);
> > +		return;
> > +	}
> > +
> > +	tpaca = &paca[cpu];
> > +	tpaca->kvm_hstate.hwthread_req = 0;
> > +	kvmppc_release_hwthread(cpu);
> > +}
> > +
> > +  
> 
> Extra blank line not needed.

Sure.

> > @@ -2858,11 +2883,13 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
> > 
> >  	/* Let secondaries go back to the offline loop */
> >  	for (i = 0; i < controlled_threads; ++i) {
> > -		kvmppc_release_hwthread(pcpu + i);
> >  		if (sip && sip->napped[i])
> >  			kvmppc_ipi_thread(pcpu + i);
> >  		cpumask_clear_cpu(pcpu + i, &vc->kvm->arch.cpu_in_guest);
> >  	}  
> 
> We are sending an IPI to the thread that has exited the guest and is
> currently napping. The IPI wakes it up so that it can executes
> offline loop. But we haven't released the hwthread yet, which means
> that hwthread_req for this thread is still set.
> 
> The thread wakes up from nap, executes the pnv_powersave_wakeup code
> where it can enter kvm_start_guest. Is this a legitimate race or am I
> missing something?

Oh I think it's just a silly mistake in my patch, good catch.
Would moving this loop below the one below solve it? I wasn't
completely happy with uglifying these loops by making the
primary release different than secondary... maybe I will just
move the difference into kvmppc_release_hwthread and which is
less intrusive to callers.

Thanks,
Nick