Re: [PATCH] KVM: Fix RTC interrupt coalescing tracking

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 7648 bytes --]

On 2013-06-24 14:19, Gleb Natapov wrote:
> This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. After
> the commit kvm_apic_set_irq() no longer returns accurate information
> about interrupt injection status if injection is done into disabled
> APIC. RTC interrupt coalescing tracking relies on the information to be
> accurate and cannot recover if it is not.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9d75193..9f4bea8 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
>  	return highest_irr;
>  }
>  
> -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> -			      int vector, int level, int trig_mode,
> -			      unsigned long *dest_map);
> +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> +			     int vector, int level, int trig_mode,
> +			     unsigned long *dest_map);

I still think __apic_accept_irq should unconditionally inject, and the
test for acpi_enabled belongs into kvm_apic_set_irq. Why should
__apic_accept_irq accept non-APIC_DM_FIXED messages if the APIC is off?
See below for another reason to refactor this part of the interface.

>  
> -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> -		      unsigned long *dest_map)
> +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> +		unsigned long *dest_map)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  
> -	__apic_accept_irq(apic, irq->delivery_mode, irq->vector,
> -			  irq->level, irq->trig_mode, dest_map);
> +	return __apic_accept_irq(apic, irq->delivery_mode, irq->vector,
> +			irq->level, irq->trig_mode, dest_map);
>  }
>  
>  static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
> @@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  	*r = -1;
>  
>  	if (irq->shorthand == APIC_DEST_SELF) {
> -		kvm_apic_set_irq(src->vcpu, irq, dest_map);
> -		*r = 1;
> +		*r = kvm_apic_set_irq(src->vcpu, irq, dest_map);
>  		return true;
>  	}
>  
> @@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  			continue;
>  		if (*r < 0)
>  			*r = 0;
> -		kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
> -		*r += 1;
> +		*r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
>  	}
>  
>  	ret = true;
> @@ -664,11 +662,15 @@ out:
>  	return ret;
>  }
>  
> -/* Set an IRQ pending in the lapic. */
> -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> -			      int vector, int level, int trig_mode,
> -			      unsigned long *dest_map)
> +/*
> + * Add a pending IRQ into lapic.
> + * Return 1 if successfully added and 0 if discarded.
> + */
> +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
> +			     int vector, int level, int trig_mode,
> +			     unsigned long *dest_map)
>  {
> +	int result = 0;
>  	struct kvm_vcpu *vcpu = apic->vcpu;
>  
>  	switch (delivery_mode) {
> @@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>  		if (dest_map)
>  			__set_bit(vcpu->vcpu_id, dest_map);
>  
> -		if (kvm_x86_ops->deliver_posted_interrupt)
> +		if (kvm_x86_ops->deliver_posted_interrupt) {
> +			result = 1;
>  			kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
> -		else {
> -			if (apic_test_and_set_irr(vector, apic)) {
> +		} else {
> +			result = !apic_test_and_set_irr(vector, apic);

This part of the revert makes no sense. If deliver_posted_interrupt is
on, we don't have this feedback anymore, thus we decided to remove it, no?

Jan

> +
> +			if (!result) {
>  				if (trig_mode)
>  					apic_debug("level trig mode repeatedly "
>  						"for vector %d", vector);
> @@ -697,7 +702,7 @@ static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>  		}
>  out:
>  		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
> -					  trig_mode, vector, false);
> +				trig_mode, vector, !result);
>  		break;
>  
>  	case APIC_DM_REMRD:
> @@ -709,12 +714,14 @@ out:
>  		break;
>  
>  	case APIC_DM_NMI:
> +		result = 1;
>  		kvm_inject_nmi(vcpu);
>  		kvm_vcpu_kick(vcpu);
>  		break;
>  
>  	case APIC_DM_INIT:
>  		if (!trig_mode || level) {
> +			result = 1;
>  			/* assumes that there are only KVM_APIC_INIT/SIPI */
>  			apic->pending_events = (1UL << KVM_APIC_INIT);
>  			/* make sure pending_events is visible before sending
> @@ -731,6 +738,7 @@ out:
>  	case APIC_DM_STARTUP:
>  		apic_debug("SIPI to vcpu %d vector 0x%02x\n",
>  			   vcpu->vcpu_id, vector);
> +		result = 1;
>  		apic->sipi_vector = vector;
>  		/* make sure sipi_vector is visible for the receiver */
>  		smp_wmb();
> @@ -752,6 +760,7 @@ out:
>  		       delivery_mode);
>  		break;
>  	}
> +	return result;
>  }
>  
>  int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2)
> @@ -1461,7 +1470,7 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> -void kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
> +int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
>  {
>  	u32 reg = kvm_apic_get_reg(apic, lvt_type);
>  	int vector, mode, trig_mode;
> @@ -1470,8 +1479,10 @@ void kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
>  		vector = reg & APIC_VECTOR_MASK;
>  		mode = reg & APIC_MODE_MASK;
>  		trig_mode = reg & APIC_LVT_LEVEL_TRIGGER;
> -		__apic_accept_irq(apic, mode, vector, 1, trig_mode, NULL);
> +		return __apic_accept_irq(apic, mode, vector, 1, trig_mode,
> +					NULL);
>  	}
> +	return 0;
>  }
>  
>  void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 61a73a0..c730ac9 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -57,9 +57,9 @@ void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
>  void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
>  int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
>  int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
> -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> -		      unsigned long *dest_map);
> -void kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
> +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> +		unsigned long *dest_map);
> +int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
>  
>  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map);
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index ef1817b..e2e6b44 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -91,8 +91,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  		if (!kvm_is_dm_lowest_prio(irq)) {
>  			if (r < 0)
>  				r = 0;
> -			kvm_apic_set_irq(vcpu, irq, dest_map);
> -			r++;
> +			r += kvm_apic_set_irq(vcpu, irq, dest_map);
>  		} else if (kvm_lapic_enabled(vcpu)) {
>  			if (!lowest)
>  				lowest = vcpu;
> @@ -101,10 +100,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  		}
>  	}
>  
> -	if (lowest) {
> -		kvm_apic_set_irq(lowest, irq, dest_map);
> -		r = 1;
> -	}
> +	if (lowest)
> +		r = kvm_apic_set_irq(lowest, irq, dest_map);
>  
>  	return r;
>  }
> --
> 			Gleb.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]