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

[Jonah Palmer <jonah.palmer@oracle.com>]

On 5/22/24 11:45 AM, 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 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 <leiyang@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   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..7456d61bc8 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, head, max, prev_avail_idx;
>>       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;
>>       }
>>
>> +    prev_avail_idx = 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)) {
> 
> I think vq->last_avail_idx - 1 could be more clear here.
> 
> Either way,
> 
> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> 

Sure thing! Will make this change in v3.

>> +        vq->used_elems[prev_avail_idx].index = elem->index;
>> +        vq->used_elems[prev_avail_idx].len = elem->len;
>> +        vq->used_elems[prev_avail_idx].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;
>> +    }
>> +
>>       vq->last_avail_idx += elem->ndescs;
>>       vq->inuse += elem->ndescs;
>>
>> --
>> 2.39.3
>>
>