[Qemu-devel] [PATCH 0/2] macio: switch over to new byte-aligned DMA helpers

[Qemu-devel] [PATCH 0/2] macio: switch over to new byte-aligned DMA helpers

[Mark Cave-Ayland]

Here is a tidied up version of my patch to convert the macio controller over to
using the new byte-aligned DMA helpers.

The first patch is just a hack and temporarily disables unaligned iovec
truncation in the DMA helper (as discussed in the recent thread) until Paolo or
someone else can devise a proper solution. Without this, the subsequent switch
over to the DMA helpers will appear to work during a Darwin PPC install but the
resulting image is corrupt and will fail to boot.

The second patch is the real one and switches the macio controller over to use
the new byte-aligned DMA helpers. Here I see a speed-up of around 2.5x-3x for
a typical Darwin PPC installation compared to the previous code.

Aurelien, I'd be grateful if you could test the TRIM path as I know this is
something you've had issues with before and I couldn't quite figure out how to
reproduce your TRIM tests from before.

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

Mark Cave-Ayland (2):
  dma-helpers.c: [HACK] disable iovec truncation to nearest sector size
  macio: switch over to new byte-aligned DMA helpers

 dma-helpers.c  |    2 +
 hw/ide/macio.c |  213 ++++++++------------------------------------------------
 2 files changed, 30 insertions(+), 185 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/2] dma-helpers.c: [HACK] disable iovec truncation to nearest sector size

[Mark Cave-Ayland]

This is not a production fix but is included to allow the following patch to
work until a proper solution is found. Otherwise iovecs which are not an
exact multiple of 0x200 are artificially truncated causing corruption to the
unaligned accesses required by Darwin PPC on the macio controller.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 dma-helpers.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/dma-helpers.c b/dma-helpers.c
index b521d84..693394a 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -160,9 +160,11 @@ static void dma_blk_cb(void *opaque, int ret)
         return;
     }
 
+    /* FIXME: this breaks unaligned DMA accesses 
     if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
         qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
     }
+    */
 
     dbs->acb = dbs->io_func(dbs->offset, &dbs->iov,
                             dma_blk_cb, dbs, dbs->io_func_opaque);
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/2] macio: switch over to new byte-aligned DMA helpers

[Mark Cave-Ayland]

Now that the DMA helpers are byte-aligned they can be called directly from
the macio routines rather than emulating byte-aligned accesses via multiple
block-level accesses.

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

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 42ad68a..e4e567e 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -52,187 +52,6 @@ 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_preadv()/bdrv_co_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)
-{
-    DBDMA_io *io = opaque;
-    MACIOIDEState *m = io->opaque;
-    IDEState *s = idebus_active_if(&m->bus);
-    dma_addr_t dma_addr, dma_len;
-    void *mem;
-    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);
-
-    sector_num = (offset >> 9);
-    nsector = (io->len >> 9);
-
-    MACIO_DPRINTF("--- DMA read transfer (0x%" HWADDR_PRIx ",0x%x): "
-                  "sector_num: %" PRId64 ", nsector: %d\n", io->addr, io->len,
-                  sector_num, nsector);
-
-    dma_addr = io->addr;
-    dma_len = io->len;
-    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
-                         DMA_DIRECTION_FROM_DEVICE);
-
-    if (offset & (align - 1)) {
-        head_bytes = offset & (align - 1);
-
-        MACIO_DPRINTF("--- DMA unaligned head: sector %" PRId64 ", "
-                      "discarding %zu bytes\n", sector_num, head_bytes);
-
-        qemu_iovec_add(&io->iov, &io->head_remainder, head_bytes);
-
-        bytes += offset & (align - 1);
-        offset = offset & ~(align - 1);
-    }
-
-    qemu_iovec_add(&io->iov, mem, io->len);
-
-    if ((offset + bytes) & (align - 1)) {
-        tail_bytes = (offset + bytes) & (align - 1);
-
-        MACIO_DPRINTF("--- DMA unaligned tail: sector %" PRId64 ", "
-                      "discarding bytes %zu\n", sector_num, tail_bytes);
-
-        qemu_iovec_add(&io->iov, &io->tail_remainder, align - tail_bytes);
-        bytes = ROUND_UP(bytes, align);
-    }
-
-    s->io_buffer_size -= io->len;
-    s->io_buffer_index += io->len;
-
-    io->len = 0;
-
-    MACIO_DPRINTF("--- Block read transfer - sector_num: %" PRIx64 "  "
-                  "nsector: %x\n", (offset >> 9), (bytes >> 9));
-
-    s->bus->dma->aiocb = blk_aio_preadv(blk, offset, &io->iov, 0, cb, io);
-}
-
-static void pmac_dma_write(BlockBackend *blk,
-                         int64_t offset, int bytes,
-                         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;
-    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);
-
-    sector_num = (offset >> 9);
-    nsector = (io->len >> 9);
-
-    MACIO_DPRINTF("--- DMA write transfer (0x%" HWADDR_PRIx ",0x%x): "
-                  "sector_num: %" PRId64 ", nsector: %d\n", io->addr, io->len,
-                  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 (offset & (align - 1)) {
-        head_bytes = offset & (align - 1);
-        sector_num = ((offset & ~(align - 1)) >> 9);
-
-        MACIO_DPRINTF("--- DMA unaligned head: pre-reading head sector %"
-                      PRId64 "\n", sector_num);
-
-        blk_pread(s->blk, (sector_num << 9), &io->head_remainder, align);
-
-        qemu_iovec_add(&io->iov, &io->head_remainder, head_bytes);
-        qemu_iovec_add(&io->iov, mem, io->len);
-
-        bytes += offset & (align - 1);
-        offset = offset & ~(align - 1);
-
-        unaligned_head = true;
-    }
-
-    if ((offset + bytes) & (align - 1)) {
-        tail_bytes = (offset + bytes) & (align - 1);
-        sector_num = (((offset + bytes) & ~(align - 1)) >> 9);
-
-        MACIO_DPRINTF("--- DMA unaligned tail: pre-reading tail sector %"
-                      PRId64 "\n", sector_num);
-
-        blk_pread(s->blk, (sector_num << 9), &io->tail_remainder, align);
-
-        if (!unaligned_head) {
-            qemu_iovec_add(&io->iov, mem, io->len);
-        }
-
-        qemu_iovec_add(&io->iov, &io->tail_remainder + tail_bytes,
-                       align - tail_bytes);
-
-        bytes = ROUND_UP(bytes, align);
-
-        unaligned_tail = true;
-    }
-
-    if (!unaligned_head && !unaligned_tail) {
-        qemu_iovec_add(&io->iov, mem, io->len);
-    }
-
-    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", (offset >> 9), (bytes >> 9));
-
-    s->bus->dma->aiocb = blk_aio_pwritev(blk, offset, &io->iov, 0, cb, io);
-}
-
-static void pmac_dma_trim(BlockBackend *blk,
-                        int64_t offset, int bytes,
-                        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;
-
-    qemu_iovec_destroy(&io->iov);
-    qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
-
-    dma_addr = io->addr;
-    dma_len = io->len;
-    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
-                         DMA_DIRECTION_TO_DEVICE);
-
-    qemu_iovec_add(&io->iov, mem, io->len);
-    s->io_buffer_size -= io->len;
-    s->io_buffer_index += io->len;
-    io->len = 0;
-
-    s->bus->dma->aiocb = ide_issue_trim(offset, &io->iov, cb, io, blk);
-}
-
 static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
 {
     DBDMA_io *io = opaque;
@@ -244,6 +63,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
 
     if (ret < 0) {
         MACIO_DPRINTF("DMA error: %d\n", ret);
+        qemu_sglist_destroy(&s->sg);
         ide_atapi_io_error(s, ret);
         goto done;
     }
@@ -258,6 +78,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");
+        qemu_sglist_destroy(&s->sg);
         ide_atapi_cmd_ok(s);
         m->dma_active = false;
         goto done;
@@ -280,7 +101,15 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     /* Calculate current offset */
     offset = ((int64_t)s->lba << 11) + s->io_buffer_index;
 
-    pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io);
+    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);
+    s->io_buffer_size -= io->len;
+    s->io_buffer_index += io->len;
+    io->len = 0;
+
+    s->bus->dma->aiocb = dma_blk_read(s->blk, &s->sg, offset,
+                                      pmac_ide_atapi_transfer_cb, io);
     return;
 
 done:
@@ -305,6 +134,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
     if (ret < 0) {
         MACIO_DPRINTF("DMA error: %d\n", ret);
+        qemu_sglist_destroy(&s->sg);
         ide_dma_error(s);
         goto done;
     }
@@ -319,6 +149,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
     if (s->io_buffer_size <= 0) {
         MACIO_DPRINTF("End of IDE transfer\n");
+        qemu_sglist_destroy(&s->sg);
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
         m->dma_active = false;
@@ -333,15 +164,27 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     /* Calculate number of sectors */
     offset = (ide_get_sector(s) << 9) + s->io_buffer_index;
 
+    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);
+    s->io_buffer_size -= io->len;
+    s->io_buffer_index += io->len;
+    io->len = 0;
+
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
-        pmac_dma_read(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
+        s->bus->dma->aiocb = dma_blk_read(s->blk, &s->sg, offset,
+                                          pmac_ide_atapi_transfer_cb, io);
         break;
     case IDE_DMA_WRITE:
-        pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
+        s->bus->dma->aiocb = dma_blk_write(s->blk, &s->sg, offset,
+                                           pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_TRIM:
-        pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
+        s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk), &s->sg,
+                                        offset, ide_issue_trim, s->blk,
+                                        pmac_ide_transfer_cb, io,
+                                        DMA_DIRECTION_TO_DEVICE);
         break;
     default:
         abort();
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/2] macio: switch over to new byte-aligned DMA helpers

[John Snow]

On 05/27/2016 04:48 AM, Mark Cave-Ayland wrote:
> Now that the DMA helpers are byte-aligned they can be called directly from
> the macio routines rather than emulating byte-aligned accesses via multiple
> block-level accesses.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/ide/macio.c |  213 ++++++++------------------------------------------------

^ _cool_ ^

I assume you'll actually not be needing me to deal with this until you
resolve the HACK in the pre-requisite patch, yes?

>  1 file changed, 28 insertions(+), 185 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 42ad68a..e4e567e 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -52,187 +52,6 @@ static const int debug_macio = 0;
>  
>  #define MACIO_PAGE_SIZE 4096
>  

Re: [Qemu-devel] [PATCH 2/2] macio: switch over to new byte-aligned DMA helpers

[Mark Cave-Ayland]

On 27/05/16 16:02, John Snow wrote:

> On 05/27/2016 04:48 AM, Mark Cave-Ayland wrote:
>> Now that the DMA helpers are byte-aligned they can be called directly from
>> the macio routines rather than emulating byte-aligned accesses via multiple
>> block-level accesses.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/ide/macio.c |  213 ++++++++------------------------------------------------
> 
> ^ _cool_ ^
> 
> I assume you'll actually not be needing me to deal with this until you
> resolve the HACK in the pre-requisite patch, yes?

Yes, that's correct. The reason I posted it as-is was to provide Paolo
with a test case and for Aurelien to verify the TRIM behaviour. Once the
hack is no longer required, I'll resubmit it properly.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/2] macio: switch over to new byte-aligned DMA helpers

[Aurelien Jarno]

On 2016-05-27 09:48, Mark Cave-Ayland wrote:
> Here is a tidied up version of my patch to convert the macio controller over to
> using the new byte-aligned DMA helpers.
> 
> The first patch is just a hack and temporarily disables unaligned iovec
> truncation in the DMA helper (as discussed in the recent thread) until Paolo or
> someone else can devise a proper solution. Without this, the subsequent switch
> over to the DMA helpers will appear to work during a Darwin PPC install but the
> resulting image is corrupt and will fail to boot.
> 
> The second patch is the real one and switches the macio controller over to use
> the new byte-aligned DMA helpers. Here I see a speed-up of around 2.5x-3x for
> a typical Darwin PPC installation compared to the previous code.
> 
> Aurelien, I'd be grateful if you could test the TRIM path as I know this is
> something you've had issues with before and I couldn't quite figure out how to
> reproduce your TRIM tests from before.

I have just tested the TRIM path, all works fine with your 2 patches
applied.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 0/2] macio: switch over to new byte-aligned DMA helpers

[Mark Cave-Ayland]

On 30/05/16 22:19, Aurelien Jarno wrote:

> On 2016-05-27 09:48, Mark Cave-Ayland wrote:
>> Here is a tidied up version of my patch to convert the macio controller over to
>> using the new byte-aligned DMA helpers.
>>
>> The first patch is just a hack and temporarily disables unaligned iovec
>> truncation in the DMA helper (as discussed in the recent thread) until Paolo or
>> someone else can devise a proper solution. Without this, the subsequent switch
>> over to the DMA helpers will appear to work during a Darwin PPC install but the
>> resulting image is corrupt and will fail to boot.
>>
>> The second patch is the real one and switches the macio controller over to use
>> the new byte-aligned DMA helpers. Here I see a speed-up of around 2.5x-3x for
>> a typical Darwin PPC installation compared to the previous code.
>>
>> Aurelien, I'd be grateful if you could test the TRIM path as I know this is
>> something you've had issues with before and I couldn't quite figure out how to
>> reproduce your TRIM tests from before.
> 
> I have just tested the TRIM path, all works fine with your 2 patches
> applied.

Thanks a lot for testing this, Aurelien. In that case I'll resubmit
patch 2 officially once Paolo has figured out the correct solution for
the iovec truncation.


ATB,

Mark.