Virtualization - Mar 2022 - [PATCH v2 02/14] vhost: Add Shadow VirtQueue kick forwarding capabilities

? 2022/3/2 ??2:49, Eugenio Perez Martin ??:
> On Mon, Feb 28, 2022 at 3:57 AM Jason Wang<jasowang at 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 at 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