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: Wed, 6 Feb 2013 20:49:23 -0200 Message-ID: <20130206224923.GA12266@amt.cnet> References: <20130131133837.GB23213@redhat.com> <20130131134443.GA4419@amt.cnet> <20130131135556.GC23213@redhat.com> <20130204005700.GA2705@amt.cnet> <20130204095553.GK23213@redhat.com> <20130204144345.GA11328@amt.cnet> <20130204171301.GB10756@redhat.com> <20130204195952.GA15856@amt.cnet> <20130204204729.GA16442@amt.cnet> <20130205073250.GT23213@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]:50209 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757907Ab3BFXBo (ORCPT ); Wed, 6 Feb 2013 18:01:44 -0500 Content-Disposition: inline In-Reply-To: <20130205073250.GT23213@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Feb 05, 2013 at 09:32:50AM +0200, Gleb Natapov wrote: > On Mon, Feb 04, 2013 at 06:47:30PM -0200, Marcelo Tosatti wrote: > > On Mon, Feb 04, 2013 at 05:59:52PM -0200, Marcelo Tosatti wrote: > > > On Mon, Feb 04, 2013 at 07:13:01PM +0200, Gleb Natapov wrote: > > > > On Mon, Feb 04, 2013 at 12:43:45PM -0200, Marcelo Tosatti wrote: > > > > > > > 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. > > > > Not sure I understand the flow of events in your description correctly. As I > > > > understand it at 4 set_irq() will return incorrect result. Basically > > > > when PIR is set to 1 while IRR has 1 for the vector the value of > > > > set_irq() will be incorrect. > > > > > > At 4 it has not been coalesced: it has been queued to IRR. > > > At 6 it has been coalesced: PIR bit merged into IRR bit. > > > > Yes, that's the case. > > > > > Frankly I do not see how it can be fixed > > > > without any race with present HW PIR design. > > > > > > At kvm_accept_apic_interrupt, check IRR before setting PIR bit, if IRR > > > already set, don't set PIR. > Need to check both IRR and PIR. Something like that: > > apic_accept_interrupt() { > if (PIR || IRR) > return coalesced; > else > set PIR; > } > > This has two problems. Firs is that interrupt that can be delivered will > be not (IRR is cleared just after it was tested), but it will be reported > as coalesced, so this is benign race. Yes, and the same condition exists today with IRR, its fine. > Second is that interrupt may be > reported as delivered, but it will be coalesced (possible only with the self > IPI with the same vector): > > Starting condition: PIR=0, IRR=0 vcpu is in a guest mode > > io thread | vcpu > accept_apic_interrupt() | > PIR and IRR is zero | > set PIR | > return delivered | > | self IPI > | set IRR > | merge PIR to IRR (*) > > At (*) interrupt that was reported as delivered is coalesced. Only vcpu itself should send self-IPI, so its fine. > > Or: > > > > apic_accept_interrupt() { > > > > 1. Read ORIG_PIR=PIR, ORIG_IRR=IRR. > > Never set IRR when HWAPIC enabled, even if outside of guest mode. > > 2. Set PIR and let HW or SW VM-entry transfer it to IRR. > > 3. set_irq return value: (ORIG_PIR or ORIG_IRR set). > > } > > > This can report interrupt as coalesced, but it will be eventually delivered > as separate interrupt: > > Starting condition: PIR=0, IRR=1 vcpu is in a guest mode > > io thread | vcpu > | > accept_apic_interrupt() | > ORIG_PIR=0, ORIG_IRR=1 | > | EOI > | clear IRR, set ISR > set PIR | > return coalesced | > | clear PIR, set IRR > | EOI > | clear IRR, set ISR (*) > > At (*) interrupt that was reported as coalesced is delivered. > > > So still no perfect solution. But first one has much less serious > problems for our practical needs. > > > Two or more concurrent set_irq can race with each other, though. Can > > either document the race or add a lock. > > > > -- > Gleb. Ok, then: accept_apic_irq: 1. coalesced = test_and_set_bit(PIR) 2. set KVM_REQ_EVENT bit (*) 3. if (vcpu->in_guest_mode) 4. if (test_and_set_bit(pir notification bit)) 5. send PIR IPI 6. return coalesced Other sites: A: On VM-entry, after disabling interrupts, but before the last check for ->requests, clear pir notification bit (unconditionally). (*) This is _necessary_ also because during VM-exit a PIR IPI interrupt can be missed, so the KVM_REQ_EVENT indicates that SW is responsible for PIR->IRR transfer.