[Qemu-devel] Re: [PATCH] ioeventfd: minor fixups

[Michael S. Tsirkin]

On Wed, Dec 29, 2010 at 04:52:46PM +0200, Michael S. Tsirkin wrote:
> This is on top of the ioeventfd patches, and
> fixes two potential issues when ioeventfd is mixed with vhost-net:
> - ioeventfd could start running then get stopped for vhost-net.
>   For example, vm state change handlers run in no particular order.
>   This would be hard to debug. Prevent this by always running state
>   callback before ioeventfd start and after ioeventfd stop.
> - Separate the flags for user-requested ioeventfd and for ioeventfd
>   being overridden by vhost backend.
>   This will make it possible to enable/disable vhost depending on
>   guest behaviour.
> 
> I'll probably split this patch in two, and merge into the
> appropriate patches in the ioeventfd series.
> 
> Compile-tested only so far, would appreciate feedback/test reports.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>


Stefan - just to clarify, I'm waiting for you ack
to merge this + the ioeventfd patches.

> diff --git a/hw/vhost.c b/hw/vhost.c
> index 6082da2..1d09ed0 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -630,6 +630,17 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
>  int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>  {
>      int i, r;
> +    /* A binding must support vmstate notifiers (for migration to work) */
> +    if (!vdev->binding->vmstate_change) {
> +        fprintf(stderr, "binding does not support vmstate notifiers\n");
> +        r = -ENOSYS;
> +        goto fail;
> +    }
> +    if (!vdev->binding->set_guest_notifiers) {
> +        fprintf(stderr, "binding does not support guest notifiers\n");
> +        r = -ENOSYS;
> +        goto fail;
> +    }
>      if (!vdev->binding->set_guest_notifiers) {
>          fprintf(stderr, "binding does not support guest notifiers\n");
>          r = -ENOSYS;
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ec1bf8d..ccb3e63 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -54,8 +54,6 @@ typedef struct VirtIONet
>      uint8_t nouni;
>      uint8_t nobcast;
>      uint8_t vhost_started;
> -    bool vm_running;
> -    VMChangeStateEntry *vmstate;
>      struct {
>          int in_use;
>          int first_multi;
> @@ -102,7 +100,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, 
> const uint8_t *config)
>  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;
> +        (n->status & VIRTIO_NET_S_LINK_UP) && n->vdev.vm_running;
>  }
>  
>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> @@ -453,7 +451,7 @@ static void virtio_net_handle_rx(VirtIODevice *vdev, 
> VirtQueue *vq)
>  static int virtio_net_can_receive(VLANClientState *nc)
>  {
>      VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
> -    if (!n->vm_running) {
> +    if (!n->vdev.vm_running) {
>          return 0;
>      }
>  
> @@ -708,7 +706,7 @@ static int32_t virtio_net_flush_tx(VirtIONet *n, 
> VirtQueue *vq)
>          return num_packets;
>      }
>  
> -    assert(n->vm_running);
> +    assert(n->vdev.vm_running);
>  
>      if (n->async_tx.elem.out_num) {
>          virtio_queue_set_notification(n->tx_vq, 0);
> @@ -769,7 +767,7 @@ 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) {
> +    if (!n->vdev.vm_running) {
>          n->tx_waiting = 1;
>          return;
>      }
> @@ -796,7 +794,7 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, 
> VirtQueue *vq)
>      }
>      n->tx_waiting = 1;
>      /* This happens when device was stopped but VCPU wasn't. */
> -    if (!n->vm_running) {
> +    if (!n->vdev.vm_running) {
>          return;
>      }
>      virtio_queue_set_notification(vq, 0);
> @@ -806,7 +804,7 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, 
> VirtQueue *vq)
>  static void virtio_net_tx_timer(void *opaque)
>  {
>      VirtIONet *n = opaque;
> -    assert(n->vm_running);
> +    assert(n->vdev.vm_running);
>  
>      n->tx_waiting = 0;
>  
> @@ -823,7 +821,7 @@ static void virtio_net_tx_bh(void *opaque)
>      VirtIONet *n = opaque;
>      int32_t ret;
>  
> -    assert(n->vm_running);
> +    assert(n->vdev.vm_running);
>  
>      n->tx_waiting = 0;
>  
> @@ -988,16 +986,6 @@ static NetClientInfo net_virtio_info = {
>      .link_status_changed = virtio_net_set_link_status,
>  };
>  
> -static void virtio_net_vmstate_change(void *opaque, int running, int reason)
> -{
> -    VirtIONet *n = opaque;
> -    n->vm_running = running;
> -    /* This is called when vm is started/stopped,
> -     * it will start/stop vhost backend if appropriate
> -     * e.g. after migration. */
> -    virtio_net_set_status(&n->vdev, n->vdev.status);
> -}
> -
>  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
>                                virtio_net_conf *net)
>  {
> @@ -1052,7 +1040,6 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf 
> *conf,
>      n->qdev = dev;
>      register_savevm(dev, "virtio-net", -1, VIRTIO_NET_VM_VERSION,
>                      virtio_net_save, virtio_net_load, n);
> -    n->vmstate = qemu_add_vm_change_state_handler(virtio_net_vmstate_change, 
> n);
>  
>      add_boot_device_path(conf->bootindex, dev, "/address@hidden");
>  
> @@ -1062,7 +1049,6 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf 
> *conf,
>  void virtio_net_exit(VirtIODevice *vdev)
>  {
>      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> -    qemu_del_vm_change_state_handler(n->vmstate);
>  
>      /* This will stop vhost backend if appropriate. */
>      virtio_net_set_status(vdev, 0);
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index b2181fc..75b5e53 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -112,8 +112,8 @@ typedef struct {
>      /* Max. number of ports we can have for a the virtio-serial device */
>      uint32_t max_virtserial_ports;
>      virtio_net_conf net;
> +    bool ioeventfd_disabled;
>      bool ioeventfd_started;
> -    VMChangeStateEntry *vm_change_state_entry;
>  } VirtIOPCIProxy;
>  
>  /* virtio device */
> @@ -251,6 +251,7 @@ static int virtio_pci_start_ioeventfd(VirtIOPCIProxy 
> *proxy)
>      int n, r;
>  
>      if (!(proxy->flags & VIRTIO_PCI_FLAG_USE_IOEVENTFD) ||
> +        proxy->ioeventfd_disabled ||
>          proxy->ioeventfd_started) {
>          return 0;
>      }
> @@ -280,7 +281,7 @@ assign_error:
>          virtio_pci_set_host_notifier_internal(proxy, n, false);
>      }
>      proxy->ioeventfd_started = false;
> -    proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
> +    proxy->ioeventfd_disabled = true;
>      return r;
>  }
>  
> @@ -350,13 +351,16 @@ static void virtio_ioport_write(void *opaque, uint32_t 
> addr, uint32_t val)
>          virtio_queue_notify(vdev, val);
>          break;
>      case VIRTIO_PCI_STATUS:
> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> -            virtio_pci_start_ioeventfd(proxy);
> -        } else {
> +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>              virtio_pci_stop_ioeventfd(proxy);
>          }
>  
>          virtio_set_status(vdev, val & 0xFF);
> +
> +        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> +            virtio_pci_start_ioeventfd(proxy);
> +        }
> +
>          if (vdev->status == 0) {
>              virtio_reset(proxy->vdev);
>              msix_unuse_all_vectors(&proxy->pci_dev);
> @@ -618,22 +622,24 @@ static int virtio_pci_set_host_notifier(void *opaque, 
> int n, bool assign)
>      /* Stop using ioeventfd for virtqueue kick if the device starts using 
> host
>       * notifiers.  This makes it easy to avoid stepping on each others' toes.
>       */
> -    if (proxy->ioeventfd_started) {
> +    proxy->ioeventfd_disabled = assign;
> +    if (assign) {
>          virtio_pci_stop_ioeventfd(proxy);
> -        proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
>      }
>  
>      return virtio_pci_set_host_notifier_internal(proxy, n, assign);
>  }
>  
> -static void virtio_pci_vm_change_state_handler(void *opaque, int running, 
> int reason)
> +static void virtio_pci_vmstate_change(void *opaque, bool running)
>  {
>      VirtIOPCIProxy *proxy = opaque;
>  
>      if (running && (proxy->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +        virtio_set_status(proxy->vdev, proxy->vdev->status);
>          virtio_pci_start_ioeventfd(proxy);
>      } else {
>          virtio_pci_stop_ioeventfd(proxy);
> +        virtio_set_status(proxy->vdev, proxy->vdev->status);
>      }
>  }
>  
> @@ -646,6 +652,7 @@ static const VirtIOBindings virtio_pci_bindings = {
>      .get_features = virtio_pci_get_features,
>      .set_host_notifier = virtio_pci_set_host_notifier,
>      .set_guest_notifiers = virtio_pci_set_guest_notifiers,
> +    .vmstate_change = virtio_pci_vmstate_change,
>  };
>  
>  static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
> @@ -699,9 +706,6 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, 
> VirtIODevice *vdev,
>      proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>      proxy->host_features = vdev->get_features(vdev, proxy->host_features);
>  
> -    proxy->vm_change_state_entry = qemu_add_vm_change_state_handler(
> -                                        virtio_pci_vm_change_state_handler,
> -                                        proxy);
>  }
>  
>  static int virtio_blk_init_pci(PCIDevice *pci_dev)
> @@ -729,9 +733,6 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
>  
>  static int virtio_exit_pci(PCIDevice *pci_dev)
>  {
> -    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> -
> -    qemu_del_vm_change_state_handler(proxy->vm_change_state_entry);
>      return msix_uninit(pci_dev);
>  }
>  
> diff --git a/hw/virtio.c b/hw/virtio.c
> index e40296a..13c3373 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -751,11 +751,21 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>  
>  void virtio_cleanup(VirtIODevice *vdev)
>  {
> +    qemu_del_vm_change_state_handler(vdev->vmstate);
>      if (vdev->config)
>          qemu_free(vdev->config);
>      qemu_free(vdev->vq);
>  }
>  
> +static void virtio_vmstate_change(void *opaque, int running, int reason)
> +{
> +    VirtIODevice *vdev = opaque;
> +    vdev->vm_running = running;
> +    if (vdev->binding->vmstate_change) {
> +        vdev->binding->vmstate_change(vdev->binding_opaque, running);
> +    }
> +}
> +
>  VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
>                                   size_t config_size, size_t struct_size)
>  {
> @@ -782,6 +792,8 @@ VirtIODevice *virtio_common_init(const char *name, 
> uint16_t device_id,
>      else
>          vdev->config = NULL;
>  
> +    vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change, 
> vdev);
> +
>      return vdev;
>  }
>  
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 5ae521c..d8546d5 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -95,6 +95,7 @@ typedef struct {
>      unsigned (*get_features)(void * opaque);
>      int (*set_guest_notifiers)(void * opaque, bool assigned);
>      int (*set_host_notifier)(void * opaque, int n, bool assigned);
> +    void (*vmstate_change)(void * opaque, bool running);
>  } VirtIOBindings;
>  
>  #define VIRTIO_PCI_QUEUE_MAX 64
> @@ -123,6 +124,8 @@ struct VirtIODevice
>      const VirtIOBindings *binding;
>      void *binding_opaque;
>      uint16_t device_id;
> +    bool vm_running;
> +    VMChangeStateEntry *vmstate;
>  };
>  
>  static inline void virtio_set_status(VirtIODevice *vdev, uint8_t val)