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