[Qemu-devel] [PATCH] scsi-disk: correct SCSI error reporting

[Laurent Vivier]

Hi,

scsi-disk.c seems to not report to SCSI controller (through completion()
routine) the good error value.

I've tested this by using a little program trying to write beyond the
end of the disk (see attachment read10.c)

On real disk:

# ./read10 /dev/sg0
READ_10 duration=40 millisecs, resid=0, msg_status=0 status=2 sense=0
driver_status 8

If I use this command with qemu and a scsi disk, the result is:

# ./read10 /dev/sg0
sym0: bad DSA (5da40ff) in done queue.
sd 0:0:0:0: ABORT operation started
sym0: SCSI BUS reset detected.
sym0: SCSI BUS has been reset.
sd 0:0:0:0: ABORT operation complete.
sd 0:0:0:0: ABORT operation started
sd 0:0:0:0: ABORT operation failed.
sd 0:0:0:0: DEVICE RESET operation started
 target0:0:0: control msgout: c.
lsi_scsi: error: Unimplemented message 0x0c
 target0:0:0: has been reset
sd 0:0:0:0: DEVICE RESET operation complete.
sd 0:0:0:0: DEVICE RESET operation started
 target0:0:0: control msgout: c.
lsi_scsi: error: Unimplemented message 0x0c
 target0:0:0: has been reset
sd 0:0:0:0: DEVICE RESET operation complete.
sd 0:0:0:0: M_REJECT received (0:0).
sd 0:0:0:0: M_REJECT received (0:0).
READ_10 duration=30032 millisecs, resid=0, msg_status=0 status=0 sense=0
driver_status 0

The following patch corrects this issue by sending to the controller not
the sense code but the status code.

The result of the command is now:

# ./read10 /dev/sg0
READ_10 duration=0 millisecs, resid=0, msg_status=0 status=2 sense=0
driver_status 8

This patch has been tested with linux and windows XP.

The VERIFY commands has been implemented (empty) because previously it
was simply ignored (and it is needed by windows to format disks).

Regards,
Laurent
-- 
----------------- address@hidden  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <scsi/sg.h>

/* modified file from /usr/share/doc/sg3-utils/examples/sg_simple16.c
 *  * Copyright (C) 2001 D. Gilbert
 *   */

#define READ10_REPLY_LEN 512
#define READ10_CMD_LEN 10

#define EBUFF_SZ 256

int main(int argc, char * argv[])
{
    int sg_fd, k, ok;
    unsigned char r10CmdBlk [READ10_CMD_LEN] =
                {0x28, 0, 0x7f, 0xff, 0xff, 0xff, 0, 0, 1, 0};
    sg_io_hdr_t io_hdr;
    char * file_name = 0;
    char ebuff[EBUFF_SZ];
    unsigned char inBuff[READ10_REPLY_LEN];
    unsigned char sense_buffer[32];

    for (k = 1; k < argc; ++k) {
        if (*argv[k] == '-') {
            printf("Unrecognized switch: %s\n", argv[k]);
            file_name = 0;
            break;
        }
        else if (0 == file_name)
            file_name = argv[k];
        else {
            printf("too many arguments\n");
            file_name = 0;
            break;
        }
    }
    if (0 == file_name) {
        printf("Usage: 'sg_simple10 <sg_device>'\n");
        return 1;
    }

    if ((sg_fd = open(file_name, O_RDWR)) < 0) {
        snprintf(ebuff, EBUFF_SZ,
                 "sg_simple10: error opening file: %s", file_name);
        perror(ebuff);
        return 1;
    }
    /* Prepare READ_10 command */
    memset(&io_hdr, 0, sizeof(sg_io_hdr_t));
    io_hdr.interface_id = 'S';
    io_hdr.cmd_len = sizeof(r10CmdBlk);
    /* io_hdr.iovec_count = 0; */  /* memset takes care of this */
    io_hdr.mx_sb_len = sizeof(sense_buffer);
    io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
    io_hdr.dxfer_len = READ10_REPLY_LEN;
    io_hdr.dxferp = inBuff;
    io_hdr.cmdp = r10CmdBlk;
    io_hdr.sbp = sense_buffer;
    io_hdr.timeout = 20000;     /* 20000 millisecs == 20 seconds */
    /* io_hdr.flags = 0; */     /* take defaults: indirect IO, etc */
    /* io_hdr.pack_id = 0; */
    /* io_hdr.usr_ptr = NULL; */

    if (ioctl(sg_fd, SG_IO, &io_hdr) < 0) {
        perror("sg_simple10: Inquiry SG_IO ioctl error");
        close(sg_fd);
        return 1;
    }

    printf("READ_10 duration=%u millisecs, resid=%d, msg_status=%d status=%d 
sense=%d driver_status %x\n",
           io_hdr.duration, io_hdr.resid, (int)io_hdr.msg_status,
           io_hdr.status, sense_buffer[2], io_hdr.driver_status);

    close(sg_fd);
    return 0;
}

Signed-off-by: Laurent Vivier <address@hidden>
---
 hw/scsi-disk.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Index: qemu/hw/scsi-disk.c
===================================================================
--- qemu.orig/hw/scsi-disk.c    2008-10-01 10:00:11.000000000 +0200
+++ qemu/hw/scsi-disk.c 2008-10-02 10:51:22.000000000 +0200
@@ -34,6 +34,9 @@ do { fprintf(stderr, "scsi-disk: " fmt ,
 #define SENSE_HARDWARE_ERROR  4
 #define SENSE_ILLEGAL_REQUEST 5
 
+#define STATUS_GOOD            0
+#define STATUS_CHECK_CONDITION 2
+
 #define SCSI_DMA_BUF_SIZE    131072
 
 typedef struct SCSIRequest {
@@ -124,7 +127,7 @@ static SCSIRequest *scsi_find_request(SC
 }
 
 /* Helper function for command completion.  */
-static void scsi_command_complete(SCSIRequest *r, int sense)
+static void scsi_command_complete(SCSIRequest *r, int status, int sense)
 {
     SCSIDeviceState *s = r->dev;
     uint32_t tag;
@@ -132,7 +135,7 @@ static void scsi_command_complete(SCSIRe
     s->sense = sense;
     tag = r->tag;
     scsi_remove_request(r);
-    s->completion(s->opaque, SCSI_REASON_DONE, tag, sense);
+    s->completion(s->opaque, SCSI_REASON_DONE, tag, status);
 }
 
 /* Cancel a pending data transfer.  */
@@ -157,7 +160,8 @@ static void scsi_read_complete(void * op
 
     if (ret) {
         DPRINTF("IO error\n");
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        s->completion(s->opaque, SCSI_REASON_DATA, r->tag, 0);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_NO_SENSE);
         return;
     }
     DPRINTF("Data ready tag=0x%x len=%d\n", r->tag, r->buf_len);
@@ -176,7 +180,7 @@ static void scsi_read_data(SCSIDevice *d
     if (!r) {
         BADF("Bad read tag 0x%x\n", tag);
         /* ??? This is the wrong error.  */
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
         return;
     }
     if (r->sector_count == (uint32_t)-1) {
@@ -187,7 +191,7 @@ static void scsi_read_data(SCSIDevice *d
     }
     DPRINTF("Read sector_count=%d\n", r->sector_count);
     if (r->sector_count == 0) {
-        scsi_command_complete(r, SENSE_NO_SENSE);
+        scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
         return;
     }
 
@@ -199,7 +203,7 @@ static void scsi_read_data(SCSIDevice *d
     r->aiocb = bdrv_aio_read(s->bdrv, r->sector, r->dma_buf, n,
                              scsi_read_complete, r);
     if (r->aiocb == NULL)
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
     r->sector += n;
     r->sector_count -= n;
 }
@@ -217,7 +221,7 @@ static void scsi_write_complete(void * o
 
     r->aiocb = NULL;
     if (r->sector_count == 0) {
-        scsi_command_complete(r, SENSE_NO_SENSE);
+        scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
     } else {
         len = r->sector_count * 512;
         if (len > SCSI_DMA_BUF_SIZE) {
@@ -241,7 +245,7 @@ static int scsi_write_data(SCSIDevice *d
     r = scsi_find_request(s, tag);
     if (!r) {
         BADF("Bad write tag 0x%x\n", tag);
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_HARDWARE_ERROR);
         return 1;
     }
     if (r->aiocb)
@@ -251,7 +255,8 @@ static int scsi_write_data(SCSIDevice *d
         r->aiocb = bdrv_aio_write(s->bdrv, r->sector, r->dma_buf, n,
                                   scsi_write_complete, r);
         if (r->aiocb == NULL)
-            scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+            scsi_command_complete(r, STATUS_CHECK_CONDITION,
+                                  SENSE_HARDWARE_ERROR);
         r->sector += n;
         r->sector_count -= n;
     } else {
@@ -670,7 +675,7 @@ static int32_t scsi_send_command(SCSIDev
             outbuf[7] = 0;
             r->buf_len = 8;
         } else {
-            scsi_command_complete(r, SENSE_NOT_READY);
+            scsi_command_complete(r, STATUS_CHECK_CONDITION, SENSE_NOT_READY);
             return 0;
         }
        break;
@@ -754,14 +759,17 @@ static int32_t scsi_send_command(SCSIDev
         outbuf[3] = 8;
         r->buf_len = 16;
         break;
+    case 0x2f:
+        DPRINTF("Verify (sector %d, count %d)\n", lba, len);
+        break;
     default:
        DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
     fail:
-        scsi_command_complete(r, SENSE_ILLEGAL_REQUEST);
+        scsi_command_complete(r, STATUS_CHECK_CONDITION, 
SENSE_ILLEGAL_REQUEST);
        return 0;
     }
     if (r->sector_count == 0 && r->buf_len == 0) {
-        scsi_command_complete(r, SENSE_NO_SENSE);
+        scsi_command_complete(r, STATUS_GOOD, SENSE_NO_SENSE);
     }
     len = r->sector_count * 512 + r->buf_len;
     if (is_write) {