[Qemu-devel] [PATCH v2 0/6] scsi-disk: improve the emulation of theMODE SENSE command

[Qemu-devel] [PATCH v2 0/6] scsi-disk: improve the emulation of theMODE SENSE command

[Bernhard Kohl]

This series fixes some issues with the MODE SENSE command.

I have an OS which fails during this command. It works fine with
real SCSI disk hardware.

Changes v1 -> v2:

Applied all review comments from Kevin.

1/6: unchanged
2/6: Kevin's comments
3/6: Kevin's comments, renamed the subject of the patch,
     improved handling of page control (PC) field
4/6: unchanged
5/6: new patch, bug found during rework
6/6: new patch, due to Kevin's comments

[PATCH v2 1/6] scsi-disk: fix the mode data length field returned by the MODE SENSE command
[PATCH v2 2/6] scsi-disk: fix the mode data header returned by the MODE SENSE(10) command
[PATCH v2 3/6] scsi-disk: respect the page control (PC) field in the MODE SENSE command
[PATCH v2 4/6] scsi-disk: fix the block descriptor returned by the MODE SENSE command
[PATCH v2 5/6] scsi-disk: return CHECK CONDITION for unknown page codes in the MODE SENSE command
[PATCH v2 6/6] scsi-disk: fix the check of the DBD bit in the MODE SENSE command

 hw/scsi-disk.c |   85 +++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 66 insertions(+), 19 deletions(-)

Thanks
Bernhard

[Qemu-devel] [PATCH v2 1/6] scsi-disk: fix the mode data length field returned by the MODE SENSE command

[Bernhard Kohl]

The MODE DATA LENGTH field indicates the length in bytes of the following
data that is available to be transferred. The mode data length does not include
the number of bytes in the MODE DATA LENGTH field.

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-disk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 07a6d86..b627ffe 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -653,7 +653,7 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
     }
 
     buflen = p - outbuf;
-    outbuf[0] = buflen - 4;
+    outbuf[0] = buflen - 1;
     if (buflen > req->cmd.xfer)
         buflen = req->cmd.xfer;
     return buflen;
-- 
1.7.2.2

[Qemu-devel] [PATCH v2 2/6] scsi-disk: fix the mode data header returned by the MODE SENSE(10) command

[Bernhard Kohl]

The header for the  MODE SENSE(10) command is 8 bytes long.

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-disk.c |   33 +++++++++++++++++++++++++++------
 1 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index b627ffe..942ae96 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -607,6 +607,7 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
     uint64_t nb_sectors;
     int page, dbd, buflen;
     uint8_t *p;
+    uint8_t dev_specific_param;
 
     dbd = req->cmd.buf[1]  & 0x8;
     page = req->cmd.buf[2] & 0x3f;
@@ -614,16 +615,31 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
     memset(outbuf, 0, req->cmd.xfer);
     p = outbuf;
 
-    p[1] = 0; /* Default media type.  */
-    p[3] = 0; /* Block descriptor length.  */
     if (bdrv_is_read_only(s->bs)) {
-        p[2] = 0x80; /* Readonly.  */
+        dev_specific_param = 0x80; /* Readonly.  */
+    } else {
+        dev_specific_param = 0x00;
+    }
+
+    if (req->cmd.buf[0] == MODE_SENSE) {
+        p[1] = 0; /* Default media type.  */
+        p[2] = dev_specific_param;
+        p[3] = 0; /* Block descriptor length.  */
+        p += 4;
+    } else { /* MODE_SENSE_10 */
+        p[2] = 0; /* Default media type.  */
+        p[3] = dev_specific_param;
+        p[6] = p[7] = 0; /* Block descriptor length.  */
+        p += 8;
     }
-    p += 4;
 
     bdrv_get_geometry(s->bs, &nb_sectors);
     if ((~dbd) & nb_sectors) {
-        outbuf[3] = 8; /* Block descriptor length  */
+        if (req->cmd.buf[0] == MODE_SENSE) {
+            outbuf[3] = 8; /* Block descriptor length  */
+        } else { /* MODE_SENSE_10 */
+            outbuf[7] = 8; /* Block descriptor length  */
+        }
         nb_sectors /= s->cluster_size;
         nb_sectors--;
         if (nb_sectors > 0xffffff)
@@ -653,7 +669,12 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
     }
 
     buflen = p - outbuf;
-    outbuf[0] = buflen - 1;
+    if (req->cmd.buf[0] == MODE_SENSE) {
+        outbuf[0] = buflen - 1;
+    } else { /* MODE_SENSE_10 */
+        outbuf[0] = ((buflen - 1) >> 8) & 0xff;
+        outbuf[1] = (buflen - 1) & 0xff;
+    }
     if (buflen > req->cmd.xfer)
         buflen = req->cmd.xfer;
     return buflen;
-- 
1.7.2.2

[Qemu-devel] [PATCH v2 3/6] scsi-disk: respect the page control (PC) field in the MODE SENSE command

[Bernhard Kohl]

The page control (PC) field defines the type of mode parameter values
to be returned in the mode pages:

PC=0 : Current values
PC=1 : Changeable values
PC=2 : Default values
PC=3 : Saved values

The current implementation always returns the same type of parameters.
This is OK for Current and Default values as we don't support changes
to be done by the MODE SELECT command.

For Saved values the following applies (implemented by this patch):
"A PC field value of 3h requests that the target return the saved
values of the mode parameters. Implementation of saved page parameters
is optional. Mode parameters not supported by the target shall be set
to zero. If saved values are not implemented, the command shall be
terminated with CHECK CONDITION status, the sense key set to
ILLEGAL REQUEST and the additional sense code set to
SAVING PARAMETERS NOT SUPPORTED."

For Changeable values the following applies (implemented by this patch):
"A PC field value of 1h requests that the target return a mask denoting
those mode parameters that are changeable. In the mask, the fields of
the mode parameters that are changeable shall be set to all one bits and
the fields of the mode parameters that are non-changeable (i.e. defined
by the target) shall be set to all zero bits."

In newer versions of the SCSI-2 spec the following clause was added.
"If the logical unit does not implement changeable parameters mode pages
and the device server receives a MODE SENSE command with 01b in the PC
field, then the command shall be terminated with CHECK CONDITION status,
with the sense key set to ILLEGAL REQUEST, and the additional sense code
set to INVALID FIELD IN CDB."

This was not yet included in the SCSI-2 Working Drafts from 1986-1993.
I assume that the variant to return CHECK CONDITION for PC=1 is not
widely implemented by real devices. I have a legacy OS which fails,
if MODE_SENSE returns non GOOD for PC=1. So for highest compatibility I
implemented the former variant with this patch.

The last Working Draft X3T9.2 Rev. 10L 7-SEP-93 can be found here:
http://ldkelley.com/SCSI2/SCSI2/SCSI2-08.html#8.2.10

In mode_sense_page() this patch also avoids multiple hard coded
definitions of the same mode page length. Instead I use the varable
p[1]. In fact the returned length of the mode pages 4 and 5 were wrong
(2 bytes less).

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-disk.c |   45 +++++++++++++++++++++++++++++++++++----------
 1 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 942ae96..ebc46db 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -486,16 +486,26 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
     return buflen;
 }
 
-static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p)
+static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p,
+                           int page_control)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     BlockDriverState *bdrv = s->bs;
     int cylinders, heads, secs;
 
+    /*
+     * If Changeable Values are requested, a mask denoting those mode parameters
+     * that are changeable shall be returned. As we currently don't support
+     * parameter changes via MODE_SELECT all bits are returned set to zero.
+     * The buffer was already menset to zero by the caller of this function.
+     */
     switch (page) {
     case 4: /* Rigid disk device geometry page. */
         p[0] = 4;
         p[1] = 0x16;
+        if (page_control == 1) { /* Changeable Values */
+            return p[1] + 2;
+        }
         /* if a geometry hint is available, use it */
         bdrv_get_geometry_hint(bdrv, &cylinders, &heads, &secs);
         p[2] = (cylinders >> 16) & 0xff;
@@ -520,11 +530,14 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p)
         /* Medium rotation rate [rpm], 5400 rpm */
         p[20] = (5400 >> 8) & 0xff;
         p[21] = 5400 & 0xff;
-        return 0x16;
+        return p[1] + 2;
 
     case 5: /* Flexible disk device geometry page. */
         p[0] = 5;
         p[1] = 0x1e;
+        if (page_control == 1) { /* Changeable Values */
+            return p[1] + 2;
+        }
         /* Transfer rate [kbit/s], 5Mbit/s */
         p[2] = 5000 >> 8;
         p[3] = 5000 & 0xff;
@@ -556,21 +569,27 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p)
         /* Medium rotation rate [rpm], 5400 rpm */
         p[28] = (5400 >> 8) & 0xff;
         p[29] = 5400 & 0xff;
-        return 0x1e;
+        return p[1] + 2;
 
     case 8: /* Caching page.  */
         p[0] = 8;
         p[1] = 0x12;
+        if (page_control == 1) { /* Changeable Values */
+            return p[1] + 2;
+        }
         if (bdrv_enable_write_cache(s->bs)) {
             p[2] = 4; /* WCE */
         }
-        return 20;
+        return p[1] + 2;
 
     case 0x2a: /* CD Capabilities and Mechanical Status page. */
         if (bdrv_get_type_hint(bdrv) != BDRV_TYPE_CDROM)
             return 0;
         p[0] = 0x2a;
         p[1] = 0x14;
+        if (page_control == 1) { /* Changeable Values */
+            return p[1] + 2;
+        }
         p[2] = 3; // CD-R & CD-RW read
         p[3] = 0; // Writing not supported
         p[4] = 0x7f; /* Audio, composite, digital out,
@@ -594,7 +613,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p)
         p[19] = (16 * 176) & 0xff;
         p[20] = (16 * 176) >> 8; // 16x write speed current
         p[21] = (16 * 176) & 0xff;
-        return 22;
+        return p[1] + 2;
 
     default:
         return 0;
@@ -605,13 +624,15 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     uint64_t nb_sectors;
-    int page, dbd, buflen;
+    int page, dbd, buflen, page_control;
     uint8_t *p;
     uint8_t dev_specific_param;
 
     dbd = req->cmd.buf[1]  & 0x8;
     page = req->cmd.buf[2] & 0x3f;
-    DPRINTF("Mode Sense (page %d, len %zd)\n", page, req->cmd.xfer);
+    page_control = (req->cmd.buf[2] & 0xc0) >> 6;
+    DPRINTF("Mode Sense(%d) (page %d, len %d, page_control %d)\n",
+        (req->cmd.buf[0] == MODE_SENSE) ? 6 : 10, page, len, page_control);
     memset(outbuf, 0, req->cmd.xfer);
     p = outbuf;
 
@@ -655,16 +676,20 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
         p += 8;
     }
 
+    if (page_control == 3) { /* Saved Values */
+        return -1; /* ILLEGAL_REQUEST */
+    }
+
     switch (page) {
     case 0x04:
     case 0x05:
     case 0x08:
     case 0x2a:
-        p += mode_sense_page(req, page, p);
+        p += mode_sense_page(req, page, p, page_control);
         break;
     case 0x3f:
-        p += mode_sense_page(req, 0x08, p);
-        p += mode_sense_page(req, 0x2a, p);
+        p += mode_sense_page(req, 0x08, p, page_control);
+        p += mode_sense_page(req, 0x2a, p, page_control);
         break;
     }
 
-- 
1.7.2.2

[Qemu-devel] [PATCH v2 4/6] scsi-disk: fix the block descriptor returned by the MODE SENSE command

[Bernhard Kohl]

The block descriptor contains the number of blocks, not the highest LBA.
Real hard disks return 0 if the number of blocks exceed the maximum 0xFFFFFF.

SCSI-Spec:
http://ldkelley.com/SCSI2/SCSI2/SCSI2-08.html#8.3.3
"The number of blocks field specifies the number of logical blocks on the
medium to which the density code and block length fields apply. A value
of zero indicates that all of the remaining logical blocks of the logical
unit shall have the medium characteristics specified."

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-disk.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index ebc46db..aef9199 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -662,9 +662,8 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
             outbuf[7] = 8; /* Block descriptor length  */
         }
         nb_sectors /= s->cluster_size;
-        nb_sectors--;
         if (nb_sectors > 0xffffff)
-            nb_sectors = 0xffffff;
+            nb_sectors = 0;
         p[0] = 0; /* media density code */
         p[1] = (nb_sectors >> 16) & 0xff;
         p[2] = (nb_sectors >> 8) & 0xff;
-- 
1.7.2.2

[Qemu-devel] [PATCH v2 5/6] scsi-disk: return CHECK CONDITION for unknown page codes in the MODE SENSE command

[Bernhard Kohl]

SCSI-Spec:
http://ldkelley.com/SCSI2/SCSI2/SCSI2-08.html#8.2.10
"An initiator may request any one or all of the supported mode pages
from a target. If an initiator issues a MODE SENSE command with a
page code value not implemented by the target, the target shall return
CHECK CONDITION status and shall set the sense key to ILLEGAL REQUEST
and the additional sense code to INVALID FIELD IN CDB."

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-disk.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index aef9199..0041038 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -690,6 +690,8 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
         p += mode_sense_page(req, 0x08, p, page_control);
         p += mode_sense_page(req, 0x2a, p, page_control);
         break;
+    default:
+        return -1; /* ILLEGAL_REQUEST */
     }
 
     buflen = p - outbuf;
-- 
1.7.2.2

[Qemu-devel] [PATCH v2 6/6] scsi-disk: fix the check of the DBD bit in the MODE SENSE command

[Bernhard Kohl]

The DBD bit does not work as expected.

SCSI-Spec:
http://ldkelley.com/SCSI2/SCSI2/SCSI2-08.html#8.2.10
"A disable block descriptors (DBD) bit of zero indicates that the target
may return zero or more block descriptors in the returned MODE SENSE
data (see 8.3.3), at the target's discretion. A DBD bit of one
specifies that the target shall not return any block descriptors in the
returned MODE SENSE data."

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 hw/scsi-disk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 0041038..99b0464 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -655,7 +655,7 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
     }
 
     bdrv_get_geometry(s->bs, &nb_sectors);
-    if ((~dbd) & nb_sectors) {
+    if (!dbd && nb_sectors) {
         if (req->cmd.buf[0] == MODE_SENSE) {
             outbuf[3] = 8; /* Block descriptor length  */
         } else { /* MODE_SENSE_10 */
-- 
1.7.2.2

[Qemu-devel] Re: [PATCH v2 2/6] scsi-disk: fix the mode data header returned by the MODE SENSE(10) command

[Kevin Wolf]

Am 31.08.2010 11:22, schrieb Bernhard Kohl:
> The header for the  MODE SENSE(10) command is 8 bytes long.
> 
> Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
> ---
>  hw/scsi-disk.c |   33 +++++++++++++++++++++++++++------
>  1 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index b627ffe..942ae96 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -607,6 +607,7 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>      uint64_t nb_sectors;
>      int page, dbd, buflen;
>      uint8_t *p;
> +    uint8_t dev_specific_param;
>  
>      dbd = req->cmd.buf[1]  & 0x8;
>      page = req->cmd.buf[2] & 0x3f;
> @@ -614,16 +615,31 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>      memset(outbuf, 0, req->cmd.xfer);
>      p = outbuf;
>  
> -    p[1] = 0; /* Default media type.  */
> -    p[3] = 0; /* Block descriptor length.  */
>      if (bdrv_is_read_only(s->bs)) {
> -        p[2] = 0x80; /* Readonly.  */
> +        dev_specific_param = 0x80; /* Readonly.  */
> +    } else {
> +        dev_specific_param = 0x00;
> +    }
> +
> +    if (req->cmd.buf[0] == MODE_SENSE) {
> +        p[1] = 0; /* Default media type.  */
> +        p[2] = dev_specific_param;
> +        p[3] = 0; /* Block descriptor length.  */
> +        p += 4;
> +    } else { /* MODE_SENSE_10 */
> +        p[2] = 0; /* Default media type.  */
> +        p[3] = dev_specific_param;
> +        p[6] = p[7] = 0; /* Block descriptor length.  */
> +        p += 8;
>      }
> -    p += 4;
>  
>      bdrv_get_geometry(s->bs, &nb_sectors);
>      if ((~dbd) & nb_sectors) {
> -        outbuf[3] = 8; /* Block descriptor length  */
> +        if (req->cmd.buf[0] == MODE_SENSE) {
> +            outbuf[3] = 8; /* Block descriptor length  */
> +        } else { /* MODE_SENSE_10 */
> +            outbuf[7] = 8; /* Block descriptor length  */
> +        }
>          nb_sectors /= s->cluster_size;
>          nb_sectors--;
>          if (nb_sectors > 0xffffff)
> @@ -653,7 +669,12 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>      }
>  
>      buflen = p - outbuf;
> -    outbuf[0] = buflen - 1;
> +    if (req->cmd.buf[0] == MODE_SENSE) {
> +        outbuf[0] = buflen - 1;
> +    } else { /* MODE_SENSE_10 */
> +        outbuf[0] = ((buflen - 1) >> 8) & 0xff;
> +        outbuf[1] = (buflen - 1) & 0xff;

Missed that last time, but it should be buflen - 2 here, right? The mode
data length field is two bytes here.

While we're at it, maybe adding a comment before the if wouldn't hurt
which explains why we're subtracting something at all.

Kevin

[Qemu-devel] Re: [PATCH v2 2/6] scsi-disk: fix the mode data header returned by the MODE SENSE(10) command

[Bernhard Kohl]

Am 31.08.2010 11:47, schrieb ext Kevin Wolf:
>
> Am 31.08.2010 11:22, schrieb Bernhard Kohl:
> > The header for the  MODE SENSE(10) command is 8 bytes long.
> >
> > Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
> > ---
> >  hw/scsi-disk.c |   33 +++++++++++++++++++++++++++------
> >  1 files changed, 27 insertions(+), 6 deletions(-)
>

> >      buflen = p - outbuf;
> > -    outbuf[0] = buflen - 1;
> > +    if (req->cmd.buf[0] == MODE_SENSE) {
> > +        outbuf[0] = buflen - 1;
> > +    } else { /* MODE_SENSE_10 */
> > +        outbuf[0] = ((buflen - 1) >> 8) & 0xff;
> > +        outbuf[1] = (buflen - 1) & 0xff;
>
> Missed that last time, but it should be buflen - 2 here, right? The mode
> data length field is two bytes here.
>
> While we're at it, maybe adding a comment before the if wouldn't hurt
> which explains why we're subtracting something at all.
>

Yes, thats right. I'll change that in v3 and add a comment.

SCSI-Spec:
http://ldkelley.com/SCSI2/SCSI2/SCSI2-08.html#8.3.3
"When using the MODE SENSE command, the mode data length field specifies
the length in bytes of the following data that is available to be
transferred. The mode data length does not include itself."

Bernhard

[Qemu-devel] Re: [PATCH v2 1/6] scsi-disk: fix the mode data length field returned by the MODE SENSE command

[Kevin Wolf]

Am 31.08.2010 11:22, schrieb Bernhard Kohl:
> The MODE DATA LENGTH field indicates the length in bytes of the following
> data that is available to be transferred. The mode data length does not include
> the number of bytes in the MODE DATA LENGTH field.
> 
> Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>

Thanks, applied to the block branch.

Patches 3-6 look good to me as well, but they don't apply cleanly
without patch 2, which I think needs to fixed.

Kevin

[Qemu-devel] Re: [PATCH v2 1/6] scsi-disk: fix the mode data length field returned by the MODE SENSE command

[Bernhard Kohl]

Am 31.08.2010 12:26, schrieb ext Kevin Wolf:
>
> Am 31.08.2010 11:22, schrieb Bernhard Kohl:
> > The MODE DATA LENGTH field indicates the length in bytes of the 
> following
> > data that is available to be transferred. The mode data length does 
> not include
> > the number of bytes in the MODE DATA LENGTH field.
> >
> > Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
>
> Thanks, applied to the block branch.
>
> Patches 3-6 look good to me as well, but they don't apply cleanly
> without patch 2, which I think needs to fixed.
>
> Kevin
>

Ok, I'll fix patch 2 and resend the series as v3.

Bernhard