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

[PATCH-v2 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,

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

Following MST's recommendation, it includes the changes for using
bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
the associated changes to virtio-scsi + vhost/scsi code.

For vhost/scsi, this includes walking the leading iovec's length(s)
to determine where protection payload ends, and real data payload 
starts.  For virtio-scsi LLD code, this includes locating struct
blk_integrity for using blk_rq_sectors + ->tuple_size to calculate
the total bytes for outgoing cmd_pi->pi_bytes[out,in] values.

The full list of changes from last series include:

  - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
    header (mst + paolo)
  - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
    exists (nab)
  - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
  - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
    of data_num in vhost_scsi_handle_vq (nab)
  - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
  - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
  - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
  - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
    (mst + paolo + nab)
  - Use blk_integrity->tuple_size to calculate pi bytes (nab)

Please review for v3.16-rc1 code.

Thanks!

--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  |   86 +++++++++---
 drivers/vhost/scsi.c        |  305 +++++++++++++++++++++++++++++--------------
 include/linux/virtio_scsi.h |   15 ++-
 3 files changed, 292 insertions(+), 114 deletions(-)

-- 
1.7.10.4

[PATCH-v2 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.

v4 changes:
   - Use pi_bytesout + pi_bytesin instead of niovs (mst + 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>
---
 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..7344906 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;
+	u32 pi_bytesout;	/* DataOUT PI Number of bytes */
+	u32 pi_bytesin;		/* DataIN PI Number of bytes */
+	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

[PATCH-v2 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-v2 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-v2 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-v2 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.

v4 changes:
  - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
    exists (nab)
  - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
  - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
    of data_num in vhost_scsi_handle_vq (nab)
  - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
  - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
  - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)

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 |  183 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 124 insertions(+), 59 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index eabcf18..17ec04b 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,17 @@ 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
+			se_cmd->prot_pto = true;
 	} else {
 		sg_ptr = NULL;
 	}
@@ -935,7 +936,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 +968,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, prot_bytes;
+	size_t req_size;
+	u16 lun;
+	u8 *target, *lunp, task_attr;
+	bool hdr_pi;
+	void *req, *cdb;
 
 	mutex_lock(&vq->mutex);
 	/*
@@ -1003,7 +1010,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 +1040,38 @@ 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);
+		if (vs->dev.acked_features & VIRTIO_SCSI_F_T10_PI) {
+			req = &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 {
+			req = &v_req;
+			lunp = &v_req.lun[0];
+			target = &v_req.lun[1];
+			req_size = sizeof(v_req);
+			hdr_pi = false;
+		}
+
+		if (unlikely(vq->iov[0].iov_len < req_size)) {
+			pr_err("Expecting virtio-scsi header: %zu, got %zu\n",
+			       req_size, vq->iov[0].iov_len);
 			break;
 		}
-		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));
+		ret = memcpy_fromiovecend(req, &vq->iov[0], 0, 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 +1079,78 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 			continue;
 		}
 
+		data_niov = data_num;
+		prot_niov = prot_first = prot_bytes = 0;
+		/*
+		 * Determine if any protection information iovecs are preceeding
+		 * the actual data payload, and adjust data_first + data_niov
+		 * values accordingly for vhost_scsi_map_iov_to_sgl() below.
+		 *
+		 * Also extract virtio_scsi header bits for vhost_scsi_get_tag()
+		 */
+		if (hdr_pi) {
+			if (v_req_pi.pi_bytesout) {
+				if (data_direction != DMA_TO_DEVICE) {
+					vq_err(vq, "Received non zero do_pi_niov"
+						", but wrong data_direction\n");
+					goto err_cmd;
+				}
+				prot_bytes = v_req_pi.pi_bytesout;
+			} else if (v_req_pi.pi_bytesin) {
+				if (data_direction != DMA_FROM_DEVICE) {
+					vq_err(vq, "Received non zero di_pi_niov"
+						", but wrong data_direction\n");
+					goto err_cmd;
+				}
+				prot_bytes = v_req_pi.pi_bytesin;
+			}
+			if (prot_bytes) {
+				int tmp = 0;
+
+				for (i = 0; i < data_num; i++) {
+					tmp += vq->iov[data_first + i].iov_len;
+					prot_niov++;
+					if (tmp >= prot_bytes)
+						break;
+				}
+				prot_first = data_first;
+				data_first += prot_niov;
+				data_niov = data_num - 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 +1158,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
@@ -1788,7 +1853,7 @@ static int tcm_vhost_make_nexus(struct tcm_vhost_tpg *tpg,
 	tv_nexus->tvn_se_sess = transport_init_session_tags(
 					TCM_VHOST_DEFAULT_TAGS,
 					sizeof(struct tcm_vhost_cmd),
-					TARGET_PROT_NORMAL);
+					TARGET_PROT_ALL);
 	if (IS_ERR(tv_nexus->tvn_se_sess)) {
 		mutex_unlock(&tpg->tv_tpg_mutex);
 		kfree(tv_nexus);
-- 
1.7.10.4

[PATCH-v2 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.

v4 changes:
  - Convert virtio_scsi_init_hdr_pi to use pi_bytesout + pi_bytesin
    (mst + paolo + nab)
  - Use blk_integrity->tuple_size to calculate pi bytes (nab)

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 |   86 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 68 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 16bfd50..cc634b0 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,44 @@ 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)
+{
+	struct request *rq = sc->request;
+	struct blk_integrity *bi;
+
+	virtio_scsi_init_hdr((struct virtio_scsi_cmd_req *)cmd_pi, sc);
+
+	if (!rq || !scsi_prot_sg_count(sc))
+		return;
+
+	bi = blk_get_integrity(rq->rq_disk);
+
+	if (sc->sc_data_direction == DMA_TO_DEVICE)
+		cmd_pi->pi_bytesout = blk_rq_sectors(rq) * bi->tuple_size;
+	else if (sc->sc_data_direction == DMA_FROM_DEVICE)
+		cmd_pi->pi_bytesin = blk_rq_sectors(rq) * bi->tuple_size;
+}
+
 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 +556,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 +910,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 +960,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 +1039,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-v2 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

[Michael S. Tsirkin]

On Thu, May 22, 2014 at 02:26:17AM +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.

Cc rusty.
Rusty could you please Ack merging this through Nicholas' tree
together with the vhost changes?


> Also add new VIRTIO_SCSI_F_T10_PI feature bit to be used to signal
> host support.
> 
> v4 changes:

v2? Proably best to drop versioning info from commit log,
move it out to notes (after ---)

>    - Use pi_bytesout + pi_bytesin instead of niovs (mst + paolo)

Right, so maybe update the commit log above to match?
It gave me pause.

> 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..7344906 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;
> +	u32 pi_bytesout;	/* DataOUT PI Number of bytes */
> +	u32 pi_bytesin;		/* DataIN PI Number of bytes */
> +	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

Could you pad this with spaces and not tabs so it aligns nicely
with stuff above it?

>  
>  /* Response codes */
>  #define VIRTIO_SCSI_S_OK                       0
> -- 
> 1.7.10.4

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

[Paolo Bonzini]

Il 22/05/2014 04:26, Nicholas A. Bellinger ha scritto:
> 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

Wrong field name in the commit message...

> to signal to vhost/scsi how many prot_sgs to expect.

s/prot_sgs to expect/bytes to expect for protection data/

Apart from this, which can be fixed in the pull request,

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

for getting this in via the target tree.

Paolo

> v4 changes:
>   - Convert virtio_scsi_init_hdr_pi to use pi_bytesout + pi_bytesin
>     (mst + paolo + nab)
>   - Use blk_integrity->tuple_size to calculate pi bytes (nab)
>
> 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 |   86 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 68 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 16bfd50..cc634b0 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,44 @@ 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)
> +{
> +	struct request *rq = sc->request;
> +	struct blk_integrity *bi;
> +
> +	virtio_scsi_init_hdr((struct virtio_scsi_cmd_req *)cmd_pi, sc);
> +
> +	if (!rq || !scsi_prot_sg_count(sc))
> +		return;
> +
> +	bi = blk_get_integrity(rq->rq_disk);
> +
> +	if (sc->sc_data_direction == DMA_TO_DEVICE)
> +		cmd_pi->pi_bytesout = blk_rq_sectors(rq) * bi->tuple_size;
> +	else if (sc->sc_data_direction == DMA_FROM_DEVICE)
> +		cmd_pi->pi_bytesin = blk_rq_sectors(rq) * bi->tuple_size;
> +}
> +
>  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 +556,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 +910,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 +960,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 +1039,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-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

[Paolo Bonzini]

Il 22/05/2014 04:26, Nicholas A. Bellinger ha scritto:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Hi MST, MKP, Paolo & Co,
>
> Here is the v2 patch series for adding T1O protection information (PI)
> SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
> endpoints.
>
> Following MST's recommendation, it includes the changes for using
> bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
> the associated changes to virtio-scsi + vhost/scsi code.
>
> For vhost/scsi, this includes walking the leading iovec's length(s)
> to determine where protection payload ends, and real data payload
> starts.  For virtio-scsi LLD code, this includes locating struct
> blk_integrity for using blk_rq_sectors + ->tuple_size to calculate
> the total bytes for outgoing cmd_pi->pi_bytes[out,in] values.
>
> The full list of changes from last series include:
>
>   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
>     header (mst + paolo)
>   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
>     exists (nab)
>   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
>   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
>     of data_num in vhost_scsi_handle_vq (nab)
>   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
>   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
>   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
>   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
>     (mst + paolo + nab)
>   - Use blk_integrity->tuple_size to calculate pi bytes (nab)
>
> Please review for v3.16-rc1 code.
>
> Thanks!
>
> --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  |   86 +++++++++---
>  drivers/vhost/scsi.c        |  305 +++++++++++++++++++++++++++++--------------
>  include/linux/virtio_scsi.h |   15 ++-
>  3 files changed, 292 insertions(+), 114 deletions(-)
>

Looks good, thanks!

Paolo

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

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, May 22, 2014 at 02:26:17AM +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.
>
> Cc rusty.
> Rusty could you please Ack merging this through Nicholas' tree
> together with the vhost changes?

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

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

[Nicholas A. Bellinger]

On Thu, 2014-05-22 at 09:57 +0300, Michael S. Tsirkin wrote:
> On Thu, May 22, 2014 at 02:26:17AM +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.
> 
> Cc rusty.
> Rusty could you please Ack merging this through Nicholas' tree
> together with the vhost changes?
> 
> 
> > Also add new VIRTIO_SCSI_F_T10_PI feature bit to be used to signal
> > host support.
> > 
> > v4 changes:
> 
> v2? Proably best to drop versioning info from commit log,
> move it out to notes (after ---)

Dropped

> 
> >    - Use pi_bytesout + pi_bytesin instead of niovs (mst + paolo)
> 
> Right, so maybe update the commit log above to match?
> It gave me pause.

Fixed

> 
> > 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..7344906 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;
> > +	u32 pi_bytesout;	/* DataOUT PI Number of bytes */
> > +	u32 pi_bytesin;		/* DataIN PI Number of bytes */
> > +	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
> 
> Could you pad this with spaces and not tabs so it aligns nicely
> with stuff above it?

Fixed

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

[Nicholas A. Bellinger]

On Thu, 2014-05-22 at 10:37 +0200, Paolo Bonzini wrote:
> Il 22/05/2014 04:26, Nicholas A. Bellinger ha scritto:
> > 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
> 
> Wrong field name in the commit message...
> 

Fixed

> > to signal to vhost/scsi how many prot_sgs to expect.
> 
> s/prot_sgs to expect/bytes to expect for protection data/
> 

Fixed.

> Apart from this, which can be fixed in the pull request,
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> for getting this in via the target tree.
> 

Thanks Paolo!

--nab

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

[Michael S. Tsirkin]

On Thu, May 22, 2014 at 02:26:16AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi MST, MKP, Paolo & Co,
> 
> Here is the v2 patch series for adding T1O protection information (PI)
> SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
> endpoints.
> 
> Following MST's recommendation, it includes the changes for using
> bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
> the associated changes to virtio-scsi + vhost/scsi code.
> 
> For vhost/scsi, this includes walking the leading iovec's length(s)
> to determine where protection payload ends, and real data payload 
> starts.  For virtio-scsi LLD code, this includes locating struct
> blk_integrity for using blk_rq_sectors + ->tuple_size to calculate
> the total bytes for outgoing cmd_pi->pi_bytes[out,in] values.
> 
> The full list of changes from last series include:
> 
>   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
>     header (mst + paolo)
>   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
>     exists (nab)
>   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
>   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
>     of data_num in vhost_scsi_handle_vq (nab)
>   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
>   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
>   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
>   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
>     (mst + paolo + nab)
>   - Use blk_integrity->tuple_size to calculate pi bytes (nab)
> 
> Please review for v3.16-rc1 code.
> 
> Thanks!
> 
> --nab

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> 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  |   86 +++++++++---
>  drivers/vhost/scsi.c        |  305 +++++++++++++++++++++++++++++--------------
>  include/linux/virtio_scsi.h |   15 ++-
>  3 files changed, 292 insertions(+), 114 deletions(-)
> 
> -- 
> 1.7.10.4

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

[Michael S. Tsirkin]

On Thu, May 22, 2014 at 02:26:16AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi MST, MKP, Paolo & Co,
> 
> Here is the v2 patch series for adding T1O protection information (PI)
> SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
> endpoints.
> 
> Following MST's recommendation, it includes the changes for using
> bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
> the associated changes to virtio-scsi + vhost/scsi code.
> 
> For vhost/scsi, this includes walking the leading iovec's length(s)
> to determine where protection payload ends, and real data payload 
> starts.  For virtio-scsi LLD code, this includes locating struct
> blk_integrity for using blk_rq_sectors + ->tuple_size to calculate
> the total bytes for outgoing cmd_pi->pi_bytes[out,in] values.
> 
> The full list of changes from last series include:
> 
>   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
>     header (mst + paolo)
>   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
>     exists (nab)
>   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
>   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
>     of data_num in vhost_scsi_handle_vq (nab)
>   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
>   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
>   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
>   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
>     (mst + paolo + nab)
>   - Use blk_integrity->tuple_size to calculate pi bytes (nab)
> 
> Please review for v3.16-rc1 code.
> 
> Thanks!
> 
> --nab


OK, finally went over this, looks good to me:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

However, we really should stop making more changes
before fixing ANY_LAYOUT in this driver.

virtio 1.0 should be out soon and that makes ANY_LAYOUT
a required feature.



> 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  |   86 +++++++++---
>  drivers/vhost/scsi.c        |  305 +++++++++++++++++++++++++++++--------------
>  include/linux/virtio_scsi.h |   15 ++-
>  3 files changed, 292 insertions(+), 114 deletions(-)
> 
> -- 
> 1.7.10.4

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

[Paolo Bonzini]

Il 08/06/2014 18:05, Michael S. Tsirkin ha scritto:
>
> OK, finally went over this, looks good to me:
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> However, we really should stop making more changes
> before fixing ANY_LAYOUT in this driver.
>
> virtio 1.0 should be out soon and that makes ANY_LAYOUT
> a required feature.

Agreed.  virtio-blk btw is getting ANY_LAYOUT support in QEMU 2.1, and 
virtio-scsi will follow suit.

Paolo

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

[Michael S. Tsirkin]

On Thu, May 22, 2014 at 02:26:21AM +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.
> 
> v4 changes:
>   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
>     exists (nab)
>   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
>   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
>     of data_num in vhost_scsi_handle_vq (nab)
>   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
>   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
>   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
> 
> 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 |  183 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 124 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index eabcf18..17ec04b 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,17 @@ 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
> +			se_cmd->prot_pto = true;
>  	} else {
>  		sg_ptr = NULL;
>  	}
> @@ -935,7 +936,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 +968,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, prot_bytes;
> +	size_t req_size;
> +	u16 lun;
> +	u8 *target, *lunp, task_attr;
> +	bool hdr_pi;
> +	void *req, *cdb;
>  
>  	mutex_lock(&vq->mutex);
>  	/*
> @@ -1003,7 +1010,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 +1040,38 @@ 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);
> +		if (vs->dev.acked_features & VIRTIO_SCSI_F_T10_PI) {

Actually this should use the has_feature macro.
And if you did you would notice that this is wrong:
you are doing & with bit number.


Unfortunately I am reworking that API for the next
kernel, so there's a conflict with bits in
my tree.

How about you rebase on top of vhost-next in my tree
and I'll merge it all together?




> +			req = &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 {
> +			req = &v_req;
> +			lunp = &v_req.lun[0];
> +			target = &v_req.lun[1];
> +			req_size = sizeof(v_req);
> +			hdr_pi = false;
> +		}
> +
> +		if (unlikely(vq->iov[0].iov_len < req_size)) {
> +			pr_err("Expecting virtio-scsi header: %zu, got %zu\n",
> +			       req_size, vq->iov[0].iov_len);
>  			break;
>  		}
> -		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));
> +		ret = memcpy_fromiovecend(req, &vq->iov[0], 0, 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 +1079,78 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
>  			continue;
>  		}
>  
> +		data_niov = data_num;
> +		prot_niov = prot_first = prot_bytes = 0;
> +		/*
> +		 * Determine if any protection information iovecs are preceeding
> +		 * the actual data payload, and adjust data_first + data_niov
> +		 * values accordingly for vhost_scsi_map_iov_to_sgl() below.
> +		 *
> +		 * Also extract virtio_scsi header bits for vhost_scsi_get_tag()
> +		 */
> +		if (hdr_pi) {
> +			if (v_req_pi.pi_bytesout) {
> +				if (data_direction != DMA_TO_DEVICE) {
> +					vq_err(vq, "Received non zero do_pi_niov"
> +						", but wrong data_direction\n");
> +					goto err_cmd;
> +				}
> +				prot_bytes = v_req_pi.pi_bytesout;
> +			} else if (v_req_pi.pi_bytesin) {
> +				if (data_direction != DMA_FROM_DEVICE) {
> +					vq_err(vq, "Received non zero di_pi_niov"
> +						", but wrong data_direction\n");
> +					goto err_cmd;
> +				}
> +				prot_bytes = v_req_pi.pi_bytesin;
> +			}
> +			if (prot_bytes) {
> +				int tmp = 0;
> +
> +				for (i = 0; i < data_num; i++) {
> +					tmp += vq->iov[data_first + i].iov_len;
> +					prot_niov++;
> +					if (tmp >= prot_bytes)
> +						break;
> +				}
> +				prot_first = data_first;
> +				data_first += prot_niov;
> +				data_niov = data_num - 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 +1158,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
> @@ -1788,7 +1853,7 @@ static int tcm_vhost_make_nexus(struct tcm_vhost_tpg *tpg,
>  	tv_nexus->tvn_se_sess = transport_init_session_tags(
>  					TCM_VHOST_DEFAULT_TAGS,
>  					sizeof(struct tcm_vhost_cmd),
> -					TARGET_PROT_NORMAL);
> +					TARGET_PROT_ALL);
>  	if (IS_ERR(tv_nexus->tvn_se_sess)) {
>  		mutex_unlock(&tpg->tv_tpg_mutex);
>  		kfree(tv_nexus);
> -- 
> 1.7.10.4

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

[Michael S. Tsirkin]

On Thu, May 22, 2014 at 02:26:17AM +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.
> 
> v4 changes:
>    - Use pi_bytesout + pi_bytesin instead of niovs (mst + 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>
> ---
>  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..7344906 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;
> +	u32 pi_bytesout;	/* DataOUT PI Number of bytes */
> +	u32 pi_bytesin;		/* DataIN PI Number of bytes */
> +	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


I'd like to add that it's strange to add a macro and
only use it in patch 5.
I believe this is one of the reasons bug in patch 5 went
unnoticed ...

>  
>  /* Response codes */
>  #define VIRTIO_SCSI_S_OK                       0
> -- 
> 1.7.10.4

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

[Michael S. Tsirkin]

On Thu, May 22, 2014 at 02:26:16AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi MST, MKP, Paolo & Co,
> 
> Here is the v2 patch series for adding T1O protection information (PI)
> SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
> endpoints.
> 
> Following MST's recommendation, it includes the changes for using
> bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
> the associated changes to virtio-scsi + vhost/scsi code.
> 
> For vhost/scsi, this includes walking the leading iovec's length(s)
> to determine where protection payload ends, and real data payload 
> starts.  For virtio-scsi LLD code, this includes locating struct
> blk_integrity for using blk_rq_sectors + ->tuple_size to calculate
> the total bytes for outgoing cmd_pi->pi_bytes[out,in] values.
> 
> The full list of changes from last series include:
> 
>   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
>     header (mst + paolo)
>   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
>     exists (nab)
>   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
>   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
>     of data_num in vhost_scsi_handle_vq (nab)
>   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
>   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
>   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
>   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
>     (mst + paolo + nab)
>   - Use blk_integrity->tuple_size to calculate pi bytes (nab)
> 
> Please review for v3.16-rc1 code.
> 
> Thanks!
> 
> --nab

OK since this conflicts with my vhost locking
patches, I have rebased this and parked at vhost-review
branch in my tree.

git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-review

Nicholas could you please take a look at the patches and confirm this is
OK ASAP?

If yes I will pack it all up and send to Linus.



> 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  |   86 +++++++++---
>  drivers/vhost/scsi.c        |  305 +++++++++++++++++++++++++++++--------------
>  include/linux/virtio_scsi.h |   15 ++-
>  3 files changed, 292 insertions(+), 114 deletions(-)
> 
> -- 
> 1.7.10.4

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

[Nicholas A. Bellinger]

On Mon, 2014-06-09 at 16:30 +0300, Michael S. Tsirkin wrote:
> On Thu, May 22, 2014 at 02:26:16AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Hi MST, MKP, Paolo & Co,
> > 
> > Here is the v2 patch series for adding T1O protection information (PI)
> > SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
> > endpoints.
> > 
> > Following MST's recommendation, it includes the changes for using
> > bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
> > the associated changes to virtio-scsi + vhost/scsi code.
> > 
> > For vhost/scsi, this includes walking the leading iovec's length(s)
> > to determine where protection payload ends, and real data payload 
> > starts.  For virtio-scsi LLD code, this includes locating struct
> > blk_integrity for using blk_rq_sectors + ->tuple_size to calculate
> > the total bytes for outgoing cmd_pi->pi_bytes[out,in] values.
> > 
> > The full list of changes from last series include:
> > 
> >   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
> >     header (mst + paolo)
> >   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
> >     exists (nab)
> >   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
> >   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
> >     of data_num in vhost_scsi_handle_vq (nab)
> >   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
> >   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
> >   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
> >   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
> >     (mst + paolo + nab)
> >   - Use blk_integrity->tuple_size to calculate pi bytes (nab)
> > 
> > Please review for v3.16-rc1 code.
> > 
> > Thanks!
> > 
> > --nab
> 
> OK since this conflicts with my vhost locking
> patches, I have rebased this and parked at vhost-review
> branch in my tree.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-review
> 
> Nicholas could you please take a look at the patches and confirm this is
> OK ASAP?
> 
> If yes I will pack it all up and send to Linus.

>From my experience, Linus prefers to fix this type of conflict on his
own at merge time, instead of having subsystem maintainers rebase their
for-next branches to include other's commits.

Stephen (CC'ed) has included a fix in today's linux-next for the merge
conflict here:

https://lkml.org/lkml/2014/6/10/3

Please confirm, as it will be a pointer to Linus within the
target-pending/for-next PULL request.

--nab

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

[Nicholas A. Bellinger]

On Sun, 2014-06-08 at 19:05 +0300, Michael S. Tsirkin wrote:
> On Thu, May 22, 2014 at 02:26:16AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Hi MST, MKP, Paolo & Co,
> > 
> > Here is the v2 patch series for adding T1O protection information (PI)
> > SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
> > endpoints.
> > 
> > Following MST's recommendation, it includes the changes for using
> > bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
> > the associated changes to virtio-scsi + vhost/scsi code.
> > 
> > For vhost/scsi, this includes walking the leading iovec's length(s)
> > to determine where protection payload ends, and real data payload 
> > starts.  For virtio-scsi LLD code, this includes locating struct
> > blk_integrity for using blk_rq_sectors + ->tuple_size to calculate
> > the total bytes for outgoing cmd_pi->pi_bytes[out,in] values.
> > 
> > The full list of changes from last series include:
> > 
> >   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
> >     header (mst + paolo)
> >   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
> >     exists (nab)
> >   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
> >   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
> >     of data_num in vhost_scsi_handle_vq (nab)
> >   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
> >   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
> >   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
> >   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
> >     (mst + paolo + nab)
> >   - Use blk_integrity->tuple_size to calculate pi bytes (nab)
> > 
> > Please review for v3.16-rc1 code.
> > 
> > Thanks!
> > 
> > --nab
> 
> 
> OK, finally went over this, looks good to me:
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> However, we really should stop making more changes
> before fixing ANY_LAYOUT in this driver.
> 
> virtio 1.0 should be out soon and that makes ANY_LAYOUT
> a required feature.
> 

What else is required to complete the ANY_LAYOUT conversion..?

--nab

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

[Paolo Bonzini]

Il 10/06/2014 09:07, Nicholas A. Bellinger ha scritto:
>> > OK, finally went over this, looks good to me:
>> >
>> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> >
>> > However, we really should stop making more changes
>> > before fixing ANY_LAYOUT in this driver.
>> >
>> > virtio 1.0 should be out soon and that makes ANY_LAYOUT
>> > a required feature.
>> >
> What else is required to complete the ANY_LAYOUT conversion..?

Basically, you need to stop expecting the request and response headers 
to be in a separate iov, and also vhost-scsi should not expect pi to be 
in a separate iov than the main data.  A layout that has everything in 
the same iov would be fine, and similarly for a layout that has the CDB 
in a separate iov than the rest of the header.

Paolo

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

[Michael S. Tsirkin]

On Tue, Jun 10, 2014 at 12:05:12AM -0700, Nicholas A. Bellinger wrote:
> On Mon, 2014-06-09 at 16:30 +0300, Michael S. Tsirkin wrote:
> > On Thu, May 22, 2014 at 02:26:16AM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > Hi MST, MKP, Paolo & Co,
> > > 
> > > Here is the v2 patch series for adding T1O protection information (PI)
> > > SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
> > > endpoints.
> > > 
> > > Following MST's recommendation, it includes the changes for using
> > > bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
> > > the associated changes to virtio-scsi + vhost/scsi code.
> > > 
> > > For vhost/scsi, this includes walking the leading iovec's length(s)
> > > to determine where protection payload ends, and real data payload 
> > > starts.  For virtio-scsi LLD code, this includes locating struct
> > > blk_integrity for using blk_rq_sectors + ->tuple_size to calculate
> > > the total bytes for outgoing cmd_pi->pi_bytes[out,in] values.
> > > 
> > > The full list of changes from last series include:
> > > 
> > >   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
> > >     header (mst + paolo)
> > >   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
> > >     exists (nab)
> > >   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
> > >   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
> > >     of data_num in vhost_scsi_handle_vq (nab)
> > >   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
> > >   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
> > >   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
> > >   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
> > >     (mst + paolo + nab)
> > >   - Use blk_integrity->tuple_size to calculate pi bytes (nab)
> > > 
> > > Please review for v3.16-rc1 code.
> > > 
> > > Thanks!
> > > 
> > > --nab
> > 
> > OK since this conflicts with my vhost locking
> > patches, I have rebased this and parked at vhost-review
> > branch in my tree.
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-review
> > 
> > Nicholas could you please take a look at the patches and confirm this is
> > OK ASAP?
> > 
> > If yes I will pack it all up and send to Linus.
> 
> >From my experience, Linus prefers to fix this type of conflict on his
> own at merge time, instead of having subsystem maintainers rebase their
> for-next branches to include other's commits.

Cross-device type changes is exactly what vhost tree is there to allow
so I don't see a problem.

What Linus does not want is same patch in two trees.

So I see two options:
- I go ahead with my changes and you with yours and let Linus resolve
  the conflict.  This means bisect build will be broken since the
  breakage will likely not be noticed until after the merge.
- I pick up vhost scsi PI patches on my tree and you drop them from yours.

I prefer option 2 because it's cleaner wrt bisect.
But if you see a problem, pls let me know.

> Stephen (CC'ed) has included a fix in today's linux-next for the merge
> conflict here:
> 
> https://lkml.org/lkml/2014/6/10/3
> 
> Please confirm, as it will be a pointer to Linus within the
> target-pending/for-next PULL request.
> 
> --nab


Yes but this does mean people trying to bisect will
hit build breakages, not nice.

-- 
MST

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

Hi Michael,

On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> So I see two options:
> - I go ahead with my changes and you with yours and let Linus resolve
>   the conflict.  This means bisect build will be broken since the
>   breakage will likely not be noticed until after the merge.

Well, since the resolution is known, the one who submits their tree
later should tell Linus (as suggested by Nicholas).  That is part of
the point of the linux-next tree ... and therefore there would be no
bisect problem.

> > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > conflict here:
> > 
> > https://lkml.org/lkml/2014/6/10/3
> > 
> > Please confirm, as it will be a pointer to Linus within the
> > target-pending/for-next PULL request.
> 
> Yes but this does mean people trying to bisect will
> hit build breakages, not nice.

Not necessarily.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Michael S. Tsirkin]

On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
> Hi Michael,
> 
> On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > So I see two options:
> > - I go ahead with my changes and you with yours and let Linus resolve
> >   the conflict.  This means bisect build will be broken since the
> >   breakage will likely not be noticed until after the merge.
> 
> Well, since the resolution is known, the one who submits their tree
> later should tell Linus (as suggested by Nicholas).  That is part of
> the point of the linux-next tree ... and therefore there would be no
> bisect problem.
> 
> > > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > > conflict here:
> > > 
> > > https://lkml.org/lkml/2014/6/10/3
> > > 
> > > Please confirm, as it will be a pointer to Linus within the
> > > target-pending/for-next PULL request.
> > 
> > Yes but this does mean people trying to bisect will
> > hit build breakages, not nice.
> 
> Not necessarily.
> 
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au


I don't see how that's possible.
Here's a point you might have missed.
Nicholas's patch isn't just introducing a merge conflict.
It is also buggy.
Replacing bit access with has_feature silently fixes the bug.

So if we want to avoid bisect breakage target tree will
have to be rebased.

And if doing that anyway, I don't see any reason not
to merge everything through the vhost tree, esp
since I already put the patches there. Less work for
everyone involved.

-- 
MST

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

Hi Michael,

On Tue, 10 Jun 2014 16:02:08 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> I don't see how that's possible.
> Here's a point you might have missed.
> Nicholas's patch isn't just introducing a merge conflict.
> It is also buggy.

Ah ha!  Indeed that is a different kettle of fish. :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Nicholas A. Bellinger]

On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
> > Hi Michael,
> > 
> > On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > So I see two options:
> > > - I go ahead with my changes and you with yours and let Linus resolve
> > >   the conflict.  This means bisect build will be broken since the
> > >   breakage will likely not be noticed until after the merge.
> > 
> > Well, since the resolution is known, the one who submits their tree
> > later should tell Linus (as suggested by Nicholas).  That is part of
> > the point of the linux-next tree ... and therefore there would be no
> > bisect problem.
> > 
> > > > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > > > conflict here:
> > > > 
> > > > https://lkml.org/lkml/2014/6/10/3
> > > > 
> > > > Please confirm, as it will be a pointer to Linus within the
> > > > target-pending/for-next PULL request.
> > > 
> > > Yes but this does mean people trying to bisect will
> > > hit build breakages, not nice.
> > 
> > Not necessarily.
> > 
> > -- 
> > Cheers,
> > Stephen Rothwell                    sfr@canb.auug.org.au
> 
> 
> I don't see how that's possible.
> Here's a point you might have missed.
> Nicholas's patch isn't just introducing a merge conflict.
> It is also buggy.
> Replacing bit access with has_feature silently fixes the bug.
> 
> So if we want to avoid bisect breakage target tree will
> have to be rebased.
> 
> And if doing that anyway, I don't see any reason not
> to merge everything through the vhost tree, esp
> since I already put the patches there. Less work for
> everyone involved.
> 

The problem is with Sagi's recent changes wrt to including T10 PI bytes
into expected data transfer length in target-core, you'll end up
introducing a different bug into your tree..  ;)

Why don't I simply add Stephen's patch to use vhost_has_feature() in
target-pending/for-next, and we just make sure that the vhost PULL
request goes out after target-pending..?

--nab

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

[Michael S. Tsirkin]

On Tue, Jun 10, 2014 at 10:39:17AM -0700, Nicholas A. Bellinger wrote:
> On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
> > > Hi Michael,
> > > 
> > > On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > So I see two options:
> > > > - I go ahead with my changes and you with yours and let Linus resolve
> > > >   the conflict.  This means bisect build will be broken since the
> > > >   breakage will likely not be noticed until after the merge.
> > > 
> > > Well, since the resolution is known, the one who submits their tree
> > > later should tell Linus (as suggested by Nicholas).  That is part of
> > > the point of the linux-next tree ... and therefore there would be no
> > > bisect problem.
> > > 
> > > > > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > > > > conflict here:
> > > > > 
> > > > > https://lkml.org/lkml/2014/6/10/3
> > > > > 
> > > > > Please confirm, as it will be a pointer to Linus within the
> > > > > target-pending/for-next PULL request.
> > > > 
> > > > Yes but this does mean people trying to bisect will
> > > > hit build breakages, not nice.
> > > 
> > > Not necessarily.
> > > 
> > > -- 
> > > Cheers,
> > > Stephen Rothwell                    sfr@canb.auug.org.au
> > 
> > 
> > I don't see how that's possible.
> > Here's a point you might have missed.
> > Nicholas's patch isn't just introducing a merge conflict.
> > It is also buggy.
> > Replacing bit access with has_feature silently fixes the bug.
> > 
> > So if we want to avoid bisect breakage target tree will
> > have to be rebased.
> > 
> > And if doing that anyway, I don't see any reason not
> > to merge everything through the vhost tree, esp
> > since I already put the patches there. Less work for
> > everyone involved.
> > 
> 
> The problem is with Sagi's recent changes wrt to including T10 PI bytes
> into expected data transfer length in target-core, you'll end up
> introducing a different bug into your tree..  ;)
> 
> Why don't I simply add Stephen's patch to use vhost_has_feature() in
> target-pending/for-next, and we just make sure that the vhost PULL
> request goes out after target-pending..?
> 
> --nab

Because that depends on vhost API changes :)

-- 
MST

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

[Michael S. Tsirkin]

On Tue, Jun 10, 2014 at 10:39:17AM -0700, Nicholas A. Bellinger wrote:
> On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
> > > Hi Michael,
> > > 
> > > On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > So I see two options:
> > > > - I go ahead with my changes and you with yours and let Linus resolve
> > > >   the conflict.  This means bisect build will be broken since the
> > > >   breakage will likely not be noticed until after the merge.
> > > 
> > > Well, since the resolution is known, the one who submits their tree
> > > later should tell Linus (as suggested by Nicholas).  That is part of
> > > the point of the linux-next tree ... and therefore there would be no
> > > bisect problem.
> > > 
> > > > > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > > > > conflict here:
> > > > > 
> > > > > https://lkml.org/lkml/2014/6/10/3
> > > > > 
> > > > > Please confirm, as it will be a pointer to Linus within the
> > > > > target-pending/for-next PULL request.
> > > > 
> > > > Yes but this does mean people trying to bisect will
> > > > hit build breakages, not nice.
> > > 
> > > Not necessarily.
> > > 
> > > -- 
> > > Cheers,
> > > Stephen Rothwell                    sfr@canb.auug.org.au
> > 
> > 
> > I don't see how that's possible.
> > Here's a point you might have missed.
> > Nicholas's patch isn't just introducing a merge conflict.
> > It is also buggy.
> > Replacing bit access with has_feature silently fixes the bug.
> > 
> > So if we want to avoid bisect breakage target tree will
> > have to be rebased.
> > 
> > And if doing that anyway, I don't see any reason not
> > to merge everything through the vhost tree, esp
> > since I already put the patches there. Less work for
> > everyone involved.
> > 
> 
> The problem is with Sagi's recent changes wrt to including T10 PI bytes
> into expected data transfer length in target-core, you'll end up
> introducing a different bug into your tree..  ;)

I thought you wanted to fix it after -rc1 anyway?

> Why don't I simply add Stephen's patch to use vhost_has_feature() in
> target-pending/for-next, and we just make sure that the vhost PULL
> request goes out after target-pending..?
> 
> --nab

I can drop the PI feature bit in rc1. You will apply Sagi's changes and
then enable the feature in rc2?

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

[Nicholas A. Bellinger]

On Tue, 2014-06-10 at 22:35 +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 10, 2014 at 10:39:17AM -0700, Nicholas A. Bellinger wrote:
> > On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
> > > > Hi Michael,
> > > > 
> > > > On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >
> > > > > So I see two options:
> > > > > - I go ahead with my changes and you with yours and let Linus resolve
> > > > >   the conflict.  This means bisect build will be broken since the
> > > > >   breakage will likely not be noticed until after the merge.
> > > > 
> > > > Well, since the resolution is known, the one who submits their tree
> > > > later should tell Linus (as suggested by Nicholas).  That is part of
> > > > the point of the linux-next tree ... and therefore there would be no
> > > > bisect problem.
> > > > 
> > > > > > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > > > > > conflict here:
> > > > > > 
> > > > > > https://lkml.org/lkml/2014/6/10/3
> > > > > > 
> > > > > > Please confirm, as it will be a pointer to Linus within the
> > > > > > target-pending/for-next PULL request.
> > > > > 
> > > > > Yes but this does mean people trying to bisect will
> > > > > hit build breakages, not nice.
> > > > 
> > > > Not necessarily.
> > > > 
> > > > -- 
> > > > Cheers,
> > > > Stephen Rothwell                    sfr@canb.auug.org.au
> > > 
> > > 
> > > I don't see how that's possible.
> > > Here's a point you might have missed.
> > > Nicholas's patch isn't just introducing a merge conflict.
> > > It is also buggy.
> > > Replacing bit access with has_feature silently fixes the bug.
> > > 
> > > So if we want to avoid bisect breakage target tree will
> > > have to be rebased.
> > > 
> > > And if doing that anyway, I don't see any reason not
> > > to merge everything through the vhost tree, esp
> > > since I already put the patches there. Less work for
> > > everyone involved.
> > > 
> > 
> > The problem is with Sagi's recent changes wrt to including T10 PI bytes
> > into expected data transfer length in target-core, you'll end up
> > introducing a different bug into your tree..  ;)
> 
> I thought you wanted to fix it after -rc1 anyway?
> 

I've merged Sagi's patches and fixed up the resulting vhost-scsi
breakage in target-pending/for-next here:

https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?h=for-next&id=4101fc0abffeef604b14a707a3f9c9e2a8e39085

Only the qla2xxx target DIF related breakage remains, because those
patches are going through the scsi/for-next tree and will need to be
fixed in -rc2.

> > Why don't I simply add Stephen's patch to use vhost_has_feature() in
> > target-pending/for-next, and we just make sure that the vhost PULL
> > request goes out after target-pending..?
> > 
> > --nab
> 
> I can drop the PI feature bit in rc1. You will apply Sagi's changes and
> then enable the feature in rc2?

That is not necessary.  I'll just apply Stephen's patch to use
vhost_has_feature(), and then we make sure that target-pending is merged
before vhost to avoid the build breakage.

I'll be sending out the target-pending PULL request tomorrow afternoon
and will CC' you.

--nab

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

[Nicholas A. Bellinger]

On Tue, 2014-06-10 at 21:45 +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 10, 2014 at 10:39:17AM -0700, Nicholas A. Bellinger wrote:
> > On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
> > > > Hi Michael,
> > > > 
> > > > On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >
> > > > > So I see two options:
> > > > > - I go ahead with my changes and you with yours and let Linus resolve
> > > > >   the conflict.  This means bisect build will be broken since the
> > > > >   breakage will likely not be noticed until after the merge.
> > > > 
> > > > Well, since the resolution is known, the one who submits their tree
> > > > later should tell Linus (as suggested by Nicholas).  That is part of
> > > > the point of the linux-next tree ... and therefore there would be no
> > > > bisect problem.
> > > > 
> > > > > > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > > > > > conflict here:
> > > > > > 
> > > > > > https://lkml.org/lkml/2014/6/10/3
> > > > > > 
> > > > > > Please confirm, as it will be a pointer to Linus within the
> > > > > > target-pending/for-next PULL request.
> > > > > 
> > > > > Yes but this does mean people trying to bisect will
> > > > > hit build breakages, not nice.
> > > > 
> > > > Not necessarily.
> > > > 
> > > > -- 
> > > > Cheers,
> > > > Stephen Rothwell                    sfr@canb.auug.org.au
> > > 
> > > 
> > > I don't see how that's possible.
> > > Here's a point you might have missed.
> > > Nicholas's patch isn't just introducing a merge conflict.
> > > It is also buggy.
> > > Replacing bit access with has_feature silently fixes the bug.
> > > 
> > > So if we want to avoid bisect breakage target tree will
> > > have to be rebased.
> > > 
> > > And if doing that anyway, I don't see any reason not
> > > to merge everything through the vhost tree, esp
> > > since I already put the patches there. Less work for
> > > everyone involved.
> > > 
> > 
> > The problem is with Sagi's recent changes wrt to including T10 PI bytes
> > into expected data transfer length in target-core, you'll end up
> > introducing a different bug into your tree..  ;)
> > 
> > Why don't I simply add Stephen's patch to use vhost_has_feature() in
> > target-pending/for-next, and we just make sure that the vhost PULL
> > request goes out after target-pending..?
> > 
> > --nab
> 
> Because that depends on vhost API changes :)
> 

Ugh, right.

In that case, let's see what Linus (CC'ed) prefers to do..

--nab

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

[James Bottomley]

On Tue, 2014-06-10 at 12:57 -0700, Nicholas A. Bellinger wrote:
> On Tue, 2014-06-10 at 21:45 +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 10, 2014 at 10:39:17AM -0700, Nicholas A. Bellinger wrote:
> > > On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
> > > > > Hi Michael,
> > > > > 
> > > > > On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >
> > > > > > So I see two options:
> > > > > > - I go ahead with my changes and you with yours and let Linus resolve
> > > > > >   the conflict.  This means bisect build will be broken since the
> > > > > >   breakage will likely not be noticed until after the merge.
> > > > > 
> > > > > Well, since the resolution is known, the one who submits their tree
> > > > > later should tell Linus (as suggested by Nicholas).  That is part of
> > > > > the point of the linux-next tree ... and therefore there would be no
> > > > > bisect problem.
> > > > > 
> > > > > > > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > > > > > > conflict here:
> > > > > > > 
> > > > > > > https://lkml.org/lkml/2014/6/10/3
> > > > > > > 
> > > > > > > Please confirm, as it will be a pointer to Linus within the
> > > > > > > target-pending/for-next PULL request.
> > > > > > 
> > > > > > Yes but this does mean people trying to bisect will
> > > > > > hit build breakages, not nice.
> > > > > 
> > > > > Not necessarily.
> > > > > 
> > > > > -- 
> > > > > Cheers,
> > > > > Stephen Rothwell                    sfr@canb.auug.org.au
> > > > 
> > > > 
> > > > I don't see how that's possible.
> > > > Here's a point you might have missed.
> > > > Nicholas's patch isn't just introducing a merge conflict.
> > > > It is also buggy.
> > > > Replacing bit access with has_feature silently fixes the bug.
> > > > 
> > > > So if we want to avoid bisect breakage target tree will
> > > > have to be rebased.
> > > > 
> > > > And if doing that anyway, I don't see any reason not
> > > > to merge everything through the vhost tree, esp
> > > > since I already put the patches there. Less work for
> > > > everyone involved.
> > > > 
> > > 
> > > The problem is with Sagi's recent changes wrt to including T10 PI bytes
> > > into expected data transfer length in target-core, you'll end up
> > > introducing a different bug into your tree..  ;)
> > > 
> > > Why don't I simply add Stephen's patch to use vhost_has_feature() in
> > > target-pending/for-next, and we just make sure that the vhost PULL
> > > request goes out after target-pending..?
> > > 
> > > --nab
> > 
> > Because that depends on vhost API changes :)
> > 
> 
> Ugh, right.
> 
> In that case, let's see what Linus (CC'ed) prefers to do..

Build a branch with all the patches dependent on the new API based on
top of the vhost tree.  Then you send it to Linus after the vhost tree
is merged (otherwise you end up being the person sending the vhost
tree).

That way there's no API breakage and we get a consistent always
buildable tree.

James

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

[Nicholas A. Bellinger]

On Tue, 2014-06-10 at 13:09 -0700, James Bottomley wrote:
> On Tue, 2014-06-10 at 12:57 -0700, Nicholas A. Bellinger wrote:
> > On Tue, 2014-06-10 at 21:45 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 10, 2014 at 10:39:17AM -0700, Nicholas A. Bellinger wrote:
> > > > On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
> > > > > > Hi Michael,
> > > > > > 
> > > > > > On Tue, 10 Jun 2014 12:42:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > So I see two options:
> > > > > > > - I go ahead with my changes and you with yours and let Linus resolve
> > > > > > >   the conflict.  This means bisect build will be broken since the
> > > > > > >   breakage will likely not be noticed until after the merge.
> > > > > > 
> > > > > > Well, since the resolution is known, the one who submits their tree
> > > > > > later should tell Linus (as suggested by Nicholas).  That is part of
> > > > > > the point of the linux-next tree ... and therefore there would be no
> > > > > > bisect problem.
> > > > > > 
> > > > > > > > Stephen (CC'ed) has included a fix in today's linux-next for the merge
> > > > > > > > conflict here:
> > > > > > > > 
> > > > > > > > https://lkml.org/lkml/2014/6/10/3
> > > > > > > > 
> > > > > > > > Please confirm, as it will be a pointer to Linus within the
> > > > > > > > target-pending/for-next PULL request.
> > > > > > > 
> > > > > > > Yes but this does mean people trying to bisect will
> > > > > > > hit build breakages, not nice.
> > > > > > 
> > > > > > Not necessarily.
> > > > > > 
> > > > > > -- 
> > > > > > Cheers,
> > > > > > Stephen Rothwell                    sfr@canb.auug.org.au
> > > > > 
> > > > > 
> > > > > I don't see how that's possible.
> > > > > Here's a point you might have missed.
> > > > > Nicholas's patch isn't just introducing a merge conflict.
> > > > > It is also buggy.
> > > > > Replacing bit access with has_feature silently fixes the bug.
> > > > > 
> > > > > So if we want to avoid bisect breakage target tree will
> > > > > have to be rebased.
> > > > > 
> > > > > And if doing that anyway, I don't see any reason not
> > > > > to merge everything through the vhost tree, esp
> > > > > since I already put the patches there. Less work for
> > > > > everyone involved.
> > > > > 
> > > > 
> > > > The problem is with Sagi's recent changes wrt to including T10 PI bytes
> > > > into expected data transfer length in target-core, you'll end up
> > > > introducing a different bug into your tree..  ;)
> > > > 
> > > > Why don't I simply add Stephen's patch to use vhost_has_feature() in
> > > > target-pending/for-next, and we just make sure that the vhost PULL
> > > > request goes out after target-pending..?
> > > > 
> > > > --nab
> > > 
> > > Because that depends on vhost API changes :)
> > > 
> > 
> > Ugh, right.
> > 
> > In that case, let's see what Linus (CC'ed) prefers to do..
> 
> Build a branch with all the patches dependent on the new API based on
> top of the vhost tree.  Then you send it to Linus after the vhost tree
> is merged (otherwise you end up being the person sending the vhost
> tree).
> 
> That way there's no API breakage and we get a consistent always
> buildable tree.
> 

That would work, or I can simply include a pointer to Stephen's patch in
the target-pending PULL request after the vhost API changes are merged
and Linus can apply himself..

Linus, which do you prefer..?

--nab

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

[Linus Torvalds]

On Tue, Jun 10, 2014 at 1:25 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
>
> That would work, or I can simply include a pointer to Stephen's patch in
> the target-pending PULL request after the vhost API changes are merged
> and Linus can apply himself..

Yes. That way I'll include it in the merge, and everything should just work.

          Linus

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

[Nicholas A. Bellinger]

On Tue, 2014-06-10 at 13:56 -0700, Linus Torvalds wrote:
> On Tue, Jun 10, 2014 at 1:25 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> >
> > That would work, or I can simply include a pointer to Stephen's patch in
> > the target-pending PULL request after the vhost API changes are merged
> > and Linus can apply himself..
> 
> Yes. That way I'll include it in the merge, and everything should just work.
> 

<nod>, sounds good.

MST, please drop the target related patches from your tree, and go ahead
and send your PULL request now.

--nab

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

[Michael S. Tsirkin]

On Tue, Jun 10, 2014 at 02:20:28PM -0700, Nicholas A. Bellinger wrote:
> On Tue, 2014-06-10 at 13:56 -0700, Linus Torvalds wrote:
> > On Tue, Jun 10, 2014 at 1:25 PM, Nicholas A. Bellinger
> > <nab@linux-iscsi.org> wrote:
> > >
> > > That would work, or I can simply include a pointer to Stephen's patch in
> > > the target-pending PULL request after the vhost API changes are merged
> > > and Linus can apply himself..
> > 
> > Yes. That way I'll include it in the merge, and everything should just work.
> > 
> 
> <nod>, sounds good.
> 
> MST, please drop the target related patches from your tree, and go ahead
> and send your PULL request now.
> 
> --nab

ack.
Doing it right now.