| Message ID | 1422945003-24538-6-git-send-email-nab@daterainc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 03, 2015 at 06:29:59AM +0000, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger <nab@linux-iscsi.org> > > This patch adds ANY_LAYOUT support with a new vqs[].vq.handle_kick() > callback in vhost_scsi_handle_vqal(). > > It calculates data_direction + exp_data_len for the new tcm_vhost_cmd > descriptor by walking both outgoing + incoming iovecs using iov_iter, > assuming the layout of outgoing request header + T10_PI + Data payload > comes first. > > It also uses copy_from_iter() to copy leading virtio-scsi request header > that may or may not include SCSI CDB, that returns a re-calculated iovec > to start of T10_PI or Data SGL memory. > > v2 changes: > - Fix up vhost_scsi_handle_vqal comments > - Minor vhost_scsi_handle_vqal simplifications > - Add missing minimum virtio-scsi response buffer size check > - Fix pi_bytes* error message typo > - Convert to use vhost_skip_iovec_bytes() common code > - Add max_niov sanity checks vs. out + in offset into vq > > v3 changes: > - Convert to copy_from_iter usage > - Update vhost_scsi_handle_vqal comments for iov_iter usage > - Convert prot_bytes offset to use iov_iter_advance > - Drop max_niov usage in vhost_scsi_handle_vqal > - Drop vhost_skip_iovec_bytes in favour of iov_iter > > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > --- > drivers/vhost/scsi.c | 260 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 260 insertions(+) > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c > index 7dfff15..e1fe003 100644 > --- a/drivers/vhost/scsi.c > +++ b/drivers/vhost/scsi.c > @@ -1062,6 +1062,266 @@ vhost_scsi_send_bad_target(struct vhost_scsi *vs, > } > > static void > +vhost_scsi_handle_vqal(struct vhost_scsi *vs, struct vhost_virtqueue *vq) > +{ > + struct tcm_vhost_tpg **vs_tpg, *tpg; > + struct virtio_scsi_cmd_req v_req; > + struct virtio_scsi_cmd_req_pi v_req_pi; > + struct tcm_vhost_cmd *cmd; > + struct iov_iter out_iter, in_iter, prot_iter, data_iter; > + u64 tag; > + u32 exp_data_len, data_direction; > + unsigned out, in, i; > + int head, ret, prot_bytes; > + size_t req_size, rsp_size = sizeof(struct virtio_scsi_cmd_resp); > + size_t out_size, in_size; > + u16 lun; > + u8 *target, *lunp, task_attr; > + bool t10_pi = vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI); > + void *req, *cdb; > + > + mutex_lock(&vq->mutex); > + /* > + * We can handle the vq only after the endpoint is setup by calling the > + * VHOST_SCSI_SET_ENDPOINT ioctl. > + */ > + vs_tpg = vq->private_data; > + if (!vs_tpg) > + goto out; > + > + vhost_disable_notify(&vs->dev, vq); > + > + for (;;) { > + head = vhost_get_vq_desc(vq, vq->iov, > + ARRAY_SIZE(vq->iov), &out, &in, > + NULL, NULL); > + pr_debug("vhost_get_vq_desc: head: %d, out: %u in: %u\n", > + head, out, in); > + /* On error, stop handling until the next kick. */ > + if (unlikely(head < 0)) > + break; > + /* Nothing new? Wait for eventfd to tell us they refilled. */ > + if (head == vq->num) { > + if (unlikely(vhost_enable_notify(&vs->dev, vq))) { > + vhost_disable_notify(&vs->dev, vq); > + continue; > + } > + break; > + } > + /* > + * Check for a sane response buffer so we can report early > + * errors back to the guest. > + */ > + if (unlikely(vq->iov[out].iov_len < rsp_size)) { > + vq_err(vq, "Expecting at least virtio_scsi_cmd_resp" > + " size, got %zu bytes\n", vq->iov[out].iov_len); > + break; > + } > + /* > + * Setup pointers and values based upon different virtio-scsi > + * request header if T10_PI is enabled in KVM guest. > + */ > + if (t10_pi) { > + req = &v_req_pi; > + req_size = sizeof(v_req_pi); > + lunp = &v_req_pi.lun[0]; > + target = &v_req_pi.lun[1]; > + } else { > + req = &v_req; > + req_size = sizeof(v_req); > + lunp = &v_req.lun[0]; > + target = &v_req.lun[1]; > + } > + /* > + * Determine data_direction for ANY_LAYOUT It's not just for ANY_LAYOUT though, is it? After patchset is fully applied this will run for all guests? > by calculating the > + * total outgoing iovec sizes / incoming iovec sizes vs. > + * virtio-scsi request / response headers respectively. > + * > + * FIXME: Not correct for BIDI operation > + */ > + out_size = in_size = 0; > + for (i = 0; i < out; i++) > + out_size += vq->iov[i].iov_len; > + for (i = out; i < out + in; i++) > + in_size += vq->iov[i].iov_len; Why not use iov_length? > + /* > + * Any associated T10_PI bytes for the outgoing / incoming > + * payloads are included in calculation of exp_data_len here. > + */ > + if (out_size > req_size) { > + data_direction = DMA_TO_DEVICE; > + exp_data_len = out_size - req_size; > + } else if (in_size > rsp_size) { > + data_direction = DMA_FROM_DEVICE; > + exp_data_len = in_size - rsp_size; > + } else { > + data_direction = DMA_NONE; > + exp_data_len = 0; > + } We must validate this doesn't cause exp_data_len to be negative. > + /* > + * Copy over the virtio-scsi request header, which when > + * ANY_LAYOUT is enabled may span multiple iovecs, or a > + * single iovec may contain both the header + outgoing > + * WRITE payloads. > + * > + * copy_from_iter() is modifying the iovecs as copies over > + * req_size bytes into req, so the returned out_iter.iov[0] > + * will contain the correct start + offset of the outgoing > + * WRITE payload, if DMA_TO_DEVICE is set. > + */ > + iov_iter_init(&out_iter, READ, &vq->iov[0], out, I'd prefer just using vq->iov here, then we know that any iov[number] user is left-over that needs to be converted to use iterators. > + (data_direction == DMA_TO_DEVICE) ? > + req_size + exp_data_len : req_size); Hmm does not look safe: iovecs are pre-validated with in_size/out_size, not with req_size. The following should be equivalent, should it not? iov_iter_init(&out_iter, READ, &vq->iov[0], out, out_size); > + > + ret = copy_from_iter(req, req_size, &out_iter); > + if (unlikely(ret != req_size)) { > + vq_err(vq, "Faulted on copy_from_iter\n"); > + vhost_scsi_send_bad_target(vs, vq, head, out); > + continue; > + } > + /* virtio-scsi spec requires byte 0 of the lun to be 1 */ > + if (unlikely(*lunp != 1)) { > + vq_err(vq, "Illegal virtio-scsi lun: %u\n", *lunp); > + vhost_scsi_send_bad_target(vs, vq, head, out); > + continue; > + } > + > + tpg = ACCESS_ONCE(vs_tpg[*target]); > + if (unlikely(!tpg)) { > + /* Target does not exist, fail the request */ > + vhost_scsi_send_bad_target(vs, vq, head, out); > + continue; > + } > + /* > + * Determine start of T10_PI or data payload iovec in ANY_LAYOUT > + * mode based upon data_direction. Same comments as above. > + * > + * For DMA_TO_DEVICE, this is iov_out from copy_from_iter() > + * with the already recalculated iov_base + iov_len. > + * > + * For DMA_FROM_DEVICE, the iovec will be just past the end > + * of the virtio-scsi response header in either the same > + * or immediately following iovec. > + */ > + prot_bytes = 0; > + > + if (data_direction == DMA_TO_DEVICE) { > + data_iter = out_iter; > + } else if (data_direction == DMA_FROM_DEVICE) { > + iov_iter_init(&in_iter, WRITE, &vq->iov[out], in, > + rsp_size + exp_data_len); > + iov_iter_advance(&in_iter, rsp_size); > + data_iter = in_iter; > + } > + /* > + * If T10_PI header + payload is present, setup prot_iter values > + * and recalculate data_iter for vhost_scsi_mapal() mapping to > + * host scatterlists via get_user_pages_fast(). > + */ > + if (t10_pi) { > + if (v_req_pi.pi_bytesout) { > + if (data_direction != DMA_TO_DEVICE) { > + vq_err(vq, "Received non zero pi_bytesout," > + " but wrong data_direction\n"); > + vhost_scsi_send_bad_target(vs, vq, head, out); > + continue; > + } > + prot_bytes = vhost32_to_cpu(vq, v_req_pi.pi_bytesout); > + } else if (v_req_pi.pi_bytesin) { > + if (data_direction != DMA_FROM_DEVICE) { > + vq_err(vq, "Received non zero pi_bytesin," > + " but wrong data_direction\n"); > + vhost_scsi_send_bad_target(vs, vq, head, out); > + continue; > + } > + prot_bytes = vhost32_to_cpu(vq, v_req_pi.pi_bytesin); > + } > + /* > + * Set prot_iter to data_iter, and advance past any > + * preceeding prot_bytes that may be present. > + * > + * Also fix up the exp_data_len to reflect only the > + * actual data payload length. > + */ > + if (prot_bytes) { > + exp_data_len -= prot_bytes; > + prot_iter = data_iter; > + iov_iter_advance(&data_iter, prot_bytes); > + } > + tag = vhost64_to_cpu(vq, v_req_pi.tag); > + task_attr = v_req_pi.task_attr; > + cdb = &v_req_pi.cdb[0]; > + lun = ((v_req_pi.lun[2] << 8) | v_req_pi.lun[3]) & 0x3FFF; > + } else { > + tag = vhost64_to_cpu(vq, v_req.tag); > + task_attr = v_req.task_attr; > + cdb = &v_req.cdb[0]; > + lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF; > + } > + /* > + * Check that the recieved CDB size does not exceeded our received. > + * hardcoded max for vhost-scsi, then get a pre-allocated > + * cmd descriptor for the new virtio-scsi tag. > + * > + * TODO what if cdb was too small for varlen cdb header? > + */ > + if (unlikely(scsi_command_size(cdb) > TCM_VHOST_MAX_CDB_SIZE)) { > + vq_err(vq, "Received SCSI CDB with command_size: %d that" > + " exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n", > + scsi_command_size(cdb), TCM_VHOST_MAX_CDB_SIZE); > + vhost_scsi_send_bad_target(vs, vq, head, out); > + continue; > + } > + cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr, > + exp_data_len + prot_bytes, > + data_direction); > + if (IS_ERR(cmd)) { > + vq_err(vq, "vhost_scsi_get_tag failed %ld\n", > + PTR_ERR(cmd)); > + vhost_scsi_send_bad_target(vs, vq, head, out); > + continue; > + } > + cmd->tvc_vhost = vs; > + cmd->tvc_vq = vq; > + cmd->tvc_resp_iov = &vq->iov[out]; > + cmd->tvc_in_iovs = in; > + > + pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n", > + cmd->tvc_cdb[0], cmd->tvc_lun); > + pr_debug("cmd: %p exp_data_len: %d, prot_bytes: %d data_direction:" > + " %d\n", cmd, exp_data_len, prot_bytes, data_direction); > + > + if (data_direction != DMA_NONE) { > + ret = vhost_scsi_mapal(cmd, > + prot_bytes, &prot_iter, > + exp_data_len, &data_iter); > + if (unlikely(ret)) { > + vq_err(vq, "Failed to map iov to sgl\n"); > + tcm_vhost_release_cmd(&cmd->tvc_se_cmd); > + vhost_scsi_send_bad_target(vs, vq, head, out); > + continue; > + } > + } > + /* > + * Save the descriptor from vhost_get_vq_desc() to be used to > + * complete the virtio-scsi request in TCM callback context via > + * vhost_scsi_queue_data_in() and vhost_scsi_queue_status() > + */ > + cmd->tvc_vq_desc = head; > + /* > + * Dispatch cmd descriptor for cmwq execution in process > + * context provided by vhost_scsi_workqueue. This also ensures > + * cmd is executed on the same kworker CPU as this vhost > + * thread to gain positive L2 cache locality effects. > + */ > + INIT_WORK(&cmd->work, tcm_vhost_submission_work); > + queue_work(tcm_vhost_workqueue, &cmd->work); > + } > +out: > + mutex_unlock(&vq->mutex); > +} > + > +static void > vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq) > { > struct tcm_vhost_tpg **vs_tpg; > -- > 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> + * copy_from_iter() is modifying the iovecs as copies over > + * req_size bytes into req, so the returned out_iter.iov[0] > + * will contain the correct start + offset of the outgoing > + * WRITE payload, if DMA_TO_DEVICE is set. > + */ Does it, in fact, do this? I thought so too, but Al Viro corrected me that it's not supposed to.
On Tue, Feb 03, 2015 at 06:29:59AM +0000, Nicholas A. Bellinger wrote: > + * Copy over the virtio-scsi request header, which when > + * ANY_LAYOUT is enabled may span multiple iovecs, or a > + * single iovec may contain both the header + outgoing > + * WRITE payloads. > + * > + * copy_from_iter() is modifying the iovecs as copies over > + * req_size bytes into req, so the returned out_iter.iov[0] > + * will contain the correct start + offset of the outgoing > + * WRITE payload, if DMA_TO_DEVICE is set. It does no such thing. What it does, though, is changing out_iter so that subsequent copy_from_iter() will return the data you want. Note that out_iter.iov[0] will contain the correct _segment_ of that vector, with the data you want at out_iter.iov_offset bytes from the beginning of that segment. .iov may come to point to subsequent segments and .iov_offset keeps changing, but segments themselves are never changed. > + */ > + iov_iter_init(&out_iter, READ, &vq->iov[0], out, ^^^^ WRITE, please - as in "this is the source of some write, we'll be copying _from_ it". READ would be "destination of some read, we'll be copying into it". > + (data_direction == DMA_TO_DEVICE) ? > + req_size + exp_data_len : req_size); > + > + ret = copy_from_iter(req, req_size, &out_iter); ... > + /* > + * Determine start of T10_PI or data payload iovec in ANY_LAYOUT > + * mode based upon data_direction. > + * > + * For DMA_TO_DEVICE, this is iov_out from copy_from_iter() > + * with the already recalculated iov_base + iov_len. ITYM "this is out_iter, which is already pointing to the right place" AFAICS, the actual use is correct, it's just that the comments are confused. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 03, 2015 at 11:56:16PM +0000, Al Viro wrote: > On Tue, Feb 03, 2015 at 06:29:59AM +0000, Nicholas A. Bellinger wrote: > > + * Copy over the virtio-scsi request header, which when > > + * ANY_LAYOUT is enabled may span multiple iovecs, or a > > + * single iovec may contain both the header + outgoing > > + * WRITE payloads. > > + * > > + * copy_from_iter() is modifying the iovecs as copies over > > + * req_size bytes into req, so the returned out_iter.iov[0] > > + * will contain the correct start + offset of the outgoing > > + * WRITE payload, if DMA_TO_DEVICE is set. > > It does no such thing. What it does, though, is changing out_iter so > that subsequent copy_from_iter() will return the data you want. Note > that out_iter.iov[0] will contain the correct _segment_ of that vector, > with the data you want at out_iter.iov_offset bytes from the beginning > of that segment. .iov may come to point to subsequent segments and .iov_offset > keeps changing, but segments themselves are never changed. > > > + */ > > + iov_iter_init(&out_iter, READ, &vq->iov[0], out, > ^^^^ WRITE, please - as in "this is > the source of some write, we'll be copying _from_ it". READ would be > "destination of some read, we'll be copying into it". > > > + (data_direction == DMA_TO_DEVICE) ? > > + req_size + exp_data_len : req_size); > > + > > + ret = copy_from_iter(req, req_size, &out_iter); > > ... > > > + /* > > + * Determine start of T10_PI or data payload iovec in ANY_LAYOUT > > + * mode based upon data_direction. > > + * > > + * For DMA_TO_DEVICE, this is iov_out from copy_from_iter() > > + * with the already recalculated iov_base + iov_len. > > ITYM "this is out_iter, which is already pointing to the right place" > > AFAICS, the actual use is correct, it's just that the comments are confused. Looks like it'd be nicer to pass iters around as much as possible, try to reduce the amount of poking at the underlying iov. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-02-03 at 12:14 +0200, Michael S. Tsirkin wrote: > On Tue, Feb 03, 2015 at 06:29:59AM +0000, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > > > This patch adds ANY_LAYOUT support with a new vqs[].vq.handle_kick() > > callback in vhost_scsi_handle_vqal(). > > > > It calculates data_direction + exp_data_len for the new tcm_vhost_cmd > > descriptor by walking both outgoing + incoming iovecs using iov_iter, > > assuming the layout of outgoing request header + T10_PI + Data payload > > comes first. > > > > It also uses copy_from_iter() to copy leading virtio-scsi request header > > that may or may not include SCSI CDB, that returns a re-calculated iovec > > to start of T10_PI or Data SGL memory. > > > > v2 changes: > > - Fix up vhost_scsi_handle_vqal comments > > - Minor vhost_scsi_handle_vqal simplifications > > - Add missing minimum virtio-scsi response buffer size check > > - Fix pi_bytes* error message typo > > - Convert to use vhost_skip_iovec_bytes() common code > > - Add max_niov sanity checks vs. out + in offset into vq > > > > v3 changes: > > - Convert to copy_from_iter usage > > - Update vhost_scsi_handle_vqal comments for iov_iter usage > > - Convert prot_bytes offset to use iov_iter_advance > > - Drop max_niov usage in vhost_scsi_handle_vqal > > - Drop vhost_skip_iovec_bytes in favour of iov_iter > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > > --- > > drivers/vhost/scsi.c | 260 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 260 insertions(+) > > > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c > > index 7dfff15..e1fe003 100644 > > --- a/drivers/vhost/scsi.c > > +++ b/drivers/vhost/scsi.c > > @@ -1062,6 +1062,266 @@ vhost_scsi_send_bad_target(struct vhost_scsi *vs, > > } > > > > static void > > +vhost_scsi_handle_vqal(struct vhost_scsi *vs, struct vhost_virtqueue *vq) > > +{ > > + struct tcm_vhost_tpg **vs_tpg, *tpg; > > + struct virtio_scsi_cmd_req v_req; > > + struct virtio_scsi_cmd_req_pi v_req_pi; > > + struct tcm_vhost_cmd *cmd; > > + struct iov_iter out_iter, in_iter, prot_iter, data_iter; > > + u64 tag; > > + u32 exp_data_len, data_direction; > > + unsigned out, in, i; > > + int head, ret, prot_bytes; > > + size_t req_size, rsp_size = sizeof(struct virtio_scsi_cmd_resp); > > + size_t out_size, in_size; > > + u16 lun; > > + u8 *target, *lunp, task_attr; > > + bool t10_pi = vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI); > > + void *req, *cdb; > > + > > + mutex_lock(&vq->mutex); > > + /* > > + * We can handle the vq only after the endpoint is setup by calling the > > + * VHOST_SCSI_SET_ENDPOINT ioctl. > > + */ > > + vs_tpg = vq->private_data; > > + if (!vs_tpg) > > + goto out; > > + > > + vhost_disable_notify(&vs->dev, vq); > > + > > + for (;;) { > > + head = vhost_get_vq_desc(vq, vq->iov, > > + ARRAY_SIZE(vq->iov), &out, &in, > > + NULL, NULL); > > + pr_debug("vhost_get_vq_desc: head: %d, out: %u in: %u\n", > > + head, out, in); > > + /* On error, stop handling until the next kick. */ > > + if (unlikely(head < 0)) > > + break; > > + /* Nothing new? Wait for eventfd to tell us they refilled. */ > > + if (head == vq->num) { > > + if (unlikely(vhost_enable_notify(&vs->dev, vq))) { > > + vhost_disable_notify(&vs->dev, vq); > > + continue; > > + } > > + break; > > + } > > + /* > > + * Check for a sane response buffer so we can report early > > + * errors back to the guest. > > + */ > > + if (unlikely(vq->iov[out].iov_len < rsp_size)) { > > + vq_err(vq, "Expecting at least virtio_scsi_cmd_resp" > > + " size, got %zu bytes\n", vq->iov[out].iov_len); > > + break; > > + } > > + /* > > + * Setup pointers and values based upon different virtio-scsi > > + * request header if T10_PI is enabled in KVM guest. > > + */ > > + if (t10_pi) { > > + req = &v_req_pi; > > + req_size = sizeof(v_req_pi); > > + lunp = &v_req_pi.lun[0]; > > + target = &v_req_pi.lun[1]; > > + } else { > > + req = &v_req; > > + req_size = sizeof(v_req); > > + lunp = &v_req.lun[0]; > > + target = &v_req.lun[1]; > > + } > > + /* > > + * Determine data_direction for ANY_LAYOUT > > It's not just for ANY_LAYOUT though, is it? > After patchset is fully applied this will run for > all guests? Dropping ANY_LAYOUT specific wording here.. > > > by calculating the > > + * total outgoing iovec sizes / incoming iovec sizes vs. > > + * virtio-scsi request / response headers respectively. > > + * > > + * FIXME: Not correct for BIDI operation > > + */ > > + out_size = in_size = 0; > > + for (i = 0; i < out; i++) > > + out_size += vq->iov[i].iov_len; > > + for (i = out; i < out + in; i++) > > + in_size += vq->iov[i].iov_len; > > Why not use iov_length? <nod>, converted to use iov_length. > > > + /* > > + * Any associated T10_PI bytes for the outgoing / incoming > > + * payloads are included in calculation of exp_data_len here. > > + */ > > + if (out_size > req_size) { > > + data_direction = DMA_TO_DEVICE; > > + exp_data_len = out_size - req_size; > > + } else if (in_size > rsp_size) { > > + data_direction = DMA_FROM_DEVICE; > > + exp_data_len = in_size - rsp_size; > > + } else { > > + data_direction = DMA_NONE; > > + exp_data_len = 0; > > + } > > We must validate this doesn't cause exp_data_len to be negative. > AFAICT, exp_data_len is always >= 0 here. > > + /* > > + * Copy over the virtio-scsi request header, which when > > + * ANY_LAYOUT is enabled may span multiple iovecs, or a > > + * single iovec may contain both the header + outgoing > > + * WRITE payloads. > > + * > > + * copy_from_iter() is modifying the iovecs as copies over > > + * req_size bytes into req, so the returned out_iter.iov[0] > > + * will contain the correct start + offset of the outgoing > > + * WRITE payload, if DMA_TO_DEVICE is set. > > + */ > > + iov_iter_init(&out_iter, READ, &vq->iov[0], out, > > I'd prefer just using vq->iov here, then we know that > any iov[number] user is left-over that needs to be converted > to use iterators. > Done. > > + (data_direction == DMA_TO_DEVICE) ? > > + req_size + exp_data_len : req_size); > > Hmm does not look safe: iovecs are pre-validated with > in_size/out_size, not with req_size. > The following should be equivalent, should it not? > iov_iter_init(&out_iter, READ, &vq->iov[0], out, > out_size); > <nod>, using out_size here instead. > > > + > > + ret = copy_from_iter(req, req_size, &out_iter); > > + if (unlikely(ret != req_size)) { > > + vq_err(vq, "Faulted on copy_from_iter\n"); > > + vhost_scsi_send_bad_target(vs, vq, head, out); > > + continue; > > + } > > + /* virtio-scsi spec requires byte 0 of the lun to be 1 */ > > + if (unlikely(*lunp != 1)) { > > + vq_err(vq, "Illegal virtio-scsi lun: %u\n", *lunp); > > + vhost_scsi_send_bad_target(vs, vq, head, out); > > + continue; > > + } > > + > > + tpg = ACCESS_ONCE(vs_tpg[*target]); > > + if (unlikely(!tpg)) { > > + /* Target does not exist, fail the request */ > > + vhost_scsi_send_bad_target(vs, vq, head, out); > > + continue; > > + } > > + /* > > + * Determine start of T10_PI or data payload iovec in ANY_LAYOUT > > + * mode based upon data_direction. > > Same comments as above. > Fixed. > > + * > > + * For DMA_TO_DEVICE, this is iov_out from copy_from_iter() > > + * with the already recalculated iov_base + iov_len. > > + * > > + * For DMA_FROM_DEVICE, the iovec will be just past the end > > + * of the virtio-scsi response header in either the same > > + * or immediately following iovec. > > + */ > > + prot_bytes = 0; > > + > > + if (data_direction == DMA_TO_DEVICE) { > > + data_iter = out_iter; > > + } else if (data_direction == DMA_FROM_DEVICE) { > > + iov_iter_init(&in_iter, WRITE, &vq->iov[out], in, > > + rsp_size + exp_data_len); > > + iov_iter_advance(&in_iter, rsp_size); > > + data_iter = in_iter; > > + } > > + /* > > + * If T10_PI header + payload is present, setup prot_iter values > > + * and recalculate data_iter for vhost_scsi_mapal() mapping to > > + * host scatterlists via get_user_pages_fast(). > > + */ > > + if (t10_pi) { > > + if (v_req_pi.pi_bytesout) { > > + if (data_direction != DMA_TO_DEVICE) { > > + vq_err(vq, "Received non zero pi_bytesout," > > + " but wrong data_direction\n"); > > + vhost_scsi_send_bad_target(vs, vq, head, out); > > + continue; > > + } > > + prot_bytes = vhost32_to_cpu(vq, v_req_pi.pi_bytesout); > > + } else if (v_req_pi.pi_bytesin) { > > + if (data_direction != DMA_FROM_DEVICE) { > > + vq_err(vq, "Received non zero pi_bytesin," > > + " but wrong data_direction\n"); > > + vhost_scsi_send_bad_target(vs, vq, head, out); > > + continue; > > + } > > + prot_bytes = vhost32_to_cpu(vq, v_req_pi.pi_bytesin); > > + } > > + /* > > + * Set prot_iter to data_iter, and advance past any > > + * preceeding prot_bytes that may be present. > > + * > > + * Also fix up the exp_data_len to reflect only the > > + * actual data payload length. > > + */ > > + if (prot_bytes) { > > + exp_data_len -= prot_bytes; > > + prot_iter = data_iter; > > + iov_iter_advance(&data_iter, prot_bytes); > > + } > > + tag = vhost64_to_cpu(vq, v_req_pi.tag); > > + task_attr = v_req_pi.task_attr; > > + cdb = &v_req_pi.cdb[0]; > > + lun = ((v_req_pi.lun[2] << 8) | v_req_pi.lun[3]) & 0x3FFF; > > + } else { > > + tag = vhost64_to_cpu(vq, v_req.tag); > > + task_attr = v_req.task_attr; > > + cdb = &v_req.cdb[0]; > > + lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF; > > + } > > + /* > > + * Check that the recieved CDB size does not exceeded our > > received. > Fixed. --nab -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 04, 2015 at 01:40:25AM -0800, Nicholas A. Bellinger wrote: > > > + /* > > > + * Any associated T10_PI bytes for the outgoing / incoming > > > + * payloads are included in calculation of exp_data_len here. > > > + */ > > > + if (out_size > req_size) { > > > + data_direction = DMA_TO_DEVICE; > > > + exp_data_len = out_size - req_size; > > > + } else if (in_size > rsp_size) { > > > + data_direction = DMA_FROM_DEVICE; > > > + exp_data_len = in_size - rsp_size; > > > + } else { > > > + data_direction = DMA_NONE; > > > + exp_data_len = 0; > > > + } > > > > We must validate this doesn't cause exp_data_len to be negative. > > > > AFAICT, exp_data_len is always >= 0 here. What guarantees out_size > req_size and in_size > rsp_size, respectively?
On Tue, 2015-02-03 at 23:56 +0000, Al Viro wrote: > On Tue, Feb 03, 2015 at 06:29:59AM +0000, Nicholas A. Bellinger wrote: > > + * Copy over the virtio-scsi request header, which when > > + * ANY_LAYOUT is enabled may span multiple iovecs, or a > > + * single iovec may contain both the header + outgoing > > + * WRITE payloads. > > + * > > + * copy_from_iter() is modifying the iovecs as copies over > > + * req_size bytes into req, so the returned out_iter.iov[0] > > + * will contain the correct start + offset of the outgoing > > + * WRITE payload, if DMA_TO_DEVICE is set. > > It does no such thing. What it does, though, is changing out_iter so > that subsequent copy_from_iter() will return the data you want. Note > that out_iter.iov[0] will contain the correct _segment_ of that vector, > with the data you want at out_iter.iov_offset bytes from the beginning > of that segment. .iov may come to point to subsequent segments and .iov_offset > keeps changing, but segments themselves are never changed. Yes, sorry. Updating that comment to read: /* * Copy over the virtio-scsi request header, which for a * ANY_LAYOUT enabled guest may span multiple iovecs, or a * single iovec may contain both the header + outgoing * WRITE payloads. * * copy_from_iter() copies over req_size bytes, and sets up * out_iter.iov[0] + out_iter.iov_offset to contain the start * of the outgoing WRITE payload, if DMA_TO_DEVICE is set. */ > > > + */ > > + iov_iter_init(&out_iter, READ, &vq->iov[0], out, > ^^^^ WRITE, please - as in "this is > the source of some write, we'll be copying _from_ it". READ would be > "destination of some read, we'll be copying into it". > Ahh, missed that iov_iter_get_pages() is already doing the write bit inversion of get_user_pages_fast() for in_iter -> DMA_FROM_DEVICE. Fixed to use correct i->type assignments. > > + (data_direction == DMA_TO_DEVICE) ? > > + req_size + exp_data_len : req_size); > > + > > + ret = copy_from_iter(req, req_size, &out_iter); > > ... > > > + /* > > + * Determine start of T10_PI or data payload iovec in ANY_LAYOUT > > + * mode based upon data_direction. > > + * > > + * For DMA_TO_DEVICE, this is iov_out from copy_from_iter() > > + * with the already recalculated iov_base + iov_len. > > ITYM "this is out_iter, which is already pointing to the right place" > Updated. > AFAICS, the actual use is correct, it's just that the comments are confused. Thanks for your review. --nab -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 04, 2015 at 02:11:20AM -0800, Nicholas A. Bellinger wrote: > On Tue, 2015-02-03 at 23:56 +0000, Al Viro wrote: > > On Tue, Feb 03, 2015 at 06:29:59AM +0000, Nicholas A. Bellinger wrote: > > > + * Copy over the virtio-scsi request header, which when > > > + * ANY_LAYOUT is enabled may span multiple iovecs, or a > > > + * single iovec may contain both the header + outgoing > > > + * WRITE payloads. > > > + * > > > + * copy_from_iter() is modifying the iovecs as copies over > > > + * req_size bytes into req, so the returned out_iter.iov[0] > > > + * will contain the correct start + offset of the outgoing > > > + * WRITE payload, if DMA_TO_DEVICE is set. > > > > It does no such thing. What it does, though, is changing out_iter so > > that subsequent copy_from_iter() will return the data you want. Note > > that out_iter.iov[0] will contain the correct _segment_ of that vector, > > with the data you want at out_iter.iov_offset bytes from the beginning > > of that segment. .iov may come to point to subsequent segments and .iov_offset > > keeps changing, but segments themselves are never changed. > > Yes, sorry. Updating that comment to read: > > /* > * Copy over the virtio-scsi request header, which for a > * ANY_LAYOUT enabled guest may span multiple iovecs, or a > * single iovec may contain both the header + outgoing > * WRITE payloads. > * > * copy_from_iter() copies over req_size bytes, and sets up > * out_iter.iov[0] + out_iter.iov_offset to contain the start > * of the outgoing WRITE payload, if DMA_TO_DEVICE is set. > */ I'm still confused wrt what this refers to. You don't actually play with iovs directly anymore, why bother explaining what happens to the underlying iov? Can we just say copy_from_iter will advance out_iter, so that it will point at the start of the outgoing WRITE payload, if DMA_TO_DEVICE is set. Seems clearer to me. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-02-04 at 10:42 +0100, Michael S. Tsirkin wrote: > On Wed, Feb 04, 2015 at 01:40:25AM -0800, Nicholas A. Bellinger wrote: > > > > + /* > > > > + * Any associated T10_PI bytes for the outgoing / incoming > > > > + * payloads are included in calculation of exp_data_len here. > > > > + */ > > > > + if (out_size > req_size) { > > > > + data_direction = DMA_TO_DEVICE; > > > > + exp_data_len = out_size - req_size; > > > > + } else if (in_size > rsp_size) { > > > > + data_direction = DMA_FROM_DEVICE; > > > > + exp_data_len = in_size - rsp_size; > > > > + } else { > > > > + data_direction = DMA_NONE; > > > > + exp_data_len = 0; > > > > + } > > > > > > We must validate this doesn't cause exp_data_len to be negative. > > > > > > > AFAICT, exp_data_len is always >= 0 here. > > What guarantees out_size > req_size and in_size > rsp_size, > respectively? > Mmm, point taken. So moving this part after copy_from_iter() ensures that at least req_size bytes exists of out_size. Making this change now. For in_size > rsp_size there is no guarantee, and falls back to data_direction = DMA_NONE + exp_data_len = 0; Is this what you had in mind..? --nab -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-02-04 at 11:20 +0100, Michael S. Tsirkin wrote: > On Wed, Feb 04, 2015 at 02:11:20AM -0800, Nicholas A. Bellinger wrote: > > On Tue, 2015-02-03 at 23:56 +0000, Al Viro wrote: > > > On Tue, Feb 03, 2015 at 06:29:59AM +0000, Nicholas A. Bellinger wrote: > > > > + * Copy over the virtio-scsi request header, which when > > > > + * ANY_LAYOUT is enabled may span multiple iovecs, or a > > > > + * single iovec may contain both the header + outgoing > > > > + * WRITE payloads. > > > > + * > > > > + * copy_from_iter() is modifying the iovecs as copies over > > > > + * req_size bytes into req, so the returned out_iter.iov[0] > > > > + * will contain the correct start + offset of the outgoing > > > > + * WRITE payload, if DMA_TO_DEVICE is set. > > > > > > It does no such thing. What it does, though, is changing out_iter so > > > that subsequent copy_from_iter() will return the data you want. Note > > > that out_iter.iov[0] will contain the correct _segment_ of that vector, > > > with the data you want at out_iter.iov_offset bytes from the beginning > > > of that segment. .iov may come to point to subsequent segments and .iov_offset > > > keeps changing, but segments themselves are never changed. > > > > Yes, sorry. Updating that comment to read: > > > > /* > > * Copy over the virtio-scsi request header, which for a > > * ANY_LAYOUT enabled guest may span multiple iovecs, or a > > * single iovec may contain both the header + outgoing > > * WRITE payloads. > > * > > * copy_from_iter() copies over req_size bytes, and sets up > > * out_iter.iov[0] + out_iter.iov_offset to contain the start > > * of the outgoing WRITE payload, if DMA_TO_DEVICE is set. > > */ > > I'm still confused wrt what this refers to. > You don't actually play with iovs directly anymore, > why bother explaining what happens to the underlying iov? > Can we just say > copy_from_iter will advance out_iter, so that it will point > at the start of the outgoing WRITE payload, if DMA_TO_DEVICE is > set. > > Seems clearer to me. > Updated. --nab -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 04, 2015 at 02:41:07AM -0800, Nicholas A. Bellinger wrote: > On Wed, 2015-02-04 at 10:42 +0100, Michael S. Tsirkin wrote: > > On Wed, Feb 04, 2015 at 01:40:25AM -0800, Nicholas A. Bellinger wrote: > > > > > + /* > > > > > + * Any associated T10_PI bytes for the outgoing / incoming > > > > > + * payloads are included in calculation of exp_data_len here. > > > > > + */ > > > > > + if (out_size > req_size) { > > > > > + data_direction = DMA_TO_DEVICE; > > > > > + exp_data_len = out_size - req_size; > > > > > + } else if (in_size > rsp_size) { > > > > > + data_direction = DMA_FROM_DEVICE; > > > > > + exp_data_len = in_size - rsp_size; > > > > > + } else { > > > > > + data_direction = DMA_NONE; > > > > > + exp_data_len = 0; > > > > > + } > > > > > > > > We must validate this doesn't cause exp_data_len to be negative. > > > > > > > > > > AFAICT, exp_data_len is always >= 0 here. > > > > What guarantees out_size > req_size and in_size > rsp_size, > > respectively? > > > > Mmm, point taken. > > So moving this part after copy_from_iter() ensures that at least > req_size bytes exists of out_size. Making this change now. > > For in_size > rsp_size there is no guarantee, and falls back to > data_direction = DMA_NONE + exp_data_len = 0; > > Is this what you had in mind..? > > --nab Hmm what do you mean by "there is no guarantee"? What will happen if in_size < rsp_size because guest supplied an invalid descriptor?
On Wed, Feb 04, 2015 at 02:41:07AM -0800, Nicholas A. Bellinger wrote: > On Wed, 2015-02-04 at 10:42 +0100, Michael S. Tsirkin wrote: > > On Wed, Feb 04, 2015 at 01:40:25AM -0800, Nicholas A. Bellinger wrote: > > > > > + /* > > > > > + * Any associated T10_PI bytes for the outgoing / incoming > > > > > + * payloads are included in calculation of exp_data_len here. > > > > > + */ > > > > > + if (out_size > req_size) { > > > > > + data_direction = DMA_TO_DEVICE; > > > > > + exp_data_len = out_size - req_size; > > > > > + } else if (in_size > rsp_size) { > > > > > + data_direction = DMA_FROM_DEVICE; > > > > > + exp_data_len = in_size - rsp_size; > > > > > + } else { > > > > > + data_direction = DMA_NONE; > > > > > + exp_data_len = 0; > > > > > + } > > > > > > > > We must validate this doesn't cause exp_data_len to be negative. > > > > > > > > > > AFAICT, exp_data_len is always >= 0 here. > > > > What guarantees out_size > req_size and in_size > rsp_size, > > respectively? > > > > Mmm, point taken. > > So moving this part after copy_from_iter() ensures that at least > req_size bytes exists of out_size. Making this change now. > > For in_size > rsp_size there is no guarantee, and falls back to > data_direction = DMA_NONE + exp_data_len = 0; > > Is this what you had in mind..? > > --nab I don't see any problems with this.
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index 7dfff15..e1fe003 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -1062,6 +1062,266 @@ vhost_scsi_send_bad_target(struct vhost_scsi *vs, } static void +vhost_scsi_handle_vqal(struct vhost_scsi *vs, struct vhost_virtqueue *vq) +{ + struct tcm_vhost_tpg **vs_tpg, *tpg; + struct virtio_scsi_cmd_req v_req; + struct virtio_scsi_cmd_req_pi v_req_pi; + struct tcm_vhost_cmd *cmd; + struct iov_iter out_iter, in_iter, prot_iter, data_iter; + u64 tag; + u32 exp_data_len, data_direction; + unsigned out, in, i; + int head, ret, prot_bytes; + size_t req_size, rsp_size = sizeof(struct virtio_scsi_cmd_resp); + size_t out_size, in_size; + u16 lun; + u8 *target, *lunp, task_attr; + bool t10_pi = vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI); + void *req, *cdb; + + mutex_lock(&vq->mutex); + /* + * We can handle the vq only after the endpoint is setup by calling the + * VHOST_SCSI_SET_ENDPOINT ioctl. + */ + vs_tpg = vq->private_data; + if (!vs_tpg) + goto out; + + vhost_disable_notify(&vs->dev, vq); + + for (;;) { + head = vhost_get_vq_desc(vq, vq->iov, + ARRAY_SIZE(vq->iov), &out, &in, + NULL, NULL); + pr_debug("vhost_get_vq_desc: head: %d, out: %u in: %u\n", + head, out, in); + /* On error, stop handling until the next kick. */ + if (unlikely(head < 0)) + break; + /* Nothing new? Wait for eventfd to tell us they refilled. */ + if (head == vq->num) { + if (unlikely(vhost_enable_notify(&vs->dev, vq))) { + vhost_disable_notify(&vs->dev, vq); + continue; + } + break; + } + /* + * Check for a sane response buffer so we can report early + * errors back to the guest. + */ + if (unlikely(vq->iov[out].iov_len < rsp_size)) { + vq_err(vq, "Expecting at least virtio_scsi_cmd_resp" + " size, got %zu bytes\n", vq->iov[out].iov_len); + break; + } + /* + * Setup pointers and values based upon different virtio-scsi + * request header if T10_PI is enabled in KVM guest. + */ + if (t10_pi) { + req = &v_req_pi; + req_size = sizeof(v_req_pi); + lunp = &v_req_pi.lun[0]; + target = &v_req_pi.lun[1]; + } else { + req = &v_req; + req_size = sizeof(v_req); + lunp = &v_req.lun[0]; + target = &v_req.lun[1]; + } + /* + * Determine data_direction for ANY_LAYOUT by calculating the + * total outgoing iovec sizes / incoming iovec sizes vs. + * virtio-scsi request / response headers respectively. + * + * FIXME: Not correct for BIDI operation + */ + out_size = in_size = 0; + for (i = 0; i < out; i++) + out_size += vq->iov[i].iov_len; + for (i = out; i < out + in; i++) + in_size += vq->iov[i].iov_len; + /* + * Any associated T10_PI bytes for the outgoing / incoming + * payloads are included in calculation of exp_data_len here. + */ + if (out_size > req_size) { + data_direction = DMA_TO_DEVICE; + exp_data_len = out_size - req_size; + } else if (in_size > rsp_size) { + data_direction = DMA_FROM_DEVICE; + exp_data_len = in_size - rsp_size; + } else { + data_direction = DMA_NONE; + exp_data_len = 0; + } + /* + * Copy over the virtio-scsi request header, which when + * ANY_LAYOUT is enabled may span multiple iovecs, or a + * single iovec may contain both the header + outgoing + * WRITE payloads. + * + * copy_from_iter() is modifying the iovecs as copies over + * req_size bytes into req, so the returned out_iter.iov[0] + * will contain the correct start + offset of the outgoing + * WRITE payload, if DMA_TO_DEVICE is set. + */ + iov_iter_init(&out_iter, READ, &vq->iov[0], out, + (data_direction == DMA_TO_DEVICE) ? + req_size + exp_data_len : req_size); + + ret = copy_from_iter(req, req_size, &out_iter); + if (unlikely(ret != req_size)) { + vq_err(vq, "Faulted on copy_from_iter\n"); + vhost_scsi_send_bad_target(vs, vq, head, out); + continue; + } + /* virtio-scsi spec requires byte 0 of the lun to be 1 */ + if (unlikely(*lunp != 1)) { + vq_err(vq, "Illegal virtio-scsi lun: %u\n", *lunp); + vhost_scsi_send_bad_target(vs, vq, head, out); + continue; + } + + tpg = ACCESS_ONCE(vs_tpg[*target]); + if (unlikely(!tpg)) { + /* Target does not exist, fail the request */ + vhost_scsi_send_bad_target(vs, vq, head, out); + continue; + } + /* + * Determine start of T10_PI or data payload iovec in ANY_LAYOUT + * mode based upon data_direction. + * + * For DMA_TO_DEVICE, this is iov_out from copy_from_iter() + * with the already recalculated iov_base + iov_len. + * + * For DMA_FROM_DEVICE, the iovec will be just past the end + * of the virtio-scsi response header in either the same + * or immediately following iovec. + */ + prot_bytes = 0; + + if (data_direction == DMA_TO_DEVICE) { + data_iter = out_iter; + } else if (data_direction == DMA_FROM_DEVICE) { + iov_iter_init(&in_iter, WRITE, &vq->iov[out], in, + rsp_size + exp_data_len); + iov_iter_advance(&in_iter, rsp_size); + data_iter = in_iter; + } + /* + * If T10_PI header + payload is present, setup prot_iter values + * and recalculate data_iter for vhost_scsi_mapal() mapping to + * host scatterlists via get_user_pages_fast(). + */ + if (t10_pi) { + if (v_req_pi.pi_bytesout) { + if (data_direction != DMA_TO_DEVICE) { + vq_err(vq, "Received non zero pi_bytesout," + " but wrong data_direction\n"); + vhost_scsi_send_bad_target(vs, vq, head, out); + continue; + } + prot_bytes = vhost32_to_cpu(vq, v_req_pi.pi_bytesout); + } else if (v_req_pi.pi_bytesin) { + if (data_direction != DMA_FROM_DEVICE) { + vq_err(vq, "Received non zero pi_bytesin," + " but wrong data_direction\n"); + vhost_scsi_send_bad_target(vs, vq, head, out); + continue; + } + prot_bytes = vhost32_to_cpu(vq, v_req_pi.pi_bytesin); + } + /* + * Set prot_iter to data_iter, and advance past any + * preceeding prot_bytes that may be present. + * + * Also fix up the exp_data_len to reflect only the + * actual data payload length. + */ + if (prot_bytes) { + exp_data_len -= prot_bytes; + prot_iter = data_iter; + iov_iter_advance(&data_iter, prot_bytes); + } + tag = vhost64_to_cpu(vq, v_req_pi.tag); + task_attr = v_req_pi.task_attr; + cdb = &v_req_pi.cdb[0]; + lun = ((v_req_pi.lun[2] << 8) | v_req_pi.lun[3]) & 0x3FFF; + } else { + tag = vhost64_to_cpu(vq, v_req.tag); + task_attr = v_req.task_attr; + cdb = &v_req.cdb[0]; + lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF; + } + /* + * Check that the recieved CDB size does not exceeded our + * hardcoded max for vhost-scsi, then get a pre-allocated + * cmd descriptor for the new virtio-scsi tag. + * + * TODO what if cdb was too small for varlen cdb header? + */ + if (unlikely(scsi_command_size(cdb) > TCM_VHOST_MAX_CDB_SIZE)) { + vq_err(vq, "Received SCSI CDB with command_size: %d that" + " exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n", + scsi_command_size(cdb), TCM_VHOST_MAX_CDB_SIZE); + vhost_scsi_send_bad_target(vs, vq, head, out); + continue; + } + cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr, + exp_data_len + prot_bytes, + data_direction); + if (IS_ERR(cmd)) { + vq_err(vq, "vhost_scsi_get_tag failed %ld\n", + PTR_ERR(cmd)); + vhost_scsi_send_bad_target(vs, vq, head, out); + continue; + } + cmd->tvc_vhost = vs; + cmd->tvc_vq = vq; + cmd->tvc_resp_iov = &vq->iov[out]; + cmd->tvc_in_iovs = in; + + pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n", + cmd->tvc_cdb[0], cmd->tvc_lun); + pr_debug("cmd: %p exp_data_len: %d, prot_bytes: %d data_direction:" + " %d\n", cmd, exp_data_len, prot_bytes, data_direction); + + if (data_direction != DMA_NONE) { + ret = vhost_scsi_mapal(cmd, + prot_bytes, &prot_iter, + exp_data_len, &data_iter); + if (unlikely(ret)) { + vq_err(vq, "Failed to map iov to sgl\n"); + tcm_vhost_release_cmd(&cmd->tvc_se_cmd); + vhost_scsi_send_bad_target(vs, vq, head, out); + continue; + } + } + /* + * Save the descriptor from vhost_get_vq_desc() to be used to + * complete the virtio-scsi request in TCM callback context via + * vhost_scsi_queue_data_in() and vhost_scsi_queue_status() + */ + cmd->tvc_vq_desc = head; + /* + * Dispatch cmd descriptor for cmwq execution in process + * context provided by vhost_scsi_workqueue. This also ensures + * cmd is executed on the same kworker CPU as this vhost + * thread to gain positive L2 cache locality effects. + */ + INIT_WORK(&cmd->work, tcm_vhost_submission_work); + queue_work(tcm_vhost_workqueue, &cmd->work); + } +out: + mutex_unlock(&vq->mutex); +} + +static void vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq) { struct tcm_vhost_tpg **vs_tpg;