Re: [Qemu-devel] [PATCH 09/35] atapi/scsi-disk: make mode page values coherent between the two

[Kevin Wolf <kwolf@redhat.com>]

Am 13.10.2011 13:03, schrieb Paolo Bonzini:
> This patch adds to scsi-disk the missing mode page 0x01 for both disk
> and CD-ROM drives, and mode page 0x0e for CD drives only.
> 
> A few offsets were wrong in atapi.c.  Also change the 2Ah mode page to
> expose DVD media read capabilities in the IDE cdrom.  This lets you run
> dvd+rw-mediainfo on the virtual DVD drives.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/atapi.c |   12 ++++++------
>  hw/scsi-disk.c |   33 ++++++++++++++++++++++++---------
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 10f161f..c6eb8f0 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -751,7 +751,7 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf)
>  
>              buf[8] = 0x2a;
>              buf[9] = 0x12;
> -            buf[10] = 0x00;
> +            buf[10] = 0x3b; /* read CDR/CDRW/DVDROM/DVDR/DVDRAM */
>              buf[11] = 0x00;
>  
>              /* Claim PLAY_AUDIO capability (0x01) since some Linux
> @@ -760,14 +760,14 @@ static void cmd_mode_sense(IDEState *s, uint8_t *buf)
>              buf[13] = 3 << 5;
>              buf[14] = (1 << 0) | (1 << 3) | (1 << 5);
>              if (s->tray_locked) {
> -                buf[6] |= 1 << 1;
> +                buf[14] |= 1 << 1;
>              }
> -            buf[15] = 0x00;
> -            cpu_to_ube16(&buf[16], 706);
> +            buf[15] = 0x00; /* No volume & mute control, no changer */
> +            cpu_to_ube16(&buf[16], 704); /* 4x read speed */
>              buf[18] = 0;
>              buf[19] = 2;
> -            cpu_to_ube16(&buf[20], 512);
> -            cpu_to_ube16(&buf[22], 706);
> +            cpu_to_ube16(&buf[20], 512); /* 512k buffer */
> +            cpu_to_ube16(&buf[22], 704); /* 4x read speed current */
>              buf[24] = 0;
>              buf[25] = 0;
>              buf[26] = 0;
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index a4aadf2..837747f 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -607,6 +607,8 @@ static int mode_sense_page(SCSIDiskState *s, int page, uint8_t **p_outbuf,
>          [MODE_PAGE_HD_GEOMETRY]            = (1 << TYPE_DISK),
>          [MODE_PAGE_FLEXIBLE_DISK_GEOMETRY] = (1 << TYPE_DISK),
>          [MODE_PAGE_CACHING]                = (1 << TYPE_DISK) | (1 << TYPE_ROM),
> +        [MODE_PAGE_R_W_ERROR]              = (1 << TYPE_DISK) | (1 << TYPE_ROM),
> +        [MODE_PAGE_AUDIO_CTL]              = (1 << TYPE_ROM),
>          [MODE_PAGE_CAPABILITIES]           = (1 << TYPE_ROM),
>      };
>  
> @@ -707,13 +709,26 @@ static int mode_sense_page(SCSIDiskState *s, int page, uint8_t **p_outbuf,
>          }
>          break;
>  
> +    case MODE_PAGE_R_W_ERROR:
> +        p[1] = 10;
> +        p[2] = 0x80; /* Automatic Write Reallocation Enabled */
> +        if (s->qdev.type == TYPE_ROM) {
> +            p[3] = 0x20; /* Read Retry Count */
> +        }
> +        break;
> +
> +    case MODE_PAGE_AUDIO_CTL:
> +        p[1] = 14;
> +        break;
> +
>      case MODE_PAGE_CAPABILITIES:
>          p[1] = 0x14;
>          if (page_control == 1) { /* Changeable Values */
>              break;
>          }
> -        p[2] = 3; // CD-R & CD-RW read
> -        p[3] = 0; // Writing not supported
> +
> +        p[2] = 0x3b; /* CD-R & CD-RW read */
> +        p[3] = 0; /* Writing not supported */
>          p[4] = 0x7f; /* Audio, composite, digital out,
>                          mode 2 form 1&2, multi session */
>          p[5] = 0xff; /* CD DA, DA accurate, RW supported,
> @@ -723,17 +738,17 @@ static int mode_sense_page(SCSIDiskState *s, int page, uint8_t **p_outbuf,
>          /* Locking supported, jumper present, eject, tray */
>          p[7] = 0; /* no volume & mute control, no
>                       changer */
> -        p[8] = (50 * 176) >> 8; // 50x read speed
> +        p[8] = (50 * 176) >> 8; /* 50x read speed */
>          p[9] = (50 * 176) & 0xff;
> -        p[10] = 0 >> 8; // No volume
> -        p[11] = 0 & 0xff;
> -        p[12] = 2048 >> 8; // 2M buffer
> +        p[10] = 0 >> 8; /* Volume */
> +        p[11] = 2 & 0xff;

If I understand correctly what the internet says (I don't have a copy of
MMC-3 and MMC-5 doesn't describe it any more), this is still part of the
volume. I'm not sure why we should provide exactly two different volume
levels, but if you think this makes sense, please change p[10] to be 2 >> 8.

> +        p[12] = 2048 >> 8; /* 2M buffer */
>          p[13] = 2048 & 0xff;
> -        p[14] = (16 * 176) >> 8; // 16x read speed current
> +        p[14] = (16 * 176) >> 8; /* 16x read speed current */
>          p[15] = (16 * 176) & 0xff;
> -        p[18] = (16 * 176) >> 8; // 16x write speed
> +        p[18] = (16 * 176) >> 8; /* 16x write speed */
>          p[19] = (16 * 176) & 0xff;
> -        p[20] = (16 * 176) >> 8; // 16x write speed current
> +        p[20] = (16 * 176) >> 8; /* 16x write speed current */
>          p[21] = (16 * 176) & 0xff;
>          break;

Kevin