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

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

[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

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

[Alex Williamson]

On Tue, 2014-03-18 at 15:54 +0100, Paolo Bonzini wrote:
> 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(+)

Makes sense

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

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

[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

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

[Alex Williamson]

On Tue, 2014-03-18 at 15:54 +0100, Paolo Bonzini wrote:
> 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(-)

I might choose to keep the "irq_level" variable name in the new
functions since dealing with both "edge" and "level" variables is a bit
confusing.  Some handy comments describing the return value were also
dropped.  Functionally identical though

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

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

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

[Paolo Bonzini]

Il 20/03/2014 21:25, Alex Williamson ha scritto:
> I might choose to keep the "irq_level" variable name in the new
> functions since dealing with both "edge" and "level" variables is a bit
> confusing.

Good point.  Will rename for v2.

Paolo

[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

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

[Jan Kiszka]

On 2014-03-18 15:54, Paolo Bonzini wrote:
> 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)

static? (Didn't the compiler bark at you?)

Jan

> +{
> +	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;
>  }
> 
-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

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

[Paolo Bonzini]

Il 18/03/2014 16:10, Jan Kiszka ha scritto:
>> > +void kvm_ioapic_inject_all(struct kvm_ioapic *ioapic, unsigned long irr)
> static? (Didn't the compiler bark at you?)

I re-checked and it didn't indeed.  Fixed this locally.

Paolo

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

[Alex Williamson]

On Tue, 2014-03-18 at 15:54 +0100, Paolo Bonzini wrote:
> 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)
> +{

nit, kvm_ioapic_state.irr is a u32

> +	u32 idx;
> +
> +	rtc_irq_eoi_tracking_reset(ioapic);
> +	for_each_set_bit(idx, &irr, 32) {

Can we use IOAPIC_NUM_PINS in place of 32?

> +		printk("ioapic_service for GSI %d\n", idx);

Is this leftover debugging?  Maybe give it a loglevel if it's
intentional.  Looks reasonable otherwise.  Thanks,

Alex

> +		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;
>  }

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

[Paolo Bonzini]

Il 20/03/2014 21:24, Alex Williamson ha scritto:
>> >
>> > +void kvm_ioapic_inject_all(struct kvm_ioapic *ioapic, unsigned long irr)
>> > +{
> nit, kvm_ioapic_state.irr is a u32

Yes, but for_each_set_bit requires unsigned long.

>> > +	u32 idx;
>> > +
>> > +	rtc_irq_eoi_tracking_reset(ioapic);
>> > +	for_each_set_bit(idx, &irr, 32) {
>
> Can we use IOAPIC_NUM_PINS in place of 32?

Yes, indeeed.

> > +		printk("ioapic_service for GSI %d\n", idx);
>
> Is this leftover debugging?  Maybe give it a loglevel if it's
> intentional.  Looks reasonable otherwise.  Thanks,

I'll remove it.  Thanks for the review!

Paolo

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