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 <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>
> 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