[Qemu-devel] [PULL v2 0/6] Net patches

[Qemu-devel] [PULL v2 0/6] Net patches

[Stefan Hajnoczi]

v2:
 * Dropped netdev qdev change which broke aarch64 qtests

The following changes since commit 30eaca3acdf17d7bcbd1213eb149c02037edfb0b:

  Merge remote-tracking branch 'remotes/spice/tags/pull-spice-20140902-1' into staging (2014-09-02 10:26:10 +0100)

are available in the git repository at:


  git://github.com/stefanha/qemu.git tags/net-pull-request

for you to fetch changes up to 086abc1ccd0fa5103345adda819e6c6436949579:

  virtio-net: purge outstanding packets when starting vhost (2014-09-04 17:19:09 +0100)

----------------------------------------------------------------
Net patches

----------------------------------------------------------------
Michael S. Tsirkin (5):
      virtio-net: don't run bh on vm stopped
      virtio: don't call device on !vm_running
      net: invoke callback when purging queue
      net: complete all queued packets on VM stop
      virtio-net: purge outstanding packets when starting vhost

zhanghailiang (1):
      net: Forbid dealing with packets when VM is not running

 hw/net/virtio-net.c | 29 ++++++++++++++++++++++++++---
 hw/virtio/virtio.c  |  9 ++++++++-
 net/net.c           | 40 +++++++++++++++++++++++++++++++++++++++-
 net/queue.c         |  3 +++
 4 files changed, 76 insertions(+), 5 deletions(-)

-- 
1.9.3

[Qemu-devel] [PULL 1/6] net: Forbid dealing with packets when VM is not running

[Stefan Hajnoczi]

From: zhanghailiang <zhang.zhanghailiang@huawei.com>

For all NICs(except virtio-net) emulated by qemu,
Such as e1000, rtl8139, pcnet and ne2k_pci,
Qemu can still receive packets when VM is not running.

If this happened in *migration's* last PAUSE VM stage, but
before the end of the migration, the new receiving packets will possibly dirty
parts of RAM which has been cached in *iovec*(will be sent asynchronously) and
dirty parts of new RAM which will be missed.
This will lead serious network fault in VM.

To avoid this, we forbid receiving packets in generic net code when
VM is not running.

Bug reproduction steps:
(1) Start a VM which configured at least one NIC
(2) In VM, open several Terminal and do *Ping IP -i 0.1*
(3) Migrate the VM repeatedly between two Hosts
And the *PING* command in VM will very likely fail with message:
'Destination HOST Unreachable', the NIC in VM will stay unavailable unless you
run 'service network restart'

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Jason Wang <jasowang@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 net/net.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/net.c b/net/net.c
index 6d930ea..962c05f 100644
--- a/net/net.c
+++ b/net/net.c
@@ -41,6 +41,7 @@
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/dealloc-visitor.h"
+#include "sysemu/sysemu.h"
 
 /* Net bridge is currently not supported for W32. */
 #if !defined(_WIN32)
@@ -452,6 +453,12 @@ void qemu_set_vnet_hdr_len(NetClientState *nc, int len)
 
 int qemu_can_send_packet(NetClientState *sender)
 {
+    int vm_running = runstate_is_running();
+
+    if (!vm_running) {
+        return 0;
+    }
+
     if (!sender->peer) {
         return 1;
     }
-- 
1.9.3

[Qemu-devel] [PULL 2/6] virtio-net: don't run bh on vm stopped

[Stefan Hajnoczi]

From: "Michael S. Tsirkin" <mst@redhat.com>

commit 783e7706937fe15523b609b545587a028a2bdd03
    virtio-net: stop/start bh when appropriate

is incomplete: BH might execute within the same main loop iteration but
after vmstop, so in theory, we might trigger an assertion.
I was unable to reproduce this in practice,
but it seems clear enough that the potential is there, so worth fixing.

Cc: qemu-stable@nongnu.org
Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/virtio-net.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 268eff9..365e266 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1224,7 +1224,12 @@ static void virtio_net_tx_timer(void *opaque)
     VirtIONetQueue *q = opaque;
     VirtIONet *n = q->n;
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
-    assert(vdev->vm_running);
+    /* This happens when device was stopped but BH wasn't. */
+    if (!vdev->vm_running) {
+        /* Make sure tx waiting is set, so we'll run when restarted. */
+        assert(q->tx_waiting);
+        return;
+    }
 
     q->tx_waiting = 0;
 
@@ -1244,7 +1249,12 @@ static void virtio_net_tx_bh(void *opaque)
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
     int32_t ret;
 
-    assert(vdev->vm_running);
+    /* This happens when device was stopped but BH wasn't. */
+    if (!vdev->vm_running) {
+        /* Make sure tx waiting is set, so we'll run when restarted. */
+        assert(q->tx_waiting);
+        return;
+    }
 
     q->tx_waiting = 0;
 
-- 
1.9.3

[Qemu-devel] [PULL 3/6] virtio: don't call device on !vm_running

[Stefan Hajnoczi]

From: "Michael S. Tsirkin" <mst@redhat.com>

On vm stop, virtio changes vm_running state
too soon, so callbacks can get envoked with
vm_running = false;

Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5c98180..ac22238 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1108,7 +1108,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     bool backend_run = running && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK);
-    vdev->vm_running = running;
+
+    if (running) {
+        vdev->vm_running = running;
+    }
 
     if (backend_run) {
         virtio_set_status(vdev, vdev->status);
@@ -1121,6 +1124,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
     if (!backend_run) {
         virtio_set_status(vdev, vdev->status);
     }
+
+    if (!running) {
+        vdev->vm_running = running;
+    }
 }
 
 void virtio_init(VirtIODevice *vdev, const char *name,
-- 
1.9.3

Re: [Qemu-devel] [PULL 3/6] virtio: don't call device on !vm_running

[Christian Borntraeger]

On 04/09/14 18:38, Stefan Hajnoczi wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> On vm stop, virtio changes vm_running state
> too soon, so callbacks can get envoked with
> vm_running = false;
> 
> Cc: qemu-stable@nongnu.org
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5c98180..ac22238 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1108,7 +1108,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      bool backend_run = running && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK);
> -    vdev->vm_running = running;
> +
> +    if (running) {
> +        vdev->vm_running = running;
> +    }
> 
>      if (backend_run) {
>          virtio_set_status(vdev, vdev->status);
> @@ -1121,6 +1124,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
>      if (!backend_run) {
>          virtio_set_status(vdev, vdev->status);
>      }
> +
> +    if (!running) {
> +        vdev->vm_running = running;
> +    }
>  }
> 
>  void virtio_init(VirtIODevice *vdev, const char *name,
> 


This broke managedsave on s390x:
qemu-system-s390x: hw/net/virtio-net.c:1348: virtio_net_save: Assertion `!n->vhost_started' failed.
reverting that commit makes it working again.
Any ideas?

Christian

Re: [Qemu-devel] [PULL 3/6] virtio: don't call device on !vm_running

[Michael S. Tsirkin]

On Thu, Sep 11, 2014 at 02:19:42PM +0200, Christian Borntraeger wrote:
> On 04/09/14 18:38, Stefan Hajnoczi wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > 
> > On vm stop, virtio changes vm_running state
> > too soon, so callbacks can get envoked with
> > vm_running = false;
> > 
> > Cc: qemu-stable@nongnu.org
> > Cc: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/virtio/virtio.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 5c98180..ac22238 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1108,7 +1108,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
> >      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> >      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> >      bool backend_run = running && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK);
> > -    vdev->vm_running = running;
> > +
> > +    if (running) {
> > +        vdev->vm_running = running;
> > +    }
> > 
> >      if (backend_run) {
> >          virtio_set_status(vdev, vdev->status);
> > @@ -1121,6 +1124,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
> >      if (!backend_run) {
> >          virtio_set_status(vdev, vdev->status);
> >      }
> > +
> > +    if (!running) {
> > +        vdev->vm_running = running;
> > +    }
> >  }
> > 
> >  void virtio_init(VirtIODevice *vdev, const char *name,
> > 
> 
> 
> This broke managedsave on s390x:
> qemu-system-s390x: hw/net/virtio-net.c:1348: virtio_net_save: Assertion `!n->vhost_started' failed.
> reverting that commit makes it working again.
> Any ideas?
> 
> Christian


Does it help if you apply
[PULL v2 09/16] vhost_net: cleanup start/stop condition
on top?

Re: [Qemu-devel] [PULL 3/6] virtio: don't call device on !vm_running

[Christian Borntraeger]

On 11/09/14 14:38, Michael S. Tsirkin wrote:
>> > 
>> > This broke managedsave on s390x:
>> > qemu-system-s390x: hw/net/virtio-net.c:1348: virtio_net_save: Assertion `!n->vhost_started' failed.
>> > reverting that commit makes it working again.
>> > Any ideas?
>> > 
>> > Christian
> 
> Does it help if you apply
> [PULL v2 09/16] vhost_net: cleanup start/stop condition
> on top?
> 

This patch is part of my tree (I am testing qemu/master + some s390 specific patches)

Christian

[Qemu-devel] [PULL 4/6] net: invoke callback when purging queue

[Stefan Hajnoczi]

From: "Michael S. Tsirkin" <mst@redhat.com>

devices rely on packet callbacks eventually running,
but we violate this rule whenever we purge the queue.
To fix, invoke callbacks on all packets on purge.
Set length to 0, this way callers can detect that
this happened and re-queue if necessary.

Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 net/queue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/queue.c b/net/queue.c
index 859d02a..f948318 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -233,6 +233,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
         if (packet->sender == from) {
             QTAILQ_REMOVE(&queue->packets, packet, entry);
             queue->nq_count--;
+            if (packet->sent_cb) {
+                packet->sent_cb(packet->sender, 0);
+            }
             g_free(packet);
         }
     }
-- 
1.9.3

[Qemu-devel] [PULL 5/6] net: complete all queued packets on VM stop

[Stefan Hajnoczi]

From: "Michael S. Tsirkin" <mst@redhat.com>

This completes all packets, ensuring that callbacks
will not run when VM is stopped.

Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 net/net.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index 962c05f..7acc162 100644
--- a/net/net.c
+++ b/net/net.c
@@ -48,6 +48,7 @@
 # define CONFIG_NET_BRIDGE
 #endif
 
+static VMChangeStateEntry *net_change_state_entry;
 static QTAILQ_HEAD(, NetClientState) net_clients;
 
 const char *host_net_devices[] = {
@@ -511,7 +512,8 @@ void qemu_purge_queued_packets(NetClientState *nc)
     qemu_net_queue_purge(nc->peer->incoming_queue, nc);
 }
 
-void qemu_flush_queued_packets(NetClientState *nc)
+static
+void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge)
 {
     nc->receive_disabled = 0;
 
@@ -525,9 +527,17 @@ void qemu_flush_queued_packets(NetClientState *nc)
          * the file descriptor (for tap, for example).
          */
         qemu_notify_event();
+    } else if (purge) {
+        /* Unable to empty the queue, purge remaining packets */
+        qemu_net_queue_purge(nc->incoming_queue, nc);
     }
 }
 
+void qemu_flush_queued_packets(NetClientState *nc)
+{
+    qemu_flush_or_purge_queued_packets(nc, false);
+}
+
 static ssize_t qemu_send_packet_async_with_flags(NetClientState *sender,
                                                  unsigned flags,
                                                  const uint8_t *buf, int size,
@@ -1175,6 +1185,22 @@ void qmp_set_link(const char *name, bool up, Error **errp)
     }
 }
 
+static void net_vm_change_state_handler(void *opaque, int running,
+                                        RunState state)
+{
+    /* Complete all queued packets, to guarantee we don't modify
+     * state later when VM is not running.
+     */
+    if (!running) {
+        NetClientState *nc;
+        NetClientState *tmp;
+
+        QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) {
+            qemu_flush_or_purge_queued_packets(nc, true);
+        }
+    }
+}
+
 void net_cleanup(void)
 {
     NetClientState *nc;
@@ -1190,6 +1216,8 @@ void net_cleanup(void)
             qemu_del_net_client(nc);
         }
     }
+
+    qemu_del_vm_change_state_handler(net_change_state_entry);
 }
 
 void net_check_clients(void)
@@ -1275,6 +1303,9 @@ int net_init_clients(void)
 #endif
     }
 
+    net_change_state_entry =
+        qemu_add_vm_change_state_handler(net_vm_change_state_handler, NULL);
+
     QTAILQ_INIT(&net_clients);
 
     if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)
-- 
1.9.3

[Qemu-devel] [PULL 6/6] virtio-net: purge outstanding packets when starting vhost

[Stefan Hajnoczi]

From: "Michael S. Tsirkin" <mst@redhat.com>

whenever we start vhost, virtio could have outstanding packets
queued, when they complete later we'll modify the ring
while vhost is processing it.

To prevent this, purge outstanding packets on vhost start.

Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/virtio-net.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 365e266..826a2a5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -125,10 +125,23 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
         return;
     }
     if (!n->vhost_started) {
-        int r;
+        int r, i;
+
         if (!vhost_net_query(get_vhost_net(nc->peer), vdev)) {
             return;
         }
+
+        /* Any packets outstanding? Purge them to avoid touching rings
+         * when vhost is running.
+         */
+        for (i = 0;  i < queues; i++) {
+            NetClientState *qnc = qemu_get_subqueue(n->nic, i);
+
+            /* Purge both directions: TX and RX. */
+            qemu_net_queue_purge(qnc->peer->incoming_queue, qnc);
+            qemu_net_queue_purge(qnc->incoming_queue, qnc->peer);
+        }
+
         n->vhost_started = 1;
         r = vhost_net_start(vdev, n->nic->ncs, queues);
         if (r < 0) {
-- 
1.9.3

Re: [Qemu-devel] [PULL v2 0/6] Net patches

[Peter Maydell]

On 4 September 2014 17:38, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> v2:
>  * Dropped netdev qdev change which broke aarch64 qtests
>
> The following changes since commit 30eaca3acdf17d7bcbd1213eb149c02037edfb0b:
>
>   Merge remote-tracking branch 'remotes/spice/tags/pull-spice-20140902-1' into staging (2014-09-02 10:26:10 +0100)
>
> are available in the git repository at:
>
>
>   git://github.com/stefanha/qemu.git tags/net-pull-request
>
> for you to fetch changes up to 086abc1ccd0fa5103345adda819e6c6436949579:
>
>   virtio-net: purge outstanding packets when starting vhost (2014-09-04 17:19:09 +0100)
>
> ----------------------------------------------------------------
> Net patches
>
> ----------------------------------------------------------------
> Michael S. Tsirkin (5):
>       virtio-net: don't run bh on vm stopped
>       virtio: don't call device on !vm_running
>       net: invoke callback when purging queue
>       net: complete all queued packets on VM stop
>       virtio-net: purge outstanding packets when starting vhost
>
> zhanghailiang (1):
>       net: Forbid dealing with packets when VM is not running

Applied, thanks.

-- PMM