| Message ID | 20100927164130.GA7211@redhat.com |
|---|---|
| State | New |
| Headers | show |
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); >
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); > > > >
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); > > > > > > >
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!
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);
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(-)