On Tue, Jun 7, 2022 at 8:12 AM Jason Wang <jasow...@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 <epere...@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);
>
