Re: [PATCH 0/2] powerpc/xive: fix CPU hot unplug

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3994 bytes --]

On Tue, Oct 03, 2017 at 08:24:07AM +0200, Cédric Le Goater wrote:
> On 10/03/2017 05:36 AM, David Gibson wrote:
> > On Mon, Oct 02, 2017 at 06:27:20PM +0200, Cédric Le Goater wrote:
> >> On 09/23/2017 10:26 AM, Cédric Le Goater wrote:
> >>> Hi,
> >>>
> >>> Here are a couple of small fixes to support CPU hot unplug. There are
> >>> still some issues to be investigated as, in some occasions, after a
> >>> couple of plug and unplug, the cpu which was removed receives a 'lost'
> >>> interrupt. This showed to be the decrementer under QEMU.
> >>
> >> So this seems to be a QEMU issue only which can be solved by 
> >> removing the DEE bit from the LPCR on P9 processor when the CPU 
> >> is stopped in rtas. PECE3 bit on P8 processors. 
> >>
> >> I think these patches are valuable fixes for 4.14. The first 
> >> is trivial and the second touches the common xive part but it
> >> is only called on the pseries platform.  
> >>
> >> Could you please take a look ?
> > 
> > Sorry, I think I've missed something here.
> > 
> > Is there a qemu bug involved in this?  Has there been a patch sent
> > that I didn't spot?
> 
> 
> No, not yet, but I will today probably. something like below to stop
> the decrementer when a CPU is stopped:
> 
> 	--- qemu.git.orig/hw/ppc/spapr_rtas.c
> 	+++ qemu.git/hw/ppc/spapr_rtas.c
> 	@@ -174,6 +174,15 @@ static void rtas_start_cpu(PowerPCCPU *c
> 	         kvm_cpu_synchronize_state(cs);
> 	 
> 	         env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> 	+
> 	+        /* Enable DECR interrupt */
> 	+        if (env->mmu_model == POWERPC_MMU_3_00) {
> 	+            env->spr[SPR_LPCR] |= LPCR_DEE;
> 	+        } else {
> 	+            /* P7 and P8 both have same bit for DECR */
> 	+            env->spr[SPR_LPCR] |= LPCR_P8_PECE3;
> 	+        }
> 	+
> 	         env->nip = start;
> 	         env->gpr[3] = r3;
> 	         cs->halted = 0;
> 	@@ -210,6 +219,13 @@ static void rtas_stop_self(PowerPCCPU *c
> 	      * no need to bother with specific bits, we just clear it.
> 	      */
> 	     env->msr = 0;
> 	+
> 	+    if (env->mmu_model == POWERPC_MMU_3_00) {
> 	+        env->spr[SPR_LPCR] &= ~LPCR_DEE;
> 	+    } else {
> 	+        /* P7 and P8 both have same bit for DECR */
> 	+        env->spr[SPR_LPCR] &= ~LPCR_P8_PECE3;
> 	+    }
> 	 }
> 	 
> 	 static inline int sysparm_st(target_ulong addr, target_ulong len,
> 	
> I haven't yet because I fail to understand why the decrementer is not 
> interrupting the dying CPU under xics as it is the case under XIVE.

Oh.. ok.  This sounds very similar to the problem Nikunj hit under TCG
with decrementer interrupts waking up a supposedly dead CPU.  He had a
couple of proposed fixes, but we got bogged down trying to work out
why  (with TCG at least) it only seemed to bite after a system_reset,
and not on initial boot up.

> Also I am not sure this hack is of any use :
> 
>     /*
>      * While stopping a CPU, the guest calls H_CPPR which
>      * effectively disables interrupts on XICS level.
>      * However decrementer interrupts in TCG can still
>      * wake the CPU up so here we disable interrupts in MSR
>      * as well.
>      * As rtas_start_cpu() resets the whole MSR anyway, there is
>      * no need to bother with specific bits, we just clear it.
>      */
>     env->msr = 0;

Ok.. why do you think this isn't of use?  I'm pretty sure this is
necessary for the TCG case, since MSR is checked in cpu_has_work(),
which could otherwise wake up the "dead" cpu.

> and the different CPU states are confusing. Nikunj already to a look
> at this when trying to fix the TCG reboot. Anyway, the QEMU patch 
> should (re)start a thread. This is not the place to discuss.
> 
> Thanks,
> 
> C.  
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]