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

[Jason Wang <jasowang@redhat.com>]

在 2022/7/8 18:10, 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.


I may miss someting, maybe you can give me an example on this (I assume 
the size of the svq is the same as what guest can see).


>
> 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.


So two more questions here:

1) what's the reason of rewinding via virtio_queue_get_in_use()?

2) it looks like we could end up with underflow with the above math?

Thanks


>
>> 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!
>