[Qemu-devel] [PATCH 0/4] macio: change DMA methods over to offset/len implementation

[Qemu-devel] [PATCH 0/4] macio: change DMA methods over to offset/len implementation

[Mark Cave-Ayland]

This patchset follows on from my recent work on fixing issues with the
macio controller, and remodels the new pmac_dma_read() and pmac_dma_write()
functions in a similar manner to the unaligned block functions.

With this in place, long chains of overlapping unaligned requests as used
by OS X/Darwin will now work correctly without introducting torn sector
errors when writing to disk.

Also included are some tidy-ups as a result of the above changes.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Mark Cave-Ayland (4):
  macio: switch pmac_dma_read() over to new offset/len implementation
  macio: switch pmac_dma_write() over to new offset/len implementation
  macio: update comment/constants to reflect the new code
  macio: remove remainder_len DBDMA_io property

 hw/ide/macio.c             |  271 +++++++++++++++++---------------------------
 include/hw/ppc/mac_dbdma.h |    4 +-
 2 files changed, 105 insertions(+), 170 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/4] macio: switch pmac_dma_read() over to new offset/len implementation

[Mark Cave-Ayland]

For better handling of unaligned block device accesses.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c |  102 ++++++++++++++++++++++----------------------------------
 1 file changed, 40 insertions(+), 62 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 585a27b..f1ac001 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -52,7 +52,7 @@ static const int debug_macio = 0;
 #define MACIO_PAGE_SIZE 4096
 
 static void pmac_dma_read(BlockBackend *blk,
-                          int64_t sector_num, int nb_sectors,
+                          int64_t offset, unsigned int bytes,
                           void (*cb)(void *opaque, int ret), void *opaque)
 {
     DBDMA_io *io = opaque;
@@ -60,76 +60,48 @@ static void pmac_dma_read(BlockBackend *blk,
     IDEState *s = idebus_active_if(&m->bus);
     dma_addr_t dma_addr, dma_len;
     void *mem;
-    int nsector, remainder;
+    int64_t sector_num;
+    int nsector;
+    uint64_t align = BDRV_SECTOR_SIZE;
+    size_t head_bytes, tail_bytes;
 
     qemu_iovec_destroy(&io->iov);
     qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
 
-    if (io->remainder_len > 0) {
-        /* Return remainder of request */
-        int transfer = MIN(io->remainder_len, io->len);
+    sector_num = (offset >> 9);
+    nsector = (io->len >> 9);
 
-        MACIO_DPRINTF("--- DMA read pop     - bounce addr: %p addr: %"
-                      HWADDR_PRIx " remainder_len: %x\n",
-                      &io->remainder + (0x200 - transfer), io->addr,
-                      io->remainder_len);
+    MACIO_DPRINTF("--- DMA read transfer (0x%" HWADDR_PRIx ",0x%x): "
+                  "sector_num: %ld, nsector: %d\n", io->addr, io->len,
+                  sector_num, nsector);
 
-        cpu_physical_memory_write(io->addr,
-                                  &io->remainder + (0x200 - transfer),
-                                  transfer);
+    dma_addr = io->addr;
+    dma_len = io->len;
+    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
+                         DMA_DIRECTION_FROM_DEVICE);
 
-        io->remainder_len -= transfer;
-        io->len -= transfer;
-        io->addr += transfer;
+    if (offset & (align - 1)) {
+        head_bytes = offset & (align - 1);
 
-        s->io_buffer_index += transfer;
-        s->io_buffer_size -= transfer;
+        MACIO_DPRINTF("--- DMA unaligned head: sector %ld, "
+                      "discarding %ld bytes\n", sector_num, head_bytes);
 
-        if (io->remainder_len != 0) {
-            /* Still waiting for remainder */
-            return;
-        }
+        qemu_iovec_add(&io->iov, &io->remainder, head_bytes);
 
-        if (io->len == 0) {
-            MACIO_DPRINTF("--- finished all read processing; go and finish\n");
-            cb(opaque, 0);
-            return;
-        }
+        bytes += offset & (align - 1);
+        offset = offset & ~(align - 1);
     }
 
-    if (s->drive_kind == IDE_CD) {
-        sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9);
-    } else {
-        sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
-    }
+    qemu_iovec_add(&io->iov, mem, io->len);
 
-    nsector = ((io->len + 0x1ff) >> 9);
-    remainder = (nsector << 9) - io->len;
+    if ((offset + bytes) & (align - 1)) {
+        tail_bytes = (offset + bytes) & (align - 1);
 
-    MACIO_DPRINTF("--- DMA read transfer - addr: %" HWADDR_PRIx " len: %x\n",
-                  io->addr, io->len);
-
-    dma_addr = io->addr;
-    dma_len = io->len;
-    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
-                         DMA_DIRECTION_FROM_DEVICE);
+        MACIO_DPRINTF("--- DMA unaligned tail: sector %ld, "
+                      "discarding bytes %ld\n", sector_num, tail_bytes);
 
-    if (!remainder) {
-        MACIO_DPRINTF("--- DMA read aligned - addr: %" HWADDR_PRIx
-                      " len: %x\n", io->addr, io->len);
-        qemu_iovec_add(&io->iov, mem, io->len);
-    } else {
-        MACIO_DPRINTF("--- DMA read unaligned - addr: %" HWADDR_PRIx
-                      " len: %x\n", io->addr, io->len);
-        qemu_iovec_add(&io->iov, mem, io->len);
-
-        MACIO_DPRINTF("--- DMA read push    - bounce addr: %p "
-                      "remainder_len: %x\n",
-                      &io->remainder + 0x200 - remainder, remainder);
-        qemu_iovec_add(&io->iov, &io->remainder + 0x200 - remainder,
-                       remainder);
-
-        io->remainder_len = remainder;
+        qemu_iovec_add(&io->iov, &io->remainder, align - tail_bytes);
+        bytes = ROUND_UP(bytes, align);
     }
 
     s->io_buffer_size -= io->len;
@@ -137,11 +109,11 @@ static void pmac_dma_read(BlockBackend *blk,
 
     io->len = 0;
 
-    MACIO_DPRINTF("--- Block read transfer   - sector_num: %"PRIx64"  "
-                  "nsector: %x\n",
-                  sector_num, nsector);
+    MACIO_DPRINTF("--- Block read transfer - sector_num: %" PRIx64 "  "
+                  "nsector: %x\n", (offset >> 9), (bytes >> 9));
 
-    m->aiocb = blk_aio_readv(blk, sector_num, &io->iov, nsector, cb, io);
+    m->aiocb = blk_aio_readv(blk, (offset >> 9), &io->iov, (bytes >> 9),
+                             cb, io);
 }
 
 static void pmac_dma_write(BlockBackend *blk,
@@ -246,6 +218,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     IDEState *s = idebus_active_if(&m->bus);
     int64_t sector_num;
     int nsector, remainder;
+    int64_t offset;
 
     MACIO_DPRINTF("\ns is %p\n", s);
     MACIO_DPRINTF("io_buffer_index: %x\n", s->io_buffer_index);
@@ -294,10 +267,13 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     nsector = (io->len + 0x1ff) >> 9;
     remainder = io->len & 0x1ff;
 
+    /* Calculate current offset */
+    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
+
     MACIO_DPRINTF("nsector: %d   remainder: %x\n", nsector, remainder);
     MACIO_DPRINTF("sector: %"PRIx64"   %zx\n", sector_num, io->iov.size / 512);
 
-    pmac_dma_read(s->blk, sector_num, nsector, pmac_ide_atapi_transfer_cb, io);
+    pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io);
     return;
 
 done:
@@ -315,6 +291,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     IDEState *s = idebus_active_if(&m->bus);
     int64_t sector_num;
     int nsector, remainder;
+    int64_t offset;
 
     MACIO_DPRINTF("pmac_ide_transfer_cb\n");
 
@@ -349,6 +326,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
     /* Calculate number of sectors */
     sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
+    offset = (ide_get_sector(s) << 9) + s->io_buffer_index;
     nsector = (io->len + 0x1ff) >> 9;
     remainder = io->len & 0x1ff;
 
@@ -359,7 +337,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
-        pmac_dma_read(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io);
+        pmac_dma_read(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_WRITE:
         pmac_dma_write(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io);
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/4] macio: switch pmac_dma_write() over to new offset/len implementation

[Mark Cave-Ayland]

In particular, this fixes a bug whereby chains of overlapping head/tail chains
would incorrectly write over each other's remainder cache. This is the access
pattern used by OS X/Darwin and fixes an issue with a corrupt Darwin
installation in my local tests.

While we are here, rename the DBDMA_io struct property remainder to
head_remainder for clarification.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c             |  116 ++++++++++++++++++++------------------------
 include/hw/ppc/mac_dbdma.h |    3 +-
 2 files changed, 55 insertions(+), 64 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index f1ac001..92f35e2 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -86,7 +86,7 @@ static void pmac_dma_read(BlockBackend *blk,
         MACIO_DPRINTF("--- DMA unaligned head: sector %ld, "
                       "discarding %ld bytes\n", sector_num, head_bytes);
 
-        qemu_iovec_add(&io->iov, &io->remainder, head_bytes);
+        qemu_iovec_add(&io->iov, &io->head_remainder, head_bytes);
 
         bytes += offset & (align - 1);
         offset = offset & ~(align - 1);
@@ -100,7 +100,7 @@ static void pmac_dma_read(BlockBackend *blk,
         MACIO_DPRINTF("--- DMA unaligned tail: sector %ld, "
                       "discarding bytes %ld\n", sector_num, tail_bytes);
 
-        qemu_iovec_add(&io->iov, &io->remainder, align - tail_bytes);
+        qemu_iovec_add(&io->iov, &io->tail_remainder, align - tail_bytes);
         bytes = ROUND_UP(bytes, align);
     }
 
@@ -117,7 +117,7 @@ static void pmac_dma_read(BlockBackend *blk,
 }
 
 static void pmac_dma_write(BlockBackend *blk,
-                         int64_t sector_num, int nb_sectors,
+                         int64_t offset, int bytes,
                          void (*cb)(void *opaque, int ret), void *opaque)
 {
     DBDMA_io *io = opaque;
@@ -125,90 +125,80 @@ static void pmac_dma_write(BlockBackend *blk,
     IDEState *s = idebus_active_if(&m->bus);
     dma_addr_t dma_addr, dma_len;
     void *mem;
-    int nsector, remainder;
-    int extra = 0;
+    int64_t sector_num;
+    int nsector;
+    uint64_t align = BDRV_SECTOR_SIZE;
+    size_t head_bytes, tail_bytes;
+    bool unaligned_head = false, unaligned_tail = false;
 
     qemu_iovec_destroy(&io->iov);
     qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
 
-    if (io->remainder_len > 0) {
-        /* Return remainder of request */
-        int transfer = MIN(io->remainder_len, io->len);
+    sector_num = (offset >> 9);
+    nsector = (io->len >> 9);
 
-        MACIO_DPRINTF("--- processing write remainder %x\n", transfer);
-        cpu_physical_memory_read(io->addr,
-                                 &io->remainder + (0x200 - transfer),
-                                 transfer);
+    MACIO_DPRINTF("--- DMA write transfer (0x%" HWADDR_PRIx ",0x%x): "
+                  "sector_num: %ld, nsector: %d\n", io->addr, io->len,
+                  sector_num, nsector);
 
-        io->remainder_len -= transfer;
-        io->len -= transfer;
-        io->addr += transfer;
+    dma_addr = io->addr;
+    dma_len = io->len;
+    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
+                         DMA_DIRECTION_TO_DEVICE);
 
-        s->io_buffer_index += transfer;
-        s->io_buffer_size -= transfer;
+    if (offset & (align - 1)) {
+        head_bytes = offset & (align - 1);
+        sector_num = ((offset & ~(align - 1)) >> 9);
 
-        if (io->remainder_len != 0) {
-            /* Still waiting for remainder */
-            return;
-        }
+        MACIO_DPRINTF("--- DMA unaligned head: pre-reading head sector %ld\n",
+                      sector_num);
 
-        MACIO_DPRINTF("--> prepending bounce buffer with size 0x200\n");
+        blk_pread(s->blk, (sector_num << 9), &io->head_remainder, align);
 
-        /* Sector transfer complete - prepend to request */
-        qemu_iovec_add(&io->iov, &io->remainder, 0x200);
-        extra = 1;
-    }
+        qemu_iovec_add(&io->iov, &io->head_remainder, head_bytes);
+        qemu_iovec_add(&io->iov, mem, io->len);
 
-    if (s->drive_kind == IDE_CD) {
-        sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9);
-    } else {
-        sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
+        bytes += offset & (align - 1);
+        offset = offset & ~(align - 1);
+
+        unaligned_head = true;
     }
 
-    nsector = (io->len >> 9);
-    remainder = io->len - (nsector << 9);
+    if ((offset + bytes) & (align - 1)) {
+        tail_bytes = (offset + bytes) & (align - 1);
+        sector_num = (((offset + bytes) & ~(align - 1)) >> 9);
 
-    MACIO_DPRINTF("--- DMA write transfer - addr: %" HWADDR_PRIx " len: %x\n",
-                  io->addr, io->len);
-    MACIO_DPRINTF("xxx remainder: %x\n", remainder);
-    MACIO_DPRINTF("xxx sector_num: %"PRIx64"   nsector: %x\n",
-                  sector_num, nsector);
+        MACIO_DPRINTF("--- DMA unaligned tail: pre-reading tail sector %ld\n",
+                      sector_num);
 
-    dma_addr = io->addr;
-    dma_len = io->len;
-    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
-                         DMA_DIRECTION_TO_DEVICE);
+        blk_pread(s->blk, (sector_num << 9), &io->tail_remainder, align);
 
-    if (!remainder) {
-        MACIO_DPRINTF("--- DMA write aligned - addr: %" HWADDR_PRIx
-                      " len: %x\n", io->addr, io->len);
-        qemu_iovec_add(&io->iov, mem, io->len);
-    } else {
-        /* Write up to last complete sector */
-        MACIO_DPRINTF("--- DMA write unaligned - addr: %" HWADDR_PRIx
-                      " len: %x\n", io->addr, (nsector << 9));
-        qemu_iovec_add(&io->iov, mem, (nsector << 9));
+        if (!unaligned_head) {
+            qemu_iovec_add(&io->iov, mem, io->len);
+        }
 
-        MACIO_DPRINTF("--- DMA write read    - bounce addr: %p "
-                      "remainder_len: %x\n", &io->remainder, remainder);
-        cpu_physical_memory_read(io->addr + (nsector << 9), &io->remainder,
-                                 remainder);
+        qemu_iovec_add(&io->iov, &io->tail_remainder + tail_bytes,
+                       align - tail_bytes);
+
+        bytes = ROUND_UP(bytes, align);
 
-        io->remainder_len = 0x200 - remainder;
+        unaligned_tail = true;
+    }
 
-        MACIO_DPRINTF("xxx remainder_len: %x\n", io->remainder_len);
+    if (!unaligned_head && !unaligned_tail) {
+        qemu_iovec_add(&io->iov, mem, io->len);
     }
 
-    s->io_buffer_size -= ((nsector + extra) << 9);
-    s->io_buffer_index += ((nsector + extra) << 9);
+    s->io_buffer_size -= io->len;
+    s->io_buffer_index += io->len;
 
     io->len = 0;
 
-    MACIO_DPRINTF("--- Block write transfer   - sector_num: %"PRIx64"  "
-                  "nsector: %x\n", sector_num, nsector + extra);
+    MACIO_DPRINTF("--- Block write transfer - sector_num: %" PRIx64 "  "
+                  "nsector: %x\n", (offset >> 9), (bytes >> 9));
 
-    m->aiocb = blk_aio_writev(blk, sector_num, &io->iov, nsector + extra, cb,
-                              io);
+    m->aiocb = blk_aio_writev(blk, (offset >> 9), &io->iov, (bytes >> 9),
+                              cb, io);
 }
 
 static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
@@ -340,7 +330,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         pmac_dma_read(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_WRITE:
-        pmac_dma_write(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io);
+        pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_TRIM:
         MACIO_DPRINTF("TRIM command issued!");
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index c580327..7f247fa 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -40,7 +40,8 @@ struct DBDMA_io {
     /* DMA is in progress, don't start another one */
     bool processing;
     /* unaligned last sector of a request */
-    uint8_t remainder[0x200];
+    uint8_t head_remainder[0x200];
+    uint8_t tail_remainder[0x200];
     int remainder_len;
     QEMUIOVector iov;
 };
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/4] macio: update comment/constants to reflect the new code

[Mark Cave-Ayland]

With the offset/len functions taking care of all of the alignment mapping
in isolation from the DMA tranasaction, many comments are now unnecessary.
Remove these and tidy up a few constants at the same time.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c |   46 +++++++++++++---------------------------------
 1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 92f35e2..f722bbc 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -51,6 +51,13 @@ static const int debug_macio = 0;
 
 #define MACIO_PAGE_SIZE 4096
 
+/*
+ * Unaligned DMA read/write access functions required for OS X/Darwin which
+ * don't perform DMA transactions on sector boundaries. These functions are
+ * modelled on bdrv_co_do_preadv()/bdrv_co_do_pwritev() and so should be
+ * easy to remove if the unaligned block APIs are ever exposed.
+ */
+
 static void pmac_dma_read(BlockBackend *blk,
                           int64_t offset, unsigned int bytes,
                           void (*cb)(void *opaque, int ret), void *opaque)
@@ -206,20 +213,12 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     DBDMA_io *io = opaque;
     MACIOIDEState *m = io->opaque;
     IDEState *s = idebus_active_if(&m->bus);
-    int64_t sector_num;
-    int nsector, remainder;
     int64_t offset;
 
-    MACIO_DPRINTF("\ns is %p\n", s);
-    MACIO_DPRINTF("io_buffer_index: %x\n", s->io_buffer_index);
-    MACIO_DPRINTF("io_buffer_size: %x   packet_transfer_size: %x\n",
-                  s->io_buffer_size, s->packet_transfer_size);
-    MACIO_DPRINTF("lba: %x\n", s->lba);
-    MACIO_DPRINTF("io_addr: %" HWADDR_PRIx "  io_len: %x\n", io->addr,
-                  io->len);
+    MACIO_DPRINTF("pmac_ide_atapi_transfer_cb\n");
 
     if (ret < 0) {
-        MACIO_DPRINTF("THERE WAS AN ERROR!  %d\n", ret);
+        MACIO_DPRINTF("DMA error: %d\n", ret);
         ide_atapi_io_error(s, ret);
         goto done;
     }
@@ -233,6 +232,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     }
 
     if (s->io_buffer_size <= 0) {
+        MACIO_DPRINTF("End of IDE transfer\n");
         ide_atapi_cmd_ok(s);
         m->dma_active = false;
         goto done;
@@ -252,22 +252,13 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
         goto done;
     }
 
-    /* Calculate number of sectors */
-    sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9);
-    nsector = (io->len + 0x1ff) >> 9;
-    remainder = io->len & 0x1ff;
-
     /* Calculate current offset */
     offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
 
-    MACIO_DPRINTF("nsector: %d   remainder: %x\n", nsector, remainder);
-    MACIO_DPRINTF("sector: %"PRIx64"   %zx\n", sector_num, io->iov.size / 512);
-
     pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io);
     return;
 
 done:
-    MACIO_DPRINTF("done DMA\n\n");
     block_acct_done(blk_get_stats(s->blk), &s->acct);
     io->dma_end(opaque);
 
@@ -279,14 +270,12 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     DBDMA_io *io = opaque;
     MACIOIDEState *m = io->opaque;
     IDEState *s = idebus_active_if(&m->bus);
-    int64_t sector_num;
-    int nsector, remainder;
     int64_t offset;
 
     MACIO_DPRINTF("pmac_ide_transfer_cb\n");
 
     if (ret < 0) {
-        MACIO_DPRINTF("DMA error\n");
+        MACIO_DPRINTF("DMA error: %d\n", ret);
         m->aiocb = NULL;
         ide_dma_error(s);
         io->remainder_len = 0;
@@ -302,7 +291,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     }
 
     if (s->io_buffer_size <= 0) {
-        MACIO_DPRINTF("end of transfer\n");
+        MACIO_DPRINTF("End of IDE transfer\n");
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
         m->dma_active = false;
@@ -315,15 +304,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     }
 
     /* Calculate number of sectors */
-    sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
     offset = (ide_get_sector(s) << 9) + s->io_buffer_index;
-    nsector = (io->len + 0x1ff) >> 9;
-    remainder = io->len & 0x1ff;
-
-    s->nsector -= nsector;
-
-    MACIO_DPRINTF("nsector: %d   remainder: %x\n", nsector, remainder);
-    MACIO_DPRINTF("sector: %"PRIx64"   %x\n", sector_num, nsector);
 
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
@@ -333,7 +314,6 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_TRIM:
-        MACIO_DPRINTF("TRIM command issued!");
         break;
     }
 
@@ -537,7 +517,7 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
     if (s->drive_kind == IDE_CD) {
         s->io_buffer_size = s->packet_transfer_size;
     } else {
-        s->io_buffer_size = s->nsector * 0x200;
+        s->io_buffer_size = s->nsector * BDRV_SECTOR_SIZE;
     }
 
     MACIO_DPRINTF("\n\n------------ IDE transfer\n");
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/4] macio: remove remainder_len DBDMA_io property

[Mark Cave-Ayland]

Since the block alignment code is now effectively independent of the DMA
implementation, this variable is no longer required and can be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c             |   13 -------------
 include/hw/ppc/mac_dbdma.h |    1 -
 2 files changed, 14 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index f722bbc..a3461a2 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -278,7 +278,6 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         MACIO_DPRINTF("DMA error: %d\n", ret);
         m->aiocb = NULL;
         ide_dma_error(s);
-        io->remainder_len = 0;
         goto done;
     }
 
@@ -509,9 +508,6 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
                             BlockCompletionFunc *cb)
 {
     MACIOIDEState *m = container_of(dma, MACIOIDEState, dma);
-    DBDMAState *dbdma = m->dbdma;
-    DBDMA_io *io;
-    int i;
 
     s->io_buffer_index = 0;
     if (s->drive_kind == IDE_CD) {
@@ -526,15 +522,6 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
     MACIO_DPRINTF("lba: %x    size: %x\n", s->lba, s->io_buffer_size);
     MACIO_DPRINTF("-------------------------\n");
 
-    for (i = 0; i < DBDMA_CHANNELS; i++) {
-        io = &dbdma->channels[i].io;
-
-        if (io->opaque == m) {
-            io->remainder_len = 0;
-        }
-    }
-
-    MACIO_DPRINTF("\n");
     m->dma_active = true;
     DBDMA_kick(m->dbdma);
 }
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 7f247fa..c687021 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -42,7 +42,6 @@ struct DBDMA_io {
     /* unaligned last sector of a request */
     uint8_t head_remainder[0x200];
     uint8_t tail_remainder[0x200];
-    int remainder_len;
     QEMUIOVector iov;
 };
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/4] macio: change DMA methods over to offset/len implementation

[John Snow]

On 05/31/2015 04:05 PM, Mark Cave-Ayland wrote:
> This patchset follows on from my recent work on fixing issues with the
> macio controller, and remodels the new pmac_dma_read() and pmac_dma_write()
> functions in a similar manner to the unaligned block functions.
> 
> With this in place, long chains of overlapping unaligned requests as used
> by OS X/Darwin will now work correctly without introducting torn sector
> errors when writing to disk.
> 
> Also included are some tidy-ups as a result of the above changes.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Mark Cave-Ayland (4):
>   macio: switch pmac_dma_read() over to new offset/len implementation
>   macio: switch pmac_dma_write() over to new offset/len implementation
>   macio: update comment/constants to reflect the new code
>   macio: remove remainder_len DBDMA_io property
> 
>  hw/ide/macio.c             |  271 +++++++++++++++++---------------------------
>  include/hw/ppc/mac_dbdma.h |    4 +-
>  2 files changed, 105 insertions(+), 170 deletions(-)
> 

More 32/64bit printf string problems:

macio.c:81:  sector_num is int64_t (PRId64)
macio.c:93:  sector_num
             head_bytes is size_t (%zu)
macio.c:107: sector_num
             tail_bytes is size_t (%zu)
macio.c:147: sector_num
macio.c:160: sector_num
macio.c:178: sector_num

But that's an unsatisfying response, so how about:

Tested-by: John Snow <jsnow@redhat.com>

Fixes the problem as far as I can tell. I'll comb it in a little more
detail later. Have you tested this patchset with OSX et al to make sure
it doesn't introduce any obvious regression on that side of things?

Thanks!
--js

Re: [Qemu-devel] [PATCH 0/4] macio: change DMA methods over to offset/len implementation

[Mark Cave-Ayland]

On 02/06/15 00:09, John Snow wrote:

> On 05/31/2015 04:05 PM, Mark Cave-Ayland wrote:
>> This patchset follows on from my recent work on fixing issues with the
>> macio controller, and remodels the new pmac_dma_read() and pmac_dma_write()
>> functions in a similar manner to the unaligned block functions.
>>
>> With this in place, long chains of overlapping unaligned requests as used
>> by OS X/Darwin will now work correctly without introducting torn sector
>> errors when writing to disk.
>>
>> Also included are some tidy-ups as a result of the above changes.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> Mark Cave-Ayland (4):
>>   macio: switch pmac_dma_read() over to new offset/len implementation
>>   macio: switch pmac_dma_write() over to new offset/len implementation
>>   macio: update comment/constants to reflect the new code
>>   macio: remove remainder_len DBDMA_io property
>>
>>  hw/ide/macio.c             |  271 +++++++++++++++++---------------------------
>>  include/hw/ppc/mac_dbdma.h |    4 +-
>>  2 files changed, 105 insertions(+), 170 deletions(-)
>>
> 
> More 32/64bit printf string problems:
> 
> macio.c:81:  sector_num is int64_t (PRId64)
> macio.c:93:  sector_num
>              head_bytes is size_t (%zu)
> macio.c:107: sector_num
>              tail_bytes is size_t (%zu)
> macio.c:147: sector_num
> macio.c:160: sector_num
> macio.c:178: sector_num

Ah oops. Do you need me to correct? And do you have a quick way of
testing a 32-bit build on a 64-bit OS? (-m32)?

> But that's an unsatisfying response, so how about:
> 
> Tested-by: John Snow <jsnow@redhat.com>
> 
> Fixes the problem as far as I can tell. I'll comb it in a little more
> detail later. Have you tested this patchset with OSX et al to make sure
> it doesn't introduce any obvious regression on that side of things?

Most of the work was done on Darwin (which definitely does unaligned
accesses) and I booted an OS X CDROM through to the point where the hard
disk started installing, so I'm reasonably confident in the patch. And
more so that it's based upon the existing block alignment code in io.c.

Basically the point of fixing up the -M g3beige/mac99 loadvm/savevm
(which is almost there except for DBDMA) in the last release was to help
debug this. At least I could get to a point where I could start QEMU
with -loadvm, run a single cp command and then md5 the results to check
for errors rather than having to wait for an entire OS install.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/4] macio: change DMA methods over to offset/len implementation

[John Snow]

On 06/01/2015 07:18 PM, Mark Cave-Ayland wrote:
> On 02/06/15 00:09, John Snow wrote:
> 
>> On 05/31/2015 04:05 PM, Mark Cave-Ayland wrote:
>>> This patchset follows on from my recent work on fixing issues with the
>>> macio controller, and remodels the new pmac_dma_read() and pmac_dma_write()
>>> functions in a similar manner to the unaligned block functions.
>>>
>>> With this in place, long chains of overlapping unaligned requests as used
>>> by OS X/Darwin will now work correctly without introducting torn sector
>>> errors when writing to disk.
>>>
>>> Also included are some tidy-ups as a result of the above changes.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> Mark Cave-Ayland (4):
>>>   macio: switch pmac_dma_read() over to new offset/len implementation
>>>   macio: switch pmac_dma_write() over to new offset/len implementation
>>>   macio: update comment/constants to reflect the new code
>>>   macio: remove remainder_len DBDMA_io property
>>>
>>>  hw/ide/macio.c             |  271 +++++++++++++++++---------------------------
>>>  include/hw/ppc/mac_dbdma.h |    4 +-
>>>  2 files changed, 105 insertions(+), 170 deletions(-)
>>>
>>
>> More 32/64bit printf string problems:
>>
>> macio.c:81:  sector_num is int64_t (PRId64)
>> macio.c:93:  sector_num
>>              head_bytes is size_t (%zu)
>> macio.c:107: sector_num
>>              tail_bytes is size_t (%zu)
>> macio.c:147: sector_num
>> macio.c:160: sector_num
>> macio.c:178: sector_num
> 
> Ah oops. Do you need me to correct? And do you have a quick way of
> testing a 32-bit build on a 64-bit OS? (-m32)?
> 

Unfortunately that's the best I've got. For my particular case I tend to
use this:

./configure --enable-debug '--extra-cflags=-m32
-I/usr/lib/glib-2.0/include' '--extra-ldflags=-m32 -L/usr/lib/iscsi'
--disable-glusterfs

and that helps guide my F21 through the unholy machinations necessary to
produce a 32-ish bit build. I've tried to fix this in the past, but I
keep running into edge cases for e.g. cross compilation and issues with
how different distros handle multi-lib/multi-arch, so it remains sort of
hacky and bad.

Wait to send v2 until after I look at the series a little more
carefully, in case there's something else.

>> But that's an unsatisfying response, so how about:
>>
>> Tested-by: John Snow <jsnow@redhat.com>
>>
>> Fixes the problem as far as I can tell. I'll comb it in a little more
>> detail later. Have you tested this patchset with OSX et al to make sure
>> it doesn't introduce any obvious regression on that side of things?
> 
> Most of the work was done on Darwin (which definitely does unaligned
> accesses) and I booted an OS X CDROM through to the point where the hard
> disk started installing, so I'm reasonably confident in the patch. And
> more so that it's based upon the existing block alignment code in io.c.
> 
> Basically the point of fixing up the -M g3beige/mac99 loadvm/savevm
> (which is almost there except for DBDMA) in the last release was to help
> debug this. At least I could get to a point where I could start QEMU
> with -loadvm, run a single cp command and then md5 the results to check
> for errors rather than having to wait for an entire OS install.
> 

If it's good enough for the mac-minded among us, it's good enough for me!

> 
> ATB,
> 
> Mark.
> 

Thanks again!
--js

Re: [Qemu-devel] [PATCH 0/4] macio: change DMA methods over to offset/len implementation

[Laurent Vivier]

----- Original Message -----
> From: "Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>
> To: "John Snow" <jsnow@redhat.com>, agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org
> Sent: Tuesday, June 2, 2015 1:18:42 AM
> Subject: Re: [Qemu-devel] [PATCH 0/4] macio: change DMA methods over to offset/len implementation
> 
> On 02/06/15 00:09, John Snow wrote:
> 
> > On 05/31/2015 04:05 PM, Mark Cave-Ayland wrote:
> >> This patchset follows on from my recent work on fixing issues with the
> >> macio controller, and remodels the new pmac_dma_read() and
> >> pmac_dma_write()
> >> functions in a similar manner to the unaligned block functions.
> >>
> >> With this in place, long chains of overlapping unaligned requests as used
> >> by OS X/Darwin will now work correctly without introducting torn sector
> >> errors when writing to disk.
> >>
> >> Also included are some tidy-ups as a result of the above changes.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>
> >> Mark Cave-Ayland (4):
> >>   macio: switch pmac_dma_read() over to new offset/len implementation
> >>   macio: switch pmac_dma_write() over to new offset/len implementation
> >>   macio: update comment/constants to reflect the new code
> >>   macio: remove remainder_len DBDMA_io property
> >>
> >>  hw/ide/macio.c             |  271
> >>  +++++++++++++++++---------------------------
> >>  include/hw/ppc/mac_dbdma.h |    4 +-
> >>  2 files changed, 105 insertions(+), 170 deletions(-)
> >>
> > 
> > More 32/64bit printf string problems:
> > 
> > macio.c:81:  sector_num is int64_t (PRId64)
> > macio.c:93:  sector_num
> >              head_bytes is size_t (%zu)
> > macio.c:107: sector_num
> >              tail_bytes is size_t (%zu)
> > macio.c:147: sector_num
> > macio.c:160: sector_num
> > macio.c:178: sector_num
> 
> Ah oops. Do you need me to correct? And do you have a quick way of
> testing a 32-bit build on a 64-bit OS? (-m32)?

You can create an LXC container with 32bit binaries on a 64bit kernel.

Something like "sudo lxc-create -t debian -n debian32 -- -a i686"
or "sudo lxc-create -t fedora -n fedora32 -- -a i686"

Available templates are: /usr/share/lxc/templates (remove the "lxc-")

Laurent

Re: [Qemu-devel] [PATCH 1/4] macio: switch pmac_dma_read() over to new offset/len implementation

[John Snow]

On 05/31/2015 04:05 PM, Mark Cave-Ayland wrote:
> For better handling of unaligned block device accesses.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/ide/macio.c |  102 ++++++++++++++++++++++----------------------------------
>  1 file changed, 40 insertions(+), 62 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 585a27b..f1ac001 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -52,7 +52,7 @@ static const int debug_macio = 0;
>  #define MACIO_PAGE_SIZE 4096
>  
>  static void pmac_dma_read(BlockBackend *blk,
> -                          int64_t sector_num, int nb_sectors,
> +                          int64_t offset, unsigned int bytes,
>                            void (*cb)(void *opaque, int ret), void *opaque)
>  {
>      DBDMA_io *io = opaque;
> @@ -60,76 +60,48 @@ static void pmac_dma_read(BlockBackend *blk,
>      IDEState *s = idebus_active_if(&m->bus);
>      dma_addr_t dma_addr, dma_len;
>      void *mem;
> -    int nsector, remainder;
> +    int64_t sector_num;
> +    int nsector;
> +    uint64_t align = BDRV_SECTOR_SIZE;
> +    size_t head_bytes, tail_bytes;
>  
>      qemu_iovec_destroy(&io->iov);
>      qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
>  
> -    if (io->remainder_len > 0) {
> -        /* Return remainder of request */
> -        int transfer = MIN(io->remainder_len, io->len);
> +    sector_num = (offset >> 9);
> +    nsector = (io->len >> 9);
>  
> -        MACIO_DPRINTF("--- DMA read pop     - bounce addr: %p addr: %"
> -                      HWADDR_PRIx " remainder_len: %x\n",
> -                      &io->remainder + (0x200 - transfer), io->addr,
> -                      io->remainder_len);
> +    MACIO_DPRINTF("--- DMA read transfer (0x%" HWADDR_PRIx ",0x%x): "
> +                  "sector_num: %ld, nsector: %d\n", io->addr, io->len,
> +                  sector_num, nsector);
>  
> -        cpu_physical_memory_write(io->addr,
> -                                  &io->remainder + (0x200 - transfer),
> -                                  transfer);
> +    dma_addr = io->addr;
> +    dma_len = io->len;
> +    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
> +                         DMA_DIRECTION_FROM_DEVICE);
>  
> -        io->remainder_len -= transfer;
> -        io->len -= transfer;
> -        io->addr += transfer;
> +    if (offset & (align - 1)) {
> +        head_bytes = offset & (align - 1);
>  
> -        s->io_buffer_index += transfer;
> -        s->io_buffer_size -= transfer;
> +        MACIO_DPRINTF("--- DMA unaligned head: sector %ld, "
> +                      "discarding %ld bytes\n", sector_num, head_bytes);
>  
> -        if (io->remainder_len != 0) {
> -            /* Still waiting for remainder */
> -            return;
> -        }
> +        qemu_iovec_add(&io->iov, &io->remainder, head_bytes);
>  
> -        if (io->len == 0) {
> -            MACIO_DPRINTF("--- finished all read processing; go and finish\n");
> -            cb(opaque, 0);
> -            return;
> -        }
> +        bytes += offset & (align - 1);
> +        offset = offset & ~(align - 1);
>      }
>  
> -    if (s->drive_kind == IDE_CD) {
> -        sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9);
> -    } else {
> -        sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
> -    }
> +    qemu_iovec_add(&io->iov, mem, io->len);
>  
> -    nsector = ((io->len + 0x1ff) >> 9);
> -    remainder = (nsector << 9) - io->len;
> +    if ((offset + bytes) & (align - 1)) {
> +        tail_bytes = (offset + bytes) & (align - 1);
>  
> -    MACIO_DPRINTF("--- DMA read transfer - addr: %" HWADDR_PRIx " len: %x\n",
> -                  io->addr, io->len);
> -
> -    dma_addr = io->addr;
> -    dma_len = io->len;
> -    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
> -                         DMA_DIRECTION_FROM_DEVICE);
> +        MACIO_DPRINTF("--- DMA unaligned tail: sector %ld, "
> +                      "discarding bytes %ld\n", sector_num, tail_bytes);
>  
> -    if (!remainder) {
> -        MACIO_DPRINTF("--- DMA read aligned - addr: %" HWADDR_PRIx
> -                      " len: %x\n", io->addr, io->len);
> -        qemu_iovec_add(&io->iov, mem, io->len);
> -    } else {
> -        MACIO_DPRINTF("--- DMA read unaligned - addr: %" HWADDR_PRIx
> -                      " len: %x\n", io->addr, io->len);
> -        qemu_iovec_add(&io->iov, mem, io->len);
> -
> -        MACIO_DPRINTF("--- DMA read push    - bounce addr: %p "
> -                      "remainder_len: %x\n",
> -                      &io->remainder + 0x200 - remainder, remainder);
> -        qemu_iovec_add(&io->iov, &io->remainder + 0x200 - remainder,
> -                       remainder);
> -
> -        io->remainder_len = remainder;
> +        qemu_iovec_add(&io->iov, &io->remainder, align - tail_bytes);
> +        bytes = ROUND_UP(bytes, align);
>      }
>  
>      s->io_buffer_size -= io->len;
> @@ -137,11 +109,11 @@ static void pmac_dma_read(BlockBackend *blk,
>  
>      io->len = 0;
>  
> -    MACIO_DPRINTF("--- Block read transfer   - sector_num: %"PRIx64"  "
> -                  "nsector: %x\n",
> -                  sector_num, nsector);
> +    MACIO_DPRINTF("--- Block read transfer - sector_num: %" PRIx64 "  "
> +                  "nsector: %x\n", (offset >> 9), (bytes >> 9));
>  
> -    m->aiocb = blk_aio_readv(blk, sector_num, &io->iov, nsector, cb, io);
> +    m->aiocb = blk_aio_readv(blk, (offset >> 9), &io->iov, (bytes >> 9),
> +                             cb, io);
>  }
>  
>  static void pmac_dma_write(BlockBackend *blk,
> @@ -246,6 +218,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>      IDEState *s = idebus_active_if(&m->bus);
>      int64_t sector_num;
>      int nsector, remainder;
> +    int64_t offset;
>  
>      MACIO_DPRINTF("\ns is %p\n", s);
>      MACIO_DPRINTF("io_buffer_index: %x\n", s->io_buffer_index);
> @@ -294,10 +267,13 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>      nsector = (io->len + 0x1ff) >> 9;
>      remainder = io->len & 0x1ff;
>  
> +    /* Calculate current offset */
> +    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
> +

Bike shedding: Worth an ATAPI_BLOCK_SIZE_BITS define or similar? I guess
we don't really currently try to avoid magic constants for 512, 2048,
etc etc anywhere else, so ... nevermind. Just a project for a different
day. Forget I said anything.

>      MACIO_DPRINTF("nsector: %d   remainder: %x\n", nsector, remainder);
>      MACIO_DPRINTF("sector: %"PRIx64"   %zx\n", sector_num, io->iov.size / 512);
>  
> -    pmac_dma_read(s->blk, sector_num, nsector, pmac_ide_atapi_transfer_cb, io);
> +    pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io);
>      return;
>  
>  done:
> @@ -315,6 +291,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>      IDEState *s = idebus_active_if(&m->bus);
>      int64_t sector_num;
>      int nsector, remainder;
> +    int64_t offset;
>  
>      MACIO_DPRINTF("pmac_ide_transfer_cb\n");
>  
> @@ -349,6 +326,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>  
>      /* Calculate number of sectors */
>      sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
> +    offset = (ide_get_sector(s) << 9) + s->io_buffer_index;
>      nsector = (io->len + 0x1ff) >> 9;
>      remainder = io->len & 0x1ff;
>  
> @@ -359,7 +337,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>  
>      switch (s->dma_cmd) {
>      case IDE_DMA_READ:
> -        pmac_dma_read(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io);
> +        pmac_dma_read(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>          break;
>      case IDE_DMA_WRITE:
>          pmac_dma_write(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io);
> 

Re: [Qemu-devel] [PATCH 0/4] macio: change DMA methods over to offset/len implementation

[John Snow]

On 05/31/2015 04:05 PM, Mark Cave-Ayland wrote:
> This patchset follows on from my recent work on fixing issues with the
> macio controller, and remodels the new pmac_dma_read() and pmac_dma_write()
> functions in a similar manner to the unaligned block functions.
> 
> With this in place, long chains of overlapping unaligned requests as used
> by OS X/Darwin will now work correctly without introducting torn sector
> errors when writing to disk.
> 
> Also included are some tidy-ups as a result of the above changes.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Mark Cave-Ayland (4):
>   macio: switch pmac_dma_read() over to new offset/len implementation
>   macio: switch pmac_dma_write() over to new offset/len implementation
>   macio: update comment/constants to reflect the new code
>   macio: remove remainder_len DBDMA_io property
> 
>  hw/ide/macio.c             |  271 +++++++++++++++++---------------------------
>  include/hw/ppc/mac_dbdma.h |    4 +-
>  2 files changed, 105 insertions(+), 170 deletions(-)
> 

Provided the printfs get fixed:

Reviewed-by: John Snow <jsnow@redhat.com>

If you resend, I'll stage it.

Thanks!
--js