From patchwork Fri Apr 27 14:19:42 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexander Graf X-Patchwork-Id: 155483 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 66733B6FAC for ; Sat, 28 Apr 2012 00:19:46 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760215Ab2D0OTp (ORCPT ); Fri, 27 Apr 2012 10:19:45 -0400 Received: from cantor2.suse.de ([195.135.220.15]:59823 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756683Ab2D0OTo convert rfc822-to-8bit (ORCPT ); Fri, 27 Apr 2012 10:19:44 -0400 Received: from relay2.suse.de (unknown [195.135.220.254]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id AD2EB936F7; Fri, 27 Apr 2012 16:19:43 +0200 (CEST) Subject: Re: [PATCH 2/2] KVM: PPC: Book3S: Call into C interrupt handlers Mime-Version: 1.0 (Apple Message framework v1257) From: Alexander Graf In-Reply-To: Date: Fri, 27 Apr 2012 16:19:42 +0200 Cc: kvm list , kvm-ppc , Benjamin Herrenschmidt Message-Id: References: <1335435543-19690-1-git-send-email-agraf@suse.de> <1335435543-19690-2-git-send-email-agraf@suse.de> <20120427054810.GC1216@drongo> To: Paul Mackerras X-Mailer: Apple Mail (2.1257) Sender: kvm-ppc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm-ppc@vger.kernel.org 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 --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: