Re: [PATCH RFC v2 04/13] hw/virtio/vhost-shadow-virtqueue: used callback

On 2026/08/21 8:42, Connor Kite wrote:
> On Wed, Aug 19, 2026 at 10:15 PM Akihiko Odaki
> <[email protected]> wrote:
>>
>>
>> Skipping event_notifier_set() only suppresses this guest interrupt, and
>> disabling SVQ notifications only suppresses backend call notifications;
>> it does not stop backend ring processing. The error should be propagated
>> with virtio_error().
>>
>> I looked into the virtqueue_fill() and virtqueue_flush() calls in the
>> existing error path. The virtqueue_fill() call detaches the element and
>> publishes it to the guest. Detaching the element is necessary and should
>> be done in the new error path too. On the other hand, publishing the
>> failed element is wrong and should not be done.
>>
>> virtqueue_flush(vq, i) is still required to publish and account for the
>> preceding successfully filled elements.
>>
>> Regards,
>> Akihiko Odaki
> 
> Thank you for the explanation!  I was not aware of virtio_error.  This
> error path will now follow the following order:
> 
>      if (r < 0) {
>          virtio_error(svq->vdev, "Vhost shadow virtqueue error while
> handling used element.\n");
>          virtqueue_detach_element(vq, elem, 0);
>          g_free(elem);

This must be removed. elem is declared g_autofree, so returning after an 
explicit free would free it twice.

>          virtqueue_flush(vq, i);

virtio_error() must come last. It sets vdev->broken, after which 
virtqueue_flush() takes its disabled-device shortcut and merely 
subtracts i; it does not publish the preceding used entries.

>          return;
>      }
> 
> One thing still seems a little strange here about how vq->inuse is
> modified by these operations.  The count provided to virtqueue_flush
> in the svq code is the number of elements that have been added.
> However, depending on which type of flush occurs, the value of
> vq->inuse is decreased by either this count (virtqueue_split_flush) or
> the number of descriptors in the flushed elements
> (virtqueue_packed_flush, virtqueue_ordered_flush).  Also, the
> virtqueue_detach_element always decrements vq->inuse by the number of
> descriptors.
> 
> I'm possibly just misunderstanding something, but this seems like
> vq->inuse could potentially end up in an incorrect state.  Maybe that
> doesn't matter in this case once virtio_error gets called...

The corresponding virtqueue_pop() function that increments vq->inuse 
also changes the count conditionally with the negotiated features, so I 
expect they are balanced.

Regards,
Akihiko Odaki