[PATCH-v2 00/15] target: Add WRITE_STRIP + READ_INSERT support

[PATCH-v2 00/15] target: Add WRITE_STRIP + READ_INSERT support

[Nicholas A. Bellinger]

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

Hi MKP, Sagi, Quinn, & Co,

Here is -v2 for adding target internal WRITE_STRIP + READ_INSERT
DIX emulation using target_core_sbc.c + crct10dif.ko code.

This mode is for allowing T10-PI capable fabrics to interoperate with
backend devices that have not yet had protection support enabled.
These operations can be done internally using target DIF emulation,
or by the fabric driver's own HBA/NIC hardware offloads.

This includes initial support for tcm_loop, vhost/scsi, tcm_qla2xxx
and iscsi/iser-target using a new endpoint 'fabric_prot_type' configfs
attribute, and associated changes to allow FILEIO + IBLOCK + RAMDISK
backends to function in fabric_prot_type > 0 mode.

The v2 changes include:
  - Allow iblock + ramdisk to function with fabric_prot_type
  - Add fabric_prot_type attributes for vhost/scsi + tcm_qla2xxx +
    iscsi/iser-target
  - Add missing TARGET_PROT_ALL for tcm_qla2xxx

Please review for v4.1 code.

Thank you,

--nab

Nicholas Bellinger (15):
  target: Convert DIF emulation to use cmd->prot_type
  target: Add protected fabric + unprotected device support
  target: Update SPC/SBC emulation for sess_prot_type
  target: Move cmd->prot_op check into target_check_write_prot
  target: Add internal WRITE_STRIP support
  target: Move cmd->prot_op check into target_check_read_prot
  target: Add internal READ_INSERT support
  target/file: Add checks for backend DIF emulation
  target/iblock: Add checks for backend DIF emulation
  target/rd: Add checks for backend DIF emulation
  loopback: Add fabric_prot_type attribute support
  vhost/scsi: Add fabric_prot_type attribute support
  tcm_qla2xxx: Set TARGET_PROT_ALL for sup_prot_ops
  tcm_qla2xxx: Add fabric_prot_type attribute support
  iscsi/iser-target: Add fabric_prot_type attribute support

 drivers/scsi/qla2xxx/tcm_qla2xxx.c           | 46 +++++++++++++++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.h           |  1 +
 drivers/target/iscsi/iscsi_target_configfs.c | 22 ++++++++
 drivers/target/iscsi/iscsi_target_tpg.c      | 19 +++++++
 drivers/target/iscsi/iscsi_target_tpg.h      |  1 +
 drivers/target/loopback/tcm_loop.c           | 54 +++++++++++++++++-
 drivers/target/loopback/tcm_loop.h           |  1 +
 drivers/target/target_core_file.c            |  8 +--
 drivers/target/target_core_iblock.c          |  2 +-
 drivers/target/target_core_rd.c              |  6 +-
 drivers/target/target_core_sbc.c             | 73 ++++++++++++++++++-------
 drivers/target/target_core_spc.c             | 14 +++--
 drivers/target/target_core_transport.c       | 82 ++++++++++++++++++++++------
 drivers/vhost/scsi.c                         | 52 +++++++++++++++++-
 include/target/iscsi/iscsi_target_core.h     |  2 +
 include/target/target_core_base.h            |  1 +
 include/target/target_core_fabric.h          |  8 +++
 17 files changed, 340 insertions(+), 52 deletions(-)

-- 
1.9.1

[PATCH-v2 01/15] target: Convert DIF emulation to use cmd->prot_type

[Nicholas A. Bellinger]

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

This patch changes existing DIF emulation to check the command descriptor's
prot_type, instead of what the backend device is exposing in pi_prot_type.

Since this value is already set in sbc_check_prot(), go ahead and use it to
allow protected fabrics to function with unprotected devices.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 9a2f9d3..95a7a74 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1167,7 +1167,7 @@ sbc_dif_generate(struct se_cmd *cmd)
 			sdt = paddr + offset;
 			sdt->guard_tag = cpu_to_be16(crc_t10dif(daddr + j,
 						dev->dev_attrib.block_size));
-			if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
+			if (cmd->prot_type == TARGET_DIF_TYPE1_PROT)
 				sdt->ref_tag = cpu_to_be32(sector & 0xffffffff);
 			sdt->app_tag = 0;
 
@@ -1186,9 +1186,10 @@ sbc_dif_generate(struct se_cmd *cmd)
 }
 
 static sense_reason_t
-sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
+sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
 		  const void *p, sector_t sector, unsigned int ei_lba)
 {
+	struct se_device *dev = cmd->se_dev;
 	int block_size = dev->dev_attrib.block_size;
 	__be16 csum;
 
@@ -1201,7 +1202,7 @@ sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
 		return TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED;
 	}
 
-	if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT &&
+	if (cmd->prot_type == TARGET_DIF_TYPE1_PROT &&
 	    be32_to_cpu(sdt->ref_tag) != (sector & 0xffffffff)) {
 		pr_err("DIFv1 Type 1 reference failed on sector: %llu tag: 0x%08x"
 		       " sector MSB: 0x%08x\n", (unsigned long long)sector,
@@ -1209,7 +1210,7 @@ sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
 		return TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED;
 	}
 
-	if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE2_PROT &&
+	if (cmd->prot_type == TARGET_DIF_TYPE2_PROT &&
 	    be32_to_cpu(sdt->ref_tag) != ei_lba) {
 		pr_err("DIFv1 Type 2 reference failed on sector: %llu tag: 0x%08x"
 		       " ei_lba: 0x%08x\n", (unsigned long long)sector,
@@ -1293,7 +1294,7 @@ sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 				 (unsigned long long)sector, sdt->guard_tag,
 				 sdt->app_tag, be32_to_cpu(sdt->ref_tag));
 
-			rc = sbc_dif_v1_verify(dev, sdt, daddr + j, sector,
+			rc = sbc_dif_v1_verify(cmd, sdt, daddr + j, sector,
 					       ei_lba);
 			if (rc) {
 				kunmap_atomic(paddr);
@@ -1354,7 +1355,7 @@ __sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 				continue;
 			}
 
-			rc = sbc_dif_v1_verify(dev, sdt, daddr + j, sector,
+			rc = sbc_dif_v1_verify(cmd, sdt, daddr + j, sector,
 					       ei_lba);
 			if (rc) {
 				kunmap_atomic(paddr);
-- 
1.9.1

Re: [PATCH-v2 01/15] target: Convert DIF emulation to use cmd->prot_type

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch changes existing DIF emulation to check the command descriptor's
> prot_type, instead of what the backend device is exposing in pi_prot_type.
>
> Since this value is already set in sbc_check_prot(), go ahead and use it to
> allow protected fabrics to function with unprotected devices.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 9a2f9d3..95a7a74 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -1167,7 +1167,7 @@ sbc_dif_generate(struct se_cmd *cmd)
>   			sdt = paddr + offset;
>   			sdt->guard_tag = cpu_to_be16(crc_t10dif(daddr + j,
>   						dev->dev_attrib.block_size));
> -			if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
> +			if (cmd->prot_type == TARGET_DIF_TYPE1_PROT)
>   				sdt->ref_tag = cpu_to_be32(sector & 0xffffffff);
>   			sdt->app_tag = 0;
>
> @@ -1186,9 +1186,10 @@ sbc_dif_generate(struct se_cmd *cmd)
>   }
>
>   static sense_reason_t
> -sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
> +sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
>   		  const void *p, sector_t sector, unsigned int ei_lba)
>   {
> +	struct se_device *dev = cmd->se_dev;
>   	int block_size = dev->dev_attrib.block_size;
>   	__be16 csum;
>
> @@ -1201,7 +1202,7 @@ sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
>   		return TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED;
>   	}
>
> -	if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT &&
> +	if (cmd->prot_type == TARGET_DIF_TYPE1_PROT &&
>   	    be32_to_cpu(sdt->ref_tag) != (sector & 0xffffffff)) {
>   		pr_err("DIFv1 Type 1 reference failed on sector: %llu tag: 0x%08x"
>   		       " sector MSB: 0x%08x\n", (unsigned long long)sector,
> @@ -1209,7 +1210,7 @@ sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
>   		return TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED;
>   	}
>
> -	if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE2_PROT &&
> +	if (cmd->prot_type == TARGET_DIF_TYPE2_PROT &&
>   	    be32_to_cpu(sdt->ref_tag) != ei_lba) {
>   		pr_err("DIFv1 Type 2 reference failed on sector: %llu tag: 0x%08x"
>   		       " ei_lba: 0x%08x\n", (unsigned long long)sector,
> @@ -1293,7 +1294,7 @@ sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
>   				 (unsigned long long)sector, sdt->guard_tag,
>   				 sdt->app_tag, be32_to_cpu(sdt->ref_tag));
>
> -			rc = sbc_dif_v1_verify(dev, sdt, daddr + j, sector,
> +			rc = sbc_dif_v1_verify(cmd, sdt, daddr + j, sector,
>   					       ei_lba);
>   			if (rc) {
>   				kunmap_atomic(paddr);
> @@ -1354,7 +1355,7 @@ __sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
>   				continue;
>   			}
>
> -			rc = sbc_dif_v1_verify(dev, sdt, daddr + j, sector,
> +			rc = sbc_dif_v1_verify(cmd, sdt, daddr + j, sector,
>   					       ei_lba);
>   			if (rc) {
>   				kunmap_atomic(paddr);
>

Looks good.

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH-v2 01/15] target: Convert DIF emulation to use cmd->prot_type

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@daterainc.com> writes:

nab> This patch changes existing DIF emulation to check the command
nab> descriptor's prot_type, instead of what the backend device is
nab> exposing in pi_prot_type.

nab> Since this value is already set in sbc_check_prot(), go ahead and
nab> use it to allow protected fabrics to function with unprotected
nab> devices.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Nicholas A. Bellinger]

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

This patch adds a new target_core_fabric_ops callback for allowing fabric
drivers to expose a TPG attribute for signaling when a T10-PI protected
fabric wants to function with an un-protected device without T10-PI.

This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
operations when functioning with non T10-PI enabled devices, seperate
from any available hw offloads the fabric supports.

This is done using a new se_sess->sess_prot_type that is set at fabric
session creation time based upon the TPG attribute.  It currently cannot
be changed for individual sessions after initial creation.

Also, update existing target_core_sbc.c code to honor sess_prot_type when
setting up cmd->prot_op + cmd->prot_type assignments.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Doug Gilbert <dgilbert@interlog.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
 drivers/target/target_core_transport.c |  8 +++++++
 include/target/target_core_base.h      |  1 +
 include/target/target_core_fabric.h    |  8 +++++++
 4 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 95a7a74..5b3564a 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
 }
 
 static int
-sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
+sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
 		       bool is_write, struct se_cmd *cmd)
 {
 	if (is_write) {
-		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
-					 TARGET_PROT_DOUT_INSERT;
+		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
+			       protect ? TARGET_PROT_DOUT_PASS :
+			       TARGET_PROT_DOUT_INSERT;
 		switch (protect) {
 		case 0x0:
 		case 0x3:
@@ -610,8 +611,9 @@ sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
 			return -EINVAL;
 		}
 	} else {
-		cmd->prot_op = protect ? TARGET_PROT_DIN_PASS :
-					 TARGET_PROT_DIN_STRIP;
+		cmd->prot_op = fabric_prot ? TARGET_PROT_DIN_INSERT :
+			       protect ? TARGET_PROT_DIN_PASS :
+			       TARGET_PROT_DIN_STRIP;
 		switch (protect) {
 		case 0x0:
 		case 0x1:
@@ -644,11 +646,15 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 	       u32 sectors, bool is_write)
 {
 	u8 protect = cdb[1] >> 5;
+	int sp_ops = cmd->se_sess->sup_prot_ops;
+	int pi_prot_type = dev->dev_attrib.pi_prot_type;
+	bool fabric_prot = false;
 
 	if (!cmd->t_prot_sg || !cmd->t_prot_nents) {
-		if (protect && !dev->dev_attrib.pi_prot_type) {
-			pr_err("CDB contains protect bit, but device does not"
-			       " advertise PROTECT=1 feature bit\n");
+		if (protect &&
+		    !dev->dev_attrib.pi_prot_type && !cmd->se_sess->sess_prot_type) {
+			pr_err("CDB contains protect bit, but device + fabric does"
+			       " not advertise PROTECT=1 feature bit\n");
 			return TCM_INVALID_CDB_FIELD;
 		}
 		if (cmd->prot_pto)
@@ -669,15 +675,28 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 		cmd->reftag_seed = cmd->t_task_lba;
 		break;
 	case TARGET_DIF_TYPE0_PROT:
+		/*
+		 * See if the fabric supports T10-PI, and the session has been
+		 * configured to allow export PROTECT=1 feature bit with backend
+		 * devices that don't support T10-PI.
+		 */
+		fabric_prot = is_write ?
+			      (sp_ops & (TARGET_PROT_DOUT_PASS | TARGET_PROT_DOUT_STRIP)) :
+			      (sp_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DIN_INSERT));
+
+		if (fabric_prot && cmd->se_sess->sess_prot_type) {
+			pi_prot_type = cmd->se_sess->sess_prot_type;
+			break;
+		}
+		/* Fallthrough */
 	default:
 		return TCM_NO_SENSE;
 	}
 
-	if (sbc_set_prot_op_checks(protect, dev->dev_attrib.pi_prot_type,
-				   is_write, cmd))
+	if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
 		return TCM_INVALID_CDB_FIELD;
 
-	cmd->prot_type = dev->dev_attrib.pi_prot_type;
+	cmd->prot_type = pi_prot_type;
 	cmd->prot_length = dev->prot_length * sectors;
 
 	/**
@@ -1231,6 +1250,9 @@ sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
 	unsigned int i, len, left;
 	unsigned int offset = sg_off;
 
+	if (!sg)
+		return;
+
 	left = sectors * dev->prot_length;
 
 	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4a00ed5..aef989e 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -322,11 +322,19 @@ void __transport_register_session(
 	struct se_session *se_sess,
 	void *fabric_sess_ptr)
 {
+	struct target_core_fabric_ops *tfo = se_tpg->se_tpg_tfo;
 	unsigned char buf[PR_REG_ISID_LEN];
 
 	se_sess->se_tpg = se_tpg;
 	se_sess->fabric_sess_ptr = fabric_sess_ptr;
 	/*
+	 * Determine if fabric allows for T10-PI feature bits to be exposed
+	 * to initiators for device backends with !dev->dev_attrib.pi_prot_type
+	 */
+	if (tfo->tpg_check_prot_fabric_only)
+		se_sess->sess_prot_type = tfo->tpg_check_prot_fabric_only(se_tpg);
+
+	/*
 	 * Used by struct se_node_acl's under ConfigFS to locate active se_session-t
 	 *
 	 * Only set for struct se_session's that will actually be moving I/O.
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 672150b..fe25a78 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -616,6 +616,7 @@ struct se_session {
 	unsigned		sess_tearing_down:1;
 	u64			sess_bin_isid;
 	enum target_prot_op	sup_prot_ops;
+	enum target_prot_type	sess_prot_type;
 	struct se_node_acl	*se_node_acl;
 	struct se_portal_group *se_tpg;
 	void			*fabric_sess_ptr;
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 2f4a250..c93cfdf 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -27,6 +27,14 @@ struct target_core_fabric_ops {
 	 * inquiry response
 	 */
 	int (*tpg_check_demo_mode_login_only)(struct se_portal_group *);
+	/*
+	 * Optionally used as a configfs tunable to determine when
+	 * target-core should signal the PROTECT=1 feature bit for
+	 * backends that don't support T10-PI, so that either fabric
+	 * HW offload or target-core emulation performs the associated
+	 * WRITE_STRIP and READ_INSERT operations.
+	 */
+	int (*tpg_check_prot_fabric_only)(struct se_portal_group *);
 	struct se_node_acl *(*tpg_alloc_fabric_acl)(
 					struct se_portal_group *);
 	void (*tpg_release_fabric_acl)(struct se_portal_group *,
-- 
1.9.1

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch adds a new target_core_fabric_ops callback for allowing fabric
> drivers to expose a TPG attribute for signaling when a T10-PI protected
> fabric wants to function with an un-protected device without T10-PI.
>
> This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
> operations when functioning with non T10-PI enabled devices, seperate
> from any available hw offloads the fabric supports.
>
> This is done using a new se_sess->sess_prot_type that is set at fabric
> session creation time based upon the TPG attribute.  It currently cannot
> be changed for individual sessions after initial creation.
>
> Also, update existing target_core_sbc.c code to honor sess_prot_type when
> setting up cmd->prot_op + cmd->prot_type assignments.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Doug Gilbert <dgilbert@interlog.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
>   drivers/target/target_core_transport.c |  8 +++++++
>   include/target/target_core_base.h      |  1 +
>   include/target/target_core_fabric.h    |  8 +++++++
>   4 files changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 95a7a74..5b3564a 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
>   }
>
>   static int
> -sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
> +sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
>   		       bool is_write, struct se_cmd *cmd)
>   {
>   	if (is_write) {
> -		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
> -					 TARGET_PROT_DOUT_INSERT;
> +		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
> +			       protect ? TARGET_PROT_DOUT_PASS :
> +			       TARGET_PROT_DOUT_INSERT;

In this case, if the protect=1 and fabric_prot=1 we will strip won't we?
I think that the protect condition should come first.

>   		switch (protect) {
>   		case 0x0:
>   		case 0x3:
> @@ -610,8 +611,9 @@ sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
>   			return -EINVAL;
>   		}
>   	} else {
> -		cmd->prot_op = protect ? TARGET_PROT_DIN_PASS :
> -					 TARGET_PROT_DIN_STRIP;
> +		cmd->prot_op = fabric_prot ? TARGET_PROT_DIN_INSERT :
> +			       protect ? TARGET_PROT_DIN_PASS :
> +			       TARGET_PROT_DIN_STRIP;

Same here.

>   		switch (protect) {
>   		case 0x0:
>   		case 0x1:
> @@ -644,11 +646,15 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   	       u32 sectors, bool is_write)
>   {
>   	u8 protect = cdb[1] >> 5;
> +	int sp_ops = cmd->se_sess->sup_prot_ops;
> +	int pi_prot_type = dev->dev_attrib.pi_prot_type;
> +	bool fabric_prot = false;
>
>   	if (!cmd->t_prot_sg || !cmd->t_prot_nents) {
> -		if (protect && !dev->dev_attrib.pi_prot_type) {
> -			pr_err("CDB contains protect bit, but device does not"
> -			       " advertise PROTECT=1 feature bit\n");
> +		if (protect &&
> +		    !dev->dev_attrib.pi_prot_type && !cmd->se_sess->sess_prot_type) {
> +			pr_err("CDB contains protect bit, but device + fabric does"
> +			       " not advertise PROTECT=1 feature bit\n");

Can you place unlikely on these conditions?

>   			return TCM_INVALID_CDB_FIELD;
>   		}
>   		if (cmd->prot_pto)
> @@ -669,15 +675,28 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   		cmd->reftag_seed = cmd->t_task_lba;
>   		break;
>   	case TARGET_DIF_TYPE0_PROT:
> +		/*
> +		 * See if the fabric supports T10-PI, and the session has been
> +		 * configured to allow export PROTECT=1 feature bit with backend
> +		 * devices that don't support T10-PI.
> +		 */
> +		fabric_prot = is_write ?
> +			      (sp_ops & (TARGET_PROT_DOUT_PASS | TARGET_PROT_DOUT_STRIP)) :

Shouldn't this be converted to bool using !!()?


> +			      (sp_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DIN_INSERT));
> +
> +		if (fabric_prot && cmd->se_sess->sess_prot_type) {
> +			pi_prot_type = cmd->se_sess->sess_prot_type;
> +			break;
> +		}
> +		/* Fallthrough */
>   	default:
>   		return TCM_NO_SENSE;
>   	}
>
> -	if (sbc_set_prot_op_checks(protect, dev->dev_attrib.pi_prot_type,
> -				   is_write, cmd))
> +	if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
>   		return TCM_INVALID_CDB_FIELD;
>
> -	cmd->prot_type = dev->dev_attrib.pi_prot_type;
> +	cmd->prot_type = pi_prot_type;
>   	cmd->prot_length = dev->prot_length * sectors;
>
>   	/**
> @@ -1231,6 +1250,9 @@ sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
>   	unsigned int i, len, left;
>   	unsigned int offset = sg_off;
>
> +	if (!sg)
> +		return;
> +
>   	left = sectors * dev->prot_length;
>
>   	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 4a00ed5..aef989e 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -322,11 +322,19 @@ void __transport_register_session(
>   	struct se_session *se_sess,
>   	void *fabric_sess_ptr)
>   {
> +	struct target_core_fabric_ops *tfo = se_tpg->se_tpg_tfo;
>   	unsigned char buf[PR_REG_ISID_LEN];
>
>   	se_sess->se_tpg = se_tpg;
>   	se_sess->fabric_sess_ptr = fabric_sess_ptr;
>   	/*
> +	 * Determine if fabric allows for T10-PI feature bits to be exposed
> +	 * to initiators for device backends with !dev->dev_attrib.pi_prot_type
> +	 */
> +	if (tfo->tpg_check_prot_fabric_only)
> +		se_sess->sess_prot_type = tfo->tpg_check_prot_fabric_only(se_tpg);
> +
> +	/*
>   	 * Used by struct se_node_acl's under ConfigFS to locate active se_session-t
>   	 *
>   	 * Only set for struct se_session's that will actually be moving I/O.
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 672150b..fe25a78 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -616,6 +616,7 @@ struct se_session {
>   	unsigned		sess_tearing_down:1;
>   	u64			sess_bin_isid;
>   	enum target_prot_op	sup_prot_ops;
> +	enum target_prot_type	sess_prot_type;
>   	struct se_node_acl	*se_node_acl;
>   	struct se_portal_group *se_tpg;
>   	void			*fabric_sess_ptr;
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 2f4a250..c93cfdf 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -27,6 +27,14 @@ struct target_core_fabric_ops {
>   	 * inquiry response
>   	 */
>   	int (*tpg_check_demo_mode_login_only)(struct se_portal_group *);
> +	/*
> +	 * Optionally used as a configfs tunable to determine when
> +	 * target-core should signal the PROTECT=1 feature bit for
> +	 * backends that don't support T10-PI, so that either fabric
> +	 * HW offload or target-core emulation performs the associated
> +	 * WRITE_STRIP and READ_INSERT operations.
> +	 */
> +	int (*tpg_check_prot_fabric_only)(struct se_portal_group *);
>   	struct se_node_acl *(*tpg_alloc_fabric_acl)(
>   					struct se_portal_group *);
>   	void (*tpg_release_fabric_acl)(struct se_portal_group *,
>

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Nicholas A. Bellinger]

On Mon, 2015-03-30 at 10:51 +0300, Sagi Grimberg wrote:
> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch adds a new target_core_fabric_ops callback for allowing fabric
> > drivers to expose a TPG attribute for signaling when a T10-PI protected
> > fabric wants to function with an un-protected device without T10-PI.
> >
> > This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
> > operations when functioning with non T10-PI enabled devices, seperate
> > from any available hw offloads the fabric supports.
> >
> > This is done using a new se_sess->sess_prot_type that is set at fabric
> > session creation time based upon the TPG attribute.  It currently cannot
> > be changed for individual sessions after initial creation.
> >
> > Also, update existing target_core_sbc.c code to honor sess_prot_type when
> > setting up cmd->prot_op + cmd->prot_type assignments.
> >
> > Cc: Martin Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Doug Gilbert <dgilbert@interlog.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
> >   drivers/target/target_core_transport.c |  8 +++++++
> >   include/target/target_core_base.h      |  1 +
> >   include/target/target_core_fabric.h    |  8 +++++++
> >   4 files changed, 50 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index 95a7a74..5b3564a 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
> >   }
> >
> >   static int
> > -sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
> > +sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
> >   		       bool is_write, struct se_cmd *cmd)
> >   {
> >   	if (is_write) {
> > -		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
> > -					 TARGET_PROT_DOUT_INSERT;
> > +		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
> > +			       protect ? TARGET_PROT_DOUT_PASS :
> > +			       TARGET_PROT_DOUT_INSERT;
> 
> In this case, if the protect=1 and fabric_prot=1 we will strip won't we?
> I think that the protect condition should come first.
> 

Mmm, not sure I follow..

sbc_check_prot() is only ever passing fabric_prot=1 when se_cmd prot
SGLs are present and se_dev does not accept PI, regardless of protect.

For the protect=1 and fabric_prot=1 case, prot_op is set to
TARGET_PROT_DOUT_STRIP requesting the WRITE_STRIP operation by either HW
fabric offload or target DIX software emulation because the backend
device cannot accept PI.

> >   		switch (protect) {
> >   		case 0x0:
> >   		case 0x3:
> > @@ -610,8 +611,9 @@ sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
> >   			return -EINVAL;
> >   		}
> >   	} else {
> > -		cmd->prot_op = protect ? TARGET_PROT_DIN_PASS :
> > -					 TARGET_PROT_DIN_STRIP;
> > +		cmd->prot_op = fabric_prot ? TARGET_PROT_DIN_INSERT :
> > +			       protect ? TARGET_PROT_DIN_PASS :
> > +			       TARGET_PROT_DIN_STRIP;
> 
> Same here.
> 
> >   		switch (protect) {
> >   		case 0x0:
> >   		case 0x1:
> > @@ -644,11 +646,15 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
> >   	       u32 sectors, bool is_write)
> >   {
> >   	u8 protect = cdb[1] >> 5;
> > +	int sp_ops = cmd->se_sess->sup_prot_ops;
> > +	int pi_prot_type = dev->dev_attrib.pi_prot_type;
> > +	bool fabric_prot = false;
> >
> >   	if (!cmd->t_prot_sg || !cmd->t_prot_nents) {
> > -		if (protect && !dev->dev_attrib.pi_prot_type) {
> > -			pr_err("CDB contains protect bit, but device does not"
> > -			       " advertise PROTECT=1 feature bit\n");
> > +		if (protect &&
> > +		    !dev->dev_attrib.pi_prot_type && !cmd->se_sess->sess_prot_type) {
> > +			pr_err("CDB contains protect bit, but device + fabric does"
> > +			       " not advertise PROTECT=1 feature bit\n");
> 
> Can you place unlikely on these conditions?
> 

Done.

> >   			return TCM_INVALID_CDB_FIELD;
> >   		}
> >   		if (cmd->prot_pto)
> > @@ -669,15 +675,28 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
> >   		cmd->reftag_seed = cmd->t_task_lba;
> >   		break;
> >   	case TARGET_DIF_TYPE0_PROT:
> > +		/*
> > +		 * See if the fabric supports T10-PI, and the session has been
> > +		 * configured to allow export PROTECT=1 feature bit with backend
> > +		 * devices that don't support T10-PI.
> > +		 */
> > +		fabric_prot = is_write ?
> > +			      (sp_ops & (TARGET_PROT_DOUT_PASS | TARGET_PROT_DOUT_STRIP)) :
> 
> Shouldn't this be converted to bool using !!()?
> 
> 

Fixed.

> > +			      (sp_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DIN_INSERT));
> > +
> > +		if (fabric_prot && cmd->se_sess->sess_prot_type) {
> > +			pi_prot_type = cmd->se_sess->sess_prot_type;
> > +			break;
> > +		}
> > +		/* Fallthrough */
> >   	default:
> >   		return TCM_NO_SENSE;
> >   	}
> >
> > -	if (sbc_set_prot_op_checks(protect, dev->dev_attrib.pi_prot_type,
> > -				   is_write, cmd))
> > +	if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
> >   		return TCM_INVALID_CDB_FIELD;
> >
> > -	cmd->prot_type = dev->dev_attrib.pi_prot_type;
> > +	cmd->prot_type = pi_prot_type;
> >   	cmd->prot_length = dev->prot_length * sectors;
> >
> >   	/**
> > @@ -1231,6 +1250,9 @@ sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
> >   	unsigned int i, len, left;
> >   	unsigned int offset = sg_off;
> >
> > +	if (!sg)
> > +		return;
> > +
> >   	left = sectors * dev->prot_length;
> >
> >   	for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 4a00ed5..aef989e 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -322,11 +322,19 @@ void __transport_register_session(
> >   	struct se_session *se_sess,
> >   	void *fabric_sess_ptr)
> >   {
> > +	struct target_core_fabric_ops *tfo = se_tpg->se_tpg_tfo;
> >   	unsigned char buf[PR_REG_ISID_LEN];
> >
> >   	se_sess->se_tpg = se_tpg;
> >   	se_sess->fabric_sess_ptr = fabric_sess_ptr;
> >   	/*
> > +	 * Determine if fabric allows for T10-PI feature bits to be exposed
> > +	 * to initiators for device backends with !dev->dev_attrib.pi_prot_type
> > +	 */
> > +	if (tfo->tpg_check_prot_fabric_only)
> > +		se_sess->sess_prot_type = tfo->tpg_check_prot_fabric_only(se_tpg);
> > +
> > +	/*
> >   	 * Used by struct se_node_acl's under ConfigFS to locate active se_session-t
> >   	 *
> >   	 * Only set for struct se_session's that will actually be moving I/O.
> > diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> > index 672150b..fe25a78 100644
> > --- a/include/target/target_core_base.h
> > +++ b/include/target/target_core_base.h
> > @@ -616,6 +616,7 @@ struct se_session {
> >   	unsigned		sess_tearing_down:1;
> >   	u64			sess_bin_isid;
> >   	enum target_prot_op	sup_prot_ops;
> > +	enum target_prot_type	sess_prot_type;
> >   	struct se_node_acl	*se_node_acl;
> >   	struct se_portal_group *se_tpg;
> >   	void			*fabric_sess_ptr;
> > diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> > index 2f4a250..c93cfdf 100644
> > --- a/include/target/target_core_fabric.h
> > +++ b/include/target/target_core_fabric.h
> > @@ -27,6 +27,14 @@ struct target_core_fabric_ops {
> >   	 * inquiry response
> >   	 */
> >   	int (*tpg_check_demo_mode_login_only)(struct se_portal_group *);
> > +	/*
> > +	 * Optionally used as a configfs tunable to determine when
> > +	 * target-core should signal the PROTECT=1 feature bit for
> > +	 * backends that don't support T10-PI, so that either fabric
> > +	 * HW offload or target-core emulation performs the associated
> > +	 * WRITE_STRIP and READ_INSERT operations.
> > +	 */
> > +	int (*tpg_check_prot_fabric_only)(struct se_portal_group *);
> >   	struct se_node_acl *(*tpg_alloc_fabric_acl)(
> >   					struct se_portal_group *);
> >   	void (*tpg_release_fabric_acl)(struct se_portal_group *,
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Sagi Grimberg]

On 4/1/2015 8:49 AM, Nicholas A. Bellinger wrote:
> On Mon, 2015-03-30 at 10:51 +0300, Sagi Grimberg wrote:
>> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>>
>>> This patch adds a new target_core_fabric_ops callback for allowing fabric
>>> drivers to expose a TPG attribute for signaling when a T10-PI protected
>>> fabric wants to function with an un-protected device without T10-PI.
>>>
>>> This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
>>> operations when functioning with non T10-PI enabled devices, seperate
>>> from any available hw offloads the fabric supports.
>>>
>>> This is done using a new se_sess->sess_prot_type that is set at fabric
>>> session creation time based upon the TPG attribute.  It currently cannot
>>> be changed for individual sessions after initial creation.
>>>
>>> Also, update existing target_core_sbc.c code to honor sess_prot_type when
>>> setting up cmd->prot_op + cmd->prot_type assignments.
>>>
>>> Cc: Martin Petersen <martin.petersen@oracle.com>
>>> Cc: Sagi Grimberg <sagig@mellanox.com>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Doug Gilbert <dgilbert@interlog.com>
>>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
>>> ---
>>>    drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
>>>    drivers/target/target_core_transport.c |  8 +++++++
>>>    include/target/target_core_base.h      |  1 +
>>>    include/target/target_core_fabric.h    |  8 +++++++
>>>    4 files changed, 50 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
>>> index 95a7a74..5b3564a 100644
>>> --- a/drivers/target/target_core_sbc.c
>>> +++ b/drivers/target/target_core_sbc.c
>>> @@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
>>>    }
>>>
>>>    static int
>>> -sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
>>> +sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
>>>    		       bool is_write, struct se_cmd *cmd)
>>>    {
>>>    	if (is_write) {
>>> -		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
>>> -					 TARGET_PROT_DOUT_INSERT;
>>> +		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
>>> +			       protect ? TARGET_PROT_DOUT_PASS :
>>> +			       TARGET_PROT_DOUT_INSERT;
>>
>> In this case, if the protect=1 and fabric_prot=1 we will strip won't we?
>> I think that the protect condition should come first.
>>
>
> Mmm, not sure I follow..
>
> sbc_check_prot() is only ever passing fabric_prot=1 when se_cmd prot
> SGLs are present and se_dev does not accept PI, regardless of protect.
>

It's a little confusing that fabric_prot is set if the backend device
does not support PI.

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Nicholas A. Bellinger]

On Wed, 2015-04-01 at 12:04 +0300, Sagi Grimberg wrote:
> On 4/1/2015 8:49 AM, Nicholas A. Bellinger wrote:
> > On Mon, 2015-03-30 at 10:51 +0300, Sagi Grimberg wrote:
> >> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> >>> From: Nicholas Bellinger <nab@linux-iscsi.org>
> >>>
> >>> This patch adds a new target_core_fabric_ops callback for allowing fabric
> >>> drivers to expose a TPG attribute for signaling when a T10-PI protected
> >>> fabric wants to function with an un-protected device without T10-PI.
> >>>
> >>> This specifically is to allow LIO to perform WRITE_STRIP + READ_INSERT
> >>> operations when functioning with non T10-PI enabled devices, seperate
> >>> from any available hw offloads the fabric supports.
> >>>
> >>> This is done using a new se_sess->sess_prot_type that is set at fabric
> >>> session creation time based upon the TPG attribute.  It currently cannot
> >>> be changed for individual sessions after initial creation.
> >>>
> >>> Also, update existing target_core_sbc.c code to honor sess_prot_type when
> >>> setting up cmd->prot_op + cmd->prot_type assignments.
> >>>
> >>> Cc: Martin Petersen <martin.petersen@oracle.com>
> >>> Cc: Sagi Grimberg <sagig@mellanox.com>
> >>> Cc: Christoph Hellwig <hch@lst.de>
> >>> Cc: Doug Gilbert <dgilbert@interlog.com>
> >>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> >>> ---
> >>>    drivers/target/target_core_sbc.c       | 44 +++++++++++++++++++++++++---------
> >>>    drivers/target/target_core_transport.c |  8 +++++++
> >>>    include/target/target_core_base.h      |  1 +
> >>>    include/target/target_core_fabric.h    |  8 +++++++
> >>>    4 files changed, 50 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> >>> index 95a7a74..5b3564a 100644
> >>> --- a/drivers/target/target_core_sbc.c
> >>> +++ b/drivers/target/target_core_sbc.c
> >>> @@ -581,12 +581,13 @@ sbc_compare_and_write(struct se_cmd *cmd)
> >>>    }
> >>>
> >>>    static int
> >>> -sbc_set_prot_op_checks(u8 protect, enum target_prot_type prot_type,
> >>> +sbc_set_prot_op_checks(u8 protect, bool fabric_prot, enum target_prot_type prot_type,
> >>>    		       bool is_write, struct se_cmd *cmd)
> >>>    {
> >>>    	if (is_write) {
> >>> -		cmd->prot_op = protect ? TARGET_PROT_DOUT_PASS :
> >>> -					 TARGET_PROT_DOUT_INSERT;
> >>> +		cmd->prot_op = fabric_prot ? TARGET_PROT_DOUT_STRIP :
> >>> +			       protect ? TARGET_PROT_DOUT_PASS :
> >>> +			       TARGET_PROT_DOUT_INSERT;
> >>
> >> In this case, if the protect=1 and fabric_prot=1 we will strip won't we?
> >> I think that the protect condition should come first.
> >>
> >
> > Mmm, not sure I follow..
> >
> > sbc_check_prot() is only ever passing fabric_prot=1 when se_cmd prot
> > SGLs are present and se_dev does not accept PI, regardless of protect.
> >
> 
> It's a little confusing that fabric_prot is set if the backend device
> does not support PI.

Well, it's supposed to signal that fabric supports PI, but the backend
device does not.

Seems like a reasonable name to me..  Any ideas for a better one..?  ;)

--nab

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@daterainc.com> writes:

nab> This specifically is to allow LIO to perform WRITE_STRIP +
nab> READ_INSERT operations when functioning with non T10-PI enabled
nab> devices, seperate from any available hw offloads the fabric
nab> supports.

How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not
persistent?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Nicholas A. Bellinger]

On Tue, 2015-04-07 at 19:27 -0400, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <nab@daterainc.com> writes:
> 
> nab> This specifically is to allow LIO to perform WRITE_STRIP +
> nab> READ_INSERT operations when functioning with non T10-PI enabled
> nab> devices, seperate from any available hw offloads the fabric
> nab> supports.
> 
> How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not
> persistent?
> 

AFAICT, this would result in cmd->prot_op = TARGET_PROT_*_PASS and
cmd->prot_checks = 0 for RDPROTECT/WRPROTECT == 0x3 in
sbc_set_prot_op_checks() code.

Do DOUT_STRIP + DIN_INSERT need to be called if a protection buffer is
present when RDPROTECT/WRPROTECT == 0x3 if fabric_prot was cleared..?  
Or should the command be rejected when a protection buffer is present +
RDPROTECT/WRPROTECT is non-zero if fabric_prot was cleared..?

Also wrt to PI persistence across session restart, one way it can work
is to store the last se_sess->sess_prot_type in se_node_acl, and
reinstate the previous setting across session restart.  This would allow
new sessions to pickup the latest fabric_prot_type endpoint attribute
value, but make existing ones with PI enabled keep their previous
sess_prot_type settings.

WDYT..? 

--nab

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:

>> How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not
>> persistent?

nab> AFAICT, this would result in cmd->prot_op = TARGET_PROT_*_PASS and
cmd-> prot_checks = 0 for RDPROTECT/WRPROTECT == 0x3 in
nab> sbc_set_prot_op_checks() code.

nab> Do DOUT_STRIP + DIN_INSERT need to be called if a protection buffer
nab> is present when RDPROTECT/WRPROTECT == 0x3 if fabric_prot was
nab> cleared..?  Or should the command be rejected when a protection
nab> buffer is present + RDPROTECT/WRPROTECT is non-zero if fabric_prot
nab> was cleared..?

Depends how compliant you want to be.

You can synthesize PI with RDPROTECT/WRPROTECT=1 as long as the
initiator doesn't rely on app tag escapes (we don't). Most non-HDD/SSD
targets work this way.

I would suggest that you return invalid field in CDB for
RDPROTECT/WRPROTECT=3 unless the PI can be made persistent, however.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Nicholas A. Bellinger]

On Thu, 2015-04-09 at 17:45 -0400, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
> 
> >> How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not
> >> persistent?
> 
> nab> AFAICT, this would result in cmd->prot_op = TARGET_PROT_*_PASS and
> cmd-> prot_checks = 0 for RDPROTECT/WRPROTECT == 0x3 in
> nab> sbc_set_prot_op_checks() code.
> 
> nab> Do DOUT_STRIP + DIN_INSERT need to be called if a protection buffer
> nab> is present when RDPROTECT/WRPROTECT == 0x3 if fabric_prot was
> nab> cleared..?  Or should the command be rejected when a protection
> nab> buffer is present + RDPROTECT/WRPROTECT is non-zero if fabric_prot
> nab> was cleared..?
> 
> Depends how compliant you want to be.
> 
> You can synthesize PI with RDPROTECT/WRPROTECT=1 as long as the
> initiator doesn't rely on app tag escapes (we don't). Most non-HDD/SSD
> targets work this way.
> 

<nod>

> I would suggest that you return invalid field in CDB for
> RDPROTECT/WRPROTECT=3 unless the PI can be made persistent, however.
> 

Ok, after thinking about this some more, here's what I've come up with..

The following incremental patch saves the current sess_prot_type into
se_node_acl, and will always reset sess_prot_type if a previous saved
value exists.  So the PI setting for the fabric's session with backend
devices not supporting PI is persistent across session restart.

I also noticed the internal DIF emulation was not honoring
se_cmd->prot_checks for the WRPROTECT/RDPROTECT == 0x3 case, so
sbc_dif_v1_verify() has been updated to follow which checks have been
calculated based on WRPROTECT/RDPROTECT in sbc_set_prot_op_checks().

Finally in sbc_check_prot(), if PROTECT is non-zero for a backend device
with DIF disabled, and sess_prot_type is not set go ahead and return
INVALID_CDB_FIELD.

WDYT..?

--nab

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 315ff64..a75512f 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -697,9 +697,13 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 			pi_prot_type = cmd->se_sess->sess_prot_type;
 			break;
 		}
+		if (!protect)
+			return TCM_NO_SENSE;
 		/* Fallthrough */
 	default:
-		return TCM_NO_SENSE;
+		pr_err("Unable to determine pi_prot_type for CDB: 0x%02x "
+		       "PROTECT: 0x%02x\n", cdb[0], protect);
+		return TCM_INVALID_CDB_FIELD;
 	}
 
 	if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
@@ -1221,6 +1227,9 @@ sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
 	int block_size = dev->dev_attrib.block_size;
 	__be16 csum;
 
+	if (!(cmd->prot_checks & TARGET_DIF_CHECK_GUARD))
+		goto check_ref;
+
 	csum = cpu_to_be16(crc_t10dif(p, block_size));
 
 	if (sdt->guard_tag != csum) {
@@ -1230,6 +1239,10 @@ sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
 		return TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED;
 	}
 
+check_ref:
+	if (!(cmd->prot_checks & TARGET_DIF_CHECK_REFTAG))
+		return 0;
+
 	if (cmd->prot_type == TARGET_DIF_TYPE1_PROT &&
 	    be32_to_cpu(sdt->ref_tag) != (sector & 0xffffffff)) {
 		pr_err("DIFv1 Type 1 reference failed on sector: %llu tag: 0x%08x"
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f884198..3ff38b2 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -329,11 +329,17 @@ void __transport_register_session(
 	se_sess->fabric_sess_ptr = fabric_sess_ptr;
 	/*
 	 * Determine if fabric allows for T10-PI feature bits to be exposed
-	 * to initiators for device backends with !dev->dev_attrib.pi_prot_type
+	 * to initiators for device backends with !dev->dev_attrib.pi_prot_type.
+	 *
+	 * If so, then always save prot_type on a per se_node_acl node basis
+	 * and re-instate the previous sess_prot_type to avoid disabling PI
+	 * from below any previously initiator side registered LUNs.
 	 */
-	if (tfo->tpg_check_prot_fabric_only)
-		se_sess->sess_prot_type = tfo->tpg_check_prot_fabric_only(se_tpg);
-
+	if (se_nacl->saved_prot_type)
+		se_sess->sess_prot_type = se_nacl->saved_prot_type;
+	else if (tfo->tpg_check_prot_fabric_only)
+		se_sess->sess_prot_type = se_nacl->saved_prot_type =
+				tfo->tpg_check_prot_fabric_only(se_tpg);
 	/*
 	 * Used by struct se_node_acl's under ConfigFS to locate active se_session-t
 	 *
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 383110d..51dcf2b 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -590,6 +590,7 @@ struct se_node_acl {
 	bool			acl_stop:1;
 	u32			queue_depth;
 	u32			acl_index;
+	enum target_prot_type	saved_prot_type;
 #define MAX_ACL_TAG_SIZE 64
 	char			acl_tag[MAX_ACL_TAG_SIZE];
 	/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Sagi Grimberg]

On 4/10/2015 9:59 PM, Nicholas A. Bellinger wrote:
> On Thu, 2015-04-09 at 17:45 -0400, Martin K. Petersen wrote:
>>>>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:
>>
>>>> How do you handle RDPROTECT/WRPROTECT values of 3 if the PI is not
>>>> persistent?
>>
>> nab> AFAICT, this would result in cmd->prot_op = TARGET_PROT_*_PASS and
>> cmd-> prot_checks = 0 for RDPROTECT/WRPROTECT == 0x3 in
>> nab> sbc_set_prot_op_checks() code.
>>
>> nab> Do DOUT_STRIP + DIN_INSERT need to be called if a protection buffer
>> nab> is present when RDPROTECT/WRPROTECT == 0x3 if fabric_prot was
>> nab> cleared..?  Or should the command be rejected when a protection
>> nab> buffer is present + RDPROTECT/WRPROTECT is non-zero if fabric_prot
>> nab> was cleared..?
>>
>> Depends how compliant you want to be.
>>
>> You can synthesize PI with RDPROTECT/WRPROTECT=1 as long as the
>> initiator doesn't rely on app tag escapes (we don't). Most non-HDD/SSD
>> targets work this way.
>>
>
> <nod>
>
>> I would suggest that you return invalid field in CDB for
>> RDPROTECT/WRPROTECT=3 unless the PI can be made persistent, however.
>>
>
> Ok, after thinking about this some more, here's what I've come up with..
>
> The following incremental patch saves the current sess_prot_type into
> se_node_acl, and will always reset sess_prot_type if a previous saved
> value exists.  So the PI setting for the fabric's session with backend
> devices not supporting PI is persistent across session restart.
>
> I also noticed the internal DIF emulation was not honoring
> se_cmd->prot_checks for the WRPROTECT/RDPROTECT == 0x3 case, so
> sbc_dif_v1_verify() has been updated to follow which checks have been
> calculated based on WRPROTECT/RDPROTECT in sbc_set_prot_op_checks().
>
> Finally in sbc_check_prot(), if PROTECT is non-zero for a backend device
> with DIF disabled, and sess_prot_type is not set go ahead and return
> INVALID_CDB_FIELD.
>
> WDYT..?
>
> --nab
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 315ff64..a75512f 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -697,9 +697,13 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>   			pi_prot_type = cmd->se_sess->sess_prot_type;
>   			break;
>   		}
> +		if (!protect)
> +			return TCM_NO_SENSE;
>   		/* Fallthrough */
>   	default:
> -		return TCM_NO_SENSE;
> +		pr_err("Unable to determine pi_prot_type for CDB: 0x%02x "
> +		       "PROTECT: 0x%02x\n", cdb[0], protect);
> +		return TCM_INVALID_CDB_FIELD;
>   	}
>
>   	if (sbc_set_prot_op_checks(protect, fabric_prot, pi_prot_type, is_write, cmd))
> @@ -1221,6 +1227,9 @@ sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
>   	int block_size = dev->dev_attrib.block_size;
>   	__be16 csum;
>
> +	if (!(cmd->prot_checks & TARGET_DIF_CHECK_GUARD))
> +		goto check_ref;
> +
>   	csum = cpu_to_be16(crc_t10dif(p, block_size));
>
>   	if (sdt->guard_tag != csum) {
> @@ -1230,6 +1239,10 @@ sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt,
>   		return TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED;
>   	}
>
> +check_ref:
> +	if (!(cmd->prot_checks & TARGET_DIF_CHECK_REFTAG))
> +		return 0;
> +
>   	if (cmd->prot_type == TARGET_DIF_TYPE1_PROT &&
>   	    be32_to_cpu(sdt->ref_tag) != (sector & 0xffffffff)) {
>   		pr_err("DIFv1 Type 1 reference failed on sector: %llu tag: 0x%08x"
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index f884198..3ff38b2 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -329,11 +329,17 @@ void __transport_register_session(
>   	se_sess->fabric_sess_ptr = fabric_sess_ptr;
>   	/*
>   	 * Determine if fabric allows for T10-PI feature bits to be exposed
> -	 * to initiators for device backends with !dev->dev_attrib.pi_prot_type
> +	 * to initiators for device backends with !dev->dev_attrib.pi_prot_type.
> +	 *
> +	 * If so, then always save prot_type on a per se_node_acl node basis
> +	 * and re-instate the previous sess_prot_type to avoid disabling PI
> +	 * from below any previously initiator side registered LUNs.
>   	 */
> -	if (tfo->tpg_check_prot_fabric_only)
> -		se_sess->sess_prot_type = tfo->tpg_check_prot_fabric_only(se_tpg);
> -
> +	if (se_nacl->saved_prot_type)
> +		se_sess->sess_prot_type = se_nacl->saved_prot_type;
> +	else if (tfo->tpg_check_prot_fabric_only)
> +		se_sess->sess_prot_type = se_nacl->saved_prot_type =
> +				tfo->tpg_check_prot_fabric_only(se_tpg);
>   	/*
>   	 * Used by struct se_node_acl's under ConfigFS to locate active se_session-t
>   	 *
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 383110d..51dcf2b 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -590,6 +590,7 @@ struct se_node_acl {
>   	bool			acl_stop:1;
>   	u32			queue_depth;
>   	u32			acl_index;
> +	enum target_prot_type	saved_prot_type;
>   #define MAX_ACL_TAG_SIZE 64
>   	char			acl_tag[MAX_ACL_TAG_SIZE];
>   	/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
>
>
>

This looks fine to me.

Acked-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH-v2 02/15] target: Add protected fabric + unprotected device support

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:

nab> The following incremental patch saves the current sess_prot_type
nab> into se_node_acl, and will always reset sess_prot_type if a
nab> previous saved value exists.  So the PI setting for the fabric's
nab> session with backend devices not supporting PI is persistent across
nab> session restart.

nab> I also noticed the internal DIF emulation was not honoring se_cmd->
nab> prot_checks for the WRPROTECT/RDPROTECT == 0x3 case, so
nab> sbc_dif_v1_verify() has been updated to follow which checks have
nab> been calculated based on WRPROTECT/RDPROTECT in
nab> sbc_set_prot_op_checks().

nab> Finally in sbc_check_prot(), if PROTECT is non-zero for a backend
nab> device with DIF disabled, and sess_prot_type is not set go ahead
nab> and return INVALID_CDB_FIELD.

Looks good to me.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH-v2 03/15] target: Update SPC/SBC emulation for sess_prot_type

[Nicholas A. Bellinger]

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

This patch updates standard INQUIRY, INQUIRY EVPD=0x86, READ_CAPACITY_16
and control mode pages to use se_sess->sess_prot_type when determing which
type of T10-PI related feature bits can be exposed.

This is required for fabric sessions supporting T10-PI metadata to
backend devices that don't have protection enabled.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Doug Gilbert <dgilbert@interlog.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c | 13 +++++++++++--
 drivers/target/target_core_spc.c | 14 +++++++++-----
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 5b3564a..68373c9 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -93,6 +93,8 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct se_session *sess = cmd->se_sess;
+	int pi_prot_type = dev->dev_attrib.pi_prot_type;
+
 	unsigned char *rbuf;
 	unsigned char buf[32];
 	unsigned long long blocks = dev->transport->get_blocks(dev);
@@ -114,8 +116,15 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
 	 * Set P_TYPE and PROT_EN bits for DIF support
 	 */
 	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
-		if (dev->dev_attrib.pi_prot_type)
-			buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
+		/*
+		 * Only override a device's pi_prot_type if no T10-PI is
+		 * available, and sess_prot_type has been explicitly enabled.
+		 */
+		if (!pi_prot_type)
+			pi_prot_type = sess->sess_prot_type;
+
+		if (pi_prot_type)
+			buf[12] = (pi_prot_type - 1) << 1 | 0x1;
 	}
 
 	if (dev->transport->get_lbppbe)
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index f310aac..9ec6459 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -103,10 +103,12 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
 		buf[5] |= 0x8;
 	/*
 	 * Set Protection (PROTECT) bit when DIF has been enabled on the
-	 * device, and the transport supports VERIFY + PASS.
+	 * device, and the fabric supports VERIFY + PASS.  Also report
+	 * PROTECT=1 if sess_prot_type has been configured to allow T10-PI
+	 * to unprotected devices.
 	 */
 	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
-		if (dev->dev_attrib.pi_prot_type)
+		if (dev->dev_attrib.pi_prot_type || cmd->se_sess->sess_prot_type)
 			buf[5] |= 0x1;
 	}
 
@@ -480,9 +482,11 @@ spc_emulate_evpd_86(struct se_cmd *cmd, unsigned char *buf)
 	 * only for TYPE3 protection.
 	 */
 	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
-		if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
+		if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT ||
+		    cmd->se_sess->sess_prot_type == TARGET_DIF_TYPE1_PROT)
 			buf[4] = 0x5;
-		else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT)
+		else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT ||
+			cmd->se_sess->sess_prot_type == TARGET_DIF_TYPE3_PROT)
 			buf[4] = 0x4;
 	}
 
@@ -874,7 +878,7 @@ static int spc_modesense_control(struct se_cmd *cmd, u8 pc, u8 *p)
 	 * TAG field.
 	 */
 	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
-		if (dev->dev_attrib.pi_prot_type)
+		if (dev->dev_attrib.pi_prot_type || sess->sess_prot_type)
 			p[5] |= 0x80;
 	}
 
-- 
1.9.1

Re: [PATCH-v2 03/15] target: Update SPC/SBC emulation for sess_prot_type

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch updates standard INQUIRY, INQUIRY EVPD=0x86, READ_CAPACITY_16
> and control mode pages to use se_sess->sess_prot_type when determing which
> type of T10-PI related feature bits can be exposed.
>
> This is required for fabric sessions supporting T10-PI metadata to
> backend devices that don't have protection enabled.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Doug Gilbert <dgilbert@interlog.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c | 13 +++++++++++--
>   drivers/target/target_core_spc.c | 14 +++++++++-----
>   2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 5b3564a..68373c9 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -93,6 +93,8 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
>   {
>   	struct se_device *dev = cmd->se_dev;
>   	struct se_session *sess = cmd->se_sess;
> +	int pi_prot_type = dev->dev_attrib.pi_prot_type;
> +
>   	unsigned char *rbuf;
>   	unsigned char buf[32];
>   	unsigned long long blocks = dev->transport->get_blocks(dev);
> @@ -114,8 +116,15 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
>   	 * Set P_TYPE and PROT_EN bits for DIF support
>   	 */
>   	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
> -		if (dev->dev_attrib.pi_prot_type)
> -			buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
> +		/*
> +		 * Only override a device's pi_prot_type if no T10-PI is
> +		 * available, and sess_prot_type has been explicitly enabled.
> +		 */
> +		if (!pi_prot_type)
> +			pi_prot_type = sess->sess_prot_type;
> +
> +		if (pi_prot_type)
> +			buf[12] = (pi_prot_type - 1) << 1 | 0x1;
>   	}
>
>   	if (dev->transport->get_lbppbe)
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index f310aac..9ec6459 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -103,10 +103,12 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
>   		buf[5] |= 0x8;
>   	/*
>   	 * Set Protection (PROTECT) bit when DIF has been enabled on the
> -	 * device, and the transport supports VERIFY + PASS.
> +	 * device, and the fabric supports VERIFY + PASS.  Also report
> +	 * PROTECT=1 if sess_prot_type has been configured to allow T10-PI
> +	 * to unprotected devices.
>   	 */
>   	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
> -		if (dev->dev_attrib.pi_prot_type)
> +		if (dev->dev_attrib.pi_prot_type || cmd->se_sess->sess_prot_type)
>   			buf[5] |= 0x1;
>   	}
>
> @@ -480,9 +482,11 @@ spc_emulate_evpd_86(struct se_cmd *cmd, unsigned char *buf)
>   	 * only for TYPE3 protection.
>   	 */
>   	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
> -		if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
> +		if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT ||
> +		    cmd->se_sess->sess_prot_type == TARGET_DIF_TYPE1_PROT)
>   			buf[4] = 0x5;
> -		else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT)
> +		else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT ||
> +			cmd->se_sess->sess_prot_type == TARGET_DIF_TYPE3_PROT)
>   			buf[4] = 0x4;
>   	}
>
> @@ -874,7 +878,7 @@ static int spc_modesense_control(struct se_cmd *cmd, u8 pc, u8 *p)
>   	 * TAG field.
>   	 */
>   	if (sess->sup_prot_ops & (TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS)) {
> -		if (dev->dev_attrib.pi_prot_type)
> +		if (dev->dev_attrib.pi_prot_type || sess->sess_prot_type)
>   			p[5] |= 0x80;
>   	}
>
>

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH-v2 03/15] target: Update SPC/SBC emulation for sess_prot_type

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@daterainc.com> writes:

nab> This patch updates standard INQUIRY, INQUIRY EVPD=0x86,
nab> READ_CAPACITY_16 and control mode pages to use
nab> se_sess->sess_prot_type when determing which type of T10-PI related
nab> feature bits can be exposed.

nab> This is required for fabric sessions supporting T10-PI metadata to
nab> backend devices that don't have protection enabled.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH-v2 04/15] target: Move cmd->prot_op check into target_check_write_prot

[Nicholas A. Bellinger]

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

This patch moves the existing target_execute_cmd() check for
cmd->prot_op into it's own function, so it's easier to add
future support for WRITE STRIP.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index aef989e..6a24151 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1738,6 +1738,25 @@ void __target_execute_cmd(struct se_cmd *cmd)
 	}
 }
 
+static int target_check_write_prot(struct se_cmd *cmd)
+{
+	/*
+	 * Perform WRITE_INSERT of PI using software emulation when backend
+	 * device has PI enabled, if the transport has not already generated
+	 * PI using hardware WRITE_INSERT offload.
+	 */
+	switch (cmd->prot_op) {
+	case TARGET_PROT_DOUT_INSERT:
+		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
+			sbc_dif_generate(cmd);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static bool target_handle_task_attr(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
@@ -1817,15 +1836,9 @@ void target_execute_cmd(struct se_cmd *cmd)
 	cmd->t_state = TRANSPORT_PROCESSING;
 	cmd->transport_state |= CMD_T_ACTIVE|CMD_T_BUSY|CMD_T_SENT;
 	spin_unlock_irq(&cmd->t_state_lock);
-	/*
-	 * Perform WRITE_INSERT of PI using software emulation when backend
-	 * device has PI enabled, if the transport has not already generated
-	 * PI using hardware WRITE_INSERT offload.
-	 */
-	if (cmd->prot_op == TARGET_PROT_DOUT_INSERT) {
-		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
-			sbc_dif_generate(cmd);
-	}
+
+	if (target_check_write_prot(cmd))
+		return;
 
 	if (target_handle_task_attr(cmd)) {
 		spin_lock_irq(&cmd->t_state_lock);
-- 
1.9.1

Re: [PATCH-v2 04/15] target: Move cmd->prot_op check into target_check_write_prot

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch moves the existing target_execute_cmd() check for
> cmd->prot_op into it's own function, so it's easier to add
> future support for WRITE STRIP.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_transport.c | 31 ++++++++++++++++++++++---------
>   1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index aef989e..6a24151 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1738,6 +1738,25 @@ void __target_execute_cmd(struct se_cmd *cmd)
>   	}
>   }
>
> +static int target_check_write_prot(struct se_cmd *cmd)

This is not really a check routine, it actually does something with
the protection information. Maybe a better name would be
target_write_prot_action() or something in that form.

> +{
> +	/*
> +	 * Perform WRITE_INSERT of PI using software emulation when backend
> +	 * device has PI enabled, if the transport has not already generated
> +	 * PI using hardware WRITE_INSERT offload.
> +	 */
> +	switch (cmd->prot_op) {
> +	case TARGET_PROT_DOUT_INSERT:
> +		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
> +			sbc_dif_generate(cmd);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>   static bool target_handle_task_attr(struct se_cmd *cmd)
>   {
>   	struct se_device *dev = cmd->se_dev;
> @@ -1817,15 +1836,9 @@ void target_execute_cmd(struct se_cmd *cmd)
>   	cmd->t_state = TRANSPORT_PROCESSING;
>   	cmd->transport_state |= CMD_T_ACTIVE|CMD_T_BUSY|CMD_T_SENT;
>   	spin_unlock_irq(&cmd->t_state_lock);
> -	/*
> -	 * Perform WRITE_INSERT of PI using software emulation when backend
> -	 * device has PI enabled, if the transport has not already generated
> -	 * PI using hardware WRITE_INSERT offload.
> -	 */
> -	if (cmd->prot_op == TARGET_PROT_DOUT_INSERT) {
> -		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
> -			sbc_dif_generate(cmd);
> -	}
> +
> +	if (target_check_write_prot(cmd))
> +		return;
>
>   	if (target_handle_task_attr(cmd)) {
>   		spin_lock_irq(&cmd->t_state_lock);
>

Re: [PATCH-v2 04/15] target: Move cmd->prot_op check into target_check_write_prot

[Nicholas A. Bellinger]

On Mon, 2015-03-30 at 10:57 +0300, Sagi Grimberg wrote:
> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch moves the existing target_execute_cmd() check for
> > cmd->prot_op into it's own function, so it's easier to add
> > future support for WRITE STRIP.
> >
> > Cc: Martin Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/target_core_transport.c | 31 ++++++++++++++++++++++---------
> >   1 file changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index aef989e..6a24151 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -1738,6 +1738,25 @@ void __target_execute_cmd(struct se_cmd *cmd)
> >   	}
> >   }
> >
> > +static int target_check_write_prot(struct se_cmd *cmd)
> 
> This is not really a check routine, it actually does something with
> the protection information. Maybe a better name would be
> target_write_prot_action() or something in that form.

Makes sense.  Renamed to target_write_prot_action()

Re: [PATCH-v2 04/15] target: Move cmd->prot_op check into target_check_write_prot

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@linux-iscsi.org> writes:

>> This patch moves the existing target_execute_cmd() check for
>> cmd->prot_op into it's own function, so it's easier to add future
>> support for WRITE STRIP.

nab> Makes sense.  Renamed to target_write_prot_action()

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH-v2 05/15] target: Add internal WRITE_STRIP support

[Nicholas A. Bellinger]

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

This patch adds WRITE_STRIP support in target_check_write_prot() that
invokes sbc_dif_verify_write() for checking T10-PI metadata before
submitting the I/O to a backend driver.

Upon verify failure, the specific sense code is propigated up the
failure path up to transport_generic_request_failure().

Also, update sbc_dif_verify_write() to only perform the subsequent
protection metadata copy when a valid *sg is passed.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_sbc.c       |  3 +++
 drivers/target/target_core_transport.c | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 68373c9..ea23b9c 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -1342,6 +1342,9 @@ sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
 		kunmap_atomic(paddr);
 		kunmap_atomic(daddr);
 	}
+	if (!sg)
+		return 0;
+
 	sbc_dif_copy_prot(cmd, sectors, false, sg, sg_off);
 
 	return 0;
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 6a24151..51b62bd 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1740,6 +1740,7 @@ void __target_execute_cmd(struct se_cmd *cmd)
 
 static int target_check_write_prot(struct se_cmd *cmd)
 {
+	u32 sectors;
 	/*
 	 * Perform WRITE_INSERT of PI using software emulation when backend
 	 * device has PI enabled, if the transport has not already generated
@@ -1750,6 +1751,21 @@ static int target_check_write_prot(struct se_cmd *cmd)
 		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
 			sbc_dif_generate(cmd);
 		break;
+	case TARGET_PROT_DOUT_STRIP:
+		if (cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_STRIP)
+			break;
+
+		sectors = cmd->data_length / cmd->se_dev->dev_attrib.block_size;
+		cmd->pi_err = sbc_dif_verify_write(cmd, cmd->t_task_lba,
+						   sectors, 0, NULL, 0);
+		if (cmd->pi_err) {
+			spin_lock_irq(&cmd->t_state_lock);
+			cmd->transport_state &= ~CMD_T_BUSY|CMD_T_SENT;
+			spin_unlock_irq(&cmd->t_state_lock);
+			transport_generic_request_failure(cmd, cmd->pi_err);
+			return -1;
+		}
+		break;
 	default:
 		break;
 	}
-- 
1.9.1

Re: [PATCH-v2 05/15] target: Add internal WRITE_STRIP support

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch adds WRITE_STRIP support in target_check_write_prot() that
> invokes sbc_dif_verify_write() for checking T10-PI metadata before
> submitting the I/O to a backend driver.
>
> Upon verify failure, the specific sense code is propigated up the
> failure path up to transport_generic_request_failure().
>
> Also, update sbc_dif_verify_write() to only perform the subsequent
> protection metadata copy when a valid *sg is passed.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_sbc.c       |  3 +++
>   drivers/target/target_core_transport.c | 16 ++++++++++++++++
>   2 files changed, 19 insertions(+)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 68373c9..ea23b9c 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -1342,6 +1342,9 @@ sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
>   		kunmap_atomic(paddr);
>   		kunmap_atomic(daddr);
>   	}
> +	if (!sg)
> +		return 0;
> +
>   	sbc_dif_copy_prot(cmd, sectors, false, sg, sg_off);
>
>   	return 0;
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 6a24151..51b62bd 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1740,6 +1740,7 @@ void __target_execute_cmd(struct se_cmd *cmd)
>
>   static int target_check_write_prot(struct se_cmd *cmd)
>   {
> +	u32 sectors;
>   	/*
>   	 * Perform WRITE_INSERT of PI using software emulation when backend
>   	 * device has PI enabled, if the transport has not already generated
> @@ -1750,6 +1751,21 @@ static int target_check_write_prot(struct se_cmd *cmd)
>   		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
>   			sbc_dif_generate(cmd);
>   		break;
> +	case TARGET_PROT_DOUT_STRIP:
> +		if (cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_STRIP)
> +			break;
> +
> +		sectors = cmd->data_length / cmd->se_dev->dev_attrib.block_size;

Better to use data_length >> ilog2(block_size) and avoiding div in the
IO path.

> +		cmd->pi_err = sbc_dif_verify_write(cmd, cmd->t_task_lba,
> +						   sectors, 0, NULL, 0);
> +		if (cmd->pi_err) {

unlikely()

> +			spin_lock_irq(&cmd->t_state_lock);
> +			cmd->transport_state &= ~CMD_T_BUSY|CMD_T_SENT;
> +			spin_unlock_irq(&cmd->t_state_lock);
> +			transport_generic_request_failure(cmd, cmd->pi_err);
> +			return -1;
> +		}
> +		break;
>   	default:
>   		break;
>   	}
>

Re: [PATCH-v2 05/15] target: Add internal WRITE_STRIP support

[Nicholas A. Bellinger]

On Mon, 2015-03-30 at 11:01 +0300, Sagi Grimberg wrote:
> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch adds WRITE_STRIP support in target_check_write_prot() that
> > invokes sbc_dif_verify_write() for checking T10-PI metadata before
> > submitting the I/O to a backend driver.
> >
> > Upon verify failure, the specific sense code is propigated up the
> > failure path up to transport_generic_request_failure().
> >
> > Also, update sbc_dif_verify_write() to only perform the subsequent
> > protection metadata copy when a valid *sg is passed.
> >
> > Cc: Martin Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/target_core_sbc.c       |  3 +++
> >   drivers/target/target_core_transport.c | 16 ++++++++++++++++
> >   2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index 68373c9..ea23b9c 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -1342,6 +1342,9 @@ sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
> >   		kunmap_atomic(paddr);
> >   		kunmap_atomic(daddr);
> >   	}
> > +	if (!sg)
> > +		return 0;
> > +
> >   	sbc_dif_copy_prot(cmd, sectors, false, sg, sg_off);
> >
> >   	return 0;
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 6a24151..51b62bd 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -1740,6 +1740,7 @@ void __target_execute_cmd(struct se_cmd *cmd)
> >
> >   static int target_check_write_prot(struct se_cmd *cmd)
> >   {
> > +	u32 sectors;
> >   	/*
> >   	 * Perform WRITE_INSERT of PI using software emulation when backend
> >   	 * device has PI enabled, if the transport has not already generated
> > @@ -1750,6 +1751,21 @@ static int target_check_write_prot(struct se_cmd *cmd)
> >   		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_INSERT))
> >   			sbc_dif_generate(cmd);
> >   		break;
> > +	case TARGET_PROT_DOUT_STRIP:
> > +		if (cmd->se_sess->sup_prot_ops & TARGET_PROT_DOUT_STRIP)
> > +			break;
> > +
> > +		sectors = cmd->data_length / cmd->se_dev->dev_attrib.block_size;
> 
> Better to use data_length >> ilog2(block_size) and avoiding div in the
> IO path.
> 

Done.

> > +		cmd->pi_err = sbc_dif_verify_write(cmd, cmd->t_task_lba,
> > +						   sectors, 0, NULL, 0);
> > +		if (cmd->pi_err) {
> 
> unlikely()
> 

Done.

Re: [PATCH-v2 05/15] target: Add internal WRITE_STRIP support

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@daterainc.com> writes:

nab> This patch adds WRITE_STRIP support in target_check_write_prot()
nab> that invokes sbc_dif_verify_write() for checking T10-PI metadata
nab> before submitting the I/O to a backend driver.

With Sagi's suggestions.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH-v2 06/15] target: Move cmd->prot_op check into target_check_read_prot

[Nicholas A. Bellinger]

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

This patch moves the existing target_complete_ok_work() check for
cmd->prot_op into it's own function, so it's easier to add future
support for READ INSERT.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 51b62bd..e603e34 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1980,16 +1980,22 @@ static void transport_handle_queue_full(
 	schedule_work(&cmd->se_dev->qf_work_queue);
 }
 
-static bool target_check_read_strip(struct se_cmd *cmd)
+static bool target_check_read_prot(struct se_cmd *cmd)
 {
 	sense_reason_t rc;
 
-	if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
-		rc = sbc_dif_read_strip(cmd);
-		if (rc) {
-			cmd->pi_err = rc;
-			return true;
+	switch (cmd->prot_op) {
+	case TARGET_PROT_DIN_STRIP:
+		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
+			rc = sbc_dif_read_strip(cmd);
+			if (rc) {
+				cmd->pi_err = rc;
+				return true;
+			}
 		}
+		break;
+	default:
+		break;
 	}
 
 	return false;
@@ -2064,8 +2070,7 @@ static void target_complete_ok_work(struct work_struct *work)
 		 * backend had PI enabled, if the transport will not be
 		 * performing hardware READ_STRIP offload.
 		 */
-		if (cmd->prot_op == TARGET_PROT_DIN_STRIP &&
-		    target_check_read_strip(cmd)) {
+		if (target_check_read_prot(cmd)) {
 			ret = transport_send_check_condition_and_sense(cmd,
 						cmd->pi_err, 0);
 			if (ret == -EAGAIN || ret == -ENOMEM)
-- 
1.9.1

Re: [PATCH-v2 06/15] target: Move cmd->prot_op check into target_check_read_prot

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch moves the existing target_complete_ok_work() check for
> cmd->prot_op into it's own function, so it's easier to add future
> support for READ INSERT.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_transport.c | 21 +++++++++++++--------
>   1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 51b62bd..e603e34 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1980,16 +1980,22 @@ static void transport_handle_queue_full(
>   	schedule_work(&cmd->se_dev->qf_work_queue);
>   }
>
> -static bool target_check_read_strip(struct se_cmd *cmd)
> +static bool target_check_read_prot(struct se_cmd *cmd)

Same comment on naming.

>   {
>   	sense_reason_t rc;
>
> -	if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
> -		rc = sbc_dif_read_strip(cmd);
> -		if (rc) {
> -			cmd->pi_err = rc;
> -			return true;
> +	switch (cmd->prot_op) {
> +	case TARGET_PROT_DIN_STRIP:
> +		if (!(cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_STRIP)) {
> +			rc = sbc_dif_read_strip(cmd);
> +			if (rc) {
> +				cmd->pi_err = rc;
> +				return true;
> +			}
>   		}
> +		break;
> +	default:
> +		break;
>   	}
>
>   	return false;
> @@ -2064,8 +2070,7 @@ static void target_complete_ok_work(struct work_struct *work)
>   		 * backend had PI enabled, if the transport will not be
>   		 * performing hardware READ_STRIP offload.
>   		 */
> -		if (cmd->prot_op == TARGET_PROT_DIN_STRIP &&
> -		    target_check_read_strip(cmd)) {
> +		if (target_check_read_prot(cmd)) {
>   			ret = transport_send_check_condition_and_sense(cmd,
>   						cmd->pi_err, 0);
>   			if (ret == -EAGAIN || ret == -ENOMEM)
>

Re: [PATCH-v2 06/15] target: Move cmd->prot_op check into target_check_read_prot

[Nicholas A. Bellinger]

On Mon, 2015-03-30 at 11:02 +0300, Sagi Grimberg wrote:
> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch moves the existing target_complete_ok_work() check for
> > cmd->prot_op into it's own function, so it's easier to add future
> > support for READ INSERT.
> >
> > Cc: Martin Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/target_core_transport.c | 21 +++++++++++++--------
> >   1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 51b62bd..e603e34 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -1980,16 +1980,22 @@ static void transport_handle_queue_full(
> >   	schedule_work(&cmd->se_dev->qf_work_queue);
> >   }
> >
> > -static bool target_check_read_strip(struct se_cmd *cmd)
> > +static bool target_check_read_prot(struct se_cmd *cmd)
> 
> Same comment on naming.

Fixed.

Re: [PATCH-v2 06/15] target: Move cmd->prot_op check into target_check_read_prot

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@daterainc.com> writes:

nab> This patch moves the existing target_complete_ok_work() check for
nab> cmd-> prot_op into it's own function, so it's easier to add future
nab> support for READ INSERT.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH-v2 07/15] target: Add internal READ_INSERT support

[Nicholas A. Bellinger]

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

This patch adds READ_INSERT support in target_check_read_prot() that
invokes sbc_dif_generate() when LIO is responsible for generating the
outgoing T10-PI.

Required for supporting fabrics that exchange protection information,
and would like to function with un-protected devices.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index e603e34..12b13da 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1994,6 +1994,12 @@ static bool target_check_read_prot(struct se_cmd *cmd)
 			}
 		}
 		break;
+	case TARGET_PROT_DIN_INSERT:
+		if (cmd->se_sess->sup_prot_ops & TARGET_PROT_DIN_INSERT)
+			break;
+
+		sbc_dif_generate(cmd);
+		break;
 	default:
 		break;
 	}
-- 
1.9.1

Re: [PATCH-v2 07/15] target: Add internal READ_INSERT support

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab@daterainc.com> writes:

nab> This patch adds READ_INSERT support in target_check_read_prot()
nab> that invokes sbc_dif_generate() when LIO is responsible for
nab> generating the outgoing T10-PI.

nab> Required for supporting fabrics that exchange protection
nab> information, and would like to function with un-protected devices.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH-v2 08/15] target/file: Add checks for backend DIF emulation

[Nicholas A. Bellinger]

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

Make sure that FILEIO only attempts to use backend DIF emulation
when it's actually enabled at device level.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index cd93935..fa54835 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -584,7 +584,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	if (data_direction == DMA_FROM_DEVICE) {
 		memset(&fd_prot, 0, sizeof(struct fd_prot));
 
-		if (cmd->prot_type) {
+		if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {
 			ret = fd_do_prot_rw(cmd, &fd_prot, false);
 			if (ret < 0)
 				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
@@ -592,7 +592,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 
 		ret = fd_do_rw(cmd, sgl, sgl_nents, 0);
 
-		if (ret > 0 && cmd->prot_type) {
+		if (ret > 0 && cmd->prot_type && dev->dev_attrib.pi_prot_type) {
 			u32 sectors = cmd->data_length / dev->dev_attrib.block_size;
 
 			rc = sbc_dif_verify_read(cmd, cmd->t_task_lba, sectors,
@@ -608,7 +608,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	} else {
 		memset(&fd_prot, 0, sizeof(struct fd_prot));
 
-		if (cmd->prot_type) {
+		if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {
 			u32 sectors = cmd->data_length / dev->dev_attrib.block_size;
 
 			ret = fd_do_prot_rw(cmd, &fd_prot, false);
@@ -646,7 +646,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 			vfs_fsync_range(fd_dev->fd_file, start, end, 1);
 		}
 
-		if (ret > 0 && cmd->prot_type) {
+		if (ret > 0 && cmd->prot_type && dev->dev_attrib.pi_prot_type) {
 			ret = fd_do_prot_rw(cmd, &fd_prot, true);
 			if (ret < 0)
 				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-- 
1.9.1

Re: [PATCH-v2 08/15] target/file: Add checks for backend DIF emulation

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> Make sure that FILEIO only attempts to use backend DIF emulation
> when it's actually enabled at device level.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/target_core_file.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index cd93935..fa54835 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -584,7 +584,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
>   	if (data_direction == DMA_FROM_DEVICE) {
>   		memset(&fd_prot, 0, sizeof(struct fd_prot));
>
> -		if (cmd->prot_type) {
> +		if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {

Maybe it's better to put it in a macro accessible from all backend
devices given that each would check now on (cmd_prot && dev_prot).

>   			ret = fd_do_prot_rw(cmd, &fd_prot, false);
>   			if (ret < 0)
>   				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> @@ -592,7 +592,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
>
>   		ret = fd_do_rw(cmd, sgl, sgl_nents, 0);
>
> -		if (ret > 0 && cmd->prot_type) {
> +		if (ret > 0 && cmd->prot_type && dev->dev_attrib.pi_prot_type) {
>   			u32 sectors = cmd->data_length / dev->dev_attrib.block_size;
>
>   			rc = sbc_dif_verify_read(cmd, cmd->t_task_lba, sectors,
> @@ -608,7 +608,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
>   	} else {
>   		memset(&fd_prot, 0, sizeof(struct fd_prot));
>
> -		if (cmd->prot_type) {
> +		if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {
>   			u32 sectors = cmd->data_length / dev->dev_attrib.block_size;
>
>   			ret = fd_do_prot_rw(cmd, &fd_prot, false);
> @@ -646,7 +646,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
>   			vfs_fsync_range(fd_dev->fd_file, start, end, 1);
>   		}
>
> -		if (ret > 0 && cmd->prot_type) {
> +		if (ret > 0 && cmd->prot_type && dev->dev_attrib.pi_prot_type) {
>   			ret = fd_do_prot_rw(cmd, &fd_prot, true);
>   			if (ret < 0)
>   				return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>

[PATCH-v2 09/15] target/iblock: Add checks for backend DIF emulation

[Nicholas A. Bellinger]

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

Make sure that IBLOCK only attempts to use backend DIF emulation
when it's actually enabled at device level.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_iblock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 2520cbf..1b7947c 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -774,7 +774,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 		sg_num--;
 	}
 
-	if (cmd->prot_type) {
+	if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {
 		int rc = iblock_alloc_bip(cmd, bio_start);
 		if (rc)
 			goto fail_put_bios;
-- 
1.9.1

[PATCH-v2 10/15] target/rd: Add checks for backend DIF emulation

[Nicholas A. Bellinger]

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

Make sure that RAMDISK only attempts to use backend DIF emulation
when it's actually enabled at device level.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_rd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 98e83ac..bedd4a1 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -419,7 +419,8 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 			data_direction == DMA_FROM_DEVICE ? "Read" : "Write",
 			cmd->t_task_lba, rd_size, rd_page, rd_offset);
 
-	if (cmd->prot_type && data_direction == DMA_TO_DEVICE) {
+	if (cmd->prot_type && se_dev->dev_attrib.pi_prot_type &&
+	    data_direction == DMA_TO_DEVICE) {
 		struct rd_dev_sg_table *prot_table;
 		struct scatterlist *prot_sg;
 		u32 sectors = cmd->data_length / se_dev->dev_attrib.block_size;
@@ -502,7 +503,8 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 	}
 	sg_miter_stop(&m);
 
-	if (cmd->prot_type && data_direction == DMA_FROM_DEVICE) {
+	if (cmd->prot_type && se_dev->dev_attrib.pi_prot_type &&
+	    data_direction == DMA_FROM_DEVICE) {
 		struct rd_dev_sg_table *prot_table;
 		struct scatterlist *prot_sg;
 		u32 sectors = cmd->data_length / se_dev->dev_attrib.block_size;
-- 
1.9.1

[PATCH-v2 11/15] loopback: Add fabric_prot_type attribute support

[Nicholas A. Bellinger]

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

This patch updates loopback to add a new fabric_prot_type TPG attribute,
used for controlling LLD level protection into LIO when the backend
device does not support T10-PI.

Also, go ahead and set DIN_PASS + DOUT_PASS so target-core knows that
it will be doing any WRITE_STRIP and READ_INSERT operations.

Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/loopback/tcm_loop.c | 54 ++++++++++++++++++++++++++++++++++++--
 drivers/target/loopback/tcm_loop.h |  1 +
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index f4618e7..797c731 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -697,6 +697,13 @@ static int tcm_loop_check_prod_mode_write_protect(struct se_portal_group *se_tpg
 	return 0;
 }
 
+static int tcm_loop_check_prot_fabric_only(struct se_portal_group *se_tpg)
+{
+	struct tcm_loop_tpg *tl_tpg = container_of(se_tpg, struct tcm_loop_tpg,
+						   tl_se_tpg);
+	return tl_tpg->tl_fabric_prot_type;
+}
+
 static struct se_node_acl *tcm_loop_tpg_alloc_fabric_acl(
 	struct se_portal_group *se_tpg)
 {
@@ -912,6 +919,46 @@ static void tcm_loop_port_unlink(
 
 /* End items for tcm_loop_port_cit */
 
+static ssize_t tcm_loop_tpg_attrib_show_fabric_prot_type(
+	struct se_portal_group *se_tpg,
+	char *page)
+{
+	struct tcm_loop_tpg *tl_tpg = container_of(se_tpg, struct tcm_loop_tpg,
+						   tl_se_tpg);
+
+	return sprintf(page, "%d\n", tl_tpg->tl_fabric_prot_type);
+}
+
+static ssize_t tcm_loop_tpg_attrib_store_fabric_prot_type(
+	struct se_portal_group *se_tpg,
+	const char *page,
+	size_t count)
+{
+	struct tcm_loop_tpg *tl_tpg = container_of(se_tpg, struct tcm_loop_tpg,
+						   tl_se_tpg);
+	unsigned long val;
+	int ret = kstrtoul(page, 0, &val);
+
+	if (ret) {
+		pr_err("kstrtoul() returned %d for fabric_prot_type\n", ret);
+		return ret;
+	}
+	if (val != 0 && val != 1 && val != 3) {
+		pr_err("Invalid qla2xxx fabric_prot_type: %lu\n", val);
+		return -EINVAL;
+	}
+	tl_tpg->tl_fabric_prot_type = val;
+
+	return count;
+}
+
+TF_TPG_ATTRIB_ATTR(tcm_loop, fabric_prot_type, S_IRUGO | S_IWUSR);
+
+static struct configfs_attribute *tcm_loop_tpg_attrib_attrs[] = {
+	&tcm_loop_tpg_attrib_fabric_prot_type.attr,
+	NULL,
+};
+
 /* Start items for tcm_loop_nexus_cit */
 
 static int tcm_loop_make_nexus(
@@ -937,7 +984,8 @@ static int tcm_loop_make_nexus(
 	/*
 	 * Initialize the struct se_session pointer
 	 */
-	tl_nexus->se_sess = transport_init_session(TARGET_PROT_ALL);
+	tl_nexus->se_sess = transport_init_session(
+				TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS);
 	if (IS_ERR(tl_nexus->se_sess)) {
 		ret = PTR_ERR(tl_nexus->se_sess);
 		goto out;
@@ -1377,6 +1425,8 @@ static int tcm_loop_register_configfs(void)
 					&tcm_loop_check_demo_mode_write_protect;
 	fabric->tf_ops.tpg_check_prod_mode_write_protect =
 					&tcm_loop_check_prod_mode_write_protect;
+	fabric->tf_ops.tpg_check_prot_fabric_only =
+					&tcm_loop_check_prot_fabric_only;
 	/*
 	 * The TCM loopback fabric module runs in demo-mode to a local
 	 * virtual SCSI device, so fabric dependent initator ACLs are
@@ -1429,7 +1479,7 @@ static int tcm_loop_register_configfs(void)
 	 */
 	fabric->tf_cit_tmpl.tfc_wwn_cit.ct_attrs = tcm_loop_wwn_attrs;
 	fabric->tf_cit_tmpl.tfc_tpg_base_cit.ct_attrs = tcm_loop_tpg_attrs;
-	fabric->tf_cit_tmpl.tfc_tpg_attrib_cit.ct_attrs = NULL;
+	fabric->tf_cit_tmpl.tfc_tpg_attrib_cit.ct_attrs = tcm_loop_tpg_attrib_attrs;
 	fabric->tf_cit_tmpl.tfc_tpg_param_cit.ct_attrs = NULL;
 	fabric->tf_cit_tmpl.tfc_tpg_np_base_cit.ct_attrs = NULL;
 	/*
diff --git a/drivers/target/loopback/tcm_loop.h b/drivers/target/loopback/tcm_loop.h
index 6ae49f2..1e72ff7 100644
--- a/drivers/target/loopback/tcm_loop.h
+++ b/drivers/target/loopback/tcm_loop.h
@@ -43,6 +43,7 @@ struct tcm_loop_nacl {
 struct tcm_loop_tpg {
 	unsigned short tl_tpgt;
 	unsigned short tl_transport_status;
+	enum target_prot_type tl_fabric_prot_type;
 	atomic_t tl_tpg_port_count;
 	struct se_portal_group tl_se_tpg;
 	struct tcm_loop_hba *tl_hba;
-- 
1.9.1

Re: [PATCH-v2 11/15] loopback: Add fabric_prot_type attribute support

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch updates loopback to add a new fabric_prot_type TPG attribute,
> used for controlling LLD level protection into LIO when the backend
> device does not support T10-PI.
>
> Also, go ahead and set DIN_PASS + DOUT_PASS so target-core knows that
> it will be doing any WRITE_STRIP and READ_INSERT operations.
>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/loopback/tcm_loop.c | 54 ++++++++++++++++++++++++++++++++++++--
>   drivers/target/loopback/tcm_loop.h |  1 +
>   2 files changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> index f4618e7..797c731 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c
> @@ -697,6 +697,13 @@ static int tcm_loop_check_prod_mode_write_protect(struct se_portal_group *se_tpg
>   	return 0;
>   }
>
> +static int tcm_loop_check_prot_fabric_only(struct se_portal_group *se_tpg)
> +{
> +	struct tcm_loop_tpg *tl_tpg = container_of(se_tpg, struct tcm_loop_tpg,
> +						   tl_se_tpg);
> +	return tl_tpg->tl_fabric_prot_type;
> +}
> +

So now loopback devices can finally protect transfers with
read_verify=0, write_generate=0?

>   static struct se_node_acl *tcm_loop_tpg_alloc_fabric_acl(
>   	struct se_portal_group *se_tpg)
>   {
> @@ -912,6 +919,46 @@ static void tcm_loop_port_unlink(
>
>   /* End items for tcm_loop_port_cit */
>
> +static ssize_t tcm_loop_tpg_attrib_show_fabric_prot_type(
> +	struct se_portal_group *se_tpg,
> +	char *page)
> +{
> +	struct tcm_loop_tpg *tl_tpg = container_of(se_tpg, struct tcm_loop_tpg,
> +						   tl_se_tpg);
> +
> +	return sprintf(page, "%d\n", tl_tpg->tl_fabric_prot_type);
> +}
> +
> +static ssize_t tcm_loop_tpg_attrib_store_fabric_prot_type(
> +	struct se_portal_group *se_tpg,
> +	const char *page,
> +	size_t count)
> +{
> +	struct tcm_loop_tpg *tl_tpg = container_of(se_tpg, struct tcm_loop_tpg,
> +						   tl_se_tpg);
> +	unsigned long val;
> +	int ret = kstrtoul(page, 0, &val);
> +
> +	if (ret) {
> +		pr_err("kstrtoul() returned %d for fabric_prot_type\n", ret);
> +		return ret;
> +	}
> +	if (val != 0 && val != 1 && val != 3) {
> +		pr_err("Invalid qla2xxx fabric_prot_type: %lu\n", val);
> +		return -EINVAL;
> +	}
> +	tl_tpg->tl_fabric_prot_type = val;
> +
> +	return count;
> +}
> +
> +TF_TPG_ATTRIB_ATTR(tcm_loop, fabric_prot_type, S_IRUGO | S_IWUSR);
> +
> +static struct configfs_attribute *tcm_loop_tpg_attrib_attrs[] = {
> +	&tcm_loop_tpg_attrib_fabric_prot_type.attr,
> +	NULL,
> +};
> +
>   /* Start items for tcm_loop_nexus_cit */
>
>   static int tcm_loop_make_nexus(
> @@ -937,7 +984,8 @@ static int tcm_loop_make_nexus(
>   	/*
>   	 * Initialize the struct se_session pointer
>   	 */
> -	tl_nexus->se_sess = transport_init_session(TARGET_PROT_ALL);
> +	tl_nexus->se_sess = transport_init_session(
> +				TARGET_PROT_DIN_PASS | TARGET_PROT_DOUT_PASS);
>   	if (IS_ERR(tl_nexus->se_sess)) {
>   		ret = PTR_ERR(tl_nexus->se_sess);
>   		goto out;
> @@ -1377,6 +1425,8 @@ static int tcm_loop_register_configfs(void)
>   					&tcm_loop_check_demo_mode_write_protect;
>   	fabric->tf_ops.tpg_check_prod_mode_write_protect =
>   					&tcm_loop_check_prod_mode_write_protect;
> +	fabric->tf_ops.tpg_check_prot_fabric_only =
> +					&tcm_loop_check_prot_fabric_only;
>   	/*
>   	 * The TCM loopback fabric module runs in demo-mode to a local
>   	 * virtual SCSI device, so fabric dependent initator ACLs are
> @@ -1429,7 +1479,7 @@ static int tcm_loop_register_configfs(void)
>   	 */
>   	fabric->tf_cit_tmpl.tfc_wwn_cit.ct_attrs = tcm_loop_wwn_attrs;
>   	fabric->tf_cit_tmpl.tfc_tpg_base_cit.ct_attrs = tcm_loop_tpg_attrs;
> -	fabric->tf_cit_tmpl.tfc_tpg_attrib_cit.ct_attrs = NULL;
> +	fabric->tf_cit_tmpl.tfc_tpg_attrib_cit.ct_attrs = tcm_loop_tpg_attrib_attrs;
>   	fabric->tf_cit_tmpl.tfc_tpg_param_cit.ct_attrs = NULL;
>   	fabric->tf_cit_tmpl.tfc_tpg_np_base_cit.ct_attrs = NULL;
>   	/*
> diff --git a/drivers/target/loopback/tcm_loop.h b/drivers/target/loopback/tcm_loop.h
> index 6ae49f2..1e72ff7 100644
> --- a/drivers/target/loopback/tcm_loop.h
> +++ b/drivers/target/loopback/tcm_loop.h
> @@ -43,6 +43,7 @@ struct tcm_loop_nacl {
>   struct tcm_loop_tpg {
>   	unsigned short tl_tpgt;
>   	unsigned short tl_transport_status;
> +	enum target_prot_type tl_fabric_prot_type;
>   	atomic_t tl_tpg_port_count;
>   	struct se_portal_group tl_se_tpg;
>   	struct tcm_loop_hba *tl_hba;
>

Re: [PATCH-v2 11/15] loopback: Add fabric_prot_type attribute support

[Nicholas A. Bellinger]

On Mon, 2015-03-30 at 11:07 +0300, Sagi Grimberg wrote:
> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch updates loopback to add a new fabric_prot_type TPG attribute,
> > used for controlling LLD level protection into LIO when the backend
> > device does not support T10-PI.
> >
> > Also, go ahead and set DIN_PASS + DOUT_PASS so target-core knows that
> > it will be doing any WRITE_STRIP and READ_INSERT operations.
> >
> > Cc: Martin Petersen <martin.petersen@oracle.com>
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/loopback/tcm_loop.c | 54 ++++++++++++++++++++++++++++++++++++--
> >   drivers/target/loopback/tcm_loop.h |  1 +
> >   2 files changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> > index f4618e7..797c731 100644
> > --- a/drivers/target/loopback/tcm_loop.c
> > +++ b/drivers/target/loopback/tcm_loop.c
> > @@ -697,6 +697,13 @@ static int tcm_loop_check_prod_mode_write_protect(struct se_portal_group *se_tpg
> >   	return 0;
> >   }
> >
> > +static int tcm_loop_check_prot_fabric_only(struct se_portal_group *se_tpg)
> > +{
> > +	struct tcm_loop_tpg *tl_tpg = container_of(se_tpg, struct tcm_loop_tpg,
> > +						   tl_se_tpg);
> > +	return tl_tpg->tl_fabric_prot_type;
> > +}
> > +
> 
> So now loopback devices can finally protect transfers with
> read_verify=0, write_generate=0?
> 

(Adding Jerome CC')

Yes.

However, a non zero tl_fabric_prot_type TPG attribute value needs to be
set before I_T nexus creation occurs in order for the loopback session
to report se_sess->sess_prot_type in SBC/SPC control bit emulation code.

Also, rtslib needs to be updated to optionally set prot_fabric_only=1
before nexus creation occurs to enable this code.

--nab

[PATCH-v2 12/15] vhost/scsi: Add fabric_prot_type attribute support

[Nicholas A. Bellinger]

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

This patch updates vhost-scsi to add a new fabric_prot_type TPG
attribute, used for controlling LLD level protection into LIO when
the backend device does not support T10-PI.

This is required for vhost-scsi to enable WRITE_STRIP + READ_INSERT
operations using software emulation + crct10dif instruction offload.

It's disabled by default and controls which se_sesion->sess_prot_type
are set at vhost_scsi_make_nexus() session registration time.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/scsi.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 8d4f3f1..27ed964 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -131,6 +131,8 @@ struct vhost_scsi_tpg {
 	int tv_tpg_port_count;
 	/* Used for vhost_scsi device reference to tpg_nexus, protected by tv_tpg_mutex */
 	int tv_tpg_vhost_count;
+	/* Used for enabling T10-PI with legacy devices */
+	int tv_fabric_prot_type;
 	/* list for vhost_scsi_list */
 	struct list_head tv_tpg_list;
 	/* Used to protect access for tpg_nexus */
@@ -431,6 +433,14 @@ vhost_scsi_parse_pr_out_transport_id(struct se_portal_group *se_tpg,
 			port_nexus_ptr);
 }
 
+static int vhost_scsi_check_prot_fabric_only(struct se_portal_group *se_tpg)
+{
+	struct vhost_scsi_tpg *tpg = container_of(se_tpg,
+				struct vhost_scsi_tpg, se_tpg);
+
+	return tpg->tv_fabric_prot_type;
+}
+
 static struct se_node_acl *
 vhost_scsi_alloc_fabric_acl(struct se_portal_group *se_tpg)
 {
@@ -1878,6 +1888,45 @@ static void vhost_scsi_free_cmd_map_res(struct vhost_scsi_nexus *nexus,
 	}
 }
 
+static ssize_t vhost_scsi_tpg_attrib_store_fabric_prot_type(
+	struct se_portal_group *se_tpg,
+	const char *page,
+	size_t count)
+{
+	struct vhost_scsi_tpg *tpg = container_of(se_tpg,
+				struct vhost_scsi_tpg, se_tpg);
+	unsigned long val;
+	int ret = kstrtoul(page, 0, &val);
+
+	if (ret) {
+		pr_err("kstrtoul() returned %d for fabric_prot_type\n", ret);
+		return ret;
+	}
+	if (val != 0 && val != 1 && val != 3) {
+		pr_err("Invalid vhost_scsi fabric_prot_type: %lu\n", val);
+		return -EINVAL;
+	}
+	tpg->tv_fabric_prot_type = val;
+
+	return count;
+}
+
+static ssize_t vhost_scsi_tpg_attrib_show_fabric_prot_type(
+	struct se_portal_group *se_tpg,
+	char *page)
+{
+	struct vhost_scsi_tpg *tpg = container_of(se_tpg,
+				struct vhost_scsi_tpg, se_tpg);
+
+	return sprintf(page, "%d\n", tpg->tv_fabric_prot_type);
+}
+TF_TPG_ATTRIB_ATTR(vhost_scsi, fabric_prot_type, S_IRUGO | S_IWUSR);
+
+static struct configfs_attribute *vhost_scsi_tpg_attrib_attrs[] = {
+	&vhost_scsi_tpg_attrib_fabric_prot_type.attr,
+	NULL,
+};
+
 static int vhost_scsi_make_nexus(struct vhost_scsi_tpg *tpg,
 				const char *name)
 {
@@ -2290,6 +2339,7 @@ static struct target_core_fabric_ops vhost_scsi_ops = {
 	.tpg_check_demo_mode_cache	= vhost_scsi_check_true,
 	.tpg_check_demo_mode_write_protect = vhost_scsi_check_false,
 	.tpg_check_prod_mode_write_protect = vhost_scsi_check_false,
+	.tpg_check_prot_fabric_only	= vhost_scsi_check_prot_fabric_only,
 	.tpg_alloc_fabric_acl		= vhost_scsi_alloc_fabric_acl,
 	.tpg_release_fabric_acl		= vhost_scsi_release_fabric_acl,
 	.tpg_get_inst_index		= vhost_scsi_tpg_get_inst_index,
@@ -2348,7 +2398,7 @@ static int vhost_scsi_register_configfs(void)
 	 */
 	fabric->tf_cit_tmpl.tfc_wwn_cit.ct_attrs = vhost_scsi_wwn_attrs;
 	fabric->tf_cit_tmpl.tfc_tpg_base_cit.ct_attrs = vhost_scsi_tpg_attrs;
-	fabric->tf_cit_tmpl.tfc_tpg_attrib_cit.ct_attrs = NULL;
+	fabric->tf_cit_tmpl.tfc_tpg_attrib_cit.ct_attrs = vhost_scsi_tpg_attrib_attrs;
 	fabric->tf_cit_tmpl.tfc_tpg_param_cit.ct_attrs = NULL;
 	fabric->tf_cit_tmpl.tfc_tpg_np_base_cit.ct_attrs = NULL;
 	fabric->tf_cit_tmpl.tfc_tpg_nacl_base_cit.ct_attrs = NULL;
-- 
1.9.1

[PATCH-v2 13/15] tcm_qla2xxx: Set TARGET_PROT_ALL for sup_prot_ops

[Nicholas A. Bellinger]

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

This patch adds the missing TARGET_PROT_ALL when initializing a new
session and declaring the capable se_sess->sup_prot_ops for T10-PI.

This is required in order to function with existing qla_target.c
DIF protection offload support.

Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index c4f66f5..57346ab 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1570,7 +1570,7 @@ static int tcm_qla2xxx_check_initiator_node_acl(
 
 	se_sess = transport_init_session_tags(num_tags,
 					      sizeof(struct qla_tgt_cmd),
-					      TARGET_PROT_NORMAL);
+					      TARGET_PROT_ALL);
 	if (IS_ERR(se_sess)) {
 		pr_err("Unable to initialize struct se_session\n");
 		return PTR_ERR(se_sess);
-- 
1.9.1

[PATCH-v2 14/15] tcm_qla2xxx: Add fabric_prot_type attribute support

[Nicholas A. Bellinger]

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

This patch updates qla2xxx target to add a new fabric_prot_type TPG
attribute, used for controlling LLD level protection into LIO when
the backend device does not support T10-PI.

This is required for qla_target.c to enable WRITE_STRIP + READ_INSERT
hardware offloads.

It's disabled by default and controls which se_sesion->sess_prot_type
are set at tcm_qla2xxx_check_initiator_node_acl() session registration
time.

Cc: Quinn Tran <quinn.tran@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
Cc: Giridhar Malavali <giridhar.malavali@qlogic.com>
Cc: Martin Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 44 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |  1 +
 2 files changed, 45 insertions(+)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 57346ab..843b53b 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -336,6 +336,14 @@ static int tcm_qla2xxx_check_demo_mode_login_only(struct se_portal_group *se_tpg
 	return tpg->tpg_attrib.demo_mode_login_only;
 }
 
+static int tcm_qla2xxx_check_prot_fabric_only(struct se_portal_group *se_tpg)
+{
+	struct tcm_qla2xxx_tpg *tpg = container_of(se_tpg,
+				struct tcm_qla2xxx_tpg, se_tpg);
+
+	return tpg->tpg_attrib.fabric_prot_type;
+}
+
 static struct se_node_acl *tcm_qla2xxx_alloc_fabric_acl(
 	struct se_portal_group *se_tpg)
 {
@@ -1091,9 +1099,44 @@ static ssize_t tcm_qla2xxx_tpg_show_dynamic_sessions(
 
 TF_TPG_BASE_ATTR_RO(tcm_qla2xxx, dynamic_sessions);
 
+static ssize_t tcm_qla2xxx_tpg_store_fabric_prot_type(
+	struct se_portal_group *se_tpg,
+	const char *page,
+	size_t count)
+{
+	struct tcm_qla2xxx_tpg *tpg = container_of(se_tpg,
+				struct tcm_qla2xxx_tpg, se_tpg);
+	unsigned long val;
+	int ret = kstrtoul(page, 0, &val);
+
+	if (ret) {
+		pr_err("kstrtoul() returned %d for fabric_prot_type\n", ret);
+		return ret;
+	}
+	if (val != 0 && val != 1 && val != 3) {
+		pr_err("Invalid qla2xxx fabric_prot_type: %lu\n", val);
+		return -EINVAL;
+	}
+	tpg->tpg_attrib.fabric_prot_type = val;
+
+	return count;
+}
+
+static ssize_t tcm_qla2xxx_tpg_show_fabric_prot_type(
+	struct se_portal_group *se_tpg,
+	char *page)
+{
+	struct tcm_qla2xxx_tpg *tpg = container_of(se_tpg,
+				struct tcm_qla2xxx_tpg, se_tpg);
+
+	return sprintf(page, "%d\n", tpg->tpg_attrib.fabric_prot_type);
+}
+TF_TPG_BASE_ATTR(tcm_qla2xxx, fabric_prot_type, S_IRUGO | S_IWUSR);
+
 static struct configfs_attribute *tcm_qla2xxx_tpg_attrs[] = {
 	&tcm_qla2xxx_tpg_enable.attr,
 	&tcm_qla2xxx_tpg_dynamic_sessions.attr,
+	&tcm_qla2xxx_tpg_fabric_prot_type.attr,
 	NULL,
 };
 
@@ -1959,6 +2002,7 @@ static struct target_core_fabric_ops tcm_qla2xxx_ops = {
 					tcm_qla2xxx_check_demo_write_protect,
 	.tpg_check_prod_mode_write_protect =
 					tcm_qla2xxx_check_prod_write_protect,
+	.tpg_check_prot_fabric_only	= tcm_qla2xxx_check_prot_fabric_only,
 	.tpg_check_demo_mode_login_only = tcm_qla2xxx_check_demo_mode_login_only,
 	.tpg_alloc_fabric_acl		= tcm_qla2xxx_alloc_fabric_acl,
 	.tpg_release_fabric_acl		= tcm_qla2xxx_release_fabric_acl,
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
index 10c0021..2329511 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
@@ -33,6 +33,7 @@ struct tcm_qla2xxx_tpg_attrib {
 	int demo_mode_write_protect;
 	int prod_mode_write_protect;
 	int demo_mode_login_only;
+	int fabric_prot_type;
 };
 
 struct tcm_qla2xxx_tpg {
-- 
1.9.1

[PATCH-v2 15/15] iscsi/iser-target: Add fabric_prot_type attribute support

[Nicholas A. Bellinger]

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

This patch updates iscsi/iser-target to add a new fabric_prot_type
TPG attribute for iser-target, used for controlling LLD level
protection into LIO when the backend device does not support T10-PI.

This is required for ib_isert to enable WRITE_STRIP + READ_INSERT
hardware offloads.

It's disabled by default and controls which se_sesion->sess_prot_type
are set at iscsi_target_locate_portal() session registration time.

Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Martin Petersen <martin.petersen@oracle.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_configfs.c | 22 ++++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_tpg.c      | 19 +++++++++++++++++++
 drivers/target/iscsi/iscsi_target_tpg.h      |  1 +
 include/target/iscsi/iscsi_target_core.h     |  2 ++
 4 files changed, 44 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index 95a67f6..9cb5ab4 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1052,6 +1052,11 @@ TPG_ATTR(default_erl, S_IRUGO | S_IWUSR);
  */
 DEF_TPG_ATTRIB(t10_pi);
 TPG_ATTR(t10_pi, S_IRUGO | S_IWUSR);
+/*
+ * Define iscsi_tpg_attrib_s_fabric_prot_type
+ */
+DEF_TPG_ATTRIB(fabric_prot_type);
+TPG_ATTR(fabric_prot_type, S_IRUGO | S_IWUSR);
 
 static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
 	&iscsi_tpg_attrib_authentication.attr,
@@ -1065,6 +1070,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
 	&iscsi_tpg_attrib_demo_mode_discovery.attr,
 	&iscsi_tpg_attrib_default_erl.attr,
 	&iscsi_tpg_attrib_t10_pi.attr,
+	&iscsi_tpg_attrib_fabric_prot_type.attr,
 	NULL,
 };
 
@@ -1882,6 +1888,20 @@ static int lio_tpg_check_prod_mode_write_protect(
 	return tpg->tpg_attrib.prod_mode_write_protect;
 }
 
+static int lio_tpg_check_prot_fabric_only(
+	struct se_portal_group *se_tpg)
+{
+	struct iscsi_portal_group *tpg = se_tpg->se_tpg_fabric_ptr;
+	/*
+	 * Only report fabric_prot_type if t10_pi has also been enabled
+	 * for incoming ib_isert sessions.
+	 */
+	if (!tpg->tpg_attrib.t10_pi)
+		return 0;
+
+	return tpg->tpg_attrib.fabric_prot_type;
+}
+
 static void lio_tpg_release_fabric_acl(
 	struct se_portal_group *se_tpg,
 	struct se_node_acl *se_acl)
@@ -1997,6 +2017,8 @@ int iscsi_target_register_configfs(void)
 				&lio_tpg_check_demo_mode_write_protect;
 	fabric->tf_ops.tpg_check_prod_mode_write_protect =
 				&lio_tpg_check_prod_mode_write_protect;
+	fabric->tf_ops.tpg_check_prot_fabric_only =
+				&lio_tpg_check_prot_fabric_only;
 	fabric->tf_ops.tpg_alloc_fabric_acl = &lio_tpg_alloc_fabric_acl;
 	fabric->tf_ops.tpg_release_fabric_acl = &lio_tpg_release_fabric_acl;
 	fabric->tf_ops.tpg_get_inst_index = &lio_tpg_get_inst_index;
diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index bdd127c..3076e6f 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -228,6 +228,7 @@ static void iscsit_set_default_tpg_attribs(struct iscsi_portal_group *tpg)
 	a->demo_mode_discovery = TA_DEMO_MODE_DISCOVERY;
 	a->default_erl = TA_DEFAULT_ERL;
 	a->t10_pi = TA_DEFAULT_T10_PI;
+	a->fabric_prot_type = TA_DEFAULT_FABRIC_PROT_TYPE;
 }
 
 int iscsit_tpg_add_portal_group(struct iscsi_tiqn *tiqn, struct iscsi_portal_group *tpg)
@@ -878,3 +879,21 @@ int iscsit_ta_t10_pi(
 
 	return 0;
 }
+
+int iscsit_ta_fabric_prot_type(
+	struct iscsi_portal_group *tpg,
+	u32 prot_type)
+{
+	struct iscsi_tpg_attrib *a = &tpg->tpg_attrib;
+
+	if ((prot_type != 0) && (prot_type != 1) && (prot_type != 3)) {
+		pr_err("Illegal value for fabric_prot_type: %u\n", prot_type);
+		return -EINVAL;
+	}
+
+	a->fabric_prot_type = prot_type;
+	pr_debug("iSCSI_TPG[%hu] - T10 Fabric Protection Type: %u\n",
+		 tpg->tpgt, prot_type);
+
+	return 0;
+}
diff --git a/drivers/target/iscsi/iscsi_target_tpg.h b/drivers/target/iscsi/iscsi_target_tpg.h
index e726533..95ff5bd 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.h
+++ b/drivers/target/iscsi/iscsi_target_tpg.h
@@ -39,5 +39,6 @@ extern int iscsit_ta_prod_mode_write_protect(struct iscsi_portal_group *, u32);
 extern int iscsit_ta_demo_mode_discovery(struct iscsi_portal_group *, u32);
 extern int iscsit_ta_default_erl(struct iscsi_portal_group *, u32);
 extern int iscsit_ta_t10_pi(struct iscsi_portal_group *, u32);
+extern int iscsit_ta_fabric_prot_type(struct iscsi_portal_group *, u32);
 
 #endif /* ISCSI_TARGET_TPG_H */
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 0e394a0..54e7af3 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -62,6 +62,7 @@
 #define TA_CACHE_CORE_NPS		0
 /* T10 protection information disabled by default */
 #define TA_DEFAULT_T10_PI		0
+#define TA_DEFAULT_FABRIC_PROT_TYPE	0
 
 #define ISCSI_IOV_DATA_BUFFER		5
 
@@ -772,6 +773,7 @@ struct iscsi_tpg_attrib {
 	u32			demo_mode_discovery;
 	u32			default_erl;
 	u8			t10_pi;
+	u32			fabric_prot_type;
 	struct iscsi_portal_group *tpg;
 };
 
-- 
1.9.1

Re: [PATCH-v2 15/15] iscsi/iser-target: Add fabric_prot_type attribute support

[Sagi Grimberg]

On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
>
> This patch updates iscsi/iser-target to add a new fabric_prot_type
> TPG attribute for iser-target, used for controlling LLD level
> protection into LIO when the backend device does not support T10-PI.
>
> This is required for ib_isert to enable WRITE_STRIP + READ_INSERT
> hardware offloads.
>
> It's disabled by default and controls which se_sesion->sess_prot_type
> are set at iscsi_target_locate_portal() session registration time.
>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Martin Petersen <martin.petersen@oracle.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>   drivers/target/iscsi/iscsi_target_configfs.c | 22 ++++++++++++++++++++++
>   drivers/target/iscsi/iscsi_target_tpg.c      | 19 +++++++++++++++++++
>   drivers/target/iscsi/iscsi_target_tpg.h      |  1 +
>   include/target/iscsi/iscsi_target_core.h     |  2 ++
>   4 files changed, 44 insertions(+)
>
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 95a67f6..9cb5ab4 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1052,6 +1052,11 @@ TPG_ATTR(default_erl, S_IRUGO | S_IWUSR);
>    */
>   DEF_TPG_ATTRIB(t10_pi);
>   TPG_ATTR(t10_pi, S_IRUGO | S_IWUSR);
> +/*
> + * Define iscsi_tpg_attrib_s_fabric_prot_type
> + */
> +DEF_TPG_ATTRIB(fabric_prot_type);
> +TPG_ATTR(fabric_prot_type, S_IRUGO | S_IWUSR);
>
>   static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
>   	&iscsi_tpg_attrib_authentication.attr,
> @@ -1065,6 +1070,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
>   	&iscsi_tpg_attrib_demo_mode_discovery.attr,
>   	&iscsi_tpg_attrib_default_erl.attr,
>   	&iscsi_tpg_attrib_t10_pi.attr,
> +	&iscsi_tpg_attrib_fabric_prot_type.attr,
>   	NULL,
>   };
>
> @@ -1882,6 +1888,20 @@ static int lio_tpg_check_prod_mode_write_protect(
>   	return tpg->tpg_attrib.prod_mode_write_protect;
>   }
>
> +static int lio_tpg_check_prot_fabric_only(
> +	struct se_portal_group *se_tpg)
> +{
> +	struct iscsi_portal_group *tpg = se_tpg->se_tpg_fabric_ptr;
> +	/*
> +	 * Only report fabric_prot_type if t10_pi has also been enabled
> +	 * for incoming ib_isert sessions.
> +	 */
> +	if (!tpg->tpg_attrib.t10_pi)
> +		return 0;
> +
> +	return tpg->tpg_attrib.fabric_prot_type;
> +}
> +
>   static void lio_tpg_release_fabric_acl(
>   	struct se_portal_group *se_tpg,
>   	struct se_node_acl *se_acl)
> @@ -1997,6 +2017,8 @@ int iscsi_target_register_configfs(void)
>   				&lio_tpg_check_demo_mode_write_protect;
>   	fabric->tf_ops.tpg_check_prod_mode_write_protect =
>   				&lio_tpg_check_prod_mode_write_protect;
> +	fabric->tf_ops.tpg_check_prot_fabric_only =
> +				&lio_tpg_check_prot_fabric_only;
>   	fabric->tf_ops.tpg_alloc_fabric_acl = &lio_tpg_alloc_fabric_acl;
>   	fabric->tf_ops.tpg_release_fabric_acl = &lio_tpg_release_fabric_acl;
>   	fabric->tf_ops.tpg_get_inst_index = &lio_tpg_get_inst_index;
> diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
> index bdd127c..3076e6f 100644
> --- a/drivers/target/iscsi/iscsi_target_tpg.c
> +++ b/drivers/target/iscsi/iscsi_target_tpg.c
> @@ -228,6 +228,7 @@ static void iscsit_set_default_tpg_attribs(struct iscsi_portal_group *tpg)
>   	a->demo_mode_discovery = TA_DEMO_MODE_DISCOVERY;
>   	a->default_erl = TA_DEFAULT_ERL;
>   	a->t10_pi = TA_DEFAULT_T10_PI;
> +	a->fabric_prot_type = TA_DEFAULT_FABRIC_PROT_TYPE;
>   }
>
>   int iscsit_tpg_add_portal_group(struct iscsi_tiqn *tiqn, struct iscsi_portal_group *tpg)
> @@ -878,3 +879,21 @@ int iscsit_ta_t10_pi(
>
>   	return 0;
>   }
> +
> +int iscsit_ta_fabric_prot_type(
> +	struct iscsi_portal_group *tpg,
> +	u32 prot_type)
> +{
> +	struct iscsi_tpg_attrib *a = &tpg->tpg_attrib;
> +
> +	if ((prot_type != 0) && (prot_type != 1) && (prot_type != 3)) {
> +		pr_err("Illegal value for fabric_prot_type: %u\n", prot_type);
> +		return -EINVAL;
> +	}
> +
> +	a->fabric_prot_type = prot_type;
> +	pr_debug("iSCSI_TPG[%hu] - T10 Fabric Protection Type: %u\n",
> +		 tpg->tpgt, prot_type);

I wander what will happen if this is modified on the fly with active
sessions, LUNs, IO...

Should we restrict this to be modified only offline (no active
sessions)?

> +
> +	return 0;
> +}
> diff --git a/drivers/target/iscsi/iscsi_target_tpg.h b/drivers/target/iscsi/iscsi_target_tpg.h
> index e726533..95ff5bd 100644
> --- a/drivers/target/iscsi/iscsi_target_tpg.h
> +++ b/drivers/target/iscsi/iscsi_target_tpg.h
> @@ -39,5 +39,6 @@ extern int iscsit_ta_prod_mode_write_protect(struct iscsi_portal_group *, u32);
>   extern int iscsit_ta_demo_mode_discovery(struct iscsi_portal_group *, u32);
>   extern int iscsit_ta_default_erl(struct iscsi_portal_group *, u32);
>   extern int iscsit_ta_t10_pi(struct iscsi_portal_group *, u32);
> +extern int iscsit_ta_fabric_prot_type(struct iscsi_portal_group *, u32);
>
>   #endif /* ISCSI_TARGET_TPG_H */
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index 0e394a0..54e7af3 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -62,6 +62,7 @@
>   #define TA_CACHE_CORE_NPS		0
>   /* T10 protection information disabled by default */
>   #define TA_DEFAULT_T10_PI		0
> +#define TA_DEFAULT_FABRIC_PROT_TYPE	0
>
>   #define ISCSI_IOV_DATA_BUFFER		5
>
> @@ -772,6 +773,7 @@ struct iscsi_tpg_attrib {
>   	u32			demo_mode_discovery;
>   	u32			default_erl;
>   	u8			t10_pi;
> +	u32			fabric_prot_type;
>   	struct iscsi_portal_group *tpg;
>   };
>
>

Re: [PATCH-v2 15/15] iscsi/iser-target: Add fabric_prot_type attribute support

[Nicholas A. Bellinger]

On Mon, 2015-03-30 at 11:11 +0300, Sagi Grimberg wrote:
> On 3/30/2015 6:28 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> >
> > This patch updates iscsi/iser-target to add a new fabric_prot_type
> > TPG attribute for iser-target, used for controlling LLD level
> > protection into LIO when the backend device does not support T10-PI.
> >
> > This is required for ib_isert to enable WRITE_STRIP + READ_INSERT
> > hardware offloads.
> >
> > It's disabled by default and controls which se_sesion->sess_prot_type
> > are set at iscsi_target_locate_portal() session registration time.
> >
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Martin Petersen <martin.petersen@oracle.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >   drivers/target/iscsi/iscsi_target_configfs.c | 22 ++++++++++++++++++++++
> >   drivers/target/iscsi/iscsi_target_tpg.c      | 19 +++++++++++++++++++
> >   drivers/target/iscsi/iscsi_target_tpg.h      |  1 +
> >   include/target/iscsi/iscsi_target_core.h     |  2 ++
> >   4 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> > index 95a67f6..9cb5ab4 100644
> > --- a/drivers/target/iscsi/iscsi_target_configfs.c
> > +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> > @@ -1052,6 +1052,11 @@ TPG_ATTR(default_erl, S_IRUGO | S_IWUSR);
> >    */
> >   DEF_TPG_ATTRIB(t10_pi);
> >   TPG_ATTR(t10_pi, S_IRUGO | S_IWUSR);
> > +/*
> > + * Define iscsi_tpg_attrib_s_fabric_prot_type
> > + */
> > +DEF_TPG_ATTRIB(fabric_prot_type);
> > +TPG_ATTR(fabric_prot_type, S_IRUGO | S_IWUSR);
> >
> >   static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
> >   	&iscsi_tpg_attrib_authentication.attr,
> > @@ -1065,6 +1070,7 @@ static struct configfs_attribute *lio_target_tpg_attrib_attrs[] = {
> >   	&iscsi_tpg_attrib_demo_mode_discovery.attr,
> >   	&iscsi_tpg_attrib_default_erl.attr,
> >   	&iscsi_tpg_attrib_t10_pi.attr,
> > +	&iscsi_tpg_attrib_fabric_prot_type.attr,
> >   	NULL,
> >   };
> >
> > @@ -1882,6 +1888,20 @@ static int lio_tpg_check_prod_mode_write_protect(
> >   	return tpg->tpg_attrib.prod_mode_write_protect;
> >   }
> >
> > +static int lio_tpg_check_prot_fabric_only(
> > +	struct se_portal_group *se_tpg)
> > +{
> > +	struct iscsi_portal_group *tpg = se_tpg->se_tpg_fabric_ptr;
> > +	/*
> > +	 * Only report fabric_prot_type if t10_pi has also been enabled
> > +	 * for incoming ib_isert sessions.
> > +	 */
> > +	if (!tpg->tpg_attrib.t10_pi)
> > +		return 0;
> > +
> > +	return tpg->tpg_attrib.fabric_prot_type;
> > +}
> > +
> >   static void lio_tpg_release_fabric_acl(
> >   	struct se_portal_group *se_tpg,
> >   	struct se_node_acl *se_acl)
> > @@ -1997,6 +2017,8 @@ int iscsi_target_register_configfs(void)
> >   				&lio_tpg_check_demo_mode_write_protect;
> >   	fabric->tf_ops.tpg_check_prod_mode_write_protect =
> >   				&lio_tpg_check_prod_mode_write_protect;
> > +	fabric->tf_ops.tpg_check_prot_fabric_only =
> > +				&lio_tpg_check_prot_fabric_only;
> >   	fabric->tf_ops.tpg_alloc_fabric_acl = &lio_tpg_alloc_fabric_acl;
> >   	fabric->tf_ops.tpg_release_fabric_acl = &lio_tpg_release_fabric_acl;
> >   	fabric->tf_ops.tpg_get_inst_index = &lio_tpg_get_inst_index;
> > diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
> > index bdd127c..3076e6f 100644
> > --- a/drivers/target/iscsi/iscsi_target_tpg.c
> > +++ b/drivers/target/iscsi/iscsi_target_tpg.c
> > @@ -228,6 +228,7 @@ static void iscsit_set_default_tpg_attribs(struct iscsi_portal_group *tpg)
> >   	a->demo_mode_discovery = TA_DEMO_MODE_DISCOVERY;
> >   	a->default_erl = TA_DEFAULT_ERL;
> >   	a->t10_pi = TA_DEFAULT_T10_PI;
> > +	a->fabric_prot_type = TA_DEFAULT_FABRIC_PROT_TYPE;
> >   }
> >
> >   int iscsit_tpg_add_portal_group(struct iscsi_tiqn *tiqn, struct iscsi_portal_group *tpg)
> > @@ -878,3 +879,21 @@ int iscsit_ta_t10_pi(
> >
> >   	return 0;
> >   }
> > +
> > +int iscsit_ta_fabric_prot_type(
> > +	struct iscsi_portal_group *tpg,
> > +	u32 prot_type)
> > +{
> > +	struct iscsi_tpg_attrib *a = &tpg->tpg_attrib;
> > +
> > +	if ((prot_type != 0) && (prot_type != 1) && (prot_type != 3)) {
> > +		pr_err("Illegal value for fabric_prot_type: %u\n", prot_type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	a->fabric_prot_type = prot_type;
> > +	pr_debug("iSCSI_TPG[%hu] - T10 Fabric Protection Type: %u\n",
> > +		 tpg->tpgt, prot_type);
> 
> I wander what will happen if this is modified on the fly with active
> sessions, LUNs, IO...
> 
> Should we restrict this to be modified only offline (no active
> sessions)?

Absolutely.

The tpg_check_prot_fabric_only() assignment of sess->sess_prot_type
is restricted to session creation time in __transport_register_session()
code, and changing of the value does not effect existing sessions.

--nab