From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting Date: Mon, 4 Feb 2013 12:43:45 -0200 Message-ID: <20130204144345.GA11328@amt.cnet> References: <20130124234339.GA18719@amt.cnet> <20130130230311.GA19081@amt.cnet> <20130131094348.GN15004@redhat.com> <20130131133245.GB3179@amt.cnet> <20130131133837.GB23213@redhat.com> <20130131134443.GA4419@amt.cnet> <20130131135556.GC23213@redhat.com> <20130204005700.GA2705@amt.cnet> <20130204095553.GK23213@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "Zhang, Yang Z" , "kvm@vger.kernel.org" , "Shan, Haitao" , "Zhang, Xiantao" , "Nakajima, Jun" , "Anvin, H Peter" To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:32389 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754917Ab3BDOny (ORCPT ); Mon, 4 Feb 2013 09:43:54 -0500 Content-Disposition: inline In-Reply-To: <20130204095553.GK23213@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Feb 04, 2013 at 11:55:53AM +0200, Gleb Natapov wrote: > On Sun, Feb 03, 2013 at 10:57:00PM -0200, Marcelo Tosatti wrote: > > On Thu, Jan 31, 2013 at 03:55:56PM +0200, Gleb Natapov wrote: > > > On Thu, Jan 31, 2013 at 11:44:43AM -0200, Marcelo Tosatti wrote: > > > > On Thu, Jan 31, 2013 at 03:38:37PM +0200, Gleb Natapov wrote: > > > > > On Thu, Jan 31, 2013 at 11:32:45AM -0200, Marcelo Tosatti wrote: > > > > > > On Thu, Jan 31, 2013 at 11:43:48AM +0200, Gleb Natapov wrote: > > > > > > > On Wed, Jan 30, 2013 at 09:03:11PM -0200, Marcelo Tosatti wrote: > > > > > > > > Posted interrupt patch: > > > > > > > > 2) Must move IN_GUEST_MODE assignment after local_irq_disable, in > > > > > > > > vcpu_enter_guest function. Otherwise: > > > > > > > > > > > > > > > > cpu0 vcpu1<->cpu1 > > > > > > > > > > > > > > > > vcpu->mode = IN_GUEST_MODE > > > > > > > > > > > > > > > > if IN_GUEST_MODE == true > > > > > > > > send IPI > > > > > > > > local_irq_disable > > > > > > > > > > > > > > > > PIR not transferred to VIRR, misses interrupt. > > > > > > > > > > > > > > > cpu0 will set KVM_REQ_EVENT, so vmentry will be aborted after > > > > > > > local_irq_disable() by ->requests check. > > > > > > > > > > > > Yes, but you don't want KVM_REQ_EVENT+kick. It defeats the purpose > > > > > > of posted interrupts. You want > > > > > > > > > > > > if vcpu in guest mode > > > > > > send posted interrupt IPI > > > > > > else > > > > > > KVM_REQ_EVENT+kick > > > > > > > > > > > I am thinking: > > > > > > > > > > set KVM_REQ_EVENT > > > > > if pi is enabled > > > > > send posted interrupt IPI > > > > > else > > > > > kick > > > > > > > > KVM_REQ_EVENT must be after sending posted interrupt IPI. Otherwise on > > > > the vcpu entry side > > > > > > > > test_and_clear(KVM_REQ_EVENT) { > > > > No bits set in PIR > > > > } > > > > > > > It should be after updating PIR, but before sending posted interrupt > > > IPI. Otherwise: > > > > > > cpu0 cpu1/vcpu > > > > > > KVM_REQ_EVENT is not set > > > set pir > > > send IPI > > > irq_disable() > > > ->request is empty. > > > set KVM_REQ_EVENT > > > > > > That's the same sequence as with IRR update, KVM_REQ_EVENT and kick > > > today. > > > > Can only send IPI if vcpu->mode == IN_GUEST_MODE, which must be set > > after interrupt flag is cleared as noted. > > > > Also KVM_REQ_EVENT is processed outside of interrupt disabled region today. > But it is checked in interrupt disabled section and vcpu entry is > aborted if event is pending. > > > > > Or maybe i don't get what you say... write a complete > > description? > > > I am saying that we do not need to move vcpu->mode = IN_GUEST_MODE to > irq_disable() section to make things work. Just do: > > set bit in pir > set KVM_REQ_EVENT > if in guest mode do IPI I see. Yeah, probably. > > > > What about item 4 below? > > > > > > > That's for Yang to answer :) > > > > "If more than one interrupt is generated with the same vector number, > > the local APIC can set the bit for the vector both in the IRR and ISR. > > This means that for the Pentium 4 and Intel Xeon processors, the IRR > > and ISR can queue two interrupts for each interrupt vector: one in the > > IRR and one in the ISR. Any additional interrupts issued for the same > > interrupt vector are collapsed into the single bit in IRR" > > > > Which would mean KVM emulation differs... "Eventually 3 interrupts can > > be queued: one in IRR, one in ISR, one in PIR". > I do not think this is the case though. PIR will be folded into IRR > either during a guest entry or by vcpu itself on receiving of > notification vector IPI. > > > > > Any example how software relies on such two-interrupts-queued-in-IRR/ISR behaviour? > Don't know about guests, but KVM relies on it to detect interrupt > coalescing. So if interrupt is set in IRR but not in PIR interrupt will > not be reported as coalesced, but it will be coalesced during PIR->IRR > merge. Yes, so: 1. IRR=1, ISR=0, PIR=0. Event: set_irq, coalesced=no. 2. IRR=0, ISR=1, PIR=0. Event: IRR->ISR transfer. 3. vcpu outside of guest mode. 4. IRR=1, ISR=1, PIR=0. Event: set_irq, coalesced=no. 5. vcpu enters guest mode. 6. IRR=1, ISR=1, PIR=1. Event: set_irq, coalesced=no. 7. HW transfers PIR into IRR. set_irq return value at 7 is incorrect, interrupt event was _not_ queued.