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.
> @@ -1111,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_iter_advance(&in_iter, rsp_size);
> data_iter = in_iter;
> + } else {
> + data_direction = DMA_NONE;
> + exp_data_len = 0;
> }
> /*
> * If T10_PI header + payload is present, setup prot_iter values
Looks good 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
