From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH 2/2] x86, apicv: Add Posted Interrupt supporting Date: Tue, 5 Feb 2013 09:32:50 +0200 Message-ID: <20130205073250.GT23213@redhat.com> References: <20130131133245.GB3179@amt.cnet> <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> 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: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:2290 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751595Ab3BEHcy (ORCPT ); Tue, 5 Feb 2013 02:32:54 -0500 Content-Disposition: inline In-Reply-To: <20130204204729.GA16442@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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. 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. > > 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.