[PATCH 0000/0002] drivers: scsi: storvsc

[PATCH 0000/0002] drivers: scsi: storvsc

[K. Y. Srinivasan]

The current Windows hosts only handle a subset of scsi commands
sent from the guest and for commands that are not supported, they are
filtered on the host side a generic failure is returned to the guest
as SRB status. The returned error code does not permit the guest to
figure out if there was an error or if the command was not supported.

Based on the input I got from the community, I have convinced the windows
developers to return an error code that allows the guest to distinguish between
unsupported command and true failures. However, it is not clear when this fix will
be shipped.

This patch set addresses this issue by filtering the ATA_16 command on the guest
(note that currently the host is filtering this command as it is not supported). I also have
a patch here the correctly handles SRB_STATUS_INVALID_REQUEST error returns - note that
the current windows hosts don't return this today.


Regards,

K. Y

[PATCH 1/2] Drivers: scsi: storvsc: Set the scsi result correctly when SRB status is INVALID

[K. Y. Srinivasan]

Currently Windows hosts only support a subset of scsi commands and for commands
that are not supported, the host returns a generic SRB failure status.
However, they have agreed to change the return value to indicate that
the command is not supported. In preparation for that, handle the 
SRB_STATUS_INVALID_REQUEST return value correctly.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/scsi/storvsc_drv.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 44c7a48..8b967c9 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -202,6 +202,7 @@ enum storvsc_request_type {
 #define SRB_STATUS_INVALID_LUN	0x20
 #define SRB_STATUS_SUCCESS	0x01
 #define SRB_STATUS_ERROR	0x04
+#define SRB_STATUS_INVALID_REQUEST 0x06
 
 /*
  * This is the end of Protocol specific defines.
@@ -779,6 +780,13 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request)
 	}
 
 	/*
+	 * If the host returns with an invalid request, set
+	 * the scsi command result correctly.
+	 */
+	if (vm_srb->srb_status == SRB_STATUS_INVALID_REQUEST)
+		scmnd->result = ILLEGAL_REQUEST << 16;
+
+	/*
 	 * If there is an error; offline the device since all
 	 * error recovery strategies would have already been
 	 * deployed on the host side.
-- 
1.7.4.1

[PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[K. Y. Srinivasan]

The current Windows hosts don't handle the ATA_16 command and furthermore
return a generic error code after filtering the command on the host side.
For now filter the command on the guest side until the host is modified to
properly handle unsupported commands.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/scsi/storvsc_drv.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 8b967c9..783bab8 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1229,8 +1229,17 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
 	/*
 	 * smartd sends this command and the host does not handle
 	 * this. So, don't send it.
+	 * The current Windows hosts implement a subset of scsi commands
+	 * and for the commands that are not implemented, the host filters
+	 * them and returns a generic failure SRB status. I have been in
+	 * discussions with the Windows team to return the appropriate SRB
+	 * status code for unsupported scsi commands and while they have
+	 * agreed to implement this, it is not clear when this change will be
+	 * available. Consequently, filtering the command before submitting it
+	 * to the host is a resonable interim solution.
 	 */
 	case SET_WINDOW:
+	case ATA_16:
 		scmnd->result = ILLEGAL_REQUEST << 16;
 		allowed = false;
 		break;
-- 
1.7.4.1

Re: [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[Jeff Garzik]

On 03/16/2012 04:24 PM, K. Y. Srinivasan wrote:
> The current Windows hosts don't handle the ATA_16 command and furthermore
> return a generic error code after filtering the command on the host side.
> For now filter the command on the guest side until the host is modified to
> properly handle unsupported commands.
>
> Signed-off-by: K. Y. Srinivasan<kys@microsoft.com>
> Reviewed-by: Haiyang Zhang<haiyangz@microsoft.com>
> ---
>   drivers/scsi/storvsc_drv.c |    9 +++++++++
>   1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 8b967c9..783bab8 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1229,8 +1229,17 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
>   	/*
>   	 * smartd sends this command and the host does not handle
>   	 * this. So, don't send it.
> +	 * The current Windows hosts implement a subset of scsi commands
> +	 * and for the commands that are not implemented, the host filters
> +	 * them and returns a generic failure SRB status. I have been in
> +	 * discussions with the Windows team to return the appropriate SRB
> +	 * status code for unsupported scsi commands and while they have
> +	 * agreed to implement this, it is not clear when this change will be
> +	 * available. Consequently, filtering the command before submitting it
> +	 * to the host is a resonable interim solution.
>   	 */
>   	case SET_WINDOW:
> +	case ATA_16:
>   		scmnd->result = ILLEGAL_REQUEST<<  16;
>   		allowed = false;

It would be even nicer to fill out a nice SCSI status for this.  For an 
illegal request such as this, the libata SCSI simulator 
(drivers/ata/libata-scsi.c) uses a more complete sense buffer setup,

	static void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk,
					u8 asc, u8 ascq)
	{
         cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;

         scsi_build_sense_buffer(0, cmd->sense_buffer, sk, asc, ascq);
	}

and calls this function for invalid/unknown/unsupported command ops thusly,

         ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x20, 0x0);
         /* "Invalid command operation code" */

Supplying asc/ascq provides additional information that may be useful in 
determining whether or not to retry, provide better error diagnostics, etc.

The asc/ascq values are found in the SCSI standards documents.

	Jeff

Re: [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[Douglas Gilbert]

On 12-03-17 04:04 PM, Jeff Garzik wrote:
> On 03/16/2012 04:24 PM, K. Y. Srinivasan wrote:
>> The current Windows hosts don't handle the ATA_16 command and furthermore
>> return a generic error code after filtering the command on the host side.
>> For now filter the command on the guest side until the host is modified to
>> properly handle unsupported commands.
>>
>> Signed-off-by: K. Y. Srinivasan<kys@microsoft.com>
>> Reviewed-by: Haiyang Zhang<haiyangz@microsoft.com>
>> ---
>> drivers/scsi/storvsc_drv.c | 9 +++++++++
>> 1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
>> index 8b967c9..783bab8 100644
>> --- a/drivers/scsi/storvsc_drv.c
>> +++ b/drivers/scsi/storvsc_drv.c
>> @@ -1229,8 +1229,17 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
>> /*
>> * smartd sends this command and the host does not handle
>> * this. So, don't send it.
>> + * The current Windows hosts implement a subset of scsi commands
>> + * and for the commands that are not implemented, the host filters
>> + * them and returns a generic failure SRB status. I have been in
>> + * discussions with the Windows team to return the appropriate SRB
>> + * status code for unsupported scsi commands and while they have
>> + * agreed to implement this, it is not clear when this change will be
>> + * available. Consequently, filtering the command before submitting it
>> + * to the host is a resonable interim solution.
>> */
>> case SET_WINDOW:
>> + case ATA_16:
>> scmnd->result = ILLEGAL_REQUEST<< 16;
>> allowed = false;
>
> It would be even nicer to fill out a nice SCSI status for this. For an illegal
> request such as this, the libata SCSI simulator (drivers/ata/libata-scsi.c) uses
> a more complete sense buffer setup,
>
> static void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk,
> u8 asc, u8 ascq)
> {
> cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
>
> scsi_build_sense_buffer(0, cmd->sense_buffer, sk, asc, ascq);
> }
>
> and calls this function for invalid/unknown/unsupported command ops thusly,
>
> ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x20, 0x0);
> /* "Invalid command operation code" */
>
> Supplying asc/ascq provides additional information that may be useful in
> determining whether or not to retry, provide better error diagnostics, etc.
>
> The asc/ascq values are found in the SCSI standards documents.

Jeff,
I agree with your suggestion and would point out that the
originally proposed:
     scmnd->result = ILLEGAL_REQUEST<<  16;

is just plain wrong. result[23:16] is a Linux invented host byte
code (they typically start with "DID_") while ILLEGAL_REQUEST is
a SCSI sense key.

Doug Gilbert

RE: [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to the host

[KY Srinivasan]

> -----Original Message-----
> From: Douglas Gilbert [mailto:dgilbert@interlog.com]
> Sent: Saturday, March 17, 2012 2:56 PM
> To: Jeff Garzik
> Cc: KY Srinivasan; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> jbottomley@parallels.com; hch@infradead.org; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 2/2] Drivers: scsi: storvsc: Don't pass ATA_16 command to
> the host
> 
> On 12-03-17 04:04 PM, Jeff Garzik wrote:
> > On 03/16/2012 04:24 PM, K. Y. Srinivasan wrote:
> >> The current Windows hosts don't handle the ATA_16 command and
> furthermore
> >> return a generic error code after filtering the command on the host side.
> >> For now filter the command on the guest side until the host is modified to
> >> properly handle unsupported commands.
> >>
> >> Signed-off-by: K. Y. Srinivasan<kys@microsoft.com>
> >> Reviewed-by: Haiyang Zhang<haiyangz@microsoft.com>
> >> ---
> >> drivers/scsi/storvsc_drv.c | 9 +++++++++
> >> 1 files changed, 9 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> >> index 8b967c9..783bab8 100644
> >> --- a/drivers/scsi/storvsc_drv.c
> >> +++ b/drivers/scsi/storvsc_drv.c
> >> @@ -1229,8 +1229,17 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd
> *scmnd)
> >> /*
> >> * smartd sends this command and the host does not handle
> >> * this. So, don't send it.
> >> + * The current Windows hosts implement a subset of scsi commands
> >> + * and for the commands that are not implemented, the host filters
> >> + * them and returns a generic failure SRB status. I have been in
> >> + * discussions with the Windows team to return the appropriate SRB
> >> + * status code for unsupported scsi commands and while they have
> >> + * agreed to implement this, it is not clear when this change will be
> >> + * available. Consequently, filtering the command before submitting it
> >> + * to the host is a resonable interim solution.
> >> */
> >> case SET_WINDOW:
> >> + case ATA_16:
> >> scmnd->result = ILLEGAL_REQUEST<< 16;
> >> allowed = false;
> >
> > It would be even nicer to fill out a nice SCSI status for this. For an illegal
> > request such as this, the libata SCSI simulator (drivers/ata/libata-scsi.c) uses
> > a more complete sense buffer setup,
> >
> > static void ata_scsi_set_sense(struct scsi_cmnd *cmd, u8 sk,
> > u8 asc, u8 ascq)
> > {
> > cmd->result = (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;
> >
> > scsi_build_sense_buffer(0, cmd->sense_buffer, sk, asc, ascq);
> > }
> >
> > and calls this function for invalid/unknown/unsupported command ops thusly,
> >
> > ata_scsi_set_sense(cmd, ILLEGAL_REQUEST, 0x20, 0x0);
> > /* "Invalid command operation code" */
> >
> > Supplying asc/ascq provides additional information that may be useful in
> > determining whether or not to retry, provide better error diagnostics, etc.
> >
> > The asc/ascq values are found in the SCSI standards documents.
> 
> Jeff,
> I agree with your suggestion and would point out that the
> originally proposed:
>      scmnd->result = ILLEGAL_REQUEST<<  16;
> 
> is just plain wrong. result[23:16] is a Linux invented host byte
> code (they typically start with "DID_") while ILLEGAL_REQUEST is
> a SCSI sense key.
> 
> Doug Gilbert
> 
Jeff, Doug,

Thank you. I will make the suggested changes.

Regards,

K. Y

Re: [PATCH 1/2] Drivers: scsi: storvsc: Set the scsi result correctly when SRB status is INVALID

[Greg KH]

On Fri, Mar 16, 2012 at 01:24:31PM -0700, K. Y. Srinivasan wrote:
> Currently Windows hosts only support a subset of scsi commands and for commands
> that are not supported, the host returns a generic SRB failure status.
> However, they have agreed to change the return value to indicate that
> the command is not supported. In preparation for that, handle the 
> SRB_STATUS_INVALID_REQUEST return value correctly.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  drivers/scsi/storvsc_drv.c |    8 ++++++++

I need an ack from the scsi maintainer before I can accept this patch.

James?