[PATCH 0/6] vhost/scsi: Add T10 PI SGL passthrough support

[PATCH 0/6] vhost/scsi: Add T10 PI SGL passthrough support

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi MST, MKP, Paolo & Co,

This is an updated patch series for adding T1O protection information (PI)
SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
endpoints.

Following Paolo's recommendations, this patch uses VIRTIO_SCSI_F_T10_PI
feature bits in both vhost/scsi and virtio-scsi to determine when the PI
enabled virtio_scsi_cmd_req_pi header should be used, instead of the
original virtio_scsi_cmd_req header.

As before, the change to attach protection information preceeding the
actual DataOUT + DataIN data payload, thus making a future improvement of
processing virtio buffers inline a possibility.

At this point the last item is the QEMU change to use VIRTIO_SCSI_F_T10_PI
when guest + host agree upon supported features.  Note this patch has been
tested in unprotected mode where one side does not support PI mode, and
correctly falls back to unprotected mode on both sides.

That said, I'd like to get these parts merged ASAP.

Please review.

--nab

RFCv2 -> PATCH changes:
  - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header in
    vhost/scsi and virtio-scsi (Paolo)
  - Remove hardcoded bits to force VIRTIO_SCSI_F_T10_PI mode for
    testing

RFCv1 -> RFCv2 changes:
  - Add virtio_scsi_cmd_req_pi header (Paolo + nab)
  - Use virtio_scsi_cmd_req_pi instead of existing ->prio (Paolo + nab)
  - Make protection buffer come before data buffer (Paolo + nab)
  - Update vhost_scsi_get_tag() parameter usage (nab)

Nicholas Bellinger (6):
  virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits
  vhost/scsi: Move sanity check into vhost_scsi_map_iov_to_sgl
  vhost/scsi: Add preallocation of protection SGLs
  vhost/scsi: Add T10 PI IOV -> SGL memory mapping logic
  vhost/scsi: Enable T10 PI IOV -> SGL memory mapping
  virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

 drivers/scsi/virtio_scsi.c  |   78 ++++++++---
 drivers/vhost/scsi.c        |  301 +++++++++++++++++++++++++++++--------------
 include/linux/virtio_scsi.h |   15 ++-
 3 files changed, 281 insertions(+), 113 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a virtio_scsi_cmd_req_pi header as recommened by
Paolo that contains do_pi_niov + di_pi_niov elements used for
signaling when protection information buffers are expected to
preceed the data buffers.

Also add new VIRTIO_SCSI_F_T10_PI feature bit to be used to signal
host support.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 include/linux/virtio_scsi.h |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index 4195b97..590d249 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -35,11 +35,23 @@ struct virtio_scsi_cmd_req {
 	u8 lun[8];		/* Logical Unit Number */
 	u64 tag;		/* Command identifier */
 	u8 task_attr;		/* Task attribute */
-	u8 prio;
+	u8 prio;		/* SAM command priority field */
 	u8 crn;
 	u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 } __packed;
 
+/* SCSI command request, followed by protection information */
+struct virtio_scsi_cmd_req_pi {
+	u8 lun[8];		/* Logical Unit Number */
+	u64 tag;		/* Command identifier */
+	u8 task_attr;		/* Task attribute */
+	u8 prio;		/* SAM command priority field */
+	u8 crn;
+	u16 do_pi_niov;		/* DataOUT PI Number of iovecs */
+	u16 di_pi_niov;		/* DataIN PI Number of iovecs */
+	u8 cdb[VIRTIO_SCSI_CDB_SIZE];
+} __packed;
+
 /* Response, followed by sense data and data-in */
 struct virtio_scsi_cmd_resp {
 	u32 sense_len;		/* Sense data length */
@@ -97,6 +109,7 @@ struct virtio_scsi_config {
 #define VIRTIO_SCSI_F_INOUT                    0
 #define VIRTIO_SCSI_F_HOTPLUG                  1
 #define VIRTIO_SCSI_F_CHANGE                   2
+#define VIRTIO_SCSI_F_T10_PI		       3
 
 /* Response codes */
 #define VIRTIO_SCSI_S_OK                       0
-- 
1.7.10.4

Re: [PATCH 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

[Michael S. Tsirkin]

On Sun, Apr 06, 2014 at 09:32:04PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch adds a virtio_scsi_cmd_req_pi header as recommened by
> Paolo that contains do_pi_niov + di_pi_niov elements used for
> signaling when protection information buffers are expected to
> preceed the data buffers.
> 
> Also add new VIRTIO_SCSI_F_T10_PI feature bit to be used to signal
> host support.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  include/linux/virtio_scsi.h |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
> index 4195b97..590d249 100644
> --- a/include/linux/virtio_scsi.h
> +++ b/include/linux/virtio_scsi.h
> @@ -35,11 +35,23 @@ struct virtio_scsi_cmd_req {
>  	u8 lun[8];		/* Logical Unit Number */
>  	u64 tag;		/* Command identifier */
>  	u8 task_attr;		/* Task attribute */
> -	u8 prio;
> +	u8 prio;		/* SAM command priority field */
>  	u8 crn;
>  	u8 cdb[VIRTIO_SCSI_CDB_SIZE];
>  } __packed;
>  
> +/* SCSI command request, followed by protection information */
> +struct virtio_scsi_cmd_req_pi {
> +	u8 lun[8];		/* Logical Unit Number */
> +	u64 tag;		/* Command identifier */
> +	u8 task_attr;		/* Task attribute */
> +	u8 prio;		/* SAM command priority field */
> +	u8 crn;

Hmm if we stick a reserved byte here, following fields will be
naturally aligned and we'd be able to get rid of __packed
instruction (which often causes gcc to generate worse code).

> +	u16 do_pi_niov;		/* DataOUT PI Number of iovecs */
> +	u16 di_pi_niov;		/* DataIN PI Number of iovecs */

So this looks like a somewhat problematic interface to me in that
it talks in terms of iovecs not bytes.
So this perpetuates the assumption that header is in a separate
iov from data (and protection is separate from data).
Arguably virtio doesn't work in terms of iovecs on the guest side so
this naming looks strange.
Further host side, get_vq_descs can in theory split a buffer to multiple
iovecs if it crosses the boundary of a memory region.

One solution is to use byte lengths here, but this does require
that vhost scsi gets rid of layout assumptions generally.
Not sure that's practical for -rc1.

Another is to do it like virtio-net does for RX, link two buffers
using the first one for data and the second one for protection.


> +	u8 cdb[VIRTIO_SCSI_CDB_SIZE];
> +} __packed;
> +
>  /* Response, followed by sense data and data-in */
>  struct virtio_scsi_cmd_resp {
>  	u32 sense_len;		/* Sense data length */
> @@ -97,6 +109,7 @@ struct virtio_scsi_config {
>  #define VIRTIO_SCSI_F_INOUT                    0
>  #define VIRTIO_SCSI_F_HOTPLUG                  1
>  #define VIRTIO_SCSI_F_CHANGE                   2
> +#define VIRTIO_SCSI_F_T10_PI		       3
>  
>  /* Response codes */
>  #define VIRTIO_SCSI_S_OK                       0
> -- 
> 1.7.10.4

Re: [PATCH 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

[Paolo Bonzini]

Il 07/04/2014 05:55, Michael S. Tsirkin ha scritto:
>> > +	u16 do_pi_niov;		/* DataOUT PI Number of iovecs */
>> > +	u16 di_pi_niov;		/* DataIN PI Number of iovecs */
> So this looks like a somewhat problematic interface to me in that
> it talks in terms of iovecs not bytes.
> So this perpetuates the assumption that header is in a separate
> iov from data (and protection is separate from data).
> Arguably virtio doesn't work in terms of iovecs on the guest side so
> this naming looks strange.
> Further host side, get_vq_descs can in theory split a buffer to multiple
> iovecs if it crosses the boundary of a memory region.
>
> One solution is to use byte lengths here, but this does require
> that vhost scsi gets rid of layout assumptions generally.
> Not sure that's practical for -rc1.

Why does that require that vhost scsi gets rid of layout assumptions?

The interface uses bytes instead of iovecs as the unit, and vhost-scsi 
can add the (temporary...) requirement that do_pi_nbytes and 
di_pi_nbytes comprise an integer number of iovecs.

Paolo

Re: [PATCH 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

[Michael S. Tsirkin]

On Tue, Apr 08, 2014 at 04:31:26PM -0400, Paolo Bonzini wrote:
> Il 07/04/2014 05:55, Michael S. Tsirkin ha scritto:
> >>> +	u16 do_pi_niov;		/* DataOUT PI Number of iovecs */
> >>> +	u16 di_pi_niov;		/* DataIN PI Number of iovecs */
> >So this looks like a somewhat problematic interface to me in that
> >it talks in terms of iovecs not bytes.
> >So this perpetuates the assumption that header is in a separate
> >iov from data (and protection is separate from data).
> >Arguably virtio doesn't work in terms of iovecs on the guest side so
> >this naming looks strange.
> >Further host side, get_vq_descs can in theory split a buffer to multiple
> >iovecs if it crosses the boundary of a memory region.
> >
> >One solution is to use byte lengths here, but this does require
> >that vhost scsi gets rid of layout assumptions generally.
> >Not sure that's practical for -rc1.
> 
> Why does that require that vhost scsi gets rid of layout assumptions?
> 
> The interface uses bytes instead of iovecs as the unit, and
> vhost-scsi can add the (temporary...) requirement that do_pi_nbytes
> and di_pi_nbytes comprise an integer number of iovecs.
> 
> Paolo

That would be a reasonable intermediate step, but I'm worried there
are more assumptions lurking in tere.

In particular is it legal for PI to be in the same iov
entry as data? If not they really need to use a
separate buf entry.

-- 
MST

Re: [PATCH 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

[Paolo Bonzini]

Il 09/04/2014 09:22, Michael S. Tsirkin ha scritto:
>> > The interface uses bytes instead of iovecs as the unit, and
>> > vhost-scsi can add the (temporary...) requirement that do_pi_nbytes
>> > and di_pi_nbytes comprise an integer number of iovecs.
>> >
>> > Paolo
> That would be a reasonable intermediate step, but I'm worried there
> are more assumptions lurking in tere.
>
> In particular is it legal for PI to be in the same iov
> entry as data? If not they really need to use a
> separate buf entry.

No, in fact that would mean that do/di_pi_nbytes are not an integer 
number of iovecs.

Paolo

[PATCH 2/6] vhost/scsi: Move sanity check into vhost_scsi_map_iov_to_sgl

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Move the overflow check for sgl_count > TCM_VHOST_PREALLOC_SGLS into
vhost_scsi_map_iov_to_sgl() so that it's based on the total number
of SGLs for all IOVs, instead of single IOVs.

Also, rename TCM_VHOST_PREALLOC_PAGES -> TCM_VHOST_PREALLOC_UPAGES
to better describe pointers to user-space pages.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/scsi.c |   59 +++++++++++++++++++++-----------------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index cf50ce9..ae434db 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -57,7 +57,7 @@
 #define TCM_VHOST_MAX_CDB_SIZE 32
 #define TCM_VHOST_DEFAULT_TAGS 256
 #define TCM_VHOST_PREALLOC_SGLS 2048
-#define TCM_VHOST_PREALLOC_PAGES 2048
+#define TCM_VHOST_PREALLOC_UPAGES 2048
 
 struct vhost_scsi_inflight {
 	/* Wait for the flush operation to finish */
@@ -767,35 +767,28 @@ vhost_scsi_map_to_sgl(struct tcm_vhost_cmd *tv_cmd,
 		      struct scatterlist *sgl,
 		      unsigned int sgl_count,
 		      struct iovec *iov,
-		      int write)
+		      struct page **pages,
+		      bool write)
 {
 	unsigned int npages = 0, pages_nr, offset, nbytes;
 	struct scatterlist *sg = sgl;
 	void __user *ptr = iov->iov_base;
 	size_t len = iov->iov_len;
-	struct page **pages;
 	int ret, i;
 
-	if (sgl_count > TCM_VHOST_PREALLOC_SGLS) {
-		pr_err("vhost_scsi_map_to_sgl() psgl_count: %u greater than"
-		       " preallocated TCM_VHOST_PREALLOC_SGLS: %u\n",
-			sgl_count, TCM_VHOST_PREALLOC_SGLS);
-		return -ENOBUFS;
-	}
-
 	pages_nr = iov_num_pages(iov);
-	if (pages_nr > sgl_count)
+	if (pages_nr > sgl_count) {
+		pr_err("vhost_scsi_map_to_sgl() pages_nr: %u greater than"
+		       " sgl_count: %u\n", pages_nr, sgl_count);
 		return -ENOBUFS;
-
-	if (pages_nr > TCM_VHOST_PREALLOC_PAGES) {
+	}
+	if (pages_nr > TCM_VHOST_PREALLOC_UPAGES) {
 		pr_err("vhost_scsi_map_to_sgl() pages_nr: %u greater than"
-		       " preallocated TCM_VHOST_PREALLOC_PAGES: %u\n",
-			pages_nr, TCM_VHOST_PREALLOC_PAGES);
+		       " preallocated TCM_VHOST_PREALLOC_UPAGES: %u\n",
+			pages_nr, TCM_VHOST_PREALLOC_UPAGES);
 		return -ENOBUFS;
 	}
 
-	pages = tv_cmd->tvc_upages;
-
 	ret = get_user_pages_fast((unsigned long)ptr, pages_nr, write, pages);
 	/* No pages were pinned */
 	if (ret < 0)
@@ -825,33 +818,32 @@ out:
 static int
 vhost_scsi_map_iov_to_sgl(struct tcm_vhost_cmd *cmd,
 			  struct iovec *iov,
-			  unsigned int niov,
-			  int write)
+			  int niov,
+			  bool write)
 {
-	int ret;
-	unsigned int i;
-	u32 sgl_count;
-	struct scatterlist *sg;
+	struct scatterlist *sg = cmd->tvc_sgl;
+	unsigned int sgl_count = 0;
+	int ret, i;
 
-	/*
-	 * Find out how long sglist needs to be
-	 */
-	sgl_count = 0;
 	for (i = 0; i < niov; i++)
 		sgl_count += iov_num_pages(&iov[i]);
 
-	/* TODO overflow checking */
+	if (sgl_count > TCM_VHOST_PREALLOC_SGLS) {
+		pr_err("vhost_scsi_map_iov_to_sgl() sgl_count: %u greater than"
+			" preallocated TCM_VHOST_PREALLOC_SGLS: %u\n",
+			sgl_count, TCM_VHOST_PREALLOC_SGLS);
+		return -ENOBUFS;
+	}
 
-	sg = cmd->tvc_sgl;
 	pr_debug("%s sg %p sgl_count %u\n", __func__, sg, sgl_count);
 	sg_init_table(sg, sgl_count);
-
 	cmd->tvc_sgl_count = sgl_count;
 
-	pr_debug("Mapping %u iovecs for %u pages\n", niov, sgl_count);
+	pr_debug("Mapping iovec %p for %u pages\n", &iov[0], sgl_count);
+
 	for (i = 0; i < niov; i++) {
 		ret = vhost_scsi_map_to_sgl(cmd, sg, sgl_count, &iov[i],
-					    write);
+					    cmd->tvc_upages, write);
 		if (ret < 0) {
 			for (i = 0; i < cmd->tvc_sgl_count; i++)
 				put_page(sg_page(&cmd->tvc_sgl[i]));
@@ -859,7 +851,6 @@ vhost_scsi_map_iov_to_sgl(struct tcm_vhost_cmd *cmd,
 			cmd->tvc_sgl_count = 0;
 			return ret;
 		}
-
 		sg += ret;
 		sgl_count -= ret;
 	}
@@ -1765,7 +1756,7 @@ static int tcm_vhost_make_nexus(struct tcm_vhost_tpg *tpg,
 		}
 
 		tv_cmd->tvc_upages = kzalloc(sizeof(struct page *) *
-					TCM_VHOST_PREALLOC_PAGES, GFP_KERNEL);
+					TCM_VHOST_PREALLOC_UPAGES, GFP_KERNEL);
 		if (!tv_cmd->tvc_upages) {
 			mutex_unlock(&tpg->tv_tpg_mutex);
 			pr_err("Unable to allocate tv_cmd->tvc_upages\n");
-- 
1.7.10.4

[PATCH 3/6] vhost/scsi: Add preallocation of protection SGLs

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch updates tcm_vhost_make_nexus() to pre-allocate per descriptor
tcm_vhost_cmd->tvc_prot_sgl[] used to expose protection SGLs from within
virtio-scsi guest memory to vhost-scsi.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/scsi.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index ae434db..30402e6 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -58,6 +58,7 @@
 #define TCM_VHOST_DEFAULT_TAGS 256
 #define TCM_VHOST_PREALLOC_SGLS 2048
 #define TCM_VHOST_PREALLOC_UPAGES 2048
+#define TCM_VHOST_PREALLOC_PROT_SGLS 512
 
 struct vhost_scsi_inflight {
 	/* Wait for the flush operation to finish */
@@ -83,6 +84,7 @@ struct tcm_vhost_cmd {
 	u32 tvc_lun;
 	/* Pointer to the SGL formatted memory from virtio-scsi */
 	struct scatterlist *tvc_sgl;
+	struct scatterlist *tvc_prot_sgl;
 	struct page **tvc_upages;
 	/* Pointer to response */
 	struct virtio_scsi_cmd_resp __user *tvc_resp;
@@ -722,7 +724,7 @@ vhost_scsi_get_tag(struct vhost_virtqueue *vq,
 	struct tcm_vhost_cmd *cmd;
 	struct tcm_vhost_nexus *tv_nexus;
 	struct se_session *se_sess;
-	struct scatterlist *sg;
+	struct scatterlist *sg, *prot_sg;
 	struct page **pages;
 	int tag;
 
@@ -741,10 +743,12 @@ vhost_scsi_get_tag(struct vhost_virtqueue *vq,
 
 	cmd = &((struct tcm_vhost_cmd *)se_sess->sess_cmd_map)[tag];
 	sg = cmd->tvc_sgl;
+	prot_sg = cmd->tvc_prot_sgl;
 	pages = cmd->tvc_upages;
 	memset(cmd, 0, sizeof(struct tcm_vhost_cmd));
 
 	cmd->tvc_sgl = sg;
+	cmd->tvc_prot_sgl = prot_sg;
 	cmd->tvc_upages = pages;
 	cmd->tvc_se_cmd.map_tag = tag;
 	cmd->tvc_tag = v_req->tag;
@@ -1703,6 +1707,7 @@ static void tcm_vhost_free_cmd_map_res(struct tcm_vhost_nexus *nexus,
 		tv_cmd = &((struct tcm_vhost_cmd *)se_sess->sess_cmd_map)[i];
 
 		kfree(tv_cmd->tvc_sgl);
+		kfree(tv_cmd->tvc_prot_sgl);
 		kfree(tv_cmd->tvc_upages);
 	}
 }
@@ -1762,6 +1767,14 @@ static int tcm_vhost_make_nexus(struct tcm_vhost_tpg *tpg,
 			pr_err("Unable to allocate tv_cmd->tvc_upages\n");
 			goto out;
 		}
+
+		tv_cmd->tvc_prot_sgl = kzalloc(sizeof(struct scatterlist) *
+					TCM_VHOST_PREALLOC_PROT_SGLS, GFP_KERNEL);
+		if (!tv_cmd->tvc_prot_sgl) {
+			mutex_unlock(&tpg->tv_tpg_mutex);
+			pr_err("Unable to allocate tv_cmd->tvc_prot_sgl\n");
+			goto out;
+		}
 	}
 	/*
 	 * Since we are running in 'demo mode' this call with generate a
-- 
1.7.10.4

[PATCH 4/6] vhost/scsi: Add T10 PI IOV -> SGL memory mapping logic

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds vhost_scsi_map_iov_to_prot() to perform the mapping of
T10 data integrity memory between virtio iov + struct scatterlist using
get_user_pages_fast() following existing code.

As with vhost_scsi_map_iov_to_sgl(), this does sanity checks against the
total prot_sgl_count vs. pre-allocated SGLs, and loops across protection
iovs using vhost_scsi_map_to_sgl() to perform the actual memory mapping.

Also update tcm_vhost_release_cmd() to release associated tvc_prot_sgl[]
struct page.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/scsi.c |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 30402e6..eabcf18 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -80,6 +80,7 @@ struct tcm_vhost_cmd {
 	u64 tvc_tag;
 	/* The number of scatterlists associated with this cmd */
 	u32 tvc_sgl_count;
+	u32 tvc_prot_sgl_count;
 	/* Saved unpacked SCSI LUN for tcm_vhost_submission_work() */
 	u32 tvc_lun;
 	/* Pointer to the SGL formatted memory from virtio-scsi */
@@ -458,12 +459,16 @@ static void tcm_vhost_release_cmd(struct se_cmd *se_cmd)
 	struct tcm_vhost_cmd *tv_cmd = container_of(se_cmd,
 				struct tcm_vhost_cmd, tvc_se_cmd);
 	struct se_session *se_sess = se_cmd->se_sess;
+	int i;
 
 	if (tv_cmd->tvc_sgl_count) {
-		u32 i;
 		for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
 			put_page(sg_page(&tv_cmd->tvc_sgl[i]));
 	}
+	if (tv_cmd->tvc_prot_sgl_count) {
+		for (i = 0; i < tv_cmd->tvc_prot_sgl_count; i++)
+			put_page(sg_page(&tv_cmd->tvc_prot_sgl[i]));
+	}
 
 	tcm_vhost_put_inflight(tv_cmd->inflight);
 	percpu_ida_free(&se_sess->sess_tag_pool, se_cmd->map_tag);
@@ -861,6 +866,47 @@ vhost_scsi_map_iov_to_sgl(struct tcm_vhost_cmd *cmd,
 	return 0;
 }
 
+static int
+vhost_scsi_map_iov_to_prot(struct tcm_vhost_cmd *cmd,
+			   struct iovec *iov,
+			   int niov,
+			   bool write)
+{
+	struct scatterlist *prot_sg = cmd->tvc_prot_sgl;
+	unsigned int prot_sgl_count = 0;
+	int ret, i;
+
+	for (i = 0; i < niov; i++)
+		prot_sgl_count += iov_num_pages(&iov[i]);
+
+	if (prot_sgl_count > TCM_VHOST_PREALLOC_PROT_SGLS) {
+		pr_err("vhost_scsi_map_iov_to_prot() sgl_count: %u greater than"
+			" preallocated TCM_VHOST_PREALLOC_PROT_SGLS: %u\n",
+			prot_sgl_count, TCM_VHOST_PREALLOC_PROT_SGLS);
+		return -ENOBUFS;
+	}
+
+	pr_debug("%s prot_sg %p prot_sgl_count %u\n", __func__,
+		 prot_sg, prot_sgl_count);
+	sg_init_table(prot_sg, prot_sgl_count);
+	cmd->tvc_prot_sgl_count = prot_sgl_count;
+
+	for (i = 0; i < niov; i++) {
+		ret = vhost_scsi_map_to_sgl(cmd, prot_sg, prot_sgl_count, &iov[i],
+					    cmd->tvc_upages, write);
+		if (ret < 0) {
+			for (i = 0; i < cmd->tvc_prot_sgl_count; i++)
+				put_page(sg_page(&cmd->tvc_prot_sgl[i]));
+
+			cmd->tvc_prot_sgl_count = 0;
+			return ret;
+		}
+		prot_sg += ret;
+		prot_sgl_count -= ret;
+	}
+	return 0;
+}
+
 static void tcm_vhost_submission_work(struct work_struct *work)
 {
 	struct tcm_vhost_cmd *cmd =
-- 
1.7.10.4

[PATCH 5/6] vhost/scsi: Enable T10 PI IOV -> SGL memory mapping

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch updates vhost_scsi_handle_vq() to check for the existance
of virtio_scsi_cmd_req_pi comparing vq->iov[0].iov_len in order to
calculate seperate data + protection SGLs from data_num.

Also update tcm_vhost_submission_work() to pass the pre-allocated
cmd->tvc_prot_sgl[] memory into target_submit_cmd_map_sgls(), and
update vhost_scsi_get_tag() parameters to accept scsi_tag, lun, and
task_attr.

v3 changes:
  - Use feature_bit for determining virtio-scsi header type (Paolo)

v2 changes:
  - Use virtio_scsi_cmd_req_pi instead of existing ->prio (Paolo)
  - Make protection buffer come before data buffer (Paolo)
  - Update vhost_scsi_get_tag() parameter usage

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/scsi.c |  179 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 121 insertions(+), 58 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index eabcf18..19cd21a 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -169,7 +169,8 @@ enum {
 };
 
 enum {
-	VHOST_SCSI_FEATURES = VHOST_FEATURES | (1ULL << VIRTIO_SCSI_F_HOTPLUG)
+	VHOST_SCSI_FEATURES = VHOST_FEATURES | (1ULL << VIRTIO_SCSI_F_HOTPLUG) |
+					       (1ULL << VIRTIO_SCSI_F_T10_PI)
 };
 
 #define VHOST_SCSI_MAX_TARGET	256
@@ -720,11 +721,9 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
 }
 
 static struct tcm_vhost_cmd *
-vhost_scsi_get_tag(struct vhost_virtqueue *vq,
-			struct tcm_vhost_tpg *tpg,
-			struct virtio_scsi_cmd_req *v_req,
-			u32 exp_data_len,
-			int data_direction)
+vhost_scsi_get_tag(struct vhost_virtqueue *vq, struct tcm_vhost_tpg *tpg,
+		   unsigned char *cdb, u64 scsi_tag, u16 lun, u8 task_attr,
+		   u32 exp_data_len, int data_direction)
 {
 	struct tcm_vhost_cmd *cmd;
 	struct tcm_vhost_nexus *tv_nexus;
@@ -756,13 +755,16 @@ vhost_scsi_get_tag(struct vhost_virtqueue *vq,
 	cmd->tvc_prot_sgl = prot_sg;
 	cmd->tvc_upages = pages;
 	cmd->tvc_se_cmd.map_tag = tag;
-	cmd->tvc_tag = v_req->tag;
-	cmd->tvc_task_attr = v_req->task_attr;
+	cmd->tvc_tag = scsi_tag;
+	cmd->tvc_lun = lun;
+	cmd->tvc_task_attr = task_attr;
 	cmd->tvc_exp_data_len = exp_data_len;
 	cmd->tvc_data_direction = data_direction;
 	cmd->tvc_nexus = tv_nexus;
 	cmd->inflight = tcm_vhost_get_inflight(vq);
 
+	memcpy(cmd->tvc_cdb, cdb, TCM_VHOST_MAX_CDB_SIZE);
+
 	return cmd;
 }
 
@@ -913,18 +915,15 @@ static void tcm_vhost_submission_work(struct work_struct *work)
 		container_of(work, struct tcm_vhost_cmd, work);
 	struct tcm_vhost_nexus *tv_nexus;
 	struct se_cmd *se_cmd = &cmd->tvc_se_cmd;
-	struct scatterlist *sg_ptr, *sg_bidi_ptr = NULL;
-	int rc, sg_no_bidi = 0;
+	struct scatterlist *sg_ptr, *sg_prot_ptr = NULL;
+	int rc;
 
+	/* FIXME: BIDI operation */
 	if (cmd->tvc_sgl_count) {
 		sg_ptr = cmd->tvc_sgl;
-/* FIXME: Fix BIDI operation in tcm_vhost_submission_work() */
-#if 0
-		if (se_cmd->se_cmd_flags & SCF_BIDI) {
-			sg_bidi_ptr = NULL;
-			sg_no_bidi = 0;
-		}
-#endif
+
+		if (cmd->tvc_prot_sgl_count)
+			sg_prot_ptr = cmd->tvc_prot_sgl;
 	} else {
 		sg_ptr = NULL;
 	}
@@ -935,7 +934,7 @@ static void tcm_vhost_submission_work(struct work_struct *work)
 			cmd->tvc_lun, cmd->tvc_exp_data_len,
 			cmd->tvc_task_attr, cmd->tvc_data_direction,
 			TARGET_SCF_ACK_KREF, sg_ptr, cmd->tvc_sgl_count,
-			sg_bidi_ptr, sg_no_bidi, NULL, 0);
+			NULL, 0, sg_prot_ptr, cmd->tvc_prot_sgl_count);
 	if (rc < 0) {
 		transport_send_check_condition_and_sense(se_cmd,
 				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
@@ -967,12 +966,18 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 {
 	struct tcm_vhost_tpg **vs_tpg;
 	struct virtio_scsi_cmd_req v_req;
+	struct virtio_scsi_cmd_req_pi v_req_pi;
 	struct tcm_vhost_tpg *tpg;
 	struct tcm_vhost_cmd *cmd;
-	u32 exp_data_len, data_first, data_num, data_direction;
+	u64 tag;
+	u32 exp_data_len, data_first, data_num, data_direction, prot_first;
 	unsigned out, in, i;
-	int head, ret;
-	u8 target;
+	int head, ret, data_niov, prot_niov;
+	size_t req_size;
+	u16 lun;
+	u8 *target, *lunp, task_attr;
+	bool hdr_pi;
+	unsigned char *req, *cdb;
 
 	mutex_lock(&vq->mutex);
 	/*
@@ -1003,7 +1008,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 			break;
 		}
 
-/* FIXME: BIDI operation */
+		/* FIXME: BIDI operation */
 		if (out == 1 && in == 1) {
 			data_direction = DMA_NONE;
 			data_first = 0;
@@ -1033,29 +1038,47 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 			break;
 		}
 
-		if (unlikely(vq->iov[0].iov_len != sizeof(v_req))) {
-			vq_err(vq, "Expecting virtio_scsi_cmd_req, got %zu"
-				" bytes\n", vq->iov[0].iov_len);
-			break;
+		if (vs->dev.acked_features & VIRTIO_SCSI_F_T10_PI) {
+			if (unlikely(vq->iov[0].iov_len != sizeof(v_req_pi))) {
+				pr_err("Expecting virtio_scsi_cmd_req_pi: %zu,"
+				       " got %zu\n", sizeof(v_req_pi),
+				       vq->iov[0].iov_len);
+				break;
+			}
+			req = (unsigned char *)&v_req_pi;
+			lunp = &v_req_pi.lun[0];
+			target = &v_req_pi.lun[1];
+			req_size = sizeof(v_req_pi);
+			hdr_pi = true;
+		} else {
+			if (unlikely(vq->iov[0].iov_len != sizeof(v_req))) {
+				pr_err("Expecting virtio_scsi_cmd_req: %zu,"
+				       " got %zu\n", sizeof(v_req),
+				       vq->iov[0].iov_len);
+				break;
+			}
+			req = (unsigned char *)&v_req;
+			lunp = &v_req.lun[0];
+			target = &v_req.lun[1];
+			req_size = sizeof(v_req);
+			hdr_pi = false;
 		}
+
 		pr_debug("Calling __copy_from_user: vq->iov[0].iov_base: %p,"
-			" len: %zu\n", vq->iov[0].iov_base, sizeof(v_req));
-		ret = __copy_from_user(&v_req, vq->iov[0].iov_base,
-				sizeof(v_req));
+			 " len: %zu\n", vq->iov[0].iov_base, req_size);
+		ret = __copy_from_user(req, vq->iov[0].iov_base, req_size);
 		if (unlikely(ret)) {
 			vq_err(vq, "Faulted on virtio_scsi_cmd_req\n");
 			break;
 		}
 
 		/* virtio-scsi spec requires byte 0 of the lun to be 1 */
-		if (unlikely(v_req.lun[0] != 1)) {
+		if (unlikely(*lunp != 1)) {
 			vhost_scsi_send_bad_target(vs, vq, head, out);
 			continue;
 		}
 
-		/* Extract the tpgt */
-		target = v_req.lun[1];
-		tpg = ACCESS_ONCE(vs_tpg[target]);
+		tpg = ACCESS_ONCE(vs_tpg[*target]);
 
 		/* Target does not exist, fail the request */
 		if (unlikely(!tpg)) {
@@ -1063,17 +1086,69 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 			continue;
 		}
 
+		data_niov = data_num;
+		prot_niov = prot_first = 0;
+		/*
+		 * Determine if any proteciton information iovecs are preceeding
+		 * the actual data payload, and adjust data_niov + data_first
+		 *
+		 * Also extract virtio_scsi header bits for vhost_scsi_get_tag()
+		 */
+		if (data_num && hdr_pi) {
+			if (v_req_pi.do_pi_niov) {
+				if (data_direction != DMA_TO_DEVICE) {
+					vq_err(vq, "Received non zero do_pi_niov"
+						", but wrong data_direction\n");
+					goto err_cmd;
+				}
+				prot_niov = v_req_pi.do_pi_niov;
+			} else if (v_req_pi.di_pi_niov) {
+				if (data_direction != DMA_FROM_DEVICE) {
+					vq_err(vq, "Received non zero di_pi_niov"
+						", but wrong data_direction\n");
+					goto err_cmd;
+				}
+				prot_niov = v_req_pi.di_pi_niov;
+			}
+
+			data_niov = data_num - prot_niov;
+			prot_first = data_first;
+			data_first += prot_niov;
+
+			tag = 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 = 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;
+		}
 		exp_data_len = 0;
-		for (i = 0; i < data_num; i++)
+		for (i = 0; i < data_niov; i++)
 			exp_data_len += vq->iov[data_first + i].iov_len;
+		/*
+		 * Check that the recieved CDB size does not exceeded our
+		 * hardcoded max for vhost-scsi
+		 *
+		 * 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);
+			goto err_cmd;
+		}
 
-		cmd = vhost_scsi_get_tag(vq, tpg, &v_req,
+		cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr,
 					 exp_data_len, data_direction);
 		if (IS_ERR(cmd)) {
 			vq_err(vq, "vhost_scsi_get_tag failed %ld\n",
 					PTR_ERR(cmd));
 			goto err_cmd;
 		}
+
 		pr_debug("Allocated tv_cmd: %p exp_data_len: %d, data_direction"
 			": %d\n", cmd, exp_data_len, data_direction);
 
@@ -1081,40 +1156,28 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 		cmd->tvc_vq = vq;
 		cmd->tvc_resp = vq->iov[out].iov_base;
 
-		/*
-		 * Copy in the recieved CDB descriptor into cmd->tvc_cdb
-		 * that will be used by tcm_vhost_new_cmd_map() and down into
-		 * target_setup_cmd_from_cdb()
-		 */
-		memcpy(cmd->tvc_cdb, v_req.cdb, TCM_VHOST_MAX_CDB_SIZE);
-		/*
-		 * Check that the recieved CDB size does not exceeded our
-		 * hardcoded max for tcm_vhost
-		 */
-		/* TODO what if cdb was too small for varlen cdb header? */
-		if (unlikely(scsi_command_size(cmd->tvc_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(cmd->tvc_cdb),
-				TCM_VHOST_MAX_CDB_SIZE);
-			goto err_free;
-		}
-		cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
-
 		pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
 			cmd->tvc_cdb[0], cmd->tvc_lun);
 
+		if (prot_niov) {
+			ret = vhost_scsi_map_iov_to_prot(cmd,
+					&vq->iov[prot_first], prot_niov,
+					data_direction == DMA_FROM_DEVICE);
+			if (unlikely(ret)) {
+				vq_err(vq, "Failed to map iov to"
+					" prot_sgl\n");
+				goto err_free;
+			}
+		}
 		if (data_direction != DMA_NONE) {
 			ret = vhost_scsi_map_iov_to_sgl(cmd,
-					&vq->iov[data_first], data_num,
+					&vq->iov[data_first], data_niov,
 					data_direction == DMA_FROM_DEVICE);
 			if (unlikely(ret)) {
 				vq_err(vq, "Failed to map iov to sgl\n");
 				goto err_free;
 			}
 		}
-
 		/*
 		 * Save the descriptor from vhost_get_vq_desc() to be used to
 		 * complete the virtio-scsi request in TCM callback context via
-- 
1.7.10.4

Re: [PATCH 5/6] vhost/scsi: Enable T10 PI IOV -> SGL memory mapping

[Michael S. Tsirkin]

On Sun, Apr 06, 2014 at 09:32:08PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch updates vhost_scsi_handle_vq() to check for the existance
> of virtio_scsi_cmd_req_pi comparing vq->iov[0].iov_len in order to
> calculate seperate data + protection SGLs from data_num.
> 
> Also update tcm_vhost_submission_work() to pass the pre-allocated
> cmd->tvc_prot_sgl[] memory into target_submit_cmd_map_sgls(), and
> update vhost_scsi_get_tag() parameters to accept scsi_tag, lun, and
> task_attr.
> 
> v3 changes:
>   - Use feature_bit for determining virtio-scsi header type (Paolo)
> 
> v2 changes:
>   - Use virtio_scsi_cmd_req_pi instead of existing ->prio (Paolo)
>   - Make protection buffer come before data buffer (Paolo)
>   - Update vhost_scsi_get_tag() parameter usage
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/vhost/scsi.c |  179 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 121 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index eabcf18..19cd21a 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -169,7 +169,8 @@ enum {
>  };
>  
>  enum {
> -	VHOST_SCSI_FEATURES = VHOST_FEATURES | (1ULL << VIRTIO_SCSI_F_HOTPLUG)
> +	VHOST_SCSI_FEATURES = VHOST_FEATURES | (1ULL << VIRTIO_SCSI_F_HOTPLUG) |
> +					       (1ULL << VIRTIO_SCSI_F_T10_PI)
>  };
>  
>  #define VHOST_SCSI_MAX_TARGET	256
> @@ -720,11 +721,9 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
>  }
>  
>  static struct tcm_vhost_cmd *
> -vhost_scsi_get_tag(struct vhost_virtqueue *vq,
> -			struct tcm_vhost_tpg *tpg,
> -			struct virtio_scsi_cmd_req *v_req,
> -			u32 exp_data_len,
> -			int data_direction)
> +vhost_scsi_get_tag(struct vhost_virtqueue *vq, struct tcm_vhost_tpg *tpg,
> +		   unsigned char *cdb, u64 scsi_tag, u16 lun, u8 task_attr,
> +		   u32 exp_data_len, int data_direction)
>  {
>  	struct tcm_vhost_cmd *cmd;
>  	struct tcm_vhost_nexus *tv_nexus;
> @@ -756,13 +755,16 @@ vhost_scsi_get_tag(struct vhost_virtqueue *vq,
>  	cmd->tvc_prot_sgl = prot_sg;
>  	cmd->tvc_upages = pages;
>  	cmd->tvc_se_cmd.map_tag = tag;
> -	cmd->tvc_tag = v_req->tag;
> -	cmd->tvc_task_attr = v_req->task_attr;
> +	cmd->tvc_tag = scsi_tag;
> +	cmd->tvc_lun = lun;
> +	cmd->tvc_task_attr = task_attr;
>  	cmd->tvc_exp_data_len = exp_data_len;
>  	cmd->tvc_data_direction = data_direction;
>  	cmd->tvc_nexus = tv_nexus;
>  	cmd->inflight = tcm_vhost_get_inflight(vq);
>  
> +	memcpy(cmd->tvc_cdb, cdb, TCM_VHOST_MAX_CDB_SIZE);
> +
>  	return cmd;
>  }
>  
> @@ -913,18 +915,15 @@ static void tcm_vhost_submission_work(struct work_struct *work)
>  		container_of(work, struct tcm_vhost_cmd, work);
>  	struct tcm_vhost_nexus *tv_nexus;
>  	struct se_cmd *se_cmd = &cmd->tvc_se_cmd;
> -	struct scatterlist *sg_ptr, *sg_bidi_ptr = NULL;
> -	int rc, sg_no_bidi = 0;
> +	struct scatterlist *sg_ptr, *sg_prot_ptr = NULL;
> +	int rc;
>  
> +	/* FIXME: BIDI operation */
>  	if (cmd->tvc_sgl_count) {
>  		sg_ptr = cmd->tvc_sgl;
> -/* FIXME: Fix BIDI operation in tcm_vhost_submission_work() */
> -#if 0
> -		if (se_cmd->se_cmd_flags & SCF_BIDI) {
> -			sg_bidi_ptr = NULL;
> -			sg_no_bidi = 0;
> -		}
> -#endif
> +
> +		if (cmd->tvc_prot_sgl_count)
> +			sg_prot_ptr = cmd->tvc_prot_sgl;
>  	} else {
>  		sg_ptr = NULL;
>  	}
> @@ -935,7 +934,7 @@ static void tcm_vhost_submission_work(struct work_struct *work)
>  			cmd->tvc_lun, cmd->tvc_exp_data_len,
>  			cmd->tvc_task_attr, cmd->tvc_data_direction,
>  			TARGET_SCF_ACK_KREF, sg_ptr, cmd->tvc_sgl_count,
> -			sg_bidi_ptr, sg_no_bidi, NULL, 0);
> +			NULL, 0, sg_prot_ptr, cmd->tvc_prot_sgl_count);
>  	if (rc < 0) {
>  		transport_send_check_condition_and_sense(se_cmd,
>  				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
> @@ -967,12 +966,18 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
>  {
>  	struct tcm_vhost_tpg **vs_tpg;
>  	struct virtio_scsi_cmd_req v_req;
> +	struct virtio_scsi_cmd_req_pi v_req_pi;
>  	struct tcm_vhost_tpg *tpg;
>  	struct tcm_vhost_cmd *cmd;
> -	u32 exp_data_len, data_first, data_num, data_direction;
> +	u64 tag;
> +	u32 exp_data_len, data_first, data_num, data_direction, prot_first;
>  	unsigned out, in, i;
> -	int head, ret;
> -	u8 target;
> +	int head, ret, data_niov, prot_niov;
> +	size_t req_size;
> +	u16 lun;
> +	u8 *target, *lunp, task_attr;
> +	bool hdr_pi;
> +	unsigned char *req, *cdb;

Make req void *, then we won't need the casts?

>  
>  	mutex_lock(&vq->mutex);
>  	/*
> @@ -1003,7 +1008,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
>  			break;
>  		}
>  
> -/* FIXME: BIDI operation */
> +		/* FIXME: BIDI operation */
>  		if (out == 1 && in == 1) {
>  			data_direction = DMA_NONE;
>  			data_first = 0;
> @@ -1033,29 +1038,47 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
>  			break;
>  		}
>  
> -		if (unlikely(vq->iov[0].iov_len != sizeof(v_req))) {
> -			vq_err(vq, "Expecting virtio_scsi_cmd_req, got %zu"
> -				" bytes\n", vq->iov[0].iov_len);
> -			break;
> +		if (vs->dev.acked_features & VIRTIO_SCSI_F_T10_PI) {
> +			if (unlikely(vq->iov[0].iov_len != sizeof(v_req_pi))) {
> +				pr_err("Expecting virtio_scsi_cmd_req_pi: %zu,"
> +				       " got %zu\n", sizeof(v_req_pi),
> +				       vq->iov[0].iov_len);
> +				break;
> +			}

Hmm still relying on a specific IOV layout I see :(
It's really a violation of the spec even though it works
fine with Linux guests.
I know this isn't the only place this is broken but
can't we finally fix it?
Since you are touching this code anyway - how about
not adding more code we need to fix later?

It's not too hard - just verify total length is big enough
(instead of an exact match), then call
memcpy_fromiovecend/memcpy_toiovecend.
(or memcpy_fromiovec if you don't mind that iovec is modified).

This will help make sure
we are not making interface mistakes like we did for block
with VIRTIO_BLK_T_SCSI_CMD.


Once vhost scsi code will be clean in this respect, we can set
VIRTIO_F_ANY_LAYOUT to signal this to userspace.


> +			req = (unsigned char *)&v_req_pi;
> +			lunp = &v_req_pi.lun[0];
> +			target = &v_req_pi.lun[1];
> +			req_size = sizeof(v_req_pi);
> +			hdr_pi = true;
> +		} else {
> +			if (unlikely(vq->iov[0].iov_len != sizeof(v_req))) {
> +				pr_err("Expecting virtio_scsi_cmd_req: %zu,"
> +				       " got %zu\n", sizeof(v_req),
> +				       vq->iov[0].iov_len);
> +				break;
> +			}
> +			req = (unsigned char *)&v_req;
> +			lunp = &v_req.lun[0];
> +			target = &v_req.lun[1];
> +			req_size = sizeof(v_req);
> +			hdr_pi = false;
>  		}
> +
>  		pr_debug("Calling __copy_from_user: vq->iov[0].iov_base: %p,"
> -			" len: %zu\n", vq->iov[0].iov_base, sizeof(v_req));
> -		ret = __copy_from_user(&v_req, vq->iov[0].iov_base,
> -				sizeof(v_req));
> +			 " len: %zu\n", vq->iov[0].iov_base, req_size);
> +		ret = __copy_from_user(req, vq->iov[0].iov_base, req_size);
>  		if (unlikely(ret)) {
>  			vq_err(vq, "Faulted on virtio_scsi_cmd_req\n");
>  			break;
>  		}
>  
>  		/* virtio-scsi spec requires byte 0 of the lun to be 1 */
> -		if (unlikely(v_req.lun[0] != 1)) {
> +		if (unlikely(*lunp != 1)) {
>  			vhost_scsi_send_bad_target(vs, vq, head, out);
>  			continue;
>  		}
>  
> -		/* Extract the tpgt */
> -		target = v_req.lun[1];
> -		tpg = ACCESS_ONCE(vs_tpg[target]);
> +		tpg = ACCESS_ONCE(vs_tpg[*target]);
>  
>  		/* Target does not exist, fail the request */
>  		if (unlikely(!tpg)) {
> @@ -1063,17 +1086,69 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
>  			continue;
>  		}
>  
> +		data_niov = data_num;
> +		prot_niov = prot_first = 0;
> +		/*
> +		 * Determine if any proteciton information iovecs are preceeding
> +		 * the actual data payload, and adjust data_niov + data_first
> +		 *
> +		 * Also extract virtio_scsi header bits for vhost_scsi_get_tag()
> +		 */
> +		if (data_num && hdr_pi) {
> +			if (v_req_pi.do_pi_niov) {
> +				if (data_direction != DMA_TO_DEVICE) {
> +					vq_err(vq, "Received non zero do_pi_niov"
> +						", but wrong data_direction\n");
> +					goto err_cmd;
> +				}
> +				prot_niov = v_req_pi.do_pi_niov;
> +			} else if (v_req_pi.di_pi_niov) {
> +				if (data_direction != DMA_FROM_DEVICE) {
> +					vq_err(vq, "Received non zero di_pi_niov"
> +						", but wrong data_direction\n");
> +					goto err_cmd;
> +				}
> +				prot_niov = v_req_pi.di_pi_niov;
> +			}
> +
> +			data_niov = data_num - prot_niov;
> +			prot_first = data_first;
> +			data_first += prot_niov;
> +
> +			tag = 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 = 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;
> +		}
>  		exp_data_len = 0;
> -		for (i = 0; i < data_num; i++)
> +		for (i = 0; i < data_niov; i++)
>  			exp_data_len += vq->iov[data_first + i].iov_len;
> +		/*
> +		 * Check that the recieved CDB size does not exceeded our
> +		 * hardcoded max for vhost-scsi
> +		 *
> +		 * 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);
> +			goto err_cmd;
> +		}
>  
> -		cmd = vhost_scsi_get_tag(vq, tpg, &v_req,
> +		cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr,
>  					 exp_data_len, data_direction);
>  		if (IS_ERR(cmd)) {
>  			vq_err(vq, "vhost_scsi_get_tag failed %ld\n",
>  					PTR_ERR(cmd));
>  			goto err_cmd;
>  		}
> +
>  		pr_debug("Allocated tv_cmd: %p exp_data_len: %d, data_direction"
>  			": %d\n", cmd, exp_data_len, data_direction);
>  
> @@ -1081,40 +1156,28 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
>  		cmd->tvc_vq = vq;
>  		cmd->tvc_resp = vq->iov[out].iov_base;
>  
> -		/*
> -		 * Copy in the recieved CDB descriptor into cmd->tvc_cdb
> -		 * that will be used by tcm_vhost_new_cmd_map() and down into
> -		 * target_setup_cmd_from_cdb()
> -		 */
> -		memcpy(cmd->tvc_cdb, v_req.cdb, TCM_VHOST_MAX_CDB_SIZE);
> -		/*
> -		 * Check that the recieved CDB size does not exceeded our
> -		 * hardcoded max for tcm_vhost
> -		 */
> -		/* TODO what if cdb was too small for varlen cdb header? */
> -		if (unlikely(scsi_command_size(cmd->tvc_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(cmd->tvc_cdb),
> -				TCM_VHOST_MAX_CDB_SIZE);
> -			goto err_free;
> -		}
> -		cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
> -
>  		pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
>  			cmd->tvc_cdb[0], cmd->tvc_lun);
>  
> +		if (prot_niov) {
> +			ret = vhost_scsi_map_iov_to_prot(cmd,
> +					&vq->iov[prot_first], prot_niov,
> +					data_direction == DMA_FROM_DEVICE);
> +			if (unlikely(ret)) {
> +				vq_err(vq, "Failed to map iov to"
> +					" prot_sgl\n");
> +				goto err_free;
> +			}
> +		}
>  		if (data_direction != DMA_NONE) {
>  			ret = vhost_scsi_map_iov_to_sgl(cmd,
> -					&vq->iov[data_first], data_num,
> +					&vq->iov[data_first], data_niov,
>  					data_direction == DMA_FROM_DEVICE);
>  			if (unlikely(ret)) {
>  				vq_err(vq, "Failed to map iov to sgl\n");
>  				goto err_free;
>  			}
>  		}
> -
>  		/*
>  		 * Save the descriptor from vhost_get_vq_desc() to be used to
>  		 * complete the virtio-scsi request in TCM callback context via
> -- 
> 1.7.10.4

Re: [PATCH 5/6] vhost/scsi: Enable T10 PI IOV -> SGL memory mapping

[Paolo Bonzini]

Il 07/04/2014 05:16, Michael S. Tsirkin ha scritto:
> Hmm still relying on a specific IOV layout I see :(
> It's really a violation of the spec even though it works
> fine with Linux guests.
> I know this isn't the only place this is broken but
> can't we finally fix it?
> Since you are touching this code anyway - how about
> not adding more code we need to fix later?
>
> It's not too hard - just verify total length is big enough
> (instead of an exact match), then call
> memcpy_fromiovecend/memcpy_toiovecend.
> (or memcpy_fromiovec if you don't mind that iovec is modified).
>
> This will help make sure
> we are not making interface mistakes like we did for block
> with VIRTIO_BLK_T_SCSI_CMD.

And like we were about to make here. :(

We really should fix this for kernel 3.16 and QEMU 2.1.

Paolo

[PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch updates virtscsi_probe() to setup necessary Scsi_Host
level protection resources. (currently hardcoded to 1)

It changes virtscsi_add_cmd() to attach outgoing / incoming
protection SGLs preceeding the data payload, and is using the
new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
to signal to vhost/scsi how many prot_sgs to expect.

v3 changes:
  - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)

v2 changes:
  - Make protection buffer come before data buffer (Paolo)
  - Enable virtio_scsi_cmd_req_pi usage (Paolo)

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/virtio_scsi.c |   78 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 16bfd50..68d8d1b 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -37,6 +37,7 @@ struct virtio_scsi_cmd {
 	struct completion *comp;
 	union {
 		struct virtio_scsi_cmd_req       cmd;
+		struct virtio_scsi_cmd_req_pi    cmd_pi;
 		struct virtio_scsi_ctrl_tmf_req  tmf;
 		struct virtio_scsi_ctrl_an_req   an;
 	} req;
@@ -440,7 +441,7 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
 			    size_t req_size, size_t resp_size, gfp_t gfp)
 {
 	struct scsi_cmnd *sc = cmd->sc;
-	struct scatterlist *sgs[4], req, resp;
+	struct scatterlist *sgs[6], req, resp;
 	struct sg_table *out, *in;
 	unsigned out_num = 0, in_num = 0;
 
@@ -458,16 +459,24 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
 	sgs[out_num++] = &req;
 
 	/* Data-out buffer.  */
-	if (out)
+	if (out) {
+		/* Place WRITE protection SGLs before Data OUT payload */
+		if (scsi_prot_sg_count(sc))
+			sgs[out_num++] = scsi_prot_sglist(sc);
 		sgs[out_num++] = out->sgl;
+	}
 
 	/* Response header.  */
 	sg_init_one(&resp, &cmd->resp, resp_size);
 	sgs[out_num + in_num++] = &resp;
 
 	/* Data-in buffer */
-	if (in)
+	if (in) {
+		/* Place READ protection SGLs before Data IN payload */
+		if (scsi_prot_sg_count(sc))
+			sgs[out_num + in_num++] = scsi_prot_sglist(sc);
 		sgs[out_num + in_num++] = in->sgl;
+	}
 
 	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
 }
@@ -492,12 +501,36 @@ static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
 	return err;
 }
 
+static void virtio_scsi_init_hdr(struct virtio_scsi_cmd_req *cmd,
+				 struct scsi_cmnd *sc)
+{
+	cmd->lun[0] = 1;
+	cmd->lun[1] = sc->device->id;
+	cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
+	cmd->lun[3] = sc->device->lun & 0xff;
+	cmd->tag = (unsigned long)sc;
+	cmd->task_attr = VIRTIO_SCSI_S_SIMPLE;
+	cmd->prio = 0;
+	cmd->crn = 0;
+}
+
+static void virtio_scsi_init_hdr_pi(struct virtio_scsi_cmd_req_pi *cmd_pi,
+				    struct scsi_cmnd *sc)
+{
+	virtio_scsi_init_hdr((struct virtio_scsi_cmd_req *)cmd_pi, sc);
+
+	if (sc->sc_data_direction == DMA_TO_DEVICE)
+		cmd_pi->do_pi_niov = scsi_prot_sg_count(sc);
+	else if (sc->sc_data_direction == DMA_FROM_DEVICE)
+		cmd_pi->di_pi_niov = scsi_prot_sg_count(sc);
+}
+
 static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
 				 struct virtio_scsi_vq *req_vq,
 				 struct scsi_cmnd *sc)
 {
 	struct virtio_scsi_cmd *cmd;
-	int ret;
+	int ret, req_size;
 
 	struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
 	BUG_ON(scsi_sg_count(sc) > shost->sg_tablesize);
@@ -515,22 +548,20 @@ static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
 
 	memset(cmd, 0, sizeof(*cmd));
 	cmd->sc = sc;
-	cmd->req.cmd = (struct virtio_scsi_cmd_req){
-		.lun[0] = 1,
-		.lun[1] = sc->device->id,
-		.lun[2] = (sc->device->lun >> 8) | 0x40,
-		.lun[3] = sc->device->lun & 0xff,
-		.tag = (unsigned long)sc,
-		.task_attr = VIRTIO_SCSI_S_SIMPLE,
-		.prio = 0,
-		.crn = 0,
-	};
 
 	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
-	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
 
-	if (virtscsi_kick_cmd(req_vq, cmd,
-			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
+	if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_T10_PI)) {
+		virtio_scsi_init_hdr_pi(&cmd->req.cmd_pi, sc);
+		memcpy(cmd->req.cmd_pi.cdb, sc->cmnd, sc->cmd_len);
+		req_size = sizeof(cmd->req.cmd_pi);
+	} else {
+		virtio_scsi_init_hdr(&cmd->req.cmd, sc);
+		memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
+		req_size = sizeof(cmd->req.cmd);
+	}
+
+	if (virtscsi_kick_cmd(req_vq, cmd, req_size, sizeof(cmd->resp.cmd),
 			      GFP_ATOMIC) == 0)
 		ret = 0;
 	else
@@ -871,7 +902,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
 {
 	struct Scsi_Host *shost;
 	struct virtio_scsi *vscsi;
-	int err;
+	int err, host_prot;
 	u32 sg_elems, num_targets;
 	u32 cmd_per_lun;
 	u32 num_queues;
@@ -921,6 +952,16 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	shost->max_id = num_targets;
 	shost->max_channel = 0;
 	shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
+
+	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {
+		host_prot = SHOST_DIF_TYPE1_PROTECTION | SHOST_DIF_TYPE2_PROTECTION |
+			    SHOST_DIF_TYPE3_PROTECTION | SHOST_DIX_TYPE1_PROTECTION |
+			    SHOST_DIX_TYPE2_PROTECTION | SHOST_DIX_TYPE3_PROTECTION;
+
+		scsi_host_set_prot(shost, host_prot);
+		scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
+	}
+
 	err = scsi_add_host(shost, &vdev->dev);
 	if (err)
 		goto scsi_add_host_failed;
@@ -990,6 +1031,7 @@ static struct virtio_device_id id_table[] = {
 static unsigned int features[] = {
 	VIRTIO_SCSI_F_HOTPLUG,
 	VIRTIO_SCSI_F_CHANGE,
+	VIRTIO_SCSI_F_T10_PI,
 };
 
 static struct virtio_driver virtio_scsi_driver = {
-- 
1.7.10.4

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Sagi Grimberg]

On 4/7/2014 12:32 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch updates virtscsi_probe() to setup necessary Scsi_Host
> level protection resources. (currently hardcoded to 1)
>
> It changes virtscsi_add_cmd() to attach outgoing / incoming
> protection SGLs preceeding the data payload, and is using the
> new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> to signal to vhost/scsi how many prot_sgs to expect.
>
> v3 changes:
>    - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
>
> v2 changes:
>    - Make protection buffer come before data buffer (Paolo)
>    - Enable virtio_scsi_cmd_req_pi usage (Paolo)
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/scsi/virtio_scsi.c |   78 ++++++++++++++++++++++++++++++++++----------
>   1 file changed, 60 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 16bfd50..68d8d1b 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -37,6 +37,7 @@ struct virtio_scsi_cmd {
>   	struct completion *comp;
>   	union {
>   		struct virtio_scsi_cmd_req       cmd;
> +		struct virtio_scsi_cmd_req_pi    cmd_pi;
>   		struct virtio_scsi_ctrl_tmf_req  tmf;
>   		struct virtio_scsi_ctrl_an_req   an;
>   	} req;
> @@ -440,7 +441,7 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
>   			    size_t req_size, size_t resp_size, gfp_t gfp)
>   {
>   	struct scsi_cmnd *sc = cmd->sc;
> -	struct scatterlist *sgs[4], req, resp;
> +	struct scatterlist *sgs[6], req, resp;
>   	struct sg_table *out, *in;
>   	unsigned out_num = 0, in_num = 0;
>   
> @@ -458,16 +459,24 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
>   	sgs[out_num++] = &req;
>   
>   	/* Data-out buffer.  */
> -	if (out)
> +	if (out) {
> +		/* Place WRITE protection SGLs before Data OUT payload */
> +		if (scsi_prot_sg_count(sc))

Didn't we agree that the LLD should not discard the scmnd prot_op? Martin?

> +			sgs[out_num++] = scsi_prot_sglist(sc);
>   		sgs[out_num++] = out->sgl;
> +	}
>   
>   	/* Response header.  */
>   	sg_init_one(&resp, &cmd->resp, resp_size);
>   	sgs[out_num + in_num++] = &resp;
>   
>   	/* Data-in buffer */
> -	if (in)
> +	if (in) {
> +		/* Place READ protection SGLs before Data IN payload */
> +		if (scsi_prot_sg_count(sc))

Same here.

> +			sgs[out_num + in_num++] = scsi_prot_sglist(sc);
>   		sgs[out_num + in_num++] = in->sgl;
> +	}
>   
>   	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
>   }
> @@ -492,12 +501,36 @@ static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
>   	return err;
>   }
>   
> +static void virtio_scsi_init_hdr(struct virtio_scsi_cmd_req *cmd,
> +				 struct scsi_cmnd *sc)
> +{
> +	cmd->lun[0] = 1;
> +	cmd->lun[1] = sc->device->id;
> +	cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
> +	cmd->lun[3] = sc->device->lun & 0xff;
> +	cmd->tag = (unsigned long)sc;
> +	cmd->task_attr = VIRTIO_SCSI_S_SIMPLE;
> +	cmd->prio = 0;
> +	cmd->crn = 0;
> +}
> +
> +static void virtio_scsi_init_hdr_pi(struct virtio_scsi_cmd_req_pi *cmd_pi,
> +				    struct scsi_cmnd *sc)
> +{
> +	virtio_scsi_init_hdr((struct virtio_scsi_cmd_req *)cmd_pi, sc);
> +
> +	if (sc->sc_data_direction == DMA_TO_DEVICE)
> +		cmd_pi->do_pi_niov = scsi_prot_sg_count(sc);
> +	else if (sc->sc_data_direction == DMA_FROM_DEVICE)
> +		cmd_pi->di_pi_niov = scsi_prot_sg_count(sc);
> +}
> +
>   static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
>   				 struct virtio_scsi_vq *req_vq,
>   				 struct scsi_cmnd *sc)
>   {
>   	struct virtio_scsi_cmd *cmd;
> -	int ret;
> +	int ret, req_size;
>   
>   	struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
>   	BUG_ON(scsi_sg_count(sc) > shost->sg_tablesize);
> @@ -515,22 +548,20 @@ static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
>   
>   	memset(cmd, 0, sizeof(*cmd));
>   	cmd->sc = sc;
> -	cmd->req.cmd = (struct virtio_scsi_cmd_req){
> -		.lun[0] = 1,
> -		.lun[1] = sc->device->id,
> -		.lun[2] = (sc->device->lun >> 8) | 0x40,
> -		.lun[3] = sc->device->lun & 0xff,
> -		.tag = (unsigned long)sc,
> -		.task_attr = VIRTIO_SCSI_S_SIMPLE,
> -		.prio = 0,
> -		.crn = 0,
> -	};
>   
>   	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
> -	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
>   
> -	if (virtscsi_kick_cmd(req_vq, cmd,
> -			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
> +	if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_T10_PI)) {
> +		virtio_scsi_init_hdr_pi(&cmd->req.cmd_pi, sc);
> +		memcpy(cmd->req.cmd_pi.cdb, sc->cmnd, sc->cmd_len);
> +		req_size = sizeof(cmd->req.cmd_pi);
> +	} else {
> +		virtio_scsi_init_hdr(&cmd->req.cmd, sc);
> +		memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
> +		req_size = sizeof(cmd->req.cmd);
> +	}
> +
> +	if (virtscsi_kick_cmd(req_vq, cmd, req_size, sizeof(cmd->resp.cmd),
>   			      GFP_ATOMIC) == 0)
>   		ret = 0;
>   	else
> @@ -871,7 +902,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
>   {
>   	struct Scsi_Host *shost;
>   	struct virtio_scsi *vscsi;
> -	int err;
> +	int err, host_prot;
>   	u32 sg_elems, num_targets;
>   	u32 cmd_per_lun;
>   	u32 num_queues;
> @@ -921,6 +952,16 @@ static int virtscsi_probe(struct virtio_device *vdev)
>   	shost->max_id = num_targets;
>   	shost->max_channel = 0;
>   	shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
> +
> +	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {
> +		host_prot = SHOST_DIF_TYPE1_PROTECTION | SHOST_DIF_TYPE2_PROTECTION |
> +			    SHOST_DIF_TYPE3_PROTECTION | SHOST_DIX_TYPE1_PROTECTION |
> +			    SHOST_DIX_TYPE2_PROTECTION | SHOST_DIX_TYPE3_PROTECTION;
> +
> +		scsi_host_set_prot(shost, host_prot);
> +		scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
> +	}
> +
>   	err = scsi_add_host(shost, &vdev->dev);
>   	if (err)
>   		goto scsi_add_host_failed;
> @@ -990,6 +1031,7 @@ static struct virtio_device_id id_table[] = {
>   static unsigned int features[] = {
>   	VIRTIO_SCSI_F_HOTPLUG,
>   	VIRTIO_SCSI_F_CHANGE,
> +	VIRTIO_SCSI_F_T10_PI,
>   };
>   
>   static struct virtio_driver virtio_scsi_driver = {

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 11:03 +0300, Sagi Grimberg wrote:
> On 4/7/2014 12:32 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > level protection resources. (currently hardcoded to 1)
> >
> > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > protection SGLs preceeding the data payload, and is using the
> > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > to signal to vhost/scsi how many prot_sgs to expect.
> >
> > v3 changes:
> >    - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> >
> > v2 changes:
> >    - Make protection buffer come before data buffer (Paolo)
> >    - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/scsi/virtio_scsi.c |   78 ++++++++++++++++++++++++++++++++++----------
> >   1 file changed, 60 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> > index 16bfd50..68d8d1b 100644
> > --- a/drivers/scsi/virtio_scsi.c
> > +++ b/drivers/scsi/virtio_scsi.c
> > @@ -37,6 +37,7 @@ struct virtio_scsi_cmd {
> >   	struct completion *comp;
> >   	union {
> >   		struct virtio_scsi_cmd_req       cmd;
> > +		struct virtio_scsi_cmd_req_pi    cmd_pi;
> >   		struct virtio_scsi_ctrl_tmf_req  tmf;
> >   		struct virtio_scsi_ctrl_an_req   an;
> >   	} req;
> > @@ -440,7 +441,7 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
> >   			    size_t req_size, size_t resp_size, gfp_t gfp)
> >   {
> >   	struct scsi_cmnd *sc = cmd->sc;
> > -	struct scatterlist *sgs[4], req, resp;
> > +	struct scatterlist *sgs[6], req, resp;
> >   	struct sg_table *out, *in;
> >   	unsigned out_num = 0, in_num = 0;
> >   
> > @@ -458,16 +459,24 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
> >   	sgs[out_num++] = &req;
> >   
> >   	/* Data-out buffer.  */
> > -	if (out)
> > +	if (out) {
> > +		/* Place WRITE protection SGLs before Data OUT payload */
> > +		if (scsi_prot_sg_count(sc))
> 
> Didn't we agree that the LLD should not discard the scmnd prot_op? Martin?
> 

Without the initiator providing a protection buffer, there is not much
else that can be done here.

So for the special case where the protect CDB is set, but no protection
buffer is provided, the WRITE_INSERT / READ_STRIP emulation should kick
in on the target side.

--nab

> > +			sgs[out_num++] = scsi_prot_sglist(sc);
> >   		sgs[out_num++] = out->sgl;
> > +	}
> >   
> >   	/* Response header.  */
> >   	sg_init_one(&resp, &cmd->resp, resp_size);
> >   	sgs[out_num + in_num++] = &resp;
> >   
> >   	/* Data-in buffer */
> > -	if (in)
> > +	if (in) {
> > +		/* Place READ protection SGLs before Data IN payload */
> > +		if (scsi_prot_sg_count(sc))
> 
> Same here.
> 
> > +			sgs[out_num + in_num++] = scsi_prot_sglist(sc);
> >   		sgs[out_num + in_num++] = in->sgl;
> > +	}
> >   
> >   	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
> >   }
> >

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch updates virtscsi_probe() to setup necessary Scsi_Host
> level protection resources. (currently hardcoded to 1)
> 
> It changes virtscsi_add_cmd() to attach outgoing / incoming
> protection SGLs preceeding the data payload, and is using the
> new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> to signal to vhost/scsi how many prot_sgs to expect.
> 
> v3 changes:
>   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> 
> v2 changes:
>   - Make protection buffer come before data buffer (Paolo)
>   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

OK but we need to document the new interface in the spec
(and incidentially, this will be useful to verify the assumptions
made here and on the host side).
Could you please submit this proposal to the OASIS Virtio TC
for inclusion into the next spec draft?
Ideally as a patch against the tex source, but a prose
description would do as well.
The TC meets on a bi-weekly basis, we should be able to ratify
this quickly.


> ---
>  drivers/scsi/virtio_scsi.c |   78 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 16bfd50..68d8d1b 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -37,6 +37,7 @@ struct virtio_scsi_cmd {
>  	struct completion *comp;
>  	union {
>  		struct virtio_scsi_cmd_req       cmd;
> +		struct virtio_scsi_cmd_req_pi    cmd_pi;
>  		struct virtio_scsi_ctrl_tmf_req  tmf;
>  		struct virtio_scsi_ctrl_an_req   an;
>  	} req;
> @@ -440,7 +441,7 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
>  			    size_t req_size, size_t resp_size, gfp_t gfp)
>  {
>  	struct scsi_cmnd *sc = cmd->sc;
> -	struct scatterlist *sgs[4], req, resp;
> +	struct scatterlist *sgs[6], req, resp;
>  	struct sg_table *out, *in;
>  	unsigned out_num = 0, in_num = 0;
>  
> @@ -458,16 +459,24 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
>  	sgs[out_num++] = &req;
>  
>  	/* Data-out buffer.  */
> -	if (out)
> +	if (out) {
> +		/* Place WRITE protection SGLs before Data OUT payload */
> +		if (scsi_prot_sg_count(sc))
> +			sgs[out_num++] = scsi_prot_sglist(sc);
>  		sgs[out_num++] = out->sgl;
> +	}
>  
>  	/* Response header.  */
>  	sg_init_one(&resp, &cmd->resp, resp_size);
>  	sgs[out_num + in_num++] = &resp;
>  
>  	/* Data-in buffer */
> -	if (in)
> +	if (in) {
> +		/* Place READ protection SGLs before Data IN payload */
> +		if (scsi_prot_sg_count(sc))
> +			sgs[out_num + in_num++] = scsi_prot_sglist(sc);
>  		sgs[out_num + in_num++] = in->sgl;
> +	}
>  
>  	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
>  }
> @@ -492,12 +501,36 @@ static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
>  	return err;
>  }
>  
> +static void virtio_scsi_init_hdr(struct virtio_scsi_cmd_req *cmd,
> +				 struct scsi_cmnd *sc)
> +{
> +	cmd->lun[0] = 1;
> +	cmd->lun[1] = sc->device->id;
> +	cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
> +	cmd->lun[3] = sc->device->lun & 0xff;
> +	cmd->tag = (unsigned long)sc;
> +	cmd->task_attr = VIRTIO_SCSI_S_SIMPLE;
> +	cmd->prio = 0;
> +	cmd->crn = 0;
> +}
> +
> +static void virtio_scsi_init_hdr_pi(struct virtio_scsi_cmd_req_pi *cmd_pi,
> +				    struct scsi_cmnd *sc)
> +{
> +	virtio_scsi_init_hdr((struct virtio_scsi_cmd_req *)cmd_pi, sc);
> +
> +	if (sc->sc_data_direction == DMA_TO_DEVICE)
> +		cmd_pi->do_pi_niov = scsi_prot_sg_count(sc);
> +	else if (sc->sc_data_direction == DMA_FROM_DEVICE)
> +		cmd_pi->di_pi_niov = scsi_prot_sg_count(sc);
> +}
> +
>  static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
>  				 struct virtio_scsi_vq *req_vq,
>  				 struct scsi_cmnd *sc)
>  {
>  	struct virtio_scsi_cmd *cmd;
> -	int ret;
> +	int ret, req_size;
>  
>  	struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
>  	BUG_ON(scsi_sg_count(sc) > shost->sg_tablesize);
> @@ -515,22 +548,20 @@ static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
>  
>  	memset(cmd, 0, sizeof(*cmd));
>  	cmd->sc = sc;
> -	cmd->req.cmd = (struct virtio_scsi_cmd_req){
> -		.lun[0] = 1,
> -		.lun[1] = sc->device->id,
> -		.lun[2] = (sc->device->lun >> 8) | 0x40,
> -		.lun[3] = sc->device->lun & 0xff,
> -		.tag = (unsigned long)sc,
> -		.task_attr = VIRTIO_SCSI_S_SIMPLE,
> -		.prio = 0,
> -		.crn = 0,
> -	};
>  
>  	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
> -	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
>  
> -	if (virtscsi_kick_cmd(req_vq, cmd,
> -			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
> +	if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_T10_PI)) {
> +		virtio_scsi_init_hdr_pi(&cmd->req.cmd_pi, sc);
> +		memcpy(cmd->req.cmd_pi.cdb, sc->cmnd, sc->cmd_len);
> +		req_size = sizeof(cmd->req.cmd_pi);
> +	} else {
> +		virtio_scsi_init_hdr(&cmd->req.cmd, sc);
> +		memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
> +		req_size = sizeof(cmd->req.cmd);
> +	}
> +
> +	if (virtscsi_kick_cmd(req_vq, cmd, req_size, sizeof(cmd->resp.cmd),
>  			      GFP_ATOMIC) == 0)
>  		ret = 0;
>  	else
> @@ -871,7 +902,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
>  {
>  	struct Scsi_Host *shost;
>  	struct virtio_scsi *vscsi;
> -	int err;
> +	int err, host_prot;
>  	u32 sg_elems, num_targets;
>  	u32 cmd_per_lun;
>  	u32 num_queues;
> @@ -921,6 +952,16 @@ static int virtscsi_probe(struct virtio_device *vdev)
>  	shost->max_id = num_targets;
>  	shost->max_channel = 0;
>  	shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
> +
> +	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {
> +		host_prot = SHOST_DIF_TYPE1_PROTECTION | SHOST_DIF_TYPE2_PROTECTION |
> +			    SHOST_DIF_TYPE3_PROTECTION | SHOST_DIX_TYPE1_PROTECTION |
> +			    SHOST_DIX_TYPE2_PROTECTION | SHOST_DIX_TYPE3_PROTECTION;
> +
> +		scsi_host_set_prot(shost, host_prot);
> +		scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
> +	}
> +
>  	err = scsi_add_host(shost, &vdev->dev);
>  	if (err)
>  		goto scsi_add_host_failed;
> @@ -990,6 +1031,7 @@ static struct virtio_device_id id_table[] = {
>  static unsigned int features[] = {
>  	VIRTIO_SCSI_F_HOTPLUG,
>  	VIRTIO_SCSI_F_CHANGE,
> +	VIRTIO_SCSI_F_T10_PI,
>  };
>  
>  static struct virtio_driver virtio_scsi_driver = {
> -- 
> 1.7.10.4

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > level protection resources. (currently hardcoded to 1)
> > 
> > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > protection SGLs preceeding the data payload, and is using the
> > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > to signal to vhost/scsi how many prot_sgs to expect.
> > 
> > v3 changes:
> >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > 
> > v2 changes:
> >   - Make protection buffer come before data buffer (Paolo)
> >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> OK but we need to document the new interface in the spec
> (and incidentially, this will be useful to verify the assumptions
> made here and on the host side).
> Could you please submit this proposal to the OASIS Virtio TC
> for inclusion into the next spec draft?
> Ideally as a patch against the tex source, but a prose
> description would do as well.

Most certainly.  Please give me a bit to follow up on this, as the next
couple of days are going to be hellishly busy..

> The TC meets on a bi-weekly basis, we should be able to ratify
> this quickly.
> 

Aside from that, please consider ACK'ing the vhost specific changes so
these can make it into v3.15-rc1 code.

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > level protection resources. (currently hardcoded to 1)
> > > 
> > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > protection SGLs preceeding the data payload, and is using the
> > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > to signal to vhost/scsi how many prot_sgs to expect.
> > > 
> > > v3 changes:
> > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > 
> > > v2 changes:
> > >   - Make protection buffer come before data buffer (Paolo)
> > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > 
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: Hannes Reinecke <hare@suse.de>
> > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > OK but we need to document the new interface in the spec
> > (and incidentially, this will be useful to verify the assumptions
> > made here and on the host side).
> > Could you please submit this proposal to the OASIS Virtio TC
> > for inclusion into the next spec draft?
> > Ideally as a patch against the tex source, but a prose
> > description would do as well.
> 
> Most certainly.  Please give me a bit to follow up on this, as the next
> couple of days are going to be hellishly busy..
> 
> > The TC meets on a bi-weekly basis, we should be able to ratify
> > this quickly.
> > 
> 
> Aside from that, please consider ACK'ing the vhost specific changes so
> these can make it into v3.15-rc1 code.
> 
> --nab
> 

Hmm but what if the TC wants to change the interface somewhat?
I guess we'll still be able to fix it after the merge window -
(or worst case, revert the change) is this what you are suggesting?

-- 
MST

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 12:02 +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> > On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > 
> > > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > > level protection resources. (currently hardcoded to 1)
> > > > 
> > > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > > protection SGLs preceeding the data payload, and is using the
> > > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > > to signal to vhost/scsi how many prot_sgs to expect.
> > > > 
> > > > v3 changes:
> > > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > > 
> > > > v2 changes:
> > > >   - Make protection buffer come before data buffer (Paolo)
> > > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > > 
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > OK but we need to document the new interface in the spec
> > > (and incidentially, this will be useful to verify the assumptions
> > > made here and on the host side).
> > > Could you please submit this proposal to the OASIS Virtio TC
> > > for inclusion into the next spec draft?
> > > Ideally as a patch against the tex source, but a prose
> > > description would do as well.
> > 
> > Most certainly.  Please give me a bit to follow up on this, as the next
> > couple of days are going to be hellishly busy..
> > 
> > > The TC meets on a bi-weekly basis, we should be able to ratify
> > > this quickly.
> > > 
> > 
> > Aside from that, please consider ACK'ing the vhost specific changes so
> > these can make it into v3.15-rc1 code.
> > 
> > --nab
> > 
> 
> Hmm but what if the TC wants to change the interface somewhat?

I don't have a objection to changing the interface post-merge.

> I guess we'll still be able to fix it after the merge window -
> (or worst case, revert the change) is this what you are suggesting?
> 

Yes, but I would think that it is actually fixable.  ;)

Otherwise, a v3.16 merge is an option as well.  It's really your +
Paolo's call here. 

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 12:02 +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> > On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > 
> > > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > > level protection resources. (currently hardcoded to 1)
> > > > 
> > > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > > protection SGLs preceeding the data payload, and is using the
> > > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > > to signal to vhost/scsi how many prot_sgs to expect.
> > > > 
> > > > v3 changes:
> > > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > > 
> > > > v2 changes:
> > > >   - Make protection buffer come before data buffer (Paolo)
> > > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > > 
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > OK but we need to document the new interface in the spec
> > > (and incidentially, this will be useful to verify the assumptions
> > > made here and on the host side).
> > > Could you please submit this proposal to the OASIS Virtio TC
> > > for inclusion into the next spec draft?
> > > Ideally as a patch against the tex source, but a prose
> > > description would do as well.
> > 
> > Most certainly.  Please give me a bit to follow up on this, as the next
> > couple of days are going to be hellishly busy..
> > 
> > > The TC meets on a bi-weekly basis, we should be able to ratify
> > > this quickly.
> > > 
> > 
> > Aside from that, please consider ACK'ing the vhost specific changes so
> > these can make it into v3.15-rc1 code.
> > 
> > --nab
> > 
> 
> Hmm but what if the TC wants to change the interface somewhat?

I don't have a objection to changing the interface post-merge.

> I guess we'll still be able to fix it after the merge window -
> (or worst case, revert the change) is this what you are suggesting?
> 

Yes, but I would think that it is actually fixable.  ;)

Otherwise, a v3.16 merge is an option as well.  It's really your +
Paolo's call here. 

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Paolo Bonzini]

Il 07/04/2014 05:13, Nicholas A. Bellinger ha scritto:
>> > I guess we'll still be able to fix it after the merge window -
>> > (or worst case, revert the change) is this what you are suggesting?
>> >
> Yes, but I would think that it is actually fixable.  ;)
>
> Otherwise, a v3.16 merge is an option as well.  It's really your +
> Paolo's call here.

With s/niov/nbytes/, I think the interface is fine.  If you can do it 
this week, it is your call as target maintainer...

I don't have to do much more than give my acked-by for the changes to 
the virtio-scsi drivers, it's up to you whether to use it for 3.15 or 3.16.

Paolo

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Paolo Bonzini]

Il 07/04/2014 05:13, Nicholas A. Bellinger ha scritto:
>> > I guess we'll still be able to fix it after the merge window -
>> > (or worst case, revert the change) is this what you are suggesting?
>> >
> Yes, but I would think that it is actually fixable.  ;)
>
> Otherwise, a v3.16 merge is an option as well.  It's really your +
> Paolo's call here.

With s/niov/nbytes/, I think the interface is fine.  If you can do it 
this week, it is your call as target maintainer...

I don't have to do much more than give my acked-by for the changes to 
the virtio-scsi drivers, it's up to you whether to use it for 3.15 or 3.16.

Paolo

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > level protection resources. (currently hardcoded to 1)
> > > 
> > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > protection SGLs preceeding the data payload, and is using the
> > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > to signal to vhost/scsi how many prot_sgs to expect.
> > > 
> > > v3 changes:
> > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > 
> > > v2 changes:
> > >   - Make protection buffer come before data buffer (Paolo)
> > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > 
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: Hannes Reinecke <hare@suse.de>
> > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > OK but we need to document the new interface in the spec
> > (and incidentially, this will be useful to verify the assumptions
> > made here and on the host side).
> > Could you please submit this proposal to the OASIS Virtio TC
> > for inclusion into the next spec draft?
> > Ideally as a patch against the tex source, but a prose
> > description would do as well.
> 
> Most certainly.  Please give me a bit to follow up on this, as the next
> couple of days are going to be hellishly busy..

Ping.
We really need to get this moving to have the interface reviewed for
the next merge window.


> > The TC meets on a bi-weekly basis, we should be able to ratify
> > this quickly.
> > 
> 
> Aside from that, please consider ACK'ing the vhost specific changes so
> these can make it into v3.15-rc1 code.
> 
> --nab
> 

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> > On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > 
> > > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > > level protection resources. (currently hardcoded to 1)
> > > > 
> > > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > > protection SGLs preceeding the data payload, and is using the
> > > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > > to signal to vhost/scsi how many prot_sgs to expect.
> > > > 
> > > > v3 changes:
> > > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > > 
> > > > v2 changes:
> > > >   - Make protection buffer come before data buffer (Paolo)
> > > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > > 
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > OK but we need to document the new interface in the spec
> > > (and incidentially, this will be useful to verify the assumptions
> > > made here and on the host side).
> > > Could you please submit this proposal to the OASIS Virtio TC
> > > for inclusion into the next spec draft?
> > > Ideally as a patch against the tex source, but a prose
> > > description would do as well.
> > 
> > Most certainly.  Please give me a bit to follow up on this, as the next
> > couple of days are going to be hellishly busy..
> 
> Ping.
> We really need to get this moving to have the interface reviewed for
> the next merge window.
> 

Hi MST,

So I've finally got some cycles to get back to this code, and wanted to
verify the outstanding items you had previously raised:

- Convert vhost-scsi to be independent of IOV layout using 
  memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
  operation..?)
- Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
- Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
  iovecs.
- Ensure virtio_scsi_cmd_req_pi is naturally aligned
- Figure out why QEMU is not acking (any) vhost-scsi feature bits

Is there anything else that you'd like to see for an initial merge, or
other issues that need to be addressed with the above..?

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> > On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > 
> > > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > > level protection resources. (currently hardcoded to 1)
> > > > 
> > > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > > protection SGLs preceeding the data payload, and is using the
> > > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > > to signal to vhost/scsi how many prot_sgs to expect.
> > > > 
> > > > v3 changes:
> > > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > > 
> > > > v2 changes:
> > > >   - Make protection buffer come before data buffer (Paolo)
> > > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > > 
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > OK but we need to document the new interface in the spec
> > > (and incidentially, this will be useful to verify the assumptions
> > > made here and on the host side).
> > > Could you please submit this proposal to the OASIS Virtio TC
> > > for inclusion into the next spec draft?
> > > Ideally as a patch against the tex source, but a prose
> > > description would do as well.
> > 
> > Most certainly.  Please give me a bit to follow up on this, as the next
> > couple of days are going to be hellishly busy..
> 
> Ping.
> We really need to get this moving to have the interface reviewed for
> the next merge window.
> 

Hi MST,

So I've finally got some cycles to get back to this code, and wanted to
verify the outstanding items you had previously raised:

- Convert vhost-scsi to be independent of IOV layout using 
  memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
  operation..?)
- Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
- Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
  iovecs.
- Ensure virtio_scsi_cmd_req_pi is naturally aligned
- Figure out why QEMU is not acking (any) vhost-scsi feature bits

Is there anything else that you'd like to see for an initial merge, or
other issues that need to be addressed with the above..?

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> > On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> > > On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > > > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > > 
> > > > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > > > level protection resources. (currently hardcoded to 1)
> > > > > 
> > > > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > > > protection SGLs preceeding the data payload, and is using the
> > > > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > > > to signal to vhost/scsi how many prot_sgs to expect.
> > > > > 
> > > > > v3 changes:
> > > > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > > > 
> > > > > v2 changes:
> > > > >   - Make protection buffer come before data buffer (Paolo)
> > > > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > > > 
> > > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > 
> > > > OK but we need to document the new interface in the spec
> > > > (and incidentially, this will be useful to verify the assumptions
> > > > made here and on the host side).
> > > > Could you please submit this proposal to the OASIS Virtio TC
> > > > for inclusion into the next spec draft?
> > > > Ideally as a patch against the tex source, but a prose
> > > > description would do as well.
> > > 
> > > Most certainly.  Please give me a bit to follow up on this, as the next
> > > couple of days are going to be hellishly busy..
> > 
> > Ping.
> > We really need to get this moving to have the interface reviewed for
> > the next merge window.
> > 
> 
> Hi MST,
> 
> So I've finally got some cycles to get back to this code, and wanted to
> verify the outstanding items you had previously raised:
> 
> - Convert vhost-scsi to be independent of IOV layout using 
>   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
>   operation..?)

Ideally yes.

> - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
>   iovecs.
> - Ensure virtio_scsi_cmd_req_pi is naturally aligned

It turns out other virtio scsi commands aren't aligned?
If true we don't need to make an exception here.

> - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> 
> Is there anything else that you'd like to see for an initial merge, or
> other issues that need to be addressed with the above..?
> 
> --nab

FYI Paolo sent a spec patch for the feature.
Have you seen it?

-- 
MST

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-05-19 at 22:15 +0300, Michael S. Tsirkin wrote:
> On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> > > On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> > > > On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > > > > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > > > 
> > > > > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > > > > level protection resources. (currently hardcoded to 1)
> > > > > > 
> > > > > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > > > > protection SGLs preceeding the data payload, and is using the
> > > > > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > > > > to signal to vhost/scsi how many prot_sgs to expect.
> > > > > > 
> > > > > > v3 changes:
> > > > > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > > > > 
> > > > > > v2 changes:
> > > > > >   - Make protection buffer come before data buffer (Paolo)
> > > > > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > > > > 
> > > > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > > 
> > > > > OK but we need to document the new interface in the spec
> > > > > (and incidentially, this will be useful to verify the assumptions
> > > > > made here and on the host side).
> > > > > Could you please submit this proposal to the OASIS Virtio TC
> > > > > for inclusion into the next spec draft?
> > > > > Ideally as a patch against the tex source, but a prose
> > > > > description would do as well.
> > > > 
> > > > Most certainly.  Please give me a bit to follow up on this, as the next
> > > > couple of days are going to be hellishly busy..
> > > 
> > > Ping.
> > > We really need to get this moving to have the interface reviewed for
> > > the next merge window.
> > > 
> > 
> > Hi MST,
> > 
> > So I've finally got some cycles to get back to this code, and wanted to
> > verify the outstanding items you had previously raised:
> > 
> > - Convert vhost-scsi to be independent of IOV layout using 
> >   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
> >   operation..?)
> 
> Ideally yes.

Er, so changing vhost-scsi to be independent of IOV layout will have the
side effect of breaking existing non PI virtio-scsi logic..?

> 
> > - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> > - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
> >   iovecs.
> > - Ensure virtio_scsi_cmd_req_pi is naturally aligned
> 
> It turns out other virtio scsi commands aren't aligned?

Correct, virtio_scsi_cmd_req is currently 51 bytes.

> If true we don't need to make an exception here.

<nod>

> 
> > - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> > 
> > Is there anything else that you'd like to see for an initial merge, or
> > other issues that need to be addressed with the above..?
> > 
> > --nab
> 
> FYI Paolo sent a spec patch for the feature.
> Have you seen it?
> 

Yep, looks fine.

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-05-19 at 22:15 +0300, Michael S. Tsirkin wrote:
> On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> > > On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> > > > On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > > > > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > > > 
> > > > > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > > > > level protection resources. (currently hardcoded to 1)
> > > > > > 
> > > > > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > > > > protection SGLs preceeding the data payload, and is using the
> > > > > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > > > > to signal to vhost/scsi how many prot_sgs to expect.
> > > > > > 
> > > > > > v3 changes:
> > > > > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > > > > 
> > > > > > v2 changes:
> > > > > >   - Make protection buffer come before data buffer (Paolo)
> > > > > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > > > > 
> > > > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > > 
> > > > > OK but we need to document the new interface in the spec
> > > > > (and incidentially, this will be useful to verify the assumptions
> > > > > made here and on the host side).
> > > > > Could you please submit this proposal to the OASIS Virtio TC
> > > > > for inclusion into the next spec draft?
> > > > > Ideally as a patch against the tex source, but a prose
> > > > > description would do as well.
> > > > 
> > > > Most certainly.  Please give me a bit to follow up on this, as the next
> > > > couple of days are going to be hellishly busy..
> > > 
> > > Ping.
> > > We really need to get this moving to have the interface reviewed for
> > > the next merge window.
> > > 
> > 
> > Hi MST,
> > 
> > So I've finally got some cycles to get back to this code, and wanted to
> > verify the outstanding items you had previously raised:
> > 
> > - Convert vhost-scsi to be independent of IOV layout using 
> >   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
> >   operation..?)
> 
> Ideally yes.

Er, so changing vhost-scsi to be independent of IOV layout will have the
side effect of breaking existing non PI virtio-scsi logic..?

> 
> > - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> > - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
> >   iovecs.
> > - Ensure virtio_scsi_cmd_req_pi is naturally aligned
> 
> It turns out other virtio scsi commands aren't aligned?

Correct, virtio_scsi_cmd_req is currently 51 bytes.

> If true we don't need to make an exception here.

<nod>

> 
> > - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> > 
> > Is there anything else that you'd like to see for an initial merge, or
> > other issues that need to be addressed with the above..?
> > 
> > --nab
> 
> FYI Paolo sent a spec patch for the feature.
> Have you seen it?
> 

Yep, looks fine.

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 01:54:50PM -0700, Nicholas A. Bellinger wrote:
> On Mon, 2014-05-19 at 22:15 +0300, Michael S. Tsirkin wrote:
> > On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
> > > On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> > > > On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
> > > > > On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > > > > > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > > > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > > > > 
> > > > > > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > > > > > level protection resources. (currently hardcoded to 1)
> > > > > > > 
> > > > > > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > > > > > protection SGLs preceeding the data payload, and is using the
> > > > > > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > > > > > to signal to vhost/scsi how many prot_sgs to expect.
> > > > > > > 
> > > > > > > v3 changes:
> > > > > > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > > > > > > 
> > > > > > > v2 changes:
> > > > > > >   - Make protection buffer come before data buffer (Paolo)
> > > > > > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > > > > > 
> > > > > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > > > 
> > > > > > OK but we need to document the new interface in the spec
> > > > > > (and incidentially, this will be useful to verify the assumptions
> > > > > > made here and on the host side).
> > > > > > Could you please submit this proposal to the OASIS Virtio TC
> > > > > > for inclusion into the next spec draft?
> > > > > > Ideally as a patch against the tex source, but a prose
> > > > > > description would do as well.
> > > > > 
> > > > > Most certainly.  Please give me a bit to follow up on this, as the next
> > > > > couple of days are going to be hellishly busy..
> > > > 
> > > > Ping.
> > > > We really need to get this moving to have the interface reviewed for
> > > > the next merge window.
> > > > 
> > > 
> > > Hi MST,
> > > 
> > > So I've finally got some cycles to get back to this code, and wanted to
> > > verify the outstanding items you had previously raised:
> > > 
> > > - Convert vhost-scsi to be independent of IOV layout using 
> > >   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
> > >   operation..?)
> > 
> > Ideally yes.
> 
> Er, so changing vhost-scsi to be independent of IOV layout will have the
> side effect of breaking existing non PI virtio-scsi logic..?

Sorry I didn't make myself clear.
I merely think that all code should do memcpy_fromiovecend etc.
If done peoperly guests will not notice unless they test
VIRTIO_F_ANY_LAYOUT.



> > 
> > > - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> > > - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
> > >   iovecs.
> > > - Ensure virtio_scsi_cmd_req_pi is naturally aligned
> > 
> > It turns out other virtio scsi commands aren't aligned?
> 
> Correct, virtio_scsi_cmd_req is currently 51 bytes.
> 
> > If true we don't need to make an exception here.
> 
> <nod>
> 
> > 
> > > - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> > > 
> > > Is there anything else that you'd like to see for an initial merge, or
> > > other issues that need to be addressed with the above..?
> > > 
> > > --nab
> > 
> > FYI Paolo sent a spec patch for the feature.
> > Have you seen it?
> > 
> 
> Yep, looks fine.
> 
> --nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Paolo Bonzini]

Il 19/05/2014 21:07, Nicholas A. Bellinger ha scritto:
> Hi MST,
>
> So I've finally got some cycles to get back to this code, and wanted to
> verify the outstanding items you had previously raised:
>
> - Convert vhost-scsi to be independent of IOV layout using
>   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi
>   operation..?)
> - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of
>   iovecs.

This is the only item really required, since the bytes field is what 
will be in VIRTIO 1.0.  The other would be nice to have, but not a 
blocker for PI support.

> - Ensure virtio_scsi_cmd_req_pi is naturally aligned

mst already commented on this.

> - Figure out why QEMU is not acking (any) vhost-scsi feature bits

This is a separate bug, isn't it?  It need not block PI support.

Paolo

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Tue, 2014-05-20 at 10:35 +0200, Paolo Bonzini wrote:
> Il 19/05/2014 21:07, Nicholas A. Bellinger ha scritto:
> > Hi MST,
> >
> > So I've finally got some cycles to get back to this code, and wanted to
> > verify the outstanding items you had previously raised:
> >
> > - Convert vhost-scsi to be independent of IOV layout using
> >   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi
> >   operation..?)
> > - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> > - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of
> >   iovecs.
> 
> This is the only item really required, since the bytes field is what 
> will be in VIRTIO 1.0.  The other would be nice to have, but not a 
> blocker for PI support.
> 
> > - Ensure virtio_scsi_cmd_req_pi is naturally aligned
> 
> mst already commented on this.
> 
> > - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> 
> This is a separate bug, isn't it?  It need not block PI support.
> 

Thanks for the feedback.  I'll get the series updated to use bytes
instead of number of iovecs in virtio_scsi_cmd_req_pi, along with a
simple memcpy_fromiovecend conversion.

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Tue, 2014-05-20 at 10:35 +0200, Paolo Bonzini wrote:
> Il 19/05/2014 21:07, Nicholas A. Bellinger ha scritto:
> > Hi MST,
> >
> > So I've finally got some cycles to get back to this code, and wanted to
> > verify the outstanding items you had previously raised:
> >
> > - Convert vhost-scsi to be independent of IOV layout using
> >   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi
> >   operation..?)
> > - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> > - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of
> >   iovecs.
> 
> This is the only item really required, since the bytes field is what 
> will be in VIRTIO 1.0.  The other would be nice to have, but not a 
> blocker for PI support.
> 
> > - Ensure virtio_scsi_cmd_req_pi is naturally aligned
> 
> mst already commented on this.
> 
> > - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> 
> This is a separate bug, isn't it?  It need not block PI support.
> 

Thanks for the feedback.  I'll get the series updated to use bytes
instead of number of iovecs in virtio_scsi_cmd_req_pi, along with a
simple memcpy_fromiovecend conversion.

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Paolo Bonzini]

Il 19/05/2014 21:07, Nicholas A. Bellinger ha scritto:
> Hi MST,
>
> So I've finally got some cycles to get back to this code, and wanted to
> verify the outstanding items you had previously raised:
>
> - Convert vhost-scsi to be independent of IOV layout using
>   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi
>   operation..?)
> - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of
>   iovecs.

This is the only item really required, since the bytes field is what 
will be in VIRTIO 1.0.  The other would be nice to have, but not a 
blocker for PI support.

> - Ensure virtio_scsi_cmd_req_pi is naturally aligned

mst already commented on this.

> - Figure out why QEMU is not acking (any) vhost-scsi feature bits

This is a separate bug, isn't it?  It need not block PI support.

Paolo

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > level protection resources. (currently hardcoded to 1)
> > 
> > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > protection SGLs preceeding the data payload, and is using the
> > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > to signal to vhost/scsi how many prot_sgs to expect.
> > 
> > v3 changes:
> >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > 
> > v2 changes:
> >   - Make protection buffer come before data buffer (Paolo)
> >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> OK but we need to document the new interface in the spec
> (and incidentially, this will be useful to verify the assumptions
> made here and on the host side).
> Could you please submit this proposal to the OASIS Virtio TC
> for inclusion into the next spec draft?
> Ideally as a patch against the tex source, but a prose
> description would do as well.

Most certainly.  Please give me a bit to follow up on this, as the next
couple of days are going to be hellishly busy..

> The TC meets on a bi-weekly basis, we should be able to ratify
> this quickly.
> 

Aside from that, please consider ACK'ing the vhost specific changes so
these can make it into v3.15-rc1 code.

--nab