On Wed, 2014-03-05 at 15:38 +0200, Sagi Grimberg wrote:
> On 2/19/2014 5:50 PM, Sagi Grimberg wrote:
> > For transports which use generic new command these
> > buffers have yet to be allocated. Instead check afterwards
> > if command required prot buffers but none are provided, In
> > this case send CHECK_CONDITION response.
> >
> > Also this way, target may support protection information
> > against legacy initiators (writes are inserted and reads
> > are stripped).
> >
> > Signed-off-by: Sagi Grimberg <sagig@xxxxxxxxxxxx>
> > ---
> > drivers/target/target_core_sbc.c | 3 ---
> > drivers/target/target_core_transport.c | 21 ++++++++++++++++++---
> > 2 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index 6939947..312f522 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -633,9 +633,6 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
> > {
> > u8 protect = cdb[1] >> 5;
> >
> > - if (!cmd->t_prot_sg || !cmd->t_prot_nents)
> > - return true;
> > -
> > switch (dev->dev_attrib.pi_prot_type) {
> > case TARGET_DIF_TYPE3_PROT:
> > cmd->reftag_seed = 0xffffffff;
> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 0f9e1ea..a45d628 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -1365,6 +1365,13 @@ 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;
> > }
> > +
> > + rc = target_setup_cmd_from_cdb(se_cmd, cdb);
> > + if (rc != 0) {
> > + transport_generic_request_failure(se_cmd, rc);
> > + return 0;
> > + }
> > +
> > /*
> > * Save pointers for SGLs containing protection information,
> > * if present.
> > @@ -1374,11 +1381,19 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
> > se_cmd->t_prot_nents = sgl_prot_count;
> > }
> >
> > - rc = target_setup_cmd_from_cdb(se_cmd, cdb);
> > - if (rc != 0) {
> > - transport_generic_request_failure(se_cmd, rc);
> > + /*
> > + * Fail if protection operation requiers protection
> > + * information buffers but None are provided!
> > + */
> > + if ((!se_cmd->t_prot_sg || !se_cmd->t_prot_nents) &&
> > + (se_cmd->prot_op != TARGET_PROT_NORMAL)) {
> > + pr_err("ERROR: protection information was requested but "
> > + "protection buffers weren't provided.\n");
> > + transport_generic_request_failure(se_cmd,
> > + TCM_INVALID_CDB_FIELD);
> > return 0;
> > }
> > +
>
> It seems that loopback functionality was broken in this one (but it was
> somewhat wrong to start with).
> The thing is, the startup READs are not passed with protection buffers
> and asks the LLD to insert/strip DIF (SCSI_PROT_READ_STRIP)
> and a couple of READs later DIX is enabled and starts passing protection
> information (SCSI_PROT_READ_PASS).
> Since loopback initiator is not aware of this, startup READs will fail
> in this commit. Same for disabling write_generate/read_verify.
>
> I think that the loopback initiator should conform to the protection
> operation in scsi_cmnd.
> Meaning it should INSERT/STRIP/PASS according to what was stated in the
> SCSI command:
> SCSI_PROT_READ_STRIP: should allocate protection protection sg and
> execute target_submit_cmd_map_sgls().
> SCSI_PROT_WRITE_GENERATE: should allocate protection protection sg,
> compute protection and execute target_submit_cmd_map_sgls().
> SCSI_PROT_READ_INSERT: don't know if worth the bother for loopback.
> SCSI_PROT_WRITE_STRIP: don't know if worth the bother for loopback.
> SCSI_PROT_READ_PASS: stay the same.
> SCSI_PROT_WRITE_PASS: stay the same.
>
> The alternative is to avoid this patch but then we might miss DIF
> support when working against legacy initiators.
>
> Thoughts?
Mmmm, not sure about this one..
Considering that it's only the first two READ_10s that this effects
before normal SCSI_PROT_*_PASS operation kicks in, I'm not sure if it's
worth it to add these to tcm_loop.. Note that virtio-scsi + vhost-scsi
would need similar bits as well..
So I'd prefer to get target-core working for both types of cases, vs.
adding READ_STRIP + WRITE_GENERATE emulation logic to tcm_loop +
virtio-scsi LLDs.
MKP, any preference here..?
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
