[Qemu-devel] [PATCH 0/4] scsi-disk: improve the emulation of the MODE SENSE command

[Qemu-devel] [PATCH 0/4] scsi-disk: improve the emulation of the MODE 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.

Thanks
Bernhard

[Qemu-devel] [PATCH 1/4] 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 f43f2d0..57439f4 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -652,7 +652,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

[Qemu-devel] [PATCH 2/4] 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 |   35 ++++++++++++++++++++++++++++-------
 1 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 57439f4..927df54 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -606,6 +606,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;
@@ -613,16 +614,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.  */
+    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM ||
+        bdrv_is_read_only(s->bs)) {
+        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)
@@ -652,7 +668,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

[Qemu-devel] [PATCH 3/4] scsi-disk: fix changeable values returned by the MODE SENSE command

[Bernhard Kohl]

If the page control (PC) field in the MODE SENSE command defines Changeable
Values to be returned in the mode pages, don't return any mode page as there
is no support to change any values.

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

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 927df54..26f7345 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -604,13 +604,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;
 
@@ -654,7 +656,8 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
         p += 8;
     }
 
-    switch (page) {
+    /* Don't return pages if Changeable Values (1) are requested. */
+    if (page_control != 1) switch (page) {
     case 0x04:
     case 0x05:
     case 0x08:
-- 
1.7.2

[Qemu-devel] [PATCH 4/4] 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 26f7345..519513c 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -642,9 +642,8 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
         else /* MODE_SENSE_10 */
             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

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

[Kevin Wolf]

Am 02.08.2010 17:31, 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 |   35 ++++++++++++++++++++++++++++-------
>  1 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 57439f4..927df54 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -606,6 +606,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;
> @@ -613,16 +614,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.  */
> +    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM ||
> +        bdrv_is_read_only(s->bs)) {

This looks like a mismerge. The check for CDROMs was removed when they
became read-only by definition. Please don't reintroduce it.

> +        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  */

Please add curly braces here (see CODING_STYLE).

Kevin

Re: [Qemu-devel] [PATCH 3/4] scsi-disk: fix changeable values returned by the MODE SENSE command

[Kevin Wolf]

Am 02.08.2010 17:31, schrieb Bernhard Kohl:
> If the page control (PC) field in the MODE SENSE command defines Changeable
> Values to be returned in the mode pages, don't return any mode page as there
> is no support to change any values.
> 
> Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
> ---
>  hw/scsi-disk.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 927df54..26f7345 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -604,13 +604,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;
>  
> @@ -654,7 +656,8 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>          p += 8;
>      }
>  
> -    switch (page) {
> +    /* Don't return pages if Changeable Values (1) are requested. */
> +    if (page_control != 1) switch (page) {

Coding style (missing braces, and switch belongs on its own line).

>      case 0x04:
>      case 0x05:
>      case 0x08:

I don't think this is enough. Wouldn't this still let the command return
successfully? In fact we need it to fail:

"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 should do it if I'm not mistaken:

if (page_control == 0x01) {
    return -1;
}

Kevin

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

[Kevin Wolf]

Am 02.08.2010 17:31, schrieb 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 26f7345..519513c 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -642,9 +642,8 @@ static int scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
>          else /* MODE_SENSE_10 */
>              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;

The patch itself looks okay. However, it made me wonder what this line
wants to tell us:

    if ((~dbd) & nb_sectors) {

Is it just me or doesn't this make any sense at all? dbd is a single
bit, 0x8 if set or 0x0 otherwise. nb_sectors is the number of sectors.
Can this operation have any meaningful result? I suppose it was meant to
be something like:

    if (!dbd && nb_sectors) {

Can you please check this and add a patch 5/4 if necessary?

Kevin

Re: [Qemu-devel] [PATCH 0/4] scsi-disk: improve the emulation of the MODE SENSE command

[Kevin Wolf]

Am 02.08.2010 17:31, schrieb 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.

In general this series looks fine. I had some minor comments that I'd
like to have addressed before merging it. I'd expect that v2 gets the
remaining things right, though, it's not complicated stuff.

Kevin

Re: [Qemu-devel] [PATCH 0/4] scsi-disk: improve the emulation of the MODE SENSE command

[Bernhard Kohl]

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

Am 16.08.2010 19:39, schrieb ext Kevin Wolf:
>
> Am 02.08.2010 17:31, schrieb 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.
>
> In general this series looks fine. I had some minor comments that I'd
> like to have addressed before merging it. I'd expect that v2 gets the
> remaining things right, though, it's not complicated stuff.
>
> Kevin
>
Thanks for the review. I will rework the series and send it again.

Bernhard

[-- Attachment #2: Type: text/html, Size: 1457 bytes --]

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

[Bernhard Kohl]

Am 16.08.2010 19:02, schrieb ext Kevin Wolf:
>
> > +    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM ||
> > +        bdrv_is_read_only(s->bs)) {
>
> This looks like a mismerge. The check for CDROMs was removed when they
> became read-only by definition. Please don't reintroduce it.
>
OK, I will remove that check in v2.
>
> > +        if (req->cmd.buf[0] == MODE_SENSE)
> > +            outbuf[3] = 8; /* Block descriptor length  */
> > +        else /* MODE_SENSE_10 */
> > +            outbuf[7] = 8; /* Block descriptor length  */
>
> Please add curly braces here (see CODING_STYLE).
>
OK, I will add curly braces in v2.

Bernhard

Re: [Qemu-devel] [PATCH 3/4] scsi-disk: fix changeable values returned by the MODE SENSE command

[Bernhard Kohl]

Am 16.08.2010 19:12, schrieb ext Kevin Wolf:
>
> > @@ -654,7 +656,8 @@ static int 
> scsi_disk_emulate_mode_sense(SCSIRequest *req, uint8_t *outbuf)
> >          p += 8;
> >      }
> >
> > -    switch (page) {
> > +    /* Don't return pages if Changeable Values (1) are requested. */
> > +    if (page_control != 1) switch (page) {
>
> Coding style (missing braces, and switch belongs on its own line).
>
I restored that line to its original version in v2.
>
> >      case 0x04:
> >      case 0x05:
> >      case 0x08:
>
> I don't think this is enough. Wouldn't this still let the command return
> successfully? In fact we need it to fail:
>
> "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 clause was not yet included in early SCSI-2 spec versions. For highest
compatibility I will implement the following in v2. I found this in all spec
versions:

"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."

By the way, my legacy OS fails, if MODE_SENSE returns non GOOD for PC=1.
I assume that the variant to return CHECK CONDITION for PC=1 is not
widely implemented by real devices.

> This should do it if I'm not mistaken:
>
> if (page_control == 0x01) {
>     return -1;
> }
>
The same applies for Saved Values (PC=3) and unsupported page codes.
This is described in all spec versions too. I will implement this in v2.

Bernhard

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

[Bernhard Kohl]

Am 16.08.2010 19:34, schrieb ext Kevin Wolf:
>
> The patch itself looks okay. However, it made me wonder what this line
> wants to tell us:
>
>     if ((~dbd) & nb_sectors) {
>
> Is it just me or doesn't this make any sense at all? dbd is a single
> bit, 0x8 if set or 0x0 otherwise. nb_sectors is the number of sectors.
> Can this operation have any meaningful result? I suppose it was meant to
> be something like:
>
>     if (!dbd && nb_sectors) {
>
> Can you please check this and add a patch 5/4 if necessary?
>
For my opinion it is nonsense too. And it does not work. I tested it:

root@grml ~ # sg_modes /dev/sdb -6 -v -H -H -p 0x3f
     inquiry cdb: 12 00 00 00 24 00
     QEMU      QEMU HARDDISK     0.13   peripheral_type: disk [0x0]
     mode sense (6) cdb: 1a 00 3f 00 fc 00
     mode sense (6): requested 252 bytes but got 220 bytes
Mode parameter header from MODE SENSE(6):
  00     1f 00 00 08
   Mode data length=32, medium type=0x00, WP=0, DpoFua=0, longlba=0
   Block descriptor length=8
 > Direct access device block descriptors:
    Density code=0x0
  00     00 a0 00 00 00 00 02 00

 >> page_code=0x8, page_control=0
  00     08 12 00 00 00 00 00 00  00 00 00 00 00 00 00 00
  10     00 00 00 00
root@grml ~ # sg_modes /dev/sdb -6 -v -H -H -p 0x3f -d
     inquiry cdb: 12 00 00 00 24 00
     QEMU      QEMU HARDDISK     0.13   peripheral_type: disk [0x0]
     mode sense (6) cdb: 1a 08 3f 00 fc 00
     mode sense (6): requested 252 bytes but got 220 bytes
Mode parameter header from MODE SENSE(6):
  00     1f 00 00 08
   Mode data length=32, medium type=0x00, WP=0, DpoFua=0, longlba=0
   Block descriptor length=8
 > Direct access device block descriptors:
    Density code=0x0
  00     00 a0 00 00 00 00 02 00

 >> page_code=0x8, page_control=0
  00     08 12 00 00 00 00 00 00  00 00 00 00 00 00 00 00
  10     00 00 00 00
root@grml ~ #

I will add a patch 5/5 in v2 as you proposed.

Bernhard