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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@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

[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 
> Signed-off-by: Paolo Bonzini 
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
Signed-off-by: Paolo Bonzini 
---
 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


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html