[Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command

[Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command

[Alex Williamson]

I've found that the introduction of support for the ATAPI
GPCMD_READ_DVD_STRUCTURE command causes a problem for Windows Vista
guests under QEMU.  To test the bug, simply boot a Vista VM under QEMU
with a DVD image loaded using the -cdrom option.  If you now run
diskpart.exe, the command will hang indefinitely.

The main issue is that the read disk structure command contains a field
indicating the maximum length to be returned.  We ignore this field and
always return the maximum possible table size.  I also found that we're
getting the format byte from the wrong field in the request (byte 2 is
the MSB of the address field, we want byte 7).  The patch below fixes
these issues for me and adds a few extra comments.  Perhaps Filip can
confirm whether it still works for the original usage on Darwin/x86.
Thanks,

	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.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
--

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	Mon May 26 23:15:06 2008 -0600
@@ -1652,13 +1652,15 @@ static void ide_atapi_cmd(IDEState *s)
         {
             int media = packet[1];
             int layer = packet[6];
-            int format = packet[2];
+            int format = packet[7];
+            int length = ube16_to_cpu(packet + 8);
             uint64_t total_sectors;
 
             if (media != 0 || layer != 0)
             {
                 ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
                                     ASC_INV_FIELD_IN_CMD_PACKET);
+                break;
             }
 
             switch (format) {
@@ -1671,20 +1673,28 @@ static void ide_atapi_cmd(IDEState *s)
                         break;
                     }
 
-                    memset(buf, 0, 2052);
+                    if (length == 0)
+                        length = 2048 + 4;
 
+                    if (length < 20) {
+                        ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
+                                            ASC_INV_FIELD_IN_CMD_PACKET);
+                        break;
+                    }
+
+                    memset(buf, 0, length);
                     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;
+                    buf[7] = 0;   // default densities
 
-                    cpu_to_ube32(buf + 8, 0);
-                    cpu_to_ube32(buf + 12, total_sectors - 1);
-                    cpu_to_ube32(buf + 16, total_sectors - 1);
+                    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
 
-                    cpu_to_be16wu((uint16_t *)buf, 2048 + 4);
+                    cpu_to_be16wu((uint16_t *)buf, length);
 
-                    ide_atapi_cmd_reply(s, 2048 + 3, 2048 + 4);
+                    ide_atapi_cmd_reply(s, length, length);
                     break;
 
                 default:

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command

[Alexander Graf]

On May 27, 2008, at 7:25 AM, Alex Williamson wrote:

>
> I've found that the introduction of support for the ATAPI
> GPCMD_READ_DVD_STRUCTURE command causes a problem for Windows Vista
> guests under QEMU.  To test the bug, simply boot a Vista VM under QEMU
> with a DVD image loaded using the -cdrom option.  If you now run
> diskpart.exe, the command will hang indefinitely.

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.

> The main issue is that the read disk structure command contains a  
> field
> indicating the maximum length to be returned.  We ignore this field  
> and
> always return the maximum possible table size.  I also found that  
> we're
> getting the format byte from the wrong field in the request (byte 2 is
> the MSB of the address field, we want byte 7).  The patch below fixes
> these issues for me and adds a few extra comments.  Perhaps Filip can
> confirm whether it still works for the original usage on Darwin/x86.
> Thanks,
>
> 	Alex

Several parts of the spec are still unimplemented:

- Different formats. These should at least be defined as a TODO in the  
switch clause
- Proper allocation length interpretation. I will comment on this below
- 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.

>
>
>
> 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.
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> --
>
> 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	Mon May 26 23:15:06 2008 -0600
> @@ -1652,13 +1652,15 @@ static void ide_atapi_cmd(IDEState *s)
>         {
>             int media = packet[1];
>             int layer = packet[6];
> -            int format = packet[2];
> +            int format = packet[7];
> +            int length = ube16_to_cpu(packet + 8);

This is max_length

>
>             uint64_t total_sectors;
>
>             if (media != 0 || layer != 0)
>             {
>                 ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
>                                     ASC_INV_FIELD_IN_CMD_PACKET);
> +                break;
>             }
>
>             switch (format) {
> @@ -1671,20 +1673,28 @@ static void ide_atapi_cmd(IDEState *s)
>                         break;
>                     }
>
> -                    memset(buf, 0, 2052);
> +                    if (length == 0)
> +                        length = 2048 + 4;
>
> +                    if (length < 20) {
> +                        ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
> +                                             
> ASC_INV_FIELD_IN_CMD_PACKET);
> +                        break;
> +                    }
> +

max_length should not have any impact here

>
> +                    memset(buf, 0, length);
>                     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;
> +                    buf[7] = 0;   // default densities
>
> -                    cpu_to_ube32(buf + 8, 0);
> -                    cpu_to_ube32(buf + 12, total_sectors - 1);
> -                    cpu_to_ube32(buf + 16, total_sectors - 1);
> +                    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
>
> -                    cpu_to_be16wu((uint16_t *)buf, 2048 + 4);
> +                    cpu_to_be16wu((uint16_t *)buf, length);
>
> -                    ide_atapi_cmd_reply(s, 2048 + 3, 2048 + 4);
> +                    ide_atapi_cmd_reply(s, length, length);

this should look more like

ide_atapi_cmd_reply(s, length, max_length);

>
>                     break;
>
>                 default:
>

The normal way of doing things in the ATAPI layer (as far as I  
understood it) is, that you build the full response packet independent  
on the allocation length. Only as late as the ide_atapi_cmd_reply  
command, the packet will be truncated to whatever is necessary to fit  
it into the response buffer.
This does not need to be of your concern in this layer though, as the  
atapi abstraction layer will take care of that. So what you need to do  
here is just fill up the packet completely and call  
ide_atapi_cmd_reply with the correct max_length.

It is great to see that someone takes on these issues though,

Alex

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command

[Alex Williamson]

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.

> - 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.

> - 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.

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,

	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 <alex.williamson@hp.com>
--

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)
 
 #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);
+
+    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;
+
+                memset(buf, 0, max_len);
+                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;   // 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, minus 4 byte header */
+                cpu_to_be16wu((uint16_t *)buf, max_len > 4 ? max_len - 4 : 0);
+
+                ide_atapi_cmd_reply(s, 2048 + 4, max_len);
+                break;
+            }
+        case 0x01: /* DVD copyright information */
+            if (max_len > 4 + 4)
+                max_len = 4 + 4;
+
+            memset(buf, 0, max_len);
+            /* no copyright/region info */
+            cpu_to_be16wu((uint16_t *)buf, max_len > 4 ? max_len - 4 : 0);
+            ide_atapi_cmd_reply(s, 8, max_len);
+            break;
+        case 0x04: /* DVD disc manufacturing information */
+            if (max_len > 2048 + 4)
+                max_len = 2048 + 4;
+
+            memset(buf, 0, max_len);
+            cpu_to_be16wu((uint16_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;
             }
 
             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) {

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command

[Alexander Graf]

[-- Attachment #1: Type: text/plain, Size: 12224 bytes --]


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 <alex.williamson@hp.com>
> --
>
> 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) {
>
>
>
>


[-- Attachment #2: Type: text/html, Size: 26046 bytes --]

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command

[Alex Williamson]

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

> 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.


> > +
> > +                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.

> > 
> > +                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.

> > +
> > +            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.
        
        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

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command

[Alexander Graf]

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 (v3)

[Alex Williamson]

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 <alex.williamson@hp.com>
--

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	Mon Jun 02 16:09:28 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,22 @@ 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 */
+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
@@ -1364,6 +1381,93 @@ static inline uint8_t ide_atapi_set_prof
     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 @@ 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];
+            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);
@@ -1739,16 +1843,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) {

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Alex Williamson]

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 <alex.williamson@hp.com>
--

--- 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 */

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Alexander Graf]

[-- Attachment #1: Type: text/plain, Size: 2983 bytes --]


On Jun 3, 2008, at 12:12 AM, 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

This is perfectly fine. To clean up things, we would simply need an  
abstract CDROMState that includes the same values. The only thing we  
can't provide in that one are IDE status codes.

On Jun 3, 2008, at 12:45 AM, 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 <alex.williamson@hp.com>
> --

>
> @@ -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);

After having looked at the spec and a real dvd rom output I got  
uncertain if this is correct. Shouldn't capabilities provide the  
capabilities of the drive and not the medium?

Apart from that the patch looks fine to me.

Alex


[-- Attachment #2: Type: text/html, Size: 5476 bytes --]

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Alex Williamson]

Hi Alex,

On Tue, 2008-06-03 at 15:48 +0200, Alexander Graf wrote:

> On Jun 3, 2008, at 12:12 AM, Alex Williamson wrote:

> > @@ -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);
> 
> 
> After having looked at the spec and a real dvd rom output I got
> uncertain if this is correct. Shouldn't capabilities provide the
> capabilities of the drive and not the medium?

Yes, I agree, this is a pretty weak test, however, that's a topic for a
different patch.  I'm not changing the existing logic here with this
patch.

> Apart from that the patch looks fine to me.

Great, thanks for the review!  If there are no other comments, could
someone please apply this to the tree?  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Carlo Marcelo Arenas Belon]

On Tue, Jun 03, 2008 at 08:21:50AM -0600, Alex Williamson wrote:
> 
> On Tue, 2008-06-03 at 15:48 +0200, Alexander Graf wrote:
> 
> > On Jun 3, 2008, at 12:12 AM, Alex Williamson wrote:
> 
> > > @@ -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);
> > 
> > 
> > After having looked at the spec and a real dvd rom output I got
> > uncertain if this is correct. Shouldn't capabilities provide the
> > capabilities of the drive and not the medium?
> 
> Yes, I agree, this is a pretty weak test, however, that's a topic for a
> different patch.  I'm not changing the existing logic here with this
> patch.

this is a "sorta" valid test for the GET_CONFIGURATION call as what is needed
here is the "active" profile, that is dependent on the type of media that is
loaded into a multi profile MMC device.

using the size of the media as an indication of what type it is, is definitely
weak and likely to fail on edge cases (as indicated by the comments) and
should be replaced for a better heuristic if there is one that can be used
instead.

Carlo

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Anthony Liguori]

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 <alex.williamson@hp.com>
> --
>
> --- 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 */
>
>
>
>
>   

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Alex Williamson]

On Tue, 2008-06-03 at 11:59 -0500, Anthony Liguori wrote:
> Can you send this as an attachment or inlined as plain text?

Hmm, shows up as plain text in the archives, although there is a strange
character in front of the patch title.  Here it is again.  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 <alex.williamson@hp.com>
--

--- 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 */

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Anthony Liguori]

Alex Williamson wrote:
> On Tue, 2008-06-03 at 11:59 -0500, Anthony Liguori wrote:
>   
>> Can you send this as an attachment or inlined as plain text?
>>     
>
> Hmm, shows up as plain text in the archives, although there is a strange
> character in front of the patch title.  Here it is again.  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 <alex.williamson@hp.com>
> --
>
> --- 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);
> +}
>
>   

I think the automatic probing is good, but we should have a -drive 
parameter that allows the cd type to explicitly be set to either CDROM 
or DVD.  This can be done in a follow-up patch though.  I did some 
testing with Windows and this patch seems to do the right thing.

Tested-by: Anthony Liguori <aliguori@us.ibm.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] ide: fix ATAPI read drive structure command (v3)

[Alex Williamson]

On Wed, 2008-06-04 at 09:35 -0500, Anthony Liguori wrote:
> I think the automatic probing is good, but we should have a -drive 
> parameter that allows the cd type to explicitly be set to either CDROM 
> or DVD.  This can be done in a follow-up patch though.  I did some 
> testing with Windows and this patch seems to do the right thing.

It seems like both places the check is used we're concerned about the
type of media inserted.  I don't know enough about ISO formats, but
perhaps we could identify it as iso9660 vs udf at some level.  Thanks
for the comments, review, and testing.

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.