Re: [PATCH v2 15/19] vdpa: manual forward CVQ buffers

[Eugenio Perez Martin]

On Fri, Jul 15, 2022 at 6:08 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Jul 15, 2022 at 12:32 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Do a simple forwarding of CVQ buffers, the same work SVQ could do but
> > through callbacks. No functional change intended.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  include/hw/virtio/vhost-vdpa.h |  3 ++
> >  hw/virtio/vhost-vdpa.c         |  3 +-
> >  net/vhost-vdpa.c               | 58 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 63 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > index 7214eb47dc..1111d85643 100644
> > --- a/include/hw/virtio/vhost-vdpa.h
> > +++ b/include/hw/virtio/vhost-vdpa.h
> > @@ -15,6 +15,7 @@
> >  #include <gmodule.h>
> >
> >  #include "hw/virtio/vhost-iova-tree.h"
> > +#include "hw/virtio/vhost-shadow-virtqueue.h"
> >  #include "hw/virtio/virtio.h"
> >  #include "standard-headers/linux/vhost_types.h"
> >
> > @@ -35,6 +36,8 @@ typedef struct vhost_vdpa {
> >      /* IOVA mapping used by the Shadow Virtqueue */
> >      VhostIOVATree *iova_tree;
> >      GPtrArray *shadow_vqs;
> > +    const VhostShadowVirtqueueOps *shadow_vq_ops;
> > +    void *shadow_vq_ops_opaque;
> >      struct vhost_dev *dev;
> >      VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> >  } VhostVDPA;
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 96997210be..beaaa7049a 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -419,7 +419,8 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, 
> > struct vhost_vdpa *v,
> >      for (unsigned n = 0; n < hdev->nvqs; ++n) {
> >          g_autoptr(VhostShadowVirtqueue) svq;
> >
> > -        svq = vhost_svq_new(v->iova_tree, NULL, NULL);
> > +        svq = vhost_svq_new(v->iova_tree, v->shadow_vq_ops,
> > +                            v->shadow_vq_ops_opaque);
> >          if (unlikely(!svq)) {
> >              error_setg(errp, "Cannot create svq %u", n);
> >              return -1;
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index df1e69ee72..805c9dd6b6 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -11,11 +11,14 @@
> >
> >  #include "qemu/osdep.h"
> >  #include "clients.h"
> > +#include "hw/virtio/virtio-net.h"
> >  #include "net/vhost_net.h"
> >  #include "net/vhost-vdpa.h"
> >  #include "hw/virtio/vhost-vdpa.h"
> >  #include "qemu/config-file.h"
> >  #include "qemu/error-report.h"
> > +#include "qemu/log.h"
> > +#include "qemu/memalign.h"
> >  #include "qemu/option.h"
> >  #include "qapi/error.h"
> >  #include <linux/vhost.h>
> > @@ -187,6 +190,57 @@ static NetClientInfo net_vhost_vdpa_info = {
> >          .check_peer_type = vhost_vdpa_check_peer_type,
> >  };
> >
> > +/**
> > + * Forward buffer for the moment.
> > + */
> > +static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
> > +                                            SVQElement *svq_elem, void 
> > *opaque)
> > +{
> > +    VirtQueueElement *elem = &svq_elem->elem;
> > +    unsigned int n = elem->out_num + elem->in_num;
> > +    g_autofree struct iovec *dev_buffers = g_new(struct iovec, n);
> > +    size_t in_len, dev_written;
> > +    virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> > +    int r;
> > +
> > +    memcpy(dev_buffers, elem->out_sg, elem->out_num);
> > +    memcpy(dev_buffers + elem->out_num, elem->in_sg, elem->in_num);
> > +
> > +    r = vhost_svq_add(svq, &dev_buffers[0], elem->out_num, &dev_buffers[1],
> > +                      elem->in_num, svq_elem);
> > +    if (unlikely(r != 0)) {
> > +        if (unlikely(r == -ENOSPC)) {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device 
> > queue\n",
> > +                          __func__);
> > +        }
> > +        goto out;
> > +    }
> > +
> > +    /*
> > +     * We can poll here since we've had BQL from the time we sent the
> > +     * descriptor. Also, we need to take the answer before SVQ pulls by 
> > itself,
> > +     * when BQL is released
> > +     */
> > +    dev_written = vhost_svq_poll(svq);
> > +    if (unlikely(dev_written < sizeof(status))) {
> > +        error_report("Insufficient written data (%zu)", dev_written);
> > +    }
> > +
> > +out:
> > +    in_len = iov_from_buf(elem->in_sg, elem->in_num, 0, &status,
> > +                          sizeof(status));
> > +    if (unlikely(in_len < sizeof(status))) {
> > +        error_report("Bad device CVQ written length");
> > +    }
> > +    vhost_svq_push_elem(svq, svq_elem, MIN(in_len, sizeof(status)));
> > +    g_free(svq_elem);
> > +    return r;
> > +}
> > +
> > +static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
> > +    .avail_handler = vhost_vdpa_net_handle_ctrl_avail,
> > +};
>
> I wonder if it's possible to even remove this handler. Can we let the
> kick to be handled by virtio_net_handler_ctrl() in virtio-net.c?
>

I kind of drafted that here:
https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg02652.html

But I'm not sure about the part of not enabling the guest event
notifiers. I'd say we would need to move SVQ to the VirtIODevice
itself, which seems rather complicated at this moment.

Without applying that patch, all of the events of vDPA devices go
either to device itself or to the SVQ registered handlers. If we go
that route, we add the vq event handler, and that would depend on the
svq state + specific vq.

I'm totally ok to do it the first thing for the next merge window, or
if I have the certainty it's the accepted way to go. Otherwise, I
think we will not be able to deliver at least migration with SVQ for
this merge window again.

Thanks!