| Message ID | 20220718120117.4435-1-d.bogdanov@yadro.com |
|---|---|
| Headers | show |
| Series | add support of RSOC command | expand |
On 7/18/22 7:01 AM, Dmitry Bogdanov wrote: > Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> > Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com> > --- > drivers/target/target_core_spc.c | 595 +++++++++++++++++++++++++++++++ > include/scsi/scsi_proto.h | 3 + > 2 files changed, 598 insertions(+) > > diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c > index 4157f73977cf..506e28b14e5a 100644 > --- a/drivers/target/target_core_spc.c > +++ b/drivers/target/target_core_spc.c > +static struct target_opcode_descriptor tcm_opcode_xdwriteread10 = { > + .support = SCSI_SUPPORT_FULL, > + .opcode = XDWRITEREAD_10, > + .cdb_size = 10, > + .usage_bits = {XDWRITEREAD_10, 0x18, 0xff, 0xff, > + 0xff, 0xff, SCSI_GROUP_NUMBER_MASK, 0xff, > + 0xff, SCSI_CONTROL_MASK}, > +}; > + > +static struct target_opcode_descriptor tcm_opcode_xdwriteread32 = { > + .support = SCSI_SUPPORT_FULL, > + .serv_action_valid = 1, > + .opcode = VARIABLE_LENGTH_CMD, > + .service_action = XDWRITEREAD_32, > + .cdb_size = 32, > + .usage_bits = {VARIABLE_LENGTH_CMD, SCSI_CONTROL_MASK, 0x00, 0x00, > + 0x00, 0x00, SCSI_GROUP_NUMBER_MASK, 0x18, > + 0x00, XDWRITEREAD_32, 0x18, 0x00, > + 0xff, 0xff, 0xff, 0xff, > + 0xff, 0xff, 0xff, 0xff, > + 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, > + 0xff, 0xff, 0xff, 0xff}, > +}; > + I just removed these because they didn't work. I think the patch was added to one of Martin's tree after you made this patch.
On 7/18/22 7:01 AM, Dmitry Bogdanov wrote: > Report supported opcodes depending on a dynamic device configuration > > Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> > Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com> > --- > drivers/target/target_core_spc.c | 118 ++++++++++++++++++++++++++++-- > include/target/target_core_base.h | 1 + > 2 files changed, 114 insertions(+), 5 deletions(-) > > diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c > index 506e28b14e5a..cf516136b933 100644 > --- a/drivers/target/target_core_spc.c > +++ b/drivers/target/target_core_spc.c > @@ -1424,6 +1424,13 @@ static struct target_opcode_descriptor tcm_opcode_xdwriteread32 = { > 0xff, 0xff, 0xff, 0xff}, > }; > > +static bool tcm_is_ws_enabled(struct se_cmd *cmd) > +{ > + struct se_device *dev = cmd->se_dev; > + > + return dev->dev_attrib.emulate_tpws; > +} > + > static struct target_opcode_descriptor tcm_opcode_write_same32 = { > .support = SCSI_SUPPORT_FULL, > .serv_action_valid = 1, > @@ -1438,8 +1445,16 @@ static struct target_opcode_descriptor tcm_opcode_write_same32 = { > 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, > 0xff, 0xff, 0xff, 0xff}, > + .enabled = tcm_is_ws_enabled, > }; I'm not sure what's incorrect. I think your patch is correct but the write same code is wrong. If emulate_tpws is 0, we will still execute the command. We actually only fail with TCM_UNSUPPORTED_SCSI_OPCODE if it's a WRITE_SAME with the UNMAP bit = 1 and emulate_tpws=0. If it's just a normal WRITE_SAME we maybe go by if by max_write_same_len is greater than zero? Maybe that was a mistake and sbc_setup_write_same needs a emulate_tpws check. > static struct target_opcode_descriptor tcm_opcode_sync_cache = { > @@ -1502,6 +1533,14 @@ static struct target_opcode_descriptor tcm_opcode_sync_cache16 = { > 0xff, 0xff, SCSI_GROUP_NUMBER_MASK, SCSI_CONTROL_MASK}, > }; > > +static bool tcm_is_unmap_enabled(struct se_cmd *cmd) > +{ > + struct sbc_ops *ops = cmd->protocol_data; > + struct se_device *dev = cmd->se_dev; > + > + return ops->execute_unmap && dev->dev_attrib.emulate_tpu; > +} Just a trivial nit. You had an extra space there.
On Thu, Aug 11, 2022 at 10:38:29PM -0500, Mike Christie wrote: > «Внимание! Данное письмо от внешнего адресата!» > > On 7/18/22 7:01 AM, Dmitry Bogdanov wrote: > > Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> > > Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com> > > --- > > drivers/target/target_core_spc.c | 595 +++++++++++++++++++++++++++++++ > > include/scsi/scsi_proto.h | 3 + > > 2 files changed, 598 insertions(+) > > > > diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c > > index 4157f73977cf..506e28b14e5a 100644 > > --- a/drivers/target/target_core_spc.c > > +++ b/drivers/target/target_core_spc.c > > > > +static struct target_opcode_descriptor tcm_opcode_xdwriteread10 = { > > + .support = SCSI_SUPPORT_FULL, > > + .opcode = XDWRITEREAD_10, > > + .cdb_size = 10, > > + .usage_bits = {XDWRITEREAD_10, 0x18, 0xff, 0xff, > > + 0xff, 0xff, SCSI_GROUP_NUMBER_MASK, 0xff, > > + 0xff, SCSI_CONTROL_MASK}, > > +}; > > + > > +static struct target_opcode_descriptor tcm_opcode_xdwriteread32 = { > > + .support = SCSI_SUPPORT_FULL, > > + .serv_action_valid = 1, > > + .opcode = VARIABLE_LENGTH_CMD, > > + .service_action = XDWRITEREAD_32, > > + .cdb_size = 32, > > + .usage_bits = {VARIABLE_LENGTH_CMD, SCSI_CONTROL_MASK, 0x00, 0x00, > > + 0x00, 0x00, SCSI_GROUP_NUMBER_MASK, 0x18, > > + 0x00, XDWRITEREAD_32, 0x18, 0x00, > > + 0xff, 0xff, 0xff, 0xff, > > + 0xff, 0xff, 0xff, 0xff, > > + 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, > > + 0xff, 0xff, 0xff, 0xff}, > > +}; > > + > > I just removed these because they didn't work. I think the patch was added to > one of Martin's tree after you made this patch. Yes, I saw, Iwill remove XDWRITEREAD_* in the next revision.
On Thu, Aug 11, 2022 at 10:43:05PM -0500, Mike Christie wrote: > «Внимание! Данное письмо от внешнего адресата!» > > On 7/18/22 7:01 AM, Dmitry Bogdanov wrote: > > Report supported opcodes depending on a dynamic device configuration > > > > Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> > > Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com> > > --- > > drivers/target/target_core_spc.c | 118 ++++++++++++++++++++++++++++-- > > include/target/target_core_base.h | 1 + > > 2 files changed, 114 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c > > index 506e28b14e5a..cf516136b933 100644 > > --- a/drivers/target/target_core_spc.c > > +++ b/drivers/target/target_core_spc.c > > @@ -1424,6 +1424,13 @@ static struct target_opcode_descriptor tcm_opcode_xdwriteread32 = { > > 0xff, 0xff, 0xff, 0xff}, > > }; > > > > +static bool tcm_is_ws_enabled(struct se_cmd *cmd) > > +{ > > + struct se_device *dev = cmd->se_dev; > > + > > + return dev->dev_attrib.emulate_tpws; > > +} > > + > > static struct target_opcode_descriptor tcm_opcode_write_same32 = { > > .support = SCSI_SUPPORT_FULL, > > .serv_action_valid = 1, > > @@ -1438,8 +1445,16 @@ static struct target_opcode_descriptor tcm_opcode_write_same32 = { > > 0x00, 0x00, 0x00, 0x00, > > 0x00, 0x00, 0x00, 0x00, > > 0xff, 0xff, 0xff, 0xff}, > > + .enabled = tcm_is_ws_enabled, > > }; > > I'm not sure what's incorrect. I think your patch is correct but the write > same code is wrong. > > If emulate_tpws is 0, we will still execute the command. We actually only fail > with TCM_UNSUPPORTED_SCSI_OPCODE if it's a WRITE_SAME with the UNMAP bit = 1 > and emulate_tpws=0. > > If it's just a normal WRITE_SAME we maybe go by if by max_write_same_len is > greater than zero? Maybe that was a mistake and sbc_setup_write_same needs > a emulate_tpws check. Looks like emulate_tpws was introduced exaclty for WS+UNMAP bit case and it can not be used in tcm_is_ws_enabled as only check. Because of WS is actually two different commands selected by UNMAP bit it is unable somehow to differentiate them in RSOC. So I will reformulate the check in tcm_is_ws_enabled to be true if some of cases is supported by the backstore device. + return (dev->dev_attrib.emulate_tpws && !!ops->execute_unmap) || + !!ops->execute_write_same; > > > static struct target_opcode_descriptor tcm_opcode_sync_cache = { > > @@ -1502,6 +1533,14 @@ static struct target_opcode_descriptor tcm_opcode_sync_cache16 = { > > 0xff, 0xff, SCSI_GROUP_NUMBER_MASK, SCSI_CONTROL_MASK}, > > }; > > > > +static bool tcm_is_unmap_enabled(struct se_cmd *cmd) > > +{ > > + struct sbc_ops *ops = cmd->protocol_data; > > + struct se_device *dev = cmd->se_dev; > > + > > + return ops->execute_unmap && dev->dev_attrib.emulate_tpu; > > +} > > Just a trivial nit. You had an extra space there. yep, will fix
On Fri, Aug 12, 2022 at 11:03:07AM +0300, Dmitry Bogdanov wrote: > > > + .support = SCSI_SUPPORT_FULL, > > > + .opcode = XDWRITEREAD_10, > > > + .cdb_size = 10, > > > + .usage_bits = {XDWRITEREAD_10, 0x18, 0xff, 0xff, > > > + 0xff, 0xff, SCSI_GROUP_NUMBER_MASK, 0xff, > > > + 0xff, SCSI_CONTROL_MASK}, > > > +}; > > one of Martin's tree after you made this patch. > Yes, I saw, Iwill remove XDWRITEREAD_* in the next revision. What this does point out is that the way the patches are done, we have a fundamental issue with these descriptors being potentially out of sync with the actually supported commands. Once way to fix this would be to add a parse callback to these dscriptors to unwind sbc_parse_cdb. The big downside would be an extra expensive indirect call per command, though.
On Sat, Aug 13, 2022 at 12:49:43AM -0700, Christoph Hellwig wrote: > > On Fri, Aug 12, 2022 at 11:03:07AM +0300, Dmitry Bogdanov wrote: > > > > + .support = SCSI_SUPPORT_FULL, > > > > + .opcode = XDWRITEREAD_10, > > > > + .cdb_size = 10, > > > > + .usage_bits = {XDWRITEREAD_10, 0x18, 0xff, 0xff, > > > > + 0xff, 0xff, SCSI_GROUP_NUMBER_MASK, 0xff, > > > > + 0xff, SCSI_CONTROL_MASK}, > > > > +}; > > > > one of Martin's tree after you made this patch. > > Yes, I saw, Iwill remove XDWRITEREAD_* in the next revision. > > What this does point out is that the way the patches are done, > we have a fundamental issue with these descriptors being potentially > out of sync with the actually supported commands. Once way to fix > this would be to add a parse callback to these dscriptors to unwind > sbc_parse_cdb. The big downside would be an extra expensive indirect > call per command, though. Yes, there is such a risk. I was raising it in our company 2 years ago when I did this patchset. We agreed that, until there is somebody who can notice about it, it's OK :). It's happened not so often. There was just one case when we changed RSOC structs - when I was adding support of ACA condition. Recently someone wants to have some defence against fuzzy-logic attacks, may be he (or someone else) will intergate RSOC descriptors into sbc_parse_cmd within his task.