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

[Stefan Hajnoczi]

On Tue, Feb 09, 2021 at 04:37:56PM +0100, Eugenio Pérez wrote:
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index ac963bf23d..884818b109 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -55,6 +55,8 @@ struct vhost_iommu {
>      QLIST_ENTRY(vhost_iommu) iommu_next;
>  };
>  
> +typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;

There is already another declaration in
hw/virtio/vhost-shadow-virtqueue.h. Should vhost.h include
vhost-shadow-virtqueue.h?

This is becoming confusing:
1. typedef in vhost-shadow-virtqueue.h
2. typedef in vhost.h
3. typedef in vhost-shadow-virtqueue.c

3 typedefs is a bit much :). I suggest:
1. typedef in vhost-shadow-virtqueue.h
2. #include "vhost-shadow-virtqueue.h" in vhost.h
3. struct VhostShadowVirtqueue (no typedef redefinition) in 
vhost-shadow-virtqueue.c

That should make the code easier to understand, navigate with tools, and
if a change is made (e.g. renaming the struct) then it won't be
necessary to change things in 3 places.

> +
>  typedef struct VhostDevConfigOps {
>      /* Vhost device config space changed callback
>       */
> @@ -83,7 +85,9 @@ struct vhost_dev {
>      uint64_t backend_cap;
>      bool started;
>      bool log_enabled;
> +    bool sw_lm_enabled;

Rename to shadow_vqs_enabled?

>      uint64_t log_size;
> +    VhostShadowVirtqueue **shadow_vqs;
>      Error *migration_blocker;
>      const VhostOps *vhost_ops;
>      void *opaque;
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> b/hw/virtio/vhost-shadow-virtqueue.c
> index b5d2645ae0..01f282d434 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -8,9 +8,12 @@
>   */
>  
>  #include "hw/virtio/vhost-shadow-virtqueue.h"
> +#include "hw/virtio/vhost.h"
> +
> +#include "standard-headers/linux/vhost_types.h"
>  
>  #include "qemu/error-report.h"
> -#include "qemu/event_notifier.h"
> +#include "qemu/main-loop.h"
>  
>  /* Shadow virtqueue to relay notifications */
>  typedef struct VhostShadowVirtqueue {
> @@ -18,8 +21,95 @@ typedef struct VhostShadowVirtqueue {
>      EventNotifier kick_notifier;
>      /* Shadow call notifier, sent to vhost */
>      EventNotifier call_notifier;
> +
> +    /* Borrowed virtqueue's guest to host notifier. */
> +    EventNotifier host_notifier;

The purpose of these EventNotifier fields is not completely clear to me.
Here is how I interpret the comments:

1. The vhost device is set up to use kick_notifier/call_notifier when
   the shadow vq is enabled.

2. host_notifier is the guest-visible vq's host notifier. This is set up
   when the shadow vq is enabled.

But I'm not confident this is correct. Maybe you could expand the
comment to make it clear what is happening?

> +
> +    /* Virtio queue shadowing */
> +    VirtQueue *vq;
>  } VhostShadowVirtqueue;
>  
> +/* Forward guest notifications */
> +static void vhost_handle_guest_kick(EventNotifier *n)
> +{
> +    VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue,
> +                                             host_notifier);
> +
> +    if (event_notifier_test_and_clear(n)) {
> +        event_notifier_set(&svq->kick_notifier);
> +    }
> +}

This function looks incomplete. You can make review easier by indicating
the state of the code:

  /* TODO pop requests from vq and put them onto vhost vq */

I'm not sure why it's useful to include this incomplete function in the
patch. Maybe the host notifier is already intercepted by the
guest-visible vq is still mapped directly to the vhost vq so this works?
An explanation in comments or the commit description would be helpful.

> +
> +/*
> + * Start shadow virtqueue operation.
> + * @dev vhost device
> + * @hidx vhost virtqueue index
> + * @svq Shadow Virtqueue
> + *
> + * Run in RCU context
> + */
> +bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev,
> +                               unsigned idx,
> +                               VhostShadowVirtqueue *svq)
> +{
> +    EventNotifier *vq_host_notifier = 
> virtio_queue_get_host_notifier(svq->vq);
> +    struct vhost_vring_file kick_file = {
> +        .index = idx,
> +        .fd = event_notifier_get_fd(&svq->kick_notifier),
> +    };
> +    int r;
> +
> +    /* Check that notifications are still going directly to vhost dev */
> +    assert(virtio_queue_host_notifier_status(svq->vq));
> +
> +    event_notifier_init_fd(&svq->host_notifier,
> +                           event_notifier_get_fd(vq_host_notifier));
> +    event_notifier_set_handler(&svq->host_notifier, vhost_handle_guest_kick);

If I understand correctly svq->host_notifier only exists as an easy way
to use container_of() in vhost_handle_guest_kick?

svq->host_notifier does not actually own the fd and therefore
event_notifier_cleanup() must never be called on it?

Please document this.

> +
> +    r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file);
> +    if (unlikely(r != 0)) {
> +        error_report("Couldn't set kick fd: %s", strerror(errno));
> +        goto err_set_vring_kick;
> +    }
> +
> +    /* Check for pending notifications from the guest */
> +    vhost_handle_guest_kick(&svq->host_notifier);
> +
> +    return true;

host_notifier is still registered with the vhost device so now the
kernel vhost thread and QEMU are both monitoring the ioeventfd at the
same time? Did I miss a vhost_set_vring_call() somewhere?

> +
> +err_set_vring_kick:
> +    event_notifier_set_handler(&svq->host_notifier, NULL);
> +
> +    return false;
> +}
> +
> +/*
> + * Stop shadow virtqueue operation.
> + * @dev vhost device
> + * @idx vhost queue index
> + * @svq Shadow Virtqueue
> + *
> + * Run in RCU context
> + */
> +void vhost_shadow_vq_stop_rcu(struct vhost_dev *dev,
> +                              unsigned idx,
> +                              VhostShadowVirtqueue *svq)
> +{
> +    EventNotifier *vq_host_notifier = 
> virtio_queue_get_host_notifier(svq->vq);
> +    struct vhost_vring_file kick_file = {
> +        .index = idx,
> +        .fd = event_notifier_get_fd(vq_host_notifier),
> +    };
> +    int r;
> +
> +    /* Restore vhost kick */
> +    r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file);
> +    /* Cannot do a lot of things */
> +    assert(r == 0);
> +
> +    event_notifier_set_handler(&svq->host_notifier, NULL);

It may be necessary to call event_notifier_set(vq_host_notifier) before
vhost_set_vring_kick() so that the vhost kernel thread looks at the
vring immediately. That covers the case where svq->kick_notifier was
just set but not yet handled by the vhost kernel thread.

I'm not 100% sure this race condition can occur, but couldn't find
anything that prevents it.

> +err:
> +    for (; idx >= 0; --idx) {
> +        vhost_shadow_vq_free(dev->shadow_vqs[idx]);
> +    }
> +    g_free(dev->shadow_vqs[idx]);

Should this be g_free(dev->shadow_vqs)?

signature.asc
Description: PGP signature