[PATCH] IDE BMDMAState structure corruption with DMA_MULTI_THREAD

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

[-- Attachment #1.1.1: Type: text/plain, Size: 1215 bytes --]

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

[-- Attachment #1.1.2: xen-safe-ide-dma.patch --]
[-- Type: text/x-patch, Size: 1701 bytes --]

--- 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 #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3237 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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