Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support

[Eugenio Perez Martin <eperezma@redhat.com>]

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
>