Re: [PATCH 2/6] virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support

On 5/9/24 9:13 AM, Eugenio Perez Martin wrote:
> On Mon, May 6, 2024 at 5:06 PM Jonah Palmer <[email protected]> wrote:
>>
>> Add VIRTIO_F_IN_ORDER feature support in virtqueue_split_pop and
>> virtqueue_packed_pop.
>>
>> VirtQueueElements popped from the available/descritpor ring are added to
>> the VirtQueue's used_elems array in-order and in the same fashion as
>> they would be added the used and descriptor rings, respectively.
>>
>> This will allow us to keep track of the current order, what elements
>> have been written, as well as an element's essential data after being
>> processed.
>>
>> Tested-by: Lei Yang <[email protected]>
>> Signed-off-by: Jonah Palmer <[email protected]>
>> ---
>>   hw/virtio/virtio.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 893a072c9d..e6eb1bb453 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1506,7 +1506,7 @@ static void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_nu
>>
>>   static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>>   {
>> -    unsigned int i, head, max;
>> +    unsigned int i, j, head, max;
>>       VRingMemoryRegionCaches *caches;
>>       MemoryRegionCache indirect_desc_cache;
>>       MemoryRegionCache *desc_cache;
>> @@ -1539,6 +1539,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>>           goto done;
>>       }
>>
>> +    j = vq->last_avail_idx;
>> +
>>       if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) {
>>           goto done;
>>       }
>> @@ -1630,6 +1632,12 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
>>           elem->in_sg[i] = iov[out_num + i];
>>       }
>>
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
>> +        vq->used_elems[j].index = elem->index;
>> +        vq->used_elems[j].len = elem->len;
>> +        vq->used_elems[j].ndescs = elem->ndescs;
>> +    }
>> +
>>       vq->inuse++;
>>
>>       trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
>> @@ -1758,6 +1766,13 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t sz)
>>
>>       elem->index = id;
>>       elem->ndescs = (desc_cache == &indirect_desc_cache) ? 1 : elem_entries;
>> +
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
>> +        vq->used_elems[vq->last_avail_idx].index = elem->index;
>> +        vq->used_elems[vq->last_avail_idx].len = elem->len;
>> +        vq->used_elems[vq->last_avail_idx].ndescs = elem->ndescs;
>> +    }
>> +
> 
> I suggest using a consistent style between packed and split: Either
> always use vq->last_avail_idx or j. If you use j, please rename to
> something more related to the usage, as j is usually for iterations.
> 
> In my opinion I think vq->last_avail_idx is better.
> 
> 

Totally agree. The reason I used a separate variable in 
virtqueue_split_pop was to capture the value of vq->last_avail_idx 
before it got incremented in the next line.

Not sure if it actually matters whether or not I use the value of 
last_avail_idx before or after it's incremented. I don't think it does 
but, in any case, I opted to use the value before it was incremented so 
as to be consistent with virtqueue_packed_pop, where last_avail_idx is 
used before it's incremented.

I'll change j to something more meaningful though. Maybe 
'init_last_avail_idx'? Hmm... will need to think on it.

>>       vq->last_avail_idx += elem->ndescs;
>>       vq->inuse += elem->ndescs;
>>
>> --
>> 2.39.3
>>
>