| Message ID | 20240520130048.1483177-4-jonah.palmer@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio,vhost: Add VIRTIO_F_IN_ORDER support | expand |
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 >
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 >>>> >>> >> >
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);
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(-)