> -----Original Message-----
> From: Eugenio Perez Martin <[email protected]>
> Sent: 2025年12月19日 16:12
> To: Wafer <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Angus Chen
> <[email protected]>
> Subject: Re: [PATCH v3 3/4] vhost: SVQ get the indirect descriptors from used
> ring
>
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
>
>
> On Tue, Dec 16, 2025 at 2:57 AM Wafer Xie <[email protected]> wrote:
> >
> > From: wafer Xie <[email protected]>
> >
> > For the used ring, based on the state of the SVQ descriptor, if it is
> > indirect, retrieve the corresponding indirect buffer by index and then
> > increment the freeing counter.
> > Once all descriptors in this buffer have been released, update the
> > current buffer state to SVQ_INDIRECT_BUF_FREED.
> >
> > Signed-off-by: wafer Xie <[email protected]>
> > ---
> > hw/virtio/vhost-shadow-virtqueue.c | 43
> > +++++++++++++++++++++++++++---
> > 1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index 85eff1d841..adee52f50b 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -469,12 +469,47 @@ static VirtQueueElement
> *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> > return NULL;
> > }
> >
> > + /* Check if indirect descriptors were used */
> > + int indirect_buf_idx = svq->desc_state[used_elem.id].indirect_buf_idx;
> > + bool used_indirect = (indirect_buf_idx >= 0);
> > +
> > + /* Update indirect buffer state if it was used */
> > + if (used_indirect) {
> > + SVQIndirectDescBuf *buf = &svq->indirect_bufs[indirect_buf_idx];
> > + unsigned int ndescs = svq->desc_state[used_elem.id].ndescs;
>
> Nitpick, we could just populate num = and reuse here.
>
Thank you for the suggestion. I will modify this in the next version of the
patch.
> > +
> > + /* Increment freeing_descs for descriptors returned from used ring
> */
> > + buf->freeing_descs += ndescs;
> > +
> > + /* Check if all descs are accounted for (freed + freeing == num) */
> > + if (buf->freed_descs + buf->freeing_descs >= buf->num_descs) {
> > + /* Reset buffer to freed state */
> > + buf->state = SVQ_INDIRECT_BUF_FREED;
> > + buf->freed_descs = buf->num_descs;
> > + buf->freeing_descs = 0;
> > + buf->freed_head = 0;
> > + }
> > +
> > + svq->desc_state[used_elem.id].indirect_buf_idx = -1;
>
> Why not continue using the buffer in a circular manner?
>
The last descriptor in the array cannot wrap around and be chained with the
descriptor at index 0 when used as an indirect descriptor table, as indirect
descriptors must be contiguous.
> > + }
> > +
> > num = svq->desc_state[used_elem.id].ndescs;
> > svq->desc_state[used_elem.id].ndescs = 0;
> > - last_used_chain = vhost_svq_last_desc_of_chain(svq, num,
> used_elem.id);
> > - svq->desc_next[last_used_chain] = svq->free_head;
> > - svq->free_head = used_elem.id;
> > - svq->num_free += num;
> > +
> > + /*
> > + * If using indirect descriptors, only 1 main descriptor is used.
> > + * Otherwise, we used 'num' descriptors in a chain.
> > + */
> > + if (used_indirect) {
> > + svq->desc_next[used_elem.id] = svq->free_head;
> > + svq->free_head = used_elem.id;
> > + svq->num_free += 1;
> > + } else {
> > + last_used_chain = vhost_svq_last_desc_of_chain(svq, num,
> used_elem.id);
> > + svq->desc_next[last_used_chain] = svq->free_head;
> > + svq->free_head = used_elem.id;
> > + svq->num_free += num;
> > + }
>
> I think the two paths are equivalent if the descriptor chain length is
> 1 except for the num_free += indirect ? num : 1, can we merge both paths
> and just make it conditional?
>
Thank you for the suggestion. I will modify this in the next version of the
patch.
>
> >
> > *len = used_elem.len;
> > return g_steal_pointer(&svq->desc_state[used_elem.id].elem);
> > --
> > 2.48.1
> >
