[Qemu-devel] [PATCH 0/2] dma-helpers: explicitly pass alignment into dma-helpers

[Qemu-devel] [PATCH 0/2] dma-helpers: explicitly pass alignment into dma-helpers

[Mark Cave-Ayland]

This is a follow-up to the thread at
https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01396.html which
introduces an explicit alignment to the DMA helpers to facilitate conversion
of the macio controller over to use the now byte-aligned DMA helpers.

Patch 1 introduces an alignment parameter as suggested by Paolo above, whilst
patch 2 performs the conversion for the macio controller.

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

Mark Cave-Ayland (2):
  dma-helpers: explicitly pass alignment into dma-helpers
  macio: switch over to new byte-aligned DMA helpers

 dma-helpers.c        |   20 ++---
 hw/block/nvme.c      |    6 +-
 hw/ide/ahci.c        |    2 +
 hw/ide/core.c        |    6 +-
 hw/ide/macio.c       |  213 +++++++-------------------------------------------
 hw/scsi/scsi-disk.c  |    2 +
 include/sysemu/dma.h |    6 +-
 7 files changed, 53 insertions(+), 202 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[Mark Cave-Ayland]

The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
necessarily the case for all platforms. Use this as the default alignment for
all current callers.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 dma-helpers.c        |   20 +++++++++++---------
 hw/block/nvme.c      |    6 ++++--
 hw/ide/ahci.c        |    2 ++
 hw/ide/core.c        |    6 +++---
 hw/scsi/scsi-disk.c  |    2 ++
 include/sysemu/dma.h |    6 +++---
 6 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 9defc10..3caa2ab 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -73,6 +73,7 @@ typedef struct {
     AioContext *ctx;
     BlockAIOCB *acb;
     QEMUSGList *sg;
+    uint32_t align;
     uint64_t offset;
     DMADirection dir;
     int sg_cur_index;
@@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
         return;
     }
 
-    if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
-        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
+    if (dbs->iov.size & (dbs->align - 1)) {
+        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1));
     }
 
     dbs->acb = dbs->io_func(dbs->offset, &dbs->iov,
@@ -199,7 +200,7 @@ static const AIOCBInfo dma_aiocb_info = {
 };
 
 BlockAIOCB *dma_blk_io(AioContext *ctx,
-    QEMUSGList *sg, uint64_t offset,
+    QEMUSGList *sg, uint64_t offset, uint32_t align,
     DMAIOFunc *io_func, void *io_func_opaque,
     BlockCompletionFunc *cb,
     void *opaque, DMADirection dir)
@@ -212,6 +213,7 @@ BlockAIOCB *dma_blk_io(AioContext *ctx,
     dbs->sg = sg;
     dbs->ctx = ctx;
     dbs->offset = offset;
+    dbs->align = align;
     dbs->sg_cur_index = 0;
     dbs->sg_cur_byte = 0;
     dbs->dir = dir;
@@ -234,11 +236,11 @@ BlockAIOCB *dma_blk_read_io_func(int64_t offset, QEMUIOVector *iov,
 }
 
 BlockAIOCB *dma_blk_read(BlockBackend *blk,
-                         QEMUSGList *sg, uint64_t offset,
+                         QEMUSGList *sg, uint64_t offset, uint32_t align,
                          void (*cb)(void *opaque, int ret), void *opaque)
 {
-    return dma_blk_io(blk_get_aio_context(blk),
-                      sg, offset, dma_blk_read_io_func, blk, cb, opaque,
+    return dma_blk_io(blk_get_aio_context(blk), sg, offset, align,
+                      dma_blk_read_io_func, blk, cb, opaque,
                       DMA_DIRECTION_FROM_DEVICE);
 }
 
@@ -252,11 +254,11 @@ BlockAIOCB *dma_blk_write_io_func(int64_t offset, QEMUIOVector *iov,
 }
 
 BlockAIOCB *dma_blk_write(BlockBackend *blk,
-                          QEMUSGList *sg, uint64_t offset,
+                          QEMUSGList *sg, uint64_t offset, uint32_t align,
                           void (*cb)(void *opaque, int ret), void *opaque)
 {
-    return dma_blk_io(blk_get_aio_context(blk),
-                      sg, offset, dma_blk_write_io_func, blk, cb, opaque,
+    return dma_blk_io(blk_get_aio_context(blk), sg, offset, align,
+                      dma_blk_write_io_func, blk, cb, opaque,
                       DMA_DIRECTION_TO_DEVICE);
 }
 
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cef3bb4..b380142 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -258,8 +258,10 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
     req->has_sg = true;
     dma_acct_start(n->conf.blk, &req->acct, &req->qsg, acct);
     req->aiocb = is_write ?
-        dma_blk_write(n->conf.blk, &req->qsg, data_offset, nvme_rw_cb, req) :
-        dma_blk_read(n->conf.blk, &req->qsg, data_offset, nvme_rw_cb, req);
+        dma_blk_write(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE,
+                      nvme_rw_cb, req) :
+        dma_blk_read(n->conf.blk, &req->qsg, data_offset, BDRV_SECTOR_SIZE,
+                     nvme_rw_cb, req);
 
     return NVME_NO_COMPLETE;
 }
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 63ead21..3c19bda 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1009,6 +1009,7 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
                        &ncq_tfs->sglist, BLOCK_ACCT_READ);
         ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist,
                                       ncq_tfs->lba << BDRV_SECTOR_BITS,
+                                      BDRV_SECTOR_SIZE,
                                       ncq_cb, ncq_tfs);
         break;
     case WRITE_FPDMA_QUEUED:
@@ -1022,6 +1023,7 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs)
                        &ncq_tfs->sglist, BLOCK_ACCT_WRITE);
         ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist,
                                        ncq_tfs->lba << BDRV_SECTOR_BITS,
+                                       BDRV_SECTOR_SIZE,
                                        ncq_cb, ncq_tfs);
         break;
     default:
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7291677..43709e5 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -882,15 +882,15 @@ static void ide_dma_cb(void *opaque, int ret)
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
         s->bus->dma->aiocb = dma_blk_read(s->blk, &s->sg, offset,
-                                          ide_dma_cb, s);
+                                          BDRV_SECTOR_SIZE, ide_dma_cb, s);
         break;
     case IDE_DMA_WRITE:
         s->bus->dma->aiocb = dma_blk_write(s->blk, &s->sg, offset,
-                                           ide_dma_cb, s);
+                                           BDRV_SECTOR_SIZE, ide_dma_cb, s);
         break;
     case IDE_DMA_TRIM:
         s->bus->dma->aiocb = dma_blk_io(blk_get_aio_context(s->blk),
-                                        &s->sg, offset,
+                                        &s->sg, offset, BDRV_SECTOR_SIZE,
                                         ide_issue_trim, s->blk, ide_dma_cb, s,
                                         DMA_DIRECTION_TO_DEVICE);
         break;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 88beaf4..a963191 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -341,6 +341,7 @@ static void scsi_do_read(SCSIDiskReq *r, int ret)
         r->req.resid -= r->req.sg->size;
         r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk),
                                   r->req.sg, r->sector << BDRV_SECTOR_BITS,
+                                  BDRV_SECTOR_SIZE,
                                   sdc->dma_readv, r, scsi_dma_complete, r,
                                   DMA_DIRECTION_FROM_DEVICE);
     } else {
@@ -539,6 +540,7 @@ static void scsi_write_data(SCSIRequest *req)
         r->req.resid -= r->req.sg->size;
         r->req.aiocb = dma_blk_io(blk_get_aio_context(s->qdev.conf.blk),
                                   r->req.sg, r->sector << BDRV_SECTOR_BITS,
+                                  BDRV_SECTOR_SIZE,
                                   sdc->dma_writev, r, scsi_dma_complete, r,
                                   DMA_DIRECTION_TO_DEVICE);
     } else {
diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
index 34c8eaf..c228c66 100644
--- a/include/sysemu/dma.h
+++ b/include/sysemu/dma.h
@@ -199,14 +199,14 @@ typedef BlockAIOCB *DMAIOFunc(int64_t offset, QEMUIOVector *iov,
                               void *opaque);
 
 BlockAIOCB *dma_blk_io(AioContext *ctx,
-                       QEMUSGList *sg, uint64_t offset,
+                       QEMUSGList *sg, uint64_t offset, uint32_t align,
                        DMAIOFunc *io_func, void *io_func_opaque,
                        BlockCompletionFunc *cb, void *opaque, DMADirection dir);
 BlockAIOCB *dma_blk_read(BlockBackend *blk,
-                         QEMUSGList *sg, uint64_t offset,
+                         QEMUSGList *sg, uint64_t offset, uint32_t align,
                          BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *dma_blk_write(BlockBackend *blk,
-                          QEMUSGList *sg, uint64_t offset,
+                          QEMUSGList *sg, uint64_t offset, uint32_t align,
                           BlockCompletionFunc *cb, void *opaque);
 uint64_t dma_buf_read(uint8_t *ptr, int32_t len, QEMUSGList *sg);
 uint64_t dma_buf_write(uint8_t *ptr, int32_t len, QEMUSGList *sg);
-- 
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 76f97c2..9742c00 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;
-    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;
-    io->dir = DMA_DIRECTION_FROM_DEVICE;
-    io->dma_len = io->len;
-    io->dma_mem = dma_memory_map(&address_space_memory, dma_addr, &io->dma_len,
-                                 io->dir);
-
-    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, io->dma_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;
-    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;
-    io->dir = DMA_DIRECTION_TO_DEVICE;
-    io->dma_len = io->len;
-    io->dma_mem = dma_memory_map(&address_space_memory, dma_addr, &io->dma_len,
-                                 io->dir);
-
-    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, io->dma_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, io->dma_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, io->dma_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;
-
-    qemu_iovec_destroy(&io->iov);
-    qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
-
-    dma_addr = io->addr;
-    io->dir = DMA_DIRECTION_TO_DEVICE;
-    io->dma_len = io->len;
-    io->dma_mem = dma_memory_map(&address_space_memory, dma_addr, &io->dma_len,
-                                 io->dir);
-
-    qemu_iovec_add(&io->iov, io->dma_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;
@@ -282,7 +103,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, 0x1,
+                                      pmac_ide_atapi_transfer_cb, io);
     return;
 
 done:
@@ -310,6 +139,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;
     }
@@ -324,6 +154,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;
@@ -338,15 +169,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, 0x1,
+                                          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, 0x1,
+                                           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, 0x1, 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 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[Eric Blake]

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

On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote:
> The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
> necessarily the case for all platforms. Use this as the default alignment for
> all current callers.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

> @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
>          return;
>      }
>  
> -    if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
> -        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
> +    if (dbs->iov.size & (dbs->align - 1)) {
> +        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1));

Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size,
dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)?
Semantically it is the same, but the macros make it obvious what the
bit-twiddling is doing.

Unless you think that needs a tweak,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

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

On 10/09/2016 11:43 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 ++++++++------------------------------------------------
>  1 file changed, 28 insertions(+), 185 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[Mark Cave-Ayland]

On 10/10/16 17:34, Eric Blake wrote:

> On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote:
>> The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
>> necessarily the case for all platforms. Use this as the default alignment for
>> all current callers.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
> 
>> @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
>>          return;
>>      }
>>  
>> -    if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
>> -        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
>> +    if (dbs->iov.size & (dbs->align - 1)) {
>> +        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1));
> 
> Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size,
> dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)?
> Semantically it is the same, but the macros make it obvious what the
> bit-twiddling is doing.
> 
> Unless you think that needs a tweak,
> Reviewed-by: Eric Blake <eblake@redhat.com>

I can't say I feel too strongly about it since there are plenty of other
examples of this style in the codebase, so I'm happy to go with whatever
John/Paolo are most happy with.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[John Snow]

On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote:
> On 10/10/16 17:34, Eric Blake wrote:
>
>> On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote:
>>> The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
>>> necessarily the case for all platforms. Use this as the default alignment for
>>> all current callers.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>
>>> @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
>>>          return;
>>>      }
>>>
>>> -    if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
>>> -        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
>>> +    if (dbs->iov.size & (dbs->align - 1)) {
>>> +        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1));
>>
>> Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size,
>> dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)?
>> Semantically it is the same, but the macros make it obvious what the
>> bit-twiddling is doing.
>>
>> Unless you think that needs a tweak,
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> I can't say I feel too strongly about it since there are plenty of other
> examples of this style in the codebase, so I'm happy to go with whatever
> John/Paolo are most happy with.
>
>
> ATB,
>
> Mark.
>

I can't pretend I am consistent, but when in doubt use the macro. Not 
worth a respin IMO, but I think this falls out of my jurisdiction :)

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

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

[John Snow]

On 10/09/2016 12:43 PM, 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.
>

_cool_

> 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 76f97c2..9742c00 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;
> -    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;
> -    io->dir = DMA_DIRECTION_FROM_DEVICE;
> -    io->dma_len = io->len;
> -    io->dma_mem = dma_memory_map(&address_space_memory, dma_addr, &io->dma_len,
> -                                 io->dir);
> -
> -    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, io->dma_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;
> -    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;
> -    io->dir = DMA_DIRECTION_TO_DEVICE;
> -    io->dma_len = io->len;
> -    io->dma_mem = dma_memory_map(&address_space_memory, dma_addr, &io->dma_len,
> -                                 io->dir);
> -
> -    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, io->dma_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, io->dma_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, io->dma_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;
> -
> -    qemu_iovec_destroy(&io->iov);
> -    qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
> -
> -    dma_addr = io->addr;
> -    io->dir = DMA_DIRECTION_TO_DEVICE;
> -    io->dma_len = io->len;
> -    io->dma_mem = dma_memory_map(&address_space_memory, dma_addr, &io->dma_len,
> -                                 io->dir);
> -
> -    qemu_iovec_add(&io->iov, io->dma_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;
> @@ -282,7 +103,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, 0x1,
> +                                      pmac_ide_atapi_transfer_cb, io);
>      return;
>
>  done:
> @@ -310,6 +139,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;
>      }
> @@ -324,6 +154,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;
> @@ -338,15 +169,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, 0x1,
> +                                          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, 0x1,
> +                                           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, 0x1, ide_issue_trim, s->blk,
> +                                        pmac_ide_transfer_cb, io,
> +                                        DMA_DIRECTION_TO_DEVICE);
>          break;
>      default:
>          abort();
>

Looks good, and I assume you've tested it well. I might be pushing it, 
but any plans in your future for MacIO qtests?

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

Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[Kevin Wolf]

Am 11.10.2016 um 17:47 hat John Snow geschrieben:
> On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote:
> >On 10/10/16 17:34, Eric Blake wrote:
> >
> >>On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote:
> >>>The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
> >>>necessarily the case for all platforms. Use this as the default alignment for
> >>>all current callers.
> >>>
> >>>Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>>---
> >>
> >>>@@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
> >>>         return;
> >>>     }
> >>>
> >>>-    if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
> >>>-        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
> >>>+    if (dbs->iov.size & (dbs->align - 1)) {
> >>>+        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1));
> >>
> >>Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size,
> >>dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)?
> >>Semantically it is the same, but the macros make it obvious what the
> >>bit-twiddling is doing.
> >>
> >>Unless you think that needs a tweak,
> >>Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> >I can't say I feel too strongly about it since there are plenty of other
> >examples of this style in the codebase, so I'm happy to go with whatever
> >John/Paolo are most happy with.
> >
> >
> >ATB,
> >
> >Mark.
> >
> 
> I can't pretend I am consistent, but when in doubt use the macro.
> Not worth a respin IMO, but I think this falls out of my
> jurisdiction :)
> 
> Acked-by: John Snow <jsnow@redhat.com>

dma-helpers.c is officially unmaintained, and as the other patch is
clearly IDE, I think the series should go through your tree.

Kevin

Re: [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[John Snow]

On 10/12/2016 06:22 AM, Kevin Wolf wrote:
> Am 11.10.2016 um 17:47 hat John Snow geschrieben:
>> On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote:
>>> On 10/10/16 17:34, Eric Blake wrote:
>>>
>>>> On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote:
>>>>> The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
>>>>> necessarily the case for all platforms. Use this as the default alignment for
>>>>> all current callers.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>
>>>>> @@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
>>>>>         return;
>>>>>     }
>>>>>
>>>>> -    if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
>>>>> -        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
>>>>> +    if (dbs->iov.size & (dbs->align - 1)) {
>>>>> +        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & (dbs->align - 1));
>>>>
>>>> Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size,
>>>> dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)?
>>>> Semantically it is the same, but the macros make it obvious what the
>>>> bit-twiddling is doing.
>>>>
>>>> Unless you think that needs a tweak,
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>> I can't say I feel too strongly about it since there are plenty of other
>>> examples of this style in the codebase, so I'm happy to go with whatever
>>> John/Paolo are most happy with.
>>>
>>>
>>> ATB,
>>>
>>> Mark.
>>>
>>
>> I can't pretend I am consistent, but when in doubt use the macro.
>> Not worth a respin IMO, but I think this falls out of my
>> jurisdiction :)
>>
>> Acked-by: John Snow <jsnow@redhat.com>
>
> dma-helpers.c is officially unmaintained, and as the other patch is
> clearly IDE, I think the series should go through your tree.
>
> Kevin
>

Oh! I was under the impression that Paolo had the dma-helpers. My 
mistake. I will test and stage this.

--js