On Fri, Jul 8, 2022 at 11:12 AM Jason Wang <jasow...@redhat.com> wrote:
>
> On Thu, Jul 7, 2022 at 2:40 AM Eugenio Pérez <epere...@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 <epere...@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!
