On Mon, 2012-03-19 at 16:50 +0000, KY Srinivasan wrote: > > > -----Original Message----- > > From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx] > > Sent: Monday, March 19, 2012 12:13 PM > > To: KY Srinivasan > > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > devel@xxxxxxxxxxxxxxxxxxxxxx; ohering@xxxxxxxx; hch@xxxxxxxxxxxxx; linux- > > scsi@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH RESEND 1/2] Drivers: scsi: storvsc: Set the scsi result correctly > > when SRB status is INVALID > > > > On Sun, 2012-03-18 at 17:12 -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. > > > > > > I would like to thank Jeff Garzik <jgpobox@xxxxxxxxx> and > > > Douglas Gilbert <dgilbert@xxxxxxxxxxxx> for suggesting the correct approach > > > here. > > > > > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx> > > > Reviewed-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> > > > --- > > > drivers/scsi/storvsc_drv.c | 12 ++++++++++++ > > > 1 files changed, 12 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c > > > index 44c7a48..018c363 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 > > > > I don't really think this is the correct approach. We already have a > > SCSI error return for this, which you're now translating in the driver > > and hypervisor. Rather than have a special byte return of > > SRB_STATUS_INVALID_REQUEST, why not have the hypervisor do the right > > thing and fill in the ILLEGAL_REQUEST sense return. That way you don't > > need a special error code and you don't need to construct the sense > > buffer in the driver. Now HyperV will be correctly set up for both pass > > through and emulated responses. It's surely not much work and you > > already process sense data correctly in storvsc_command_completion(), so > > you wouldn't need any patches to the driver for this approach. > > James, the issue here is that currently shipping Windows hosts don't even do > what I am handling here. Right, I understand that. > Based on the input I got from you and Christoph, > I convinced the windows team to at least return the SRB status that indicates > an illegal request. I will suggest to them that perhaps they should also set the > correct sense code and so I would not need this patch. Not also; instead of. There's no need for an extra SRB status. Just return the standard check condition sense data. > However, keep in mind > that there is no current ETA on when Windows will ship with these changes - Windows 8 > may ship with code where they would return an invalid SRB status, but they are not > setting the sense code, hence this patch. When the Window host does the "right thing" > I will clean this up, but I don't know when that will be. I thought you just said you'd only just asked them if they could implemented it, in which case no version of windows currently ships with this, correct? > More importantly, the second patch in this series where I filter out > the ATA_16 command > on the guest is really important for us. Without that patch on a range > on windows hosts > including the current beta version of windows8 where the host is > returning a generic > error in response to ATA_16 command, we cannot boot many Linux > distros. If you > prefer, I can drop the first patch and re-submit the second patch for > consideration now. I'm not sure about that either. You presumably translate SRB_STATUS_ERROR into DID_TARGET_FAILURE. That should cause the termination of the command with prejudice in exactly the same way as an ILLEGAL_REQUEST sense code would (minus the useful error information), so what's causing the boot failure? James _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
