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 <pbonz...@redhat.com>
>> ---
>> 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();
}
