Re: [RFC PATCH v4 15/20] vhost: Shadow virtqueue buffers forwarding

[Markus Armbruster]

Eugenio Perez Martin <eperezma@redhat.com> writes:

> On Tue, Oct 12, 2021 at 7:21 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Eugenio Pérez <eperezma@redhat.com> writes:
>>
>> > Initial version of shadow virtqueue that actually forward buffers. There
>> > are no iommu support at the moment, and that will be addressed in future
>> > patches of this series. Since all vhost-vdpa devices uses forced IOMMU,
>> > this means that SVQ is not usable at this point of the series on any
>> > device.
>> >
>> > For simplicity it only supports modern devices, that expects vring
>> > in little endian, with split ring and no event idx or indirect
>> > descriptors. Support for them will not be added in this series.
>> >
>> > It reuses the VirtQueue code for the device part. The driver part is
>> > based on Linux's virtio_ring driver, but with stripped functionality
>> > and optimizations so it's easier to review. Later commits add simpler
>> > ones.
>> >
>> > SVQ uses VIRTIO_CONFIG_S_DEVICE_STOPPED to pause the device and
>> > retrieve its status (next available idx the device was going to
>> > consume) race-free. It can later reset the device to replace vring
>> > addresses etc. When SVQ starts qemu can resume consuming the guest's
>> > driver ring from that state, without notice from the latter.
>> >
>> > This status bit VIRTIO_CONFIG_S_DEVICE_STOPPED is currently discussed
>> > in VirtIO, and is implemented in qemu VirtIO-net devices in previous
>> > commits.
>> >
>> > Removal of _S_DEVICE_STOPPED bit (in other words, resuming the device)
>> > can be done in the future if an use case arises. At this moment we can
>> > just rely on reseting the full device.
>> >
>> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> > ---
>> >  qapi/net.json                      |   2 +-
>> >  hw/virtio/vhost-shadow-virtqueue.c | 237 ++++++++++++++++++++++++++++-
>> >  hw/virtio/vhost-vdpa.c             | 109 ++++++++++++-
>> >  3 files changed, 337 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/qapi/net.json b/qapi/net.json
>> > index fe546b0e7c..1f4a55f2c5 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -86,7 +86,7 @@
>> >  #
>> >  # @name: the device name of the VirtIO device
>> >  #
>> > -# @enable: true to use the alternate shadow VQ notifications
>> > +# @enable: true to use the alternate shadow VQ buffers fowarding path
>>
>> Uh, why does the flag change meaning half-way through this series?
>>
>
> Before this patch, the SVQ mode just makes an extra hop for
> notifications. Guest ones are now received by qemu via ioeventfd, and
> qemu forwards them to the device using a different eventfd. The
> reverse is also true: the device ones will be received by qemu by
> device call fd, and then qemu will forward them to the guest using a
> different irqfd.
>
> This intermediate step is not very useful by itself, but helps for
> checking that that part of the communication works fine, with no need
> for shadow virtqueue to understand vring format. Doing that way also
> produces smaller patches.
>
> So it makes sense to me to tell what QMP command does exactly at every
> point of the series. However I can directly document it as "use the
> alternate shadow VQ buffers forwarding path" from the beginning.
>
> Does this make sense, or will it be better to write the final
> intention of the command?
>
> Thanks!

Working your explanation into commit messages and possibly comments
should do.