[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command
From: |
Alexander Graf |
Subject: |
Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command |
Date: |
Mon, 2 Jun 2008 17:42:14 +0200 |
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
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
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
length
field works, but a couple comments...
+static void ide_dvd_read_structure(IDEState *s)
+{
+ const uint8_t *packet = s->io_buffer;
+ uint8_t *buf =s->io_buffer;
+ int format = packet[7];
+ int max_len = 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
return values as error codes and len returns, everything would be
fine. Right now that's not the case with all of the other ide cdrom
code, so don't worry too much about that.
+
+ memset(buf, 0, max_len);
+ buf[4] = 1; // DVD-ROM, part version 1
+ buf[5] = 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
option
in the table I'm looking (Table 409). I assume this has more to do
with
writing than reading.
In MMC-2 the field is defined as "minimum rate", not "maximum rate"
+ buf[6] = 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.
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
the layer.
Table 249 – 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
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
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
not
consistent with this command and the format code is in the
range
00h – 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. From the way I read the spec I would
say that on CD-Roms you only get Invalid Field. This is something that
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
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,
Have fun! It's definitely a nice tool to find out these corner cases.
Alex
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command, Alexander Graf, 2008/06/02
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command, Alex Williamson, 2008/06/02
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command,
Alexander Graf <=
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Alex Williamson, 2008/06/02
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Alex Williamson, 2008/06/02
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Alexander Graf, 2008/06/03
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Alex Williamson, 2008/06/03
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Carlo Marcelo Arenas Belon, 2008/06/03
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Anthony Liguori, 2008/06/03
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Alex Williamson, 2008/06/04
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Anthony Liguori, 2008/06/04
- Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3), Alex Williamson, 2008/06/04