[PATCH 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support

[PATCH 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support

[Jonah Palmer]

The goal of these patches is to add support to a variety of virtio and
vhost devices for the VIRTIO_F_IN_ORDER transport feature. This feature
indicates that all buffers are used by the device in the same order in
which they were made available by the driver.

These patches attempt to implement a generalized, non-device-specific
solution to support this feature.

The core feature behind this solution is a buffer mechanism in the form
of a VirtQueue's used_elems VirtQueueElement array. This allows devices
who always use buffers in-order by default to have a minimal overhead
impact. Devices that may not always use buffers in-order likely will
experience a performance hit. How large that performance hit is will
depend on how frequent elements are completed out-of-order.

A VirtQueue whose device who uses this feature will use its used_elems
VirtQueueElement array to hold used VirtQueueElements. The index that
used elements are placed in used_elems is the same index on the
used/descriptor ring that would satisfy the in-order requirement. In
other words, used elements are placed in their in-order locations on
used_elems and are only written to the used/descriptor ring once the
elements on used_elems are able to continue their expected order.

To differentiate between a "used" and "unused" element on the used_elems
array (a "used" element being an element that has returned from
processing and an "unused" element being an element that has not yet
been processed), we added a boolean 'filled' member to the
VirtQueueElement struct. This flag is set to true when the element comes
back from processing (virtqueue_ordered_fill) and then set back to false
once it's been written to the used/descriptor ring
(virtqueue_ordered_flush).

---
v1: Move series from RFC to PATCH for submission.

Jonah Palmer (6):
  virtio: Add bool to VirtQueueElement
  virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support
  virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
  virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support
  vhost,vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits
  virtio: Add VIRTIO_F_IN_ORDER property definition

 hw/block/vhost-user-blk.c    |   1 +
 hw/net/vhost_net.c           |   2 +
 hw/scsi/vhost-scsi.c         |   1 +
 hw/scsi/vhost-user-scsi.c    |   1 +
 hw/virtio/vhost-user-fs.c    |   1 +
 hw/virtio/vhost-user-vsock.c |   1 +
 hw/virtio/virtio.c           | 118 ++++++++++++++++++++++++++++++++++-
 include/hw/virtio/virtio.h   |   5 +-
 net/vhost-vdpa.c             |   1 +
 9 files changed, 127 insertions(+), 4 deletions(-)

-- 
2.39.3

[PATCH 1/6] virtio: Add bool to VirtQueueElement

[Jonah Palmer]

Add the boolean 'filled' member to the VirtQueueElement structure. The
use of this boolean will signify if the element has been written to the
used / descriptor ring or not. This boolean is used to support the
VIRTIO_F_IN_ORDER feature.

Tested-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 include/hw/virtio/virtio.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 7d5ffdc145..9ed9c3763c 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -69,6 +69,7 @@ typedef struct VirtQueueElement
     unsigned int ndescs;
     unsigned int out_num;
     unsigned int in_num;
+    bool filled;
     hwaddr *in_addr;
     hwaddr *out_addr;
     struct iovec *in_sg;
-- 
2.39.3

[PATCH 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support

[Jonah Palmer]

Add VIRTIO_F_IN_ORDER feature support in virtqueue_split_pop and
virtqueue_packed_pop.

VirtQueueElements popped from the available/descritpor ring are added to
the VirtQueue's used_elems array in-order and in the same fashion as
they would be added the used and descriptor rings, respectively.

This will allow us to keep track of the current order, what elements
have been written, as well as an element's essential data after being
processed.

Tested-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/virtio.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 893a072c9d..e6eb1bb453 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1506,7 +1506,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu
 
 static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
 {
-    unsigned int i, head, max;
+    unsigned int i, j, head, max;
     VRingMemoryRegionCaches *caches;
     MemoryRegionCache indirect_desc_cache;
     MemoryRegionCache *desc_cache;
@@ -1539,6 +1539,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
         goto done;
     }
 
+    j = vq->last_avail_idx;
+
     if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) {
         goto done;
     }
@@ -1630,6 +1632,12 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
         elem->in_sg[i] = iov[out_num + i];
     }
 
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
+        vq->used_elems[j].index = elem->index;
+        vq->used_elems[j].len = elem->len;
+        vq->used_elems[j].ndescs = elem->ndescs;
+    }
+
     vq->inuse++;
 
     trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
@@ -1758,6 +1766,13 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz)
 
     elem->index = id;
     elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries;
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
+        vq->used_elems[vq->last_avail_idx].index = elem->index;
+        vq->used_elems[vq->last_avail_idx].len = elem->len;
+        vq->used_elems[vq->last_avail_idx].ndescs = elem->ndescs;
+    }
+
     vq->last_avail_idx += elem->ndescs;
     vq->inuse += elem->ndescs;
 
-- 
2.39.3

[PATCH 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support

[Jonah Palmer]

Add VIRTIO_F_IN_ORDER feature support for virtqueue_fill operations.

The goal of the virtqueue_fill operation when the VIRTIO_F_IN_ORDER
feature has been negotiated is to search for this now-used element,
set its length, and mark the element as filled in the VirtQueue's
used_elems array.

By marking the element as filled, it will indicate that this element is
ready to be flushed, so long as the element is in-order.

Tested-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/virtio.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e6eb1bb453..064046b5e2 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -873,6 +873,28 @@ static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem,
     vq->used_elems[idx].ndescs = elem->ndescs;
 }
 
+static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
+                                   unsigned int len)
+{
+    unsigned int i = vq->used_idx;
+
+    /* Search for element in vq->used_elems */
+    while (i != vq->last_avail_idx) {
+        /* Found element, set length and mark as filled */
+        if (vq->used_elems[i].index == elem->index) {
+            vq->used_elems[i].len = len;
+            vq->used_elems[i].filled = true;
+            break;
+        }
+
+        i += vq->used_elems[i].ndescs;
+
+        if (i >= vq->vring.num) {
+            i -= vq->vring.num;
+        }
+    }
+}
+
 static void virtqueue_packed_fill_desc(VirtQueue *vq,
                                        const VirtQueueElement *elem,
                                        unsigned int idx,
@@ -923,7 +945,9 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
         return;
     }
 
-    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
+        virtqueue_ordered_fill(vq, elem, len);
+    } else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
         virtqueue_packed_fill(vq, elem, len, idx);
     } else {
         virtqueue_split_fill(vq, elem, len, idx);
-- 
2.39.3

[PATCH 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support

[Jonah Palmer]

Add VIRTIO_F_IN_ORDER feature support for virtqueue_flush operations.

The goal of the virtqueue_flush operation when the VIRTIO_F_IN_ORDER
feature has been negotiated is to write elements to the used/descriptor
ring in-order and then update used_idx.

The function iterates through the VirtQueueElement used_elems array
in-order starting at vq->used_idx. If the element is valid (filled), the
element is written to the used/descriptor ring. This process continues
until we find an invalid (not filled) element.

If any elements were written, the used_idx is updated.

Tested-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/virtio.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 064046b5e2..0efed2c88e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1006,6 +1006,77 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
     }
 }
 
+static void virtqueue_ordered_flush(VirtQueue *vq)
+{
+    unsigned int i = vq->used_idx;
+    unsigned int ndescs = 0;
+    uint16_t old = vq->used_idx;
+    bool packed;
+    VRingUsedElem uelem;
+
+    packed = virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED);
+
+    if (packed) {
+        if (unlikely(!vq->vring.desc)) {
+            return;
+        }
+    } else if (unlikely(!vq->vring.used)) {
+        return;
+    }
+
+    /* First expected in-order element isn't ready, nothing to do */
+    if (!vq->used_elems[i].filled) {
+        return;
+    }
+
+    /* Write first expected in-order element to used ring (split VQs) */
+    if (!packed) {
+        uelem.id = vq->used_elems[i].index;
+        uelem.len = vq->used_elems[i].len;
+        vring_used_write(vq, &uelem, i);
+    }
+
+    ndescs += vq->used_elems[i].ndescs;
+    i += ndescs;
+    if (i >= vq->vring.num) {
+        i -= vq->vring.num;
+    }
+
+    /* Search for more filled elements in-order */
+    while (vq->used_elems[i].filled) {
+        if (packed) {
+            virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs, false);
+        } else {
+            uelem.id = vq->used_elems[i].index;
+            uelem.len = vq->used_elems[i].len;
+            vring_used_write(vq, &uelem, i);
+        }
+
+        vq->used_elems[i].filled = false;
+        ndescs += vq->used_elems[i].ndescs;
+        i += ndescs;
+        if (i >= vq->vring.num) {
+            i -= vq->vring.num;
+        }
+    }
+
+    if (packed) {
+        virtqueue_packed_fill_desc(vq, &vq->used_elems[vq->used_idx], 0, true);
+        vq->used_idx += ndescs;
+        if (vq->used_idx >= vq->vring.num) {
+            vq->used_idx -= vq->vring.num;
+            vq->used_wrap_counter ^= 1;
+            vq->signalled_used_valid = false;
+        }
+    } else {
+        vring_used_idx_set(vq, i);
+        if (unlikely((int16_t)(i - vq->signalled_used) < (uint16_t)(i - old))) {
+            vq->signalled_used_valid = false;
+        }
+    }
+    vq->inuse -= ndescs;
+}
+
 void virtqueue_flush(VirtQueue *vq, unsigned int count)
 {
     if (virtio_device_disabled(vq->vdev)) {
@@ -1013,7 +1084,9 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
         return;
     }
 
-    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
+        virtqueue_ordered_flush(vq);
+    } else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
         virtqueue_packed_flush(vq, count);
     } else {
         virtqueue_split_flush(vq, count);
-- 
2.39.3

[PATCH 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits

[Jonah Palmer via]

Add support for the VIRTIO_F_IN_ORDER feature across a variety of vhost
devices.

The inclusion of VIRTIO_F_IN_ORDER in the feature bits arrays for these
devices ensures that the backend is capable of offering and providing
support for this feature, and that it can be disabled if the backend
does not support it.

Tested-by: Lei Yang <leiyang@redhat.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/block/vhost-user-blk.c    | 1 +
 hw/net/vhost_net.c           | 2 ++
 hw/scsi/vhost-scsi.c         | 1 +
 hw/scsi/vhost-user-scsi.c    | 1 +
 hw/virtio/vhost-user-fs.c    | 1 +
 hw/virtio/vhost-user-vsock.c | 1 +
 net/vhost-vdpa.c             | 1 +
 7 files changed, 8 insertions(+)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9e6bbc6950..1dd0a8ef63 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -51,6 +51,7 @@ static const int user_feature_bits[] = {
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_IN_ORDER,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index fd1a93701a..eb0b1c06e5 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -48,6 +48,7 @@ static const int kernel_feature_bits[] = {
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_IN_ORDER,
     VIRTIO_NET_F_HASH_REPORT,
     VHOST_INVALID_FEATURE_BIT
 };
@@ -76,6 +77,7 @@ static const int user_feature_bits[] = {
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_IN_ORDER,
     VIRTIO_NET_F_RSS,
     VIRTIO_NET_F_HASH_REPORT,
     VIRTIO_NET_F_GUEST_USO4,
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index ae26bc19a4..40e7630191 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -38,6 +38,7 @@ static const int kernel_feature_bits[] = {
     VIRTIO_RING_F_EVENT_IDX,
     VIRTIO_SCSI_F_HOTPLUG,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_IN_ORDER,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index a63b1f4948..1d59951ab7 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -36,6 +36,7 @@ static const int user_feature_bits[] = {
     VIRTIO_RING_F_EVENT_IDX,
     VIRTIO_SCSI_F_HOTPLUG,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_IN_ORDER,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index cca2cd41be..9243dbb128 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -33,6 +33,7 @@ static const int user_feature_bits[] = {
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_IN_ORDER,
 
     VHOST_INVALID_FEATURE_BIT
 };
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 9431b9792c..cc7e4e47b4 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -21,6 +21,7 @@ static const int user_feature_bits[] = {
     VIRTIO_RING_F_INDIRECT_DESC,
     VIRTIO_RING_F_EVENT_IDX,
     VIRTIO_F_NOTIFY_ON_EMPTY,
+    VIRTIO_F_IN_ORDER,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 85e73dd6a7..ed3185acfa 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -62,6 +62,7 @@ const int vdpa_feature_bits[] = {
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_RING_RESET,
     VIRTIO_F_VERSION_1,
+    VIRTIO_F_IN_ORDER,
     VIRTIO_NET_F_CSUM,
     VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
     VIRTIO_NET_F_CTRL_MAC_ADDR,
-- 
2.39.3

[PATCH 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition

[Jonah Palmer]

Extend the virtio device property definitions to include the
VIRTIO_F_IN_ORDER feature.

The default state of this feature is disabled, allowing it to be
explicitly enabled where it's supported.

Tested-by: Lei Yang <leiyang@redhat.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 include/hw/virtio/virtio.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9ed9c3763c..30c23400e3 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -370,7 +370,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
     DEFINE_PROP_BIT64("packed", _state, _field, \
                       VIRTIO_F_RING_PACKED, false), \
     DEFINE_PROP_BIT64("queue_reset", _state, _field, \
-                      VIRTIO_F_RING_RESET, true)
+                      VIRTIO_F_RING_RESET, true), \
+    DEFINE_PROP_BIT64("in_order", _state, _field, \
+                      VIRTIO_F_IN_ORDER, false)
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
-- 
2.39.3

Re: [PATCH 1/6] virtio: Add bool to VirtQueueElement

[Eugenio Perez Martin]

On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add the boolean 'filled' member to the VirtQueueElement structure. The
> use of this boolean will signify if the element has been written to the
> used / descriptor ring or not. This boolean is used to support the
> VIRTIO_F_IN_ORDER feature.
>
> Tested-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  include/hw/virtio/virtio.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 7d5ffdc145..9ed9c3763c 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -69,6 +69,7 @@ typedef struct VirtQueueElement
>      unsigned int ndescs;
>      unsigned int out_num;
>      unsigned int in_num;
> +    bool filled;

in_order_filled? I cannot come with a good name for this. Maybe we can
add a comment on top of the variable so we know what it is used for?

>      hwaddr *in_addr;
>      hwaddr *out_addr;
>      struct iovec *in_sg;
> --
> 2.39.3
>

Re: [PATCH 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support

[Eugenio Perez Martin]

On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add VIRTIO_F_IN_ORDER feature support in virtqueue_split_pop and
> virtqueue_packed_pop.
>
> VirtQueueElements popped from the available/descritpor ring are added to
> the VirtQueue's used_elems array in-order and in the same fashion as
> they would be added the used and descriptor rings, respectively.
>
> This will allow us to keep track of the current order, what elements
> have been written, as well as an element's essential data after being
> processed.
>
> Tested-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 893a072c9d..e6eb1bb453 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1506,7 +1506,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu
>
>  static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>  {
> -    unsigned int i, head, max;
> +    unsigned int i, j, head, max;
>      VRingMemoryRegionCaches *caches;
>      MemoryRegionCache indirect_desc_cache;
>      MemoryRegionCache *desc_cache;
> @@ -1539,6 +1539,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>          goto done;
>      }
>
> +    j = vq->last_avail_idx;
> +
>      if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) {
>          goto done;
>      }
> @@ -1630,6 +1632,12 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>          elem->in_sg[i] = iov[out_num + i];
>      }
>
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
> +        vq->used_elems[j].index = elem->index;
> +        vq->used_elems[j].len = elem->len;
> +        vq->used_elems[j].ndescs = elem->ndescs;
> +    }
> +
>      vq->inuse++;
>
>      trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
> @@ -1758,6 +1766,13 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz)
>
>      elem->index = id;
>      elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries;
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
> +        vq->used_elems[vq->last_avail_idx].index = elem->index;
> +        vq->used_elems[vq->last_avail_idx].len = elem->len;
> +        vq->used_elems[vq->last_avail_idx].ndescs = elem->ndescs;
> +    }
> +

I suggest using a consistent style between packed and split: Either
always use vq->last_avail_idx or j. If you use j, please rename to
something more related to the usage, as j is usually for iterations.

In my opinion I think vq->last_avail_idx is better.


>      vq->last_avail_idx += elem->ndescs;
>      vq->inuse += elem->ndescs;
>
> --
> 2.39.3
>

Re: [PATCH 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support

[Eugenio Perez Martin]

On Mon, May 6, 2024 at 5:05 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add VIRTIO_F_IN_ORDER feature support for virtqueue_fill operations.
>
> The goal of the virtqueue_fill operation when the VIRTIO_F_IN_ORDER
> feature has been negotiated is to search for this now-used element,
> set its length, and mark the element as filled in the VirtQueue's
> used_elems array.
>
> By marking the element as filled, it will indicate that this element is
> ready to be flushed, so long as the element is in-order.
>
> Tested-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e6eb1bb453..064046b5e2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -873,6 +873,28 @@ static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem,
>      vq->used_elems[idx].ndescs = elem->ndescs;
>  }
>
> +static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
> +                                   unsigned int len)
> +{
> +    unsigned int i = vq->used_idx;
> +
> +    /* Search for element in vq->used_elems */
> +    while (i != vq->last_avail_idx) {
> +        /* Found element, set length and mark as filled */
> +        if (vq->used_elems[i].index == elem->index) {
> +            vq->used_elems[i].len = len;
> +            vq->used_elems[i].filled = true;
> +            break;
> +        }
> +
> +        i += vq->used_elems[i].ndescs;
> +
> +        if (i >= vq->vring.num) {
> +            i -= vq->vring.num;
> +        }
> +    }

This has a subtle problem: ndescs and elems->id are controlled by the
guest, so it could make QEMU to loop forever looking for the right
descriptor. For each iteration, the code must control that the
variable "i" will be different for the next iteration, and that there
will be no more than vq->last_avail_idx - vq->used_idx iterations.

Apart of that, I think it makes more sense to split the logical
sections of the function this way:
/* declarations */
i = vq->used_idx

/* Search for element in vq->used_elems */
while (vq->used_elems[i].index != elem->index &&
vq->used_elems[i].index i != vq->last_avail_idx && ...) {
...
}

/* Set length and mark as filled */
vq->used_elems[i].len = len;
vq->used_elems[i].filled = true;
---

But I'm ok either way.

> +}
> +
>  static void virtqueue_packed_fill_desc(VirtQueue *vq,
>                                         const VirtQueueElement *elem,
>                                         unsigned int idx,
> @@ -923,7 +945,9 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>          return;
>      }
>
> -    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
> +        virtqueue_ordered_fill(vq, elem, len);
> +    } else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>          virtqueue_packed_fill(vq, elem, len, idx);
>      } else {
>          virtqueue_split_fill(vq, elem, len, idx);
> --
> 2.39.3
>

Re: [PATCH 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support

[Eugenio Perez Martin]

On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add VIRTIO_F_IN_ORDER feature support for virtqueue_flush operations.
>
> The goal of the virtqueue_flush operation when the VIRTIO_F_IN_ORDER
> feature has been negotiated is to write elements to the used/descriptor
> ring in-order and then update used_idx.
>
> The function iterates through the VirtQueueElement used_elems array
> in-order starting at vq->used_idx. If the element is valid (filled), the
> element is written to the used/descriptor ring. This process continues
> until we find an invalid (not filled) element.
>
> If any elements were written, the used_idx is updated.
>
> Tested-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 064046b5e2..0efed2c88e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1006,6 +1006,77 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>      }
>  }
>
> +static void virtqueue_ordered_flush(VirtQueue *vq)
> +{
> +    unsigned int i = vq->used_idx;
> +    unsigned int ndescs = 0;
> +    uint16_t old = vq->used_idx;
> +    bool packed;
> +    VRingUsedElem uelem;
> +
> +    packed = virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED);
> +
> +    if (packed) {
> +        if (unlikely(!vq->vring.desc)) {
> +            return;
> +        }
> +    } else if (unlikely(!vq->vring.used)) {
> +        return;
> +    }
> +
> +    /* First expected in-order element isn't ready, nothing to do */
> +    if (!vq->used_elems[i].filled) {
> +        return;
> +    }
> +
> +    /* Write first expected in-order element to used ring (split VQs) */
> +    if (!packed) {
> +        uelem.id = vq->used_elems[i].index;
> +        uelem.len = vq->used_elems[i].len;
> +        vring_used_write(vq, &uelem, i);
> +    }
> +
> +    ndescs += vq->used_elems[i].ndescs;
> +    i += ndescs;
> +    if (i >= vq->vring.num) {
> +        i -= vq->vring.num;
> +    }
> +
> +    /* Search for more filled elements in-order */
> +    while (vq->used_elems[i].filled) {
> +        if (packed) {
> +            virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs, false);
> +        } else {
> +            uelem.id = vq->used_elems[i].index;
> +            uelem.len = vq->used_elems[i].len;
> +            vring_used_write(vq, &uelem, i);
> +        }
> +
> +        vq->used_elems[i].filled = false;
> +        ndescs += vq->used_elems[i].ndescs;
> +        i += ndescs;
> +        if (i >= vq->vring.num) {
> +            i -= vq->vring.num;
> +        }
> +    }
> +

I may be missing something, but you have split out the first case as a
special one, totally out of the while loop. Can't it be contained in
the loop checking !(packed && i == vq->used_idx)? That would avoid
code duplication.

A comment can be added in the line of "first entry of packed is
written the last so the guest does not see invalid descriptors".

> +    if (packed) {
> +        virtqueue_packed_fill_desc(vq, &vq->used_elems[vq->used_idx], 0, true);
> +        vq->used_idx += ndescs;
> +        if (vq->used_idx >= vq->vring.num) {
> +            vq->used_idx -= vq->vring.num;
> +            vq->used_wrap_counter ^= 1;
> +            vq->signalled_used_valid = false;
> +        }
> +    } else {
> +        vring_used_idx_set(vq, i);
> +        if (unlikely((int16_t)(i - vq->signalled_used) < (uint16_t)(i - old))) {
> +            vq->signalled_used_valid = false;
> +        }
> +    }
> +    vq->inuse -= ndescs;
> +}
> +
>  void virtqueue_flush(VirtQueue *vq, unsigned int count)
>  {
>      if (virtio_device_disabled(vq->vdev)) {
> @@ -1013,7 +1084,9 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
>          return;
>      }
>
> -    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
> +        virtqueue_ordered_flush(vq);
> +    } else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>          virtqueue_packed_flush(vq, count);
>      } else {
>          virtqueue_split_flush(vq, count);
> --
> 2.39.3
>

Re: [PATCH 1/6] virtio: Add bool to VirtQueueElement

[Jonah Palmer]

On 5/9/24 8:32 AM, Eugenio Perez Martin wrote:
> On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> Add the boolean 'filled' member to the VirtQueueElement structure. The
>> use of this boolean will signify if the element has been written to the
>> used / descriptor ring or not. This boolean is used to support the
>> VIRTIO_F_IN_ORDER feature.
>>
>> Tested-by: Lei Yang <leiyang@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   include/hw/virtio/virtio.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 7d5ffdc145..9ed9c3763c 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -69,6 +69,7 @@ typedef struct VirtQueueElement
>>       unsigned int ndescs;
>>       unsigned int out_num;
>>       unsigned int in_num;
>> +    bool filled;
> 
> in_order_filled? I cannot come with a good name for this. Maybe we can
> add a comment on top of the variable so we know what it is used for?
> 

Will do! I can change the name to be more obvious as well.

>>       hwaddr *in_addr;
>>       hwaddr *out_addr;
>>       struct iovec *in_sg;
>> --
>> 2.39.3
>>
> 

Re: [PATCH 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support

[Jonah Palmer]

On 5/9/24 9:13 AM, Eugenio Perez Martin wrote:
> On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> Add VIRTIO_F_IN_ORDER feature support in virtqueue_split_pop and
>> virtqueue_packed_pop.
>>
>> VirtQueueElements popped from the available/descritpor ring are added to
>> the VirtQueue's used_elems array in-order and in the same fashion as
>> they would be added the used and descriptor rings, respectively.
>>
>> This will allow us to keep track of the current order, what elements
>> have been written, as well as an element's essential data after being
>> processed.
>>
>> Tested-by: Lei Yang <leiyang@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   hw/virtio/virtio.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 893a072c9d..e6eb1bb453 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1506,7 +1506,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu
>>
>>   static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>>   {
>> -    unsigned int i, head, max;
>> +    unsigned int i, j, head, max;
>>       VRingMemoryRegionCaches *caches;
>>       MemoryRegionCache indirect_desc_cache;
>>       MemoryRegionCache *desc_cache;
>> @@ -1539,6 +1539,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>>           goto done;
>>       }
>>
>> +    j = vq->last_avail_idx;
>> +
>>       if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) {
>>           goto done;
>>       }
>> @@ -1630,6 +1632,12 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>>           elem->in_sg[i] = iov[out_num + i];
>>       }
>>
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
>> +        vq->used_elems[j].index = elem->index;
>> +        vq->used_elems[j].len = elem->len;
>> +        vq->used_elems[j].ndescs = elem->ndescs;
>> +    }
>> +
>>       vq->inuse++;
>>
>>       trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
>> @@ -1758,6 +1766,13 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz)
>>
>>       elem->index = id;
>>       elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries;
>> +
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
>> +        vq->used_elems[vq->last_avail_idx].index = elem->index;
>> +        vq->used_elems[vq->last_avail_idx].len = elem->len;
>> +        vq->used_elems[vq->last_avail_idx].ndescs = elem->ndescs;
>> +    }
>> +
> 
> I suggest using a consistent style between packed and split: Either
> always use vq->last_avail_idx or j. If you use j, please rename to
> something more related to the usage, as j is usually for iterations.
> 
> In my opinion I think vq->last_avail_idx is better.
> 
> 

Totally agree. The reason I used a separate variable in 
virtqueue_split_pop was to capture the value of vq->last_avail_idx 
before it got incremented in the next line.

Not sure if it actually matters whether or not I use the value of 
last_avail_idx before or after it's incremented. I don't think it does 
but, in any case, I opted to use the value before it was incremented so 
as to be consistent with virtqueue_packed_pop, where last_avail_idx is 
used before it's incremented.

I'll change j to something more meaningful though. Maybe 
'init_last_avail_idx'? Hmm... will need to think on it.

>>       vq->last_avail_idx += elem->ndescs;
>>       vq->inuse += elem->ndescs;
>>
>> --
>> 2.39.3
>>
> 

Re: [PATCH 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support

[Jonah Palmer]

On 5/9/24 10:08 AM, Eugenio Perez Martin wrote:
> On Mon, May 6, 2024 at 5:05 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> Add VIRTIO_F_IN_ORDER feature support for virtqueue_fill operations.
>>
>> The goal of the virtqueue_fill operation when the VIRTIO_F_IN_ORDER
>> feature has been negotiated is to search for this now-used element,
>> set its length, and mark the element as filled in the VirtQueue's
>> used_elems array.
>>
>> By marking the element as filled, it will indicate that this element is
>> ready to be flushed, so long as the element is in-order.
>>
>> Tested-by: Lei Yang <leiyang@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   hw/virtio/virtio.c | 26 +++++++++++++++++++++++++-
>>   1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index e6eb1bb453..064046b5e2 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -873,6 +873,28 @@ static void virtqueue_packed_fill(VirtQueue *vq, const VirtQueueElement *elem,
>>       vq->used_elems[idx].ndescs = elem->ndescs;
>>   }
>>
>> +static void virtqueue_ordered_fill(VirtQueue *vq, const VirtQueueElement *elem,
>> +                                   unsigned int len)
>> +{
>> +    unsigned int i = vq->used_idx;
>> +
>> +    /* Search for element in vq->used_elems */
>> +    while (i != vq->last_avail_idx) {
>> +        /* Found element, set length and mark as filled */
>> +        if (vq->used_elems[i].index == elem->index) {
>> +            vq->used_elems[i].len = len;
>> +            vq->used_elems[i].filled = true;
>> +            break;
>> +        }
>> +
>> +        i += vq->used_elems[i].ndescs;
>> +
>> +        if (i >= vq->vring.num) {
>> +            i -= vq->vring.num;
>> +        }
>> +    }
> 
> This has a subtle problem: ndescs and elems->id are controlled by the
> guest, so it could make QEMU to loop forever looking for the right
> descriptor. For each iteration, the code must control that the
> variable "i" will be different for the next iteration, and that there
> will be no more than vq->last_avail_idx - vq->used_idx iterations.
> 

Very true and something I was worried about, e.g. what if, for some 
strange reason, we could never get i == vq->last_avail_idx.

Perhaps as a surefire way to make sure we terminate appropriately, as 
you mentioned, 'i' should not increase by more than the distance between 
used_idx and last_avail_idx. If it does, we exit the while loop:

unsigned int steps = 0;
unsigned int max_steps = (vq->last_avail_idx + vq->vring.num -
                           vq->used_idx) % vq->vring.num;
while (steps <= max_steps) {
     ...
     steps += vq->used_elems[i].ndescs;
     ...
}

Though if we do find that steps <= max_steps, should we treat this as an 
error or give some kind of warning? Since I believe that, under normal 
behavior, we shouldn't find ourselves in a situation where we weren't 
able to find the matching VirtQueueElement in the used_elems array. And 
not setting 'vq->used_elems[i].filled = true' may cause issues later.

> Apart of that, I think it makes more sense to split the logical
> sections of the function this way:
> /* declarations */
> i = vq->used_idx
> 
> /* Search for element in vq->used_elems */
> while (vq->used_elems[i].index != elem->index &&
> vq->used_elems[i].index i != vq->last_avail_idx && ...) {
> ...
> }
> 
> /* Set length and mark as filled */
> vq->used_elems[i].len = len;
> vq->used_elems[i].filled = true;
> ---
> 
> But I'm ok either way.
> 

Let me know what you think of the proposed solution above. It doesn't 
explicitly separate the search and find operation like you're proposing 
here but it does clearly show the bounds of our search.

But doing:

while (vq->used_elems[i].index != elem->index &&
        vq->used_elems[i].index != vq->last_avail_idx &&
        steps <= max_steps) {
     ...
}

Works too.

>> +}
>> +
>>   static void virtqueue_packed_fill_desc(VirtQueue *vq,
>>                                          const VirtQueueElement *elem,
>>                                          unsigned int idx,
>> @@ -923,7 +945,9 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>>           return;
>>       }
>>
>> -    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
>> +        virtqueue_ordered_fill(vq, elem, len);
>> +    } else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>>           virtqueue_packed_fill(vq, elem, len, idx);
>>       } else {
>>           virtqueue_split_fill(vq, elem, len, idx);
>> --
>> 2.39.3
>>
> 

Re: [PATCH 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support

[Jonah Palmer]

On 5/10/24 3:48 AM, Eugenio Perez Martin wrote:
> On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> Add VIRTIO_F_IN_ORDER feature support for virtqueue_flush operations.
>>
>> The goal of the virtqueue_flush operation when the VIRTIO_F_IN_ORDER
>> feature has been negotiated is to write elements to the used/descriptor
>> ring in-order and then update used_idx.
>>
>> The function iterates through the VirtQueueElement used_elems array
>> in-order starting at vq->used_idx. If the element is valid (filled), the
>> element is written to the used/descriptor ring. This process continues
>> until we find an invalid (not filled) element.
>>
>> If any elements were written, the used_idx is updated.
>>
>> Tested-by: Lei Yang <leiyang@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   hw/virtio/virtio.c | 75 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 064046b5e2..0efed2c88e 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1006,6 +1006,77 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>>       }
>>   }
>>
>> +static void virtqueue_ordered_flush(VirtQueue *vq)
>> +{
>> +    unsigned int i = vq->used_idx;
>> +    unsigned int ndescs = 0;
>> +    uint16_t old = vq->used_idx;
>> +    bool packed;
>> +    VRingUsedElem uelem;
>> +
>> +    packed = virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED);
>> +
>> +    if (packed) {
>> +        if (unlikely(!vq->vring.desc)) {
>> +            return;
>> +        }
>> +    } else if (unlikely(!vq->vring.used)) {
>> +        return;
>> +    }
>> +
>> +    /* First expected in-order element isn't ready, nothing to do */
>> +    if (!vq->used_elems[i].filled) {
>> +        return;
>> +    }
>> +
>> +    /* Write first expected in-order element to used ring (split VQs) */
>> +    if (!packed) {
>> +        uelem.id = vq->used_elems[i].index;
>> +        uelem.len = vq->used_elems[i].len;
>> +        vring_used_write(vq, &uelem, i);
>> +    }
>> +
>> +    ndescs += vq->used_elems[i].ndescs;
>> +    i += ndescs;
>> +    if (i >= vq->vring.num) {
>> +        i -= vq->vring.num;
>> +    }
>> +
>> +    /* Search for more filled elements in-order */
>> +    while (vq->used_elems[i].filled) {
>> +        if (packed) {
>> +            virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs, false);
>> +        } else {
>> +            uelem.id = vq->used_elems[i].index;
>> +            uelem.len = vq->used_elems[i].len;
>> +            vring_used_write(vq, &uelem, i);
>> +        }
>> +
>> +        vq->used_elems[i].filled = false;
>> +        ndescs += vq->used_elems[i].ndescs;
>> +        i += ndescs;
>> +        if (i >= vq->vring.num) {
>> +            i -= vq->vring.num;
>> +        }
>> +    }
>> +
> 
> I may be missing something, but you have split out the first case as a
> special one, totally out of the while loop. Can't it be contained in
> the loop checking !(packed && i == vq->used_idx)? That would avoid
> code duplication.
> 
> A comment can be added in the line of "first entry of packed is
> written the last so the guest does not see invalid descriptors".
> 

Yea this was intentional for the reason you've given above. It was 
either the solution above or, as you suggest, handling this in the while 
loop:

if (!vq->used_elems[i].filled) {
     return;
}

while (vq->used_elems[i].filled) {
     if (packed && i != vq->used_idx) {
         virtqueue_packed_fill_desc(...);
     } else {
         ...
     }
     ...
}

I did consider this option at the time of writing this patch but I 
must've overcomplicated it in my head somehow and thought the current 
solution was the simpler one. However, after looking it over again, your 
suggestion is indeed the cleaner one.

Will adjust this in v2! Thanks for your time reviewing these!

>> +    if (packed) {
>> +        virtqueue_packed_fill_desc(vq, &vq->used_elems[vq->used_idx], 0, true);
>> +        vq->used_idx += ndescs;
>> +        if (vq->used_idx >= vq->vring.num) {
>> +            vq->used_idx -= vq->vring.num;
>> +            vq->used_wrap_counter ^= 1;
>> +            vq->signalled_used_valid = false;
>> +        }
>> +    } else {
>> +        vring_used_idx_set(vq, i);
>> +        if (unlikely((int16_t)(i - vq->signalled_used) < (uint16_t)(i - old))) {
>> +            vq->signalled_used_valid = false;
>> +        }
>> +    }
>> +    vq->inuse -= ndescs;
>> +}
>> +
>>   void virtqueue_flush(VirtQueue *vq, unsigned int count)
>>   {
>>       if (virtio_device_disabled(vq->vdev)) {
>> @@ -1013,7 +1084,9 @@ void virtqueue_flush(VirtQueue *vq, unsigned int count)
>>           return;
>>       }
>>
>> -    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>> +    if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_IN_ORDER)) {
>> +        virtqueue_ordered_flush(vq);
>> +    } else if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
>>           virtqueue_packed_flush(vq, count);
>>       } else {
>>           virtqueue_split_flush(vq, count);
>> --
>> 2.39.3
>>
>