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

[Jason Wang]

On Thu, Mar 3, 2022 at 5:25 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> 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.

I see, thanks for the clarification.

>
> > Thanks
> >
> >
>