[Qemu-devel] [PULL 0/7] SCSI patches for 2012-09-21

[Qemu-devel] [PULL 0/7] SCSI patches for 2012-09-21

[Paolo Bonzini]

Anthony,

The following changes since commit c26032b2c91721245bfec542d94f37a0238e986e:

  target-xtensa: don't emit extra tcg_gen_goto_tb (2012-09-21 03:07:27 +0400)

are available in the git repository at:

  git://github.com/bonzini/qemu.git scsi-next

for you to fetch changes up to 1109c894052751df99962c009fd7dbae397721f5:

  SCSI: Standard INQUIRY data should report HiSup flag as set. (2012-09-21 16:17:49 +0200)

Boring stuff, only cleanups and bugfixes, except for patch 1.

----------------------------------------------------------------
Paolo Bonzini (4):
      scsi-disk: introduce check_lba_range
      scsi-disk: fix check for out-of-range LBA
      scsi: introduce scsi_cdb_length and scsi_data_cdb_length
      scsi-disk: use scsi_data_cdb_length

Ronnie Sahlberg (3):
      iSCSI: We need to support SG_IO also from iscsi_ioctl()
      iSCSI: We dont need to explicitely call qemu_notify_event() any more
      SCSI: Standard INQUIRY data should report HiSup flag as set.

 block/iscsi.c  | 23 +++++++++++++++++------
 hw/scsi-bus.c  | 23 ++++++++++++++++++-----
 hw/scsi-disk.c | 44 ++++++++++++++++++++++++++------------------
 hw/scsi.h      |  2 ++
 4 file modificati, 63 inserzioni(+), 29 rimozioni(-)
-- 
1.7.12

[Qemu-devel] [PATCH 1/7] iSCSI: We need to support SG_IO also from iscsi_ioctl()

[Paolo Bonzini]

From: Ronnie Sahlberg <ronniesahlberg@gmail.com>

We need to support SG_IO from the synchronous iscsi_ioctl() since
scsi-block uses this to do an INQ to the device to discover its properties
This patch makes scsi-block work with iscsi.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 17 +++++++++++++++++
 1 file modificato, 17 inserzioni(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 0b96165..ea16609 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -628,9 +628,17 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     return &acb->common;
 }
 
+
+static void ioctl_cb(void *opaque, int status)
+{
+    int *p_status = opaque;
+    *p_status = status;
+}
+
 static int iscsi_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
     IscsiLun *iscsilun = bs->opaque;
+    int status;
 
     switch (req) {
     case SG_GET_VERSION_NUM:
@@ -639,6 +647,15 @@ static int iscsi_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
     case SG_GET_SCSI_ID:
         ((struct sg_scsi_id *)buf)->scsi_type = iscsilun->type;
         break;
+    case SG_IO:
+        status = -EINPROGRESS;
+        iscsi_aio_ioctl(bs, req, buf, ioctl_cb, &status);
+
+        while (status == -EINPROGRESS) {
+            qemu_aio_wait();
+        }
+
+        return 0;
     default:
         return -1;
     }
-- 
1.7.12

[Qemu-devel] [PATCH 2/7] iSCSI: We dont need to explicitely call qemu_notify_event() any more

[Paolo Bonzini]

From: Ronnie Sahlberg <ronniesahlberg@gmail.com>

We no longer need to explicitely call qemu_notify_event() any more
since this is now done automatically any time the filehandles we listen
to change.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 6 ------
 1 file modificato, 6 rimozioni(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index ea16609..fb001b9 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -167,12 +167,6 @@ iscsi_set_events(IscsiLun *iscsilun)
 
     }
 
-    /* If we just added an event, the callback might be delayed
-     * unless we call qemu_notify_event().
-     */
-    if (ev & ~iscsilun->events) {
-        qemu_notify_event();
-    }
     iscsilun->events = ev;
 }
 
-- 
1.7.12

[Qemu-devel] [PATCH 3/7] scsi-disk: introduce check_lba_range

[Paolo Bonzini]

Abstract the test for an out-of-range (starting block, block count)
pair.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c | 24 ++++++++++++++++--------
 1 file modificato, 16 inserzioni(+), 8 rimozioni(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 1585683..3959603 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1449,6 +1449,18 @@ invalid_field:
     return;
 }
 
+static inline bool check_lba_range(SCSIDiskState *s,
+                                   uint64_t sector_num, uint32_t nb_sectors)
+{
+    /*
+     * The first line tests that no overflow happens when computing the last
+     * sector.  The second line tests that the last accessed sector is in
+     * range.
+     */
+    return (sector_num <= sector_num + nb_sectors &&
+            sector_num + nb_sectors - 1 <= s->qdev.max_lba);
+}
+
 typedef struct UnmapCBData {
     SCSIDiskReq *r;
     uint8_t *inbuf;
@@ -1473,8 +1485,7 @@ static void scsi_unmap_complete(void *opaque, int ret)
     if (data->count > 0 && !r->req.io_canceled) {
         sector_num = ldq_be_p(&data->inbuf[0]);
         nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL;
-        if (sector_num > sector_num + nb_sectors ||
-            sector_num + nb_sectors - 1 > s->qdev.max_lba) {
+        if (!check_lba_range(s, sector_num, nb_sectors)) {
             scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
             goto done;
         }
@@ -1802,8 +1813,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
             scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
             return 0;
         }
-        if (r->req.cmd.lba > r->req.cmd.lba + nb_sectors ||
-            r->req.cmd.lba + nb_sectors - 1 > s->qdev.max_lba) {
+        if (!check_lba_range(s, r->req.cmd.lba, nb_sectors)) {
             goto illegal_lba;
         }
 
@@ -1878,8 +1888,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
         if (r->req.cmd.buf[1] & 0xe0) {
             goto illegal_request;
         }
-        if (r->req.cmd.lba > r->req.cmd.lba + len ||
-            r->req.cmd.lba + len - 1 > s->qdev.max_lba) {
+        if (!check_lba_range(s, r->req.cmd.lba, len)) {
             goto illegal_lba;
         }
         r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
@@ -1907,8 +1916,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
         if (r->req.cmd.buf[1] & 0xe0) {
             goto illegal_request;
         }
-        if (r->req.cmd.lba > r->req.cmd.lba + len ||
-            r->req.cmd.lba + len - 1 > s->qdev.max_lba) {
+        if (!check_lba_range(s, r->req.cmd.lba, len)) {
             goto illegal_lba;
         }
         r->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
-- 
1.7.12

[Qemu-devel] [PATCH 4/7] scsi-disk: fix check for out-of-range LBA

[Paolo Bonzini]

This fix is needed to correctly handle 0-block read and writes.
Without it, a 0-block access at LBA 0 would underflow.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c | 6 +++++-
 1 file modificato, 5 inserzioni(+). 1 rimozione(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 3959603..d621852 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1456,9 +1456,13 @@ static inline bool check_lba_range(SCSIDiskState *s,
      * The first line tests that no overflow happens when computing the last
      * sector.  The second line tests that the last accessed sector is in
      * range.
+     *
+     * Careful, the computations should not underflow for nb_sectors == 0,
+     * and a 0-block read to the first LBA beyond the end of device is
+     * valid.
      */
     return (sector_num <= sector_num + nb_sectors &&
-            sector_num + nb_sectors - 1 <= s->qdev.max_lba);
+            sector_num + nb_sectors <= s->qdev.max_lba + 1);
 }
 
 typedef struct UnmapCBData {
-- 
1.7.12

[Qemu-devel] [PATCH 5/7] scsi: introduce scsi_cdb_length and scsi_data_cdb_length

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-bus.c | 23 ++++++++++++++++++-----
 hw/scsi.h     |  2 ++
 2 file modificati, 20 inserzioni(+), 5 rimozioni(-)

diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 4981a02..058d3b2 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -801,26 +801,39 @@ static int ata_passthrough_16_xfer_size(SCSIDevice *dev, uint8_t *buf)
     return xfer * unit;
 }
 
-static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+uint32_t scsi_data_cdb_length(uint8_t *buf)
+{
+    if ((buf[0] >> 5) == 0 && buf[4] == 0) {
+        return 256;
+    } else {
+        return scsi_cdb_length(buf);
+    }
+}
+
+uint32_t scsi_cdb_length(uint8_t *buf)
 {
     switch (buf[0] >> 5) {
     case 0:
-        cmd->xfer = buf[4];
+        return buf[4];
         break;
     case 1:
     case 2:
-        cmd->xfer = lduw_be_p(&buf[7]);
+        return lduw_be_p(&buf[7]);
         break;
     case 4:
-        cmd->xfer = ldl_be_p(&buf[10]) & 0xffffffffULL;
+        return ldl_be_p(&buf[10]) & 0xffffffffULL;
         break;
     case 5:
-        cmd->xfer = ldl_be_p(&buf[6]) & 0xffffffffULL;
+        return ldl_be_p(&buf[6]) & 0xffffffffULL;
         break;
     default:
         return -1;
     }
+}
 
+static int scsi_req_length(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
+{
+    cmd->xfer = scsi_cdb_length(buf);
     switch (buf[0]) {
     case TEST_UNIT_READY:
     case REWIND:
diff --git a/hw/scsi.h b/hw/scsi.h
index 1aeee46..b8f7357 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -218,6 +218,8 @@ extern const struct SCSISense sense_code_WRITE_PROTECTED;
 
 #define SENSE_CODE(x) sense_code_ ## x
 
+uint32_t scsi_data_cdb_length(uint8_t *buf);
+uint32_t scsi_cdb_length(uint8_t *buf);
 int scsi_sense_valid(SCSISense sense);
 int scsi_build_sense(uint8_t *in_buf, int in_len,
                      uint8_t *buf, int len, bool fixed);
-- 
1.7.12

[Qemu-devel] [PATCH 6/7] scsi-disk: use scsi_data_cdb_length

[Paolo Bonzini]

This simplifies and unifies the parsing of READ, WRITE and WRITE SAME
commands.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c | 14 +++++---------
 1 file modificato, 5 inserzioni(+), 9 rimozioni(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index d621852..4ffca7a 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1808,11 +1808,8 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
         DPRINTF("Unmap (len %lu)\n", (long)r->req.cmd.xfer);
         break;
     case WRITE_SAME_10:
-        nb_sectors = lduw_be_p(&req->cmd.buf[7]);
-        goto write_same;
     case WRITE_SAME_16:
-        nb_sectors = ldl_be_p(&req->cmd.buf[10]) & 0xffffffffULL;
-    write_same:
+        nb_sectors = scsi_data_cdb_length(r->req.cmd.buf);
         if (bdrv_is_read_only(s->qdev.conf.bs)) {
             scsi_check_condition(r, SENSE_CODE(WRITE_PROTECTED));
             return 0;
@@ -1872,7 +1869,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
-    int32_t len;
+    uint32_t len;
     uint8_t command;
 
     command = buf[0];
@@ -1882,13 +1879,13 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
         return 0;
     }
 
+    len = scsi_data_cdb_length(r->req.cmd.buf);
     switch (command) {
     case READ_6:
     case READ_10:
     case READ_12:
     case READ_16:
-        len = r->req.cmd.xfer / s->qdev.blocksize;
-        DPRINTF("Read (sector %" PRId64 ", count %d)\n", r->req.cmd.lba, len);
+        DPRINTF("Read (sector %" PRId64 ", count %u)\n", r->req.cmd.lba, len);
         if (r->req.cmd.buf[1] & 0xe0) {
             goto illegal_request;
         }
@@ -1913,8 +1910,7 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
     case VERIFY_10:
     case VERIFY_12:
     case VERIFY_16:
-        len = r->req.cmd.xfer / s->qdev.blocksize;
-        DPRINTF("Write %s(sector %" PRId64 ", count %d)\n",
+        DPRINTF("Write %s(sector %" PRId64 ", count %u)\n",
                 (command & 0xe) == 0xe ? "And Verify " : "",
                 r->req.cmd.lba, len);
         if (r->req.cmd.buf[1] & 0xe0) {
-- 
1.7.12

[Qemu-devel] [PATCH 7/7] SCSI: Standard INQUIRY data should report HiSup flag as set.

[Paolo Bonzini]

From: Ronnie Sahlberg <ronniesahlberg@gmail.com>

QEMU as far as I know only reports LUN numbers using the modes that
are described in SAM4.
As such, since all LUN numbers generated by the SCSI emulation in QEMU
follow SAM4, we should set the HiSup bit in the standard INQUIRY data
to indicate such.

>From SAM4:
  4.6.3 LUNs overview
  All LUN formats described in this standard are hierarchical in
  structure even when only a single level in that hierarchy is used.
  The HISUP bit shall be set to one in the standard INQUIRY data
  (see SPC-4) when any LUN format described in this standard is used.
  Non-hierarchical formats are outside the scope of this standard.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 hw/scsi-disk.c | 2 +-
 1 file modificato, 1 inserzione(+). 1 rimozione(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 4ffca7a..95e9158 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -678,7 +678,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
      * is actually implemented, but we're good enough.
      */
     outbuf[2] = 5;
-    outbuf[3] = 2; /* Format 2 */
+    outbuf[3] = 2 | 0x10; /* Format 2, HiSup */
 
     if (buflen > 36) {
         outbuf[4] = buflen - 5; /* Additional Length = (Len - 1) - 4 */
-- 
1.7.12

Re: [Qemu-devel] [PULL 0/7] SCSI patches for 2012-09-21

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Anthony,
>
> The following changes since commit c26032b2c91721245bfec542d94f37a0238e986e:
>
>   target-xtensa: don't emit extra tcg_gen_goto_tb (2012-09-21 03:07:27 +0400)
>
> are available in the git repository at:
>
>   git://github.com/bonzini/qemu.git scsi-next
>
> for you to fetch changes up to 1109c894052751df99962c009fd7dbae397721f5:
>
>   SCSI: Standard INQUIRY data should report HiSup flag as set. (2012-09-21 16:17:49 +0200)
>

Pulled. Thanks.

Regards,

Anthony Liguori

> Boring stuff, only cleanups and bugfixes, except for patch 1.
>
> ----------------------------------------------------------------
> Paolo Bonzini (4):
>       scsi-disk: introduce check_lba_range
>       scsi-disk: fix check for out-of-range LBA
>       scsi: introduce scsi_cdb_length and scsi_data_cdb_length
>       scsi-disk: use scsi_data_cdb_length
>
> Ronnie Sahlberg (3):
>       iSCSI: We need to support SG_IO also from iscsi_ioctl()
>       iSCSI: We dont need to explicitely call qemu_notify_event() any more
>       SCSI: Standard INQUIRY data should report HiSup flag as set.
>
>  block/iscsi.c  | 23 +++++++++++++++++------
>  hw/scsi-bus.c  | 23 ++++++++++++++++++-----
>  hw/scsi-disk.c | 44 ++++++++++++++++++++++++++------------------
>  hw/scsi.h      |  2 ++
>  4 file modificati, 63 inserzioni(+), 29 rimozioni(-)
> -- 
> 1.7.12