KVM: Fix RTC interrupt coalescing tracking

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 <[email protected]>

arch/x86/kvm/lapic.c +0 -0

index 9d75193..9f4bea8 100644

arch/x86/kvm/lapic.c +14 -12

-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)

arch/x86/kvm/lapic.c +11 -3

+
+			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)

arch/x86/kvm/lapic.h +3 -3

index 61a73a0..c730ac9 100644
@@ -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);

virt/kvm/irq_comm.c +3 -7

index ef1817b..e2e6b44 100644
@@ -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.

Quoted 15 lines · Gleb Natapov

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 <[email protected]>

arch/x86/kvm/lapic.c +0 -0

index 9d75193..9f4bea8 100644
@@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 	return highest_irr;
 }
 

arch/x86/kvm/lapic.c int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu) · line 410

Quoted 6 lines

arch/x86/kvm/lapic.c +3 -3

-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.

Quoted 59 lines

arch/x86/kvm/lapic.c +14 -12

-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);
 

arch/x86/kvm/lapic.c static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 691

Quoted 8 lines

arch/x86/kvm/lapic.c +4 -3

-		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

Quoted 108 lines

arch/x86/kvm/lapic.c +11 -3

+
+			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)

arch/x86/kvm/lapic.h +3 -3

index 61a73a0..c730ac9 100644
@@ -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);

virt/kvm/irq_comm.c +3 -6

index ef1817b..e2e6b44 100644
@@ -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);
 

Quoted 5 lines

virt/kvm/irq_comm.c +0 -1

 	return r;
 }
--

Gleb.

Quoted 16 lines · Jan Kiszka

Quoted 15 lines · Gleb Natapov

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 <[email protected]>

arch/x86/kvm/lapic.c +0 -0

index 9d75193..9f4bea8 100644
@@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 	return highest_irr;
 }
 

Quoted 12 lines

arch/x86/kvm/lapic.c int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu) · line 410

Quoted 6 lines

arch/x86/kvm/lapic.c +3 -3

-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.

10.4.7.2 Local APIC State After It Has Been Software Disabled

The local APIC will respond normally to INIT, NMI, SMI, and SIPI messages.

Quoted 60 lines

Quoted 60 lines

arch/x86/kvm/lapic.c +14 -12

-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);
 

Quoted 12 lines

arch/x86/kvm/lapic.c static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, · line 691

Quoted 8 lines

arch/x86/kvm/lapic.c +4 -3

-		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?

Agree, but I wanted to do clear revert and fix on top.