Re: [Qemu-devel] [PATCH] virtio-scsi: fix cdb/sense

[Alexey Kardashevskiy]

On 03/11/2015 08:18 PM, Michael S. Tsirkin wrote:

Commit "virtio-scsi: use standard-headers" added
cdb and sense into req/rep structures, which
breaks uses of sizeof for these structures,
since qemu adds its own arrays on top.

To fix, replace sizeof with offsetof everywhere.

This fixes breakage in SLOF on PPC64.

Reported-by: Fam Zheng <f...@redhat.com>
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---

Note: this is very lightly tested.
Help with testing will be very much appreciated!

  include/hw/virtio/virtio-scsi.h |  6 +++---
  hw/scsi/virtio-scsi.c           | 28 ++++++++++++++++++----------
  2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index de2c739..25d96f3 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -136,15 +136,15 @@ typedef struct VirtIOSCSIReq {
      union {
          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));
+QEMU_BUILD_BUG_ON(offsetof(VirtIOSCSIReq, req.cmd.cdb) !=
+                  offsetof(VirtIOSCSIReq, req.cmd) +
+                  offsetof(VirtIOSCSICmdReq, cdb));

  #define DEFINE_VIRTIO_SCSI_PROPERTIES(_state, _conf_field)                    
 \
      DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1),      
 \
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index cfb52e8..52bc00c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -47,7 +47,8 @@ VirtIOSCSIReq *virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue 
*vq)
      const size_t zero_skip = offsetof(VirtIOSCSIReq, elem)
                               + sizeof(VirtQueueElement);

-    req = g_slice_alloc(sizeof(*req) + vs->cdb_size);
+    req = g_slice_alloc(sizeof(*req) + MAX(vs->cdb_size, VIRTIO_SCSI_CDB_SIZE) 
-
+                        VIRTIO_SCSI_CDB_SIZE);
      req->vq = vq;
      req->dev = s;
      qemu_sglist_init(&req->qsgl, DEVICE(s), 8, &address_space_memory);
@@ -62,7 +63,8 @@ void virtio_scsi_free_req(VirtIOSCSIReq *req)

      qemu_iovec_destroy(&req->resp_iov);
      qemu_sglist_destroy(&req->qsgl);
-    g_slice_free1(sizeof(*req) + vs->cdb_size, req);
+    g_slice_free1(sizeof(*req) + MAX(vs->cdb_size, VIRTIO_SCSI_CDB_SIZE) -
+                        VIRTIO_SCSI_CDB_SIZE, req);
  }

  static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
@@ -213,8 +215,10 @@ static void *virtio_scsi_load_request(QEMUFile *f, 
SCSIRequest *sreq)
      assert(req->elem.in_num <= ARRAY_SIZE(req->elem.in_sg));
      assert(req->elem.out_num <= ARRAY_SIZE(req->elem.out_sg));

-    if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
-                              sizeof(VirtIOSCSICmdResp) + vs->sense_size) < 0) 
{
+    if (virtio_scsi_parse_req(req, offsetof(VirtIOSCSICmdReq, cdb) +
+                              vs->cdb_size,
+                              offsetof(VirtIOSCSICmdResp, sense) +
+                              vs->sense_size) < 0) {
          error_report("invalid SCSI request migration data");
          exit(1);
      }
@@ -439,7 +443,7 @@ 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);
+    req->resp_size = offsetof(VirtIOSCSICmdResp, sense);
      virtio_scsi_complete_req(req);
  }

@@ -462,8 +466,10 @@ static void virtio_scsi_command_complete(SCSIRequest *r, 
uint32_t status,
      } else {
          req->resp.cmd.resid = 0;
          sense_len = scsi_req_get_sense(r, sense, sizeof(sense));
-        sense_len = MIN(sense_len, req->resp_iov.size - sizeof(req->resp.cmd));
-        qemu_iovec_from_buf(&req->resp_iov, sizeof(req->resp.cmd),
+        sense_len = MIN(sense_len, req->resp_iov.size -
+                        offsetof(typeof(req->resp.cmd), sense));
+        qemu_iovec_from_buf(&req->resp_iov,
+                            offsetof(typeof(req->resp.cmd), sense),
                              sense, sense_len);
          req->resp.cmd.sense_len = virtio_tswap32(vdev, sense_len);
      }
@@ -522,8 +528,10 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, 
VirtIOSCSIReq *req)
      SCSIDevice *d;
      int rc;

-    rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
-                               sizeof(VirtIOSCSICmdResp) + vs->sense_size);
+    rc = virtio_scsi_parse_req(req, offsetof(VirtIOSCSICmdReq, cdb) +
+                               vs->cdb_size,
+                               offsetof(VirtIOSCSICmdResp, sense) +
+                               vs->sense_size);
      if (rc < 0) {
          if (rc == -ENOTSUP) {
              virtio_scsi_fail_cmd_req(req);
@@ -544,7 +552,7 @@ bool virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, 
VirtIOSCSIReq *req)
      }
      req->sreq = scsi_req_new(d, req->req.cmd.tag,
                               virtio_scsi_get_lun(req->req.cmd.lun),
-                             req->req.cdb, req);
+                             req->req.cmd.cdb, req);

      if (req->sreq->cmd.mode != SCSI_XFER_NONE
          && (req->sreq->cmd.mode != req->mode ||

--
Alexey