From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753952AbbCaBNY (ORCPT ); Mon, 30 Mar 2015 21:13:24 -0400 Received: from mga03.intel.com ([134.134.136.65]:33033 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753543AbbCaBNV convert rfc822-to-8bit (ORCPT ); Mon, 30 Mar 2015 21:13:21 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,497,1422950400"; d="scan'208";a="700508334" From: "Wu, Feng" To: Marcelo Tosatti CC: "hpa@zytor.com" , "tglx@linutronix.de" , "mingo@redhat.com" , "x86@kernel.org" , "gleb@kernel.org" , "pbonzini@redhat.com" , "dwmw2@infradead.org" , "joro@8bytes.org" , "alex.williamson@redhat.com" , "jiang.liu@linux.intel.com" , "eric.auger@linaro.org" , "linux-kernel@vger.kernel.org" , "iommu@lists.linux-foundation.org" , "kvm@vger.kernel.org" , "Wu, Feng" Subject: RE: [v3 24/26] KVM: Update Posted-Interrupts Descriptor when vCPU is blocked Thread-Topic: [v3 24/26] KVM: Update Posted-Interrupts Descriptor when vCPU is blocked Thread-Index: AQHQa0UnP5NczCVPzkGKMOL7AgXv+501yPkw Date: Tue, 31 Mar 2015 01:13:14 +0000 Message-ID: References: <20150226234048.GA7784@amt.cnet> <20150304120608.GA26762@amt.cnet> <20150312011526.GA5878@amt.cnet> <20150325231738.GA7333@amt.cnet> <20150327193013.GA6502@amt.cnet> <20150330235533.GA15792@amt.cnet> In-Reply-To: <20150330235533.GA15792@amt.cnet> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: Marcelo Tosatti [mailto:mtosatti@redhat.com] > Sent: Tuesday, March 31, 2015 7:56 AM > To: Wu, Feng > Cc: hpa@zytor.com; tglx@linutronix.de; mingo@redhat.com; x86@kernel.org; > gleb@kernel.org; pbonzini@redhat.com; dwmw2@infradead.org; > joro@8bytes.org; alex.williamson@redhat.com; jiang.liu@linux.intel.com; > eric.auger@linaro.org; linux-kernel@vger.kernel.org; > iommu@lists.linux-foundation.org; kvm@vger.kernel.org > Subject: Re: [v3 24/26] KVM: Update Posted-Interrupts Descriptor when vCPU is > blocked > > On Mon, Mar 30, 2015 at 04:46:55AM +0000, Wu, Feng wrote: > > > > > > > -----Original Message----- > > > From: Marcelo Tosatti [mailto:mtosatti@redhat.com] > > > Sent: Saturday, March 28, 2015 3:30 AM > > > To: Wu, Feng > > > Cc: hpa@zytor.com; tglx@linutronix.de; mingo@redhat.com; > x86@kernel.org; > > > gleb@kernel.org; pbonzini@redhat.com; dwmw2@infradead.org; > > > joro@8bytes.org; alex.williamson@redhat.com; jiang.liu@linux.intel.com; > > > eric.auger@linaro.org; linux-kernel@vger.kernel.org; > > > iommu@lists.linux-foundation.org; kvm@vger.kernel.org > > > Subject: Re: [v3 24/26] KVM: Update Posted-Interrupts Descriptor when > vCPU > > > is blocked > > > > > > On Fri, Mar 27, 2015 at 06:34:14AM +0000, Wu, Feng wrote: > > > > > > Currently, the following code is executed before local_irq_disable() is > > > called, > > > > > > so do you mean 1)moving local_irq_disable() to the place before it. 2) > after > > > > > interrupt > > > > > > is disabled, set KVM_REQ_EVENT in case the ON bit is set? > > > > > > > > > > 2) after interrupt is disabled, set KVM_REQ_EVENT in case the ON bit > > > > > is set. > > > > > > > > Here is my understanding about your comments here: > > > > - Disable interrupts > > > > - Check 'ON' > > > > - Set KVM_REQ_EVENT if 'ON' is set > > > > > > > > Then we can put the above code inside " if > > > (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) " > > > > just like it used to be. However, I still have some questions about this > > > comment: > > > > > > > > 1. Where should I set KVM_REQ_EVENT? In function vcpu_enter_guest(), > or > > > other places? > > > > > > See below: > > > > > > > If in vcpu_enter_guest(), since currently local_irq_disable() is called after > > > 'KVM_REQ_EVENT' > > > > is checked, is it helpful to set KVM_REQ_EVENT after local_irq_disable() is > > > called? > > > > > > local_irq_disable(); > > > > > > *** add code here *** > > > > So we need add code like the following here, right? > > > > if ('ON' is set) > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > Yes. > > > > if (vcpu->mode == EXITING_GUEST_MODE || vcpu->requests > > > ^^^^^^^^^^^^^^ > > Point *1. > > > > || need_resched() || signal_pending(current)) { > > > vcpu->mode = OUTSIDE_GUEST_MODE; > > > smp_wmb(); > > > local_irq_enable(); > > > preempt_enable(); > > > vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); > > > r = 1; > > > goto cancel_injection; > > > } > > > > > > > 2. 'ON' is set by VT-d hardware, it can be set even when interrupt is > disabled > > > (the related bit in PIR is also set). > > > > > > Yes, we are checking if the HW has set an interrupt in PIR while > > > outside VM (which requires PIR->VIRR transfer by software). > > > > > > If the interrupt it set by hardware after local_irq_disable(), > > > VMX-entry will handle the interrupt and perform the PIR->VIRR > > > transfer and reevaluate interrupts, injecting to guest > > > if necessary, is that correct ? > > > > > > > So does it make sense to check 'ON' and set KVM_REQ_EVENT accordingly > > > after interrupt is disabled? > > > > > > To replace the costly > > > > > > + */ > > > + if (kvm_x86_ops->hwapic_irr_update) > > > + kvm_x86_ops->hwapic_irr_update(vcpu, > > > + kvm_lapic_find_highest_irr(vcpu)); > > > > > > Yes, i think so. > > > > After adding the "checking ON and setting KVM_REQ_EVENT" operations > listed in my > > comments above, do you mean we still need to keep the costly code above > > inside "if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {}" in > function > > vcpu_enter_guest() as it used to be? If yes, my question is what is the exact > purpose > > of "checking ON and setting KVM_REQ_EVENT" operations? Here is the code > flow in > > vcpu_enter_guest(): > > > > 1. Check KVM_REQ_EVENT, if it is set, sync pir->virr > > 2. Disable interrupts > > 3. Check ON and set KVM_REQ_EVENT -- Here, we set KVM_REQ_EVENT, but > it is > > checked in the step 1, which means, we cannot get any benefits even we set it > here, > > since the "pir->virr" sync operation was done in step 1, between step 3 and > VM-Entry, > > we don't synchronize the pir to virr. So even we set KVM_REQ_EVENT here, > the interrupts > > remaining in PIR cannot be delivered to guest during this VM-Entry, right? > > Please check point *1 above. The code will go back to > > "if (kvm_check_request(KVM_REQ_EVENT, vcpu)" > > And perform the pir->virr sync. Ah, yes, that is the point I was missing. Thanks for pointing this out! Thanks, Feng