All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] ide: Implement VPD response for ATAPI
@ 2014-12-04 18:59 John Snow
  2014-12-09 10:35 ` Stefan Hajnoczi
  0 siblings, 1 reply; 3+ messages in thread
From: John Snow @ 2014-12-04 18:59 UTC (permalink / raw
  To: qemu-devel; +Cc: kwolf, pbonzini, John Snow, armbru, stefanha

SCSI devices have multiple kinds of queries they need to respond
to, as defined in the "cmd inquiry" section in MMC-6 and SPC-3.

Relevent sections:
MMC-6 revision 2g:
      Non-VPD response data and pointer to SPC-3;
      Section 6.8 "Inquiry Command"
SPC-3 revision 23:
      Inquiry command and error handling:
      Section 6.4 "INQUIRY command"
      VPD data pages format:
      Section 7.6 "Vital product data parameters"

We implement these Vital Product Data queries for SCSI, but not for
ATAPI through IDE. The result is that if you are looking for the WWN
identifier via tools such as sg3_utils, you will be unable to query
our CD/DVD rom device to obtain it.

This patch adds the minimum number of mandatory responses as defined
by SPC-3, which include the "supported pages" response (page 0x00)
and the "Device Identification" response (page 0x83). It also correctly
responds when it receives a request for an illegal page to improve
error output from related tools.

The Device ID page contains an arbitrary list of identification
strings of various formats; the ID strings included in this patch
were chosen to mimic those provided by the libata driver when
emulating this SCSI query (model, serial, and wwn when present.)

Example:

# libata emulated response
[root@localhost ~]# sg_inq --id /dev/sda
VPD INQUIRY: Device Identification page
  Designation descriptor number 1, descriptor length: 24
    designator_type: vendor specific [0x0],  code_set: ASCII
    associated with the addressed logical unit
      vendor specific: QM00001
  Designation descriptor number 2, descriptor length: 72
    designator_type: T10 vendor identification,  code_set: ASCII
    associated with the addressed logical unit
      vendor id: ATA
      vendor specific: QEMU HARDDISK                           QM00001

# QEMU generated ATAPI response, with WWN
[root@localhost ~]# sg_inq --id /dev/sr0
VPD INQUIRY: Device Identification page
  Designation descriptor number 1, descriptor length: 24
    designator_type: vendor specific [0x0],  code_set: ASCII
    associated with the addressed logical unit
      vendor specific: QM00005
  Designation descriptor number 2, descriptor length: 72
    designator_type: T10 vendor identification,  code_set: ASCII
    associated with the addressed logical unit
      vendor id: ATA
      vendor specific: QEMU DVD-ROM                            QM00005
  Designation descriptor number 3, descriptor length: 12
    designator_type: NAA,  code_set: Binary
    associated with the addressed logical unit
      NAA 5, IEEE Company_id: 0xc50
      Vendor Specific Identifier: 0x15ea71bb
      [0x5000c50015ea71bb]

See also: hw/scsi/scsi-disk.c, scsi_disk_emulate_inquiry()

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c    | 103 +++++++++++++++++++++++++++++++++++++++++++++++-------
 hw/ide/internal.h |   1 +
 2 files changed, 92 insertions(+), 12 deletions(-)

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];
+    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);
+            }
+            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;
+            }
+            buf[idx++] = 0x02; /* Ascii */
+            buf[idx++] = 0x01; /* T10 Vendor */
+            buf[idx++] = 0x00;
+            buf[idx++] = 68;
+            padstr8(buf + idx, 8, "ATA"); /* Generic T10 vendor */
+            idx += 8;
+            padstr8(buf + idx, 40, s->drive_model_str);
+            idx += 40;
+            padstr8(buf + idx, 20, s->drive_serial_str);
+            idx += 20;
+
+            /* Entry 3: WWN */
+            if (s->wwn && (idx + 12 <= max_len)) {
+                /* 4 byte header + 8 byte wwn */
+                buf[idx++] = 0x01; /* Binary */
+                buf[idx++] = 0x03; /* NAA */
+                buf[idx++] = 0x00;
+                buf[idx++] = 0x08;
+                stq_be_p(&buf[idx], s->wwn);
+                idx += 8;
+            }
+
+            /* Size of the keys we added, minus the header: */
+            buf[3] = idx - 4;
+
+            break;
+        default:
+            /* SPC-3, revision 23 sec. 6.4 */
+            ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
+                                ASC_INV_FIELD_IN_CMD_PACKET);
+            return;
+        }
+    } else {
+        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);
+        idx = 36;
+    }
+
+ out:
+    ide_atapi_cmd_reply(s, idx, max_len);
+    return;
 }
 
 static void cmd_get_configuration(IDEState *s, uint8_t *buf)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 8a3eca4..c998003 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -296,6 +296,7 @@ typedef struct IDEDMAOps IDEDMAOps;
 #define ASC_INCOMPATIBLE_FORMAT              0x30
 #define ASC_MEDIUM_NOT_PRESENT               0x3a
 #define ASC_SAVING_PARAMETERS_NOT_SUPPORTED  0x39
+#define ASC_DATA_PHASE_ERROR                 0x4b
 #define ASC_MEDIA_REMOVAL_PREVENTED          0x53
 
 #define CFA_NO_ERROR            0x00
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [Qemu-devel] [PATCH] ide: Implement VPD response for ATAPI
  2014-12-04 18:59 [Qemu-devel] [PATCH] ide: Implement VPD response for ATAPI John Snow
@ 2014-12-09 10:35 ` Stefan Hajnoczi
  2014-12-10 17:56   ` John Snow
  0 siblings, 1 reply; 3+ messages in thread
From: Stefan Hajnoczi @ 2014-12-09 10:35 UTC (permalink / raw
  To: John Snow; +Cc: kwolf, pbonzini, qemu-devel, armbru

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

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.

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

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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Qemu-devel] [PATCH] ide: Implement VPD response for ATAPI
  2014-12-09 10:35 ` Stefan Hajnoczi
@ 2014-12-10 17:56   ` John Snow
  0 siblings, 0 replies; 3+ messages in thread
From: John Snow @ 2014-12-10 17:56 UTC (permalink / raw
  To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, qemu-devel, armbru



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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2014-12-10 17:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-04 18:59 [Qemu-devel] [PATCH] ide: Implement VPD response for ATAPI John Snow
2014-12-09 10:35 ` Stefan Hajnoczi
2014-12-10 17:56   ` John Snow

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.