Patchwork [06/15] scsi: Update sense code handling

login
register
mail settings
Submitter Hannes Reinecke
Date Nov. 24, 2010, 11:16 a.m.
Message ID <1290597370-21365-7-git-send-email-hare@suse.de>
Download mbox | patch
Permalink /patch/72866/
State New
Headers show

Comments

Hannes Reinecke - Nov. 24, 2010, 11:16 a.m.
The SCSI spec has a quite detailed list of sense codes available.
It even mandates the use of specific ones for some failure cases.
The current implementation just has one type of 'generic' error
which is actually a violation of the spec in certain cases.
This patch introduces various predefined sense codes to have the
sense code reporting more in line with the spec.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Acked-by: Christoph Hellwig <hch@lst.de>
---
 hw/scsi-bus.c     |   92 ++++++++++++++++++++++++++++++++++++++++++++
 hw/scsi-disk.c    |  109 +++++++++++++++++++++++++++--------------------------
 hw/scsi-generic.c |   76 ++++++++++++++++++++++++++-----------
 hw/scsi.h         |   38 ++++++++++++++++++
 4 files changed, 239 insertions(+), 76 deletions(-)
Kevin Wolf - Nov. 25, 2010, 2:33 p.m.
Am 24.11.2010 12:16, schrieb Hannes Reinecke:
> The SCSI spec has a quite detailed list of sense codes available.
> It even mandates the use of specific ones for some failure cases.
> The current implementation just has one type of 'generic' error
> which is actually a violation of the spec in certain cases.
> This patch introduces various predefined sense codes to have the
> sense code reporting more in line with the spec.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Acked-by: Christoph Hellwig <hch@lst.de>
> ---
>  hw/scsi-bus.c     |   92 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/scsi-disk.c    |  109 +++++++++++++++++++++++++++--------------------------
>  hw/scsi-generic.c |   76 ++++++++++++++++++++++++++-----------
>  hw/scsi.h         |   38 ++++++++++++++++++
>  4 files changed, 239 insertions(+), 76 deletions(-)
> 
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 93f0e9a..afdf0ad 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -388,6 +388,98 @@ int scsi_req_parse(SCSIRequest *req, uint8_t *buf)
>      return 0;
>  }
>  
> +/*
> + * Predefined sense codes
> + */
> +
> +/* No sense data available */
> +const struct SCSISense sense_code_NO_SENSE = {
> +    .key = NO_SENSE , .asc = 0x00 , .ascq = 0x00
> +};
> +
> +/* LUN not ready, Manual intervention required */
> +const struct SCSISense sense_code_LUN_NOT_READY = {
> +    .key = NOT_READY, .asc = 0x04, .ascq = 0x03
> +};
> +
> +/* LUN not ready, Medium not present */
> +const struct SCSISense sense_code_NO_MEDIUM = {
> +    .key = NOT_READY, .asc = 0x3a, .ascq = 0x00
> +};
> +
> +/* Hardware error, internal target failure */
> +const struct SCSISense sense_code_TARGET_FAILURE = {
> +    .key = HARDWARE_ERROR, .asc = 0x44, .ascq = 0x00
> +};
> +
> +/* Illegal request, invalid command operation code */
> +const struct SCSISense sense_code_INVALID_OPCODE = {
> +    .key = ILLEGAL_REQUEST, .asc = 0x20, .ascq = 0x00
> +};
> +
> +/* Illegal request, LBA out of range */
> +const struct SCSISense sense_code_LBA_OUT_OF_RANGE = {
> +    .key = ILLEGAL_REQUEST, .asc = 0x21, .ascq = 0x00
> +};
> +
> +/* Illegal request, Invalid field in CDB */
> +const struct SCSISense sense_code_INVALID_FIELD = {
> +    .key = ILLEGAL_REQUEST, .asc = 0x24, .ascq = 0x00
> +};
> +
> +/* Illegal request, LUN not supported */
> +const struct SCSISense sense_code_LUN_NOT_SUPPORTED = {
> +    .key = ILLEGAL_REQUEST, .asc = 0x25, .ascq = 0x00
> +};
> +
> +/* Command aborted, I/O process terminated */
> +const struct SCSISense sense_code_IO_ERROR = {
> +    .key = ABORTED_COMMAND, .asc = 0x00, .ascq = 0x06
> +};
> +
> +/* Command aborted, I_T Nexus loss occurred */
> +const struct SCSISense sense_code_I_T_NEXUS_LOSS = {
> +    .key = ABORTED_COMMAND, .asc = 0x29, .ascq = 0x07
> +};
> +
> +/* Command aborted, Logical Unit failure */
> +const struct SCSISense sense_code_LUN_FAILURE = {
> +    .key = ABORTED_COMMAND, .asc = 0x3e, .ascq = 0x01
> +};
> +
> +/*
> + * scsi_build_sense
> + *
> + * Build a sense buffer
> + */
> +int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed)
> +{
> +    if (len < 8)
> +        return 0;
> +    if (fixed && len < 14)
> +        return 0;
> +
> +    memset(buf, 0, len);
> +    if (fixed) {
> +        /* Return fixed format sense buffer */
> +        buf[0] = 0xf0;
> +        buf[2] = sense.key;
> +        buf[7] = 7;
> +        buf[12] = sense.asc;
> +        buf[13] = sense.ascq;
> +        len = 14;

My spec says: "Device servers shall return at least 18 bytes of
parameter data in response to a REQUEST SENSE command if the allocation
length is 18 or greater and the DESC bit is set to zero."

So should this be MIN(len, 18) instead?

Kevin
Hannes Reinecke - Dec. 21, 2010, 11:56 a.m.
On 11/25/2010 03:33 PM, Kevin Wolf wrote:
> Am 24.11.2010 12:16, schrieb Hannes Reinecke:
>> The SCSI spec has a quite detailed list of sense codes available.
>> It even mandates the use of specific ones for some failure cases.
>> The current implementation just has one type of 'generic' error
>> which is actually a violation of the spec in certain cases.
>> This patch introduces various predefined sense codes to have the
>> sense code reporting more in line with the spec.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Acked-by: Christoph Hellwig <hch@lst.de>
>> ---
>>  hw/scsi-bus.c     |   92 ++++++++++++++++++++++++++++++++++++++++++++
>>  hw/scsi-disk.c    |  109 +++++++++++++++++++++++++++--------------------------
>>  hw/scsi-generic.c |   76 ++++++++++++++++++++++++++-----------
>>  hw/scsi.h         |   38 ++++++++++++++++++
>>  4 files changed, 239 insertions(+), 76 deletions(-)
>>
>> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
>> index 93f0e9a..afdf0ad 100644
>> --- a/hw/scsi-bus.c
>> +++ b/hw/scsi-bus.c
>> @@ -388,6 +388,98 @@ int scsi_req_parse(SCSIRequest *req, uint8_t *buf)
>>      return 0;
>>  }
>>  
>> +/*
>> + * Predefined sense codes
>> + */
>> +
>> +/* No sense data available */
>> +const struct SCSISense sense_code_NO_SENSE = {
>> +    .key = NO_SENSE , .asc = 0x00 , .ascq = 0x00
>> +};
>> +
>> +/* LUN not ready, Manual intervention required */
>> +const struct SCSISense sense_code_LUN_NOT_READY = {
>> +    .key = NOT_READY, .asc = 0x04, .ascq = 0x03
>> +};
>> +
>> +/* LUN not ready, Medium not present */
>> +const struct SCSISense sense_code_NO_MEDIUM = {
>> +    .key = NOT_READY, .asc = 0x3a, .ascq = 0x00
>> +};
>> +
>> +/* Hardware error, internal target failure */
>> +const struct SCSISense sense_code_TARGET_FAILURE = {
>> +    .key = HARDWARE_ERROR, .asc = 0x44, .ascq = 0x00
>> +};
>> +
>> +/* Illegal request, invalid command operation code */
>> +const struct SCSISense sense_code_INVALID_OPCODE = {
>> +    .key = ILLEGAL_REQUEST, .asc = 0x20, .ascq = 0x00
>> +};
>> +
>> +/* Illegal request, LBA out of range */
>> +const struct SCSISense sense_code_LBA_OUT_OF_RANGE = {
>> +    .key = ILLEGAL_REQUEST, .asc = 0x21, .ascq = 0x00
>> +};
>> +
>> +/* Illegal request, Invalid field in CDB */
>> +const struct SCSISense sense_code_INVALID_FIELD = {
>> +    .key = ILLEGAL_REQUEST, .asc = 0x24, .ascq = 0x00
>> +};
>> +
>> +/* Illegal request, LUN not supported */
>> +const struct SCSISense sense_code_LUN_NOT_SUPPORTED = {
>> +    .key = ILLEGAL_REQUEST, .asc = 0x25, .ascq = 0x00
>> +};
>> +
>> +/* Command aborted, I/O process terminated */
>> +const struct SCSISense sense_code_IO_ERROR = {
>> +    .key = ABORTED_COMMAND, .asc = 0x00, .ascq = 0x06
>> +};
>> +
>> +/* Command aborted, I_T Nexus loss occurred */
>> +const struct SCSISense sense_code_I_T_NEXUS_LOSS = {
>> +    .key = ABORTED_COMMAND, .asc = 0x29, .ascq = 0x07
>> +};
>> +
>> +/* Command aborted, Logical Unit failure */
>> +const struct SCSISense sense_code_LUN_FAILURE = {
>> +    .key = ABORTED_COMMAND, .asc = 0x3e, .ascq = 0x01
>> +};
>> +
>> +/*
>> + * scsi_build_sense
>> + *
>> + * Build a sense buffer
>> + */
>> +int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed)
>> +{
>> +    if (len < 8)
>> +        return 0;
>> +    if (fixed && len < 14)
>> +        return 0;
>> +
>> +    memset(buf, 0, len);
>> +    if (fixed) {
>> +        /* Return fixed format sense buffer */
>> +        buf[0] = 0xf0;
>> +        buf[2] = sense.key;
>> +        buf[7] = 7;
>> +        buf[12] = sense.asc;
>> +        buf[13] = sense.ascq;
>> +        len = 14;
> 
> My spec says: "Device servers shall return at least 18 bytes of
> parameter data in response to a REQUEST SENSE command if the allocation
> length is 18 or greater and the DESC bit is set to zero."
> 
> So should this be MIN(len, 18) instead?
> 
Yes, you are correct.
And we should actually always return sense data, even if the length
is smaller than the minimum.

Fixed in my megasas git tree; will be included in the next round of
patches.

Cheers,

Hannes

Patch

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 93f0e9a..afdf0ad 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -388,6 +388,98 @@  int scsi_req_parse(SCSIRequest *req, uint8_t *buf)
     return 0;
 }
 
+/*
+ * Predefined sense codes
+ */
+
+/* No sense data available */
+const struct SCSISense sense_code_NO_SENSE = {
+    .key = NO_SENSE , .asc = 0x00 , .ascq = 0x00
+};
+
+/* LUN not ready, Manual intervention required */
+const struct SCSISense sense_code_LUN_NOT_READY = {
+    .key = NOT_READY, .asc = 0x04, .ascq = 0x03
+};
+
+/* LUN not ready, Medium not present */
+const struct SCSISense sense_code_NO_MEDIUM = {
+    .key = NOT_READY, .asc = 0x3a, .ascq = 0x00
+};
+
+/* Hardware error, internal target failure */
+const struct SCSISense sense_code_TARGET_FAILURE = {
+    .key = HARDWARE_ERROR, .asc = 0x44, .ascq = 0x00
+};
+
+/* Illegal request, invalid command operation code */
+const struct SCSISense sense_code_INVALID_OPCODE = {
+    .key = ILLEGAL_REQUEST, .asc = 0x20, .ascq = 0x00
+};
+
+/* Illegal request, LBA out of range */
+const struct SCSISense sense_code_LBA_OUT_OF_RANGE = {
+    .key = ILLEGAL_REQUEST, .asc = 0x21, .ascq = 0x00
+};
+
+/* Illegal request, Invalid field in CDB */
+const struct SCSISense sense_code_INVALID_FIELD = {
+    .key = ILLEGAL_REQUEST, .asc = 0x24, .ascq = 0x00
+};
+
+/* Illegal request, LUN not supported */
+const struct SCSISense sense_code_LUN_NOT_SUPPORTED = {
+    .key = ILLEGAL_REQUEST, .asc = 0x25, .ascq = 0x00
+};
+
+/* Command aborted, I/O process terminated */
+const struct SCSISense sense_code_IO_ERROR = {
+    .key = ABORTED_COMMAND, .asc = 0x00, .ascq = 0x06
+};
+
+/* Command aborted, I_T Nexus loss occurred */
+const struct SCSISense sense_code_I_T_NEXUS_LOSS = {
+    .key = ABORTED_COMMAND, .asc = 0x29, .ascq = 0x07
+};
+
+/* Command aborted, Logical Unit failure */
+const struct SCSISense sense_code_LUN_FAILURE = {
+    .key = ABORTED_COMMAND, .asc = 0x3e, .ascq = 0x01
+};
+
+/*
+ * scsi_build_sense
+ *
+ * Build a sense buffer
+ */
+int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed)
+{
+    if (len < 8)
+        return 0;
+    if (fixed && len < 14)
+        return 0;
+
+    memset(buf, 0, len);
+    if (fixed) {
+        /* Return fixed format sense buffer */
+        buf[0] = 0xf0;
+        buf[2] = sense.key;
+        buf[7] = 7;
+        buf[12] = sense.asc;
+        buf[13] = sense.ascq;
+        len = 14;
+    } else {
+        /* Return descriptor format sense buffer */
+        buf[0] = 0x72;
+        buf[1] = sense.key;
+        buf[2] = sense.asc;
+        buf[3] = sense.ascq;
+        len = 8;
+    }
+
+    return len;
+}
+
 static const char *scsi_command_name(uint8_t cmd)
 {
     static const char *names[] = {
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 58e5f5b..a71607e 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -49,10 +49,6 @@  do { fprintf(stderr, "scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 
 typedef struct SCSIDiskState SCSIDiskState;
 
-typedef struct SCSISense {
-    uint8_t key;
-} SCSISense;
-
 typedef struct SCSIDiskReq {
     SCSIRequest req;
     /* ??? We should probably keep track of whether the data transfer is
@@ -110,24 +106,19 @@  static void scsi_disk_clear_sense(SCSIDiskState *s)
     memset(&s->sense, 0, sizeof(s->sense));
 }
 
-static void scsi_disk_set_sense(SCSIDiskState *s, uint8_t key)
-{
-    s->sense.key = key;
-}
-
-static void scsi_req_set_status(SCSIDiskReq *r, int status, int sense_code)
+static void scsi_req_set_status(SCSIDiskReq *r, int status, SCSISense sense)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
 
     r->req.status = status;
-    scsi_disk_set_sense(s, sense_code);
+    s->sense = sense;
 }
 
 /* Helper function for command completion.  */
-static void scsi_command_complete(SCSIDiskReq *r, int status, int sense)
+static void scsi_command_complete(SCSIDiskReq *r, int status, SCSISense sense)
 {
-    DPRINTF("Command complete tag=0x%x status=%d sense=%d\n",
-            r->req.tag, status, sense);
+    DPRINTF("Command complete tag=0x%x status=%d sense=%d/%d/%d\n",
+            r->req.tag, status, sense.key, sense.asc, sense.ascq);
     scsi_req_set_status(r, status, sense);
     scsi_req_complete(&r->req);
     scsi_remove_request(r);
@@ -183,7 +174,7 @@  static void scsi_read_request(SCSIDiskReq *r)
     }
     DPRINTF("Read sector_count=%d\n", r->sector_count);
     if (r->sector_count == 0) {
-        scsi_command_complete(r, GOOD, NO_SENSE);
+        scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
         return;
     }
 
@@ -208,9 +199,12 @@  static void scsi_read_data(SCSIDevice *d, uint32_t tag)
 
     r = scsi_find_request(s, tag);
     if (!r) {
+        SCSIBus *bus;
+
         BADF("Bad read tag 0x%x\n", tag);
-        /* ??? This is the wrong error.  */
-        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
+        bus = scsi_bus_from_device(d);
+        s->sense = SENSE_CODE(I_T_NEXUS_LOSS);
+        bus->complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
         return;
     }
 
@@ -243,8 +237,13 @@  static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         if (type == SCSI_REQ_STATUS_RETRY_READ) {
             r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, 0);
         }
-        scsi_command_complete(r, CHECK_CONDITION,
-                HARDWARE_ERROR);
+        if (error == EBADR) {
+                scsi_command_complete(r, CHECK_CONDITION,
+                                      SENSE_CODE(TARGET_FAILURE));
+        } else {
+                scsi_command_complete(r, CHECK_CONDITION,
+                                      SENSE_CODE(IO_ERROR));
+        }
         bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
 
@@ -269,7 +268,7 @@  static void scsi_write_complete(void * opaque, int ret)
     r->sector += n;
     r->sector_count -= n;
     if (r->sector_count == 0) {
-        scsi_command_complete(r, GOOD, NO_SENSE);
+        scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
     } else {
         len = r->sector_count * 512;
         if (len > SCSI_DMA_BUF_SIZE) {
@@ -292,7 +291,7 @@  static void scsi_write_request(SCSIDiskReq *r)
         r->req.aiocb = bdrv_aio_writev(s->bs, r->sector, &r->qiov, n,
                                    scsi_write_complete, r);
         if (r->req.aiocb == NULL) {
-            scsi_write_complete(r, -EIO);
+            scsi_write_complete(r, -EBADR);
         }
     } else {
         /* Invoke completion routine to fetch data from host.  */
@@ -310,8 +309,12 @@  static int scsi_write_data(SCSIDevice *d, uint32_t tag)
     DPRINTF("Write data tag=0x%x\n", tag);
     r = scsi_find_request(s, tag);
     if (!r) {
+        SCSIBus *bus;
+
         BADF("Bad write tag 0x%x\n", tag);
-        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
+        bus = scsi_bus_from_device(d);
+        s->sense = SENSE_CODE(I_T_NEXUS_LOSS);
+        bus->complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
         return 1;
     }
 
@@ -351,7 +354,7 @@  static void scsi_dma_restart_bh(void *opaque)
             case SCSI_REQ_STATUS_RETRY_FLUSH:
                 ret = scsi_disk_emulate_command(r, r->iov.iov_base);
                 if (ret == 0) {
-                    scsi_command_complete(r, GOOD, NO_SENSE);
+                    scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
                 }
             }
         }
@@ -833,30 +836,19 @@  static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
     case TEST_UNIT_READY:
         if (!bdrv_is_inserted(s->bs))
             goto not_ready;
-	break;
+        break;
     case REQUEST_SENSE:
         if (req->cmd.xfer < 4)
             goto illegal_request;
-        memset(outbuf, 0, 4);
-        buflen = 4;
-        if (s->sense.key == NOT_READY && req->cmd.xfer >= 18) {
-            memset(outbuf, 0, 18);
-            buflen = 18;
-            outbuf[7] = 10;
-            /* asc 0x3a, ascq 0: Medium not present */
-            outbuf[12] = 0x3a;
-            outbuf[13] = 0;
-        }
-        outbuf[0] = 0xf0;
-        outbuf[1] = 0;
-        outbuf[2] = s->sense.key;
+        buflen = scsi_build_sense(s->sense, outbuf, req->cmd.xfer,
+                                  req->cmd.xfer > 13);
         scsi_disk_clear_sense(s);
         break;
     case INQUIRY:
         buflen = scsi_disk_emulate_inquiry(req, outbuf);
         if (buflen < 0)
             goto illegal_request;
-	break;
+        break;
     case MODE_SENSE:
     case MODE_SENSE_10:
         buflen = scsi_disk_emulate_mode_sense(req, outbuf);
@@ -889,14 +881,14 @@  static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
             /* load/eject medium */
             bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
         }
-	break;
+        break;
     case ALLOW_MEDIUM_REMOVAL:
         bdrv_set_locked(s->bs, req->cmd.buf[4] & 1);
-	break;
+        break;
     case READ_CAPACITY:
         /* The normal LEN field for this command is zero.  */
-	memset(outbuf, 0, 8);
-	bdrv_get_geometry(s->bs, &nb_sectors);
+        memset(outbuf, 0, 8);
+        bdrv_get_geometry(s->bs, &nb_sectors);
         if (!nb_sectors)
             goto not_ready;
         nb_sectors /= s->cluster_size;
@@ -916,7 +908,7 @@  static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
         outbuf[6] = s->cluster_size * 2;
         outbuf[7] = 0;
         buflen = 8;
-	break;
+        break;
     case SYNCHRONIZE_CACHE:
         ret = bdrv_flush(s->bs);
         if (ret < 0) {
@@ -981,17 +973,22 @@  static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
         }
         break;
     default:
-        goto illegal_request;
+        scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
+        return -1;
     }
-    scsi_req_set_status(r, GOOD, NO_SENSE);
+    scsi_req_set_status(r, GOOD, SENSE_CODE(NO_SENSE));
     return buflen;
 
 not_ready:
-    scsi_command_complete(r, CHECK_CONDITION, NOT_READY);
+    if (!bdrv_is_inserted(s->bs)) {
+        scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(NO_MEDIUM));
+    } else {
+        scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(LUN_NOT_READY));
+    }
     return -1;
 
 illegal_request:
-    scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
+    scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_FIELD));
     return -1;
 }
 
@@ -1026,7 +1023,8 @@  static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
 
     if (scsi_req_parse(&r->req, buf) != 0) {
         BADF("Unsupported command length, command %x\n", command);
-        goto fail;
+        scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
+        return 0;
     }
 #ifdef DEBUG_SCSI
     {
@@ -1041,8 +1039,11 @@  static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
     if (lun || buf[1] >> 5) {
         /* Only LUN 0 supported.  */
         DPRINTF("Unimplemented LUN %d\n", lun ? lun : buf[1] >> 5);
-        if (command != REQUEST_SENSE && command != INQUIRY)
-            goto fail;
+        if (command != REQUEST_SENSE && command != INQUIRY) {
+            scsi_command_complete(r, CHECK_CONDITION,
+                                  SENSE_CODE(LUN_NOT_SUPPORTED));
+            return 0;
+        }
     }
     switch (command) {
     case TEST_UNIT_READY:
@@ -1125,15 +1126,17 @@  static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
         break;
     default:
         DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
+        scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_OPCODE));
+        return 0;
     fail:
-        scsi_command_complete(r, CHECK_CONDITION, ILLEGAL_REQUEST);
+        scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(INVALID_FIELD));
         return 0;
     illegal_lba:
-        scsi_command_complete(r, CHECK_CONDITION, HARDWARE_ERROR);
+        scsi_command_complete(r, CHECK_CONDITION, SENSE_CODE(LBA_OUT_OF_RANGE));
         return 0;
     }
     if (r->sector_count == 0 && r->iov.iov_len == 0) {
-        scsi_command_complete(r, GOOD, NO_SENSE);
+        scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE));
     }
     len = r->sector_count * 512 + r->iov.iov_len;
     if (is_write) {
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 9be1cca..a095f64 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -66,6 +66,23 @@  struct SCSIGenericState
     uint8_t senselen;
 };
 
+static int scsi_set_sense(SCSIGenericState *s, SCSISense sense)
+{
+    int len;
+
+    len = scsi_build_sense(sense, s->sensebuf, SCSI_SENSE_BUF_SIZE, 0);
+    s->driver_status = SG_ERR_DRIVER_SENSE;
+
+    return len;
+}
+
+static void scsi_clear_sense(SCSIGenericState *s)
+{
+    memset(s->sensebuf, 0, SCSI_SENSE_BUF_SIZE);
+    s->senselen = 0;
+    s->driver_status = 0;
+}
+
 static SCSIGenericReq *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
 {
     SCSIRequest *req;
@@ -95,18 +112,31 @@  static void scsi_command_complete(void *opaque, int ret)
     if (s->driver_status & SG_ERR_DRIVER_SENSE)
         s->senselen = r->io_header.sb_len_wr;
 
-    if (ret != 0)
-        r->req.status = BUSY;
-    else {
+    if (ret != 0) {
+        switch(ret) {
+            case -EINVAL:
+                s->senselen = scsi_set_sense(s, SENSE_CODE(TARGET_FAILURE));
+                break;
+            case -EBADR:
+                s->senselen = scsi_set_sense(s, SENSE_CODE(TARGET_FAILURE));
+                break;
+            default:
+                s->senselen = scsi_set_sense(s, SENSE_CODE(IO_ERROR));
+                break;
+        }
+        s->driver_status = SG_ERR_DRIVER_SENSE;
+        r->req.status = CHECK_CONDITION;
+    } else {
         if (s->driver_status & SG_ERR_DRIVER_TIMEOUT) {
             r->req.status = BUSY;
             BADF("Driver Timeout\n");
-        } else if (r->io_header.status)
+        } else if (r->io_header.status) {
             r->req.status = r->io_header.status;
-        else if (s->driver_status & SG_ERR_DRIVER_SENSE)
+        } else if (s->driver_status & SG_ERR_DRIVER_SENSE) {
             r->req.status = CHECK_CONDITION;
-        else
+        } else {
             r->req.status = GOOD;
+        }
     }
     DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
             r, r->req.tag, r->req.status);
@@ -187,9 +217,12 @@  static void scsi_read_data(SCSIDevice *d, uint32_t tag)
     DPRINTF("scsi_read_data 0x%x\n", tag);
     r = scsi_find_request(s, tag);
     if (!r) {
+        SCSIBus *bus;
+
         BADF("Bad read tag 0x%x\n", tag);
-        /* ??? This is the wrong error.  */
-        scsi_command_complete(r, -EINVAL);
+        scsi_set_sense(s, SENSE_CODE(I_T_NEXUS_LOSS));
+        bus = scsi_bus_from_device(d);
+        bus->complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
         return;
     }
 
@@ -211,12 +244,14 @@  static void scsi_read_data(SCSIDevice *d, uint32_t tag)
                 r->buf[0], r->buf[1], r->buf[2], r->buf[3],
                 r->buf[4], r->buf[5], r->buf[6], r->buf[7]);
         r->req.bus->complete(r->req.bus, SCSI_REASON_DATA, r->req.tag, s->senselen);
+        /* Clear sensebuf after REQUEST_SENSE */
+        scsi_clear_sense(s);
         return;
     }
 
     ret = execute_command(s->bs, r, SG_DXFER_FROM_DEV, scsi_read_complete);
     if (ret == -1) {
-        scsi_command_complete(r, -EINVAL);
+        scsi_command_complete(r, -EBADR);
         return;
     }
 }
@@ -253,9 +288,12 @@  static int scsi_write_data(SCSIDevice *d, uint32_t tag)
     DPRINTF("scsi_write_data 0x%x\n", tag);
     r = scsi_find_request(s, tag);
     if (!r) {
+        SCSIBus *bus;
+
         BADF("Bad write tag 0x%x\n", tag);
-        /* ??? This is the wrong error.  */
-        scsi_command_complete(r, -EINVAL);
+        scsi_set_sense(s, SENSE_CODE(I_T_NEXUS_LOSS));
+        bus = scsi_bus_from_device(d);
+        bus->complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
         return 0;
     }
 
@@ -267,7 +305,7 @@  static int scsi_write_data(SCSIDevice *d, uint32_t tag)
 
     ret = execute_command(s->bs, r, SG_DXFER_TO_DEV, scsi_write_complete);
     if (ret == -1) {
-        scsi_command_complete(r, -EINVAL);
+        scsi_command_complete(r, -EBADR);
         return 1;
     }
 
@@ -323,15 +361,7 @@  static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
         (lun != s->lun || (cmd[1] >> 5) != s->lun)) {
         DPRINTF("Unimplemented LUN %d\n", lun ? lun : cmd[1] >> 5);
 
-        s->sensebuf[0] = 0x70;
-        s->sensebuf[1] = 0x00;
-        s->sensebuf[2] = ILLEGAL_REQUEST;
-        s->sensebuf[3] = 0x00;
-        s->sensebuf[4] = 0x00;
-        s->sensebuf[5] = 0x00;
-        s->sensebuf[6] = 0x00;
-        s->senselen = 7;
-        s->driver_status = SG_ERR_DRIVER_SENSE;
+        scsi_set_sense(s, SENSE_CODE(LUN_NOT_SUPPORTED));
         bus = scsi_bus_from_device(d);
         bus->complete(bus, SCSI_REASON_DONE, tag, CHECK_CONDITION);
         return 0;
@@ -346,7 +376,7 @@  static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
 
     if (-1 == scsi_req_parse(&r->req, cmd)) {
         BADF("Unsupported command length, command %x\n", cmd[0]);
-        scsi_remove_request(r);
+        scsi_command_complete(r, -EINVAL);
         return 0;
     }
     scsi_req_fixup(&r->req);
@@ -371,7 +401,7 @@  static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag,
         r->buf = NULL;
         ret = execute_command(s->bs, r, SG_DXFER_NONE, scsi_command_complete);
         if (ret == -1) {
-            scsi_command_complete(r, -EINVAL);
+            scsi_command_complete(r, -EBADR);
             return 0;
         }
         return 0;
diff --git a/hw/scsi.h b/hw/scsi.h
index bf02adf..1196122 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -26,6 +26,12 @@  enum SCSIXferMode {
     SCSI_XFER_TO_DEV,    /*  WRITE, MODE_SELECT, ...         */
 };
 
+typedef struct SCSISense {
+    uint8_t key;
+    uint8_t asc;
+    uint8_t ascq;
+} SCSISense;
+
 typedef struct SCSIRequest {
     SCSIBus           *bus;
     SCSIDevice        *dev;
@@ -97,6 +103,38 @@  static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
 SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit);
 int scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
 
+/*
+ * Predefined sense codes
+ */
+
+/* No sense data available */
+extern const struct SCSISense sense_code_NO_SENSE;
+/* LUN not ready, Manual intervention required */
+extern const struct SCSISense sense_code_LUN_NOT_READY;
+/* LUN not ready, Medium not present */
+extern const struct SCSISense sense_code_NO_MEDIUM;
+/* Hardware error, internal target failure */
+extern const struct SCSISense sense_code_TARGET_FAILURE;
+/* Illegal request, invalid command operation code */
+extern const struct SCSISense sense_code_INVALID_OPCODE;
+/* Illegal request, LBA out of range */
+extern const struct SCSISense sense_code_LBA_OUT_OF_RANGE;
+/* Illegal request, Invalid field in CDB */
+extern const struct SCSISense sense_code_INVALID_FIELD;
+/* Illegal request, LUN not supported */
+extern const struct SCSISense sense_code_LUN_NOT_SUPPORTED;
+/* Command aborted, I/O process terminated */
+extern const struct SCSISense sense_code_IO_ERROR;
+/* Command aborted, I_T Nexus loss occurred */
+extern const struct SCSISense sense_code_I_T_NEXUS_LOSS;
+/* Command aborted, Logical Unit failure */
+extern const struct SCSISense sense_code_LUN_FAILURE;
+
+#define SENSE_CODE(x) sense_code_ ## x
+
+int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed);
+int scsi_sense_valid(SCSISense sense);
+
 SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun);
 SCSIRequest *scsi_req_find(SCSIDevice *d, uint32_t tag);
 void scsi_req_free(SCSIRequest *req);