qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] virtio: notify virtqueue via host notifier when available
@ 2019-11-05 14:09 Stefan Hajnoczi
  2019-11-06  8:53 ` Stefan Hajnoczi
  2019-11-06 11:33 ` Michael S. Tsirkin
  0 siblings, 2 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2019-11-05 14:09 UTC (permalink / raw)
  To: qemu-devel; +Cc: Yongji Xie, Michael S. Tsirkin, Stefan Hajnoczi, felipe

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 <felipe@nutanix.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
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



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] virtio: notify virtqueue via host notifier when available
  2019-11-05 14:09 [PATCH v2] virtio: notify virtqueue via host notifier when available Stefan Hajnoczi
@ 2019-11-06  8:53 ` Stefan Hajnoczi
  2019-11-06 11:33 ` Michael S. Tsirkin
  1 sibling, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2019-11-06  8:53 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Yongji Xie, qemu-devel, Stefan Hajnoczi, felipe

[-- Attachment #1: Type: text/plain, Size: 4761 bytes --]

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 <felipe@nutanix.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v2:
>  * Track host notifier enabled/disabled state per virtqueue [Yongji Xie]
>  * Tested with contrib/vhost-user-blk and contrib/vhost-user-scsi

Hi Michael,
Please consider this for QEMU 4.2 so that SeaBIOS works even when it
selects the VP_ACCESS_PCICFG access mode.

Felipe tested this successfully yesterday.

Stefan

>  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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] virtio: notify virtqueue via host notifier when available
  2019-11-05 14:09 [PATCH v2] virtio: notify virtqueue via host notifier when available Stefan Hajnoczi
  2019-11-06  8:53 ` Stefan Hajnoczi
@ 2019-11-06 11:33 ` Michael S. Tsirkin
  2019-11-07 19:02   ` Felipe Franciosi
  1 sibling, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2019-11-06 11:33 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: Yongji Xie, qemu-devel, felipe

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 <felipe@nutanix.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] virtio: notify virtqueue via host notifier when available
  2019-11-06 11:33 ` Michael S. Tsirkin
@ 2019-11-07 19:02   ` Felipe Franciosi
  0 siblings, 0 replies; 4+ messages in thread
From: Felipe Franciosi @ 2019-11-07 19:02 UTC (permalink / raw)
  To: Michael S. Tsirkin, Stefan Hajnoczi; +Cc: Yongji Xie, qemu-devel

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 <mst@redhat.com> 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 <felipe@nutanix.com>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> 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



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-11-07 19:04 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 14:09 [PATCH v2] virtio: notify virtqueue via host notifier when available Stefan Hajnoczi
2019-11-06  8:53 ` Stefan Hajnoczi
2019-11-06 11:33 ` Michael S. Tsirkin
2019-11-07 19:02   ` Felipe Franciosi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).