From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH v7 2/3] x86, apicv: add virtual interrupt delivery support Date: Thu, 20 Dec 2012 21:07:47 -0200 Message-ID: <20121220230747.GB18767@amt.cnet> References: <1355722250-7122-1-git-send-email-yang.z.zhang@intel.com> <1355722250-7122-3-git-send-email-yang.z.zhang@intel.com> <20121220005936.GA21033@amt.cnet> <20121220064206.GC17584@redhat.com> <20121220125316.GA7750@amt.cnet> <20121220131232.GG17584@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Yang Zhang , kvm@vger.kernel.org, haitao.shan@intel.com, Kevin Tian To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:31631 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752061Ab2LTXH4 (ORCPT ); Thu, 20 Dec 2012 18:07:56 -0500 Content-Disposition: inline In-Reply-To: <20121220131232.GG17584@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Dec 20, 2012 at 03:12:32PM +0200, Gleb Natapov wrote: > On Thu, Dec 20, 2012 at 10:53:16AM -0200, Marcelo Tosatti wrote: > > On Thu, Dec 20, 2012 at 08:42:06AM +0200, Gleb Natapov wrote: > > > On Wed, Dec 19, 2012 at 10:59:36PM -0200, Marcelo Tosatti wrote: > > > > On Mon, Dec 17, 2012 at 01:30:49PM +0800, Yang Zhang wrote: > > > > > From: Yang Zhang > > > > > > > > > > Virtual interrupt delivery avoids KVM to inject vAPIC interrupts > > > > > manually, which is fully taken care of by the hardware. This needs > > > > > some special awareness into existing interrupr injection path: > > > > > > > > > > - for pending interrupt, instead of direct injection, we may need > > > > > update architecture specific indicators before resuming to guest. > > > > > > > > > > - A pending interrupt, which is masked by ISR, should be also > > > > > considered in above update action, since hardware will decide > > > > > when to inject it at right time. Current has_interrupt and > > > > > get_interrupt only returns a valid vector from injection p.o.v. > > > > > > > > > > Signed-off-by: Kevin Tian > > > > > Signed-off-by: Yang Zhang > > > > > > > > > > > > Resuming previous discussion: > > > > > > > > > > How about to recaculate irr_pending according the VIRR on each > > > > > > vmexit? > > > > > > > > > > > No need really. Since HW can only clear VIRR the only situation that > > > > > may > > > > > happen is that irr_pending will be true but VIRR is empty and > > > > > apic_find_highest_irr() will return correct result in this case. > > > > > > > > Self-IPI does cause VIRR to be set, see "29.1.5 Self-IPI > > > > Virtualization". > > > > > > > True. But as I said later in that discussion once irr_pending is set > > > to true it never becomes false, so the optimization is effectively > > > disable. We can set it to true doing apic initialization to make it > > > explicit. > > > > Its just confusing, to have a variable which has different meanings > > in different configurations. I would rather have it explicit that > > its not used rather than check every time the i read the code. > > > > if (apic_vid() == 0 && !apic->irr_pending) > > return -1; > > > I'd prefer to avoid this additional if() especially as its sole purpose > is documentation. We can add comment instead. Note that irr_pending > is just a hint anyway. It can be true when no interrupt is pending in > irr. We can even rename it to irr_pending_hint or something. Works for me (documentation). > > Not sure if you can skip it, its probably necessary to calculate it > > before HW does so (say migration etc). > kvm_apic_has_interrupt() is not called during migration and > kvm_apic_post_state_restore() calls apic_update_ppr() explicitly. > I am not sure it is needed though since migrated value should be already > correct anyway. Ok, best force isr_count to 1 if apic vintr enabled (and add a comment, please).