[Qemu-devel] [PULL 0/5] SCSI fixed for -rc1

[Qemu-devel] [PULL 0/5] SCSI fixed for -rc1

[Paolo Bonzini]

The following changes since commit e89001f72edde37fb36fa7c964daa1bbeb2eca26:

  pc: Fix RTC CMOS info on RAM for ram_size < 1MiB (2012-08-18 16:54:23 +0000)

are available in the git repository at:

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

for you to fetch changes up to 07a5298c3067ed63f7d82d8c1a6f479146447289:

  virtio-scsi: add backwards-compatibility properties for 1.1 and earlier machines (2012-08-20 15:58:47 +0200)

They include a different fix for libiscsi AIO cancellation (which only affects
libiscsi users), and fix compat properties for virtio-scsi.

----------------------------------------------------------------
Paolo Bonzini (5):
      Revert "iscsi: Fix NULL dereferences / races between task completion and abort"
      iscsi: move iscsi_schedule_bh and iscsi_readv_writev_bh_cb
      iscsi: simplify iscsi_schedule_bh
      iscsi: fix races between task completion and abort
      virtio-scsi: add backwards-compatibility properties for 1.1 and earlier machines

 block/iscsi.c    | 122 ++++++++++++++++++++++++++++---------------------------
 hw/pc_piix.c     |   8 ++++
 hw/virtio-scsi.c |   7 ----
 hw/virtio-scsi.h |   9 +++-
 4 file modificati, 79 inserzioni(+), 67 rimozioni(-)
-- 
1.7.11.2

[Qemu-devel] [PATCH 1/5] Revert "iscsi: Fix NULL dereferences / races between task completion and abort"

[Paolo Bonzini]

This reverts commit 64e69e80920d82df3fa679bc41b13770d2f99360.  The commit
returned immediately from iscsi_aio_cancel, risking corruption in case the
following happens:

    guest                  qemu                 target
  =========================================================================
    send write 1 -------->
                           send write 1 -------->
    cancel write 1 ------>
                           cancel write 1 ------>
       <------------------ cancellation processed
    send write 2 -------->
                           send write 2 -------->
                               <---------------- completed write 2
       <------------------ completed write 2
                               <---------------- completed write 1
                               <---------------- cancellation not done

Here, the guest would see write 2 superseding write 1, when in fact the
outcome could have been the opposite.  The right behavior is to return
only after the target says whether the cancellation was done or not, and
it will be implemented by the next three patches.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 55 ++++++++++++++++++++++++++++++++-----------------------
 1 file modificato, 32 inserzioni(+), 23 rimozioni(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index bb9cf82..219f927 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -76,10 +76,6 @@ static void
 iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
                     void *private_data)
 {
-    IscsiAIOCB *acb = (IscsiAIOCB *)private_data;
-
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static void
@@ -88,15 +84,15 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
     IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
     IscsiLun *iscsilun = acb->iscsilun;
 
-    acb->canceled = 1;
-
     acb->common.cb(acb->common.opaque, -ECANCELED);
+    acb->canceled = 1;
 
-    /* send a task mgmt call to the target to cancel the task on the target
-     * this also cancels the task in libiscsi
-     */
+    /* send a task mgmt call to the target to cancel the task on the target */
     iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
-                                     iscsi_abort_task_cb, &acb);
+                                     iscsi_abort_task_cb, NULL);
+
+    /* then also cancel the task locally in libiscsi */
+    iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task);
 }
 
 static AIOPool iscsi_aio_pool = {
@@ -183,18 +179,11 @@ iscsi_readv_writev_bh_cb(void *p)
 
     qemu_bh_delete(acb->bh);
 
-    if (!acb->canceled) {
+    if (acb->canceled == 0) {
         acb->common.cb(acb->common.opaque, acb->status);
     }
 
     qemu_aio_release(acb);
-
-    if (acb->canceled) {
-        return;
-    }
-
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 
@@ -208,8 +197,10 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
 
     g_free(acb->buf);
 
-    if (acb->canceled) {
+    if (acb->canceled != 0) {
         qemu_aio_release(acb);
+        scsi_free_scsi_task(acb->task);
+        acb->task = NULL;
         return;
     }
 
@@ -221,6 +212,8 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    scsi_free_scsi_task(acb->task);
+    acb->task = NULL;
 }
 
 static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
@@ -305,8 +298,10 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
 
     trace_iscsi_aio_read16_cb(iscsi, status, acb, acb->canceled);
 
-    if (acb->canceled) {
+    if (acb->canceled != 0) {
         qemu_aio_release(acb);
+        scsi_free_scsi_task(acb->task);
+        acb->task = NULL;
         return;
     }
 
@@ -318,6 +313,8 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    scsi_free_scsi_task(acb->task);
+    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -417,8 +414,10 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
 {
     IscsiAIOCB *acb = opaque;
 
-    if (acb->canceled) {
+    if (acb->canceled != 0) {
         qemu_aio_release(acb);
+        scsi_free_scsi_task(acb->task);
+        acb->task = NULL;
         return;
     }
 
@@ -430,6 +429,8 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    scsi_free_scsi_task(acb->task);
+    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -467,8 +468,10 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
 {
     IscsiAIOCB *acb = opaque;
 
-    if (acb->canceled) {
+    if (acb->canceled != 0) {
         qemu_aio_release(acb);
+        scsi_free_scsi_task(acb->task);
+        acb->task = NULL;
         return;
     }
 
@@ -480,6 +483,8 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    scsi_free_scsi_task(acb->task);
+    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -523,8 +528,10 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
 {
     IscsiAIOCB *acb = opaque;
 
-    if (acb->canceled) {
+    if (acb->canceled != 0) {
         qemu_aio_release(acb);
+        scsi_free_scsi_task(acb->task);
+        acb->task = NULL;
         return;
     }
 
@@ -553,6 +560,8 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    scsi_free_scsi_task(acb->task);
+    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
-- 
1.7.11.2

[Qemu-devel] [PATCH 2/5] iscsi: move iscsi_schedule_bh and iscsi_readv_writev_bh_cb

[Paolo Bonzini]

Put these functions at the beginning, to avoid forward references
in the next patches.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 56 ++++++++++++++++++++++++++++----------------------------
 1 file modificato, 28 inserzioni(+), 28 rimozioni(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 219f927..600e334 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -73,6 +73,34 @@ struct IscsiTask {
 };
 
 static void
+iscsi_readv_writev_bh_cb(void *p)
+{
+    IscsiAIOCB *acb = p;
+
+    qemu_bh_delete(acb->bh);
+
+    if (acb->canceled == 0) {
+        acb->common.cb(acb->common.opaque, acb->status);
+    }
+
+    qemu_aio_release(acb);
+}
+
+static int
+iscsi_schedule_bh(QEMUBHFunc *cb, IscsiAIOCB *acb)
+{
+    acb->bh = qemu_bh_new(cb, acb);
+    if (!acb->bh) {
+        error_report("oom: could not create iscsi bh");
+        return -EIO;
+    }
+
+    qemu_bh_schedule(acb->bh);
+    return 0;
+}
+
+
+static void
 iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
                     void *private_data)
 {
@@ -159,34 +187,6 @@ iscsi_process_write(void *arg)
 }
 
 
-static int
-iscsi_schedule_bh(QEMUBHFunc *cb, IscsiAIOCB *acb)
-{
-    acb->bh = qemu_bh_new(cb, acb);
-    if (!acb->bh) {
-        error_report("oom: could not create iscsi bh");
-        return -EIO;
-    }
-
-    qemu_bh_schedule(acb->bh);
-    return 0;
-}
-
-static void
-iscsi_readv_writev_bh_cb(void *p)
-{
-    IscsiAIOCB *acb = p;
-
-    qemu_bh_delete(acb->bh);
-
-    if (acb->canceled == 0) {
-        acb->common.cb(acb->common.opaque, acb->status);
-    }
-
-    qemu_aio_release(acb);
-}
-
-
 static void
 iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
                      void *command_data, void *opaque)
-- 
1.7.11.2

[Qemu-devel] [PATCH 3/5] iscsi: simplify iscsi_schedule_bh

[Paolo Bonzini]

It is always used with the same callback, remove the argument.  And
its return value is never used, assume allocation succeeds.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 24 +++++++++---------------
 1 file modificato, 9 inserzioni(+), 15 rimozioni(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 600e334..7b09795 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -73,7 +73,7 @@ struct IscsiTask {
 };
 
 static void
-iscsi_readv_writev_bh_cb(void *p)
+iscsi_bh_cb(void *p)
 {
     IscsiAIOCB *acb = p;
 
@@ -86,17 +86,11 @@ iscsi_readv_writev_bh_cb(void *p)
     qemu_aio_release(acb);
 }
 
-static int
-iscsi_schedule_bh(QEMUBHFunc *cb, IscsiAIOCB *acb)
+static void
+iscsi_schedule_bh(IscsiAIOCB *acb)
 {
-    acb->bh = qemu_bh_new(cb, acb);
-    if (!acb->bh) {
-        error_report("oom: could not create iscsi bh");
-        return -EIO;
-    }
-
+    acb->bh = qemu_bh_new(iscsi_bh_cb, acb);
     qemu_bh_schedule(acb->bh);
-    return 0;
 }
 
 
@@ -211,7 +205,7 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
         acb->status = -EIO;
     }
 
-    iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    iscsi_schedule_bh(acb);
     scsi_free_scsi_task(acb->task);
     acb->task = NULL;
 }
@@ -312,7 +306,7 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
         acb->status = -EIO;
     }
 
-    iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    iscsi_schedule_bh(acb);
     scsi_free_scsi_task(acb->task);
     acb->task = NULL;
 }
@@ -428,7 +422,7 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
         acb->status = -EIO;
     }
 
-    iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    iscsi_schedule_bh(acb);
     scsi_free_scsi_task(acb->task);
     acb->task = NULL;
 }
@@ -482,7 +476,7 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
         acb->status = -EIO;
     }
 
-    iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    iscsi_schedule_bh(acb);
     scsi_free_scsi_task(acb->task);
     acb->task = NULL;
 }
@@ -559,7 +553,7 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
         memcpy(acb->ioh->sbp, &acb->task->datain.data[2], ss);
     }
 
-    iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
+    iscsi_schedule_bh(acb);
     scsi_free_scsi_task(acb->task);
     acb->task = NULL;
 }
-- 
1.7.11.2

[Qemu-devel] [PATCH 4/5] iscsi: fix races between task completion and abort

[Paolo Bonzini]

This patch fixes two main issues with block/iscsi.c:

1) iscsi_task_mgmt_abort_task_async calls iscsi_scsi_task_cancel which
was also directly called in iscsi_aio_cancel

2) a race between task completion and task abortion could happen cause
the scsi_free_scsi_task were done before iscsi_schedule_bh has finished.
To fix this, all the freeing of IscsiTasks and releasing of the AIOCBs
is centralized in iscsi_bh_cb, independent of whether the SCSI command
has completed or was cancelled.

3) iscsi_aio_cancel was not synchronously waiting for the end of the
command.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/iscsi.c | 59 ++++++++++++++++++++++++++++++-----------------------------
 1 file modificato, 30 inserzioni(+), 29 rimozioni(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 7b09795..4828b83 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -83,12 +83,20 @@ iscsi_bh_cb(void *p)
         acb->common.cb(acb->common.opaque, acb->status);
     }
 
+    if (acb->task != NULL) {
+        scsi_free_scsi_task(acb->task);
+        acb->task = NULL;
+    }
+
     qemu_aio_release(acb);
 }
 
 static void
 iscsi_schedule_bh(IscsiAIOCB *acb)
 {
+    if (acb->bh) {
+        return;
+    }
     acb->bh = qemu_bh_new(iscsi_bh_cb, acb);
     qemu_bh_schedule(acb->bh);
 }
@@ -98,6 +106,10 @@ static void
 iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
                     void *private_data)
 {
+    IscsiAIOCB *acb = private_data;
+
+    acb->status = -ECANCELED;
+    iscsi_schedule_bh(acb);
 }
 
 static void
@@ -106,15 +118,19 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
     IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
     IscsiLun *iscsilun = acb->iscsilun;
 
-    acb->common.cb(acb->common.opaque, -ECANCELED);
+    if (acb->status != -EINPROGRESS) {
+        return;
+    }
+
     acb->canceled = 1;
 
     /* send a task mgmt call to the target to cancel the task on the target */
     iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
-                                     iscsi_abort_task_cb, NULL);
+                                     iscsi_abort_task_cb, acb);
 
-    /* then also cancel the task locally in libiscsi */
-    iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task);
+    while (acb->status == -EINPROGRESS) {
+        qemu_aio_wait();
+    }
 }
 
 static AIOPool iscsi_aio_pool = {
@@ -192,9 +208,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
     g_free(acb->buf);
 
     if (acb->canceled != 0) {
-        qemu_aio_release(acb);
-        scsi_free_scsi_task(acb->task);
-        acb->task = NULL;
         return;
     }
 
@@ -206,8 +219,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
@@ -236,6 +247,8 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
     acb->qiov     = qiov;
 
     acb->canceled   = 0;
+    acb->bh         = NULL;
+    acb->status     = -EINPROGRESS;
 
     /* XXX we should pass the iovec to write16 to avoid the extra copy */
     /* this will allow us to get rid of 'buf' completely */
@@ -293,9 +306,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
     trace_iscsi_aio_read16_cb(iscsi, status, acb, acb->canceled);
 
     if (acb->canceled != 0) {
-        qemu_aio_release(acb);
-        scsi_free_scsi_task(acb->task);
-        acb->task = NULL;
         return;
     }
 
@@ -307,8 +317,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -334,6 +342,8 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num,
     acb->qiov     = qiov;
 
     acb->canceled    = 0;
+    acb->bh          = NULL;
+    acb->status      = -EINPROGRESS;
     acb->read_size   = qemu_read_size;
     acb->buf         = NULL;
 
@@ -409,9 +419,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
     IscsiAIOCB *acb = opaque;
 
     if (acb->canceled != 0) {
-        qemu_aio_release(acb);
-        scsi_free_scsi_task(acb->task);
-        acb->task = NULL;
         return;
     }
 
@@ -423,8 +430,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -439,6 +444,8 @@ iscsi_aio_flush(BlockDriverState *bs,
 
     acb->iscsilun = iscsilun;
     acb->canceled   = 0;
+    acb->bh         = NULL;
+    acb->status     = -EINPROGRESS;
 
     acb->task = iscsi_synchronizecache10_task(iscsi, iscsilun->lun,
                                          0, 0, 0, 0,
@@ -463,9 +470,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
     IscsiAIOCB *acb = opaque;
 
     if (acb->canceled != 0) {
-        qemu_aio_release(acb);
-        scsi_free_scsi_task(acb->task);
-        acb->task = NULL;
         return;
     }
 
@@ -477,8 +481,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -495,6 +497,8 @@ iscsi_aio_discard(BlockDriverState *bs,
 
     acb->iscsilun = iscsilun;
     acb->canceled   = 0;
+    acb->bh         = NULL;
+    acb->status     = -EINPROGRESS;
 
     list[0].lba = sector_qemu2lun(sector_num, iscsilun);
     list[0].num = nb_sectors * BDRV_SECTOR_SIZE / iscsilun->block_size;
@@ -523,9 +527,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
     IscsiAIOCB *acb = opaque;
 
     if (acb->canceled != 0) {
-        qemu_aio_release(acb);
-        scsi_free_scsi_task(acb->task);
-        acb->task = NULL;
         return;
     }
 
@@ -554,8 +555,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
@@ -573,6 +572,8 @@ static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
 
     acb->iscsilun = iscsilun;
     acb->canceled    = 0;
+    acb->bh          = NULL;
+    acb->status      = -EINPROGRESS;
     acb->buf         = NULL;
     acb->ioh         = buf;
 
-- 
1.7.11.2

[Qemu-devel] [PATCH 5/5] virtio-scsi: add backwards-compatibility properties for 1.1 and earlier machines

[Paolo Bonzini]

Hotplug and parameter change are new in 1.2, disable them via compat
properties for pc-1.1 and earlier.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pc_piix.c     | 8 ++++++++
 hw/virtio-scsi.c | 7 -------
 hw/virtio-scsi.h | 9 ++++++++-
 3 file modificati, 16 inserzioni(+), 8 rimozioni(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 0c0096f..a960710 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -360,6 +360,14 @@ static QEMUMachine pc_machine_v1_2 = {
 
 #define PC_COMPAT_1_1 \
         {\
+            .driver   = "virtio-scsi-pci",\
+            .property = "hotplug",\
+            .value    = "off",\
+        },{\
+            .driver   = "virtio-scsi-pci",\
+            .property = "param_change",\
+            .value    = "off",\
+        },{\
             .driver   = "VGA",\
             .property = "vgamem_mb",\
             .value    = stringify(8),\
diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c
index 5f737ac..c1b47a8 100644
--- a/hw/virtio-scsi.c
+++ b/hw/virtio-scsi.c
@@ -24,11 +24,6 @@
 #define VIRTIO_SCSI_MAX_TARGET  255
 #define VIRTIO_SCSI_MAX_LUN     16383
 
-/* Feature Bits */
-#define VIRTIO_SCSI_F_INOUT                    0
-#define VIRTIO_SCSI_F_HOTPLUG                  1
-#define VIRTIO_SCSI_F_CHANGE                   2
-
 /* Response codes */
 #define VIRTIO_SCSI_S_OK                       0
 #define VIRTIO_SCSI_S_OVERRUN                  1
@@ -561,8 +556,6 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
                                          uint32_t requested_features)
 {
-    requested_features |= (1UL << VIRTIO_SCSI_F_HOTPLUG);
-    requested_features |= (1UL << VIRTIO_SCSI_F_CHANGE);
     return requested_features;
 }
 
diff --git a/hw/virtio-scsi.h b/hw/virtio-scsi.h
index 4bc889d..91924f6 100644
--- a/hw/virtio-scsi.h
+++ b/hw/virtio-scsi.h
@@ -21,6 +21,11 @@
 /* The ID for virtio_scsi */
 #define VIRTIO_ID_SCSI  8
 
+/* Feature Bits */
+#define VIRTIO_SCSI_F_INOUT                    0
+#define VIRTIO_SCSI_F_HOTPLUG                  1
+#define VIRTIO_SCSI_F_CHANGE                   2
+
 struct VirtIOSCSIConf {
     uint32_t num_queues;
     uint32_t max_sectors;
@@ -31,6 +36,8 @@ struct VirtIOSCSIConf {
     DEFINE_VIRTIO_COMMON_FEATURES(_state, _features_field), \
     DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1), \
     DEFINE_PROP_UINT32("max_sectors", _state, _conf_field.max_sectors, 0xFFFF), \
-    DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128)
+    DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128), \
+    DEFINE_PROP_BIT("hotplug", _state, _features_field, VIRTIO_SCSI_F_HOTPLUG, true), \
+    DEFINE_PROP_BIT("param_change", _state, _features_field, VIRTIO_SCSI_F_CHANGE, true)
 
 #endif /* _QEMU_VIRTIO_SCSI_H */
-- 
1.7.11.2