Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
On Mon, May 20, 2024 at 3:01 PM Jonah Palmer 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
> ---
> 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
> 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
>
Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
On 5/22/24 12:07 PM, Eugenio Perez Martin wrote:
On Mon, May 20, 2024 at 3:01 PM Jonah Palmer 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
---
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
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
+
+/* 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
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
Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
On Thu, May 23, 2024 at 12:30 PM Jonah Palmer wrote:
>
>
>
> On 5/22/24 12:07 PM, Eugenio Perez Martin wrote:
> > On Mon, May 20, 2024 at 3:01 PM Jonah Palmer
> > 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
> >> ---
> >> 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
>
> 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
> >
>
> 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
> >>
> >
>
Re: [PATCH v2 3/6] virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support
On 5/23/24 6:47 AM, Eugenio Perez Martin wrote:
On Thu, May 23, 2024 at 12:30 PM Jonah Palmer wrote:
On 5/22/24 12:07 PM, Eugenio Perez Martin wrote:
On Mon, May 20, 2024 at 3:01 PM Jonah Palmer 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
---
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
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
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
