Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Anthony Liguori <anthony@codemonkey.ws>]

Can you send this as an attachment or inlined as plain text?

Regards,

Anthony Liguori

Alex Williamson wrote:
> This patch just updates the previous to apply cleanly since the get
> configuration change went in.  Thanks,
>
> 	Alex
>
> On Mon, 2008-06-02 at 16:12 -0600, Alex Williamson wrote:
>   
>> Hi Alex,
>>
>> I think I've incorporated all your comments.  I tested with sg_raw; the
>> embedded size value is constant regardless of the allocation length in
>> the command.  I added extra parameters to ide_dvd_read_structure() but I
>> still had to pass the IDEState pointer for things like the sector count,
>> and maybe others in the future as more command options are added.  I
>> implemented the 0xff command based on what I saw using sg_raw on real
>> hardware.  I'll note that it seems to be device dependent whether this
>> command works when the drive is empty or contains CD media.
>>
>> Hopefully we're getting close, let me know if you have further comments.
>> Thanks,
>>
>> 	Alex
>>
>>     
>
> Fix ATAPI read drive structure command
>
> Previous version ignored the allocation length parameter and read the
> format byte from the wrong location.  Re-implement to support the full
> requirements for DVD-ROM and allow for easy extension later.
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> --
>
> --- a/trunk/hw/ide.c	2008-06-02 16:37:12.000000000 -0600
> +++ b/trunk/hw/ide.c	2008-06-02 16:40:11.000000000 -0600
> @@ -351,6 +351,7 @@
>  #define ASC_ILLEGAL_OPCODE                   0x20
>  #define ASC_LOGICAL_BLOCK_OOR                0x21
>  #define ASC_INV_FIELD_IN_CMD_PACKET          0x24
> +#define ASC_INCOMPATIBLE_FORMAT              0x30
>  #define ASC_MEDIUM_NOT_PRESENT               0x3a
>  #define ASC_SAVING_PARAMETERS_NOT_SUPPORTED  0x39
>  
> @@ -434,6 +435,22 @@
>      int media_changed;
>  } IDEState;
>  
> +/* XXX: DVDs that could fit on a CD will be reported as a CD */
> +static inline int media_present(IDEState *s)
> +{
> +    return (s->nb_sectors > 0);
> +}
> +
> +static inline int media_is_dvd(IDEState *s)
> +{
> +    return (media_present(s) && s->nb_sectors > CD_MAX_SECTORS);
> +}
> +
> +static inline int media_is_cd(IDEState *s)
> +{
> +    return (media_present(s) && s->nb_sectors <= CD_MAX_SECTORS);
> +}
> +
>  #define BM_STATUS_DMAING 0x01
>  #define BM_STATUS_ERROR  0x02
>  #define BM_STATUS_INT    0x04
> @@ -1364,6 +1381,93 @@
>      return 4;
>  }
>  
> +static int ide_dvd_read_structure(IDEState *s, int format,
> +                                  const uint8_t *packet, uint8_t *buf)
> +{
> +    switch (format) {
> +        case 0x0: /* Physical format information */
> +            {
> +                int layer = packet[6];
> +                uint64_t total_sectors;
> +
> +                if (layer != 0)
> +                    return -ASC_INV_FIELD_IN_CMD_PACKET;
> +
> +                bdrv_get_geometry(s->bs, &total_sectors);
> +                total_sectors >>= 2;
> +                if (total_sectors == 0)
> +                    return -ASC_MEDIUM_NOT_PRESENT;
> +
> +                buf[4] = 1;   /* DVD-ROM, part version 1 */
> +                buf[5] = 0xf; /* 120mm disc, minimum rate unspecified */
> +                buf[6] = 1;   /* one layer, read-only (per MMC-2 spec) */
> +                buf[7] = 0;   /* default densities */
> +
> +                /* FIXME: 0x30000 per spec? */
> +                cpu_to_ube32(buf + 8, 0); /* start sector */
> +                cpu_to_ube32(buf + 12, total_sectors - 1); /* end sector */
> +                cpu_to_ube32(buf + 16, total_sectors - 1); /* l0 end sector */
> +
> +                /* Size of buffer, not including 2 byte size field */
> +                cpu_to_be16wu((uint16_t *)buf, 2048 + 2);
> +
> +                /* 2k data + 4 byte header */
> +                return (2048 + 4);
> +            }
> +
> +        case 0x01: /* DVD copyright information */
> +            buf[4] = 0; /* no copyright data */
> +            buf[5] = 0; /* no region restrictions */
> +
> +            /* Size of buffer, not including 2 byte size field */
> +            cpu_to_be16wu((uint16_t *)buf, 4 + 2);
> +
> +            /* 4 byte header + 4 byte data */
> +            return (4 + 4);
> +
> +        case 0x03: /* BCA information - invalid field for no BCA info */
> +            return -ASC_INV_FIELD_IN_CMD_PACKET;
> +
> +        case 0x04: /* DVD disc manufacturing information */
> +            /* Size of buffer, not including 2 byte size field */
> +            cpu_to_be16wu((uint16_t *)buf, 2048 + 2);
> +
> +            /* 2k data + 4 byte header */
> +            return (2048 + 4);
> +
> +        case 0xff:
> +            /*
> +             * This lists all the command capabilities above.  Add new ones
> +             * in order and update the length and buffer return values.
> +             */
> +
> +            buf[4] = 0x00; /* Physical format */
> +            buf[5] = 0x40; /* Not writable, is readable */
> +            cpu_to_be16wu((uint16_t *)(buf + 6), 2048 + 4);
> +
> +            buf[8] = 0x01; /* Copyright info */
> +            buf[9] = 0x40; /* Not writable, is readable */
> +            cpu_to_be16wu((uint16_t *)(buf + 10), 4 + 4);
> +
> +            buf[12] = 0x03; /* BCA info */
> +            buf[13] = 0x40; /* Not writable, is readable */
> +            cpu_to_be16wu((uint16_t *)(buf + 14), 188 + 4);
> +
> +            buf[16] = 0x04; /* Manufacturing info */
> +            buf[17] = 0x40; /* Not writable, is readable */
> +            cpu_to_be16wu((uint16_t *)(buf + 18), 2048 + 4);
> +
> +            /* Size of buffer, not including 2 byte size field */
> +            cpu_to_be16wu((uint16_t *)buf, 16 + 2);
> +
> +            /* data written + 4 byte header */
> +            return (16 + 4);
> +
> +        default: /* TODO: formats beyond DVD-ROM requires */
> +            return -ASC_INV_FIELD_IN_CMD_PACKET;
> +    }
> +}
> +
>  static void ide_atapi_cmd(IDEState *s)
>  {
>      const uint8_t *packet;
> @@ -1651,42 +1755,42 @@
>      case GPCMD_READ_DVD_STRUCTURE:
>          {
>              int media = packet[1];
> -            int layer = packet[6];
> -            int format = packet[2];
> -            uint64_t total_sectors;
> +            int format = packet[7];
> +            int ret;
>  
> -            if (media != 0 || layer != 0)
> -            {
> +            max_len = ube16_to_cpu(packet + 8);
> +
> +            if (format < 0xff && media_is_cd(s)) {
>                  ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
> -                                    ASC_INV_FIELD_IN_CMD_PACKET);
> +                                    ASC_INCOMPATIBLE_FORMAT);
> +                break;
>              }
>  
> +            memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * 512 + 4 ?
> +                   IDE_DMA_BUF_SECTORS * 512 + 4 : max_len);
> +
>              switch (format) {
> -                case 0:
> -                    bdrv_get_geometry(s->bs, &total_sectors);
> -                    total_sectors >>= 2;
> -                    if (total_sectors == 0) {
> -                        ide_atapi_cmd_error(s, SENSE_NOT_READY,
> -                                            ASC_MEDIUM_NOT_PRESENT);
> +                case 0x00 ... 0x7f:
> +                case 0xff:
> +                    if (media == 0) {
> +                        ret = ide_dvd_read_structure(s, format, packet, buf);
> +
> +                        if (ret < 0)
> +                            ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST, -ret);
> +                        else
> +                            ide_atapi_cmd_reply(s, ret, max_len);
> +
>                          break;
>                      }
> +                    /* TODO: BD support, fall through for now */
>  
> -                    memset(buf, 0, 2052);
> -
> -                    buf[4] = 1;   // DVD-ROM, part version 1
> -                    buf[5] = 0xf; // 120mm disc, maximum rate unspecified
> -                    buf[6] = 0;   // one layer, embossed data
> -                    buf[7] = 0;
> -
> -                    cpu_to_ube32(buf + 8, 0);
> -                    cpu_to_ube32(buf + 12, total_sectors - 1);
> -                    cpu_to_ube32(buf + 16, total_sectors - 1);
> -
> -                    cpu_to_be16wu((uint16_t *)buf, 2048 + 4);
> -
> -                    ide_atapi_cmd_reply(s, 2048 + 3, 2048 + 4);
> -                    break;
> -
> +                /* Generic disk structures */
> +                case 0x80: /* TODO: AACS volume identifier */
> +                case 0x81: /* TODO: AACS media serial number */
> +                case 0x82: /* TODO: AACS media identifier */
> +                case 0x83: /* TODO: AACS media key block */
> +                case 0x90: /* TODO: List of recognized format layers */
> +                case 0xc0: /* TODO: Write protection status */
>                  default:
>                      ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
>                                          ASC_INV_FIELD_IN_CMD_PACKET);
> @@ -1741,16 +1845,11 @@
>              /* 
>               * the number of sectors from the media tells us which profile
>               * to use as current.  0 means there is no media
> -             *
> -             * XXX: fails to detect correctly DVDs with less data burned
> -             *      than what a CD can hold
>               */
> -            if (s -> nb_sectors) {
> -                if (s -> nb_sectors > CD_MAX_SECTORS)
> -                    cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
> -                else
> -                    cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
> -            }
> +            if (media_is_dvd(s))
> +                cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
> +            else if (media_is_cd(s))
> +                cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
>  
>              buf[10] = 0x02 | 0x01; /* persistent and current */
>              len = 12; /* headers: 8 + 4 */
>
>
>
>
>