* [PATCH v2 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support
@ 2024-05-20 13:00 Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Jonah Palmer @ 2024-05-20 13:00 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
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 frequently elements are completed out-of-order.
A VirtQueue whose device 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 'in_order_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).
---
v2: Make 'in_order_filled' more descriptive.
Change 'j' to more descriptive var name in virtqueue_split_pop.
Use more definitive search conditional in virtqueue_ordered_fill.
Avoid code duplication in 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 | 119 ++++++++++++++++++++++++++++++++++-
include/hw/virtio/virtio.h | 6 +-
net/vhost-vdpa.c | 1 +
9 files changed, 129 insertions(+), 4 deletions(-)
--
2.39.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/6] virtio: Add bool to VirtQueueElement
2024-05-20 13:00 [PATCH v2 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
@ 2024-05-20 13:00 ` Jonah Palmer
2024-05-22 15:44 ` Eugenio Perez Martin
2024-05-20 13:00 ` [PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support Jonah Palmer
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Jonah Palmer @ 2024-05-20 13:00 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.
Tested-by: Lei Yang <leiyang@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 7d5ffdc145..88e70c1ae1 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.39.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support
2024-05-20 13:00 [PATCH v2 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
@ 2024-05-20 13:00 ` Jonah Palmer
2024-05-22 15:45 ` Eugenio Perez Martin
2024-05-20 13:00 ` [PATCH v2 3/6] virtio: virtqueue_ordered_fill " Jonah Palmer
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Jonah Palmer @ 2024-05-20 13:00 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.
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..7456d61bc8 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, head, max, prev_avail_idx;
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;
}
+ prev_avail_idx = 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[prev_avail_idx].index = elem->index;
+ vq->used_elems[prev_avail_idx].len = elem->len;
+ vq->used_elems[prev_avail_idx].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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
2024-05-20 13:00 [PATCH v2 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support Jonah Palmer
@ 2024-05-20 13:00 ` Jonah Palmer
2024-05-22 16:07 ` Eugenio Perez Martin
2024-05-20 13:00 ` [PATCH v2 4/6] virtio: virtqueue_ordered_flush " Jonah Palmer
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Jonah Palmer @ 2024-05-20 13:00 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.
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
hw/virtio/virtio.c | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7456d61bc8..01b6b32460 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -873,6 +873,38 @@ 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;
+ 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->vring.num - 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;
+ }
+ }
+}
+
static void virtqueue_packed_fill_desc(VirtQueue *vq,
const VirtQueueElement *elem,
unsigned int idx,
@@ -923,7 +955,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support
2024-05-20 13:00 [PATCH v2 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
` (2 preceding siblings ...)
2024-05-20 13:00 ` [PATCH v2 3/6] virtio: virtqueue_ordered_fill " Jonah Palmer
@ 2024-05-20 13:00 ` Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits Jonah Palmer via
2024-05-20 13:00 ` [PATCH v2 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition Jonah Palmer
5 siblings, 0 replies; 15+ messages in thread
From: Jonah Palmer @ 2024-05-20 13:00 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>
---
hw/virtio/virtio.c | 66 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 01b6b32460..39b91beece 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1016,6 +1016,68 @@ 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].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 += 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)) {
@@ -1023,7 +1085,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits
2024-05-20 13:00 [PATCH v2 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
` (3 preceding siblings ...)
2024-05-20 13:00 ` [PATCH v2 4/6] virtio: virtqueue_ordered_flush " Jonah Palmer
@ 2024-05-20 13:00 ` Jonah Palmer via
2024-05-20 13:00 ` [PATCH v2 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition Jonah Palmer
5 siblings, 0 replies; 15+ messages in thread
From: Jonah Palmer via @ 2024-05-20 13:00 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.
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition
2024-05-20 13:00 [PATCH v2 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
` (4 preceding siblings ...)
2024-05-20 13:00 ` [PATCH v2 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits Jonah Palmer via
@ 2024-05-20 13:00 ` Jonah Palmer
5 siblings, 0 replies; 15+ messages in thread
From: Jonah Palmer @ 2024-05-20 13:00 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.
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 88e70c1ae1..d33345ecc5 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -371,7 +371,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/6] virtio: Add bool to VirtQueueElement
2024-05-20 13:00 ` [PATCH v2 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
@ 2024-05-22 15:44 ` Eugenio Perez Martin
2024-05-23 9:55 ` Jonah Palmer
0 siblings, 1 reply; 15+ messages in thread
From: Eugenio Perez Martin @ 2024-05-22 15:44 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 Mon, May 20, 2024 at 3:01 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> 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.
>
> Tested-by: Lei Yang <leiyang@redhat.com>
The code has changed from the version that Lei tested, so we should
drop this tag until he re-test again.
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 7d5ffdc145..88e70c1ae1 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.39.3
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support
2024-05-20 13:00 ` [PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support Jonah Palmer
@ 2024-05-22 15:45 ` Eugenio Perez Martin
2024-05-23 9:56 ` Jonah Palmer
0 siblings, 1 reply; 15+ messages in thread
From: Eugenio Perez Martin @ 2024-05-22 15:45 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 Mon, May 20, 2024 at 3:01 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..7456d61bc8 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, head, max, prev_avail_idx;
> 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;
> }
>
> + prev_avail_idx = 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)) {
I think vq->last_avail_idx - 1 could be more clear here.
Either way,
Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> + vq->used_elems[prev_avail_idx].index = elem->index;
> + vq->used_elems[prev_avail_idx].len = elem->len;
> + vq->used_elems[prev_avail_idx].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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
2024-05-20 13:00 ` [PATCH v2 3/6] virtio: virtqueue_ordered_fill " Jonah Palmer
@ 2024-05-22 16:07 ` Eugenio Perez Martin
2024-05-23 10:29 ` Jonah Palmer
0 siblings, 1 reply; 15+ messages in thread
From: Eugenio Perez Martin @ 2024-05-22 16:07 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 Mon, May 20, 2024 at 3:01 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> 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.
>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
> hw/virtio/virtio.c | 36 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7456d61bc8..01b6b32460 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -873,6 +873,38 @@ 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;
> + 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->vring.num - vq->used_idx)
> + % vq->vring.num;
I may be missing something, but (+vq->vring.num) is redundant if we (%
vq->vring.num), isn't it?
> +
> + /* 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;
> + }
> + }
> +}
> +
Let's report an error if we finish the loop. I think:
qemu_log_mask(LOG_GUEST_ERROR,
"%s: %s cannot fill buffer id %u\n",
__func__, vdev->name, elem->index);
(or similar) should do.
apart form that,
Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> static void virtqueue_packed_fill_desc(VirtQueue *vq,
> const VirtQueueElement *elem,
> unsigned int idx,
> @@ -923,7 +955,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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/6] virtio: Add bool to VirtQueueElement
2024-05-22 15:44 ` Eugenio Perez Martin
@ 2024-05-23 9:55 ` Jonah Palmer
0 siblings, 0 replies; 15+ messages in thread
From: Jonah Palmer @ 2024-05-23 9:55 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: qemu-devel, mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky
On 5/22/24 11:44 AM, Eugenio Perez Martin wrote:
> On Mon, May 20, 2024 at 3:01 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> 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.
>>
>> Tested-by: Lei Yang <leiyang@redhat.com>
>
> The code has changed from the version that Lei tested, so we should
> drop this tag until he re-test again.
>
> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>
My apologies. I wasn't sure if I should've removed the tag for all
changes or just the significant changes.
>> 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 7d5ffdc145..88e70c1ae1 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.39.3
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support
2024-05-22 15:45 ` Eugenio Perez Martin
@ 2024-05-23 9:56 ` Jonah Palmer
0 siblings, 0 replies; 15+ messages in thread
From: Jonah Palmer @ 2024-05-23 9:56 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: qemu-devel, mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky
On 5/22/24 11:45 AM, Eugenio Perez Martin wrote:
> On Mon, May 20, 2024 at 3:01 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..7456d61bc8 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, head, max, prev_avail_idx;
>> 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;
>> }
>>
>> + prev_avail_idx = 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)) {
>
> I think vq->last_avail_idx - 1 could be more clear here.
>
> Either way,
>
> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>
Sure thing! Will make this change in v3.
>> + vq->used_elems[prev_avail_idx].index = elem->index;
>> + vq->used_elems[prev_avail_idx].len = elem->len;
>> + vq->used_elems[prev_avail_idx].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
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
2024-05-22 16:07 ` Eugenio Perez Martin
@ 2024-05-23 10:29 ` Jonah Palmer
2024-05-23 10:47 ` Eugenio Perez Martin
0 siblings, 1 reply; 15+ messages in thread
From: Jonah Palmer @ 2024-05-23 10:29 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: qemu-devel, mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky
On 5/22/24 12:07 PM, Eugenio Perez Martin wrote:
> On Mon, May 20, 2024 at 3:01 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> 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.
>>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>> hw/virtio/virtio.c | 36 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 7456d61bc8..01b6b32460 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -873,6 +873,38 @@ 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;
>> + 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->vring.num - vq->used_idx)
>> + % vq->vring.num;
>
> I may be missing something, but (+vq->vring.num) is redundant if we (%
> vq->vring.num), isn't it?
>
It ensures the result is always non-negative (e.g. when
vq->last_avail_idx < vq->used_idx).
I wasn't sure how different platforms or compilers would handle
something like -5 % 10, so to be safe I included the '+ vq->vring.num'.
For example, on my system, in test.c;
#include <stdio.h>
int main() {
unsigned int result = -5 % 10;
printf("Result of -5 %% 10 is: %d\n", result);
return 0;
}
# gcc -o test test.c
# ./test
Result of -5 % 10 is: -5
>> +
>> + /* 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;
>> + }
>> + }
>> +}
>> +
>
> Let's report an error if we finish the loop. I think:
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: %s cannot fill buffer id %u\n",
> __func__, vdev->name, elem->index);
>
> (or similar) should do.
>
> apart form that,
>
> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>
Gotcha. Will add this in v3.
Thank you Eugenio!
>> static void virtqueue_packed_fill_desc(VirtQueue *vq,
>> const VirtQueueElement *elem,
>> unsigned int idx,
>> @@ -923,7 +955,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
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
2024-05-23 10:29 ` Jonah Palmer
@ 2024-05-23 10:47 ` Eugenio Perez Martin
2024-05-23 11:10 ` Jonah Palmer
0 siblings, 1 reply; 15+ messages in thread
From: Eugenio Perez Martin @ 2024-05-23 10:47 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 Thu, May 23, 2024 at 12:30 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
>
>
> On 5/22/24 12:07 PM, Eugenio Perez Martin wrote:
> > On Mon, May 20, 2024 at 3:01 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> >>
> >> 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.
> >>
> >> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> >> ---
> >> hw/virtio/virtio.c | 36 +++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 35 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 7456d61bc8..01b6b32460 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -873,6 +873,38 @@ 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;
> >> + 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->vring.num - vq->used_idx)
> >> + % vq->vring.num;
> >
> > I may be missing something, but (+vq->vring.num) is redundant if we (%
> > vq->vring.num), isn't it?
> >
>
> It ensures the result is always non-negative (e.g. when
> vq->last_avail_idx < vq->used_idx).
>
> I wasn't sure how different platforms or compilers would handle
> something like -5 % 10, so to be safe I included the '+ vq->vring.num'.
>
> For example, on my system, in test.c;
>
> #include <stdio.h>
>
> int main() {
> unsigned int result = -5 % 10;
> printf("Result of -5 %% 10 is: %d\n", result);
> return 0;
> }
>
> # gcc -o test test.c
>
> # ./test
> Result of -5 % 10 is: -5
>
I think the modulo is being done in signed ints in your test, and then
converting a signed int to an unsigned int. Like result = (-5 % 10).
The unsigned wrap is always defined in C, and vq->last_avail_idx and
vq->used_idx are both unsigned. Here is a closer test:
int main(void) {
unsigned int a = -5, b = 2;
unsigned int result = (b-a) % 10;
printf("Result of -5 %% 10 is: %u\n", result);
return 0;
}
But it is a good catch for signed ints for sure :).
Thanks!
> >> +
> >> + /* 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;
> >> + }
> >> + }
> >> +}
> >> +
> >
> > Let's report an error if we finish the loop. I think:
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "%s: %s cannot fill buffer id %u\n",
> > __func__, vdev->name, elem->index);
> >
> > (or similar) should do.
> >
> > apart form that,
> >
> > Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> >
>
> Gotcha. Will add this in v3.
>
> Thank you Eugenio!
>
> >> static void virtqueue_packed_fill_desc(VirtQueue *vq,
> >> const VirtQueueElement *elem,
> >> unsigned int idx,
> >> @@ -923,7 +955,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
> >>
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
2024-05-23 10:47 ` Eugenio Perez Martin
@ 2024-05-23 11:10 ` Jonah Palmer
0 siblings, 0 replies; 15+ messages in thread
From: Jonah Palmer @ 2024-05-23 11:10 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: qemu-devel, mst, raphael, kwolf, hreitz, jasowang, pbonzini, fam,
stefanha, qemu-block, schalla, leiyang, virtio-fs, si-wei.liu,
boris.ostrovsky
On 5/23/24 6:47 AM, Eugenio Perez Martin wrote:
> On Thu, May 23, 2024 at 12:30 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>>
>>
>> On 5/22/24 12:07 PM, Eugenio Perez Martin wrote:
>>> On Mon, May 20, 2024 at 3:01 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>>> ---
>>>> hw/virtio/virtio.c | 36 +++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 35 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>> index 7456d61bc8..01b6b32460 100644
>>>> --- a/hw/virtio/virtio.c
>>>> +++ b/hw/virtio/virtio.c
>>>> @@ -873,6 +873,38 @@ 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;
>>>> + 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->vring.num - vq->used_idx)
>>>> + % vq->vring.num;
>>>
>>> I may be missing something, but (+vq->vring.num) is redundant if we (%
>>> vq->vring.num), isn't it?
>>>
>>
>> It ensures the result is always non-negative (e.g. when
>> vq->last_avail_idx < vq->used_idx).
>>
>> I wasn't sure how different platforms or compilers would handle
>> something like -5 % 10, so to be safe I included the '+ vq->vring.num'.
>>
>> For example, on my system, in test.c;
>>
>> #include <stdio.h>
>>
>> int main() {
>> unsigned int result = -5 % 10;
>> printf("Result of -5 %% 10 is: %d\n", result);
>> return 0;
>> }
>>
>> # gcc -o test test.c
>>
>> # ./test
>> Result of -5 % 10 is: -5
>>
>
> I think the modulo is being done in signed ints in your test, and then
> converting a signed int to an unsigned int. Like result = (-5 % 10).
>
> The unsigned wrap is always defined in C, and vq->last_avail_idx and
> vq->used_idx are both unsigned. Here is a closer test:
> int main(void) {
> unsigned int a = -5, b = 2;
> unsigned int result = (b-a) % 10;
> printf("Result of -5 %% 10 is: %u\n", result);
> return 0;
> }
>
> But it is a good catch for signed ints for sure :).
>
> Thanks!
>
Ah, I see now! Thanks for the clarification. In that case, I'll remove
the '+ vq->vring.num' in v3.
>>>> +
>>>> + /* 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;
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>
>>> Let's report an error if we finish the loop. I think:
>>> qemu_log_mask(LOG_GUEST_ERROR,
>>> "%s: %s cannot fill buffer id %u\n",
>>> __func__, vdev->name, elem->index);
>>>
>>> (or similar) should do.
>>>
>>> apart form that,
>>>
>>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>>>
>>
>> Gotcha. Will add this in v3.
>>
>> Thank you Eugenio!
>>
>>>> static void virtqueue_packed_fill_desc(VirtQueue *vq,
>>>> const VirtQueueElement *elem,
>>>> unsigned int idx,
>>>> @@ -923,7 +955,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
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-05-23 11:12 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-20 13:00 [PATCH v2 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
2024-05-22 15:44 ` Eugenio Perez Martin
2024-05-23 9:55 ` Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support Jonah Palmer
2024-05-22 15:45 ` Eugenio Perez Martin
2024-05-23 9:56 ` Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 3/6] virtio: virtqueue_ordered_fill " Jonah Palmer
2024-05-22 16:07 ` Eugenio Perez Martin
2024-05-23 10:29 ` Jonah Palmer
2024-05-23 10:47 ` Eugenio Perez Martin
2024-05-23 11:10 ` Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 4/6] virtio: virtqueue_ordered_flush " Jonah Palmer
2024-05-20 13:00 ` [PATCH v2 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits Jonah Palmer via
2024-05-20 13:00 ` [PATCH v2 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition Jonah Palmer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).