Re: [Qemu-devel] [PATCH 2/3] virtio: add virtqueue_rewind()

[Ladi Prosek]

On Thu, Sep 8, 2016 at 8:44 AM, Roman Kagan <address@hidden> wrote:
> On Wed, Sep 07, 2016 at 05:20:48PM +0200, Ladi Prosek wrote:
>> From: Stefan Hajnoczi <address@hidden>
>>
>> virtqueue_discard() requires a VirtQueueElement but virtio-balloon does
>> not migrate its in-use element.  Introduce a new function that is
>> similar to virtqueue_discard() but doesn't require a VirtQueueElement.
>>
>> This will allow virtio-balloon to access element again after migration
>> with the usual proviso that the guest may have modified the vring since
>> last time.
>>
>> Cc: Michael S. Tsirkin <address@hidden>
>> Cc: Roman Kagan <address@hidden>
>> Cc: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: Ladi Prosek <address@hidden>
>> ---
>>  hw/virtio/virtio.c         | 22 ++++++++++++++++++++++
>>  include/hw/virtio/virtio.h |  1 +
>>  2 files changed, 23 insertions(+)
>
> Reviewed-by: Roman Kagan <address@hidden>
>
> One question though:
>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 74c085c..3de6029 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -272,6 +272,28 @@ void virtqueue_discard(VirtQueue *vq, const 
>> VirtQueueElement *elem,
>>      virtqueue_unmap_sg(vq, elem, len);
>>  }
>>
>> +/* virtqueue_rewind:
>> + * @vq: The #VirtQueue
>> + * @num: Number of elements to push back
>> + *
>> + * Pretend that elements weren't popped from the virtqueue.  The next
>> + * virtqueue_pop() will refetch the oldest element.
>> + *
>> + * Use virtqueue_discard() instead if you have a VirtQueueElement.
>> + *
>> + * Returns: true on success, false if @num is greater than the number of in 
>> use
>> + * elements.
>> + */
>> +bool virtqueue_rewind(VirtQueue *vq, unsigned int num)
>> +{
>> +    if (num > vq->inuse) {
>> +        return false;
>> +    }
>> +    vq->last_avail_idx -= num;
>> +    vq->inuse -= num;
>> +    return true;
>> +}
>> +
>
> Presumably you envision rewinding by something other than ->inuse.  Do
> you have in mind a usecase for that, or is it just a matter of API
> symmetry or whatnot?

It may not always be correct to rewind by ->inuse. The elements that
are in use do not necessarily have to be at the end of the available
ring. Example:

elem1 = virtqueue_pop();
elem2 = virtqueue_pop();
elem3 = virtqueue_pop();

virtqueue_push(elem2);

Now inuse == 2 but rewinding by 2 would be incorrect because elem2 has
already been pushed to the used ring.

So it is a dangerous API, which I believe is why Stefan added the num
parameter even though it is currently not needed.

Thanks,
Ladi