| Message ID | 1395394081-16252-3-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,