Linux Virtualization - Oct 2021 - [RFC PATCH v4 11/20] vhost: Route host->guest notification through shadow virtqueue

? 2021/10/15 ??12:39, Eugenio Perez Martin ??:
> On Wed, Oct 13, 2021 at 5:47 AM Jason Wang <jasowang at redhat.com>
wrote:
>>
>> ? 2021/10/1 ??3:05, 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.
>>>
>>> Since the use of vhost_virtqueue_start can unmasks and discard call
>>> events, vhost_virtqueue_start should be modified in one of these
ways:
>>> * Split in two: One of them uses all logic to start a queue with no
>>>     side effects for the guest, and another one tha actually
assumes that
>>>     the guest has just started the device. Vdpa should use just the
>>>     former.
>>> * Actually store and check if the guest notifier is masked, and do
it
>>>     conditionally.
>>> * Left as it is, and duplicate all the logic in vhost-vdpa.
>>>
>>> Signed-off-by: Eugenio P?rez <eperezma at redhat.com>
>>> ---
>>>    hw/virtio/vhost-shadow-virtqueue.c | 19 +++++++++++++++
>>>    hw/virtio/vhost-vdpa.c             | 38
+++++++++++++++++++++++++++++-
>>>    2 files changed, 56 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c
b/hw/virtio/vhost-shadow-virtqueue.c
>>> index 21dc99ab5d..3fe129cf63 100644
>>> --- a/hw/virtio/vhost-shadow-virtqueue.c
>>> +++ b/hw/virtio/vhost-shadow-virtqueue.c
>>> @@ -53,6 +53,22 @@ static void
vhost_handle_guest_kick(EventNotifier *n)
>>>        event_notifier_set(&svq->kick_notifier);
>>>    }
>>>
>>> +/* Forward vhost notifications */
>>> +static void vhost_svq_handle_call_no_test(EventNotifier *n)
>>> +{
>>> +    VhostShadowVirtqueue *svq = container_of(n,
VhostShadowVirtqueue,
>>> +                                             call_notifier);
>>> +
>>> +    event_notifier_set(&svq->guest_call_notifier);
>>> +}
>>> +
>>> +static void vhost_svq_handle_call(EventNotifier *n)
>>> +{
>>> +    if (likely(event_notifier_test_and_clear(n))) {
>>> +        vhost_svq_handle_call_no_test(n);
>>> +    }
>>> +}
>>> +
>>>    /*
>>>     * Obtain the SVQ call notifier, where vhost device notifies SVQ
that there
>>>     * exists pending used buffers.
>>> @@ -180,6 +196,8 @@ VhostShadowVirtqueue *vhost_svq_new(struct
vhost_dev *dev, int idx)
>>>        }
>>>
>>>        svq->vq = virtio_get_queue(dev->vdev, vq_idx);
>>> +    event_notifier_set_handler(&svq->call_notifier,
>>> +                               vhost_svq_handle_call);
>>>        return g_steal_pointer(&svq);
>>>
>>>    err_init_call_notifier:
>>> @@ -195,6 +213,7 @@ err_init_kick_notifier:
>>>    void vhost_svq_free(VhostShadowVirtqueue *vq)
>>>    {
>>>        event_notifier_cleanup(&vq->kick_notifier);
>>> +    event_notifier_set_handler(&vq->call_notifier, NULL);
>>>        event_notifier_cleanup(&vq->call_notifier);
>>>        g_free(vq);
>>>    }
>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>> index bc34de2439..6c5f4c98b8 100644
>>> --- a/hw/virtio/vhost-vdpa.c
>>> +++ b/hw/virtio/vhost-vdpa.c
>>> @@ -712,13 +712,40 @@ static bool vhost_vdpa_svq_start_vq(struct
vhost_dev *dev, unsigned idx)
>>>    {
>>>        struct vhost_vdpa *v = dev->opaque;
>>>        VhostShadowVirtqueue *svq =
g_ptr_array_index(v->shadow_vqs, idx);
>>> -    return vhost_svq_start(dev, idx, svq);
>>> +    EventNotifier *vhost_call_notifier =
vhost_svq_get_svq_call_notifier(svq);
>>> +    struct vhost_vring_file vhost_call_file = {
>>> +        .index = idx + dev->vq_index,
>>> +        .fd = event_notifier_get_fd(vhost_call_notifier),
>>> +    };
>>> +    int r;
>>> +    bool b;
>>> +
>>> +    /* Set shadow vq -> guest notifier */
>>> +    assert(v->call_fd[idx]);
>>
>> We need aovid the asser() here. On which case we can hit this?
>>
> I would say that there is no way we can actually hit it, so let's
remove it.
>
>>> +    vhost_svq_set_guest_call_notifier(svq, v->call_fd[idx]);
>>> +
>>> +    b = vhost_svq_start(dev, idx, svq);
>>> +    if (unlikely(!b)) {
>>> +        return false;
>>> +    }
>>> +
>>> +    /* Set device -> SVQ notifier */
>>> +    r = vhost_vdpa_set_vring_dev_call(dev, &vhost_call_file);
>>> +    if (unlikely(r)) {
>>> +        error_report("vhost_vdpa_set_vring_call for shadow vq
failed");
>>> +        return false;
>>> +    }
>>
>> Similar to kick, do we need to set_vring_call() before
vhost_svq_start()?
>>
> It should not matter at this moment because the device should not be
> started at this point and device calls should not run
> vhost_svq_handle_call until BQL is released.

Yes, we stop virtqueue before.

>
> The "logic" of doing it after is to make clear that svq must be
fully
> initialized before processing device calls, even in the case that we
> extract SVQ in its own iothread or similar. But this could be done
> before vhost_svq_start for sure.
>
>>> +
>>> +    /* Check for pending calls */
>>> +    event_notifier_set(vhost_call_notifier);
>>
>> Interesting, can this result spurious interrupt?
>>
> This actually "queues" a vhost_svq_handle_call after the BQL
release,
> where the device should be fully reset. In that regard, if there are
> no used descriptors there will not be an irq raised to the guest. Does
> that answer the question? Or have I missed something?

Yes, please explain this in the comment.

>
>>> +    return true;
>>>    }
>>>
>>>    static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool
enable)
>>>    {
>>>        struct vhost_dev *hdev = v->dev;
>>>        unsigned n;
>>> +    int r;
>>>
>>>        if (enable == v->shadow_vqs_enabled) {
>>>            return hdev->nvqs;
>>> @@ -752,9 +779,18 @@ static unsigned vhost_vdpa_enable_svq(struct
vhost_vdpa *v, bool enable)
>>>        if (!enable) {
>>>            /* Disable all queues or clean up failed start */
>>>            for (n = 0; n < v->shadow_vqs->len; ++n) {
>>> +            struct vhost_vring_file file = {
>>> +                .index = vhost_vdpa_get_vq_index(hdev, n),
>>> +                .fd = v->call_fd[n],
>>> +            };
>>> +
>>> +            r = vhost_vdpa_set_vring_call(hdev, &file);
>>> +            assert(r == 0);
>>> +
>>>                unsigned vq_idx = vhost_vdpa_get_vq_index(hdev, n);
>>>                VhostShadowVirtqueue *svq =
g_ptr_array_index(v->shadow_vqs, n);
>>>                vhost_svq_stop(hdev, n, svq);
>>> +            /* TODO: This can unmask or override call fd! */
>>
>> I don't get this comment. Does this mean the current code can't
work
>> with mask_notifiers? If yes, this is something we need to fix.
>>
> Yes, but it will be addressed in the next series. I should have
> explained it bette here, sorry :).

Ok.

Thanks

>
> Thanks!
>
>> Thanks
>>
>>
>>>                vhost_virtqueue_start(hdev, hdev->vdev,
&hdev->vqs[n], vq_idx);
>>>            }
>>>