[Qemu-devel] [PATCH 1.0] virtio-blk: pass full status to the guest

[Qemu-devel] [PATCH 1.0] virtio-blk: pass full status to the guest

[Paolo Bonzini]

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.
---
	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

Re: [Qemu-devel] [PATCH 1.0] virtio-blk: pass full status to the guest

[Anthony Liguori]

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);

Re: [Qemu-devel] [PATCH 1.0] virtio-blk: pass full status to the guest

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH 1.0] virtio-blk: pass full status to the guest

[Paolo Bonzini]

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