[Qemu-devel] [PATCH 1/2] virtio-net: drop assert on vm stop

[Qemu-devel] [PATCH 1/2] virtio-net: drop assert on vm stop

[Michael S. Tsirkin]

On vm stop, vm_running state set to stopped
before device is notified, so callbacks can get envoked with
vm_running = false; and this is not an error.

Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/virtio-net.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 826a2a5..2040eac 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1125,8 +1125,6 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
         return num_packets;
     }
 
-    assert(vdev->vm_running);
-
     if (q->async_tx.elem.out_num) {
         virtio_queue_set_notification(q->tx_vq, 0);
         return num_packets;
-- 
MST

[Qemu-devel] [PATCH 2/2] Revert "virtio: don't call device on !vm_running"

[Michael S. Tsirkin]

This reverts commit a1bc7b827e422e1ff065640d8ec5347c4aadfcd8.
    virtio: don't call device on !vm_running
It turns out that virtio net assumes that vm_running
is updated before device status callback in many places,
so this change leads to asserts.
Previous commit fixes the root issue that motivated
a1bc7b827e422e1ff065640d8ec5347c4aadfcd8 differently,
so there's no longer a need for this change.

In the future, we might be able to drop checking vm_running
completely, and check vm state directly.

Reported-by: Dietmar Maurer <dietmar@proxmox.com>
Cc: qemu-stable@nongnu.org
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ac22238..5c98180 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1108,10 +1108,7 @@ 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);
-
-    if (running) {
-        vdev->vm_running = running;
-    }
+    vdev->vm_running = running;
 
     if (backend_run) {
         virtio_set_status(vdev, vdev->status);
@@ -1124,10 +1121,6 @@ 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,
-- 
MST

Re: [Qemu-devel] [PATCH 1/2] virtio-net: drop assert on vm stop

[Jason Wang]

On 09/12/2014 12:18 AM, Michael S. Tsirkin wrote:
> On vm stop, vm_running state set to stopped
> before device is notified, so callbacks can get envoked with
> vm_running = false; and this is not an error.

This is consistent with virtio-blk which also has such kinds of
callbacks. This fixes the issue of qemu crashing when stop during
transmission.

Acked-by: Jason Wang <jasowang@redhat.com>
> Cc: qemu-stable@nongnu.org
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/virtio-net.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 826a2a5..2040eac 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1125,8 +1125,6 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>          return num_packets;
>      }
>  
> -    assert(vdev->vm_running);
> -
>      if (q->async_tx.elem.out_num) {
>          virtio_queue_set_notification(q->tx_vq, 0);
>          return num_packets;

Re: [Qemu-devel] [PATCH 2/2] Revert "virtio: don't call device on !vm_running"

[Jason Wang]

On 09/12/2014 12:18 AM, Michael S. Tsirkin wrote:
> This reverts commit a1bc7b827e422e1ff065640d8ec5347c4aadfcd8.
>     virtio: don't call device on !vm_running
> It turns out that virtio net assumes that vm_running
> is updated before device status callback in many places,
> so this change leads to asserts.
> Previous commit fixes the root issue that motivated
> a1bc7b827e422e1ff065640d8ec5347c4aadfcd8 differently,
> so there's no longer a need for this change.
>
> In the future, we might be able to drop checking vm_running
> completely, and check vm state directly.

Acked-by: Jason Wang <jasowang@redhat.com>
> Reported-by: Dietmar Maurer <dietmar@proxmox.com>
> Cc: qemu-stable@nongnu.org
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ac22238..5c98180 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1108,10 +1108,7 @@ 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);
> -
> -    if (running) {
> -        vdev->vm_running = running;
> -    }
> +    vdev->vm_running = running;
>  
>      if (backend_run) {
>          virtio_set_status(vdev, vdev->status);
> @@ -1124,10 +1121,6 @@ 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,

Re: [Qemu-devel] [PATCH 2/2] Revert "virtio: don't call device on !vm_running"

[Christian Borntraeger]

On 09/11/2014 06:18 PM, Michael S. Tsirkin wrote:
> This reverts commit a1bc7b827e422e1ff065640d8ec5347c4aadfcd8.
>     virtio: don't call device on !vm_running
> It turns out that virtio net assumes that vm_running
> is updated before device status callback in many places,
> so this change leads to asserts.
> Previous commit fixes the root issue that motivated
> a1bc7b827e422e1ff065640d8ec5347c4aadfcd8 differently,
> so there's no longer a need for this change.
> 
> In the future, we might be able to drop checking vm_running
> completely, and check vm state directly.
> 
> Reported-by: Dietmar Maurer <dietmar@proxmox.com>
> Cc: qemu-stable@nongnu.org
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>


> ---
>  hw/virtio/virtio.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ac22238..5c98180 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1108,10 +1108,7 @@ 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);
> -
> -    if (running) {
> -        vdev->vm_running = running;
> -    }
> +    vdev->vm_running = running;
> 
>      if (backend_run) {
>          virtio_set_status(vdev, vdev->status);
> @@ -1124,10 +1121,6 @@ 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,
>