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 <[email protected]> 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 <[email protected]>
>> ---
>>   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 <[email protected]>
> 

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
>>
>