Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
On Wed, Feb 04, 2015 at 02:55:12AM -0800, Nicholas A. Bellinger wrote: > On Wed, 2015-02-04 at 02:41 -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..? > > > > Something akin to: > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c > index f72fc56..daf10b7 100644 > --- a/drivers/vhost/scsi.c > +++ b/drivers/vhost/scsi.c > @@ -1047,30 +1047,12 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct > vhost_virtqueue *vq) > target = &v_req.lun[1]; > } > /* > - * Determine data_direction 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 = iov_length(vq->iov, out); > in_size = iov_length(&vq->iov[out], in); > > /* > - * 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 for a >* ANY_LAYOUT enabled guest may span multiple iovecs, or a >* single iovec may contain both the header + outgoing > @@ -1102,8 +1084,9 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct > vhost_virtqueue *vq) > continue; > } > /* > - * Determine start of T10_PI or data payload iovec based upon > - * data_direction. > + * Determine data_direction by calculating the total outgoing > + * iovec sizes + incoming iovec sizes vs. virtio-scsi request + > + * response headers respectively. >* >* For DMA_TO_DEVICE this is out_iter, which is already pointing >* to the right place. > @@ -,16 +1094,27 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct > vhost_virtqueue *vq) >* 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. > + * > + * Any associated T10_PI bytes for the outgoing / incoming > + * payloads are included in calculation of exp_data_len here. >*/ > prot_bytes = 0; > > - if (data_direction == DMA_TO_DEVICE) { > + if (out_size > req_size) { > + data_direction = DMA_TO_DEVICE; > + exp_data_len = out_size - req_size; > data_iter = out_iter; > - } else if (data_direction == DMA_FROM_DEVICE) { > + } else if (in_size > rsp_size) { > + data_direction = DMA_FROM_DEVICE; > + exp_data_len = in_size - rsp_size; > + >
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
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. -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
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? -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
On Wed, 2015-02-04 at 02:41 -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..? > Something akin to: diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index f72fc56..daf10b7 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -1047,30 +1047,12 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq) target = &v_req.lun[1]; } /* -* Determine data_direction 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 = iov_length(vq->iov, out); in_size = iov_length(&vq->iov[out], in); /* -* 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 for a * ANY_LAYOUT enabled guest may span multiple iovecs, or a * single iovec may contain both the header + outgoing @@ -1102,8 +1084,9 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq) continue; } /* -* Determine start of T10_PI or data payload iovec based upon -* data_direction. +* Determine data_direction by calculating the total outgoing +* iovec sizes + incoming iovec sizes vs. virtio-scsi request + +* response headers respectively. * * For DMA_TO_DEVICE this is out_iter, which is already pointing * to the right place. @@ -,16 +1094,27 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq) * 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. +* +* Any associated T10_PI bytes for the outgoing / incoming +* payloads are included in calculation of exp_data_len here. */ prot_bytes = 0; - if (data_direction == DMA_TO_DEVICE) { + if (out_size > req_size) { + data_direction = DMA_TO_DEVICE; + exp_data_len = out_size - req_size; data_iter = out_iter; - } else if (data_direction == DMA_FROM_DEVICE) { + } else if (in_size > rsp_size) { + data_direction = DMA_FROM_DEVICE; + exp_data_len = in_size - rsp_size; + iov_iter_init(&in_iter, READ, &vq->iov[out], in, rsp_size + exp_data_len); iov_ite
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
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 +, Al Viro wrote: > > > On Tue, Feb 03, 2015 at 06:29:59AM +, 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
On Wed, Feb 04, 2015 at 02:11:20AM -0800, Nicholas A. Bellinger wrote: > On Tue, 2015-02-03 at 23:56 +, Al Viro wrote: > > On Tue, Feb 03, 2015 at 06:29:59AM +, 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
On Tue, 2015-02-03 at 23:56 +, Al Viro wrote: > On Tue, Feb 03, 2015 at 06:29:59AM +, 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
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? -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
On Tue, 2015-02-03 at 12:14 +0200, Michael S. Tsirkin wrote: > On Tue, Feb 03, 2015 at 06:29:59AM +, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger > > > > 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 > > Cc: Paolo Bonzini > > Signed-off-by: Nicholas Bellinger > > --- > > 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
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
On Tue, Feb 03, 2015 at 11:56:16PM +, Al Viro wrote: > On Tue, Feb 03, 2015 at 06:29:59AM +, 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
On Tue, Feb 03, 2015 at 06:29:59AM +, 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
> + * 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. -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
On Tue, Feb 03, 2015 at 06:29:59AM +, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger > > 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 > Cc: Paolo Bonzini > Signed-off-by: Nicholas Bellinger > --- > 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. > +