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

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(-)

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

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