On 5/23/24 6:47 AM, Eugenio Perez Martin wrote:
On Thu, May 23, 2024 at 12:30 PM Jonah Palmer <jonah.pal...@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.pal...@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.pal...@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 <epere...@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
