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

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

v1->v2:
	more comments in patch 3
	change argument name in patch 3 from level to irq_level
	use IOAPIC_NUM_PINS in patch 4 as a limit to for_each_set_bit
	remove debug printk in patch 4

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 | 107 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 69 insertions(+), 38 deletions(-)

-- 
1.8.3.1

[PATCH v2 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.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
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 v2 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.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
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 v2 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.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2:
	more comments
	change argument name from level to irq_level

 virt/kvm/ioapic.c | 74 +++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 24 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 25e16a6898ed..270f7fe73f39 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -163,6 +163,55 @@ 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 irq_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 (!irq_level) {
+		ioapic->irr &= ~mask;
+		ret = 1;
+		goto out;
+	}
+
+	/*
+	 * Return 0 for coalesced interrupts; for edge-triggered interrupts,
+	 * this only happens if a previous edge has not been delivered due
+	 * do masking.  For level interrupts, the remote_irr field tells
+	 * us if the interrupt is waiting for an EOI.
+	 *
+	 * RTC is special: it is edge-triggered, but userspace likes to know
+	 * if it has been already ack-ed via EOI because coalesced RTC
+	 * interrupts lead to time drift in Windows guests.  So we track
+	 * EOI manually for the RTC interrupt.
+	 */
+	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 +357,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 v2 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>
---
	v1->v2:
	use IOAPIC_NUM_PINS as a limit to for_each_set_bit
	remove debug printk

 virt/kvm/ioapic.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 270f7fe73f39..d4b601547f1f 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -212,6 +212,18 @@ out:
 	return ret;
 }
 
+static 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, IOAPIC_NUM_PINS)
+		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);
@@ -612,9 +624,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 v2 4/4] KVM: ioapic: reinject pending interrupts on KVM_SET_IRQCHIP

[Alex Williamson]

On Fri, 2014-03-21 at 10:28 +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>
> ---
> 	v1->v2:
> 	use IOAPIC_NUM_PINS as a limit to for_each_set_bit
> 	remove debug printk

Looks good and thanks for the extended comment in patch 3.

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

>  virt/kvm/ioapic.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 270f7fe73f39..d4b601547f1f 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -212,6 +212,18 @@ out:
>  	return ret;
>  }
>  
> +static 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, IOAPIC_NUM_PINS)
> +		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);
> @@ -612,9 +624,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 v2 1/4] KVM: ioapic: merge ioapic_deliver into ioapic_service

[Eduardo Habkost]

On Fri, Mar 21, 2014 at 10:27:58AM +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.
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Just code movement, no functional changes.

(The change from entry->fields.trig_mode to irqe.trig_mode is not purely
code movement, but kvm_irq_delivery_to_apic() doesn't change
irq->trig_mode, so it's OK).

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Mar 21, 2014 at 10:27:59AM +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.
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> 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);
> +

Now, every call to ioapic_service() for an edge interrupt clears the IRR
bit immediately (assuming the mask is unset).

If the IRR bit is immediately zero on delivery, why won't this break the
edge detection logic on kvm_ioapic_set_irq()? Am I missing some
additional detail?

In other words, won't this cause spurious interrupts if
kvm_ioapic_set_irq(..., true) is called twice?

-- 
Eduardo

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

[Radim Krčmář]

2014-03-21 10:28+0100, Paolo Bonzini:
> We will reuse it to process a nonzero IRR that is passed to KVM_SET_IRQCHIP.
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v1->v2:
> 	more comments
> 	change argument name from level to irq_level
> 
>  virt/kvm/ioapic.c | 74 +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 24 deletions(-)
> 
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 25e16a6898ed..270f7fe73f39 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -163,6 +163,55 @@ 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 irq_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 (!irq_level) {
> +		ioapic->irr &= ~mask;
> +		ret = 1;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Return 0 for coalesced interrupts; for edge-triggered interrupts,
> +	 * this only happens if a previous edge has not been delivered due
> +	 * do masking.  For level interrupts, the remote_irr field tells
          (^ to)
> +	 * us if the interrupt is waiting for an EOI.

How do we know we haven't delivered an edge-triggered interrupt when
ioapic_service() always clears this IRR bit?

> +	 *
> +	 * RTC is special: it is edge-triggered, but userspace likes to know
> +	 * if it has been already ack-ed via EOI because coalesced RTC
> +	 * interrupts lead to time drift in Windows guests.  So we track
> +	 * EOI manually for the RTC interrupt.
> +	 */

The comment makes me think we should not be coalescing more, but we do.

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

(Fun fact: GCC 4.8.2 doesn't optimize it as well as
   edge ? old_irr == ioapic->irr : 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 +357,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
> 
> 
> --
> 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

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

[Paolo Bonzini]

Il 21/03/2014 19:34, Eduardo Habkost ha scritto:
>> > +	if (irqe.trig_mode == IOAPIC_EDGE_TRIG)
>> > +		ioapic->irr &= ~(1 << irq);
>> > +
> Now, every call to ioapic_service() for an edge interrupt clears the IRR
> bit immediately (assuming the mask is unset).
>
> If the IRR bit is immediately zero on delivery, why won't this break the
> edge detection logic on kvm_ioapic_set_irq()? Am I missing some
> additional detail?

That logic will still trigger if the interrupt is masked in the IOAPIC's 
ioredirtbl.

> In other words, won't this cause spurious interrupts if
> kvm_ioapic_set_irq(..., true) is called twice?

Yes, and this is why I don't like this patch very much.  Basically it 
leaves it up to userspace to only send edge-triggered interrupts on an 
actual rising edge and never do two consecutive kvm_ioapic_set_irq(..., 
true) ioctls.

On the other hand, treating IRR this way is how QEMU's userspace IOAPIC 
works already, so the chance of bugs is smaller than any alternative; 
and the alternatives aren't that good either.  For example, I had 
thought about using the remote_irr bit to store the status.  In order to 
keep the old behavior where remote_irr is zero for edge-triggered 
interrupts, the bit can be masked out when reading the ioredirtbl.

KVM_SET_IRQCHIP then could look at irr & ~remote_irr to find interrupts 
that have to be delivered.  However, I was afraid that this would cause 
problems on migration from new to old kernels, which would let userspace 
see remote_irr=1 for edge-triggered interrupts.

Paolo

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

[Paolo Bonzini]

Il 21/03/2014 19:58, Radim Krčmář ha scritto:
>> > +	/*
>> > +	 * Return 0 for coalesced interrupts; for edge-triggered interrupts,
>> > +	 * this only happens if a previous edge has not been delivered due
>> > +	 * do masking.  For level interrupts, the remote_irr field tells
>           (^ to)
>> > +	 * us if the interrupt is waiting for an EOI.
>
> How do we know we haven't delivered an edge-triggered interrupt when
> ioapic_service() always clears this IRR bit?

We know we have at least delivered it to the local APIC(s).  If it is 
masked in the ioredirtbl, ioapic_service doesn't clear the IRR.

Paolo

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

[Radim Krčmář]

2014-03-23 09:44+0100, Paolo Bonzini:
> Il 21/03/2014 19:58, Radim Krčmář ha scritto:
> >>> +	/*
> >>> +	 * Return 0 for coalesced interrupts; for edge-triggered interrupts,
> >>> +	 * this only happens if a previous edge has not been delivered due
> >>> +	 * do masking.  For level interrupts, the remote_irr field tells
> >          (^ to)
> >>> +	 * us if the interrupt is waiting for an EOI.
> >
> >How do we know we haven't delivered an edge-triggered interrupt when
> >ioapic_service() always clears this IRR bit?
> 
> We know we have at least delivered it to the local APIC(s).  If it
> is masked in the ioredirtbl, ioapic_service doesn't clear the IRR.

I see, LAPIC can't "refuse" an interrupt, so kvm_irq_delivery_to_apic()
won't (mustn't) fail. (The code looks fragile if it returned -1.)

Thanks.

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

[Radim Krčmář]

2014-03-21 10:28+0100, 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>
> ---
> 	v1->v2:
> 	use IOAPIC_NUM_PINS as a limit to for_each_set_bit
> 	remove debug printk
> 
>  virt/kvm/ioapic.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 270f7fe73f39..d4b601547f1f 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -212,6 +212,18 @@ out:
>  	return ret;
>  }
>  
> +static 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, IOAPIC_NUM_PINS)
> +		ioapic_set_irq(ioapic, idx, 1, true);
> +
> +	kvm_rtc_eoi_tracking_restore_all(ioapic);

(We shouldn't have RTC interrupt with pending EOI in irr, so the
 function could be independent.
 I'd prefer 'ioapic->irr = 0' here ...)

> +}
> +
> +
>  static void update_handled_vectors(struct kvm_ioapic *ioapic)
>  {
>  	DECLARE_BITMAP(handled_vectors, 256);
> @@ -612,9 +624,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
> 
> --
> 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

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

[Radim Krčmář]

2014-03-21 10:27+0100, 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
> 
> v1->v2:
> 	more comments in patch 3
> 	change argument name in patch 3 from level to irq_level
> 	use IOAPIC_NUM_PINS in patch 4 as a limit to for_each_set_bit
> 	remove debug printk in patch 4

Nice solution to a tricky problem,

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> 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 | 107 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 69 insertions(+), 38 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> --
> 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

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

[Paolo Bonzini]

Il 24/03/2014 18:58, Radim Krčmář ha scritto:
>> > +	rtc_irq_eoi_tracking_reset(ioapic);
>> > +	for_each_set_bit(idx, &irr, IOAPIC_NUM_PINS)
>> > +		ioapic_set_irq(ioapic, idx, 1, true);
>> > +
>> > +	kvm_rtc_eoi_tracking_restore_all(ioapic);
> (We shouldn't have RTC interrupt with pending EOI in irr, so the
>  function could be independent.

If the RTC state gets out of sync you get a BUG_ON, so I preferred to be 
safe and first inject the interrupts without any recorded recipient of 
GSI 8; and then put everything together based on both LAPIC and IOAPIC 
state.

>  I'd prefer 'ioapic->irr = 0' here ...)

The point is that "ioapic->irr = 0" is overriding the previous memcpy, 
because state->irr is used as argument to kvm_ioapic_inject_all instead. 
  So I think "iopic->irr = 0" should stay close to the memcpy.

Paolo

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

[Radim Krčmář]

2014-03-24 19:14+0100, Paolo Bonzini:
> Il 24/03/2014 18:58, Radim Krčmář ha scritto:
> > I'd prefer 'ioapic->irr = 0' here ...)
> 
> The point is that "ioapic->irr = 0" is overriding the previous
> memcpy, because state->irr is used as argument to
> kvm_ioapic_inject_all instead.  So I think "iopic->irr = 0" should
> stay close to the memcpy.

Yeah, I was just spouting ... my reasoning was that we clear irr only
because it's going to be recomputed, so that code is more related.
(The function name would need to change though.)