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 <m...@redhat.com>
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, "/ethernet-...@0");
>
> @@ -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)
