From: Nicholas Bellinger <[email protected]>
Hi MKP & SCSI folks,
This series contains initial support for target mode DIF Type1+Type3
emulation within target core, RAMDISK_MCP device backend, and tcm_loop
fabric driver.
DIF emulation is enabled via a new 'pi_prot_type' device attribute
within configfs, which is set after initial device configuration and
before target fabric LUN export occurs.
The DIF read/write verify emulation has been made generic enough so
it can be used by other backend drivers (eg: FILEIO), as well as
DIF v2 in the near future. Also note that the majority of the logic
has been groked from existing scsi_debug.c code.
The current plan is to enable basic support for emulated backends with
tcm_loop for v3.14 code, and then move onto IBLOCK backend support
(that requires BLOCK layer changes), and then onto other more complex
fabric driver support + hardware offloads (iser-target that Sagi + Or
are currently working on) and DIF support into KVM guest using
virtio-scsi + vhost-scsi.
Here is a quick snippet of the code in action with v3.13-rc3:
[ 846.774085] scsi8 : TCM_Loopback
[ 846.778044] scsi 8:0:1:0: Direct-Access LIO-ORG RAMDISK-MCP 4.0 PQ: 0 ANSI: 5
[ 846.780160] init_tag_map: adjusted depth to 256
[ 846.781309] init_tag_map: adjusted depth to 256
[ 846.782424] init_tag_map: adjusted depth to 256
[ 846.783685] sd 8:0:1:0: Attached scsi generic sg1 type 0
[ 846.783923] sd 8:0:1:0: [sda] Enabling DIF Type 1 protection
[ 846.783929] sd 8:0:1:0: [sda] 2097152 512-byte logical blocks: (1.07 GB/1.00 GiB)
[ 846.784111] sd 8:0:1:0: [sda] Write Protect is off
[ 846.784114] sd 8:0:1:0: [sda] Mode Sense: 43 00 00 08
[ 846.784157] sd 8:0:1:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
[ 846.785357] sda: unknown partition table
[ 846.795464] sd 8:0:1:0: [sda] Enabling DIX T10-DIF-TYPE1-CRC protection
[ 846.797278] sd 8:0:1:0: [sda] DIF application tag size 2
[ 846.798951] sd 8:0:1:0: [sda] Attached SCSI disk
<First 4k WRITE_10 performing Host Generate + Target Verify>
[ 901.477786] Entering sd_dif_type1_generate >>>>>>>>>>>>>>>
[ 901.477809] SD TYPE1 Generate: sector: 0 guard_tag: 0x5fd1 app_tag: 0x0000 ref_tag: 0
[ 901.477823] SD TYPE1 Generate: sector: 1 guard_tag: 0x41de app_tag: 0x0000 ref_tag: 1
[ 901.477835] SD TYPE1 Generate: sector: 2 guard_tag: 0x27b1 app_tag: 0x0000 ref_tag: 2
[ 901.477848] SD TYPE1 Generate: sector: 3 guard_tag: 0xeb4c app_tag: 0x0000 ref_tag: 3
[ 901.477860] SD TYPE1 Generate: sector: 4 guard_tag: 0x5d81 app_tag: 0x0000 ref_tag: 4
[ 901.477872] SD TYPE1 Generate: sector: 5 guard_tag: 0xda61 app_tag: 0x0000 ref_tag: 5
[ 901.477884] SD TYPE1 Generate: sector: 6 guard_tag: 0x80df app_tag: 0x0000 ref_tag: 6
[ 901.477896] SD TYPE1 Generate: sector: 7 guard_tag: 0x5ede app_tag: 0x0000 ref_tag: 7
[ 901.477940] sd_prep_fn: CDB: 0x2a blk_integrity_rq(rq): 1
[ 901.481576] DIF WRITE sector: 0 guard_tag: 0x5fd1 app_tag: 0x0000 ref_tag: 0
[ 901.481591] DIF WRITE sector: 1 guard_tag: 0x41de app_tag: 0x0000 ref_tag: 1
[ 901.481615] DIF WRITE sector: 2 guard_tag: 0x27b1 app_tag: 0x0000 ref_tag: 2
[ 901.481637] DIF WRITE sector: 3 guard_tag: 0xeb4c app_tag: 0x0000 ref_tag: 3
[ 901.481649] DIF WRITE sector: 4 guard_tag: 0x5d81 app_tag: 0x0000 ref_tag: 4
[ 901.481661] DIF WRITE sector: 5 guard_tag: 0xda61 app_tag: 0x0000 ref_tag: 5
[ 901.481673] DIF WRITE sector: 6 guard_tag: 0x80df app_tag: 0x0000 ref_tag: 6
[ 901.481685] DIF WRITE sector: 7 guard_tag: 0x5ede app_tag: 0x0000 ref_tag: 7
<Subsequent 4k READ_10 performing Target Verify + Host Verify>
[ 901.522883] sd_prep_fn: CDB: 0x28 blk_integrity_rq(rq): 1
[ 901.528637] DIF READ sector: 0 guard_tag: 0x5fd1 app_tag: 0x0000 ref_tag: 0
[ 901.528655] DIF READ sector: 1 guard_tag: 0x41de app_tag: 0x0000 ref_tag: 1
[ 901.528667] DIF READ sector: 2 guard_tag: 0x27b1 app_tag: 0x0000 ref_tag: 2
[ 901.528679] DIF READ sector: 3 guard_tag: 0xeb4c app_tag: 0x0000 ref_tag: 3
[ 901.528691] DIF READ sector: 4 guard_tag: 0x5d81 app_tag: 0x0000 ref_tag: 4
[ 901.528703] DIF READ sector: 5 guard_tag: 0xda61 app_tag: 0x0000 ref_tag: 5
[ 901.528715] DIF READ sector: 6 guard_tag: 0x80df app_tag: 0x0000 ref_tag: 6
[ 901.528727] DIF READ sector: 7 guard_tag: 0x5ede app_tag: 0x0000 ref_tag: 7
[ 901.528788] Entering bio_integrity_verify sector: 0 >>>>>>>>>>>>>>>>>>>>>>>
[ 901.535807] bio_integrity_verify: bio->bi_idx: 1 bio->bi_vcnt: 1
[ 901.538222] SD TYPE1 Verify sector: 0 guard_tag: 0x5fd1 app_tag: 0x0000 ref_tag: 0
[ 901.538226] SD TYPE1 Verify sector: 1 guard_tag: 0x41de app_tag: 0x0000 ref_tag: 1
[ 901.538230] SD TYPE1 Verify sector: 2 guard_tag: 0x27b1 app_tag: 0x0000 ref_tag: 2
[ 901.538234] SD TYPE1 Verify sector: 3 guard_tag: 0xeb4c app_tag: 0x0000 ref_tag: 3
[ 901.538238] SD TYPE1 Verify sector: 4 guard_tag: 0x5d81 app_tag: 0x0000 ref_tag: 4
[ 901.538241] SD TYPE1 Verify sector: 5 guard_tag: 0xda61 app_tag: 0x0000 ref_tag: 5
[ 901.538245] SD TYPE1 Verify sector: 6 guard_tag: 0x80df app_tag: 0x0000 ref_tag: 6
[ 901.538248] SD TYPE1 Verify sector: 7 guard_tag: 0x5ede app_tag: 0x0000 ref_tag: 7
Please review as v3.14-rc1 code.
Thank you,
--nab
Nicholas Bellinger (14):
target: Add DIF related base definitions
target: Add DIF CHECK_CONDITION ASC/ASCQ exception cases
target/sbc: Add sbc_check_prot + update sbc_parse_cdb for DIF
target/sbc: Add DIF TYPE1+TYPE3 read/write verify emulation
target/spc: Add protection bit to standard INQUIRY output
target/spc: Add protection related bits to INQUIRY EVPD=0x86
target/sbc: Add P_TYPE + PROT_EN bits to READ_CAPACITY_16
target/spc: Expose ATO bit in control mode page
target/configfs: Expose protection device attributes
target: Add protection SGLs to target_submit_cmd_map_sgls
target/rd: Refactor rd_build_device_space + rd_release_device_space
target/rd: Add support for protection SGL setup + release
target/rd: Add DIF protection into rd_execute_rw
tcm_loop: Enable DIF/DIX modes in SCSI host LLD
drivers/target/loopback/tcm_loop.c | 12 +-
drivers/target/target_core_configfs.c | 12 ++
drivers/target/target_core_device.c | 65 ++++++++
drivers/target/target_core_internal.h | 2 +
drivers/target/target_core_rd.c | 264 ++++++++++++++++++++++++++------
drivers/target/target_core_rd.h | 4 +
drivers/target/target_core_sbc.c | 224 +++++++++++++++++++++++++++
drivers/target/target_core_spc.c | 27 ++++
drivers/target/target_core_transport.c | 46 +++++-
drivers/vhost/scsi.c | 2 +-
include/target/target_core_backend.h | 6 +
include/target/target_core_base.h | 62 ++++++++
include/target/target_core_fabric.h | 3 +-
13 files changed, 678 insertions(+), 51 deletions(-)
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch adds support to target_submit_cmd_map_sgls() for
accepting 'sgl_prot' + 'sgl_prot_count' parameters for
DIF protection information.
Note the passed parameters are stored at se_cmd->t_prot_sg
and se_cmd->t_prot_nents respectively.
Also, update tcm_loop and vhost-scsi fabrics usage of
target_submit_cmd_map_sgls() to take into account the
new parameters.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/loopback/tcm_loop.c | 2 +-
drivers/target/target_core_transport.c | 16 ++++++++++++++--
drivers/vhost/scsi.c | 2 +-
include/target/target_core_fabric.h | 3 ++-
4 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 1b41e67..c6f1427 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -217,7 +217,7 @@ static void tcm_loop_submission_work(struct work_struct *work)
scsi_bufflen(sc), tcm_loop_sam_attr(sc),
sc->sc_data_direction, 0,
scsi_sglist(sc), scsi_sg_count(sc),
- sgl_bidi, sgl_bidi_count);
+ sgl_bidi, sgl_bidi_count, NULL, 0);
if (rc < 0) {
set_host_byte(sc, DID_NO_CONNECT);
goto out_done;
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 707ee17..65b42b4 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1284,6 +1284,8 @@ transport_generic_map_mem_to_cmd(struct se_cmd *cmd, struct scatterlist *sgl,
* @sgl_count: scatterlist count for unidirectional mapping
* @sgl_bidi: struct scatterlist memory for bidirectional READ mapping
* @sgl_bidi_count: scatterlist count for bidirectional READ mapping
+ * @sgl_prot: struct scatterlist memory protection information
+ * @sgl_prot_count: scatterlist count for protection information
*
* Returns non zero to signal active I/O shutdown failure. All other
* setup exceptions will be returned as a SCSI CHECK_CONDITION response,
@@ -1296,7 +1298,8 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
u32 data_length, int task_attr, int data_dir, int flags,
struct scatterlist *sgl, u32 sgl_count,
- struct scatterlist *sgl_bidi, u32 sgl_bidi_count)
+ struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
+ struct scatterlist *sgl_prot, u32 sgl_prot_count)
{
struct se_portal_group *se_tpg;
sense_reason_t rc;
@@ -1338,6 +1341,14 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
target_put_sess_cmd(se_sess, se_cmd);
return 0;
}
+ /*
+ * Save pointers for SGLs containing protection information,
+ * if present.
+ */
+ if (sgl_prot_count) {
+ se_cmd->t_prot_sg = sgl_prot;
+ se_cmd->t_prot_nents = sgl_prot_count;
+ }
rc = target_setup_cmd_from_cdb(se_cmd, cdb);
if (rc != 0) {
@@ -1380,6 +1391,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
return 0;
}
}
+
/*
* Check if we need to delay processing because of ALUA
* Active/NonOptimized primary access state..
@@ -1419,7 +1431,7 @@ int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
{
return target_submit_cmd_map_sgls(se_cmd, se_sess, cdb, sense,
unpacked_lun, data_length, task_attr, data_dir,
- flags, NULL, 0, NULL, 0);
+ flags, NULL, 0, NULL, 0, NULL, 0);
}
EXPORT_SYMBOL(target_submit_cmd);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index f175629..84488a8 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -889,7 +889,7 @@ static void tcm_vhost_submission_work(struct work_struct *work)
cmd->tvc_lun, cmd->tvc_exp_data_len,
cmd->tvc_task_attr, cmd->tvc_data_direction,
TARGET_SCF_ACK_KREF, sg_ptr, cmd->tvc_sgl_count,
- sg_bidi_ptr, sg_no_bidi);
+ sg_bidi_ptr, sg_no_bidi, NULL, 0);
if (rc < 0) {
transport_send_check_condition_and_sense(se_cmd,
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 4cf4fda..0218d68 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -105,7 +105,8 @@ sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u32);
sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
int target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
unsigned char *, unsigned char *, u32, u32, int, int, int,
- struct scatterlist *, u32, struct scatterlist *, u32);
+ struct scatterlist *, u32, struct scatterlist *, u32,
+ struct scatterlist *, u32);
int target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
unsigned char *, u32, u32, int, int, int);
int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch updates spc_emulate_evpd_86() (extended INQUIRY) to
report GRD_CHK (Guard Check) and REF_CHK (Reference Check) bits
when DIF emulation is enabled by the backend device.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_spc.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 6b06f01..d99eb95 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -441,6 +441,15 @@ spc_emulate_evpd_86(struct se_cmd *cmd, unsigned char *buf)
struct se_device *dev = cmd->se_dev;
buf[3] = 0x3c;
+ /*
+ * Set GRD_CHK + REF_CHK for TYPE1 protection, or GRD_CHK
+ * only for TYPE3 protection.
+ */
+ if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE1_PROT)
+ buf[4] = 0x5;
+ else if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE3_PROT)
+ buf[4] = 0x4;
+
/* Set HEADSUP, ORDSUP, SIMPSUP */
buf[5] = 0x07;
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch adds DIF related definitions to target_core_base.h
that includes enums for target_prot_op + target_prot_type +
target_prot_version + target_guard_type + target_pi_error.
Also included is struct se_dif_v1_tuple, along with changes
to struct se_cmd, struct se_dev_attrib, and struct se_device.
Also, add new se_subsystem_api->[init,free]_prot() callers
used by target core code to setup backend specific protection
information after the device has been configured.
Enums taken from Sagi Grimberg's original patch.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
target: more defs
Signed-off-by: Nicholas Bellinger <[email protected]>
---
include/target/target_core_backend.h | 2 ++
include/target/target_core_base.h | 59 ++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 39e0114..930f30d 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -41,6 +41,8 @@ struct se_subsystem_api {
unsigned int (*get_io_opt)(struct se_device *);
unsigned char *(*get_sense_buffer)(struct se_cmd *);
bool (*get_write_cache)(struct se_device *);
+ int (*init_prot)(struct se_device *);
+ void (*free_prot)(struct se_device *);
};
struct sbc_ops {
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 45412a6..15f402c 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -166,6 +166,7 @@ enum se_cmd_flags_table {
SCF_COMPARE_AND_WRITE = 0x00080000,
SCF_COMPARE_AND_WRITE_POST = 0x00100000,
SCF_CMD_XCOPY_PASSTHROUGH = 0x00200000,
+ SCF_PROT = 0x00400000,
};
/* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
@@ -414,6 +415,45 @@ struct se_tmr_req {
struct list_head tmr_list;
};
+enum target_prot_op {
+ TARGET_PROT_NORMAL,
+ TARGET_PROT_READ_INSERT,
+ TARGET_PROT_WRITE_INSERT,
+ TARGET_PROT_READ_STRIP,
+ TARGET_PROT_WRITE_STRIP,
+ TARGET_PROT_READ_PASS,
+ TARGET_PROT_WRITE_PASS,
+};
+
+enum target_prot_type {
+ TARGET_DIF_TYPE0_PROT,
+ TARGET_DIF_TYPE1_PROT,
+ TARGET_DIF_TYPE2_PROT,
+ TARGET_DIF_TYPE3_PROT,
+};
+
+enum target_prot_version {
+ TARGET_DIF_V1 = 1,
+ TARGET_DIF_V2 = 2,
+};
+
+enum target_guard_type {
+ TARGET_DIX_GUARD_CRC = 1,
+ TARGET_DIX_GUARD_IP = 2,
+};
+
+enum target_pi_error {
+ TARGET_GUARD_CHECK_FAILED = 0x1,
+ TARGET_APPTAG_CHECK_FAILED = 0x2,
+ TARGET_REFTAG_CHECK_FAILED = 0x3,
+};
+
+struct se_dif_v1_tuple {
+ __be16 guard_tag;
+ __be16 app_tag;
+ __be32 ref_tag;
+};
+
struct se_cmd {
/* SAM response code being sent to initiator */
u8 scsi_status;
@@ -498,6 +538,20 @@ struct se_cmd {
/* Used for lun->lun_ref counting */
bool lun_ref_active;
+
+ /* DIF related members */
+ enum target_prot_op prot_op;
+ enum target_prot_type prot_type;
+ enum target_guard_type bg_type;
+ u16 bg_seed;
+ u16 reftag_seed;
+ u32 apptag_seed;
+ u32 prot_length;
+ struct scatterlist *t_prot_sg;
+ unsigned int t_prot_nents;
+ bool prot_interleaved;
+ enum target_pi_error pi_err;
+ u32 block_num;
};
struct se_ua {
@@ -609,6 +663,9 @@ struct se_dev_attrib {
int emulate_tpws;
int emulate_caw;
int emulate_3pc;
+ enum target_prot_type pi_prot_type;
+ enum target_prot_version pi_prot_version;
+ enum target_guard_type pi_guard_type;
int enforce_pr_isids;
int is_nonrot;
int emulate_rest_reord;
@@ -739,6 +796,8 @@ struct se_device {
/* Linked list for struct se_hba struct se_device list */
struct list_head dev_list;
struct se_lun xcopy_lun;
+ /* Protection Information */
+ int prot_length;
};
struct se_hba {
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch updates tcm_loop_driver_probe() to set protection
information using scsi_host_set_prot() and scsi_host_set_guard(),
which currently enabled all modes of DIF/DIX protection, minus
DIF TYPE0.
Also, update tcm_loop_submission_work() to pass struct scsi_cmnd
related protection into target_submit_cmd_map_sgls() during CDB
dispatch.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/loopback/tcm_loop.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index c6f1427..8d3355f 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -217,7 +217,8 @@ static void tcm_loop_submission_work(struct work_struct *work)
scsi_bufflen(sc), tcm_loop_sam_attr(sc),
sc->sc_data_direction, 0,
scsi_sglist(sc), scsi_sg_count(sc),
- sgl_bidi, sgl_bidi_count, NULL, 0);
+ sgl_bidi, sgl_bidi_count,
+ scsi_prot_sglist(sc), scsi_prot_sg_count(sc));
if (rc < 0) {
set_host_byte(sc, DID_NO_CONNECT);
goto out_done;
@@ -462,7 +463,7 @@ static int tcm_loop_driver_probe(struct device *dev)
{
struct tcm_loop_hba *tl_hba;
struct Scsi_Host *sh;
- int error;
+ int error, host_prot;
tl_hba = to_tcm_loop_hba(dev);
@@ -486,6 +487,13 @@ static int tcm_loop_driver_probe(struct device *dev)
sh->max_channel = 0;
sh->max_cmd_len = TL_SCSI_MAX_CMD_LEN;
+ host_prot = SHOST_DIF_TYPE1_PROTECTION | SHOST_DIF_TYPE2_PROTECTION |
+ SHOST_DIF_TYPE3_PROTECTION | SHOST_DIX_TYPE1_PROTECTION |
+ SHOST_DIX_TYPE2_PROTECTION | SHOST_DIX_TYPE3_PROTECTION;
+
+ scsi_host_set_prot(sh, host_prot);
+ scsi_host_set_guard(sh, SHOST_DIX_GUARD_CRC);
+
error = scsi_add_host(sh, &tl_hba->dev);
if (error) {
pr_err("%s: scsi_add_host failed\n", __func__);
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch adds rd_build_prot_space() + rd_release_prot_space() logic
to setup + release protection information scatterlists.
It also adds rd_init_prot() + rd_free_prot() se_subsystem_api
callbacks used by target core code for setup + release of
protection information.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_rd.c | 81 +++++++++++++++++++++++++++++++++++++++
drivers/target/target_core_rd.h | 4 ++
2 files changed, 85 insertions(+)
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index fdc5022..dd99844 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -226,6 +226,64 @@ static int rd_build_device_space(struct rd_dev *rd_dev)
return 0;
}
+static void rd_release_prot_space(struct rd_dev *rd_dev)
+{
+ struct rd_dev_sg_table *sg_table;
+ u32 page_count;
+
+ if (!rd_dev->sg_prot_array || !rd_dev->sg_prot_count)
+ return;
+
+ sg_table = rd_dev->sg_prot_array;
+
+ page_count = rd_release_sgl_table(rd_dev, rd_dev->sg_prot_array,
+ rd_dev->sg_prot_count);
+
+ pr_debug("CORE_RD[%u] - Released protection space for Ramdisk"
+ " Device ID: %u, pages %u in %u tables total bytes %lu\n",
+ rd_dev->rd_host->rd_host_id, rd_dev->rd_dev_id, page_count,
+ rd_dev->sg_table_count, (unsigned long)page_count * PAGE_SIZE);
+
+ rd_dev->sg_prot_array = NULL;
+ rd_dev->sg_prot_count = 0;
+}
+
+static int rd_build_prot_space(struct rd_dev *rd_dev, int prot_length)
+{
+ struct rd_dev_sg_table *sg_table;
+ u32 total_sg_needed, sg_tables;
+ u32 max_sg_per_table = (RD_MAX_ALLOCATION_SIZE /
+ sizeof(struct scatterlist));
+ int rc;
+
+ if (rd_dev->rd_flags & RDF_NULLIO)
+ return 0;
+
+ total_sg_needed = rd_dev->rd_page_count / prot_length;
+
+ sg_tables = (total_sg_needed / max_sg_per_table) + 1;
+
+ sg_table = kzalloc(sg_tables * sizeof(struct rd_dev_sg_table), GFP_KERNEL);
+ if (!sg_table) {
+ pr_err("Unable to allocate memory for Ramdisk protection"
+ " scatterlist tables\n");
+ return -ENOMEM;
+ }
+
+ rd_dev->sg_prot_array = sg_table;
+ rd_dev->sg_prot_count = sg_tables;
+
+ rc = rd_allocate_sgl_table(rd_dev, sg_table, total_sg_needed, 0xff);
+ if (rc)
+ return rc;
+
+ pr_debug("CORE_RD[%u] - Built Ramdisk Device ID: %u prot space of"
+ " %u pages in %u tables\n", rd_dev->rd_host->rd_host_id,
+ rd_dev->rd_dev_id, total_sg_needed, rd_dev->sg_prot_count);
+
+ return 0;
+}
+
static struct se_device *rd_alloc_device(struct se_hba *hba, const char *name)
{
struct rd_dev *rd_dev;
@@ -280,6 +338,7 @@ static void rd_free_device(struct se_device *dev)
{
struct rd_dev *rd_dev = RD_DEV(dev);
+ rd_release_prot_space(rd_dev);
rd_release_device_space(rd_dev);
kfree(rd_dev);
}
@@ -482,6 +541,26 @@ static sector_t rd_get_blocks(struct se_device *dev)
return blocks_long;
}
+static int rd_init_prot(struct se_device *dev)
+{
+ struct rd_dev *rd_dev = RD_DEV(dev);
+
+ if (!dev->dev_attrib.pi_prot_type)
+ return 0;
+
+ return rd_build_prot_space(rd_dev, dev->prot_length);
+}
+
+static void rd_free_prot(struct se_device *dev)
+{
+ struct rd_dev *rd_dev = RD_DEV(dev);
+
+ if (dev->dev_attrib.pi_prot_type)
+ return;
+
+ rd_release_prot_space(rd_dev);
+}
+
static struct sbc_ops rd_sbc_ops = {
.execute_rw = rd_execute_rw,
};
@@ -507,6 +586,8 @@ static struct se_subsystem_api rd_mcp_template = {
.show_configfs_dev_params = rd_show_configfs_dev_params,
.get_device_type = sbc_get_device_type,
.get_blocks = rd_get_blocks,
+ .init_prot = rd_init_prot,
+ .free_prot = rd_free_prot,
};
int __init rd_module_init(void)
diff --git a/drivers/target/target_core_rd.h b/drivers/target/target_core_rd.h
index 1789d1e..cc46a6a 100644
--- a/drivers/target/target_core_rd.h
+++ b/drivers/target/target_core_rd.h
@@ -33,8 +33,12 @@ struct rd_dev {
u32 rd_page_count;
/* Number of SG tables in sg_table_array */
u32 sg_table_count;
+ /* Number of SG tables in sg_prot_array */
+ u32 sg_prot_count;
/* Array of rd_dev_sg_table_t containing scatterlists */
struct rd_dev_sg_table *sg_table_array;
+ /* Array of rd_dev_sg_table containing protection scatterlists */
+ struct rd_dev_sg_table *sg_prot_array;
/* Ramdisk HBA device is connected to */
struct rd_host *rd_host;
} ____cacheline_aligned;
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch adds support for DIF protection into rd_execute_rw() code
for WRITE/READ I/O using sbc_dif_verify_[write,read]() logic.
It also adds rd_get_prot_table() for locating protection SGLs
assoicated with the ramdisk backend device.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_rd.c | 67 +++++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index dd99844..3fd51eb 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -363,6 +363,26 @@ static struct rd_dev_sg_table *rd_get_sg_table(struct rd_dev *rd_dev, u32 page)
return NULL;
}
+static struct rd_dev_sg_table *rd_get_prot_table(struct rd_dev *rd_dev, u32 page)
+{
+ struct rd_dev_sg_table *sg_table;
+ u32 i, sg_per_table = (RD_MAX_ALLOCATION_SIZE /
+ sizeof(struct scatterlist));
+
+ i = page / sg_per_table;
+ if (i < rd_dev->sg_prot_count) {
+ sg_table = &rd_dev->sg_prot_array[i];
+ if ((sg_table->page_start_offset <= page) &&
+ (sg_table->page_end_offset >= page))
+ return sg_table;
+ }
+
+ pr_err("Unable to locate struct prot rd_dev_sg_table for page: %u\n",
+ page);
+
+ return NULL;
+}
+
static sense_reason_t
rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
enum dma_data_direction data_direction)
@@ -377,6 +397,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
u32 rd_page;
u32 src_len;
u64 tmp;
+ sense_reason_t rc;
if (dev->rd_flags & RDF_NULLIO) {
target_complete_cmd(cmd, SAM_STAT_GOOD);
@@ -399,6 +420,29 @@ 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->se_cmd_flags & SCF_PROT) && se_dev->dev_attrib.pi_prot_type &&
+ data_direction == DMA_TO_DEVICE) {
+ sector_t sector = cmd->data_length / se_dev->dev_attrib.block_size;
+ struct rd_dev_sg_table *prot_table;
+ struct scatterlist *prot_sg;
+ u32 prot_offset, prot_page;
+
+ tmp = cmd->t_task_lba * se_dev->prot_length;
+ prot_offset = do_div(tmp, PAGE_SIZE);
+ prot_page = tmp;
+
+ prot_table = rd_get_prot_table(dev, prot_page);
+ if (!prot_table)
+ return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+ prot_sg = &prot_table->sg_table[prot_page - prot_table->page_start_offset];
+
+ rc = sbc_dif_verify_write(cmd, cmd->t_task_lba, sector, 0,
+ prot_sg, prot_offset);
+ if (rc)
+ return rc;
+ }
+
src_len = PAGE_SIZE - rd_offset;
sg_miter_start(&m, sgl, sgl_nents,
data_direction == DMA_FROM_DEVICE ?
@@ -460,6 +504,29 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
}
sg_miter_stop(&m);
+ if ((cmd->se_cmd_flags & SCF_PROT) && se_dev->dev_attrib.pi_prot_type &&
+ data_direction == DMA_FROM_DEVICE) {
+ sector_t sector = cmd->data_length / se_dev->dev_attrib.block_size;
+ struct rd_dev_sg_table *prot_table;
+ struct scatterlist *prot_sg;
+ u32 prot_offset, prot_page;
+
+ tmp = cmd->t_task_lba * se_dev->prot_length;
+ prot_offset = do_div(tmp, PAGE_SIZE);
+ prot_page = tmp;
+
+ prot_table = rd_get_prot_table(dev, prot_page);
+ if (!prot_table)
+ return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+ prot_sg = &prot_table->sg_table[prot_page - prot_table->page_start_offset];
+
+ rc = sbc_dif_verify_read(cmd, cmd->t_task_lba, sector, 0,
+ prot_sg, prot_offset);
+ if (rc)
+ return rc;
+ }
+
target_complete_cmd(cmd, SAM_STAT_GOOD);
return 0;
}
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch refactors rd_build_device_space() + rd_release_device_space()
into rd_allocate_sgl_table() + rd_release_device_space() so that they
may be used seperatly for setup + release of protection information
scatterlists.
Also add explicit memset of pages within rd_allocate_sgl_table() based
upon passed 'init_payload' value.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_rd.c | 116 ++++++++++++++++++++++++---------------
1 file changed, 71 insertions(+), 45 deletions(-)
diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
index 4ffe5f2..fdc5022 100644
--- a/drivers/target/target_core_rd.c
+++ b/drivers/target/target_core_rd.c
@@ -78,23 +78,14 @@ static void rd_detach_hba(struct se_hba *hba)
hba->hba_ptr = NULL;
}
-/* rd_release_device_space():
- *
- *
- */
-static void rd_release_device_space(struct rd_dev *rd_dev)
+static u32 rd_release_sgl_table(struct rd_dev *rd_dev, struct rd_dev_sg_table *sg_table,
+ u32 sg_table_count)
{
- u32 i, j, page_count = 0, sg_per_table;
- struct rd_dev_sg_table *sg_table;
struct page *pg;
struct scatterlist *sg;
+ u32 i, j, page_count = 0, sg_per_table;
- if (!rd_dev->sg_table_array || !rd_dev->sg_table_count)
- return;
-
- sg_table = rd_dev->sg_table_array;
-
- for (i = 0; i < rd_dev->sg_table_count; i++) {
+ for (i = 0; i < sg_table_count; i++) {
sg = sg_table[i].sg_table;
sg_per_table = sg_table[i].rd_sg_count;
@@ -105,16 +96,31 @@ static void rd_release_device_space(struct rd_dev *rd_dev)
page_count++;
}
}
-
kfree(sg);
}
+ kfree(sg_table);
+ return page_count;
+}
+
+static void rd_release_device_space(struct rd_dev *rd_dev)
+{
+ struct rd_dev_sg_table *sg_table;
+ u32 page_count;
+
+ if (!rd_dev->sg_table_array || !rd_dev->sg_table_count)
+ return;
+
+ sg_table = rd_dev->sg_table_array;
+
+ page_count = rd_release_sgl_table(rd_dev, rd_dev->sg_table_array,
+ rd_dev->sg_table_count);
+
pr_debug("CORE_RD[%u] - Released device space for Ramdisk"
" Device ID: %u, pages %u in %u tables total bytes %lu\n",
rd_dev->rd_host->rd_host_id, rd_dev->rd_dev_id, page_count,
rd_dev->sg_table_count, (unsigned long)page_count * PAGE_SIZE);
- kfree(sg_table);
rd_dev->sg_table_array = NULL;
rd_dev->sg_table_count = 0;
}
@@ -124,38 +130,15 @@ static void rd_release_device_space(struct rd_dev *rd_dev)
*
*
*/
-static int rd_build_device_space(struct rd_dev *rd_dev)
+static int rd_allocate_sgl_table(struct rd_dev *rd_dev, struct rd_dev_sg_table *sg_table,
+ u32 total_sg_needed, unsigned char init_payload)
{
- u32 i = 0, j, page_offset = 0, sg_per_table, sg_tables, total_sg_needed;
+ u32 i = 0, j, page_offset = 0, sg_per_table;
u32 max_sg_per_table = (RD_MAX_ALLOCATION_SIZE /
sizeof(struct scatterlist));
- struct rd_dev_sg_table *sg_table;
struct page *pg;
struct scatterlist *sg;
-
- if (rd_dev->rd_page_count <= 0) {
- pr_err("Illegal page count: %u for Ramdisk device\n",
- rd_dev->rd_page_count);
- return -EINVAL;
- }
-
- /* Don't need backing pages for NULLIO */
- if (rd_dev->rd_flags & RDF_NULLIO)
- return 0;
-
- total_sg_needed = rd_dev->rd_page_count;
-
- sg_tables = (total_sg_needed / max_sg_per_table) + 1;
-
- sg_table = kzalloc(sg_tables * sizeof(struct rd_dev_sg_table), GFP_KERNEL);
- if (!sg_table) {
- pr_err("Unable to allocate memory for Ramdisk"
- " scatterlist tables\n");
- return -ENOMEM;
- }
-
- rd_dev->sg_table_array = sg_table;
- rd_dev->sg_table_count = sg_tables;
+ unsigned char *p;
while (total_sg_needed) {
sg_per_table = (total_sg_needed > max_sg_per_table) ?
@@ -186,16 +169,59 @@ static int rd_build_device_space(struct rd_dev *rd_dev)
}
sg_assign_page(&sg[j], pg);
sg[j].length = PAGE_SIZE;
+
+ p = kmap(pg);
+ memset(p, init_payload, PAGE_SIZE);
+ kunmap(pg);
}
page_offset += sg_per_table;
total_sg_needed -= sg_per_table;
}
+ return 0;
+}
+
+static int rd_build_device_space(struct rd_dev *rd_dev)
+{
+ struct rd_dev_sg_table *sg_table;
+ u32 sg_tables, total_sg_needed;
+ u32 max_sg_per_table = (RD_MAX_ALLOCATION_SIZE /
+ sizeof(struct scatterlist));
+ int rc;
+
+ if (rd_dev->rd_page_count <= 0) {
+ pr_err("Illegal page count: %u for Ramdisk device\n",
+ rd_dev->rd_page_count);
+ return -EINVAL;
+ }
+
+ /* Don't need backing pages for NULLIO */
+ if (rd_dev->rd_flags & RDF_NULLIO)
+ return 0;
+
+ total_sg_needed = rd_dev->rd_page_count;
+
+ sg_tables = (total_sg_needed / max_sg_per_table) + 1;
+
+ sg_table = kzalloc(sg_tables * sizeof(struct rd_dev_sg_table), GFP_KERNEL);
+ if (!sg_table) {
+ pr_err("Unable to allocate memory for Ramdisk"
+ " scatterlist tables\n");
+ return -ENOMEM;
+ }
+
+ rd_dev->sg_table_array = sg_table;
+ rd_dev->sg_table_count = sg_tables;
+
+ rc = rd_allocate_sgl_table(rd_dev, sg_table, total_sg_needed, 0x00);
+ if (rc)
+ return rc;
+
pr_debug("CORE_RD[%u] - Built Ramdisk Device ID: %u space of"
- " %u pages in %u tables\n", rd_dev->rd_host->rd_host_id,
- rd_dev->rd_dev_id, rd_dev->rd_page_count,
- rd_dev->sg_table_count);
+ " %u pages in %u tables\n", rd_dev->rd_host->rd_host_id,
+ rd_dev->rd_dev_id, rd_dev->rd_page_count,
+ rd_dev->sg_table_count);
return 0;
}
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch adds support for exposing DIF protection device
attributes via configfs. This includes:
pi_prot_type: Protection Type (0, 1, 3 currently support)
pi_prot_version: Protection Version (DIF v1 currently supported)
pi_guard_type: Guard Type (1=DIF CRC, 2=IP CRC)
Within se_dev_set_pi_prot_type() it also adds the se_subsystem_api
device callbacks to setup per device protection information.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_configfs.c | 12 ++++++
drivers/target/target_core_device.c | 65 +++++++++++++++++++++++++++++++++
drivers/target/target_core_internal.h | 2 +
3 files changed, 79 insertions(+)
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index 272755d..0f1101c 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -643,6 +643,15 @@ SE_DEV_ATTR(emulate_caw, S_IRUGO | S_IWUSR);
DEF_DEV_ATTRIB(emulate_3pc);
SE_DEV_ATTR(emulate_3pc, S_IRUGO | S_IWUSR);
+DEF_DEV_ATTRIB(pi_prot_type);
+SE_DEV_ATTR(pi_prot_type, S_IRUGO | S_IWUSR);
+
+DEF_DEV_ATTRIB_RO(pi_prot_version);
+SE_DEV_ATTR_RO(pi_prot_version);
+
+DEF_DEV_ATTRIB(pi_guard_type);
+SE_DEV_ATTR(pi_guard_type, S_IRUGO | S_IWUSR);
+
DEF_DEV_ATTRIB(enforce_pr_isids);
SE_DEV_ATTR(enforce_pr_isids, S_IRUGO | S_IWUSR);
@@ -702,6 +711,9 @@ static struct configfs_attribute *target_core_dev_attrib_attrs[] = {
&target_core_dev_attrib_emulate_tpws.attr,
&target_core_dev_attrib_emulate_caw.attr,
&target_core_dev_attrib_emulate_3pc.attr,
+ &target_core_dev_attrib_pi_prot_type.attr,
+ &target_core_dev_attrib_pi_prot_version.attr,
+ &target_core_dev_attrib_pi_guard_type.attr,
&target_core_dev_attrib_enforce_pr_isids.attr,
&target_core_dev_attrib_is_nonrot.attr,
&target_core_dev_attrib_emulate_rest_reord.attr,
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 207b340..2b59beb 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -918,6 +918,67 @@ int se_dev_set_emulate_3pc(struct se_device *dev, int flag)
return 0;
}
+int se_dev_set_pi_prot_type(struct se_device *dev, int flag)
+{
+ int rc, old_prot = dev->dev_attrib.pi_prot_type;
+
+ if (flag != 0 && flag != 1 && flag != 2 && flag != 3) {
+ pr_err("Illegal value %d for pi_prot_type\n", flag);
+ return -EINVAL;
+ }
+ if (flag == 2) {
+ pr_err("DIF TYPE2 protection currently not supported\n");
+ return -ENOSYS;
+ }
+ if (!dev->transport->init_prot || !dev->transport->free_prot) {
+ pr_err("DIF protection not supported by backend: %s\n",
+ dev->transport->name);
+ return -ENOSYS;
+ }
+ if (!(dev->dev_flags & DF_CONFIGURED)) {
+ pr_err("DIF protection requires device to be configured\n");
+ return -ENODEV;
+ }
+ if (dev->export_count) {
+ pr_err("dev[%p]: Unable to change SE Device PROT type while"
+ " export_count is %d\n", dev, dev->export_count);
+ return -EINVAL;
+ }
+
+ dev->dev_attrib.pi_prot_type = flag;
+
+ if (flag && !old_prot) {
+ rc = dev->transport->init_prot(dev);
+ if (rc) {
+ dev->dev_attrib.pi_prot_type = old_prot;
+ return rc;
+ }
+ } else if (!flag && old_prot) {
+ dev->transport->free_prot(dev);
+ }
+ pr_debug("dev[%p]: SE Device Protection Type: %d\n", dev, flag);
+
+ return 0;
+}
+
+int se_dev_set_pi_guard_type(struct se_device *dev, int flag)
+{
+ if (flag != 1 && flag != 2) {
+ pr_err("Illegal value %d for pi_guard_type\n", flag);
+ return -EINVAL;
+ }
+ if (dev->export_count) {
+ pr_err("dev[%p]: Unable to change SE Device GUARD type while"
+ " export_count is %d\n", dev, dev->export_count);
+ return -EINVAL;
+ }
+
+ dev->dev_attrib.pi_guard_type = flag;
+ pr_debug("dev[%p]: SE Device Guard Type: %d\n", dev, flag);
+
+ return 0;
+}
+
int se_dev_set_enforce_pr_isids(struct se_device *dev, int flag)
{
if ((flag != 0) && (flag != 1)) {
@@ -1415,6 +1476,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
dev->dev_link_magic = SE_DEV_LINK_MAGIC;
dev->se_hba = hba;
dev->transport = hba->transport;
+ dev->prot_length = sizeof(struct se_dif_v1_tuple);
INIT_LIST_HEAD(&dev->dev_list);
INIT_LIST_HEAD(&dev->dev_sep_list);
@@ -1455,6 +1517,9 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
dev->dev_attrib.emulate_tpws = DA_EMULATE_TPWS;
dev->dev_attrib.emulate_caw = DA_EMULATE_CAW;
dev->dev_attrib.emulate_3pc = DA_EMULATE_3PC;
+ dev->dev_attrib.pi_prot_type = TARGET_DIF_TYPE0_PROT;
+ dev->dev_attrib.pi_prot_version = TARGET_DIF_V1;
+ dev->dev_attrib.pi_guard_type = TARGET_DIX_GUARD_CRC;
dev->dev_attrib.enforce_pr_isids = DA_ENFORCE_PR_ISIDS;
dev->dev_attrib.is_nonrot = DA_IS_NONROT;
dev->dev_attrib.emulate_rest_reord = DA_EMULATE_REST_REORD;
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 47b63b0..79d3bc3 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -35,6 +35,8 @@ int se_dev_set_emulate_tpu(struct se_device *, int);
int se_dev_set_emulate_tpws(struct se_device *, int);
int se_dev_set_emulate_caw(struct se_device *, int);
int se_dev_set_emulate_3pc(struct se_device *, int);
+int se_dev_set_pi_prot_type(struct se_device *, int);
+int se_dev_set_pi_guard_type(struct se_device *, int);
int se_dev_set_enforce_pr_isids(struct se_device *, int);
int se_dev_set_is_nonrot(struct se_device *, int);
int se_dev_set_emulate_rest_reord(struct se_device *dev, int);
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch adds support for DIF related CHECK_CONDITION ASC/ASCQ
exception cases into transport_send_check_condition_and_sense().
This includes:
LOGICAL BLOCK GUARD CHECK FAILED
LOGICAL BLOCK APPLICATION TAG CHECK FAILED
LOGICAL BLOCK REFERENCE TAG CHECK FAILED
that used by DIF TYPE1 and TYPE3 failure cases.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_transport.c | 30 ++++++++++++++++++++++++++++++
include/target/target_core_base.h | 3 +++
2 files changed, 33 insertions(+)
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 91953da..707ee17 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2648,6 +2648,36 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
buffer[SPC_ASC_KEY_OFFSET] = 0x1d;
buffer[SPC_ASCQ_KEY_OFFSET] = 0x00;
break;
+ case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED:
+ /* CURRENT ERROR */
+ buffer[0] = 0x70;
+ buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
+ /* ILLEGAL REQUEST */
+ buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
+ /* LOGICAL BLOCK GUARD CHECK FAILED */
+ buffer[SPC_ASC_KEY_OFFSET] = 0x10;
+ buffer[SPC_ASCQ_KEY_OFFSET] = 0x01;
+ break;
+ case TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED:
+ /* CURRENT ERROR */
+ buffer[0] = 0x70;
+ buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
+ /* ILLEGAL REQUEST */
+ buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
+ /* LOGICAL BLOCK APPLICATION TAG CHECK FAILED */
+ buffer[SPC_ASC_KEY_OFFSET] = 0x10;
+ buffer[SPC_ASCQ_KEY_OFFSET] = 0x02;
+ break;
+ case TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED:
+ /* CURRENT ERROR */
+ buffer[0] = 0x70;
+ buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
+ /* ILLEGAL REQUEST */
+ buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
+ /* LOGICAL BLOCK REFERENCE TAG CHECK FAILED */
+ buffer[SPC_ASC_KEY_OFFSET] = 0x10;
+ buffer[SPC_ASCQ_KEY_OFFSET] = 0x03;
+ break;
case TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE:
default:
/* CURRENT ERROR */
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 15f402c..9a6e091 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -206,6 +206,9 @@ enum tcm_sense_reason_table {
TCM_OUT_OF_RESOURCES = R(0x12),
TCM_PARAMETER_LIST_LENGTH_ERROR = R(0x13),
TCM_MISCOMPARE_VERIFY = R(0x14),
+ TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED = R(0x15),
+ TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED = R(0x16),
+ TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED = R(0x17),
#undef R
};
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch updates sbc_emulate_readcapacity_16() to set
P_TYPE and PROT_EN bits when DIF emulation is enabled by
the backend device.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_sbc.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 366b9bb..22599e8 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -106,6 +106,11 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
buf[9] = (dev->dev_attrib.block_size >> 16) & 0xff;
buf[10] = (dev->dev_attrib.block_size >> 8) & 0xff;
buf[11] = dev->dev_attrib.block_size & 0xff;
+ /*
+ * Set P_TYPE and PROT_EN bits for DIF support
+ */
+ if (dev->dev_attrib.pi_prot_type)
+ buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
if (dev->transport->get_lbppbe)
buf[13] = dev->transport->get_lbppbe(dev) & 0x0f;
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch updates spc_modesense_control() to set the Application
Tag Owner (ATO) bit when when DIF emulation is enabled by the
backend device.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_spc.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index d99eb95..4360c80 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -799,6 +799,19 @@ static int spc_modesense_control(struct se_device *dev, u8 pc, u8 *p)
* status (see SAM-4).
*/
p[5] = (dev->dev_attrib.emulate_tas) ? 0x40 : 0x00;
+ /*
+ * From spc4r30, section 7.5.7 Control mode page
+ *
+ * Application Tag Owner (ATO) bit set to one.
+ *
+ * If the ATO bit is set to one the device server shall not modify the
+ * LOGICAL BLOCK APPLICATION TAG field and, depending on the protection
+ * type, shall not modify the contents of the LOGICAL BLOCK REFERENCE
+ * TAG field.
+ */
+ if (dev->dev_attrib.pi_prot_type)
+ p[5] |= 0x80;
+
p[8] = 0xff;
p[9] = 0xff;
p[11] = 30;
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch updates spc_emulate_inquiry_std() to set the
PROTECT bit when DIF emulation is enabled by the backend
device.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_spc.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 021c3f4..6b06f01 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -100,6 +100,11 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char *buf)
*/
if (dev->dev_attrib.emulate_3pc)
buf[5] |= 0x8;
+ /*
+ * Set Protection (PROTECT) bit when DIF has been enabled.
+ */
+ if (dev->dev_attrib.pi_prot_type)
+ buf[5] |= 0x1;
buf[7] = 0x2; /* CmdQue=1 */
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch adds sbc_check_prot() for performing various DIF
related CDB sanity checks, along with setting SCF_PROT once
sanity checks have passed.
Also, add calls in sbc_parse_cdb() for READ_[10,12,16] +
WRITE_[10,12,16] to perform DIF sanity checking.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_sbc.c | 39 ++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 52ae54e..600ffcb 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -563,6 +563,27 @@ sbc_compare_and_write(struct se_cmd *cmd)
return TCM_NO_SENSE;
}
+bool
+sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
+{
+ if (!dev->dev_attrib.pi_prot_type)
+ return true;
+
+ if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE2_PROT &&
+ (cdb[1] & 0xe0))
+ return false;
+
+ if (!(cdb[1] & 0xe0)) {
+ pr_warn("Target: Unprotected READ/WRITE to DIF device\n");
+ return true;
+ }
+ if (!cmd->t_prot_sg || !cmd->t_prot_nents)
+ return true;
+
+ cmd->se_cmd_flags |= SCF_PROT;
+ return true;
+}
+
sense_reason_t
sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
{
@@ -581,6 +602,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
cmd->execute_cmd = sbc_execute_rw;
break;
case READ_10:
+ if (!sbc_check_prot(dev, cmd, cdb))
+ return TCM_UNSUPPORTED_SCSI_OPCODE;
+
sectors = transport_get_sectors_10(cdb);
cmd->t_task_lba = transport_lba_32(cdb);
cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
@@ -588,6 +612,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
cmd->execute_cmd = sbc_execute_rw;
break;
case READ_12:
+ if (!sbc_check_prot(dev, cmd, cdb))
+ return TCM_UNSUPPORTED_SCSI_OPCODE;
+
sectors = transport_get_sectors_12(cdb);
cmd->t_task_lba = transport_lba_32(cdb);
cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
@@ -595,6 +622,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
cmd->execute_cmd = sbc_execute_rw;
break;
case READ_16:
+ if (!sbc_check_prot(dev, cmd, cdb))
+ return TCM_UNSUPPORTED_SCSI_OPCODE;
+
sectors = transport_get_sectors_16(cdb);
cmd->t_task_lba = transport_lba_64(cdb);
cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
@@ -610,6 +640,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
break;
case WRITE_10:
case WRITE_VERIFY:
+ if (!sbc_check_prot(dev, cmd, cdb))
+ return TCM_UNSUPPORTED_SCSI_OPCODE;
+
sectors = transport_get_sectors_10(cdb);
cmd->t_task_lba = transport_lba_32(cdb);
if (cdb[1] & 0x8)
@@ -619,6 +652,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
cmd->execute_cmd = sbc_execute_rw;
break;
case WRITE_12:
+ if (!sbc_check_prot(dev, cmd, cdb))
+ return TCM_UNSUPPORTED_SCSI_OPCODE;
+
sectors = transport_get_sectors_12(cdb);
cmd->t_task_lba = transport_lba_32(cdb);
if (cdb[1] & 0x8)
@@ -628,6 +664,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
cmd->execute_cmd = sbc_execute_rw;
break;
case WRITE_16:
+ if (!sbc_check_prot(dev, cmd, cdb))
+ return TCM_UNSUPPORTED_SCSI_OPCODE;
+
sectors = transport_get_sectors_16(cdb);
cmd->t_task_lba = transport_lba_64(cdb);
if (cdb[1] & 0x8)
--
1.7.10.4
From: Nicholas Bellinger <[email protected]>
This patch adds support for DIF read/write verify emulation
for TARGET_DIF_TYPE1_PROT + TARGET_DIF_TYPE3_PROT operation.
This includes sbc_dif_verify_write() + sbc_dif_verify_read()
calls accessable by backend drivers to perform DIF verify
for SGL based data and protection information.
Also included is sbc_dif_copy_prot() logic to copy protection
information to/from backend provided protection SGLs.
Based on scsi_debug.c DIF TYPE1+TYPE3 emulation.
Cc: Martin K. Petersen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Or Gerlitz <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
---
drivers/target/target_core_sbc.c | 180 ++++++++++++++++++++++++++++++++++
include/target/target_core_backend.h | 4 +
2 files changed, 184 insertions(+)
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 600ffcb..366b9bb 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -23,6 +23,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/ratelimit.h>
+#include <linux/crc-t10dif.h>
#include <asm/unaligned.h>
#include <scsi/scsi.h>
#include <scsi/scsi_tcq.h>
@@ -998,3 +999,182 @@ err:
return ret;
}
EXPORT_SYMBOL(sbc_execute_unmap);
+
+static sense_reason_t
+sbc_dif_v1_verify(struct se_device *dev, struct se_dif_v1_tuple *sdt,
+ const void *p, sector_t sector, unsigned int ei_lba)
+{
+ int block_size = dev->dev_attrib.block_size;
+ __be16 csum;
+
+ if (dev->dev_attrib.pi_guard_type == TARGET_DIX_GUARD_CRC)
+ csum = cpu_to_be16(crc_t10dif(p, block_size));
+ else
+ csum = (__force __be16)ip_compute_csum(p, block_size);
+
+ if (sdt->guard_tag != csum) {
+ pr_err("DIFv1 checksum failed on sector %llu guard tag 0x%04x"
+ " csum 0x%04x\n", (unsigned long long)sector,
+ be16_to_cpu(sdt->guard_tag), be16_to_cpu(csum));
+ return TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED;
+ }
+
+ if (dev->dev_attrib.pi_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,
+ be32_to_cpu(sdt->ref_tag), (u32)(sector & 0xffffffff));
+ return TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED;
+ }
+
+ if (dev->dev_attrib.pi_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,
+ be32_to_cpu(sdt->ref_tag), ei_lba);
+ return TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED;
+ }
+
+ return 0;
+}
+
+static void
+sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read,
+ struct scatterlist *sg, int sg_off)
+{
+ struct se_device *dev = cmd->se_dev;
+ struct scatterlist *psg;
+ void *paddr, *addr;
+ unsigned int i, len, left;
+
+ left = sectors * dev->prot_length;
+
+ for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) {
+
+ len = min(psg->length, left);
+ paddr = kmap_atomic(sg_page(psg)) + psg->offset;
+ addr = kmap_atomic(sg_page(sg)) + sg_off;
+
+ if (read)
+ memcpy(paddr, addr, len);
+ else
+ memcpy(addr, paddr, len);
+
+ left -= len;
+ kunmap_atomic(paddr);
+ kunmap_atomic(addr);
+ }
+}
+
+sense_reason_t
+sbc_dif_verify_write(struct se_cmd *cmd, sector_t start, unsigned int sectors,
+ unsigned int ei_lba, struct scatterlist *sg, int sg_off)
+{
+ struct se_device *dev = cmd->se_dev;
+ struct se_dif_v1_tuple *sdt;
+ struct scatterlist *dsg, *psg = cmd->t_prot_sg;
+ sector_t sector = start;
+ void *daddr, *paddr;
+ int i, j, offset = 0;
+ sense_reason_t rc;
+
+ for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
+ daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+ paddr = kmap_atomic(sg_page(psg)) + psg->offset;
+
+ for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
+
+ if (offset >= psg->length) {
+ kunmap_atomic(paddr);
+ psg = sg_next(psg);
+ paddr = kmap_atomic(sg_page(psg)) + psg->offset;
+ offset = 0;
+ }
+
+ sdt = paddr + offset;
+
+ pr_debug("DIF WRITE sector: %llu guard_tag: 0x%04x"
+ " app_tag: 0x%04x ref_tag: %u\n",
+ (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,
+ ei_lba);
+ if (rc) {
+ kunmap_atomic(paddr);
+ kunmap_atomic(daddr);
+ return rc;
+ }
+
+ sector++;
+ ei_lba++;
+ offset += sizeof(struct se_dif_v1_tuple);
+ }
+
+ kunmap_atomic(paddr);
+ kunmap_atomic(daddr);
+ }
+ sbc_dif_copy_prot(cmd, sectors, false, sg, sg_off);
+
+ return 0;
+}
+EXPORT_SYMBOL(sbc_dif_verify_write);
+
+sense_reason_t
+sbc_dif_verify_read(struct se_cmd *cmd, sector_t start, unsigned int sectors,
+ unsigned int ei_lba, struct scatterlist *sg, int sg_off)
+{
+ struct se_device *dev = cmd->se_dev;
+ struct se_dif_v1_tuple *sdt;
+ struct scatterlist *dsg;
+ sector_t sector = start;
+ void *daddr, *paddr;
+ int i, j, offset = sg_off;
+ sense_reason_t rc;
+
+ for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) {
+ daddr = kmap_atomic(sg_page(dsg)) + dsg->offset;
+ paddr = kmap_atomic(sg_page(sg)) + sg->offset;
+
+ for (j = 0; j < dsg->length; j += dev->dev_attrib.block_size) {
+
+ if (offset >= sg->length) {
+ kunmap_atomic(paddr);
+ sg = sg_next(sg);
+ paddr = kmap_atomic(sg_page(sg)) + sg->offset;
+ offset = 0;
+ }
+
+ sdt = paddr + offset;
+
+ pr_debug("DIF READ sector: %llu guard_tag: 0x%04x"
+ " app_tag: 0x%04x ref_tag: %u\n",
+ (unsigned long long)sector, sdt->guard_tag,
+ sdt->app_tag, be32_to_cpu(sdt->ref_tag));
+
+ if (sdt->app_tag == cpu_to_be16(0xffff)) {
+ sector++;
+ continue;
+ }
+
+ rc = sbc_dif_v1_verify(dev, sdt, daddr + j, sector,
+ ei_lba);
+ if (rc) {
+ kunmap_atomic(paddr);
+ kunmap_atomic(daddr);
+ return rc;
+ }
+
+ sector++;
+ ei_lba++;
+ offset += sizeof(struct se_dif_v1_tuple);
+ }
+
+ kunmap_atomic(paddr);
+ kunmap_atomic(daddr);
+ }
+ sbc_dif_copy_prot(cmd, sectors, true, sg, sg_off);
+
+ return 0;
+}
+EXPORT_SYMBOL(sbc_dif_verify_read);
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 930f30d..eb1dbbe 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -72,6 +72,10 @@ sense_reason_t sbc_execute_unmap(struct se_cmd *cmd,
sense_reason_t (*do_unmap_fn)(struct se_cmd *cmd, void *priv,
sector_t lba, sector_t nolb),
void *priv);
+sense_reason_t sbc_dif_verify_write(struct se_cmd *, sector_t, unsigned int,
+ unsigned int, struct scatterlist *, int);
+sense_reason_t sbc_dif_verify_read(struct se_cmd *, sector_t, unsigned int,
+ unsigned int, struct scatterlist *, int);
void transport_set_vpd_proto_id(struct t10_vpd *, unsigned char *);
int transport_set_vpd_assoc(struct t10_vpd *, unsigned char *);
--
1.7.10.4
On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> This patch updates sbc_emulate_readcapacity_16() to set
> P_TYPE and PROT_EN bits when DIF emulation is enabled by
> the backend device.
>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: Or Gerlitz <[email protected]>
> Signed-off-by: Nicholas Bellinger <[email protected]>
> ---
> drivers/target/target_core_sbc.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 366b9bb..22599e8 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -106,6 +106,11 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
> buf[9] = (dev->dev_attrib.block_size >> 16) & 0xff;
> buf[10] = (dev->dev_attrib.block_size >> 8) & 0xff;
> buf[11] = dev->dev_attrib.block_size & 0xff;
> + /*
> + * Set P_TYPE and PROT_EN bits for DIF support
> + */
> + if (dev->dev_attrib.pi_prot_type)
> + buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
>
> if (dev->transport->get_lbppbe)
> buf[13] = dev->transport->get_lbppbe(dev) & 0x0f;
Hey Nic,
What about FORMAT_UNIT emulation?
The backstore protection configuration is done at the target side via
configfs/targetcli, if you publish DIF support in
INQUERY_EVPD/READ_CAPACITY you need to accept protection information format?
Did I miss that one? or is it still under WIP?
Sagi.
On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> This patch adds support for DIF protection into rd_execute_rw() code
> for WRITE/READ I/O using sbc_dif_verify_[write,read]() logic.
>
> It also adds rd_get_prot_table() for locating protection SGLs
> assoicated with the ramdisk backend device.
>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: Or Gerlitz <[email protected]>
> Signed-off-by: Nicholas Bellinger <[email protected]>
> ---
> drivers/target/target_core_rd.c | 67 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 67 insertions(+)
>
> diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
> index dd99844..3fd51eb 100644
> --- a/drivers/target/target_core_rd.c
> +++ b/drivers/target/target_core_rd.c
> @@ -363,6 +363,26 @@ static struct rd_dev_sg_table *rd_get_sg_table(struct rd_dev *rd_dev, u32 page)
> return NULL;
> }
>
> +static struct rd_dev_sg_table *rd_get_prot_table(struct rd_dev *rd_dev, u32 page)
> +{
> + struct rd_dev_sg_table *sg_table;
> + u32 i, sg_per_table = (RD_MAX_ALLOCATION_SIZE /
> + sizeof(struct scatterlist));
> +
> + i = page / sg_per_table;
> + if (i < rd_dev->sg_prot_count) {
> + sg_table = &rd_dev->sg_prot_array[i];
> + if ((sg_table->page_start_offset <= page) &&
> + (sg_table->page_end_offset >= page))
> + return sg_table;
> + }
> +
> + pr_err("Unable to locate struct prot rd_dev_sg_table for page: %u\n",
> + page);
> +
> + return NULL;
> +}
> +
> static sense_reason_t
> rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
> enum dma_data_direction data_direction)
> @@ -377,6 +397,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
> u32 rd_page;
> u32 src_len;
> u64 tmp;
> + sense_reason_t rc;
>
> if (dev->rd_flags & RDF_NULLIO) {
> target_complete_cmd(cmd, SAM_STAT_GOOD);
> @@ -399,6 +420,29 @@ 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->se_cmd_flags & SCF_PROT) && se_dev->dev_attrib.pi_prot_type &&
> + data_direction == DMA_TO_DEVICE) {
> + sector_t sector = cmd->data_length / se_dev->dev_attrib.block_size;
> + struct rd_dev_sg_table *prot_table;
> + struct scatterlist *prot_sg;
> + u32 prot_offset, prot_page;
> +
> + tmp = cmd->t_task_lba * se_dev->prot_length;
> + prot_offset = do_div(tmp, PAGE_SIZE);
> + prot_page = tmp;
> +
> + prot_table = rd_get_prot_table(dev, prot_page);
> + if (!prot_table)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> +
> + prot_sg = &prot_table->sg_table[prot_page - prot_table->page_start_offset];
> +
> + rc = sbc_dif_verify_write(cmd, cmd->t_task_lba, sector, 0,
> + prot_sg, prot_offset);
> + if (rc)
> + return rc;
> + }
> +
> src_len = PAGE_SIZE - rd_offset;
> sg_miter_start(&m, sgl, sgl_nents,
> data_direction == DMA_FROM_DEVICE ?
> @@ -460,6 +504,29 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
> }
> sg_miter_stop(&m);
>
> + if ((cmd->se_cmd_flags & SCF_PROT) && se_dev->dev_attrib.pi_prot_type &&
> + data_direction == DMA_FROM_DEVICE) {
> + sector_t sector = cmd->data_length / se_dev->dev_attrib.block_size;
> + struct rd_dev_sg_table *prot_table;
> + struct scatterlist *prot_sg;
> + u32 prot_offset, prot_page;
> +
> + tmp = cmd->t_task_lba * se_dev->prot_length;
> + prot_offset = do_div(tmp, PAGE_SIZE);
> + prot_page = tmp;
> +
> + prot_table = rd_get_prot_table(dev, prot_page);
> + if (!prot_table)
> + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> +
> + prot_sg = &prot_table->sg_table[prot_page - prot_table->page_start_offset];
> +
> + rc = sbc_dif_verify_read(cmd, cmd->t_task_lba, sector, 0,
> + prot_sg, prot_offset);
> + if (rc)
> + return rc;
> + }
> +
> target_complete_cmd(cmd, SAM_STAT_GOOD);
> return 0;
> }
I wander how we can skip sbc_dif_verify_xxxx if the transport already
offloaded DIF verify.
I think that the transport should signal the core layer that it is able
to offload DIF (ADD/STRIP/PASS/VERIFY), in which case the core should
turn off the backstore DIF verify emulation to sustain performance. I
assume that if backstore DIF verify is offloaded as well it does not
really matter.
Sagi.
On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> This patch adds support for DIF related CHECK_CONDITION ASC/ASCQ
> exception cases into transport_send_check_condition_and_sense().
>
> This includes:
>
> LOGICAL BLOCK GUARD CHECK FAILED
> LOGICAL BLOCK APPLICATION TAG CHECK FAILED
> LOGICAL BLOCK REFERENCE TAG CHECK FAILED
>
> that used by DIF TYPE1 and TYPE3 failure cases.
>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: Or Gerlitz <[email protected]>
> Signed-off-by: Nicholas Bellinger <[email protected]>
> ---
> drivers/target/target_core_transport.c | 30 ++++++++++++++++++++++++++++++
> include/target/target_core_base.h | 3 +++
> 2 files changed, 33 insertions(+)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 91953da..707ee17 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2648,6 +2648,36 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
> buffer[SPC_ASC_KEY_OFFSET] = 0x1d;
> buffer[SPC_ASCQ_KEY_OFFSET] = 0x00;
> break;
> + case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED:
> + /* CURRENT ERROR */
> + buffer[0] = 0x70;
> + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
> + /* ILLEGAL REQUEST */
> + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
> + /* LOGICAL BLOCK GUARD CHECK FAILED */
> + buffer[SPC_ASC_KEY_OFFSET] = 0x10;
> + buffer[SPC_ASCQ_KEY_OFFSET] = 0x01;
You have Enums for ASCQs (TARGET_GUARD_CHECK_FAILED,
TARGET_APPTAG_CHECK_FAILED, TARGET_REFTAG_CHECK_FAILED). either use them
or loose them.
> + break;
> + case TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED:
> + /* CURRENT ERROR */
> + buffer[0] = 0x70;
> + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
> + /* ILLEGAL REQUEST */
> + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
> + /* LOGICAL BLOCK APPLICATION TAG CHECK FAILED */
> + buffer[SPC_ASC_KEY_OFFSET] = 0x10;
> + buffer[SPC_ASCQ_KEY_OFFSET] = 0x02;
> + break;
> + case TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED:
> + /* CURRENT ERROR */
> + buffer[0] = 0x70;
> + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
> + /* ILLEGAL REQUEST */
> + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
> + /* LOGICAL BLOCK REFERENCE TAG CHECK FAILED */
> + buffer[SPC_ASC_KEY_OFFSET] = 0x10;
> + buffer[SPC_ASCQ_KEY_OFFSET] = 0x03;
> + break;
> case TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE:
> default:
> /* CURRENT ERROR */
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 15f402c..9a6e091 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -206,6 +206,9 @@ enum tcm_sense_reason_table {
> TCM_OUT_OF_RESOURCES = R(0x12),
> TCM_PARAMETER_LIST_LENGTH_ERROR = R(0x13),
> TCM_MISCOMPARE_VERIFY = R(0x14),
> + TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED = R(0x15),
> + TCM_LOGICAL_BLOCK_APP_TAG_CHECK_FAILED = R(0x16),
> + TCM_LOGICAL_BLOCK_REF_TAG_CHECK_FAILED = R(0x17),
> #undef R
> };
>
On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> This patch adds DIF related definitions to target_core_base.h
> that includes enums for target_prot_op + target_prot_type +
> target_prot_version + target_guard_type + target_pi_error.
>
> Also included is struct se_dif_v1_tuple, along with changes
> to struct se_cmd, struct se_dev_attrib, and struct se_device.
>
> Also, add new se_subsystem_api->[init,free]_prot() callers
> used by target core code to setup backend specific protection
> information after the device has been configured.
>
> Enums taken from Sagi Grimberg's original patch.
>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: Or Gerlitz <[email protected]>
> Signed-off-by: Nicholas Bellinger <[email protected]>
>
> target: more defs
>
> Signed-off-by: Nicholas Bellinger <[email protected]>
> ---
> include/target/target_core_backend.h | 2 ++
> include/target/target_core_base.h | 59 ++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
> index 39e0114..930f30d 100644
> --- a/include/target/target_core_backend.h
> +++ b/include/target/target_core_backend.h
> @@ -41,6 +41,8 @@ struct se_subsystem_api {
> unsigned int (*get_io_opt)(struct se_device *);
> unsigned char *(*get_sense_buffer)(struct se_cmd *);
> bool (*get_write_cache)(struct se_device *);
> + int (*init_prot)(struct se_device *);
> + void (*free_prot)(struct se_device *);
> };
>
> struct sbc_ops {
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 45412a6..15f402c 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -166,6 +166,7 @@ enum se_cmd_flags_table {
> SCF_COMPARE_AND_WRITE = 0x00080000,
> SCF_COMPARE_AND_WRITE_POST = 0x00100000,
> SCF_CMD_XCOPY_PASSTHROUGH = 0x00200000,
> + SCF_PROT = 0x00400000,
> };
>
> /* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
> @@ -414,6 +415,45 @@ struct se_tmr_req {
> struct list_head tmr_list;
> };
>
> +enum target_prot_op {
> + TARGET_PROT_NORMAL,
> + TARGET_PROT_READ_INSERT,
> + TARGET_PROT_WRITE_INSERT,
> + TARGET_PROT_READ_STRIP,
> + TARGET_PROT_WRITE_STRIP,
> + TARGET_PROT_READ_PASS,
> + TARGET_PROT_WRITE_PASS,
> +};
> +
> +enum target_prot_type {
> + TARGET_DIF_TYPE0_PROT,
> + TARGET_DIF_TYPE1_PROT,
> + TARGET_DIF_TYPE2_PROT,
> + TARGET_DIF_TYPE3_PROT,
> +};
> +
> +enum target_prot_version {
> + TARGET_DIF_V1 = 1,
> + TARGET_DIF_V2 = 2,
> +};
> +
> +enum target_guard_type {
> + TARGET_DIX_GUARD_CRC = 1,
> + TARGET_DIX_GUARD_IP = 2,
> +};
> +
> +enum target_pi_error {
> + TARGET_GUARD_CHECK_FAILED = 0x1,
> + TARGET_APPTAG_CHECK_FAILED = 0x2,
> + TARGET_REFTAG_CHECK_FAILED = 0x3,
> +};
> +
> +struct se_dif_v1_tuple {
> + __be16 guard_tag;
> + __be16 app_tag;
> + __be32 ref_tag;
> +};
> +
> struct se_cmd {
> /* SAM response code being sent to initiator */
> u8 scsi_status;
> @@ -498,6 +538,20 @@ struct se_cmd {
>
> /* Used for lun->lun_ref counting */
> bool lun_ref_active;
> +
> + /* DIF related members */
> + enum target_prot_op prot_op;
> + enum target_prot_type prot_type;
> + enum target_guard_type bg_type;
> + u16 bg_seed;
> + u16 reftag_seed;
> + u32 apptag_seed;
> + u32 prot_length;
> + struct scatterlist *t_prot_sg;
> + unsigned int t_prot_nents;
> + bool prot_interleaved;
> + enum target_pi_error pi_err;
> + u32 block_num;
> };
Some of these guys are unreferenced... I figured these should provide
necessary info both for the transport and the backstores.
Regarding prot_interleaved, I don't remember if we agreed to allow
backstores to store the protection interleaved with the data, which
seems to make some sense in pSCSI.
Anyway, I added this flag simply because some transports support it
(iSER) and it might be useful to avoid de-interleaving + re-interleaving
the buffers.
In my toy example I modified fileio to support both storing
data+protection interleaved and storing protection in a seperate
file.protection
(without verify - left it for the transport) and let the user choose via
configfs (protection_handover).
I think we should hear more opinions here.
>
> struct se_ua {
> @@ -609,6 +663,9 @@ struct se_dev_attrib {
> int emulate_tpws;
> int emulate_caw;
> int emulate_3pc;
> + enum target_prot_type pi_prot_type;
> + enum target_prot_version pi_prot_version;
> + enum target_guard_type pi_guard_type;
> int enforce_pr_isids;
> int is_nonrot;
> int emulate_rest_reord;
> @@ -739,6 +796,8 @@ struct se_device {
> /* Linked list for struct se_hba struct se_device list */
> struct list_head dev_list;
> struct se_lun xcopy_lun;
> + /* Protection Information */
> + int prot_length;
> };
>
> struct se_hba {
On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> This patch adds support for exposing DIF protection device
> attributes via configfs. This includes:
>
> pi_prot_type: Protection Type (0, 1, 3 currently support)
> pi_prot_version: Protection Version (DIF v1 currently supported)
> pi_guard_type: Guard Type (1=DIF CRC, 2=IP CRC)
>
> Within se_dev_set_pi_prot_type() it also adds the se_subsystem_api
> device callbacks to setup per device protection information.
Suggestion, expose pi_prot_format and call transport->init_prot() there.
It is more explicit
and this routine should be called upon getting FORMAT_UNIT as well.
> Cc: Martin K. Petersen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: Or Gerlitz <[email protected]>
> Signed-off-by: Nicholas Bellinger <[email protected]>
> ---
> drivers/target/target_core_configfs.c | 12 ++++++
> drivers/target/target_core_device.c | 65 +++++++++++++++++++++++++++++++++
> drivers/target/target_core_internal.h | 2 +
> 3 files changed, 79 insertions(+)
>
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index 272755d..0f1101c 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -643,6 +643,15 @@ SE_DEV_ATTR(emulate_caw, S_IRUGO | S_IWUSR);
> DEF_DEV_ATTRIB(emulate_3pc);
> SE_DEV_ATTR(emulate_3pc, S_IRUGO | S_IWUSR);
>
> +DEF_DEV_ATTRIB(pi_prot_type);
> +SE_DEV_ATTR(pi_prot_type, S_IRUGO | S_IWUSR);
> +
> +DEF_DEV_ATTRIB_RO(pi_prot_version);
> +SE_DEV_ATTR_RO(pi_prot_version);
> +
> +DEF_DEV_ATTRIB(pi_guard_type);
> +SE_DEV_ATTR(pi_guard_type, S_IRUGO | S_IWUSR);
> +
> DEF_DEV_ATTRIB(enforce_pr_isids);
> SE_DEV_ATTR(enforce_pr_isids, S_IRUGO | S_IWUSR);
>
> @@ -702,6 +711,9 @@ static struct configfs_attribute *target_core_dev_attrib_attrs[] = {
> &target_core_dev_attrib_emulate_tpws.attr,
> &target_core_dev_attrib_emulate_caw.attr,
> &target_core_dev_attrib_emulate_3pc.attr,
> + &target_core_dev_attrib_pi_prot_type.attr,
> + &target_core_dev_attrib_pi_prot_version.attr,
> + &target_core_dev_attrib_pi_guard_type.attr,
> &target_core_dev_attrib_enforce_pr_isids.attr,
> &target_core_dev_attrib_is_nonrot.attr,
> &target_core_dev_attrib_emulate_rest_reord.attr,
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index 207b340..2b59beb 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -918,6 +918,67 @@ int se_dev_set_emulate_3pc(struct se_device *dev, int flag)
> return 0;
> }
>
> +int se_dev_set_pi_prot_type(struct se_device *dev, int flag)
> +{
> + int rc, old_prot = dev->dev_attrib.pi_prot_type;
> +
> + if (flag != 0 && flag != 1 && flag != 2 && flag != 3) {
> + pr_err("Illegal value %d for pi_prot_type\n", flag);
> + return -EINVAL;
> + }
> + if (flag == 2) {
> + pr_err("DIF TYPE2 protection currently not supported\n");
> + return -ENOSYS;
> + }
> + if (!dev->transport->init_prot || !dev->transport->free_prot) {
> + pr_err("DIF protection not supported by backend: %s\n",
> + dev->transport->name);
> + return -ENOSYS;
> + }
> + if (!(dev->dev_flags & DF_CONFIGURED)) {
> + pr_err("DIF protection requires device to be configured\n");
> + return -ENODEV;
> + }
> + if (dev->export_count) {
> + pr_err("dev[%p]: Unable to change SE Device PROT type while"
> + " export_count is %d\n", dev, dev->export_count);
> + return -EINVAL;
> + }
> +
> + dev->dev_attrib.pi_prot_type = flag;
> +
> + if (flag && !old_prot) {
> + rc = dev->transport->init_prot(dev);
> + if (rc) {
> + dev->dev_attrib.pi_prot_type = old_prot;
> + return rc;
> + }
> + } else if (!flag && old_prot) {
> + dev->transport->free_prot(dev);
> + }
> + pr_debug("dev[%p]: SE Device Protection Type: %d\n", dev, flag);
> +
> + return 0;
> +}
> +
> +int se_dev_set_pi_guard_type(struct se_device *dev, int flag)
> +{
> + if (flag != 1 && flag != 2) {
> + pr_err("Illegal value %d for pi_guard_type\n", flag);
> + return -EINVAL;
> + }
> + if (dev->export_count) {
> + pr_err("dev[%p]: Unable to change SE Device GUARD type while"
> + " export_count is %d\n", dev, dev->export_count);
> + return -EINVAL;
> + }
> +
> + dev->dev_attrib.pi_guard_type = flag;
> + pr_debug("dev[%p]: SE Device Guard Type: %d\n", dev, flag);
> +
> + return 0;
> +}
> +
> int se_dev_set_enforce_pr_isids(struct se_device *dev, int flag)
> {
> if ((flag != 0) && (flag != 1)) {
> @@ -1415,6 +1476,7 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
> dev->dev_link_magic = SE_DEV_LINK_MAGIC;
> dev->se_hba = hba;
> dev->transport = hba->transport;
> + dev->prot_length = sizeof(struct se_dif_v1_tuple);
>
> INIT_LIST_HEAD(&dev->dev_list);
> INIT_LIST_HEAD(&dev->dev_sep_list);
> @@ -1455,6 +1517,9 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
> dev->dev_attrib.emulate_tpws = DA_EMULATE_TPWS;
> dev->dev_attrib.emulate_caw = DA_EMULATE_CAW;
> dev->dev_attrib.emulate_3pc = DA_EMULATE_3PC;
> + dev->dev_attrib.pi_prot_type = TARGET_DIF_TYPE0_PROT;
> + dev->dev_attrib.pi_prot_version = TARGET_DIF_V1;
> + dev->dev_attrib.pi_guard_type = TARGET_DIX_GUARD_CRC;
> dev->dev_attrib.enforce_pr_isids = DA_ENFORCE_PR_ISIDS;
> dev->dev_attrib.is_nonrot = DA_IS_NONROT;
> dev->dev_attrib.emulate_rest_reord = DA_EMULATE_REST_REORD;
> diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
> index 47b63b0..79d3bc3 100644
> --- a/drivers/target/target_core_internal.h
> +++ b/drivers/target/target_core_internal.h
> @@ -35,6 +35,8 @@ int se_dev_set_emulate_tpu(struct se_device *, int);
> int se_dev_set_emulate_tpws(struct se_device *, int);
> int se_dev_set_emulate_caw(struct se_device *, int);
> int se_dev_set_emulate_3pc(struct se_device *, int);
> +int se_dev_set_pi_prot_type(struct se_device *, int);
> +int se_dev_set_pi_guard_type(struct se_device *, int);
> int se_dev_set_enforce_pr_isids(struct se_device *, int);
> int se_dev_set_is_nonrot(struct se_device *, int);
> int se_dev_set_emulate_rest_reord(struct se_device *dev, int);
On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> This patch adds sbc_check_prot() for performing various DIF
> related CDB sanity checks, along with setting SCF_PROT once
> sanity checks have passed.
>
> Also, add calls in sbc_parse_cdb() for READ_[10,12,16] +
> WRITE_[10,12,16] to perform DIF sanity checking.
>
> Cc: Martin K. Petersen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Sagi Grimberg <[email protected]>
> Cc: Or Gerlitz <[email protected]>
> Signed-off-by: Nicholas Bellinger <[email protected]>
> ---
> drivers/target/target_core_sbc.c | 39 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 52ae54e..600ffcb 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -563,6 +563,27 @@ sbc_compare_and_write(struct se_cmd *cmd)
> return TCM_NO_SENSE;
> }
>
> +bool
> +sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
> +{
> + if (!dev->dev_attrib.pi_prot_type)
> + return true;
> +
> + if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE2_PROT &&
> + (cdb[1] & 0xe0))
> + return false;
> +
> + if (!(cdb[1] & 0xe0)) {
> + pr_warn("Target: Unprotected READ/WRITE to DIF device\n");
> + return true;
> + }
> + if (!cmd->t_prot_sg || !cmd->t_prot_nents)
> + return true;
> +
> + cmd->se_cmd_flags |= SCF_PROT;
Isn't this the place to fill the se_cmd DIF execution parameters?
prot_op, prot_type, guard_type, initial_reftag, apptag etc...
Next, all parties interested in DIF execution should look in se_cmd
(backstore, transport).
> + return true;
> +}
> +
> sense_reason_t
> sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> {
> @@ -581,6 +602,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> cmd->execute_cmd = sbc_execute_rw;
> break;
> case READ_10:
> + if (!sbc_check_prot(dev, cmd, cdb))
> + return TCM_UNSUPPORTED_SCSI_OPCODE;
> +
> sectors = transport_get_sectors_10(cdb);
> cmd->t_task_lba = transport_lba_32(cdb);
> cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
> @@ -588,6 +612,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> cmd->execute_cmd = sbc_execute_rw;
> break;
> case READ_12:
> + if (!sbc_check_prot(dev, cmd, cdb))
> + return TCM_UNSUPPORTED_SCSI_OPCODE;
> +
> sectors = transport_get_sectors_12(cdb);
> cmd->t_task_lba = transport_lba_32(cdb);
> cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
> @@ -595,6 +622,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> cmd->execute_cmd = sbc_execute_rw;
> break;
> case READ_16:
> + if (!sbc_check_prot(dev, cmd, cdb))
> + return TCM_UNSUPPORTED_SCSI_OPCODE;
> +
> sectors = transport_get_sectors_16(cdb);
> cmd->t_task_lba = transport_lba_64(cdb);
> cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
> @@ -610,6 +640,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> break;
> case WRITE_10:
> case WRITE_VERIFY:
> + if (!sbc_check_prot(dev, cmd, cdb))
> + return TCM_UNSUPPORTED_SCSI_OPCODE;
> +
> sectors = transport_get_sectors_10(cdb);
> cmd->t_task_lba = transport_lba_32(cdb);
> if (cdb[1] & 0x8)
> @@ -619,6 +652,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> cmd->execute_cmd = sbc_execute_rw;
> break;
> case WRITE_12:
> + if (!sbc_check_prot(dev, cmd, cdb))
> + return TCM_UNSUPPORTED_SCSI_OPCODE;
> +
> sectors = transport_get_sectors_12(cdb);
> cmd->t_task_lba = transport_lba_32(cdb);
> if (cdb[1] & 0x8)
> @@ -628,6 +664,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
> cmd->execute_cmd = sbc_execute_rw;
> break;
> case WRITE_16:
> + if (!sbc_check_prot(dev, cmd, cdb))
> + return TCM_UNSUPPORTED_SCSI_OPCODE;
> +
> sectors = transport_get_sectors_16(cdb);
> cmd->t_task_lba = transport_lba_64(cdb);
> if (cdb[1] & 0x8)
>>>>> "Nic" == Nicholas A Bellinger <[email protected]> writes:
Nic> This series contains initial support for target mode DIF
Nic> Type1+Type3 emulation within target core, RAMDISK_MCP device
Nic> backend, and tcm_loop fabric driver.
Super cool! Do you have a git tree so I can start tinkering with this?
I'll start reviewing the pieces tomorrow.
--
Martin K. Petersen Oracle Linux Engineering
On Thu, 2014-01-09 at 21:00 -0500, Martin K. Petersen wrote:
> >>>>> "Nic" == Nicholas A Bellinger <[email protected]> writes:
>
> Nic> This series contains initial support for target mode DIF
> Nic> Type1+Type3 emulation within target core, RAMDISK_MCP device
> Nic> backend, and tcm_loop fabric driver.
>
> Super cool! Do you have a git tree so I can start tinkering with this?
Most certainly, it's been pushed to target-pending/auto-next.
>
> I'll start reviewing the pieces tomorrow.
Excellent.
Thanks MKP!
--nab
On Thu, 2014-01-09 at 12:24 +0200, Sagi Grimberg wrote:
> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[email protected]>
> >
> > This patch updates sbc_emulate_readcapacity_16() to set
> > P_TYPE and PROT_EN bits when DIF emulation is enabled by
> > the backend device.
> >
> > Cc: Martin K. Petersen <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Hannes Reinecke <[email protected]>
> > Cc: Sagi Grimberg <[email protected]>
> > Cc: Or Gerlitz <[email protected]>
> > Signed-off-by: Nicholas Bellinger <[email protected]>
> > ---
> > drivers/target/target_core_sbc.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index 366b9bb..22599e8 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -106,6 +106,11 @@ sbc_emulate_readcapacity_16(struct se_cmd *cmd)
> > buf[9] = (dev->dev_attrib.block_size >> 16) & 0xff;
> > buf[10] = (dev->dev_attrib.block_size >> 8) & 0xff;
> > buf[11] = dev->dev_attrib.block_size & 0xff;
> > + /*
> > + * Set P_TYPE and PROT_EN bits for DIF support
> > + */
> > + if (dev->dev_attrib.pi_prot_type)
> > + buf[12] = (dev->dev_attrib.pi_prot_type - 1) << 1 | 0x1;
> >
> > if (dev->transport->get_lbppbe)
> > buf[13] = dev->transport->get_lbppbe(dev) & 0x0f;
>
> Hey Nic,
>
> What about FORMAT_UNIT emulation?
Would certainly be useful to have..
> The backstore protection configuration is done at the target side via
> configfs/targetcli, if you publish DIF support in
> INQUERY_EVPD/READ_CAPACITY you need to accept protection information format?
Mmmm, these two bits bits are following what scsi_debug is currently
exposing minus FORMAT_UNIT support..?
MKP..?
--nab
On Thu, 2014-01-09 at 12:32 +0200, Sagi Grimberg wrote:
> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[email protected]>
> >
> > This patch adds support for DIF protection into rd_execute_rw() code
> > for WRITE/READ I/O using sbc_dif_verify_[write,read]() logic.
> >
> > It also adds rd_get_prot_table() for locating protection SGLs
> > assoicated with the ramdisk backend device.
> >
> > Cc: Martin K. Petersen <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Hannes Reinecke <[email protected]>
> > Cc: Sagi Grimberg <[email protected]>
> > Cc: Or Gerlitz <[email protected]>
> > Signed-off-by: Nicholas Bellinger <[email protected]>
> > ---
> > drivers/target/target_core_rd.c | 67 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 67 insertions(+)
> >
> > diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
> > index dd99844..3fd51eb 100644
> > --- a/drivers/target/target_core_rd.c
> > +++ b/drivers/target/target_core_rd.c
> > @@ -363,6 +363,26 @@ static struct rd_dev_sg_table *rd_get_sg_table(struct rd_dev *rd_dev, u32 page)
> > return NULL;
> > }
> >
> > +static struct rd_dev_sg_table *rd_get_prot_table(struct rd_dev *rd_dev, u32 page)
> > +{
> > + struct rd_dev_sg_table *sg_table;
> > + u32 i, sg_per_table = (RD_MAX_ALLOCATION_SIZE /
> > + sizeof(struct scatterlist));
> > +
> > + i = page / sg_per_table;
> > + if (i < rd_dev->sg_prot_count) {
> > + sg_table = &rd_dev->sg_prot_array[i];
> > + if ((sg_table->page_start_offset <= page) &&
> > + (sg_table->page_end_offset >= page))
> > + return sg_table;
> > + }
> > +
> > + pr_err("Unable to locate struct prot rd_dev_sg_table for page: %u\n",
> > + page);
> > +
> > + return NULL;
> > +}
> > +
> > static sense_reason_t
> > rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
> > enum dma_data_direction data_direction)
> > @@ -377,6 +397,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
> > u32 rd_page;
> > u32 src_len;
> > u64 tmp;
> > + sense_reason_t rc;
> >
> > if (dev->rd_flags & RDF_NULLIO) {
> > target_complete_cmd(cmd, SAM_STAT_GOOD);
> > @@ -399,6 +420,29 @@ 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->se_cmd_flags & SCF_PROT) && se_dev->dev_attrib.pi_prot_type &&
> > + data_direction == DMA_TO_DEVICE) {
> > + sector_t sector = cmd->data_length / se_dev->dev_attrib.block_size;
> > + struct rd_dev_sg_table *prot_table;
> > + struct scatterlist *prot_sg;
> > + u32 prot_offset, prot_page;
> > +
> > + tmp = cmd->t_task_lba * se_dev->prot_length;
> > + prot_offset = do_div(tmp, PAGE_SIZE);
> > + prot_page = tmp;
> > +
> > + prot_table = rd_get_prot_table(dev, prot_page);
> > + if (!prot_table)
> > + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> > +
> > + prot_sg = &prot_table->sg_table[prot_page - prot_table->page_start_offset];
> > +
> > + rc = sbc_dif_verify_write(cmd, cmd->t_task_lba, sector, 0,
> > + prot_sg, prot_offset);
> > + if (rc)
> > + return rc;
> > + }
> > +
> > src_len = PAGE_SIZE - rd_offset;
> > sg_miter_start(&m, sgl, sgl_nents,
> > data_direction == DMA_FROM_DEVICE ?
> > @@ -460,6 +504,29 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
> > }
> > sg_miter_stop(&m);
> >
> > + if ((cmd->se_cmd_flags & SCF_PROT) && se_dev->dev_attrib.pi_prot_type &&
> > + data_direction == DMA_FROM_DEVICE) {
> > + sector_t sector = cmd->data_length / se_dev->dev_attrib.block_size;
> > + struct rd_dev_sg_table *prot_table;
> > + struct scatterlist *prot_sg;
> > + u32 prot_offset, prot_page;
> > +
> > + tmp = cmd->t_task_lba * se_dev->prot_length;
> > + prot_offset = do_div(tmp, PAGE_SIZE);
> > + prot_page = tmp;
> > +
> > + prot_table = rd_get_prot_table(dev, prot_page);
> > + if (!prot_table)
> > + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> > +
> > + prot_sg = &prot_table->sg_table[prot_page - prot_table->page_start_offset];
> > +
> > + rc = sbc_dif_verify_read(cmd, cmd->t_task_lba, sector, 0,
> > + prot_sg, prot_offset);
> > + if (rc)
> > + return rc;
> > + }
> > +
> > target_complete_cmd(cmd, SAM_STAT_GOOD);
> > return 0;
> > }
>
> I wander how we can skip sbc_dif_verify_xxxx if the transport already
> offloaded DIF verify.
> I think that the transport should signal the core layer that it is able
> to offload DIF (ADD/STRIP/PASS/VERIFY), in which case the core should
> turn off the backstore DIF verify emulation to sustain performance.
So IBLOCK + PSCSI backends will need to be a non interleaved protection
PASS for fast path operation, and backend protection emulation is
reserved for RAMDISK and perhaps a special FILEIO full emulation mode.
> I assume that if backstore DIF verify is offloaded as well it does not
> really matter.
That is my assumption of well..
--nab
On Thu, 2014-01-09 at 12:43 +0200, Sagi Grimberg wrote:
> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[email protected]>
> >
> > This patch adds support for DIF related CHECK_CONDITION ASC/ASCQ
> > exception cases into transport_send_check_condition_and_sense().
> >
> > This includes:
> >
> > LOGICAL BLOCK GUARD CHECK FAILED
> > LOGICAL BLOCK APPLICATION TAG CHECK FAILED
> > LOGICAL BLOCK REFERENCE TAG CHECK FAILED
> >
> > that used by DIF TYPE1 and TYPE3 failure cases.
> >
> > Cc: Martin K. Petersen <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Hannes Reinecke <[email protected]>
> > Cc: Sagi Grimberg <[email protected]>
> > Cc: Or Gerlitz <[email protected]>
> > Signed-off-by: Nicholas Bellinger <[email protected]>
> > ---
> > drivers/target/target_core_transport.c | 30 ++++++++++++++++++++++++++++++
> > include/target/target_core_base.h | 3 +++
> > 2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 91953da..707ee17 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -2648,6 +2648,36 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
> > buffer[SPC_ASC_KEY_OFFSET] = 0x1d;
> > buffer[SPC_ASCQ_KEY_OFFSET] = 0x00;
> > break;
> > + case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED:
> > + /* CURRENT ERROR */
> > + buffer[0] = 0x70;
> > + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
> > + /* ILLEGAL REQUEST */
> > + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
> > + /* LOGICAL BLOCK GUARD CHECK FAILED */
> > + buffer[SPC_ASC_KEY_OFFSET] = 0x10;
> > + buffer[SPC_ASCQ_KEY_OFFSET] = 0x01;
>
> You have Enums for ASCQs (TARGET_GUARD_CHECK_FAILED,
> TARGET_APPTAG_CHECK_FAILED, TARGET_REFTAG_CHECK_FAILED). either use them
> or loose them.
<nod>, dropping these extra definitions.
--nab
On Thu, 2014-01-09 at 13:01 +0200, Sagi Grimberg wrote:
> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[email protected]>
> >
> > This patch adds support for exposing DIF protection device
> > attributes via configfs. This includes:
> >
> > pi_prot_type: Protection Type (0, 1, 3 currently support)
> > pi_prot_version: Protection Version (DIF v1 currently supported)
> > pi_guard_type: Guard Type (1=DIF CRC, 2=IP CRC)
> >
> > Within se_dev_set_pi_prot_type() it also adds the se_subsystem_api
> > device callbacks to setup per device protection information.
>
> Suggestion, expose pi_prot_format and call transport->init_prot() there.
> It is more explicit
> and this routine should be called upon getting FORMAT_UNIT as well.
<nod>, working on this next for FILEIO following your original example
code.
Thanks Sagi!
--nab
On Thu, 2014-01-09 at 16:58 +0200, Sagi Grimberg wrote:
> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[email protected]>
> >
> > This patch adds sbc_check_prot() for performing various DIF
> > related CDB sanity checks, along with setting SCF_PROT once
> > sanity checks have passed.
> >
> > Also, add calls in sbc_parse_cdb() for READ_[10,12,16] +
> > WRITE_[10,12,16] to perform DIF sanity checking.
> >
> > Cc: Martin K. Petersen <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Hannes Reinecke <[email protected]>
> > Cc: Sagi Grimberg <[email protected]>
> > Cc: Or Gerlitz <[email protected]>
> > Signed-off-by: Nicholas Bellinger <[email protected]>
> > ---
> > drivers/target/target_core_sbc.c | 39 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 39 insertions(+)
> >
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index 52ae54e..600ffcb 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -563,6 +563,27 @@ sbc_compare_and_write(struct se_cmd *cmd)
> > return TCM_NO_SENSE;
> > }
> >
> > +bool
> > +sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
> > +{
> > + if (!dev->dev_attrib.pi_prot_type)
> > + return true;
> > +
> > + if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE2_PROT &&
> > + (cdb[1] & 0xe0))
> > + return false;
> > +
> > + if (!(cdb[1] & 0xe0)) {
> > + pr_warn("Target: Unprotected READ/WRITE to DIF device\n");
> > + return true;
> > + }
> > + if (!cmd->t_prot_sg || !cmd->t_prot_nents)
> > + return true;
> > +
> > + cmd->se_cmd_flags |= SCF_PROT;
>
> Isn't this the place to fill the se_cmd DIF execution parameters?
> prot_op, prot_type, guard_type, initial_reftag, apptag etc...
> Next, all parties interested in DIF execution should look in se_cmd
> (backstore, transport).
Yes, working on this for -v2 as well. :)
--nab
On 01/09/2014 10:21 PM, Nicholas A. Bellinger wrote:
>> What about FORMAT_UNIT emulation?
>
> Would certainly be useful to have..
>
>> The backstore protection configuration is done at the target side via
>> configfs/targetcli, if you publish DIF support in
>> INQUERY_EVPD/READ_CAPACITY you need to accept protection information format?
>
> Mmmm, these two bits bits are following what scsi_debug is currently
> exposing minus FORMAT_UNIT support..?
>
> MKP..?
Yes, don't you need FORMAT UNIT because protection information is going
to mean the pi-enabled lun will need to report less blocks? The ramdisk
backstore changes in this series allocate extra space for PI info, but
my understanding was that especially for emulation with block and fileio
backstores, everything needs to go in the same amount of space.
Furthermore, if we want PI info stored along with the blocks, then block
and fileio backstore formats are no longer going to be 1:1 -- requiring
offset calculations, non-aligned read-modify-write, and all that
unpleasantness to be handled?
I've looked at the patchset and the code looks good to me, so I guess
I'm trying to understand the scope of future work needed on the
backstore side for this support to be functional.
Regards -- Andy
On Fri, 2014-01-10 at 11:50 -0800, Andy Grover wrote:
> On 01/09/2014 10:21 PM, Nicholas A. Bellinger wrote:
> >> What about FORMAT_UNIT emulation?
> >
> > Would certainly be useful to have..
> >
> >> The backstore protection configuration is done at the target side via
> >> configfs/targetcli, if you publish DIF support in
> >> INQUERY_EVPD/READ_CAPACITY you need to accept protection information format?
> >
> > Mmmm, these two bits bits are following what scsi_debug is currently
> > exposing minus FORMAT_UNIT support..?
> >
> > MKP..?
>
> Yes, don't you need FORMAT UNIT because protection information is going
> to mean the pi-enabled lun will need to report less blocks?
FORMAT_UNIT is simply a mechanism that allows the client to setup the
protection information remotely, to complement the per device configfs
attribute that does the same thing from the target side.
> The ramdisk backstore changes in this series allocate extra space for
> PI info, but my understanding was that especially for emulation with
> block and fileio backstores, everything needs to go in the same amount
> of space.
>
No, that's only for the interleaved case.
> Furthermore, if we want PI info stored along with the blocks, then block
> and fileio backstore formats are no longer going to be 1:1 -- requiring
> offset calculations, non-aligned read-modify-write, and all that
> unpleasantness to be handled?
>
I'm currently not intending to support interleaved mode into the
backend, given that backends not doing emulation expect these to be in
seperate SGLs to start.
--nab
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 52ae54e..600ffcb 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -563,6 +563,27 @@ sbc_compare_and_write(struct se_cmd *cmd)
return TCM_NO_SENSE;
}
+bool
+sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
+{
+ if (!dev->dev_attrib.pi_prot_type)
+ return true;
+
+ if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE2_PROT &&
+ (cdb[1] & 0xe0))
+ return false;
+
+ if (!(cdb[1] & 0xe0)) {
+ pr_warn("Target: Unprotected READ/WRITE to DIF device\n");
+ return true;
+ }
You may want to remove this warning. I left it in scsi_debug upstream
for the convenience of tinkerers but quiesce it in my own tree.
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> This patch updates spc_emulate_inquiry_std() to set the PROTECT bit
nab> when DIF emulation is enabled by the backend device.
Technically, PROTECT only indicates whether a device is *capable* of
storing PI. But in our case I guess that's OK. It doesn't make much
sense to create a PI LUN and then not have PI enabled.
Reviewed-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> This patch updates spc_emulate_evpd_86() (extended INQUIRY) to
nab> report GRD_CHK (Guard Check) and REF_CHK (Reference Check) bits
nab> when DIF emulation is enabled by the backend device.
Reviewed-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> This patch updates sbc_emulate_readcapacity_16() to set P_TYPE and
nab> PROT_EN bits when DIF emulation is enabled by the backend device.
Reviewed-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
Sagi> What about FORMAT_UNIT emulation? The backstore protection
Sagi> configuration is done at the target side via configfs/targetcli,
I don't know of any non-disk devices that actually implement FORMAT
UNIT. Usually such configuration is done using the array management
interface.
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
>> The backstore protection configuration is done at the target side via
>> configfs/targetcli, if you publish DIF support in
>> INQUERY_EVPD/READ_CAPACITY you need to accept protection information
>> format?
nab> Mmmm, these two bits bits are following what scsi_debug is
nab> currently exposing minus FORMAT_UNIT support..?
PROT_EN and P_TYPE should be fine for now.
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "Andy" == Andy Grover <[email protected]> writes:
Andy> Yes, don't you need FORMAT UNIT because protection information is
Andy> going to mean the pi-enabled lun will need to report less blocks?
Modern disk drives won't shrink when you reformat them with PI. This is
a result of an IDEMA agreement about LBA counts.
And if you create a 10GB PI LUN on an array you'll get 10GB for data.
Andy> The ramdisk backstore changes in this series allocate extra space
Andy> for PI info, but my understanding was that especially for
Andy> emulation with block and fileio backstores, everything needs to go
Andy> in the same amount of space.
For both file and block I'd recommend we store the PI in a separate
block device or file unless the backing device is PI-capable.
Andy> Furthermore, if we want PI info stored along with the blocks, then
Andy> block and fileio backstore formats are no longer going to be 1:1
Andy> -- requiring offset calculations, non-aligned read-modify-write,
Andy> and all that unpleasantness to be handled?
I only think interleaved makes sense if you're passing the PI through
instead of emulating.
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> This patch updates spc_modesense_control() to set the Application
nab> Tag Owner (ATO) bit when when DIF emulation is enabled by the
nab> backend device.
Reviewed-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> This patch adds support for exposing DIF protection device
nab> attributes via configfs. This includes:
nab> pi_prot_type: Protection Type (0, 1, 3 currently support)
nab> pi_prot_version: Protection Version (DIF v1 currently supported)
What's DIF v2?
nab> pi_guard_type: Guard Type (1=DIF CRC, 2=IP CRC)
The IP checksum is only supported by DIX between OS and initiator, not
by the target. I guess we could signal to the initiator via a
vendor-private VPD that IP checksum is supported directly. But now what
we have hardware-accelerated T10 CRC I don't think it's a big deal.
(scsi_debug supports IP checksum because it's both initiator and
target).
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
Sagi> I wander how we can skip sbc_dif_verify_xxxx if the transport
Sagi> already offloaded DIF verify. I think that the transport should
Sagi> signal the core layer that it is able to offload DIF
Sagi> (ADD/STRIP/PASS/VERIFY), in which case the core should turn off
Sagi> the backstore DIF verify emulation to sustain performance.
Yeah, for SAS and FC it would be nice to leverage DIX and let the ASIC
do the actual checking and splitting. I assume the same is true for your
hw.
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> This patch updates tcm_loop_driver_probe() to set protection
nab> information using scsi_host_set_prot() and scsi_host_set_guard(),
nab> which currently enabled all modes of DIF/DIX protection, minus DIF
nab> TYPE0.
ITYM, DIX Type 0. And ideally you'd have a way to choose between IP and
T10 CRC for the guard type.
Otherwise OK.
Reviewed-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
On 1/10/2014 10:46 PM, Martin K. Petersen wrote:
>>>>>> "Andy" == Andy Grover <[email protected]> writes:
> Andy> Yes, don't you need FORMAT UNIT because protection information is
> Andy> going to mean the pi-enabled lun will need to report less blocks?
>
> Modern disk drives won't shrink when you reformat them with PI. This is
> a result of an IDEMA agreement about LBA counts.
>
> And if you create a 10GB PI LUN on an array you'll get 10GB for data.
>
> Andy> The ramdisk backstore changes in this series allocate extra space
> Andy> for PI info, but my understanding was that especially for
> Andy> emulation with block and fileio backstores, everything needs to go
> Andy> in the same amount of space.
>
> For both file and block I'd recommend we store the PI in a separate
> block device or file unless the backing device is PI-capable.
>
> Andy> Furthermore, if we want PI info stored along with the blocks, then
> Andy> block and fileio backstore formats are no longer going to be 1:1
> Andy> -- requiring offset calculations, non-aligned read-modify-write,
> Andy> and all that unpleasantness to be handled?
>
> I only think interleaved makes sense if you're passing the PI through
> instead of emulating.
>
I agree, I implemented interleaved mode just as a proof of concept that
our HW can perform offload in that manner.
I assume we can stick with non-interleaved, although it can be added as
a user option.
Sagi.
<SNIP>
>> I wander how we can skip sbc_dif_verify_xxxx if the transport already
>> offloaded DIF verify.
>> I think that the transport should signal the core layer that it is able
>> to offload DIF (ADD/STRIP/PASS/VERIFY), in which case the core should
>> turn off the backstore DIF verify emulation to sustain performance.
> So IBLOCK + PSCSI backends will need to be a non interleaved protection
> PASS for fast path operation, and backend protection emulation is
> reserved for RAMDISK and perhaps a special FILEIO full emulation mode.
But can't we avoid that if transport already verified? This will kill
performance.
On 1/10/2014 9:00 AM, Nicholas A. Bellinger wrote:
> On Thu, 2014-01-09 at 13:01 +0200, Sagi Grimberg wrote:
>> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <[email protected]>
>>>
>>> This patch adds support for exposing DIF protection device
>>> attributes via configfs. This includes:
>>>
>>> pi_prot_type: Protection Type (0, 1, 3 currently support)
>>> pi_prot_version: Protection Version (DIF v1 currently supported)
>>> pi_guard_type: Guard Type (1=DIF CRC, 2=IP CRC)
>>>
>>> Within se_dev_set_pi_prot_type() it also adds the se_subsystem_api
>>> device callbacks to setup per device protection information.
>> Suggestion, expose pi_prot_format and call transport->init_prot() there.
>> It is more explicit
>> and this routine should be called upon getting FORMAT_UNIT as well.
> <nod>, working on this next for FILEIO following your original example
> code.
I would suggest to keep user interface the same for all backstores.
Let me send you my latest WIP example for FILEIO that follows the
pi_prot_format notation and you can arrange it as you wish.
> Thanks Sagi!
>
> --nab
>
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/10/2014 9:04 AM, Nicholas A. Bellinger wrote:
> On Thu, 2014-01-09 at 16:58 +0200, Sagi Grimberg wrote:
>> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <[email protected]>
>>>
>>> This patch adds sbc_check_prot() for performing various DIF
>>> related CDB sanity checks, along with setting SCF_PROT once
>>> sanity checks have passed.
>>>
>>> Also, add calls in sbc_parse_cdb() for READ_[10,12,16] +
>>> WRITE_[10,12,16] to perform DIF sanity checking.
>>>
>>> Cc: Martin K. Petersen <[email protected]>
>>> Cc: Christoph Hellwig <[email protected]>
>>> Cc: Hannes Reinecke <[email protected]>
>>> Cc: Sagi Grimberg <[email protected]>
>>> Cc: Or Gerlitz <[email protected]>
>>> Signed-off-by: Nicholas Bellinger <[email protected]>
>>> ---
>>> drivers/target/target_core_sbc.c | 39 ++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
>>> index 52ae54e..600ffcb 100644
>>> --- a/drivers/target/target_core_sbc.c
>>> +++ b/drivers/target/target_core_sbc.c
>>> @@ -563,6 +563,27 @@ sbc_compare_and_write(struct se_cmd *cmd)
>>> return TCM_NO_SENSE;
>>> }
>>>
>>> +bool
>>> +sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
>>> +{
>>> + if (!dev->dev_attrib.pi_prot_type)
>>> + return true;
>>> +
>>> + if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE2_PROT &&
>>> + (cdb[1] & 0xe0))
>>> + return false;
>>> +
>>> + if (!(cdb[1] & 0xe0)) {
>>> + pr_warn("Target: Unprotected READ/WRITE to DIF device\n");
>>> + return true;
>>> + }
>>> + if (!cmd->t_prot_sg || !cmd->t_prot_nents)
>>> + return true;
>>> +
>>> + cmd->se_cmd_flags |= SCF_PROT;
>> Isn't this the place to fill the se_cmd DIF execution parameters?
>> prot_op, prot_type, guard_type, initial_reftag, apptag etc...
>> Next, all parties interested in DIF execution should look in se_cmd
>> (backstore, transport).
> Yes, working on this for -v2 as well. :)
OK, so just to be clear, both the transport and the backstore will be
look in se_cmd protection attributes when executing IO correct?
On 1/10/2014 10:39 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
> Sagi> What about FORMAT_UNIT emulation? The backstore protection
> Sagi> configuration is done at the target side via configfs/targetcli,
>
> I don't know of any non-disk devices that actually implement FORMAT
> UNIT. Usually such configuration is done using the array management
> interface.
>
Hmm,
So this takes me to a corner I still don't understand, if a LUN is
pre-formatted as T10-protected, what happens to unwritten blocks read?
I mean, SCSI login executes some reads from sevel LBAs which will
probably fail as blocks are unwritten.
What is the usage model? perform Initiator login and then format the LUN
on the target node? This is why I thought FORMAT_UNIT should be implemented.
I understand this corner will disappear in DIF v2 (following DIX1.1
draft) with ESCAPE flags.
On 1/10/2014 10:39 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
> Sagi> What about FORMAT_UNIT emulation? The backstore protection
> Sagi> configuration is done at the target side via configfs/targetcli,
>
> I don't know of any non-disk devices that actually implement FORMAT
> UNIT. Usually such configuration is done using the array management
> interface.
>
Hmm,
So this takes me to a corner I still don't understand, if a LUN is
pre-formatted as T10-protected, what happens to unwritten blocks read?
I mean, SCSI login executes some reads from several LBAs which will
probably fail as blocks are unwritten.
What is the usage model? perform Initiator login and then format the LUN
on the target node? This is why I thought FORMAT_UNIT should be implemented.
I understand this corner will disappear in DIF v2 (following DIX1.1
draft) with ESCAPE flags.
<SNIP>
> nab> This patch adds support for exposing DIF protection device
> nab> attributes via configfs. This includes:
>
> nab> pi_prot_type: Protection Type (0, 1, 3 currently support)
> nab> pi_prot_version: Protection Version (DIF v1 currently supported)
>
> What's DIF v2?
>
> nab> pi_guard_type: Guard Type (1=DIF CRC, 2=IP CRC)
>
> The IP checksum is only supported by DIX between OS and initiator, not
> by the target. I guess we could signal to the initiator via a
> vendor-private VPD that IP checksum is supported directly. But now what
> we have hardware-accelerated T10 CRC I don't think it's a big deal.
shouldn't it stick around if it is not deprecated yet, the transport is
required to support ip-csum->CRC conversion anyhow.
> (scsi_debug supports IP checksum because it's both initiator and
> target).
>
On 1/10/2014 11:06 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
> Sagi> I wander how we can skip sbc_dif_verify_xxxx if the transport
> Sagi> already offloaded DIF verify. I think that the transport should
> Sagi> signal the core layer that it is able to offload DIF
> Sagi> (ADD/STRIP/PASS/VERIFY), in which case the core should turn off
> Sagi> the backstore DIF verify emulation to sustain performance.
>
> Yeah, for SAS and FC it would be nice to leverage DIX and let the ASIC
> do the actual checking and splitting. I assume the same is true for your
> hw.
>
Correct! we should avoid a duplicate DIF processing. if the HW supports
offloading DIF processing, target core can shutdown backstore
verfy_read/verify_write methods.
>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
>> I don't know of any non-disk devices that actually implement FORMAT
>> UNIT. Usually such configuration is done using the array management
>> interface.
Sagi> So this takes me to a corner I still don't understand, if a LUN is
Sagi> pre-formatted as T10-protected, what happens to unwritten blocks
Sagi> read? I mean, SCSI login executes some reads from sevel LBAs
Sagi> which will probably fail as blocks are unwritten.
Per SBC, PI must be initialized to 0xffffffffffffffff. Since an app tag
value of 0xffff is an escape, this will prevent both target and
initiator from performing PI-verification when that block is read.
If a block is subsequently written and no PI is sent from the host
(WRPROTECT=0), the target must generate valid PI for each block.
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
>> The IP checksum is only supported by DIX between OS and initiator,
>> not by the target. I guess we could signal to the initiator via a
>> vendor-private VPD that IP checksum is supported directly. But now
>> what we have hardware-accelerated T10 CRC I don't think it's a big
>> deal.
Sagi> shouldn't it stick around if it is not deprecated yet, the
Sagi> transport is required to support ip-csum->CRC conversion anyhow.
SBC mandates that the guard tag on the wire and on the target device be
the T10 CRC. The IP checksum is a DIX-optimization for application-HBA
exchanges. The only place you should support the IP checksum is in the
initiator.
Note that you could conceivably do a T10 CRC to IP checksum conversion
on writes received by the target and store the IP checksum on disk. And
then convert back to T10 CRC when the data is eventually read. But it
makes no sense to do that given that you will have to do the T10 CRC
calculation regardless. Even if the backing store is DIX-capable and
supports the IP checksum.
--
Martin K. Petersen Oracle Linux Engineering
On 1/12/2014 2:33 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
>>> I don't know of any non-disk devices that actually implement FORMAT
>>> UNIT. Usually such configuration is done using the array management
>>> interface.
> Sagi> So this takes me to a corner I still don't understand, if a LUN is
> Sagi> pre-formatted as T10-protected, what happens to unwritten blocks
> Sagi> read? I mean, SCSI login executes some reads from sevel LBAs
> Sagi> which will probably fail as blocks are unwritten.
>
> Per SBC, PI must be initialized to 0xffffffffffffffff. Since an app tag
> value of 0xffff is an escape, this will prevent both target and
> initiator from performing PI-verification when that block is read.
OK, so this is an implicit escape (which will become explicit in
DIX1.1?). So I will open that in DIF RDMA verbs.
> If a block is subsequently written and no PI is sent from the host
> (WRPROTECT=0), the target must generate valid PI for each block.
>
On 1/12/2014 2:43 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
>>> The IP checksum is only supported by DIX between OS and initiator,
>>> not by the target. I guess we could signal to the initiator via a
>>> vendor-private VPD that IP checksum is supported directly. But now
>>> what we have hardware-accelerated T10 CRC I don't think it's a big
>>> deal.
> Sagi> shouldn't it stick around if it is not deprecated yet, the
> Sagi> transport is required to support ip-csum->CRC conversion anyhow.
>
> SBC mandates that the guard tag on the wire and on the target device be
> the T10 CRC. The IP checksum is a DIX-optimization for application-HBA
> exchanges. The only place you should support the IP checksum is in the
> initiator.
Right.
> Note that you could conceivably do a T10 CRC to IP checksum conversion
> on writes received by the target and store the IP checksum on disk. And
> then convert back to T10 CRC when the data is eventually read. But it
> makes no sense to do that given that you will have to do the T10 CRC
> calculation regardless. Even if the backing store is DIX-capable and
> supports the IP checksum.
>
I agree, but for backstore DIF (SW) emulation it will make sense.
>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
>> Per SBC, PI must be initialized to 0xffffffffffffffff. Since an app
>> tag value of 0xffff is an escape, this will prevent both target and
>> initiator from performing PI-verification when that block is read.
Sagi> OK, so this is an implicit escape (which will become explicit in
Sagi> DIX1.1?). So I will open that in DIF RDMA verbs.
DIX 1.0 says:
"If a storage device returns a value of 0xFFFF in the application tag
and the device is formatted with T10 PI Type 1 or 2 protection, the I/O
controller must disable integrity checking for that sector."
In DIX 1.1 it is explicit by way of the DIX_APP_ESCAPE flag.
--
Martin K. Petersen Oracle Linux Engineering
On 14-01-12 07:13 AM, Sagi Grimberg wrote:
> On 1/10/2014 10:39 PM, Martin K. Petersen wrote:
>>>>>>> "Sagi" == Sagi Grimberg <[email protected]> writes:
>> Sagi> What about FORMAT_UNIT emulation? The backstore protection
>> Sagi> configuration is done at the target side via configfs/targetcli,
>>
>> I don't know of any non-disk devices that actually implement FORMAT
>> UNIT. Usually such configuration is done using the array management
>> interface.
>>
>
> Hmm,
>
> So this takes me to a corner I still don't understand, if a LUN is pre-formatted
> as T10-protected, what happens to unwritten blocks read?
> I mean, SCSI login executes some reads from sevel LBAs which will probably fail
> as blocks are unwritten.
Some observations: I haven't seen any disks pre-formatted
with T10-protection, they usually come pre-formatted without
T10-protection. Seeing it can take 10 hours plus to re-format
a big disk with T10-protection, it is not a bad idea though.
After formatting with T10-protection blocks seem to read as
zeros and the PI as all 0xff_s. [The blocks full of zeros may
have been the previous state.] Provisioning might come into
play, if so it makes sense for a FORMAT UNIT to leave all
blocks unmapped. In that case you should check the LBPRZ bit
and hope that it is set (found in READ CAPACITY(16) and the
Logical block provisioning VPD page). Also if provisioning
is in use, the GET LBA STATUS command will tell you if a
block is unmapped (although I don't have any disks that
support that command). So a tentative READ, for example
checking if a disk has a partition table, could be preceded
by a GET LBA STATUS command. IMO, if provisioning is enabled,
LBPRZ=0 then the GET LBA STATUS command should be mandatory.
Otherwise a tentative READ is a lucky dip.
Rob Elliott is trying to push the "feature set" idea used by
the ATA command set, into the SCSI command set. He doesn't
seem to have got much traction at T10 yet. Feature sets seem
to give vendors clearer guidance on what they should implement
to properly support new features. Much pain and bloat is
caused to the Linux SCSI subsystem (and other OSes) by vendors
who half implement new features.
Doug Gilbert
>>>>> "Doug" == Douglas Gilbert <[email protected]> writes:
>> So this takes me to a corner I still don't understand, if a LUN is
>> pre-formatted as T10-protected, what happens to unwritten blocks
>> read? I mean, SCSI login executes some reads from sevel LBAs which
>> will probably fail as blocks are unwritten.
Doug> Some observations: I haven't seen any disks pre-formatted with
Doug> T10-protection, they usually come pre-formatted without
Doug> T10-protection.
Depends where you buy them. All the drives we ship arrive formatted with
T10 PI from the manufacturer.
However, nobody expects you to format a LUN on an array. When you create
a LUN on a PI-capable storage array, T10 PI is a storage management
interface option just like size, RAID level, etc. Upon creation, arrays
zero out newly provisioned LUN blocks and write parity, etc. If PI is
enabled on a LUN, blocks are written with all zeroes in the data block
and all ones in the trailing PI tuple. This is all part of the regular
LUN setup procedure.
In any case. The usage model is that you never format a disk unless you
are a tinkerer and bought a retail SAS drive at Fry's. Drives will be
delivered by your server vendor formatted with PI and ready to go.
If you use a non-disk storage device, whether or not to enable PI is
part of the LUN provisioning procedure (array management console, RAID
or flash controller card config utility, etc.)
Doug> So a tentative READ, for example checking if a disk has a
Doug> partition table, could be preceded by a GET LBA STATUS
Doug> command. IMO, if provisioning is enabled, LBPRZ=0 then the GET LBA
Doug> STATUS command should be mandatory. Otherwise a tentative READ is
Doug> a lucky dip.
It's perfectly valid to do a legacy/unprotected READ from a T10 PI
device. Doesn't matter whether the blocks are unwritten or not.
--
Martin K. Petersen Oracle Linux Engineering
On 14-01-12 12:21 PM, Martin K. Petersen wrote:
>>>>>> "Doug" == Douglas Gilbert <[email protected]> writes:
>
>>> So this takes me to a corner I still don't understand, if a LUN is
>>> pre-formatted as T10-protected, what happens to unwritten blocks
>>> read? I mean, SCSI login executes some reads from sevel LBAs which
>>> will probably fail as blocks are unwritten.
>
> Doug> Some observations: I haven't seen any disks pre-formatted with
> Doug> T10-protection, they usually come pre-formatted without
> Doug> T10-protection.
>
> Depends where you buy them. All the drives we ship arrive formatted with
> T10 PI from the manufacturer.
>
> However, nobody expects you to format a LUN on an array. When you create
> a LUN on a PI-capable storage array, T10 PI is a storage management
> interface option just like size, RAID level, etc. Upon creation, arrays
> zero out newly provisioned LUN blocks and write parity, etc. If PI is
> enabled on a LUN, blocks are written with all zeroes in the data block
> and all ones in the trailing PI tuple. This is all part of the regular
> LUN setup procedure.
>
> In any case. The usage model is that you never format a disk unless you
> are a tinkerer and bought a retail SAS drive at Fry's. Drives will be
> delivered by your server vendor formatted with PI and ready to go.
Only tinkerers would contribute to something like
the scsi_debug driver. After all, the pros could
rely on their T10 compliant vendor equipment :-)
And you are right, I do like to test sg_format
against something real. Perhaps sg_format is the
utility those server vendors use. Another
tinkerer called James Bottomley wrote the original
sg_format code, according to my notes.
> If you use a non-disk storage device, whether or not to enable PI is
> part of the LUN provisioning procedure (array management console, RAID
> or flash controller card config utility, etc.)
>
> Doug> So a tentative READ, for example checking if a disk has a
> Doug> partition table, could be preceded by a GET LBA STATUS
> Doug> command. IMO, if provisioning is enabled, LBPRZ=0 then the GET LBA
> Doug> STATUS command should be mandatory. Otherwise a tentative READ is
> Doug> a lucky dip.
>
> It's perfectly valid to do a legacy/unprotected READ from a T10 PI
> device. Doesn't matter whether the blocks are unwritten or not.
Ah, the current SBC-3 draft (sbc3r36.pdf) does say if one
does a READ from a disk with logical provisioning enabled
and LBPRZ=0 then the block data is "vendor specific" and
the PI, if any, is all 0xff bytes. That last bit was added
in sbc3r34.pdf (and it was "any value" prior to that).
Back to the original question, I don't think Sagi was
asking if it was valid to do a legacy/unprotected READ,
it was what to expect with a protected READ on
unwritten blocks:
> So this takes me to a corner I still don't understand, if
> a LUN is pre-formatted as T10-protected, what happens to
> unwritten blocks read?
So the precise answer is: the PI will be all 0xff bytes,
unless logical provisioning is enabled, LBPRZ=0 and the
device's compliance predates sbc3r34.pdf (November 2012).
Doug Gilbert
> Back to the original question, I don't think Sagi was
> asking if it was valid to do a legacy/unprotected READ,
> it was what to expect with a protected READ on
> unwritten blocks:
>
> > So this takes me to a corner I still don't understand, if
> > a LUN is pre-formatted as T10-protected, what happens to
> > unwritten blocks read?
>
> So the precise answer is: the PI will be all 0xff bytes,
> unless logical provisioning is enabled, LBPRZ=0 and the
> device's compliance predates sbc3r34.pdf (November 2012).
>
> Doug Gilbert
>
>
Thanks Doug,
This answer translates directly into a fix in my T10-PI offload API.
Sagi.
Hey MKP,
On Fri, 2014-01-10 at 16:01 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
>
> nab> This patch adds support for exposing DIF protection device
> nab> attributes via configfs. This includes:
>
> nab> pi_prot_type: Protection Type (0, 1, 3 currently support)
> nab> pi_prot_version: Protection Version (DIF v1 currently supported)
>
> What's DIF v2?
>
This would be the proposed 16-byte protection scheme for SBC4.
> nab> pi_guard_type: Guard Type (1=DIF CRC, 2=IP CRC)
>
> The IP checksum is only supported by DIX between OS and initiator, not
> by the target. I guess we could signal to the initiator via a
> vendor-private VPD that IP checksum is supported directly. But now what
> we have hardware-accelerated T10 CRC I don't think it's a big deal.
>
> (scsi_debug supports IP checksum because it's both initiator and
> target).
>
In that case, dropping the IP checksum related code now..
--nab
On Fri, 2014-01-10 at 16:09 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
>
> nab> This patch updates tcm_loop_driver_probe() to set protection
> nab> information using scsi_host_set_prot() and scsi_host_set_guard(),
> nab> which currently enabled all modes of DIF/DIX protection, minus DIF
> nab> TYPE0.
>
> ITYM, DIX Type 0.
Yes. Fixed up the commit message.
> And ideally you'd have a way to choose between IP and
> T10 CRC for the guard type.
>
Sure, this can be exposed at a tcm_loop configfs attribute that is
accessible after scsi_add_host() has been called, which I assume is OK
to do..
--nab
On Mon, 2014-01-13 at 10:30 -0800, Nicholas A. Bellinger wrote:
> Hey MKP,
>
> On Fri, 2014-01-10 at 16:01 -0500, Martin K. Petersen wrote:
> > >>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
> >
> > nab> This patch adds support for exposing DIF protection device
> > nab> attributes via configfs. This includes:
> >
> > nab> pi_prot_type: Protection Type (0, 1, 3 currently support)
> > nab> pi_prot_version: Protection Version (DIF v1 currently supported)
> >
> > What's DIF v2?
> >
>
> This would be the proposed 16-byte protection scheme for SBC4.
What proposed 16 byte scheme? The only DIF proposals I know for SBC-4
are 13-185R0 and 12-369R0 and that's a couple of new algorithms and
types because we cannot change the 8 byte PI.
James
On Sun, 2014-01-12 at 13:53 +0200, Sagi Grimberg wrote:
> <SNIP>
>
> >> I wander how we can skip sbc_dif_verify_xxxx if the transport already
> >> offloaded DIF verify.
> >> I think that the transport should signal the core layer that it is able
> >> to offload DIF (ADD/STRIP/PASS/VERIFY), in which case the core should
> >> turn off the backstore DIF verify emulation to sustain performance.
> > So IBLOCK + PSCSI backends will need to be a non interleaved protection
> > PASS for fast path operation, and backend protection emulation is
> > reserved for RAMDISK and perhaps a special FILEIO full emulation mode.
>
> But can't we avoid that if transport already verified? This will kill
> performance.
>
Sure, this will end up being configurable per backend if the fabric does
a CHECK + STRIP, or CHECK + PASS.
--nab
On Sun, 2014-01-12 at 13:59 +0200, Sagi Grimberg wrote:
> On 1/10/2014 9:04 AM, Nicholas A. Bellinger wrote:
> > On Thu, 2014-01-09 at 16:58 +0200, Sagi Grimberg wrote:
> >> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
<SNIP>
> >>> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> >>> index 52ae54e..600ffcb 100644
> >>> --- a/drivers/target/target_core_sbc.c
> >>> +++ b/drivers/target/target_core_sbc.c
> >>> @@ -563,6 +563,27 @@ sbc_compare_and_write(struct se_cmd *cmd)
> >>> return TCM_NO_SENSE;
> >>> }
> >>>
> >>> +bool
> >>> +sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
> >>> +{
> >>> + if (!dev->dev_attrib.pi_prot_type)
> >>> + return true;
> >>> +
> >>> + if (dev->dev_attrib.pi_prot_type == TARGET_DIF_TYPE2_PROT &&
> >>> + (cdb[1] & 0xe0))
> >>> + return false;
> >>> +
> >>> + if (!(cdb[1] & 0xe0)) {
> >>> + pr_warn("Target: Unprotected READ/WRITE to DIF device\n");
> >>> + return true;
> >>> + }
> >>> + if (!cmd->t_prot_sg || !cmd->t_prot_nents)
> >>> + return true;
> >>> +
> >>> + cmd->se_cmd_flags |= SCF_PROT;
> >> Isn't this the place to fill the se_cmd DIF execution parameters?
> >> prot_op, prot_type, guard_type, initial_reftag, apptag etc...
> >> Next, all parties interested in DIF execution should look in se_cmd
> >> (backstore, transport).
> > Yes, working on this for -v2 as well. :)
>
> OK, so just to be clear, both the transport and the backstore will be
> look in se_cmd protection attributes when executing IO correct?
>
Correct.
--nab
On Mon, 2014-01-13 at 10:52 -0800, James Bottomley wrote:
> On Mon, 2014-01-13 at 10:30 -0800, Nicholas A. Bellinger wrote:
> > Hey MKP,
> >
> > On Fri, 2014-01-10 at 16:01 -0500, Martin K. Petersen wrote:
> > > >>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
> > >
> > > nab> This patch adds support for exposing DIF protection device
> > > nab> attributes via configfs. This includes:
> > >
> > > nab> pi_prot_type: Protection Type (0, 1, 3 currently support)
> > > nab> pi_prot_version: Protection Version (DIF v1 currently supported)
> > >
> > > What's DIF v2?
> > >
> >
> > This would be the proposed 16-byte protection scheme for SBC4.
>
> What proposed 16 byte scheme? The only DIF proposals I know for SBC-4
> are 13-185R0 and 12-369R0 and that's a couple of new algorithms and
> types because we cannot change the 8 byte PI.
>
Then I'm probably getting the SBC version wrong.. It's the one that
includes using CRC32C for the block guard, and larger space for
reference tag as mentioned by MKP.
--nab
On Mon, 2014-01-13 at 11:27 -0800, Nicholas A. Bellinger wrote:
> On Mon, 2014-01-13 at 10:52 -0800, James Bottomley wrote:
> > On Mon, 2014-01-13 at 10:30 -0800, Nicholas A. Bellinger wrote:
> > > Hey MKP,
> > >
> > > On Fri, 2014-01-10 at 16:01 -0500, Martin K. Petersen wrote:
> > > > >>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
> > > >
> > > > nab> This patch adds support for exposing DIF protection device
> > > > nab> attributes via configfs. This includes:
> > > >
> > > > nab> pi_prot_type: Protection Type (0, 1, 3 currently support)
> > > > nab> pi_prot_version: Protection Version (DIF v1 currently supported)
> > > >
> > > > What's DIF v2?
> > > >
> > >
> > > This would be the proposed 16-byte protection scheme for SBC4.
> >
> > What proposed 16 byte scheme? The only DIF proposals I know for SBC-4
> > are 13-185R0 and 12-369R0 and that's a couple of new algorithms and
> > types because we cannot change the 8 byte PI.
> >
>
> Then I'm probably getting the SBC version wrong.. It's the one that
> includes using CRC32C for the block guard, and larger space for
> reference tag as mentioned by MKP.
Well, this isn't reducing my confusion: I think you're referring to
12-369r0 which proposes to *eliminate* the reference tag (by moving it
into the CRC calculation) and use the recovered 4 bytes to expand the
CRC to CRC32 and add two bytes to the application tag, so they both
become 4 bytes long, but the new PI still occupies only 8 bytes on disk.
Perhaps it's also better to clarify the terminology: The PI is composed
of a Guard field, which is the checksum, an application tag, which is
usable by the application for anything it wants and a reference tag
which is designed to be derived from the on-disk location so it can be
used to detect misplaced writes. In Type 1 the reference tag has to be
the lower 31 bits of the LBA and in type 3 it's application defined. In
all current types, the guard is two bytes and the application tag two
bytes.
James
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> Sure, this can be exposed at a tcm_loop configfs attribute that is
nab> accessible after scsi_add_host() has been called, which I assume is
nab> OK to do..
As long as it's set before we discover devices.
--
Martin K. Petersen Oracle Linux Engineering
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
>> What proposed 16 byte scheme? The only DIF proposals I know for
>> SBC-4 are 13-185R0 and 12-369R0 and that's a couple of new algorithms
>> and types because we cannot change the 8 byte PI.
nab> Then I'm probably getting the SBC version wrong.. It's the one
nab> that includes using CRC32C for the block guard, and larger space
nab> for reference tag as mentioned by MKP.
This is the Type 4 we have been shopping among various vendors. It
predates and is simpler than HP's proposal (which met resistance in T10
and was subsequently dropped). So we revived our original Type 4
proposal which is 16 bytes of protection information per interval
(CRC32C, 48-bit LBA and 6 bytes of app tag). The proposal has been
sitting around for a while waiting for SBC-4 to open.
--
Martin K. Petersen Oracle Linux Engineering
On Mon, 2014-01-13 at 15:19 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
>
> >> What proposed 16 byte scheme? The only DIF proposals I know for
> >> SBC-4 are 13-185R0 and 12-369R0 and that's a couple of new algorithms
> >> and types because we cannot change the 8 byte PI.
>
> nab> Then I'm probably getting the SBC version wrong.. It's the one
> nab> that includes using CRC32C for the block guard, and larger space
> nab> for reference tag as mentioned by MKP.
>
> This is the Type 4 we have been shopping among various vendors. It
> predates and is simpler than HP's proposal (which met resistance in T10
> and was subsequently dropped). So we revived our original Type 4
> proposal which is 16 bytes of protection information per interval
> (CRC32C, 48-bit LBA and 6 bytes of app tag). The proposal has been
> sitting around for a while waiting for SBC-4 to open.
I'm intrigued by this: how do you get the extra space, since I heard all
the drive vendors were adamant that 520 was it for the current
manufacturing processes.
James
>>>>> "James" == James Bottomley <[email protected]> writes:
James> I'm intrigued by this: how do you get the extra space, since I
James> heard all the drive vendors were adamant that 520 was it for the
James> current manufacturing processes.
Well, you've been able to get 528-byte sector drives for a long
time. They are used inside arrays that use both PI and internal
metadata.
In any case Type 4 was never intended for 512-byte sectors. The existing
16-bit T10 CRC is pretty useless for 4096-byte blocks so the proposal
was aimed at 4096+16 (but obviously that tidbit is outside of the T10
spec that doesn't mandate block sizes or accompanying PI types).
--
Martin K. Petersen Oracle Linux Engineering
On 1/10/2014 8:53 AM, Nicholas A. Bellinger wrote:
> On Thu, 2014-01-09 at 12:43 +0200, Sagi Grimberg wrote:
>> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <[email protected]>
>>>
>>> This patch adds support for DIF related CHECK_CONDITION ASC/ASCQ
>>> exception cases into transport_send_check_condition_and_sense().
>>>
>>> This includes:
>>>
>>> LOGICAL BLOCK GUARD CHECK FAILED
>>> LOGICAL BLOCK APPLICATION TAG CHECK FAILED
>>> LOGICAL BLOCK REFERENCE TAG CHECK FAILED
>>>
>>> that used by DIF TYPE1 and TYPE3 failure cases.
>>>
>>> Cc: Martin K. Petersen <[email protected]>
>>> Cc: Christoph Hellwig <[email protected]>
>>> Cc: Hannes Reinecke <[email protected]>
>>> Cc: Sagi Grimberg <[email protected]>
>>> Cc: Or Gerlitz <[email protected]>
>>> Signed-off-by: Nicholas Bellinger <[email protected]>
>>> ---
>>> drivers/target/target_core_transport.c | 30 ++++++++++++++++++++++++++++++
>>> include/target/target_core_base.h | 3 +++
>>> 2 files changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>>> index 91953da..707ee17 100644
>>> --- a/drivers/target/target_core_transport.c
>>> +++ b/drivers/target/target_core_transport.c
>>> @@ -2648,6 +2648,36 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
>>> buffer[SPC_ASC_KEY_OFFSET] = 0x1d;
>>> buffer[SPC_ASCQ_KEY_OFFSET] = 0x00;
>>> break;
>>> + case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED:
>>> + /* CURRENT ERROR */
>>> + buffer[0] = 0x70;
>>> + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
>>> + /* ILLEGAL REQUEST */
>>> + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
>>> + /* LOGICAL BLOCK GUARD CHECK FAILED */
>>> + buffer[SPC_ASC_KEY_OFFSET] = 0x10;
>>> + buffer[SPC_ASCQ_KEY_OFFSET] = 0x01;
>>
Hey Nic,
In my iSER patches I constructed the same sense buffer (call
isert_pi_err_sense_buffer) and called isert_put_rsponse. So I should
call this routine instead correct?
On Tue, 2014-01-14 at 09:44 +0200, Sagi Grimberg wrote:
> On 1/10/2014 8:53 AM, Nicholas A. Bellinger wrote:
> > On Thu, 2014-01-09 at 12:43 +0200, Sagi Grimberg wrote:
> >> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> >>> From: Nicholas Bellinger <[email protected]>
> >>>
> >>> This patch adds support for DIF related CHECK_CONDITION ASC/ASCQ
> >>> exception cases into transport_send_check_condition_and_sense().
> >>>
> >>> This includes:
> >>>
> >>> LOGICAL BLOCK GUARD CHECK FAILED
> >>> LOGICAL BLOCK APPLICATION TAG CHECK FAILED
> >>> LOGICAL BLOCK REFERENCE TAG CHECK FAILED
> >>>
> >>> that used by DIF TYPE1 and TYPE3 failure cases.
> >>>
> >>> Cc: Martin K. Petersen <[email protected]>
> >>> Cc: Christoph Hellwig <[email protected]>
> >>> Cc: Hannes Reinecke <[email protected]>
> >>> Cc: Sagi Grimberg <[email protected]>
> >>> Cc: Or Gerlitz <[email protected]>
> >>> Signed-off-by: Nicholas Bellinger <[email protected]>
> >>> ---
> >>> drivers/target/target_core_transport.c | 30 ++++++++++++++++++++++++++++++
> >>> include/target/target_core_base.h | 3 +++
> >>> 2 files changed, 33 insertions(+)
> >>>
> >>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> >>> index 91953da..707ee17 100644
> >>> --- a/drivers/target/target_core_transport.c
> >>> +++ b/drivers/target/target_core_transport.c
> >>> @@ -2648,6 +2648,36 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
> >>> buffer[SPC_ASC_KEY_OFFSET] = 0x1d;
> >>> buffer[SPC_ASCQ_KEY_OFFSET] = 0x00;
> >>> break;
> >>> + case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED:
> >>> + /* CURRENT ERROR */
> >>> + buffer[0] = 0x70;
> >>> + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
> >>> + /* ILLEGAL REQUEST */
> >>> + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
> >>> + /* LOGICAL BLOCK GUARD CHECK FAILED */
> >>> + buffer[SPC_ASC_KEY_OFFSET] = 0x10;
> >>> + buffer[SPC_ASCQ_KEY_OFFSET] = 0x01;
> >>
>
> Hey Nic,
>
> In my iSER patches I constructed the same sense buffer (call
> isert_pi_err_sense_buffer) and called isert_put_rsponse. So I should
> call this routine instead correct?
Yes, it should be OK to use this for generating CHECK_CONDITION from
fabric protection failures in isert_completion_rdma_write() code after
device->unreg_rdma_mem() has been called.
--nab
On 1/14/2014 10:53 AM, Nicholas A. Bellinger wrote:
> On Tue, 2014-01-14 at 09:44 +0200, Sagi Grimberg wrote:
>> On 1/10/2014 8:53 AM, Nicholas A. Bellinger wrote:
>>> On Thu, 2014-01-09 at 12:43 +0200, Sagi Grimberg wrote:
>>>> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
>>>>> From: Nicholas Bellinger <[email protected]>
>>>>>
>>>>> This patch adds support for DIF related CHECK_CONDITION ASC/ASCQ
>>>>> exception cases into transport_send_check_condition_and_sense().
>>>>>
>>>>> This includes:
>>>>>
>>>>> LOGICAL BLOCK GUARD CHECK FAILED
>>>>> LOGICAL BLOCK APPLICATION TAG CHECK FAILED
>>>>> LOGICAL BLOCK REFERENCE TAG CHECK FAILED
>>>>>
>>>>> that used by DIF TYPE1 and TYPE3 failure cases.
>>>>>
>>>>> Cc: Martin K. Petersen <[email protected]>
>>>>> Cc: Christoph Hellwig <[email protected]>
>>>>> Cc: Hannes Reinecke <[email protected]>
>>>>> Cc: Sagi Grimberg <[email protected]>
>>>>> Cc: Or Gerlitz <[email protected]>
>>>>> Signed-off-by: Nicholas Bellinger <[email protected]>
>>>>> ---
>>>>> drivers/target/target_core_transport.c | 30 ++++++++++++++++++++++++++++++
>>>>> include/target/target_core_base.h | 3 +++
>>>>> 2 files changed, 33 insertions(+)
>>>>>
>>>>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>>>>> index 91953da..707ee17 100644
>>>>> --- a/drivers/target/target_core_transport.c
>>>>> +++ b/drivers/target/target_core_transport.c
>>>>> @@ -2648,6 +2648,36 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
>>>>> buffer[SPC_ASC_KEY_OFFSET] = 0x1d;
>>>>> buffer[SPC_ASCQ_KEY_OFFSET] = 0x00;
>>>>> break;
>>>>> + case TCM_LOGICAL_BLOCK_GUARD_CHECK_FAILED:
>>>>> + /* CURRENT ERROR */
>>>>> + buffer[0] = 0x70;
>>>>> + buffer[SPC_ADD_SENSE_LEN_OFFSET] = 10;
>>>>> + /* ILLEGAL REQUEST */
>>>>> + buffer[SPC_SENSE_KEY_OFFSET] = ILLEGAL_REQUEST;
>>>>> + /* LOGICAL BLOCK GUARD CHECK FAILED */
>>>>> + buffer[SPC_ASC_KEY_OFFSET] = 0x10;
>>>>> + buffer[SPC_ASCQ_KEY_OFFSET] = 0x01;
>> Hey Nic,
>>
>> In my iSER patches I constructed the same sense buffer (call
>> isert_pi_err_sense_buffer) and called isert_put_rsponse. So I should
>> call this routine instead correct?
> Yes, it should be OK to use this for generating CHECK_CONDITION from
> fabric protection failures in isert_completion_rdma_write() code after
> device->unreg_rdma_mem() has been called.
>
> --nab
>
Will do that.
On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <[email protected]>
>
> Hi MKP & SCSI folks,
>
> This series contains initial support for target mode DIF Type1+Type3
> emulation within target core, RAMDISK_MCP device backend, and tcm_loop
> fabric driver.
>
> DIF emulation is enabled via a new 'pi_prot_type' device attribute
> within configfs, which is set after initial device configuration and
> before target fabric LUN export occurs.
>
> The DIF read/write verify emulation has been made generic enough so
> it can be used by other backend drivers (eg: FILEIO), as well as
> DIF v2 in the near future. Also note that the majority of the logic
> has been groked from existing scsi_debug.c code.
>
> The current plan is to enable basic support for emulated backends with
> tcm_loop for v3.14 code, and then move onto IBLOCK backend support
> (that requires BLOCK layer changes)
Hey Nic,
Can you please elaborate on what BLOCK layer changes are required?
I didn't spot any misses from Looking at
Documentation/block/data-integrity.txt.
Am I missing something?
Sagi.
On Wed, 2014-01-15 at 20:03 +0200, sagi grimberg wrote:
> On 1/8/2014 10:36 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <[email protected]>
> >
> > Hi MKP & SCSI folks,
> >
> > This series contains initial support for target mode DIF Type1+Type3
> > emulation within target core, RAMDISK_MCP device backend, and tcm_loop
> > fabric driver.
> >
> > DIF emulation is enabled via a new 'pi_prot_type' device attribute
> > within configfs, which is set after initial device configuration and
> > before target fabric LUN export occurs.
> >
> > The DIF read/write verify emulation has been made generic enough so
> > it can be used by other backend drivers (eg: FILEIO), as well as
> > DIF v2 in the near future. Also note that the majority of the logic
> > has been groked from existing scsi_debug.c code.
> >
> > The current plan is to enable basic support for emulated backends with
> > tcm_loop for v3.14 code, and then move onto IBLOCK backend support
> > (that requires BLOCK layer changes)
>
> Hey Nic,
> Can you please elaborate on what BLOCK layer changes are required?
> I didn't spot any misses from Looking at
> Documentation/block/data-integrity.txt.
>
> Am I missing something?
The issue is that existing fs/bio-integrity.c code always assumes
client/initiator mode, in that it will attempt to
bio_integrity_generate() protection information in the submit_bio WRITE
path, and bio_integrity_verify() of protection information in the
bio_endio READ completion path.
So we'll need a server/target passthrough mode where existing protection
information can be attached during bio setup in IBLOCK code, and the
response be propagated back up without the extra verify, including
propagating up potential Guard + Reference tag failures to the original
submit_bio() caller.
--nab
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> The issue is that existing fs/bio-integrity.c code always assumes
nab> client/initiator mode, in that it will attempt to
nab> bio_integrity_generate() protection information in the submit_bio
nab> WRITE path, and bio_integrity_verify() of protection information in
nab> the bio_endio READ completion path.
Only if the submit_bio() caller hasn't attached protection information
already. If you submit a bio with a bip already attached the block layer
will not generate/verify.
--
Martin K. Petersen Oracle Linux Engineering
On Wed, 2014-01-15 at 20:42 -0500, Martin K. Petersen wrote:
> >>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
>
> nab> The issue is that existing fs/bio-integrity.c code always assumes
> nab> client/initiator mode, in that it will attempt to
> nab> bio_integrity_generate() protection information in the submit_bio
> nab> WRITE path, and bio_integrity_verify() of protection information in
> nab> the bio_endio READ completion path.
>
> Only if the submit_bio() caller hasn't attached protection information
> already. If you submit a bio with a bip already attached the block layer
> will not generate/verify.
>
Mmm, missed that detail. So that would take care of the passthrough for
the WRITE case then..
How about a passthrough on the READ completion side for target fabrics
doing a hardware VERIFY..? Any preferences how this should work..?
Also, for IBLOCK responses to properly generate GUARD + REFERENCE tag
CHECK_CONDITION sense failures from an underlying device VERIFY failure,
there will also need to be a way to propagate up the failure status
through bio_endio(). I assume bio_integrity_payload is the proper place
for adding something like this..?
One other item from my TODO list for IBLOCK protection support is how to
go about determining which DIF type to expose in the target, based upon
what is currently enabled on the underlying struct block_device. I'm
currently trying to deduce the protection type by looking at struct
blk_integrity, but it would be helpful to make this explicit with
bi->prot_type.
--nab
>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
nab> Mmm, missed that detail. So that would take care of the
nab> passthrough for the WRITE case then..
nab> How about a passthrough on the READ completion side for target
nab> fabrics doing a hardware VERIFY..? Any preferences how this should
nab> work..?
Just attach a bip and issue a READ.
nab> Also, for IBLOCK responses to properly generate GUARD + REFERENCE
nab> tag CHECK_CONDITION sense failures from an underlying device VERIFY
nab> failure, there will also need to be a way to propagate up the
nab> failure status through bio_endio(). I assume bio_integrity_payload
nab> is the proper place for adding something like this..?
In my qualification tooling I have a patch that passes the info up the
stack. But it's a bit of a hack (declares a completion status on the
stack that shared between all clones). The alternative is to create new
errno values for each case.
nab> One other item from my TODO list for IBLOCK protection support is
nab> how to go about determining which DIF type to expose in the target,
nab> based upon what is currently enabled on the underlying struct
nab> block_device. I'm currently trying to deduce the protection type
nab> by looking at struct blk_integrity, but it would be helpful to make
nab> this explicit with bi->prot_type.
You need to match on the integrity profile string. T10-DIF-TYPE1-IP
means it's DIF Type 1 format with IP checksum.
--
Martin K. Petersen Oracle Linux Engineering
On 1/16/2014 3:42 AM, Martin K. Petersen wrote:
>>>>>> "nab" == Nicholas A Bellinger <[email protected]> writes:
> nab> The issue is that existing fs/bio-integrity.c code always assumes
> nab> client/initiator mode, in that it will attempt to
> nab> bio_integrity_generate() protection information in the submit_bio
> nab> WRITE path, and bio_integrity_verify() of protection information in
> nab> the bio_endio READ completion path.
>
> Only if the submit_bio() caller hasn't attached protection information
> already. If you submit a bio with a bip already attached the block layer
> will not generate/verify.
>
Yes, that was my understanding as well.
Thanks Martin,
Sagi.