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

[Alexander Graf <agraf@suse.de>]

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