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

[Alex Williamson <alex.williamson@hp.com>]

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.