From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53853) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVdFo-00057p-Au for qemu-devel@nongnu.org; Wed, 11 Mar 2015 05:48:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVdFi-0005Xp-Ui for qemu-devel@nongnu.org; Wed, 11 Mar 2015 05:48:40 -0400 Received: from mail-wi0-x22c.google.com ([2a00:1450:400c:c05::22c]:35517) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVdFi-0005XI-KG for qemu-devel@nongnu.org; Wed, 11 Mar 2015 05:48:34 -0400 Received: by wibbs8 with SMTP id bs8so37384923wib.0 for ; Wed, 11 Mar 2015 02:48:33 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <55000F6B.1020808@redhat.com> Date: Wed, 11 Mar 2015 10:48:27 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1424295164-4774-1-git-send-email-mst@redhat.com> <1424295164-4774-41-git-send-email-mst@redhat.com> <20150311071416.GI1437@ad.nay.redhat.com> <20150311090446-mutt-send-email-mst@redhat.com> In-Reply-To: <20150311090446-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 40/96] virtio-scsi: use standard-headers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Fam Zheng Cc: Peter Maydell , Anthony Liguori , qemu-devel@nongnu.org, dgilbert@redhat.com, Juan Quintela On 11/03/2015 09:31, Michael S. Tsirkin wrote: > On Wed, Mar 11, 2015 at 03:14:16PM +0800, Fam Zheng wrote: >> On Wed, 02/18 22:48, Michael S. Tsirkin wrote: >>> Drop duplicated code. >>> >>> Signed-off-by: Michael S. Tsirkin >>> Acked-by: Paolo Bonzini >> >> Michael, this breaks virtio-scsi-pci: >> >> qemu-system-x86_64: wrong size for virtio-scsi headers > > Testing this now. > > 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)); No, this is too ugly. Can I just queue a revert? Paolo > #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 || > >