? 2022/1/31 ??6:18, Eugenio Perez Martin ??:> On Fri, Jan 28, 2022 at 7:29 AM Jason Wang <jasowang at redhat.com>
wrote:
>>
>> ? 2022/1/22 ??4:27, Eugenio P?rez ??:
>>> This function allows the vhost-vdpa backend to override kick_fd.
>>>
>>> Signed-off-by: Eugenio P?rez <eperezma at redhat.com>
>>> ---
>>> hw/virtio/vhost-shadow-virtqueue.h | 1 +
>>> hw/virtio/vhost-shadow-virtqueue.c | 45
++++++++++++++++++++++++++++++
>>> 2 files changed, 46 insertions(+)
>>>
>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.h
b/hw/virtio/vhost-shadow-virtqueue.h
>>> index 400effd9f2..a56ecfc09d 100644
>>> --- a/hw/virtio/vhost-shadow-virtqueue.h
>>> +++ b/hw/virtio/vhost-shadow-virtqueue.h
>>> @@ -15,6 +15,7 @@
>>>
>>> typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
>>>
>>> +void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int
svq_kick_fd);
>>> const EventNotifier *vhost_svq_get_dev_kick_notifier(
>>> const
VhostShadowVirtqueue *svq);
>>>
>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c
b/hw/virtio/vhost-shadow-virtqueue.c
>>> index bd87110073..21534bc94d 100644
>>> --- a/hw/virtio/vhost-shadow-virtqueue.c
>>> +++ b/hw/virtio/vhost-shadow-virtqueue.c
>>> @@ -11,6 +11,7 @@
>>> #include "hw/virtio/vhost-shadow-virtqueue.h"
>>>
>>> #include "qemu/error-report.h"
>>> +#include "qemu/main-loop.h"
>>>
>>> /* Shadow virtqueue to relay notifications */
>>> typedef struct VhostShadowVirtqueue {
>>> @@ -18,8 +19,20 @@ 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 register on
the event
>>> + * loop and access the associated shadow virtqueue easily. If
we use the
>>> + * VirtQueue, we don't have an easy way to retrieve it.
>>> + *
>>> + * So shadow virtqueue must not clean it, or we would lose
VirtQueue one.
>>> + */
>>> + EventNotifier svq_kick;
>>> } VhostShadowVirtqueue;
>>>
>>> +#define INVALID_SVQ_KICK_FD -1
>>> +
>>> /**
>>> * The notifier that SVQ will use to notify the device.
>>> */
>>> @@ -29,6 +42,35 @@ const EventNotifier
*vhost_svq_get_dev_kick_notifier(
>>> return &svq->hdev_kick;
>>> }
>>>
>>> +/**
>>> + * Set a new file descriptor for the guest to kick SVQ and notify
for avail
>>> + *
>>> + * @svq The svq
>>> + * @svq_kick_fd The new svq kick fd
>>> + */
>>> +void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int
svq_kick_fd)
>>> +{
>>> + EventNotifier tmp;
>>> + bool check_old = INVALID_SVQ_KICK_FD !>>> +
event_notifier_get_fd(&svq->svq_kick);
>>> +
>>> + if (check_old) {
>>> + event_notifier_set_handler(&svq->svq_kick, NULL);
>>> + event_notifier_init_fd(&tmp,
event_notifier_get_fd(&svq->svq_kick));
>>> + }
>>
>> It looks to me we don't do similar things in vhost-net. Any reason
for
>> caring about the old svq_kick?
>>
> Do you mean to check for old kick_fd in case we miss notifications,
> and explicitly omit the INVALID_SVQ_KICK_FD?
Yes.
>
> If you mean qemu's vhost-net, I guess it's because the device's
kick
> fd is never changed in all the vhost device lifecycle, it's only set
> at the beginning. Previous RFC also depended on that, but you
> suggested better vhost and SVQ in v4 feedback if I understood
> correctly [1]. Or am I missing something?
No, I forgot that. But in this case we should have a better dealing with
the the conversion from valid fd to -1 by disabling the handler.
>
> Qemu's vhost-net does not need to use this because it is not polling
> it. For kernel's vhost, I guess the closest is the use of pollstop and
> pollstart at vhost_vring_ioctl.
>
> In my opinion, I think that SVQ code size can benefit from now
> allowing to override kick_fd from the start of the operation. Not from
> initialization, but start. But I can see the benefits of having the
> change into account from this moment so it's more resilient to the
> future.
>
>>> +
>>> + /*
>>> + * event_notifier_set_handler already checks for guest's
notifications if
>>> + * they arrive to the new file descriptor in the switch, so
there is no
>>> + * need to explicitely check for them.
>>> + */
>>> + event_notifier_init_fd(&svq->svq_kick, svq_kick_fd);
>>> +
>>> + if (!check_old || event_notifier_test_and_clear(&tmp)) {
>>> + event_notifier_set(&svq->hdev_kick);
>>
>> Any reason we need to kick the device directly here?
>>
> At this point of the series only notifications are forwarded, not
> buffers. If kick_fd is set, we need to check the old one, the same way
> as vhost checks the masked notifier in case of change.
I meant we need to kick the svq instead of vhost-vdpa in this case?
Thanks
>
> Thanks!
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg03152.html
> , from "I'd suggest to not depend on this since it:"
>
>
>> Thanks
>>
>>
>>> + }
>>> +}
>>> +
>>> /**
>>> * Creates vhost shadow virtqueue, and instruct vhost device to
use the shadow
>>> * methods and file descriptors.
>>> @@ -52,6 +94,9 @@ VhostShadowVirtqueue *vhost_svq_new(void)
>>> goto err_init_hdev_call;
>>> }
>>>
>>> + /* Placeholder descriptor, it should be deleted at set_kick_fd
*/
>>> + event_notifier_init_fd(&svq->svq_kick,
INVALID_SVQ_KICK_FD);
>>> +
>>> return g_steal_pointer(&svq);
>>>
>>> err_init_hdev_call: