From: "Michael S. Tsirkin" <mst@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: famz@redhat.com, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 7/7] virtio-scsi: add support for the any_layout feature
Date: Thu, 12 Jun 2014 15:33:26 +0300 [thread overview]
Message-ID: <20140612123326.GA23239@redhat.com> (raw)
In-Reply-To: <1402574948-11203-8-git-send-email-pbonzini@redhat.com>
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 <pbonzini@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.
> - if (req->elem.out_num) {
> - req->req.buf = req->elem.out_sg[0].iov_base;
> - }
>
> - if (req->elem.in_sg[0].iov_len < resp_size) {
> + if (qemu_iovec_concat_iov(&req->resp_iov,
> + &req->elem.in_sg[0], req->elem.in_num, 0,
> + resp_size) < resp_size) {
> return -EINVAL;
> }
> - req->resp.buf = req->elem.in_sg[0].iov_base;
> req->resp_size = resp_size;
>
> - if (req->elem.out_num > 1) {
> - qemu_sgl_concat(req, &req->elem.out_sg[1],
> - &req->elem.out_addr[1],
> - req->elem.out_num - 1, 0);
> - } else {
> - qemu_sgl_concat(req, &req->elem.in_sg[1],
> - &req->elem.in_addr[1],
> - req->elem.in_num - 1, 0);
> + out_size = qemu_sgl_concat(req, &req->elem.out_sg[0],
> + &req->elem.out_addr[0], req->elem.out_num,
> + req_size);
> + in_size = qemu_sgl_concat(req, &req->elem.in_sg[0],
> + &req->elem.in_addr[0], req->elem.in_num,
> + resp_size);
> +
> + if (out_size && in_size) {
> + return -ENOTSUP;
> }
>
> return 0;
> @@ -214,27 +223,27 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
>
> static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
> {
> - SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf->lun);
> + SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
> SCSIRequest *r, *next;
> BusChild *kid;
> int target;
>
> /* Here VIRTIO_SCSI_S_OK means "FUNCTION COMPLETE". */
> - req->resp.tmf->response = VIRTIO_SCSI_S_OK;
> + req->resp.tmf.response = VIRTIO_SCSI_S_OK;
>
> - tswap32s(&req->req.tmf->subtype);
> - switch (req->req.tmf->subtype) {
> + tswap32s(&req->req.tmf.subtype);
> + switch (req->req.tmf.subtype) {
> case VIRTIO_SCSI_T_TMF_ABORT_TASK:
> case VIRTIO_SCSI_T_TMF_QUERY_TASK:
> if (!d) {
> goto fail;
> }
> - if (d->lun != virtio_scsi_get_lun(req->req.tmf->lun)) {
> + if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
> goto incorrect_lun;
> }
> QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
> VirtIOSCSIReq *cmd_req = r->hba_private;
> - if (cmd_req && cmd_req->req.cmd->tag == req->req.tmf->tag) {
> + if (cmd_req && cmd_req->req.cmd.tag == req->req.tmf.tag) {
> break;
> }
> }
> @@ -244,11 +253,11 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
> * check for it in the loop above.
> */
> assert(r->hba_private);
> - if (req->req.tmf->subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK) {
> + if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK) {
> /* "If the specified command is present in the task set, then
> * return a service response set to FUNCTION SUCCEEDED".
> */
> - req->resp.tmf->response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
> + req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
> } else {
> scsi_req_cancel(r);
> }
> @@ -259,7 +268,7 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
> if (!d) {
> goto fail;
> }
> - if (d->lun != virtio_scsi_get_lun(req->req.tmf->lun)) {
> + if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
> goto incorrect_lun;
> }
> s->resetting++;
> @@ -273,16 +282,16 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
> if (!d) {
> goto fail;
> }
> - if (d->lun != virtio_scsi_get_lun(req->req.tmf->lun)) {
> + if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
> goto incorrect_lun;
> }
> QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
> if (r->hba_private) {
> - if (req->req.tmf->subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
> + if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
> /* "If there is any command present in the task set, then
> * return a service response set to FUNCTION SUCCEEDED".
> */
> - req->resp.tmf->response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
> + req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
> break;
> } else {
> scsi_req_cancel(r);
> @@ -292,7 +301,7 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
> break;
>
> case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
> - target = req->req.tmf->lun[1];
> + target = req->req.tmf.lun[1];
> s->resetting++;
> QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
> d = DO_UPCAST(SCSIDevice, qdev, kid->child);
> @@ -305,18 +314,18 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
>
> case VIRTIO_SCSI_T_TMF_CLEAR_ACA:
> default:
> - req->resp.tmf->response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
> + req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_REJECTED;
> break;
> }
>
> return;
>
> incorrect_lun:
> - req->resp.tmf->response = VIRTIO_SCSI_S_INCORRECT_LUN;
> + req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
> return;
>
> fail:
> - req->resp.tmf->response = VIRTIO_SCSI_S_BAD_TARGET;
> + req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
> }
>
> static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> @@ -333,8 +342,8 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> continue;
> }
>
> - tswap32s(&req->req.tmf->type);
> - if (req->req.tmf->type == VIRTIO_SCSI_T_TMF) {
> + tswap32s(&req->req.tmf.type);
> + if (req->req.tmf.type == VIRTIO_SCSI_T_TMF) {
> if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlTMFReq),
> sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
> virtio_scsi_bad_req();
> @@ -342,14 +351,14 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> virtio_scsi_do_tmf(s, req);
> }
>
> - } else if (req->req.tmf->type == VIRTIO_SCSI_T_AN_QUERY ||
> - req->req.tmf->type == VIRTIO_SCSI_T_AN_SUBSCRIBE) {
> + } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
> + req->req.tmf.type == VIRTIO_SCSI_T_AN_SUBSCRIBE) {
> if (virtio_scsi_parse_req(req, sizeof(VirtIOSCSICtrlANReq),
> sizeof(VirtIOSCSICtrlANResp)) < 0) {
> virtio_scsi_bad_req();
> } else {
> - req->resp.an->event_actual = 0;
> - req->resp.an->response = VIRTIO_SCSI_S_OK;
> + req->resp.an.event_actual = 0;
> + req->resp.an.response = VIRTIO_SCSI_S_OK;
> }
> }
> virtio_scsi_complete_req(req);
> @@ -358,6 +367,10 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>
> 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);
> virtio_scsi_complete_req(req);
> }
>
> @@ -372,16 +385,17 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
> return;
> }
>
> - req->resp.cmd->response = VIRTIO_SCSI_S_OK;
> - req->resp.cmd->status = status;
> - if (req->resp.cmd->status == GOOD) {
> - req->resp.cmd->resid = tswap32(resid);
> + req->resp.cmd.response = VIRTIO_SCSI_S_OK;
> + req->resp.cmd.status = status;
> + if (req->resp.cmd.status == GOOD) {
> + req->resp.cmd.resid = tswap32(resid);
> } else {
> - req->resp.cmd->resid = 0;
> + req->resp.cmd.resid = 0;
> sense_len = scsi_req_get_sense(r, sense, sizeof(sense));
> - sense_len = MIN(sense_len, req->resp_size - sizeof(req->resp.cmd));
> - memcpy(req->resp.cmd->sense, sense, sense_len);
> - req->resp.cmd->sense_len = tswap32(sense_len);
> + sense_len = MIN(sense_len, req->resp_iov.size - sizeof(req->resp.cmd));
> + qemu_iovec_from_buf(&req->resp_iov, sizeof(req->resp.cmd),
> + &req->resp, sense_len);
> + req->resp.cmd.sense_len = tswap32(sense_len);
> }
> virtio_scsi_complete_cmd_req(req);
> }
> @@ -401,16 +415,16 @@ static void virtio_scsi_request_cancelled(SCSIRequest *r)
> return;
> }
> if (req->dev->resetting) {
> - req->resp.cmd->response = VIRTIO_SCSI_S_RESET;
> + req->resp.cmd.response = VIRTIO_SCSI_S_RESET;
> } else {
> - req->resp.cmd->response = VIRTIO_SCSI_S_ABORTED;
> + req->resp.cmd.response = VIRTIO_SCSI_S_ABORTED;
> }
> virtio_scsi_complete_req(req);
> }
>
> static void virtio_scsi_fail_cmd_req(VirtIOSCSIReq *req)
> {
> - req->resp.cmd->response = VIRTIO_SCSI_S_FAILURE;
> + req->resp.cmd.response = VIRTIO_SCSI_S_FAILURE;
> virtio_scsi_complete_req(req);
> }
>
> @@ -426,26 +440,27 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
> while ((req = virtio_scsi_pop_req(s, vq))) {
> SCSIDevice *d;
> int rc;
> - if (req->elem.out_num > 1 && req->elem.in_num > 1) {
> - virtio_scsi_fail_cmd_req(req);
> - continue;
> - }
>
> rc = virtio_scsi_parse_req(req, sizeof(VirtIOSCSICmdReq) + vs->cdb_size,
> sizeof(VirtIOSCSICmdResp) + vs->sense_size);
> if (rc < 0) {
> - virtio_scsi_bad_req();
> + if (rc == -ENOTSUP) {
> + virtio_scsi_fail_cmd_req(req);
> + } else {
> + virtio_scsi_bad_req();
> + }
> + continue;
> }
>
> - d = virtio_scsi_device_find(s, req->req.cmd->lun);
> + d = virtio_scsi_device_find(s, req->req.cmd.lun);
> if (!d) {
> - req->resp.cmd->response = VIRTIO_SCSI_S_BAD_TARGET;
> + req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
> virtio_scsi_complete_cmd_req(req);
> continue;
> }
> - req->sreq = scsi_req_new(d, req->req.cmd->tag,
> - virtio_scsi_get_lun(req->req.cmd->lun),
> - req->req.cmd->cdb, req);
> + req->sreq = scsi_req_new(d, req->req.cmd.tag,
> + virtio_scsi_get_lun(req->req.cmd.lun),
> + req->req.cdb, req);
>
> if (req->sreq->cmd.mode != SCSI_XFER_NONE) {
> int req_mode =
> @@ -453,7 +468,7 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
>
> if (req->sreq->cmd.mode != req_mode ||
> req->sreq->cmd.xfer > req->qsgl.size) {
> - req->resp.cmd->response = VIRTIO_SCSI_S_OVERRUN;
> + req->resp.cmd.response = VIRTIO_SCSI_S_OVERRUN;
> virtio_scsi_complete_cmd_req(req);
> continue;
> }
> @@ -574,7 +589,7 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
> virtio_scsi_bad_req();
> }
>
> - evt = req->resp.event;
> + evt = &req->resp.event;
> memset(evt, 0, sizeof(VirtIOSCSIEvent));
> evt->event = event;
> evt->reason = reason;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index fa9d997..ca7a0bd 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -268,6 +268,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>
> #define PC_COMPAT_2_0 \
> {\
> + .driver = "virtio-scsi-pci",\
> + .property = "any_layout",\
> + .value = "off",\
> + },{\
> .driver = "apic",\
> .property = "version",\
> .value = stringify(0x11),\
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 42b1024..de0615b 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -84,14 +84,13 @@
> #define VIRTIO_SCSI_EVT_RESET_RESCAN 1
> #define VIRTIO_SCSI_EVT_RESET_REMOVED 2
>
> -/* SCSI command request, followed by data-out */
> +/* SCSI command request, followed by CDB and data-out */
> typedef struct {
> uint8_t lun[8]; /* Logical Unit Number */
> uint64_t tag; /* Command identifier */
> uint8_t task_attr; /* Task attribute */
> uint8_t prio;
> uint8_t crn;
> - uint8_t cdb[];
> } QEMU_PACKED VirtIOSCSICmdReq;
>
> /* Response, followed by sense data and data-in */
> @@ -101,7 +100,6 @@ typedef struct {
> uint16_t status_qualifier; /* Status qualifier */
> uint8_t status; /* Command completion status */
> uint8_t response; /* Response values */
> - uint8_t sense[];
> } QEMU_PACKED VirtIOSCSICmdResp;
>
> /* Task Management Request */
> --
> 1.9.3
next prev parent reply other threads:[~2014-06-12 12:33 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-12 12:09 [Qemu-devel] [PATCH 0/7] virtio-scsi: do not rely on iov boundaries Paolo Bonzini
2014-06-12 12:09 ` [Qemu-devel] [PATCH 1/7] util: add return value to qemu_iovec_concat_iov Paolo Bonzini
2014-06-12 12:09 ` [Qemu-devel] [PATCH 2/7] virtio-scsi: start preparing for any_layout Paolo Bonzini
2014-06-12 12:09 ` [Qemu-devel] [PATCH 3/7] virtio-scsi: add target swap for VirtIOSCSICtrlTMFReq fields Paolo Bonzini
2014-06-12 12:09 ` [Qemu-devel] [PATCH 4/7] virtio-scsi: add extra argument and return type to qemu_sgl_concat Paolo Bonzini
2014-06-12 12:09 ` [Qemu-devel] [PATCH 5/7] virtio-scsi: prepare sense data handling for any_layout Paolo Bonzini
2014-06-12 12:09 ` [Qemu-devel] [PATCH 6/7] virtio-scsi: introduce virtio_scsi_complete_cmd_req Paolo Bonzini
2014-06-12 12:09 ` [Qemu-devel] [PATCH 7/7] virtio-scsi: add support for the any_layout feature Paolo Bonzini
2014-06-12 12:33 ` Michael S. Tsirkin [this message]
2014-06-12 13:52 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140612123326.GA23239@redhat.com \
--to=mst@redhat.com \
--cc=famz@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.