[Qemu-devel] [PULL 0/7] SCSI patches for 2013-06-18 (including 1.5.1 patches)

[Qemu-devel] [PULL 0/7] SCSI patches for 2013-06-18 (including 1.5.1 patches)

[Paolo Bonzini]

The following changes since commit afd59989db90683fa127fec501d2633bcfbd6379:

  intc/xilinx_intc: Dont lower IRQ when HIE cleared (2013-06-18 09:45:00 +0200)

are available in the git repository at:

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

for you to fetch changes up to 1288844e7c4ede59509bf697a4cca0f2215a1ce3:

  iscsi: reorganize iscsi_readcapacity_sync (2013-06-18 12:43:03 +0200)

----------------------------------------------------------------
Paolo Bonzini (3):
      scsi-generic: fix sign extension of READ CAPACITY(10) data
      iscsi: simplify freeing of tasks
      iscsi: reorganize iscsi_readcapacity_sync

Pavel Hrdina (3):
      scsi: reset cdrom tray statuses on scsi_disk_reset
      scsi-generic: check the return value of bdrv_aio_ioctl in execute_command
      scsi-disk: scsi-block device for scsi pass-through should not be removable

Stefan Hajnoczi (1):
      vhost-scsi: fix k->set_guest_notifiers() NULL dereference

 block/iscsi.c          | 129 +++++++++++++++++++++----------------------------
 hw/scsi/scsi-disk.c    |  17 +++++--
 hw/scsi/scsi-generic.c |   8 ++-
 hw/scsi/vhost-scsi.c   |   2 +-
 4 files changed, 77 insertions(+), 79 deletions(-)
-- 
1.8.1.4

[Qemu-devel] [PATCH 1/7] scsi: reset cdrom tray statuses on scsi_disk_reset

[Paolo Bonzini]

From: Pavel Hrdina <phrdina@redhat.com>

Tray statuses should be also reset. Some guests may lock the tray and
right after resetting the guest it should be unlocked and closed. This
is done on power-on, reset and resume from suspend/hibernate on bare-metal.

This fix is already committed for IDE CD.
Check the commit a7f3d65b65b8c86a5ff0c0abcfefb45e2ec6fe4c.

Test results on bare-metal:
  - on reset/power-on the CD-ROM tray is closed even before the monitor
    is turned on
  - on resume from suspend/hibernate the tray is also closed before
    the monitor is turned on

>From test results it seems that this behavior is OS and probably BIOS
independent.

Cc: qemu-stable@nongnu.org
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index c8d2a99..02733dc 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1984,6 +1984,9 @@ static void scsi_disk_reset(DeviceState *dev)
         nb_sectors--;
     }
     s->qdev.max_lba = nb_sectors;
+    /* reset tray statuses */
+    s->tray_locked = 0;
+    s->tray_open = 0;
 }
 
 static void scsi_destroy(SCSIDevice *dev)
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/7] scsi-generic: fix sign extension of READ CAPACITY(10) data

[Paolo Bonzini]

Issuing the READ CAPACITY(10) command in the guest will cause QEMU
to update its knowledge of the maximum accessible LBA in the disk.
The recorded maximum LBA will be wrong if the disk is bigger than
1TB, because ldl_be_p returns a signed int.

When this is fixed, a latent bug will be unmasked.  If the READ
CAPACITY(10) command reported an overflow (0xFFFFFFFF), we must
not overwrite the previously-known maximum accessible LBA, or the guest
will fail to access the disk above the first 2TB.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-generic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 2a9a561..19bd36c 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -198,9 +198,10 @@ static void scsi_read_complete(void * opaque, int ret)
         scsi_command_complete(r, 0);
     } else {
         /* Snoop READ CAPACITY output to set the blocksize.  */
-        if (r->req.cmd.buf[0] == READ_CAPACITY_10) {
+        if (r->req.cmd.buf[0] == READ_CAPACITY_10 &&
+            (ldl_be_p(&r->buf[0]) != 0xffffffffU || s->max_lba == 0)) {
             s->blocksize = ldl_be_p(&r->buf[4]);
-            s->max_lba = ldl_be_p(&r->buf[0]);
+            s->max_lba = ldl_be_p(&r->buf[0]) & 0xffffffffULL;
         } else if (r->req.cmd.buf[0] == SERVICE_ACTION_IN_16 &&
                    (r->req.cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
             s->blocksize = ldl_be_p(&r->buf[8]);
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/7] scsi-generic: check the return value of bdrv_aio_ioctl in execute_command

[Paolo Bonzini]

From: Pavel Hrdina <phrdina@redhat.com>

This fixes the bug introduced by this commit ad54ae80c73f.
The bdrv_aio_ioctl() still could return null and we should return an error
in that case.

Cc: qemu-stable@nongnu.org
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-generic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 19bd36c..8f195be 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -174,6 +174,9 @@ static int execute_command(BlockDriverState *bdrv,
     r->io_header.flags |= SG_FLAG_DIRECT_IO;
 
     r->req.aiocb = bdrv_aio_ioctl(bdrv, SG_IO, &r->io_header, complete, r);
+    if (r->req.aiocb == NULL) {
+        return -EIO;
+    }
 
     return 0;
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/7] scsi-disk: scsi-block device for scsi pass-through should not be removable

[Paolo Bonzini]

From: Pavel Hrdina <phrdina@redhat.com>

This patch adds a new SCSI_DISK_F_NO_REMOVABLE_DEVOPS feature. By this
feature we can set that the scsi-block (scsi pass-through) device will still
be removable from the guest side, but from monitor it cannot be removed.

Cc: qemu-stable@nongnu.org
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 02733dc..74e6a14 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -61,8 +61,9 @@ typedef struct SCSIDiskReq {
     BlockAcctCookie acct;
 } SCSIDiskReq;
 
-#define SCSI_DISK_F_REMOVABLE   0
-#define SCSI_DISK_F_DPOFUA      1
+#define SCSI_DISK_F_REMOVABLE             0
+#define SCSI_DISK_F_DPOFUA                1
+#define SCSI_DISK_F_NO_REMOVABLE_DEVOPS   2
 
 struct SCSIDiskState
 {
@@ -2110,7 +2111,8 @@ static int scsi_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (s->features & (1 << SCSI_DISK_F_REMOVABLE)) {
+    if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
+            !(s->features & (1 << SCSI_DISK_F_NO_REMOVABLE_DEVOPS))) {
         bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_removable_block_ops, s);
     } else {
         bdrv_set_dev_ops(s->qdev.conf.bs, &scsi_disk_block_ops, s);
@@ -2322,6 +2324,12 @@ static int scsi_block_initfn(SCSIDevice *dev)
     } else {
         s->qdev.blocksize = 512;
     }
+
+    /* Makes the scsi-block device not removable by using HMP and QMP eject
+     * command.
+     */
+    s->features |= (1 << SCSI_DISK_F_NO_REMOVABLE_DEVOPS);
+
     return scsi_initfn(&s->qdev);
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 5/7] vhost-scsi: fix k->set_guest_notifiers() NULL dereference

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

Coverity picked up a copy-paste bug.  In vhost_scsi_start() we check for
!k->set_guest_notifiers and error out.  The check probably got copied
but instead of erroring we actually use the function pointer!

Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Asias He <asias@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/vhost-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index d7a1c33..785e93f 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -123,7 +123,7 @@ static void vhost_scsi_stop(VHostSCSI *s)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     int ret = 0;
 
-    if (!k->set_guest_notifiers) {
+    if (k->set_guest_notifiers) {
         ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
         if (ret < 0) {
                 error_report("vhost guest notifier cleanup failed: %d\n", ret);
-- 
1.8.1.4

[Qemu-devel] [PATCH 6/7] iscsi: simplify freeing of tasks

[Paolo Bonzini]

Always free them in the iscsi_aio_*_acb functions and remove the
checks in their callers.  Remove ifs when the task struct was
previously dereferenced (spotted by Coverity).

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 35 ++++++++++-------------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index f7199c1..6171b01 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -218,10 +218,8 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
         if (status == SCSI_STATUS_CHECK_CONDITION
             && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION
             && acb->retries-- > 0) {
-            if (acb->task != NULL) {
-                scsi_free_scsi_task(acb->task);
-                acb->task = NULL;
-            }
+            scsi_free_scsi_task(acb->task);
+            acb->task = NULL;
             if (iscsi_aio_writev_acb(acb) == 0) {
                 iscsi_set_events(acb->iscsilun);
                 return;
@@ -303,6 +301,7 @@ iscsi_aio_writev_acb(IscsiAIOCB *acb)
                                    acb);
 #endif
     if (ret != 0) {
+        scsi_free_scsi_task(acb->task);
         g_free(acb->buf);
         return -1;
     }
@@ -333,9 +332,6 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
     acb->retries     = ISCSI_CMD_RETRIES;
 
     if (iscsi_aio_writev_acb(acb) != 0) {
-        if (acb->task) {
-            scsi_free_scsi_task(acb->task);
-        }
         qemu_aio_release(acb);
         return NULL;
     }
@@ -364,10 +360,8 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
         if (status == SCSI_STATUS_CHECK_CONDITION
             && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION
             && acb->retries-- > 0) {
-            if (acb->task != NULL) {
-                scsi_free_scsi_task(acb->task);
-                acb->task = NULL;
-            }
+            scsi_free_scsi_task(acb->task);
+            acb->task = NULL;
             if (iscsi_aio_readv_acb(acb) == 0) {
                 iscsi_set_events(acb->iscsilun);
                 return;
@@ -445,6 +439,7 @@ iscsi_aio_readv_acb(IscsiAIOCB *acb)
                                    NULL,
                                    acb);
     if (ret != 0) {
+        scsi_free_scsi_task(acb->task);
         return -1;
     }
 
@@ -480,9 +475,6 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
     acb->retries     = ISCSI_CMD_RETRIES;
 
     if (iscsi_aio_readv_acb(acb) != 0) {
-        if (acb->task) {
-            scsi_free_scsi_task(acb->task);
-        }
         qemu_aio_release(acb);
         return NULL;
     }
@@ -509,10 +501,8 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
         if (status == SCSI_STATUS_CHECK_CONDITION
             && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION
             && acb->retries-- > 0) {
-            if (acb->task != NULL) {
-                scsi_free_scsi_task(acb->task);
-                acb->task = NULL;
-            }
+            scsi_free_scsi_task(acb->task);
+            acb->task = NULL;
             if (iscsi_aio_flush_acb(acb) == 0) {
                 iscsi_set_events(acb->iscsilun);
                 return;
@@ -589,10 +579,8 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
         if (status == SCSI_STATUS_CHECK_CONDITION
             && acb->task->sense.key == SCSI_SENSE_UNIT_ATTENTION
             && acb->retries-- > 0) {
-            if (acb->task != NULL) {
-                scsi_free_scsi_task(acb->task);
-                acb->task = NULL;
-            }
+            scsi_free_scsi_task(acb->task);
+            acb->task = NULL;
             if (iscsi_aio_discard_acb(acb) == 0) {
                 iscsi_set_events(acb->iscsilun);
                 return;
@@ -647,9 +635,6 @@ iscsi_aio_discard(BlockDriverState *bs,
     acb->retries     = ISCSI_CMD_RETRIES;
 
     if (iscsi_aio_discard_acb(acb) != 0) {
-        if (acb->task) {
-            scsi_free_scsi_task(acb->task);
-        }
         qemu_aio_release(acb);
         return NULL;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 7/7] iscsi: reorganize iscsi_readcapacity_sync

[Paolo Bonzini]

Avoid the goto, and use the same retry logic for the 10- and 16-
byte versions.

Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 94 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 46 insertions(+), 48 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6171b01..0bbf0b1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -931,60 +931,58 @@ static int iscsi_readcapacity_sync(IscsiLun *iscsilun)
     int ret = 0;
     int retries = ISCSI_CMD_RETRIES; 
 
-try_again:
-    switch (iscsilun->type) {
-    case TYPE_DISK:
-        task = iscsi_readcapacity16_sync(iscsilun->iscsi, iscsilun->lun);
-        if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-            if (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
-                    && task->sense.key == SCSI_SENSE_UNIT_ATTENTION
-                    && retries-- > 0) {
-                scsi_free_scsi_task(task);
-                goto try_again;
-            }
-            error_report("iSCSI: failed to send readcapacity16 command.");
-            ret = -EINVAL;
-            goto out;
-        }
-        rc16 = scsi_datain_unmarshall(task);
-        if (rc16 == NULL) {
-            error_report("iSCSI: Failed to unmarshall readcapacity16 data.");
-            ret = -EINVAL;
-            goto out;
-        }
-        iscsilun->block_size = rc16->block_length;
-        iscsilun->num_blocks = rc16->returned_lba + 1;
-        break;
-    case TYPE_ROM:
-        task = iscsi_readcapacity10_sync(iscsilun->iscsi, iscsilun->lun, 0, 0);
-        if (task == NULL || task->status != SCSI_STATUS_GOOD) {
-            error_report("iSCSI: failed to send readcapacity10 command.");
-            ret = -EINVAL;
-            goto out;
-        }
-        rc10 = scsi_datain_unmarshall(task);
-        if (rc10 == NULL) {
-            error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
-            ret = -EINVAL;
-            goto out;
+    do {
+        if (task != NULL) {
+            scsi_free_scsi_task(task);
+            task = NULL;
         }
-        iscsilun->block_size = rc10->block_size;
-        if (rc10->lba == 0) {
-            /* blank disk loaded */
-            iscsilun->num_blocks = 0;
-        } else {
-            iscsilun->num_blocks = rc10->lba + 1;
+
+        switch (iscsilun->type) {
+        case TYPE_DISK:
+            task = iscsi_readcapacity16_sync(iscsilun->iscsi, iscsilun->lun);
+            if (task != NULL && task->status == SCSI_STATUS_GOOD) {
+                rc16 = scsi_datain_unmarshall(task);
+                if (rc16 == NULL) {
+                    error_report("iSCSI: Failed to unmarshall readcapacity16 data.");
+                    ret = -EINVAL;
+                } else {
+                    iscsilun->block_size = rc16->block_length;
+                    iscsilun->num_blocks = rc16->returned_lba + 1;
+                }
+            }
+            break;
+        case TYPE_ROM:
+            task = iscsi_readcapacity10_sync(iscsilun->iscsi, iscsilun->lun, 0, 0);
+            if (task != NULL && task->status == SCSI_STATUS_GOOD) {
+                rc10 = scsi_datain_unmarshall(task);
+                if (rc10 == NULL) {
+                    error_report("iSCSI: Failed to unmarshall readcapacity10 data.");
+                    ret = -EINVAL;
+                } else {
+                    iscsilun->block_size = rc10->block_size;
+                    if (rc10->lba == 0) {
+                        /* blank disk loaded */
+                        iscsilun->num_blocks = 0;
+                    } else {
+                        iscsilun->num_blocks = rc10->lba + 1;
+                    }
+                }
+            }
+            break;
+        default:
+            return 0;
         }
-        break;
-    default:
-        break;
-    }
+    } while (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
+             && task->sense.key == SCSI_SENSE_UNIT_ATTENTION
+             && retries-- > 0);
 
-out:
+    if (task == NULL || task->status != SCSI_STATUS_GOOD) {
+        error_report("iSCSI: failed to send readcapacity10 command.");
+        ret = -EINVAL;
+    }
     if (task) {
         scsi_free_scsi_task(task);
     }
-
     return ret;
 }
 
-- 
1.8.1.4