Re: [PATCH 07/18] KVM: PPC: Book3S HV: Consolidate code that checks reason for wake from nap

[Alexander Graf <agraf@suse.de>]

On 08.01.2014, at 11:25, Paul Mackerras <paulus@samba.org> wrote:

> Currently in book3s_hv_rmhandlers.S we have three places where we
> have woken up from nap mode and we check the reason field in SRR1
> to see what event woke us up.  This consolidates them into a new
> function, kvmppc_check_wake_reason.  It looks at the wake reason
> field in SRR1, and if it indicates that an external interrupt caused
> the wakeup, calls kvmppc_read_intr to check what sort of interrupt
> it was.
> 
> This also consolidates the two places where we synthesize an external
> interrupt (0x500 vector) for the guest.  Now, if the guest exit code
> finds that there was an external interrupt which has been handled
> (i.e. it was an IPI indicating that there is now an interrupt pending
> for the guest), it jumps to deliver_guest_interrupt, which is in the
> last part of the guest entry code, where we synthesize guest external
> and decrementer interrupts.  That code has been streamlined a little
> and now clears LPCR[MER] when appropriate as well as setting it.
> 
> The extra clearing of any pending IPI on a secondary, offline CPU
> thread before going back to nap mode has been removed.  It is no longer
> necessary now that we have code to read and acknowledge IPIs in the
> guest exit path.
> 
> This fixes a minor bug in the H_CEDE real-mode handling - previously,
> if we found that other threads were already exiting the guest when we
> were about to go to nap mode, we would branch to the cede wakeup path
> and end up looking in SRR1 for a wakeup reason.  Now we branch to a
> point after we have checked the wakeup reason.
> 
> This also fixes a minor bug in kvmppc_read_intr - previously it could
> return 0xff rather than 1, in the case where we find that a host IPI
> is pending after we have cleared the IPI.  Now it returns 1.
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 192 +++++++++++++-------------------
> 1 file changed, 77 insertions(+), 115 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index c0a5d8a..61125426 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -192,8 +192,10 @@ kvm_novcpu_wakeup:
> 	stb	r0, HSTATE_NAPPING(r13)
> 	stb	r0, HSTATE_HWTHREAD_REQ(r13)
> 
> +	/* check the wake reason */
> +	bl	kvmppc_check_wake_reason
> +	
> 	/* see if any other thread is already exiting */
> -	li	r12, 0
> 	lwz	r0, VCORE_ENTRY_EXIT(r5)
> 	cmpwi	r0, 0x100
> 	bge	kvm_novcpu_exit
> @@ -203,23 +205,14 @@ kvm_novcpu_wakeup:
> 	li	r0, 1
> 	sld	r0, r0, r7
> 	addi	r6, r5, VCORE_NAPPING_THREADS
> -4:	lwarx	r3, 0, r6
> -	andc	r3, r3, r0
> -	stwcx.	r3, 0, r6
> +4:	lwarx	r7, 0, r6
> +	andc	r7, r7, r0
> +	stwcx.	r7, 0, r6
> 	bne	4b
> 
> -	/* Check the wake reason in SRR1 to see why we got here */
> -	mfspr	r3, SPRN_SRR1
> -	rlwinm	r3, r3, 44-31, 0x7	/* extract wake reason field */
> -	cmpwi	r3, 4			/* was it an external interrupt? */
> -	bne	kvm_novcpu_exit		/* if not, exit the guest */
> -
> -	/* extern interrupt - read and handle it */
> -	li	r12, BOOK3S_INTERRUPT_EXTERNAL
> -	bl	kvmppc_read_intr
> +	/* See if the wake reason means we need to exit */
> 	cmpdi	r3, 0
> 	bge	kvm_novcpu_exit
> -	li	r12, 0
> 
> 	/* Got an IPI but other vcpus aren't yet exiting, must be a latecomer */
> 	ld	r4, HSTATE_KVM_VCPU(r13)
> @@ -263,40 +256,16 @@ kvm_start_guest:
> 	 */
> 
> 	/* Check the wake reason in SRR1 to see why we got here */
> -	mfspr	r3,SPRN_SRR1
> -	rlwinm	r3,r3,44-31,0x7		/* extract wake reason field */
> -	cmpwi	r3,4			/* was it an external interrupt? */
> -	bne	27f			/* if not */
> -	ld	r5,HSTATE_XICS_PHYS(r13)
> -	li	r7,XICS_XIRR		/* if it was an external interrupt, */
> -	lwzcix	r8,r5,r7		/* get and ack the interrupt */
> -	sync
> -	clrldi.	r9,r8,40		/* get interrupt source ID. */
> -	beq	28f			/* none there? */
> -	cmpwi	r9,XICS_IPI		/* was it an IPI? */
> -	bne	29f
> -	li	r0,0xff
> -	li	r6,XICS_MFRR
> -	stbcix	r0,r5,r6		/* clear IPI */
> -	stwcix	r8,r5,r7		/* EOI the interrupt */
> -	sync				/* order loading of vcpu after that */
> +	bl	kvmppc_check_wake_reason
> +	cmpdi	r3, 0
> +	bge	kvm_no_guest
> 
> 	/* get vcpu pointer, NULL if we have no vcpu to run */
> 	ld	r4,HSTATE_KVM_VCPU(r13)
> 	cmpdi	r4,0
> 	/* if we have no vcpu to run, go back to sleep */
> 	beq	kvm_no_guest
> -	b	30f
> 
> -27:	/* XXX should handle hypervisor maintenance interrupts etc. here */
> -	b	kvm_no_guest
> -28:	/* SRR1 said external but ICP said nope?? */
> -	b	kvm_no_guest
> -29:	/* External non-IPI interrupt to offline secondary thread? help?? */
> -	stw	r8,HSTATE_SAVED_XIRR(r13)
> -	b	kvm_no_guest
> -
> -30:
> 	/* Set HSTATE_DSCR(r13) to something sensible */
> 	LOAD_REG_ADDR(r6, dscr_default)
> 	ld	r6, 0(r6)
> @@ -314,18 +283,6 @@ kvm_start_guest:
> 	 * visible we could be given another vcpu.
> 	 */
> 	lwsync
> -	/* Clear any pending IPI - we're an offline thread */
> -	ld	r5, HSTATE_XICS_PHYS(r13)
> -	li	r7, XICS_XIRR
> -	lwzcix	r3, r5, r7		/* ack any pending interrupt */
> -	rlwinm.	r0, r3, 0, 0xffffff	/* any pending? */
> -	beq	37f
> -	sync
> -	li	r0, 0xff
> -	li	r6, XICS_MFRR
> -	stbcix	r0, r5, r6		/* clear the IPI */
> -	stwcix	r3, r5, r7		/* EOI it */
> -37:	sync
> 
> 	/* increment the nap count and then go to nap mode */
> 	ld	r4, HSTATE_KVM_VCORE(r13)
> @@ -821,47 +778,46 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_206)
> 	mtctr	r6
> 	mtxer	r7
> 
> +kvmppc_cede_reentry:		/* r4 = vcpu, r13 = paca */
> 	ld	r10, VCPU_PC(r4)
> 	ld	r11, VCPU_MSR(r4)
> -kvmppc_cede_reentry:		/* r4 = vcpu, r13 = paca */
> 	ld	r6, VCPU_SRR0(r4)
> 	ld	r7, VCPU_SRR1(r4)
> +	mtspr	SPRN_SRR0, r6
> +	mtspr	SPRN_SRR1, r7
> 
> +deliver_guest_interrupt:
> 	/* r11 = vcpu->arch.msr & ~MSR_HV */
> 	rldicl	r11, r11, 63 - MSR_HV_LG, 1
> 	rotldi	r11, r11, 1 + MSR_HV_LG
> 	ori	r11, r11, MSR_ME
> 
> 	/* Check if we can deliver an external or decrementer interrupt now */
> -	ld	r0,VCPU_PENDING_EXC(r4)
> -	lis	r8,(1 << BOOK3S_IRQPRIO_EXTERNAL_LEVEL)@h
> -	and	r0,r0,r8
> -	cmpdi	cr1,r0,0
> -	andi.	r0,r11,MSR_EE
> -	beq	cr1,11f
> +	ld	r0, VCPU_PENDING_EXC(r4)
> +	rldicl	r0, r0, 64 - BOOK3S_IRQPRIO_EXTERNAL_LEVEL, 63
> +	cmpdi	cr1, r0, 0
> +	andi.	r8, r11, MSR_EE
> BEGIN_FTR_SECTION
> -	mfspr	r8,SPRN_LPCR
> -	ori	r8,r8,LPCR_MER
> -	mtspr	SPRN_LPCR,r8
> +	mfspr	r8, SPRN_LPCR
> +	/* Insert EXTERNAL_LEVEL bit into LPCR at the MER bit position */
> +	rldimi	r8, r0, LPCR_MER_SH, 63 - LPCR_MER_SH
> +	mtspr	SPRN_LPCR, r8
> 	isync
> END_FTR_SECTION_IFSET(CPU_FTR_ARCH_206)
> 	beq	5f
> -	li	r0,BOOK3S_INTERRUPT_EXTERNAL
> -12:	mr	r6,r10
> +	li	r0, BOOK3S_INTERRUPT_EXTERNAL
> +	bne	cr1, 12f
> +	mfspr	r0, SPRN_DEC
> +	cmpwi	r0, 0
> +	li	r0, BOOK3S_INTERRUPT_DECREMENTER
> +	bge	5f
> +
> +12:	mtspr	SPRN_SRR0, r10
> 	mr	r10,r0
> -	mr	r7,r11
> +	mtspr	SPRN_SRR1, r11
> 	li	r11,(MSR_ME << 1) | 1	/* synthesize MSR_SF | MSR_ME */
> 	rotldi	r11,r11,63
> -	b	5f
> -11:	beq	5f
> -	mfspr	r0,SPRN_DEC
> -	cmpwi	r0,0
> -	li	r0,BOOK3S_INTERRUPT_DECREMENTER
> -	blt	12b
> -
> -	/* Move SRR0 and SRR1 into the respective regs */
> -5:	mtspr	SPRN_SRR0, r6
> -	mtspr	SPRN_SRR1, r7
> +5:
> 
> /*
>  * Required state:
> @@ -1049,39 +1005,19 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_206)
> 	/* External interrupt, first check for host_ipi. If this is
> 	 * set, we know the host wants us out so let's do it now
> 	 */
> -do_ext_interrupt:
> 	bl	kvmppc_read_intr
> 	cmpdi	r3, 0
> 	bgt	ext_interrupt_to_host
> 
> -	/* Allright, looks like an IPI for the guest, we need to set MER */
> 	/* Check if any CPU is heading out to the host, if so head out too */
> 	ld	r5, HSTATE_KVM_VCORE(r13)
> 	lwz	r0, VCORE_ENTRY_EXIT(r5)
> 	cmpwi	r0, 0x100
> 	bge	ext_interrupt_to_host
> 
> -	/* See if there is a pending interrupt for the guest */
> -	mfspr	r8, SPRN_LPCR
> -	ld	r0, VCPU_PENDING_EXC(r9)
> -	/* Insert EXTERNAL_LEVEL bit into LPCR at the MER bit position */
> -	rldicl.	r0, r0, 64 - BOOK3S_IRQPRIO_EXTERNAL_LEVEL, 63
> -	rldimi	r8, r0, LPCR_MER_SH, 63 - LPCR_MER_SH
> -	beq	2f
> -
> -	/* And if the guest EE is set, we can deliver immediately, else
> -	 * we return to the guest with MER set
> -	 */
> -	andi.	r0, r11, MSR_EE
> -	beq	2f
> -	mtspr	SPRN_SRR0, r10
> -	mtspr	SPRN_SRR1, r11
> -	li	r10, BOOK3S_INTERRUPT_EXTERNAL
> -	li	r11, (MSR_ME << 1) | 1	/* synthesize MSR_SF | MSR_ME */
> -	rotldi	r11, r11, 63
> -2:	mr	r4, r9
> -	mtspr	SPRN_LPCR, r8
> -	b	fast_guest_return
> +	/* Return to guest after delivering any pending interrupt */
> +	mr	r4, r9
> +	b	deliver_guest_interrupt
> 
> ext_interrupt_to_host:
> 
> @@ -1890,7 +1826,6 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_206)
> 	isync
> 	li	r0,NAPPING_CEDE
> 	stb	r0,HSTATE_NAPPING(r13)
> -	mr	r4,r3
> 	lwz	r7,VCORE_ENTRY_EXIT(r5)
> 	cmpwi	r7,0x100
> 	bge	33f		/* another thread already exiting */
> @@ -1943,6 +1878,11 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_206)
> 	nap
> 	b	.
> 
> +33:	mr	r4, r3
> +	li	r3, 0
> +	li	r12, 0
> +	b	34f
> +
> kvm_end_cede:
> 	/* get vcpu pointer */
> 	ld	r4, HSTATE_KVM_VCPU(r13)
> @@ -1972,12 +1912,15 @@ kvm_end_cede:
> 	ld	r29, VCPU_GPR(R29)(r4)
> 	ld	r30, VCPU_GPR(R30)(r4)
> 	ld	r31, VCPU_GPR(R31)(r4)
> + 
> +	/* Check the wake reason in SRR1 to see why we got here */
> +	bl	kvmppc_check_wake_reason
> 
> 	/* clear our bit in vcore->napping_threads */
> -33:	ld	r5,HSTATE_KVM_VCORE(r13)
> -	lbz	r3,HSTATE_PTID(r13)
> +34:	ld	r5,HSTATE_KVM_VCORE(r13)
> +	lbz	r7,HSTATE_PTID(r13)
> 	li	r0,1
> -	sld	r0,r0,r3
> +	sld	r0,r0,r7
> 	addi	r6,r5,VCORE_NAPPING_THREADS
> 32:	lwarx	r7,0,r6
> 	andc	r7,r7,r0
> @@ -1986,23 +1929,18 @@ kvm_end_cede:
> 	li	r0,0
> 	stb	r0,HSTATE_NAPPING(r13)
> 
> -	/* Check the wake reason in SRR1 to see why we got here */
> -	mfspr	r3, SPRN_SRR1
> -	rlwinm	r3, r3, 44-31, 0x7	/* extract wake reason field */
> -	cmpwi	r3, 4			/* was it an external interrupt? */
> -	li	r12, BOOK3S_INTERRUPT_EXTERNAL
> +	/* See if the wake reason means we need to exit */
> +	stw	r12, VCPU_TRAP(r4)
> 	mr	r9, r4
> -	ld	r10, VCPU_PC(r9)
> -	ld	r11, VCPU_MSR(r9)
> -	beq	do_ext_interrupt	/* if so */
> +	cmpdi	r3, 0
> +	bgt	guest_exit_cont
> 
> 	/* see if any other thread is already exiting */
> 	lwz	r0,VCORE_ENTRY_EXIT(r5)

^

> 	cmpwi	r0,0x100
> -	blt	kvmppc_cede_reentry	/* if not go back to guest */
> +	bge	guest_exit_cont
> 
> -	/* some threads are exiting, so go to the guest exit path */
> -	b	hcall_real_fallback
> +	b	kvmppc_cede_reentry	/* if not go back to guest */
> 
> 	/* cede when already previously prodded case */
> kvm_cede_prodded:
> @@ -2033,6 +1971,29 @@ machine_check_realmode:
> 	b	fast_interrupt_c_return
> 
> /*
> + * Check the reason we woke from nap, and take appropriate action.
> + * Returns:
> + *	0 if nothing needs to be done
> + *	1 if something happened that needs to be handled by the host
> + *	-1 if there was a guest wakeup (IPI)
> + *
> + * Also sets r12 to the interrupt vector for any interrupt that needs
> + * to be handled now by the host (0x500 for external interrupt), or zero.
> + */
> +kvmppc_check_wake_reason:
> +	mfspr	r6, SPRN_SRR1
> +	rlwinm	r6, r6, 44-31, 0x7	/* extract wake reason field */
> +	cmpwi	r6, 4			/* was it an external interrupt? */
> +	li	r12, BOOK3S_INTERRUPT_EXTERNAL
> +	beq	kvmppc_read_intr	/* if so, see what it was */
> +	li	r3, 0
> +	li	r12, 0
> +	cmpwi	r6, 6			/* was it the decrementer? */
> +	beq	0f
> +	li	r3, 1			/* anything else, return 1 */
> +0:	blr

Tricky :). Please document somewhere (in a follow-up patch) which registers this function actually uses. Up there you assume that it doesn't modify r5 for example.


Alex