[Xen-devel] [PATCH] IDE BMDMAState structure corruption with DMA_MULTI_THREAD

["Richard W.M. Jones" <rjones@xxxxxxxxxx>]

As I reported here:
http://lists.xensource.com/archives/html/xen-devel/2007-05/msg00492.html
I was experiencing qemu-dm segfaulting when trying to install FreeBSD 32 
fullvirt on a heavily loaded machine.
Dan Berrange and I tracked this down today to the BMDMAState structure 
being corrupted when a second DMA request was initiated by the guest 
before the first one had been completed.  Because the DMA thread and the 
main thread share a pointer to a single BMDMAState, bm->dma_cb is set to 
NULL by the main thread, and later the DMA thread jumps to this address 
(in dma_thread_loop, at the line 'len1 = bm->dma_cb(bm->ide_if, 
prd.addr, len);').
The attached patch corrects this by passing the whole structure between 
the threads, rather than a pointer to the structure (5 words rather than 
1, so there is a small amount of extra overhead).
With this patch I have been able to complete the FreeBSD FV install 
under load successfully.
Rich.

--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom.  Registered in
England and Wales under Company Registration No. 03798903

--- tools/ioemu/hw/ide.c.old    2007-05-15 14:02:34.000000000 +0100
+++ tools/ioemu/hw/ide.c        2007-05-15 19:25:06.000000000 +0100
@@ -402,10 +402,36 @@
 static void ide_dma_loop(BMDMAState *bm);
 static void dma_thread_loop(BMDMAState *bm);
 
+static int
+really_read (int fd, void *buf, size_t size)
+{
+    int r;
+
+again:
+    r = read (fd, buf, size);
+    if (r <= 0 || r == size) return r;
+    buf += r;
+    size -= r;
+    goto again;
+}
+
+static int
+really_write (int fd, void *buf, size_t size)
+{
+    int r;
+
+again:
+    r = write (fd, buf, size);
+    if (r == -1 || r == size) return r;
+    buf += r;
+    size -= r;
+    goto again;
+}
+
 extern int suspend_requested;
 static void *dma_thread_func(void* opaque)
 {
-    BMDMAState* req;
+    BMDMAState req;
     fd_set fds;
     int rv, nfds = file_pipes[0] + 1;
     struct timeval tm;
@@ -420,9 +446,12 @@
         rv = select(nfds, &fds, NULL, NULL, &tm);
         
         if (rv != 0) {
-            if (read(file_pipes[0], &req, sizeof(req)) == 0)
+            rv = really_read(file_pipes[0], &req, sizeof(req));
+           if (rv <= 0) {
+               if (rv == -1) perror ("qemu-dm: read");
                 return NULL;
-            dma_thread_loop(req);
+           }
+            dma_thread_loop(&req);
         } else {
             if (suspend_requested)  {
                 /* Need to tidy up the DMA thread so that we don't end up 
@@ -2371,7 +2400,8 @@
 #ifdef DMA_MULTI_THREAD
 static void ide_dma_loop(BMDMAState *bm)
 {
-    write(file_pipes[1], &bm, sizeof(bm));
+    if (really_write(file_pipes[1], bm, sizeof(*bm)) == -1)
+       perror ("qemu-dm: write");
 }
 static void dma_thread_loop(BMDMAState *bm)
 #else  /* DMA_MULTI_THREAD */

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature

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