Re: [Xen-devel] [PATCH][1/3] evtchn race condition

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

> > So, for example, the code that checks evtchn_pending[] and 
> > then clears a bit in evtchn_pending_sel is totally screwed. 
> > It races evtchn_send() res-setting the evtchn_pending[] bit! 
> > Fortunately, the comment that 'evtchn_pending_sel is shared 
> > by other event channels' is actually false right now. The 
> > *only* event channel a VMX domain cares about is its iopacket_port.
> 
> Ah... Ok. I'll rework the hvm code with the correct ordering and give it
> a test...

Okay, here's a patch for you to try out. It's only build-tested, but
it's at least a good starting point.

 -- Keir

diff -r 2d31ebf402e1 xen/arch/x86/vmx_io.c
--- a/xen/arch/x86/vmx_io.c     Wed Jan 25 13:36:35 2006
+++ b/xen/arch/x86/vmx_io.c     Wed Jan 25 16:47:51 2006
@@ -721,56 +721,46 @@
     }
 }
 
-int vmx_clear_pending_io_event(struct vcpu *v)
+
+/*
+ * NOTE! This function assumes that iopacket_port is the only active
+ * event channel for this domain. This may not be the case if using
+ * experimental paravirtualized drivers. 
+ * 
+ * The correct answer is probably to have a separate set of event channel
+ * ports for the VMX context.
+ */
+void vmx_check_events(struct vcpu *v)
 {
     struct domain *d = v->domain;
     int port = iopacket_port(d);
 
-    /* evtchn_pending_sel bit is shared by other event channels. */
-    if (!d->shared_info->evtchn_pending[port/BITS_PER_LONG])
-        clear_bit(port/BITS_PER_LONG, &v->vcpu_info->evtchn_pending_sel);
-
-    /* Note: VMX domains may need upcalls as well. */
-    if (!v->vcpu_info->evtchn_pending_sel)
-        clear_bit(0, &v->vcpu_info->evtchn_upcall_pending);
-
-    /* Clear the pending bit for port. */
-    return test_and_clear_bit(port, &d->shared_info->evtchn_pending[0]);
-}
-
-/* Because we've cleared the pending events first, we need to guarantee that
- * all events to be handled by xen for VMX domains are taken care of here.
- *
- * interrupts are guaranteed to be checked before resuming guest.
- * VMX upcalls have been already arranged for if necessary.
- */
-void vmx_check_events(struct vcpu *v)
-{
-    /* clear the event *before* checking for work. This should avoid
-       the set-and-check races */
-    if (vmx_clear_pending_io_event(v))
+    if ( !v->vcpu_info->evtchn_upcall_pending )
+        return;
+
+    v->vcpu_info->evtchn_upcall_pending = 0;
+
+    smp_mb__before_clear_bit();
+    clear_bit(port/BITS_PER_LONG, &v->vcpu_info->evtchn_pending_sel);
+    smp_mb__after_clear_bit();
+
+    if ( test_and_clear_bit(port, &d->shared_info->evtchn_pending[0]) )
         vmx_io_assist(v);
 }
 
 /* On exit from vmx_wait_io, we're guaranteed to have a I/O response from
    the device model */
-void vmx_wait_io()
+void vmx_wait_io(void)
 {
     extern void do_block();
-    int port = iopacket_port(current->domain);
-
-    do {
-        if (!test_bit(port, &current->domain->shared_info->evtchn_pending[0]))
-            do_block();
-
+
+    for ( ; ; )
+    {
         vmx_check_events(current);
-        if (!test_bit(ARCH_VMX_IO_WAIT, &current->arch.arch_vmx.flags))
-            break;
-        /* Events other than IOPACKET_PORT might have woken us up. In that
-           case, safely go back to sleep. */
-        clear_bit(port/BITS_PER_LONG, &current->vcpu_info->evtchn_pending_sel);
-        clear_bit(0, &current->vcpu_info->evtchn_upcall_pending);
-    } while(1);
+        if ( !test_bit(ARCH_VMX_IO_WAIT, &current->arch.arch_vmx.flags) )
+            break;
+        do_block();
+    }
 }
 
 /* Simple minded Local APIC priority implementation. Fix later */

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