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

[Paolo Bonzini]

Il 12/06/2014 14:33, Michael S. Tsirkin ha scritto:
> On Thu, Jun 12, 2014 at 02:09:08PM +0200, Paolo Bonzini wrote:
>> 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 <address@hidden>
>> ---
>>  hw/scsi/virtio-scsi.c           | 161 
>> ++++++++++++++++++++++------------------
>>  include/hw/i386/pc.h            |   4 +
>>  include/hw/virtio/virtio-scsi.h |   4 +-
>>  3 files changed, 93 insertions(+), 76 deletions(-)
>>
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 107e9cb..391717d 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -27,21 +27,26 @@ typedef struct VirtIOSCSIReq {
>>      QEMUSGList qsgl;
>>      SCSIRequest *sreq;
>>      size_t resp_size;
>> +    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 +66,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_malloc(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 +90,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 +133,29 @@ 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[0], req->elem.out_num, 0,
>> +                   &req->req, req_size) < req_size) {
>>          return -EINVAL;
>>      }
> 
> I'd like to suggest converting &req->elem.out_sg[0] to
> plain req->elem.out_sg. We can then easily greap for
> in_sg[X] out_sg[X] and take that as a sign that
> any_layout rules are violated.

Very good idea, and it found a place where I was using iov_len instead 
of the iov_size() function.

I also noticed that I'm using in_num > 1 to check for read vs. write 
commands.  All fixed like this:

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index ddfe5c7..73626fa 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -27,6 +27,7 @@ typedef struct VirtIOSCSIReq {
     QEMUSGList qsgl;
     SCSIRequest *sreq;
     size_t resp_size;
+    enum SCSIXferMode mode;
     QEMUIOVector resp_iov;
     union {
         VirtIOSCSICmdResp     cmd;
         VirtIOSCSICtrlTMFResp tmf;
@@ -158,6 +162,12 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
         return -ENOTSUP;
     }
 
+    if (out_size) {
+        req->mode = SCSI_XFER_TO_DEV;
+    } else if (in_size) {
+        req->mode = SCSI_XFER_FROM_DEV;
+    }
+
     return 0;
 }
 
@@ -213,10 +223,7 @@ 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;
 }
@@ -463,15 +470,11 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, 
VirtQueue *vq)
                                  req->req.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.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->resp.cmd.response = VIRTIO_SCSI_S_OVERRUN;
+            virtio_scsi_complete_cmd_req(req);
+            continue;
         }
 
         n = scsi_req_enqueue(req->sreq);
@@ -575,7 +578,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();
     }
 
@@ -584,7 +587,7 @@ 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();
     }