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

[Eugenio Perez Martin]

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