Re: [RFC PATCH v9 04/23] vhost: Get vring base from vq, not svq

[Eugenio Perez Martin]

On Fri, Jul 8, 2022 at 11:12 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jul 7, 2022 at 2:40 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > The used idx used to match with this, but it will not match from the
> > moment we introduce svq_inject.
>
> It might be better to explain what "svq_inject" means here.
>

Good point, I'll change for the next version.

> > Rewind all the descriptors not used by
> > vdpa device and get the vq state properly.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  include/hw/virtio/virtio.h | 1 +
> >  hw/virtio/vhost-vdpa.c     | 7 +++----
> >  hw/virtio/virtio.c         | 5 +++++
> >  3 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index db1c0ddf6b..4b51ab9d06 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -302,6 +302,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, 
> > int n);
> >  hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n);
> >  hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n);
> >  unsigned int virtio_queue_get_last_avail_idx(VirtIODevice *vdev, int n);
> > +unsigned int virtio_queue_get_in_use(const VirtQueue *vq);
> >  void virtio_queue_set_last_avail_idx(VirtIODevice *vdev, int n,
> >                                       unsigned int idx);
> >  void virtio_queue_restore_last_avail_idx(VirtIODevice *vdev, int n);
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 2ee8009594..de76128030 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -1189,12 +1189,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);
> > -
> > +        const VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
> >          /*
> >           * Setting base as last used idx, so destination will see as 
> > available
> >           * all the entries that the device did not use, including the 
> > in-flight
> > @@ -1203,7 +1201,8 @@ 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;
> > +        ring->num = virtio_queue_get_last_avail_idx(dev->vdev, 
> > ring->index) -
> > +                    virtio_queue_get_in_use(vq);
>
> I think we need to change the above comment as well otherwise readers
> might get confused.
>

Re-thinking this: This part has always been buggy, so this is actually
a fix. I'll tag it for next versions or, even better, send it
separately.

But the comment still holds: We cannot use the device's used idx since
it could not match with the guest visible one. This is actually easy
to trigger if we migrate a guest many times with traffic.

Maybe it's cleaner to export directly used_idx from VirtQueue? Extra
care is needed with packed vq, but SVQ still does not support it. I
didn't want to duplicate that logic in virtio ring handling.

> I wonder why we need to bother at this time. Is this an issue for
> networking devices?

Every device has this issue when migrating as soon as the device's
used index is not the same as the guest's one.

> And for block device, it's not sufficient since
> there's no guarantee that the descriptor is handled in order?
>

Right, that part still hold here.

Thanks!