| Message ID | 1320236380-26180-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/02/2011 07:19 AM, Paolo Bonzini wrote: > When SCSI passthrough is being used by the guest with virtio-blk, the > guest is not able to detect disk failures. This is because the status > field is expected by the guest driver to include also the msg_status, > host_status and driver_status fields, but the device is only passing > down the SCSI status. > > The patch fixes this, and also makes sure that the guest always sees a > CHECK_CONDITION status when there is valid sense data. > --- Applied. Thanks. Regards, Anthony Liguori > Sorry for missing the -rc deadline, testing this requires > Fibre Channel stuff so I needed someone to do it for me. :) > > hw/virtio-blk.c | 16 +++++++++++++++- > 1 files changed, 15 insertions(+), 1 deletions(-) > > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index 179747a..8beaa20 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -16,6 +16,7 @@ > #include "trace.h" > #include "blockdev.h" > #include "virtio-blk.h" > +#include "scsi-defs.h" > #ifdef __linux__ > # include<scsi/sg.h> > #endif > @@ -231,7 +232,20 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) > status = VIRTIO_BLK_S_OK; > } > > - stl_p(&req->scsi->errors, hdr.status); > + /* > + * From SCSI-Generic-HOWTO: "Some lower level drivers (e.g. ide-scsi) > + * clear the masked_status field [hence status gets cleared too, see > + * block/scsi_ioctl.c] even when a CHECK_CONDITION or COMMAND_TERMINATED > + * status has occurred. However they do set DRIVER_SENSE in driver_status > + * field. Also a (sb_len_wr> 0) indicates there is a sense buffer. > + */ > + if (hdr.status == 0&& hdr.sb_len_wr> 0) { > + hdr.status = CHECK_CONDITION; > + } > + > + stl_p(&req->scsi->errors, > + hdr.status | (hdr.msg_status<< 8) | > + (hdr.host_status<< 16) | (hdr.driver_status<< 24)); > stl_p(&req->scsi->residual, hdr.resid); > stl_p(&req->scsi->sense_len, hdr.sb_len_wr); > stl_p(&req->scsi->data_len, hdr.dxfer_len);
On Wed, Nov 02, 2011 at 01:19:40PM +0100, Paolo Bonzini wrote: > When SCSI passthrough is being used by the guest with virtio-blk, the > guest is not able to detect disk failures. This is because the status > field is expected by the guest driver to include also the msg_status, > host_status and driver_status fields, but the device is only passing > down the SCSI status. > > The patch fixes this, and also makes sure that the guest always sees a > CHECK_CONDITION status when there is valid sense data. Do the bit offsets happen to be defined in some header? Also, Signed-off-by is missing? > --- > Sorry for missing the -rc deadline, testing this requires > Fibre Channel stuff so I needed someone to do it for me. :) > > hw/virtio-blk.c | 16 +++++++++++++++- > 1 files changed, 15 insertions(+), 1 deletions(-) > > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index 179747a..8beaa20 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -16,6 +16,7 @@ > #include "trace.h" > #include "blockdev.h" > #include "virtio-blk.h" > +#include "scsi-defs.h" > #ifdef __linux__ > # include <scsi/sg.h> > #endif > @@ -231,7 +232,20 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) > status = VIRTIO_BLK_S_OK; > } > > - stl_p(&req->scsi->errors, hdr.status); > + /* > + * From SCSI-Generic-HOWTO: "Some lower level drivers (e.g. ide-scsi) > + * clear the masked_status field [hence status gets cleared too, see > + * block/scsi_ioctl.c] even when a CHECK_CONDITION or COMMAND_TERMINATED > + * status has occurred. However they do set DRIVER_SENSE in driver_status > + * field. Also a (sb_len_wr > 0) indicates there is a sense buffer. > + */ > + if (hdr.status == 0 && hdr.sb_len_wr > 0) { > + hdr.status = CHECK_CONDITION; > + } > + > + stl_p(&req->scsi->errors, > + hdr.status | (hdr.msg_status << 8) | > + (hdr.host_status << 16) | (hdr.driver_status << 24)); > stl_p(&req->scsi->residual, hdr.resid); > stl_p(&req->scsi->sense_len, hdr.sb_len_wr); > stl_p(&req->scsi->data_len, hdr.dxfer_len); > -- > 1.7.6.4
On 11/02/2011 02:00 PM, Michael S. Tsirkin wrote: > > When SCSI passthrough is being used by the guest with virtio-blk, the > > guest is not able to detect disk failures. This is because the status > > field is expected by the guest driver to include also the msg_status, > > host_status and driver_status fields, but the device is only passing > > down the SCSI status. > > > > The patch fixes this, and also makes sure that the guest always sees a > > CHECK_CONDITION status when there is valid sense data. > > Do the bit offsets happen to be defined in some header? Nope, the closest match is #define msg_byte(result) (((result) >> 8) & 0xff) #define host_byte(result) (((result) >> 16) & 0xff) #define driver_byte(result) (((result) >> 24) & 0xff) in Linux scsi/scsi.h > Also, Signed-off-by is missing? Ugh. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 179747a..8beaa20 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -16,6 +16,7 @@ #include "trace.h" #include "blockdev.h" #include "virtio-blk.h" +#include "scsi-defs.h" #ifdef __linux__ # include <scsi/sg.h> #endif @@ -231,7 +232,20 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) status = VIRTIO_BLK_S_OK; } - stl_p(&req->scsi->errors, hdr.status); + /* + * From SCSI-Generic-HOWTO: "Some lower level drivers (e.g. ide-scsi) + * clear the masked_status field [hence status gets cleared too, see + * block/scsi_ioctl.c] even when a CHECK_CONDITION or COMMAND_TERMINATED + * status has occurred. However they do set DRIVER_SENSE in driver_status + * field. Also a (sb_len_wr > 0) indicates there is a sense buffer. + */ + if (hdr.status == 0 && hdr.sb_len_wr > 0) { + hdr.status = CHECK_CONDITION; + } + + stl_p(&req->scsi->errors, + hdr.status | (hdr.msg_status << 8) | + (hdr.host_status << 16) | (hdr.driver_status << 24)); stl_p(&req->scsi->residual, hdr.resid); stl_p(&req->scsi->sense_len, hdr.sb_len_wr); stl_p(&req->scsi->data_len, hdr.dxfer_len);