From: Eugenio Perez Martin <eperezma@redhat.com>
To: Jonah Palmer <jonah.palmer@oracle.com>
Cc: qemu-devel@nongnu.org, mst@redhat.com, raphael@enfabrica.net,
kwolf@redhat.com, hreitz@redhat.com, jasowang@redhat.com,
pbonzini@redhat.com, fam@euphon.net, stefanha@redhat.com,
qemu-block@nongnu.org, schalla@marvell.com, leiyang@redhat.com,
virtio-fs@lists.linux.dev, si-wei.liu@oracle.com,
boris.ostrovsky@oracle.com
Subject: Re: [PATCH 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
Date: Thu, 9 May 2024 16:08:29 +0200 [thread overview]
Message-ID: <CAJaqyWcTSQ4hyzfYRENE93GnHMhgvY4X_gz3ydnpYUMz1_J8aA@mail.gmail.com> (raw)
In-Reply-To: <20240506150428.1203387-4-jonah.palmer@oracle.com>
On Mon, May 6, 2024 at 5:05 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add VIRTIO_F_IN_ORDER feature support for virtqueue_fill operations.
>
> The goal of the virtqueue_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 is
> ready to be flushed, so long as the element is in-order.
>
> Tested-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
> hw/virtio/virtio.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e6eb1bb453..064046b5e2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -873,6 +873,28 @@ 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 = vq->used_idx;
> +
> + /* Search for element in vq->used_elems */
> + while (i != vq->last_avail_idx) {
> + /* 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].filled = true;
> + break;
> + }
> +
> + i += vq->used_elems[i].ndescs;
> +
> + if (i >= vq->vring.num) {
> + i -= vq->vring.num;
> + }
> + }
This has a subtle problem: ndescs and elems->id are controlled by the
guest, so it could make QEMU to loop forever looking for the right
descriptor. For each iteration, the code must control that the
variable "i" will be different for the next iteration, and that there
will be no more than vq->last_avail_idx - vq->used_idx iterations.
Apart of that, I think it makes more sense to split the logical
sections of the function this way:
/* declarations */
i = vq->used_idx
/* Search for element in vq->used_elems */
while (vq->used_elems[i].index != elem->index &&
vq->used_elems[i].index i != vq->last_avail_idx && ...) {
...
}
/* Set length and mark as filled */
vq->used_elems[i].len = len;
vq->used_elems[i].filled = true;
---
But I'm ok either way.
> +}
> +
> static void virtqueue_packed_fill_desc(VirtQueue *vq,
> const VirtQueueElement *elem,
> unsigned int idx,
> @@ -923,7 +945,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
>
next prev parent reply other threads:[~2024-05-09 14:10 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 15:04 [PATCH 0/6] virtio,vhost: Add VIRTIO_F_IN_ORDER support Jonah Palmer
2024-05-06 15:04 ` [PATCH 1/6] virtio: Add bool to VirtQueueElement Jonah Palmer
2024-05-09 12:32 ` Eugenio Perez Martin
2024-05-10 10:26 ` Jonah Palmer
2024-05-06 15:04 ` [PATCH 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support Jonah Palmer
2024-05-09 13:13 ` Eugenio Perez Martin
2024-05-10 10:52 ` Jonah Palmer
2024-05-06 15:04 ` [PATCH 3/6] virtio: virtqueue_ordered_fill " Jonah Palmer
2024-05-09 14:08 ` Eugenio Perez Martin [this message]
2024-05-10 12:01 ` Jonah Palmer
2024-05-06 15:04 ` [PATCH 4/6] virtio: virtqueue_ordered_flush " Jonah Palmer
2024-05-10 7:48 ` Eugenio Perez Martin
2024-05-10 13:07 ` Jonah Palmer
2024-05-06 15:04 ` [PATCH 5/6] vhost, vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits Jonah Palmer via
2024-05-06 15:04 ` [PATCH 6/6] virtio: Add VIRTIO_F_IN_ORDER property definition Jonah Palmer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJaqyWcTSQ4hyzfYRENE93GnHMhgvY4X_gz3ydnpYUMz1_J8aA@mail.gmail.com \
--to=eperezma@redhat.com \
--cc=boris.ostrovsky@oracle.com \
--cc=fam@euphon.net \
--cc=hreitz@redhat.com \
--cc=jasowang@redhat.com \
--cc=jonah.palmer@oracle.com \
--cc=kwolf@redhat.com \
--cc=leiyang@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=raphael@enfabrica.net \
--cc=schalla@marvell.com \
--cc=si-wei.liu@oracle.com \
--cc=stefanha@redhat.com \
--cc=virtio-fs@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).