[Qemu-devel] [PULL 00/18] Block patches

[Qemu-devel] [PULL 00/18] Block patches

[Kevin Wolf]

The following changes since commit 96d19bcbf5f679bbaaeab001b572c367fbfb2b03:

  ahci: Unbreak bar registration (2011-05-16 10:15:47 -0500)

are available in the git repository at:
  git://repo.or.cz/qemu/kevin.git for-anthony

Alexander Graf (1):
      ahci: Fix non-NCQ accesses for LBA > 16bits

Andrea Arcangeli (1):
      ide: cleanup warnings

Dmitry Konishchev (1):
      qemu_img: is_not_zero() optimization

Jan Kiszka (1):
      ahci: Fix crashes on duplicate BH registration

Jes Sorensen (2):
      qemu-img.c: Remove superfluous parenthesis
      Add documentation for qemu_progress_{init,print}()

Kevin Wolf (2):
      posix-aio-compat: Fix idle_threads counter
      ide: Turn debug messages into assertions

Markus Armbruster (6):
      ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"
      scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"
      defaults: ide-cd, ide-hd and scsi-cd devices suppress default CD-ROM
      block QMP: Deprecate query-block's "type", drop info block's "type="
      blockdev: Store -drive option media in DriveInfo
      block: Remove type hint, it's guest matter, doesn't belong here

Stefan Hajnoczi (3):
      qemu-tool: Stub out qemu-timer functions
      qed: Periodically flush and clear need check bit
      qed: support for growing images

Stefan Weil (1):
      hw/xen_disk: Remove unused local variable

 block.c            |   32 +-----------
 block.h            |    5 --
 block/qed.c        |  126 ++++++++++++++++++++++++++++++++++++++++++++++-
 block/qed.h        |    7 +++
 block_int.h        |    1 -
 blockdev.c         |    5 +-
 blockdev.h         |    1 +
 hw/ide/ahci.c      |   35 +++++++++++--
 hw/ide/core.c      |   10 ++--
 hw/ide/internal.h  |    2 +-
 hw/ide/pci.c       |    8 +--
 hw/ide/qdev.c      |   81 ++++++++++++++++++++++++-------
 hw/scsi-disk.c     |  137 +++++++++++++++++++++++++++++++++++++++-------------
 hw/xen_devconfig.c |    2 +-
 hw/xen_disk.c      |    4 +-
 posix-aio-compat.c |    6 +--
 qemu-common.h      |    2 +-
 qemu-img.c         |   35 +++++++++++--
 qemu-progress.c    |   24 ++++++++-
 qemu-tool.c        |   25 ++++++++++
 qmp-commands.hx    |   11 ++--
 trace-events       |    3 +
 vl.c               |    3 +
 23 files changed, 436 insertions(+), 129 deletions(-)

[Qemu-devel] [PATCH 01/18] ide: cleanup warnings

[Kevin Wolf]

From: Andrea Arcangeli <aarcange@redhat.com>

Add \n.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 65cb56c..f5ac932 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -298,9 +298,9 @@ void bmdma_cmd_writeb(void *opaque, uint32_t addr, uint32_t val)
                 qemu_aio_flush();
 #ifdef DEBUG_IDE
                 if (bm->bus->dma->aiocb)
-                    printf("ide_dma_cancel: aiocb still pending");
+                    printf("ide_dma_cancel: aiocb still pending\n");
                 if (bm->status & BM_STATUS_DMAING)
-                    printf("ide_dma_cancel: BM_STATUS_DMAING still pending");
+                    printf("ide_dma_cancel: BM_STATUS_DMAING still pending\n");
 #endif
             }
         } else {
-- 
1.7.2.3

[Qemu-devel] [PATCH 02/18] posix-aio-compat: Fix idle_threads counter

[Kevin Wolf]

A thread should only be counted as idle when it really is waiting for new
requests. Without this patch, sometimes too few threads are started as busy
threads are counted as idle.

Not sure if it makes a difference in practice outside some artificial
qemu-io/qemu-img tests, but I think the change makes sense in any case.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 posix-aio-compat.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index 6d4df9d..f3cc868 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -322,7 +322,9 @@ static void *aio_thread(void *unused)
 
         while (QTAILQ_EMPTY(&request_list) &&
                !(ret == ETIMEDOUT)) {
+            idle_threads++;
             ret = cond_timedwait(&cond, &lock, &ts);
+            idle_threads--;
         }
 
         if (QTAILQ_EMPTY(&request_list))
@@ -331,7 +333,6 @@ static void *aio_thread(void *unused)
         aiocb = QTAILQ_FIRST(&request_list);
         QTAILQ_REMOVE(&request_list, aiocb, node);
         aiocb->active = 1;
-        idle_threads--;
         mutex_unlock(&lock);
 
         switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
@@ -353,13 +354,11 @@ static void *aio_thread(void *unused)
 
         mutex_lock(&lock);
         aiocb->ret = ret;
-        idle_threads++;
         mutex_unlock(&lock);
 
         if (kill(pid, aiocb->ev_signo)) die("kill failed");
     }
 
-    idle_threads--;
     cur_threads--;
     mutex_unlock(&lock);
 
@@ -371,7 +370,6 @@ static void spawn_thread(void)
     sigset_t set, oldset;
 
     cur_threads++;
-    idle_threads++;
 
     /* block all signals */
     if (sigfillset(&set)) die("sigfillset");
-- 
1.7.2.3

[Qemu-devel] [PATCH 03/18] qemu-img.c: Remove superfluous parenthesis

[Kevin Wolf]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e825123..1da5484 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -785,7 +785,7 @@ static int img_convert(int argc, char **argv)
 
         nb_sectors = total_sectors;
         local_progress = (float)100 /
-            (nb_sectors / MIN(nb_sectors, (cluster_sectors)));
+            (nb_sectors / MIN(nb_sectors, cluster_sectors));
 
         for(;;) {
             int64_t bs_num;
@@ -856,7 +856,7 @@ static int img_convert(int argc, char **argv)
         sector_num = 0; // total number of sectors converted so far
         nb_sectors = total_sectors - sector_num;
         local_progress = (float)100 /
-            (nb_sectors / MIN(nb_sectors, (IO_BUF_SIZE / 512)));
+            (nb_sectors / MIN(nb_sectors, IO_BUF_SIZE / 512));
 
         for(;;) {
             nb_sectors = total_sectors - sector_num;
@@ -1331,7 +1331,7 @@ static int img_rebase(int argc, char **argv)
         bdrv_get_geometry(bs, &num_sectors);
 
         local_progress = (float)100 /
-            (num_sectors / MIN(num_sectors, (IO_BUF_SIZE / 512)));
+            (num_sectors / MIN(num_sectors, IO_BUF_SIZE / 512));
         for (sector = 0; sector < num_sectors; sector += n) {
 
             /* How many sectors can we handle with the next read? */
-- 
1.7.2.3

[Qemu-devel] [PATCH 04/18] hw/xen_disk: Remove unused local variable

[Kevin Wolf]

From: Stefan Weil <weil@mail.berlios.de>

cppcheck report:
hw/xen_disk.c:309: style:
 Variable 'len' is assigned a value that is never used

Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/xen_disk.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 233c8c9..0c298af 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -316,7 +316,7 @@ static int ioreq_map(struct ioreq *ioreq)
 static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
 {
     struct XenBlkDev *blkdev = ioreq->blkdev;
-    int i, rc, len = 0;
+    int i, rc;
     off_t pos;
 
     if (ioreq->req.nr_segments && ioreq_map(ioreq) == -1) {
@@ -339,7 +339,6 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
                               ioreq->v.iov[i].iov_len);
                 goto err;
             }
-            len += ioreq->v.iov[i].iov_len;
             pos += ioreq->v.iov[i].iov_len;
         }
         break;
@@ -359,7 +358,6 @@ static int ioreq_runio_qemu_sync(struct ioreq *ioreq)
                               ioreq->v.iov[i].iov_len);
                 goto err;
             }
-            len += ioreq->v.iov[i].iov_len;
             pos += ioreq->v.iov[i].iov_len;
         }
         break;
-- 
1.7.2.3

[Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Kevin Wolf]

These printfs aren't really debug messages, but clearly indicate a bug if they
ever become effective. Noone uses DEBUG_IDE, let's re-enable the check
unconditionally and make it an assertion instead of printfs in the device
emulation.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/ide/pci.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index f5ac932..a4726ad 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -296,12 +296,8 @@ void bmdma_cmd_writeb(void *opaque, uint32_t addr, uint32_t val)
              */
             if (bm->bus->dma->aiocb) {
                 qemu_aio_flush();
-#ifdef DEBUG_IDE
-                if (bm->bus->dma->aiocb)
-                    printf("ide_dma_cancel: aiocb still pending\n");
-                if (bm->status & BM_STATUS_DMAING)
-                    printf("ide_dma_cancel: BM_STATUS_DMAING still pending\n");
-#endif
+                assert(bm->bus->dma->aiocb == NULL);
+                assert((bm->status & BM_STATUS_DMAING) == 0);
             }
         } else {
             bm->cur_addr = bm->addr;
-- 
1.7.2.3

[Qemu-devel] [PATCH 06/18] Add documentation for qemu_progress_{init, print}()

[Kevin Wolf]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-common.h   |    2 +-
 qemu-progress.c |   24 +++++++++++++++++++++---
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/qemu-common.h b/qemu-common.h
index bba8dfe..b851b20 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -341,7 +341,7 @@ void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count,
 
 void qemu_progress_init(int enabled, float min_skip);
 void qemu_progress_end(void);
-void qemu_progress_print(float percent, int max);
+void qemu_progress_print(float delta, int max);
 
 #define QEMU_FILE_TYPE_BIOS   0
 #define QEMU_FILE_TYPE_KEYMAP 1
diff --git a/qemu-progress.c b/qemu-progress.c
index a4894c0..8ebe8ef 100644
--- a/qemu-progress.c
+++ b/qemu-progress.c
@@ -96,6 +96,13 @@ static void progress_dummy_init(void)
     state.end = progress_dummy_end;
 }
 
+/*
+ * Initialize progress reporting.
+ * If @enabled is false, actual reporting is suppressed.  The user can
+ * still trigger a report by sending a SIGUSR1.
+ * Reports are also suppressed unless we've had at least @min_skip
+ * percent progress since the last report.
+ */
 void qemu_progress_init(int enabled, float min_skip)
 {
     state.min_skip = min_skip;
@@ -111,14 +118,25 @@ void qemu_progress_end(void)
     state.end();
 }
 
-void qemu_progress_print(float percent, int max)
+/*
+ * Report progress.
+ * @delta is how much progress we made.
+ * If @max is zero, @delta is an absolut value of the total job done.
+ * Else, @delta is a progress delta since the last call, as a fraction
+ * of @max.  I.e. the delta is @delta * @max / 100. This allows
+ * relative accounting of functions which may be a different fraction of
+ * the full job, depending on the context they are called in. I.e.
+ * a function might be considered 40% of the full job if used from
+ * bdrv_img_create() but only 20% if called from img_convert().
+ */
+void qemu_progress_print(float delta, int max)
 {
     float current;
 
     if (max == 0) {
-        current = percent;
+        current = delta;
     } else {
-        current = state.current + percent / 100 * max;
+        current = state.current + delta / 100 * max;
     }
     if (current > 100) {
         current = 100;
-- 
1.7.2.3

[Qemu-devel] [PATCH 07/18] ahci: Fix crashes on duplicate BH registration

[Kevin Wolf]

From: Jan Kiszka <jan.kiszka@siemens.com>

If ahci_dma_set_inactive is called a while there is still a pending BH
from a previous run, we will crash on the second run of
ahci_check_cmd_bh as it overwrites AHCIDevice::check_bh. Avoid this
broken and redundant duplicate registration.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/ahci.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index c6e0c77..744d19d 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1066,9 +1066,11 @@ static int ahci_dma_set_inactive(IDEDMA *dma)
 
     ad->dma_cb = NULL;
 
-    /* maybe we still have something to process, check later */
-    ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
-    qemu_bh_schedule(ad->check_bh);
+    if (!ad->check_bh) {
+        /* maybe we still have something to process, check later */
+        ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
+        qemu_bh_schedule(ad->check_bh);
+    }
 
     return 0;
 }
-- 
1.7.2.3

[Qemu-devel] [PATCH 08/18] qemu-tool: Stub out qemu-timer functions

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Block drivers may use timers for flushing metadata to disk or
reconnecting to a network drive.  Stub out the following functions in
qemu-tool.c:

QEMUTimer *qemu_new_timer_ns(QEMUClock *clock, int scale,
                             QEMUTimerCB *cb, void *opaque)
void qemu_free_timer(QEMUTimer *ts)
void qemu_del_timer(QEMUTimer *ts)
void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
int64_t qemu_get_clock_ns(QEMUClock *clock)

They will result in timers never firing when linked against qemu-tool.o.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-tool.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/qemu-tool.c b/qemu-tool.c
index f4a6ad0..41e5c41 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -19,6 +19,7 @@
 #include <sys/time.h>
 
 QEMUClock *rt_clock;
+QEMUClock *vm_clock;
 
 FILE *logfile;
 
@@ -71,3 +72,27 @@ int qemu_set_fd_handler2(int fd,
 void qemu_notify_event(void)
 {
 }
+
+QEMUTimer *qemu_new_timer(QEMUClock *clock, int scale,
+                          QEMUTimerCB *cb, void *opaque)
+{
+    return qemu_malloc(1);
+}
+
+void qemu_free_timer(QEMUTimer *ts)
+{
+    qemu_free(ts);
+}
+
+void qemu_del_timer(QEMUTimer *ts)
+{
+}
+
+void qemu_mod_timer(QEMUTimer *ts, int64_t expire_time)
+{
+}
+
+int64_t qemu_get_clock_ns(QEMUClock *clock)
+{
+    return 0;
+}
-- 
1.7.2.3

[Qemu-devel] [PATCH 09/18] qed: Periodically flush and clear need check bit

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

One strategy to limit the startup delay of consistency check when
opening image files is to ensure that the file is marked dirty for as
little time as possible.

QED currently marks the image dirty when the first allocating write
request is issued and clears the dirty bit again when the image is
cleanly closed.  In practice that means the image is marked dirty for
most of a guest's lifetime and prone to being in a dirty state upon
crash or power failure.

It is safe to clear the dirty bit after all allocating write requests
have completed and a flush has been performed.  This patch adds a timer
after the last allocating write request completes.  When the timer fires
it will flush and then clear the dirty bit.  The timer is set to 5
seconds and is cancelled upon arrival of a new allocating write request.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c  |  104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block/qed.h  |    7 ++++
 trace-events |    3 ++
 3 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index c8c5930..d8d6ea2 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -12,6 +12,7 @@
  *
  */
 
+#include "qemu-timer.h"
 #include "trace.h"
 #include "qed.h"
 #include "qerror.h"
@@ -291,6 +292,88 @@ static CachedL2Table *qed_new_l2_table(BDRVQEDState *s)
 
 static void qed_aio_next_io(void *opaque, int ret);
 
+static void qed_plug_allocating_write_reqs(BDRVQEDState *s)
+{
+    assert(!s->allocating_write_reqs_plugged);
+
+    s->allocating_write_reqs_plugged = true;
+}
+
+static void qed_unplug_allocating_write_reqs(BDRVQEDState *s)
+{
+    QEDAIOCB *acb;
+
+    assert(s->allocating_write_reqs_plugged);
+
+    s->allocating_write_reqs_plugged = false;
+
+    acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs);
+    if (acb) {
+        qed_aio_next_io(acb, 0);
+    }
+}
+
+static void qed_finish_clear_need_check(void *opaque, int ret)
+{
+    /* Do nothing */
+}
+
+static void qed_flush_after_clear_need_check(void *opaque, int ret)
+{
+    BDRVQEDState *s = opaque;
+
+    bdrv_aio_flush(s->bs, qed_finish_clear_need_check, s);
+
+    /* No need to wait until flush completes */
+    qed_unplug_allocating_write_reqs(s);
+}
+
+static void qed_clear_need_check(void *opaque, int ret)
+{
+    BDRVQEDState *s = opaque;
+
+    if (ret) {
+        qed_unplug_allocating_write_reqs(s);
+        return;
+    }
+
+    s->header.features &= ~QED_F_NEED_CHECK;
+    qed_write_header(s, qed_flush_after_clear_need_check, s);
+}
+
+static void qed_need_check_timer_cb(void *opaque)
+{
+    BDRVQEDState *s = opaque;
+
+    /* The timer should only fire when allocating writes have drained */
+    assert(!QSIMPLEQ_FIRST(&s->allocating_write_reqs));
+
+    trace_qed_need_check_timer_cb(s);
+
+    qed_plug_allocating_write_reqs(s);
+
+    /* Ensure writes are on disk before clearing flag */
+    bdrv_aio_flush(s->bs, qed_clear_need_check, s);
+}
+
+static void qed_start_need_check_timer(BDRVQEDState *s)
+{
+    trace_qed_start_need_check_timer(s);
+
+    /* Use vm_clock so we don't alter the image file while suspended for
+     * migration.
+     */
+    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(vm_clock) +
+                   get_ticks_per_sec() * QED_NEED_CHECK_TIMEOUT);
+}
+
+/* It's okay to call this multiple times or when no timer is started */
+static void qed_cancel_need_check_timer(BDRVQEDState *s)
+{
+    trace_qed_cancel_need_check_timer(s);
+    qemu_del_timer(s->need_check_timer);
+}
+
 static int bdrv_qed_open(BlockDriverState *bs, int flags)
 {
     BDRVQEDState *s = bs->opaque;
@@ -406,7 +489,10 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
             BdrvCheckResult result = {0};
 
             ret = qed_check(s, &result, true);
-            if (!ret && !result.corruptions && !result.check_errors) {
+            if (ret) {
+                goto out;
+            }
+            if (!result.corruptions && !result.check_errors) {
                 /* Ensure fixes reach storage before clearing check bit */
                 bdrv_flush(s->bs);
 
@@ -416,6 +502,9 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
         }
     }
 
+    s->need_check_timer = qemu_new_timer_ns(vm_clock,
+                                            qed_need_check_timer_cb, s);
+
 out:
     if (ret) {
         qed_free_l2_cache(&s->l2_cache);
@@ -428,6 +517,9 @@ static void bdrv_qed_close(BlockDriverState *bs)
 {
     BDRVQEDState *s = bs->opaque;
 
+    qed_cancel_need_check_timer(s);
+    qemu_free_timer(s->need_check_timer);
+
     /* Ensure writes reach stable storage */
     bdrv_flush(bs->file);
 
@@ -809,6 +901,8 @@ static void qed_aio_complete(QEDAIOCB *acb, int ret)
         acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs);
         if (acb) {
             qed_aio_next_io(acb, 0);
+        } else if (s->header.features & QED_F_NEED_CHECK) {
+            qed_start_need_check_timer(s);
         }
     }
 }
@@ -1014,11 +1108,17 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
 {
     BDRVQEDState *s = acb_to_s(acb);
 
+    /* Cancel timer when the first allocating request comes in */
+    if (QSIMPLEQ_EMPTY(&s->allocating_write_reqs)) {
+        qed_cancel_need_check_timer(s);
+    }
+
     /* Freeze this request if another allocating write is in progress */
     if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs)) {
         QSIMPLEQ_INSERT_TAIL(&s->allocating_write_reqs, acb, next);
     }
-    if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs)) {
+    if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs) ||
+        s->allocating_write_reqs_plugged) {
         return; /* wait for existing request to finish */
     }
 
diff --git a/block/qed.h b/block/qed.h
index 1d1421f..388fdb3 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -78,6 +78,9 @@ enum {
     QED_MIN_TABLE_SIZE = 1,        /* in clusters */
     QED_MAX_TABLE_SIZE = 16,
     QED_DEFAULT_TABLE_SIZE = 4,
+
+    /* Delay to flush and clean image after last allocating write completes */
+    QED_NEED_CHECK_TIMEOUT = 5,    /* in seconds */
 };
 
 typedef struct {
@@ -157,6 +160,10 @@ typedef struct {
 
     /* Allocating write request queue */
     QSIMPLEQ_HEAD(, QEDAIOCB) allocating_write_reqs;
+    bool allocating_write_reqs_plugged;
+
+    /* Periodic flush and clear need check flag */
+    QEMUTimer *need_check_timer;
 } BDRVQEDState;
 
 enum {
diff --git a/trace-events b/trace-events
index a00b63c..385cb00 100644
--- a/trace-events
+++ b/trace-events
@@ -220,6 +220,9 @@ disable qed_write_table(void *s, uint64_t offset, void *table, unsigned int inde
 disable qed_write_table_cb(void *s, void *table, int flush, int ret) "s %p table %p flush %d ret %d"
 
 # block/qed.c
+disable qed_need_check_timer_cb(void *s) "s %p"
+disable qed_start_need_check_timer(void *s) "s %p"
+disable qed_cancel_need_check_timer(void *s) "s %p"
 disable qed_aio_complete(void *s, void *acb, int ret) "s %p acb %p ret %d"
 disable qed_aio_setup(void *s, void *acb, int64_t sector_num, int nb_sectors, void *opaque, int is_write) "s %p acb %p sector_num %"PRId64" nb_sectors %d opaque %p is_write %d"
 disable qed_aio_next_io(void *s, void *acb, int ret, uint64_t cur_pos) "s %p acb %p ret %d cur_pos %"PRIu64""
-- 
1.7.2.3

[Qemu-devel] [PATCH 10/18] qemu_img: is_not_zero() optimization

[Kevin Wolf]

From: Dmitry Konishchev <konishchev@gmail.com>

I run qemu-img under profiler and realized, that most of CPU time is
consumed by is_not_zero() function. I had made a couple of optimizations
on it and got the following output for `time qemu-img convert -O qcow2
volume.qcow2 snapshot.qcow2`:

Original qemu-img:
real 0m56.159s
user 0m34.670s
sys  0m12.079s

Patched qemu-img:
real 0m34.805s
user 0m18.445s
sys  0m12.552s

Signed-off-by: Dmitry Konishchev <konishchev@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c |   29 ++++++++++++++++++++++++++---
 1 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 1da5484..4f162d1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -496,14 +496,37 @@ static int img_commit(int argc, char **argv)
     return 0;
 }
 
+/*
+ * Checks whether the sector is not a zero sector.
+ *
+ * Attention! The len must be a multiple of 4 * sizeof(long) due to
+ * restriction of optimizations in this function.
+ */
 static int is_not_zero(const uint8_t *sector, int len)
 {
+    /*
+     * Use long as the biggest available internal data type that fits into the
+     * CPU register and unroll the loop to smooth out the effect of memory
+     * latency.
+     */
+
     int i;
-    len >>= 2;
-    for(i = 0;i < len; i++) {
-        if (((uint32_t *)sector)[i] != 0)
+    long d0, d1, d2, d3;
+    const long * const data = (const long *) sector;
+
+    len /= sizeof(long);
+
+    for(i = 0; i < len; i += 4) {
+        d0 = data[i + 0];
+        d1 = data[i + 1];
+        d2 = data[i + 2];
+        d3 = data[i + 3];
+
+        if (d0 || d1 || d2 || d3) {
             return 1;
+        }
     }
+
     return 0;
 }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 11/18] qed: support for growing images

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

The .bdrv_truncate() operation resizes images and growing is easy to
implement in QED.  Simply check that the new size is valid and then
update the image_size header field to reflect the new size.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index d8d6ea2..da0bf31 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1333,7 +1333,27 @@ static BlockDriverAIOCB *bdrv_qed_aio_flush(BlockDriverState *bs,
 
 static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset)
 {
-    return -ENOTSUP;
+    BDRVQEDState *s = bs->opaque;
+    uint64_t old_image_size;
+    int ret;
+
+    if (!qed_is_image_size_valid(offset, s->header.cluster_size,
+                                 s->header.table_size)) {
+        return -EINVAL;
+    }
+
+    /* Shrinking is currently not supported */
+    if ((uint64_t)offset < s->header.image_size) {
+        return -ENOTSUP;
+    }
+
+    old_image_size = s->header.image_size;
+    s->header.image_size = offset;
+    ret = qed_write_header_sync(s);
+    if (ret < 0) {
+        s->header.image_size = old_image_size;
+    }
+    return ret;
 }
 
 static int64_t bdrv_qed_getlength(BlockDriverState *bs)
-- 
1.7.2.3

[Qemu-devel] [PATCH 12/18] ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

An "ide-drive" is either a hard disk or a CD-ROM, depending on the
associated BlockDriverState's type hint.  Unclean; disk vs. CD belongs
to the guest part, not the host part.

Have separate qdevs "ide-hd" and "ide-cd" to model disk vs. CD in
the guest part.

Keep ide-drive for backward compatibility.

"ide-disk" would perhaps be a nicer name than "ide-hd", but there's
already "scsi-disk", which is like "ide-drive", and will be likewise
split in the next commit.  {ide,scsi}-{hd,cd} is the best consistent
set of names I could find within the backward compatibility
straightjacket.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c     |   11 ++++--
 hw/ide/internal.h |    2 +-
 hw/ide/qdev.c     |   83 ++++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 74 insertions(+), 22 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 90f553b..542ed65 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1592,13 +1592,15 @@ void ide_bus_reset(IDEBus *bus)
     bus->dma->ops->reset(bus->dma);
 }
 
-int ide_init_drive(IDEState *s, BlockDriverState *bs,
+int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
                    const char *version, const char *serial)
 {
     int cylinders, heads, secs;
     uint64_t nb_sectors;
 
     s->bs = bs;
+    s->drive_kind = kind;
+
     bdrv_get_geometry(bs, &nb_sectors);
     bdrv_guess_geometry(bs, &cylinders, &heads, &secs);
     if (cylinders < 1 || cylinders > 16383) {
@@ -1623,8 +1625,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs,
     s->smart_autosave = 1;
     s->smart_errors = 0;
     s->smart_selftest_count = 0;
-    if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM) {
-        s->drive_kind = IDE_CD;
+    if (kind == IDE_CD) {
         bdrv_set_change_cb(bs, cdrom_change_cb, s);
         bs->buffer_alignment = 2048;
     } else {
@@ -1729,7 +1730,9 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
         dinfo = i == 0 ? hd0 : hd1;
         ide_init1(bus, i);
         if (dinfo) {
-            if (ide_init_drive(&bus->ifs[i], dinfo->bdrv, NULL,
+            if (ide_init_drive(&bus->ifs[i], dinfo->bdrv,
+                               bdrv_get_type_hint(dinfo->bdrv) == BDRV_TYPE_CDROM ? IDE_CD : IDE_HD,
+                               NULL,
                                *dinfo->serial ? dinfo->serial : NULL) < 0) {
                 error_report("Can't set up IDE drive %s", dinfo->id);
                 exit(1);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index aa198b6..c2b35ec 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -558,7 +558,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-int ide_init_drive(IDEState *s, BlockDriverState *bs,
+int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
                    const char *version, const char *serial);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 2bb5c27..3bca726 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -98,7 +98,9 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
 {
     DeviceState *dev;
 
-    dev = qdev_create(&bus->qbus, "ide-drive");
+    dev = qdev_create(&bus->qbus,
+                      bdrv_get_type_hint(drive->bdrv) == BDRV_TYPE_CDROM
+                      ? "ide-cd" : "ide-hd");
     qdev_prop_set_uint32(dev, "unit", unit);
     qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv);
     qdev_init_nofail(dev);
@@ -118,7 +120,7 @@ typedef struct IDEDrive {
     IDEDevice dev;
 } IDEDrive;
 
-static int ide_drive_initfn(IDEDevice *dev)
+static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
     IDEState *s = bus->ifs + dev->unit;
@@ -134,7 +136,7 @@ static int ide_drive_initfn(IDEDevice *dev)
         }
     }
 
-    if (ide_init_drive(s, dev->conf.bs, dev->version, serial) < 0) {
+    if (ide_init_drive(s, dev->conf.bs, kind, dev->version, serial) < 0) {
         return -1;
     }
 
@@ -151,22 +153,69 @@ static int ide_drive_initfn(IDEDevice *dev)
     return 0;
 }
 
-static IDEDeviceInfo ide_drive_info = {
-    .qdev.name  = "ide-drive",
-    .qdev.fw_name  = "drive",
-    .qdev.size  = sizeof(IDEDrive),
-    .init       = ide_drive_initfn,
-    .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),
-        DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),
-        DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),
-        DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),
-        DEFINE_PROP_END_OF_LIST(),
+static int ide_hd_initfn(IDEDevice *dev)
+{
+    return ide_dev_initfn(dev, IDE_HD);
+}
+
+static int ide_cd_initfn(IDEDevice *dev)
+{
+    return ide_dev_initfn(dev, IDE_CD);
+}
+
+static int ide_drive_initfn(IDEDevice *dev)
+{
+    return ide_dev_initfn(dev,
+                          bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
+                          ? IDE_CD : IDE_HD);
+}
+
+#define DEFINE_IDE_DEV_PROPERTIES()                     \
+    DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1), \
+    DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),        \
+    DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
+    DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial)
+
+static IDEDeviceInfo ide_dev_info[] = {
+    {
+        .qdev.name    = "ide-hd",
+        .qdev.fw_name = "drive",
+        .qdev.desc    = "virtual IDE disk",
+        .qdev.size    = sizeof(IDEDrive),
+        .init         = ide_hd_initfn,
+        .qdev.props   = (Property[]) {
+            DEFINE_IDE_DEV_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name    = "ide-cd",
+        .qdev.fw_name = "drive",
+        .qdev.desc    = "virtual IDE CD-ROM",
+        .qdev.size    = sizeof(IDEDrive),
+        .init         = ide_cd_initfn,
+        .qdev.props   = (Property[]) {
+            DEFINE_IDE_DEV_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name    = "ide-drive", /* legacy -device ide-drive */
+        .qdev.fw_name = "drive",
+        .qdev.desc    = "virtual IDE disk or CD-ROM (legacy)",
+        .qdev.size    = sizeof(IDEDrive),
+        .init         = ide_drive_initfn,
+        .qdev.props   = (Property[]) {
+            DEFINE_IDE_DEV_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
     }
 };
 
-static void ide_drive_register(void)
+static void ide_dev_register(void)
 {
-    ide_qdev_register(&ide_drive_info);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(ide_dev_info); i++) {
+        ide_qdev_register(&ide_dev_info[i]);
+    }
 }
-device_init(ide_drive_register);
+device_init(ide_dev_register);
-- 
1.7.2.3

[Qemu-devel] [PATCH 13/18] scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

A "scsi-disk" is either a hard disk or a CD-ROM, depending on the
associated BlockDriverState's type hint.  Unclean; disk vs. CD belongs
to the guest part, not the host part.

Have separate qdevs "scsi-hd" and "scsi-cd" to model disk vs. CD in
the guest part.

Keep scsi-disk for backward compatibility.

Don't copy scsi-disk property removable to scsi-cd.  It's not used and
always zero(!) there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/scsi-disk.c |  136 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 103 insertions(+), 33 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index b05e654..8df8518 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -65,6 +65,8 @@ typedef struct SCSIDiskReq {
     uint32_t status;
 } SCSIDiskReq;
 
+typedef enum { SCSI_HD, SCSI_CD } SCSIDriveKind;
+
 struct SCSIDiskState
 {
     SCSIDevice qdev;
@@ -78,6 +80,7 @@ struct SCSIDiskState
     char *version;
     char *serial;
     SCSISense sense;
+    SCSIDriveKind drive_kind;
 };
 
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
@@ -406,7 +409,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             return -1;
         }
 
-        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
+        if (s->drive_kind == SCSI_CD) {
             outbuf[buflen++] = 5;
         } else {
             outbuf[buflen++] = 0;
@@ -424,7 +427,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[buflen++] = 0x00; // list of supported pages (this page)
             outbuf[buflen++] = 0x80; // unit serial number
             outbuf[buflen++] = 0x83; // device identification
-            if (bdrv_get_type_hint(s->bs) != BDRV_TYPE_CDROM) {
+            if (s->drive_kind == SCSI_HD) {
                 outbuf[buflen++] = 0xb0; // block limits
                 outbuf[buflen++] = 0xb2; // thin provisioning
             }
@@ -477,7 +480,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             unsigned int opt_io_size =
                     s->qdev.conf.opt_io_size / s->qdev.blocksize;
 
-            if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
+            if (s->drive_kind == SCSI_CD) {
                 DPRINTF("Inquiry (EVPD[%02X] not supported for CDROM\n",
                         page_code);
                 return -1;
@@ -547,7 +550,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
         return buflen;
     }
 
-    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
+    if (s->drive_kind == SCSI_CD) {
         outbuf[0] = 5;
         outbuf[1] = 0x80;
         memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
@@ -678,7 +681,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p,
         return p[1] + 2;
 
     case 0x2a: /* CD Capabilities and Mechanical Status page. */
-        if (bdrv_get_type_hint(bdrv) != BDRV_TYPE_CDROM)
+        if (s->drive_kind != SCSI_CD)
             return 0;
         p[0] = 0x2a;
         p[1] = 0x14;
@@ -905,7 +908,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
             goto illegal_request;
         break;
     case START_STOP:
-        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM && (req->cmd.buf[4] & 2)) {
+        if (s->drive_kind == SCSI_CD && (req->cmd.buf[4] & 2)) {
             /* load/eject medium */
             bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
         }
@@ -1232,10 +1235,9 @@ static void scsi_destroy(SCSIDevice *dev)
     blockdev_mark_auto_del(s->qdev.conf.bs);
 }
 
-static int scsi_disk_initfn(SCSIDevice *dev)
+static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
-    int is_cd;
     DriveInfo *dinfo;
 
     if (!s->qdev.conf.bs) {
@@ -1243,9 +1245,9 @@ static int scsi_disk_initfn(SCSIDevice *dev)
         return -1;
     }
     s->bs = s->qdev.conf.bs;
-    is_cd = bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM;
+    s->drive_kind = kind;
 
-    if (!is_cd && !bdrv_is_inserted(s->bs)) {
+    if (kind == SCSI_HD && !bdrv_is_inserted(s->bs)) {
         error_report("Device needs media, but drive is empty");
         return -1;
     }
@@ -1265,7 +1267,7 @@ static int scsi_disk_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (is_cd) {
+    if (kind == SCSI_CD) {
         s->qdev.blocksize = 2048;
     } else {
         s->qdev.blocksize = s->qdev.conf.logical_block_size;
@@ -1275,35 +1277,103 @@ static int scsi_disk_initfn(SCSIDevice *dev)
 
     s->qdev.type = TYPE_DISK;
     qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
-    bdrv_set_removable(s->bs, is_cd);
+    bdrv_set_removable(s->bs, kind == SCSI_CD);
     add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
     return 0;
 }
 
-static SCSIDeviceInfo scsi_disk_info = {
-    .qdev.name    = "scsi-disk",
-    .qdev.fw_name = "disk",
-    .qdev.desc    = "virtual scsi disk or cdrom",
-    .qdev.size    = sizeof(SCSIDiskState),
-    .qdev.reset   = scsi_disk_reset,
-    .init         = scsi_disk_initfn,
-    .destroy      = scsi_destroy,
-    .send_command = scsi_send_command,
-    .read_data    = scsi_read_data,
-    .write_data   = scsi_write_data,
-    .cancel_io    = scsi_cancel_io,
-    .get_buf      = scsi_get_buf,
-    .qdev.props   = (Property[]) {
-        DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
-        DEFINE_PROP_STRING("ver",  SCSIDiskState, version),
-        DEFINE_PROP_STRING("serial",  SCSIDiskState, serial),
-        DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),
-        DEFINE_PROP_END_OF_LIST(),
-    },
+static int scsi_hd_initfn(SCSIDevice *dev)
+{
+    return scsi_initfn(dev, SCSI_HD);
+}
+
+static int scsi_cd_initfn(SCSIDevice *dev)
+{
+    return scsi_initfn(dev, SCSI_CD);
+}
+
+static int scsi_disk_initfn(SCSIDevice *dev)
+{
+    SCSIDriveKind kind;
+
+    if (!dev->conf.bs) {
+        kind = SCSI_HD;         /* will die in scsi_initfn() */
+    } else {
+        kind = bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
+            ? SCSI_CD : SCSI_HD;
+    }
+
+    return scsi_initfn(dev, kind);
+}
+
+#define DEFINE_SCSI_DISK_PROPERTIES()                           \
+    DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),          \
+    DEFINE_PROP_STRING("ver",  SCSIDiskState, version),         \
+    DEFINE_PROP_STRING("serial",  SCSIDiskState, serial)
+
+static SCSIDeviceInfo scsi_disk_info[] = {
+    {
+        .qdev.name    = "scsi-hd",
+        .qdev.fw_name = "disk",
+        .qdev.desc    = "virtual SCSI disk",
+        .qdev.size    = sizeof(SCSIDiskState),
+        .qdev.reset   = scsi_disk_reset,
+        .init         = scsi_hd_initfn,
+        .destroy      = scsi_destroy,
+        .send_command = scsi_send_command,
+        .read_data    = scsi_read_data,
+        .write_data   = scsi_write_data,
+        .cancel_io    = scsi_cancel_io,
+        .get_buf      = scsi_get_buf,
+        .qdev.props   = (Property[]) {
+            DEFINE_SCSI_DISK_PROPERTIES(),
+            DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name    = "scsi-cd",
+        .qdev.fw_name = "disk",
+        .qdev.desc    = "virtual SCSI CD-ROM",
+        .qdev.size    = sizeof(SCSIDiskState),
+        .qdev.reset   = scsi_disk_reset,
+        .init         = scsi_cd_initfn,
+        .destroy      = scsi_destroy,
+        .send_command = scsi_send_command,
+        .read_data    = scsi_read_data,
+        .write_data   = scsi_write_data,
+        .cancel_io    = scsi_cancel_io,
+        .get_buf      = scsi_get_buf,
+        .qdev.props   = (Property[]) {
+            DEFINE_SCSI_DISK_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        },
+    },{
+        .qdev.name    = "scsi-disk", /* legacy -device scsi-disk */
+        .qdev.fw_name = "disk",
+        .qdev.desc    = "virtual SCSI disk or CD-ROM (legacy)",
+        .qdev.size    = sizeof(SCSIDiskState),
+        .qdev.reset   = scsi_disk_reset,
+        .init         = scsi_disk_initfn,
+        .destroy      = scsi_destroy,
+        .send_command = scsi_send_command,
+        .read_data    = scsi_read_data,
+        .write_data   = scsi_write_data,
+        .cancel_io    = scsi_cancel_io,
+        .get_buf      = scsi_get_buf,
+        .qdev.props   = (Property[]) {
+            DEFINE_SCSI_DISK_PROPERTIES(),
+            DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    }
 };
 
 static void scsi_disk_register_devices(void)
 {
-    scsi_qdev_register(&scsi_disk_info);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(scsi_disk_info); i++) {
+        scsi_qdev_register(&scsi_disk_info[i]);
+    }
 }
 device_init(scsi_disk_register_devices)
-- 
1.7.2.3

[Qemu-devel] [PATCH 14/18] defaults: ide-cd, ide-hd and scsi-cd devices suppress default CD-ROM

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

ide-hd has to suppress the default CD-ROM, or else you can't put one
on secondary master without -nodefaults.

Unlike legacy scsi-disk, scsi-cd suppresses default CD-ROM.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 vl.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index bffba69..b362871 100644
--- a/vl.c
+++ b/vl.c
@@ -279,7 +279,10 @@ static struct {
     { .driver = "isa-serial",           .flag = &default_serial    },
     { .driver = "isa-parallel",         .flag = &default_parallel  },
     { .driver = "isa-fdc",              .flag = &default_floppy    },
+    { .driver = "ide-cd",               .flag = &default_cdrom     },
+    { .driver = "ide-hd",               .flag = &default_cdrom     },
     { .driver = "ide-drive",            .flag = &default_cdrom     },
+    { .driver = "scsi-cd",              .flag = &default_cdrom     },
     { .driver = "virtio-serial-pci",    .flag = &default_virtcon   },
     { .driver = "virtio-serial-s390",   .flag = &default_virtcon   },
     { .driver = "virtio-serial",        .flag = &default_virtcon   },
-- 
1.7.2.3

[Qemu-devel] [PATCH 15/18] block QMP: Deprecate query-block's "type", drop info block's "type="

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

query-block's specification documents response member "type" with
values "hd", "cdrom", "floppy", "unknown".

Its value is unreliable: a block device used as floppy has type
"floppy" if created with if=floppy, but type "hd" if created with
if=none.

That's because with if=none, the type is at best a declaration of
intent: the drive can be connected to any guest device.  Its type is
really the guest device's business.  Reporting it here is wrong.

No known user of QMP uses "type".  It's unlikely that any unknown
users exist, because its value is useless unless you know how the
block device was created.  But then you also know the true value.

Fixing the broken value risks breaking (hypothetical!) clients that
somehow rely on the current behavior.  Not fixing the value risks
breaking (hypothetical!) clients that rely on the value to be
accurate.  Can't entirely avoid hypothetical lossage.  Change the
value to be always "unknown".

This makes "info block" always report "type=unknown".  Pointless.
Change it to not report the type.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c         |   20 +++-----------------
 qmp-commands.hx |   11 ++++++-----
 2 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index f403718..9de7450 100644
--- a/block.c
+++ b/block.c
@@ -1704,9 +1704,8 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
 
     bs_dict = qobject_to_qdict(obj);
 
-    monitor_printf(mon, "%s: type=%s removable=%d",
+    monitor_printf(mon, "%s: removable=%d",
                         qdict_get_str(bs_dict, "device"),
-                        qdict_get_str(bs_dict, "type"),
                         qdict_get_bool(bs_dict, "removable"));
 
     if (qdict_get_bool(bs_dict, "removable")) {
@@ -1747,23 +1746,10 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
         QObject *bs_obj;
-        const char *type = "unknown";
-
-        switch(bs->type) {
-        case BDRV_TYPE_HD:
-            type = "hd";
-            break;
-        case BDRV_TYPE_CDROM:
-            type = "cdrom";
-            break;
-        case BDRV_TYPE_FLOPPY:
-            type = "floppy";
-            break;
-        }
 
-        bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
+        bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
                                     "'removable': %i, 'locked': %i }",
-                                    bs->device_name, type, bs->removable,
+                                    bs->device_name, bs->removable,
                                     bs->locked);
 
         if (bs->drv) {
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fbd98ee..a9f109a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1039,7 +1039,8 @@ Each json-object contain the following:
 
 - "device": device name (json-string)
 - "type": device type (json-string)
-         - Possible values: "hd", "cdrom", "floppy", "unknown"
+         - deprecated, retained for backward compatibility
+         - Possible values: "unknown"
 - "removable": true if the device is removable, false otherwise (json-bool)
 - "locked": true if the device is locked, false otherwise (json-bool)
 - "inserted": only present if the device is inserted, it is a json-object
@@ -1070,25 +1071,25 @@ Example:
                "encrypted":false,
                "file":"disks/test.img"
             },
-            "type":"hd"
+            "type":"unknown"
          },
          {
             "device":"ide1-cd0",
             "locked":false,
             "removable":true,
-            "type":"cdrom"
+            "type":"unknown"
          },
          {
             "device":"floppy0",
             "locked":false,
             "removable":true,
-            "type": "floppy"
+            "type":"unknown"
          },
          {
             "device":"sd0",
             "locked":false,
             "removable":true,
-            "type":"floppy"
+            "type":"unknown"
          }
       ]
    }
-- 
1.7.2.3

[Qemu-devel] [PATCH 16/18] blockdev: Store -drive option media in DriveInfo

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

DriveInfo is closely tied to -drive, and like -drive, it mixes
information about host and guest part of the block device.  Unlike
DriveInfo, BlockDriverState should be about the host part only.

One of the remaining guest bits there is the "type hint".  -drive
option media sets it, and qdevs "ide-drive", "scsi-disk" and non-qdev
IF_XEN devices check it to pick HD vs. CD.

Communicate -drive option media via new DriveInfo member media_cd
instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c         |    1 +
 blockdev.h         |    1 +
 hw/ide/core.c      |    3 +--
 hw/ide/qdev.c      |   10 ++++------
 hw/scsi-disk.c     |    5 +++--
 hw/xen_devconfig.c |    2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5429621..28727df 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -488,6 +488,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 	    break;
 	case MEDIA_CDROM:
             bdrv_set_type_hint(dinfo->bdrv, BDRV_TYPE_CDROM);
+            dinfo->media_cd = 1;
 	    break;
 	}
         break;
diff --git a/blockdev.h b/blockdev.h
index 2c9e780..3587786 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -33,6 +33,7 @@ struct DriveInfo {
     int bus;
     int unit;
     int auto_del;               /* see blockdev_mark_auto_del() */
+    int media_cd;
     QemuOpts *opts;
     char serial[BLOCK_SERIAL_STRLEN + 1];
     QTAILQ_ENTRY(DriveInfo) next;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 542ed65..45410e8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1731,8 +1731,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
         ide_init1(bus, i);
         if (dinfo) {
             if (ide_init_drive(&bus->ifs[i], dinfo->bdrv,
-                               bdrv_get_type_hint(dinfo->bdrv) == BDRV_TYPE_CDROM ? IDE_CD : IDE_HD,
-                               NULL,
+                               dinfo->media_cd ? IDE_CD : IDE_HD, NULL,
                                *dinfo->serial ? dinfo->serial : NULL) < 0) {
                 error_report("Can't set up IDE drive %s", dinfo->id);
                 exit(1);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 3bca726..3f9dc89 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -98,9 +98,7 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
 {
     DeviceState *dev;
 
-    dev = qdev_create(&bus->qbus,
-                      bdrv_get_type_hint(drive->bdrv) == BDRV_TYPE_CDROM
-                      ? "ide-cd" : "ide-hd");
+    dev = qdev_create(&bus->qbus, drive->media_cd ? "ide-cd" : "ide-hd");
     qdev_prop_set_uint32(dev, "unit", unit);
     qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv);
     qdev_init_nofail(dev);
@@ -165,9 +163,9 @@ static int ide_cd_initfn(IDEDevice *dev)
 
 static int ide_drive_initfn(IDEDevice *dev)
 {
-    return ide_dev_initfn(dev,
-                          bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
-                          ? IDE_CD : IDE_HD);
+    DriveInfo *dinfo = drive_get_by_blockdev(dev->conf.bs);
+
+    return ide_dev_initfn(dev, dinfo->media_cd ? IDE_CD : IDE_HD);
 }
 
 #define DEFINE_IDE_DEV_PROPERTIES()                     \
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 8df8518..397b9d6 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1295,12 +1295,13 @@ static int scsi_cd_initfn(SCSIDevice *dev)
 static int scsi_disk_initfn(SCSIDevice *dev)
 {
     SCSIDriveKind kind;
+    DriveInfo *dinfo;
 
     if (!dev->conf.bs) {
         kind = SCSI_HD;         /* will die in scsi_initfn() */
     } else {
-        kind = bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
-            ? SCSI_CD : SCSI_HD;
+        dinfo = drive_get_by_blockdev(dev->conf.bs);
+        kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
     }
 
     return scsi_initfn(dev, kind);
diff --git a/hw/xen_devconfig.c b/hw/xen_devconfig.c
index 8d50216..3a92155 100644
--- a/hw/xen_devconfig.c
+++ b/hw/xen_devconfig.c
@@ -96,7 +96,7 @@ int xen_config_dev_blk(DriveInfo *disk)
 {
     char fe[256], be[256];
     int vdev = 202 * 256 + 16 * disk->unit;
-    int cdrom = disk->bdrv->type == BDRV_TYPE_CDROM;
+    int cdrom = disk->media_cd;
     const char *devtype = cdrom ? "cdrom" : "disk";
     const char *mode    = cdrom ? "r"     : "w";
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 17/18] block: Remove type hint, it's guest matter, doesn't belong here

[Kevin Wolf]

From: Markus Armbruster <armbru@redhat.com>

No users of bdrv_get_type_hint() left.  bdrv_set_type_hint() can make
the media removable by side effect.  Make that explicit.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c     |   12 ------------
 block.h     |    5 -----
 block_int.h |    1 -
 blockdev.c  |    4 ++--
 4 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index 9de7450..effa86f 100644
--- a/block.c
+++ b/block.c
@@ -1305,13 +1305,6 @@ void bdrv_set_geometry_hint(BlockDriverState *bs,
     bs->secs = secs;
 }
 
-void bdrv_set_type_hint(BlockDriverState *bs, int type)
-{
-    bs->type = type;
-    bs->removable = ((type == BDRV_TYPE_CDROM ||
-                      type == BDRV_TYPE_FLOPPY));
-}
-
 void bdrv_set_translation_hint(BlockDriverState *bs, int translation)
 {
     bs->translation = translation;
@@ -1428,11 +1421,6 @@ void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
     }
 }
 
-int bdrv_get_type_hint(BlockDriverState *bs)
-{
-    return bs->type;
-}
-
 int bdrv_get_translation_hint(BlockDriverState *bs)
 {
     return bs->translation;
diff --git a/block.h b/block.h
index 52e9cad..da7d39c 100644
--- a/block.h
+++ b/block.h
@@ -152,9 +152,6 @@ int bdrv_has_zero_init(BlockDriverState *bs);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 	int *pnum);
 
-#define BDRV_TYPE_HD     0
-#define BDRV_TYPE_CDROM  1
-#define BDRV_TYPE_FLOPPY 2
 #define BIOS_ATA_TRANSLATION_AUTO   0
 #define BIOS_ATA_TRANSLATION_NONE   1
 #define BIOS_ATA_TRANSLATION_LBA    2
@@ -163,7 +160,6 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 
 void bdrv_set_geometry_hint(BlockDriverState *bs,
                             int cyls, int heads, int secs);
-void bdrv_set_type_hint(BlockDriverState *bs, int type);
 void bdrv_set_translation_hint(BlockDriverState *bs, int translation);
 void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs);
@@ -177,7 +173,6 @@ typedef enum FDriveType {
 void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
                                    int *max_track, int *last_sect,
                                    FDriveType drive_in, FDriveType *drive);
-int bdrv_get_type_hint(BlockDriverState *bs);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
                        BlockErrorAction on_write_error);
diff --git a/block_int.h b/block_int.h
index 545ad11..fa91337 100644
--- a/block_int.h
+++ b/block_int.h
@@ -194,7 +194,6 @@ struct BlockDriverState {
     /* NOTE: the following infos are only hints for real hardware
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;
-    int type;
     BlockErrorAction on_read_error, on_write_error;
     char device_name[32];
     unsigned long *dirty_bitmap;
diff --git a/blockdev.c b/blockdev.c
index 28727df..6e0eb83 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -487,7 +487,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             }
 	    break;
 	case MEDIA_CDROM:
-            bdrv_set_type_hint(dinfo->bdrv, BDRV_TYPE_CDROM);
+            bdrv_set_removable(dinfo->bdrv, 1);
             dinfo->media_cd = 1;
 	    break;
 	}
@@ -496,7 +496,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
         /* FIXME: This isn't really a floppy, but it's a reasonable
            approximation.  */
     case IF_FLOPPY:
-        bdrv_set_type_hint(dinfo->bdrv, BDRV_TYPE_FLOPPY);
+        bdrv_set_removable(dinfo->bdrv, 1);
         break;
     case IF_PFLASH:
     case IF_MTD:
-- 
1.7.2.3

[Qemu-devel] [PATCH 18/18] ahci: Fix non-NCQ accesses for LBA > 16bits

[Kevin Wolf]

From: Alexander Graf <agraf@suse.de>

AHCI provides two ways of reading/writing data:

 1) NCQ
 2) ATA commands with the LBA in the command FIS

In the second code path, we didn't handle any LBAs that were bigger than
16 bits, so whenever a guest that used high LBA numbers wanted to access
data, the LBA got truncated down to 16 bits, giving the guest garbage.

This patch adds support for LBAs higher than 16 bits. I've tested that it
works just fine with SeaBIOS and Linux guests. This patch also unbreaks
the often reported grub errors people have seen with AHCI.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/ahci.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 744d19d..1f008a3 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -884,8 +884,31 @@ static int handle_cmd(AHCIState *s, int port, int slot)
         }
 
         if (ide_state->drive_kind != IDE_CD) {
-            ide_set_sector(ide_state, (cmd_fis[6] << 16) | (cmd_fis[5] << 8) |
-                           cmd_fis[4]);
+            /*
+             * We set the sector depending on the sector defined in the FIS.
+             * Unfortunately, the spec isn't exactly obvious on this one.
+             *
+             * Apparently LBA48 commands set fis bytes 10,9,8,6,5,4 to the
+             * 48 bit sector number. ATA_CMD_READ_DMA_EXT is an example for
+             * such a command.
+             *
+             * Non-LBA48 commands however use 7[lower 4 bits],6,5,4 to define a
+             * 28-bit sector number. ATA_CMD_READ_DMA is an example for such
+             * a command.
+             *
+             * Since the spec doesn't explicitly state what each field should
+             * do, I simply assume non-used fields as reserved and OR everything
+             * together, independent of the command.
+             */
+            ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40)
+                                    | ((uint64_t)cmd_fis[9] << 32)
+                                    /* This is used for LBA48 commands */
+                                    | ((uint64_t)cmd_fis[8] << 24)
+                                    /* This is used for non-LBA48 commands */
+                                    | ((uint64_t)(cmd_fis[7] & 0xf) << 24)
+                                    | ((uint64_t)cmd_fis[6] << 16)
+                                    | ((uint64_t)cmd_fis[5] << 8)
+                                    | cmd_fis[4]);
         }
 
         /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
-- 
1.7.2.3

Re: [Qemu-devel] [PULL 00/18] Block patches

[Anthony Liguori]

On 05/19/2011 07:33 AM, Kevin Wolf wrote:
> The following changes since commit 96d19bcbf5f679bbaaeab001b572c367fbfb2b03:
>
>    ahci: Unbreak bar registration (2011-05-16 10:15:47 -0500)
>
> are available in the git repository at:
>    git://repo.or.cz/qemu/kevin.git for-anthony

Pulled.  Thanks.

Regards,

Anthony Liguori

>
> Alexander Graf (1):
>        ahci: Fix non-NCQ accesses for LBA>  16bits
>
> Andrea Arcangeli (1):
>        ide: cleanup warnings
>
> Dmitry Konishchev (1):
>        qemu_img: is_not_zero() optimization
>
> Jan Kiszka (1):
>        ahci: Fix crashes on duplicate BH registration
>
> Jes Sorensen (2):
>        qemu-img.c: Remove superfluous parenthesis
>        Add documentation for qemu_progress_{init,print}()
>
> Kevin Wolf (2):
>        posix-aio-compat: Fix idle_threads counter
>        ide: Turn debug messages into assertions
>
> Markus Armbruster (6):
>        ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"
>        scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"
>        defaults: ide-cd, ide-hd and scsi-cd devices suppress default CD-ROM
>        block QMP: Deprecate query-block's "type", drop info block's "type="
>        blockdev: Store -drive option media in DriveInfo
>        block: Remove type hint, it's guest matter, doesn't belong here
>
> Stefan Hajnoczi (3):
>        qemu-tool: Stub out qemu-timer functions
>        qed: Periodically flush and clear need check bit
>        qed: support for growing images
>
> Stefan Weil (1):
>        hw/xen_disk: Remove unused local variable
>
>   block.c            |   32 +-----------
>   block.h            |    5 --
>   block/qed.c        |  126 ++++++++++++++++++++++++++++++++++++++++++++++-
>   block/qed.h        |    7 +++
>   block_int.h        |    1 -
>   blockdev.c         |    5 +-
>   blockdev.h         |    1 +
>   hw/ide/ahci.c      |   35 +++++++++++--
>   hw/ide/core.c      |   10 ++--
>   hw/ide/internal.h  |    2 +-
>   hw/ide/pci.c       |    8 +--
>   hw/ide/qdev.c      |   81 ++++++++++++++++++++++++-------
>   hw/scsi-disk.c     |  137 +++++++++++++++++++++++++++++++++++++++-------------
>   hw/xen_devconfig.c |    2 +-
>   hw/xen_disk.c      |    4 +-
>   posix-aio-compat.c |    6 +--
>   qemu-common.h      |    2 +-
>   qemu-img.c         |   35 +++++++++++--
>   qemu-progress.c    |   24 ++++++++-
>   qemu-tool.c        |   25 ++++++++++
>   qmp-commands.hx    |   11 ++--
>   trace-events       |    3 +
>   vl.c               |    3 +
>   23 files changed, 436 insertions(+), 129 deletions(-)

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Luiz Capitulino]

On Thu, 19 May 2011 14:33:19 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> These printfs aren't really debug messages, but clearly indicate a bug if they
> ever become effective.

Then we have a bug somewhere, starting a VM with:

 # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0

Where the host's CDROM is empty, triggers one of these asserts:

 qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'

> Noone uses DEBUG_IDE, let's re-enable the check
> unconditionally and make it an assertion instead of printfs in the device
> emulation.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  hw/ide/pci.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index f5ac932..a4726ad 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -296,12 +296,8 @@ void bmdma_cmd_writeb(void *opaque, uint32_t addr, uint32_t val)
>               */
>              if (bm->bus->dma->aiocb) {
>                  qemu_aio_flush();
> -#ifdef DEBUG_IDE
> -                if (bm->bus->dma->aiocb)
> -                    printf("ide_dma_cancel: aiocb still pending\n");
> -                if (bm->status & BM_STATUS_DMAING)
> -                    printf("ide_dma_cancel: BM_STATUS_DMAING still pending\n");
> -#endif
> +                assert(bm->bus->dma->aiocb == NULL);
> +                assert((bm->status & BM_STATUS_DMAING) == 0);
>              }
>          } else {
>              bm->cur_addr = bm->addr;

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Kevin Wolf]

Am 26.05.2011 23:12, schrieb Luiz Capitulino:
> On Thu, 19 May 2011 14:33:19 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> These printfs aren't really debug messages, but clearly indicate a bug if they
>> ever become effective.
> 
> Then we have a bug somewhere, starting a VM with:
> 
>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
> 
> Where the host's CDROM is empty, triggers one of these asserts:
> 
>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'

Hm, works for me. Do you need some guest OS interaction or do you get
the crash immediately?

Kevin

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Luiz Capitulino]

On Fri, 27 May 2011 08:39:05 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 26.05.2011 23:12, schrieb Luiz Capitulino:
> > On Thu, 19 May 2011 14:33:19 +0200
> > Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> >> These printfs aren't really debug messages, but clearly indicate a bug if they
> >> ever become effective.
> > 
> > Then we have a bug somewhere, starting a VM with:
> > 
> >  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
> > 
> > Where the host's CDROM is empty, triggers one of these asserts:
> > 
> >  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'
> 
> Hm, works for me. Do you need some guest OS interaction or do you get
> the crash immediately?

It crashes during boot. It's a F14 guest.

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Luiz Capitulino]

On Thu, 26 May 2011 18:12:08 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Thu, 19 May 2011 14:33:19 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > These printfs aren't really debug messages, but clearly indicate a bug if they
> > ever become effective.
> 
> Then we have a bug somewhere, starting a VM with:
> 
>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
> 
> Where the host's CDROM is empty, triggers one of these asserts:
> 
>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'

I found out why this is happening. I'm passing '-snapshot' to the command-line,
sorry for not mentioning it (I forgot I was using my devel alias).

I also found out that /usr/bin/eject in the guest won't work when
-snapshot is used. Shouldn't qemu ignore this flag when using cdrom
passthrough?

> 
> > Noone uses DEBUG_IDE, let's re-enable the check
> > unconditionally and make it an assertion instead of printfs in the device
> > emulation.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > ---
> >  hw/ide/pci.c |    8 ++------
> >  1 files changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> > index f5ac932..a4726ad 100644
> > --- a/hw/ide/pci.c
> > +++ b/hw/ide/pci.c
> > @@ -296,12 +296,8 @@ void bmdma_cmd_writeb(void *opaque, uint32_t addr, uint32_t val)
> >               */
> >              if (bm->bus->dma->aiocb) {
> >                  qemu_aio_flush();
> > -#ifdef DEBUG_IDE
> > -                if (bm->bus->dma->aiocb)
> > -                    printf("ide_dma_cancel: aiocb still pending\n");
> > -                if (bm->status & BM_STATUS_DMAING)
> > -                    printf("ide_dma_cancel: BM_STATUS_DMAING still pending\n");
> > -#endif
> > +                assert(bm->bus->dma->aiocb == NULL);
> > +                assert((bm->status & BM_STATUS_DMAING) == 0);
> >              }
> >          } else {
> >              bm->cur_addr = bm->addr;
> 

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Kevin Wolf]

Am 01.06.2011 15:44, schrieb Luiz Capitulino:
> On Thu, 26 May 2011 18:12:08 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
>> On Thu, 19 May 2011 14:33:19 +0200
>> Kevin Wolf <kwolf@redhat.com> wrote:
>>
>>> These printfs aren't really debug messages, but clearly indicate a bug if they
>>> ever become effective.
>>
>> Then we have a bug somewhere, starting a VM with:
>>
>>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
>>
>> Where the host's CDROM is empty, triggers one of these asserts:
>>
>>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'
> 
> I found out why this is happening. I'm passing '-snapshot' to the command-line,
> sorry for not mentioning it (I forgot I was using my devel alias).

And suddenly it's reproducible. :-)

I'll have a look.

> I also found out that /usr/bin/eject in the guest won't work when
> -snapshot is used. Shouldn't qemu ignore this flag when using cdrom
> passthrough?

"Won't work" means that it works like with a CD-ROM image? That would be
what I expect, as you end up having a qcow2 image with -snapshot.

Not sure what's the best way of fixing this. Maybe just ignoring
-snapshot for read-only block devices? Or we could try and forward the
eject request to the backing file if the format driver doesn't handle it.

Kevin

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Luiz Capitulino]

On Wed, 01 Jun 2011 16:02:56 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 01.06.2011 15:44, schrieb Luiz Capitulino:
> > On Thu, 26 May 2011 18:12:08 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > 
> >> On Thu, 19 May 2011 14:33:19 +0200
> >> Kevin Wolf <kwolf@redhat.com> wrote:
> >>
> >>> These printfs aren't really debug messages, but clearly indicate a bug if they
> >>> ever become effective.
> >>
> >> Then we have a bug somewhere, starting a VM with:
> >>
> >>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
> >>
> >> Where the host's CDROM is empty, triggers one of these asserts:
> >>
> >>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'
> > 
> > I found out why this is happening. I'm passing '-snapshot' to the command-line,
> > sorry for not mentioning it (I forgot I was using my devel alias).
> 
> And suddenly it's reproducible. :-)
> 
> I'll have a look.

Thanks.

> > I also found out that /usr/bin/eject in the guest won't work when
> > -snapshot is used. Shouldn't qemu ignore this flag when using cdrom
> > passthrough?
> 
> "Won't work" means that it works like with a CD-ROM image?

I guess so.

> That would be
> what I expect, as you end up having a qcow2 image with -snapshot.
> 
> Not sure what's the best way of fixing this. Maybe just ignoring
> -snapshot for read-only block devices?

I think that makes sense as the reason to use -snapshot is to avoid
writing to the image/device.

> Or we could try and forward the
> eject request to the backing file if the format driver doesn't handle it.
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.06.2011 15:44, schrieb Luiz Capitulino:
>> On Thu, 26 May 2011 18:12:08 -0300
>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> 
>>> On Thu, 19 May 2011 14:33:19 +0200
>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>
>>>> These printfs aren't really debug messages, but clearly indicate a bug if they
>>>> ever become effective.
>>>
>>> Then we have a bug somewhere, starting a VM with:
>>>
>>>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
>>>
>>> Where the host's CDROM is empty, triggers one of these asserts:
>>>
>>>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'
>> 
>> I found out why this is happening. I'm passing '-snapshot' to the command-line,
>> sorry for not mentioning it (I forgot I was using my devel alias).
>
> And suddenly it's reproducible. :-)
>
> I'll have a look.
>
>> I also found out that /usr/bin/eject in the guest won't work when
>> -snapshot is used. Shouldn't qemu ignore this flag when using cdrom
>> passthrough?
>
> "Won't work" means that it works like with a CD-ROM image? That would be
> what I expect, as you end up having a qcow2 image with -snapshot.

With a qcow2 stacked onto the host_cdrom.  The block layer forwards the
guest's eject only if the top driver has a bdrv_eject() method.  Which
qcow2 doesn't have.  I guess bdrv_eject() fails with -ENOTSUP, and
cmd_start_stop_unit() reports a funny error to the guest.

> Not sure what's the best way of fixing this. Maybe just ignoring
> -snapshot for read-only block devices?

Why not, the combination is pointless.

>                                        Or we could try and forward the
> eject request to the backing file if the format driver doesn't handle it.

For what it's worth, the "raw" driver forwards manually.

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Kevin Wolf]

Am 01.06.2011 17:32, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 01.06.2011 15:44, schrieb Luiz Capitulino:
>>> On Thu, 26 May 2011 18:12:08 -0300
>>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>>
>>>> On Thu, 19 May 2011 14:33:19 +0200
>>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>>
>>>>> These printfs aren't really debug messages, but clearly indicate a bug if they
>>>>> ever become effective.
>>>>
>>>> Then we have a bug somewhere, starting a VM with:
>>>>
>>>>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
>>>>
>>>> Where the host's CDROM is empty, triggers one of these asserts:
>>>>
>>>>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'
>>>
>>> I found out why this is happening. I'm passing '-snapshot' to the command-line,
>>> sorry for not mentioning it (I forgot I was using my devel alias).
>>
>> And suddenly it's reproducible. :-)
>>
>> I'll have a look.
>>
>>> I also found out that /usr/bin/eject in the guest won't work when
>>> -snapshot is used. Shouldn't qemu ignore this flag when using cdrom
>>> passthrough?
>>
>> "Won't work" means that it works like with a CD-ROM image? That would be
>> what I expect, as you end up having a qcow2 image with -snapshot.
> 
> With a qcow2 stacked onto the host_cdrom.  The block layer forwards the
> guest's eject only if the top driver has a bdrv_eject() method.  Which
> qcow2 doesn't have.  I guess bdrv_eject() fails with -ENOTSUP, and
> cmd_start_stop_unit() reports a funny error to the guest.

bdrv_eject ignores -ENOTSUP and only changes the virtual tray in this
case. Just the very same thing as with normal ISO images.

>> Not sure what's the best way of fixing this. Maybe just ignoring
>> -snapshot for read-only block devices?
> 
> Why not, the combination is pointless.

It could start making a difference in some obscure combinations. Imagine
a read-only image with a backing file, -snapshot and the 'commit'
monitor command.

Sounds pretty insane, but I wouldn't bet that people aren't using it...

>>                                        Or we could try and forward the
>> eject request to the backing file if the format driver doesn't handle it.
> 
> For what it's worth, the "raw" driver forwards manually.

Yeah, but copying that into each driver probably isn't the right thing
to do. For a generic implementation we could probably first try the
driver itself, if it returns -ENOTSUP we try the protocol, and if that
returns -ENOTSUP, too, we ask the backing file driver.

Can't wait to see the requests that with a qcow2 formatted CD with a
backing file in another CD drive the other one (or both) should be
ejected. :-)

Kevin

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.06.2011 17:32, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> Am 01.06.2011 15:44, schrieb Luiz Capitulino:
>>>> On Thu, 26 May 2011 18:12:08 -0300
>>>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>>>
>>>>> On Thu, 19 May 2011 14:33:19 +0200
>>>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>>>
>>>>>> These printfs aren't really debug messages, but clearly indicate a bug if they
>>>>>> ever become effective.
>>>>>
>>>>> Then we have a bug somewhere, starting a VM with:
>>>>>
>>>>>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
>>>>>
>>>>> Where the host's CDROM is empty, triggers one of these asserts:
>>>>>
>>>>>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'
>>>>
>>>> I found out why this is happening. I'm passing '-snapshot' to the command-line,
>>>> sorry for not mentioning it (I forgot I was using my devel alias).
>>>
>>> And suddenly it's reproducible. :-)
>>>
>>> I'll have a look.
>>>
>>>> I also found out that /usr/bin/eject in the guest won't work when
>>>> -snapshot is used. Shouldn't qemu ignore this flag when using cdrom
>>>> passthrough?
>>>
>>> "Won't work" means that it works like with a CD-ROM image? That would be
>>> what I expect, as you end up having a qcow2 image with -snapshot.
>> 
>> With a qcow2 stacked onto the host_cdrom.  The block layer forwards the
>> guest's eject only if the top driver has a bdrv_eject() method.  Which
>> qcow2 doesn't have.  I guess bdrv_eject() fails with -ENOTSUP, and
>> cmd_start_stop_unit() reports a funny error to the guest.
>
> bdrv_eject ignores -ENOTSUP and only changes the virtual tray in this
> case. Just the very same thing as with normal ISO images.

Missed that.  Thanks.

>>> Not sure what's the best way of fixing this. Maybe just ignoring
>>> -snapshot for read-only block devices?
>> 
>> Why not, the combination is pointless.
>
> It could start making a difference in some obscure combinations. Imagine
> a read-only image with a backing file, -snapshot and the 'commit'
> monitor command.
>
> Sounds pretty insane, but I wouldn't bet that people aren't using it...

People try all kinds of insane things.  The question is whether we can
change it anyway.

>>>                                        Or we could try and forward the
>>> eject request to the backing file if the format driver doesn't handle it.
>> 
>> For what it's worth, the "raw" driver forwards manually.
>
> Yeah, but copying that into each driver probably isn't the right thing
> to do.

I didn't mean to hold up the "raw" driver as a shining example of how to
do stuff.

>        For a generic implementation we could probably first try the
> driver itself, if it returns -ENOTSUP we try the protocol, and if that
> returns -ENOTSUP, too, we ask the backing file driver.

I don't want to start another "format vs. protocol" semantic war, just
point out that the general case is a tree of block drivers, and a
general rule for passing eject up the tree better covers that.

The current block.c provides for binary trees (bs->file and
bs->backing_hd).  When we discussed blockdev_add, we came up with much
wilder trees.

> Can't wait to see the requests that with a qcow2 formatted CD with a
> backing file in another CD drive the other one (or both) should be
> ejected. :-)

Requests like that can make you wish for a way to eject users ;)

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Kevin Wolf]

Am 06.06.2011 13:57, schrieb Markus Armbruster:
>>>> Not sure what's the best way of fixing this. Maybe just ignoring
>>>> -snapshot for read-only block devices?
>>>
>>> Why not, the combination is pointless.
>>
>> It could start making a difference in some obscure combinations. Imagine
>> a read-only image with a backing file, -snapshot and the 'commit'
>> monitor command.
>>
>> Sounds pretty insane, but I wouldn't bet that people aren't using it...
> 
> People try all kinds of insane things.  The question is whether we can
> change it anyway.

We have a backing file chain like base <- cow [<- tmp], and the drive is
read-only.

Currently, 'commit' means that tmp is committed to cow (i.e. nothing
happens because it's read-only). After changing it, we would commit the
content of cow to base and possibly corrupt other images that are based
on base.

We can hope that nobody would be hit by it in practice, but it's not a
change I'd feel very comfortable about.

>>>>                                        Or we could try and forward the
>>>> eject request to the backing file if the format driver doesn't handle it.
>>>
>>> For what it's worth, the "raw" driver forwards manually.
>>
>> Yeah, but copying that into each driver probably isn't the right thing
>> to do.
> 
> I didn't mean to hold up the "raw" driver as a shining example of how to
> do stuff.
> 
>>        For a generic implementation we could probably first try the
>> driver itself, if it returns -ENOTSUP we try the protocol, and if that
>> returns -ENOTSUP, too, we ask the backing file driver.
> 
> I don't want to start another "format vs. protocol" semantic war, just
> point out that the general case is a tree of block drivers, and a
> general rule for passing eject up the tree better covers that.
> 
> The current block.c provides for binary trees (bs->file and
> bs->backing_hd).  When we discussed blockdev_add, we came up with much
> wilder trees.

I didn't mean to imply that you suggested raw was a shining example.
However, if we're going for the wild trees (or even graphs), there's no
simple option of doing it in generic code any more. Backing files and
protocols (for lack of a better term) are special in the tree because
they are a concept that the block layer knows. For everything else the
least we need is a hint from the block driver.

In any case passing up the tree means that you need to decide which
child is the one to try first, or if there are children to which it
shouldn't be passed at all.

Kevin

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 06.06.2011 13:57, schrieb Markus Armbruster:
>>>>> Not sure what's the best way of fixing this. Maybe just ignoring
>>>>> -snapshot for read-only block devices?
>>>>
>>>> Why not, the combination is pointless.
>>>
>>> It could start making a difference in some obscure combinations. Imagine
>>> a read-only image with a backing file, -snapshot and the 'commit'
>>> monitor command.
>>>
>>> Sounds pretty insane, but I wouldn't bet that people aren't using it...
>> 
>> People try all kinds of insane things.  The question is whether we can
>> change it anyway.
>
> We have a backing file chain like base <- cow [<- tmp], and the drive is
> read-only.
>
> Currently, 'commit' means that tmp is committed to cow (i.e. nothing
> happens because it's read-only). After changing it, we would commit the
> content of cow to base and possibly corrupt other images that are based
> on base.
>
> We can hope that nobody would be hit by it in practice, but it's not a
> change I'd feel very comfortable about.

So the one effect -snapshot has on a read-only drive is to neuter the
commit command.  Hmm.

Naive question: shouldn't commit require the drive to be read/write?  It
writes both the backing image and the COW...

[...]

Re: [Qemu-devel] [PATCH 05/18] ide: Turn debug messages into assertions

[Kevin Wolf]

Am 01.06.2011 15:44, schrieb Luiz Capitulino:
> On Thu, 26 May 2011 18:12:08 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
>> On Thu, 19 May 2011 14:33:19 +0200
>> Kevin Wolf <kwolf@redhat.com> wrote:
>>
>>> These printfs aren't really debug messages, but clearly indicate a bug if they
>>> ever become effective.
>>
>> Then we have a bug somewhere, starting a VM with:
>>
>>  # qemu -hda disks/test.img -enable-kvm -m 1G -cdrom /dev/sr0
>>
>> Where the host's CDROM is empty, triggers one of these asserts:
>>
>>  qmp-unstable/hw/ide/pci.c:299: bmdma_cmd_writeb: Assertion `bm->bus->dma->aiocb == ((void *)0)'
> 
> I found out why this is happening. I'm passing '-snapshot' to the command-line,
> sorry for not mentioning it (I forgot I was using my devel alias).

Okay, so I think we have at least two problems here:

* qcow2 doesn't implement bdrv_is_inserted, so the IDE emulation thinks
  there is a medium when there isn't. Reading produces only errors.

* This seems to be an error case where bdrv_aio_readv fails
  immediately. In this case, qcow2 calls the callback directly, but it
  should use a bottom half. This leads to an unexpected state in IDE
  and lets the assertion fail.

I think we must fix the latter. Not sure what to do about the former,
it's probably similar to forwarding bdrv_eject().

Kevin