diff mbox

[2/2] KVM: PPC: Book3S: Call into C interrupt handlers

Message ID CC095701-3BB5-42C5-AF3A-C569275FE69F@suse.de
State New, archived
Headers show

Commit Message

Alexander Graf April 27, 2012, 2:19 p.m. UTC 
On 27.04.2012, at 13:23, Alexander Graf wrote:

> 
> On 27.04.2012, at 07:48, Paul Mackerras wrote:
> 
>> On Thu, Apr 26, 2012 at 12:19:03PM +0200, Alexander Graf wrote:
>> 
>>> So switch the code over to call into the Linux C handlers from C code,
>>> speeding up everything along the way.
>> 
>> I have to say this patch makes me pretty uneasy.  There are a few
>> things that look wrong to me, but more than that, it seems to me that
>> there would be a lot of careful thought needed to make sure that the
>> approach is bullet-proof.
> 
> Yay, finally some review on it :). This method is currently used identically in booke hv, so everything we find broken here also applies there!
> 
>> The first thing is that you are filling in the registers, and in
>> particular r1, in a subroutine, so you are potentially making regs.r1
>> point to a stack frame that no longer exists by the time we look at it
>> inside do_IRQ or timer_interrupt.  So, for example, a stack trace
>> could go completely off the rails at that point.  Quite possibly gcc
>> will inline the kvmppc_fill_pt_regs function, which would probably
>> save you, but I don't think you should rely on that.
> 
> Ugh. Right.
> 
>> The second thing is, why do you save just r1, ip, msr, and lr?  Why
>> those ones and no others?  A performance monitor interrupt might well
>> decide to save all the registers away plus a stack trace, and to see
>> all the GPRs as 0 could be very confusing.
> 
> Well, any other state at that point is pretty useless, since we've long deferred from the original IP the interrupt arrived at. So if a perfmon module reads out other GPRs there, they are basically guaranteed to be useless anyway, no?
> 
>> Thirdly, if preemption is enabled, it could well be that the interrupt
>> will wake up a higher priority task which should run before we
>> continue.  On 64-bit you are probably saved by the soft_irq_enable
>> calls, which will (I think) call schedule() if a reschedule is
>> pending, but on 32-bit soft_irq_enable does nothing.
> 
> At that point preemption is disabled.
> 
>> Fourthly, as Ben said, you should be setting regs->trap.
> 
> Yup :). Very good catch that one.
> 
>> Have you measured a performance improvement with this patch?  If so
>> how big was it?
> 
> Yeah, I tried things on 970 in an mfsprg/mtsprg busy loop. I measured 3 different variants:
> 
> C irq handling:		1004944 exits/sec
> asm irq handling:		1001774 exits/sec
> asm + hsrr patch:		994719 exits/sec
> 
> So as you can see, that code change does have quite an impact. But maybe the added complexity isn't worth it? Either way, we should try and find a solution that works the same way for booke and book3s - I don't want such an integral part to differ all that much.

How about this patch? This at least gets rid of the hsrr performance penalty:



--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/powerpc/kvm/book3s_segment.S b/arch/powerpc/kvm/book3s_segment.S
index 6bae0a9..8b2fc66 100644
--- a/arch/powerpc/kvm/book3s_segment.S
+++ b/arch/powerpc/kvm/book3s_segment.S
@@ -198,6 +198,7 @@  kvmppc_interrupt:
 	/* Save guest PC and MSR */
 #ifdef CONFIG_PPC64
 BEGIN_FTR_SECTION
+	mr	r10, r12
 	andi.	r0,r12,0x2
 	beq	1f
 	mfspr	r3,SPRN_HSRR0
@@ -317,23 +318,17 @@  no_dcbz32_off:
 	 * Having set up SRR0/1 with the address where we want
 	 * to continue with relocation on (potentially in module
 	 * space), we either just go straight there with rfi[d],
-	 * or we jump to an interrupt handler with bctr if there
-	 * is an interrupt to be handled first.  In the latter
-	 * case, the rfi[d] at the end of the interrupt handler
-	 * will get us back to where we want to continue.
+	 * or we jump to an interrupt handler if there is an
+	 * interrupt to be handled first.  In the latter case,
+	 * the rfi[d] at the end of the interrupt handler will
+	 * get us back to where we want to continue.
 	 */
 
-	cmpwi	r12, BOOK3S_INTERRUPT_EXTERNAL
-	beq	1f
-	cmpwi	r12, BOOK3S_INTERRUPT_DECREMENTER
-	beq	1f
-	cmpwi	r12, BOOK3S_INTERRUPT_PERFMON
-1:	mtctr	r12
-
 	/* Register usage at this point:
 	 *
 	 * R1       = host R1
 	 * R2       = host R2
+	 * R10      = raw exit handler id
 	 * R12      = exit handler id
 	 * R13      = shadow vcpu (32-bit) or PACA (64-bit)
 	 * SVCPU.*  = guest *
@@ -343,12 +338,26 @@  no_dcbz32_off:
 	PPC_LL	r6, HSTATE_HOST_MSR(r13)
 	PPC_LL	r8, HSTATE_VMHANDLER(r13)
 
-	/* Restore host msr -> SRR1 */
+#ifdef CONFIG_PPC64
+BEGIN_FTR_SECTION
+	andi.	r0,r10,0x2
+	beq	1f
+	mtspr	SPRN_HSRR1, r6
+	mtspr	SPRN_HSRR0, r8
+END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
+#endif
+1:	/* Restore host msr -> SRR1 */
 	mtsrr1	r6
 	/* Load highmem handler address */
 	mtsrr0	r8
 
 	/* RFI into the highmem handler, or jump to interrupt handler */
-	beqctr
+	cmpwi	r12, BOOK3S_INTERRUPT_EXTERNAL
+	beqa	BOOK3S_INTERRUPT_EXTERNAL
+	cmpwi	r12, BOOK3S_INTERRUPT_DECREMENTER
+	beqa	BOOK3S_INTERRUPT_DECREMENTER
+	cmpwi	r12, BOOK3S_INTERRUPT_PERFMON
+	beqa	BOOK3S_INTERRUPT_PERFMON
+
 	RFI
 kvmppc_handler_trampoline_exit_end: