From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K3BVE-0004Jw-WE for qemu-devel@nongnu.org; Mon, 02 Jun 2008 10:59:17 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K3BVE-0004Jk-Fm for qemu-devel@nongnu.org; Mon, 02 Jun 2008 10:59:16 -0400 Received: from [199.232.76.173] (port=48253 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K3BVE-0004Jh-7V for qemu-devel@nongnu.org; Mon, 02 Jun 2008 10:59:16 -0400 Received: from g5t0006.atlanta.hp.com ([15.192.0.43]:36210) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1K3BVD-0008Si-Pz for qemu-devel@nongnu.org; Mon, 02 Jun 2008 10:59:16 -0400 Subject: Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command From: Alex Williamson In-Reply-To: <13EAABC4-ED0C-4B91-9DF4-9486F0A1259E@suse.de> References: <1211865917.8201.56.camel@bling> <1212004084.6821.30.camel@lappy> <13EAABC4-ED0C-4B91-9DF4-9486F0A1259E@suse.de> Content-Type: text/plain; charset=utf-8 Date: Mon, 02 Jun 2008 08:58:55 -0600 Message-Id: <1212418735.6656.32.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: Alexander Graf Cc: qemu-devel@nongnu.org Hi Alex, On Mon, 2008-06-02 at 12:33 +0200, Alexander Graf wrote: > On May 28, 2008, at 9:48 PM, Alex Williamson wrote: >=20 > See below for comments. I haven't tested the patch though, all > comments are purely based on looking at the patch and mmc-2 spec. FWIW, I'm working off this draft of the MMC-6 spec that google was able to find online: http://www.t10.org/ftp/t10/drafts/mmc6/mmc6r01.pdf > Great job so far, Thanks, I still need to look into sg_raw to see how the internal length field works, but a couple comments... > > +static void ide_dvd_read_structure(IDEState *s) > > +{ > > + const uint8_t *packet =3D s->io_buffer; > > + uint8_t *buf =3Ds->io_buffer; > > + int format =3D packet[7]; > > + int max_len =3D ube16_to_cpu(packet + 8); >=20 >=20 > These look like function parameters to me. If you don't think it's too redundant to pass IDEState and the rest, because I'll need IDEState to call ide_atapi_cmd_*(). Perhaps the callee could do those, but it looks like it could become ugly pretty quick. > > + > > + memset(buf, 0, max_len); > > + buf[4] =3D 1; // DVD-ROM, part version 1 > > + buf[5] =3D 0xf; // 120mm disc, maximum rate > > unspecified >=20 >=20 > I guess this means "minimum rate"? I'm not sure exactly what "Not specified" means, but it's a valid option in the table I'm looking (Table 409). I assume this has more to do with writing than reading. > >=20 > > + buf[6] =3D 0; // one layer, embossed data >=20 >=20 > Layer type should be 1 (R/O Layer). Layer type 1 is a recordable area, which implies DVD-R to me. Embossed means that it's pre-recorded and read-only, at least as I read it. > > + > > + if (!MEDIA_IS_DVD(s)) { > > + if (format < 0xc0) > > + ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST, > > + ASC_INCOMPATIBLE_FORMAT); > > + else > > + ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST, > > + > > ASC_INV_FIELD_IN_CMD_PACKET); > > + break; >=20 >=20 > I can't seem to find that in the Spec. In MMC-2 I found: >=20 >=20 > When a READ DVD STRUCTURE Command is issued for CD media, with format > codes 00h - FEh, the=20 > command shall be terminated with CHECK CONDITION status, sense key set > to ILLEGAL REQUEST and the=20 > additional sense code set to CANNOT READ MEDIUM- INCOMPATIBLE FORMAT. Here's where I got that interpretation (6.22.2.5): When the Drive/media combination does not support the specified Format code, the command shall be terminated with CHECK CONDITION status and SK/ASC/ASCQ shall be set to ILLEGAL REQUEST/INVALID FIELD IN CDB. =20 If a READ DISC STRUCTURE command is issued for media that is not consistent with this command and the format code is in the range 00h =E2=80=93 BFh, the command shall be terminated with CHECK CON= DITION status and SK/ASC/ASCQ shall be set to ILLEGAL REQUEST/CANNOT READ MEDIUM/INCOMPATIBLE FORMAT. =20 If there is no medium or an incompatible medium is installed, a request for Format FFh shall be fulfilled using a Drive specific media type. Maybe I shouldn't be basing this on a draft spec? I'll try to play with sg_raw and clean up the rest. Thanks for the comments, Alex --=20 Alex Williamson HP Open Source & Linux Org.