[v1] vhost: fix possible wrap in SVQ descriptor ring

[PATCH RESEND] vhost: fix possible wrap in SVQ descriptor ring

Posted by Hawkins Jiawei 11 months, 3 weeks ago

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.

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 <yin31149@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 */
+    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

Re: [PATCH RESEND] vhost: fix possible wrap in SVQ descriptor ring

Posted by Eugenio Perez Martin 11 months, 3 weeks ago

On Sat, May 6, 2023 at 5:01 PM Hawkins Jiawei <yin31149@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 <yin31149@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 <eperezma@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
>

Re: [PATCH RESEND] vhost: fix possible wrap in SVQ descriptor ring

Posted by Lei Yang 11 months, 3 weeks ago

QE applied this patch to do sanity testing on vhost-net, there is no
any regression problem.

Tested-by: Lei Yang <leiyang@redhat.com>



On Tue, May 9, 2023 at 1:28 AM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Sat, May 6, 2023 at 5:01 PM Hawkins Jiawei <yin31149@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 <yin31149@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 <eperezma@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
> >
>
>

Re: [PATCH RESEND] vhost: fix possible wrap in SVQ descriptor ring

Posted by Lei Yang 11 months, 3 weeks ago

QE applied this patch to do sanity testing on vhost-vdpa, there is no any
regression problem.

Tested-by: Lei Yang <leiyang@redhat.com>


On Wed, May 10, 2023 at 9:32 AM Lei Yang <leiyang@redhat.com> wrote:

> QE applied this patch to do sanity testing on vhost-net, there is no
> any regression problem.
>
> Tested-by: Lei Yang <leiyang@redhat.com>
>
>
>
> On Tue, May 9, 2023 at 1:28 AM Eugenio Perez Martin <eperezma@redhat.com>
> wrote:
> >
> > On Sat, May 6, 2023 at 5:01 PM Hawkins Jiawei <yin31149@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 <yin31149@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 <eperezma@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
> > >
> >
> >
>

Re: [PATCH RESEND] vhost: fix possible wrap in SVQ descriptor ring

Posted by Hawkins Jiawei 11 months, 3 weeks ago

Hi Eugenio,
Thanks for reviewing.

On 2023/5/9 1:26, Eugenio Perez Martin wrote:
> On Sat, May 6, 2023 at 5:01 PM Hawkins Jiawei <yin31149@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.

I will add this in the commit message in v2 patch.

>
>> 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 <yin31149@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 <eperezma@redhat.com>
>

Thanks for your suggestion. I will remove the
comment in v2 patch, with this tag on.


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