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

[Jason Wang]

在 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.

Thanks