[Qemu-devel] [PATCH 0/4] Small SCSI command improvements

[Qemu-devel] [PATCH 0/4] Small SCSI command improvements

[Paolo Bonzini]

These includes implementation of a new MMC command, and adding the WWN
property also to SCSI disks.

Paolo Bonzini (4):
  scsi: simplify handling of the VPD page length field
  scsi: add a qdev property for the disk's WWN
  atapi: implement READ DISC INFORMATION
  scsi-disk: implement READ DISC INFORMATION

 hw/ide/atapi.c |   31 +++++++++++++++++++++++++
 hw/scsi-defs.h |    1 +
 hw/scsi-disk.c |   69 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 92 insertions(+), 9 deletions(-)

-- 
1.7.10.1

[Qemu-devel] [PATCH 1/4] scsi: simplify and fix handling of the VPD page length field

[Paolo Bonzini]

The last four bytes of the thin provisioning page were cut out.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 045c764..2bd8907 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -511,6 +511,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
     int buflen = 0;
+    int start;
 
     if (req->cmd.buf[1] & 0x1) {
         /* Vital product data */
@@ -519,14 +520,14 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
         outbuf[buflen++] = s->qdev.type & 0x1f;
         outbuf[buflen++] = page_code ; // this page
         outbuf[buflen++] = 0x00;
+        outbuf[buflen++] = 0x00;
+        start = buflen;
 
         switch (page_code) {
         case 0x00: /* Supported page codes, mandatory */
         {
-            int pages;
             DPRINTF("Inquiry EVPD[Supported pages] "
                     "buffer size %zd\n", req->cmd.xfer);
-            pages = buflen++;
             outbuf[buflen++] = 0x00; // list of supported pages (this page)
             if (s->serial) {
                 outbuf[buflen++] = 0x80; // unit serial number
@@ -536,7 +537,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
                 outbuf[buflen++] = 0xb0; // block limits
                 outbuf[buflen++] = 0xb2; // thin provisioning
             }
-            outbuf[pages] = buflen - pages - 1; // number of pages
             break;
         }
         case 0x80: /* Device serial number, optional */
@@ -555,7 +555,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
 
             DPRINTF("Inquiry EVPD[Serial number] "
                     "buffer size %zd\n", req->cmd.xfer);
-            outbuf[buflen++] = l;
             memcpy(outbuf+buflen, s->serial, l);
             buflen += l;
             break;
@@ -573,7 +572,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             DPRINTF("Inquiry EVPD[Device identification] "
                     "buffer size %zd\n", req->cmd.xfer);
 
-            outbuf[buflen++] = 4 + id_len;
             outbuf[buflen++] = 0x2; // ASCII
             outbuf[buflen++] = 0;   // not officially assigned
             outbuf[buflen++] = 0;   // reserved
@@ -598,8 +596,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
                 return -1;
             }
             /* required VPD size with unmap support */
-            outbuf[3] = buflen = 0x3c;
-
+            buflen = 0x40;
             memset(outbuf + 4, 0, buflen - 4);
 
             /* optimal transfer length granularity */
@@ -621,7 +618,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
         }
         case 0xb2: /* thin provisioning */
         {
-            outbuf[3] = buflen = 8;
+            buflen = 8;
             outbuf[4] = 0;
             outbuf[5] = 0x60; /* write_same 10/16 supported */
             outbuf[6] = s->qdev.conf.discard_granularity ? 2 : 1;
@@ -632,6 +629,8 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             return -1;
         }
         /* done with EVPD */
+        assert(buflen - start <= 255);
+        outbuf[start - 1] = buflen - start;
         return buflen;
     }
 
-- 
1.7.10.1

[Qemu-devel] [PATCH 2/4] scsi: add a qdev property for the disk's WWN

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 2bd8907..8882f69 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -68,6 +68,7 @@ struct SCSIDiskState
     bool media_changed;
     bool media_event;
     bool eject_request;
+    uint64_t wwn;
     QEMUBH *bh;
     char *version;
     char *serial;
@@ -576,9 +577,17 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[buflen++] = 0;   // not officially assigned
             outbuf[buflen++] = 0;   // reserved
             outbuf[buflen++] = id_len; // length of data following
-
             memcpy(outbuf+buflen, str, id_len);
             buflen += id_len;
+
+            if (s->wwn) {
+                outbuf[buflen++] = 0x1; // Binary
+                outbuf[buflen++] = 0x3; // NAA
+                outbuf[buflen++] = 0;   // reserved
+                outbuf[buflen++] = 8;
+                stq_be_p(&outbuf[buflen], s->wwn);
+                buflen += 8;
+            }
             break;
         }
         case 0xb0: /* block limits */
@@ -1913,6 +1922,7 @@ static Property scsi_hd_properties[] = {
                     SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
                     SCSI_DISK_F_DPOFUA, false),
+    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1957,6 +1967,7 @@ static TypeInfo scsi_hd_info = {
 
 static Property scsi_cd_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
+    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2018,6 +2029,7 @@ static Property scsi_disk_properties[] = {
                     SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
                     SCSI_DISK_F_DPOFUA, false),
+    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.7.10.1

[Qemu-devel] [PATCH 3/4] atapi: implement READ DISC INFORMATION

[Paolo Bonzini]

This command is not necessary for CD-ROM and DVD-ROM, but some versions of
udev trip on its absence.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        Kevin, ok to merge these ATAPI changes via the SCSI tree?

 hw/ide/atapi.c |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 5919cf5..7510886 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -956,6 +956,36 @@ static void cmd_read_cdvd_capacity(IDEState *s, uint8_t* buf)
     ide_atapi_cmd_reply(s, 8, 8);
 }
 
+static void cmd_read_disc_information(IDEState *s, uint8_t* buf)
+{
+    uint8_t type = buf[1] & 7;
+    uint32_t max_len = ube16_to_cpu(buf + 7);
+
+    /* Types 1/2 are only defined for Blu-Ray.  */
+    if (type != 0) {
+        ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
+                            ASC_INV_FIELD_IN_CMD_PACKET);
+        return;
+    }
+
+    memset(buf, 0, 34);
+    buf[1] = 32;
+    buf[2] = 0xe; /* last session complete, disc finalized */
+    buf[3] = 1;   /* first track on disc */
+    buf[4] = 1;   /* # of sessions */
+    buf[5] = 1;   /* first track of last session */
+    buf[6] = 1;   /* last track of last session */
+    buf[7] = 0x20; /* unrestricted use */
+    buf[8] = 0x00; /* CD-ROM or DVD-ROM */
+    /* 9-10-11: most significant byte corresponding bytes 4-5-6 */
+    /* 12-23: not meaningful for CD-ROM or DVD-ROM */
+    /* 24-31: disc bar code */
+    /* 32: disc application code */
+    /* 33: number of OPC tables */
+
+    ide_atapi_cmd_reply(s, 34, max_len);
+}
+
 static void cmd_read_dvd_structure(IDEState *s, uint8_t* buf)
 {
     int max_len;
@@ -1048,6 +1078,7 @@ static const struct {
     [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
     [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
     [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
+    [ 0x51 ] = { cmd_read_disc_information,         CHECK_READY },
     [ 0xbb ] = { cmd_set_speed,                     0 },
     [ 0xbd ] = { cmd_mechanism_status,              0 },
     [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
-- 
1.7.10.1

[Qemu-devel] [PATCH 4/4] scsi-disk: implement READ DISC INFORMATION

[Paolo Bonzini]

This command is not necessary for CD-ROM and DVD-ROM, but some versions of
udev trip on its absence.

Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-defs.h |    1 +
 hw/scsi-disk.c |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index 219c84d..3c9f1b5 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -82,6 +82,7 @@
 #define GET_EVENT_STATUS_NOTIFICATION 0x4a
 #define LOG_SELECT            0x4c
 #define LOG_SENSE             0x4d
+#define READ_DISC_INFORMATION 0x51
 #define RESERVE_TRACK         0x53
 #define MODE_SELECT_10        0x55
 #define RESERVE_10            0x56
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 8882f69..c4ad2ba 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -713,6 +713,39 @@ static inline bool media_is_cd(SCSIDiskState *s)
     return nb_sectors <= CD_MAX_SECTORS;
 }
 
+static int scsi_read_disc_information(SCSIDiskState *s, SCSIDiskReq *r,
+                                      uint8_t *outbuf)
+{
+    uint8_t type = r->req.cmd.buf[1] & 7;
+
+    if (s->qdev.type != TYPE_ROM) {
+        return -1;
+    }
+
+    /* Types 1/2 are only defined for Blu-Ray.  */
+    if (type != 0) {
+        scsi_check_condition(r, SENSE_CODE(INVALID_FIELD));
+        return -1;
+    }
+
+    memset(outbuf, 0, 34);
+    outbuf[1] = 32;
+    outbuf[2] = 0xe; /* last session complete, disc finalized */
+    outbuf[3] = 1;   /* first track on disc */
+    outbuf[4] = 1;   /* # of sessions */
+    outbuf[5] = 1;   /* first track of last session */
+    outbuf[6] = 1;   /* last track of last session */
+    outbuf[7] = 0x20; /* unrestricted use */
+    outbuf[8] = 0x00; /* CD-ROM or DVD-ROM */
+    /* 9-10-11: most significant byte corresponding bytes 4-5-6 */
+    /* 12-23: not meaningful for CD-ROM or DVD-ROM */
+    /* 24-31: disc bar code */
+    /* 32: disc application code */
+    /* 33: number of OPC tables */
+
+    return 34;
+}
+
 static int scsi_read_dvd_structure(SCSIDiskState *s, SCSIDiskReq *r,
                                    uint8_t *outbuf)
 {
@@ -1352,6 +1385,12 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
             goto illegal_request;
         }
         break;
+    case READ_DISC_INFORMATION:
+        buflen = scsi_read_disc_information(s, r, outbuf);
+        if (buflen < 0) {
+            goto illegal_request;
+        }
+        break;
     case READ_DVD_STRUCTURE:
         buflen = scsi_read_dvd_structure(s, r, outbuf);
         if (buflen < 0) {
@@ -1479,6 +1518,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
     case ALLOW_MEDIUM_REMOVAL:
     case READ_CAPACITY_10:
     case READ_TOC:
+    case READ_DISC_INFORMATION:
     case READ_DVD_STRUCTURE:
     case GET_CONFIGURATION:
     case GET_EVENT_STATUS_NOTIFICATION:
-- 
1.7.10.1

Re: [Qemu-devel] [PATCH 3/4] atapi: implement READ DISC INFORMATION

[Kevin Wolf]

Am 11.06.2012 07:24, schrieb Paolo Bonzini:
> This command is not necessary for CD-ROM and DVD-ROM, but some versions of
> udev trip on its absence.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         Kevin, ok to merge these ATAPI changes via the SCSI tree?
> 
>  hw/ide/atapi.c |   31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 5919cf5..7510886 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -956,6 +956,36 @@ static void cmd_read_cdvd_capacity(IDEState *s, uint8_t* buf)
>      ide_atapi_cmd_reply(s, 8, 8);
>  }
>  
> +static void cmd_read_disc_information(IDEState *s, uint8_t* buf)
> +{
> +    uint8_t type = buf[1] & 7;
> +    uint32_t max_len = ube16_to_cpu(buf + 7);
> +
> +    /* Types 1/2 are only defined for Blu-Ray.  */
> +    if (type != 0) {
> +        ide_atapi_cmd_error(s, ILLEGAL_REQUEST,
> +                            ASC_INV_FIELD_IN_CMD_PACKET);
> +        return;
> +    }
> +
> +    memset(buf, 0, 34);
> +    buf[1] = 32;
> +    buf[2] = 0xe; /* last session complete, disc finalized */
> +    buf[3] = 1;   /* first track on disc */
> +    buf[4] = 1;   /* # of sessions */
> +    buf[5] = 1;   /* first track of last session */
> +    buf[6] = 1;   /* last track of last session */
> +    buf[7] = 0x20; /* unrestricted use */
> +    buf[8] = 0x00; /* CD-ROM or DVD-ROM */
> +    /* 9-10-11: most significant byte corresponding bytes 4-5-6 */
> +    /* 12-23: not meaningful for CD-ROM or DVD-ROM */
> +    /* 24-31: disc bar code */
> +    /* 32: disc application code */
> +    /* 33: number of OPC tables */
> +
> +    ide_atapi_cmd_reply(s, 34, max_len);
> +}

Looks correct to me.

> +
>  static void cmd_read_dvd_structure(IDEState *s, uint8_t* buf)
>  {
>      int max_len;

> @@ -1048,6 +1078,7 @@ static const struct {
>      [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
>      [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
>      [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
> +    [ 0x51 ] = { cmd_read_disc_information,         CHECK_READY },
>      [ 0xbb ] = { cmd_set_speed,                     0 },
>      [ 0xbd ] = { cmd_mechanism_status,              0 },
>      [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },

Please keep the table sorted.

Also, if I was the SCSI maintainer, I would require qtest cases for all
four patches. ;-)

Kevin

Re: [Qemu-devel] [PATCH 3/4] atapi: implement READ DISC INFORMATION

[Paolo Bonzini]

Il 11/06/2012 14:44, Kevin Wolf ha scritto:
>> > @@ -1048,6 +1078,7 @@ static const struct {
>> >      [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
>> >      [ 0xa8 ] = { cmd_read, /* (12) */               CHECK_READY },
>> >      [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
>> > +    [ 0x51 ] = { cmd_read_disc_information,         CHECK_READY },
>> >      [ 0xbb ] = { cmd_set_speed,                     0 },
>> >      [ 0xbd ] = { cmd_mechanism_status,              0 },
>> >      [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
> Please keep the table sorted.
> 
> Also, if I was the SCSI maintainer, I would require qtest cases for all
> four patches. ;-)

Me too.  However, I'm waiting for Stefan to resubmit his virtio bindings
for qtest.

Paolo