Re: [Qemu-devel] [PATCH] ide: Implement VPD response for ATAPI

[John Snow]

On 12/09/2014 05:35 AM, Stefan Hajnoczi wrote:
> On Thu, Dec 04, 2014 at 01:59:19PM -0500, John Snow wrote:
> > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> > index c63b7e5..d27c34b 100644
> > --- a/hw/ide/atapi.c
> > +++ b/hw/ide/atapi.c
> > @@ -622,19 +622,98 @@ static void cmd_request_sense(IDEState *s, uint8_t *buf)
> >   static void cmd_inquiry(IDEState *s, uint8_t *buf)
> >   {
> >       int max_len = buf[4];
>
> What guarantees do we have about the size of buf in this function?
>
> We can't trust max_len because it comes from the guest.

buf points to io_buffer, which is always 2048 bytes; the guest can only 
specify up to 255 here, so we should always be okay truncating at the 
limit requested by the guest.

If the ambiguity of what buf is makes you nervous (say, for instance, 
some other user of this function in the future uses a buffer that is not 
s->io_buffer) I think we'd have to add another argument.

> > +    int idx = 0;
> >
> > -    buf[0] = 0x05; /* CD-ROM */
> > -    buf[1] = 0x80; /* removable */
> > -    buf[2] = 0x00; /* ISO */
> > -    buf[3] = 0x21; /* ATAPI-2 (XXX: put ATAPI-4 ?) */
> > -    buf[4] = 31; /* additional length */
> > -    buf[5] = 0; /* reserved */
> > -    buf[6] = 0; /* reserved */
> > -    buf[7] = 0; /* reserved */
> > -    padstr8(buf + 8, 8, "QEMU");
> > -    padstr8(buf + 16, 16, "QEMU DVD-ROM");
> > -    padstr8(buf + 32, 4, s->version);
> > -    ide_atapi_cmd_reply(s, 36, max_len);
> > +    /* If the EVPD (Enable Vital Product Data) bit is set in byte 1,
> > +     * we are being asked for a specific page of info indicated by byte 2. */
> > +    if (buf[1] & 0x01) {
> > +        switch (buf[2]) {
> > +        case 0x00:
> > +            /* Supported Pages */
> > +            buf[idx++] = 0x05; /* CD-ROM */
> > +            buf[idx++] = 0x00; /* Page Code (0x00) */
> > +            buf[idx++] = 0x00; /* reserved */
> > +            buf[idx++] = 0x02; /* Two pages supported: */
> > +            buf[idx++] = 0x00; /* 0x00: Supported Pages, and: */
> > +            buf[idx++] = 0x83; /* 0x83: Device Identification. */
> > +            break;
> > +        case 0x83:
> > +            /* Device Identification. Each entry is optional, but the entries
> > +             * included here are modeled after libata's VPD responses. */
> > +            buf[idx++] = 0x05; /* CD-ROM */
> > +            buf[idx++] = 0x83; /* Page Code */
> > +            buf[idx++] = 0x00;
> > +            buf[idx++] = 0x00; /* Length of encapsulated structure: */
> > +
> > +            /* Entry 1: Serial */
> > +            if (idx + 24 > max_len) {
> > +                /* Not enough room for even the first entry: */
> > +                /* 4 byte header + 20 byte string */
> > +                ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
> > +                                    ASC_DATA_PHASE_ERROR);
>
> Missing return
>
> > +            }
> > +            buf[idx++] = 0x02; /* Ascii */
> > +            buf[idx++] = 0x00; /* Vendor Specific */
> > +            buf[idx++] = 0x00;
> > +            buf[idx++] = 20;   /* Remaining length */
> > +            padstr8(buf + idx, 20, s->drive_serial_str);
> > +            idx += 20;
> > +
> > +            /* Entry 2: Drive Model and Serial */
> > +            if (idx + 72 > max_len) {
> > +                /* 4 (header) + 8 (vendor) + 60 (model & serial) */
> > +                goto out;
> > +            }
>
> I guess this time it's okay to return early when there is not enough
> space left due to optional fields?

Yes, my goal was "best effort" at giving useful data.

> Do we need to set buf[3] (which is currently 0)?

Yes.


V2 incoming...