[Qemu-devel] [PATCH v2 0/7] virtio-scsi: do not rely on iov boundaries

[Qemu-devel] [PATCH v2 0/7] virtio-scsi: do not rely on iov boundaries

[Paolo Bonzini]

The upcoming virtio 1.0 standard requires virtio devices not to
rely on iov boundaries when parsing requests or sending responses.
This series converts virtio-scsi.

v1->v2: missed a few replacements in patch 6 (Paolo)
	never compare in_num/out_num to a non-zero value (Paolo)
	do not use in_sg[0]/out_sg[0] (mst)

Paolo Bonzini (7):
  util: add return value to qemu_iovec_concat_iov
  virtio-scsi: start preparing for any_layout
  virtio-scsi: add target swap for VirtIOSCSICtrlTMFReq fields
  virtio-scsi: add extra argument and return type to qemu_sgl_concat
  virtio-scsi: prepare sense data handling for any_layout
  virtio-scsi: introduce virtio_scsi_complete_cmd_req
  virtio-scsi: add support for the any_layout feature

 hw/scsi/virtio-scsi.c           | 314 ++++++++++++++++++++++++----------------
 include/hw/i386/pc.h            |   4 +
 include/hw/virtio/virtio-scsi.h |   4 +-
 include/qemu-common.h           |   6 +-
 util/iov.c                      |  10 +-
 5 files changed, 202 insertions(+), 136 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/7] util: add return value to qemu_iovec_concat_iov

[Paolo Bonzini]

This will be necessary later to recognize the case where a
request has both dataout and datain.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu-common.h |  6 +++---
 util/iov.c            | 10 ++++++----
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 66ceceb..ae76197 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -315,9 +315,9 @@ void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov);
 void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len);
 void qemu_iovec_concat(QEMUIOVector *dst,
                        QEMUIOVector *src, size_t soffset, size_t sbytes);
-void qemu_iovec_concat_iov(QEMUIOVector *dst,
-                           struct iovec *src_iov, unsigned int src_cnt,
-                           size_t soffset, size_t sbytes);
+size_t qemu_iovec_concat_iov(QEMUIOVector *dst,
+                             struct iovec *src_iov, unsigned int src_cnt,
+                             size_t soffset, size_t sbytes);
 bool qemu_iovec_is_zero(QEMUIOVector *qiov);
 void qemu_iovec_destroy(QEMUIOVector *qiov);
 void qemu_iovec_reset(QEMUIOVector *qiov);
diff --git a/util/iov.c b/util/iov.c
index 49f8838..2b4f46d 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -295,15 +295,15 @@ void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len)
  * of src".
  * Only vector pointers are processed, not the actual data buffers.
  */
-void qemu_iovec_concat_iov(QEMUIOVector *dst,
-                           struct iovec *src_iov, unsigned int src_cnt,
-                           size_t soffset, size_t sbytes)
+size_t qemu_iovec_concat_iov(QEMUIOVector *dst,
+                             struct iovec *src_iov, unsigned int src_cnt,
+                             size_t soffset, size_t sbytes)
 {
     int i;
     size_t done;
 
     if (!sbytes) {
-        return;
+        return 0;
     }
     assert(dst->nalloc != -1);
     for (i = 0, done = 0; done < sbytes && i < src_cnt; i++) {
@@ -317,6 +317,8 @@ void qemu_iovec_concat_iov(QEMUIOVector *dst,
         }
     }
     assert(soffset == 0); /* offset beyond end of src */
+
+    return done;
 }
 
 /*
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/7] virtio-scsi: start preparing for any_layout

[Paolo Bonzini]

- Introduce virtio_scsi_init_req and virtio_scsi_free_req

- rename qemu_sgl_init_external to qemu_sgl_concat

- move virtio_scsi_parse_req from virtio_scsi_pop_req to callers
  and add header length checks to virtio_scsi_parse_req.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi.c | 121 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 72 insertions(+), 49 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b39880a..f013e35 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -15,6 +15,7 @@
 
 #include "hw/virtio/virtio-scsi.h"
 #include "qemu/error-report.h"
+#include "qemu/iov.h"
 #include <hw/scsi/scsi.h>
 #include <block/scsi.h>
 #include <hw/virtio/virtio-bus.h>
@@ -56,18 +57,35 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
     return scsi_device_find(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
 }
 
+static VirtIOSCSIReq *virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq)
+{
+    VirtIOSCSIReq *req;
+    req = g_malloc(sizeof(*req));
+
+    req->vq = vq;
+    req->dev = s;
+    req->sreq = NULL;
+    qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory);
+    return req;
+}
+
+static void virtio_scsi_free_req(VirtIOSCSIReq *req)
+{
+    qemu_sglist_destroy(&req->qsgl);
+    g_free(req);
+}
+
 static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
 {
     VirtIOSCSI *s = req->dev;
     VirtQueue *vq = req->vq;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
     virtqueue_push(vq, &req->elem, req->qsgl.size + req->elem.in_sg[0].iov_len);
-    qemu_sglist_destroy(&req->qsgl);
     if (req->sreq) {
         req->sreq->hba_private = NULL;
         scsi_req_unref(req->sreq);
     }
-    g_free(req);
+    virtio_scsi_free_req(req);
     virtio_notify(vdev, vq);
 }
 
@@ -77,50 +95,55 @@ static void virtio_scsi_bad_req(void)
     exit(1);
 }
 
-static void qemu_sgl_init_external(VirtIOSCSIReq *req, struct iovec *sg,
+static void qemu_sgl_concat(VirtIOSCSIReq *req, struct iovec *sg,
                                    hwaddr *addr, int num)
 {
     QEMUSGList *qsgl = &req->qsgl;
 
-    qemu_sglist_init(qsgl, DEVICE(req->dev), num, &address_space_memory);
     while (num--) {
         qemu_sglist_add(qsgl, *(addr++), (sg++)->iov_len);
     }
 }
 
-static void virtio_scsi_parse_req(VirtIOSCSI *s, VirtQueue *vq,
-                                  VirtIOSCSIReq *req)
+static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
+                                 unsigned req_size, unsigned resp_size)
 {
-    assert(req->elem.in_num);
-    req->vq = vq;
-    req->dev = s;
-    req->sreq = NULL;
+    if (req->elem.in_num == 0) {
+        return -EINVAL;
+    }
+
+    if (req->elem.out_sg[0].iov_len < req_size) {
+        return -EINVAL;
+    }
     if (req->elem.out_num) {
         req->req.buf = req->elem.out_sg[0].iov_base;
     }
+
+    if (req->elem.in_sg[0].iov_len < resp_size) {
+        return -EINVAL;
+    }
     req->resp.buf = req->elem.in_sg[0].iov_base;
 
     if (req->elem.out_num > 1) {
-        qemu_sgl_init_external(req, &req->elem.out_sg[1],
-                               &req->elem.out_addr[1],
-                               req->elem.out_num - 1);
+        qemu_sgl_concat(req, &req->elem.out_sg[1],
+                        &req->elem.out_addr[1],
+                        req->elem.out_num - 1);
     } else {
-        qemu_sgl_init_external(req, &req->elem.in_sg[1],
-                               &req->elem.in_addr[1],
-                               req->elem.in_num - 1);
+        qemu_sgl_concat(req, &req->elem.in_sg[1],
+                        &req->elem.in_addr[1],
+                        req->elem.in_num - 1);
     }
+
+    return 0;
 }
 
 static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq)
 {
-    VirtIOSCSIReq *req;
-    req = g_malloc(sizeof(*req));
+    VirtIOSCSIReq *req = virtio_scsi_init_req(s, vq);
     if (!virtqueue_pop(vq, &req->elem)) {
-        g_free(req);
+        virtio_scsi_free_req(req);
         return NULL;
     }
-
-    virtio_scsi_parse_req(s, vq, req);
     return req;
 }
 
@@ -143,9 +166,9 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     VirtIOSCSIReq *req;
     uint32_t n;
 
-    req = g_malloc(sizeof(*req));
     qemu_get_be32s(f, &n);
     assert(n < vs->conf.num_queues);
+    req = virtio_scsi_init_req(s, vs->cmd_vqs[n]);
     qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem));
     /* TODO: add a way for SCSIBusInfo's load_request to fail,
      * and fail migration instead of asserting here.
@@ -156,7 +179,12 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
 #endif
     assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
     assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));
-    virtio_scsi_parse_req(s, vs->cmd_vqs[n], req);
+
+    if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
+                              sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 0) {
+        error_report("invalid SCSI request migration data");
+        exit(1);
+    }
 
     scsi_req_ref(sreq);
     req->sreq = sreq;
@@ -281,29 +309,29 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOSCSIReq *req;
 
     while ((req = virtio_scsi_pop_req(s, vq))) {
-        int out_size, in_size;
-        if (req->elem.out_num < 1 || req->elem.in_num < 1) {
+        int type;
+
+        if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
+                       &type, sizeof(type)) < sizeof(type)) {
             virtio_scsi_bad_req();
-            continue;
-        }
 
-        out_size = req->elem.out_sg[0].iov_len;
-        in_size = req->elem.in_sg[0].iov_len;
-        if (req->req.tmf->type == VIRTIO_SCSI_T_TMF) {
-            if (out_size < sizeof(VirtIOSCSICtrlTMFReq) ||
-                in_size < sizeof(VirtIOSCSICtrlTMFResp)) {
+        } else if (req->req.tmf->type == VIRTIO_SCSI_T_TMF) {
+            if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlTMFReq),
+                                      sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
                 virtio_scsi_bad_req();
+            } else {
+                virtio_scsi_do_tmf(s, req);
             }
-            virtio_scsi_do_tmf(s, req);
 
         } else if (req->req.tmf->type == VIRTIO_SCSI_T_AN_QUERY ||
                    req->req.tmf->type == VIRTIO_SCSI_T_AN_SUBSCRIBE) {
-            if (out_size < sizeof(VirtIOSCSICtrlANReq) ||
-                in_size < sizeof(VirtIOSCSICtrlANResp)) {
+            if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlANReq),
+                                      sizeof(VirtIOSCSICtrlANResp)) < 0) {
                 virtio_scsi_bad_req();
+            } else {
+                req->resp.an->event_actual = 0;
+                req->resp.an->response = VIRTIO_SCSI_S_OK;
             }
-            req->resp.an->event_actual = 0;
-            req->resp.an->response = VIRTIO_SCSI_S_OK;
         }
         virtio_scsi_complete_req(req);
     }
@@ -373,23 +401,18 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
 
     while ((req = virtio_scsi_pop_req(s, vq))) {
         SCSIDevice *d;
-        int out_size, in_size;
-        if (req->elem.out_num < 1 || req->elem.in_num < 1) {
-            virtio_scsi_bad_req();
-        }
-
-        out_size = req->elem.out_sg[0].iov_len;
-        in_size = req->elem.in_sg[0].iov_len;
-        if (out_size < sizeof(VirtIOSCSICmdReq) + vs->cdb_size ||
-            in_size < sizeof(VirtIOSCSICmdResp) + vs->sense_size) {
-            virtio_scsi_bad_req();
-        }
-
+        int rc;
         if (req->elem.out_num > 1 && req->elem.in_num > 1) {
             virtio_scsi_fail_cmd_req(req);
             continue;
         }
 
+        rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
+                                   sizeof(VirtIOSCSICmdResp) + vs->sense_size);
+        if (rc < 0) {
+            virtio_scsi_bad_req();
+        }
+
         d = virtio_scsi_device_find(s, req->req.cmd->lun);
         if (!d) {
             req->resp.cmd->response = VIRTIO_SCSI_S_BAD_TARGET;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/7] virtio-scsi: add target swap for VirtIOSCSICtrlTMFReq fields

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index f013e35..ec9a536 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -207,6 +207,7 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
     /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE".  */
     req->resp.tmf->response = VIRTIO_SCSI_S_OK;
 
+    tswap32s(&req->req.tmf->subtype);
     switch (req->req.tmf->subtype) {
     case VIRTIO_SCSI_T_TMF_ABORT_TASK:
     case VIRTIO_SCSI_T_TMF_QUERY_TASK:
@@ -314,8 +315,11 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
         if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
                        &type, sizeof(type)) < sizeof(type)) {
             virtio_scsi_bad_req();
+            continue;
+        }
 
-        } else if (req->req.tmf->type == VIRTIO_SCSI_T_TMF) {
+        tswap32s(&req->req.tmf->type);
+        if (req->req.tmf->type == VIRTIO_SCSI_T_TMF) {
             if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlTMFReq),
                                       sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
                 virtio_scsi_bad_req();
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/7] virtio-scsi: add extra argument and return type to qemu_sgl_concat

[Paolo Bonzini]

Will be used for anylayout support.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index ec9a536..0718626 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -95,14 +95,27 @@ static void virtio_scsi_bad_req(void)
     exit(1);
 }
 
-static void qemu_sgl_concat(VirtIOSCSIReq *req, struct iovec *sg,
-                                   hwaddr *addr, int num)
+static size_t qemu_sgl_concat(VirtIOSCSIReq *req, struct iovec *iov,
+                              hwaddr *addr, int num, size_t skip)
 {
     QEMUSGList *qsgl = &req->qsgl;
-
-    while (num--) {
-        qemu_sglist_add(qsgl, *(addr++), (sg++)->iov_len);
+    size_t copied = 0;
+
+    while (num) {
+        if (skip >= iov->iov_len) {
+            skip -= iov->iov_len;
+        } else {
+            qemu_sglist_add(qsgl, *addr + skip, iov->iov_len - skip);
+            copied += iov->iov_len - skip;
+            skip = 0;
+        }
+        iov++;
+        addr++;
+        num--;
     }
+
+    assert(skip == 0);
+    return copied;
 }
 
 static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
@@ -127,11 +140,11 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
     if (req->elem.out_num > 1) {
         qemu_sgl_concat(req, &req->elem.out_sg[1],
                         &req->elem.out_addr[1],
-                        req->elem.out_num - 1);
+                        req->elem.out_num - 1, 0);
     } else {
         qemu_sgl_concat(req, &req->elem.in_sg[1],
                         &req->elem.in_addr[1],
-                        req->elem.in_num - 1);
+                        req->elem.in_num - 1, 0);
     }
 
     return 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/7] virtio-scsi: prepare sense data handling for any_layout

[Paolo Bonzini]

Retrieve sense and copy it to guest memory, to prepare for when we will use
qemu_iovec_from_buf.

Swap response and request, since we'll use the tail of VirtIOSCSIReq
for the CDB.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 0718626..fbc7db7 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -26,12 +26,7 @@ typedef struct VirtIOSCSIReq {
     VirtQueueElement elem;
     QEMUSGList qsgl;
     SCSIRequest *sreq;
-    union {
-        char                  *buf;
-        VirtIOSCSICmdReq      *cmd;
-        VirtIOSCSICtrlTMFReq  *tmf;
-        VirtIOSCSICtrlANReq   *an;
-    } req;
+    size_t resp_size;
     union {
         char                  *buf;
         VirtIOSCSICmdResp     *cmd;
@@ -39,6 +34,12 @@ typedef struct VirtIOSCSIReq {
         VirtIOSCSICtrlANResp  *an;
         VirtIOSCSIEvent       *event;
     } resp;
+    union {
+        char                  *buf;
+        VirtIOSCSICmdReq      *cmd;
+        VirtIOSCSICtrlTMFReq  *tmf;
+        VirtIOSCSICtrlANReq   *an;
+    } req;
 } VirtIOSCSIReq;
 
 static inline int virtio_scsi_get_lun(uint8_t *lun)
@@ -136,6 +137,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
         return -EINVAL;
     }
     req->resp.buf = req->elem.in_sg[0].iov_base;
+    req->resp_size = resp_size;
 
     if (req->elem.out_num > 1) {
         qemu_sgl_concat(req, &req->elem.out_sg[1],
@@ -358,8 +360,7 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
                                          size_t resid)
 {
     VirtIOSCSIReq *req = r->hba_private;
-    VirtIOSCSI *s = req->dev;
-    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+    uint8_t sense[SCSI_SENSE_BUF_SIZE];
     uint32_t sense_len;
 
     if (r->io_canceled) {
@@ -372,8 +373,9 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
         req->resp.cmd->resid = tswap32(resid);
     } else {
         req->resp.cmd->resid = 0;
-        sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
-                                       vs->sense_size);
+        sense_len = scsi_req_get_sense(r, sense, sizeof(sense));
+        sense_len = MIN(sense_len, req->resp_size - sizeof(req->resp.cmd));
+        memcpy(req->resp.cmd->sense, sense, sense_len);
         req->resp.cmd->sense_len = tswap32(sense_len);
     }
     virtio_scsi_complete_req(req);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 6/7] virtio-scsi: introduce virtio_scsi_complete_cmd_req

[Paolo Bonzini]

This is also related to sense handling, and will be used
by anylayout.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2: missed a few replacements in patch 6 (Paolo)

 hw/scsi/virtio-scsi.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index fbc7db7..06fda89 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -356,6 +356,11 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     }
 }
 
+static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
+{
+    virtio_scsi_complete_req(req);
+}
+
 static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
                                          size_t resid)
 {
@@ -378,7 +383,7 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
         memcpy(req->resp.cmd->sense, sense, sense_len);
         req->resp.cmd->sense_len = tswap32(sense_len);
     }
-    virtio_scsi_complete_req(req);
+    virtio_scsi_complete_cmd_req(req);
 }
 
 static QEMUSGList *virtio_scsi_get_sg_list(SCSIRequest *r)
@@ -400,13 +405,13 @@ static void virtio_scsi_request_cancelled(SCSIRequest *r)
     } else {
         req->resp.cmd->response = VIRTIO_SCSI_S_ABORTED;
     }
-    virtio_scsi_complete_req(req);
+    virtio_scsi_complete_cmd_req(req);
 }
 
 static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
 {
     req->resp.cmd->response = VIRTIO_SCSI_S_FAILURE;
-    virtio_scsi_complete_req(req);
+    virtio_scsi_complete_cmd_req(req);
 }
 
 static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
@@ -435,7 +440,7 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
         d = virtio_scsi_device_find(s, req->req.cmd->lun);
         if (!d) {
             req->resp.cmd->response = VIRTIO_SCSI_S_BAD_TARGET;
-            virtio_scsi_complete_req(req);
+            virtio_scsi_complete_cmd_req(req);
             continue;
         }
         req->sreq = scsi_req_new(d, req->req.cmd->tag,
@@ -449,7 +454,7 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
             if (req->sreq->cmd.mode != req_mode ||
                 req->sreq->cmd.xfer > req->qsgl.size) {
                 req->resp.cmd->response = VIRTIO_SCSI_S_OVERRUN;
-                virtio_scsi_complete_req(req);
+                virtio_scsi_complete_cmd_req(req);
                 continue;
             }
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 7/7] virtio-scsi: add support for the any_layout feature

[Paolo Bonzini]

Store the request and response headers by value, and let
virtio_scsi_parse_req check that there is only one of datain
and dataout.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	v1->v2:
	never compare in_num/out_num to a non-zero value (Paolo)
	do not use in_sg[0]/out_sg[0] (mst)

 hw/scsi/virtio-scsi.c           | 193 ++++++++++++++++++++++------------------
 include/hw/i386/pc.h            |   4 +
 include/hw/virtio/virtio-scsi.h |   4 +-
 3 files changed, 109 insertions(+), 92 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 06fda89..3870c47 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -27,21 +27,27 @@ typedef struct VirtIOSCSIReq {
     QEMUSGList qsgl;
     SCSIRequest *sreq;
     size_t resp_size;
+    enum SCSIXferMode mode;
+    QEMUIOVector resp_iov;
     union {
-        char                  *buf;
-        VirtIOSCSICmdResp     *cmd;
-        VirtIOSCSICtrlTMFResp *tmf;
-        VirtIOSCSICtrlANResp  *an;
-        VirtIOSCSIEvent       *event;
+        VirtIOSCSICmdResp     cmd;
+        VirtIOSCSICtrlTMFResp tmf;
+        VirtIOSCSICtrlANResp  an;
+        VirtIOSCSIEvent       event;
     } resp;
     union {
-        char                  *buf;
-        VirtIOSCSICmdReq      *cmd;
-        VirtIOSCSICtrlTMFReq  *tmf;
-        VirtIOSCSICtrlANReq   *an;
+        struct {
+            VirtIOSCSICmdReq  cmd;
+            uint8_t           cdb[];
+        } QEMU_PACKED;
+        VirtIOSCSICtrlTMFReq  tmf;
+        VirtIOSCSICtrlANReq   an;
     } req;
 } VirtIOSCSIReq;
 
+QEMU_BUILD_BUG_ON(offsetof(VirtIOSCSIReq, req.cdb) !=
+                  offsetof(VirtIOSCSIReq, req.cmd) + sizeof(VirtIOSCSICmdReq));
+
 static inline int virtio_scsi_get_lun(uint8_t *lun)
 {
     return ((lun[2] << 8) | lun[3]) & 0x3FFF;
@@ -61,17 +67,21 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
 static VirtIOSCSIReq *virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq)
 {
     VirtIOSCSIReq *req;
-    req = g_malloc(sizeof(*req));
+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+
+    req = g_malloc0(sizeof(*req) + vs->cdb_size);
 
     req->vq = vq;
     req->dev = s;
     req->sreq = NULL;
     qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory);
+    qemu_iovec_init(&req->resp_iov, 1);
     return req;
 }
 
 static void virtio_scsi_free_req(VirtIOSCSIReq *req)
 {
+    qemu_iovec_destroy(&req->resp_iov);
     qemu_sglist_destroy(&req->qsgl);
     g_free(req);
 }
@@ -81,7 +91,9 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
     VirtIOSCSI *s = req->dev;
     VirtQueue *vq = req->vq;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    virtqueue_push(vq, &req->elem, req->qsgl.size + req->elem.in_sg[0].iov_len);
+
+    qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size);
+    virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
     if (req->sreq) {
         req->sreq->hba_private = NULL;
         scsi_req_unref(req->sreq);
@@ -122,31 +134,35 @@ static size_t qemu_sgl_concat(VirtIOSCSIReq *req, struct iovec *iov,
 static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
                                  unsigned req_size, unsigned resp_size)
 {
-    if (req->elem.in_num == 0) {
-        return -EINVAL;
-    }
+    size_t in_size, out_size;
 
-    if (req->elem.out_sg[0].iov_len < req_size) {
+    if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
+                   &req->req, req_size) < req_size) {
         return -EINVAL;
     }
-    if (req->elem.out_num) {
-        req->req.buf = req->elem.out_sg[0].iov_base;
-    }
 
-    if (req->elem.in_sg[0].iov_len < resp_size) {
+    if (qemu_iovec_concat_iov(&req->resp_iov,
+                              req->elem.in_sg, req->elem.in_num, 0,
+                              resp_size) < resp_size) {
         return -EINVAL;
     }
-    req->resp.buf = req->elem.in_sg[0].iov_base;
     req->resp_size = resp_size;
 
-    if (req->elem.out_num > 1) {
-        qemu_sgl_concat(req, &req->elem.out_sg[1],
-                        &req->elem.out_addr[1],
-                        req->elem.out_num - 1, 0);
-    } else {
-        qemu_sgl_concat(req, &req->elem.in_sg[1],
-                        &req->elem.in_addr[1],
-                        req->elem.in_num - 1, 0);
+    out_size = qemu_sgl_concat(req, req->elem.out_sg,
+                               &req->elem.out_addr[0], req->elem.out_num,
+                               req_size);
+    in_size = qemu_sgl_concat(req, req->elem.in_sg,
+                              &req->elem.in_addr[0], req->elem.in_num,
+                              resp_size);
+
+    if (out_size && in_size) {
+        return -ENOTSUP;
+    }
+
+    if (out_size) {
+        req->mode = SCSI_XFER_TO_DEV;
+    } else if (in_size) {
+        req->mode = SCSI_XFER_FROM_DEV;
     }
 
     return 0;
@@ -204,37 +220,34 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
     scsi_req_ref(sreq);
     req->sreq = sreq;
     if (req->sreq->cmd.mode != SCSI_XFER_NONE) {
-        int req_mode =
-            (req->elem.in_num > 1 ? SCSI_XFER_FROM_DEV : SCSI_XFER_TO_DEV);
-
-        assert(req->sreq->cmd.mode == req_mode);
+        assert(req->sreq->cmd.mode == req->mode);
     }
     return req;
 }
 
 static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
-    SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf->lun);
+    SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
     SCSIRequest *r, *next;
     BusChild *kid;
     int target;
 
     /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE".  */
-    req->resp.tmf->response = VIRTIO_SCSI_S_OK;
+    req->resp.tmf.response = VIRTIO_SCSI_S_OK;
 
-    tswap32s(&req->req.tmf->subtype);
-    switch (req->req.tmf->subtype) {
+    tswap32s(&req->req.tmf.subtype);
+    switch (req->req.tmf.subtype) {
     case VIRTIO_SCSI_T_TMF_ABORT_TASK:
     case VIRTIO_SCSI_T_TMF_QUERY_TASK:
         if (!d) {
             goto fail;
         }
-        if (d->lun != virtio_scsi_get_lun(req->req.tmf->lun)) {
+        if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
             goto incorrect_lun;
         }
         QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
             VirtIOSCSIReq *cmd_req = r->hba_private;
-            if (cmd_req && cmd_req->req.cmd->tag == req->req.tmf->tag) {
+            if (cmd_req && cmd_req->req.cmd.tag == req->req.tmf.tag) {
                 break;
             }
         }
@@ -244,11 +257,11 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
              * check for it in the loop above.
              */
             assert(r->hba_private);
-            if (req->req.tmf->subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK) {
+            if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK) {
                 /* "If the specified command is present in the task set, then
                  * return a service response set to FUNCTION SUCCEEDED".
                  */
-                req->resp.tmf->response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
+                req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
             } else {
                 scsi_req_cancel(r);
             }
@@ -259,7 +272,7 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
         if (!d) {
             goto fail;
         }
-        if (d->lun != virtio_scsi_get_lun(req->req.tmf->lun)) {
+        if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
             goto incorrect_lun;
         }
         s->resetting++;
@@ -273,16 +286,16 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
         if (!d) {
             goto fail;
         }
-        if (d->lun != virtio_scsi_get_lun(req->req.tmf->lun)) {
+        if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
             goto incorrect_lun;
         }
         QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
             if (r->hba_private) {
-                if (req->req.tmf->subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
+                if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
                     /* "If there is any command present in the task set, then
                      * return a service response set to FUNCTION SUCCEEDED".
                      */
-                    req->resp.tmf->response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
+                    req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
                     break;
                 } else {
                     scsi_req_cancel(r);
@@ -292,7 +305,7 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
         break;
 
     case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
-        target = req->req.tmf->lun[1];
+        target = req->req.tmf.lun[1];
         s->resetting++;
         QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
              d = DO_UPCAST(SCSIDevice, qdev, kid->child);
@@ -305,18 +318,18 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 
     case VIRTIO_SCSI_T_TMF_CLEAR_ACA:
     default:
-        req->resp.tmf->response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
+        req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
         break;
     }
 
     return;
 
 incorrect_lun:
-    req->resp.tmf->response = VIRTIO_SCSI_S_INCORRECT_LUN;
+    req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
     return;
 
 fail:
-    req->resp.tmf->response = VIRTIO_SCSI_S_BAD_TARGET;
+    req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
 }
 
 static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
@@ -333,8 +346,8 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
             continue;
         }
 
-        tswap32s(&req->req.tmf->type);
-        if (req->req.tmf->type == VIRTIO_SCSI_T_TMF) {
+        tswap32s(&req->req.tmf.type);
+        if (req->req.tmf.type == VIRTIO_SCSI_T_TMF) {
             if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlTMFReq),
                                       sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
                 virtio_scsi_bad_req();
@@ -342,14 +355,14 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
                 virtio_scsi_do_tmf(s, req);
             }
 
-        } else if (req->req.tmf->type == VIRTIO_SCSI_T_AN_QUERY ||
-                   req->req.tmf->type == VIRTIO_SCSI_T_AN_SUBSCRIBE) {
+        } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
+                   req->req.tmf.type == VIRTIO_SCSI_T_AN_SUBSCRIBE) {
             if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlANReq),
                                       sizeof(VirtIOSCSICtrlANResp)) < 0) {
                 virtio_scsi_bad_req();
             } else {
-                req->resp.an->event_actual = 0;
-                req->resp.an->response = VIRTIO_SCSI_S_OK;
+                req->resp.an.event_actual = 0;
+                req->resp.an.response = VIRTIO_SCSI_S_OK;
             }
         }
         virtio_scsi_complete_req(req);
@@ -358,6 +371,10 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 
 static void virtio_scsi_complete_cmd_req(VirtIOSCSIReq *req)
 {
+    /* Sense data is not in req->resp and is copied separately
+     * in virtio_scsi_command_complete.
+     */
+    req->resp_size = sizeof(VirtIOSCSICmdResp);
     virtio_scsi_complete_req(req);
 }
 
@@ -372,16 +389,17 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
         return;
     }
 
-    req->resp.cmd->response = VIRTIO_SCSI_S_OK;
-    req->resp.cmd->status = status;
-    if (req->resp.cmd->status == GOOD) {
-        req->resp.cmd->resid = tswap32(resid);
+    req->resp.cmd.response = VIRTIO_SCSI_S_OK;
+    req->resp.cmd.status = status;
+    if (req->resp.cmd.status == GOOD) {
+        req->resp.cmd.resid = tswap32(resid);
     } else {
-        req->resp.cmd->resid = 0;
+        req->resp.cmd.resid = 0;
         sense_len = scsi_req_get_sense(r, sense, sizeof(sense));
-        sense_len = MIN(sense_len, req->resp_size - sizeof(req->resp.cmd));
-        memcpy(req->resp.cmd->sense, sense, sense_len);
-        req->resp.cmd->sense_len = tswap32(sense_len);
+        sense_len = MIN(sense_len, req->resp_iov.size - sizeof(req->resp.cmd));
+        qemu_iovec_from_buf(&req->resp_iov, sizeof(req->resp.cmd),
+                            &req->resp, sense_len);
+        req->resp.cmd.sense_len = tswap32(sense_len);
     }
     virtio_scsi_complete_cmd_req(req);
 }
@@ -401,16 +419,16 @@ static void virtio_scsi_request_cancelled(SCSIRequest *r)
         return;
     }
     if (req->dev->resetting) {
-        req->resp.cmd->response = VIRTIO_SCSI_S_RESET;
+        req->resp.cmd.response = VIRTIO_SCSI_S_RESET;
     } else {
-        req->resp.cmd->response = VIRTIO_SCSI_S_ABORTED;
+        req->resp.cmd.response = VIRTIO_SCSI_S_ABORTED;
     }
     virtio_scsi_complete_cmd_req(req);
 }
 
 static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
 {
-    req->resp.cmd->response = VIRTIO_SCSI_S_FAILURE;
+    req->resp.cmd.response = VIRTIO_SCSI_S_FAILURE;
     virtio_scsi_complete_cmd_req(req);
 }
 
@@ -426,37 +444,34 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
     while ((req = virtio_scsi_pop_req(s, vq))) {
         SCSIDevice *d;
         int rc;
-        if (req->elem.out_num > 1 && req->elem.in_num > 1) {
-            virtio_scsi_fail_cmd_req(req);
-            continue;
-        }
 
         rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
                                    sizeof(VirtIOSCSICmdResp) + vs->sense_size);
         if (rc < 0) {
-            virtio_scsi_bad_req();
+            if (rc == -ENOTSUP) {
+                virtio_scsi_fail_cmd_req(req);
+            } else {
+                virtio_scsi_bad_req();
+            }
+            continue;
         }
 
-        d = virtio_scsi_device_find(s, req->req.cmd->lun);
+        d = virtio_scsi_device_find(s, req->req.cmd.lun);
         if (!d) {
-            req->resp.cmd->response = VIRTIO_SCSI_S_BAD_TARGET;
+            req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
             virtio_scsi_complete_cmd_req(req);
             continue;
         }
-        req->sreq = scsi_req_new(d, req->req.cmd->tag,
-                                 virtio_scsi_get_lun(req->req.cmd->lun),
-                                 req->req.cmd->cdb, req);
-
-        if (req->sreq->cmd.mode != SCSI_XFER_NONE) {
-            int req_mode =
-                (req->elem.in_num > 1 ? SCSI_XFER_FROM_DEV : SCSI_XFER_TO_DEV);
-
-            if (req->sreq->cmd.mode != req_mode ||
-                req->sreq->cmd.xfer > req->qsgl.size) {
-                req->resp.cmd->response = VIRTIO_SCSI_S_OVERRUN;
-                virtio_scsi_complete_cmd_req(req);
-                continue;
-            }
+        req->sreq = scsi_req_new(d, req->req.cmd.tag,
+                                 virtio_scsi_get_lun(req->req.cmd.lun),
+                                 req->req.cdb, req);
+
+        if (req->sreq->cmd.mode != SCSI_XFER_NONE
+            && (req->sreq->cmd.mode != req->mode ||
+                req->sreq->cmd.xfer > req->qsgl.size)) {
+            req->resp.cmd.response = VIRTIO_SCSI_S_OVERRUN;
+            virtio_scsi_complete_cmd_req(req);
+            continue;
         }
 
         n = scsi_req_enqueue(req->sreq);
@@ -560,7 +575,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
         return;
     }
 
-    if (req->elem.out_num || req->elem.in_num != 1) {
+    if (req->elem.out_num) {
         virtio_scsi_bad_req();
     }
 
@@ -569,12 +584,12 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
         s->events_dropped = false;
     }
 
-    in_size = req->elem.in_sg[0].iov_len;
+    in_size = iov_size(req->elem.in_sg, req->elem.in_num);
     if (in_size < sizeof(VirtIOSCSIEvent)) {
         virtio_scsi_bad_req();
     }
 
-    evt = req->resp.event;
+    evt = &req->resp.event;
     memset(evt, 0, sizeof(VirtIOSCSIEvent));
     evt->event = event;
     evt->reason = reason;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index fa9d997..ca7a0bd 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -268,6 +268,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_0 \
         {\
+            .driver   = "virtio-scsi-pci",\
+            .property = "any_layout",\
+            .value    = "off",\
+        },{\
             .driver   = "apic",\
             .property = "version",\
             .value    = stringify(0x11),\
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 42b1024..de0615b 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -84,14 +84,13 @@
 #define VIRTIO_SCSI_EVT_RESET_RESCAN           1
 #define VIRTIO_SCSI_EVT_RESET_REMOVED          2
 
-/* SCSI command request, followed by data-out */
+/* SCSI command request, followed by CDB and data-out */
 typedef struct {
     uint8_t lun[8];              /* Logical Unit Number */
     uint64_t tag;                /* Command identifier */
     uint8_t task_attr;           /* Task attribute */
     uint8_t prio;
     uint8_t crn;
-    uint8_t cdb[];
 } QEMU_PACKED VirtIOSCSICmdReq;
 
 /* Response, followed by sense data and data-in */
@@ -101,7 +100,6 @@ typedef struct {
     uint16_t status_qualifier;   /* Status qualifier */
     uint8_t status;              /* Command completion status */
     uint8_t response;            /* Response values */
-    uint8_t sense[];
 } QEMU_PACKED VirtIOSCSICmdResp;
 
 /* Task Management Request */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/7] virtio-scsi: do not rely on iov boundaries

[Michael S. Tsirkin]

On Mon, Jun 16, 2014 at 05:17:43PM +0200, Paolo Bonzini wrote:
> The upcoming virtio 1.0 standard requires virtio devices not to
> rely on iov boundaries when parsing requests or sending responses.
> This series converts virtio-scsi.
> 
> v1->v2: missed a few replacements in patch 6 (Paolo)
> 	never compare in_num/out_num to a non-zero value (Paolo)
> 	do not use in_sg[0]/out_sg[0] (mst)


Applied.
There was a trivial conflict in the last patch, please
verify the pci branch in my tree.

Thanks!

> Paolo Bonzini (7):
>   util: add return value to qemu_iovec_concat_iov
>   virtio-scsi: start preparing for any_layout
>   virtio-scsi: add target swap for VirtIOSCSICtrlTMFReq fields
>   virtio-scsi: add extra argument and return type to qemu_sgl_concat
>   virtio-scsi: prepare sense data handling for any_layout
>   virtio-scsi: introduce virtio_scsi_complete_cmd_req
>   virtio-scsi: add support for the any_layout feature
> 
>  hw/scsi/virtio-scsi.c           | 314 ++++++++++++++++++++++++----------------
>  include/hw/i386/pc.h            |   4 +
>  include/hw/virtio/virtio-scsi.h |   4 +-
>  include/qemu-common.h           |   6 +-
>  util/iov.c                      |  10 +-
>  5 files changed, 202 insertions(+), 136 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/7] virtio-scsi: do not rely on iov boundaries

[Paolo Bonzini]

Il 18/06/2014 18:17, Michael S. Tsirkin ha scritto:
> Applied.
> There was a trivial conflict in the last patch, please
> verify the pci branch in my tree.

Hmm, I have sent it through Peter too. :)

Paolo