Patchwork [PATCHv2] virtio-net: unify vhost-net start/stop

login
register
mail settings
Submitter Michael S. Tsirkin
Date Sept. 27, 2010, 4:41 p.m.
Message ID <20100927164130.GA7211@redhat.com>
Download mbox | patch
Permalink /patch/65883/
State New
Headers show

Comments

Michael S. Tsirkin - Sept. 27, 2010, 4:41 p.m.
Move all of vhost-net start/stop logic to a single routine,
and call it from everywhere.

Additionally, start/stop vhost-net on link up/down:
we should not transmit anything if user asked us to
put the link down.

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

Sent a wrong version, sorry about the noise.

 hw/virtio-net.c |   89 ++++++++++++++++++++++++++----------------------------
 1 files changed, 43 insertions(+), 46 deletions(-)
Alex Williamson - Sept. 27, 2010, 7:43 p.m.
On Mon, 2010-09-27 at 18:41 +0200, Michael S. Tsirkin wrote:
> Move all of vhost-net start/stop logic to a single routine,
> and call it from everywhere.
> 
> Additionally, start/stop vhost-net on link up/down:
> we should not transmit anything if user asked us to
> put the link down.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Sent a wrong version, sorry about the noise.
> 
>  hw/virtio-net.c |   89 ++++++++++++++++++++++++++----------------------------
>  1 files changed, 43 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 075f72d..f87c219 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -51,6 +51,7 @@ typedef struct VirtIONet
>      uint8_t nouni;
>      uint8_t nobcast;
>      uint8_t vhost_started;
> +    bool vm_running;
>      VMChangeStateEntry *vmstate;
>      struct {
>          int in_use;
> @@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>      }
>  }
>  
> +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> +{
> +    VirtIONet *n = to_virtio_net(vdev);
> +    if (!n->nic->nc.peer) {
> +        return;
> +    }
> +    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> +        return;
> +    }
> +
> +    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) {

I don't think this works...

#define VIRTIO_CONFIG_S_DRIVER_OK       4

!!(1) == (status & 4)?

> +        return;
> +    }
> +    if (!n->vhost_started) {
> +        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> +        if (r < 0) {
> +            fprintf(stderr, "unable to start vhost net: %d: "
> +                    "falling back on userspace virtio\n", -r);

Definitely worth noting if not using vhost, but aren't we potentially
going to see this in the log a lot in that case?  Thanks,

Alex

> +        } else {
> +            n->vhost_started = 1;
> +        }
> +    } else {
> +        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> +        n->vhost_started = 0;
> +    }
> +}
> +
>  static void virtio_net_set_link_status(VLANClientState *nc)
>  {
>      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> @@ -107,6 +140,8 @@ static void virtio_net_set_link_status(VLANClientState *nc)
>  
>      if (n->status != old_status)
>          virtio_notify_config(&n->vdev);
> +
> +    virtio_net_set_status(&n->vdev, n->vdev.status);
>  }
>  
>  static void virtio_net_reset(VirtIODevice *vdev)
> @@ -120,10 +155,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
>      n->nomulti = 0;
>      n->nouni = 0;
>      n->nobcast = 0;
> -    if (n->vhost_started) {
> -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> -        n->vhost_started = 0;
> -    }
>  
>      /* Flush any MAC and VLAN filter table state */
>      n->mac_table.in_use = 0;
> @@ -726,12 +757,9 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
>  {
>      VirtIONet *n = opaque;
>  
> -    if (n->vhost_started) {
> -        /* TODO: should we really stop the backend?
> -         * If we don't, it might keep writing to memory. */
> -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> -        n->vhost_started = 0;
> -    }
> +    /* At this point, backend must be stopped, otherwise
> +     * it might keep writing to memory. */
> +    assert(!n->vhost_started);
>      virtio_save(&n->vdev, f);
>  
>      qemu_put_buffer(f, n->mac, ETH_ALEN);
> @@ -863,44 +891,14 @@ static NetClientInfo net_virtio_info = {
>      .link_status_changed = virtio_net_set_link_status,
>  };
>  
> -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> -{
> -    VirtIONet *n = to_virtio_net(vdev);
> -    if (!n->nic->nc.peer) {
> -        return;
> -    }
> -    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> -        return;
> -    }
> -
> -    if (!tap_get_vhost_net(n->nic->nc.peer)) {
> -        return;
> -    }
> -    if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> -        return;
> -    }
> -    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> -        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
> -        if (r < 0) {
> -            fprintf(stderr, "unable to start vhost net: %d: "
> -                    "falling back on userspace virtio\n", -r);
> -        } else {
> -            n->vhost_started = 1;
> -        }
> -    } else {
> -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> -        n->vhost_started = 0;
> -    }
> -}
> -
>  static void virtio_net_vmstate_change(void *opaque, int running, int reason)
>  {
>      VirtIONet *n = opaque;
> -    uint8_t status = running ? VIRTIO_CONFIG_S_DRIVER_OK : 0;
> +    n->vm_running = running;
>      /* This is called when vm is started/stopped,
> -     * it will start/stop vhost backend if * appropriate
> +     * it will start/stop vhost backend if appropriate
>       * e.g. after migration. */
> -    virtio_net_set_status(&n->vdev, n->vdev.status & status);
> +    virtio_net_set_status(&n->vdev, n->vdev.status);
>  }
>  
>  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
> @@ -951,9 +949,8 @@ void virtio_net_exit(VirtIODevice *vdev)
>      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
>      qemu_del_vm_change_state_handler(n->vmstate);
>  
> -    if (n->vhost_started) {
> -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> -    }
> +    /* This will stop vhost backend if appropriate. */
> +    virtio_net_set_status(vdev, 0);
>  
>      qemu_purge_queued_packets(&n->nic->nc);
>
Michael S. Tsirkin - Sept. 27, 2010, 9:52 p.m.
On Mon, Sep 27, 2010 at 01:43:11PM -0600, Alex Williamson wrote:
> On Mon, 2010-09-27 at 18:41 +0200, Michael S. Tsirkin wrote:
> > Move all of vhost-net start/stop logic to a single routine,
> > and call it from everywhere.
> > 
> > Additionally, start/stop vhost-net on link up/down:
> > we should not transmit anything if user asked us to
> > put the link down.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > Sent a wrong version, sorry about the noise.
> > 
> >  hw/virtio-net.c |   89 ++++++++++++++++++++++++++----------------------------
> >  1 files changed, 43 insertions(+), 46 deletions(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 075f72d..f87c219 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -51,6 +51,7 @@ typedef struct VirtIONet
> >      uint8_t nouni;
> >      uint8_t nobcast;
> >      uint8_t vhost_started;
> > +    bool vm_running;
> >      VMChangeStateEntry *vmstate;
> >      struct {
> >          int in_use;
> > @@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> >      }
> >  }
> >  
> > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > +{
> > +    VirtIONet *n = to_virtio_net(vdev);
> > +    if (!n->nic->nc.peer) {
> > +        return;
> > +    }
> > +    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > +        return;
> > +    }
> > +
> > +    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) {
> 
> I don't think this works...
> 
> #define VIRTIO_CONFIG_S_DRIVER_OK       4
> 
> !!(1) == (status & 4)?

The && converts it to a boolean.

> > +        return;
> > +    }
> > +    if (!n->vhost_started) {
> > +        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > +        if (r < 0) {
> > +            fprintf(stderr, "unable to start vhost net: %d: "
> > +                    "falling back on userspace virtio\n", -r);
> 
> Definitely worth noting if not using vhost, but aren't we potentially
> going to see this in the log a lot in that case?  Thanks,
> 
> Alex

Not really, we detect failure at start normally, and do
not proceed.
This means that somehow the device went dead: should not
happen really, but since it's easy to recover we do.

> > +        } else {
> > +            n->vhost_started = 1;
> > +        }
> > +    } else {
> > +        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > +        n->vhost_started = 0;
> > +    }
> > +}
> > +
> >  static void virtio_net_set_link_status(VLANClientState *nc)
> >  {
> >      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> > @@ -107,6 +140,8 @@ static void virtio_net_set_link_status(VLANClientState *nc)
> >  
> >      if (n->status != old_status)
> >          virtio_notify_config(&n->vdev);
> > +
> > +    virtio_net_set_status(&n->vdev, n->vdev.status);
> >  }
> >  
> >  static void virtio_net_reset(VirtIODevice *vdev)
> > @@ -120,10 +155,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
> >      n->nomulti = 0;
> >      n->nouni = 0;
> >      n->nobcast = 0;
> > -    if (n->vhost_started) {
> > -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > -        n->vhost_started = 0;
> > -    }
> >  
> >      /* Flush any MAC and VLAN filter table state */
> >      n->mac_table.in_use = 0;
> > @@ -726,12 +757,9 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
> >  {
> >      VirtIONet *n = opaque;
> >  
> > -    if (n->vhost_started) {
> > -        /* TODO: should we really stop the backend?
> > -         * If we don't, it might keep writing to memory. */
> > -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > -        n->vhost_started = 0;
> > -    }
> > +    /* At this point, backend must be stopped, otherwise
> > +     * it might keep writing to memory. */
> > +    assert(!n->vhost_started);
> >      virtio_save(&n->vdev, f);
> >  
> >      qemu_put_buffer(f, n->mac, ETH_ALEN);
> > @@ -863,44 +891,14 @@ static NetClientInfo net_virtio_info = {
> >      .link_status_changed = virtio_net_set_link_status,
> >  };
> >  
> > -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > -{
> > -    VirtIONet *n = to_virtio_net(vdev);
> > -    if (!n->nic->nc.peer) {
> > -        return;
> > -    }
> > -    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > -        return;
> > -    }
> > -
> > -    if (!tap_get_vhost_net(n->nic->nc.peer)) {
> > -        return;
> > -    }
> > -    if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > -        return;
> > -    }
> > -    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> > -        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > -        if (r < 0) {
> > -            fprintf(stderr, "unable to start vhost net: %d: "
> > -                    "falling back on userspace virtio\n", -r);
> > -        } else {
> > -            n->vhost_started = 1;
> > -        }
> > -    } else {
> > -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > -        n->vhost_started = 0;
> > -    }
> > -}
> > -
> >  static void virtio_net_vmstate_change(void *opaque, int running, int reason)
> >  {
> >      VirtIONet *n = opaque;
> > -    uint8_t status = running ? VIRTIO_CONFIG_S_DRIVER_OK : 0;
> > +    n->vm_running = running;
> >      /* This is called when vm is started/stopped,
> > -     * it will start/stop vhost backend if * appropriate
> > +     * it will start/stop vhost backend if appropriate
> >       * e.g. after migration. */
> > -    virtio_net_set_status(&n->vdev, n->vdev.status & status);
> > +    virtio_net_set_status(&n->vdev, n->vdev.status);
> >  }
> >  
> >  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
> > @@ -951,9 +949,8 @@ void virtio_net_exit(VirtIODevice *vdev)
> >      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> >      qemu_del_vm_change_state_handler(n->vmstate);
> >  
> > -    if (n->vhost_started) {
> > -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > -    }
> > +    /* This will stop vhost backend if appropriate. */
> > +    virtio_net_set_status(vdev, 0);
> >  
> >      qemu_purge_queued_packets(&n->nic->nc);
> >  
> 
>
Alex Williamson - Sept. 28, 2010, 3:16 p.m.
On Mon, 2010-09-27 at 23:52 +0200, Michael S. Tsirkin wrote:
> On Mon, Sep 27, 2010 at 01:43:11PM -0600, Alex Williamson wrote:
> > On Mon, 2010-09-27 at 18:41 +0200, Michael S. Tsirkin wrote:
> > > Move all of vhost-net start/stop logic to a single routine,
> > > and call it from everywhere.
> > > 
> > > Additionally, start/stop vhost-net on link up/down:
> > > we should not transmit anything if user asked us to
> > > put the link down.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > 
> > > Sent a wrong version, sorry about the noise.
> > > 
> > >  hw/virtio-net.c |   89 ++++++++++++++++++++++++++----------------------------
> > >  1 files changed, 43 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > index 075f72d..f87c219 100644
> > > --- a/hw/virtio-net.c
> > > +++ b/hw/virtio-net.c
> > > @@ -51,6 +51,7 @@ typedef struct VirtIONet
> > >      uint8_t nouni;
> > >      uint8_t nobcast;
> > >      uint8_t vhost_started;
> > > +    bool vm_running;
> > >      VMChangeStateEntry *vmstate;
> > >      struct {
> > >          int in_use;
> > > @@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > >      }
> > >  }
> > >  
> > > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > > +{
> > > +    VirtIONet *n = to_virtio_net(vdev);
> > > +    if (!n->nic->nc.peer) {
> > > +        return;
> > > +    }
> > > +    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > > +        return;
> > > +    }
> > > +
> > > +    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) {
> > 
> > I don't think this works...
> > 
> > #define VIRTIO_CONFIG_S_DRIVER_OK       4
> > 
> > !!(1) == (status & 4)?
> 
> The && converts it to a boolean.

Are you missing a set of parens around everything right of the ==?  ie:

if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
                           (n->status & VIRTIO_NET_S_LINK_UP) &&
                           n->vm_running)) {...

Has this actually been tested?

Alex

> > > +        return;
> > > +    }
> > > +    if (!n->vhost_started) {
> > > +        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > > +        if (r < 0) {
> > > +            fprintf(stderr, "unable to start vhost net: %d: "
> > > +                    "falling back on userspace virtio\n", -r);
> > 
> > Definitely worth noting if not using vhost, but aren't we potentially
> > going to see this in the log a lot in that case?  Thanks,
> > 
> > Alex
> 
> Not really, we detect failure at start normally, and do
> not proceed.
> This means that somehow the device went dead: should not
> happen really, but since it's easy to recover we do.
> 
> > > +        } else {
> > > +            n->vhost_started = 1;
> > > +        }
> > > +    } else {
> > > +        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > > +        n->vhost_started = 0;
> > > +    }
> > > +}
> > > +
> > >  static void virtio_net_set_link_status(VLANClientState *nc)
> > >  {
> > >      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> > > @@ -107,6 +140,8 @@ static void virtio_net_set_link_status(VLANClientState *nc)
> > >  
> > >      if (n->status != old_status)
> > >          virtio_notify_config(&n->vdev);
> > > +
> > > +    virtio_net_set_status(&n->vdev, n->vdev.status);
> > >  }
> > >  
> > >  static void virtio_net_reset(VirtIODevice *vdev)
> > > @@ -120,10 +155,6 @@ static void virtio_net_reset(VirtIODevice *vdev)
> > >      n->nomulti = 0;
> > >      n->nouni = 0;
> > >      n->nobcast = 0;
> > > -    if (n->vhost_started) {
> > > -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > > -        n->vhost_started = 0;
> > > -    }
> > >  
> > >      /* Flush any MAC and VLAN filter table state */
> > >      n->mac_table.in_use = 0;
> > > @@ -726,12 +757,9 @@ static void virtio_net_save(QEMUFile *f, void *opaque)
> > >  {
> > >      VirtIONet *n = opaque;
> > >  
> > > -    if (n->vhost_started) {
> > > -        /* TODO: should we really stop the backend?
> > > -         * If we don't, it might keep writing to memory. */
> > > -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
> > > -        n->vhost_started = 0;
> > > -    }
> > > +    /* At this point, backend must be stopped, otherwise
> > > +     * it might keep writing to memory. */
> > > +    assert(!n->vhost_started);
> > >      virtio_save(&n->vdev, f);
> > >  
> > >      qemu_put_buffer(f, n->mac, ETH_ALEN);
> > > @@ -863,44 +891,14 @@ static NetClientInfo net_virtio_info = {
> > >      .link_status_changed = virtio_net_set_link_status,
> > >  };
> > >  
> > > -static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > > -{
> > > -    VirtIONet *n = to_virtio_net(vdev);
> > > -    if (!n->nic->nc.peer) {
> > > -        return;
> > > -    }
> > > -    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > > -        return;
> > > -    }
> > > -
> > > -    if (!tap_get_vhost_net(n->nic->nc.peer)) {
> > > -        return;
> > > -    }
> > > -    if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > -        return;
> > > -    }
> > > -    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> > > -        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > > -        if (r < 0) {
> > > -            fprintf(stderr, "unable to start vhost net: %d: "
> > > -                    "falling back on userspace virtio\n", -r);
> > > -        } else {
> > > -            n->vhost_started = 1;
> > > -        }
> > > -    } else {
> > > -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > > -        n->vhost_started = 0;
> > > -    }
> > > -}
> > > -
> > >  static void virtio_net_vmstate_change(void *opaque, int running, int reason)
> > >  {
> > >      VirtIONet *n = opaque;
> > > -    uint8_t status = running ? VIRTIO_CONFIG_S_DRIVER_OK : 0;
> > > +    n->vm_running = running;
> > >      /* This is called when vm is started/stopped,
> > > -     * it will start/stop vhost backend if * appropriate
> > > +     * it will start/stop vhost backend if appropriate
> > >       * e.g. after migration. */
> > > -    virtio_net_set_status(&n->vdev, n->vdev.status & status);
> > > +    virtio_net_set_status(&n->vdev, n->vdev.status);
> > >  }
> > >  
> > >  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
> > > @@ -951,9 +949,8 @@ void virtio_net_exit(VirtIODevice *vdev)
> > >      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> > >      qemu_del_vm_change_state_handler(n->vmstate);
> > >  
> > > -    if (n->vhost_started) {
> > > -        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
> > > -    }
> > > +    /* This will stop vhost backend if appropriate. */
> > > +    virtio_net_set_status(vdev, 0);
> > >  
> > >      qemu_purge_queued_packets(&n->nic->nc);
> > >  
> > 
> >
Michael S. Tsirkin - Oct. 3, 2010, 8:17 p.m.
On Tue, Sep 28, 2010 at 09:16:34AM -0600, Alex Williamson wrote:
> On Mon, 2010-09-27 at 23:52 +0200, Michael S. Tsirkin wrote:
> > On Mon, Sep 27, 2010 at 01:43:11PM -0600, Alex Williamson wrote:
> > > On Mon, 2010-09-27 at 18:41 +0200, Michael S. Tsirkin wrote:
> > > > Move all of vhost-net start/stop logic to a single routine,
> > > > and call it from everywhere.
> > > > 
> > > > Additionally, start/stop vhost-net on link up/down:
> > > > we should not transmit anything if user asked us to
> > > > put the link down.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > 
> > > > Sent a wrong version, sorry about the noise.
> > > > 
> > > >  hw/virtio-net.c |   89 ++++++++++++++++++++++++++----------------------------
> > > >  1 files changed, 43 insertions(+), 46 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > > > index 075f72d..f87c219 100644
> > > > --- a/hw/virtio-net.c
> > > > +++ b/hw/virtio-net.c
> > > > @@ -51,6 +51,7 @@ typedef struct VirtIONet
> > > >      uint8_t nouni;
> > > >      uint8_t nobcast;
> > > >      uint8_t vhost_started;
> > > > +    bool vm_running;
> > > >      VMChangeStateEntry *vmstate;
> > > >      struct {
> > > >          int in_use;
> > > > @@ -95,6 +96,38 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > > >      }
> > > >  }
> > > >  
> > > > +static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
> > > > +{
> > > > +    VirtIONet *n = to_virtio_net(vdev);
> > > > +    if (!n->nic->nc.peer) {
> > > > +        return;
> > > > +    }
> > > > +    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    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) {
> > > 
> > > I don't think this works...
> > > 
> > > #define VIRTIO_CONFIG_S_DRIVER_OK       4
> > > 
> > > !!(1) == (status & 4)?
> > 
> > The && converts it to a boolean.
> 
> Are you missing a set of parens around everything right of the ==?  ie:
> 
> if (!!n->vhost_started == ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>                            (n->status & VIRTIO_NET_S_LINK_UP) &&
>                            n->vm_running)) {...

Yes, that's it. Thanks!

Patch

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 075f72d..f87c219 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -51,6 +51,7 @@  typedef struct VirtIONet
     uint8_t nouni;
     uint8_t nobcast;
     uint8_t vhost_started;
+    bool vm_running;
     VMChangeStateEntry *vmstate;
     struct {
         int in_use;
@@ -95,6 +96,38 @@  static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
     }
 }
 
+static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
+{
+    VirtIONet *n = to_virtio_net(vdev);
+    if (!n->nic->nc.peer) {
+        return;
+    }
+    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+        return;
+    }
+
+    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) {
+        return;
+    }
+    if (!n->vhost_started) {
+        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
+        if (r < 0) {
+            fprintf(stderr, "unable to start vhost net: %d: "
+                    "falling back on userspace virtio\n", -r);
+        } else {
+            n->vhost_started = 1;
+        }
+    } else {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
+        n->vhost_started = 0;
+    }
+}
+
 static void virtio_net_set_link_status(VLANClientState *nc)
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
@@ -107,6 +140,8 @@  static void virtio_net_set_link_status(VLANClientState *nc)
 
     if (n->status != old_status)
         virtio_notify_config(&n->vdev);
+
+    virtio_net_set_status(&n->vdev, n->vdev.status);
 }
 
 static void virtio_net_reset(VirtIODevice *vdev)
@@ -120,10 +155,6 @@  static void virtio_net_reset(VirtIODevice *vdev)
     n->nomulti = 0;
     n->nouni = 0;
     n->nobcast = 0;
-    if (n->vhost_started) {
-        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
-        n->vhost_started = 0;
-    }
 
     /* Flush any MAC and VLAN filter table state */
     n->mac_table.in_use = 0;
@@ -726,12 +757,9 @@  static void virtio_net_save(QEMUFile *f, void *opaque)
 {
     VirtIONet *n = opaque;
 
-    if (n->vhost_started) {
-        /* TODO: should we really stop the backend?
-         * If we don't, it might keep writing to memory. */
-        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), &n->vdev);
-        n->vhost_started = 0;
-    }
+    /* At this point, backend must be stopped, otherwise
+     * it might keep writing to memory. */
+    assert(!n->vhost_started);
     virtio_save(&n->vdev, f);
 
     qemu_put_buffer(f, n->mac, ETH_ALEN);
@@ -863,44 +891,14 @@  static NetClientInfo net_virtio_info = {
     .link_status_changed = virtio_net_set_link_status,
 };
 
-static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
-{
-    VirtIONet *n = to_virtio_net(vdev);
-    if (!n->nic->nc.peer) {
-        return;
-    }
-    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
-        return;
-    }
-
-    if (!tap_get_vhost_net(n->nic->nc.peer)) {
-        return;
-    }
-    if (!!n->vhost_started == !!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
-        return;
-    }
-    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
-        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
-        if (r < 0) {
-            fprintf(stderr, "unable to start vhost net: %d: "
-                    "falling back on userspace virtio\n", -r);
-        } else {
-            n->vhost_started = 1;
-        }
-    } else {
-        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
-        n->vhost_started = 0;
-    }
-}
-
 static void virtio_net_vmstate_change(void *opaque, int running, int reason)
 {
     VirtIONet *n = opaque;
-    uint8_t status = running ? VIRTIO_CONFIG_S_DRIVER_OK : 0;
+    n->vm_running = running;
     /* This is called when vm is started/stopped,
-     * it will start/stop vhost backend if * appropriate
+     * it will start/stop vhost backend if appropriate
      * e.g. after migration. */
-    virtio_net_set_status(&n->vdev, n->vdev.status & status);
+    virtio_net_set_status(&n->vdev, n->vdev.status);
 }
 
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
@@ -951,9 +949,8 @@  void virtio_net_exit(VirtIODevice *vdev)
     VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
     qemu_del_vm_change_state_handler(n->vmstate);
 
-    if (n->vhost_started) {
-        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
-    }
+    /* This will stop vhost backend if appropriate. */
+    virtio_net_set_status(vdev, 0);
 
     qemu_purge_queued_packets(&n->nic->nc);