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