[Qemu-devel] [PATCH v2 0/2] dataplane: Enable "scsi=on"

[Qemu-devel] [PATCH v2 0/2] dataplane: Enable "scsi=on"

[Fam Zheng]

This makes the SG_IO code of non-dataplane available to dataplane, so that
dataplane can use to allow scsi=on.

v2:
    [1/2] Fix scsi=off case and drop VirtIOBlockReq.scsi.
    [2/2] Pass conf to virtio_blk_handle_scsi_req.

Fam


Fam Zheng (2):
  virtio-blk: Factor out virtio_blk_handle_scsi_req from
    virtio_blk_handle_scsi
  dataplane: Support VIRTIO_BLK_T_SCSI_CMD

 hw/block/dataplane/virtio-blk.c | 18 +++++----
 hw/block/virtio-blk.c           | 83 +++++++++++++++++++++++------------------
 include/hw/virtio/virtio-blk.h  |  4 ++
 3 files changed, 60 insertions(+), 45 deletions(-)

-- 
1.9.2

[Qemu-devel] [PATCH v2 1/2] virtio-blk: Factor out virtio_blk_handle_scsi_req from virtio_blk_handle_scsi

[Fam Zheng]

The common logic to process a scsi request in a VirtQueueElement is
extracted to a function to share with dataplane.

This makes VirtIOBlockReq.scsi unused, so drop it.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 5e9433d..78e81a6 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -33,7 +33,6 @@ typedef struct VirtIOBlockReq
     VirtQueueElement elem;
     struct virtio_blk_inhdr *in;
     struct virtio_blk_outhdr *out;
-    struct virtio_scsi_inhdr *scsi;
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
@@ -125,13 +124,16 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
     return req;
 }
 
-static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
+int virtio_blk_handle_scsi_req(BlockDriverState *bs,
+                               VirtIOBlkConf *conf,
+                               VirtQueueElement *elem)
 {
+    int status = VIRTIO_BLK_S_OK;
+    struct virtio_scsi_inhdr *scsi = NULL;
 #ifdef __linux__
-    int ret;
     int i;
+    struct sg_io_hdr hdr;
 #endif
-    int status = VIRTIO_BLK_S_OK;
 
     /*
      * We require at least one output segment each for the virtio_blk_outhdr
@@ -140,63 +142,61 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
      * We also at least require the virtio_blk_inhdr, the virtio_scsi_inhdr
      * and the sense buffer pointer in the input segments.
      */
-    if (req->elem.out_num < 2 || req->elem.in_num < 3) {
-        virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
-        g_free(req);
-        return;
+    if (elem->out_num < 2 || elem->in_num < 3) {
+        status = VIRTIO_BLK_S_IOERR;
+        goto fail;
     }
 
     /*
      * The scsi inhdr is placed in the second-to-last input segment, just
      * before the regular inhdr.
      */
-    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
+    scsi = (void *)elem->in_sg[elem->in_num - 2].iov_base;
 
-    if (!req->dev->blk.scsi) {
+    /*
+     * No support for bidirection commands yet.
+     */
+    if (elem->out_num > 2 && elem->in_num > 3) {
         status = VIRTIO_BLK_S_UNSUPP;
         goto fail;
     }
 
-    /*
-     * No support for bidirection commands yet.
-     */
-    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
+    if (!conf->scsi) {
         status = VIRTIO_BLK_S_UNSUPP;
         goto fail;
     }
 
 #ifdef __linux__
-    struct sg_io_hdr hdr;
     memset(&hdr, 0, sizeof(struct sg_io_hdr));
     hdr.interface_id = 'S';
-    hdr.cmd_len = req->elem.out_sg[1].iov_len;
-    hdr.cmdp = req->elem.out_sg[1].iov_base;
+    hdr.cmd_len = elem->out_sg[1].iov_len;
+    hdr.cmdp = elem->out_sg[1].iov_base;
     hdr.dxfer_len = 0;
 
-    if (req->elem.out_num > 2) {
+    if (elem->out_num > 2) {
         /*
          * If there are more than the minimally required 2 output segments
          * there is write payload starting from the third iovec.
          */
         hdr.dxfer_direction = SG_DXFER_TO_DEV;
-        hdr.iovec_count = req->elem.out_num - 2;
+        hdr.iovec_count = elem->out_num - 2;
 
         for (i = 0; i < hdr.iovec_count; i++)
-            hdr.dxfer_len += req->elem.out_sg[i + 2].iov_len;
+            hdr.dxfer_len += elem->out_sg[i + 2].iov_len;
 
-        hdr.dxferp = req->elem.out_sg + 2;
+        hdr.dxferp = elem->out_sg + 2;
 
-    } else if (req->elem.in_num > 3) {
+    } else if (elem->in_num > 3) {
         /*
          * If we have more than 3 input segments the guest wants to actually
          * read data.
          */
         hdr.dxfer_direction = SG_DXFER_FROM_DEV;
-        hdr.iovec_count = req->elem.in_num - 3;
+        hdr.iovec_count = elem->in_num - 3;
         for (i = 0; i < hdr.iovec_count; i++)
-            hdr.dxfer_len += req->elem.in_sg[i].iov_len;
+            hdr.dxfer_len += elem->in_sg[i].iov_len;
 
-        hdr.dxferp = req->elem.in_sg;
+        hdr.dxferp = elem->in_sg;
     } else {
         /*
          * Some SCSI commands don't actually transfer any data.
@@ -204,11 +204,11 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
         hdr.dxfer_direction = SG_DXFER_NONE;
     }
 
-    hdr.sbp = req->elem.in_sg[req->elem.in_num - 3].iov_base;
-    hdr.mx_sb_len = req->elem.in_sg[req->elem.in_num - 3].iov_len;
+    hdr.sbp = elem->in_sg[elem->in_num - 3].iov_base;
+    hdr.mx_sb_len = elem->in_sg[elem->in_num - 3].iov_len;
 
-    ret = bdrv_ioctl(req->dev->bs, SG_IO, &hdr);
-    if (ret) {
+    status = bdrv_ioctl(bs, SG_IO, &hdr);
+    if (status) {
         status = VIRTIO_BLK_S_UNSUPP;
         goto fail;
     }
@@ -224,23 +224,32 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
         hdr.status = CHECK_CONDITION;
     }
 
-    stl_p(&req->scsi->errors,
+    stl_p(&scsi->errors,
           hdr.status | (hdr.msg_status << 8) |
           (hdr.host_status << 16) | (hdr.driver_status << 24));
-    stl_p(&req->scsi->residual, hdr.resid);
-    stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
-    stl_p(&req->scsi->data_len, hdr.dxfer_len);
+    stl_p(&scsi->residual, hdr.resid);
+    stl_p(&scsi->sense_len, hdr.sb_len_wr);
+    stl_p(&scsi->data_len, hdr.dxfer_len);
 
-    virtio_blk_req_complete(req, status);
-    g_free(req);
-    return;
+    return status;
 #else
     abort();
 #endif
 
 fail:
     /* Just put anything nonzero so that the ioctl fails in the guest.  */
-    stl_p(&req->scsi->errors, 255);
+    if (scsi) {
+        stl_p(&scsi->errors, 255);
+    }
+    return status;
+}
+
+static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
+{
+    int status;
+
+    status = virtio_blk_handle_scsi_req(req->dev->bs, &req->dev->blk,
+                                        &req->elem);
     virtio_blk_req_complete(req, status);
     g_free(req);
 }
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index e4c41ff..1b00b48 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -155,4 +155,8 @@ typedef struct VirtIOBlock {
 
 void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
 
+int virtio_blk_handle_scsi_req(BlockDriverState *bs,
+                               VirtIOBlkConf *conf,
+                               VirtQueueElement *elem);
+
 #endif
-- 
1.9.2

Re: [Qemu-devel] [PATCH v2 1/2] virtio-blk: Factor out virtio_blk_handle_scsi_req from virtio_blk_handle_scsi

[Paolo Bonzini]

Il 22/05/2014 09:37, Fam Zheng ha scritto:
> -static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
> +int virtio_blk_handle_scsi_req(BlockDriverState *bs,
> +                               VirtIOBlkConf *conf,
> +                               VirtQueueElement *elem)

Two more comments...

please pass the VirtIOBlock * instead of bs/conf here.

>  {
> +    int status = VIRTIO_BLK_S_OK;
> +    struct virtio_scsi_inhdr *scsi = NULL;
>  #ifdef __linux__
> -    int ret;
>      int i;
> +    struct sg_io_hdr hdr;
>  #endif
> -    int status = VIRTIO_BLK_S_OK;
>
>      /*
>       * We require at least one output segment each for the virtio_blk_outhdr
> @@ -140,63 +142,61 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>       * We also at least require the virtio_blk_inhdr, the virtio_scsi_inhdr
>       * and the sense buffer pointer in the input segments.
>       */
> -    if (req->elem.out_num < 2 || req->elem.in_num < 3) {
> -        virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> -        g_free(req);
> -        return;
> +    if (elem->out_num < 2 || elem->in_num < 3) {
> +        status = VIRTIO_BLK_S_IOERR;
> +        goto fail;
>      }
>
>      /*
>       * The scsi inhdr is placed in the second-to-last input segment, just
>       * before the regular inhdr.
>       */
> -    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
> +    scsi = (void *)elem->in_sg[elem->in_num - 2].iov_base;
>
> -    if (!req->dev->blk.scsi) {
> +    /*
> +     * No support for bidirection commands yet.
> +     */
> +    if (elem->out_num > 2 && elem->in_num > 3) {
>          status = VIRTIO_BLK_S_UNSUPP;
>          goto fail;
>      }
>
> -    /*
> -     * No support for bidirection commands yet.
> -     */
> -    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
> +    if (!conf->scsi) {
>          status = VIRTIO_BLK_S_UNSUPP;
>          goto fail;
>      }

Swapping the two conditionals unnecessarily makes the patch bigger.

Paolo

>  #ifdef __linux__
> -    struct sg_io_hdr hdr;
>      memset(&hdr, 0, sizeof(struct sg_io_hdr));
>      hdr.interface_id = 'S';
> -    hdr.cmd_len = req->elem.out_sg[1].iov_len;
> -    hdr.cmdp = req->elem.out_sg[1].iov_base;
> +    hdr.cmd_len = elem->out_sg[1].iov_len;
> +    hdr.cmdp = elem->out_sg[1].iov_base;
>      hdr.dxfer_len = 0;
>
> -    if (req->elem.out_num > 2) {
> +    if (elem->out_num > 2) {
>          /*
>           * If there are more than the minimally required 2 output segments
>           * there is write payload starting from the third iovec.
>           */
>          hdr.dxfer_direction = SG_DXFER_TO_DEV;
> -        hdr.iovec_count = req->elem.out_num - 2;
> +        hdr.iovec_count = elem->out_num - 2;
>
>          for (i = 0; i < hdr.iovec_count; i++)
> -            hdr.dxfer_len += req->elem.out_sg[i + 2].iov_len;
> +            hdr.dxfer_len += elem->out_sg[i + 2].iov_len;
>
> -        hdr.dxferp = req->elem.out_sg + 2;
> +        hdr.dxferp = elem->out_sg + 2;
>
> -    } else if (req->elem.in_num > 3) {
> +    } else if (elem->in_num > 3) {
>          /*
>           * If we have more than 3 input segments the guest wants to actually
>           * read data.
>           */
>          hdr.dxfer_direction = SG_DXFER_FROM_DEV;
> -        hdr.iovec_count = req->elem.in_num - 3;
> +        hdr.iovec_count = elem->in_num - 3;
>          for (i = 0; i < hdr.iovec_count; i++)
> -            hdr.dxfer_len += req->elem.in_sg[i].iov_len;
> +            hdr.dxfer_len += elem->in_sg[i].iov_len;
>
> -        hdr.dxferp = req->elem.in_sg;
> +        hdr.dxferp = elem->in_sg;
>      } else {
>          /*
>           * Some SCSI commands don't actually transfer any data.
> @@ -204,11 +204,11 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>          hdr.dxfer_direction = SG_DXFER_NONE;
>      }
>
> -    hdr.sbp = req->elem.in_sg[req->elem.in_num - 3].iov_base;
> -    hdr.mx_sb_len = req->elem.in_sg[req->elem.in_num - 3].iov_len;
> +    hdr.sbp = elem->in_sg[elem->in_num - 3].iov_base;
> +    hdr.mx_sb_len = elem->in_sg[elem->in_num - 3].iov_len;
>
> -    ret = bdrv_ioctl(req->dev->bs, SG_IO, &hdr);
> -    if (ret) {
> +    status = bdrv_ioctl(bs, SG_IO, &hdr);
> +    if (status) {
>          status = VIRTIO_BLK_S_UNSUPP;
>          goto fail;
>      }
> @@ -224,23 +224,32 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>          hdr.status = CHECK_CONDITION;
>      }
>
> -    stl_p(&req->scsi->errors,
> +    stl_p(&scsi->errors,
>            hdr.status | (hdr.msg_status << 8) |
>            (hdr.host_status << 16) | (hdr.driver_status << 24));
> -    stl_p(&req->scsi->residual, hdr.resid);
> -    stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
> -    stl_p(&req->scsi->data_len, hdr.dxfer_len);
> +    stl_p(&scsi->residual, hdr.resid);
> +    stl_p(&scsi->sense_len, hdr.sb_len_wr);
> +    stl_p(&scsi->data_len, hdr.dxfer_len);
>
> -    virtio_blk_req_complete(req, status);
> -    g_free(req);
> -    return;
> +    return status;
>  #else
>      abort();
>  #endif
>
>  fail:
>      /* Just put anything nonzero so that the ioctl fails in the guest.  */
> -    stl_p(&req->scsi->errors, 255);
> +    if (scsi) {
> +        stl_p(&scsi->errors, 255);
> +    }
> +    return status;
> +}
> +
> +static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
> +{
> +    int status;
> +
> +    status = virtio_blk_handle_scsi_req(req->dev->bs, &req->dev->blk,
> +                                        &req->elem);
>      virtio_blk_req_complete(req, status);
>      g_free(req);
>  }
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index e4c41ff..1b00b48 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -155,4 +155,8 @@ typedef struct VirtIOBlock {
>
>  void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
>
> +int virtio_blk_handle_scsi_req(BlockDriverState *bs,
> +                               VirtIOBlkConf *conf,
> +                               VirtQueueElement *elem);
> +
>  #endif
>

[Qemu-devel] [PATCH v2 2/2] dataplane: Support VIRTIO_BLK_T_SCSI_CMD

[Fam Zheng]

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

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 46a6824..e833045 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -201,6 +201,15 @@ static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
     bdrv_aio_flush(s->blk->conf.bs, complete_flush, req);
 }
 
+static void do_scsi_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
+                        QEMUIOVector *inhdr)
+{
+    int status;
+
+    status = virtio_blk_handle_scsi_req(s->blk->conf.bs, s->blk, elem);
+    complete_request_early(s, elem, inhdr, status);
+}
+
 static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
 {
     struct iovec *iov = elem->out_sg;
@@ -249,8 +258,7 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
         return 0;
 
     case VIRTIO_BLK_T_SCSI_CMD:
-        /* TODO support SCSI commands */
-        complete_request_early(s, elem, inhdr, VIRTIO_BLK_S_UNSUPP);
+        do_scsi_cmd(s, elem, inhdr);
         return 0;
 
     case VIRTIO_BLK_T_FLUSH:
@@ -326,12 +334,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
         return;
     }
 
-    if (blk->scsi) {
-        error_setg(errp,
-                   "device is incompatible with x-data-plane, use scsi=off");
-        return;
-    }
-
     /* If dataplane is (re-)enabled while the guest is running there could be
      * block jobs that can conflict.
      */
-- 
1.9.2

Re: [Qemu-devel] [PATCH v2 0/2] dataplane: Enable "scsi=on"

[Stefan Hajnoczi]

On Thu, May 22, 2014 at 03:37:10PM +0800, Fam Zheng wrote:
> This makes the SG_IO code of non-dataplane available to dataplane, so that
> dataplane can use to allow scsi=on.
> 
> v2:
>     [1/2] Fix scsi=off case and drop VirtIOBlockReq.scsi.
>     [2/2] Pass conf to virtio_blk_handle_scsi_req.
> 
> Fam
> 
> 
> Fam Zheng (2):
>   virtio-blk: Factor out virtio_blk_handle_scsi_req from
>     virtio_blk_handle_scsi
>   dataplane: Support VIRTIO_BLK_T_SCSI_CMD
> 
>  hw/block/dataplane/virtio-blk.c | 18 +++++----
>  hw/block/virtio-blk.c           | 83 +++++++++++++++++++++++------------------
>  include/hw/virtio/virtio-blk.h  |  4 ++
>  3 files changed, 60 insertions(+), 45 deletions(-)

I wonder if it's okay to use synchronous bdrv_ioctl() but the existing
code already does it.  It would be bad to block the thread.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>