Re: [RFC PATCH 01/12] vhost: Get vring base from vq, not svq

[Eugenio Perez Martin <eperezma@redhat.com>]

On Mon, Jul 18, 2022 at 7:48 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > The SVQ vring used idx usually match with the guest visible one, as long
> > as all the guest buffers (GPA) maps to exactly one buffer within qemu's
> > VA. However, as we can see in virtqueue_map_desc, a single guest buffer
> > could map to many buffers in SVQ vring.
> >
> > The solution is to stop using the device's used idx and check for the
> > last avail idx. Since we cannot report in-flight descriptors with vdpa,
> > let's rewind all of them.
> >
> > Fixes: 6d0b22266633 ("vdpa: Adapt vhost_vdpa_get_vring_base to SVQ")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-vdpa.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 795ed5a049..18820498b3 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -1194,11 +1194,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
> >                                         struct vhost_vring_state *ring)
> >  {
> >      struct vhost_vdpa *v = dev->opaque;
> > -    int vdpa_idx = ring->index - dev->vq_index;
> >      int ret;
> >
> >      if (v->shadow_vqs_enabled) {
> > -        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx);
> > +        VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
> >
> >          /*
> >           * Setting base as last used idx, so destination will see as available
> > @@ -1208,7 +1207,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
> >           * TODO: This is ok for networking, but other kinds of devices might
> >           * have problems with these retransmissions.
> >           */
> > -        ring->num = svq->last_used_idx;
> > +        while (virtqueue_rewind(vq, 1)) {
> > +            continue;
> > +        }
>
> Will this leak mapped VirtQueueElement?
>

vhost_get_vring_base op is called only on the device's teardown path,
so they should be free by vhost_svq_stop.

However, you have a point that maybe vhost_get_vring_base should not
trust in that cleaning, even for -stable.

So I think it should be better to squash this and the next one as the
same fix. But it's doing two things at the same time: One of them is
to use the right state (as vring_base), and another one is not
reverting in-flight descriptors.

Thanks!

> Thanks
>
> > +        ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
> >          return 0;
> >      }
> >
> > --
> > 2.31.1
> >
>