[Qemu-devel] [PATCH v3 0/9] virtio-blk: Converge VirtIOBlockRequest into VirtIOBlockReq

[Qemu-devel] [PATCH v3 0/9] virtio-blk: Converge VirtIOBlockRequest into VirtIOBlockReq

[Fam Zheng]

This unifies the request structure used by dataplane and non-dataplane code,
while dropping unnessary fields for bounce buffer and read flag.

Applies on top of Stefan's block tree.

v3: Address Paolo's comments:

[06/09] virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr
        Add assertion and comments on inhdr.
        Fix the offset and iov_discard_back.

[08/09] virtio-blk: Fill in VirtIOBlockReq.out in dataplane code
        Allocate req in process_request.

[09/09] virtio-blk: Fix and clean up the in_sg and out_sg check
        New.



Fam Zheng (9):
  virtio-blk: Move VirtIOBlockReq to header
  virtio-blk: Convert VirtIOBlockReq.elem to pointer
  virtio-blk: Drop bounce buffer from dataplane code
  virtio-blk: Drop VirtIOBlockRequest.read
  virtio-blk: Replace VirtIOBlockRequest with VirtIOBlockReq
  virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr
  virtio-blk: Convert VirtIOBlockReq.out to structrue
  virtio-blk: Fill in VirtIOBlockReq.out in dataplane code
  virtio-blk: Fix and clean up the in_sg and out_sg check

 hw/block/dataplane/virtio-blk.c | 167 +++++++++++++---------------------------
 hw/block/virtio-blk.c           | 113 ++++++++++++++-------------
 include/hw/virtio/virtio-blk.h  |  11 +++
 3 files changed, 124 insertions(+), 167 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH v3 1/9] virtio-blk: Move VirtIOBlockReq to header

[Fam Zheng]

For later reusing by dataplane code.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c          | 11 -----------
 include/hw/virtio/virtio-blk.h | 11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 0b1446e..49507ac 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -27,17 +27,6 @@
 #endif
 #include "hw/virtio/virtio-bus.h"
 
-typedef struct VirtIOBlockReq
-{
-    VirtIOBlock *dev;
-    VirtQueueElement elem;
-    struct virtio_blk_inhdr *in;
-    struct virtio_blk_outhdr *out;
-    QEMUIOVector qiov;
-    struct VirtIOBlockReq *next;
-    BlockAcctCookie acct;
-} VirtIOBlockReq;
-
 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 {
     VirtIOBlock *s = req->dev;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 4bc9b54..6fc43f1 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -17,6 +17,7 @@
 #include "hw/virtio/virtio.h"
 #include "hw/block/block.h"
 #include "sysemu/iothread.h"
+#include "include/block/block.h"
 
 #define TYPE_VIRTIO_BLK "virtio-blk-device"
 #define VIRTIO_BLK(obj) \
@@ -133,6 +134,16 @@ typedef struct VirtIOBlock {
 #endif
 } VirtIOBlock;
 
+typedef struct VirtIOBlockReq {
+    VirtIOBlock *dev;
+    VirtQueueElement elem;
+    struct virtio_blk_inhdr *in;
+    struct virtio_blk_outhdr *out;
+    QEMUIOVector qiov;
+    struct VirtIOBlockReq *next;
+    BlockAcctCookie acct;
+} VirtIOBlockReq;
+
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
 
-- 
2.0.0

Re: [Qemu-devel] [PATCH v3 1/9] virtio-blk: Move VirtIOBlockReq to header

[Stefan Hajnoczi]

On Fri, Jun 06, 2014 at 09:53:22AM +0800, Fam Zheng wrote:
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 4bc9b54..6fc43f1 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -17,6 +17,7 @@
>  #include "hw/virtio/virtio.h"
>  #include "hw/block/block.h"
>  #include "sysemu/iothread.h"
> +#include "include/block/block.h"

Why not #include "block/block.h"?  The other includes don't explicitly
use the include/ prefix either.

[Qemu-devel] [PATCH v3 2/9] virtio-blk: Convert VirtIOBlockReq.elem to pointer

[Fam Zheng]

This will make converging with dataplane code easier.

Add virtio_blk_free_request to handle the freeing of request internal
fields.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c          | 85 +++++++++++++++++++++++-------------------
 include/hw/virtio/virtio-blk.h |  2 +-
 2 files changed, 47 insertions(+), 40 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 49507ac..388741e 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -27,6 +27,22 @@
 #endif
 #include "hw/virtio/virtio-bus.h"
 
+static VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
+{
+    VirtIOBlockReq *req = g_malloc0(sizeof(*req));
+    req->dev = s;
+    req->elem = g_slice_new0(VirtQueueElement);
+    return req;
+}
+
+static void virtio_blk_free_request(VirtIOBlockReq *req)
+{
+    if (req) {
+        g_slice_free(VirtQueueElement, req->elem);
+        g_free(req);
+    }
+}
+
 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 {
     VirtIOBlock *s = req->dev;
@@ -35,7 +51,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
     trace_virtio_blk_req_complete(req, status);
 
     stb_p(&req->in->status, status);
-    virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
+    virtqueue_push(s->vq, req->elem, req->qiov.size + sizeof(*req->in));
     virtio_notify(vdev, s->vq);
 }
 
@@ -51,7 +67,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     } else if (action == BDRV_ACTION_REPORT) {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         bdrv_acct_done(s->bs, &req->acct);
-        g_free(req);
+        virtio_blk_free_request(req);
     }
 
     bdrv_error_action(s->bs, action, is_read, error);
@@ -72,7 +88,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
 
     virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
     bdrv_acct_done(req->dev->bs, &req->acct);
-    g_free(req);
+    virtio_blk_free_request(req);
 }
 
 static void virtio_blk_flush_complete(void *opaque, int ret)
@@ -87,27 +103,16 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
 
     virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
     bdrv_acct_done(req->dev->bs, &req->acct);
-    g_free(req);
-}
-
-static VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
-{
-    VirtIOBlockReq *req = g_malloc(sizeof(*req));
-    req->dev = s;
-    req->qiov.size = 0;
-    req->next = NULL;
-    return req;
+    virtio_blk_free_request(req);
 }
 
 static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
 {
     VirtIOBlockReq *req = virtio_blk_alloc_request(s);
 
-    if (req != NULL) {
-        if (!virtqueue_pop(s->vq, &req->elem)) {
-            g_free(req);
-            return NULL;
-        }
+    if (!virtqueue_pop(s->vq, req->elem)) {
+        virtio_blk_free_request(req);
+        return NULL;
     }
 
     return req;
@@ -236,9 +241,9 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
     int status;
 
-    status = virtio_blk_handle_scsi_req(req->dev, &req->elem);
+    status = virtio_blk_handle_scsi_req(req->dev, req->elem);
     virtio_blk_req_complete(req, status);
-    g_free(req);
+    virtio_blk_free_request(req);
 }
 
 typedef struct MultiReqBuffer {
@@ -340,19 +345,19 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
 {
     uint32_t type;
 
-    if (req->elem.out_num < 1 || req->elem.in_num < 1) {
+    if (req->elem->out_num < 1 || req->elem->in_num < 1) {
         error_report("virtio-blk missing headers");
         exit(1);
     }
 
-    if (req->elem.out_sg[0].iov_len < sizeof(*req->out) ||
-        req->elem.in_sg[req->elem.in_num - 1].iov_len < sizeof(*req->in)) {
+    if (req->elem->out_sg[0].iov_len < sizeof(*req->out) ||
+        req->elem->in_sg[req->elem->in_num - 1].iov_len < sizeof(*req->in)) {
         error_report("virtio-blk header not in correct element");
         exit(1);
     }
 
-    req->out = (void *)req->elem.out_sg[0].iov_base;
-    req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
+    req->out = (void *)req->elem->out_sg[0].iov_base;
+    req->in = (void *)req->elem->in_sg[req->elem->in_num - 1].iov_base;
 
     type = ldl_p(&req->out->type);
 
@@ -367,23 +372,23 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
          * NB: per existing s/n string convention the string is
          * terminated by '\0' only when shorter than buffer.
          */
-        strncpy(req->elem.in_sg[0].iov_base,
+        strncpy(req->elem->in_sg[0].iov_base,
                 s->blk.serial ? s->blk.serial : "",
-                MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
+                MIN(req->elem->in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
-        g_free(req);
+        virtio_blk_free_request(req);
     } else if (type & VIRTIO_BLK_T_OUT) {
-        qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
-                                 req->elem.out_num - 1);
+        qemu_iovec_init_external(&req->qiov, &req->elem->out_sg[1],
+                                 req->elem->out_num - 1);
         virtio_blk_handle_write(req, mrb);
     } else if (type == VIRTIO_BLK_T_IN || type == VIRTIO_BLK_T_BARRIER) {
         /* VIRTIO_BLK_T_IN is 0, so we can't just & it. */
-        qemu_iovec_init_external(&req->qiov, &req->elem.in_sg[0],
-                                 req->elem.in_num - 1);
+        qemu_iovec_init_external(&req->qiov, &req->elem->in_sg[0],
+                                 req->elem->in_num - 1);
         virtio_blk_handle_read(req);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
-        g_free(req);
+        virtio_blk_free_request(req);
     }
 }
 
@@ -598,7 +603,8 @@ static void virtio_blk_save(QEMUFile *f, void *opaque)
     
     while (req) {
         qemu_put_sbyte(f, 1);
-        qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
+        qemu_put_buffer(f, (unsigned char *)req->elem,
+                        sizeof(VirtQueueElement));
         req = req->next;
     }
     qemu_put_sbyte(f, 0);
@@ -620,14 +626,15 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 
     while (qemu_get_sbyte(f)) {
         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
-        qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
+        qemu_get_buffer(f, (unsigned char *)req->elem,
+                        sizeof(VirtQueueElement));
         req->next = s->rq;
         s->rq = req;
 
-        virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
-            req->elem.in_num, 1);
-        virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
-            req->elem.out_num, 0);
+        virtqueue_map_sg(req->elem->in_sg, req->elem->in_addr,
+            req->elem->in_num, 1);
+        virtqueue_map_sg(req->elem->out_sg, req->elem->out_addr,
+            req->elem->out_num, 0);
     }
 
     return 0;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 6fc43f1..1932502 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -136,7 +136,7 @@ typedef struct VirtIOBlock {
 
 typedef struct VirtIOBlockReq {
     VirtIOBlock *dev;
-    VirtQueueElement elem;
+    VirtQueueElement *elem;
     struct virtio_blk_inhdr *in;
     struct virtio_blk_outhdr *out;
     QEMUIOVector qiov;
-- 
2.0.0

Re: [Qemu-devel] [PATCH v3 2/9] virtio-blk: Convert VirtIOBlockReq.elem to pointer

[Stefan Hajnoczi]

On Fri, Jun 06, 2014 at 09:53:23AM +0800, Fam Zheng wrote:
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 49507ac..388741e 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -27,6 +27,22 @@
>  #endif
>  #include "hw/virtio/virtio-bus.h"
>  
> +static VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
> +{
> +    VirtIOBlockReq *req = g_malloc0(sizeof(*req));

The virtio-blk.c code used g_malloc() but since we're using
g_slice_new0() for the VirtQueueElement, we might as well use the slice
allocator here too.

[Qemu-devel] [PATCH v3 3/9] virtio-blk: Drop bounce buffer from dataplane code

[Fam Zheng]

The block layer will handle the unaligned request.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index c10b7b7..3d1e9e1 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -29,8 +29,6 @@ typedef struct {
     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
     VirtQueueElement *elem;         /* saved data from the virtqueue */
     QEMUIOVector qiov;              /* original request iovecs */
-    struct iovec bounce_iov;        /* used if guest buffers are unaligned */
-    QEMUIOVector bounce_qiov;       /* bounce buffer iovecs */
     bool read;                      /* read or write? */
 } VirtIOBlockRequest;
 
@@ -85,14 +83,6 @@ static void complete_rdwr(void *opaque, int ret)
 
     trace_virtio_blk_data_plane_complete_request(req->s, req->elem->index, ret);
 
-    if (req->read && req->bounce_iov.iov_base) {
-        qemu_iovec_from_buf(&req->qiov, 0, req->bounce_iov.iov_base, len);
-    }
-
-    if (req->bounce_iov.iov_base) {
-        qemu_vfree(req->bounce_iov.iov_base);
-    }
-
     qemu_iovec_from_buf(req->inhdr, 0, &hdr, sizeof(hdr));
     qemu_iovec_destroy(req->inhdr);
     g_slice_free(QEMUIOVector, req->inhdr);
@@ -152,21 +142,6 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
 
     qiov = &req->qiov;
 
-    if (!bdrv_qiov_is_aligned(s->blk->conf.bs, qiov)) {
-        void *bounce_buffer = qemu_blockalign(s->blk->conf.bs, qiov->size);
-
-        /* Populate bounce buffer with data for writes */
-        if (!read) {
-            qemu_iovec_to_buf(qiov, 0, bounce_buffer, qiov->size);
-        }
-
-        /* Redirect I/O to aligned bounce buffer */
-        req->bounce_iov.iov_base = bounce_buffer;
-        req->bounce_iov.iov_len = qiov->size;
-        qemu_iovec_init_external(&req->bounce_qiov, &req->bounce_iov, 1);
-        qiov = &req->bounce_qiov;
-    }
-
     nb_sectors = qiov->size / BDRV_SECTOR_SIZE;
 
     if (read) {
-- 
2.0.0

[Qemu-devel] [PATCH v3 4/9] virtio-blk: Drop VirtIOBlockRequest.read

[Fam Zheng]

Since it's set but not used.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 3d1e9e1..4e5e458 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -29,7 +29,6 @@ typedef struct {
     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
     VirtQueueElement *elem;         /* saved data from the virtqueue */
     QEMUIOVector qiov;              /* original request iovecs */
-    bool read;                      /* read or write? */
 } VirtIOBlockRequest;
 
 struct VirtIOBlockDataPlane {
@@ -137,7 +136,6 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
     req->s = s;
     req->elem = elem;
     req->inhdr = inhdr;
-    req->read = read;
     qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
 
     qiov = &req->qiov;
-- 
2.0.0

[Qemu-devel] [PATCH v3 5/9] virtio-blk: Replace VirtIOBlockRequest with VirtIOBlockReq

[Fam Zheng]

Field "inhdr" is added temporarily for a more mechanical change, and
will be dropped in the next commit.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 34 +++++++++++++++-------------------
 include/hw/virtio/virtio-blk.h  |  4 ++++
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 4e5e458..70e8c14 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -24,13 +24,6 @@
 #include "hw/virtio/virtio-bus.h"
 #include "qom/object_interfaces.h"
 
-typedef struct {
-    VirtIOBlockDataPlane *s;
-    QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
-    VirtQueueElement *elem;         /* saved data from the virtqueue */
-    QEMUIOVector qiov;              /* original request iovecs */
-} VirtIOBlockRequest;
-
 struct VirtIOBlockDataPlane {
     bool started;
     bool starting;
@@ -68,7 +61,7 @@ static void notify_guest(VirtIOBlockDataPlane *s)
 
 static void complete_rdwr(void *opaque, int ret)
 {
-    VirtIOBlockRequest *req = opaque;
+    VirtIOBlockReq *req = opaque;
     struct virtio_blk_inhdr hdr;
     int len;
 
@@ -80,7 +73,8 @@ static void complete_rdwr(void *opaque, int ret)
         len = 0;
     }
 
-    trace_virtio_blk_data_plane_complete_request(req->s, req->elem->index, ret);
+    trace_virtio_blk_data_plane_complete_request(req->dev->dataplane,
+                                                 req->elem->index, ret);
 
     qemu_iovec_from_buf(req->inhdr, 0, &hdr, sizeof(hdr));
     qemu_iovec_destroy(req->inhdr);
@@ -90,9 +84,9 @@ static void complete_rdwr(void *opaque, int ret)
      * written to, but for virtio-blk it seems to be the number of bytes
      * transferred plus the status bytes.
      */
-    vring_push(&req->s->vring, req->elem, len + sizeof(hdr));
-    notify_guest(req->s);
-    g_slice_free(VirtIOBlockRequest, req);
+    vring_push(&req->dev->dataplane->vring, req->elem, len + sizeof(hdr));
+    notify_guest(req->dev->dataplane);
+    g_slice_free(VirtIOBlockReq, req);
 }
 
 static void complete_request_early(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
@@ -128,14 +122,15 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
                         int64_t sector_num, VirtQueueElement *elem,
                         QEMUIOVector *inhdr)
 {
-    VirtIOBlockRequest *req = g_slice_new0(VirtIOBlockRequest);
+    VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
+    VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
     QEMUIOVector *qiov;
     int nb_sectors;
 
     /* Fill in virtio block metadata needed for completion */
-    req->s = s;
     req->elem = elem;
     req->inhdr = inhdr;
+    req->dev = dev;
     qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
 
     qiov = &req->qiov;
@@ -153,7 +148,7 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
 
 static void complete_flush(void *opaque, int ret)
 {
-    VirtIOBlockRequest *req = opaque;
+    VirtIOBlockReq *req = opaque;
     unsigned char status;
 
     if (ret == 0) {
@@ -162,15 +157,16 @@ static void complete_flush(void *opaque, int ret)
         status = VIRTIO_BLK_S_IOERR;
     }
 
-    complete_request_early(req->s, req->elem, req->inhdr, status);
-    g_slice_free(VirtIOBlockRequest, req);
+    complete_request_early(req->dev->dataplane, req->elem, req->inhdr, status);
+    g_slice_free(VirtIOBlockReq, req);
 }
 
 static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
                          QEMUIOVector *inhdr)
 {
-    VirtIOBlockRequest *req = g_slice_new(VirtIOBlockRequest);
-    req->s = s;
+    VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
+    VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
+    req->dev = dev;
     req->elem = elem;
     req->inhdr = inhdr;
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 1932502..9ce0957 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -142,6 +142,10 @@ typedef struct VirtIOBlockReq {
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
+
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+    QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
+#endif
 } VirtIOBlockReq;
 
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
-- 
2.0.0

[Qemu-devel] [PATCH v3 6/9] virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr

[Fam Zheng]

In current virtio spec, inhdr is a single byte, and is unlikely to
change for both functionality and compatibility considerations.
Non-dataplane uses .in, and we are on the way to converge them. So
let's unify it to get cleaner code.

Remove .inhdr and use .in.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 52 +++++++++++++++--------------------------
 include/hw/virtio/virtio-blk.h  |  4 ----
 2 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 70e8c14..4585494 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -76,9 +76,7 @@ static void complete_rdwr(void *opaque, int ret)
     trace_virtio_blk_data_plane_complete_request(req->dev->dataplane,
                                                  req->elem->index, ret);
 
-    qemu_iovec_from_buf(req->inhdr, 0, &hdr, sizeof(hdr));
-    qemu_iovec_destroy(req->inhdr);
-    g_slice_free(QEMUIOVector, req->inhdr);
+    stb_p(&req->in->status, hdr.status);
 
     /* According to the virtio specification len should be the number of bytes
      * written to, but for virtio-blk it seems to be the number of bytes
@@ -90,24 +88,20 @@ static void complete_rdwr(void *opaque, int ret)
 }
 
 static void complete_request_early(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                                   QEMUIOVector *inhdr, unsigned char status)
+                                   struct virtio_blk_inhdr *inhdr,
+                                   unsigned char status)
 {
-    struct virtio_blk_inhdr hdr = {
-        .status = status,
-    };
+    stb_p(&inhdr->status, status);
 
-    qemu_iovec_from_buf(inhdr, 0, &hdr, sizeof(hdr));
-    qemu_iovec_destroy(inhdr);
-    g_slice_free(QEMUIOVector, inhdr);
-
-    vring_push(&s->vring, elem, sizeof(hdr));
+    vring_push(&s->vring, elem, sizeof(*inhdr));
     notify_guest(s);
 }
 
 /* Get disk serial number */
 static void do_get_id_cmd(VirtIOBlockDataPlane *s,
                           struct iovec *iov, unsigned int iov_cnt,
-                          VirtQueueElement *elem, QEMUIOVector *inhdr)
+                          VirtQueueElement *elem,
+                          struct virtio_blk_inhdr *inhdr)
 {
     char id[VIRTIO_BLK_ID_BYTES];
 
@@ -120,7 +114,7 @@ static void do_get_id_cmd(VirtIOBlockDataPlane *s,
 static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
                         struct iovec *iov, unsigned iov_cnt,
                         int64_t sector_num, VirtQueueElement *elem,
-                        QEMUIOVector *inhdr)
+                        struct virtio_blk_inhdr *inhdr)
 {
     VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
     VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
@@ -129,8 +123,8 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
 
     /* Fill in virtio block metadata needed for completion */
     req->elem = elem;
-    req->inhdr = inhdr;
     req->dev = dev;
+    req->in = inhdr;
     qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
 
     qiov = &req->qiov;
@@ -157,24 +151,24 @@ static void complete_flush(void *opaque, int ret)
         status = VIRTIO_BLK_S_IOERR;
     }
 
-    complete_request_early(req->dev->dataplane, req->elem, req->inhdr, status);
+    complete_request_early(req->dev->dataplane, req->elem, req->in, status);
     g_slice_free(VirtIOBlockReq, req);
 }
 
 static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                         QEMUIOVector *inhdr)
+                         struct virtio_blk_inhdr *inhdr)
 {
     VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
     VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
     req->dev = dev;
     req->elem = elem;
-    req->inhdr = inhdr;
+    req->in = inhdr;
 
     bdrv_aio_flush(s->blk->conf.bs, complete_flush, req);
 }
 
 static void do_scsi_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                        QEMUIOVector *inhdr)
+                        struct virtio_blk_inhdr *inhdr)
 {
     int status;
 
@@ -189,8 +183,7 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
     unsigned out_num = elem->out_num;
     unsigned in_num = elem->in_num;
     struct virtio_blk_outhdr outhdr;
-    QEMUIOVector *inhdr;
-    size_t in_size;
+    struct virtio_blk_inhdr *inhdr;
 
     /* Copy in outhdr */
     if (unlikely(iov_to_buf(iov, out_num, 0, &outhdr,
@@ -200,17 +193,12 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
     }
     iov_discard_front(&iov, &out_num, sizeof(outhdr));
 
+    /* This is always true because it is only 1 byte, but checked here in case
+     * the header gets bigger in the future. */
+    assert(in_iov[in_num - 1].iov_len >= sizeof(*inhdr));
     /* Grab inhdr for later */
-    in_size = iov_size(in_iov, in_num);
-    if (in_size < sizeof(struct virtio_blk_inhdr)) {
-        error_report("virtio_blk request inhdr too short");
-        return -EFAULT;
-    }
-    inhdr = g_slice_new(QEMUIOVector);
-    qemu_iovec_init(inhdr, 1);
-    qemu_iovec_concat_iov(inhdr, in_iov, in_num,
-            in_size - sizeof(struct virtio_blk_inhdr),
-            sizeof(struct virtio_blk_inhdr));
+    inhdr = (void *)in_iov[in_num - 1].iov_base
+            + in_iov[in_num - 1].iov_len - sizeof(*inhdr);
     iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
 
     /* TODO Linux sets the barrier bit even when not advertised! */
@@ -243,8 +231,6 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
 
     default:
         error_report("virtio-blk unsupported request type %#x", outhdr.type);
-        qemu_iovec_destroy(inhdr);
-        g_slice_free(QEMUIOVector, inhdr);
         return -EFAULT;
     }
 }
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 9ce0957..1932502 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -142,10 +142,6 @@ typedef struct VirtIOBlockReq {
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
-
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
-#endif
 } VirtIOBlockReq;
 
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
-- 
2.0.0

Re: [Qemu-devel] [PATCH v3 6/9] virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr

[Stefan Hajnoczi]

On Fri, Jun 06, 2014 at 09:53:27AM +0800, Fam Zheng wrote:
> @@ -200,17 +193,12 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
>      }
>      iov_discard_front(&iov, &out_num, sizeof(outhdr));
>  
> +    /* This is always true because it is only 1 byte, but checked here in case
> +     * the header gets bigger in the future. */
> +    assert(in_iov[in_num - 1].iov_len >= sizeof(*inhdr));
>      /* Grab inhdr for later */
> -    in_size = iov_size(in_iov, in_num);
> -    if (in_size < sizeof(struct virtio_blk_inhdr)) {
> -        error_report("virtio_blk request inhdr too short");
> -        return -EFAULT;
> -    }

This assertion can be triggered by the guest.  It even accesses
undefined memory when in_num == 0.

Please be careful, we need to validate guest input.

Stefan

[Qemu-devel] [PATCH v3 7/9] virtio-blk: Convert VirtIOBlockReq.out to structrue

[Fam Zheng]

The virtio code currently assumes that the outhdr is in its own iovec.
This is not guaranteed by the spec, so we should relax this assumption.

Convert the VirtIOBlockReq.out field to structrue so that we can use
iov_to_buf and then discard the header from the beginning of iovec.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c          | 20 ++++++++++++++------
 include/hw/virtio/virtio-blk.h |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 388741e..2282e61 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu-common.h"
+#include "qemu/iov.h"
 #include "qemu/error-report.h"
 #include "trace.h"
 #include "hw/block/block.h"
@@ -81,7 +82,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     trace_virtio_blk_rw_complete(req, ret);
 
     if (ret) {
-        bool is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
+        bool is_read = !(ldl_p(&req->out.type) & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
             return;
     }
@@ -287,7 +288,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     BlockRequest *blkreq;
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = ldq_p(&req->out.sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
 
@@ -321,7 +322,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 {
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = ldq_p(&req->out.sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
 
@@ -344,22 +345,29 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
     MultiReqBuffer *mrb)
 {
     uint32_t type;
+    struct iovec *iov = req->elem->out_sg;
+    unsigned out_num = req->elem->out_num;
 
     if (req->elem->out_num < 1 || req->elem->in_num < 1) {
         error_report("virtio-blk missing headers");
         exit(1);
     }
 
-    if (req->elem->out_sg[0].iov_len < sizeof(*req->out) ||
+    if (req->elem->out_sg[0].iov_len < sizeof(req->out) ||
         req->elem->in_sg[req->elem->in_num - 1].iov_len < sizeof(*req->in)) {
         error_report("virtio-blk header not in correct element");
         exit(1);
     }
 
-    req->out = (void *)req->elem->out_sg[0].iov_base;
+    if (unlikely(iov_to_buf(iov, out_num, 0, &req->out,
+                            sizeof(req->out)) != sizeof(req->out))) {
+        error_report("virtio-blk request outhdr too short");
+        exit(1);
+    }
+    iov_discard_front(&iov, &out_num, sizeof(req->out));
     req->in = (void *)req->elem->in_sg[req->elem->in_num - 1].iov_base;
 
-    type = ldl_p(&req->out->type);
+    type = ldl_p(&req->out.type);
 
     if (type & VIRTIO_BLK_T_FLUSH) {
         virtio_blk_handle_flush(req, mrb);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 1932502..df9b6f9 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -138,7 +138,7 @@ typedef struct VirtIOBlockReq {
     VirtIOBlock *dev;
     VirtQueueElement *elem;
     struct virtio_blk_inhdr *in;
-    struct virtio_blk_outhdr *out;
+    struct virtio_blk_outhdr out;
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
-- 
2.0.0

[Qemu-devel] [PATCH v3 8/9] virtio-blk: Fill in VirtIOBlockReq.out in dataplane code

[Fam Zheng]

VirtIOBlockReq is allocated in process_request, and freed in command
functions.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 102 +++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 58 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 4585494..c78d04e 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -87,55 +87,49 @@ static void complete_rdwr(void *opaque, int ret)
     g_slice_free(VirtIOBlockReq, req);
 }
 
-static void complete_request_early(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                                   struct virtio_blk_inhdr *inhdr,
-                                   unsigned char status)
+static void complete_request_early(VirtIOBlockReq *req, unsigned char status)
 {
-    stb_p(&inhdr->status, status);
+    stb_p(&req->in->status, status);
 
-    vring_push(&s->vring, elem, sizeof(*inhdr));
-    notify_guest(s);
+    vring_push(&req->dev->dataplane->vring, req->elem, sizeof(*req->in));
+    notify_guest(req->dev->dataplane);
+    g_slice_free(VirtIOBlockReq, req);
 }
 
 /* Get disk serial number */
-static void do_get_id_cmd(VirtIOBlockDataPlane *s,
-                          struct iovec *iov, unsigned int iov_cnt,
-                          VirtQueueElement *elem,
-                          struct virtio_blk_inhdr *inhdr)
+static void do_get_id_cmd(VirtIOBlockReq *req,
+                          struct iovec *iov, unsigned int iov_cnt)
 {
     char id[VIRTIO_BLK_ID_BYTES];
 
     /* Serial number not NUL-terminated when longer than buffer */
-    strncpy(id, s->blk->serial ? s->blk->serial : "", sizeof(id));
+    strncpy(id, req->dev->blk.serial ? req->dev->blk.serial : "", sizeof(id));
     iov_from_buf(iov, iov_cnt, 0, id, sizeof(id));
-    complete_request_early(s, elem, inhdr, VIRTIO_BLK_S_OK);
+    complete_request_early(req, VIRTIO_BLK_S_OK);
 }
 
-static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
-                        struct iovec *iov, unsigned iov_cnt,
-                        int64_t sector_num, VirtQueueElement *elem,
-                        struct virtio_blk_inhdr *inhdr)
+
+static void do_rdwr_cmd(bool read, VirtIOBlockReq *req,
+                        struct iovec *iov, unsigned iov_cnt)
 {
-    VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
-    VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
     QEMUIOVector *qiov;
     int nb_sectors;
+    int64_t sector_num;
 
-    /* Fill in virtio block metadata needed for completion */
-    req->elem = elem;
-    req->dev = dev;
-    req->in = inhdr;
     qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
 
     qiov = &req->qiov;
 
+    sector_num = req->out.sector * 512 / BDRV_SECTOR_SIZE;
     nb_sectors = qiov->size / BDRV_SECTOR_SIZE;
 
     if (read) {
-        bdrv_aio_readv(s->blk->conf.bs, sector_num, qiov, nb_sectors,
+        bdrv_aio_readv(req->dev->blk.conf.bs,
+                       sector_num, qiov, nb_sectors,
                        complete_rdwr, req);
     } else {
-        bdrv_aio_writev(s->blk->conf.bs, sector_num, qiov, nb_sectors,
+        bdrv_aio_writev(req->dev->blk.conf.bs,
+                        sector_num, qiov, nb_sectors,
                         complete_rdwr, req);
     }
 }
@@ -151,29 +145,21 @@ static void complete_flush(void *opaque, int ret)
         status = VIRTIO_BLK_S_IOERR;
     }
 
-    complete_request_early(req->dev->dataplane, req->elem, req->in, status);
-    g_slice_free(VirtIOBlockReq, req);
+    complete_request_early(req, status);
 }
 
-static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                         struct virtio_blk_inhdr *inhdr)
+static void do_flush_cmd(VirtIOBlockReq *req)
 {
-    VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
-    VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
-    req->dev = dev;
-    req->elem = elem;
-    req->in = inhdr;
 
-    bdrv_aio_flush(s->blk->conf.bs, complete_flush, req);
+    bdrv_aio_flush(req->dev->blk.conf.bs, complete_flush, req);
 }
 
-static void do_scsi_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                        struct virtio_blk_inhdr *inhdr)
+static void do_scsi_cmd(VirtIOBlockReq *req)
 {
     int status;
 
-    status = virtio_blk_handle_scsi_req(VIRTIO_BLK(s->vdev), elem);
-    complete_request_early(s, elem, inhdr, status);
+    status = virtio_blk_handle_scsi_req(req->dev, req->elem);
+    complete_request_early(req, status);
 }
 
 static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
@@ -182,55 +168,55 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
     struct iovec *in_iov = elem->in_sg;
     unsigned out_num = elem->out_num;
     unsigned in_num = elem->in_num;
-    struct virtio_blk_outhdr outhdr;
-    struct virtio_blk_inhdr *inhdr;
+    VirtIOBlockReq *req;
 
+    req = g_slice_new(VirtIOBlockReq);
+    req->dev = VIRTIO_BLK(s->vdev);
+    req->elem = elem;
     /* Copy in outhdr */
-    if (unlikely(iov_to_buf(iov, out_num, 0, &outhdr,
-                            sizeof(outhdr)) != sizeof(outhdr))) {
+    if (unlikely(iov_to_buf(iov, out_num, 0, &req->out,
+                            sizeof(req->out)) != sizeof(req->out))) {
         error_report("virtio-blk request outhdr too short");
+        g_slice_free(VirtIOBlockReq, req);
         return -EFAULT;
     }
-    iov_discard_front(&iov, &out_num, sizeof(outhdr));
+    iov_discard_front(&iov, &out_num, sizeof(req->out));
 
     /* This is always true because it is only 1 byte, but checked here in case
      * the header gets bigger in the future. */
-    assert(in_iov[in_num - 1].iov_len >= sizeof(*inhdr));
+    assert(in_iov[in_num - 1].iov_len >= sizeof(*req->in));
     /* Grab inhdr for later */
-    inhdr = (void *)in_iov[in_num - 1].iov_base
-            + in_iov[in_num - 1].iov_len - sizeof(*inhdr);
+    req->in = (void *)in_iov[in_num - 1].iov_base
+            + in_iov[in_num - 1].iov_len - sizeof(*req->in);
     iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
 
     /* TODO Linux sets the barrier bit even when not advertised! */
-    outhdr.type &= ~VIRTIO_BLK_T_BARRIER;
+    req->out.type &= ~VIRTIO_BLK_T_BARRIER;
 
-    switch (outhdr.type) {
+    switch (req->out.type) {
     case VIRTIO_BLK_T_IN:
-        do_rdwr_cmd(s, true, in_iov, in_num,
-                    outhdr.sector * 512 / BDRV_SECTOR_SIZE,
-                    elem, inhdr);
+        do_rdwr_cmd(true, req, in_iov, in_num);
         return 0;
 
     case VIRTIO_BLK_T_OUT:
-        do_rdwr_cmd(s, false, iov, out_num,
-                    outhdr.sector * 512 / BDRV_SECTOR_SIZE,
-                    elem, inhdr);
+        do_rdwr_cmd(false, req, iov, out_num);
         return 0;
 
     case VIRTIO_BLK_T_SCSI_CMD:
-        do_scsi_cmd(s, elem, inhdr);
+        do_scsi_cmd(req);
         return 0;
 
     case VIRTIO_BLK_T_FLUSH:
-        do_flush_cmd(s, elem, inhdr);
+        do_flush_cmd(req);
         return 0;
 
     case VIRTIO_BLK_T_GET_ID:
-        do_get_id_cmd(s, in_iov, in_num, elem, inhdr);
+        do_get_id_cmd(req, in_iov, in_num);
         return 0;
 
     default:
-        error_report("virtio-blk unsupported request type %#x", outhdr.type);
+        error_report("virtio-blk unsupported request type %#x", req->out.type);
+        g_slice_free(VirtIOBlockReq, req);
         return -EFAULT;
     }
 }
-- 
2.0.0

[Qemu-devel] [PATCH v3 9/9] virtio-blk: Fix and clean up the in_sg and out_sg check

[Fam Zheng]

out_sg is checked by iov_to_buf below, so it can be dropped.

Add assert and iov_discard_back around in_sg, as the in_sg is handled in
dataplane code.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 2282e61..cd1a8a7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -345,7 +345,9 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
     MultiReqBuffer *mrb)
 {
     uint32_t type;
+    struct iovec *in_iov = req->elem->in_sg;
     struct iovec *iov = req->elem->out_sg;
+    unsigned in_num = req->elem->in_num;
     unsigned out_num = req->elem->out_num;
 
     if (req->elem->out_num < 1 || req->elem->in_num < 1) {
@@ -353,19 +355,18 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
         exit(1);
     }
 
-    if (req->elem->out_sg[0].iov_len < sizeof(req->out) ||
-        req->elem->in_sg[req->elem->in_num - 1].iov_len < sizeof(*req->in)) {
-        error_report("virtio-blk header not in correct element");
-        exit(1);
-    }
-
     if (unlikely(iov_to_buf(iov, out_num, 0, &req->out,
                             sizeof(req->out)) != sizeof(req->out))) {
         error_report("virtio-blk request outhdr too short");
         exit(1);
     }
     iov_discard_front(&iov, &out_num, sizeof(req->out));
-    req->in = (void *)req->elem->in_sg[req->elem->in_num - 1].iov_base;
+    assert(in_iov[in_num - 1].iov_len >=
+           sizeof(struct virtio_blk_inhdr));
+    req->in = (void *)in_iov[in_num - 1].iov_base
+              + in_iov[in_num - 1].iov_len
+              - sizeof(struct virtio_blk_inhdr);
+    iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
 
     type = ldl_p(&req->out.type);
 
-- 
2.0.0

Re: [Qemu-devel] [PATCH v3 9/9] virtio-blk: Fix and clean up the in_sg and out_sg check

[Stefan Hajnoczi]

On Fri, Jun 06, 2014 at 09:53:30AM +0800, Fam Zheng wrote:
> @@ -353,19 +355,18 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
>          exit(1);
>      }
>  
> -    if (req->elem->out_sg[0].iov_len < sizeof(req->out) ||
> -        req->elem->in_sg[req->elem->in_num - 1].iov_len < sizeof(*req->in)) {
> -        error_report("virtio-blk header not in correct element");
> -        exit(1);
> -    }
> -
>      if (unlikely(iov_to_buf(iov, out_num, 0, &req->out,
>                              sizeof(req->out)) != sizeof(req->out))) {
>          error_report("virtio-blk request outhdr too short");
>          exit(1);
>      }
>      iov_discard_front(&iov, &out_num, sizeof(req->out));
> -    req->in = (void *)req->elem->in_sg[req->elem->in_num - 1].iov_base;
> +    assert(in_iov[in_num - 1].iov_len >=
> +           sizeof(struct virtio_blk_inhdr));

Why use assert() when the rest of the function uses error_report() +
exit(1)?  Please keep the code consistent.

Re: [Qemu-devel] [PATCH v3 9/9] virtio-blk: Fix and clean up the in_sg and out_sg check

[Paolo Bonzini]

Il 06/06/2014 15:16, Stefan Hajnoczi ha scritto:
>> > -    req->in = (void *)req->elem->in_sg[req->elem->in_num - 1].iov_base;
>> > +    assert(in_iov[in_num - 1].iov_len >=
>> > +           sizeof(struct virtio_blk_inhdr));
> Why use assert() when the rest of the function uses error_report() +
> exit(1)?  Please keep the code consistent.

Sorry, that's my fault.  I suggested assert thinking that zero-length 
iovecs wouldn't be possible here.

Paolo

Re: [Qemu-devel] [PATCH v3 0/9] virtio-blk: Converge VirtIOBlockRequest into VirtIOBlockReq

[Paolo Bonzini]

Il 06/06/2014 03:53, Fam Zheng ha scritto:
> This unifies the request structure used by dataplane and non-dataplane code,
> while dropping unnessary fields for bounce buffer and read flag.
>
> Applies on top of Stefan's block tree.
>
> v3: Address Paolo's comments:
>
> [06/09] virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr
>         Add assertion and comments on inhdr.
>         Fix the offset and iov_discard_back.
>
> [08/09] virtio-blk: Fill in VirtIOBlockReq.out in dataplane code
>         Allocate req in process_request.
>
> [09/09] virtio-blk: Fix and clean up the in_sg and out_sg check
>         New.
>
>
>
> Fam Zheng (9):
>   virtio-blk: Move VirtIOBlockReq to header
>   virtio-blk: Convert VirtIOBlockReq.elem to pointer
>   virtio-blk: Drop bounce buffer from dataplane code
>   virtio-blk: Drop VirtIOBlockRequest.read
>   virtio-blk: Replace VirtIOBlockRequest with VirtIOBlockReq
>   virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr
>   virtio-blk: Convert VirtIOBlockReq.out to structrue
>   virtio-blk: Fill in VirtIOBlockReq.out in dataplane code
>   virtio-blk: Fix and clean up the in_sg and out_sg check
>
>  hw/block/dataplane/virtio-blk.c | 167 +++++++++++++---------------------------
>  hw/block/virtio-blk.c           | 113 ++++++++++++++-------------
>  include/hw/virtio/virtio-blk.h  |  11 +++
>  3 files changed, 124 insertions(+), 167 deletions(-)
>

Nice diffstat, too. :)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v3 0/9] virtio-blk: Converge VirtIOBlockRequest into VirtIOBlockReq

[Stefan Hajnoczi]

On Fri, Jun 06, 2014 at 09:53:21AM +0800, Fam Zheng wrote:
> This unifies the request structure used by dataplane and non-dataplane code,
> while dropping unnessary fields for bounce buffer and read flag.
> 
> Applies on top of Stefan's block tree.
> 
> v3: Address Paolo's comments:
> 
> [06/09] virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr
>         Add assertion and comments on inhdr.
>         Fix the offset and iov_discard_back.
> 
> [08/09] virtio-blk: Fill in VirtIOBlockReq.out in dataplane code
>         Allocate req in process_request.
> 
> [09/09] virtio-blk: Fix and clean up the in_sg and out_sg check
>         New.
> 
> 
> 
> Fam Zheng (9):
>   virtio-blk: Move VirtIOBlockReq to header
>   virtio-blk: Convert VirtIOBlockReq.elem to pointer
>   virtio-blk: Drop bounce buffer from dataplane code
>   virtio-blk: Drop VirtIOBlockRequest.read
>   virtio-blk: Replace VirtIOBlockRequest with VirtIOBlockReq
>   virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr
>   virtio-blk: Convert VirtIOBlockReq.out to structrue
>   virtio-blk: Fill in VirtIOBlockReq.out in dataplane code
>   virtio-blk: Fix and clean up the in_sg and out_sg check
> 
>  hw/block/dataplane/virtio-blk.c | 167 +++++++++++++---------------------------
>  hw/block/virtio-blk.c           | 113 ++++++++++++++-------------
>  include/hw/virtio/virtio-blk.h  |  11 +++
>  3 files changed, 124 insertions(+), 167 deletions(-)

Looks pretty good, just left a few minor comments.