On May 28, 2008, at 9:48 PM, Alex Williamson wrote: Hi Alex,
Thanks for the comments.
On Tue, 2008-05-27 at 09:46 +0200, Alexander Graf wrote:
It might be a good idea to go through all the commands and see if they
really return dynamic lengths if they have to. Apparently this is not
the first one failing to do so.
Agreed, but this patch is only focused on this one command for now.
Several parts of the spec are still unimplemented:
- Different formats. These should at least be defined as a TODO in the
switch clause
Done. I added a few more formats listed as required for DVD-ROM
support, and re-architected the code to make it easier to fill in others
later, with appropriate TODOs.
Looks a lot better than the previous one, thank you! One thing that bugs me is the tight coupling with the IDE layer. One day someone might want to add a SCSI DVD-Rom emulation, which should be a no-brainer if things weren't coupled so tightly to the IDE/ATAPI layer. This is just a remark, no need to change it in your patch. You might want to take a look at cdrom.c though.
- Proper allocation length interpretation. I will comment on this below
How does this work when the data response also includes a data length?
Should I adjust that, or risk a bug in the guest OS indexing off of it's
buffer? In the patch below I chose to fix the response data length, it
needs some minor changes if that isn't correct. I'm also interpreting
the request max_len as including the 4 byte header, but the response
length as being only the data part. Please double check, this is the
first time I've looked at the MMC spec.
max_len simply truncates the response packet. No more and no less :-). You might really want to play with sg_raw a bit and see what happens in exactly these cases on a real DVD-Rom.
- Check if the media is a CD and fail if true. It might be a good idea
to reuse the detection from the GET_CONFIGURATION command and put it
into a function, so it can be reused in several places.
Done. It seems too trivial for a function, so I just made a macro.
Fabrice usually prefers inline functions over macros.
New version below, I've really only tested media 0/format 0, but I think
the rest is correct. Please let me know if you have further comments.
Thanks,
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.
Great job so far,
Alex
Alex
Fix ATAPI read drive structure command
Make use of the allocation length field in the command and only return
the number of bytes requested. Fix location of format byte in command.
Add comments for more fields as we fill them in. Restructure code and
add appropriate TODOs for yet to be implemented functionality.
Signed-off-by: Alex Williamson <address@hidden>
--
diff -r 1f1541286539 trunk/hw/ide.c
--- a/trunk/hw/ide.c Sun May 25 15:01:05 2008 -0600
+++ b/trunk/hw/ide.c Wed May 28 13:39:19 2008 -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
@@ -433,6 +434,11 @@ typedef struct IDEState {
uint8_t *mdata_storage;
int media_changed;
} IDEState;
+
+/* XXX: DVDs that could fit on a CD will be reported as a CD */
+#define MEDIA_PRESENT(s) (s->nb_sectors > 0)
+#define MEDIA_IS_DVD(s) (MEDIA_PRESENT(s) && (s)->nb_sectors > CD_MAX_SECTORS)
+#define MEDIA_IS_CD(s) (MEDIA_PRESENT(s) && (s)->nb_sectors <= CD_MAX_SECTORS)
These should be inline functions
#define BM_STATUS_DMAING 0x01
#define BM_STATUS_ERROR 0x02
@@ -1364,6 +1370,79 @@ static inline uint8_t ide_atapi_set_prof
return 4;
}
+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. +
+ switch (format) {
+ case 0x0: /* Physical format information */
+ {
+ int layer = packet[6];
+ uint64_t total_sectors;
+
+ if (layer != 0) {
+ ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
+ ASC_INV_FIELD_IN_CMD_PACKET);
+ break;
+ }
+
+ 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);
+ break;
+ }
+
+ if (max_len > 2048 + 4)
+ max_len = 2048 + 4;
No need to truncate max_len. Only min(len, max_len) will be copied and zeroing buf is large enough. If you want to keep the OS from overwriting qemu internal buffers, better make a safe memset wrapper that writes at most IDE_DMA_BUF_SECTORS*512 + 4 bytes, which is the length of io_buffer.
+
+ 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"?
+ buf[6] = 0; // one layer, embossed data
Layer type should be 1 (R/O Layer).
+ buf[7] = 0; // default densities
+
+ /* FIXME: 0x30000 per spec? */
+ cpu_to_ube32(buf + 8, 0); // start sector
I don't really understand that part of the spec either.
+ cpu_to_ube32(buf + 12, total_sectors - 1); // end sector
+ cpu_to_ube32(buf + 16, total_sectors - 1); // l0 end sector
+
+ /* Size of buffer, minus 4 byte header */
+ cpu_to_be16wu((uint16_t *)buf, max_len > 4 ? max_len - 4 : 0);
This is wrong. The Data Length always specifies the amount of data that could be read from there on. If the buffer actually gets written does not change this number. So if I read the spec correctly this should be len-2 with len being 2048+4. A good idea might be to use sg_raw on a real DVD drive and see what that returns. +
+ ide_atapi_cmd_reply(s, 2048 + 4, max_len);
+ break;
+ }
+ case 0x01: /* DVD copyright information */
+ if (max_len > 4 + 4)
+ max_len = 4 + 4;
See above
+
+ memset(buf, 0, max_len);
+ /* no copyright/region info */
+ cpu_to_be16wu((uint16_t *)buf, max_len > 4 ? max_len - 4 : 0);
See above
+ ide_atapi_cmd_reply(s, 8, max_len);
+ break;
+ case 0x04: /* DVD disc manufacturing information */
+ if (max_len > 2048 + 4)
+ max_len = 2048 + 4;
See above
+
+ memset(buf, 0, max_len);
+ cpu_to_be16wu((uint16_t *)buf, 0);
*((uint32_t*)buf) = 0;
+ ide_atapi_cmd_reply(s, 4, max_len);
+ break;
+ case 0x03: /* BCA information - invalid field for no BCA info */
+ /* TODO: formats beyond DVD-ROM requires */
+ default:
+ ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
+ ASC_INV_FIELD_IN_CMD_PACKET);
+ break;
+ }
+}
+
static void ide_atapi_cmd(IDEState *s)
{
const uint8_t *packet;
@@ -1651,42 +1730,41 @@ static void ide_atapi_cmd(IDEState *s)
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];
- if (media != 0 || layer != 0)
- {
+ /* Disc structure capability list */
+ if (format == 0xff) {
+ /* TODO: media independent, so separate from below */
ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
ASC_INV_FIELD_IN_CMD_PACKET);
+ break;
+ }
+
+ 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.
}
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:
+ if (media == 0) {
+ ide_dvd_read_structure(s);
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);
@@ -1739,16 +1817,12 @@ static void ide_atapi_cmd(IDEState *s)
/*
* 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);
+
len = 8; /* header completed */
if (max_len > len) {
|