WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: "Woller, Thomas" <thomas.woller@xxxxxxx>
Subject: Re: [Xen-devel] [PATCH][1/3] evtchn race condition
From: Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>
Date: Wed, 25 Jan 2006 16:52:50 +0000
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Wed, 25 Jan 2006 17:03:24 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: Your message of "Wed, 25 Jan 2006 09:56:02 CST." <F35BBA9A7A4D684FBFCEA191F108E539106538EF@xxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
> > 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
<Prev in Thread] Current Thread [Next in Thread>