diff mbox series

[RFC,v8,02/21] vhost: Add custom used buffer callback

Message ID 20220519191306.821774-3-eperezma@redhat.com
State New
Headers show
Series Net Control VQ support with asid in vDPA SVQ | expand

Commit Message

Eugenio Perez Martin May 19, 2022, 7:12 p.m. UTC
The callback allows SVQ users to know the VirtQueue requests and
responses. QEMU can use this to synchronize virtio device model state,
allowing to migrate it with minimum changes to the migration code.

In the case of networking, this will be used to inspect control
virtqueue messages.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.h | 16 +++++++++++++++-
 include/hw/virtio/vhost-vdpa.h     |  2 ++
 hw/virtio/vhost-shadow-virtqueue.c |  9 ++++++++-
 hw/virtio/vhost-vdpa.c             |  3 ++-
 4 files changed, 27 insertions(+), 3 deletions(-)

Comments

Jason Wang June 7, 2022, 6:12 a.m. UTC | #1
在 2022/5/20 03:12, Eugenio Pérez 写道:
> The callback allows SVQ users to know the VirtQueue requests and
> responses. QEMU can use this to synchronize virtio device model state,
> allowing to migrate it with minimum changes to the migration code.
>
> In the case of networking, this will be used to inspect control
> virtqueue messages.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-shadow-virtqueue.h | 16 +++++++++++++++-
>   include/hw/virtio/vhost-vdpa.h     |  2 ++
>   hw/virtio/vhost-shadow-virtqueue.c |  9 ++++++++-
>   hw/virtio/vhost-vdpa.c             |  3 ++-
>   4 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> index c132c994e9..6593f07db3 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -15,6 +15,13 @@
>   #include "standard-headers/linux/vhost_types.h"
>   #include "hw/virtio/vhost-iova-tree.h"
>   
> +typedef void (*VirtQueueElementCallback)(VirtIODevice *vdev,
> +                                         const VirtQueueElement *elem);


Nit: I wonder if something like "VirtQueueCallback" is sufficient (e.g 
kernel use "callback" directly)


> +
> +typedef struct VhostShadowVirtqueueOps {
> +    VirtQueueElementCallback used_elem_handler;
> +} VhostShadowVirtqueueOps;
> +
>   /* Shadow virtqueue to relay notifications */
>   typedef struct VhostShadowVirtqueue {
>       /* Shadow vring */
> @@ -59,6 +66,12 @@ typedef struct VhostShadowVirtqueue {
>        */
>       uint16_t *desc_next;
>   
> +    /* Optional callbacks */
> +    const VhostShadowVirtqueueOps *ops;


Can we merge map_ops to ops?


> +
> +    /* Optional custom used virtqueue element handler */
> +    VirtQueueElementCallback used_elem_cb;


This seems not used in this series.

Thanks


> +
>       /* Next head to expose to the device */
>       uint16_t shadow_avail_idx;
>   
> @@ -85,7 +98,8 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>                        VirtQueue *vq);
>   void vhost_svq_stop(VhostShadowVirtqueue *svq);
>   
> -VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree);
> +VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree,
> +                                    const VhostShadowVirtqueueOps *ops);
>   
>   void vhost_svq_free(gpointer vq);
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(VhostShadowVirtqueue, vhost_svq_free);
> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> index a29dbb3f53..f1ba46a860 100644
> --- a/include/hw/virtio/vhost-vdpa.h
> +++ b/include/hw/virtio/vhost-vdpa.h
> @@ -17,6 +17,7 @@
>   #include "hw/virtio/vhost-iova-tree.h"
>   #include "hw/virtio/virtio.h"
>   #include "standard-headers/linux/vhost_types.h"
> +#include "hw/virtio/vhost-shadow-virtqueue.h"
>   
>   typedef struct VhostVDPAHostNotifier {
>       MemoryRegion mr;
> @@ -35,6 +36,7 @@ typedef struct vhost_vdpa {
>       /* IOVA mapping used by the Shadow Virtqueue */
>       VhostIOVATree *iova_tree;
>       GPtrArray *shadow_vqs;
> +    const VhostShadowVirtqueueOps *shadow_vq_ops;
>       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 56c96ebd13..167db8be45 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -410,6 +410,10 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
>                   break;
>               }
>   
> +            if (svq->ops && svq->ops->used_elem_handler) {
> +                svq->ops->used_elem_handler(svq->vdev, elem);
> +            }
> +
>               if (unlikely(i >= svq->vring.num)) {
>                   qemu_log_mask(LOG_GUEST_ERROR,
>                            "More than %u used buffers obtained in a %u size SVQ",
> @@ -607,12 +611,14 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
>    * shadow methods and file descriptors.
>    *
>    * @iova_tree: Tree to perform descriptors translations
> + * @ops: SVQ operations hooks
>    *
>    * Returns the new virtqueue or NULL.
>    *
>    * In case of error, reason is reported through error_report.
>    */
> -VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree)
> +VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree,
> +                                    const VhostShadowVirtqueueOps *ops)
>   {
>       g_autofree VhostShadowVirtqueue *svq = g_new0(VhostShadowVirtqueue, 1);
>       int r;
> @@ -634,6 +640,7 @@ VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree)
>       event_notifier_init_fd(&svq->svq_kick, VHOST_FILE_UNBIND);
>       event_notifier_set_handler(&svq->hdev_call, vhost_svq_handle_call);
>       svq->iova_tree = iova_tree;
> +    svq->ops = ops;
>       return g_steal_pointer(&svq);
>   
>   err_init_hdev_call:
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 66f054a12c..7677b337e6 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -418,7 +418,8 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
>   
>       shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
>       for (unsigned n = 0; n < hdev->nvqs; ++n) {
> -        g_autoptr(VhostShadowVirtqueue) svq = vhost_svq_new(v->iova_tree);
> +        g_autoptr(VhostShadowVirtqueue) svq = vhost_svq_new(v->iova_tree,
> +                                                            v->shadow_vq_ops);
>   
>           if (unlikely(!svq)) {
>               error_setg(errp, "Cannot create svq %u", n);
Eugenio Perez Martin June 8, 2022, 7:38 p.m. UTC | #2
On Tue, Jun 7, 2022 at 8:12 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/5/20 03:12, Eugenio Pérez 写道:
> > The callback allows SVQ users to know the VirtQueue requests and
> > responses. QEMU can use this to synchronize virtio device model state,
> > allowing to migrate it with minimum changes to the migration code.
> >
> > In the case of networking, this will be used to inspect control
> > virtqueue messages.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.h | 16 +++++++++++++++-
> >   include/hw/virtio/vhost-vdpa.h     |  2 ++
> >   hw/virtio/vhost-shadow-virtqueue.c |  9 ++++++++-
> >   hw/virtio/vhost-vdpa.c             |  3 ++-
> >   4 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> > index c132c994e9..6593f07db3 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -15,6 +15,13 @@
> >   #include "standard-headers/linux/vhost_types.h"
> >   #include "hw/virtio/vhost-iova-tree.h"
> >
> > +typedef void (*VirtQueueElementCallback)(VirtIODevice *vdev,
> > +                                         const VirtQueueElement *elem);
>
>
> Nit: I wonder if something like "VirtQueueCallback" is sufficient (e.g
> kernel use "callback" directly)
>

I didn't think about the notification part of the "callback" but more
on the function callback, to notify the net or vhost-vdpa net
subsystem :). But I think it can be named your way for sure.

If we ever have other callbacks closer to vq than to vq elements to
rename it later shouldn't be a big deal.

>
> > +
> > +typedef struct VhostShadowVirtqueueOps {
> > +    VirtQueueElementCallback used_elem_handler;
> > +} VhostShadowVirtqueueOps;
> > +
> >   /* Shadow virtqueue to relay notifications */
> >   typedef struct VhostShadowVirtqueue {
> >       /* Shadow vring */
> > @@ -59,6 +66,12 @@ typedef struct VhostShadowVirtqueue {
> >        */
> >       uint16_t *desc_next;
> >
> > +    /* Optional callbacks */
> > +    const VhostShadowVirtqueueOps *ops;
>
>
> Can we merge map_ops to ops?
>

It can be merged, but they are set by different actors.

map_ops is received by hw/virtio/vhost-vdpa, while this ops depends on
the kind of device. Is it ok to fill the ops members "by chunks"?

>
> > +
> > +    /* Optional custom used virtqueue element handler */
> > +    VirtQueueElementCallback used_elem_cb;
>
>
> This seems not used in this series.
>

Right, this is a leftover. Thanks for pointing it out!

Thanks!

> Thanks
>
>
> > +
> >       /* Next head to expose to the device */
> >       uint16_t shadow_avail_idx;
> >
> > @@ -85,7 +98,8 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
> >                        VirtQueue *vq);
> >   void vhost_svq_stop(VhostShadowVirtqueue *svq);
> >
> > -VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree);
> > +VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree,
> > +                                    const VhostShadowVirtqueueOps *ops);
> >
> >   void vhost_svq_free(gpointer vq);
> >   G_DEFINE_AUTOPTR_CLEANUP_FUNC(VhostShadowVirtqueue, vhost_svq_free);
> > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > index a29dbb3f53..f1ba46a860 100644
> > --- a/include/hw/virtio/vhost-vdpa.h
> > +++ b/include/hw/virtio/vhost-vdpa.h
> > @@ -17,6 +17,7 @@
> >   #include "hw/virtio/vhost-iova-tree.h"
> >   #include "hw/virtio/virtio.h"
> >   #include "standard-headers/linux/vhost_types.h"
> > +#include "hw/virtio/vhost-shadow-virtqueue.h"
> >
> >   typedef struct VhostVDPAHostNotifier {
> >       MemoryRegion mr;
> > @@ -35,6 +36,7 @@ typedef struct vhost_vdpa {
> >       /* IOVA mapping used by the Shadow Virtqueue */
> >       VhostIOVATree *iova_tree;
> >       GPtrArray *shadow_vqs;
> > +    const VhostShadowVirtqueueOps *shadow_vq_ops;
> >       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 56c96ebd13..167db8be45 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -410,6 +410,10 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> >                   break;
> >               }
> >
> > +            if (svq->ops && svq->ops->used_elem_handler) {
> > +                svq->ops->used_elem_handler(svq->vdev, elem);
> > +            }
> > +
> >               if (unlikely(i >= svq->vring.num)) {
> >                   qemu_log_mask(LOG_GUEST_ERROR,
> >                            "More than %u used buffers obtained in a %u size SVQ",
> > @@ -607,12 +611,14 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
> >    * shadow methods and file descriptors.
> >    *
> >    * @iova_tree: Tree to perform descriptors translations
> > + * @ops: SVQ operations hooks
> >    *
> >    * Returns the new virtqueue or NULL.
> >    *
> >    * In case of error, reason is reported through error_report.
> >    */
> > -VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree)
> > +VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree,
> > +                                    const VhostShadowVirtqueueOps *ops)
> >   {
> >       g_autofree VhostShadowVirtqueue *svq = g_new0(VhostShadowVirtqueue, 1);
> >       int r;
> > @@ -634,6 +640,7 @@ VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree)
> >       event_notifier_init_fd(&svq->svq_kick, VHOST_FILE_UNBIND);
> >       event_notifier_set_handler(&svq->hdev_call, vhost_svq_handle_call);
> >       svq->iova_tree = iova_tree;
> > +    svq->ops = ops;
> >       return g_steal_pointer(&svq);
> >
> >   err_init_hdev_call:
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 66f054a12c..7677b337e6 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -418,7 +418,8 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
> >
> >       shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
> >       for (unsigned n = 0; n < hdev->nvqs; ++n) {
> > -        g_autoptr(VhostShadowVirtqueue) svq = vhost_svq_new(v->iova_tree);
> > +        g_autoptr(VhostShadowVirtqueue) svq = vhost_svq_new(v->iova_tree,
> > +                                                            v->shadow_vq_ops);
> >
> >           if (unlikely(!svq)) {
> >               error_setg(errp, "Cannot create svq %u", n);
>
diff mbox series

Patch

diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
index c132c994e9..6593f07db3 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -15,6 +15,13 @@ 
 #include "standard-headers/linux/vhost_types.h"
 #include "hw/virtio/vhost-iova-tree.h"
 
+typedef void (*VirtQueueElementCallback)(VirtIODevice *vdev,
+                                         const VirtQueueElement *elem);
+
+typedef struct VhostShadowVirtqueueOps {
+    VirtQueueElementCallback used_elem_handler;
+} VhostShadowVirtqueueOps;
+
 /* Shadow virtqueue to relay notifications */
 typedef struct VhostShadowVirtqueue {
     /* Shadow vring */
@@ -59,6 +66,12 @@  typedef struct VhostShadowVirtqueue {
      */
     uint16_t *desc_next;
 
+    /* Optional callbacks */
+    const VhostShadowVirtqueueOps *ops;
+
+    /* Optional custom used virtqueue element handler */
+    VirtQueueElementCallback used_elem_cb;
+
     /* Next head to expose to the device */
     uint16_t shadow_avail_idx;
 
@@ -85,7 +98,8 @@  void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
                      VirtQueue *vq);
 void vhost_svq_stop(VhostShadowVirtqueue *svq);
 
-VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree);
+VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree,
+                                    const VhostShadowVirtqueueOps *ops);
 
 void vhost_svq_free(gpointer vq);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(VhostShadowVirtqueue, vhost_svq_free);
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index a29dbb3f53..f1ba46a860 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -17,6 +17,7 @@ 
 #include "hw/virtio/vhost-iova-tree.h"
 #include "hw/virtio/virtio.h"
 #include "standard-headers/linux/vhost_types.h"
+#include "hw/virtio/vhost-shadow-virtqueue.h"
 
 typedef struct VhostVDPAHostNotifier {
     MemoryRegion mr;
@@ -35,6 +36,7 @@  typedef struct vhost_vdpa {
     /* IOVA mapping used by the Shadow Virtqueue */
     VhostIOVATree *iova_tree;
     GPtrArray *shadow_vqs;
+    const VhostShadowVirtqueueOps *shadow_vq_ops;
     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 56c96ebd13..167db8be45 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -410,6 +410,10 @@  static void vhost_svq_flush(VhostShadowVirtqueue *svq,
                 break;
             }
 
+            if (svq->ops && svq->ops->used_elem_handler) {
+                svq->ops->used_elem_handler(svq->vdev, elem);
+            }
+
             if (unlikely(i >= svq->vring.num)) {
                 qemu_log_mask(LOG_GUEST_ERROR,
                          "More than %u used buffers obtained in a %u size SVQ",
@@ -607,12 +611,14 @@  void vhost_svq_stop(VhostShadowVirtqueue *svq)
  * shadow methods and file descriptors.
  *
  * @iova_tree: Tree to perform descriptors translations
+ * @ops: SVQ operations hooks
  *
  * Returns the new virtqueue or NULL.
  *
  * In case of error, reason is reported through error_report.
  */
-VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree)
+VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree,
+                                    const VhostShadowVirtqueueOps *ops)
 {
     g_autofree VhostShadowVirtqueue *svq = g_new0(VhostShadowVirtqueue, 1);
     int r;
@@ -634,6 +640,7 @@  VhostShadowVirtqueue *vhost_svq_new(VhostIOVATree *iova_tree)
     event_notifier_init_fd(&svq->svq_kick, VHOST_FILE_UNBIND);
     event_notifier_set_handler(&svq->hdev_call, vhost_svq_handle_call);
     svq->iova_tree = iova_tree;
+    svq->ops = ops;
     return g_steal_pointer(&svq);
 
 err_init_hdev_call:
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 66f054a12c..7677b337e6 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -418,7 +418,8 @@  static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
 
     shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
     for (unsigned n = 0; n < hdev->nvqs; ++n) {
-        g_autoptr(VhostShadowVirtqueue) svq = vhost_svq_new(v->iova_tree);
+        g_autoptr(VhostShadowVirtqueue) svq = vhost_svq_new(v->iova_tree,
+                                                            v->shadow_vq_ops);
 
         if (unlikely(!svq)) {
             error_setg(errp, "Cannot create svq %u", n);