Re: [PATCH v2] virtio: notify virtqueue via host notifier when available

[Felipe Franciosi]

Thanks Stefan for the quick fix! Sorry for not adding a Tested-by.
It's implicit. :)

F.

> On Nov 6, 2019, at 11:33 AM, Michael S. Tsirkin <address@hidden> wrote:
> 
> On Tue, Nov 05, 2019 at 03:09:46PM +0100, Stefan Hajnoczi wrote:
>> Host notifiers are used in several cases:
>> 1. Traditional ioeventfd where virtqueue notifications are handled in
>>   the main loop thread.
>> 2. IOThreads (aio_handle_output) where virtqueue notifications are
>>   handled in an IOThread AioContext.
>> 3. vhost where virtqueue notifications are handled by kernel vhost or
>>   a vhost-user device backend.
>> 
>> Most virtqueue notifications from the guest use the ioeventfd mechanism,
>> but there are corner cases where QEMU code calls virtio_queue_notify().
>> This currently honors the host notifier for the IOThreads
>> aio_handle_output case, but not for the vhost case.  The result is that
>> vhost does not receive virtqueue notifications from QEMU when
>> virtio_queue_notify() is called.
>> 
>> This patch extends virtio_queue_notify() to set the host notifier
>> whenever it is enabled instead of calling the vq->(aio_)handle_output()
>> function directly.  We track the host notifier state for each virtqueue
>> separately since some devices may use it only for certain virtqueues.
>> 
>> This fixes the vhost case although it does add a trip through the
>> eventfd for the traditional ioeventfd case.  I don't think it's worth
>> adding a fast path for the traditional ioeventfd case because calling
>> virtio_queue_notify() is rare when ioeventfd is enabled.
>> 
>> Reported-by: Felipe Franciosi <address@hidden>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
> 
> queued, thanks!
> 
>> ---
>> v2:
>> * Track host notifier enabled/disabled state per virtqueue [Yongji Xie]
>> * Tested with contrib/vhost-user-blk and contrib/vhost-user-scsi
>> 
>> hw/virtio/virtio-bus.c     | 4 ++++
>> hw/virtio/virtio.c         | 9 ++++++++-
>> include/hw/virtio/virtio.h | 1 +
>> 3 files changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>> index b2c804292e..d6332d45c3 100644
>> --- a/hw/virtio/virtio-bus.c
>> +++ b/hw/virtio/virtio-bus.c
>> @@ -288,6 +288,10 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, 
>> int n, bool assign)
>>         k->ioeventfd_assign(proxy, notifier, n, false);
>>     }
>> 
>> +    if (r == 0) {
>> +        virtio_queue_set_host_notifier_enabled(vq, assign);
>> +    }
>> +
>>     return r;
>> }
>> 
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 762df12f4c..04716b5f6c 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -128,6 +128,7 @@ struct VirtQueue
>>     VirtIODevice *vdev;
>>     EventNotifier guest_notifier;
>>     EventNotifier host_notifier;
>> +    bool host_notifier_enabled;
>>     QLIST_ENTRY(VirtQueue) node;
>> };
>> 
>> @@ -2271,7 +2272,7 @@ void virtio_queue_notify(VirtIODevice *vdev, int n)
>>     }
>> 
>>     trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
>> -    if (vq->handle_aio_output) {
>> +    if (vq->host_notifier_enabled) {
>>         event_notifier_set(&vq->host_notifier);
>>     } else if (vq->handle_output) {
>>         vq->handle_output(vdev, vq);
>> @@ -3145,6 +3146,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>>         vdev->vq[i].vector = VIRTIO_NO_VECTOR;
>>         vdev->vq[i].vdev = vdev;
>>         vdev->vq[i].queue_index = i;
>> +        vdev->vq[i].host_notifier_enabled = false;
>>     }
>> 
>>     vdev->name = name;
>> @@ -3436,6 +3438,11 @@ EventNotifier 
>> *virtio_queue_get_host_notifier(VirtQueue *vq)
>>     return &vq->host_notifier;
>> }
>> 
>> +void virtio_queue_set_host_notifier_enabled(VirtQueue *vq, bool enabled)
>> +{
>> +    vq->host_notifier_enabled = enabled;
>> +}
>> +
>> int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
>>                                       MemoryRegion *mr, bool assign)
>> {
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 3448d67d2a..c32a815303 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -312,6 +312,7 @@ int virtio_device_grab_ioeventfd(VirtIODevice *vdev);
>> void virtio_device_release_ioeventfd(VirtIODevice *vdev);
>> bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev);
>> EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
>> +void virtio_queue_set_host_notifier_enabled(VirtQueue *vq, bool enabled);
>> void virtio_queue_host_notifier_read(EventNotifier *n);
>> void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext 
>> *ctx,
>>                                                 VirtIOHandleAIOOutput 
>> handle_output);
>> -- 
>> 2.23.0