RE: [PATCH 1/1] scsi: target: core: Add 8Fh VPD page

[Sergey Samoylenko <s.samoylenko@yadro.com>]

Hi David,

> Hi Sergey,
>
> On Thu, 29 Jul 2021 23:19:43 +0300, Sergey Samoylenko wrote:
>
>> The 8Fh VPD page announces the capabilities supported by
>> the TCM XCOPY manager. It helps to expand the coverage of
>> the third-party copy manager with SCSI testing utilities.
>
> Please list which initiators use this VPD page, if you know of any.
I know that the ESXi 7.0 requests the 8Fh VPD page. ESXi is one of
a few initiators who is using the XCOPY commands (vmkfstools tool).

> Also, is there any test coverage for this? I don't see anything in
> libiscsi...
After activating XCOPY in a target we got an error from
the SCSI.ReceiveCopyResults.CopyStatus test in the libiscsi.
Discussing with Bart, we decided to implement the 8Fh VPD page
for announcing TCM XCOPY features.
It is here: https://github.com/sahlberg/libiscsi/pull/353

The libiscsi has an initial version for parsing 8Fh VPD. This is used
in the SCSI.ReceiveCopyResults.CopyStatus test. It is a good idea
to add test coverage for 8Fh VPD in libiscsi. I should do this.

>
>> Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
>> Reviewed-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
>> Reviewed-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
>> Signed-off-by: Sergey Samoylenko <s.samoylenko@yadro.com>
>> ---
>>  drivers/target/target_core_spc.c | 230 ++++++++++++++++++++++++++++++-
>>  1 file changed, 226 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
>> index 22703a0dbd07..169341712b10 100644
>> --- a/drivers/target/target_core_spc.c
>> +++ b/drivers/target/target_core_spc.c
> ...
>> +/* Third-party Copy VPD page */
>> +static sense_reason_t
>> +spc_emulate_evpd_8f(struct se_cmd *cmd, unsigned char *buf)
>> +{
>> +	struct se_device *dev = cmd->se_dev;
>> +	int off;
>> +	u16 page_len;
>> +
>> +	if (!dev->dev_attrib.emulate_3pc)
>> +		return TCM_INVALID_CDB_FIELD;
>> +
>> +	/*
>> +	 * Since the Third-party copy manager in TCM is quite simple
>> +	 * and supports only two commands, the function sets
>> +	 * many descriptor parameters as constants.
>> +	 *
>> +	 * As the Copy manager supports the EXTENDED COPY(LID1) command,
>> +	 * the Third-party Copy VPD page should include five mandatory
>> +	 * Third-party copy descriptors. Its are:
>> +	 *   0001h - Supported Commands
>> +	 *   0004h - Parameter Data
>> +	 *   0008h - Supported Descriptors
>> +	 *   000Ch - Supported CSCD Descriptor IDs
>> +	 *   8001h - General Copy Operations
>> +	 *
>> +	 * See spc4 section 7.8.17
>> +	 */
>> +
>> +	off = 4;
>> +
>> +	/* fill descriptors */
>> +	off += spc_evpd_8f_encode_supp_cmds(&buf[off]);
>> +	off += spc_evpd_8f_encode_param_data(&buf[off]);
>> +	off += spc_evpd_8f_encode_supp_descrs(&buf[off]);
>> +	off += spc_evpd_8f_encode_supp_cscd_descr_id(&buf[off]);
>> +	off += spc_evpd_8f_encode_general_copy_ops(&buf[off]);
>
> This looks risky in terms of buf overrun. I think it'd be good to pass
> a @remaining or @buf_end param to these helper functions.

I thought about it, but spc_emulate_evpd_XX functions have a prototype:
	sense_reason_t	(*emulate)(struct se_cmd *, unsigned char *);
and they don't know anything about buffer length. I can use the
"SE_INQUIRY_BUF" definition, but I don't like this solution.
	
We can change the prototype of spc_emulate_evpd_XX functions, like:
	static struct {
		uint8_t page;
		sense_reason_t	(*emulate)(struct se_cmd *, unsigned char *buf, size_t len);
	} evpd_handlers[] = {
		...
	};
and return the TCM_OUT_OF_RESOURCES if we try to overrun buffer
in spc_emulate_evpd_XX. But this will require changing all "emulate_evpd" functions.

David, what do you think of this?

>
> Cheers, David

Best regards, Sergey