Re: [PATCH v2 02/14] vhost: Add Shadow VirtQueue kick forwarding capabilities

[Eugenio Perez Martin]

On Thu, Mar 3, 2022 at 8:12 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/3/2 上午2:49, Eugenio Perez Martin 写道:
> > On Mon, Feb 28, 2022 at 3:57 AM Jason Wang<jasowang@redhat.com>  wrote:
> >> 在 2022/2/27 下午9:40, Eugenio Pérez 写道:
> >>> At this mode no buffer forwarding will be performed in SVQ mode: Qemu
> >>> will just forward the guest's kicks to the device.
> >>>
> >>> Host memory notifiers regions are left out for simplicity, and they will
> >>> not be addressed in this series.
> >>>
> >>> Signed-off-by: Eugenio Pérez<eperezma@redhat.com>
> >>> ---
> >>>    hw/virtio/vhost-shadow-virtqueue.h |  14 +++
> >>>    include/hw/virtio/vhost-vdpa.h     |   4 +
> >>>    hw/virtio/vhost-shadow-virtqueue.c |  52 +++++++++++
> >>>    hw/virtio/vhost-vdpa.c             | 145 ++++++++++++++++++++++++++++-
> >>>    4 files changed, 213 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> >>> b/hw/virtio/vhost-shadow-virtqueue.h
> >>> index f1519e3c7b..1cbc87d5d8 100644
> >>> --- a/hw/virtio/vhost-shadow-virtqueue.h
> >>> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> >>> @@ -18,8 +18,22 @@ typedef struct VhostShadowVirtqueue {
> >>>        EventNotifier hdev_kick;
> >>>        /* Shadow call notifier, sent to vhost */
> >>>        EventNotifier hdev_call;
> >>> +
> >>> +    /*
> >>> +     * Borrowed virtqueue's guest to host notifier. To borrow it in this 
> >>> event
> >>> +     * notifier allows to recover the VhostShadowVirtqueue from the 
> >>> event loop
> >>> +     * easily. If we use the VirtQueue's one, we don't have an easy way 
> >>> to
> >>> +     * retrieve VhostShadowVirtqueue.
> >>> +     *
> >>> +     * So shadow virtqueue must not clean it, or we would lose VirtQueue 
> >>> one.
> >>> +     */
> >>> +    EventNotifier svq_kick;
> >>>    } VhostShadowVirtqueue;
> >>>
> >>> +void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int 
> >>> svq_kick_fd);
> >>> +
> >>> +void vhost_svq_stop(VhostShadowVirtqueue *svq);
> >>> +
> >>>    VhostShadowVirtqueue *vhost_svq_new(void);
> >>>
> >>>    void vhost_svq_free(gpointer vq);
> >>> diff --git a/include/hw/virtio/vhost-vdpa.h 
> >>> b/include/hw/virtio/vhost-vdpa.h
> >>> index 3ce79a646d..009a9f3b6b 100644
> >>> --- a/include/hw/virtio/vhost-vdpa.h
> >>> +++ b/include/hw/virtio/vhost-vdpa.h
> >>> @@ -12,6 +12,8 @@
> >>>    #ifndef HW_VIRTIO_VHOST_VDPA_H
> >>>    #define HW_VIRTIO_VHOST_VDPA_H
> >>>
> >>> +#include <gmodule.h>
> >>> +
> >>>    #include "hw/virtio/virtio.h"
> >>>    #include "standard-headers/linux/vhost_types.h"
> >>>
> >>> @@ -27,6 +29,8 @@ typedef struct vhost_vdpa {
> >>>        bool iotlb_batch_begin_sent;
> >>>        MemoryListener listener;
> >>>        struct vhost_vdpa_iova_range iova_range;
> >>> +    bool shadow_vqs_enabled;
> >>> +    GPtrArray *shadow_vqs;
> >>>        struct vhost_dev *dev;
> >>>        VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> >>>    } VhostVDPA;
> >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> >>> b/hw/virtio/vhost-shadow-virtqueue.c
> >>> index 019cf1950f..a5d0659f86 100644
> >>> --- a/hw/virtio/vhost-shadow-virtqueue.c
> >>> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> >>> @@ -11,6 +11,56 @@
> >>>    #include "hw/virtio/vhost-shadow-virtqueue.h"
> >>>
> >>>    #include "qemu/error-report.h"
> >>> +#include "qemu/main-loop.h"
> >>> +#include "linux-headers/linux/vhost.h"
> >>> +
> >>> +/** Forward guest notifications */
> >>> +static void vhost_handle_guest_kick(EventNotifier *n)
> >>> +{
> >>> +    VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue,
> >>> +                                             svq_kick);
> >>> +    event_notifier_test_and_clear(n);
> >>> +    event_notifier_set(&svq->hdev_kick);
> >>> +}
> >>> +
> >>> +/**
> >>> + * Set a new file descriptor for the guest to kick the SVQ and notify 
> >>> for avail
> >>> + *
> >>> + * @svq          The svq
> >>> + * @svq_kick_fd  The svq kick fd
> >>> + *
> >>> + * Note that the SVQ will never close the old file descriptor.
> >>> + */
> >>> +void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int 
> >>> svq_kick_fd)
> >>> +{
> >>> +    EventNotifier *svq_kick = &svq->svq_kick;
> >>> +    bool poll_stop = VHOST_FILE_UNBIND != 
> >>> event_notifier_get_fd(svq_kick);
> >> I wonder if this is robust. E.g is there any chance that may end up with
> >> both poll_stop and poll_start are false?
> >>
> > I cannot make that happen in qemu, but the function supports that case
> > well: It will do nothing. It's more or less the same code as used in
> > the vhost kernel, and is the expected behaviour if you send two
> > VHOST_FILE_UNBIND one right after another to me.
>
>
> I would think it's just stop twice.
>
>
> >
> >> If not, can we simple detect poll_stop as below and treat !poll_start
> >> and poll_stop?
> >>
> > I'm not sure what does it add. Is there an unexpected consequence with
> > the current do-nothing behavior I've missed?
>
>
> I'm not sure, but it feels odd if poll_start is not the reverse value of
> poll_stop.
>

If we want to not to restrict the inputs, we need to handle for situations:

a) old_fd = -1, new_fd = -1,

This is the situation you described, and it's basically a no-op.
poll_stop == poll_start == false.

If we make poll_stop = true and poll_stop = false, we call
event_notifier_set_handler(-1, ...). Hopefully it will return just an
error.

If we make poll_stop = false and poll_stop = true, we are calling
event_notifier_set(-1) and event_notifier_set_handler(-1,
poll_callback). Same situation, hopefully an error, but unexpected.

b) old_fd = -1, new_fd = >-1,

We need to start polling the new_fd. No need for stop polling the
old_fd, since we are not polling it actually.

c) old_fd = >-1, new_fd = >-1,

We need to stop polling the old_fd and start polling the new one.

If we make poll_stop = true and poll_stop = false, we don't register a
new polling function for the new kick_fd so we will miss guest's
kicks.

If we make poll_stop = false and poll_stop = true, we keep polling the
old file descriptor too, so whatever it gets assigned to could call
vhost_handle_guest_kick if it does not override poll callback.

We *could* detect if old_fd == new_fd so we skip all the work, but I
think it is not worth it to complicate the code, since we're only
being called with the kick_fd at dev start.

d) c) old_fd = >-1, new_fd = -1,

We need to stop polling, or we could get invalid kicks callbacks if it
gets writed after this. No need to poll anything beyond this.

> Thanks
>
>