[Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
DMA access logic for macio which provides the following benefits:

1) Reduced code complexity

The existing macio IDE/ATAPI functions were becoming extremely difficult to
follow through the various callbacks. By splitting up the functions in this
way it becomes much easier to follow the DMA-specific sections of code.

2) Future-proofing

If/when the block layer becomes able to handle unaligned DMA accesses directly
then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
with their unaligned-capable bdrv_*() equivalents without having to change any
other logic.

3) Fix intermittent CDROM detection under -M g3beige

The code refactoring now correctly handles non-block ATAPI transfers which
fixes the problem with intermittent CDROM detection with Darwin under
-M g3beige.

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


Mark Cave-Ayland (2):
  macio: move unaligned DMA read code into separate pmac_dma_read()
    function
  macio: move unaligned DMA write code into separate pmac_dma_write()
    function

 hw/ide/macio.c             |  487 +++++++++++++++++++++++---------------------
 include/hw/ppc/mac_dbdma.h |    4 -
 2 files changed, 254 insertions(+), 237 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [RFC 1/2] macio: move unaligned DMA read code into separate pmac_dma_read() function

[Mark Cave-Ayland]

This considerably helps simplify the complexity of the macio read routines and
by switching macio CDROM accesses to use the new code, fixes the issue with
the CDROM device being detected intermittently by Darwin/OS X.

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

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index f6074f2..4061bd6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -51,128 +51,165 @@ static const int debug_macio = 0;
 
 #define MACIO_PAGE_SIZE 4096
 
-static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
+static void pmac_dma_read(BlockBackend *blk,
+                          int64_t sector_num, int nb_sectors,
+                          void (*cb)(void *opaque, int ret), void *opaque)
 {
     DBDMA_io *io = opaque;
     MACIOIDEState *m = io->opaque;
     IDEState *s = idebus_active_if(&m->bus);
-    int unaligned;
+    dma_addr_t dma_addr, dma_len;
+    void *mem;
+    int nsector, remainder;
 
-    if (ret < 0) {
-        m->aiocb = NULL;
-        qemu_sglist_destroy(&s->sg);
-        ide_atapi_io_error(s, ret);
-        io->remainder_len = 0;
-        goto done;
+    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);
+
+        MACIO_DPRINTF("--- DMA read pop     - bounce addr: %p addr: %"
+                      HWADDR_PRIx " remainder_len: %x\n",
+                      &io->remainder + (0x200 - transfer), io->addr,
+                      io->remainder_len);
+
+        cpu_physical_memory_write(io->addr,
+                                  &io->remainder + (0x200 - transfer),
+                                  transfer);
+
+        io->remainder_len -= transfer;
+        io->len -= transfer;
+        io->addr += transfer;
+
+        s->io_buffer_index += transfer;
+        s->io_buffer_size -= transfer;
+
+        if (io->remainder_len != 0) {
+            /* Still waiting for remainder */
+            return;
+        }
+
+        if (io->len == 0) {
+            MACIO_DPRINTF("--- finished all read processing; go and finish\n");
+            cb(opaque, 0);
+            return;
+        }
     }
 
-    if (!m->dma_active) {
-        MACIO_DPRINTF("waiting for data (%#x - %#x - %x)\n",
-                      s->nsector, io->len, s->status);
-        /* data not ready yet, wait for the channel to get restarted */
-        io->processing = false;
-        return;
+    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);
     }
 
-    MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
+    nsector = ((io->len + 0x1ff) >> 9);
+    remainder = (nsector << 9) - io->len;
 
-    if (s->io_buffer_size > 0) {
-        m->aiocb = NULL;
-        qemu_sglist_destroy(&s->sg);
+    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);
+
+    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);
 
-        s->packet_transfer_size -= s->io_buffer_size;
+        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);
 
-        s->io_buffer_index += s->io_buffer_size;
-        s->lba += s->io_buffer_index >> 11;
-        s->io_buffer_index &= 0x7ff;
+        io->remainder_len = remainder;
     }
 
-    s->io_buffer_size = MIN(io->len, s->packet_transfer_size);
+    s->io_buffer_size -= io->len;
+    s->io_buffer_index += io->len;
 
-    MACIO_DPRINTF("remainder: %d io->len: %d size: %d\n", io->remainder_len,
-                  io->len, s->packet_transfer_size);
-    if (io->remainder_len && io->len) {
-        /* guest wants the rest of its previous transfer */
-        int remainder_len = MIN(io->remainder_len, io->len);
+    io->len = 0;
 
-        MACIO_DPRINTF("copying remainder %d bytes\n", remainder_len);
+    MACIO_DPRINTF("--- Block read transfer   - sector_num: %lx  nsector: %x\n",
+                  sector_num, nsector);
 
-        cpu_physical_memory_write(io->addr, io->remainder + 0x200 -
-                                  remainder_len, remainder_len);
+    m->aiocb = blk_aio_readv(blk, sector_num, &io->iov, nsector, cb, io);
+}
 
-        io->addr += remainder_len;
-        io->len -= remainder_len;
-        s->io_buffer_size = remainder_len;
-        io->remainder_len -= remainder_len;
-        /* treat remainder as individual transfer, start again */
-        qemu_sglist_init(&s->sg, DEVICE(m), io->len / MACIO_PAGE_SIZE + 1,
-                         &address_space_memory);
-        pmac_ide_atapi_transfer_cb(opaque, 0);
+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;
+
+    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);
+
+    if (ret < 0) {
+        MACIO_DPRINTF("THERE WAS AN ERROR!  %d\n", ret);
+        ide_atapi_io_error(s, ret);
+        goto done;
+    }
+
+    if (!m->dma_active) {
+        MACIO_DPRINTF("waiting for data (%#x - %#x - %x)\n",
+                      s->nsector, io->len, s->status);
+        /* data not ready yet, wait for the channel to get restarted */
+        io->processing = false;
         return;
     }
 
-    if (!s->packet_transfer_size) {
-        MACIO_DPRINTF("end of transfer\n");
+    if (s->io_buffer_size <= 0) {
         ide_atapi_cmd_ok(s);
         m->dma_active = false;
+        goto done;
     }
 
     if (io->len == 0) {
-        MACIO_DPRINTF("end of DMA\n");
+        MACIO_DPRINTF("End of DMA transfer\n");
         goto done;
     }
 
-    /* launch next transfer */
-
-    /* handle unaligned accesses first, get them over with and only do the
-       remaining bulk transfer using our async DMA helpers */
-    unaligned = io->len & 0x1ff;
-    if (unaligned) {
-        int sector_num = (s->lba << 2) + (s->io_buffer_index >> 9);
-        int nsector = io->len >> 9;
-
-        MACIO_DPRINTF("precopying unaligned %d bytes to %#" HWADDR_PRIx "\n",
-                      unaligned, io->addr + io->len - unaligned);
-
-        blk_read(s->blk, sector_num + nsector, io->remainder, 1);
-        cpu_physical_memory_write(io->addr + io->len - unaligned,
-                                  io->remainder, unaligned);
-
-        io->len -= unaligned;
-    }
-
-    MACIO_DPRINTF("io->len = %#x\n", io->len);
-
-    qemu_sglist_init(&s->sg, DEVICE(m), io->len / MACIO_PAGE_SIZE + 1,
-                     &address_space_memory);
-    qemu_sglist_add(&s->sg, io->addr, io->len);
-    io->addr += s->io_buffer_size;
-    io->remainder_len = MIN(s->packet_transfer_size - s->io_buffer_size,
-                            (0x200 - unaligned) & 0x1ff);
-    MACIO_DPRINTF("set remainder to: %d\n", io->remainder_len);
-
-    /* We would read no data from the block layer, thus not get a callback.
-       Just fake completion manually. */
-    if (!io->len) {
-        pmac_ide_atapi_transfer_cb(opaque, 0);
-        return;
+    if (s->lba == -1) {
+        /* Non-block ATAPI transfer - just copy to RAM */
+        s->io_buffer_size = MIN(s->io_buffer_size, io->len);
+        cpu_physical_memory_write(io->addr, s->io_buffer, s->io_buffer_size);
+        ide_atapi_cmd_ok(s);
+        m->dma_active = false;
+        goto done;
     }
 
-    io->len = 0;
+    /* 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;
 
-    MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n",
-                  (s->lba << 2) + (s->io_buffer_index >> 9),
-                  s->packet_transfer_size, s->dma_cmd);
+    MACIO_DPRINTF("nsector: %d   remainder: %x\n", nsector, remainder);
+    MACIO_DPRINTF("sector: %lx   %lx\n", sector_num, io->iov.size / 512);
 
-    m->aiocb = dma_blk_read(s->blk, &s->sg,
-                            (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9),
-                            pmac_ide_atapi_transfer_cb, io);
+    pmac_dma_read(s->blk, sector_num, nsector, pmac_ide_atapi_transfer_cb, io);
     return;
 
 done:
-    MACIO_DPRINTF("done DMA\n");
+    MACIO_DPRINTF("done DMA\n\n");
     block_acct_done(blk_get_stats(s->blk), &s->acct);
     io->dma_end(opaque);
+
+    return;
 }
 
 static void pmac_ide_transfer_cb(void *opaque, int ret)
@@ -364,33 +401,14 @@ static void pmac_ide_transfer(DBDMA_io *io)
 
     MACIO_DPRINTF("\n");
 
-    s->io_buffer_size = 0;
     if (s->drive_kind == IDE_CD) {
-
-        /* Handle non-block ATAPI DMA transfers */
-        if (s->lba == -1) {
-            s->io_buffer_size = MIN(io->len, s->packet_transfer_size);
-            block_acct_start(blk_get_stats(s->blk), &s->acct, s->io_buffer_size,
-                             BLOCK_ACCT_READ);
-            MACIO_DPRINTF("non-block ATAPI DMA transfer size: %d\n",
-                          s->io_buffer_size);
-
-            /* Copy ATAPI buffer directly to RAM and finish */
-            cpu_physical_memory_write(io->addr, s->io_buffer,
-                                      s->io_buffer_size);
-            ide_atapi_cmd_ok(s);
-            m->dma_active = false;
-
-            MACIO_DPRINTF("end of non-block ATAPI DMA transfer\n");
-            block_acct_done(blk_get_stats(s->blk), &s->acct);
-            io->dma_end(io);
-            return;
-        }
-
         block_acct_start(blk_get_stats(s->blk), &s->acct, io->len,
                          BLOCK_ACCT_READ);
+
         pmac_ide_atapi_transfer_cb(io, 0);
         return;
+    } else {
+        s->io_buffer_size = 0;
     }
 
     switch (s->dma_cmd) {
@@ -566,6 +584,28 @@ 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;
+
+    if (s->drive_kind == IDE_CD) {
+        s->io_buffer_index = 0;
+        s->io_buffer_size = s->packet_transfer_size;
+
+        MACIO_DPRINTF("\n\n------------ IDE transfer\n");
+        MACIO_DPRINTF("buffer_size: %x   buffer_index: %x\n",
+                      s->io_buffer_size, s->io_buffer_index);
+        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;
-- 
1.7.10.4

[Qemu-devel] [RFC 2/2] macio: move unaligned DMA write code into separate pmac_dma_write() function

[Mark Cave-Ayland]

Similarly switch the macio IDE routines over to use the new function and
tidy-up the remaining code as required.

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

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 4061bd6..47e64b1 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -143,6 +143,100 @@ static void pmac_dma_read(BlockBackend *blk,
     m->aiocb = blk_aio_readv(blk, sector_num, &io->iov, nsector, cb, io);
 }
 
+static void pmac_dma_write(BlockBackend *blk,
+                         int64_t sector_num, int nb_sectors,
+                         void (*cb)(void *opaque, int ret), void *opaque)
+{
+    DBDMA_io *io = opaque;
+    MACIOIDEState *m = io->opaque;
+    IDEState *s = idebus_active_if(&m->bus);
+    dma_addr_t dma_addr, dma_len;
+    void *mem;
+    int nsector, remainder;
+    int extra = 0;
+
+    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);
+
+        MACIO_DPRINTF("--- processing write remainder %x\n", transfer);
+        cpu_physical_memory_read(io->addr,
+                                 &io->remainder + (0x200 - transfer),
+                                 transfer);
+
+        io->remainder_len -= transfer;
+        io->len -= transfer;
+        io->addr += transfer;
+
+        s->io_buffer_index += transfer;
+        s->io_buffer_size -= transfer;
+
+        if (io->remainder_len != 0) {
+            /* Still waiting for remainder */
+            return;
+        }
+
+        MACIO_DPRINTF("--> prepending bounce buffer with size 0x200\n");
+
+        /* Sector transfer complete - prepend to request */
+        qemu_iovec_add(&io->iov, &io->remainder, 0x200);
+        extra = 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);
+    }
+
+    nsector = (io->len >> 9);
+    remainder = io->len - (nsector << 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: %lx   nsector: %x\n", sector_num, nsector);
+
+    dma_addr = io->addr;
+    dma_len = io->len;
+    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
+                         DMA_DIRECTION_TO_DEVICE);
+
+    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));
+
+        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);
+
+        io->remainder_len = 0x200 - remainder;
+
+        MACIO_DPRINTF("xxx remainder_len: %x\n", io->remainder_len);
+    }
+
+    s->io_buffer_size -= ((nsector + extra) << 9);
+    s->io_buffer_index += ((nsector + extra) << 9);
+
+    io->len = 0;
+
+    MACIO_DPRINTF("--- Block write transfer   - sector_num: %lx  "
+                  "nsector: %x\n", sector_num, nsector + extra);
+
+    m->aiocb = blk_aio_writev(blk, sector_num, &io->iov, nsector + extra, cb,
+                              io);
+}
+
 static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
 {
     DBDMA_io *io = opaque;
@@ -217,24 +311,19 @@ 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);
-    int n = 0;
     int64_t sector_num;
-    int unaligned;
+    int nsector, remainder;
+
+    MACIO_DPRINTF("pmac_ide_transfer_cb\n");
 
     if (ret < 0) {
         MACIO_DPRINTF("DMA error\n");
         m->aiocb = NULL;
-        qemu_sglist_destroy(&s->sg);
         ide_dma_error(s);
         io->remainder_len = 0;
         goto done;
     }
 
-    if (--io->requests) {
-        /* More requests still in flight */
-        return;
-    }
-
     if (!m->dma_active) {
         MACIO_DPRINTF("waiting for data (%#x - %#x - %x)\n",
                       s->nsector, io->len, s->status);
@@ -243,155 +332,48 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         return;
     }
 
-    sector_num = ide_get_sector(s);
-    MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
-    if (s->io_buffer_size > 0) {
-        m->aiocb = NULL;
-        qemu_sglist_destroy(&s->sg);
-        n = (s->io_buffer_size + 0x1ff) >> 9;
-        sector_num += n;
-        ide_set_sector(s, sector_num);
-        s->nsector -= n;
-    }
-
-    if (io->finish_remain_read) {
-        /* Finish a stale read from the last iteration */
-        io->finish_remain_read = false;
-        cpu_physical_memory_write(io->finish_addr, io->remainder,
-                                  io->finish_len);
-    }
-
-    MACIO_DPRINTF("remainder: %d io->len: %d nsector: %d "
-                  "sector_num: %" PRId64 "\n",
-                  io->remainder_len, io->len, s->nsector, sector_num);
-    if (io->remainder_len && io->len) {
-        /* guest wants the rest of its previous transfer */
-        int remainder_len = MIN(io->remainder_len, io->len);
-        uint8_t *p = &io->remainder[0x200 - remainder_len];
-
-        MACIO_DPRINTF("copying remainder %d bytes at %#" HWADDR_PRIx "\n",
-                      remainder_len, io->addr);
-
-        switch (s->dma_cmd) {
-        case IDE_DMA_READ:
-            cpu_physical_memory_write(io->addr, p, remainder_len);
-            break;
-        case IDE_DMA_WRITE:
-            cpu_physical_memory_read(io->addr, p, remainder_len);
-            break;
-        case IDE_DMA_TRIM:
-            break;
-        }
-        io->addr += remainder_len;
-        io->len -= remainder_len;
-        io->remainder_len -= remainder_len;
-
-        if (s->dma_cmd == IDE_DMA_WRITE && !io->remainder_len) {
-            io->requests++;
-            qemu_iovec_reset(&io->iov);
-            qemu_iovec_add(&io->iov, io->remainder, 0x200);
-
-            m->aiocb = blk_aio_writev(s->blk, sector_num - 1, &io->iov, 1,
-                                      pmac_ide_transfer_cb, io);
-        }
-    }
-
-    if (s->nsector == 0 && !io->remainder_len) {
+    if (s->io_buffer_size <= 0) {
         MACIO_DPRINTF("end of transfer\n");
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
         m->dma_active = false;
+        goto done;
     }
 
     if (io->len == 0) {
-        MACIO_DPRINTF("end of DMA\n");
+        MACIO_DPRINTF("End of DMA transfer\n");
         goto done;
     }
 
-    /* launch next transfer */
-
-    s->io_buffer_index = 0;
-    s->io_buffer_size = MIN(io->len, s->nsector * 512);
-
-    /* handle unaligned accesses first, get them over with and only do the
-       remaining bulk transfer using our async DMA helpers */
-    unaligned = io->len & 0x1ff;
-    if (unaligned) {
-        int nsector = io->len >> 9;
-
-        MACIO_DPRINTF("precopying unaligned %d bytes to %#" HWADDR_PRIx "\n",
-                      unaligned, io->addr + io->len - unaligned);
-
-        switch (s->dma_cmd) {
-        case IDE_DMA_READ:
-            io->requests++;
-            io->finish_addr = io->addr + io->len - unaligned;
-            io->finish_len = unaligned;
-            io->finish_remain_read = true;
-            qemu_iovec_reset(&io->iov);
-            qemu_iovec_add(&io->iov, io->remainder, 0x200);
-
-            m->aiocb = blk_aio_readv(s->blk, sector_num + nsector, &io->iov, 1,
-                                     pmac_ide_transfer_cb, io);
-            break;
-        case IDE_DMA_WRITE:
-            /* cache the contents in our io struct */
-            cpu_physical_memory_read(io->addr + io->len - unaligned,
-                                     io->remainder + io->remainder_len,
-                                     unaligned);
-            break;
-        case IDE_DMA_TRIM:
-            break;
-        }
-    }
-
-    MACIO_DPRINTF("io->len = %#x\n", io->len);
-
-    qemu_sglist_init(&s->sg, DEVICE(m), io->len / MACIO_PAGE_SIZE + 1,
-                     &address_space_memory);
-    qemu_sglist_add(&s->sg, io->addr, io->len);
-    io->addr += io->len + unaligned;
-    io->remainder_len = (0x200 - unaligned) & 0x1ff;
-    MACIO_DPRINTF("set remainder to: %d\n", io->remainder_len);
-
-    /* Only subsector reads happening */
-    if (!io->len) {
-        if (!io->requests) {
-            io->requests++;
-            pmac_ide_transfer_cb(opaque, ret);
-        }
-        return;
-    }
+    /* Calculate number of sectors */
+    sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9);
+    nsector = (io->len + 0x1ff) >> 9;
+    remainder = io->len & 0x1ff;
 
-    io->len = 0;
+    s->nsector -= nsector;
 
-    MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n",
-                  sector_num, n, s->nsector, s->dma_cmd);
+    MACIO_DPRINTF("nsector: %d   remainder: %x\n", nsector, remainder);
+    MACIO_DPRINTF("sector: %lx   %x\n", sector_num, nsector);
 
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
-        m->aiocb = dma_blk_read(s->blk, &s->sg, sector_num,
-                                pmac_ide_transfer_cb, io);
+        pmac_dma_read(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_WRITE:
-        m->aiocb = dma_blk_write(s->blk, &s->sg, sector_num,
-                                 pmac_ide_transfer_cb, io);
+        pmac_dma_write(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_TRIM:
-        m->aiocb = dma_blk_io(s->blk, &s->sg, sector_num,
-                              ide_issue_trim, pmac_ide_transfer_cb, io,
-                              DMA_DIRECTION_TO_DEVICE);
+        MACIO_DPRINTF("TRIM command issued!");
         break;
     }
 
-    io->requests++;
     return;
 
 done:
     if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
         block_acct_done(blk_get_stats(s->blk), &s->acct);
     }
-    io->dma_end(io);
+    io->dma_end(opaque);
 }
 
 static void pmac_ide_transfer(DBDMA_io *io)
@@ -407,8 +389,6 @@ static void pmac_ide_transfer(DBDMA_io *io)
 
         pmac_ide_atapi_transfer_cb(io, 0);
         return;
-    } else {
-        s->io_buffer_size = 0;
     }
 
     switch (s->dma_cmd) {
@@ -424,7 +404,6 @@ static void pmac_ide_transfer(DBDMA_io *io)
         break;
     }
 
-    io->requests++;
     pmac_ide_transfer_cb(io, 0);
 }
 
@@ -588,22 +567,24 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
     DBDMA_io *io;
     int i;
 
+    s->io_buffer_index = 0;
     if (s->drive_kind == IDE_CD) {
-        s->io_buffer_index = 0;
         s->io_buffer_size = s->packet_transfer_size;
+    } else {
+        s->io_buffer_size = s->nsector * 0x200;
+    }
 
-        MACIO_DPRINTF("\n\n------------ IDE transfer\n");
-        MACIO_DPRINTF("buffer_size: %x   buffer_index: %x\n",
-                      s->io_buffer_size, s->io_buffer_index);
-        MACIO_DPRINTF("lba: %x    size: %x\n", s->lba, s->io_buffer_size);
-        MACIO_DPRINTF("-------------------------\n");
+    MACIO_DPRINTF("\n\n------------ IDE transfer\n");
+    MACIO_DPRINTF("buffer_size: %x   buffer_index: %x\n",
+                  s->io_buffer_size, s->io_buffer_index);
+    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;
+    for (i = 0; i < DBDMA_CHANNELS; i++) {
+        io = &dbdma->channels[i].io;
 
-            if (io->opaque == m) {
-                io->remainder_len = 0;
-            }
+        if (io->opaque == m) {
+            io->remainder_len = 0;
         }
     }
 
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index d7db06c..c580327 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -43,10 +43,6 @@ struct DBDMA_io {
     uint8_t remainder[0x200];
     int remainder_len;
     QEMUIOVector iov;
-    bool finish_remain_read;
-    hwaddr finish_addr;
-    hwaddr finish_len;
-    int requests;
 };
 
 /*
-- 
1.7.10.4

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Alexander Graf]

On 09.03.15 23:24, Mark Cave-Ayland wrote:
> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
> DMA access logic for macio which provides the following benefits:
> 
> 1) Reduced code complexity
> 
> The existing macio IDE/ATAPI functions were becoming extremely difficult to
> follow through the various callbacks. By splitting up the functions in this
> way it becomes much easier to follow the DMA-specific sections of code.
> 
> 2) Future-proofing
> 
> If/when the block layer becomes able to handle unaligned DMA accesses directly
> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
> with their unaligned-capable bdrv_*() equivalents without having to change any
> other logic.
> 
> 3) Fix intermittent CDROM detection under -M g3beige
> 
> The code refactoring now correctly handles non-block ATAPI transfers which
> fixes the problem with intermittent CDROM detection with Darwin under
> -M g3beige.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Works for me, I'd still prefer to just see unaligned bdrv_*() functions
and remove most of the logic we have here.

Either way, I think this is a reasonable intermediate step.


Alex

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

On Mon, Mar 09, 2015 at 10:24:51PM +0000, Mark Cave-Ayland wrote:
> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
> DMA access logic for macio which provides the following benefits:
> 
> 1) Reduced code complexity
> 
> The existing macio IDE/ATAPI functions were becoming extremely difficult to
> follow through the various callbacks. By splitting up the functions in this
> way it becomes much easier to follow the DMA-specific sections of code.
> 
> 2) Future-proofing
> 
> If/when the block layer becomes able to handle unaligned DMA accesses directly
> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
> with their unaligned-capable bdrv_*() equivalents without having to change any
> other logic.
> 
> 3) Fix intermittent CDROM detection under -M g3beige
> 
> The code refactoring now correctly handles non-block ATAPI transfers which
> fixes the problem with intermittent CDROM detection with Darwin under
> -M g3beige.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (2):
>   macio: move unaligned DMA read code into separate pmac_dma_read()
>     function
>   macio: move unaligned DMA write code into separate pmac_dma_write()
>     function
> 
>  hw/ide/macio.c             |  487 +++++++++++++++++++++++---------------------
>  include/hw/ppc/mac_dbdma.h |    4 -
>  2 files changed, 254 insertions(+), 237 deletions(-)

CCing John Snow, who is now maintaining IDE/ATAPI/AHCI.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

On 17/03/15 07:23, Alexander Graf wrote:

> On 09.03.15 23:24, Mark Cave-Ayland wrote:
>> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
>> DMA access logic for macio which provides the following benefits:
>>
>> 1) Reduced code complexity
>>
>> The existing macio IDE/ATAPI functions were becoming extremely difficult to
>> follow through the various callbacks. By splitting up the functions in this
>> way it becomes much easier to follow the DMA-specific sections of code.
>>
>> 2) Future-proofing
>>
>> If/when the block layer becomes able to handle unaligned DMA accesses directly
>> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
>> with their unaligned-capable bdrv_*() equivalents without having to change any
>> other logic.
>>
>> 3) Fix intermittent CDROM detection under -M g3beige
>>
>> The code refactoring now correctly handles non-block ATAPI transfers which
>> fixes the problem with intermittent CDROM detection with Darwin under
>> -M g3beige.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Works for me, I'd still prefer to just see unaligned bdrv_*() functions
> and remove most of the logic we have here.
> 
> Either way, I think this is a reasonable intermediate step.

Ping? John - I've added you as CC as per Stefan's request.


ATB,

Mark.

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[John Snow]

On 04/28/2015 04:57 PM, Mark Cave-Ayland wrote:
> On 17/03/15 07:23, Alexander Graf wrote:
>
>> On 09.03.15 23:24, Mark Cave-Ayland wrote:
>>> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
>>> DMA access logic for macio which provides the following benefits:
>>>
>>> 1) Reduced code complexity
>>>
>>> The existing macio IDE/ATAPI functions were becoming extremely difficult to
>>> follow through the various callbacks. By splitting up the functions in this
>>> way it becomes much easier to follow the DMA-specific sections of code.
>>>
>>> 2) Future-proofing
>>>
>>> If/when the block layer becomes able to handle unaligned DMA accesses directly
>>> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
>>> with their unaligned-capable bdrv_*() equivalents without having to change any
>>> other logic.
>>>
>>> 3) Fix intermittent CDROM detection under -M g3beige
>>>
>>> The code refactoring now correctly handles non-block ATAPI transfers which
>>> fixes the problem with intermittent CDROM detection with Darwin under
>>> -M g3beige.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> Works for me, I'd still prefer to just see unaligned bdrv_*() functions
>> and remove most of the logic we have here.
>>
>> Either way, I think this is a reasonable intermediate step.
>
> Ping? John - I've added you as CC as per Stefan's request.
>
>
> ATB,
>
> Mark.
>

Yup, didn't forget. Plan to review/test tomorrow.

Thanks,
--js

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

On 28/04/15 22:07, John Snow wrote:

> On 04/28/2015 04:57 PM, Mark Cave-Ayland wrote:
>> On 17/03/15 07:23, Alexander Graf wrote:
>>
>>> On 09.03.15 23:24, Mark Cave-Ayland wrote:
>>>> This patchset attempts to separate out the IDE/ATAPI logic from the
>>>> unaligned
>>>> DMA access logic for macio which provides the following benefits:
>>>>
>>>> 1) Reduced code complexity
>>>>
>>>> The existing macio IDE/ATAPI functions were becoming extremely
>>>> difficult to
>>>> follow through the various callbacks. By splitting up the functions
>>>> in this
>>>> way it becomes much easier to follow the DMA-specific sections of code.
>>>>
>>>> 2) Future-proofing
>>>>
>>>> If/when the block layer becomes able to handle unaligned DMA
>>>> accesses directly
>>>> then it should be possible to switch out pmac_dma_read() and
>>>> pmac_dma_write()
>>>> with their unaligned-capable bdrv_*() equivalents without having to
>>>> change any
>>>> other logic.
>>>>
>>>> 3) Fix intermittent CDROM detection under -M g3beige
>>>>
>>>> The code refactoring now correctly handles non-block ATAPI transfers
>>>> which
>>>> fixes the problem with intermittent CDROM detection with Darwin under
>>>> -M g3beige.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> Works for me, I'd still prefer to just see unaligned bdrv_*() functions
>>> and remove most of the logic we have here.
>>>
>>> Either way, I think this is a reasonable intermediate step.
>>
>> Ping? John - I've added you as CC as per Stefan's request.
>>
>>
>> ATB,
>>
>> Mark.
>>
> 
> Yup, didn't forget. Plan to review/test tomorrow.
> 
> Thanks,
> --js

Great, thanks! FWIW this patchset is a WIP to try and fix the bug
introduced by 3e300fa6ad4ee19b16339c25773dec8df0bfb982 which breaks
installation of my Darwin PPC test image (I still see exactly the same
problem with my patch, however the code paths are somewhat clearer). So
if you have some ideas as to why the above commit broke the install, I'd
be very interested to hear them - I can easily put together some
reproducer instructions.


ATB,

Mark.

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[John Snow]

On 03/09/2015 06:24 PM, Mark Cave-Ayland wrote:
> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
> DMA access logic for macio which provides the following benefits:
> 
> 1) Reduced code complexity
> 
> The existing macio IDE/ATAPI functions were becoming extremely difficult to
> follow through the various callbacks. By splitting up the functions in this
> way it becomes much easier to follow the DMA-specific sections of code.
> 
> 2) Future-proofing
> 
> If/when the block layer becomes able to handle unaligned DMA accesses directly
> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
> with their unaligned-capable bdrv_*() equivalents without having to change any
> other logic.
> 
> 3) Fix intermittent CDROM detection under -M g3beige
> 
> The code refactoring now correctly handles non-block ATAPI transfers which
> fixes the problem with intermittent CDROM detection with Darwin under
> -M g3beige.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (2):
>   macio: move unaligned DMA read code into separate pmac_dma_read()
>     function
>   macio: move unaligned DMA write code into separate pmac_dma_write()
>     function
> 
>  hw/ide/macio.c             |  487 +++++++++++++++++++++++---------------------
>  include/hw/ppc/mac_dbdma.h |    4 -
>  2 files changed, 254 insertions(+), 237 deletions(-)
> 

I haven't been able to produce meaningful debug info for the problem
this patchset tries to help highlight (yet?), but it is an improvement
nonetheless, so given that it doesn't appear to make anything worse, and
this version is much nicer to follow, I'll stage this for the IDE branch.

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

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

On 18/05/15 20:54, John Snow wrote:

> On 03/09/2015 06:24 PM, Mark Cave-Ayland wrote:
>> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
>> DMA access logic for macio which provides the following benefits:
>>
>> 1) Reduced code complexity
>>
>> The existing macio IDE/ATAPI functions were becoming extremely difficult to
>> follow through the various callbacks. By splitting up the functions in this
>> way it becomes much easier to follow the DMA-specific sections of code.
>>
>> 2) Future-proofing
>>
>> If/when the block layer becomes able to handle unaligned DMA accesses directly
>> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
>> with their unaligned-capable bdrv_*() equivalents without having to change any
>> other logic.
>>
>> 3) Fix intermittent CDROM detection under -M g3beige
>>
>> The code refactoring now correctly handles non-block ATAPI transfers which
>> fixes the problem with intermittent CDROM detection with Darwin under
>> -M g3beige.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>>
>> Mark Cave-Ayland (2):
>>   macio: move unaligned DMA read code into separate pmac_dma_read()
>>     function
>>   macio: move unaligned DMA write code into separate pmac_dma_write()
>>     function
>>
>>  hw/ide/macio.c             |  487 +++++++++++++++++++++++---------------------
>>  include/hw/ppc/mac_dbdma.h |    4 -
>>  2 files changed, 254 insertions(+), 237 deletions(-)
>>
> 
> I haven't been able to produce meaningful debug info for the problem
> this patchset tries to help highlight (yet?), but it is an improvement
> nonetheless, so given that it doesn't appear to make anything worse, and
> this version is much nicer to follow, I'll stage this for the IDE branch.
> 
> Acked-by: John Snow <jsnow@redhat.com>

Thanks John. Even though you haven't managed to figure out the problem
the patchset attempts to solve, were you at least able to reproduce the
image corruption locally?

That said, the patchset is still worth including just for the fact that
it fixes the flaky CDROM detection here.


ATB,

Mark.

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[John Snow]

On 05/19/2015 04:50 PM, Mark Cave-Ayland wrote:
> On 18/05/15 20:54, John Snow wrote:
> 
>> On 03/09/2015 06:24 PM, Mark Cave-Ayland wrote:
>>> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
>>> DMA access logic for macio which provides the following benefits:
>>>
>>> 1) Reduced code complexity
>>>
>>> The existing macio IDE/ATAPI functions were becoming extremely difficult to
>>> follow through the various callbacks. By splitting up the functions in this
>>> way it becomes much easier to follow the DMA-specific sections of code.
>>>
>>> 2) Future-proofing
>>>
>>> If/when the block layer becomes able to handle unaligned DMA accesses directly
>>> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
>>> with their unaligned-capable bdrv_*() equivalents without having to change any
>>> other logic.
>>>
>>> 3) Fix intermittent CDROM detection under -M g3beige
>>>
>>> The code refactoring now correctly handles non-block ATAPI transfers which
>>> fixes the problem with intermittent CDROM detection with Darwin under
>>> -M g3beige.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>>
>>> Mark Cave-Ayland (2):
>>>   macio: move unaligned DMA read code into separate pmac_dma_read()
>>>     function
>>>   macio: move unaligned DMA write code into separate pmac_dma_write()
>>>     function
>>>
>>>  hw/ide/macio.c             |  487 +++++++++++++++++++++++---------------------
>>>  include/hw/ppc/mac_dbdma.h |    4 -
>>>  2 files changed, 254 insertions(+), 237 deletions(-)
>>>
>>
>> I haven't been able to produce meaningful debug info for the problem
>> this patchset tries to help highlight (yet?), but it is an improvement
>> nonetheless, so given that it doesn't appear to make anything worse, and
>> this version is much nicer to follow, I'll stage this for the IDE branch.
>>
>> Acked-by: John Snow <jsnow@redhat.com>
> 
> Thanks John. Even though you haven't managed to figure out the problem
> the patchset attempts to solve, were you at least able to reproduce the
> image corruption locally?
> 
> That said, the patchset is still worth including just for the fact that
> it fixes the flaky CDROM detection here.
> 
> 
> ATB,
> 
> Mark.
> 

Yeah, I reproduced the problem you're describing and spent a chunk of my
time debugging it and trying to figure out a section of the trace that
coincides with "the problem," but was unable to find anything of
particular interest.

I do notice that sometimes we appear to start a new transfer almost
immediately after one completes, but the code in place to sleep that
action until the guest finishes programming the DMA command seems to
catch it and nothing gets maliciously perturbed.

I still wonder somewhat that with the move to async and the strange
order of how darwin appears to program DMA transfers that we're hitting
some weird race, but I think that how reliably I hit the exact same
problem means that I should think again :)

I'll keep poking.

--js

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

On 19/05/15 22:01, John Snow wrote:

>> Thanks John. Even though you haven't managed to figure out the problem
>> the patchset attempts to solve, were you at least able to reproduce the
>> image corruption locally?
>>
>> That said, the patchset is still worth including just for the fact that
>> it fixes the flaky CDROM detection here.
>>
>>
>> ATB,
>>
>> Mark.
>>
> 
> Yeah, I reproduced the problem you're describing and spent a chunk of my
> time debugging it and trying to figure out a section of the trace that
> coincides with "the problem," but was unable to find anything of
> particular interest.

Well that's definitely a good start :)  I did spend some time enabling
tracepoints on the block layer for both good and bad commits, and the
only obvious difference I could see was the batching between multiple
read/write requests.

> I do notice that sometimes we appear to start a new transfer almost
> immediately after one completes, but the code in place to sleep that
> action until the guest finishes programming the DMA command seems to
> catch it and nothing gets maliciously perturbed.
> 
> I still wonder somewhat that with the move to async and the strange
> order of how darwin appears to program DMA transfers that we're hitting
> some weird race, but I think that how reliably I hit the exact same
> problem means that I should think again :)
> 
> I'll keep poking.

The only other thing I had in the back of my mind was whether the async
code needs some kind of extra write barrier implemented when used in
this way. Unfortunately I haven't had a chance to dig into the block
layer to figure out how to attempt this yet.


ATB,

Mark.

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[John Snow]

On 03/09/2015 06:24 PM, Mark Cave-Ayland wrote:
> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
> DMA access logic for macio which provides the following benefits:
> 
> 1) Reduced code complexity
> 
> The existing macio IDE/ATAPI functions were becoming extremely difficult to
> follow through the various callbacks. By splitting up the functions in this
> way it becomes much easier to follow the DMA-specific sections of code.
> 
> 2) Future-proofing
> 
> If/when the block layer becomes able to handle unaligned DMA accesses directly
> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
> with their unaligned-capable bdrv_*() equivalents without having to change any
> other logic.
> 
> 3) Fix intermittent CDROM detection under -M g3beige
> 
> The code refactoring now correctly handles non-block ATAPI transfers which
> fixes the problem with intermittent CDROM detection with Darwin under
> -M g3beige.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (2):
>   macio: move unaligned DMA read code into separate pmac_dma_read()
>     function
>   macio: move unaligned DMA write code into separate pmac_dma_write()
>     function
> 
>  hw/ide/macio.c             |  487 +++++++++++++++++++++++---------------------
>  include/hw/ppc/mac_dbdma.h |    4 -
>  2 files changed, 254 insertions(+), 237 deletions(-)
> 

Code fails 32 bit build due to %lx debug prints. I'll edit them
accordingly if that is OK by you.

--js

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

On 22/05/15 18:55, John Snow wrote:

> On 03/09/2015 06:24 PM, Mark Cave-Ayland wrote:
>> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
>> DMA access logic for macio which provides the following benefits:
>>
>> 1) Reduced code complexity
>>
>> The existing macio IDE/ATAPI functions were becoming extremely difficult to
>> follow through the various callbacks. By splitting up the functions in this
>> way it becomes much easier to follow the DMA-specific sections of code.
>>
>> 2) Future-proofing
>>
>> If/when the block layer becomes able to handle unaligned DMA accesses directly
>> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
>> with their unaligned-capable bdrv_*() equivalents without having to change any
>> other logic.
>>
>> 3) Fix intermittent CDROM detection under -M g3beige
>>
>> The code refactoring now correctly handles non-block ATAPI transfers which
>> fixes the problem with intermittent CDROM detection with Darwin under
>> -M g3beige.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>>
>> Mark Cave-Ayland (2):
>>   macio: move unaligned DMA read code into separate pmac_dma_read()
>>     function
>>   macio: move unaligned DMA write code into separate pmac_dma_write()
>>     function
>>
>>  hw/ide/macio.c             |  487 +++++++++++++++++++++++---------------------
>>  include/hw/ppc/mac_dbdma.h |    4 -
>>  2 files changed, 254 insertions(+), 237 deletions(-)
>>
> 
> Code fails 32 bit build due to %lx debug prints. I'll edit them
> accordingly if that is OK by you.

Please go right ahead :)  Do you need a proper re-spin without the RFC
prefix? If so, I can make the changes there if that helps?


ATB,

Mark.

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[John Snow]

On 05/22/2015 02:16 PM, Mark Cave-Ayland wrote:
> On 22/05/15 18:55, John Snow wrote:
> 
>> On 03/09/2015 06:24 PM, Mark Cave-Ayland wrote:
>>> This patchset attempts to separate out the IDE/ATAPI logic from the unaligned
>>> DMA access logic for macio which provides the following benefits:
>>>
>>> 1) Reduced code complexity
>>>
>>> The existing macio IDE/ATAPI functions were becoming extremely difficult to
>>> follow through the various callbacks. By splitting up the functions in this
>>> way it becomes much easier to follow the DMA-specific sections of code.
>>>
>>> 2) Future-proofing
>>>
>>> If/when the block layer becomes able to handle unaligned DMA accesses directly
>>> then it should be possible to switch out pmac_dma_read() and pmac_dma_write()
>>> with their unaligned-capable bdrv_*() equivalents without having to change any
>>> other logic.
>>>
>>> 3) Fix intermittent CDROM detection under -M g3beige
>>>
>>> The code refactoring now correctly handles non-block ATAPI transfers which
>>> fixes the problem with intermittent CDROM detection with Darwin under
>>> -M g3beige.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>>
>>> Mark Cave-Ayland (2):
>>>   macio: move unaligned DMA read code into separate pmac_dma_read()
>>>     function
>>>   macio: move unaligned DMA write code into separate pmac_dma_write()
>>>     function
>>>
>>>  hw/ide/macio.c             |  487 +++++++++++++++++++++++---------------------
>>>  include/hw/ppc/mac_dbdma.h |    4 -
>>>  2 files changed, 254 insertions(+), 237 deletions(-)
>>>
>>
>> Code fails 32 bit build due to %lx debug prints. I'll edit them
>> accordingly if that is OK by you.
> 
> Please go right ahead :)  Do you need a proper re-spin without the RFC
> prefix? If so, I can make the changes there if that helps?
> 
> 
> ATB,
> 
> Mark.
> 

Nah, the tags seem to get dropped when you pull everything into git
anyway, so it's no biggie.

Thanks,
--js



As a post-script, can Darwin/PPC use a different mechanism for ATA at
all, or is macio the sole ATA interface we support here?

I want to see if I can pinpoint when a "good" bath and when a bad path
diverges with respect to the disk contents ...

Or if you have other ideas on how to identify the transfer that causes
the issue, I'm all ears.

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

On 22/05/15 19:20, John Snow wrote:

>>> Code fails 32 bit build due to %lx debug prints. I'll edit them
>>> accordingly if that is OK by you.
>>
>> Please go right ahead :)  Do you need a proper re-spin without the RFC
>> prefix? If so, I can make the changes there if that helps?
>>
>>
>> ATB,
>>
>> Mark.
>>
> 
> Nah, the tags seem to get dropped when you pull everything into git
> anyway, so it's no biggie.
> 
> Thanks,
> --js
> 
> 
> 
> As a post-script, can Darwin/PPC use a different mechanism for ATA at
> all, or is macio the sole ATA interface we support here?
> 
> I want to see if I can pinpoint when a "good" bath and when a bad path
> diverges with respect to the disk contents ...
> 
> Or if you have other ideas on how to identify the transfer that causes
> the issue, I'm all ears.

Mounting the ISO manually I can see there is CMD646ATA.kext in the
/System/Library/Extensions directory, so in theory you should be able to
add a CMD646 PCI interface (OpenBIOS will enumerate the PCI bus as
presented by QEMU) and it might work.


ATB,

Mark.

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[Mark Cave-Ayland]

On 22/05/15 19:20, John Snow wrote:

> As a post-script, can Darwin/PPC use a different mechanism for ATA at
> all, or is macio the sole ATA interface we support here?
> 
> I want to see if I can pinpoint when a "good" bath and when a bad path
> diverges with respect to the disk contents ...
> 
> Or if you have other ideas on how to identify the transfer that causes
> the issue, I'm all ears.

After a fairly involved weekend, I've managed to track this one down. It
was an actually an issue with the chaining of write requests with
unaligned accesses, whereby alternating patterns of head, head/tail and
tail would end up overwriting the common remainder section and so copy
the wrong sector back to the disk.

The bad news is that this has definitely been broken since QEMU 2.1+ so
anyone using a Darwin/OS X PPC image is very likely to have an
installation that won't boot, or corruption in an image if it was
originally installed on an older QEMU version :(

Note that as I figured out what was happening my code was morphing into
a very similar pattern to that of
bdrv_co_do_preadv()/bdrv_co_do_pwritev() (except mine still calculated
with sectors) and so I bit the bullet and moved them over to match the
existing style. This has the advantage that the algorithm is similar to
an existing one, plus if unaligned block APIs become available in the
future then switching them over should be fairly trivial.

I'll post an updated patchset shortly. John, if you could confirm it
fixes the issue on your side that would be great.


ATB,

Mark.

Re: [Qemu-devel] [RFC 0/2] macio: split out unaligned DMA access into separate functions

[John Snow]

On 05/31/2015 03:54 PM, Mark Cave-Ayland wrote:
> On 22/05/15 19:20, John Snow wrote:
> 
>> As a post-script, can Darwin/PPC use a different mechanism for ATA at
>> all, or is macio the sole ATA interface we support here?
>>
>> I want to see if I can pinpoint when a "good" bath and when a bad path
>> diverges with respect to the disk contents ...
>>
>> Or if you have other ideas on how to identify the transfer that causes
>> the issue, I'm all ears.
> 
> After a fairly involved weekend, I've managed to track this one down. It
> was an actually an issue with the chaining of write requests with
> unaligned accesses, whereby alternating patterns of head, head/tail and
> tail would end up overwriting the common remainder section and so copy
> the wrong sector back to the disk.
> 

Great!

> The bad news is that this has definitely been broken since QEMU 2.1+ so
> anyone using a Darwin/OS X PPC image is very likely to have an
> installation that won't boot, or corruption in an image if it was
> originally installed on an older QEMU version :(
> 

Bummer. On the other hand, the number of complaints I have seen is...
zero. So here's hoping it isn't the apocalypse.

> Note that as I figured out what was happening my code was morphing into
> a very similar pattern to that of
> bdrv_co_do_preadv()/bdrv_co_do_pwritev() (except mine still calculated
> with sectors) and so I bit the bullet and moved them over to match the
> existing style. This has the advantage that the algorithm is similar to
> an existing one, plus if unaligned block APIs become available in the
> future then switching them over should be fairly trivial.
> 
> I'll post an updated patchset shortly. John, if you could confirm it
> fixes the issue on your side that would be great.
> 
> 
> ATB,
> 
> Mark.
> 

Sure thing. Had unexpected PTO last week, so give me a pinch to clear
the todo list and I'll get to testing the patchset.

--js