Re: [PATCH] KVM: VMX: fix interrupt lost when enable virtual interrupt delivery

[Gleb Natapov <gleb@redhat.com>]

On Tue, Feb 26, 2013 at 08:34:16PM +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> In the platform which supporing virtual interrupt delivery feature,
> hardware will clear vIRR atomatically when target vcpu is running.
> So software should not modify vIRR when target vcpu is running. This
> patch will record the virtual interrupt into posted_irr when delivering
> virtual interrupt to guest. And then sync posted_irr into vIRR in target
> vcpu context.
> 
> The patch to enable Posted Interrupt has the similar logic. Since we are
> still discussing it, so split this part from Posted Interrupt patch to
> fix the virtual interrupt delivery issue.
> 
What the point of having this over posted interrupt patches considering
that PI patches do this and more?

> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  arch/x86/kvm/lapic.c |   39 ++++++++++++++++++++++++++++++++++++++-
>  arch/x86/kvm/lapic.h |    3 +++
>  arch/x86/kvm/vmx.c   |    3 ++-
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 02b51dd..98bc37e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -357,6 +357,11 @@ static u8 count_vectors(void *bitmap)
>  	return count;
>  }
>  
> +static inline bool apic_test_irr(int vec, struct kvm_lapic *apic)
> +{
> +	return apic_test_vector(vec, apic->regs + APIC_IRR);
> +}
> +
>  static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
>  {
>  	apic->irr_pending = true;
> @@ -368,6 +373,26 @@ static inline int apic_search_irr(struct kvm_lapic *apic)
>  	return find_highest_vector(apic->regs + APIC_IRR);
>  }
>  
> +static inline void kvm_apic_update_irr(struct kvm_lapic *apic)
> +{
> +	int index;
> +
> +	if (!kvm_x86_ops->vm_has_apicv(apic->vcpu->kvm))
> +		return;
> +
> +	if (apic->posted_irr_changed) {
> +		spin_lock(&apic->posted_irr_lock);
> +		for_each_set_bit(index,
> +			(unsigned long *)(&apic->posted_irr_changed), 8) {
> +			*((u32 *)(apic->regs + APIC_IRR + index * 0x10)) |=
> +						apic->posted_irr[index];
> +			apic->posted_irr[index] = 0;
> +		}
> +		apic->posted_irr_changed = 0;
> +		spin_unlock(&apic->posted_irr_lock);
> +	}
> +}
> +
>  static inline int apic_find_highest_irr(struct kvm_lapic *apic)
>  {
>  	int result;
> @@ -379,6 +404,7 @@ static inline int apic_find_highest_irr(struct kvm_lapic *apic)
>  	if (!apic->irr_pending)
>  		return -1;
>  
> +	kvm_apic_update_irr(apic);
>  	result = apic_search_irr(apic);
>  	ASSERT(result == -1 || result >= 16);
>  
> @@ -700,7 +726,17 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>  		} else
>  			apic_clear_vector(vector, apic->regs + APIC_TMR);
>  
> -		result = !apic_test_and_set_irr(vector, apic);
> +		if (kvm_x86_ops->vm_has_apicv(vcpu->kvm)) {
> +			spin_lock(&apic->posted_irr_lock);
> +			if (!apic_test_irr(vector, apic))
> +				result = !test_and_set_bit(vector,
> +					(unsigned long *)apic->posted_irr);
> +			if (result)
> +				apic->posted_irr_changed |= 1 << (vector >> 5);
> +			spin_unlock(&apic->posted_irr_lock);
> +		} else {
> +			result = !apic_test_and_set_irr(vector, apic);
> +		}
>  		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
>  					  trig_mode, vector, !result);
>  		if (!result) {
> @@ -1567,6 +1603,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
>  	static_key_slow_inc(&apic_sw_disabled.key); /* sw disabled at reset */
>  	kvm_lapic_reset(vcpu);
>  	kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
> +	spin_lock_init(&apic->posted_irr_lock);
>  
>  	return 0;
>  nomem_free_apic:
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 1676d34..38bebc8 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -20,6 +20,9 @@ struct kvm_lapic {
>  	u32 divide_count;
>  	struct kvm_vcpu *vcpu;
>  	bool irr_pending;
> +	u32 posted_irr[8];
> +	u8 posted_irr_changed;
> +	spinlock_t posted_irr_lock;
>  	/* Number of bits set in ISR. */
>  	s16 isr_count;
>  	/* The highest vector set in ISR; if -1 - invalid, must scan ISR. */
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c1b3041..c8e6036 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -84,7 +84,8 @@ module_param(vmm_exclusive, bool, S_IRUGO);
>  static bool __read_mostly fasteoi = 1;
>  module_param(fasteoi, bool, S_IRUGO);
>  
> -static bool __read_mostly enable_apicv_reg_vid;
> +static bool __read_mostly enable_apicv_reg_vid = 1;
> +module_param(enable_apicv_reg_vid, bool, S_IRUGO);
>  
>  /*
>   * If nested=1, nested virtualization is supported, i.e., guests may use
> -- 
> 1.7.1

--
			Gleb.