[PATCH 0/3] KVM: Fix lost IRQ acks for RTC

[PATCH 0/3] KVM: Fix lost IRQ acks for RTC

[Joerg Roedel]

Hi,

here is a small patch-set to fix a race condition which
happens when an RTC-IRQ is migrated to another VCPU while it
is being handled by the guest.

The RTC-EOI handling in KVM requires that all sent interrupt
messages to the VCPUs need to be acked before another
RTC-IRQ can be sent. When an EOI signal from the guest is
lost, it will never see an RTC interrupt again (until it
reboots).

This is easily reproducible with a Linux guest executing
this loop:

	$ while true;do time hwclock --show --test --debug;done

When the guest has multiple vcpus and the RTC-IRQ is
regularily migrated (e.g. by irqbalance), the race condition
will be hit after some time and the hwclock tool will fail
with:

	select() to /dev/rtc to wait for clock tick timed out...synchronization failed

The race condition happens because of the way the EOI
backtracking between local APIC and IOAPIC works in KVM. The
destination VCPU and vector is part of the IOAPIC state.
When the guest sends an EOI to the local APIC the vector is
matched against the destinations stored in the IOAPIC and
ACKed there too if it matches.

The problem begins when a VCPU handles an RTC interrupt and
at the same time another VCPU migrates the RTC-IRQ away from
that VCPU. This updates the IOAPIC state in KVM to
the new destination, so that the EOI sent from the first
VCPU does not match anymore in the IOAPIC, hence losing the
RTC-EOI.

This patch-set fixes the race-condition by adding explicit
back-tracking information for RTC-IRQs. The rtc_status
struct already holds a dest_map bitmap to store which VCPUs
receveived an RTC-IRQ. This is extended to also hold the
vector that was sent to this VCPU.

This information is then used to match EOI signals from the
guest to the RTC. This explicit back-tracking fixes the
issue.

Regards,

	Joerg

Joerg Roedel (3):
  kvm: x86: Convert ioapic->rtc_status.dest_map to a struct
  kvm: x86: Track irq vectors in ioapic->rtc_status.dest_map
  kvm: x86: Check dest_map->vector to match eoi signals for rtc

 arch/x86/kvm/ioapic.c   | 30 +++++++++++++++++++++---------
 arch/x86/kvm/ioapic.h   | 17 +++++++++++++++--
 arch/x86/kvm/irq_comm.c |  2 +-
 arch/x86/kvm/lapic.c    | 14 ++++++++------
 arch/x86/kvm/lapic.h    |  7 +++++--
 5 files changed, 50 insertions(+), 20 deletions(-)

-- 
1.9.1

[PATCH 1/3] kvm: x86: Convert ioapic->rtc_status.dest_map to a struct

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Currently this is a bitmap which tracks which CPUs we expect
an EOI from. Move this bitmap to a struct so that we can
track additional information there.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kvm/ioapic.c   | 13 +++++++------
 arch/x86/kvm/ioapic.h   | 10 ++++++++--
 arch/x86/kvm/irq_comm.c |  2 +-
 arch/x86/kvm/lapic.c    | 10 +++++-----
 arch/x86/kvm/lapic.h    |  7 +++++--
 5 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 1facfd6..f2c9906 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -94,7 +94,7 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
 static void rtc_irq_eoi_tracking_reset(struct kvm_ioapic *ioapic)
 {
 	ioapic->rtc_status.pending_eoi = 0;
-	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
+	bitmap_zero(ioapic->rtc_status.dest_map.map, KVM_MAX_VCPUS);
 }
 
 static void kvm_rtc_eoi_tracking_restore_all(struct kvm_ioapic *ioapic);
@@ -117,16 +117,16 @@ static void __rtc_irq_eoi_tracking_restore_one(struct kvm_vcpu *vcpu)
 		return;
 
 	new_val = kvm_apic_pending_eoi(vcpu, e->fields.vector);
-	old_val = test_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
+	old_val = test_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map.map);
 
 	if (new_val == old_val)
 		return;
 
 	if (new_val) {
-		__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
+		__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map.map);
 		ioapic->rtc_status.pending_eoi++;
 	} else {
-		__clear_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
+		__clear_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map.map);
 		ioapic->rtc_status.pending_eoi--;
 		rtc_status_pending_eoi_check_valid(ioapic);
 	}
@@ -156,7 +156,8 @@ static void kvm_rtc_eoi_tracking_restore_all(struct kvm_ioapic *ioapic)
 
 static void rtc_irq_eoi(struct kvm_ioapic *ioapic, struct kvm_vcpu *vcpu)
 {
-	if (test_and_clear_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map)) {
+	if (test_and_clear_bit(vcpu->vcpu_id,
+			       ioapic->rtc_status.dest_map.map)) {
 		--ioapic->rtc_status.pending_eoi;
 		rtc_status_pending_eoi_check_valid(ioapic);
 	}
@@ -346,7 +347,7 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
 		 */
 		BUG_ON(ioapic->rtc_status.pending_eoi != 0);
 		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
-				ioapic->rtc_status.dest_map);
+					       &ioapic->rtc_status.dest_map);
 		ioapic->rtc_status.pending_eoi = (ret < 0 ? 0 : ret);
 	} else
 		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 2d16dc2..af72989 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -40,9 +40,14 @@ struct kvm_vcpu;
 #define RTC_GSI -1U
 #endif
 
+struct dest_map {
+	DECLARE_BITMAP(map, KVM_MAX_VCPUS);
+};
+
+
 struct rtc_status {
 	int pending_eoi;
-	DECLARE_BITMAP(dest_map, KVM_MAX_VCPUS);
+	struct dest_map dest_map;
 };
 
 union kvm_ioapic_redirect_entry {
@@ -118,7 +123,8 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
 		       int level, bool line_status);
 void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
-		struct kvm_lapic_irq *irq, unsigned long *dest_map);
+			     struct kvm_lapic_irq *irq,
+			     struct dest_map *dest_map);
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 8fc89ef..4537d17 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -53,7 +53,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
 }
 
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
-		struct kvm_lapic_irq *irq, unsigned long *dest_map)
+		struct kvm_lapic_irq *irq, struct dest_map *dest_map)
 {
 	int i, r = -1;
 	struct kvm_vcpu *vcpu, *lowest = NULL;
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 36591fa..5b78c97 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -491,10 +491,10 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 
 static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 			     int vector, int level, int trig_mode,
-			     unsigned long *dest_map);
+			     struct dest_map *dest_map);
 
 int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
-		unsigned long *dest_map)
+		     struct dest_map *dest_map)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
@@ -676,7 +676,7 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 }
 
 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)
+		struct kvm_lapic_irq *irq, int *r, struct dest_map *dest_map)
 {
 	struct kvm_apic_map *map;
 	unsigned long bitmap = 1;
@@ -819,7 +819,7 @@ out:
  */
 static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 			     int vector, int level, int trig_mode,
-			     unsigned long *dest_map)
+			     struct dest_map *dest_map)
 {
 	int result = 0;
 	struct kvm_vcpu *vcpu = apic->vcpu;
@@ -840,7 +840,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		result = 1;
 
 		if (dest_map)
-			__set_bit(vcpu->vcpu_id, dest_map);
+			__set_bit(vcpu->vcpu_id, dest_map->map);
 
 		if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
 			if (trig_mode)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 41bdb35..80e8d32 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -42,6 +42,9 @@ struct kvm_lapic {
 	unsigned long pending_events;
 	unsigned int sipi_vector;
 };
+
+struct dest_map;
+
 int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
@@ -60,11 +63,11 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu);
 void __kvm_apic_update_irr(u32 *pir, void *regs);
 void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
 int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
-		unsigned long *dest_map);
+		     struct dest_map *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);
+		struct kvm_lapic_irq *irq, int *r, struct dest_map *dest_map);
 
 u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
 int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
-- 
1.9.1

[PATCH 2/3] kvm: x86: Track irq vectors in ioapic->rtc_status.dest_map

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This allows backtracking later in case the rtc irq has been
moved to another vcpu/vector.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kvm/ioapic.h | 7 +++++++
 arch/x86/kvm/lapic.c  | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index af72989..7d2692a 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -41,7 +41,14 @@ struct kvm_vcpu;
 #endif
 
 struct dest_map {
+	/* vcpu bitmap where IRQ has been sent */
 	DECLARE_BITMAP(map, KVM_MAX_VCPUS);
+
+	/*
+	 * Vector sent to a given vcpu, only valid when
+	 * the vcpu's bit in map is set
+	 */
+	u8 vectors[KVM_MAX_VCPUS];
 };
 
 
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 5b78c97..5ea7ef0 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -839,8 +839,10 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 
 		result = 1;
 
-		if (dest_map)
+		if (dest_map) {
 			__set_bit(vcpu->vcpu_id, dest_map->map);
+			dest_map->vectors[vcpu->vcpu_id] = vector;
+		}
 
 		if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
 			if (trig_mode)
-- 
1.9.1

[PATCH 3/3] kvm: x86: Check dest_map->vector to match eoi signals for rtc

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Using the vector stored at interrupt delivery makes the eoi
matching safe agains irq migration in the ioapic.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kvm/ioapic.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index f2c9906..9db4709 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -237,10 +237,17 @@ static void kvm_ioapic_inject_all(struct kvm_ioapic *ioapic, unsigned long irr)
 void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, ulong *ioapic_handled_vectors)
 {
 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
+	struct dest_map *dest_map = &ioapic->rtc_status.dest_map;
 	union kvm_ioapic_redirect_entry *e;
 	int index;
 
 	spin_lock(&ioapic->lock);
+
+	/* Make sure we see any missing RTC EOI */
+	if (test_bit(vcpu->vcpu_id, dest_map->map))
+		__set_bit(dest_map->vectors[vcpu->vcpu_id],
+			  ioapic_handled_vectors);
+
 	for (index = 0; index < IOAPIC_NUM_PINS; index++) {
 		e = &ioapic->redirtbl[index];
 		if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
@@ -408,8 +415,14 @@ static void kvm_ioapic_eoi_inject_work(struct work_struct *work)
 static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
 			struct kvm_ioapic *ioapic, int vector, int trigger_mode)
 {
-	int i;
+	struct dest_map *dest_map = &ioapic->rtc_status.dest_map;
 	struct kvm_lapic *apic = vcpu->arch.apic;
+	int i;
+
+	/* RTC special handling */
+	if (test_bit(vcpu->vcpu_id, dest_map->map) &&
+	    vector == dest_map->vectors[vcpu->vcpu_id])
+		rtc_irq_eoi(ioapic, vcpu);
 
 	for (i = 0; i < IOAPIC_NUM_PINS; i++) {
 		union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i];
@@ -417,8 +430,6 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
 		if (ent->fields.vector != vector)
 			continue;
 
-		if (i == RTC_GSI)
-			rtc_irq_eoi(ioapic, vcpu);
 		/*
 		 * We are dropping lock while calling ack notifiers because ack
 		 * notifier callbacks for assigned devices call into IOAPIC
-- 
1.9.1

Re: [PATCH 0/3] KVM: Fix lost IRQ acks for RTC

[Paolo Bonzini]

On 29/02/2016 16:04, Joerg Roedel wrote:
> Hi,
> 
> here is a small patch-set to fix a race condition which
> happens when an RTC-IRQ is migrated to another VCPU while it
> is being handled by the guest.
> 
> The RTC-EOI handling in KVM requires that all sent interrupt
> messages to the VCPUs need to be acked before another
> RTC-IRQ can be sent. When an EOI signal from the guest is
> lost, it will never see an RTC interrupt again (until it
> reboots).
> 
> This is easily reproducible with a Linux guest executing
> this loop:
> 
> 	$ while true;do time hwclock --show --test --debug;done
> 
> When the guest has multiple vcpus and the RTC-IRQ is
> regularily migrated (e.g. by irqbalance), the race condition
> will be hit after some time and the hwclock tool will fail
> with:
> 
> 	select() to /dev/rtc to wait for clock tick timed out...synchronization failed
> 
> The race condition happens because of the way the EOI
> backtracking between local APIC and IOAPIC works in KVM. The
> destination VCPU and vector is part of the IOAPIC state.
> When the guest sends an EOI to the local APIC the vector is
> matched against the destinations stored in the IOAPIC and
> ACKed there too if it matches.
> 
> The problem begins when a VCPU handles an RTC interrupt and
> at the same time another VCPU migrates the RTC-IRQ away from
> that VCPU. This updates the IOAPIC state in KVM to
> the new destination, so that the EOI sent from the first
> VCPU does not match anymore in the IOAPIC, hence losing the
> RTC-EOI.
> 
> This patch-set fixes the race-condition by adding explicit
> back-tracking information for RTC-IRQs. The rtc_status
> struct already holds a dest_map bitmap to store which VCPUs
> receveived an RTC-IRQ. This is extended to also hold the
> vector that was sent to this VCPU.
> 
> This information is then used to match EOI signals from the
> guest to the RTC. This explicit back-tracking fixes the
> issue.
> 
> Regards,

Nice patches, really.  Ok to wait until 4.6?

Paolo

Re: [PATCH 0/3] KVM: Fix lost IRQ acks for RTC

[Joerg Roedel]

On Mon, Feb 29, 2016 at 04:12:42PM +0100, Paolo Bonzini wrote:
> > This information is then used to match EOI signals from the
> > guest to the RTC. This explicit back-tracking fixes the
> > issue.
> 
> Nice patches, really.  Ok to wait until 4.6?

Thanks. Putting them into v4.6 is fine for me.


	Joerg

Re: [PATCH 0/3] KVM: Fix lost IRQ acks for RTC

[Steve Rutherford]

This issue seems generic to level triggered interrupts as well as RTC
interrupts. It looks like KVM hacks around the issue with level
triggered interrupts by clearing the remote IRR when an IRQ is
reconfigured. Seems like an (admittedly lossy) way to handle this
issue with the RTC-IRQ would be to follow the lead of level-triggered
interrupts, and clear the pending EOIs when reconfiguring the RTC-IRQ.

[Given that we are already talking about this, this could be viewed as
a good time to go back and fix the issues with the remote IRR in the
IOAPIC.]

On Mon, Feb 29, 2016 at 7:30 AM, Joerg Roedel <joro@8bytes.org> wrote:
> On Mon, Feb 29, 2016 at 04:12:42PM +0100, Paolo Bonzini wrote:
>> > This information is then used to match EOI signals from the
>> > guest to the RTC. This explicit back-tracking fixes the
>> > issue.
>>
>> Nice patches, really.  Ok to wait until 4.6?
>
> Thanks. Putting them into v4.6 is fine for me.
>
>
>         Joerg
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html