[PATCH 0/4] KVM: cleanup ioapic and fix KVM_SET_IRQCHIP with irr != 0

[PATCH 0/4] KVM: cleanup ioapic and fix KVM_SET_IRQCHIP with irr != 0

[Paolo Bonzini]

Unlike the old qemu-kvm, which really never did that, with new QEMU
it is for some reason somewhat likely to migrate a VM with a nonzero
IRR in the ioapic.  In the case of ISA edge-triggered interrupts,
this represents an interrupt that has not left the IOAPIC, which would
be okay but it is not handled right by KVM_SET_IRQCHIP.  Because the
interrupt is never injected, the guest never acknowledges it, the host
never deasserts the pin and new interrupts are dropped.

There are two problems to solve.

The obvious one is that interrupts are not reinjected upon KVM_SET_IRQCHIP,
which is taken care of by patches 3-4.

The second is that right now the IRR value depends on the falling edge
of the interrupt (as passed by the userspace via kvm_ioapic_set_irq).
This is unnecessary, and may lead to spurious reinjection in the
destination of migration; instead, we can clear the (internal-only)
IRR bit as soon as the interrupt leaves the IOAPIC.  This is done by
patch 2, which patch 1 prepares for.

This fixes migration of Windows guests without HPET.  Please review.

Paolo

Paolo Bonzini (4):
  KVM: ioapic: merge ioapic_deliver into ioapic_service
  KVM: ioapic: clear IRR for edge-triggered interrupts at delivery
  KVM: ioapic: extract body of kvm_ioapic_set_irq
  KVM: ioapic: reinject pending interrupts on KVM_SET_IRQCHIP

 virt/kvm/ioapic.c | 97 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 38 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] KVM: ioapic: merge ioapic_deliver into ioapic_service

[Paolo Bonzini]

Commonize the handling of masking, which was absent for kvm_ioapic_set_irq.
Setting remote_irr does not need a separate function either, and merging
the two functions avoids confusion.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/ioapic.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 1539d3757a04..0b4914147b9d 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -50,7 +50,7 @@
 #else
 #define ioapic_debug(fmt, arg...)
 #endif
-static int ioapic_deliver(struct kvm_ioapic *vioapic, int irq,
+static int ioapic_service(struct kvm_ioapic *vioapic, int irq,
 		bool line_status);
 
 static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
@@ -163,23 +163,6 @@ static bool rtc_irq_check_coalesced(struct kvm_ioapic *ioapic)
 	return false;
 }
 
-static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx,
-		bool line_status)
-{
-	union kvm_ioapic_redirect_entry *pent;
-	int injected = -1;
-
-	pent = &ioapic->redirtbl[idx];
-
-	if (!pent->fields.mask) {
-		injected = ioapic_deliver(ioapic, idx, line_status);
-		if (injected && pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
-			pent->fields.remote_irr = 1;
-	}
-
-	return injected;
-}
-
 static void update_handled_vectors(struct kvm_ioapic *ioapic)
 {
 	DECLARE_BITMAP(handled_vectors, 256);
@@ -282,12 +265,15 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 	}
 }
 
-static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
+static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
 {
 	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
 	struct kvm_lapic_irq irqe;
 	int ret;
 
+	if (entry->fields.mask)
+		return -1;
+
 	ioapic_debug("dest=%x dest_mode=%x delivery_mode=%x "
 		     "vector=%x trig_mode=%x\n",
 		     entry->fields.dest_id, entry->fields.dest_mode,
@@ -310,6 +296,9 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
 	} else
 		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
 
+	if (ret && irqe.trig_mode == IOAPIC_LEVEL_TRIG)
+		entry->fields.remote_irr = 1;
+
 	return ret;
 }
 
@@ -393,7 +382,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
 
 		ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
 		ent->fields.remote_irr = 0;
-		if (!ent->fields.mask && (ioapic->irr & (1 << i)))
+		if (ioapic->irr & (1 << i))
 			ioapic_service(ioapic, i, false);
 	}
 }
-- 
1.8.3.1

[PATCH 2/4] KVM: ioapic: clear IRR for edge-triggered interrupts at delivery

[Paolo Bonzini]

This ensures that IRR bits are set in the KVM_GET_IRQCHIP result only if
the interrupt is still sitting in the IOAPIC.  After the next patches, it
avoids spurious reinjection of the interrupt when KVM_SET_IRQCHIP is
called.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/ioapic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 0b4914147b9d..25e16a6898ed 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -288,6 +288,9 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
 	irqe.level = 1;
 	irqe.shorthand = 0;
 
+	if (irqe.trig_mode == IOAPIC_EDGE_TRIG)
+		ioapic->irr &= ~(1 << irq);
+
 	if (irq == RTC_GSI && line_status) {
 		BUG_ON(ioapic->rtc_status.pending_eoi != 0);
 		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
-- 
1.8.3.1

[PATCH 3/4] KVM: ioapic: extract body of kvm_ioapic_set_irq

[Paolo Bonzini]

We will reuse it to process a nonzero IRR that is passed to KVM_SET_IRQCHIP.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/ioapic.c | 63 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 25e16a6898ed..7a573f1bb79e 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -163,6 +163,44 @@ static bool rtc_irq_check_coalesced(struct kvm_ioapic *ioapic)
 	return false;
 }
 
+static int ioapic_set_irq(struct kvm_ioapic *ioapic, unsigned int irq,
+		int level, bool line_status)
+{
+	union kvm_ioapic_redirect_entry entry;
+	u32 mask = 1 << irq;
+	u32 old_irr;
+	int edge, ret;
+
+	entry = ioapic->redirtbl[irq];
+	edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
+
+	if (!level) {
+		ioapic->irr &= ~mask;
+		ret = 1;
+		goto out;
+	}
+
+	if (irq == RTC_GSI && line_status &&
+		rtc_irq_check_coalesced(ioapic)) {
+		ret = 0;
+		goto out;
+	}
+
+	old_irr = ioapic->irr;
+	ioapic->irr |= mask;
+	if ((edge && old_irr == ioapic->irr) ||
+	    (!edge && entry.fields.remote_irr)) {
+		ret = 0;
+		goto out;
+	}
+
+	ret = ioapic_service(ioapic, irq, line_status);
+
+out:
+	trace_kvm_ioapic_set_irq(entry.bits, irq, ret == 0);
+	return ret;
+}
+
 static void update_handled_vectors(struct kvm_ioapic *ioapic)
 {
 	DECLARE_BITMAP(handled_vectors, 256);
@@ -308,38 +346,15 @@ static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
 int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
 		       int level, bool line_status)
 {
-	u32 old_irr;
-	u32 mask = 1 << irq;
-	union kvm_ioapic_redirect_entry entry;
 	int ret, irq_level;
 
 	BUG_ON(irq < 0 || irq >= IOAPIC_NUM_PINS);
 
 	spin_lock(&ioapic->lock);
-	old_irr = ioapic->irr;
 	irq_level = __kvm_irq_line_state(&ioapic->irq_states[irq],
 					 irq_source_id, level);
-	entry = ioapic->redirtbl[irq];
-	if (!irq_level) {
-		ioapic->irr &= ~mask;
-		ret = 1;
-	} else {
-		int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
+	ret = ioapic_set_irq(ioapic, irq, irq_level, line_status);
 
-		if (irq == RTC_GSI && line_status &&
-			rtc_irq_check_coalesced(ioapic)) {
-			ret = 0; /* coalesced */
-			goto out;
-		}
-		ioapic->irr |= mask;
-		if ((edge && old_irr != ioapic->irr) ||
-		    (!edge && !entry.fields.remote_irr))
-			ret = ioapic_service(ioapic, irq, line_status);
-		else
-			ret = 0; /* report coalesced interrupt */
-	}
-out:
-	trace_kvm_ioapic_set_irq(entry.bits, irq, ret == 0);
 	spin_unlock(&ioapic->lock);
 
 	return ret;
-- 
1.8.3.1

[PATCH 4/4] KVM: ioapic: reinject pending interrupts on KVM_SET_IRQCHIP

[Paolo Bonzini]

After the previous patches, an interrupt whose bit is set in the IRR
register will never be in the LAPIC's IRR and has never been injected
on the migration source.  So inject it on the destination.

This fixes migration of Windows guests without HPET (they use the RTC
to trigger the scheduler tick, and lose it after migration).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/ioapic.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 7a573f1bb79e..63fb432ab502 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -201,6 +201,19 @@ out:
 	return ret;
 }
 
+void kvm_ioapic_inject_all(struct kvm_ioapic *ioapic, unsigned long irr)
+{
+	u32 idx;
+
+	rtc_irq_eoi_tracking_reset(ioapic);
+	for_each_set_bit(idx, &irr, 32) {
+		printk("ioapic_service for GSI %d\n", idx);
+		ioapic_set_irq(ioapic, idx, 1, true);
+	}
+	kvm_rtc_eoi_tracking_restore_all(ioapic);
+}
+
+
 static void update_handled_vectors(struct kvm_ioapic *ioapic)
 {
 	DECLARE_BITMAP(handled_vectors, 256);
@@ -601,9 +614,10 @@ int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 
 	spin_lock(&ioapic->lock);
 	memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
+	ioapic->irr = 0;
 	update_handled_vectors(ioapic);
 	kvm_vcpu_request_scan_ioapic(kvm);
-	kvm_rtc_eoi_tracking_restore_all(ioapic);
+	kvm_ioapic_inject_all(ioapic, state->irr);
 	spin_unlock(&ioapic->lock);
 	return 0;
 }
-- 
1.8.3.1

[PATCH resend 0/4] KVM: cleanup ioapic and fix KVM_SET_IRQCHIP with irr != 0

[Paolo Bonzini]

Unlike the old qemu-kvm, which really never did that, with new QEMU
it is for some reason somewhat likely to migrate a VM with a nonzero
IRR in the ioapic.  In the case of ISA edge-triggered interrupts,
this represents an interrupt that has not left the IOAPIC, which would
be okay but it is not handled right by KVM_SET_IRQCHIP.  Because the
interrupt is never injected, the guest never acknowledges it, the host
never deasserts the pin and new interrupts are dropped.

There are two problems to solve.

The obvious one is that interrupts are not reinjected upon KVM_SET_IRQCHIP,
which is taken care of by patches 3-4.

The second is that right now the IRR value depends on the falling edge
of the interrupt (as passed by the userspace via kvm_ioapic_set_irq).
This is unnecessary, and may lead to spurious reinjection in the
destination of migration; instead, we can clear the (internal-only)
IRR bit as soon as the interrupt leaves the IOAPIC.  This is done by
patch 2, which patch 1 prepares for.

This fixes migration of Windows guests without HPET.  Please review.

Paolo

Paolo Bonzini (4):
  KVM: ioapic: merge ioapic_deliver into ioapic_service
  KVM: ioapic: clear IRR for edge-triggered interrupts at delivery
  KVM: ioapic: extract body of kvm_ioapic_set_irq
  KVM: ioapic: reinject pending interrupts on KVM_SET_IRQCHIP

 virt/kvm/ioapic.c | 97 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 38 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] KVM: ioapic: merge ioapic_deliver into ioapic_service

[Paolo Bonzini]

Commonize the handling of masking, which was absent for kvm_ioapic_set_irq.
Setting remote_irr does not need a separate function either, and merging
the two functions avoids confusion.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/ioapic.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 1539d3757a04..0b4914147b9d 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -50,7 +50,7 @@
 #else
 #define ioapic_debug(fmt, arg...)
 #endif
-static int ioapic_deliver(struct kvm_ioapic *vioapic, int irq,
+static int ioapic_service(struct kvm_ioapic *vioapic, int irq,
 		bool line_status);
 
 static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
@@ -163,23 +163,6 @@ static bool rtc_irq_check_coalesced(struct kvm_ioapic *ioapic)
 	return false;
 }
 
-static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx,
-		bool line_status)
-{
-	union kvm_ioapic_redirect_entry *pent;
-	int injected = -1;
-
-	pent = &ioapic->redirtbl[idx];
-
-	if (!pent->fields.mask) {
-		injected = ioapic_deliver(ioapic, idx, line_status);
-		if (injected && pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
-			pent->fields.remote_irr = 1;
-	}
-
-	return injected;
-}
-
 static void update_handled_vectors(struct kvm_ioapic *ioapic)
 {
 	DECLARE_BITMAP(handled_vectors, 256);
@@ -282,12 +265,15 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 	}
 }
 
-static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
+static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
 {
 	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
 	struct kvm_lapic_irq irqe;
 	int ret;
 
+	if (entry->fields.mask)
+		return -1;
+
 	ioapic_debug("dest=%x dest_mode=%x delivery_mode=%x "
 		     "vector=%x trig_mode=%x\n",
 		     entry->fields.dest_id, entry->fields.dest_mode,
@@ -310,6 +296,9 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
 	} else
 		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
 
+	if (ret && irqe.trig_mode == IOAPIC_LEVEL_TRIG)
+		entry->fields.remote_irr = 1;
+
 	return ret;
 }
 
@@ -393,7 +382,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
 
 		ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
 		ent->fields.remote_irr = 0;
-		if (!ent->fields.mask && (ioapic->irr & (1 << i)))
+		if (ioapic->irr & (1 << i))
 			ioapic_service(ioapic, i, false);
 	}
 }
-- 
1.8.3.1

Re: [PATCH 1/4] KVM: ioapic: merge ioapic_deliver into ioapic_service

[Alex Williamson]

On Tue, 2014-03-18 at 15:54 +0100, Paolo Bonzini wrote:
> Commonize the handling of masking, which was absent for kvm_ioapic_set_irq.
> Setting remote_irr does not need a separate function either, and merging
> the two functions avoids confusion.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  virt/kvm/ioapic.c | 29 +++++++++--------------------
>  1 file changed, 9 insertions(+), 20 deletions(-)

Code consolidation, no functional change

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 1539d3757a04..0b4914147b9d 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -50,7 +50,7 @@
>  #else
>  #define ioapic_debug(fmt, arg...)
>  #endif
> -static int ioapic_deliver(struct kvm_ioapic *vioapic, int irq,
> +static int ioapic_service(struct kvm_ioapic *vioapic, int irq,
>  		bool line_status);
>  
>  static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
> @@ -163,23 +163,6 @@ static bool rtc_irq_check_coalesced(struct kvm_ioapic *ioapic)
>  	return false;
>  }
>  
> -static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx,
> -		bool line_status)
> -{
> -	union kvm_ioapic_redirect_entry *pent;
> -	int injected = -1;
> -
> -	pent = &ioapic->redirtbl[idx];
> -
> -	if (!pent->fields.mask) {
> -		injected = ioapic_deliver(ioapic, idx, line_status);
> -		if (injected && pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
> -			pent->fields.remote_irr = 1;
> -	}
> -
> -	return injected;
> -}
> -
>  static void update_handled_vectors(struct kvm_ioapic *ioapic)
>  {
>  	DECLARE_BITMAP(handled_vectors, 256);
> @@ -282,12 +265,15 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>  	}
>  }
>  
> -static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
> +static int ioapic_service(struct kvm_ioapic *ioapic, int irq, bool line_status)
>  {
>  	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
>  	struct kvm_lapic_irq irqe;
>  	int ret;
>  
> +	if (entry->fields.mask)
> +		return -1;
> +
>  	ioapic_debug("dest=%x dest_mode=%x delivery_mode=%x "
>  		     "vector=%x trig_mode=%x\n",
>  		     entry->fields.dest_id, entry->fields.dest_mode,
> @@ -310,6 +296,9 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
>  	} else
>  		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
>  
> +	if (ret && irqe.trig_mode == IOAPIC_LEVEL_TRIG)
> +		entry->fields.remote_irr = 1;
> +
>  	return ret;
>  }
>  
> @@ -393,7 +382,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
>  
>  		ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
>  		ent->fields.remote_irr = 0;
> -		if (!ent->fields.mask && (ioapic->irr & (1 << i)))
> +		if (ioapic->irr & (1 << i))
>  			ioapic_service(ioapic, i, false);
>  	}
>  }