[Qemu-devel] [PATCHv2 0/6] stable migration image on a stopped vm

[Qemu-devel] [PATCHv2 0/6] stable migration image on a stopped vm

[Michael S. Tsirkin]

I sent the exact same patches offline to some of the Cc'd,
there's been no change but they have been tested by Jason now.
I'll queue them up unless there are some comments.

What these patches do is arrive at a state where migrate to exec with vm
stopped produces a stable result.

With these patches applied it's stable, under the following
conditions:

1. run with -rtc clock=vm otherwise the rtc clock
   keeps running with vm stopped.
2. kvm clock compiled out (-kvmclock in -cpu should
   have same efect but does not, this is a bug,
   and it breaks migration between hosts with
   and without kvmclock support in kernel).
3. ratelimit set to a very large value (like 100g)

Tested-by: Jason Wang <jasowang@redhat.com>


Michael S. Tsirkin (6):
  virtio-net: don't dma while vm is stopped
  cpus: flush all requests on each vm stop
  migration/savevm: no need to flush requests
  virtio-net: stop/start bh on vm start/stop
  migration: stable ram block ordering
  migration: allow rate > 4g

 arch_init.c     |   35 +++++++++++++++++++++++++++++++++++
 buffered_file.c |    9 ++++++---
 cpu-common.h    |    3 +++
 cpus.c          |    2 ++
 exec.c          |   24 ++++++++++++++++++++++--
 hw/hw.h         |    8 ++++----
 hw/virtio-net.c |   39 +++++++++++++++++++++++++++++----------
 kvm-all.c       |    2 +-
 migration.c     |    8 ++++----
 savevm.c        |    8 ++------
 10 files changed, 108 insertions(+), 30 deletions(-)

-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 1/6] virtio-net: don't dma while vm is stopped

[Michael S. Tsirkin]

DMA into memory while VM is stopped makes it
hard to debug migration (consequitive saves
result in different files).
Fixing this completely is a large effort,
this patch does this for virtio-net.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio-net.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 1d61f19..43a2b3d 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -424,6 +424,9 @@ static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq)
 static int virtio_net_can_receive(VLANClientState *nc)
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
+    if (!n->vm_running) {
+        return 0;
+    }
 
     if (!virtio_queue_ready(n->rx_vq) ||
         !(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK))
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 2/6] cpus: flush all requests on each vm stop

[Michael S. Tsirkin]

Make sure disk is in consistent state.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
---
 cpus.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/cpus.c b/cpus.c
index 91a0fb1..d421a96 100644
--- a/cpus.c
+++ b/cpus.c
@@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
         cpu_disable_ticks();
         vm_running = 0;
         pause_all_vcpus();
+        qemu_aio_flush();
+        bdrv_flush_all();
         vm_state_notify(0, reason);
         monitor_protocol_event(QEVENT_STOP, NULL);
     }
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 3/6] migration/savevm: no need to flush requests

[Michael S. Tsirkin]

There's no need to flush requests after vmstop
as vmstop does it for us automatically now.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
---
 migration.c |    2 --
 savevm.c    |    4 ----
 2 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/migration.c b/migration.c
index 9ee8b17..15f7f35 100644
--- a/migration.c
+++ b/migration.c
@@ -368,8 +368,6 @@ void migrate_fd_put_ready(void *opaque)
         DPRINTF("done iterating\n");
         vm_stop(0);
 
-        qemu_aio_flush();
-        bdrv_flush_all();
         if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
             if (old_vm_running) {
                 vm_start();
diff --git a/savevm.c b/savevm.c
index 4e49765..49e78a5 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1575,8 +1575,6 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
     saved_vm_running = vm_running;
     vm_stop(0);
 
-    bdrv_flush_all();
-
     ret = qemu_savevm_state_begin(mon, f, 0, 0);
     if (ret < 0)
         goto out;
@@ -1885,8 +1883,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
     }
-    /* ??? Should this occur after vm_stop?  */
-    qemu_aio_flush();
 
     saved_vm_running = vm_running;
     vm_stop(0);
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 4/6] virtio-net: stop/start bh on vm start/stop

[Michael S. Tsirkin]

Avoid sending out packets, and modifying
device state, when VM is stopped.

Add a bunch or assert statements to verify this does not happen.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio-net.c |   36 ++++++++++++++++++++++++++----------
 1 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 43a2b3d..366a801 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -675,11 +675,12 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 {
     VirtQueueElement elem;
     int32_t num_packets = 0;
-
     if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
         return num_packets;
     }
 
+    assert(n->vm_running);
+
     if (n->async_tx.elem.out_num) {
         virtio_queue_set_notification(n->tx_vq, 0);
         return num_packets;
@@ -737,6 +738,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIONet *n = to_virtio_net(vdev);
+    assert(n->vm_running);
 
     if (n->tx_waiting) {
         virtio_queue_set_notification(vq, 1);
@@ -754,6 +756,7 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
 static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIONet *n = to_virtio_net(vdev);
+    assert(n->vm_running);
 
     if (unlikely(n->tx_waiting)) {
         return;
@@ -766,6 +769,7 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
 static void virtio_net_tx_timer(void *opaque)
 {
     VirtIONet *n = opaque;
+    assert(n->vm_running);
 
     n->tx_waiting = 0;
 
@@ -781,6 +785,9 @@ static void virtio_net_tx_bh(void *opaque)
 {
     VirtIONet *n = opaque;
     int32_t ret;
+    if (!n->vm_running) {
+        return;
+    }
 
     n->tx_waiting = 0;
 
@@ -926,15 +933,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
     n->mac_table.first_multi = i;
-
-    if (n->tx_waiting) {
-        if (n->tx_timer) {
-            qemu_mod_timer(n->tx_timer,
-                           qemu_get_clock(vm_clock) + n->tx_timeout);
-        } else {
-            qemu_bh_schedule(n->tx_bh);
-        }
-    }
     return 0;
 }
 
@@ -962,6 +960,24 @@ static void virtio_net_vmstate_change(void *opaque, int running, int reason)
      * it will start/stop vhost backend if appropriate
      * e.g. after migration. */
     virtio_net_set_status(&n->vdev, n->vdev.status);
+
+    if (!n->tx_waiting) {
+        return;
+    }
+    if (running) {
+        if (n->tx_timer) {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+        } else {
+            qemu_bh_schedule(n->tx_bh);
+        }
+    } else {
+        if (n->tx_timer) {
+            qemu_del_timer(n->tx_timer);
+        } else {
+            qemu_bh_cancel(n->tx_bh);
+        }
+    }
 }
 
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 5/6] migration: stable ram block ordering

[Michael S. Tsirkin]

This makes ram block ordering under migration stable, ordered by offset.
This is especially useful for migration to exec, for debugging.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
---
 arch_init.c  |   35 +++++++++++++++++++++++++++++++++++
 cpu-common.h |    3 +++
 exec.c       |   24 ++++++++++++++++++++++--
 kvm-all.c    |    2 +-
 4 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 4486925..e32e289 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -23,6 +23,7 @@
  */
 #include <stdint.h>
 #include <stdarg.h>
+#include <stdlib.h>
 #ifndef _WIN32
 #include <sys/types.h>
 #include <sys/mman.h>
@@ -212,6 +213,39 @@ uint64_t ram_bytes_total(void)
     return total;
 }
 
+static int block_compar(const void *a, const void *b)
+{
+    RAMBlock * const *ablock = a;
+    RAMBlock * const *bblock = b;
+    if ((*ablock)->offset < (*bblock)->offset) {
+        return -1;
+    } else if ((*ablock)->offset > (*bblock)->offset) {
+        return 1;
+    }
+    return 0;
+}
+
+static void sort_ram_list(void)
+{
+    RAMBlock *block, *nblock, **blocks;
+    int n;
+    n = 0;
+    QLIST_FOREACH(block, &ram_list.blocks, next) {
+        ++n;
+    }
+    blocks = qemu_malloc(n * sizeof *blocks);
+    n = 0;
+    QLIST_FOREACH_SAFE(block, &ram_list.blocks, next, nblock) {
+        blocks[n++] = block;
+        QLIST_REMOVE(block, next);
+    }
+    qsort(blocks, n, sizeof *blocks, block_compar);
+    while (--n >= 0) {
+        QLIST_INSERT_HEAD(&ram_list.blocks, blocks[n], next);
+    }
+    qemu_free(blocks);
+}
+
 int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
 {
     ram_addr_t addr;
@@ -234,6 +268,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         bytes_transferred = 0;
         last_block = NULL;
         last_offset = 0;
+        sort_ram_list();
 
         /* Make sure all dirty bits are set */
         QLIST_FOREACH(block, &ram_list.blocks, next) {
diff --git a/cpu-common.h b/cpu-common.h
index a543b5d..bb6b137 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -46,6 +46,9 @@ ram_addr_t qemu_ram_alloc(DeviceState *dev, const char *name, ram_addr_t size);
 void qemu_ram_free(ram_addr_t addr);
 /* This should only be used for ram local to a device.  */
 void *qemu_get_ram_ptr(ram_addr_t addr);
+/* Same but slower, to use for migration, where the order of
+ * RAMBlocks must not change. */
+void *qemu_safe_ram_ptr(ram_addr_t addr);
 /* This should not be used by devices.  */
 int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
 ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);
diff --git a/exec.c b/exec.c
index db9ff55..6c8f635 100644
--- a/exec.c
+++ b/exec.c
@@ -2030,10 +2030,10 @@ void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end,
 
     /* we modify the TLB cache so that the dirty bit will be set again
        when accessing the range */
-    start1 = (unsigned long)qemu_get_ram_ptr(start);
+    start1 = (unsigned long)qemu_safe_ram_ptr(start);
     /* Chek that we don't span multiple blocks - this breaks the
        address comparisons below.  */
-    if ((unsigned long)qemu_get_ram_ptr(end - 1) - start1
+    if ((unsigned long)qemu_safe_ram_ptr(end - 1) - start1
             != (end - 1) - start) {
         abort();
     }
@@ -2858,6 +2858,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
     new_block->length = size;
 
     QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
+    fprintf(stderr, "alloc ram %s len 0x%x\n", new_block->idstr, (int)new_block->length);
 
     ram_list.phys_dirty = qemu_realloc(ram_list.phys_dirty,
                                        last_ram_offset() >> TARGET_PAGE_BITS);
@@ -2931,6 +2932,25 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
     return NULL;
 }
 
+/* Return a host pointer to ram allocated with qemu_ram_alloc.
+ * Same as qemu_get_ram_ptr but avoid reordering ramblocks.
+ */
+void *qemu_safe_ram_ptr(ram_addr_t addr)
+{
+    RAMBlock *block;
+
+    QLIST_FOREACH(block, &ram_list.blocks, next) {
+        if (addr - block->offset < block->length) {
+            return block->host + (addr - block->offset);
+        }
+    }
+
+    fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
+    abort();
+
+    return NULL;
+}
+
 int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
 {
     RAMBlock *block;
diff --git a/kvm-all.c b/kvm-all.c
index 37b99c7..cae24bb 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -162,7 +162,7 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
     mem.slot = slot->slot;
     mem.guest_phys_addr = slot->start_addr;
     mem.memory_size = slot->memory_size;
-    mem.userspace_addr = (unsigned long)qemu_get_ram_ptr(slot->phys_offset);
+    mem.userspace_addr = (unsigned long)qemu_safe_ram_ptr(slot->phys_offset);
     mem.flags = slot->flags;
     if (s->migration_log) {
         mem.flags |= KVM_MEM_LOG_DIRTY_PAGES;
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv2 6/6] migration: allow rate > 4g

[Michael S. Tsirkin]

I'd like to disable bandwidth limit or make it very high,
Use int64_t all over to make values >= 4g work.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
---
 buffered_file.c |    9 ++++++---
 hw/hw.h         |    8 ++++----
 migration.c     |    6 ++++--
 savevm.c        |    4 ++--
 4 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 1836e7e..8435a31 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -206,20 +206,23 @@ static int buffered_rate_limit(void *opaque)
     return 0;
 }
 
-static size_t buffered_set_rate_limit(void *opaque, size_t new_rate)
+static int64_t buffered_set_rate_limit(void *opaque, int64_t new_rate)
 {
     QEMUFileBuffered *s = opaque;
-
     if (s->has_error)
         goto out;
 
+    if (new_rate > SIZE_MAX) {
+        new_rate = SIZE_MAX;
+    }
+
     s->xfer_limit = new_rate / 10;
     
 out:
     return s->xfer_limit;
 }
 
-static size_t buffered_get_rate_limit(void *opaque)
+static int64_t buffered_get_rate_limit(void *opaque)
 {
     QEMUFileBuffered *s = opaque;
   
diff --git a/hw/hw.h b/hw/hw.h
index 9d2cfc2..77bfb58 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -39,8 +39,8 @@ typedef int (QEMUFileRateLimit)(void *opaque);
  * the new actual bandwidth. It should be new_rate if everything goes ok, and
  * the old rate otherwise
  */
-typedef size_t (QEMUFileSetRateLimit)(void *opaque, size_t new_rate);
-typedef size_t (QEMUFileGetRateLimit)(void *opaque);
+typedef int64_t (QEMUFileSetRateLimit)(void *opaque, int64_t new_rate);
+typedef int64_t (QEMUFileGetRateLimit)(void *opaque);
 
 QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer,
                          QEMUFileGetBufferFunc *get_buffer,
@@ -83,8 +83,8 @@ unsigned int qemu_get_be16(QEMUFile *f);
 unsigned int qemu_get_be32(QEMUFile *f);
 uint64_t qemu_get_be64(QEMUFile *f);
 int qemu_file_rate_limit(QEMUFile *f);
-size_t qemu_file_set_rate_limit(QEMUFile *f, size_t new_rate);
-size_t qemu_file_get_rate_limit(QEMUFile *f);
+int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
+int64_t qemu_file_get_rate_limit(QEMUFile *f);
 int qemu_file_has_error(QEMUFile *f);
 void qemu_file_set_error(QEMUFile *f);
 
diff --git a/migration.c b/migration.c
index 15f7f35..e5ba51c 100644
--- a/migration.c
+++ b/migration.c
@@ -32,7 +32,7 @@
 #endif
 
 /* Migration speed throttling */
-static uint32_t max_throttle = (32 << 20);
+static int64_t max_throttle = (32 << 20);
 
 static MigrationState *current_migration;
 
@@ -136,7 +136,9 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
     FdMigrationState *s;
 
     d = qdict_get_int(qdict, "value");
-    d = MAX(0, MIN(UINT32_MAX, d));
+    if (d < 0) {
+        d = 0;
+    }
     max_throttle = d;
 
     s = migrate_to_fms(current_migration);
diff --git a/savevm.c b/savevm.c
index 49e78a5..90aa237 100644
--- a/savevm.c
+++ b/savevm.c
@@ -611,7 +611,7 @@ int qemu_file_rate_limit(QEMUFile *f)
     return 0;
 }
 
-size_t qemu_file_get_rate_limit(QEMUFile *f)
+int64_t qemu_file_get_rate_limit(QEMUFile *f)
 {
     if (f->get_rate_limit)
         return f->get_rate_limit(f->opaque);
@@ -619,7 +619,7 @@ size_t qemu_file_get_rate_limit(QEMUFile *f)
     return 0;
 }
 
-size_t qemu_file_set_rate_limit(QEMUFile *f, size_t new_rate)
+int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate)
 {
     /* any failed or completed migration keeps its state to allow probing of
      * migration data, but has no associated file anymore */
-- 
1.7.3.2.91.g446ac

[Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Michael S. Tsirkin]

Avoid sending out packets, and modifying
device state, when VM is stopped.
Add assert statements to verify this does not happen.

Avoid scheduling bh when vhost-net is started.

Stop bh when driver disabled bus mastering
(we must not access memory after this).

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

Respinning just this one patch:
virtio-net: stop/start bh on vm start/stop

it turns out under kvm vcpu can be running after vm stop
notifier callbacks are done. And it makes sense: vcpu is
just another device, so we should not
depend on the order of vmstate notifiers.
The state callback is thus a request for device to avoid changing state
of other devices, not a guarantee that other devices
will not send requests to it.

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 43a2b3d..260433f 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -99,7 +99,13 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
     }
 }
 
-static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
+bool virtio_net_started(VirtIONet *n, uint8_t status)
+{
+    return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+        (n->status & VIRTIO_NET_S_LINK_UP) && n->vm_running;
+}
+
+static void virtio_net_vhost_status(struct VirtIODevice *vdev, uint8_t status)
 {
     VirtIONet *n = to_virtio_net(vdev);
     if (!n->nic->nc.peer) {
@@ -112,9 +118,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
     if (!tap_get_vhost_net(n->nic->nc.peer)) {
         return;
     }
-    if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
-                               (n->status & VIRTIO_NET_S_LINK_UP) &&
-                               n->vm_running)) {
+    if (!!n->vhost_started == virtio_net_started(n, status)) {
         return;
     }
     if (!n->vhost_started) {
@@ -131,6 +135,30 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
     }
 }
 
+static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
+{
+    virtio_net_vhost_status(vdev, status);
+
+    if (!n->tx_waiting) {
+        return;
+    }
+
+    if (virtio_net_started(n, status) && !n->vhost_started) {
+        if (n->tx_timer) {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+        } else {
+            qemu_bh_schedule(n->tx_bh);
+        }
+    } else {
+        if (n->tx_timer) {
+            qemu_del_timer(n->tx_timer);
+        } else {
+            qemu_bh_cancel(n->tx_bh);
+        }
+    }
+}
+
 static void virtio_net_set_link_status(VLANClientState *nc)
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
@@ -675,11 +703,13 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 {
     VirtQueueElement elem;
     int32_t num_packets = 0;
 
     if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
         return num_packets;
     }
 
+    assert(n->vm_running);
+
     if (n->async_tx.elem.out_num) {
         virtio_queue_set_notification(n->tx_vq, 0);
         return num_packets;
@@ -738,6 +767,12 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIONet *n = to_virtio_net(vdev);
 
+    /* This happens when device was stopped but VCPU wasn't. */
+    if (!n->vm_running) {
+        n->tx_waiting = 1;
+        return;
+    }
+
     if (n->tx_waiting) {
         virtio_queue_set_notification(vq, 1);
         qemu_del_timer(n->tx_timer);
@@ -758,14 +793,19 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
     if (unlikely(n->tx_waiting)) {
         return;
     }
+    n->tx_waiting = 1;
+    /* This happens when device was stopped but VCPU wasn't. */
+    if (!n->vm_running) {
+        return;
+    }
     virtio_queue_set_notification(vq, 0);
     qemu_bh_schedule(n->tx_bh);
-    n->tx_waiting = 1;
 }
 
 static void virtio_net_tx_timer(void *opaque)
 {
     VirtIONet *n = opaque;
+    assert(n->vm_running);
 
     n->tx_waiting = 0;
 
@@ -782,6 +822,8 @@ static void virtio_net_tx_bh(void *opaque)
     VirtIONet *n = opaque;
     int32_t ret;
 
+    assert(n->vm_running);
+
     n->tx_waiting = 0;
 
     /* Just in case the driver is not ready on more */
@@ -926,15 +968,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
     n->mac_table.first_multi = i;
-
-    if (n->tx_waiting) {
-        if (n->tx_timer) {
-            qemu_mod_timer(n->tx_timer,
-                           qemu_get_clock(vm_clock) + n->tx_timeout);
-        } else {
-            qemu_bh_schedule(n->tx_bh);
-        }
-    }
     return 0;
 }
 

Re: [Qemu-devel] [PATCHv2 2/6] cpus: flush all requests on each vm stop

[Marcelo Tosatti]

On Wed, Nov 24, 2010 at 05:52:58PM +0200, Michael S. Tsirkin wrote:
> Make sure disk is in consistent state.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Jason Wang <jasowang@redhat.com>
> ---
>  cpus.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 91a0fb1..d421a96 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
>          cpu_disable_ticks();
>          vm_running = 0;
>          pause_all_vcpus();
> +        qemu_aio_flush();
> +        bdrv_flush_all();

Can you move these after vm_state_notify? qemu-kvm stops vcpus there.

>          vm_state_notify(0, reason);
>          monitor_protocol_event(QEVENT_STOP, NULL);
>      }

Re: [Qemu-devel] [PATCHv2 2/6] cpus: flush all requests on each vm stop

[Michael S. Tsirkin]

On Tue, Nov 30, 2010 at 10:45:40AM -0200, Marcelo Tosatti wrote:
> On Wed, Nov 24, 2010 at 05:52:58PM +0200, Michael S. Tsirkin wrote:
> > Make sure disk is in consistent state.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Tested-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  cpus.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 91a0fb1..d421a96 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
> >          cpu_disable_ticks();
> >          vm_running = 0;
> >          pause_all_vcpus();
> > +        qemu_aio_flush();
> > +        bdrv_flush_all();
> 
> Can you move these after vm_state_notify? qemu-kvm stops vcpus there.

Right. Other devices might do the same. Will do.

> >          vm_state_notify(0, reason);
> >          monitor_protocol_event(QEVENT_STOP, NULL);
> >      }
> 
> 

Re: [Qemu-devel] [PATCHv2 2/6] cpus: flush all requests on each vm stop

[Michael S. Tsirkin]

On Tue, Nov 30, 2010 at 10:45:40AM -0200, Marcelo Tosatti wrote:
> On Wed, Nov 24, 2010 at 05:52:58PM +0200, Michael S. Tsirkin wrote:
> > Make sure disk is in consistent state.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Tested-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  cpus.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 91a0fb1..d421a96 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
> >          cpu_disable_ticks();
> >          vm_running = 0;
> >          pause_all_vcpus();
> > +        qemu_aio_flush();
> > +        bdrv_flush_all();
> 
> Can you move these after vm_state_notify? qemu-kvm stops vcpus there.
> 
> >          vm_state_notify(0, reason);
> >          monitor_protocol_event(QEVENT_STOP, NULL);
> >      }

Like this:

cpus: flush all requests on each vm stop

Make sure disk is in consistent state.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>

---

diff --git a/cpus.c b/cpus.c
index 91a0fb1..d421a96 100644
--- a/cpus.c
+++ b/cpus.c
@@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
         cpu_disable_ticks();
         vm_running = 0;
         pause_all_vcpus();
+        qemu_aio_flush();
+        bdrv_flush_all();
         vm_state_notify(0, reason);
         monitor_protocol_event(QEVENT_STOP, NULL);
     }

Re: [Qemu-devel] [PATCHv2 2/6] cpus: flush all requests on each vm stop

[Marcelo Tosatti]

On Tue, Nov 30, 2010 at 03:34:29PM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 30, 2010 at 10:45:40AM -0200, Marcelo Tosatti wrote:
> > On Wed, Nov 24, 2010 at 05:52:58PM +0200, Michael S. Tsirkin wrote:
> > > Make sure disk is in consistent state.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Tested-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  cpus.c |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/cpus.c b/cpus.c
> > > index 91a0fb1..d421a96 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
> > >          cpu_disable_ticks();
> > >          vm_running = 0;
> > >          pause_all_vcpus();
> > > +        qemu_aio_flush();
> > > +        bdrv_flush_all();
> > 
> > Can you move these after vm_state_notify? qemu-kvm stops vcpus there.
> > 
> > >          vm_state_notify(0, reason);
> > >          monitor_protocol_event(QEVENT_STOP, NULL);
> > >      }
> 
> Like this:
> 
> cpus: flush all requests on each vm stop
> 
> Make sure disk is in consistent state.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Jason Wang <jasowang@redhat.com>
> 
> ---
> 
> diff --git a/cpus.c b/cpus.c
> index 91a0fb1..d421a96 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
>          cpu_disable_ticks();
>          vm_running = 0;
>          pause_all_vcpus();
> +        qemu_aio_flush();
> +        bdrv_flush_all();
>          vm_state_notify(0, reason);
>          monitor_protocol_event(QEVENT_STOP, NULL);
>      }

No, after vm_state_notify.

Re: [Qemu-devel] [PATCHv2 2/6] cpus: flush all requests on each vm stop

[Michael S. Tsirkin]

On Tue, Nov 30, 2010 at 11:46:48AM -0200, Marcelo Tosatti wrote:
> On Tue, Nov 30, 2010 at 03:34:29PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 30, 2010 at 10:45:40AM -0200, Marcelo Tosatti wrote:
> > > On Wed, Nov 24, 2010 at 05:52:58PM +0200, Michael S. Tsirkin wrote:
> > > > Make sure disk is in consistent state.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Tested-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  cpus.c |    2 ++
> > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/cpus.c b/cpus.c
> > > > index 91a0fb1..d421a96 100644
> > > > --- a/cpus.c
> > > > +++ b/cpus.c
> > > > @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
> > > >          cpu_disable_ticks();
> > > >          vm_running = 0;
> > > >          pause_all_vcpus();
> > > > +        qemu_aio_flush();
> > > > +        bdrv_flush_all();
> > > 
> > > Can you move these after vm_state_notify? qemu-kvm stops vcpus there.
> > > 
> > > >          vm_state_notify(0, reason);
> > > >          monitor_protocol_event(QEVENT_STOP, NULL);
> > > >      }
> > 
> > Like this:
> > 
> > cpus: flush all requests on each vm stop
> > 
> > Make sure disk is in consistent state.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Tested-by: Jason Wang <jasowang@redhat.com>
> > 
> > ---
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 91a0fb1..d421a96 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
> >          cpu_disable_ticks();
> >          vm_running = 0;
> >          pause_all_vcpus();
> > +        qemu_aio_flush();
> > +        bdrv_flush_all();
> >          vm_state_notify(0, reason);
> >          monitor_protocol_event(QEVENT_STOP, NULL);
> >      }
> 
> No, after vm_state_notify.


Like this then:


    cpus: flush all requests on each vm stop
    
    Flush all requests once we have stopped all
    cpus and devices.
    Make sure disk is in consistent state.
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
    Tested-by: Jason Wang <jasowang@redhat.com>

diff --git a/cpus.c b/cpus.c
index 91a0fb1..0309189 100644
--- a/cpus.c
+++ b/cpus.c
@@ -111,6 +111,8 @@ static void do_vm_stop(int reason)
         vm_running = 0;
         pause_all_vcpus();
         vm_state_notify(0, reason);
+        qemu_aio_flush();
+        bdrv_flush_all();
         monitor_protocol_event(QEVENT_STOP, NULL);
     }
 }

[Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Jason Wang]

Michael S. Tsirkin writes:
 > Avoid sending out packets, and modifying
 > device state, when VM is stopped.
 > Add assert statements to verify this does not happen.
 > 
 > Avoid scheduling bh when vhost-net is started.
 > 
 > Stop bh when driver disabled bus mastering
 > (we must not access memory after this).
 > 
 > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
 > 

There's no need to disable it bh we call qemu_aio_flush() after
vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
stop its timer in vm state change handler, not only for virtio-net?

 > ---
 > 
 > Respinning just this one patch:
 > virtio-net: stop/start bh on vm start/stop
 > 
 > it turns out under kvm vcpu can be running after vm stop
 > notifier callbacks are done. And it makes sense: vcpu is
 > just another device, so we should not
 > depend on the order of vmstate notifiers.
 > The state callback is thus a request for device to avoid changing state
 > of other devices, not a guarantee that other devices
 > will not send requests to it.
 > 
 > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
 > index 43a2b3d..260433f 100644
 > --- a/hw/virtio-net.c
 > +++ b/hw/virtio-net.c
 > @@ -99,7 +99,13 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
 >      }
 >  }
 >  
 > -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 > +bool virtio_net_started(VirtIONet *n, uint8_t status)
 > +{
 > +    return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
 > +        (n->status & VIRTIO_NET_S_LINK_UP) && n->vm_running;
 > +}
 > +
 > +static void virtio_net_vhost_status(struct VirtIODevice *vdev, uint8_t status)
 >  {
 >      VirtIONet *n = to_virtio_net(vdev);
 >      if (!n->nic->nc.peer) {
 > @@ -112,9 +118,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 >      if (!tap_get_vhost_net(n->nic->nc.peer)) {
 >          return;
 >      }
 > -    if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
 > -                               (n->status & VIRTIO_NET_S_LINK_UP) &&
 > -                               n->vm_running)) {
 > +    if (!!n->vhost_started == virtio_net_started(n, status)) {
 >          return;
 >      }
 >      if (!n->vhost_started) {
 > @@ -131,6 +135,30 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 >      }
 >  }
 >  
 > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 > +{
 > +    virtio_net_vhost_status(vdev, status);
 > +
 > +    if (!n->tx_waiting) {
 > +        return;
 > +    }
 > +
 > +    if (virtio_net_started(n, status) && !n->vhost_started) {
 > +        if (n->tx_timer) {
 > +            qemu_mod_timer(n->tx_timer,
 > +                           qemu_get_clock(vm_clock) + n->tx_timeout);
 > +        } else {
 > +            qemu_bh_schedule(n->tx_bh);
 > +        }
 > +    } else {
 > +        if (n->tx_timer) {
 > +            qemu_del_timer(n->tx_timer);
 > +        } else {
 > +            qemu_bh_cancel(n->tx_bh);
 > +        }
 > +    }
 > +}
 > +
 >  static void virtio_net_set_link_status(VLANClientState *nc)
 >  {
 >      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
 > @@ -675,11 +703,13 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 >  {
 >      VirtQueueElement elem;
 >      int32_t num_packets = 0;
 >  
 >      if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
 >          return num_packets;
 >      }
 >  
 > +    assert(n->vm_running);
 > +
 >      if (n->async_tx.elem.out_num) {
 >          virtio_queue_set_notification(n->tx_vq, 0);
 >          return num_packets;
 > @@ -738,6 +767,12 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
 >  {
 >      VirtIONet *n = to_virtio_net(vdev);
 >  
 > +    /* This happens when device was stopped but VCPU wasn't. */
 > +    if (!n->vm_running) {
 > +        n->tx_waiting = 1;
 > +        return;
 > +    }
 > +
 >      if (n->tx_waiting) {
 >          virtio_queue_set_notification(vq, 1);
 >          qemu_del_timer(n->tx_timer);
 > @@ -758,14 +793,19 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
 >      if (unlikely(n->tx_waiting)) {
 >          return;
 >      }
 > +    n->tx_waiting = 1;
 > +    /* This happens when device was stopped but VCPU wasn't. */
 > +    if (!n->vm_running) {
 > +        return;
 > +    }
 >      virtio_queue_set_notification(vq, 0);
 >      qemu_bh_schedule(n->tx_bh);
 > -    n->tx_waiting = 1;
 >  }
 >  
 >  static void virtio_net_tx_timer(void *opaque)
 >  {
 >      VirtIONet *n = opaque;
 > +    assert(n->vm_running);
 >  
 >      n->tx_waiting = 0;
 >  
 > @@ -782,6 +822,8 @@ static void virtio_net_tx_bh(void *opaque)
 >      VirtIONet *n = opaque;
 >      int32_t ret;
 >  
 > +    assert(n->vm_running);
 > +
 >      n->tx_waiting = 0;
 >  
 >      /* Just in case the driver is not ready on more */
 > @@ -926,15 +968,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 >          }
 >      }
 >      n->mac_table.first_multi = i;
 > -
 > -    if (n->tx_waiting) {
 > -        if (n->tx_timer) {
 > -            qemu_mod_timer(n->tx_timer,
 > -                           qemu_get_clock(vm_clock) + n->tx_timeout);
 > -        } else {
 > -            qemu_bh_schedule(n->tx_bh);
 > -        }
 > -    }
 >      return 0;
 >  }
 >  
 > 

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Michael S. Tsirkin]

On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
>  > Avoid sending out packets, and modifying
>  > device state, when VM is stopped.
>  > Add assert statements to verify this does not happen.
>  > 
>  > Avoid scheduling bh when vhost-net is started.
>  > 
>  > Stop bh when driver disabled bus mastering
>  > (we must not access memory after this).
>  > 
>  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  > 
> 
> There's no need to disable it bh we call qemu_aio_flush() after
> vm_state_notify() in do_vm_stop().

Yes, but bh can resubmit itself, so flush is not enough.
Note that vm stop is not the only reason to avoid
running bh: when vhost-net is enabled we should avoid it too.
Makes sense?

> And for timer, looks like every device should
> stop its timer in vm state change handler, not only for virtio-net?

Yes. We'll have to fix these things one at a time, I don't
have a general solution.

>  > ---
>  > 
>  > Respinning just this one patch:
>  > virtio-net: stop/start bh on vm start/stop
>  > 
>  > it turns out under kvm vcpu can be running after vm stop
>  > notifier callbacks are done. And it makes sense: vcpu is
>  > just another device, so we should not
>  > depend on the order of vmstate notifiers.
>  > The state callback is thus a request for device to avoid changing state
>  > of other devices, not a guarantee that other devices
>  > will not send requests to it.
>  > 
>  > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>  > index 43a2b3d..260433f 100644
>  > --- a/hw/virtio-net.c
>  > +++ b/hw/virtio-net.c
>  > @@ -99,7 +99,13 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>  >      }
>  >  }
>  >  
>  > -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  > +bool virtio_net_started(VirtIONet *n, uint8_t status)
>  > +{
>  > +    return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>  > +        (n->status & VIRTIO_NET_S_LINK_UP) && n->vm_running;
>  > +}
>  > +
>  > +static void virtio_net_vhost_status(struct VirtIODevice *vdev, uint8_t status)
>  >  {
>  >      VirtIONet *n = to_virtio_net(vdev);
>  >      if (!n->nic->nc.peer) {
>  > @@ -112,9 +118,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  >      if (!tap_get_vhost_net(n->nic->nc.peer)) {
>  >          return;
>  >      }
>  > -    if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>  > -                               (n->status & VIRTIO_NET_S_LINK_UP) &&
>  > -                               n->vm_running)) {
>  > +    if (!!n->vhost_started == virtio_net_started(n, status)) {
>  >          return;
>  >      }
>  >      if (!n->vhost_started) {
>  > @@ -131,6 +135,30 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  >      }
>  >  }
>  >  
>  > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  > +{
>  > +    virtio_net_vhost_status(vdev, status);
>  > +
>  > +    if (!n->tx_waiting) {
>  > +        return;
>  > +    }
>  > +
>  > +    if (virtio_net_started(n, status) && !n->vhost_started) {
>  > +        if (n->tx_timer) {
>  > +            qemu_mod_timer(n->tx_timer,
>  > +                           qemu_get_clock(vm_clock) + n->tx_timeout);
>  > +        } else {
>  > +            qemu_bh_schedule(n->tx_bh);
>  > +        }
>  > +    } else {
>  > +        if (n->tx_timer) {
>  > +            qemu_del_timer(n->tx_timer);
>  > +        } else {
>  > +            qemu_bh_cancel(n->tx_bh);
>  > +        }
>  > +    }
>  > +}
>  > +
>  >  static void virtio_net_set_link_status(VLANClientState *nc)
>  >  {
>  >      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
>  > @@ -675,11 +703,13 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
>  >  {
>  >      VirtQueueElement elem;
>  >      int32_t num_packets = 0;
>  >  
>  >      if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>  >          return num_packets;
>  >      }
>  >  
>  > +    assert(n->vm_running);
>  > +
>  >      if (n->async_tx.elem.out_num) {
>  >          virtio_queue_set_notification(n->tx_vq, 0);
>  >          return num_packets;
>  > @@ -738,6 +767,12 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
>  >  {
>  >      VirtIONet *n = to_virtio_net(vdev);
>  >  
>  > +    /* This happens when device was stopped but VCPU wasn't. */
>  > +    if (!n->vm_running) {
>  > +        n->tx_waiting = 1;
>  > +        return;
>  > +    }
>  > +
>  >      if (n->tx_waiting) {
>  >          virtio_queue_set_notification(vq, 1);
>  >          qemu_del_timer(n->tx_timer);
>  > @@ -758,14 +793,19 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
>  >      if (unlikely(n->tx_waiting)) {
>  >          return;
>  >      }
>  > +    n->tx_waiting = 1;
>  > +    /* This happens when device was stopped but VCPU wasn't. */
>  > +    if (!n->vm_running) {
>  > +        return;
>  > +    }
>  >      virtio_queue_set_notification(vq, 0);
>  >      qemu_bh_schedule(n->tx_bh);
>  > -    n->tx_waiting = 1;
>  >  }
>  >  
>  >  static void virtio_net_tx_timer(void *opaque)
>  >  {
>  >      VirtIONet *n = opaque;
>  > +    assert(n->vm_running);
>  >  
>  >      n->tx_waiting = 0;
>  >  
>  > @@ -782,6 +822,8 @@ static void virtio_net_tx_bh(void *opaque)
>  >      VirtIONet *n = opaque;
>  >      int32_t ret;
>  >  
>  > +    assert(n->vm_running);
>  > +
>  >      n->tx_waiting = 0;
>  >  
>  >      /* Just in case the driver is not ready on more */
>  > @@ -926,15 +968,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>  >          }
>  >      }
>  >      n->mac_table.first_multi = i;
>  > -
>  > -    if (n->tx_waiting) {
>  > -        if (n->tx_timer) {
>  > -            qemu_mod_timer(n->tx_timer,
>  > -                           qemu_get_clock(vm_clock) + n->tx_timeout);
>  > -        } else {
>  > -            qemu_bh_schedule(n->tx_bh);
>  > -        }
>  > -    }
>  >      return 0;
>  >  }
>  >  
>  > 

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Michael S. Tsirkin]

On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
>  > Avoid sending out packets, and modifying
>  > device state, when VM is stopped.
>  > Add assert statements to verify this does not happen.
>  > 
>  > Avoid scheduling bh when vhost-net is started.
>  > 
>  > Stop bh when driver disabled bus mastering
>  > (we must not access memory after this).
>  > 
>  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  > 
> 
> There's no need to disable it bh we call qemu_aio_flush() after
> vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
> stop its timer in vm state change handler, not only for virtio-net?

BTW I fixed some typos. Here a fixed version.
Jason, could you review/test please?

virtio-net: stop/start bh when appropriate

Avoid sending out packets, and modifying
memory, when VM is stopped.
Add assert statements to verify this does not happen.

Avoid scheduling bh when vhost-net is started.

Stop bh when driver disabled bus mastering
(we must not access memory after this).

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 43a2b3d..5881961 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -99,9 +99,14 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
     }
 }
 
-static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
+static bool virtio_net_started(VirtIONet *n, uint8_t status)
+{
+    return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+        (n->status & VIRTIO_NET_S_LINK_UP) && n->vm_running;
+}
+
+static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
 {
-    VirtIONet *n = to_virtio_net(vdev);
     if (!n->nic->nc.peer) {
         return;
     }
@@ -112,9 +117,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
     if (!tap_get_vhost_net(n->nic->nc.peer)) {
         return;
     }
-    if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
-                               (n->status & VIRTIO_NET_S_LINK_UP) &&
-                               n->vm_running)) {
+    if (!!n->vhost_started == virtio_net_started(n, status)) {
         return;
     }
     if (!n->vhost_started) {
@@ -131,6 +134,32 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
     }
 }
 
+static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
+{
+    VirtIONet *n = to_virtio_net(vdev);
+
+    virtio_net_vhost_status(n, status);
+
+    if (!n->tx_waiting) {
+        return;
+    }
+
+    if (virtio_net_started(n, status) && !n->vhost_started) {
+        if (n->tx_timer) {
+            qemu_mod_timer(n->tx_timer,
+                           qemu_get_clock(vm_clock) + n->tx_timeout);
+        } else {
+            qemu_bh_schedule(n->tx_bh);
+        }
+    } else {
+        if (n->tx_timer) {
+            qemu_del_timer(n->tx_timer);
+        } else {
+            qemu_bh_cancel(n->tx_bh);
+        }
+    }
+}
+
 static void virtio_net_set_link_status(VLANClientState *nc)
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
@@ -675,11 +704,12 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 {
     VirtQueueElement elem;
     int32_t num_packets = 0;
-
     if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
         return num_packets;
     }
 
+    assert(n->vm_running);
+
     if (n->async_tx.elem.out_num) {
         virtio_queue_set_notification(n->tx_vq, 0);
         return num_packets;
@@ -738,6 +768,12 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIONet *n = to_virtio_net(vdev);
 
+    /* This happens when device was stopped but VCPU wasn't. */
+    if (!n->vm_running) {
+        n->tx_waiting = 1;
+        return;
+    }
+
     if (n->tx_waiting) {
         virtio_queue_set_notification(vq, 1);
         qemu_del_timer(n->tx_timer);
@@ -758,14 +794,19 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
     if (unlikely(n->tx_waiting)) {
         return;
     }
+    n->tx_waiting = 1;
+    /* This happens when device was stopped but VCPU wasn't. */
+    if (!n->vm_running) {
+        return;
+    }
     virtio_queue_set_notification(vq, 0);
     qemu_bh_schedule(n->tx_bh);
-    n->tx_waiting = 1;
 }
 
 static void virtio_net_tx_timer(void *opaque)
 {
     VirtIONet *n = opaque;
+    assert(n->vm_running);
 
     n->tx_waiting = 0;
 
@@ -782,6 +823,8 @@ static void virtio_net_tx_bh(void *opaque)
     VirtIONet *n = opaque;
     int32_t ret;
 
+    assert(n->vm_running);
+
     n->tx_waiting = 0;
 
     /* Just in case the driver is not ready on more */
@@ -926,15 +969,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
     n->mac_table.first_multi = i;
-
-    if (n->tx_waiting) {
-        if (n->tx_timer) {
-            qemu_mod_timer(n->tx_timer,
-                           qemu_get_clock(vm_clock) + n->tx_timeout);
-        } else {
-            qemu_bh_schedule(n->tx_bh);
-        }
-    }
     return 0;
 }
 

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Jason Wang]

Michael S. Tsirkin writes:
 > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
 > > Michael S. Tsirkin writes:
 > >  > Avoid sending out packets, and modifying
 > >  > device state, when VM is stopped.
 > >  > Add assert statements to verify this does not happen.
 > >  > 
 > >  > Avoid scheduling bh when vhost-net is started.
 > >  > 
 > >  > Stop bh when driver disabled bus mastering
 > >  > (we must not access memory after this).
 > >  > 
 > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
 > >  > 
 > > 
 > > There's no need to disable it bh we call qemu_aio_flush() after
 > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
 > > stop its timer in vm state change handler, not only for virtio-net?
 > 
 > BTW I fixed some typos. Here a fixed version.
 > Jason, could you review/test please?
 > 

Sure.

 > virtio-net: stop/start bh when appropriate
 > 
 > Avoid sending out packets, and modifying
 > memory, when VM is stopped.
 > Add assert statements to verify this does not happen.
 > 
 > Avoid scheduling bh when vhost-net is started.
 > 
 > Stop bh when driver disabled bus mastering
 > (we must not access memory after this).
 > 
 > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
 > Tested-by: Jason Wang <jasowang@redhat.com>
 > 
 > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
 > index 43a2b3d..5881961 100644
 > --- a/hw/virtio-net.c
 > +++ b/hw/virtio-net.c
 > @@ -99,9 +99,14 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
 >      }
 >  }
 >  
 > -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 > +static bool virtio_net_started(VirtIONet *n, uint8_t status)
 > +{
 > +    return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
 > +        (n->status & VIRTIO_NET_S_LINK_UP) && n->vm_running;
 > +}
 > +
 > +static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
 >  {
 > -    VirtIONet *n = to_virtio_net(vdev);
 >      if (!n->nic->nc.peer) {
 >          return;
 >      }
 > @@ -112,9 +117,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 >      if (!tap_get_vhost_net(n->nic->nc.peer)) {
 >          return;
 >      }
 > -    if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
 > -                               (n->status & VIRTIO_NET_S_LINK_UP) &&
 > -                               n->vm_running)) {
 > +    if (!!n->vhost_started == virtio_net_started(n, status)) {
 >          return;
 >      }
 >      if (!n->vhost_started) {
 > @@ -131,6 +134,32 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 >      }
 >  }
 >  
 > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
 > +{
 > +    VirtIONet *n = to_virtio_net(vdev);
 > +
 > +    virtio_net_vhost_status(n, status);
 > +
 > +    if (!n->tx_waiting) {
 > +        return;
 > +    }
 > +
 > +    if (virtio_net_started(n, status) && !n->vhost_started) {
 > +        if (n->tx_timer) {
 > +            qemu_mod_timer(n->tx_timer,
 > +                           qemu_get_clock(vm_clock) + n->tx_timeout);
 > +        } else {
 > +            qemu_bh_schedule(n->tx_bh);
 > +        }
 > +    } else {
 > +        if (n->tx_timer) {
 > +            qemu_del_timer(n->tx_timer);
 > +        } else {
 > +            qemu_bh_cancel(n->tx_bh);
 > +        }
 > +    }
 > +}
 > +
 >  static void virtio_net_set_link_status(VLANClientState *nc)
 >  {
 >      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
 > @@ -675,11 +704,12 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, VirtQueue *vq)
 >  {
 >      VirtQueueElement elem;
 >      int32_t num_packets = 0;
 > -
 >      if (!(n->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
 >          return num_packets;
 >      }
 >  
 > +    assert(n->vm_running);
 > +
 >      if (n->async_tx.elem.out_num) {
 >          virtio_queue_set_notification(n->tx_vq, 0);
 >          return num_packets;
 > @@ -738,6 +768,12 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
 >  {
 >      VirtIONet *n = to_virtio_net(vdev);
 >  
 > +    /* This happens when device was stopped but VCPU wasn't. */
 > +    if (!n->vm_running) {
 > +        n->tx_waiting = 1;
 > +        return;
 > +    }
 > +
 >      if (n->tx_waiting) {
 >          virtio_queue_set_notification(vq, 1);
 >          qemu_del_timer(n->tx_timer);
 > @@ -758,14 +794,19 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, VirtQueue *vq)
 >      if (unlikely(n->tx_waiting)) {
 >          return;
 >      }
 > +    n->tx_waiting = 1;
 > +    /* This happens when device was stopped but VCPU wasn't. */
 > +    if (!n->vm_running) {
 > +        return;
 > +    }
 >      virtio_queue_set_notification(vq, 0);
 >      qemu_bh_schedule(n->tx_bh);
 > -    n->tx_waiting = 1;
 >  }
 >  
 >  static void virtio_net_tx_timer(void *opaque)
 >  {
 >      VirtIONet *n = opaque;
 > +    assert(n->vm_running);
 >  
 >      n->tx_waiting = 0;
 >  
 > @@ -782,6 +823,8 @@ static void virtio_net_tx_bh(void *opaque)
 >      VirtIONet *n = opaque;
 >      int32_t ret;
 >  
 > +    assert(n->vm_running);
 > +
 >      n->tx_waiting = 0;
 >  
 >      /* Just in case the driver is not ready on more */
 > @@ -926,15 +969,6 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 >          }
 >      }
 >      n->mac_table.first_multi = i;
 > -
 > -    if (n->tx_waiting) {
 > -        if (n->tx_timer) {
 > -            qemu_mod_timer(n->tx_timer,
 > -                           qemu_get_clock(vm_clock) + n->tx_timeout);
 > -        } else {
 > -            qemu_bh_schedule(n->tx_bh);
 > -        }
 > -    }
 >      return 0;
 >  }
 >  

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Jason Wang]

Michael S. Tsirkin writes:
 > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
 > > Michael S. Tsirkin writes:
 > >  > Avoid sending out packets, and modifying
 > >  > device state, when VM is stopped.
 > >  > Add assert statements to verify this does not happen.
 > >  > 
 > >  > Avoid scheduling bh when vhost-net is started.
 > >  > 
 > >  > Stop bh when driver disabled bus mastering
 > >  > (we must not access memory after this).
 > >  > 
 > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
 > >  > 
 > > 
 > > There's no need to disable it bh we call qemu_aio_flush() after
 > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
 > > stop its timer in vm state change handler, not only for virtio-net?
 > 
 > BTW I fixed some typos. Here a fixed version.
 > Jason, could you review/test please?
 > 

Have done the test, it's more stable than before but still get small deltas in
cpu section. I didn't find any interesting difference by checking the
CPUX86State in the dest in kvm_arch_load_regs(), any thought on this?

BTW, looks like the error_code was missed in saving the cpu state:

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 35a1a51..145bb38 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -687,7 +687,7 @@ typedef struct CPUX86State {
     uint64_t pat;
 
     /* exception/interrupt handling */
-    int error_code;
+    uint32_t error_code;
     int exception_is_int;
     target_ulong exception_next_eip;
     target_ulong dr[8]; /* debug registers */
@@ -935,7 +935,7 @@ CPUState *pc_new_cpu(const char *cpu_model);
 #define cpu_list_id x86_cpu_list
 #define cpudef_setup	x86_cpudef_setup
 
-#define CPU_SAVE_VERSION 12
+#define CPU_SAVE_VERSION 13
 
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 4398801..fa231d8 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -474,6 +474,8 @@ static const VMStateDescription vmstate_cpu = {
         VMSTATE_UINT64_V(xcr0, CPUState, 12),
         VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
         VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
+
+        VMSTATE_UINT32_V(error_code, CPUState, 13),
         VMSTATE_END_OF_LIST()
         /* The above list is not sorted /wrt version numbers, watch out! */
     }

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Michael S. Tsirkin]

On Thu, Dec 02, 2010 at 08:56:30PM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
>  > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
>  > > Michael S. Tsirkin writes:
>  > >  > Avoid sending out packets, and modifying
>  > >  > device state, when VM is stopped.
>  > >  > Add assert statements to verify this does not happen.
>  > >  > 
>  > >  > Avoid scheduling bh when vhost-net is started.
>  > >  > 
>  > >  > Stop bh when driver disabled bus mastering
>  > >  > (we must not access memory after this).
>  > >  > 
>  > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  > >  > 
>  > > 
>  > > There's no need to disable it bh we call qemu_aio_flush() after
>  > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
>  > > stop its timer in vm state change handler, not only for virtio-net?
>  > 
>  > BTW I fixed some typos. Here a fixed version.
>  > Jason, could you review/test please?
>  > 
> 
> Have done the test, it's more stable than before but still get small deltas in
> cpu section. I didn't find any interesting difference by checking the
> CPUX86State in the dest in kvm_arch_load_regs(), any thought on this?

So which offsets are different?

> BTW, looks like the error_code was missed in saving the cpu state:

Post this as a separate patch please.

> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 35a1a51..145bb38 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -687,7 +687,7 @@ typedef struct CPUX86State {
>      uint64_t pat;
>  
>      /* exception/interrupt handling */
> -    int error_code;
> +    uint32_t error_code;
>      int exception_is_int;
>      target_ulong exception_next_eip;
>      target_ulong dr[8]; /* debug registers */
> @@ -935,7 +935,7 @@ CPUState *pc_new_cpu(const char *cpu_model);
>  #define cpu_list_id x86_cpu_list
>  #define cpudef_setup	x86_cpudef_setup
>  
> -#define CPU_SAVE_VERSION 12
> +#define CPU_SAVE_VERSION 13
>  
>  /* MMU modes definitions */
>  #define MMU_MODE0_SUFFIX _kernel
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 4398801..fa231d8 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -474,6 +474,8 @@ static const VMStateDescription vmstate_cpu = {
>          VMSTATE_UINT64_V(xcr0, CPUState, 12),
>          VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
>          VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
> +
> +        VMSTATE_UINT32_V(error_code, CPUState, 13),
>          VMSTATE_END_OF_LIST()
>          /* The above list is not sorted /wrt version numbers, watch out! */
>      }

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Michael S. Tsirkin]

On Thu, Dec 02, 2010 at 08:56:30PM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
>  > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
>  > > Michael S. Tsirkin writes:
>  > >  > Avoid sending out packets, and modifying
>  > >  > device state, when VM is stopped.
>  > >  > Add assert statements to verify this does not happen.
>  > >  > 
>  > >  > Avoid scheduling bh when vhost-net is started.
>  > >  > 
>  > >  > Stop bh when driver disabled bus mastering
>  > >  > (we must not access memory after this).
>  > >  > 
>  > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  > >  > 
>  > > 
>  > > There's no need to disable it bh we call qemu_aio_flush() after
>  > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
>  > > stop its timer in vm state change handler, not only for virtio-net?
>  > 
>  > BTW I fixed some typos. Here a fixed version.
>  > Jason, could you review/test please?
>  > 
> 
> Have done the test, it's more stable than before but still get small deltas in
> cpu section.

And just to clarify: no more deltas in the memory section?

> I didn't find any interesting difference by checking the
> CPUX86State in the dest in kvm_arch_load_regs(), any thought on this?
> 
> BTW, looks like the error_code was missed in saving the cpu state:
> 
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 35a1a51..145bb38 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -687,7 +687,7 @@ typedef struct CPUX86State {
>      uint64_t pat;
>  
>      /* exception/interrupt handling */
> -    int error_code;
> +    uint32_t error_code;
>      int exception_is_int;
>      target_ulong exception_next_eip;
>      target_ulong dr[8]; /* debug registers */
> @@ -935,7 +935,7 @@ CPUState *pc_new_cpu(const char *cpu_model);
>  #define cpu_list_id x86_cpu_list
>  #define cpudef_setup	x86_cpudef_setup
>  
> -#define CPU_SAVE_VERSION 12
> +#define CPU_SAVE_VERSION 13
>  
>  /* MMU modes definitions */
>  #define MMU_MODE0_SUFFIX _kernel
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 4398801..fa231d8 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -474,6 +474,8 @@ static const VMStateDescription vmstate_cpu = {
>          VMSTATE_UINT64_V(xcr0, CPUState, 12),
>          VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
>          VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
> +
> +        VMSTATE_UINT32_V(error_code, CPUState, 13),
>          VMSTATE_END_OF_LIST()
>          /* The above list is not sorted /wrt version numbers, watch out! */
>      }

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Jason Wang]

Michael S. Tsirkin writes:
 > On Thu, Dec 02, 2010 at 08:56:30PM +0800, Jason Wang wrote:
 > > Michael S. Tsirkin writes:
 > >  > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
 > >  > > Michael S. Tsirkin writes:
 > >  > >  > Avoid sending out packets, and modifying
 > >  > >  > device state, when VM is stopped.
 > >  > >  > Add assert statements to verify this does not happen.
 > >  > >  > 
 > >  > >  > Avoid scheduling bh when vhost-net is started.
 > >  > >  > 
 > >  > >  > Stop bh when driver disabled bus mastering
 > >  > >  > (we must not access memory after this).
 > >  > >  > 
 > >  > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
 > >  > >  > 
 > >  > > 
 > >  > > There's no need to disable it bh we call qemu_aio_flush() after
 > >  > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
 > >  > > stop its timer in vm state change handler, not only for virtio-net?
 > >  > 
 > >  > BTW I fixed some typos. Here a fixed version.
 > >  > Jason, could you review/test please?
 > >  > 
 > > 
 > > Have done the test, it's more stable than before but still get small deltas in
 > > cpu section.
 > 
 > And just to clarify: no more deltas in the memory section?
 > 

Yes.

And the offset for cpu section is 1161-1165 and sometimes I get deltas for ide
section at offset 295 and 314.

 > > I didn't find any interesting difference by checking the
 > > CPUX86State in the dest in kvm_arch_load_regs(), any thought on this?
 > > 
 > > BTW, looks like the error_code was missed in saving the cpu state:
 > > 
 > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
 > > index 35a1a51..145bb38 100644
 > > --- a/target-i386/cpu.h
 > > +++ b/target-i386/cpu.h
 > > @@ -687,7 +687,7 @@ typedef struct CPUX86State {
 > >      uint64_t pat;
 > >  
 > >      /* exception/interrupt handling */
 > > -    int error_code;
 > > +    uint32_t error_code;
 > >      int exception_is_int;
 > >      target_ulong exception_next_eip;
 > >      target_ulong dr[8]; /* debug registers */
 > > @@ -935,7 +935,7 @@ CPUState *pc_new_cpu(const char *cpu_model);
 > >  #define cpu_list_id x86_cpu_list
 > >  #define cpudef_setup	x86_cpudef_setup
 > >  
 > > -#define CPU_SAVE_VERSION 12
 > > +#define CPU_SAVE_VERSION 13
 > >  
 > >  /* MMU modes definitions */
 > >  #define MMU_MODE0_SUFFIX _kernel
 > > diff --git a/target-i386/machine.c b/target-i386/machine.c
 > > index 4398801..fa231d8 100644
 > > --- a/target-i386/machine.c
 > > +++ b/target-i386/machine.c
 > > @@ -474,6 +474,8 @@ static const VMStateDescription vmstate_cpu = {
 > >          VMSTATE_UINT64_V(xcr0, CPUState, 12),
 > >          VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
 > >          VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
 > > +
 > > +        VMSTATE_UINT32_V(error_code, CPUState, 13),
 > >          VMSTATE_END_OF_LIST()
 > >          /* The above list is not sorted /wrt version numbers, watch out! */
 > >      }

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Michael S. Tsirkin]

On Thu, Dec 02, 2010 at 10:19:55PM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
>  > On Thu, Dec 02, 2010 at 08:56:30PM +0800, Jason Wang wrote:
>  > > Michael S. Tsirkin writes:
>  > >  > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
>  > >  > > Michael S. Tsirkin writes:
>  > >  > >  > Avoid sending out packets, and modifying
>  > >  > >  > device state, when VM is stopped.
>  > >  > >  > Add assert statements to verify this does not happen.
>  > >  > >  > 
>  > >  > >  > Avoid scheduling bh when vhost-net is started.
>  > >  > >  > 
>  > >  > >  > Stop bh when driver disabled bus mastering
>  > >  > >  > (we must not access memory after this).
>  > >  > >  > 
>  > >  > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  > >  > >  > 
>  > >  > > 
>  > >  > > There's no need to disable it bh we call qemu_aio_flush() after
>  > >  > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
>  > >  > > stop its timer in vm state change handler, not only for virtio-net?
>  > >  > 
>  > >  > BTW I fixed some typos. Here a fixed version.
>  > >  > Jason, could you review/test please?
>  > >  > 
>  > > 
>  > > Have done the test, it's more stable than before but still get small deltas in
>  > > cpu section.
>  > 
>  > And just to clarify: no more deltas in the memory section?
>  > 
> 
> Yes.
> 
> And the offset for cpu section is 1161-1165

As far as I can say the state is in
target-i386/machine.c
static const VMStateDescription vmstate_cpu.
Need to do some math to find this:

I think this is mtrr_var, but maybe my math is off.
I would sugest printing out the state and see
what is changed exactly.


> and sometimes I get deltas for ide
> section at offset 295 and 314.

I see that ide has some bh processing.  Most likely that starts io after
vmstop?  I suggest adding a vm state handler and checking vm status in
ide_dma_restart_bh.

Start with an assert, just for debug.

Also, what if we use virtio-blk?


>  > > I didn't find any interesting difference by checking the
>  > > CPUX86State in the dest in kvm_arch_load_regs(), any thought on this?
>  > > 
>  > > BTW, looks like the error_code was missed in saving the cpu state:
>  > > 
>  > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>  > > index 35a1a51..145bb38 100644
>  > > --- a/target-i386/cpu.h
>  > > +++ b/target-i386/cpu.h
>  > > @@ -687,7 +687,7 @@ typedef struct CPUX86State {
>  > >      uint64_t pat;
>  > >  
>  > >      /* exception/interrupt handling */
>  > > -    int error_code;
>  > > +    uint32_t error_code;
>  > >      int exception_is_int;
>  > >      target_ulong exception_next_eip;
>  > >      target_ulong dr[8]; /* debug registers */
>  > > @@ -935,7 +935,7 @@ CPUState *pc_new_cpu(const char *cpu_model);
>  > >  #define cpu_list_id x86_cpu_list
>  > >  #define cpudef_setup	x86_cpudef_setup
>  > >  
>  > > -#define CPU_SAVE_VERSION 12
>  > > +#define CPU_SAVE_VERSION 13
>  > >  
>  > >  /* MMU modes definitions */
>  > >  #define MMU_MODE0_SUFFIX _kernel
>  > > diff --git a/target-i386/machine.c b/target-i386/machine.c
>  > > index 4398801..fa231d8 100644
>  > > --- a/target-i386/machine.c
>  > > +++ b/target-i386/machine.c
>  > > @@ -474,6 +474,8 @@ static const VMStateDescription vmstate_cpu = {
>  > >          VMSTATE_UINT64_V(xcr0, CPUState, 12),
>  > >          VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
>  > >          VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
>  > > +
>  > > +        VMSTATE_UINT32_V(error_code, CPUState, 13),
>  > >          VMSTATE_END_OF_LIST()
>  > >          /* The above list is not sorted /wrt version numbers, watch out! */
>  > >      }

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Michael S. Tsirkin]

On Thu, Dec 02, 2010 at 10:19:55PM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
>  > On Thu, Dec 02, 2010 at 08:56:30PM +0800, Jason Wang wrote:
>  > > Michael S. Tsirkin writes:
>  > >  > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
>  > >  > > Michael S. Tsirkin writes:
>  > >  > >  > Avoid sending out packets, and modifying
>  > >  > >  > device state, when VM is stopped.
>  > >  > >  > Add assert statements to verify this does not happen.
>  > >  > >  > 
>  > >  > >  > Avoid scheduling bh when vhost-net is started.
>  > >  > >  > 
>  > >  > >  > Stop bh when driver disabled bus mastering
>  > >  > >  > (we must not access memory after this).
>  > >  > >  > 
>  > >  > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  > >  > >  > 
>  > >  > > 
>  > >  > > There's no need to disable it bh we call qemu_aio_flush() after
>  > >  > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
>  > >  > > stop its timer in vm state change handler, not only for virtio-net?
>  > >  > 
>  > >  > BTW I fixed some typos. Here a fixed version.
>  > >  > Jason, could you review/test please?
>  > >  > 
>  > > 
>  > > Have done the test, it's more stable than before but still get small deltas in
>  > > cpu section.
>  > 
>  > And just to clarify: no more deltas in the memory section?
>  > 
> 
> Yes.
> 
> And the offset for cpu section is 1161-1165 and sometimes I get deltas for ide
> section at offset 295 and 314.


Kevin, could you take a look please?

Jason, does the following do anything?

Subject: ide: cancel bh on vm stop

If bh is running on vm stop, ide state might change
after vm stop is completed. Solve by deleting bh on stop.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 484e0ca..8d86114 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -698,8 +698,13 @@ void ide_dma_restart_cb(void *opaque, int running, int reason)
 {
     BMDMAState *bm = opaque;
 
-    if (!running)
+    if (!running) {
+        if (bm->bh) {
+            qemu_bh_delete(bm->bh);
+            bm->bh = NULL;
+        }
         return;
+    }
 
     if (!bm->bh) {
         bm->bh = qemu_bh_new(ide_dma_restart_bh, bm);

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Kevin Wolf]

Am 02.12.2010 19:27, schrieb Michael S. Tsirkin:
> On Thu, Dec 02, 2010 at 10:19:55PM +0800, Jason Wang wrote:
>> Michael S. Tsirkin writes:
>>  > On Thu, Dec 02, 2010 at 08:56:30PM +0800, Jason Wang wrote:
>>  > > Michael S. Tsirkin writes:
>>  > >  > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
>>  > >  > > Michael S. Tsirkin writes:
>>  > >  > >  > Avoid sending out packets, and modifying
>>  > >  > >  > device state, when VM is stopped.
>>  > >  > >  > Add assert statements to verify this does not happen.
>>  > >  > >  > 
>>  > >  > >  > Avoid scheduling bh when vhost-net is started.
>>  > >  > >  > 
>>  > >  > >  > Stop bh when driver disabled bus mastering
>>  > >  > >  > (we must not access memory after this).
>>  > >  > >  > 
>>  > >  > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>  > >  > >  > 
>>  > >  > > 
>>  > >  > > There's no need to disable it bh we call qemu_aio_flush() after
>>  > >  > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
>>  > >  > > stop its timer in vm state change handler, not only for virtio-net?
>>  > >  > 
>>  > >  > BTW I fixed some typos. Here a fixed version.
>>  > >  > Jason, could you review/test please?
>>  > >  > 
>>  > > 
>>  > > Have done the test, it's more stable than before but still get small deltas in
>>  > > cpu section.
>>  > 
>>  > And just to clarify: no more deltas in the memory section?
>>  > 
>>
>> Yes.
>>
>> And the offset for cpu section is 1161-1165 and sometimes I get deltas for ide
>> section at offset 295 and 314.
> 
> 
> Kevin, could you take a look please?
> 
> Jason, does the following do anything?
> 
> Subject: ide: cancel bh on vm stop
> 
> If bh is running on vm stop, ide state might change
> after vm stop is completed. Solve by deleting bh on stop.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 484e0ca..8d86114 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -698,8 +698,13 @@ void ide_dma_restart_cb(void *opaque, int running, int reason)
>  {
>      BMDMAState *bm = opaque;
>  
> -    if (!running)
> +    if (!running) {
> +        if (bm->bh) {
> +            qemu_bh_delete(bm->bh);
> +            bm->bh = NULL;
> +        }
>          return;
> +    }
>  
>      if (!bm->bh) {
>          bm->bh = qemu_bh_new(ide_dma_restart_bh, bm);

Doesn't look incorrect to me, though I would be surprised if you ever
hit the case where bm->bh is not NULL. It would mean that immediately
after a cont the VM is stopped again. This bottom half is only ever used
in the vm_change_handler.

Considering that above was mentioned that a qemu_aio_flush() is run, I
also don't think that it makes any difference.

Kevin

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Kevin Wolf]

Am 01.12.2010 06:59, schrieb Michael S. Tsirkin:
> On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
>> Michael S. Tsirkin writes:
>>  > Avoid sending out packets, and modifying
>>  > device state, when VM is stopped.
>>  > Add assert statements to verify this does not happen.
>>  > 
>>  > Avoid scheduling bh when vhost-net is started.
>>  > 
>>  > Stop bh when driver disabled bus mastering
>>  > (we must not access memory after this).
>>  > 
>>  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>  > 
>>
>> There's no need to disable it bh we call qemu_aio_flush() after
>> vm_state_notify() in do_vm_stop().
> 
> Yes, but bh can resubmit itself, so flush is not enough.
> Note that vm stop is not the only reason to avoid
> running bh: when vhost-net is enabled we should avoid it too.
> Makes sense?

If I understand correctly, qemu_aio_flush() is supposed to also execute
the resubmitted BH, so that you really end up with an empty BH list.

Kevin

Re: [Qemu-devel] [PATCHv3 4/6] virtio-net: stop/start bh when appropriate

[Jason Wang]

Michael S. Tsirkin writes:
 > On Thu, Dec 02, 2010 at 10:19:55PM +0800, Jason Wang wrote:
 > > Michael S. Tsirkin writes:
 > >  > On Thu, Dec 02, 2010 at 08:56:30PM +0800, Jason Wang wrote:
 > >  > > Michael S. Tsirkin writes:
 > >  > >  > On Wed, Dec 01, 2010 at 01:45:09PM +0800, Jason Wang wrote:
 > >  > >  > > Michael S. Tsirkin writes:
 > >  > >  > >  > Avoid sending out packets, and modifying
 > >  > >  > >  > device state, when VM is stopped.
 > >  > >  > >  > Add assert statements to verify this does not happen.
 > >  > >  > >  > 
 > >  > >  > >  > Avoid scheduling bh when vhost-net is started.
 > >  > >  > >  > 
 > >  > >  > >  > Stop bh when driver disabled bus mastering
 > >  > >  > >  > (we must not access memory after this).
 > >  > >  > >  > 
 > >  > >  > >  > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
 > >  > >  > >  > 
 > >  > >  > > 
 > >  > >  > > There's no need to disable it bh we call qemu_aio_flush() after
 > >  > >  > > vm_state_notify() in do_vm_stop(). And for timer, looks like every device should
 > >  > >  > > stop its timer in vm state change handler, not only for virtio-net?
 > >  > >  > 
 > >  > >  > BTW I fixed some typos. Here a fixed version.
 > >  > >  > Jason, could you review/test please?
 > >  > >  > 
 > >  > > 
 > >  > > Have done the test, it's more stable than before but still get small deltas in
 > >  > > cpu section.
 > >  > 
 > >  > And just to clarify: no more deltas in the memory section?
 > >  > 
 > > 
 > > Yes.
 > > 
 > > And the offset for cpu section is 1161-1165
 > 
 > As far as I can say the state is in
 > target-i386/machine.c
 > static const VMStateDescription vmstate_cpu.
 > Need to do some math to find this:
 > 
 > I think this is mtrr_var, but maybe my math is off.
 > I would sugest printing out the state and see
 > what is changed exactly.
 > 

Try printing CPUX86State through gdb and the filed used to do the save/restore
are the same. Have done the check for mtrr_var and the value are same for both
src and dst. And looks like it was never used by kvm.

 > 
 > > and sometimes I get deltas for ide
 > > section at offset 295 and 314.
 > 
 > I see that ide has some bh processing.  Most likely that starts io after
 > vmstop?  I suggest adding a vm state handler and checking vm status in
 > ide_dma_restart_bh.
 > 
 > Start with an assert, just for debug.
 > 
 > Also, what if we use virtio-blk?
 > 

One byte delta for virtio-blk section at offset 377. And also get delta for ide
section ( so I didn't try your patch of stopping bh of ide becuse for virtio-blk
we even do not use ide ).

 > 
 > >  > > I didn't find any interesting difference by checking the
 > >  > > CPUX86State in the dest in kvm_arch_load_regs(), any thought on this?
 > >  > > 
 > >  > > BTW, looks like the error_code was missed in saving the cpu state:
 > >  > > 
 > >  > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
 > >  > > index 35a1a51..145bb38 100644
 > >  > > --- a/target-i386/cpu.h
 > >  > > +++ b/target-i386/cpu.h
 > >  > > @@ -687,7 +687,7 @@ typedef struct CPUX86State {
 > >  > >      uint64_t pat;
 > >  > >  
 > >  > >      /* exception/interrupt handling */
 > >  > > -    int error_code;
 > >  > > +    uint32_t error_code;
 > >  > >      int exception_is_int;
 > >  > >      target_ulong exception_next_eip;
 > >  > >      target_ulong dr[8]; /* debug registers */
 > >  > > @@ -935,7 +935,7 @@ CPUState *pc_new_cpu(const char *cpu_model);
 > >  > >  #define cpu_list_id x86_cpu_list
 > >  > >  #define cpudef_setup	x86_cpudef_setup
 > >  > >  
 > >  > > -#define CPU_SAVE_VERSION 12
 > >  > > +#define CPU_SAVE_VERSION 13
 > >  > >  
 > >  > >  /* MMU modes definitions */
 > >  > >  #define MMU_MODE0_SUFFIX _kernel
 > >  > > diff --git a/target-i386/machine.c b/target-i386/machine.c
 > >  > > index 4398801..fa231d8 100644
 > >  > > --- a/target-i386/machine.c
 > >  > > +++ b/target-i386/machine.c
 > >  > > @@ -474,6 +474,8 @@ static const VMStateDescription vmstate_cpu = {
 > >  > >          VMSTATE_UINT64_V(xcr0, CPUState, 12),
 > >  > >          VMSTATE_UINT64_V(xstate_bv, CPUState, 12),
 > >  > >          VMSTATE_YMMH_REGS_VARS(ymmh_regs, CPUState, CPU_NB_REGS, 12),
 > >  > > +
 > >  > > +        VMSTATE_UINT32_V(error_code, CPUState, 13),
 > >  > >          VMSTATE_END_OF_LIST()
 > >  > >          /* The above list is not sorted /wrt version numbers, watch out! */
 > >  > >      }

Re: [Qemu-devel] [PATCHv2 2/6] cpus: flush all requests on each vm stop

[Marcelo Tosatti]

On Tue, Nov 30, 2010 at 04:05:31PM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 30, 2010 at 11:46:48AM -0200, Marcelo Tosatti wrote:
> > On Tue, Nov 30, 2010 at 03:34:29PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Nov 30, 2010 at 10:45:40AM -0200, Marcelo Tosatti wrote:
> > > > On Wed, Nov 24, 2010 at 05:52:58PM +0200, Michael S. Tsirkin wrote:
> > > > > Make sure disk is in consistent state.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Tested-by: Jason Wang <jasowang@redhat.com>
> > > > > ---
> > > > >  cpus.c |    2 ++
> > > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/cpus.c b/cpus.c
> > > > > index 91a0fb1..d421a96 100644
> > > > > --- a/cpus.c
> > > > > +++ b/cpus.c
> > > > > @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
> > > > >          cpu_disable_ticks();
> > > > >          vm_running = 0;
> > > > >          pause_all_vcpus();
> > > > > +        qemu_aio_flush();
> > > > > +        bdrv_flush_all();
> > > > 
> > > > Can you move these after vm_state_notify? qemu-kvm stops vcpus there.
> > > > 
> > > > >          vm_state_notify(0, reason);
> > > > >          monitor_protocol_event(QEVENT_STOP, NULL);
> > > > >      }
> > > 
> > > Like this:
> > > 
> > > cpus: flush all requests on each vm stop
> > > 
> > > Make sure disk is in consistent state.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Tested-by: Jason Wang <jasowang@redhat.com>
> > > 
> > > ---
> > > 
> > > diff --git a/cpus.c b/cpus.c
> > > index 91a0fb1..d421a96 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -110,6 +110,8 @@ static void do_vm_stop(int reason)
> > >          cpu_disable_ticks();
> > >          vm_running = 0;
> > >          pause_all_vcpus();
> > > +        qemu_aio_flush();
> > > +        bdrv_flush_all();
> > >          vm_state_notify(0, reason);
> > >          monitor_protocol_event(QEVENT_STOP, NULL);
> > >      }
> > 
> > No, after vm_state_notify.
> 
> 
> Like this then:
> 
> 
>     cpus: flush all requests on each vm stop
>     
>     Flush all requests once we have stopped all
>     cpus and devices.
>     Make sure disk is in consistent state.
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Tested-by: Jason Wang <jasowang@redhat.com>
> 
> diff --git a/cpus.c b/cpus.c
> index 91a0fb1..0309189 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -111,6 +111,8 @@ static void do_vm_stop(int reason)
>          vm_running = 0;
>          pause_all_vcpus();
>          vm_state_notify(0, reason);
> +        qemu_aio_flush();
> +        bdrv_flush_all();
>          monitor_protocol_event(QEVENT_STOP, NULL);
>      }
>  }

ACK.