[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER su
From: |
Eugenio Perez Martin |
Subject: |
Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support |
Date: |
Thu, 23 May 2024 12:47:58 +0200 |
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
> >>
> >
>
[PATCH v2 4/6] virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support, Jonah Palmer, 2024/05/20
[PATCH v2 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support, Jonah Palmer, 2024/05/20
[PATCH v2 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits, Jonah Palmer, 2024/05/20