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

[Qemu-devel] [PATCH v2 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>

v2:
  Fix debug format strings on 32-bit platforms
  Add John's Reviewed-by tags
  Rebase onto git master

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 v2 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>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 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..52ee4ac 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: %" PRId64 ", 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 %" PRId64 ", "
+                      "discarding %zu 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 %" PRId64 ", "
+                      "discarding bytes %zu\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 v2 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>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 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 52ee4ac..85e315f 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 %" PRId64 ", "
                       "discarding %zu 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 %" PRId64 ", "
                       "discarding bytes %zu\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: %" PRId64 ", 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 %"
+                      PRId64 "\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 %"
+                      PRId64 "\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 v2 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>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 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 85e315f..d353bd2 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 v2 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>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 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 d353bd2..dd52d50 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 v2 0/4] macio: change DMA methods over to offset/len implementation

[John Snow]

On 06/04/2015 05:59 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>
> 
> v2:
>   Fix debug format strings on 32-bit platforms
>   Add John's Reviewed-by tags
>   Rebase onto git master
> 
> 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(-)
> 

Thanks, staged:
https://github.com/jnsnow/qemu/commits/ide

(I really need to work on automating the "staged" emails, or at least
come up with a template for myself...)

--js