From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K3Imu-0007tF-9M for qemu-devel@nongnu.org; Mon, 02 Jun 2008 18:46:00 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K3Ims-0007sA-JM for qemu-devel@nongnu.org; Mon, 02 Jun 2008 18:45:59 -0400 Received: from [199.232.76.173] (port=33615 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K3Ims-0007rn-Ak for qemu-devel@nongnu.org; Mon, 02 Jun 2008 18:45:58 -0400 Received: from g1t0027.austin.hp.com ([15.216.28.34]:47680) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1K3Imr-0000zN-C6 for qemu-devel@nongnu.org; Mon, 02 Jun 2008 18:45:58 -0400 Subject: Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3) From: Alex Williamson In-Reply-To: <1212444720.13411.12.camel@lappy> References: <1211865917.8201.56.camel@bling> <1212004084.6821.30.camel@lappy> <13EAABC4-ED0C-4B91-9DF4-9486F0A1259E@suse.de> <1212418735.6656.32.camel@lappy> <8F70DBBA-DB05-4B03-886B-A2182F3A3AC6@suse.de> <1212444720.13411.12.camel@lappy> Content-Type: text/plain; charset=utf-8 Date: Mon, 02 Jun 2008 16:45:37 -0600 Message-Id: <1212446737.13411.15.camel@lappy> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Alexander Graf 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, >=20 > 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. >=20 > Hopefully we're getting close, let me know if you have further comments= . > Thanks, >=20 > Alex >=20 =EF=BB=BFFix 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 -- --- 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 =20 @@ -434,6 +435,22 @@ int media_changed; } IDEState; =20 +/* 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 <=3D CD_MAX_SECTORS); +} + #define BM_STATUS_DMAING 0x01 #define BM_STATUS_ERROR 0x02 #define BM_STATUS_INT 0x04 @@ -1364,6 +1381,93 @@ return 4; } =20 +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 =3D packet[6]; + uint64_t total_sectors; + + if (layer !=3D 0) + return -ASC_INV_FIELD_IN_CMD_PACKET; + + bdrv_get_geometry(s->bs, &total_sectors); + total_sectors >>=3D 2; + if (total_sectors =3D=3D 0) + return -ASC_MEDIUM_NOT_PRESENT; + + buf[4] =3D 1; /* DVD-ROM, part version 1 */ + buf[5] =3D 0xf; /* 120mm disc, minimum rate unspecified = */ + buf[6] =3D 1; /* one layer, read-only (per MMC-2 spec)= */ + buf[7] =3D 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 sec= tor */ + + /* 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] =3D 0; /* no copyright data */ + buf[5] =3D 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 o= nes + * in order and update the length and buffer return values. + */ + + buf[4] =3D 0x00; /* Physical format */ + buf[5] =3D 0x40; /* Not writable, is readable */ + cpu_to_be16wu((uint16_t *)(buf + 6), 2048 + 4); + + buf[8] =3D 0x01; /* Copyright info */ + buf[9] =3D 0x40; /* Not writable, is readable */ + cpu_to_be16wu((uint16_t *)(buf + 10), 4 + 4); + + buf[12] =3D 0x03; /* BCA info */ + buf[13] =3D 0x40; /* Not writable, is readable */ + cpu_to_be16wu((uint16_t *)(buf + 14), 188 + 4); + + buf[16] =3D 0x04; /* Manufacturing info */ + buf[17] =3D 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 =3D packet[1]; - int layer =3D packet[6]; - int format =3D packet[2]; - uint64_t total_sectors; + int format =3D packet[7]; + int ret; =20 - if (media !=3D 0 || layer !=3D 0) - { + max_len =3D 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; } =20 + 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 >>=3D 2; - if (total_sectors =3D=3D 0) { - ide_atapi_cmd_error(s, SENSE_NOT_READY, - ASC_MEDIUM_NOT_PRESENT); + case 0x00 ... 0x7f: + case 0xff: + if (media =3D=3D 0) { + ret =3D 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 */ =20 - memset(buf, 0, 2052); - - buf[4] =3D 1; // DVD-ROM, part version 1 - buf[5] =3D 0xf; // 120mm disc, maximum rate unspecif= ied - buf[6] =3D 0; // one layer, embossed data - buf[7] =3D 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 @@ /*=20 * the number of sectors from the media tells us which profi= le * 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); =20 buf[10] =3D 0x02 | 0x01; /* persistent and current */ len =3D 12; /* headers: 8 + 4 */