* [PATCH v4 1/6] virtio: Add bool to VirtQueueElement
2024-07-10 12:55 [PATCH v4 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
@ 2024-07-10 12:55 ` Jonah Palmer
2024-07-10 12:55 ` [PATCH v4 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support Jonah Palmer
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Jonah Palmer @ 2024-07-10 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam, eperezma,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky, jonah.palmer
Add the boolean 'in_order_filled' member to the VirtQueueElement structure.
The use of this boolean will signify whether the element has been processed
and is ready to be flushed (so long as the element is in-order). This
boolean is used to support the VIRTIO_F_IN_ORDER feature.
Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
include/hw/virtio/virtio.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 7512afbc84..fdc827f82e 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -69,6 +69,8 @@ typedef struct VirtQueueElement
unsigned int ndescs;
unsigned int out_num;
unsigned int in_num;
+ /* Element has been processed (VIRTIO_F_IN_ORDER) */
+ bool in_order_filled;
hwaddr *in_addr;
hwaddr *out_addr;
struct iovec *in_sg;
--
2.43.5
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v4 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support
2024-07-10 12:55 [PATCH v4 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
2024-07-10 12:55 ` [PATCH v4 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
@ 2024-07-10 12:55 ` Jonah Palmer
2024-07-10 12:55 ` [PATCH v4 3/6] virtio: virtqueue_ordered_fill " Jonah Palmer
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Jonah Palmer @ 2024-07-10 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam, eperezma,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky, 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.
Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
last_avail_idx can grow well beyond vring.num, so for this reason we
need to keep the index bound between 0 and vring.num-1 for the split VQ
case.
hw/virtio/virtio.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 583a224163..98eb601b09 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1505,7 +1505,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, head, max, idx;
VRingMemoryRegionCaches *caches;
MemoryRegionCache indirect_desc_cache;
MemoryRegionCache *desc_cache;
@@ -1629,6 +1629,13 @@ 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)) {
+ idx = (vq->last_avail_idx - 1) % vq->vring.num;
+ vq->used_elems[idx].index = elem->index;
+ vq->used_elems[idx].len = elem->len;
+ vq->used_elems[idx].ndescs = elem->ndescs;
+ }
+
vq->inuse++;
trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
@@ -1762,6 +1769,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.43.5
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v4 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
2024-07-10 12:55 [PATCH v4 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
2024-07-10 12:55 ` [PATCH v4 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
2024-07-10 12:55 ` [PATCH v4 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support Jonah Palmer
@ 2024-07-10 12:55 ` Jonah Palmer
2024-07-10 12:55 ` [PATCH v4 4/6] virtio: virtqueue_ordered_flush " Jonah Palmer
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Jonah Palmer @ 2024-07-10 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam, eperezma,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky, jonah.palmer
Add VIRTIO_F_IN_ORDER feature support for the virtqueue_fill operation.
The goal of the virtqueue_ordered_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 has
been processed and is ready to be flushed, so long as the element is
in-order.
Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
hw/virtio/virtio.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 98eb601b09..0000a7b41c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -872,6 +872,46 @@ 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, steps, max_steps;
+
+ i = vq->used_idx % vq->vring.num;
+ steps = 0;
+ /*
+ * We shouldn't need to increase 'i' by more than the distance
+ * between used_idx and last_avail_idx.
+ */
+ max_steps = (vq->last_avail_idx - vq->used_idx) % vq->vring.num;
+
+ /* Search for element in vq->used_elems */
+ while (steps <= max_steps) {
+ /* 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].in_order_filled = true;
+ break;
+ }
+
+ i += vq->used_elems[i].ndescs;
+ steps += vq->used_elems[i].ndescs;
+
+ if (i >= vq->vring.num) {
+ i -= vq->vring.num;
+ }
+ }
+
+ /*
+ * We should be able to find a matching VirtQueueElement in
+ * used_elems. If we don't, this is an error.
+ */
+ if (steps >= max_steps) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: %s cannot fill buffer id %u\n",
+ __func__, vq->vdev->name, elem->index);
+ }
+}
+
static void virtqueue_packed_fill_desc(VirtQueue *vq,
const VirtQueueElement *elem,
unsigned int idx,
@@ -922,7 +962,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.43.5
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v4 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support
2024-07-10 12:55 [PATCH v4 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
` (2 preceding siblings ...)
2024-07-10 12:55 ` [PATCH v4 3/6] virtio: virtqueue_ordered_fill " Jonah Palmer
@ 2024-07-10 12:55 ` Jonah Palmer
2024-07-10 16:16 ` Eugenio Perez Martin
2024-07-10 12:55 ` [PATCH v4 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits Jonah Palmer via
2024-07-10 12:55 ` [PATCH v4 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition Jonah Palmer
5 siblings, 1 reply; 11+ messages in thread
From: Jonah Palmer @ 2024-07-10 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam, eperezma,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky, jonah.palmer
Add VIRTIO_F_IN_ORDER feature support for the virtqueue_flush operation.
The goal of the virtqueue_ordered_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.
For packed VQs, the first entry (at vq->used_idx) is written to the
descriptor ring last so the guest doesn't see any invalid descriptors.
If any elements were written, the used_idx is updated.
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
Several fixes here for the split VQ case:
- Ensure all previous write operations to buffers are completed before
updating the used_idx (via smp_wmb()).
- used_elems index 'i' should be incremented by the number of descriptors
in the current element we just processed, not by the running total of
descriptors already seen. This would've caused batched operations to
miss ordered elements when looping through the used_elems array.
- Do not keep the VQ's used_idx bound between 0 and vring.num-1 when
setting it via vring_used_idx_set().
While the packed VQ case naturally keeps used_idx bound between 0 and
vring.num-1, the split VQ case cannot. This is because used_idx is
used to compare the current event index with the new and old used
indices to decide if a notification is necessary (see
virtio_split_should_notify()). This comparison expects used_idx to be
between 0 and 65535, not 0 and vring.num-1.
hw/virtio/virtio.c | 70 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 69 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0000a7b41c..b419d8d6e7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1023,6 +1023,72 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
}
}
+static void virtqueue_ordered_flush(VirtQueue *vq)
+{
+ unsigned int i = vq->used_idx % vq->vring.num;
+ unsigned int ndescs = 0;
+ uint16_t old = vq->used_idx;
+ uint16_t new;
+ 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].in_order_filled) {
+ return;
+ }
+
+ /* Search for filled elements in-order */
+ while (vq->used_elems[i].in_order_filled) {
+ /*
+ * First entry for packed VQs is written last so the guest
+ * doesn't see invalid descriptors.
+ */
+ if (packed && i != vq->used_idx) {
+ virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs, false);
+ } else if (!packed) {
+ uelem.id = vq->used_elems[i].index;
+ uelem.len = vq->used_elems[i].len;
+ vring_used_write(vq, &uelem, i);
+ }
+
+ vq->used_elems[i].in_order_filled = false;
+ ndescs += vq->used_elems[i].ndescs;
+ i += vq->used_elems[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 {
+ /* Make sure buffer is written before we update index. */
+ smp_wmb();
+ new = old + ndescs;
+ vring_used_idx_set(vq, new);
+ if (unlikely((int16_t)(new - vq->signalled_used) < (uint16_t)(new - old))) {
+ vq->signalled_used_valid = false;
+ }
+ }
+ vq->inuse -= ndescs;
+}
+
void virtqueue_flush(VirtQueue *vq, unsigned int count)
{
if (virtio_device_disabled(vq->vdev)) {
@@ -1030,7 +1096,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.43.5
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v4 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support
2024-07-10 12:55 ` [PATCH v4 4/6] virtio: virtqueue_ordered_flush " Jonah Palmer
@ 2024-07-10 16:16 ` Eugenio Perez Martin
0 siblings, 0 replies; 11+ messages in thread
From: Eugenio Perez Martin @ 2024-07-10 16:16 UTC (permalink / raw)
To: Jonah Palmer
Cc: qemu-devel, mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky
On Wed, Jul 10, 2024 at 2:56 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add VIRTIO_F_IN_ORDER feature support for the virtqueue_flush operation.
>
> The goal of the virtqueue_ordered_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.
>
> For packed VQs, the first entry (at vq->used_idx) is written to the
> descriptor ring last so the guest doesn't see any invalid descriptors.
>
> If any elements were written, the used_idx is updated.
>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> Several fixes here for the split VQ case:
> - Ensure all previous write operations to buffers are completed before
> updating the used_idx (via smp_wmb()).
>
> - used_elems index 'i' should be incremented by the number of descriptors
> in the current element we just processed, not by the running total of
> descriptors already seen. This would've caused batched operations to
> miss ordered elements when looping through the used_elems array.
>
> - Do not keep the VQ's used_idx bound between 0 and vring.num-1 when
> setting it via vring_used_idx_set().
>
> While the packed VQ case naturally keeps used_idx bound between 0 and
> vring.num-1, the split VQ case cannot. This is because used_idx is
> used to compare the current event index with the new and old used
> indices to decide if a notification is necessary (see
> virtio_split_should_notify()). This comparison expects used_idx to be
> between 0 and 65535, not 0 and vring.num-1.
>
> hw/virtio/virtio.c | 70 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0000a7b41c..b419d8d6e7 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1023,6 +1023,72 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
> }
> }
>
> +static void virtqueue_ordered_flush(VirtQueue *vq)
> +{
> + unsigned int i = vq->used_idx % vq->vring.num;
> + unsigned int ndescs = 0;
> + uint16_t old = vq->used_idx;
> + uint16_t new;
> + 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].in_order_filled) {
> + return;
> + }
> +
> + /* Search for filled elements in-order */
> + while (vq->used_elems[i].in_order_filled) {
> + /*
> + * First entry for packed VQs is written last so the guest
> + * doesn't see invalid descriptors.
> + */
> + if (packed && i != vq->used_idx) {
> + virtqueue_packed_fill_desc(vq, &vq->used_elems[i], ndescs, false);
> + } else if (!packed) {
> + uelem.id = vq->used_elems[i].index;
> + uelem.len = vq->used_elems[i].len;
> + vring_used_write(vq, &uelem, i);
> + }
> +
> + vq->used_elems[i].in_order_filled = false;
> + ndescs += vq->used_elems[i].ndescs;
> + i += vq->used_elems[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 {
> + /* Make sure buffer is written before we update index. */
> + smp_wmb();
> + new = old + ndescs;
> + vring_used_idx_set(vq, new);
> + if (unlikely((int16_t)(new - vq->signalled_used) < (uint16_t)(new - old))) {
> + vq->signalled_used_valid = false;
> + }
> + }
> + vq->inuse -= ndescs;
> +}
> +
> void virtqueue_flush(VirtQueue *vq, unsigned int count)
> {
> if (virtio_device_disabled(vq->vdev)) {
> @@ -1030,7 +1096,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.43.5
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits
2024-07-10 12:55 [PATCH v4 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
` (3 preceding siblings ...)
2024-07-10 12:55 ` [PATCH v4 4/6] virtio: virtqueue_ordered_flush " Jonah Palmer
@ 2024-07-10 12:55 ` Jonah Palmer via
2024-07-10 12:55 ` [PATCH v4 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition Jonah Palmer
5 siblings, 0 replies; 11+ messages in thread
From: Jonah Palmer via @ 2024-07-10 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam, eperezma,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky, jonah.palmer
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.
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 fdbc30b9ce..5b7f46bbb0 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,
VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 18898afe81..a788e6937e 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_F_NOTIFICATION_DATA,
VIRTIO_NET_F_HASH_REPORT,
VHOST_INVALID_FEATURE_BIT
@@ -78,6 +79,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 3d5fe0994d..49cff2a0cb 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,
VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index cc91ade525..55e4be5b34 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,
VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index ae48cc1c96..32ee7f496d 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,
VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 802b44a07d..da3b0e0229 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,
VIRTIO_F_NOTIFICATION_DATA,
VHOST_INVALID_FEATURE_BIT
};
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index daa38428c5..03457ead66 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_F_NOTIFICATION_DATA,
VIRTIO_NET_F_CSUM,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
--
2.43.5
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v4 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition
2024-07-10 12:55 [PATCH v4 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
` (4 preceding siblings ...)
2024-07-10 12:55 ` [PATCH v4 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits Jonah Palmer via
@ 2024-07-10 12:55 ` Jonah Palmer
2024-07-20 19:16 ` Michael S. Tsirkin
5 siblings, 1 reply; 11+ messages in thread
From: Jonah Palmer @ 2024-07-10 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam, eperezma,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky, 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.
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 fdc827f82e..d2a1938757 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -373,7 +373,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.43.5
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v4 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition
2024-07-10 12:55 ` [PATCH v4 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition Jonah Palmer
@ 2024-07-20 19:16 ` Michael S. Tsirkin
2024-07-22 11:11 ` Eugenio Perez Martin
0 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2024-07-20 19:16 UTC (permalink / raw)
To: Jonah Palmer
Cc: qemu-devel, raphael, kwolf, hreitz, jasowang, pbonzini, fam,
eperezma, stefanha, qemu-block, schalla, leiyang, virtio-fs,
si-wei.liu, boris.ostrovsky
On Wed, Jul 10, 2024 at 08:55:19AM -0400, Jonah Palmer wrote:
> 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.
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
Given release is close, it's likely wise.
However, I think we should flip the default in the future
release.
> ---
> 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 fdc827f82e..d2a1938757 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -373,7 +373,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.43.5
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition
2024-07-20 19:16 ` Michael S. Tsirkin
@ 2024-07-22 11:11 ` Eugenio Perez Martin
2024-07-22 11:31 ` Eugenio Perez Martin
0 siblings, 1 reply; 11+ messages in thread
From: Eugenio Perez Martin @ 2024-07-22 11:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jonah Palmer, qemu-devel, raphael, kwolf, hreitz, jasowang,
pbonzini, fam, stefanha, qemu-block, schalla, leiyang, virtio-fs,
si-wei.liu, boris.ostrovsky
On Sat, Jul 20, 2024 at 9:16 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 10, 2024 at 08:55:19AM -0400, Jonah Palmer wrote:
> > 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.
> >
> > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>
>
> Given release is close, it's likely wise.
> However, I think we should flip the default in the future
> release.
>
Should we post a new version with v9.2 tag enabling it?
> > ---
> > 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 fdc827f82e..d2a1938757 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -373,7 +373,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.43.5
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition
2024-07-22 11:11 ` Eugenio Perez Martin
@ 2024-07-22 11:31 ` Eugenio Perez Martin
0 siblings, 0 replies; 11+ messages in thread
From: Eugenio Perez Martin @ 2024-07-22 11:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jonah Palmer, qemu-devel, raphael, kwolf, hreitz, jasowang,
pbonzini, fam, stefanha, qemu-block, schalla, leiyang, virtio-fs,
si-wei.liu, boris.ostrovsky
On Mon, Jul 22, 2024 at 1:11 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Sat, Jul 20, 2024 at 9:16 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 10, 2024 at 08:55:19AM -0400, Jonah Palmer wrote:
> > > 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.
> > >
> > > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> >
> >
> > Given release is close, it's likely wise.
> > However, I think we should flip the default in the future
> > release.
> >
>
> Should we post a new version with v9.2 tag enabling it?
>
Sorry, actually I think this needs some more thought. Maybe in_order
hurts the performance of devices that are usually out of order, like
blk. Should we enable only for virtio-net and let each device code
decide?
> > > ---
> > > 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 fdc827f82e..d2a1938757 100644
> > > --- a/include/hw/virtio/virtio.h
> > > +++ b/include/hw/virtio/virtio.h
> > > @@ -373,7 +373,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.43.5
> >
^ permalink raw reply [flat|nested] 11+ messages in thread