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;
+
+ /* 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;
+ }
+ }
+}
+
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.3On 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
>
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
>> +
>> + /* 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
>>
>
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
> >>
> >
>
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: -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 <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
>>>>
>>>
>>
>
© 2016 - 2024 Red Hat, Inc.