Re: [Xen-devel] [PATCH] xen: do not loose level interrupt notifications

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 27 Oct 2011, Keir Fraser wrote:
> On 27/10/2011 11:37, "Stefano Stabellini" <Stefano.Stabellini@xxxxxxxxxxxxx>
> wrote:
> 
> > PV on HVM guests can loose level interrupts coming from emulated
> > devices: we are missing code to retry to inject a pirq in the guest if
> > it corresponds to a level interrupt and the interrupt has been raised
> > while the guest is servicing the first one.
> > 
> > The same thing could also happen with PV guests, including dom0, even
> > though it is much more unlikely. In case of PV guests the scenario would
> > be the following:
> > 
> > 1) a device raises a level interrupt and xen injects it into the
> > guest;
> > 
> > 2) the guest is temporarely stuck: it does not ack it or eoi it;
> > 
> > 3) the xen timer kicks in and eois the interrupt;
> > 
> > 4) the device thinks it is all fine and sends a second interrupt;
> > 
> > 5) Xen fails to inject the second interrupt into the guest because the
> > guest has still the event channel pending bit set;
> > 
> > at this point the guest looses the second interrupt notification, that
> > is not supposed to happen with level interrupts and it might cause
> > problems with some devices.
> 
> You can't really lose a level-triggered interrupt. In step (4) the device
> isn't really actively involved in sending another interrupt -- it never
> deasserted its INTx line, and nor will it until the guest's ISR quenches the
> interrupt at the device. If the guest misses such an interrupt, and doesn't
> execute the relevant ISR when it should, then another interrupt will simply
> be raised by the interrupt controller when the guest does finally EOI the
> interrupt. Because the device is *still* asserting the line.
> 
> Well, that's the PV case anyway. I don't see any problem with our handling
> of the PV case.

OK, you convinced me.


> Is PV-HVM so different?

Yes, it is. In the PV on HVM case we need to reassert an emulated
interrupt if the guest EOIs it without quenching the interrupt in the
ISR.
We are doing it already in the emulated code path but we are not doing
it for interrupts that have been remapped into pirqs.

That said, if we don't care about the PV case we can simplify the patch,
I am appending a new one that only takes care of the PV on HVM case.

---


xen: re-inject emulated level pirqs in PV on HVM guests if still asserted

PV on HVM guests can loose level interrupts coming from emulated devices
if they have been remapped onto event channels.  The reason is that we
are missing the code to inject a pirq again in the guest when the guest
EOIs it, if it corresponds to an emulated level interrupt and the
interrupt is still asserted.

Fix this issue and also return error when the guest tries to get the
irq_status of a non-existing pirq.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

diff -r c681dd5aecf3 xen/arch/x86/physdev.c
--- a/xen/arch/x86/physdev.c    Tue Oct 25 19:22:09 2011 +0100
+++ b/xen/arch/x86/physdev.c    Thu Oct 27 11:30:55 2011 +0000
@@ -11,6 +11,7 @@
 #include <asm/current.h>
 #include <asm/io_apic.h>
 #include <asm/msi.h>
+#include <asm/hvm/irq.h>
 #include <asm/hypercall.h>
 #include <public/xen.h>
 #include <public/physdev.h>
@@ -270,6 +271,18 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
         if ( !is_hvm_domain(v->domain) ||
              domain_pirq_to_irq(v->domain, eoi.irq) > 0 )
             pirq_guest_eoi(pirq);
+        if ( is_hvm_domain(v->domain) &&
+                domain_pirq_to_emuirq(v->domain, eoi.irq) > 0 )
+        {
+            struct hvm_irq *hvm_irq = &v->domain->arch.hvm_domain.irq;
+            int gsi = domain_pirq_to_emuirq(v->domain, eoi.irq);
+
+            /* if this is a level irq and count > 0, send another
+             * notification */ 
+            if ( gsi >= NR_ISAIRQS /* ISA irqs are edge triggered */
+                    && hvm_irq->gsi_assert_count[gsi] )
+                send_guest_pirq(v->domain, pirq);
+        }
         spin_unlock(&v->domain->event_lock);
         ret = 0;
         break;
@@ -328,9 +341,10 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
             break;
         irq_status_query.flags = 0;
         if ( is_hvm_domain(v->domain) &&
-             domain_pirq_to_irq(v->domain, irq) <= 0 )
+             domain_pirq_to_irq(v->domain, irq) <= 0 &&
+             domain_pirq_to_emuirq(v->domain, irq) == IRQ_UNBOUND )
         {
-            ret = copy_to_guest(arg, &irq_status_query, 1) ? -EFAULT : 0;
+            ret = -EINVAL;
             break;
         }
 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel