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:43:07 +0200 Message-ID: <20130205134307.GL23213@redhat.com> References: <20130204204729.GA16442@amt.cnet> <20130205080035.GU23213@redhat.com> <20130205105438.GC23213@redhat.com> <20130205111633.GD23213@redhat.com> <20130205132926.GJ23213@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]:12225 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753862Ab3BENnJ (ORCPT ); Tue, 5 Feb 2013 08:43:09 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Feb 05, 2013 at 01:40:44PM +0000, Zhang, Yang Z wrote: > Gleb Natapov wrote on 2013-02-05: > > 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. > Yes, but the issue will be fixed with pir patches. With posted interrupt, it will touch PIR instead IRR and access PIR is allowed by HW. > So the current code is broken? -- Gleb.