[Qemu-devel] [PATCH 0/5] scsi related fixes

[Qemu-devel] [PATCH 0/5] scsi related fixes

[Christian Borntraeger]

Paolo,

this patch series was created during bringup of virtio-scsi on s390x.
We used a tape library as a test vehicle.

so here is the set of patches that allowed us to use it via virtio-scsi.
Any comments? Ok to apply?

Christian


Christian Hoff (5):
  scsi: Fix data length == SCSI_SENSE_BUF_SIZE
  scsi: Fix LOAD_UNLOAD
  scsi: Add basic support for SCSI media changer commands.
  scsi: Fix transfer length for READ POSITION commands.
  scsi: Ensure command and transfer lengths are set for all SCSI stream
    commands.

 hw/scsi-bus.c  |   91 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 hw/scsi-defs.h |   14 ++++++++-
 2 files changed, 93 insertions(+), 12 deletions(-)

[Qemu-devel] [PATCH 1/5] scsi: Fix data length == SCSI_SENSE_BUF_SIZE

[Christian Borntraeger]

From: Christian Hoff <christian.hoff@de.ibm.com>

Fix the edge case where the sense data length is exactly the same
as SCSI_SENSE_BUF_SIZE.
This makes SCSI requests work that use all of the available 95 byte
sense data.

Signed-off-by: Christian Hoff <christian.hoff@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/scsi-bus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 64e709e..d1779a2 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -1295,7 +1295,7 @@ void scsi_req_complete(SCSIRequest *req, int status)
     assert(req->status == -1);
     req->status = status;
 
-    assert(req->sense_len < sizeof(req->sense));
+    assert(req->sense_len <= sizeof(req->sense));
     if (status == GOOD) {
         req->sense_len = 0;
     }
-- 
1.7.0.4

[Qemu-devel] [PATCH 2/5] scsi: Fix LOAD_UNLOAD

[Christian Borntraeger]

From: Christian Hoff <christian.hoff@de.ibm.com>

Change operation code of LOAD_UNLOAD command to 0x1b as described in
section 7.3 of the SCSI Stream Commands spec.

Signed-off-by: Christian Hoff <christian.hoff@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/scsi-bus.c  |    6 +++---
 hw/scsi-defs.h |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index d1779a2..0e484d2 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -721,7 +721,6 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
     case SYNCHRONIZE_CACHE_16:
     case LOCATE_16:
     case LOCK_UNLOCK_CACHE:
-    case LOAD_UNLOAD:
     case SET_CD_SPEED:
     case SET_LIMITS:
     case WRITE_LONG_10:
@@ -833,7 +832,7 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
         }
         break;
     case REWIND:
-    case START_STOP:
+    case LOAD_UNLOAD:
         cmd->len = 6;
         cmd->xfer = 0;
         break;
@@ -1128,7 +1127,8 @@ static const char *scsi_command_name(uint8_t cmd)
         [ COPY                     ] = "COPY",
         [ ERASE                    ] = "ERASE",
         [ MODE_SENSE               ] = "MODE_SENSE",
-        [ START_STOP               ] = "START_STOP",
+        [ START_STOP               ] = "START_STOP/LOAD_UNLOAD",
+        /* LOAD_UNLOAD and START_STOP use the same operation code */
         [ RECEIVE_DIAGNOSTIC       ] = "RECEIVE_DIAGNOSTIC",
         [ SEND_DIAGNOSTIC          ] = "SEND_DIAGNOSTIC",
         [ ALLOW_MEDIUM_REMOVAL     ] = "ALLOW_MEDIUM_REMOVAL",
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index 354ed7b..2b0db4b 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -44,6 +44,7 @@
 #define COPY                  0x18
 #define ERASE                 0x19
 #define MODE_SENSE            0x1a
+#define LOAD_UNLOAD           0x1b
 #define START_STOP            0x1b
 #define RECEIVE_DIAGNOSTIC    0x1c
 #define SEND_DIAGNOSTIC       0x1d
@@ -114,7 +115,6 @@
 #define MAINTENANCE_IN        0xa3
 #define MAINTENANCE_OUT       0xa4
 #define MOVE_MEDIUM           0xa5
-#define LOAD_UNLOAD           0xa6
 #define SET_READ_AHEAD        0xa7
 #define READ_12               0xa8
 #define WRITE_12              0xaa
-- 
1.7.0.4

[Qemu-devel] [PATCH 3/5] scsi: Add basic support for SCSI media changer commands.

[Christian Borntraeger]

From: Christian Hoff <christian.hoff@de.ibm.com>

This adds basic support for SCSI media changer commands.
Not all commands are supported as of now, but enough to cover
basic functionality.

Signed-off-by: Christian Hoff <christian.hoff@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/scsi-bus.c  |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/scsi-defs.h |    4 +++
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 0e484d2..a93d3da 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -724,7 +724,6 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
     case SET_CD_SPEED:
     case SET_LIMITS:
     case WRITE_LONG_10:
-    case MOVE_MEDIUM:
     case UPDATE_BLOCK:
     case RESERVE_TRACK:
     case SET_READ_AHEAD:
@@ -852,6 +851,43 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
     return 0;
 }
 
+static int scsi_req_medium_changer_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+{
+    switch (buf[0]) {
+    /* medium changer commands */
+    case EXCHANGE_MEDIUM:
+        cmd->xfer = 0;
+        cmd->len = 12;
+        break;
+    case INITIALIZE_ELEMENT_STATUS:
+        cmd->xfer = 0;
+        cmd->len = 6;
+        break;
+    case INITIALIZE_ELEMENT_STATUS_WITH_RANGE:
+        cmd->xfer = 0;
+        cmd->len = 10;
+        break;
+    case MOVE_MEDIUM:
+        cmd->xfer = 0;
+        cmd->len = 12;
+        break;
+    case POSITION_TO_ELEMENT:
+        cmd->xfer = 0;
+        cmd->len = 10;
+        break;
+    case READ_ELEMENT_STATUS:
+        cmd->xfer = buf[9] | (buf[8] << 8) | (buf[7] << 16);
+        cmd->len = 12;
+        break;
+
+    /* generic commands */
+    default:
+        return scsi_req_length(cmd, dev, buf);
+    }
+    return 0;
+}
+
+
 static void scsi_cmd_xfer_mode(SCSICommand *cmd)
 {
     switch (cmd->buf[0]) {
@@ -928,11 +964,18 @@ int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
     int rc;
 
-    if (dev->type == TYPE_TAPE) {
+    switch (dev->type) {
+    case TYPE_TAPE:
         rc = scsi_req_stream_length(cmd, dev, buf);
-    } else {
+        break;
+    case TYPE_MEDIUM_CHANGER:
+        rc = scsi_req_medium_changer_length(cmd, dev, buf);
+        break;
+    default:
         rc = scsi_req_length(cmd, dev, buf);
+        break;
     }
+
     if (rc != 0)
         return rc;
 
@@ -1110,7 +1153,8 @@ static const char *scsi_command_name(uint8_t cmd)
         [ REQUEST_SENSE            ] = "REQUEST_SENSE",
         [ FORMAT_UNIT              ] = "FORMAT_UNIT",
         [ READ_BLOCK_LIMITS        ] = "READ_BLOCK_LIMITS",
-        [ REASSIGN_BLOCKS          ] = "REASSIGN_BLOCKS",
+        [ REASSIGN_BLOCKS          ] = "REASSIGN_BLOCKS/INITIALIZE ELEMENT STATUS",
+        /* LOAD_UNLOAD and INITIALIZE_ELEMENT_STATUS use the same operation code */
         [ READ_6                   ] = "READ_6",
         [ WRITE_6                  ] = "WRITE_6",
         [ SET_CAPACITY             ] = "SET_CAPACITY",
@@ -1135,7 +1179,8 @@ static const char *scsi_command_name(uint8_t cmd)
         [ READ_CAPACITY_10         ] = "READ_CAPACITY_10",
         [ READ_10                  ] = "READ_10",
         [ WRITE_10                 ] = "WRITE_10",
-        [ SEEK_10                  ] = "SEEK_10",
+        [ SEEK_10                  ] = "SEEK_10/POSITION_TO_ELEMENT",
+        /* SEEK_10 and POSITION_TO_ELEMENT use the same operation code */
         [ WRITE_VERIFY_10          ] = "WRITE_VERIFY_10",
         [ VERIFY_10                ] = "VERIFY_10",
         [ SEARCH_HIGH              ] = "SEARCH_HIGH",
@@ -1146,7 +1191,8 @@ static const char *scsi_command_name(uint8_t cmd)
         /* READ_POSITION and PRE_FETCH use the same operation code */
         [ SYNCHRONIZE_CACHE        ] = "SYNCHRONIZE_CACHE",
         [ LOCK_UNLOCK_CACHE        ] = "LOCK_UNLOCK_CACHE",
-        [ READ_DEFECT_DATA         ] = "READ_DEFECT_DATA",
+        [ READ_DEFECT_DATA         ] = "READ_DEFECT_DATA/INITIALIZE_ELEMENT_STATUS_WITH_RANGE",
+        /* READ_DEFECT_DATA and INITIALIZE_ELEMENT_STATUS_WITH_RANGE use the same operation code */
         [ MEDIUM_SCAN              ] = "MEDIUM_SCAN",
         [ COMPARE                  ] = "COMPARE",
         [ COPY_VERIFY              ] = "COPY_VERIFY",
@@ -1190,6 +1236,7 @@ static const char *scsi_command_name(uint8_t cmd)
         [ REPORT_LUNS              ] = "REPORT_LUNS",
         [ BLANK                    ] = "BLANK",
         [ MOVE_MEDIUM              ] = "MOVE_MEDIUM",
+        [ EXCHANGE_MEDIUM          ] = "EXCHANGE MEDIUM",
         [ LOAD_UNLOAD              ] = "LOAD_UNLOAD",
         [ READ_12                  ] = "READ_12",
         [ WRITE_12                 ] = "WRITE_12",
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index 2b0db4b..2c40855 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -29,6 +29,7 @@
 #define REQUEST_SENSE         0x03
 #define FORMAT_UNIT           0x04
 #define READ_BLOCK_LIMITS     0x05
+#define INITIALIZE_ELEMENT_STATUS 0x07
 #define REASSIGN_BLOCKS       0x07
 #define READ_6                0x08
 #define WRITE_6               0x0a
@@ -54,6 +55,7 @@
 #define WRITE_10              0x2a
 #define SEEK_10               0x2b
 #define LOCATE_10             0x2b
+#define POSITION_TO_ELEMENT   0x2b
 #define WRITE_VERIFY_10       0x2e
 #define VERIFY_10             0x2f
 #define SEARCH_HIGH           0x30
@@ -64,6 +66,7 @@
 #define READ_POSITION         0x34
 #define SYNCHRONIZE_CACHE     0x35
 #define LOCK_UNLOCK_CACHE     0x36
+#define INITIALIZE_ELEMENT_STATUS_WITH_RANGE 0x37
 #define READ_DEFECT_DATA      0x37
 #define MEDIUM_SCAN           0x38
 #define COMPARE               0x39
@@ -115,6 +118,7 @@
 #define MAINTENANCE_IN        0xa3
 #define MAINTENANCE_OUT       0xa4
 #define MOVE_MEDIUM           0xa5
+#define EXCHANGE_MEDIUM       0xa6
 #define SET_READ_AHEAD        0xa7
 #define READ_12               0xa8
 #define WRITE_12              0xaa
-- 
1.7.0.4

[Qemu-devel] [PATCH 4/5] scsi: Fix transfer length for READ POSITION commands.

[Christian Borntraeger]

From: Christian Hoff <christian.hoff@de.ibm.com>

The transfer length depends on the specific service action
code, as defined in the SCSI stream commands spec section 7.7.
Up to now only the extended form was supported.

Signed-off-by: Christian Hoff <christian.hoff@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/scsi-bus.c  |   16 +++++++++++++++-
 hw/scsi-defs.h |    8 ++++++++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index a93d3da..9854321 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -839,7 +839,21 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
         cmd->xfer = buf[13] | (buf[12] << 8);
         break;
     case READ_POSITION:
-        cmd->xfer = buf[8] | (buf[7] << 8);
+        switch (buf[1] & 0x1f) /* operation code */ {
+        case SHORT_FORM_BLOCK_ID:
+        case SHORT_FORM_VENDOR_SPECIFIC:
+            cmd->xfer = 20;
+            break;
+        case LONG_FORM:
+            cmd->xfer = 32;
+            break;
+        case EXTENDED_FORM:
+            cmd->xfer = buf[8] | (buf[7] << 8);
+            break;
+        default:
+            return -1;
+        }
+
         break;
     case FORMAT_UNIT:
         cmd->xfer = buf[4] | (buf[3] << 8);
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index 2c40855..57d0866 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -144,6 +144,14 @@
 #define SAI_READ_CAPACITY_16  0x10
 
 /*
+ * READ POSITION service action codes
+ */
+#define SHORT_FORM_BLOCK_ID  0x00
+#define SHORT_FORM_VENDOR_SPECIFIC 0x01
+#define LONG_FORM            0x06
+#define EXTENDED_FORM        0x08
+
+/*
  *  SAM Status codes
  */
 
-- 
1.7.0.4

[Qemu-devel] [PATCH 5/5] scsi: Ensure command and transfer lengths are set for all SCSI stream commands.

[Christian Borntraeger]

From: Christian Hoff <christian.hoff@de.ibm.com>

scsi-generic relies on those values to be correct, so it is important that
those values are initialized properly prior to submitting the SCSI command.

Signed-off-by: Christian Hoff <christian.hoff@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/scsi-bus.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 9854321..3c3f190 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -817,7 +817,11 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
     switch (buf[0]) {
     /* stream commands */
     case ERASE_12:
+        cmd->len = 12;
+        cmd->xfer = 0;
+        break;
     case ERASE_16:
+        cmd->len = 16;
         cmd->xfer = 0;
         break;
     case READ_6:
@@ -836,9 +840,12 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
         cmd->xfer = 0;
         break;
     case SPACE_16:
+        cmd->len = 16;
         cmd->xfer = buf[13] | (buf[12] << 8);
         break;
     case READ_POSITION:
+        cmd->len = 10;
+
         switch (buf[1] & 0x1f) /* operation code */ {
         case SHORT_FORM_BLOCK_ID:
         case SHORT_FORM_VENDOR_SPECIFIC:
@@ -856,6 +863,7 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
 
         break;
     case FORMAT_UNIT:
+        cmd->len = 6;
         cmd->xfer = buf[4] | (buf[3] << 8);
         break;
     /* generic commands */
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 0/5] scsi related fixes

[Paolo Bonzini]

Il 14/06/2012 15:55, Christian Borntraeger ha scritto:
> Paolo,
> 
> this patch series was created during bringup of virtio-scsi on s390x.
> We used a tape library as a test vehicle.

Awesome!

> so here is the set of patches that allowed us to use it via virtio-scsi.
> Any comments? Ok to apply?

All looks good, I'll check them more carefully against the spec in a
couple of weeks and include them in the scsi branch.

Paolo

Re: [Qemu-devel] [PATCH 5/5] scsi: Ensure command and transfer lengths are set for all SCSI stream commands.

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

Il 14/06/2012 15:55, Christian Borntraeger ha scritto:
> From: Christian Hoff <christian.hoff@de.ibm.com>
> 
> scsi-generic relies on those values to be correct, so it is important that
> those values are initialized properly prior to submitting the SCSI command.

This and the similar code in patch 3/5 can be replaced by the attached
patch.  You can test the result at git://github.com/bonzini/qemu.git
branch scsi-devel.

Paolo

[-- Attachment #2: 0001-scsi-Ensure-command-and-transfer-lengths-are-set-for.patch --]
[-- Type: text/x-patch, Size: 2775 bytes --]

>From 387da72123720635aeb27b3b67ee3f060b926f3b Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Thu, 14 Jun 2012 16:13:49 +0200
Subject: [PATCH] scsi: Ensure command and transfer lengths are set for all
 SCSI devices

scsi-generic relies on those values to be correct, so it is important that
those values are initialized properly for all device types.

Reported-by: Christian Hoff <christian.hoff@de.ibm.com>
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 99e37b5..7ad6538 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -723,20 +723,16 @@ static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
     switch (buf[0] >> 5) {
     case 0:
         cmd->xfer = buf[4];
-        cmd->len = 6;
         break;
     case 1:
     case 2:
         cmd->xfer = lduw_be_p(&buf[7]);
-        cmd->len = 10;
         break;
     case 4:
         cmd->xfer = ldl_be_p(&buf[10]) & 0xffffffffULL;
-        cmd->len = 16;
         break;
     case 5:
         cmd->xfer = ldl_be_p(&buf[6]) & 0xffffffffULL;
-        cmd->len = 12;
         break;
     default:
         return -1;
@@ -873,7 +869,6 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
     case READ_REVERSE:
     case RECOVER_BUFFERED_DATA:
     case WRITE_6:
-        cmd->len = 6;
         cmd->xfer = buf[4] | (buf[3] << 8) | (buf[2] << 16);
         if (buf[1] & 0x01) { /* fixed */
             cmd->xfer *= dev->blocksize;
@@ -883,7 +878,6 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
     case READ_REVERSE_16:
     case VERIFY_16:
     case WRITE_16:
-        cmd->len = 16;
         cmd->xfer = buf[14] | (buf[13] << 8) | (buf[12] << 16);
         if (buf[1] & 0x01) { /* fixed */
             cmd->xfer *= dev->blocksize;
@@ -891,7 +885,6 @@ static int scsi_req_stream_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *bu
         break;
     case REWIND:
     case LOAD_UNLOAD:
-        cmd->len = 6;
         cmd->xfer = 0;
         break;
     case SPACE_16:
@@ -989,6 +982,24 @@ int scsi_req_parse(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
     int rc;
 
+    switch (buf[0] >> 5) {
+    case 0:
+        cmd->len = 6;
+        break;
+    case 1:
+    case 2:
+        cmd->len = 10;
+        break;
+    case 4:
+        cmd->len = 16;
+        break;
+    case 5:
+        cmd->len = 12;
+        break;
+    default:
+        return -1;
+    }
+
     if (dev->type == TYPE_TAPE) {
         rc = scsi_req_stream_length(cmd, dev, buf);
     } else {
-- 
1.7.10.2