[Qemu-devel] [patch 0/3] properly report SCSI status codes

[Qemu-devel] [patch 0/3] properly report SCSI status codes

[Marcelo Tosatti]

The scsi-disk implementation passes the sense key information to the 
driver ->completion() callback.

The LSI driver (and esp / usb storage too, apparently) are expecting a
scsi status code. The sense key information should be returned in 
the sense buffer or later upon a REQUEST SENSE command.

For example, right now the guest interprets a completed command with 
HARDWARE_ERROR sense key as scsi status code "CONDITION MET".

As a separate note, the current barrier implementation in Linux ignores
SCSI status errors (James Bottomley sent a patch to linux-scsi a few 
minutes ago on that). 

-- 

[Qemu-devel] [patch 1/3] Fix scsi-disk sense-key/status confusion

[Marcelo Tosatti]

[-- Attachment #1: scsi-status-code --]
[-- Type: text/plain, Size: 5190 bytes --]

The scsi-disk implementation sends out sense key information as
status code.

The sense key should be reported in the sense buffer, or later upon
REQUEST SENSE command.


Index: kvm-userspace.io/qemu/hw/scsi-disk.c
===================================================================
--- kvm-userspace.io.orig/qemu/hw/scsi-disk.c
+++ kvm-userspace.io/qemu/hw/scsi-disk.c
@@ -34,6 +34,18 @@ do { fprintf(stderr, "scsi-disk: " fmt ,
 #define SENSE_HARDWARE_ERROR  4
 #define SENSE_ILLEGAL_REQUEST 5
 
+#define SCSI_OK 0
+#define SCSI_CHECK_COND 0x2
+#define SCSI_COND_MET 0x4
+#define SCSI_BUSY 0x8
+#define SCSI_INTERMEDIATE 0x10
+#define SCSI_INTER_MET 0x14
+#define SCSI_RES_CONFLICT 0x18
+#define SCSI_CMD_TERMINATED 0x22
+#define SCSI_QUEUE_FULL 0x28
+#define SCSI_ACA_ACTIVE 0x30
+#define SCSI_TASK_ABORTED 0x40
+
 #define SCSI_DMA_BUF_SIZE    65536
 
 typedef struct SCSIRequest {
@@ -124,7 +136,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 +144,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 +169,7 @@ static void scsi_read_complete(void * op
 
     if (ret) {
         DPRINTF("IO error\n");
-        scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+        scsi_command_complete(r, SCSI_CHECK_COND, SENSE_HARDWARE_ERROR);
         return;
     }
     DPRINTF("Data ready tag=0x%x len=%d\n", r->tag, r->buf_len);
@@ -175,8 +187,7 @@ static void scsi_read_data(SCSIDevice *d
     r = scsi_find_request(s, tag);
     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, SCSI_CHECK_COND, SENSE_HARDWARE_ERROR);
         return;
     }
     if (r->sector_count == (uint32_t)-1) {
@@ -187,7 +198,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, SCSI_OK, SENSE_NO_SENSE);
         return;
     }
 
@@ -199,7 +210,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, SCSI_CHECK_COND, SENSE_HARDWARE_ERROR);
     r->sector += n;
     r->sector_count -= n;
 }
@@ -217,7 +228,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, SCSI_OK, SENSE_NO_SENSE);
     } else {
         len = r->sector_count * 512;
         if (len > SCSI_DMA_BUF_SIZE) {
@@ -241,7 +252,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, SCSI_CHECK_COND, SENSE_HARDWARE_ERROR);
         return 1;
     }
     if (r->aiocb)
@@ -251,7 +262,7 @@ 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, SCSI_CHECK_COND, SENSE_HARDWARE_ERROR);
         r->sector += n;
         r->sector_count -= n;
     } else {
@@ -602,7 +613,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, SCSI_CHECK_COND, SENSE_NOT_READY);
             return 0;
         }
 	break;
@@ -624,7 +635,7 @@ static int32_t scsi_send_command(SCSIDev
         ret = bdrv_flush(s->bdrv);
         if (ret) {
             DPRINTF("IO error on bdrv_flush\n");
-            scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+            scsi_command_complete(r, SCSI_CHECK_COND, SENSE_HARDWARE_ERROR);
             return 0;
         }
         break;
@@ -694,11 +705,11 @@ static int32_t scsi_send_command(SCSIDev
     default:
 	DPRINTF("Unknown SCSI command (%2.2x)\n", buf[0]);
     fail:
-        scsi_command_complete(r, SENSE_ILLEGAL_REQUEST);
+        scsi_command_complete(r, SCSI_CHECK_COND, 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, SCSI_OK, SENSE_NO_SENSE);
     }
     len = r->sector_count * 512 + r->buf_len;
     if (is_write) {

-- 

[Qemu-devel] [patch 2/3] scsi-disk aio_flush

[Marcelo Tosatti]

[-- Attachment #1: scsi-disk-flush --]
[-- Type: text/plain, Size: 1361 bytes --]

On top of Ian's bdrv_flush error / bdrv_aio_patches.

This is similar to his recent patch, but uses the new scsi_command_complete()
arguments.

Index: kvm-userspace.io/qemu/hw/scsi-disk.c
===================================================================
--- kvm-userspace.io.orig/qemu/hw/scsi-disk.c
+++ kvm-userspace.io/qemu/hw/scsi-disk.c
@@ -273,6 +273,16 @@ static int scsi_write_data(SCSIDevice *d
     return 0;
 }
 
+static void scsi_flush_complete(void *opaque, int ret)
+{
+    SCSIRequest *r = (SCSIRequest *)opaque;
+
+    if (ret)
+        scsi_command_complete(r, SCSI_CHECK_COND, SENSE_HARDWARE_ERROR);
+    else
+        scsi_command_complete(r, SCSI_OK, SENSE_NO_SENSE);
+}
+
 /* Return a pointer to the data buffer.  */
 static uint8_t *scsi_get_buf(SCSIDevice *d, uint32_t tag)
 {
@@ -632,13 +642,8 @@ static int32_t scsi_send_command(SCSIDev
         break;
     case 0x35:
         DPRINTF("Syncronise cache (sector %d, count %d)\n", lba, len);
-        ret = bdrv_flush(s->bdrv);
-        if (ret) {
-            DPRINTF("IO error on bdrv_flush\n");
-            scsi_command_complete(r, SCSI_CHECK_COND, SENSE_HARDWARE_ERROR);
-            return 0;
-        }
-        break;
+        bdrv_aio_flush(s->bdrv, scsi_flush_complete, r);
+        return 0;
     case 0x43:
         {
             int start_track, format, msf, toclen;

-- 

[Qemu-devel] [patch 3/3] LSI rename sense variable

[Marcelo Tosatti]

[-- Attachment #1: lsi-fix --]
[-- Type: text/plain, Size: 1708 bytes --]

Rename "sense" to "status", which is what it actually means.


Index: kvm-userspace.io/qemu/hw/lsi53c895a.c
===================================================================
--- kvm-userspace.io.orig/qemu/hw/lsi53c895a.c
+++ kvm-userspace.io/qemu/hw/lsi53c895a.c
@@ -171,7 +171,7 @@ typedef struct {
     uint32_t script_ram_base;
 
     int carry; /* ??? Should this be an a visible register somewhere?  */
-    int sense;
+    int status;
     /* Action to take at the end of a MSG IN phase.
        0 = COMMAND, 1 = disconect, 2 = DATA OUT, 3 = DATA IN.  */
     int msg_action;
@@ -597,8 +597,8 @@ static void lsi_command_complete(void *o
 
     out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
     if (reason == SCSI_REASON_DONE) {
-        DPRINTF("Command complete sense=%d\n", (int)arg);
-        s->sense = arg;
+        DPRINTF("Command complete status=%d\n", (int)arg);
+        s->status = arg;
         s->command_complete = 2;
         if (s->waiting && s->dbc != 0) {
             /* Raise phase mismatch for short transfers.  */
@@ -665,14 +665,14 @@ static void lsi_do_command(LSIState *s)
 
 static void lsi_do_status(LSIState *s)
 {
-    uint8_t sense;
-    DPRINTF("Get status len=%d sense=%d\n", s->dbc, s->sense);
+    uint8_t status;
+    DPRINTF("Get status len=%d status=%d\n", s->dbc, s->status);
     if (s->dbc != 1)
         BADF("Bad Status move\n");
     s->dbc = 1;
-    sense = s->sense;
-    s->sfbr = sense;
-    cpu_physical_memory_write(s->dnad, &sense, 1);
+    status = s->status;
+    s->sfbr = status;
+    cpu_physical_memory_write(s->dnad, &status, 1);
     lsi_set_phase(s, PHASE_MI);
     s->msg_action = 1;
     lsi_add_msg_byte(s, 0); /* COMMAND COMPLETE */

--