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 15:29:26 +0200 Message-ID: <20130205132926.GJ23213@redhat.com> References: <20130204171301.GB10756@redhat.com> <20130204195952.GA15856@amt.cnet> <20130204204729.GA16442@amt.cnet> <20130205080035.GU23213@redhat.com> <20130205105438.GC23213@redhat.com> <20130205111633.GD23213@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Marcelo Tosatti , "kvm@vger.kernel.org" , "Shan, Haitao" , "Zhang, Xiantao" , "Nakajima, Jun" , "Anvin, H Peter" To: "Zhang, Yang Z" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:46842 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753914Ab3BEN33 (ORCPT ); Tue, 5 Feb 2013 08:29:29 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Feb 05, 2013 at 01:26:42PM +0000, Zhang, Yang Z wrote: > Gleb Natapov wrote on 2013-02-05: > > On Tue, Feb 05, 2013 at 10:58:28AM +0000, Zhang, Yang Z wrote: > >> Gleb Natapov wrote on 2013-02-05: > >>> On Tue, Feb 05, 2013 at 10:35:55AM +0000, Zhang, Yang Z wrote: > >>>> Gleb Natapov wrote on 2013-02-05: > >>>>> On Tue, Feb 05, 2013 at 05:57:14AM +0000, Zhang, Yang Z wrote: > >>>>>> Marcelo Tosatti wrote on 2013-02-05: > >>>>>>> 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. > >>>>>>>> > >>>>>>>>> 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. > >>>>>>> > >>>>>>> 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). > >>>>>>> } > >>>>>>> > >>>>>>> Two or more concurrent set_irq can race with each other, though. Can > >>>>>>> either document the race or add a lock. > >>>>>> According the SDM, software should not touch the IRR when target vcpu > > is > >>>>> running. Instead, use locked way to access PIR. So your solution may > >>>>> wrong. Then your apicv patches are broken, because they do exactly > >>>>> that. > >>>> Which code is broken? > >>>> > >>> The one that updates IRR directly on the apic page. > >> No, all the updates are ensuring the target vcpu is not running. So > >> it's safe to touch IRR. > >> > > Not at all. Read the code. > Sorry. I still cannot figure out which code is wrong. All the places call sync_pir_to_irr() are on target vcpu. > Can you point out the code? Thanks. > I am taking about vapic patches which are already in, not pir patches. -- Gleb.