From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K3ZrJ-0006ov-U0 for qemu-devel@nongnu.org; Tue, 03 Jun 2008 12:59:42 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K3ZrI-0006nm-Ja for qemu-devel@nongnu.org; Tue, 03 Jun 2008 12:59:41 -0400 Received: from [199.232.76.173] (port=40881 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K3ZrH-0006nW-RO for qemu-devel@nongnu.org; Tue, 03 Jun 2008 12:59:39 -0400 Received: from an-out-0708.google.com ([209.85.132.240]:19941) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1K3ZrH-00059K-0r for qemu-devel@nongnu.org; Tue, 03 Jun 2008 12:59:39 -0400 Received: by an-out-0708.google.com with SMTP id d18so638227and.130 for ; Tue, 03 Jun 2008 09:59:38 -0700 (PDT) Message-ID: <4845786C.5090908@codemonkey.ws> Date: Tue, 03 Jun 2008 11:59:24 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3) 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> <1212446737.13411.15.camel@lappy> In-Reply-To: <1212446737.13411.15.camel@lappy> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 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 > -- > > --- 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 */ > > > > >