QE applied this patch to do sanity testing on vhost-net, there is no any regression problem.
Tested-by: Lei Yang <leiy...@redhat.com> On Tue, May 9, 2023 at 1:28 AM Eugenio Perez Martin <epere...@redhat.com> wrote: > > On Sat, May 6, 2023 at 5:01 PM Hawkins Jiawei <yin31...@gmail.com> wrote: > > > > QEMU invokes vhost_svq_add() when adding a guest's element into SVQ. > > In vhost_svq_add(), it uses vhost_svq_available_slots() to check > > whether QEMU can add the element into the SVQ. If there is > > enough space, then QEMU combines some out descriptors and > > some in descriptors into one descriptor chain, and add it into > > svq->vring.desc by vhost_svq_vring_write_descs(). > > > > Yet the problem is that, `svq->shadow_avail_idx - svq->shadow_used_idx` > > in vhost_svq_available_slots() return the number of occupied elements, > > or the number of descriptor chains, instead of the number of occupied > > descriptors, which may cause wrapping in SVQ descriptor ring. > > > > Here is an example. In vhost_handle_guest_kick(), QEMU forwards > > as many available buffers to device by virtqueue_pop() and > > vhost_svq_add_element(). virtqueue_pop() return a guest's element, > > and use vhost_svq_add_elemnt(), a wrapper to vhost_svq_add(), to > > add this element into SVQ. If QEMU invokes virtqueue_pop() and > > vhost_svq_add_element() `svq->vring.num` times, vhost_svq_available_slots() > > thinks QEMU just ran out of slots and everything should work fine. > > But in fact, virtqueue_pop() return `svq-vring.num` elements or > > descriptor chains, more than `svq->vring.num` descriptors, due to > > guest memory fragmentation, and this cause wrapping in SVQ descriptor ring. > > > > The bug is valid even before marking the descriptors used. If the > guest memory is fragmented, SVQ must add chains so it can try to add > more descriptors than possible. > > > Therefore, this patch adds `num_free` field in VhostShadowVirtqueue > > structure, updates this field in vhost_svq_add() and > > vhost_svq_get_buf(), to record the number of free descriptors. > > Then we can avoid wrap in SVQ descriptor ring by refactoring > > vhost_svq_available_slots(). > > > > Fixes: 100890f7ca ("vhost: Shadow virtqueue buffers forwarding") > > Signed-off-by: Hawkins Jiawei <yin31...@gmail.com> > > --- > > hw/virtio/vhost-shadow-virtqueue.c | 9 ++++++++- > > hw/virtio/vhost-shadow-virtqueue.h | 3 +++ > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c > > b/hw/virtio/vhost-shadow-virtqueue.c > > index 8361e70d1b..e1c6952b10 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > @@ -68,7 +68,7 @@ bool vhost_svq_valid_features(uint64_t features, Error > > **errp) > > */ > > static uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq) > > { > > - return svq->vring.num - (svq->shadow_avail_idx - svq->shadow_used_idx); > > + return svq->num_free; > > } > > > > /** > > @@ -263,6 +263,9 @@ int vhost_svq_add(VhostShadowVirtqueue *svq, const > > struct iovec *out_sg, > > return -EINVAL; > > } > > > > + /* Update the size of SVQ vring free descriptors */ > > + svq->num_free -= ndescs; > > + > > svq->desc_state[qemu_head].elem = elem; > > svq->desc_state[qemu_head].ndescs = ndescs; > > vhost_svq_kick(svq); > > @@ -450,6 +453,9 @@ static VirtQueueElement > > *vhost_svq_get_buf(VhostShadowVirtqueue *svq, > > svq->desc_next[last_used_chain] = svq->free_head; > > svq->free_head = used_elem.id; > > > > + /* Update the size of SVQ vring free descriptors */ > > No need for this comment. > > Apart from that, > > Acked-by: Eugenio Pérez <epere...@redhat.com> > > > + svq->num_free += num; > > + > > *len = used_elem.len; > > return g_steal_pointer(&svq->desc_state[used_elem.id].elem); > > } > > @@ -659,6 +665,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, > > VirtIODevice *vdev, > > svq->iova_tree = iova_tree; > > > > svq->vring.num = virtio_queue_get_num(vdev, > > virtio_get_queue_index(vq)); > > + svq->num_free = svq->vring.num; > > driver_size = vhost_svq_driver_area_size(svq); > > device_size = vhost_svq_device_area_size(svq); > > svq->vring.desc = qemu_memalign(qemu_real_host_page_size(), > > driver_size); > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h > > b/hw/virtio/vhost-shadow-virtqueue.h > > index 926a4897b1..6efe051a70 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.h > > +++ b/hw/virtio/vhost-shadow-virtqueue.h > > @@ -107,6 +107,9 @@ typedef struct VhostShadowVirtqueue { > > > > /* Next head to consume from the device */ > > uint16_t last_used_idx; > > + > > + /* Size of SVQ vring free descriptors */ > > + uint16_t num_free; > > } VhostShadowVirtqueue; > > > > bool vhost_svq_valid_features(uint64_t features, Error **errp); > > -- > > 2.25.1 > > > >