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: Fri, 8 Feb 2013 11:46:16 -0200 Message-ID: <20130208134616.GA18427@amt.cnet> References: <20130204095553.GK23213@redhat.com> <20130204144345.GA11328@amt.cnet> <20130204171301.GB10756@redhat.com> <20130204195952.GA15856@amt.cnet> <20130204204729.GA16442@amt.cnet> <20130205073250.GT23213@redhat.com> <20130206224923.GA12266@amt.cnet> <20130207140111.GL7837@redhat.com> <20130207214947.GA30619@amt.cnet> <20130208122844.GD19412@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]:3837 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757096Ab3BHNqn (ORCPT ); Fri, 8 Feb 2013 08:46:43 -0500 Content-Disposition: inline In-Reply-To: <20130208122844.GD19412@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Feb 08, 2013 at 02:28:44PM +0200, Gleb Natapov wrote: > On Thu, Feb 07, 2013 at 07:49:47PM -0200, Marcelo Tosatti wrote: > > On Thu, Feb 07, 2013 at 04:01:11PM +0200, Gleb Natapov wrote: > > > On Wed, Feb 06, 2013 at 08:49:23PM -0200, Marcelo Tosatti wrote: > > > > > 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. > > > > > > > It is fine only because this is not happening in practice (I hope) for single interrupt > > > we care about. Otherwise this is serious problem. > > > > Coalesced information is only interesting for non IPI cases, that > > is, device emulation (at the moment, at least). > > > And incorrect result will be returned for an interrupt injected by an emulated device > in the scenario above. > > > The above cause can happen when loading APIC registers, but delivered > > is not interesting in that case. Good to document, however. > > > > > > > > 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 > > > Do not see how it will help. > > > > > > Starting condition: PIR=0, IRR=1 vcpu is in a guest mode > > > > > > io thread | vcpu > > > accept_apic_interrupt() | > > > coalesced = 0, PIR=1 | > > > vcpu in a guest mode: | > > > send PIR IPI | > > > | receive PIR IPI > > > | merge PIR to IRR (*) > > > return not coalesced | > > > > > > At (*) interrupt that was reported as delivered is coalesced. > > > > Of course! > > > > > The point is that we need to check PIR and IRR atomically and this is > > > impossible. > > > > Ok, next try: > > > > 1. orig_irr = read irr from vapic page > > 2. if (orig_irr == 0) > > 3. return test_and_test_bit(pir) > > 4. return 0 > > > I think this is exactly same solution we are discussing above: > > apic_accept_interrupt() { > if (PIR || IRR) > return coalesced; > else > set PIR; > } Its not exactly the same. Without test_and_set_bit(PIR) two vcpus can race. > with the same self-IPI problem. IMO this is the best we can do and will > work correctly for RTC interrupt re-injection case. Yes.