From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K3CAt-0000Pw-Em for qemu-devel@nongnu.org; Mon, 02 Jun 2008 11:42:19 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K3CAt-0000Pd-09 for qemu-devel@nongnu.org; Mon, 02 Jun 2008 11:42:19 -0400 Received: from [199.232.76.173] (port=45878 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K3CAs-0000PY-O9 for qemu-devel@nongnu.org; Mon, 02 Jun 2008 11:42:18 -0400 Received: from ns.suse.de ([195.135.220.2]:34571 helo=mx1.suse.de) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1K3CAs-0001HI-FF for qemu-devel@nongnu.org; Mon, 02 Jun 2008 11:42:18 -0400 Message-Id: <8F70DBBA-DB05-4B03-886B-A2182F3A3AC6@suse.de> From: Alexander Graf In-Reply-To: <1212418735.6656.32.camel@lappy> Content-Type: text/plain; charset=WINDOWS-1252; format=flowed; delsp=yes Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 (Apple Message framework v919.2) Subject: Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command Date: Mon, 2 Jun 2008 17:42:14 +0200 References: <1211865917.8201.56.camel@bling> <1212004084.6821.30.camel@lappy> <13EAABC4-ED0C-4B91-9DF4-9486F0A1259E@suse.de> <1212418735.6656.32.camel@lappy> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: qemu-devel@nongnu.org Hi Alex, On Jun 2, 2008, at 4:58 PM, Alex Williamson wrote: > > 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: > > >> >> 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 =20 > able > to find online: > > http://www.t10.org/ftp/t10/drafts/mmc6/mmc6r01.pdf I'm having a really hard time reading any MMC specs > 2. The MMC-2 one =20= from t10 is actually quite nice and should be sufficient for now. > > >> Great job so far, > > Thanks, I still need to look into sg_raw to see how the internal =20 > 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); >> >> >> 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. That's actually exactly what I'd love to see. If we could use the =20 return values as error codes and len returns, everything would be =20 fine. Right now that's not the case with all of the other ide cdrom =20 code, so don't worry too much about that. > > > >>> + >>> + memset(buf, 0, max_len); >>> + buf[4] =3D 1; // DVD-ROM, part version 1 >>> + buf[5] =3D 0xf; // 120mm disc, maximum rate >>> unspecified >> >> >> I guess this means "minimum rate"? > > I'm not sure exactly what "Not specified" means, but it's a valid =20 > option > in the table I'm looking (Table 409). I assume this has more to do =20= > with > writing than reading. In MMC-2 the field is defined as "minimum rate", not "maximum rate" > > >>> >>> + buf[6] =3D 0; // one layer, embossed data >> >> >> Layer type should be 1 (R/O Layer). > > Layer type 1 is a recordable area, which implies DVD-R to me. =20 > Embossed > means that it's pre-recorded and read-only, at least as I read it. In MMC-2 Table 249: The Layer Type field (Table 249) indicates the read/write ability of =20 the layer. Table 249 =96 Layer Type Field Layer Type Code Layer Type 0001b Read-only layer 0010b Recordable layer 0100b ReWritable layer Others Reserved Maybe this means a finished DVD-R layer though. Funny thing is that =20 type 0 is not defined here. > > >>> + >>> + 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; >> >> >> I can't seem to find that in the Spec. In MMC-2 I found: >> >> >> When a READ DVD STRUCTURE Command is issued for CD media, with format >> codes 00h - FEh, the >> command shall be terminated with CHECK CONDITION status, sense key =20= >> set >> to ILLEGAL REQUEST and the >> 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. > > > If a READ DISC STRUCTURE command is issued for media that is =20= > not > consistent with this command and the format code is in the =20 > range > 00h =96 BFh, the command shall be terminated with CHECK = CONDITION > status and SK/ASC/ASCQ shall be set to ILLEGAL REQUEST/CANNOT > READ MEDIUM/INCOMPATIBLE FORMAT. I agree that this is a hard one. =46rom the way I read the spec I would =20= say that on CD-Roms you only get Invalid Field. This is something that =20= you should try with sg_raw on a real drive. > > > If there is no medium or an incompatible medium is installed, a > request for Format FFh shall be fulfilled using a Drive =20 > specific > media type. > > Maybe I shouldn't be basing this on a draft spec? I'll try to play =20= > with > sg_raw and clean up the rest. Thanks for the comments, Have fun! It's definitely a nice tool to find out these corner cases. Alex=