From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [PATCH 2/5]KVM:x86, apicv: adjust for virtual interrupt delivery
Date: Wed, 19 Sep 2012 17:53:23 +0300
Message-ID: <5059DC63.4010002@redhat.com>
References: <D9137FCD9CFF644B965863BCFBEDABB877AC53@SHSMSX101.ccr.corp.intel.com> <5048CDB9.4040202@redhat.com> <D9137FCD9CFF644B965863BCFBEDABB8781AD3@SHSMSX101.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>
To: "Li, Jiongxi" <jiongxi.li@intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:14168 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751887Ab2ISOxZ (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 19 Sep 2012 10:53:25 -0400
In-Reply-To: <D9137FCD9CFF644B965863BCFBEDABB8781AD3@SHSMSX101.ccr.corp.intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 09/14/2012 05:15 PM, Li, Jiongxi wrote:
> 
>> 
>> > @@ -5293,16 +5300,27 @@ static int vcpu_enter_guest(struct kvm_vcpu
>> *vcpu)
>> >  	}
>> >
>> >  	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
>> > +		/* update archtecture specific hints for APIC virtual interrupt delivery
>> */
>> > +		if (kvm_apic_vid_enabled(vcpu))
>> > +			kvm_x86_ops->update_irq(vcpu);
>> > +
>> 
>> Not defined.
> This function is defined in patch 3/5. Because virtual interrupt delivery is not enabled in this patch. So this function is not called. Since we will enable this feature by default, so maybe we can merge PATCH 2,3,4 together into one patch.

That will make it hard to review.  You might try to build the eoi exit
bitmap in the first patch, and do the rest (including posted interrupts)
in a following patch.  If you can split it further, it will be helpful.

>> 
>> >  		inject_pending_event(vcpu);
>> >
>> >  		/* enable NMI/IRQ window open exits if needed */
>> >  		if (vcpu->arch.nmi_pending)
>> >  			kvm_x86_ops->enable_nmi_window(vcpu);
>> > -		else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>> > +		else if (kvm_apic_vid_enabled(vcpu)) {
>> > +			if (kvm_cpu_has_interrupt_apic_vid(vcpu))
>> > +				kvm_x86_ops->enable_irq_window(vcpu);
>> > +		} else if (kvm_cpu_has_interrupt(vcpu) || req_int_win)
>> >  			kvm_x86_ops->enable_irq_window(vcpu);
>> >
>> >  		if (kvm_lapic_enabled(vcpu)) {
>> > -			update_cr8_intercept(vcpu);
>> > +			/* no need for tpr_threshold update if APIC virtual
>> > +			 * interrupt delivery is enabled
>> > +			 */
>> > +			if (!kvm_apic_vid_enabled(vcpu))
>> > +				update_cr8_intercept(vcpu);
>> 
>> Perhaps the arch function should do the ignoring.
> You means putting the 'vid_enabled' judgement in 'kvm_x86_ops->update_cr8_intercept'? Is it just out of the reason that reducing the code change in common code?

One option is to replace all the code above with

  kvm_x86_ops->update_irq()

where

  static void vmx_update_irq()
  {
      if (vid) {
          ... do vid stuff ...
      } else
          kvm_process_interrupt_queue(); // all the non-vid code above,
in a function
  }

So instead of kvm_apic_vid_enabled() scattered throughout the code we
have just one check.  svm_update_irq() can just call
kvm_process_interrupt_queue() and work as before, and later we can add
the AVIC code (the svm equivalent of APIC-V).


-- 
error compiling committee.c: too many arguments to function