Re: [RFC v2 06/13] vhost: Route host->guest notification through shadow virtqueue

[Jason Wang <jasowang@redhat.com>]

在 2021/3/16 上午3:48, Eugenio Pérez 写道:
> On one hand it uses a mutex to synchronize guest masking with SVQ start
> and stop, because otherwise guest mask could race with the SVQ
> stop code, sending an incorrect call notifier to vhost device. This
> would prevent further communication.


Is this becuase of the OOB monitor? If yes, can we simply drop the QMP 
command and introduce cli paramter for vhost backend?

Thanks


>
> On the other hand it needs to add an event to synchronize guest
> unmasking with call handling. Not doing that way could cause the guest
> to receive notifications after its unmask call. This could be done
> through the mutex but the event solution is cheaper for the buffer
> forwarding.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-shadow-virtqueue.h |   3 +
>   include/hw/virtio/vhost.h          |   1 +
>   hw/virtio/vhost-shadow-virtqueue.c | 127 +++++++++++++++++++++++++++++
>   hw/virtio/vhost.c                  |  29 ++++++-
>   4 files changed, 157 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> index c891c6510d..2ca4b92b12 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -17,6 +17,9 @@
>   
>   typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
>   
> +void vhost_shadow_vq_mask(VhostShadowVirtqueue *svq, EventNotifier *masked);
> +void vhost_shadow_vq_unmask(VhostShadowVirtqueue *svq);
> +
>   bool vhost_shadow_vq_start(struct vhost_dev *dev,
>                              unsigned idx,
>                              VhostShadowVirtqueue *svq);
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 7ffdf9aea0..2f556bd3d5 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -29,6 +29,7 @@ struct vhost_virtqueue {
>       unsigned long long used_phys;
>       unsigned used_size;
>       bool notifier_is_masked;
> +    QemuRecMutex masked_mutex;
>       EventNotifier masked_notifier;
>       struct vhost_dev *dev;
>   };
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 3e43399e9c..8f6ffa729a 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -32,8 +32,22 @@ typedef struct VhostShadowVirtqueue {
>        */
>       EventNotifier host_notifier;
>   
> +    /* (Possible) masked notifier */
> +    struct {
> +        EventNotifier *n;
> +
> +        /*
> +         * Event to confirm unmasking.
> +         * set when the masked notifier has no uses
> +         */
> +        QemuEvent is_free;
> +    } masked_notifier;
> +
>       /* Virtio queue shadowing */
>       VirtQueue *vq;
> +
> +    /* Virtio device */
> +    VirtIODevice *vdev;
>   } VhostShadowVirtqueue;
>   
>   /* Forward guest notifications */
> @@ -49,6 +63,70 @@ static void vhost_handle_guest_kick(EventNotifier *n)
>       event_notifier_set(&svq->kick_notifier);
>   }
>   
> +/* Forward vhost notifications */
> +static void vhost_shadow_vq_handle_call_no_test(EventNotifier *n)
> +{
> +    VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue,
> +                                             call_notifier);
> +    EventNotifier *masked_notifier;
> +
> +    /* Signal start of using masked notifier */
> +    qemu_event_reset(&svq->masked_notifier.is_free);
> +    masked_notifier = qatomic_load_acquire(&svq->masked_notifier.n);
> +    if (!masked_notifier) {
> +        qemu_event_set(&svq->masked_notifier.is_free);
> +    }
> +
> +    if (!masked_notifier) {
> +        unsigned n = virtio_get_queue_index(svq->vq);
> +        virtio_queue_invalidate_signalled_used(svq->vdev, n);
> +        virtio_notify_irqfd(svq->vdev, svq->vq);
> +    } else {
> +        event_notifier_set(svq->masked_notifier.n);
> +    }
> +
> +    if (masked_notifier) {
> +        /* Signal not using it anymore */
> +        qemu_event_set(&svq->masked_notifier.is_free);
> +    }
> +}
> +
> +static void vhost_shadow_vq_handle_call(EventNotifier *n)
> +{
> +
> +    if (likely(event_notifier_test_and_clear(n))) {
> +        vhost_shadow_vq_handle_call_no_test(n);
> +    }
> +}
> +
> +/*
> + * Mask the shadow virtqueue.
> + *
> + * It can be called from a guest masking vmexit or shadow virtqueue start
> + * through QMP.
> + *
> + * @vq Shadow virtqueue
> + * @masked Masked notifier to signal instead of guest
> + */
> +void vhost_shadow_vq_mask(VhostShadowVirtqueue *svq, EventNotifier *masked)
> +{
> +    qatomic_store_release(&svq->masked_notifier.n, masked);
> +}
> +
> +/*
> + * Unmask the shadow virtqueue.
> + *
> + * It can be called from a guest unmasking vmexit or shadow virtqueue start
> + * through QMP.
> + *
> + * @vq Shadow virtqueue
> + */
> +void vhost_shadow_vq_unmask(VhostShadowVirtqueue *svq)
> +{
> +    qatomic_store_release(&svq->masked_notifier.n, NULL);
> +    qemu_event_wait(&svq->masked_notifier.is_free);
> +}
> +
>   /*
>    * Restore the vhost guest to host notifier, i.e., disables svq effect.
>    */
> @@ -103,8 +181,39 @@ bool vhost_shadow_vq_start(struct vhost_dev *dev,
>           goto err_set_vring_kick;
>       }
>   
> +    /* Set vhost call */
> +    file.fd = event_notifier_get_fd(&svq->call_notifier),
> +    r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> +    if (unlikely(r != 0)) {
> +        error_report("Couldn't set call fd: %s", strerror(errno));
> +        goto err_set_vring_call;
> +    }
> +
> +
> +    /*
> +     * Lock to avoid a race condition between guest setting masked status and
> +     * us.
> +     */
> +    QEMU_LOCK_GUARD(&dev->vqs[idx].masked_mutex);
> +    /* Set shadow vq -> guest notifier */
> +    assert(dev->shadow_vqs_enabled);
> +    vhost_virtqueue_mask(dev, dev->vdev, dev->vq_index + idx,
> +                         dev->vqs[idx].notifier_is_masked);
> +
> +    if (dev->vqs[idx].notifier_is_masked &&
> +               event_notifier_test_and_clear(&dev->vqs[idx].masked_notifier)) {
> +        /* Check for pending notifications from the device */
> +        vhost_shadow_vq_handle_call_no_test(&svq->call_notifier);
> +    }
> +
>       return true;
>   
> +err_set_vring_call:
> +    r = vhost_shadow_vq_restore_vdev_host_notifier(dev, idx, svq);
> +    if (unlikely(r < 0)) {
> +        error_report("Couldn't restore vq kick fd: %s", strerror(-r));
> +    }
> +
>   err_set_vring_kick:
>       event_notifier_set_handler(&svq->host_notifier, NULL);
>   
> @@ -126,7 +235,19 @@ void vhost_shadow_vq_stop(struct vhost_dev *dev,
>           error_report("Couldn't restore vq kick fd: %s", strerror(-r));
>       }
>   
> +    assert(!dev->shadow_vqs_enabled);
> +
>       event_notifier_set_handler(&svq->host_notifier, NULL);
> +
> +    /*
> +     * Lock to avoid a race condition between guest setting masked status and
> +     * us.
> +     */
> +    QEMU_LOCK_GUARD(&dev->vqs[idx].masked_mutex);
> +
> +    /* Restore vhost call */
> +    vhost_virtqueue_mask(dev, dev->vdev, dev->vq_index + idx,
> +                         dev->vqs[idx].notifier_is_masked);
>   }
>   
>   /*
> @@ -154,6 +275,10 @@ VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx)
>       }
>   
>       svq->vq = virtio_get_queue(dev->vdev, vq_idx);
> +    svq->vdev = dev->vdev;
> +    event_notifier_set_handler(&svq->call_notifier,
> +                               vhost_shadow_vq_handle_call);
> +    qemu_event_init(&svq->masked_notifier.is_free, true);
>       return g_steal_pointer(&svq);
>   
>   err_init_call_notifier:
> @@ -168,7 +293,9 @@ err_init_kick_notifier:
>    */
>   void vhost_shadow_vq_free(VhostShadowVirtqueue *vq)
>   {
> +    qemu_event_destroy(&vq->masked_notifier.is_free);
>       event_notifier_cleanup(&vq->kick_notifier);
> +    event_notifier_set_handler(&vq->call_notifier, NULL);
>       event_notifier_cleanup(&vq->call_notifier);
>       g_free(vq);
>   }
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 4858a35df6..eab3e334f2 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1224,7 +1224,8 @@ static int vhost_sw_live_migration_stop(struct vhost_dev *dev)
>   {
>       int idx;
>   
> -    dev->shadow_vqs_enabled = false;
> +    /* Can be read by vhost_virtqueue_mask, from vm exit */
> +    qatomic_store_release(&dev->shadow_vqs_enabled, false);
>   
>       for (idx = 0; idx < dev->nvqs; ++idx) {
>           vhost_shadow_vq_stop(dev, idx, dev->shadow_vqs[idx]);
> @@ -1248,7 +1249,8 @@ static int vhost_sw_live_migration_start(struct vhost_dev *dev)
>           }
>       }
>   
> -    dev->shadow_vqs_enabled = true;
> +    /* Can be read by vhost_virtqueue_mask, from vm exit */
> +    qatomic_store_release(&dev->shadow_vqs_enabled, true);
>       for (idx = 0; idx < dev->nvqs; ++idx) {
>           bool ok = vhost_shadow_vq_start(dev, idx, dev->shadow_vqs[idx]);
>           if (unlikely(!ok)) {
> @@ -1259,7 +1261,7 @@ static int vhost_sw_live_migration_start(struct vhost_dev *dev)
>       return 0;
>   
>   err_start:
> -    dev->shadow_vqs_enabled = false;
> +    qatomic_store_release(&dev->shadow_vqs_enabled, false);
>       for (stop_idx = 0; stop_idx < idx; stop_idx++) {
>           vhost_shadow_vq_stop(dev, idx, dev->shadow_vqs[stop_idx]);
>       }
> @@ -1343,6 +1345,7 @@ static int vhost_virtqueue_init(struct vhost_dev *dev,
>           goto fail_call;
>       }
>   
> +    qemu_rec_mutex_init(&vq->masked_mutex);
>       vq->dev = dev;
>   
>       return 0;
> @@ -1353,6 +1356,7 @@ fail_call:
>   
>   static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
>   {
> +    qemu_rec_mutex_destroy(&vq->masked_mutex);
>       event_notifier_cleanup(&vq->masked_notifier);
>   }
>   
> @@ -1591,6 +1595,25 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
>       /* should only be called after backend is connected */
>       assert(hdev->vhost_ops);
>   
> +    /* Avoid race condition with shadow virtqueue stop/start */
> +    QEMU_LOCK_GUARD(&hdev->vqs[index].masked_mutex);
> +
> +    /* Set by QMP thread, so using acquire semantics */
> +    if (qatomic_load_acquire(&hdev->shadow_vqs_enabled)) {
> +        if (mask) {
> +            vhost_shadow_vq_mask(hdev->shadow_vqs[index],
> +                                 &hdev->vqs[index].masked_notifier);
> +        } else {
> +            vhost_shadow_vq_unmask(hdev->shadow_vqs[index]);
> +        }
> +
> +        /*
> +         * Vhost call fd must remain the same since shadow vq is not polling
> +         * for changes
> +         */
> +        return;
> +    }
> +
>       if (mask) {
>           assert(vdev->use_guest_notifier_mask);
>           file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier);