Re: [PATCH v2 03/14] vhost: Add Shadow VirtQueue call forwarding capabilities

[Eugenio Perez Martin]

On Mon, Feb 28, 2022 at 4:18 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/2/27 下午9:41, Eugenio Pérez 写道:
> > This will make qemu aware of the device used buffers, allowing it to
> > write the guest memory with its contents if needed.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.h |  4 ++++
> >   hw/virtio/vhost-shadow-virtqueue.c | 34 ++++++++++++++++++++++++++++++
> >   hw/virtio/vhost-vdpa.c             | 31 +++++++++++++++++++++++++--
> >   3 files changed, 67 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> > b/hw/virtio/vhost-shadow-virtqueue.h
> > index 1cbc87d5d8..1d4c160d0a 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -28,9 +28,13 @@ typedef struct VhostShadowVirtqueue {
> >        * So shadow virtqueue must not clean it, or we would lose VirtQueue 
> > one.
> >        */
> >       EventNotifier svq_kick;
> > +
> > +    /* Guest's call notifier, where the SVQ calls guest. */
> > +    EventNotifier svq_call;
> >   } VhostShadowVirtqueue;
> >
> >   void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int 
> > svq_kick_fd);
> > +void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int 
> > call_fd);
> >
> >   void vhost_svq_stop(VhostShadowVirtqueue *svq);
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index a5d0659f86..54c701a196 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -23,6 +23,38 @@ static void vhost_handle_guest_kick(EventNotifier *n)
> >       event_notifier_set(&svq->hdev_kick);
> >   }
> >
> > +/* Forward vhost notifications */
> > +static void vhost_svq_handle_call(EventNotifier *n)
> > +{
> > +    VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue,
> > +                                             hdev_call);
> > +    event_notifier_test_and_clear(n);
> > +    event_notifier_set(&svq->svq_call);
> > +}
> > +
> > +/**
> > + * Set the call notifier for the SVQ to call the guest
> > + *
> > + * @svq Shadow virtqueue
> > + * @call_fd call notifier
> > + *
> > + * Called on BQL context.
> > + */
> > +void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int 
> > call_fd)
>
>
> I think we need to have consistent naming for both kick and call. Note
> that in patch 2 we had
>
> vhost_svq_set_svq_kick_fd
>
> Maybe it's better to use vhost_svq_set_guest_call_fd() here.
>

I think the same, I will replace it for the next version.

>
> > +{
> > +    if (call_fd == VHOST_FILE_UNBIND) {
> > +        /*
> > +         * Fail event_notifier_set if called handling device call.
> > +         *
> > +         * SVQ still needs device notifications, since it needs to keep
> > +         * forwarding used buffers even with the unbind.
> > +         */
> > +        memset(&svq->svq_call, 0, sizeof(svq->svq_call));
>
>
> I may miss something but shouldn't we stop polling svq_call here like
>
> event_notifier_set_handle(&svq->svq_call, false);
>

SVQ never polls that descriptor: It uses that descriptor to call (as
notify) the guest at vhost_svq_flush when SVQ uses descriptors.

svq_kick, svq_call: Descriptors that the guest send to SVQ
hdev_kick, hdev_call: Descriptors that qemu/SVQ send to the device.

I admit it is confusing when reading the code but I cannot come up
with a better naming. Maybe it helps to add a diagram at the top of
the file like:

+-------+-> svq_kick_fd ->+-----+-> hdev_kick ->+-----+
| Guest |                 | SVQ |               | Dev |
+-------+<- svq_call_fd <-+-----+<- hdev_call <-+-----+

Thanks!

> ?
>
> Thanks
>
>