Skip to contentSkip to conversation

IDE BMDMAState structure corruption with DMA_MULTI_THREAD

PATCH New Richard W.M. Jones 1 participant 1 message May 15, 2007

lore
Share & tools
lore

Richard W.M. Jones

author
#0 lore raw

PATCH 1/1 IDE BMDMAState structure corruption with DMA_MULTI_THREAD

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.

Signature · 4 lines
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
ioemu/hw/ide.c renamed from ioemu/hw/ide.c.old +34 -4
@@ -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 */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel