[Qemu-devel] [PATCH 0/4] Block DMA helpers

[Qemu-devel] [PATCH 0/4] Block DMA helpers

[Avi Kivity]

The current dma mapping API is fairly complex to use: mapping can fail
when bounce resources are exhausted, requiring the caller to reschedule,
and the interface is fairly low level.

This patchset introduces block device oriented helpers to centralize this
logic.  Devices need only submit a scatter/gather list, and the helpers
will do all the dirty work of mapping, rescheduling, and unmapping.  IDE
has been converted to the new API.

Avi Kivity (4):
  Add a scatter-gather list type and accessors
  Add qemu_iovec_reset()
  Introduce block dma helpers
  Convert IDE to use new dma helpers

 Makefile.target |    2 +-
 cutils.c        |    6 ++
 dma-helpers.c   |  146 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dma.h           |   32 ++++++++++++
 hw/ide.c        |   76 ++++-------------------------
 qemu-common.h   |    1 +
 6 files changed, 196 insertions(+), 67 deletions(-)
 create mode 100644 dma-helpers.c
 create mode 100644 dma.h

[Qemu-devel] [PATCH 1/4] Add a scatter-gather list type and accessors

[Avi Kivity]

Scatter-gather lists are used extensively in dma-capable devices; a
single data structure allows more code reuse later on.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 Makefile.target |    2 +-
 dma-helpers.c   |   29 +++++++++++++++++++++++++++++
 dma.h           |   24 ++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletions(-)
 create mode 100644 dma-helpers.c
 create mode 100644 dma.h

diff --git a/Makefile.target b/Makefile.target
index 372d185..28ba17f 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -500,7 +500,7 @@ endif #CONFIG_BSD_USER
 # System emulator target
 ifndef CONFIG_USER_ONLY
 
-OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o
+OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o dma-helpers.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
 OBJS+=virtio.o virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o
diff --git a/dma-helpers.c b/dma-helpers.c
new file mode 100644
index 0000000..315834e
--- /dev/null
+++ b/dma-helpers.c
@@ -0,0 +1,29 @@
+#include "dma.h"
+
+
+void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint)
+{
+    qsg->sg = qemu_malloc(alloc_hint * sizeof(ScatterGatherEntry));
+    qsg->nsg = 0;
+    qsg->nalloc = alloc_hint;
+    qsg->size = 0;
+}
+
+void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
+                     target_phys_addr_t len)
+{
+    if (qsg->nsg == qsg->nalloc) {
+        qsg->nalloc = 2 * qsg->nalloc + 1;
+        qsg->sg = qemu_realloc(qsg->sg, qsg->nalloc * sizeof(ScatterGatherEntry));
+    }
+    qsg->sg[qsg->nsg].base = base;
+    qsg->sg[qsg->nsg].len = len;
+    qsg->size += len;
+    ++qsg->nsg;
+}
+
+void qemu_sglist_destroy(QEMUSGList *qsg)
+{
+    qemu_free(qsg->sg);
+}
+
diff --git a/dma.h b/dma.h
new file mode 100644
index 0000000..3b56fa6
--- /dev/null
+++ b/dma.h
@@ -0,0 +1,24 @@
+#ifndef DMA_H
+#define DMA_H
+
+#include <stdio.h>
+#include "cpu.h"
+
+typedef struct {
+    target_phys_addr_t base;
+    target_phys_addr_t len;
+} ScatterGatherEntry;
+
+typedef struct {
+    ScatterGatherEntry *sg;
+    int nsg;
+    int nalloc;
+    target_phys_addr_t size;
+} QEMUSGList;
+
+void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint);
+void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
+                     target_phys_addr_t len);
+void qemu_sglist_destroy(QEMUSGList *qsg);
+
+#endif
-- 
1.6.1.1

[Qemu-devel] [PATCH 2/4] Add qemu_iovec_reset()

[Avi Kivity]

Add a helper to zero out an existing iovec.  Removes the need to deallocate
and reallocate it.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 cutils.c      |    6 ++++++
 qemu-common.h |    1 +
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/cutils.c b/cutils.c
index 1090aa4..658746b 100644
--- a/cutils.c
+++ b/cutils.c
@@ -129,6 +129,12 @@ void qemu_iovec_destroy(QEMUIOVector *qiov)
     qemu_free(qiov->iov);
 }
 
+void qemu_iovec_reset(QEMUIOVector *qiov)
+{
+    qiov->niov = 0;
+    qiov->size = 0;
+}
+
 void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf)
 {
     uint8_t *p = (uint8_t *)buf;
diff --git a/qemu-common.h b/qemu-common.h
index 8aef558..db33493 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -201,6 +201,7 @@ typedef struct QEMUIOVector {
 void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint);
 void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len);
 void qemu_iovec_destroy(QEMUIOVector *qiov);
+void qemu_iovec_reset(QEMUIOVector *qiov);
 void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf);
 void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count);
 
-- 
1.6.1.1

[Qemu-devel] [PATCH 3/4] Introduce block dma helpers

[Avi Kivity]

These helpers perform read/write requests on entire scatter/gather lists,
relieving the device emulation code from mapping and unmapping physical
memory, and from looping when map resources are exhausted.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 dma-helpers.c |  119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 dma.h         |    8 ++++
 2 files changed, 126 insertions(+), 1 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 315834e..9679f5d 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -1,5 +1,5 @@
 #include "dma.h"
-
+#include "block_int.h"
 
 void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint)
 {
@@ -27,3 +27,120 @@ void qemu_sglist_destroy(QEMUSGList *qsg)
     qemu_free(qsg->sg);
 }
 
+typedef struct {
+    BlockDriverState *bs;
+    BlockDriverAIOCB *acb;
+    QEMUSGList *sg;
+    uint64_t sector_num;
+    int is_write;
+    int sg_cur_index;
+    target_phys_addr_t sg_cur_byte;
+    QEMUIOVector iov;
+    QEMUBH *bh;
+} DMABlockState;
+
+static void dma_bdrv_cb(void *opaque, int ret);
+
+static void reschedule_dma(void *opaque)
+{
+    DMABlockState *dbs = (DMABlockState *)opaque;
+
+    qemu_bh_delete(dbs->bh);
+    dbs->bh = NULL;
+    dma_bdrv_cb(opaque, 0);
+}
+
+static void continue_after_map_failure(void *opaque)
+{
+    DMABlockState *dbs = (DMABlockState *)opaque;
+
+    dbs->bh = qemu_bh_new(reschedule_dma, dbs);
+    qemu_bh_schedule(dbs->bh);
+}
+
+static void dma_bdrv_cb(void *opaque, int ret)
+{
+    DMABlockState *dbs = (DMABlockState *)opaque;
+    target_phys_addr_t cur_addr, cur_len;
+    void *mem;
+    int i;
+
+    dbs->sector_num += dbs->iov.size / 512;
+    for (i = 0; i < dbs->iov.niov; ++i) {
+        cpu_physical_memory_unmap(dbs->iov.iov[i].iov_base,
+                                  dbs->iov.iov[i].iov_len, !dbs->is_write,
+                                  dbs->iov.iov[i].iov_len);
+    }
+    qemu_iovec_reset(&dbs->iov);
+
+    if (dbs->sg_cur_index == dbs->sg->nsg || ret < 0) {
+        dbs->acb->cb(dbs->acb->opaque, ret);
+        qemu_iovec_destroy(&dbs->iov);
+        qemu_aio_release(dbs->acb);
+        qemu_free(dbs);
+        return;
+    }
+
+    while (dbs->sg_cur_index < dbs->sg->nsg) {
+        cur_addr = dbs->sg->sg[dbs->sg_cur_index].base + dbs->sg_cur_byte;
+        cur_len = dbs->sg->sg[dbs->sg_cur_index].len - dbs->sg_cur_byte;
+        mem = cpu_physical_memory_map(cur_addr, &cur_len, !dbs->is_write);
+        if (!mem)
+            break;
+        qemu_iovec_add(&dbs->iov, mem, cur_len);
+        dbs->sg_cur_byte += cur_len;
+        if (dbs->sg_cur_byte == dbs->sg->sg[dbs->sg_cur_index].len) {
+            dbs->sg_cur_byte = 0;
+            ++dbs->sg_cur_index;
+        }
+    }
+
+    if (dbs->iov.size == 0) {
+        cpu_register_map_client(dbs, continue_after_map_failure);
+        return;
+    }
+
+    if (dbs->is_write) {
+        bdrv_aio_writev(dbs->bs, dbs->sector_num, &dbs->iov,
+                        dbs->iov.size / 512, dma_bdrv_cb, dbs);
+    } else {
+        bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
+                       dbs->iov.size / 512, dma_bdrv_cb, dbs);
+    }
+}
+
+static BlockDriverAIOCB *dma_bdrv_io(
+    BlockDriverState *bs, QEMUSGList *sg, uint64_t sector_num,
+    BlockDriverCompletionFunc *cb, void *opaque,
+    int is_write)
+{
+    DMABlockState *dbs = qemu_malloc(sizeof(*dbs));
+
+    dbs->bs = bs;
+    dbs->acb = qemu_aio_get(bs, cb, opaque);
+    dbs->sg = sg;
+    dbs->sector_num = sector_num;
+    dbs->sg_cur_index = 0;
+    dbs->sg_cur_byte = 0;
+    dbs->is_write = is_write;
+    dbs->bh = NULL;
+    qemu_iovec_init(&dbs->iov, sg->nsg);
+    dma_bdrv_cb(dbs, 0);
+    return dbs->acb;
+}
+
+
+BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
+                                QEMUSGList *sg, uint64_t sector,
+                                void (*cb)(void *opaque, int ret), void *opaque)
+{
+    return dma_bdrv_io(bs, sg, sector, cb, opaque, 0);
+}
+
+BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
+                                 QEMUSGList *sg, uint64_t sector,
+                                 void (*cb)(void *opaque, int ret), void *opaque)
+{
+    return dma_bdrv_io(bs, sg, sector, cb, opaque, 1);
+}
+
diff --git a/dma.h b/dma.h
index 3b56fa6..5ce54fb 100644
--- a/dma.h
+++ b/dma.h
@@ -3,6 +3,7 @@
 
 #include <stdio.h>
 #include "cpu.h"
+#include "block.h"
 
 typedef struct {
     target_phys_addr_t base;
@@ -21,4 +22,11 @@ void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
                      target_phys_addr_t len);
 void qemu_sglist_destroy(QEMUSGList *qsg);
 
+BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
+                                QEMUSGList *sg, uint64_t sector,
+                                BlockDriverCompletionFunc *cb, void *opaque);
+BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
+                                 QEMUSGList *sg, uint64_t sector,
+                                 BlockDriverCompletionFunc *cb, void *opaque);
+
 #endif
-- 
1.6.1.1

[Qemu-devel] [PATCH 4/4] Convert IDE to use new dma helpers

[Avi Kivity]

Use the new dma block helpers to perform dma disk I/O.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/ide.c |   76 ++++++++-----------------------------------------------------
 1 files changed, 10 insertions(+), 66 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index bcaee46..1db0bcd 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -33,6 +33,7 @@
 #include "ppc_mac.h"
 #include "mac_dbdma.h"
 #include "sh.h"
+#include "dma.h"
 
 /* debug IDE devices */
 //#define DEBUG_IDE
@@ -423,7 +424,7 @@ typedef struct IDEState {
     int atapi_dma; /* true if dma is requested for the packet cmd */
     /* ATA DMA state */
     int io_buffer_size;
-    QEMUIOVector iovec;
+    QEMUSGList sg;
     /* PIO transfer handling */
     int req_nb_sectors; /* number of sectors per interrupt */
     EndTransferFunc *end_transfer_func;
@@ -876,10 +877,8 @@ static int dma_buf_prepare(BMDMAState *bm, int is_write)
         uint32_t size;
     } prd;
     int l, len;
-    void *mem;
-    target_phys_addr_t l1;
 
-    qemu_iovec_init(&s->iovec, s->nsector / (TARGET_PAGE_SIZE/512) + 1);
+    qemu_sglist_init(&s->sg, s->nsector / (TARGET_PAGE_SIZE/512) + 1);
     s->io_buffer_size = 0;
     for(;;) {
         if (bm->cur_prd_len == 0) {
@@ -900,15 +899,10 @@ static int dma_buf_prepare(BMDMAState *bm, int is_write)
         }
         l = bm->cur_prd_len;
         if (l > 0) {
-            l1 = l;
-            mem = cpu_physical_memory_map(bm->cur_prd_addr, &l1, is_write);
-            if (!mem) {
-                break;
-            }
-            qemu_iovec_add(&s->iovec, mem, l1);
-            bm->cur_prd_addr += l1;
-            bm->cur_prd_len -= l1;
-            s->io_buffer_size += l1;
+            qemu_sglist_add(&s->sg, bm->cur_prd_addr, l);
+            bm->cur_prd_addr += l;
+            bm->cur_prd_len -= l;
+            s->io_buffer_size += l;
         }
     }
     return 1;
@@ -916,14 +910,7 @@ static int dma_buf_prepare(BMDMAState *bm, int is_write)
 
 static void dma_buf_commit(IDEState *s, int is_write)
 {
-    int i;
-
-    for (i = 0; i < s->iovec.niov; ++i) {
-        cpu_physical_memory_unmap(s->iovec.iov[i].iov_base,
-                                  s->iovec.iov[i].iov_len, is_write,
-                                  s->iovec.iov[i].iov_len);
-    }
-    qemu_iovec_destroy(&s->iovec);
+    qemu_sglist_destroy(&s->sg);
 }
 
 static void ide_dma_error(IDEState *s)
@@ -1006,39 +993,6 @@ static int dma_buf_rw(BMDMAState *bm, int is_write)
     return 1;
 }
 
-typedef struct {
-    BMDMAState *bm;
-    void (*cb)(void *opaque, int ret);
-    QEMUBH *bh;
-} MapFailureContinuation;
-
-static void reschedule_dma(void *opaque)
-{
-    MapFailureContinuation *cont = opaque;
-
-    cont->cb(cont->bm, 0);
-    qemu_bh_delete(cont->bh);
-    qemu_free(cont);
-}
-
-static void continue_after_map_failure(void *opaque)
-{
-    MapFailureContinuation *cont = opaque;
-
-    cont->bh = qemu_bh_new(reschedule_dma, opaque);
-    qemu_bh_schedule(cont->bh);
-}
-
-static void wait_for_bounce_buffer(BMDMAState *bmdma,
-                                   void (*cb)(void *opaque, int ret))
-{
-    MapFailureContinuation *cont = qemu_malloc(sizeof(*cont));
-
-    cont->bm = bmdma;
-    cont->cb = cb;
-    cpu_register_map_client(cont, continue_after_map_failure);
-}
-
 static void ide_read_dma_cb(void *opaque, int ret)
 {
     BMDMAState *bm = opaque;
@@ -1080,15 +1034,10 @@ static void ide_read_dma_cb(void *opaque, int ret)
     s->io_buffer_size = n * 512;
     if (dma_buf_prepare(bm, 1) == 0)
         goto eot;
-    if (!s->iovec.niov) {
-        wait_for_bounce_buffer(bm, ide_read_dma_cb);
-        return;
-    }
 #ifdef DEBUG_AIO
     printf("aio_read: sector_num=%" PRId64 " n=%d\n", sector_num, n);
 #endif
-    bm->aiocb = bdrv_aio_readv(s->bs, sector_num, &s->iovec, n,
-                               ide_read_dma_cb, bm);
+    bm->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num, ide_read_dma_cb, bm);
     ide_dma_submit_check(s, ide_read_dma_cb, bm);
 }
 
@@ -1209,15 +1158,10 @@ static void ide_write_dma_cb(void *opaque, int ret)
     /* launch next transfer */
     if (dma_buf_prepare(bm, 0) == 0)
         goto eot;
-    if (!s->iovec.niov) {
-        wait_for_bounce_buffer(bm, ide_write_dma_cb);
-        return;
-    }
 #ifdef DEBUG_AIO
     printf("aio_write: sector_num=%" PRId64 " n=%d\n", sector_num, n);
 #endif
-    bm->aiocb = bdrv_aio_writev(s->bs, sector_num, &s->iovec, n,
-                                ide_write_dma_cb, bm);
+    bm->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num, ide_write_dma_cb, bm);
     ide_dma_submit_check(s, ide_write_dma_cb, bm);
 }
 
-- 
1.6.1.1

[Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Anthony Liguori]

Avi Kivity wrote:
> Scatter-gather lists are used extensively in dma-capable devices; a
> single data structure allows more code reuse later on.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  Makefile.target |    2 +-
>  dma-helpers.c   |   29 +++++++++++++++++++++++++++++
>  dma.h           |   24 ++++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 1 deletions(-)
>  create mode 100644 dma-helpers.c
>  create mode 100644 dma.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 372d185..28ba17f 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -500,7 +500,7 @@ endif #CONFIG_BSD_USER
>  # System emulator target
>  ifndef CONFIG_USER_ONLY
>  
> -OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o
> +OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o dma-helpers.o
>  # virtio has to be here due to weird dependency between PCI and virtio-net.
>  # need to fix this properly
>  OBJS+=virtio.o virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o
> diff --git a/dma-helpers.c b/dma-helpers.c
> new file mode 100644
> index 0000000..315834e
> --- /dev/null
> +++ b/dma-helpers.c
> @@ -0,0 +1,29 @@
>   

Needs copyright/license.

> +#include "dma.h"
> +
> +
> +void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint)
> +{
> +    qsg->sg = qemu_malloc(alloc_hint * sizeof(ScatterGatherEntry));
>   

Would be nice to check for malloc failures and fail gracefully at least.

> +    qsg->nsg = 0;
> +    qsg->nalloc = alloc_hint;
> +    qsg->size = 0;
> +}
> +
> +void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
> +                     target_phys_addr_t len)
> +{
> +    if (qsg->nsg == qsg->nalloc) {
> +        qsg->nalloc = 2 * qsg->nalloc + 1;
>   

Do you really want exponential growth verses linear growth?  The sg 
lists should be relatively small so linear growth should be fine.

Regards,

Anthony Liguori

> +        qsg->sg = qemu_realloc(qsg->sg, qsg->nalloc * sizeof(ScatterGatherEntry));
> +    }
> +    qsg->sg[qsg->nsg].base = base;
> +    qsg->sg[qsg->nsg].len = len;
> +    qsg->size += len;
> +    ++qsg->nsg;
> +}
> +
> +void qemu_sglist_destroy(QEMUSGList *qsg)
> +{
> +    qemu_free(qsg->sg);
> +}
> +
> diff --git a/dma.h b/dma.h
> new file mode 100644
> index 0000000..3b56fa6
> --- /dev/null
> +++ b/dma.h
> @@ -0,0 +1,24 @@
> +#ifndef DMA_H
> +#define DMA_H
> +
> +#include <stdio.h>
> +#include "cpu.h"
> +
> +typedef struct {
> +    target_phys_addr_t base;
> +    target_phys_addr_t len;
> +} ScatterGatherEntry;
> +
> +typedef struct {
> +    ScatterGatherEntry *sg;
> +    int nsg;
> +    int nalloc;
> +    target_phys_addr_t size;
> +} QEMUSGList;
> +
> +void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint);
> +void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
> +                     target_phys_addr_t len);
> +void qemu_sglist_destroy(QEMUSGList *qsg);
> +
> +#endif
>   

[Qemu-devel] Re: [PATCH 3/4] Introduce block dma helpers

[Anthony Liguori]

Avi Kivity wrote:
> These helpers perform read/write requests on entire scatter/gather lists,
> relieving the device emulation code from mapping and unmapping physical
> memory, and from looping when map resources are exhausted.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
>   
<snip>
> +BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
> +                                QEMUSGList *sg, uint64_t sector,
> +                                void (*cb)(void *opaque, int ret), void *opaque)
> +{
> +    return dma_bdrv_io(bs, sg, sector, cb, opaque, 0);
> +}
> +
> +BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
> +                                 QEMUSGList *sg, uint64_t sector,
> +                                 void (*cb)(void *opaque, int ret), void *opaque)
> +{
> +    return dma_bdrv_io(bs, sg, sector, cb, opaque, 1);
> +}
>   

This came out really nicely.

Regards,

Anthony Liguori

> diff --git a/dma.h b/dma.h
> index 3b56fa6..5ce54fb 100644
> --- a/dma.h
> +++ b/dma.h
> @@ -3,6 +3,7 @@
>  
>  #include <stdio.h>
>  #include "cpu.h"
> +#include "block.h"
>  
>  typedef struct {
>      target_phys_addr_t base;
> @@ -21,4 +22,11 @@ void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
>                       target_phys_addr_t len);
>  void qemu_sglist_destroy(QEMUSGList *qsg);
>  
> +BlockDriverAIOCB *dma_bdrv_read(BlockDriverState *bs,
> +                                QEMUSGList *sg, uint64_t sector,
> +                                BlockDriverCompletionFunc *cb, void *opaque);
> +BlockDriverAIOCB *dma_bdrv_write(BlockDriverState *bs,
> +                                 QEMUSGList *sg, uint64_t sector,
> +                                 BlockDriverCompletionFunc *cb, void *opaque);
> +
>  #endif
>   

[Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Scatter-gather lists are used extensively in dma-capable devices; a
>> single data structure allows more code reuse later on.
>>
>> diff --git a/dma-helpers.c b/dma-helpers.c
>> new file mode 100644
>> index 0000000..315834e
>> --- /dev/null
>> +++ b/dma-helpers.c
>> @@ -0,0 +1,29 @@
>>   
>
> Needs copyright/license.

Sure.

Is it possible to have a blanket license for files which don't have 
explicit terms?  I don't much like boilerplate.

>
>> +#include "dma.h"
>> +
>> +
>> +void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint)
>> +{
>> +    qsg->sg = qemu_malloc(alloc_hint * sizeof(ScatterGatherEntry));
>>   
>
> Would be nice to check for malloc failures and fail gracefully at least.

Do you mean an exit(1)?  If so we could just put it in qemu_malloc().

Propagation is usually not possible since hardware is not expected to 
run out of memory.  It will also be a never-tested code path.

>>
>> +void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
>> +                     target_phys_addr_t len)
>> +{
>> +    if (qsg->nsg == qsg->nalloc) {
>> +        qsg->nalloc = 2 * qsg->nalloc + 1;
>>   
>
> Do you really want exponential growth verses linear growth?  The sg 
> lists should be relatively small so linear growth should be fine.
>

I expect this to trigger rarely since the allocation hint should suffice 
nearly 100% of the time.  But in case we miss, it's better to reallocate 
as little as possible.

(what I really want is std::vector<>)

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Avi Kivity wrote:
>>> Scatter-gather lists are used extensively in dma-capable devices; a
>>> single data structure allows more code reuse later on.
>>>
>>> diff --git a/dma-helpers.c b/dma-helpers.c
>>> new file mode 100644
>>> index 0000000..315834e
>>> --- /dev/null
>>> +++ b/dma-helpers.c
>>> @@ -0,0 +1,29 @@
>>>   
>>
>> Needs copyright/license.
>
> Sure.
>
> Is it possible to have a blanket license for files which don't have 
> explicit terms?  I don't much like boilerplate.

I'd greatly prefer not to.  You can refer to a COPYING and we can have a 
default COPYING file but a copyright is really needed as far as I 
understand it.

>>
>>> +#include "dma.h"
>>> +
>>> +
>>> +void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint)
>>> +{
>>> +    qsg->sg = qemu_malloc(alloc_hint * sizeof(ScatterGatherEntry));
>>>   
>>
>> Would be nice to check for malloc failures and fail gracefully at least.
>
> Do you mean an exit(1)?  If so we could just put it in qemu_malloc().

In theory, some users may be able to cope with malloc failure.  In 
practice, I don't think anyone can.  I'm open to suggestion.

> I expect this to trigger rarely since the allocation hint should 
> suffice nearly 100% of the time.  But in case we miss, it's better to 
> reallocate as little as possible.
>
> (what I really want is std::vector<>)

Which I'm pretty sure has a linear growth strategy :-)

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Avi Kivity]

Anthony Liguori wrote:
>>
>> Is it possible to have a blanket license for files which don't have 
>> explicit terms?  I don't much like boilerplate.
>
> I'd greatly prefer not to.  You can refer to a COPYING and we can have 
> a default COPYING file but a copyright is really needed as far as I 
> understand it.
>

Okay.  I'll add an explicit license and leave the generic license to our 
esteemed maintainers.

>>> Would be nice to check for malloc failures and fail gracefully at 
>>> least.
>>
>> Do you mean an exit(1)?  If so we could just put it in qemu_malloc().
>
> In theory, some users may be able to cope with malloc failure.  In 
> practice, I don't think anyone can.  I'm open to suggestion.

malloc() will never fail on Linux with overcommit enabled; since Linux 
is fairly useless without overcommit, it means you'll never see a failure.

Other ways of allocating memory (stack growth, first access to anonymous 
memory) are not covered.  They can fail (most ungracefully) without 
strict overcommit control.

So I suggest to have qemu_malloc() and its friends abort on failure.

>> I expect this to trigger rarely since the allocation hint should 
>> suffice nearly 100% of the time.  But in case we miss, it's better to 
>> reallocate as little as possible.
>>
>> (what I really want is std::vector<>)
>
> Which I'm pretty sure has a linear growth strategy :-)

Not in any of the implementations I'm familiar with.  I believe 
std::vector<> is required to have amortized O(1) append operations.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>>
>>> Is it possible to have a blanket license for files which don't have 
>>> explicit terms?  I don't much like boilerplate.
>>
>> I'd greatly prefer not to.  You can refer to a COPYING and we can 
>> have a default COPYING file but a copyright is really needed as far 
>> as I understand it.
>>
>
> Okay.  I'll add an explicit license and leave the generic license to 
> our esteemed maintainers.
>
>>>> Would be nice to check for malloc failures and fail gracefully at 
>>>> least.
>>>
>>> Do you mean an exit(1)?  If so we could just put it in qemu_malloc().
>>
>> In theory, some users may be able to cope with malloc failure.  In 
>> practice, I don't think anyone can.  I'm open to suggestion.
>
> malloc() will never fail on Linux with overcommit enabled; since Linux 
> is fairly useless without overcommit, it means you'll never see a 
> failure.

Sure it will.  You just have to run out of VA space.

> Other ways of allocating memory (stack growth, first access to 
> anonymous memory) are not covered.  They can fail (most ungracefully) 
> without strict overcommit control.
>
> So I suggest to have qemu_malloc() and its friends abort on failure.

As I said, I'm not at all opposed to this.  glib does this and it makes 
life a lot easier.

>>> I expect this to trigger rarely since the allocation hint should 
>>> suffice nearly 100% of the time.  But in case we miss, it's better 
>>> to reallocate as little as possible.
>>>
>>> (what I really want is std::vector<>)
>>
>> Which I'm pretty sure has a linear growth strategy :-)
>
> Not in any of the implementations I'm familiar with.  I believe 
> std::vector<> is required to have amortized O(1) append operations.

Linear growth doesn't imply element-by-element growth.  You can have a 
coefficient > 1.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Avi Kivity]

Anthony Liguori wrote:
>>
>> malloc() will never fail on Linux with overcommit enabled; since 
>> Linux is fairly useless without overcommit, it means you'll never see 
>> a failure.
>
> Sure it will.  You just have to run out of VA space.
>

That's not recoverable, and is certainly a qemu bug if it happens.  
Especially on 47-bit hosts.

>>>> I expect this to trigger rarely since the allocation hint should 
>>>> suffice nearly 100% of the time.  But in case we miss, it's better 
>>>> to reallocate as little as possible.
>>>>
>>>> (what I really want is std::vector<>)
>>>
>>> Which I'm pretty sure has a linear growth strategy :-)
>>
>> Not in any of the implementations I'm familiar with.  I believe 
>> std::vector<> is required to have amortized O(1) append operations.
>
> Linear growth doesn't imply element-by-element growth.  You can have a 
> coefficient > 1.
>

If you have a coefficient, then it's exponential growth, even if it's 
less than two.  If you have an addition, then it's linear growth, even 
if the addend is greater than one.

  size *= k -> exponential
  size += k -> linear


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Paul Brook]

> > +    if (qsg->nsg == qsg->nalloc) {
> > +        qsg->nalloc = 2 * qsg->nalloc + 1;
>
> Do you really want exponential growth verses linear growth?  The sg
> lists should be relatively small so linear growth should be fine.

If they're small it shouldn't make a any real difference...

Paul

Re: [Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Paul Brook]

> >>> Would be nice to check for malloc failures and fail gracefully at
> >>> least.
> >>
> >> Do you mean an exit(1)?  If so we could just put it in qemu_malloc().
> >
> > In theory, some users may be able to cope with malloc failure.  In
> > practice, I don't think anyone can.  I'm open to suggestion.
>
> malloc() will never fail on Linux with overcommit enabled; since Linux
> is fairly useless without overcommit, it means you'll never see a failure.

I disagree about "Linux is fairly useless without overcommit". Certain common 
linux applications maybe, however...

> Other ways of allocating memory (stack growth, first access to anonymous
> memory) are not covered.  They can fail (most ungracefully) without
> strict overcommit control.
>
> So I suggest to have qemu_malloc() and its friends abort on failure.

I'll support this.  In theory it's sometimes possible to recover from Out Of 
Memory conditions. However in practice I don't think there's any real scope 
for this in qemu.  If were allocating huge chunks of ram on the fly then 
something else is already badly wrong.

IMHO pushing the error up (down?) the callchain is unlikely to provide the 
user with significantly better information, we may as well bail out in 
qemu_malloc.

Paul

Re: [Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[M. Warner Losh]

In message: <4989FC3B.7010407@us.ibm.com>
            Anthony Liguori <aliguori@us.ibm.com> writes:
: Avi Kivity wrote:
: > Anthony Liguori wrote:
: >> Avi Kivity wrote:
: >>> Scatter-gather lists are used extensively in dma-capable devices; a
: >>> single data structure allows more code reuse later on.
: >>>
: >>> diff --git a/dma-helpers.c b/dma-helpers.c
: >>> new file mode 100644
: >>> index 0000000..315834e
: >>> --- /dev/null
: >>> +++ b/dma-helpers.c
: >>> @@ -0,0 +1,29 @@
: >>>   
: >>
: >> Needs copyright/license.
: >
: > Sure.
: >
: > Is it possible to have a blanket license for files which don't have 
: > explicit terms?  I don't much like boilerplate.
: 
: I'd greatly prefer not to.  You can refer to a COPYING and we can have a 
: default COPYING file but a copyright is really needed as far as I 
: understand it.

The copyright exists regardless of any notices being on the file.  In
the absence of a license to copy, however, the default legally defined
license is 'you may not copy it at all'.  Based on this, you need to
have an explicit license for all files.  However, it only needs to be
clear what the license for a given file is.  You don't have to have
the license in each and every file.

Warner

Re: [Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Anthony Liguori]

M. Warner Losh wrote:
> In message: <4989FC3B.7010407@us.ibm.com>
>             Anthony Liguori <aliguori@us.ibm.com> writes:
> : Avi Kivity wrote:
> : > Anthony Liguori wrote:
> : >> Avi Kivity wrote:
> : >>> Scatter-gather lists are used extensively in dma-capable devices; a
> : >>> single data structure allows more code reuse later on.
> : >>>
> : >>> diff --git a/dma-helpers.c b/dma-helpers.c
> : >>> new file mode 100644
> : >>> index 0000000..315834e
> : >>> --- /dev/null
> : >>> +++ b/dma-helpers.c
> : >>> @@ -0,0 +1,29 @@
> : >>>   
> : >>
> : >> Needs copyright/license.
> : >
> : > Sure.
> : >
> : > Is it possible to have a blanket license for files which don't have 
> : > explicit terms?  I don't much like boilerplate.
> : 
> : I'd greatly prefer not to.  You can refer to a COPYING and we can have a 
> : default COPYING file but a copyright is really needed as far as I 
> : understand it.
>
> The copyright exists regardless of any notices being on the file.  In
> the absence of a license to copy, however, the default legally defined
> license is 'you may not copy it at all'.  Based on this, you need to
> have an explicit license for all files.  However, it only needs to be
> clear what the license for a given file is.  You don't have to have
> the license in each and every file.
>   

To put it quite simply, I can't take a patch that introduces a new file 
without a copyright/license in the header.  So there's no need to 
discuss it further than that :-)

Regards,

Anthony Liguori

> Warner
>   

Re: [Qemu-devel] Re: [PATCH 1/4] Add a scatter-gather list type and accessors

[Avi Kivity]

Paul Brook wrote:
> I disagree about "Linux is fairly useless without overcommit". Certain common 
> linux applications maybe, however...
>   

qemu is one.  The use of fork() to run scripts means qemu's memory 
commit can double, even though in practice this memory will not be used.

I've tried running with strict overcommit control once, the desktop 
crashed left and right even after adding ridiculous amounts of swap.

>> Other ways of allocating memory (stack growth, first access to anonymous
>> memory) are not covered.  They can fail (most ungracefully) without
>> strict overcommit control.
>>
>> So I suggest to have qemu_malloc() and its friends abort on failure.
>>     
>
> I'll support this.  In theory it's sometimes possible to recover from Out Of 
> Memory conditions. However in practice I don't think there's any real scope 
> for this in qemu.  If were allocating huge chunks of ram on the fly then 
> something else is already badly wrong.
>
> IMHO pushing the error up (down?) the callchain is unlikely to provide the 
> user with significantly better information, we may as well bail out in 
> qemu_malloc.
>   

I'll post a patch.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] [PATCH 1/4] Add a scatter-gather list type and accessors

[Avi Kivity]

Scatter-gather lists are used extensively in dma-capable devices; a
single data structure allows more code reuse later on.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 Makefile.target |    2 +-
 dma-helpers.c   |   38 ++++++++++++++++++++++++++++++++++++++
 dma.h           |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 1 deletions(-)
 create mode 100644 dma-helpers.c
 create mode 100644 dma.h

diff --git a/Makefile.target b/Makefile.target
index 372d185..28ba17f 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -500,7 +500,7 @@ endif #CONFIG_BSD_USER
 # System emulator target
 ifndef CONFIG_USER_ONLY
 
-OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o
+OBJS=vl.o osdep.o monitor.o pci.o loader.o isa_mmio.o machine.o dma-helpers.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
 OBJS+=virtio.o virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o
diff --git a/dma-helpers.c b/dma-helpers.c
new file mode 100644
index 0000000..11ad3a4
--- /dev/null
+++ b/dma-helpers.c
@@ -0,0 +1,38 @@
+/*
+ * DMA helper functions
+ *
+ * Copyright (c) 2009 Red Hat
+ *
+ * This work is licensed under the terms of the GNU General Public License
+ * (GNU GPL), version 2 or later.
+ */
+
+#include "dma.h"
+
+
+void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint)
+{
+    qsg->sg = qemu_malloc(alloc_hint * sizeof(ScatterGatherEntry));
+    qsg->nsg = 0;
+    qsg->nalloc = alloc_hint;
+    qsg->size = 0;
+}
+
+void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
+                     target_phys_addr_t len)
+{
+    if (qsg->nsg == qsg->nalloc) {
+        qsg->nalloc = 2 * qsg->nalloc + 1;
+        qsg->sg = qemu_realloc(qsg->sg, qsg->nalloc * sizeof(ScatterGatherEntry));
+    }
+    qsg->sg[qsg->nsg].base = base;
+    qsg->sg[qsg->nsg].len = len;
+    qsg->size += len;
+    ++qsg->nsg;
+}
+
+void qemu_sglist_destroy(QEMUSGList *qsg)
+{
+    qemu_free(qsg->sg);
+}
+
diff --git a/dma.h b/dma.h
new file mode 100644
index 0000000..16af50f
--- /dev/null
+++ b/dma.h
@@ -0,0 +1,33 @@
+/*
+ * DMA helper functions
+ *
+ * Copyright (c) 2009 Red Hat
+ *
+ * This work is licensed under the terms of the GNU General Public License
+ * (GNU GPL), version 2 or later.
+ */
+
+#ifndef DMA_H
+#define DMA_H
+
+#include <stdio.h>
+#include "cpu.h"
+
+typedef struct {
+    target_phys_addr_t base;
+    target_phys_addr_t len;
+} ScatterGatherEntry;
+
+typedef struct {
+    ScatterGatherEntry *sg;
+    int nsg;
+    int nalloc;
+    target_phys_addr_t size;
+} QEMUSGList;
+
+void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint);
+void qemu_sglist_add(QEMUSGList *qsg, target_phys_addr_t base,
+                     target_phys_addr_t len);
+void qemu_sglist_destroy(QEMUSGList *qsg);
+
+#endif
-- 
1.6.1.1