Add VIRTIO_F_IN_ORDER support to vhost shadow virtqueue

[PATCH 5/7] vhost: factor out the detach buf logic in SVQ

Posted by Eugenio Pérez 1 month, 1 week ago

This code path is modified to handle in order devices.  Abstract here so
we can generalize on the caller.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index e7e3c9155cd0..2d8fc82cc06f 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -473,11 +473,24 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
     return i;
 }
 
+G_GNUC_WARN_UNUSED_RESULT
+static VirtQueueElement *vhost_svq_detach_buf(VhostShadowVirtqueue *svq,
+                                              uint16_t id)
+{
+    uint16_t num = svq->desc_state[id].ndescs;
+    uint16_t last_used_chain = vhost_svq_last_desc_of_chain(svq, num, id);
+
+    svq->desc_state[last_used_chain].next = svq->free_head;
+    svq->free_head = id;
+
+    return g_steal_pointer(&svq->desc_state[id].elem);
+}
+
 G_GNUC_WARN_UNUSED_RESULT
 static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
                                            uint32_t *len)
 {
-    uint16_t last_used, last_used_chain, num;
+    uint16_t last_used;
 
     if (!vhost_svq_more_used(svq)) {
         return NULL;
@@ -500,14 +513,9 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
         return NULL;
     }
 
-    num = svq->desc_state[last_used].ndescs;
+    svq->num_free += svq->desc_state[last_used].ndescs;
     svq->desc_state[last_used].ndescs = 0;
-    last_used_chain = vhost_svq_last_desc_of_chain(svq, num, last_used);
-    svq->desc_state[last_used_chain].next = svq->free_head;
-    svq->free_head = last_used;
-    svq->num_free += num;
-
-    return g_steal_pointer(&svq->desc_state[last_used].elem);
+    return vhost_svq_detach_buf(svq, last_used);
 }
 
 /**
-- 
2.53.0

Re: [PATCH 5/7] vhost: factor out the detach buf logic in SVQ

Posted by Jason Wang 1 month ago

On Thu, Mar 5, 2026 at 1:36 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> This code path is modified to handle in order devices.  Abstract here so
> we can generalize on the caller.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-shadow-virtqueue.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index e7e3c9155cd0..2d8fc82cc06f 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -473,11 +473,24 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
>      return i;
>  }
>
> +G_GNUC_WARN_UNUSED_RESULT
> +static VirtQueueElement *vhost_svq_detach_buf(VhostShadowVirtqueue *svq,
> +                                              uint16_t id)
> +{
> +    uint16_t num = svq->desc_state[id].ndescs;
> +    uint16_t last_used_chain = vhost_svq_last_desc_of_chain(svq, num, id);
> +
> +    svq->desc_state[last_used_chain].next = svq->free_head;
> +    svq->free_head = id;
> +
> +    return g_steal_pointer(&svq->desc_state[id].elem);
> +}
> +
>  G_GNUC_WARN_UNUSED_RESULT
>  static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>                                             uint32_t *len)
>  {
> -    uint16_t last_used, last_used_chain, num;
> +    uint16_t last_used;
>
>      if (!vhost_svq_more_used(svq)) {
>          return NULL;
> @@ -500,14 +513,9 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>          return NULL;
>      }
>
> -    num = svq->desc_state[last_used].ndescs;
> +    svq->num_free += svq->desc_state[last_used].ndescs;
>      svq->desc_state[last_used].ndescs = 0;

Any reason we don't factor those two lines?

> -    last_used_chain = vhost_svq_last_desc_of_chain(svq, num, last_used);
> -    svq->desc_state[last_used_chain].next = svq->free_head;
> -    svq->free_head = last_used;
> -    svq->num_free += num;
> -
> -    return g_steal_pointer(&svq->desc_state[last_used].elem);
> +    return vhost_svq_detach_buf(svq, last_used);
>  }
>
>  /**
> --
> 2.53.0
>

Thanks

Re: [PATCH 5/7] vhost: factor out the detach buf logic in SVQ

Posted by Eugenio Perez Martin 1 month ago

On Mon, Mar 9, 2026 at 9:57 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Mar 5, 2026 at 1:36 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > This code path is modified to handle in order devices.  Abstract here so
> > we can generalize on the caller.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-shadow-virtqueue.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > index e7e3c9155cd0..2d8fc82cc06f 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -473,11 +473,24 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> >      return i;
> >  }
> >
> > +G_GNUC_WARN_UNUSED_RESULT
> > +static VirtQueueElement *vhost_svq_detach_buf(VhostShadowVirtqueue *svq,
> > +                                              uint16_t id)
> > +{
> > +    uint16_t num = svq->desc_state[id].ndescs;
> > +    uint16_t last_used_chain = vhost_svq_last_desc_of_chain(svq, num, id);
> > +
> > +    svq->desc_state[last_used_chain].next = svq->free_head;
> > +    svq->free_head = id;
> > +
> > +    return g_steal_pointer(&svq->desc_state[id].elem);
> > +}
> > +
> >  G_GNUC_WARN_UNUSED_RESULT
> >  static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> >                                             uint32_t *len)
> >  {
> > -    uint16_t last_used, last_used_chain, num;
> > +    uint16_t last_used;
> >
> >      if (!vhost_svq_more_used(svq)) {
> >          return NULL;
> > @@ -500,14 +513,9 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> >          return NULL;
> >      }
> >
> > -    num = svq->desc_state[last_used].ndescs;
> > +    svq->num_free += svq->desc_state[last_used].ndescs;
> >      svq->desc_state[last_used].ndescs = 0;
>
> Any reason we don't factor those two lines?
>

They are common for split and split_in_order case.

> > -    last_used_chain = vhost_svq_last_desc_of_chain(svq, num, last_used);
> > -    svq->desc_state[last_used_chain].next = svq->free_head;
> > -    svq->free_head = last_used;
> > -    svq->num_free += num;
> > -
> > -    return g_steal_pointer(&svq->desc_state[last_used].elem);
> > +    return vhost_svq_detach_buf(svq, last_used);
> >  }
> >
> >  /**
> > --
> > 2.53.0
> >
>
> Thanks
>

Re: [PATCH 5/7] vhost: factor out the detach buf logic in SVQ

Posted by Jason Wang 1 month ago

On Mon, Mar 9, 2026 at 5:43 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Mon, Mar 9, 2026 at 9:57 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Mar 5, 2026 at 1:36 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > This code path is modified to handle in order devices.  Abstract here so
> > > we can generalize on the caller.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  hw/virtio/vhost-shadow-virtqueue.c | 24 ++++++++++++++++--------
> > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > index e7e3c9155cd0..2d8fc82cc06f 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -473,11 +473,24 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > >      return i;
> > >  }
> > >
> > > +G_GNUC_WARN_UNUSED_RESULT
> > > +static VirtQueueElement *vhost_svq_detach_buf(VhostShadowVirtqueue *svq,
> > > +                                              uint16_t id)
> > > +{
> > > +    uint16_t num = svq->desc_state[id].ndescs;
> > > +    uint16_t last_used_chain = vhost_svq_last_desc_of_chain(svq, num, id);
> > > +
> > > +    svq->desc_state[last_used_chain].next = svq->free_head;
> > > +    svq->free_head = id;
> > > +
> > > +    return g_steal_pointer(&svq->desc_state[id].elem);
> > > +}
> > > +
> > >  G_GNUC_WARN_UNUSED_RESULT
> > >  static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> > >                                             uint32_t *len)
> > >  {
> > > -    uint16_t last_used, last_used_chain, num;
> > > +    uint16_t last_used;
> > >
> > >      if (!vhost_svq_more_used(svq)) {
> > >          return NULL;
> > > @@ -500,14 +513,9 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> > >          return NULL;
> > >      }
> > >
> > > -    num = svq->desc_state[last_used].ndescs;
> > > +    svq->num_free += svq->desc_state[last_used].ndescs;
> > >      svq->desc_state[last_used].ndescs = 0;
> >
> > Any reason we don't factor those two lines?
> >
>
> They are common for split and split_in_order case.

Exactly, so why not factoring out them as well? Or you mean
vhost_svq_detach_buf is only for in-order (then we probably need to
rename).

Thanks

>
> > > -    last_used_chain = vhost_svq_last_desc_of_chain(svq, num, last_used);
> > > -    svq->desc_state[last_used_chain].next = svq->free_head;
> > > -    svq->free_head = last_used;
> > > -    svq->num_free += num;
> > > -
> > > -    return g_steal_pointer(&svq->desc_state[last_used].elem);
> > > +    return vhost_svq_detach_buf(svq, last_used);
> > >  }
> > >
> > >  /**
> > > --
> > > 2.53.0
> > >
> >
> > Thanks
> >
>

Re: [PATCH 5/7] vhost: factor out the detach buf logic in SVQ

Posted by Eugenio Perez Martin 1 month ago

On Tue, Mar 10, 2026 at 4:04 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Mar 9, 2026 at 5:43 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
> >
> > On Mon, Mar 9, 2026 at 9:57 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Mar 5, 2026 at 1:36 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > This code path is modified to handle in order devices.  Abstract here so
> > > > we can generalize on the caller.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  hw/virtio/vhost-shadow-virtqueue.c | 24 ++++++++++++++++--------
> > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > > index e7e3c9155cd0..2d8fc82cc06f 100644
> > > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > > @@ -473,11 +473,24 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
> > > >      return i;
> > > >  }
> > > >
> > > > +G_GNUC_WARN_UNUSED_RESULT
> > > > +static VirtQueueElement *vhost_svq_detach_buf(VhostShadowVirtqueue *svq,
> > > > +                                              uint16_t id)
> > > > +{
> > > > +    uint16_t num = svq->desc_state[id].ndescs;
> > > > +    uint16_t last_used_chain = vhost_svq_last_desc_of_chain(svq, num, id);
> > > > +
> > > > +    svq->desc_state[last_used_chain].next = svq->free_head;
> > > > +    svq->free_head = id;
> > > > +
> > > > +    return g_steal_pointer(&svq->desc_state[id].elem);
> > > > +}
> > > > +
> > > >  G_GNUC_WARN_UNUSED_RESULT
> > > >  static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> > > >                                             uint32_t *len)
> > > >  {
> > > > -    uint16_t last_used, last_used_chain, num;
> > > > +    uint16_t last_used;
> > > >
> > > >      if (!vhost_svq_more_used(svq)) {
> > > >          return NULL;
> > > > @@ -500,14 +513,9 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
> > > >          return NULL;
> > > >      }
> > > >
> > > > -    num = svq->desc_state[last_used].ndescs;
> > > > +    svq->num_free += svq->desc_state[last_used].ndescs;
> > > >      svq->desc_state[last_used].ndescs = 0;
> > >
> > > Any reason we don't factor those two lines?
> > >
> >
> > They are common for split and split_in_order case.
>
> Exactly, so why not factoring out them as well? Or you mean
> vhost_svq_detach_buf is only for in-order (then we probably need to
> rename).
>

This is the relevant code after applying the series:
static VirtQueueElement *vhost_svq_detach_buf(VhostShadowVirtqueue *svq,
                                              uint16_t id)
{
    if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_IN_ORDER)) {
        return vhost_svq_detach_buf_split_in_order(svq, id);
    } else {
        return vhost_svq_detach_buf_split(svq, id);
    }
}

G_GNUC_WARN_UNUSED_RESULT
static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
                                           uint32_t *len)
{
    uint16_t last_used;

    if (!vhost_svq_more_used(svq)) {
        return NULL;
    }

    /* Only get used array entries after they have been exposed by dev */
    smp_rmb();

    if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_IN_ORDER)) {
        int32_t r;
        r = vhost_svq_get_last_used_split_in_order(svq, len);
        if (r < 0) {
            return NULL;
        }

        last_used = r;
    } else {
        last_used = vhost_svq_get_last_used_split(svq, len);
    }

    if (unlikely(last_used >= svq->vring.num)) {
        qemu_log_mask(LOG_GUEST_ERROR, "Device %s says index %u is used",
                      svq->vdev->name, last_used);
        return NULL;
    }

    if (unlikely(!svq->desc_state[last_used].ndescs)) {
        qemu_log_mask(LOG_GUEST_ERROR,
            "Device %s says index %u is used, but it was not available",
            svq->vdev->name, last_used);
        return NULL;
    }

    svq->num_free += svq->desc_state[last_used].ndescs;
    svq->desc_state[last_used].ndescs = 0;
    return vhost_svq_detach_buf(svq, last_used);
}
---

Whether the code is in vhost_svq_detach_buf or vhost_svq_get_buf it's
abstracted from the in order or the out of order version.

The code is in vhost_svq_get_buf because the long term plan is to make
vhost_svq_detach_buf similar to your VIRTQUEUE_CALL macro in the Linux
kernel—a dispatcher based on the acked features. But I'm willing to
move it to vhost_svq_detach_buf in this series if you prefer.

Do you want a V2 with the two lines of code moved to vhost_svq_detach_buf?

Thanks!

[PATCH 1/7] virtio: Allow to fill a whole virtqueue in order
[PATCH 2/7] vhost: move svq next desc array to descs state struct
[PATCH 3/7] vhost: factor out the descriptor next fetching
[PATCH 4/7] vhost: factor out the get of last used desc in SVQ
[PATCH 5/7] vhost: factor out the detach buf logic in SVQ
[PATCH 6/7] vhost: add in_order feature to shadow virtqueue
[PATCH 7/7] vhost: accept in order feature flag