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

[Jason Wang <jasowang@redhat.com>]

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;
 >  }
 >