|
From: | Jonah Palmer |
Subject: | Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support |
Date: | Thu, 23 May 2024 07:10:45 -0400 |
User-agent: | Mozilla Thunderbird |
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: -5I 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
[Prev in Thread] | Current Thread | [Next in Thread] |