Re: [PATCH v9 0/7] KVM: VMX: Add Posted Interrupt supporting

From: Gleb Natapov <gleb@redhat.com>
To: "Zhang, Yang Z" <yang.z.zhang@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"mtosatti@redhat.com" <mtosatti@redhat.com>,
	"Zhang, Xiantao" <xiantao.zhang@intel.com>,
	"Nakajima, Jun" <jun.nakajima@intel.com>
Subject: Re: [PATCH v9 0/7] KVM: VMX: Add Posted Interrupt supporting
Date: Thu, 11 Apr 2013 08:44:17 +0300	[thread overview]
Message-ID: <20130411054417.GM17919@redhat.com> (raw)
In-Reply-To: <A9667DDFB95DB7438FA9D7D576C3D87E09A075E0@SHSMSX101.ccr.corp.intel.com>

On Thu, Apr 11, 2013 at 01:03:30AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-04-10:
> > On Wed, Apr 10, 2013 at 09:22:50PM +0800, Yang Zhang wrote:
> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >> 
> >> The follwoing patches are adding the Posted Interrupt supporting to KVM:
> >> The first patch enables the feature 'acknowledge interrupt on vmexit'.Since
> >> it is required by Posted interrupt, we need to enable it firstly.
> >> 
> >> And the subsequent patches are adding the posted interrupt supporting:
> >> Posted Interrupt allows APIC interrupts to inject into guest directly
> >> without any vmexit.
> >> 
> >> - When delivering a interrupt to guest, if target vcpu is running,
> >>   update Posted-interrupt requests bitmap and send a notification event
> >>   to the vcpu. Then the vcpu will handle this interrupt automatically,
> >>   without any software involvemnt.
> >> - If target vcpu is not running or there already a notification event
> >>   pending in the vcpu, do nothing. The interrupt will be handled by
> >>   next vm entry
> >> Changes from v8 to v9:
> >> * Add tracing in PI case when deliver interrupt.
> >> * Scan ioapic when updating SPIV register.
> > Do not see it at the patch series. Have I missed it?
> The change is in forth patch:
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6796218..4ccdc94 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -134,11 +134,7 @@ static inline void apic_set_spiv(struct kvm_lapic *apic, u32 val)
>  			static_key_slow_inc(&apic_sw_disabled.key);
>  	}
>  	apic_set_reg(apic, APIC_SPIV, val);
> -}
> -
> -static inline int apic_enabled(struct kvm_lapic *apic)
> -{
> -	return kvm_apic_sw_enabled(apic) &&	kvm_apic_hw_enabled(apic);
> +	kvm_make_request(KVM_REQ_SCAN_IOAPIC, apic->vcpu);
>  }
> 
OK, see it now. Thanks.

> As you mentioned, since it will call apic_enabled() to check whether apic is enabled in vcpu_scan_ioapic. So we must ensure rescan ioapic when apic state changed.
> And I found recalculate_apic_map() doesn't track the enable/disable apic by software approach. So make_scan_ioapic_request in recalculate_apic_map() is not enough.
> We also should force rescan ioapic when apic state is changed via software approach(update spiv reg).
> 
10.4.7.2 Local APIC State After It Has Been Software Disabled says:

  Pending interrupts in the IRR and ISR registers are held and require
  masking or handling by the CPU.

My understanding is that we should treat software disabled APIC as a
valid target for an interrupt. vcpu_scan_ioapic() should check
kvm_apic_hw_enabled() only.

--
			Gleb.