Skip to contentSkip to conversation

tcm: Add native 32-byte CDB support

PATCH In review Nicholas A. Bellinger 2 participants 4 messages Sep 17, 2010 – Sep 20, 2010

Subsystems (2)
SCSI SCSI target
lore
Share & tools
lore

Nicholas A. Bellinger

author
#0 lore raw

PATCH 1/3 tcm: Add native 32-byte CDB support

From: Nicholas Bellinger <[email protected]>

This patch adds a '#define TCM_MAX_COMMAND_SIZE 32' and converts all inline TCM core and subsystem CDB size defines to use TCM_MAX_COMMAND_SIZE instead of the 16-byte MAX_COMMAND_SIZE. This includes the conversion of MAX_COMMAND_SIZE in certain places to use include/scsi/scsi.h:scsi_command_size() to properly extract the size of a received CDB based on opcode.

This patch also includes the conversion of the FILEIO, IBLOCK, PSCSI, RAMDISK and STGT subsystem modules to use TCM_MAX_COMMAND_SIZE for their default internal inline CDB size.

It also adds transport_lba_64_ext() and transport_get_sectors_32() callers into target_core_transport.c to be used for extracting a 64-bit logical block address and 32-bit transfer length (in block) from 32-byte CDBs. Finally this patch adds split_cdb_XX_32() and split_cdb_XX_32() for handling the generation of 32-byte CDBs for individual struct se_task.

Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/target_core_file.h      |    2 +-
 drivers/target/target_core_iblock.h    |    2 +-
 drivers/target/target_core_pscsi.c     |    2 +-
 drivers/target/target_core_pscsi.h     |    2 +-
 drivers/target/target_core_rd.h        |    2 +-
 drivers/target/target_core_scdb.c      |   38 +++++++++++++++++++++++++++
 drivers/target/target_core_scdb.h      |    2 +
 drivers/target/target_core_stgt.h      |    2 +-
 drivers/target/target_core_transport.c |   44 ++++++++++++++++++++++++++++---
 include/target/target_core_base.h      |   13 +++++++++
 10 files changed, 98 insertions(+), 11 deletions(-)
drivers/target/target_core_file.h +1 -1
index 7831034..35726f1 100644
@@ -26,7 +26,7 @@ extern int linux_blockdevice_check(int, int);
 
 struct fd_request {
 	/* SCSI CDB from iSCSI Command PDU */
-	unsigned char	fd_scsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char	fd_scsi_cdb[TCM_MAX_COMMAND_SIZE];
 	/* Data Direction */
 	u8		fd_data_direction;
 	/* Total length of request */
drivers/target/target_core_iblock.h +1 -1
index 76c39a9..3582aac 100644
@@ -12,7 +12,7 @@
 extern struct se_global *se_global;
 
 struct iblock_req {
-	unsigned char ib_scsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char ib_scsi_cdb[TCM_MAX_COMMAND_SIZE];
 	atomic_t ib_bio_cnt;
 	u32	ib_sg_count;
 	void	*ib_buf;
drivers/target/target_core_pscsi.c +1 -1
index 803853d..beca807 100644
@@ -743,7 +743,7 @@ static inline void pscsi_blk_init_request(
 	 * Load the referenced struct se_task's SCSI CDB into
 	 * include/linux/blkdev.h:struct request->cmd
 	 */
-	pt->pscsi_req->cmd_len = COMMAND_SIZE(pt->pscsi_cdb[0]);
+	pt->pscsi_req->cmd_len = scsi_command_size(pt->pscsi_cdb);
 	memcpy(pt->pscsi_req->cmd, pt->pscsi_cdb, pt->pscsi_req->cmd_len);
 	/*
 	 * Setup pointer for outgoing sense data.
drivers/target/target_core_pscsi.h +1 -1
index db09d6a..086e6f1 100644
@@ -28,7 +28,7 @@ extern int linux_blockdevice_check(int, int);
 #include <linux/kobject.h>
 
 struct pscsi_plugin_task {
-	unsigned char pscsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char pscsi_cdb[TCM_MAX_COMMAND_SIZE];
 	unsigned char pscsi_sense[SCSI_SENSE_BUFFERSIZE];
 	int	pscsi_direction;
 	int	pscsi_result;
drivers/target/target_core_rd.h +1 -1
index 8170b41..2008807 100644
@@ -31,7 +31,7 @@ void rd_module_exit(void);
 
 struct rd_request {
 	/* SCSI CDB from iSCSI Command PDU */
-	unsigned char	rd_scsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char	rd_scsi_cdb[TCM_MAX_COMMAND_SIZE];
 	/* Data Direction */
 	u8		rd_data_direction;
 	/* Total length of request */
drivers/target/target_core_scdb.c +38 -0
index 8bcfeaf..76a7de0 100644
@@ -28,6 +28,7 @@
 
 #include <linux/net.h>
 #include <linux/string.h>
+#include <scsi/scsi.h>
 
 #include <target/target_core_base.h>
 #include <target/target_core_transport.h>

@@ -147,3 +148,40 @@ void split_cdb_RW_16(
 	cdb[0] = (rw) ? 0x8a : 0x88;
 	split_cdb_XX_16(lba, sectors, &cdb[0]);
 }
+
+/*
+ *	split_cdb_XX_32():
+ *	
+ * 	64-bit LBA w/ 32-bit SECTORS such as READ_32, WRITE_32 and XDWRITEREAD_32
+ */
+void split_cdb_XX_32(
+	unsigned long long lba,
+	u32 *sectors,
+	unsigned char *cdb)
+{
+	cdb[12] = (lba >> 56) & 0xff;
+	cdb[13] = (lba >> 48) & 0xff;
+	cdb[14] = (lba >> 40) & 0xff;
+	cdb[15] = (lba >> 32) & 0xff;
+	cdb[16] = (lba >> 24) & 0xff;
+	cdb[17] = (lba >> 16) & 0xff;
+	cdb[18] = (lba >> 8) & 0xff;
+	cdb[19] = lba & 0xff;
+	cdb[28] = (*sectors >> 24) & 0xff;
+	cdb[29] = (*sectors >> 16) & 0xff;
+	cdb[30] = (*sectors >> 8) & 0xff;
+	cdb[31] = *sectors & 0xff;
+}
+
+void split_cdb_RW_32(
+	unsigned long long lba,
+	u32 *sectors,
+	unsigned char *cdb,
+	int rw)
+{
+	/*
+	 * Set service action for VARIABLE_LENGTH_CMD
+	 */
+	cdb[9] = (rw) ? WRITE_32 : READ_32;
+	split_cdb_XX_32(lba, sectors, &cdb[0]);	
+}
drivers/target/target_core_scdb.h +2 -0
index 57688e2..1b0dc74 100644
@@ -9,5 +9,7 @@ extern void split_cdb_XX_12(unsigned long long, u32 *, unsigned char *);
 extern void split_cdb_RW_12(unsigned long long, u32 *, unsigned char *, int);
 extern void split_cdb_XX_16(unsigned long long, u32 *, unsigned char *);
 extern void split_cdb_RW_16(unsigned long long, u32 *, unsigned char *, int);
+extern void split_cdb_XX_32(unsigned long long, u32 *, unsigned char *);
+extern void split_cdb_RW_32(unsigned long long, u32 *, unsigned char *, int);
 
 #endif /* TARGET_CORE_SCDB_H */
drivers/target/target_core_stgt.h +1 -1
index b82300c..8582776 100644
@@ -23,7 +23,7 @@ extern int linux_blockdevice_check(int, int);
 #include <linux/kobject.h>
 
 struct stgt_plugin_task {
-	unsigned char stgt_cdb[MAX_COMMAND_SIZE];
+	unsigned char stgt_cdb[TCM_MAX_COMMAND_SIZE];
 	unsigned char stgt_sense[SCSI_SENSE_BUFFERSIZE];
 	int	stgt_direction;
 	int	stgt_result;
drivers/target/target_core_transport.c +39 -5

86 lines not on this page · raw message

include/target/target_core_base.h +13 -0
index 97e9715..eb0a228 100644
@@ -17,6 +17,19 @@
 /* Maximum Number of LUNs per Target Portal Group */
 #define TRANSPORT_MAX_LUNS_PER_TPG		256
 /*
+ * By default we use 32-byte CDBs in TCM Core and subsystem plugin code.
+ *
+ * Note that both include/scsi/scsi_cmnd.h:MAX_COMMAND_SIZE and
+ * include/linux/blkdev.h:BLOCK_MAX_CDB as of v2.6.36-rc4 still use
+ * 16-byte CDBs by default and require an extra allocation for
+ * 32-byte CDBs to becasue of legacy issues.
+ *
+ * Within TCM Core there are no such legacy limitiations, so we go ahead
+ * use 32-byte CDBs by default and use include/scsi/scsi.h:scsi_command_size()
+ * within all TCM Core and subsystem plugin code.
+ */
+#define TCM_MAX_COMMAND_SIZE			32
+/*
  * From include/scsi/scsi_cmnd.h:SCSI_SENSE_BUFFERSIZE, currently
  * defined 96, but the real limit is 252 (or 260 including the header)
  */
Signature · 1 line
1.5.6.5
#1 lore raw
Quoted 63 lines · Nicholas A. Bellinger

From: Nicholas Bellinger <[email protected]>

This patch adds a '#define TCM_MAX_COMMAND_SIZE 32' and converts all inline TCM core and subsystem CDB size defines to use TCM_MAX_COMMAND_SIZE instead of the 16-byte MAX_COMMAND_SIZE. This includes the conversion of MAX_COMMAND_SIZE in certain places to use include/scsi/scsi.h:scsi_command_size() to properly extract the size of a received CDB based on opcode.

This patch also includes the conversion of the FILEIO, IBLOCK, PSCSI, RAMDISK and STGT subsystem modules to use TCM_MAX_COMMAND_SIZE for their default internal inline CDB size.

It also adds transport_lba_64_ext() and transport_get_sectors_32() callers into target_core_transport.c to be used for extracting a 64-bit logical block address and 32-bit transfer length (in block) from 32-byte CDBs. Finally this patch adds split_cdb_XX_32() and split_cdb_XX_32() for handling the generation of 32-byte CDBs for individual struct se_task.

Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/target_core_file.h      |    2 +-
 drivers/target/target_core_iblock.h    |    2 +-
 drivers/target/target_core_pscsi.c     |    2 +-
 drivers/target/target_core_pscsi.h     |    2 +-
 drivers/target/target_core_rd.h        |    2 +-
 drivers/target/target_core_scdb.c      |   38 +++++++++++++++++++++++++++
 drivers/target/target_core_scdb.h      |    2 +
 drivers/target/target_core_stgt.h      |    2 +-
 drivers/target/target_core_transport.c |   44 ++++++++++++++++++++++++++++---
 include/target/target_core_base.h      |   13 +++++++++
 10 files changed, 98 insertions(+), 11 deletions(-)
drivers/target/target_core_file.h +1 -1
index 7831034..35726f1 100644
@@ -26,7 +26,7 @@ extern int linux_blockdevice_check(int, int);
 
 struct fd_request {
 	/* SCSI CDB from iSCSI Command PDU */
-	unsigned char	fd_scsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char	fd_scsi_cdb[TCM_MAX_COMMAND_SIZE];
 	/* Data Direction */
 	u8		fd_data_direction;
 	/* Total length of request */
drivers/target/target_core_iblock.h +1 -1
index 76c39a9..3582aac 100644
@@ -12,7 +12,7 @@
 extern struct se_global *se_global;
 
 struct iblock_req {
-	unsigned char ib_scsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char ib_scsi_cdb[TCM_MAX_COMMAND_SIZE];
 	atomic_t ib_bio_cnt;
 	u32	ib_sg_count;
 	void	*ib_buf;
drivers/target/target_core_pscsi.c +0 -0
index 803853d..beca807 100644

drivers/target/target_core_pscsi.c static inline void pscsi_blk_init_request( · line 747

Quoted 6 lines
drivers/target/target_core_pscsi.c +1 -1
@@ -743,7 +743,7 @@ static inline void pscsi_blk_init_request(
 	 * Load the referenced struct se_task's SCSI CDB into
 	 * include/linux/blkdev.h:struct request->cmd
 	 */
-	pt->pscsi_req->cmd_len = COMMAND_SIZE(pt->pscsi_cdb[0]);
+	pt->pscsi_req->cmd_len = scsi_command_size(pt->pscsi_cdb);
 	memcpy(pt->pscsi_req->cmd, pt->pscsi_cdb, pt->pscsi_req->cmd_len);

Boom, only 16 bytes at req->cmd.

For larger than 16 bytes commands at the block/scsi-ml level you need to:

	request->cmd = my_buffer_valid_until_complete;
	request->cmd_len = scsi_command_size()

if above "my_buffer_valid_until_complet" is always available you can just do this regardless. If you need to dynamically allocate it (and deallocate) then you should do an if > MAX_COMMAND_SIZE

Quoted 7 lines
drivers/target/target_core_pscsi.c +0 -0
 	/*
 	 * Setup pointer for outgoing sense data.
drivers/target/target_core_pscsi.h +0 -0
index db09d6a..086e6f1 100644

drivers/target/target_core_pscsi.h extern int linux_blockdevice_check(int, int); · line 34

Quoted 8 lines
drivers/target/target_core_pscsi.h +1 -1
@@ -28,7 +28,7 @@ extern int linux_blockdevice_check(int, int);
 #include <linux/kobject.h>
 
 struct pscsi_plugin_task {
-	unsigned char pscsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char pscsi_cdb[TCM_MAX_COMMAND_SIZE];
 	unsigned char pscsi_sense[SCSI_SENSE_BUFFERSIZE];
 	int	pscsi_direction;
 	int	pscsi_result;

pscsi is when you directly Q to a scsi LLD via midlayer right? Note that most scsi LLDs don't support > 16 or 12 bytes. The regular scsi despatch will check this for you and return an error. How are these dispatched? blk_execute_rq* ?

Quoted 38 lines
drivers/target/target_core_rd.h +1 -1
index 8170b41..2008807 100644
@@ -31,7 +31,7 @@ void rd_module_exit(void);
 
 struct rd_request {
 	/* SCSI CDB from iSCSI Command PDU */
-	unsigned char	rd_scsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char	rd_scsi_cdb[TCM_MAX_COMMAND_SIZE];
 	/* Data Direction */
 	u8		rd_data_direction;
 	/* Total length of request */
drivers/target/target_core_scdb.c +10 -0
index 8bcfeaf..76a7de0 100644
@@ -28,6 +28,7 @@
 
 #include <linux/net.h>
 #include <linux/string.h>
+#include <scsi/scsi.h>
 
 #include <target/target_core_base.h>
 #include <target/target_core_transport.h>

@@ -147,3 +148,40 @@ void split_cdb_RW_16(
 	cdb[0] = (rw) ? 0x8a : 0x88;
 	split_cdb_XX_16(lba, sectors, &cdb[0]);
 }
+
+/*
+ *	split_cdb_XX_32():
+ *	
+ * 	64-bit LBA w/ 32-bit SECTORS such as READ_32, WRITE_32 and XDWRITEREAD_32
+ */
+void split_cdb_XX_32(
+	unsigned long long lba,
+	u32 *sectors,

drivers/target/target_core_scdb.c void split_cdb_RW_16( · line 169

Quoted 10 lines
drivers/target/target_core_scdb.c +10 -0
+	unsigned char *cdb)
+{
+	cdb[12] = (lba >> 56) & 0xff;
+	cdb[13] = (lba >> 48) & 0xff;
+	cdb[14] = (lba >> 40) & 0xff;
+	cdb[15] = (lba >> 32) & 0xff;
+	cdb[16] = (lba >> 24) & 0xff;
+	cdb[17] = (lba >> 16) & 0xff;
+	cdb[18] = (lba >> 8) & 0xff;
+	cdb[19] = lba & 0xff;

put_unaligned_be64

drivers/target/target_core_scdb.c void split_cdb_RW_16( · line 173

Quoted 4 lines
drivers/target/target_core_scdb.c +4 -0
+	cdb[28] = (*sectors >> 24) & 0xff;
+	cdb[29] = (*sectors >> 16) & 0xff;
+	cdb[30] = (*sectors >> 8) & 0xff;
+	cdb[31] = *sectors & 0xff;

put_unaligned_be32

Quoted 74 lines
drivers/target/target_core_scdb.c +14 -0
+}
+
+void split_cdb_RW_32(
+	unsigned long long lba,
+	u32 *sectors,
+	unsigned char *cdb,
+	int rw)
+{
+	/*
+	 * Set service action for VARIABLE_LENGTH_CMD
+	 */
+	cdb[9] = (rw) ? WRITE_32 : READ_32;
+	split_cdb_XX_32(lba, sectors, &cdb[0]);	
+}
drivers/target/target_core_scdb.h +2 -0
index 57688e2..1b0dc74 100644
@@ -9,5 +9,7 @@ extern void split_cdb_XX_12(unsigned long long, u32 *, unsigned char *);
 extern void split_cdb_RW_12(unsigned long long, u32 *, unsigned char *, int);
 extern void split_cdb_XX_16(unsigned long long, u32 *, unsigned char *);
 extern void split_cdb_RW_16(unsigned long long, u32 *, unsigned char *, int);
+extern void split_cdb_XX_32(unsigned long long, u32 *, unsigned char *);
+extern void split_cdb_RW_32(unsigned long long, u32 *, unsigned char *, int);
 
 #endif /* TARGET_CORE_SCDB_H */
drivers/target/target_core_stgt.h +1 -1
index b82300c..8582776 100644
@@ -23,7 +23,7 @@ extern int linux_blockdevice_check(int, int);
 #include <linux/kobject.h>
 
 struct stgt_plugin_task {
-	unsigned char stgt_cdb[MAX_COMMAND_SIZE];
+	unsigned char stgt_cdb[TCM_MAX_COMMAND_SIZE];
 	unsigned char stgt_sense[SCSI_SENSE_BUFFERSIZE];
 	int	stgt_direction;
 	int	stgt_result;
drivers/target/target_core_transport.c +6 -3
index 49d5946..a3016f7 100644
@@ -2636,7 +2636,8 @@ static int transport_process_control_sg_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
 
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 1;

@@ -2677,7 +2678,8 @@ static int transport_process_control_nonsg_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
 
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 0;

@@ -2711,7 +2713,8 @@ static int transport_process_non_data_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
 
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 0;

drivers/target/target_core_transport.c int transport_generic_allocate_tasks( · line 2908

Quoted 5 lines
drivers/target/target_core_transport.c +1 -1
@@ -2902,7 +2905,7 @@ int transport_generic_allocate_tasks(
 	/*
 	 * Copy the original CDB into T_TASK(cmd).
 	 */
-	memcpy(T_TASK(cmd)->t_task_cdb, cdb, MAX_COMMAND_SIZE);
+	memcpy(T_TASK(cmd)->t_task_cdb, cdb, scsi_command_size(cdb));

scsi_command_size can return up to 260 dependent on command. So you'll need a min() to make sure. I'd do a tsk_cdb_cpy() wrapper that does the proper min() for everyone. Later that wrapper can be made smarter for large commands up to 260.

Quoted 8 lines
drivers/target/target_core_transport.c +1 -0
 	/*
 	 * Check for SAM Task Attribute Emulation
 	 */

@@ -3829,6 +3832,19 @@ static inline unsigned long long transport_lba_64(unsigned char *cdb)
 	return ((unsigned long long)__v2) | (unsigned long long)__v1 << 32;
 }
 
+/*

drivers/target/target_core_transport.c static inline unsigned long long transport_lba_64(unsigned char *cdb) · line 3845

Quoted 10 lines
drivers/target/target_core_transport.c +10 -0
+ * For VARIABLE_LENGTH_CDB w/ 32 byte extended CDBs
+ */
+static inline unsigned long long transport_lba_64_ext(unsigned char *cdb)
+{
+	unsigned int __v1, __v2;
+
+	__v1 = (cdb[12] << 24) | (cdb[13] << 16) | (cdb[14] << 8) | cdb[15];
+	__v2 = (cdb[16] << 24) | (cdb[17] << 16) | (cdb[18] << 8) | cdb[19];
+
+	return ((unsigned long long)__v2) | (unsigned long long)__v1 << 32;

get_unaligned_be64(&cdb[12]);

I'd just drop this wrapper

Quoted 14 lines
drivers/target/target_core_transport.c +7 -0
+}
+
 /*	transport_set_supported_SAM_opcode():
  *
  *

@@ -4370,6 +4386,23 @@ type_disk:
 		    (cdb[12] << 8) + cdb[13];
 }
 
+/*
+ * Used for VARIABLE_LENGTH_CDB WRITE_32 and READ_32 variants
+ */
+static inline u32 transport_get_sectors_32(
+	unsigned char *cdb,

drivers/target/target_core_transport.c type_disk: · line 4403

Quoted 10 lines
drivers/target/target_core_transport.c +10 -0
+	struct se_cmd *cmd,
+	int *ret)
+{
+	/*
+	 * Assume TYPE_DISK for non struct se_device objects.
+	 * Use 32-bit sector value.
+	 */
+	return (u32)(cdb[28] << 24) + (cdb[29] << 16) +
+		    (cdb[30] << 8) + cdb[31];
+

get_unaligned_be32

Quoted 39 lines
drivers/target/target_core_transport.c +4 -1
+}
+
 static inline u32 transport_get_size(
 	u32 sectors,
 	unsigned char *cdb,

@@ -7496,7 +7529,8 @@ u32 transport_generic_get_cdb_count(
 
 		cdb = TRANSPORT(dev)->get_cdb(task);
 		if ((cdb)) {
-			memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+			memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+				scsi_command_size(T_TASK(cmd)->t_task_cdb));
 			cmd->transport_split_cdb(task->task_lba,
 					&task->task_sectors, cdb);
 		}
include/target/target_core_base.h +13 -0
index 97e9715..eb0a228 100644
@@ -17,6 +17,19 @@
 /* Maximum Number of LUNs per Target Portal Group */
 #define TRANSPORT_MAX_LUNS_PER_TPG		256
 /*
+ * By default we use 32-byte CDBs in TCM Core and subsystem plugin code.
+ *
+ * Note that both include/scsi/scsi_cmnd.h:MAX_COMMAND_SIZE and
+ * include/linux/blkdev.h:BLOCK_MAX_CDB as of v2.6.36-rc4 still use
+ * 16-byte CDBs by default and require an extra allocation for
+ * 32-byte CDBs to becasue of legacy issues.
+ *
+ * Within TCM Core there are no such legacy limitiations, so we go ahead
+ * use 32-byte CDBs by default and use include/scsi/scsi.h:scsi_command_size()
+ * within all TCM Core and subsystem plugin code.
+ */
+#define TCM_MAX_COMMAND_SIZE			32
+/*
  * From include/scsi/scsi_cmnd.h:SCSI_SENSE_BUFFERSIZE, currently
  * defined 96, but the real limit is 252 (or 260 including the header)
  */
author
#2 lore raw
Quoted 33 lines · Boaz Harrosh
Quoted 32 lines · Nicholas A. Bellinger

From: Nicholas Bellinger <[email protected]>

This patch adds a '#define TCM_MAX_COMMAND_SIZE 32' and converts all inline TCM core and subsystem CDB size defines to use TCM_MAX_COMMAND_SIZE instead of the 16-byte MAX_COMMAND_SIZE. This includes the conversion of MAX_COMMAND_SIZE in certain places to use include/scsi/scsi.h:scsi_command_size() to properly extract the size of a received CDB based on opcode.

This patch also includes the conversion of the FILEIO, IBLOCK, PSCSI, RAMDISK and STGT subsystem modules to use TCM_MAX_COMMAND_SIZE for their default internal inline CDB size.

It also adds transport_lba_64_ext() and transport_get_sectors_32() callers into target_core_transport.c to be used for extracting a 64-bit logical block address and 32-bit transfer length (in block) from 32-byte CDBs. Finally this patch adds split_cdb_XX_32() and split_cdb_XX_32() for handling the generation of 32-byte CDBs for individual struct se_task.

Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/target_core_file.h      |    2 +-
 drivers/target/target_core_iblock.h    |    2 +-
 drivers/target/target_core_pscsi.c     |    2 +-
 drivers/target/target_core_pscsi.h     |    2 +-
 drivers/target/target_core_rd.h        |    2 +-
 drivers/target/target_core_scdb.c      |   38 +++++++++++++++++++++++++++
 drivers/target/target_core_scdb.h      |    2 +
 drivers/target/target_core_stgt.h      |    2 +-
 drivers/target/target_core_transport.c |   44 ++++++++++++++++++++++++++++---
 include/target/target_core_base.h      |   13 +++++++++
 10 files changed, 98 insertions(+), 11 deletions(-)

<SNIP>

Quoted 14 lines

drivers/target/target_core_pscsi.c static inline void pscsi_blk_init_request( · line 747

Quoted 11 lines
drivers/target/target_core_pscsi.c +1 -1
index 803853d..beca807 100644
@@ -743,7 +743,7 @@ static inline void pscsi_blk_init_request(
 	 * Load the referenced struct se_task's SCSI CDB into
 	 * include/linux/blkdev.h:struct request->cmd
 	 */
-	pt->pscsi_req->cmd_len = COMMAND_SIZE(pt->pscsi_cdb[0]);
+	pt->pscsi_req->cmd_len = scsi_command_size(pt->pscsi_cdb);
 	memcpy(pt->pscsi_req->cmd, pt->pscsi_cdb, pt->pscsi_req->cmd_len);

Boom, only 16 bytes at req->cmd.

Quoted 9 lines

For larger than 16 bytes commands at the block/scsi-ml level you need to:

	request->cmd = my_buffer_valid_until_complete;
	request->cmd_len = scsi_command_size()

if above "my_buffer_valid_until_complet" is always available you can just do this regardless. If you need to dynamically allocate it (and deallocate) then you should do an if > MAX_COMMAND_SIZE

Hi Boaz,

I was pretty there was going to be some more work required on the TCM/PSCSI case for proper > 16 byte support, thanks for spotting this one.

So what I am thinking is that we can get rid of this memcpy() all together and just always use the per struct se_task allocated struct pscsi_plugin_task *pt->pscsi_cdb[] up to the new TCM_MAX_COMMAND_SIZE (32 bytes) for the request->cmd pointer.

Is there anything special that needs to be done in order to singal that struct request->cmd will be using *tp->pscsi_cdb[], and not struct request->__cmd[BLK_MAX_CMD]..?

Quoted 9 lines
Quoted 9 lines
drivers/target/target_core_pscsi.c +0 -0
 	/*
 	 * Setup pointer for outgoing sense data.
drivers/target/target_core_pscsi.h +0 -0
index db09d6a..086e6f1 100644
@@ -28,7 +28,7 @@ extern int linux_blockdevice_check(int, int);
 #include <linux/kobject.h>
Quoted 11 lines

drivers/target/target_core_pscsi.h extern int linux_blockdevice_check(int, int); · line 34

Quoted 6 lines
drivers/target/target_core_pscsi.h +1 -1
 struct pscsi_plugin_task {
-	unsigned char pscsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char pscsi_cdb[TCM_MAX_COMMAND_SIZE];
 	unsigned char pscsi_sense[SCSI_SENSE_BUFFERSIZE];
 	int	pscsi_direction;
 	int	pscsi_result;

pscsi is when you directly Q to a scsi LLD via midlayer right? Note that most scsi LLDs don't support > 16 or 12 bytes. The regular scsi despatch will check this for you and return an error. How are these dispatched? blk_execute_rq* ?

Correct on both points. So all control path CDBs with PSCSI are treated as a passthrough. The one special thing about pSCSI and struct request passthrough is wrt SCF_SCSI_DATA_SG_IO_CDB I/Os is that TCM Core will still handle the 1 -> N mapping of TCM struct se_task->task_sg[] across a single struct se_cmd->t_mem_list for received CDB sectors > underying HW max_sectors limitiations reported by pscsi_get_max_sectors().

Quoted 42 lines
Quoted 42 lines
drivers/target/target_core_rd.h +1 -1
index 8170b41..2008807 100644
@@ -31,7 +31,7 @@ void rd_module_exit(void);
 
 struct rd_request {
 	/* SCSI CDB from iSCSI Command PDU */
-	unsigned char	rd_scsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char	rd_scsi_cdb[TCM_MAX_COMMAND_SIZE];
 	/* Data Direction */
 	u8		rd_data_direction;
 	/* Total length of request */
drivers/target/target_core_scdb.c +14 -0
index 8bcfeaf..76a7de0 100644
@@ -28,6 +28,7 @@
 
 #include <linux/net.h>
 #include <linux/string.h>
+#include <scsi/scsi.h>
 
 #include <target/target_core_base.h>
 #include <target/target_core_transport.h>

@@ -147,3 +148,40 @@ void split_cdb_RW_16(
 	cdb[0] = (rw) ? 0x8a : 0x88;
 	split_cdb_XX_16(lba, sectors, &cdb[0]);
 }
+
+/*
+ *	split_cdb_XX_32():
+ *	
+ * 	64-bit LBA w/ 32-bit SECTORS such as READ_32, WRITE_32 and XDWRITEREAD_32
+ */
+void split_cdb_XX_32(
+	unsigned long long lba,
+	u32 *sectors,
+	unsigned char *cdb)
+{
+	cdb[12] = (lba >> 56) & 0xff;
+	cdb[13] = (lba >> 48) & 0xff;
Quoted 8 lines

drivers/target/target_core_scdb.c void split_cdb_RW_16( · line 169

Quoted 6 lines
drivers/target/target_core_scdb.c +6 -0
+	cdb[14] = (lba >> 40) & 0xff;
+	cdb[15] = (lba >> 32) & 0xff;
+	cdb[16] = (lba >> 24) & 0xff;
+	cdb[17] = (lba >> 16) & 0xff;
+	cdb[18] = (lba >> 8) & 0xff;
+	cdb[19] = lba & 0xff;

put_unaligned_be64

Ok, converting all functions these in target_core_scdb.c to use put_unaligned_be*().

Quoted 7 lines

drivers/target/target_core_scdb.c void split_cdb_RW_16( · line 173

Quoted 5 lines
drivers/target/target_core_scdb.c +4 -0
+	cdb[28] = (*sectors >> 24) & 0xff;
+	cdb[29] = (*sectors >> 16) & 0xff;
+	cdb[30] = (*sectors >> 8) & 0xff;
+	cdb[31] = *sectors & 0xff;

put_unaligned_be32

<nod>

Quoted 32 lines
Quoted 32 lines
drivers/target/target_core_transport.c +6 -3
index 49d5946..a3016f7 100644
@@ -2636,7 +2636,8 @@ static int transport_process_control_sg_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
 
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 1;

@@ -2677,7 +2678,8 @@ static int transport_process_control_nonsg_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
 
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 0;

@@ -2711,7 +2713,8 @@ static int transport_process_non_data_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
Quoted 13 lines

drivers/target/target_core_transport.c int transport_generic_allocate_tasks( · line 2908

Quoted 8 lines
drivers/target/target_core_transport.c +1 -1
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 0;

@@ -2902,7 +2905,7 @@ int transport_generic_allocate_tasks(
 	/*
 	 * Copy the original CDB into T_TASK(cmd).
 	 */
-	memcpy(T_TASK(cmd)->t_task_cdb, cdb, MAX_COMMAND_SIZE);
+	memcpy(T_TASK(cmd)->t_task_cdb, cdb, scsi_command_size(cdb));

scsi_command_size can return up to 260 dependent on command. So you'll need a min() to make sure. I'd do a tsk_cdb_cpy() wrapper that does the proper min() for everyone. Later that wrapper can be made smarter for large commands up to 260.

Hmmm actually, I am wondering if we need a method for the individual backstore to report the maximum supported CDB size on a TCM subsystem plugin basis, say via a caller include/target/target_core_transport.h:
struct se_subsystem_api, and fail the extended CDB at TCM level for the pSCSI cases where struct scsi_host->max_cmd_len < scsi_command_size().

Also wrt to 260 byte CDBs for TCM/pSCSI and really everything received greater than the new default 32 for TCM_MAX_COMMAND_SIZE, this will require the extra allocation by TCM Core at both the T_TASK(cmd)->t_task_cdb[] as well as allowing pSCSI to handle this case.

I will look at implementing this soon, but I still need a way to test this code with TCM/PSCSI and scsi_debug.. Any ideas on where to look for doing this with > 32 byte CDBs..?

Quoted 22 lines
Quoted 9 lines
drivers/target/target_core_transport.c +1 -0
 	/*
 	 * Check for SAM Task Attribute Emulation
 	 */

@@ -3829,6 +3832,19 @@ static inline unsigned long long transport_lba_64(unsigned char *cdb)
 	return ((unsigned long long)__v2) | (unsigned long long)__v1 << 32;
 }
 
+/*

drivers/target/target_core_transport.c static inline unsigned long long transport_lba_64(unsigned char *cdb) · line 3845

Quoted 10 lines
drivers/target/target_core_transport.c +10 -0
+ * For VARIABLE_LENGTH_CDB w/ 32 byte extended CDBs
+ */
+static inline unsigned long long transport_lba_64_ext(unsigned char *cdb)
+{
+	unsigned int __v1, __v2;
+
+	__v1 = (cdb[12] << 24) | (cdb[13] << 16) | (cdb[14] << 8) | cdb[15];
+	__v2 = (cdb[16] << 24) | (cdb[17] << 16) | (cdb[18] << 8) | cdb[19];
+
+	return ((unsigned long long)__v2) | (unsigned long long)__v1 << 32;

get_unaligned_be64(&cdb[12]);

Quoted 1 line

I'd just drop this wrapper

Good point, I believe this is only used in a single location. transport_generic_allocate_tasks().

Quoted 19 lines
Quoted 19 lines
drivers/target/target_core_transport.c +11 -0
+}
+
 /*	transport_set_supported_SAM_opcode():
  *
  *

@@ -4370,6 +4386,23 @@ type_disk:
 		    (cdb[12] << 8) + cdb[13];
 }
 
+/*
+ * Used for VARIABLE_LENGTH_CDB WRITE_32 and READ_32 variants
+ */
+static inline u32 transport_get_sectors_32(
+	unsigned char *cdb,
+	struct se_cmd *cmd,
+	int *ret)
+{
+	/*
Quoted 8 lines

drivers/target/target_core_transport.c type_disk: · line 4403

Quoted 6 lines
drivers/target/target_core_transport.c +6 -0
+	 * Assume TYPE_DISK for non struct se_device objects.
+	 * Use 32-bit sector value.
+	 */
+	return (u32)(cdb[28] << 24) + (cdb[29] << 16) +
+		    (cdb[30] << 8) + cdb[31];
+

get_unaligned_be32

Ok, there are a few more of these that can be converted to get_unaligned_be*() and friends.

Thanks for your comments Boaz!

--nab

#3 lore raw
Quoted 67 lines · Nicholas A. Bellinger
Quoted 33 lines · Boaz Harrosh
Quoted 32 lines · Nicholas A. Bellinger

From: Nicholas Bellinger <[email protected]>

This patch adds a '#define TCM_MAX_COMMAND_SIZE 32' and converts all inline TCM core and subsystem CDB size defines to use TCM_MAX_COMMAND_SIZE instead of the 16-byte MAX_COMMAND_SIZE. This includes the conversion of MAX_COMMAND_SIZE in certain places to use include/scsi/scsi.h:scsi_command_size() to properly extract the size of a received CDB based on opcode.

This patch also includes the conversion of the FILEIO, IBLOCK, PSCSI, RAMDISK and STGT subsystem modules to use TCM_MAX_COMMAND_SIZE for their default internal inline CDB size.

It also adds transport_lba_64_ext() and transport_get_sectors_32() callers into target_core_transport.c to be used for extracting a 64-bit logical block address and 32-bit transfer length (in block) from 32-byte CDBs. Finally this patch adds split_cdb_XX_32() and split_cdb_XX_32() for handling the generation of 32-byte CDBs for individual struct se_task.

Signed-off-by Nicholas A. Bellinger <[email protected]>
---
 drivers/target/target_core_file.h      |    2 +-
 drivers/target/target_core_iblock.h    |    2 +-
 drivers/target/target_core_pscsi.c     |    2 +-
 drivers/target/target_core_pscsi.h     |    2 +-
 drivers/target/target_core_rd.h        |    2 +-
 drivers/target/target_core_scdb.c      |   38 +++++++++++++++++++++++++++
 drivers/target/target_core_scdb.h      |    2 +
 drivers/target/target_core_stgt.h      |    2 +-
 drivers/target/target_core_transport.c |   44 ++++++++++++++++++++++++++++---
 include/target/target_core_base.h      |   13 +++++++++
 10 files changed, 98 insertions(+), 11 deletions(-)

<SNIP>

Quoted 14 lines

drivers/target/target_core_pscsi.c static inline void pscsi_blk_init_request( · line 747

Quoted 11 lines
drivers/target/target_core_pscsi.c +1 -1
index 803853d..beca807 100644
@@ -743,7 +743,7 @@ static inline void pscsi_blk_init_request(
 	 * Load the referenced struct se_task's SCSI CDB into
 	 * include/linux/blkdev.h:struct request->cmd
 	 */
-	pt->pscsi_req->cmd_len = COMMAND_SIZE(pt->pscsi_cdb[0]);
+	pt->pscsi_req->cmd_len = scsi_command_size(pt->pscsi_cdb);
 	memcpy(pt->pscsi_req->cmd, pt->pscsi_cdb, pt->pscsi_req->cmd_len);

Boom, only 16 bytes at req->cmd.

Quoted 9 lines

For larger than 16 bytes commands at the block/scsi-ml level you need to:

	request->cmd = my_buffer_valid_until_complete;
	request->cmd_len = scsi_command_size()

if above "my_buffer_valid_until_complet" is always available you can just do this regardless. If you need to dynamically allocate it (and deallocate) then you should do an if > MAX_COMMAND_SIZE

Hi Boaz,

I was pretty there was going to be some more work required on the TCM/PSCSI case for proper > 16 byte support, thanks for spotting this one.

Quoted 8 lines

So what I am thinking is that we can get rid of this memcpy() all together and just always use the per struct se_task allocated struct pscsi_plugin_task *pt->pscsi_cdb[] up to the new TCM_MAX_COMMAND_SIZE (32 bytes) for the request->cmd pointer.

Is there anything special that needs to be done in order to singal that struct request->cmd will be using *tp->pscsi_cdb[], and not struct request->__cmd[BLK_MAX_CMD]..?

No that's perfect.
The tricky part in other places is to keep it allocated through the life cycle of the request. (And overriding the completion for de-allocation). But here we have no problems, the governing pscsi_plugin_task is available until after the request is complete, right?

Quoted 145 lines
Quoted 10 lines
Quoted 10 lines
drivers/target/target_core_pscsi.c +0 -0
 	/*
 	 * Setup pointer for outgoing sense data.
drivers/target/target_core_pscsi.h +0 -0
index db09d6a..086e6f1 100644
@@ -28,7 +28,7 @@ extern int linux_blockdevice_check(int, int);
 #include <linux/kobject.h>
Quoted 11 lines

drivers/target/target_core_pscsi.h extern int linux_blockdevice_check(int, int); · line 34

Quoted 6 lines
drivers/target/target_core_pscsi.h +1 -1
 struct pscsi_plugin_task {
-	unsigned char pscsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char pscsi_cdb[TCM_MAX_COMMAND_SIZE];
 	unsigned char pscsi_sense[SCSI_SENSE_BUFFERSIZE];
 	int	pscsi_direction;
 	int	pscsi_result;

pscsi is when you directly Q to a scsi LLD via midlayer right? Note that most scsi LLDs don't support > 16 or 12 bytes. The regular scsi despatch will check this for you and return an error. How are these dispatched? blk_execute_rq* ?

Correct on both points. So all control path CDBs with PSCSI are treated as a passthrough. The one special thing about pSCSI and struct request passthrough is wrt SCF_SCSI_DATA_SG_IO_CDB I/Os is that TCM Core will still handle the 1 -> N mapping of TCM struct se_task->task_sg[] across a single struct se_cmd->t_mem_list for received CDB sectors > underying HW max_sectors limitiations reported by pscsi_get_max_sectors().

Quoted 42 lines
Quoted 42 lines
drivers/target/target_core_rd.h +1 -1
index 8170b41..2008807 100644
@@ -31,7 +31,7 @@ void rd_module_exit(void);
 
 struct rd_request {
 	/* SCSI CDB from iSCSI Command PDU */
-	unsigned char	rd_scsi_cdb[MAX_COMMAND_SIZE];
+	unsigned char	rd_scsi_cdb[TCM_MAX_COMMAND_SIZE];
 	/* Data Direction */
 	u8		rd_data_direction;
 	/* Total length of request */
drivers/target/target_core_scdb.c +14 -0
index 8bcfeaf..76a7de0 100644
@@ -28,6 +28,7 @@
 
 #include <linux/net.h>
 #include <linux/string.h>
+#include <scsi/scsi.h>
 
 #include <target/target_core_base.h>
 #include <target/target_core_transport.h>

@@ -147,3 +148,40 @@ void split_cdb_RW_16(
 	cdb[0] = (rw) ? 0x8a : 0x88;
 	split_cdb_XX_16(lba, sectors, &cdb[0]);
 }
+
+/*
+ *	split_cdb_XX_32():
+ *	
+ * 	64-bit LBA w/ 32-bit SECTORS such as READ_32, WRITE_32 and XDWRITEREAD_32
+ */
+void split_cdb_XX_32(
+	unsigned long long lba,
+	u32 *sectors,
+	unsigned char *cdb)
+{
+	cdb[12] = (lba >> 56) & 0xff;
+	cdb[13] = (lba >> 48) & 0xff;
Quoted 8 lines

drivers/target/target_core_scdb.c void split_cdb_RW_16( · line 169

Quoted 6 lines
drivers/target/target_core_scdb.c +6 -0
+	cdb[14] = (lba >> 40) & 0xff;
+	cdb[15] = (lba >> 32) & 0xff;
+	cdb[16] = (lba >> 24) & 0xff;
+	cdb[17] = (lba >> 16) & 0xff;
+	cdb[18] = (lba >> 8) & 0xff;
+	cdb[19] = lba & 0xff;

put_unaligned_be64

Ok, converting all functions these in target_core_scdb.c to use put_unaligned_be*().

Quoted 7 lines

drivers/target/target_core_scdb.c void split_cdb_RW_16( · line 173

Quoted 5 lines
drivers/target/target_core_scdb.c +4 -0
+	cdb[28] = (*sectors >> 24) & 0xff;
+	cdb[29] = (*sectors >> 16) & 0xff;
+	cdb[30] = (*sectors >> 8) & 0xff;
+	cdb[31] = *sectors & 0xff;

put_unaligned_be32

<nod>

Quoted 32 lines
Quoted 32 lines
drivers/target/target_core_transport.c +6 -3
index 49d5946..a3016f7 100644
@@ -2636,7 +2636,8 @@ static int transport_process_control_sg_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
 
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 1;

@@ -2677,7 +2678,8 @@ static int transport_process_control_nonsg_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
 
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 0;

@@ -2711,7 +2713,8 @@ static int transport_process_non_data_transform(
 
 	cdb = TRANSPORT(dev)->get_cdb(task);
 	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb, MAX_COMMAND_SIZE);
+		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
+			scsi_command_size(T_TASK(cmd)->t_task_cdb));
Quoted 13 lines

drivers/target/target_core_transport.c int transport_generic_allocate_tasks( · line 2908

Quoted 8 lines
drivers/target/target_core_transport.c +1 -1
 	task->task_size = cmd->data_length;
 	task->task_sg_num = 0;

@@ -2902,7 +2905,7 @@ int transport_generic_allocate_tasks(
 	/*
 	 * Copy the original CDB into T_TASK(cmd).
 	 */
-	memcpy(T_TASK(cmd)->t_task_cdb, cdb, MAX_COMMAND_SIZE);
+	memcpy(T_TASK(cmd)->t_task_cdb, cdb, scsi_command_size(cdb));

scsi_command_size can return up to 260 dependent on command. So you'll need a min() to make sure. I'd do a tsk_cdb_cpy() wrapper that does the proper min() for everyone. Later that wrapper can be made smarter for large commands up to 260.

Hmmm actually, I am wondering if we need a method for the individual backstore to report the maximum supported CDB size on a TCM subsystem plugin basis, say via a caller include/target/target_core_transport.h:
struct se_subsystem_api, and fail the extended CDB at TCM level for the pSCSI cases where struct scsi_host->max_cmd_len < scsi_command_size().

Quoted 5 lines

Also wrt to 260 byte CDBs for TCM/pSCSI and really everything received greater than the new default 32 for TCM_MAX_COMMAND_SIZE, this will require the extra allocation by TCM Core at both the T_TASK(cmd)->t_task_cdb[] as well as allowing pSCSI to handle this case.

For me, (I'm not sure how hard it is. I still did not look, you know ;-)) I'd drop the allocation all together and just use a pointer and a size. So for instance pSCSI can just pass along it's pointer and size, all the way to the end and back. In something like iscsi the fixed 16 bytes can just point to the received header, and with an extended cdb spill over a buffer should be allocated, then de-allocated once command-response is sent.
So basically allocated once in transport, passed around as pointer. (BTW Same for sense buffer)

And by the way, I'd let the target reject un-known commands. It's a big boy and it's easiest to add new code. No other places to change. (And some fun things can be done, like target filtering). Passing the "size" test says nothing, the actual command needs implementing. Just make sure Error handling is solid. (The target already as a default: return error case)

Quoted 4 lines

I will look at implementing this soon, but I still need a way to test this code with TCM/PSCSI and scsi_debug.. Any ideas on where to look for doing this with > 32 byte CDBs..?

That's easy. You have decided that TCM_MAX_COMMAND_SIZE == 32 and anything bigger needs allocation. So for the duration of the test set TCM_MAX_COMMAND_SIZE == 16 and see how these 32s get allocated.

or

How ready is target_core_stgt.h can you pass commands to user-space and back. We can send OSD commands.

or

What if we use pscsi_plugin_task to send commands to an iscsi-initiator device that's an OSD. A bit stupid as an admin point of view, but a grate pass through exercise.

[osd-initiator] --net--> [(iscsi_tcm) - (pscsi_plugin_task) - (iscsi-initiator)] \
--net--> [osd-target]

The [osd-initiator] and [osd-target] parts are easy.

Boaz

Quoted 62 lines
Quoted 22 lines
Quoted 9 lines
drivers/target/target_core_transport.c +1 -0
 	/*
 	 * Check for SAM Task Attribute Emulation
 	 */

@@ -3829,6 +3832,19 @@ static inline unsigned long long transport_lba_64(unsigned char *cdb)
 	return ((unsigned long long)__v2) | (unsigned long long)__v1 << 32;
 }
 
+/*

drivers/target/target_core_transport.c static inline unsigned long long transport_lba_64(unsigned char *cdb) · line 3845

Quoted 10 lines
drivers/target/target_core_transport.c +10 -0
+ * For VARIABLE_LENGTH_CDB w/ 32 byte extended CDBs
+ */
+static inline unsigned long long transport_lba_64_ext(unsigned char *cdb)
+{
+	unsigned int __v1, __v2;
+
+	__v1 = (cdb[12] << 24) | (cdb[13] << 16) | (cdb[14] << 8) | cdb[15];
+	__v2 = (cdb[16] << 24) | (cdb[17] << 16) | (cdb[18] << 8) | cdb[19];
+
+	return ((unsigned long long)__v2) | (unsigned long long)__v1 << 32;

get_unaligned_be64(&cdb[12]);

Quoted 1 line

I'd just drop this wrapper

Good point, I believe this is only used in a single location. transport_generic_allocate_tasks().

Quoted 19 lines
Quoted 19 lines
drivers/target/target_core_transport.c +11 -0
+}
+
 /*	transport_set_supported_SAM_opcode():
  *
  *

@@ -4370,6 +4386,23 @@ type_disk:
 		    (cdb[12] << 8) + cdb[13];
 }
 
+/*
+ * Used for VARIABLE_LENGTH_CDB WRITE_32 and READ_32 variants
+ */
+static inline u32 transport_get_sectors_32(
+	unsigned char *cdb,
+	struct se_cmd *cmd,
+	int *ret)
+{
+	/*
Quoted 8 lines

drivers/target/target_core_transport.c type_disk: · line 4403

Quoted 6 lines
drivers/target/target_core_transport.c +6 -0
+	 * Assume TYPE_DISK for non struct se_device objects.
+	 * Use 32-bit sector value.
+	 */
+	return (u32)(cdb[28] << 24) + (cdb[29] << 16) +
+		    (cdb[30] << 8) + cdb[31];
+

get_unaligned_be32

Ok, there are a few more of these that can be converted to get_unaligned_be*() and friends.

Thanks for your comments Boaz!

--nab